fix(riscv): adjust TCBs lowest stack address when the FPU is used

components/freertos/FreeRTOS-Kernel/portable/riscv/port.c

@@ -296,6 +296,9 @@ static void vPortCleanUpCoprocArea(void *pvTCB)
     /* If the Task used any coprocessor, check if it is the actual owner of any.
      * If yes, reset the owner. */
     if (sa->sa_enable != 0) {
+        /* Restore the original lowest address of the stack in the TCB */
+        task->pxDummy6 = sa->sa_tcbstack;
+
         /* Get the core the task is pinned on */
         #if ( configNUM_CORES > 1 )
             const BaseType_t coreID = task->xDummyCoreID;
@@ -774,30 +777,56 @@ void vPortTaskPinToCore(StaticTask_t* task, int coreid)
 }
 #endif /* configNUM_CORES > 1 */
 
+
+/**
+ * @brief Function to call to simulate an `abort()` occurring in a different context than the one it's called from.
+ */
+extern void xt_unhandled_exception(void *frame);
+
 /**
  * @brief Get coprocessor save area out of the given task. If the coprocessor area is not created,
  *        it shall be allocated.
+ *
+ * @param task Task to get the coprocessor save area of
+ * @param allocate When true, memory will be allocated for the coprocessor if it hasn't been allocated yet.
+ *                 When false, the coprocessor memory will be left as NULL if not allocated.
+ * @param coproc Coprocessor number to allocate memory for
  */
-RvCoprocSaveArea* pxPortGetCoprocArea(StaticTask_t* task, int coproc)
+RvCoprocSaveArea* pxPortGetCoprocArea(StaticTask_t* task, bool allocate, int coproc)
 {
+    assert(coproc < SOC_CPU_COPROC_NUM);
+
     const UBaseType_t bottomstack = (UBaseType_t) task->pxDummy8;
     RvCoprocSaveArea* sa = pxRetrieveCoprocSaveAreaFromStackPointer(bottomstack);
     /* Check if the allocator is NULL. Since we don't have a way to get the end of the stack
      * during its initialization, we have to do this here */
     if (sa->sa_allocator == 0) {
+        /* Since the lowest stack address shall not be used as `sp` anymore, we will modify it */
+        sa->sa_tcbstack = task->pxDummy6;
         sa->sa_allocator = (UBaseType_t) task->pxDummy6;
     }
 
     /* Check if coprocessor area is allocated */
-    if (sa->sa_coprocs[coproc] == NULL) {
+    if (allocate && sa->sa_coprocs[coproc] == NULL) {
         const uint32_t coproc_sa_sizes[] = {
             RV_COPROC0_SIZE, RV_COPROC1_SIZE
         };
-        /* Allocate the save area at end of the allocator */
-        UBaseType_t allocated = sa->sa_allocator + coproc_sa_sizes[coproc];
-        sa->sa_coprocs[coproc] = (void*) allocated;
-        /* Update the allocator address for next use */
-        sa->sa_allocator = allocated;
+        /* The allocator points to a usable part of the stack, use it for the coprocessor */
+        sa->sa_coprocs[coproc] = (void*) (sa->sa_allocator);
+        sa->sa_allocator += coproc_sa_sizes[coproc];
+        /* Update the lowest address of the stack to prevent FreeRTOS performing overflow/watermark checks on the coprocessors contexts */
+        task->pxDummy6 = (void*) (sa->sa_allocator);
+        /* Make sure the Task stack pointer is not pointing to the coprocessor context area, in other words, make
+         * sure we don't have a stack overflow */
+        void* task_sp = task->pxDummy1;
+        if (task_sp <= task->pxDummy6) {
+            /* In theory we need to call vApplicationStackOverflowHook to trigger the stack overflow callback,
+             * but in practice, since we are already in an exception handler, this won't work, so let's manually
+             * trigger an exception with the previous FPU owner's TCB */
+            g_panic_abort = true;
+            g_panic_abort_details = (char *) "ERROR: Stack overflow while saving FPU context!\n";
+            xt_unhandled_exception(task_sp);
+        }
     }
     return sa;
 }
@@ -825,7 +854,8 @@ RvCoprocSaveArea* pxPortUpdateCoprocOwner(int coreid, int coproc, StaticTask_t*
     StaticTask_t* former = Atomic_SwapPointers_p32((void**) owner_addr, owner);
     /* Get the save area of former owner */
     if (former != NULL) {
-        sa = pxPortGetCoprocArea(former, coproc);
+        /* Allocate coprocessor memory if not available yet */
+        sa = pxPortGetCoprocArea(former, true, coproc);
     }
     return sa;
 }
@@ -836,7 +866,6 @@ RvCoprocSaveArea* pxPortUpdateCoprocOwner(int coreid, int coproc, StaticTask_t*
  */
 void vPortCoprocUsedInISR(void* frame)
 {
-    extern void xt_unhandled_exception(void*);
     /* Since this function is called from an exception handler, the interrupts are disabled,
      * as such, it is not possible to trigger another exception as would `abort` do.
      * Simulate an abort without actually triggering an exception. */

components/freertos/FreeRTOS-Kernel/portable/riscv/portasm.S

@@ -1,5 +1,5 @@
 /*
- * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD
+ * SPDX-FileCopyrightText: 2015-2024 Espressif Systems (Shanghai) CO LTD
  *
  * SPDX-License-Identifier: Apache-2.0
  */
@@ -193,6 +193,12 @@ rtos_save_fpu_coproc_nosave:
 #endif /* configNUM_CORES > 1 */
     /* Check if we have to restore a previous FPU context from the current TCB */
     mv    a0, s1
+    /* Do not allocate memory for the FPU yet, delay this until another task wants to use it.
+     * This guarantees that if a stack overflow occurs when allocating FPU context on the stack,
+     * the current task context is flushed and updated in the TCB, generating a correct backtrace
+     * from the panic handler.  */
+    li    a1, 0
+    li    a2, FPU_COPROC_IDX
     call  pxPortGetCoprocArea
     /* Get the enable flags from the coprocessor save area */
     lw    a1, RV_COPROC_ENABLE(a0)

components/freertos/test_apps/freertos/port/test_fpu_watermark.c

@@ -0,0 +1,82 @@
+/*
+ * SPDX-FileCopyrightText: 2024 Espressif Systems (Shanghai) CO LTD
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+#include "sdkconfig.h"
+#include <string.h>
+#include "soc/soc_caps.h"
+#include "freertos/FreeRTOS.h"
+#include "freertos/task.h"
+#include "unity.h"
+
+#define TASKS_STATUS_ARRAY_LEN  16
+
+/**
+ * On RISC-V targets that have coprocessors, the contexts are saved at the lowest address of the stack,
+ * which can lead to wrong stack watermark calculation in FreeRTOS in theory.
+ * As such, the port layer of FreeRTOS will adjust the lowest address of the stack when a coprocessor
+ * context is saved.
+ */
+#if CONFIG_IDF_TARGET_ARCH_RISCV && SOC_CPU_COPROC_NUM > 0
+
+volatile float s_float = 0.1999f;
+
+static float add_floats(float f1, float f2)
+{
+    return f1 + f2;
+}
+
+static void other_task(void* arg)
+{
+    const TaskHandle_t main_task = (TaskHandle_t) arg;
+
+    /* The second task shall also use the FPU to force a context flush on the main task */
+    add_floats(s_float, s_float);
+
+    xTaskNotifyGive(main_task);
+
+    /* The FPU context should be flushed on the main task, notify it and return */
+    vTaskDelete(NULL);
+}
+
+
+TEST_CASE("FPU: Context save does not affect stack watermark", "[freertos]")
+{
+    TaskHandle_t pvCreatedTask;
+    /* Force the FreeRTOS port layer to store an FPU context in the current task.
+     * So let's use the FPU and make sure another task, on the SAME CORE, also uses it */
+    const int core_id = xPortGetCoreID();
+    const TaskHandle_t current_handle = xTaskGetCurrentTaskHandle();
+
+    /* Get the current stack watermark */
+    const UBaseType_t before_watermark = uxTaskGetStackHighWaterMark(current_handle);
+
+    /* Use the FPU unit, the context will NOT be flushed until another task starts using it */
+    add_floats(s_float, s_float);
+
+    xTaskCreatePinnedToCore( other_task,
+                             "OtherTask",
+                             2048,
+                             (void*) current_handle,
+                             CONFIG_UNITY_FREERTOS_PRIORITY - 1,
+                             &pvCreatedTask,
+                             core_id);
+
+    vTaskDelay(10);
+
+    /* Wait for other task to complete */
+    ulTaskNotifyTake(pdTRUE, portMAX_DELAY);
+
+    const UBaseType_t after_watermark = uxTaskGetStackHighWaterMark(current_handle);
+
+    /* The current task has seen a general-purpose registers context save as well as an FPU context save
+     * so we have at least 32*2 registers saved on the stack, which represents 256 bytes.
+     * In practice, it may be very different, for example a call to printf would result is more than 1KB
+     * of additional stack space used. So let's just make sure that the watermark is bigger than 50% of the
+     * former watermark. */
+    TEST_ASSERT_TRUE(after_watermark > before_watermark / 2);
+}
+
+#endif // CONFIG_IDF_TARGET_ARCH_RISCV && SOC_CPU_COPROC_NUM > 0

components/riscv/include/riscv/rvruntime-frames.h

@@ -148,6 +148,8 @@ STRUCT_END(RvFPUSaveArea)
 STRUCT_BEGIN
 /* Enable bitmap: BIT(i) represents coprocessor i, 1 is used, 0 else */
 STRUCT_FIELD  (long, 4, RV_COPROC_ENABLE, sa_enable)
+/* Address of the original lowest stack address, convenient when the stack needs to re-initialized */
+STRUCT_FIELD  (void*, 4, RV_COPROC_TCB_STACK, sa_tcbstack)
 /* Address of the pool of memory used to allocate coprocessors save areas */
 STRUCT_FIELD  (long, 4, RV_COPROC_ALLOCATOR, sa_allocator)
 /* Pointer to the coprocessors save areas */