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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/thread.c
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,7 @@
stacksize = (char *) thread->tls - stack;

_init_tls(thread->tls);
#endif

Check warning on line 243 in core/thread.c

View workflow job for this annotation

GitHub Actions / static-tests

Uncrustify proposes the following patch: --- a/core/thread.c +++ b/core/thread.c @@ -236,8 +236,8 @@ kernel_pid_t thread_create(char *stack, int stacksize, uint8_t priority, * Make sure the TLS area is aligned as required and that the * resulting stack will also be aligned as required */ - thread->tls = (void *) ((uintptr_t) tls & ~ (TLS_ALIGN - 1)); - stacksize = (char *) thread->tls - stack; + thread->tls = (void *)((uintptr_t)tls & ~(TLS_ALIGN - 1)); + stacksize = (char *)thread->tls - stack; _init_tls(thread->tls); #endif

#if defined(DEVELHELP) || defined(SCHED_TEST_STACK) \
|| defined(MODULE_TEST_UTILS_PRINT_STACK_USAGE)
Expand Down Expand Up @@ -285,7 +285,7 @@
thread->sp = thread_stack_init(function, arg, stack, stacksize);

#if defined(DEVELHELP) || IS_ACTIVE(SCHED_TEST_STACK) || \
defined(MODULE_MPU_STACK_GUARD)
defined(MODULE_MPU_STACK_GUARD) || defined(MODULE_CORTEXM_STACK_LIMIT)
thread->stack_start = stack;
#endif

Expand Down
1 change: 1 addition & 0 deletions cpu/cortexm_common/Makefile.features
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ else ifeq ($(CPU_CORE),cortex-m3)
RUST_TARGET = thumbv7m-none-eabi
else ifeq ($(CPU_CORE),cortex-m33)
CPU_ARCH := armv8m
FEATURES_PROVIDED += cortexm_stack_limit
#RUST_TARGET = thumbv8m.main-none-eabi
else ifeq ($(CPU_CORE),cortex-m4)
CPU_ARCH := armv7m
Expand Down
17 changes: 15 additions & 2 deletions cpu/cortexm_common/thread_arch.c
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,13 @@ void NORETURN cpu_switch_context_exit(void)
UNREACHABLE();
}

#ifdef MODULE_CORTEXM_STACK_LIMIT
static void* __attribute__((used)) _get_new_stacksize(unsigned int *args) {
thread_t* t = (thread_t*) args;
return thread_get_stackstart(t);
}
#endif

#if CPU_CORE_CORTEXM_FULL_THUMB
void __attribute__((naked)) __attribute__((used)) isr_pendsv(void) {
__asm__ volatile (
Expand Down Expand Up @@ -337,8 +344,14 @@ void __attribute__((naked)) __attribute__((used)) isr_pendsv(void) {

/* current thread context is now saved */
"restore_context: \n" /* Label to skip thread state saving */

"ldr r0, [r0] \n" /* load tcb->sp to register 1 */
#ifdef MODULE_CORTEXM_STACK_LIMIT
"mov r4, r0 \n" /* Save content of R0 into R4*/
"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.

"msr psplim, r0 \n" /* Set the PSP lower limit stack */
"ldr r0, [r4] \n" /* Load tcb->sp to register 0 */
#else
"ldr r0, [r0] \n" /* load tcb->sp to register 0 */
#endif
"ldmia r0!, {r4-r11,lr} \n" /* restore other registers, including lr */
#ifdef MODULE_CORTEXM_FPU
"tst lr, #0x10 \n"
Expand Down
5 changes: 5 additions & 0 deletions cpu/cortexm_common/vectors_cortexm.c
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
/**
* @brief Allocation of the interrupt stack
*/
__attribute__((used,section(".isr_stack"))) uint8_t isr_stack[ISR_STACKSIZE];

Check warning on line 81 in cpu/cortexm_common/vectors_cortexm.c

View workflow job for this annotation

GitHub Actions / static-tests

comma should be followed by whitespace

/**
* @brief Pre-start routine for CPU-specific settings
Expand All @@ -99,6 +99,11 @@
uint32_t *dst;
const uint32_t *src = &_etext;

#ifdef __ARM_ARCH_8M_MAIN__
/* Set the lower limit of the exception stack into MSPLIM register */
__set_MSPLIM((uint32_t)&_sstack);
#endif

cortexm_init_fpu();

#ifdef MODULE_PUF_SRAM
Expand Down
2 changes: 2 additions & 0 deletions features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ groups:
- name: cpu_check_address
help: The @ref cpu_check_address can be used to check if
accessing a given address would cause a bus fault.
- name: cortexm_stack_limit
help: ARM Cortex-M PSPLIM/MSPLIM registers are available.
- name: cortexm_svc
help: ARM Cortex-M Supervisor Calls are available.
- name: cortexm_fpu
Expand Down
1 change: 1 addition & 0 deletions makefiles/features_existing.inc.mk
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ FEATURES_EXISTING := \
can_rx_mailbox \
cortexm_fpu \
cortexm_mpu \
cortexm_stack_limit \
cortexm_svc \
cpp \
cpu_arm7tdmi_gba \
Expand Down
4 changes: 4 additions & 0 deletions makefiles/features_modules.inc.mk
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,10 @@ ifneq (,$(filter periph_rtc,$(USEMODULE)))
USEMODULE += rtc_utils
endif

# select cortexm_stack_limit pseudomodule if the corresponding
# feature is used
USEMODULE += $(filter cortexm_stack_limit, $(FEATURES_USED))

# select cortexm_svc pseudomodule if the corresponding feature is used
USEMODULE += $(filter cortexm_svc, $(FEATURES_USED))

Expand Down
10 changes: 10 additions & 0 deletions makefiles/pseudomodules.inc.mk
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ PSEUDOMODULES += board_software_reset

PSEUDOMODULES += arduino_pwm
PSEUDOMODULES += arduino_serial_stdio
## @defgroup pseudomodule_arm_stack_limit arm_stack_limit
## @{
## @brief Set MSP/PSP stack lower limit
##
## Use PSPLIM and MSPLIM ARM registers to set the lower limit of a stack
## This is a protection mechanism to catch stack overflow early before it
## can corrupt adjacent memory. Only available on ARMv8-M architecture.
PSEUDOMODULES += cortexm_stack_limit
## @}

PSEUDOMODULES += can_mbox
PSEUDOMODULES += can_pm
PSEUDOMODULES += can_raw
Expand Down
11 changes: 11 additions & 0 deletions tests/cpu/cortexm_stack_limit/Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
BOARD ?= nrf5340dk-app

include ../Makefile.cpu_common

FEATURES_REQUIRED += cortexm_stack_limit

include $(RIOTBASE)/Makefile.include

ifeq (llvm,$(TOOLCHAIN))
CFLAGS += -Wno-infinite-recursion
endif
99 changes: 99 additions & 0 deletions tests/cpu/cortexm_stack_limit/main.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Copyright (C) 2024 Mesotic SAS
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/

/**
* @ingroup tests
* @{
*
* @file
* @brief Test application for the cortexm_stack_limit pseudo-module
*
* @author Dylan Laduranty <[email protected]>
*
* @}
*/

#include <stdio.h>

#include "cpu.h"
#include "thread.h"
#include "board.h"

#define CANARY_VALUE 0xdeadbeef

static struct {
unsigned int canary;
char stack[THREAD_STACKSIZE_MAIN];
} buf;

/* Tell modern GCC (12.x) to not complain that this infinite recursion is
* bound to overflow the stack - this is exactly what this test wants to do :)
*
* Also, tell older versions of GCC that do not know about -Winfinit-recursion
* that it is safe to ignore `GCC diagnostics ignored "-Winfinit-recursion"`.
* They behave as intended in this case :)
*/
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpragmas"
#pragma GCC diagnostic ignored "-Winfinite-recursion"
static int recurse(int counter)
{
if (buf.canary != CANARY_VALUE) {
#ifdef LED0_ON
LED0_ON;
#endif
#ifdef LED1_ON
LED1_ON;
#endif
#ifdef LED2_ON
LED2_ON;
#endif
#ifdef LED3_ON
LED3_ON;
#endif

for (;;) {
thread_sleep();
}
}

counter++;
/* Recursing twice here prevents the compiler from optimizing-out the recursion. */
return recurse(counter) + recurse(counter);
}
#pragma GCC diagnostic pop

static void *thread(void *arg)
{
(void) arg;

recurse(0);

return NULL;
}

int main(void)
{
puts("\nCortex-M Stack limit test\n");

puts("If the test fails, all onboard LEDs will be on");

#ifdef MODULE_CORTEXM_STACK_LIMIT
puts("The cortexm_stack_limit module is present. Expect the board to crash"\
" without onboard LEDs on.\n");
#else
puts("The cortexm_stack_limit module is missing! Expect the test to crash"\
" with all onboard LEDs on\n");
#endif

buf.canary = CANARY_VALUE;
thread_create(buf.stack, sizeof(buf.stack), THREAD_PRIORITY_MAIN + 1,
0, thread, NULL, "thread");

return 0;
}
Loading