ARMv8-M-ML CRT0 Defects

[emulator]

Debugging RP2350 startup a bit more and I think there are a couple of issues that should be fixed.

RP2350 RCP Initialization
Originally I had thought there was an issue with needing to enable the RCP on the RP2350 but it turns out the bootrom by default leaves this in the expected enabled state. What was actually happening was when the FPU was enabled the write to CPACR clobbered any default port states including deactivating the RCP. I believe we need to change the FPU activation to be RMW to avoid deactivating any coprocessors that were previously enabled.

PSPLIM
The initial stack PSPLIM is always set at startup yet in the ARMV8-M-ML port the code to update it is guarded behind the CH_DBG_ENABLE_STACK_CHECK flag which is false by default. I believe we should either assume PSPLIM is always activate for ARMv8-M-ML or only set the PSPLIM at startup conditionally when its expected to be used? I've used CRT0_INIT_PSPLIM in the patch below but I'm a little unclear whether the ARM architecture guide assumes PSPLIM should always be used with ARMv8-M-ML or whether it should be optional. I'm a little less sure about this than the above issue.

os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S

Code: Select all

--- os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(revision 17872)
+++ os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(working copy)
@@ -62,6 +62,20 @@
 #endif
 
 /**
+ * @brief   PSPLIM initialization switch.
+ * @details PSPLIM is initialized to the process stack base address providing
+ *          hardware stack overflow detection during early startup.
+ * @note    Defaults to @p FALSE because the OS must save/restore PSPLIM
+ *          per-thread (PORT_SAVE_PSPLIM == TRUE) for this to be safe.
+ *          When enabled without per-thread PSPLIM management, threads whose
+ *          stacks reside below __process_stack_base__ will UsageFault on
+ *          their first stack operation.
+ */
+#if !defined(CRT0_INIT_PSPLIM) || defined(__DOXYGEN__)
+#define CRT0_INIT_PSPLIM                    FALSE
+#endif
+
+/**
  * @brief   VTOR special register initialization.
  * @details VTOR is initialized to point to the vectors table.
  */
@@ -215,8 +229,10 @@
                 /* PSP stack pointers initialization.*/
                 ldr     r0, =__process_stack_end__
                 msr     PSP, r0
+#if CRT0_INIT_PSPLIM == TRUE
                 ldr     r0, =__process_stack_base__
                 msr     PSPLIM, r0
+#endif
 
 #if CRT0_VTOR_INIT == TRUE
                 ldr     r0, =_vectors
@@ -234,11 +250,12 @@
                 dsb
                 isb
 
-                /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
+                /* CPACR initialization, read-modify-write to preserve
+                   coprocessor bits set by bootrom (e.g. CP7/RCP on RP2350).*/
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                orr     r0, r0, #CRT0_CPACR_INIT
                 str     r0, [r1]
                 dsb
                 isb
@@ -401,8 +418,10 @@
                 /* PSP stack pointers initialization.*/
                 ldr     r0, =__c\core\()_process_stack_end__
                 msr     PSP, r0
+#if CRT0_INIT_PSPLIM == TRUE
                 ldr     r0, =__c\core\()_process_stack_base__
                 msr     PSPLIM, r0
+#endif
 
 #if CRT0_VTOR_INIT == TRUE
                 ldr     r0, =_vectors
@@ -420,11 +439,12 @@
                 dsb
                 isb
 
-                /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
+                /* CPACR initialization, read-modify-write to preserve
+                   coprocessor bits set by bootrom (e.g. CP7/RCP on RP2350).*/
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                orr     r0, r0, #CRT0_CPACR_INIT
                 str     r0, [r1]
                 dsb
                 isb

[Giovanni]

Hi,

PSPLIM must be initialized in any case, the stack is always checked vs it, the written value is meant to protect the default process stack.

The CPACR code should be more like this because it is a 32bits value:

Code: Select all

  movw    r1, #SCB_CPACR & 0xFFFF                           
  movt    r1, #SCB_CPACR >> 16                                                  
  ldr     r0, [r1]                                                              
  movw    r2, #CRT0_CPACR_INIT & 0xFFFF                                         
  movt    r2, #CRT0_CPACR_INIT >> 16                                            
  orr     r0, r0, r2                                                            
  str     r0, [r1]                                                              

Giovanni

[emulator]

Yes you are right. Let me narrow the initial patch to just CAPCR until I can figure out what I'm doing wrong with PSPLIM.

os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S

Code: Select all

--- os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(revision 17872)
+++ os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(working copy)
@@ -235,10 +235,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb
@@ -421,10 +423,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb

[emulator]

Ok a bit more poking around and thinking about the issue...

With most (?all?) STM32 devices this works because the heap is always higher than __process_stack_base__ however on the RP2350 we have....

Core 0 stack: SCRATCH_Y (0x20081000, 4KB)
Core 1 stack: SCRATCH_X (0x20080000, 4KB)
Heap: main SRAM (0x20000000, 512KB)

Which is a problem when PSPLIM gets set to __process_stack_base__ and PORT_SAVE_PSPLIM isn't enabled by default (CH_DBG_ENABLE_STACK_CHECK is false)

One option is to leave PSPLIM at the reset default (0x00000000) which is basically PSPLIM disabled if PORT_SAVE_PSPLIM is false. But I think the right thing for ARMv8-M-ML is for PORT_SAVE_PSPLIM to be true by default. The overhead is tiny and I would argue its that PSPLIM should be the norm when using ARMv8-M-ML. I was wrong the overhead of this is marginally high

I guess a third option would be setting PSPLIM to zero only on RP2350 when PORT_SAVE_PSPLIM isn't enabled but that just feels really weird.

[emulator]

Here is my latest thought about this after trying to get a bit more familiar with PSPLIM. I don't think there is any reason to set PSPLIM in crt0 just leave it at the reset default. We then handle all of it in __port_switch but only if CH_DBG_ENABLE_STACK_CHECK is true. PSPLIM always gets set at the expected value before switching context (including when creating a new one as __port_switch is called for that as well). Unless I'm missing something this is no less secure than the previous approach while being slightly less code. It also naturally supports all memory layouts without any special handling.

Code: Select all

Index: os/common/ports/ARMv8-M-ML/compilers/GCC/chcoreasm.S
===================================================================
--- os/common/ports/ARMv8-M-ML/compilers/GCC/chcoreasm.S	(revision 17872)
+++ os/common/ports/ARMv8-M-ML/compilers/GCC/chcoreasm.S	(working copy)
@@ -86,17 +86,25 @@
                 /* Saving stack limit register.*/
                 mrs     r3, PSPLIM
                 push    {r3}
-                movs    r3, #0
-                msr     PSPLIM, r3      /* Temporarily disabling stack check.*/
 #endif
 
-                /* Switching stacks.*/
+                /* Save current thread SP.*/
                 str     sp, [r1, #CONTEXT_OFFSET]
+
+#if CH_DBG_ENABLE_STACK_CHECK
+                /* Restore the new thread PSPLIM before switching SP.*/
+                ldr     r2, [r0, #CONTEXT_OFFSET]
+                ldr     r3, [r2]
+                /* MSR does not trigger a limit check! */
+                msr     PSPLIM, r3 
+#endif
+
+                /* Switch to new thread stack.*/
                 ldr     sp, [r0, #CONTEXT_OFFSET]
 
 #if CH_DBG_ENABLE_STACK_CHECK
-                pop     {r3}
-                msr     PSPLIM, r3
+                /* Discard saved PSPLIM which was already restored above.*/
+                add     sp, sp, #4
 #endif
 
 #if CORTEX_USE_FPU
Index: os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S
===================================================================
--- os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(revision 17872)
+++ os/common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(working copy)
@@ -212,11 +212,9 @@
                 ldr     r0, =__main_stack_base__
                 msr     MSPLIM, r0
 
-                /* PSP stack pointers initialization.*/
+                /* PSP stack pointer initialization.*/
                 ldr     r0, =__process_stack_end__
                 msr     PSP, r0
-                ldr     r0, =__process_stack_base__
-                msr     PSPLIM, r0
 
 #if CRT0_VTOR_INIT == TRUE
                 ldr     r0, =_vectors
@@ -235,10 +233,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb
@@ -401,8 +401,6 @@
                 /* PSP stack pointers initialization.*/
                 ldr     r0, =__c\core\()_process_stack_end__
                 msr     PSP, r0
-                ldr     r0, =__c\core\()_process_stack_base__
-                msr     PSPLIM, r0
 
 #if CRT0_VTOR_INIT == TRUE
                 ldr     r0, =_vectors
@@ -421,10 +419,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb

[Giovanni]

The reason for setting up PSPLIM in the startup is because the process stack is setup on startup and PSPLIM is expected to be set on the lower boundary of that stack in order to catch overflows. Setting it to zero or leaving at reset value disables the overflow check for main().

Giovanni

[emulator]

Ok, let me think about this for a bit. Basically the current approach is broken on non-unified memory architectures like the PR2350 but works well for unified ones. The current 'fix' for RP2350 is forcing CH_DBG_ENABLE_STACK_CHECK always on, which forces PORT_SAVE_PSPLIM to true. It works but then there is a pretty significant overhead on every context switch.

[emulator]

I should say even with this issue I think my approach to handling PSPLIM might be better as it never disables PSPLIM if CH_DBG_ENABLE_STACK_CHECK is true and should work happily if PSPLIM is set in CRT0.

[emulator]

os/common/ports/ARMv8-M-ML-ALT/chcore.h
Ok, I think the safest fix is to keep the existing defaulting of CH_DBG_ENABLE_STACK_CHECK to true on RP2350. I do worry that a user will see DBG and may think they can disable it which wont work on the RP2350. Maybe we could allow PORT_SAVE_PSPLIM to be set directly without being overwritten?

Code: Select all

--- os/common/ports/ARMv8-M-ML-ALT/chcore.h	(revision 17872)
+++ os/common/ports/ARMv8-M-ML-ALT/chcore.h	(working copy)
@@ -572,15 +572,19 @@
 #define PORT_SAVE_CONTROL               FALSE
 #endif
 
+#if !defined(PORT_SAVE_PSPLIM)
 #if (PORT_USE_SYSCALL == TRUE) || (CH_DBG_ENABLE_STACK_CHECK == TRUE) ||    \
     defined(__DOXYGEN__)
 /**
  * @brief   PSPLIM as part of the saved thread context.
+ * @details Required on platforms where thread stacks reside in a different
+ *          memory region than the process stack (e.g. RP2350).
  */
 #define PORT_SAVE_PSPLIM                TRUE
 #else
 #define PORT_SAVE_PSPLIM                FALSE
 #endif
+#endif
 

common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S
A completely independent change to fix CAPCR to not overwrite already enabled coprocessors

Code: Select all

--- common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(revision 17872)
+++ common/startup/ARMCMx/compilers/GCC/crt0_v8m-ml.S	(working copy)
@@ -235,10 +235,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb
@@ -421,10 +423,12 @@
                 isb
 
                 /* CPACR initialization.*/
-                movw    r0, #CRT0_CPACR_INIT & 0xFFFF
-                movt    r0, #CRT0_CPACR_INIT >> 16
                 movw    r1, #SCB_CPACR & 0xFFFF
                 movt    r1, #SCB_CPACR >> 16
+                ldr     r0, [r1]
+                movw    r2, #CRT0_CPACR_INIT & 0xFFFF
+                movt    r2, #CRT0_CPACR_INIT >> 16
+                orr     r0, r0, r2
                 str     r0, [r1]
                 dsb
                 isb

[Giovanni]

Hi,

Perhaps you have not seen yet changes I made in the afternoon, PSPLIM is now always switched and present. CPACR handling also changed.

Giovanni