msp uninitialized on reset leading to usage fault for non-XIP targets

[GAnthony]

I have encountered an interesting issue while debugging an intermittent usage_fault on the TI CC3220SF, when loading the program to SRAM via OpenOCD, and running with CONFIG_INIT_STACKS enabled (per below commit).

The code in __start() assumes the msp is set to the value at the beginning of the _vector_table. Which it is, normally, when running from flash.

But, when loading to SRAM via gdb/OpenOCD, the msp register is not set (to the expected address of (_main_stack + CONFIG_MAIN_STACK_SIZE)), which causes the memset() call to push r4,r5,r6 (all zeros) onto a random stack address, and (in my case) overwriting some pinmux init code, which later causes a __usage_fault().

The code works fine when running from flash [Edit: w/o a debugger attached].

Some discussion with the original author yielded this suggestion:

We probably need some code in the early boot, before the memset(),
that sets the MSP to _main_stack + CONFIG_MAIN_STACK_SIZE, that would
only be there if running from SRAM. Maybe you can key on !CONFIG_XIP,
but I'm not sure that is still valid.

I attempted this solution, but still ran into issues later on when a call to _net_thread(....., _main_stack) initializes the _main_stack with 0xaa, overwriting a return address on the current stack, and causing yet another usage fault.

So, it seems I'm missing something with regards to stack initialization for non-XIP targets, which I'd appreciate an expert could look at.

commit 0c9268784e138f665e896ec313a91d122e012174
Author: Benjamin Walsh <benjamin.walsh@windriver.com>
Date:   Mon Nov 21 12:11:18 2016 -0500

    arm: support interrupt stack with CONFIG_INIT_STACKS
   
    Use the main stack during very early boot so that we can call memset on
    the interrupt stack. Initialize the interrupt stack before it is used
    for the rest of the pre-kernel initialization.
   
    Change-Id: I6fcc9a08678afdb82e83465cda1c7a2a8c849c9b
    Signed-off-by: Benjamin Walsh <benjamin.walsh@windriver.com>

diff --git a/arch/arm/core/cortex_m/reset.S b/arch/arm/core/cortex_m/reset.S
index dedb5f0..564d0a2 100644
--- a/arch/arm/core/cortex_m/reset.S
+++ b/arch/arm/core/cortex_m/reset.S
@@ -33,6 +33,7 @@
 _ASM_FILE_PROLOGUE
 
 GTEXT(__reset)
+GTEXT(memset)
 GDATA(_interrupt_stack)
 
 /**
@@ -78,6 +79,13 @@ SECTION_SUBSEC_FUNC(TEXT,_reset_section,__start)
     msr BASEPRI, r0
 #endif
 
+#ifdef CONFIG_INIT_STACKS
+    ldr r0, =_interrupt_stack
+    mov r1, #0xaa
+    ldr r2, =CONFIG_ISR_STACK_SIZE
+    bl memset
+#endif
+

[ioannisg]

@GAnthony Really interesting issue. I believe that the root of the problem is just the fact that the MSP is not initialized, and has nothing to do with XIP. This is a common problem, also, in systems with sequential booting, where the preceding image needs to set the MSP properly, before jumping to the succeeding image. (E.g. MCUBoot does that before calling the reset() function of the Zephyr image).

If you are loading a typical, Zephyr image, all we need to do IMO, is to load the MSP with the top of the vector table (initial stack pointer). The vector table it typically in the start of the image.

In theory, we could add a small code block on the top of __reset() that initializes the MSP (using VTOR[0] value, if the register exists, or any hard-coded value) to cover cases such like this one.

@agross-linaro am I missing something here?

[agross-korg]

@ioannisg That should cover it. We need to make sure the rest of the stack inits are done properly as well. @GAnthony mentioned that the main stack still gets messed up later. Maybe that is a separate issue, but still needs to be explained.

[ioannisg]

The MSP is not supposed to be initialized with __main_stack + CONFIG_MAIN_STACK_SIZE : that one is the start of the stack of the main thread, and I think it should not be overwritten.
Instead, I believe we need to initialize the MSP with whatever is the start of our vector table (typically the start of the flash)

Edit: [I admit, thought, that I cannot immediately see the problem of initializing the MSP with __main_stack + CONFIG_MAIN_STACK_SIZE, as that area is reserved at compile-time]. Actually, typically, this is the value in VTOR[0].

[GAnthony]

Yes, _vector_start[0] is set to the top of the _main_stack in vector_table.S

zephyr/arch/arm/core/cortex_m/vector_table.S

Line 36 in 246f03c

.word _main_stack + CONFIG_MAIN_STACK_SIZE

But, main stack will get overwritten when_net_thread(....., _main_stack) is called, overwriting a return address with 0xaaaaaaaa.

So, maybe _vector_start[0] needs to be initialized to a different stack address.
Also, I was wondering if SP needs to be explicitly set on __reset?

[ioannisg]

@GAnthony I would assume you refer to _new_thread_init(...), is it correct?
So, I did some quick study, and it looks like that the _main_stack is only used during early boot in reset.S, before configuring MCU to use PSP and the _interrupt_stack. And, it should not be used again, until we jump to the main thread. So, in my opinion, the _main_stack should not contain a return address.

Which return address is stored in the _main_stack, and when does this occur?

Also, I was wondering if SP needs to be explicitly set on __reset?

In reset.S, MSP needs to point to a valid memory area, that is safe to use, until we switch to use PSP. It is not (strictly) mandatory to be the _main_stack, IMO. @agross-linaro ?

[GAnthony]

Case 1) Loading to SRAM. CONFIG_XIP=n. Any Zephyr program (in this case samples/basic/blinky).
Stepping through reset.S in the debugger (gdb/opencode):

ldr r0, =_interrupt_stack
ldr r1, =CONFIG_ISR_STACK_SIZE
adds r0, r0, r1
msr PSP, r0
movs.n r0, #2	/* switch to using PSP (bit1 of CONTROL reg) */
msr CONTROL, r0

and then:

b _PrepC

I see on the first push instruction in _PrepC(), that the arguments are pushed to msp (not psp!), and sp is now updated to be same as msp. Result, overwrite of instruction memory, __usage_fault.

Case 2) Loading to FLASH. CONFIG_XIP=y

I see on the first push instruction in _PrepC(), that the arguments are pushed to psp (as expected), and sp is now updated to be same as psp. But, somehow the program still crashes _usage_fault in a swap instruction during main().

At this point, I feel I'm fighting two issues:

1. The originally reported issue: msp not getting set on debugger reset, when CONFIG_INIT_STACKS=y.
2. Running anything after attaching via gdb/OpenOCD seems to behave strangely, and often crashes.
   Given that running all these cases from Flash (w/o gdb) always works, I'm now suspecting something in the debugger/emulation/openocd for this platform.
   Going back to use older gdb/openocd versions also shows the same issue. So, the other variable is the emulation firmware.

[ioannisg]

@GAnthony thanks for doing this investigation.
@galak, @GAnthony, @agross-linaro , I think we might have just revealed a bug, here, which I had also tried to discuss in an earlier PR few days ago, take a look: #6977.
ARM insists on invoking ISB (and DSB, perhaps) instructions after switching Stack Pointer.

For Cortex-M3: http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0552a/CHDBIBGJ.html

Note
When changing the stack pointer, software must use an ISB instruction immediately after the MSR instruction. This ensures that instructions after the ISB instruction execute using the new stack pointer. See ISB

Could you please try to add the barrier instructions after modifying the control register to switch to PSP, and check if it solves any problem?

I will, anyways, open a PR to explicitly add ISB after SP switch in all places in the arm & kernel code.

[GAnthony]

Could you please try to add the barrier instructions after modifying the control register to switch to PSP, and check if it solves any problem?

I dialed back to the test case (CONFIG_XIP=n) in the original bug report. Adding the isb after the msr CONTROL didn't improve the behavior.

[GAnthony]

So, I did some quick study, and it looks like that the _main_stack is only used during early boot in reset.S, before configuring MCU to use PSP and the _interrupt_stack. And, it should not be used again, until we jump to the main thread. So, in my opinion, the _main_stack should not contain a return address.

@ioannisg, after lots of experiments and some research, I found that the following code:

ldr r0, =_interrupt_stack
ldr r1, =CONFIG_ISR_STACK_SIZE
adds r0, r0, r1
msr PSP, r0
movs.n r0, #2	/* switch to using PSP (bit1 of CONTROL reg) */
msr CONTROL, r0

sometimes does not update the CPU to use the PSP. This leaves the CPU to still be using the MSP.

When attaching the debugger to a running target (the target would be running whatever was previously flashed), gdb halts the processor at a random point in the running program. If the halt occurs in a handler (like SysTick, which is most of the time), then the CPU is in Handler Mode (xPSR lower bits != 0).

Per a Cortex-M4 document: http://infocenter.arm.com/help/topic/com.arm.doc.dui0553a/DUI0553A_cortex_m4_dgug.pdf

Handler mode always uses the MSP, so the processor ignores explicit writes to the active stack pointer bit of the CONTROL register when in Handler mode.

This would explain why all my intermittent __usage_faults just magically disappear when running from reset out of flash, without a debugger attached, since out of reset the CPU is in "Thread Mode", and in that case, Zephyr __reset code (the setting of the PSP) works.

So, now I wonder how this issue was not seen on other platforms? How do other platforms debug a Zephyr application from __reset()?

Note: I am fully able to debug TI SDK examples from reset. But TI startup code doesn't do any MSP/PSP hijinks at boot time: it just disables ints, sets the SP to the vector_table[0], does standard .bss, vector reloc, etc., then jumps to main.