diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c b/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c index cb6ace1700..de2baa1f9b 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/port.c +++ b/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. */ diff --git a/components/freertos/FreeRTOS-Kernel/portable/riscv/portasm.S b/components/freertos/FreeRTOS-Kernel/portable/riscv/portasm.S index f63c880372..6a99e7ebb9 100644 --- a/components/freertos/FreeRTOS-Kernel/portable/riscv/portasm.S +++ b/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) diff --git a/components/freertos/test_apps/freertos/port/test_fpu_watermark.c b/components/freertos/test_apps/freertos/port/test_fpu_watermark.c new file mode 100644 index 0000000000..5f29540bbf --- /dev/null +++ b/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 +#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 diff --git a/components/riscv/include/riscv/rvruntime-frames.h b/components/riscv/include/riscv/rvruntime-frames.h index c0546ce65f..e218d6a5ad 100644 --- a/components/riscv/include/riscv/rvruntime-frames.h +++ b/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 */