Skip to content
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

cpu/cortexm: add stack limit support for Cortex-M33 #20633

Merged
merged 3 commits into from
May 7, 2024

Conversation

dylad
Copy link
Member

@dylad dylad commented Apr 27, 2024

Contribution description

ARMv8-M architecture introduces PSPLIM and MSPLIM as CPU registers to be used alongside with PSP and MSP.
The idea of these registers is to define the lower limit of a stack. If the stack pointer reaches this limit, a fault is generated before the adjacent memory of the stack can be corrupted.

This module is not enabled by default as it introduces a bit of overhead per context switch (We need to update PSPLIM every time we also change PSP).

I didn't add Cortex-M23 here on purpose. It seems to be a little be different because of

  Devices without ARMv8-M Main Extensions (i.e. Cortex-M23) lack the non-secure
  Stack Pointer Limit register hence zero is returned always in non-secure
  mode.

Testing procedure

This PR also introduces a basic test application. I don't know if it's relevant enough to be merged.
I took inspiration from tests/cpu/mpu_stack_guard except that I remove all the printf as they eat too much stack.
Here, the stack overflows at some point, but a fault is generated before the canary word is modified.
If you remove the feature from the makefile, the stack will also overflows but the canary word will be modified, then all onboard LEDs will be switch on.
All of this can also be checked with GDB.

Regarding the overhead introduces in the context switching
Here are the result from tests/bench/thread_yield_pingpong
On current master:

2024-04-27 19:09:09,063 # main(): This is RIOT! (Version: 2024.07-devel-101-g89a74-pr/cpu/cortexm33/add_splim_support)
2024-04-27 19:09:09,065 # main starting
2024-04-27 19:09:10,068 # { "result" : 386706, "ticks" : 331 }
2024-04-27 19:09:10,074 # { "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 268 }]}

With this PR and FEATURES_REQUIRED+="cortexm_stack_limit":

2024-04-27 19:11:15,783 # main(): This is RIOT! (Version: 2024.07-devel-101-g89a74-pr/cpu/cortexm33/add_splim_support)
2024-04-27 19:11:15,784 # main starting
2024-04-27 19:11:16,787 # { "result" : 371014, "ticks" : 345 }
2024-04-27 19:11:16,793 # { "threads": [{ "name": "main", "stack_size": 1536, "stack_used": 268 }]}

Issues/PRs references

None.

@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: tests Area: tests and testing framework Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: build system Area: Build system Area: cpu Area: CPU/MCU ports labels Apr 27, 2024
@dylad
Copy link
Member Author

dylad commented Apr 27, 2024

I'll take care of static tests after the first round of review.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool feature! Lgtm except for one issue (see inline), but that may also just me not having written thumb asm in a long time.

cpu/cortexm_common/thread_arch.c Outdated Show resolved Hide resolved
tests/cpu/cortexm_stack_limit/main.c Outdated Show resolved Hide resolved
"ldr r0, [r0] \n" /* load tcb->sp to register 1 */
#ifdef MODULE_CORTEXM_STACK_LIMIT
"mov r2, r0 \n" /* Save content of R0 into R2*/
"bl _get_new_stacksize \n" /* Get the new lower limit stack in R0 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been quite some time since I've last have written thumb asm, so maybe I'm just a bit lost here. But if I recall correctly r0 to r3 are used to pass arguments and return values and are assumed to be saved by the caller. In that case, r2 could be overwritten by the code _get_new_stacksize() emits, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.
When I looked at objdump, only R0 and R3 were used by this function so I just kept R2 to temporary save R0 content.
I'll try to use another register, it shouldn't be too much pain.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved to R4 for storing R0 content now. This register is restored a few instructions after anyways so it shouldn't be an issue.

@dylad dylad force-pushed the pr/cpu/cortexm33/add_splim_support branch from 5019fed to 071a1d8 Compare May 6, 2024 15:15
@dylad dylad added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 6, 2024
@riot-ci
Copy link

riot-ci commented May 6, 2024

Murdock results

✔️ PASSED

071a1d8 features.yaml: add cortexm_stack_limit entry

Success Failures Total Runtime
10082 0 10083 12m:09s

Artifacts

@maribu maribu added this pull request to the merge queue May 6, 2024
Merged via the queue into RIOT-OS:master with commit 2e9ce4d May 7, 2024
26 checks passed
@dylad
Copy link
Member Author

dylad commented May 7, 2024

Thanks!

@dylad dylad deleted the pr/cpu/cortexm33/add_splim_support branch May 7, 2024 04:58
@mguetschow mguetschow added this to the Release 2024.07 milestone Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: build system Area: Build system Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: cpu Area: CPU/MCU ports Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants