-
Notifications
You must be signed in to change notification settings - Fork 7.3k
arch: arm: cortex_m: Allow relocation of vector table in RAM #87468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
arch: arm: cortex_m: Allow relocation of vector table in RAM #87468
Conversation
92b1ec4
to
b999504
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notes and nitpicks. Nothing fatal, feature is definitely useful. Though ideally I guess we'd want to combine this with a "dynamic direct interrupts" feature with an app-visible API. As it stands it requires an app to poke values directly into the table?
arch/arm/core/cortex_m/prep_c.c
Outdated
|
||
#define VECTOR_ADDRESS ((uintptr_t)_sram_vector_start) | ||
|
||
static inline void relocate_vector_table(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason you can't just leave this in .data and rely on the pre-existing copy code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some specific alignment constraint over the vector table. We would also like to have the _vector_table section at the start of the SRAM.
(void) memcpy(_sram_vector_start, _vector_start, vector_size); | ||
SCB->VTOR = VECTOR_ADDRESS & VTOR_MASK; | ||
barrier_dsync_fence_full(); | ||
barrier_isync_fence_full(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The barrier instructions shouldn't be needed. Interrupts don't get enabled until the switch to main, which involves a full context switch and implicit barriers from the PendSV. And we absolutely shouldn't be tossing exceptions out of early init code. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with what you are saying. I have placed the barrier because of the arm documentation concerning the VTOR register (ARM doc). But I imagine that this is necessary when we would like to modify VTOR register at runtime ? Nevertheless, I've tried without the barrier and it works well.
#if defined(CONFIG_ARMV6_M_ARMV8_M_BASELINE) | ||
. = ALIGN( 1 << LOG2CEIL(4 * 64) ); | ||
#elif defined(CONFIG_ARMV7_M_ARMV8_M_MAINLINE) | ||
. = ALIGN( 1 << LOG2CEIL(4 * 32) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's generally preferred to put the alignment onto the chunk in the object file instead of manually here. Is that not already done?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, the vector table alignment is already done in the linker script (vector_table.ld) and not in the object file (from vector_table.S and isr_table.c). That's why I still do the alignment here. But I wonder why it's not done before, will try to figure it out.
.attr = K_MEM_PARTITION_P_R_U_RX, | ||
#else | ||
.attr = K_MEM_PARTITION_P_RX_U_RX, | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels wrong. You're burning a whole MPU register to support this feature when kernel .data is already walled off from userspace threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking also to reuse an already configured MPU section like code relocation section (and it can also make sense to link CONFIG_CODE_DATA_RELOCATION_SRAM and CONFIG_SRAM_VECTOR_TABLE)
Thanks for the early feedback @andyross. Yes combining with "dynamic direct interrupts" was in my mind since I'm going later (in another PR) to make use of "dynamic interrupt" to place sw_isr_table also in RAM. I'm currently trying to resolve the compilations problem with mps3 and will update the PR with your feedback. |
b999504
to
6a470ae
Compare
@@ -39,22 +39,40 @@ Z_GENERIC_SECTION(.vt_pointer_section) __attribute__((used)) void *_vector_table | |||
|
|||
#ifdef CONFIG_CPU_CORTEX_M_HAS_VTOR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does CONFIG_SRAM_VECTOR_TABLE
depend on CONFIG_CPU_CORTEX_M_HAS_VTOR
? If yes, maybe the declaration of CONFIG_SRAM_VECTOR_TABLE should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No CONFIG_SRAM_VECTOR_TABLE can be used when CONFIG_CPU_CORTEX_M_HAS_VTOR is not set but it is a special case with STM32f0 board
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a commit concerning this discussion.
arch/arm/core/mpu/arm_core_mpu.c
Outdated
#if defined(CONFIG_ARM_MPU_PXN) && defined(CONFIG_USERSPACE) | ||
.attr = K_MEM_PARTITION_P_R_U_RX, | ||
#else | ||
.attr = K_MEM_PARTITION_P_RX_U_RX, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe _sram_vector_start
only contains data. So, maybe K_MEM_PARTITION_P_RO_U_RO
is sufficient? (unless it is strategy to coalesce the MPU segments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that's right, will change it to RO and will see if CI fails.
Hi @Martinhoff-maker, |
6a470ae
to
3e8fa67
Compare
arch/Kconfig
Outdated
depends on XIP | ||
help | ||
The option specifies that the vector table should be placed at the | ||
start of SRAM instead of the start of flash. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The help should clarify further that this is a modification of a behavior resulting from eXecute in Place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, added.
ec5518c
to
58ab375
Compare
arch/Kconfig
Outdated
The option specifies that the vector table will be placed at the | ||
start of SRAM instead of the start of the flash when XIP is enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The option specifies that the vector table will be placed at the | |
start of SRAM instead of the start of the flash when XIP is enabled. | |
When XiP is enabled, this option will result in the vector table being | |
relocated from Flash to SRAM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree changed
#define VECTOR_ADDRESS ((uintptr_t)_vector_start) | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif | |
#endif /* CONFIG_SRAM_VECTOR_TABLE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not fan of that kind of commentary since the #ifdef block is really small... IMO it just add noise in the code readability...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the Linux Coding Style says "at the end of non-trivial #if or #ifdef block"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's inside another ifdef and comes after an else, that's why I thought it would be clearer to have the comment.
size_t vector_size = (size_t)_vector_end - (size_t)_vector_start; | ||
|
||
z_early_memcpy(_sram_vector_start, _vector_start, vector_size); | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#endif | |
#endif /* CONFIG_SRAM_VECTOR_TABLE */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as the other one
/* Vector table is not necessarily at the start of the ram region: in the case that you also use | ||
* code relocation, it will place the vector table after the relocated code. Relocated code | ||
* will always be at the start of the RAM SECTION because of the hardcoded generated value in file | ||
* with "gen_relocate_app.py". It can create quite big gap and lose about the size of alignment | ||
* of RAM Space |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* Vector table is not necessarily at the start of the ram region: in the case that you also use | |
* code relocation, it will place the vector table after the relocated code. Relocated code | |
* will always be at the start of the RAM SECTION because of the hardcoded generated value in file | |
* with "gen_relocate_app.py". It can create quite big gap and lose about the size of alignment | |
* of RAM Space | |
/* Vector table is not necessarily at the start of the RAM region: in the case where Zephyr code | |
* relocation is used, the vector table is placed after the relocated code. | |
* Relocated code is always placed at the start of the RAM SECTION because of the hard-coded value | |
* generated by "gen_relocate_app.py" in the file. It can create quite a big gap and lose about the | |
* size of alignment in RAM space. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
That's correct, it's not being used by the max32 port. I think it was incorrectly copied from another port when the max32 port was first done. |
58ab375
to
eb9dada
Compare
10b19d9
to
1d301ec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
1d301ec
to
986e9a8
Compare
The symbol SRAM_VECTOR_TABLE is not used in the adi/max32 soc. Removed it in order to clean up the Kconfig file and avoid confusion with the upcoming new definition of SRAM_VECTOR_TABLE symbol. (PR zephyrproject-rtos#87468) Signed-off-by: Martin Hoff <[email protected]>
Move SRAM_VECTOR_TABLE symbol from general Kconfig to Arch Kconfig because it depends on the architecture possibility to relocate the vector table. Signed-off-by: Martin Hoff <[email protected]>
Allow to place the vector table section in SRAM with CONFIG_SRAM_VECTOR_TABLE option for all cortex-m architecture that have VTOR register. Signed-off-by: Martin Hoff <[email protected]>
986e9a8
to
bc2816b
Compare
Add a basic test for vector table relocation to SRAM. This test verifies that the vector table can be relocated to SRAM and that the VTOR register correctly points to the SRAM address range. Signed-off-by: Martin Hoff <[email protected]>
bc2816b
to
e9865bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good :)
The symbol SRAM_VECTOR_TABLE is not used in the adi/max32 soc. Removed it in order to clean up the Kconfig file and avoid confusion with the upcoming new definition of SRAM_VECTOR_TABLE symbol. (PR #87468) Signed-off-by: Martin Hoff <[email protected]>
The goal of this PR is to add the support of vector table relocation for cortex_m architecture in case the symbol CONFIG_SRAM_VECTOR_TABLE is set to yes and VTOR register is present.
Actually, as said in the current Kconfig.zephyr, the symbol CONFIG_SRAM_VECTOR_TABLE should place the vector table at the start of the SRAM (which indirectly place the irq_vector_table in the SRAM too).
CONFIG_SRAM_VECTOR_TABLE is currently only used by some boards/soc:
It's hence not generic at all and would like to make this more generic for arm/cortex_m architecture which have VTOR register.
Considerations: