Skip to content

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

Merged
maribu merged 3 commits intoRIOT-OS:masterfrom
dylad:pr/cpu/cortexm33/add_splim_support
May 7, 2024
Merged

cpu/cortexm: add stack limit support for Cortex-M33#20633
maribu merged 3 commits intoRIOT-OS:masterfrom
dylad:pr/cpu/cortexm33/add_splim_support

Conversation

@dylad
Copy link
Copy Markdown
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
Copy Markdown
Member Author

dylad commented Apr 27, 2024

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

@benpicco benpicco requested a review from Teufelchen1 April 27, 2024 23:45
maribu
maribu reviewed May 3, 2024
View reviewed changes
Copy link
Copy Markdown
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
"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
Copy Markdown
Member

@maribu maribu May 3, 2024

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
Copy Markdown
Member Author

@dylad dylad May 3, 2024

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
Copy Markdown
Member Author

@dylad dylad May 6, 2024

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
Copy Markdown

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
@dylad
Copy link
Copy Markdown
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

Reviewers

@maribu maribu maribu approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030 kaspar030 is a code owner

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@Teufelchen1 Teufelchen1 Awaiting requested review from Teufelchen1

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dylad @riot-ci @maribu @mguetschow