kernel/arm: fix race condition when setting _Swap() return value

There was a possible race condition when setting the return value of a
thread that is pending, from an ISR.

A kernel function causes a thread to pend, with the following series of
steps:

- disable interrupts
- move current thread to wait_q
- call _Swap

Depending if running on M3/4 or M0+, _Swap will either issue a svc #0,
or pend PendSV directly. The same problem exists in both cases.

M3/4:
__svc will:
- enable interrupts
- trigger __pendsv

M0+:
_Swap() will enable interrupts.

__pendsv will:
- save register context including PSP into the thread struct

If an interrupt occurs between interrupts being enabled them and
__pendsv saving PSP, and the ISR sets the pending thread's return value,
this will happen:

- sees the thread in a wait_q
- removes it
- makes it ready
- calls _set_thread_return_value
- _set_thread_return_value looks at the thread's saved PSP to poke
  the value

In this scenario, PSP hasn't yet been updated by __pendsv so it's a
stale value from the previous context switch, resulting in unpredictable
word on the stack getting set to the return value.

There is no way to fix this issue and still have the return value being
delivered directly in the pending thread's exception stack frame, in the
M0+ case. There will always be a window between the unlocking of
interrupts and PendSV being handled. On M3/4, it could be possible with
the mix of SVC and PendSV, since the exception stack frame is created in
the __svc handler. However, because we want to keep the two
implementations as close as possible, and there were talks of moving
M3/4 to using PendSV only, to save an exception, the approach taken
solves both cases.

The approach taken is similar to the ARC and Nios2 ports, where
there is a field in the thread structure that holds the return value.
_Swap() then loads r0/a1 with that value just before returning.

Fixes ZEP-1289.

Change-Id: Iee7e06fe3f8ded84aff918fd43408c7f589344d9
Signed-off-by: Benjamin Walsh <benjamin.walsh@windriver.com>

arch/arm/core/offsets/offsets.c

@@ -38,6 +38,7 @@
 #include <kernel_offsets.h>
 
 GEN_OFFSET_SYM(_thread_arch_t, basepri);
+GEN_OFFSET_SYM(_thread_arch_t, swap_return_value);
 
 #ifdef CONFIG_FLOAT
 GEN_OFFSET_SYM(_thread_arch_t, preempt_float);

arch/arm/core/swap.S

@@ -234,16 +234,6 @@ SECTION_FUNC(TEXT, __svc)
 _context_switch:
 #endif
 
-    /*
-     * Set _Swap()'s default return code to -EAGAIN. This eliminates the
-     * need for the timeout code to invoke fiberRtnValueSet().
-     */
-
-    mrs  r2, PSP   /* thread mode, stack frame is on PSP */
-    ldr  r3, =_k_neg_eagain
-    ldr  r3, [r3, #0]
-    str  r3, [r2, #___esf_t_a1_OFFSET]
-
     /*
      * Unlock interrupts:
      * - in a SVC call, so protected against context switches
@@ -305,17 +295,21 @@ SECTION_FUNC(TEXT, _Swap)
     ldr r2, [r1, #_kernel_offset_to_current]
     str r0, [r2, #_thread_offset_to_basepri]
 
+    /*
+     * Set _Swap()'s default return code to -EAGAIN. This eliminates the need
+     * for the timeout code to set it itself.
+     */
+    ldr r1, =_k_neg_eagain
+    ldr r1, [r1]
+    str r1, [r2, #_thread_offset_to_swap_return_value]
+
 #if defined(CONFIG_CPU_CORTEX_M0_M0PLUS)
     /* No priority-based interrupt masking on M0/M0+,
      * pending PendSV is used instead of svc
      */
     ldr r1, =_SCS_ICSR
-    ldr r2, =_SCS_ICSR_PENDSV
-    str r2, [r1, #0]
-
-    /* load -EAGAIN as the default return value */
-    ldr r0, =_k_neg_eagain
-    ldr r0, [r0]
+    ldr r3, =_SCS_ICSR_PENDSV
+    str r3, [r1, #0]
 
     /* Unlock interrupts to allow PendSV, since it's running at prio 0xff
      *
@@ -323,12 +317,10 @@ SECTION_FUNC(TEXT, _Swap)
      * of a higher priority pending.
      */
     cpsie i
-
-    /* PC stored in stack frame by the hw */
-    bx lr
 #else /* CONFIG_CPU_CORTEX_M3_M4 */
     svc #0
-
-    /* r0 contains the return value if needed */
-    bx lr
 #endif
+
+    /* coming back from exception, r2 still holds the pointer to _current */
+    ldr r0, [r2, #_thread_offset_to_swap_return_value]
+    bx lr

arch/arm/core/thread.c

@@ -140,6 +140,8 @@ void _new_thread(char *pStackMem, unsigned stackSize,
 	tcs->callee_saved.psp = (uint32_t)pInitCtx;
 	tcs->arch.basepri = 0;
 
+	/* swap_return_value can contain garbage */
+
 	_nano_timeout_thread_init(tcs);
 
 	/* initial values in all other registers/TCS entries are irrelevant */

arch/arm/include/kernel_arch_data.h

@@ -142,6 +142,9 @@ struct _thread_arch {
 	/* interrupt locking key */
 	uint32_t basepri;
 
+	/* r0 in stack frame cannot be written to reliably */
+	uint32_t swap_return_value;
+
 #ifdef CONFIG_FLOAT
 	/*
 	 * No cooperative floating point register set structure exists for

arch/arm/include/kernel_arch_func.h

@@ -47,25 +47,10 @@ static ALWAYS_INLINE void nanoArchInit(void)
 	_CpuIdleInit();
 }
 
-/**
- *
- * @brief Set the return value for the specified fiber (inline)
- *
- * The register used to store the return value from a function call invocation
- * to <value>.  It is assumed that the specified <fiber> is pending, and thus
- * the fiber's thread is stored in its struct tcs structure.
- *
- * @param fiber pointer to the fiber
- * @param value is the value to set as a return value
- *
- * @return N/A
- */
 static ALWAYS_INLINE void
 _set_thread_return_value(struct k_thread *thread, unsigned int value)
 {
-	struct __esf *esf = (struct __esf *)thread->callee_saved.psp;
-
-	esf->a1 = value;
+	thread->arch.swap_return_value = value;
 }
 
 extern void nano_cpu_atomic_idle(unsigned int);

arch/arm/include/offsets_short_arch.h

@@ -30,6 +30,9 @@
 #define _thread_offset_to_basepri \
 	(___thread_t_arch_OFFSET + ___thread_arch_t_basepri_OFFSET)
 
+#define _thread_offset_to_swap_return_value \
+	(___thread_t_arch_OFFSET + ___thread_arch_t_swap_return_value_OFFSET)
+
 #define _thread_offset_to_preempt_float \
 	(___thread_t_arch_OFFSET + ___thread_arch_t_preempt_float_OFFSET)