From b03c8912c73fa59061d97a2f5fd5acddcc3fa356 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Fri, 30 Jun 2023 14:30:17 +0800 Subject: [PATCH 1/2] Xtensa: fix a bug that altered CPU registers in FPU exception handlers * Fixes https://github.com/espressif/esp-idf/issues/11690 --- components/xtensa/xtensa_vectors.S | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/components/xtensa/xtensa_vectors.S b/components/xtensa/xtensa_vectors.S index 40c372abe7e5..4843bbae70f9 100644 --- a/components/xtensa/xtensa_vectors.S +++ b/components/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 From 2aee63a773f3a02c6104328d8a10aa6f9c7521e8 Mon Sep 17 00:00:00 2001 From: Omar Chebib Date: Fri, 30 Jun 2023 18:25:31 +0800 Subject: [PATCH 2/2] freertos: add a unit test for FPU context switch --- .../freertos/port/test_fpu_in_task.c | 65 ++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/components/freertos/test_apps/freertos/port/test_fpu_in_task.c b/components/freertos/test_apps/freertos/port/test_fpu_in_task.c index bc160b40adce..a829ed9adf51 100644 --- a/components/freertos/test_apps/freertos/port/test_fpu_in_task.c +++ b/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