arch: Add SRAM based context for ISR

[Martinhoff-maker]

The goal of this PR to add the symbol SRAM_ISR_EXEC_CONTEXT.

It relocates all the vector table and the necessary files (isr_wrapper,...) in SRAM in order to have a fully SRAM context when an interruption is triggered (except if it's to get out of idle state with CONFIG_PM=y). It remains to the user to place ISR callbacks and the ISR dependencies in RAM. This option allow to not get latency (by accessing flash) when an interrupt is triggered in a real time application.

I know that in the case we only want the sw_isr_table in RAM (with SRAM_SW_ISR_TABLE) , write permission is not needed but it is only to simplify the modification for the moment.

I also know that the new symbol SRAM_ISR_EXEC_CONTEXT depends on the fact that the necessary files is relocated for each arch. I only do the work for cortex-m then other arch that activate the symbol might not be fully in an SRAM context. (I don't know how to inform user about this other than introducing a new ARCH_HAS_SRAM_CONTEXT_AVAILABLE (and make SRAM_ISR_EXEC_CONTEXT depends on it) but it doesn't seems right...

[danieldegrasse]

Not inherently opposed to the idea here, but this seems like a slippery slope. If you are trying to avoid all flash access during an ISR, this change won't be sufficient (data like device pointers may still be in flash, even if you move relevant ISR handers). As it stands, I'm kind of unclear what the advantage of this change is over turning on CONFIG_DYNAMIC_INTERRUPTS and directly relocating architecture files from your application CMakeLists.

Is there something I'm missing here, or is this Kconfig currently mostly for convenience (IE something you could accomplish within your application with a bit more work)?

[Martinhoff-maker]

Thanks for the review @danieldegrasse. I'm currently aware about the device pointer. I'm trying to be as generic as possible and in the case that you relocate an ISR that is not related to a driver, you don't need that part. The whole point is to have a starting point for people who trying to have a real time constraint on an ISR. That's why I say, even in the Kconfig helper, that the relocation of the ISR handler and all of his dependency (and this includes device struct for example) must be done by the user.

CONFIG_DYNAMIC_INTERRUPT include a lot of files that we don't need for this feature, that's why I prefer to split it into 2 parts.

However, It needs to be well documented I think in order to not be a slippery option (explain what user have to take care about in case of a driver, and also how to identify the dependency of the ISR handler (with Puncover for example)).

[danieldegrasse]

Thanks for the review @danieldegrasse. I'm currently aware about the device pointer. I'm trying to be as generic as possible and in the case that you relocate an ISR that is not related to a driver, you don't need that part. The whole point is to have a starting point for people who trying to have a real time constraint on an ISR. That's why I say, even in the Kconfig helper, that the relocation of the ISR handler and all of his dependency (and this includes device struct for example) must be done by the user.

CONFIG_DYNAMIC_INTERRUPT include a lot of files that we don't need for this feature, that's why I prefer to split it into 2 parts.

However, It needs to be well documented I think in order to not be a slippery option (explain what user have to take care about in case of a driver, and also how to identify the dependency of the ISR handler (with Puncover for example)).

That makes sense regarding CONFIG_DYNAMIC_INTERRUPT. Could we do something like the following?

• make CONFIG_SRAM_SW_ISR_TABLE a user-selectable Kconfig, that CONFIG_DYNAMIC_INTERRUPT enables
• Rely on the user to enable CONFIG_SRAM_VECTOR_TABLE, CONFIG_SRAM_SW_ISR_TABLE, and use zephyr_code_relocate to move files to SRAM

I think my core concern is how many caveats we have to place on the Kconfig SRAM_ISR_EXEC_CONTEXT- I worry that it will lead users to turn this on without fully understanding what is and isn't relocated, whereas if they can set CONFIG_SRAM_SW_ISR_TABLE they have a good idea of what Zephyr has located in RAM, and what they need to relocate themselves.

[Martinhoff-maker]

I totally agree with what you are saying. I just fear that somebody who wants to have an ISR RAM context will have to go through all the works that I have already done (for cortex-M). Maybe the only expression of this work might be a good documentation but where ? maybe in the code relocation ? or in the interrupt documentation ?

[danieldegrasse]

I totally agree with what you are saying. I just fear that somebody who wants to have an ISR RAM context will have to go through all the works that I have already done (for cortex-M). Maybe the only expression of this work might be a good documentation but where ? maybe in the code relocation ? or in the interrupt documentation ?

What about adding a sample demonstrating how to perform the relocation? That way users have somewhere to start and some documentation on how to continue from there

[wearyzen]

I totally agree with what you are saying. I just fear that somebody who wants to have an ISR RAM context will have to go through all the works that I have already done (for cortex-M). Maybe the only expression of this work might be a good documentation but where ? maybe in the code relocation ? or in the interrupt documentation ?

What about adding a sample demonstrating how to perform the relocation? That way users have somewhere to start and some documentation on how to continue from there

+1 for having sample/test which can be verified in ci. Also, could this be used for boards like mps3/corstone310/fvp which have low itcm? I spent a lot of time last time trying to find something working for this board before I got this, but I am wondering if this new config would have made things easier. or maybe I misunderstood how this new config is used which again suggests that a sample would help.

[Martinhoff-maker]

Agree with all of you, I will replace my last commit with an example.

[Martinhoff-maker]

V2:

• Rebased with the latest upstream
• Reverted my last commit regarding the new symbol SRAM_ISR_EXEC_CONTEXT
• Added a test that shows how to enable an SRAM based context for ISR

[wearyzen]

it would be good to resolve the small sonarqube issue but otherwise LGTM. Thanks for adding the example.

[Martinhoff-maker]

V3:

• Correction of sonarqube issue.

Btw, I feel that this PR will not be merged for zephyr 4.2 so I think I have to create migration-guide-4.3 ?

[wearyzen]

V3:

• Correction of sonarqube issue.

Btw, I feel that this PR will not be merged for zephyr 4.2 so I think I have to create migration-guide-4.3 ?

oh right, both docs should be 4.3 now instead

[Martinhoff-maker]

V4:

• Rebase with the latest upstream
• Move from 4.2 to 4.3 doc (migration guide and release)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud