Merge branch 'bugfix/fpu_registers_overwritten_v5.1' into 'release/v5.1'

Xtensa: fix a bug that altered CPU registers in FPU exception handlers (backport v5.1)

See merge request espressif/esp-idf!24607

components/freertos/FreeRTOS-Kernel-SMP/portable/xtensa/xtensa_vectors.S

@@ -3,7 +3,7 @@
  *
  * SPDX-License-Identifier: MIT
  *
- * SPDX-FileContributor: 2016-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileContributor: 2016-2023 Espressif Systems (Shanghai) CO LTD
  */
 /*
  * Copyright (c) 2015-2019 Cadence Design Systems, Inc.
@@ -1009,6 +1009,12 @@ _xt_coproc_exc:
     s32i    a2,  sp, XT_STK_A2
     s32i    a3,  sp, XT_STK_A3
     s32i    a4,  sp, XT_STK_A4
+#if portNUM_PROCESSORS > 1
+    /* If multicore, we must save two more interruptee's register to use as
+     * scratch when taking/releasing the _xt_coproc_owner_sa_lock spinlock. */
+    s32i    a6,  sp, XT_STK_A6
+    s32i    a7,  sp, XT_STK_A7
+#endif /* portNUM_PROCESSORS > 1 */
     s32i    a15, sp, XT_STK_A15
 
     /* Call the RTOS coprocessor exception hook */
@@ -1039,7 +1045,7 @@ _xt_coproc_exc:
 #if portNUM_PROCESSORS > 1
     /* If multicore, we must also acquire the _xt_coproc_owner_sa_lock spinlock
      * to ensure thread safe access of _xt_coproc_owner_sa between cores. */
-    spinlock_take a0 a2 _xt_coproc_owner_sa_lock
+    spinlock_take a6 a7 _xt_coproc_owner_sa_lock
 #endif /* portNUM_PROCESSORS > 1 */
 
     /* Get old coprocessor owner thread (save area ptr) and assign new one.  */
@@ -1050,7 +1056,7 @@ _xt_coproc_exc:
 
 #if portNUM_PROCESSORS > 1
     /* Release previously taken spinlock */
-    spinlock_release a0 a2 _xt_coproc_owner_sa_lock
+    spinlock_release a6 a7 _xt_coproc_owner_sa_lock
 #endif /* portNUM_PROCESSORS > 1 */
 
     /* Only need to context switch if new owner != old owner. */
@@ -1133,6 +1139,10 @@ _xt_coproc_exc:
     /* Can omit rsync for wsr.CPENABLE here because _xt_user_exit does it. */
 .L_xt_coproc_done:
     l32i    a15, sp, XT_STK_A15
+#if portNUM_PROCESSORS > 1
+    l32i    a6,  sp, XT_STK_A6
+    l32i    a7,  sp, XT_STK_A7
+#endif /* portNUM_PROCESSORS > 1 */
     l32i    a5,  sp, XT_STK_A5
     l32i    a4,  sp, XT_STK_A4
     l32i    a3,  sp, XT_STK_A3

components/freertos/FreeRTOS-Kernel/portable/xtensa/xtensa_vectors.S

@@ -3,7 +3,7 @@
  *
  * SPDX-License-Identifier: MIT
  *
- * SPDX-FileContributor: 2016-2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileContributor: 2016-2023 Espressif Systems (Shanghai) CO LTD
  */
 /*
  * Copyright (c) 2015-2019 Cadence Design Systems, Inc.
@@ -1016,6 +1016,12 @@ _xt_coproc_exc:
     s32i    a2,  sp, XT_STK_A2
     s32i    a3,  sp, XT_STK_A3
     s32i    a4,  sp, XT_STK_A4
+#if portNUM_PROCESSORS > 1
+    /* If multicore, we must save two more interruptee's register to use as
+     * scratch when taking/releasing the _xt_coproc_owner_sa_lock spinlock. */
+    s32i    a6,  sp, XT_STK_A6
+    s32i    a7,  sp, XT_STK_A7
+#endif /* portNUM_PROCESSORS > 1 */
     s32i    a15, sp, XT_STK_A15
 
     /* Call the RTOS coprocessor exception hook */
@@ -1046,7 +1052,7 @@ _xt_coproc_exc:
 #if portNUM_PROCESSORS > 1
     /* If multicore, we must also acquire the _xt_coproc_owner_sa_lock spinlock
      * to ensure thread safe access of _xt_coproc_owner_sa between cores. */
-    spinlock_take a0 a2 _xt_coproc_owner_sa_lock
+    spinlock_take a6 a7 _xt_coproc_owner_sa_lock
 #endif /* portNUM_PROCESSORS > 1 */
 
     /* Get old coprocessor owner thread (save area ptr) and assign new one.  */
@@ -1057,7 +1063,7 @@ _xt_coproc_exc:
 
 #if portNUM_PROCESSORS > 1
     /* Release previously taken spinlock */
-    spinlock_release a0 a2 _xt_coproc_owner_sa_lock
+    spinlock_release a6 a7 _xt_coproc_owner_sa_lock
 #endif /* portNUM_PROCESSORS > 1 */
 
     /* Only need to context switch if new owner != old owner. */
@@ -1140,6 +1146,10 @@ _xt_coproc_exc:
     /* Can omit rsync for wsr.CPENABLE here because _xt_user_exit does it. */
 .L_xt_coproc_done:
     l32i    a15, sp, XT_STK_A15
+#if portNUM_PROCESSORS > 1
+    l32i    a6,  sp, XT_STK_A6
+    l32i    a7,  sp, XT_STK_A7
+#endif /* portNUM_PROCESSORS > 1 */
     l32i    a5,  sp, XT_STK_A5
     l32i    a4,  sp, XT_STK_A4
     l32i    a3,  sp, XT_STK_A3

components/freertos/test_apps/freertos/port/test_fpu_in_task.c

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -185,5 +185,68 @@ TEST_CASE("FPU: Usage in unpinned task", "[freertos]")
     }
 }
 
+typedef struct {
+    bool negative;
+    TaskHandle_t main;
+} ParamsFPU;
+
+
+/**
+ * @brief Function performing some simple calculation using several FPU registers.
+ *        The goal is to be preempted by a task that also uses the FPU on the same core.
+ */
+void fpu_calculation(void* arg)
+{
+    ParamsFPU* p = (ParamsFPU*) arg;
+    const bool negative = p->negative;
+    const float init = negative ? -1.f : 1.f;
+    float f = init;
+
+    for(int i = 0; i < 10; i++)
+    {
+        /* The following calculation doesn't really have any meaning, we try to use several FPU registers and operations */
+        float delta = negative ? -1.1f : 1.1f;
+        for (int i = 0; i < 1000; i++) {
+            f += delta;
+            delta += negative ? -0.1f : 0.1f;
+        }
+        /* Give some time to the other to interrupt us before checking `f` value */
+        esp_rom_delay_us(1000);
+
+        /**
+         * If the FPU context was not saved properly by FreeRTOS, our value `f` will not have to correct sign!
+         * It'll have the sign of the other tasks' `f` value.
+         * Use assert to make sure the sign is correct. Using TEST_ASSERT_TRUE triggers a stack overflow.
+         */
+        assert( (negative && f < 0.0f) || (!negative && f > 0.0f) );
+        f = init;
+
+        /* Give the hand back to FreeRTOS to avoid any watchdog */
+        vTaskDelay(2);
+    }
+
+    xTaskNotifyGive(p->main);
+    vTaskDelete(NULL);
+}
+
+
+
+TEST_CASE("FPU: Unsolicited context switch between tasks using FPU", "[freertos]")
+{
+    /* Create two tasks that are on the same core and use the same FPU */
+    TaskHandle_t unity_task_handle = xTaskGetCurrentTaskHandle();
+    TaskHandle_t tasks[2];
+    ParamsFPU params[2] = {
+        { .negative = false, .main = unity_task_handle },
+        { .negative = true,  .main = unity_task_handle },
+    };
+
+    xTaskCreatePinnedToCore(fpu_calculation, "Task1", 2048, params + 0, UNITY_FREERTOS_PRIORITY + 1, &tasks[0], 1);
+    xTaskCreatePinnedToCore(fpu_calculation, "Task2", 2048, params + 1, UNITY_FREERTOS_PRIORITY + 2, &tasks[2], 1);
+
+    ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+    ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+}
+
 #endif // configNUM_CORES > 1
 #endif // SOC_CPU_HAS_FPU