Arch arm cortex m vector table rework #24012

ioannisg · 2020-04-02T10:52:14Z

Ready for review.
This PR does the following changes

removes z_arm_reserved function pointer from the Cortex-M vector table entries that correspond to exceptions reserved for future use by the ARM specification. This should be a "no-op" as these addresses are not read/written/fetched by the code. The entries are set to 0x0.
renames z_arm_reserved to z_arm_exc_spurious and use the function pointer for vector entries that correspond to existing but not used exception
adds a test to verify the behavior of z_arm_exc_spurious

galak · 2020-04-02T11:51:45Z

I wonder if we are going to make a change like this, if we should also move it out of asm code like arc has. Look at arch/arc/core/vector_table.c. I don't see any particular need for this to be asm and changing it to .c might make it be one less thing to deal with for other compilers in the future.

galak

Is z_arm_reserved used anymore after this change?

stephanosio

In general, I think setting the reserved vectors to 0 is okay; after all, we do not care about the reserved stuff and that is how reserved things are often handled (and expected to be, in case of #23972).

arch/arm/core/aarch32/cortex_m/vector_table.S

ioannisg · 2020-04-02T11:56:25Z

Is z_arm_reserved used anymore after this change?

Yes, it is used to trigger a CPU fault, see fault.S
This is needed in some cases. For example, if someone pends the SysTick IRQ without having it wired.

ioannisg · 2020-04-02T12:01:34Z

I wonder if we are going to make a change like this, if we should also move it out of asm code like arc has. Look at arch/arc/core/vector_table.c. I don't see any particular need for this to be asm and changing it to .c might make it be one less thing to deal with for other compilers in the future.

No objections to that, but I would prefer to do this once for all cortex- variants together, maybe a task for @stephanosio (?) :)

stephanosio · 2020-04-02T13:36:16Z

I wonder if we are going to make a change like this, if we should also move it out of asm code like arc has. Look at arch/arc/core/vector_table.c. I don't see any particular need for this to be asm and changing it to .c might make it be one less thing to deal with for other compilers in the future.

No objections to that, but I would prefer to do this once for all cortex- variants together, maybe a task for @stephanosio (?) :)

I believe that would be feasible for Cortex-M, though not for the rest (i.e. Cortex-A and Cortex-R).

There is one major difference between the vector tables for Cortex-M and the rest -- that the former consists of the pointers to the handlers, whereas the latter (usually) consists of the jump instructions to the handlers.

Moreover, in AArch64, the space allocated for each vector has increased so that you can actually put small handler implementations in the vector table itself.

ioannisg · 2020-04-02T15:03:44Z

I believe that would be feasible for Cortex-M, though not for the rest (i.e. Cortex-A and Cortex-R).

In this case I am less eager to do this for Cortex-M now - I am not 100% happy with having the cortex-m in .c while cortex-r, a in .S format...

MaureenHelm · 2020-04-02T22:00:22Z

I didn't go down this path before because it's a more intrusive change, but if you're ok with it then so am I. :)

I agree with @stephanosio that z_arm_reserved no longer makes sense. I think it should be renamed in this PR.

stephanosio

Looks ok to me.

z_arm_reserved can be completely removed for Cortex-R too since it is currently not used by anything and the future Cortex-A support will use z_arm_exc_spurious as well for the hypervisor vector.

ioannisg · 2020-04-03T10:39:31Z

Looks ok to me.

z_arm_reserved can be completely removed for Cortex-R too since it is currently not used by anything and the future Cortex-A support will use z_arm_exc_spurious as well for the hypervisor vector.

Would you mind doing this in your clean up work for cortex-r?

stephanosio · 2020-04-03T10:41:07Z

Looks ok to me.
z_arm_reserved can be completely removed for Cortex-R too since it is currently not used by anything and the future Cortex-A support will use z_arm_exc_spurious as well for the hypervisor vector.

Would you mind doing this in your clean up work for cortex-r?

Sure, that would work too.

zephyrbot · 2020-04-03T11:59:51Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

ioannisg · 2020-04-03T12:08:50Z

I didn't go down this path before because it's a more intrusive change, but if you're ok with it then so am I. :)

I agree with @stephanosio that z_arm_reserved no longer makes sense. I think it should be renamed in this PR.

@MaureenHelm so, i think this is a good cleanup and will solve the issue with the LPC image boot. I also did the rename of z_arm_reserved in this PR.

stephanosio

Looks ok to me overall.

Just a minor comment about the #ifs; though, this is really not that important since I am splitting Cortex-M and Cortex-A/-R asm exception handling code into separate files in the upcoming PR.

arch/arm/core/aarch32/fault_s.S

MaureenHelm

Minor typo in the test comment, but otherwise this looks great. Thanks @ioannisg

tests/arch/arm/arm_interrupt/src/arm_interrupt.c

ioannisg · 2020-04-03T15:28:33Z

Minor typo in the test comment, but otherwise this looks great. Thanks @ioannisg

I'll fix the typo, thanks for catching. Have you tested that it works for the LPC SoC, @MaureenHelm ?

MaureenHelm · 2020-04-03T16:17:26Z

Have you tested that it works for the LPC SoC, @MaureenHelm ?

yes

ioannisg · 2020-04-06T13:30:21Z

Is z_arm_reserved used anymore after this change?

Yes, it is used to trigger a CPU fault, see fault.S
This is needed in some cases. For example, if someone pends the SysTick IRQ without having it wired.

@galak this is now even unit-tested :)

Write 0x0 instead of z_arm_reserved to vector exception entries that are always reserved for future use by the ARM architecture. These vector table entries cannot be fetched to be executed by the Cortex-M exception entry, so having z_arm_reserved gives a false impression, since it is a function that may be invoked in the code. This modification is safe since these vector entries are also not supposed to be read / written by the code. Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>

If the Cortex-M core does not implement the SysTick peripheral, we should not be adding z_arm_reserved in the corresponding vector table entry. If we do have SysTick implemented but we are not using it as the system timer, we shall install the reserved interrupt at the vector table entry. Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>

If the Cortex-M core does not implement the Security Extension, we should not be adding z_arm_reserved in the corresponding vector table entry. That is because the entry is reserved by the ARM architecture. Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>

In the Cortex-M exception table we rename z_arm_reserved() function to z_arm_exc_spurious(), as it is invoked when existing (that is, non-reserved) but un-installed exceptions are triggered, accidentaly, by software, or hardware. This currently applies to SysTick and SecureFault exceptions. Since fault.S is shared between Cortex-M and other AARCH32 architectures, we keep z_arm_reserved as a defined symbol there. This commit does some additional, minor, "no-op" cleanup in #ifdef's for Cortex-M and Cortex-R. Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>

We add a simple test case to verify the behavior of z_arm_exc_spurious handler. We use the SysTick interrupt for that so the test is enabled for platforms that have but do not use the SysTick. Signed-off-by: Ioannis Glaropoulos <Ioannis.Glaropoulos@nordicsemi.no>

MaureenHelm · 2020-04-06T17:57:47Z

@galak are you ok with this?

The selection of the Cortex M systick driver to be used as a system clock driver is controlled by CONFIG_CORTEX_M_SYSTICK. To replace it by another driver CONFIG_CORTEX_M_SYSTICK must be set to 'n'. Unfortunately this also controls the interrupt vector for the systick interrupt. It is now routed to z_arm_exc_spurious. Remove the dependecy on CONFIG_CORTEX_M_SYSTICK and route to z_clock_isr as it was before zephyrproject-rtos#24012. Fixes zephyrproject-rtos#24347 Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

The selection of the Cortex M systick driver to be used as a system clock driver is controlled by CONFIG_CORTEX_M_SYSTICK. To replace it by another driver CONFIG_CORTEX_M_SYSTICK must be set to 'n'. Unfortunately this also controls the interrupt vector for the systick interrupt. It is now routed to z_arm_exc_spurious. Remove the dependecy on CONFIG_CORTEX_M_SYSTICK and route to z_clock_isr as it was before #24012. Fixes #24347 Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

The selection of the Cortex M systick driver to be used as a system clock driver is controlled by CONFIG_CORTEX_M_SYSTICK. To replace it by another driver CONFIG_CORTEX_M_SYSTICK must be set to 'n'. Unfortunately this also controls the interrupt vector for the systick interrupt. It is now routed to z_arm_exc_spurious. Remove the dependecy on CONFIG_CORTEX_M_SYSTICK and route to z_clock_isr as it was before zephyrproject-rtos#24012. Fixes zephyrproject-rtos#24347 Signed-off-by: Bobby Noelte <b0661n0e17e@gmail.com>

ioannisg requested a review from stephanosio April 2, 2020 10:52

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch 2 times, most recently from 8e1d73f to 6bd0b08 Compare April 2, 2020 10:57

ioannisg mentioned this pull request Apr 2, 2020

arch: arm: Set lpc55s69 image type in cortex-m vector table #23972

Closed

zephyrbot added the area: ARM ARM (32-bit) Architecture label Apr 2, 2020

galak reviewed Apr 2, 2020

View reviewed changes

stephanosio reviewed Apr 2, 2020

View reviewed changes

arch/arm/core/aarch32/cortex_m/vector_table.S Outdated Show resolved Hide resolved

arch/arm/core/aarch32/cortex_m/vector_table.S Outdated Show resolved Hide resolved

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch 2 times, most recently from 541a889 to 27988ce Compare April 2, 2020 14:54

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from 27988ce to fcf600e Compare April 3, 2020 08:20

stephanosio reviewed Apr 3, 2020

View reviewed changes

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from fcf600e to 49f28ca Compare April 3, 2020 11:57

zephyrbot added the area: Tests Issues related to a particular existing or missing test label Apr 3, 2020

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from 49f28ca to 697342f Compare April 3, 2020 12:03

ioannisg marked this pull request as ready for review April 3, 2020 12:06

ioannisg requested a review from MaureenHelm as a code owner April 3, 2020 12:06

ioannisg requested a review from stephanosio April 3, 2020 12:06

ioannisg added the Enhancement Changes/Updates/Additions to existing features label Apr 3, 2020

ioannisg self-assigned this Apr 3, 2020

stephanosio reviewed Apr 3, 2020

View reviewed changes

arch/arm/core/aarch32/fault_s.S Outdated Show resolved Hide resolved

arch/arm/core/aarch32/fault_s.S Outdated Show resolved Hide resolved

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from 697342f to e630b4a Compare April 3, 2020 13:32

ioannisg requested a review from stephanosio April 3, 2020 13:33

stephanosio approved these changes Apr 3, 2020

View reviewed changes

MaureenHelm approved these changes Apr 3, 2020

View reviewed changes

tests/arch/arm/arm_interrupt/src/arm_interrupt.c Outdated Show resolved Hide resolved

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from e630b4a to d714131 Compare April 3, 2020 17:02

MaureenHelm mentioned this pull request Apr 3, 2020

drivers: i2c: Add mcux flexcomm driver #23119

Merged

ioannisg mentioned this pull request Apr 4, 2020

ARM: Core Stack Improvements/Bug fixes for 2.3 release #23366

Closed

17 tasks

ioannisg added 5 commits April 6, 2020 15:31

ioannisg force-pushed the arch_arm_cortex_m_vector_table_rework branch from d714131 to daa6348 Compare April 6, 2020 13:31

galak approved these changes Apr 7, 2020

View reviewed changes

galak merged commit 56a0942 into zephyrproject-rtos:master Apr 7, 2020

b0661 mentioned this pull request Apr 14, 2020

Fix system clock driver selection for cortex m #24348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arch arm cortex m vector table rework #24012

Arch arm cortex m vector table rework #24012

ioannisg commented Apr 2, 2020 •

edited

Loading

galak commented Apr 2, 2020

galak left a comment

stephanosio left a comment

ioannisg commented Apr 2, 2020 •

edited

Loading

ioannisg commented Apr 2, 2020

stephanosio commented Apr 2, 2020 •

edited

Loading

ioannisg commented Apr 2, 2020

MaureenHelm commented Apr 2, 2020

stephanosio left a comment

ioannisg commented Apr 3, 2020

stephanosio commented Apr 3, 2020

zephyrbot commented Apr 3, 2020 •

edited

Loading

ioannisg commented Apr 3, 2020

stephanosio left a comment

MaureenHelm left a comment

ioannisg commented Apr 3, 2020

MaureenHelm commented Apr 3, 2020

ioannisg commented Apr 6, 2020

MaureenHelm commented Apr 6, 2020

Arch arm cortex m vector table rework #24012

Arch arm cortex m vector table rework #24012

Conversation

ioannisg commented Apr 2, 2020 • edited Loading

galak commented Apr 2, 2020

galak left a comment

Choose a reason for hiding this comment

stephanosio left a comment

Choose a reason for hiding this comment

ioannisg commented Apr 2, 2020 • edited Loading

ioannisg commented Apr 2, 2020

stephanosio commented Apr 2, 2020 • edited Loading

ioannisg commented Apr 2, 2020

MaureenHelm commented Apr 2, 2020

stephanosio left a comment

Choose a reason for hiding this comment

ioannisg commented Apr 3, 2020

stephanosio commented Apr 3, 2020

zephyrbot commented Apr 3, 2020 • edited Loading

ioannisg commented Apr 3, 2020

stephanosio left a comment

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

ioannisg commented Apr 3, 2020

MaureenHelm commented Apr 3, 2020

ioannisg commented Apr 6, 2020

MaureenHelm commented Apr 6, 2020

ioannisg commented Apr 2, 2020 •

edited

Loading

ioannisg commented Apr 2, 2020 •

edited

Loading

stephanosio commented Apr 2, 2020 •

edited

Loading

zephyrbot commented Apr 3, 2020 •

edited

Loading