From f08c4b823fc8cac9430fec60f3032cf95cf321b1 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Sat, 15 Oct 2022 03:25:34 +0800 Subject: [PATCH 1/2] freertos: Fix FPU ISR core pinning bug This commit fixes a bug where if an unpinned task is interrupted by a level 1 ISR that users the FPU, the FPU usage will cause the interrupted task to become pinned to the current core. Note: This bug was already fixed in SMP FreeRTOS in commit d6936177. This commit simply backports the fix to IDF FreeRTOS. --- .../freertos/port/xtensa/xtensa_vectors.S | 43 ++++++++++--------- 1 file changed, 23 insertions(+), 20 deletions(-) diff --git a/components/freertos/port/xtensa/xtensa_vectors.S b/components/freertos/port/xtensa/xtensa_vectors.S index 1560ff27d6..f38c71269f 100644 --- a/components/freertos/port/xtensa/xtensa_vectors.S +++ b/components/freertos/port/xtensa/xtensa_vectors.S @@ -903,34 +903,32 @@ _xt_coproc_exc: /* Get co-processor state save area of new owner thread. */ call0 XT_RTOS_CP_STATE /* a15 = new owner's save area */ - #ifndef CONFIG_FREERTOS_FPU_IN_ISR - beqz a15, .L_goto_invalid + #if CONFIG_FREERTOS_FPU_IN_ISR + beqz a15, .L_skip_core_pin /* CP used in ISR, skip task pinning */ + #else + beqz a15, .L_goto_invalid /* not in a thread (invalid) */ #endif - /*When FPU in ISR is enabled we could deal with zeroed a15 */ +#if configNUM_CORES > 1 + /* CP operations are incompatible with unpinned tasks. Thus we pin the task + to the current running core. */ + movi a2, pxCurrentTCB + getcoreid a3 /* a3 = current core ID */ + addx4 a2, a3, a2 + l32i a2, a2, 0 /* a2 = start of pxCurrentTCB[cpuid] */ + addi a2, a2, TASKTCB_XCOREID_OFFSET /* a2 = &TCB.xCoreID */ + s32i a3, a2, 0 /* TCB.xCoreID = current core ID */ +#endif // configNUM_CORES > 1 + +#if CONFIG_FREERTOS_FPU_IN_ISR +.L_skip_core_pin: +#endif /* Enable the co-processor's bit in CPENABLE. */ movi a0, _xt_coproc_mask rsr a4, CPENABLE /* a4 = CPENABLE */ addx4 a0, a5, a0 /* a0 = &_xt_coproc_mask[n] */ l32i a0, a0, 0 /* a0 = (n << 16) | (1 << n) */ - - /* FPU operations are incompatible with non-pinned tasks. If we have a FPU operation - here, to keep the entire thing from crashing, it's better to pin the task to whatever - core we're running on now. */ - movi a2, pxCurrentTCB - getcoreid a3 - addx4 a2, a3, a2 - l32i a2, a2, 0 /* a2 = start of pxCurrentTCB[cpuid] */ - addi a2, a2, TASKTCB_XCOREID_OFFSET /* offset to xCoreID in tcb struct */ - s32i a3, a2, 0 /* store current cpuid */ - - /* Grab correct xt_coproc_owner_sa for this core */ - movi a2, XCHAL_CP_MAX << 2 - mull a2, a2, a3 /* multiply by current processor id */ - movi a3, _xt_coproc_owner_sa /* a3 = base of owner array */ - add a3, a3, a2 /* a3 = owner area needed for this processor */ - extui a2, a0, 0, 16 /* coprocessor bitmask portion */ or a4, a4, a2 /* a4 = CPENABLE | (1 << n) */ wsr a4, CPENABLE @@ -941,6 +939,11 @@ everywhere): _xt_coproc_release assumes it works like this in order not to need locking. */ + /* Grab correct xt_coproc_owner_sa for this core */ + movi a2, XCHAL_CP_MAX << 2 + mull a2, a2, a3 /* multiply by current processor id */ + movi a3, _xt_coproc_owner_sa /* a3 = base of owner array */ + add a3, a3, a2 /* a3 = owner area needed for this processor */ /* Get old coprocessor owner thread (save area ptr) and assign new one. */ addx4 a3, a5, a3 /* a3 = &_xt_coproc_owner_sa[n] */ From d61715ab9aa2e093406b47fac12ab62483575613 Mon Sep 17 00:00:00 2001 From: Darian Leung Date: Mon, 17 Oct 2022 23:30:32 +0800 Subject: [PATCH 2/2] freertos: Backport FPU tests from master to v4.4 This commit combines and backports the FPU unit tests updates of the following commits: 423fb361e793c8ce187358e953eb2ff87d6cd3a9 d69361779ecb24e1d5cb8c33b8d77d87f182ca34 --- components/freertos/test/test_float_in_isr.c | 65 -------- components/freertos/test/test_fpu_in_isr.c | 146 +++++++++++++++++ components/freertos/test/test_fpu_in_task.c | 159 +++++++++++++++++++ 3 files changed, 305 insertions(+), 65 deletions(-) delete mode 100644 components/freertos/test/test_float_in_isr.c create mode 100644 components/freertos/test/test_fpu_in_isr.c create mode 100644 components/freertos/test/test_fpu_in_task.c diff --git a/components/freertos/test/test_float_in_isr.c b/components/freertos/test/test_float_in_isr.c deleted file mode 100644 index 3c2285dcf3..0000000000 --- a/components/freertos/test/test_float_in_isr.c +++ /dev/null @@ -1,65 +0,0 @@ -#include -#include - -#include "freertos/FreeRTOS.h" -#include "freertos/task.h" -#include "freertos/semphr.h" -#include "freertos/queue.h" -#include "esp_intr_alloc.h" -#include "unity.h" -#include "soc/cpu.h" -#include "test_utils.h" -#include "math.h" - -#define SW_ISR_LEVEL_1 7 -#ifdef CONFIG_FREERTOS_FPU_IN_ISR - -struct fp_test_context { - SemaphoreHandle_t sync; - float expected; -}; - -static void software_isr(void *arg) { - (void)arg; - BaseType_t yield; - xt_set_intclear(1 << SW_ISR_LEVEL_1); - - struct fp_test_context *ctx = (struct fp_test_context *)arg; - - for(int i = 0; i < 16; i++) { - ctx->expected = ctx->expected * 2.0f * cosf(0.0f); - } - - xSemaphoreGiveFromISR(ctx->sync, &yield); - if(yield) { - portYIELD_FROM_ISR(); - } -} - - -TEST_CASE("Floating point usage in ISR test", "[freertos]" "[fp]") -{ - struct fp_test_context ctx; - float fp_math_operation_result = 0.0f; - - intr_handle_t handle; - esp_err_t err = esp_intr_alloc(ETS_INTERNAL_SW0_INTR_SOURCE, ESP_INTR_FLAG_LEVEL1, &software_isr, &ctx, &handle); - TEST_ASSERT_EQUAL_HEX32(ESP_OK, err); - - ctx.sync = xSemaphoreCreateBinary(); - TEST_ASSERT(ctx.sync != NULL); - ctx.expected = 1.0f; - - fp_math_operation_result = cosf(0.0f); - - xt_set_intset(1 << SW_ISR_LEVEL_1); - xSemaphoreTake(ctx.sync, portMAX_DELAY); - - esp_intr_free(handle); - vSemaphoreDelete(ctx.sync); - - printf("FP math isr result: %f \n", ctx.expected); - TEST_ASSERT_FLOAT_WITHIN(0.1f, ctx.expected, fp_math_operation_result * 65536.0f); -} - -#endif diff --git a/components/freertos/test/test_fpu_in_isr.c b/components/freertos/test/test_fpu_in_isr.c new file mode 100644 index 0000000000..43bef7a374 --- /dev/null +++ b/components/freertos/test/test_fpu_in_isr.c @@ -0,0 +1,146 @@ +/* + * SPDX-FileCopyrightText: 2022 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 "freertos/semphr.h" +#include "unity.h" +#include "test_utils.h" + +#if SOC_CPU_HAS_FPU && CONFIG_FREERTOS_FPU_IN_ISR + +// We can use xtensa API here as currently, non of the RISC-V targets have an FPU +#include "xtensa/xtensa_api.h" +#include "esp_intr_alloc.h" + +#define SW_ISR_LEVEL_1 7 + +static void fpu_isr(void *arg) +{ + // Clear the interrupt + xt_set_intclear(1 << SW_ISR_LEVEL_1); + /* + Use the FPU + - We test using a calculation that will cause a change in mantissa and exponent for extra thoroughness + - cosf(0.0f) should return 1.0f, thus we are simply doubling test_float every iteration. + - Therefore, we should end up with (0.01) * (2^8) = 2.56 at the end of the loop + */ + volatile float test_float = 0.01f; + for (int i = 0; i < 8; i++) { + test_float = test_float * 2.0f * cosf(0.0f); + } + // We allow a 0.1% delta on the final result in case of any loss of precision from floating point calculations + TEST_ASSERT_FLOAT_WITHIN(0.00256f, 2.56f, test_float); +} + +/* ------------------------------------------------------------------------------------------------------------------ */ + +/* +Test FPU usage from a level 1 ISR + +Purpose: + - Test that the FPU can be used from a level 1 ISR + - Test that the ISR using the FPU does not corrupt the interrupted task's FPU context +Procedure: + - Allocate a level 1 ISR + - Task uses the FPU then triggers the ISR + - ISR uses the FPU as well (forcing the task's FPU context to be saved) + - Task continues using the FPU (forcing its FPU context to be restored) +Expected: + - ISR should use the FPU without issue + - The interrupted task can continue using the FPU without issue +*/ + +TEST_CASE("FPU: Usage in level 1 ISR", "[freertos]") +{ + intr_handle_t isr_handle; + TEST_ASSERT_EQUAL(ESP_OK, esp_intr_alloc(ETS_INTERNAL_SW0_INTR_SOURCE, ESP_INTR_FLAG_LEVEL1, &fpu_isr, NULL, &isr_handle)); + /* + Use the FPU (calculate a different value than in the ISR) + - We test using a calculation that will cause a change in mantissa and exponent for extra thoroughness + - cosf(0.0f) should return 1.0f, thus we are simply dividing test_float every iteration. + */ + // We should end up with (2.56) / (2^4) = 0.16 at the end of the first loop + volatile float test_float = 2.56f; + for (int i = 0; i < 4; i++) { + test_float = test_float / (2.0f * cosf(0.0f)); + } + // We allow a 0.1% delta on the final result in case of any loss of precision from floating point calculations + TEST_ASSERT_FLOAT_WITHIN(0.00016f, 0.16f, test_float); + + // Trigger the ISR + xt_set_intset(1 << SW_ISR_LEVEL_1); + + // Continue using the FPU from a task context after the interrupt returns + // We should end up with (0.16) / (2^4) = 0.01 at the end of the first loop + for (int i = 0; i < 4; i++) { + test_float = test_float / (2.0f * cosf(0.0f)); + } + // We allow a 0.1% delta on the final result in case of any loss of precision from floating point calculations + TEST_ASSERT_FLOAT_WITHIN(0.00001f, 0.01f, test_float); + + // Free the ISR + esp_intr_free(isr_handle); +} + +/* ------------------------------------------------------------------------------------------------------------------ */ + +/* +Test FPU usage in ISR does not affect an unpinned tasks + +Purpose: + - Test that the ISR using the FPU will not affect the interrupted task's affinity +Procedure: + - Create an unpinned task + - Unpinned task disables scheduling/preemption to ensure that it does not switch cores + - Unpinned task allocates an ISR then triggers the ISR + - The ISR interrupts the unpinned task then uses the FPU + - Task reenables scheduling/preemption and cleans up +Expected: + - The ISR using the FPU will not affect the unpinned task's affinity +*/ + +static void unpinned_task(void *arg) +{ + // Disable scheduling to make sure the current task doesn't switch cores + vTaskSuspendAll(); + + // Check that the task is unpinned + TEST_ASSERT_EQUAL(tskNO_AFFINITY, xTaskGetAffinity(NULL)); + + + // Allocate an ISR to use the FPU + intr_handle_t isr_handle; + TEST_ASSERT_EQUAL(ESP_OK, esp_intr_alloc(ETS_INTERNAL_SW0_INTR_SOURCE, ESP_INTR_FLAG_LEVEL1, &fpu_isr, NULL, &isr_handle)); + // Trigger the ISR + xt_set_intset(1 << SW_ISR_LEVEL_1); + // Free the ISR + esp_intr_free(isr_handle); + + // Task should remain unpinned after the ISR uses the FPU + TEST_ASSERT_EQUAL(tskNO_AFFINITY, xTaskGetAffinity(NULL)); + + // Reenable scheduling + xTaskResumeAll(); + + // Indicate done and self delete + xTaskNotifyGive((TaskHandle_t)arg); + vTaskDelete(NULL); +} + +TEST_CASE("FPU: Level 1 ISR does not affect unpinned task", "[freertos]") +{ + TaskHandle_t unity_task_handle = xTaskGetCurrentTaskHandle(); + xTaskCreate(unpinned_task, "unpin", 2048, (void *)unity_task_handle, UNITY_FREERTOS_PRIORITY + 1, NULL); + // Wait for task to complete + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + vTaskDelay(10); // Short delay to allow task memory to be freed +} + +#endif // SOC_CPU_HAS_FPU && CONFIG_FREERTOS_FPU_IN_ISR diff --git a/components/freertos/test/test_fpu_in_task.c b/components/freertos/test/test_fpu_in_task.c new file mode 100644 index 0000000000..cffda2718c --- /dev/null +++ b/components/freertos/test/test_fpu_in_task.c @@ -0,0 +1,159 @@ +/* + * SPDX-FileCopyrightText: 2022 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 "freertos/semphr.h" +#include "unity.h" +#include "test_utils.h" + +#if SOC_CPU_HAS_FPU + +/* ------------------------------------------------------------------------------------------------------------------ */ + +/* +Test FPU usage from a task context + +Purpose: + - Test that the FPU can be used from a task context + - Test that FPU context is properly saved and restored +Procedure: + - Create TEST_PINNED_NUM_TASKS tasks pinned to each core + - Start each task + - Each task updates a float variable and then blocks (to allow other tasks to run thus forcing the an FPU context + save and restore). +Expected: + - Correct float value calculated by each task +*/ + +#define TEST_PINNED_NUM_TASKS 3 + +static void pinned_task(void *arg) +{ + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + + /* + Use the FPU + - We test using a calculation that will cause a change in mantissa and exponent for extra thoroughness + - cosf(0.0f) should return 1.0f, thus we are simply doubling test_float every iteration. + - Therefore, we should end up with (0.01) * (2^8) = 2.56 at the end of the loop + */ + volatile float test_float = 0.01f; + for (int i = 0; i < 8; i++) { + test_float = test_float * 2.0f * cosf(0.0f); + vTaskDelay(1); // Block to cause a context switch, forcing the FPU context to be saved + } + // We allow a 0.1% delta on the final result in case of any loss of precision from floating point calculations + TEST_ASSERT_FLOAT_WITHIN(0.00256f, 2.56f, test_float); + + // Indicate done wand wait to be deleted + xSemaphoreGive((SemaphoreHandle_t)arg); + vTaskSuspend(NULL); +} + +TEST_CASE("FPU: Usage in task", "[freertos]") +{ + SemaphoreHandle_t done_sem = xSemaphoreCreateCounting(configNUM_CORES * TEST_PINNED_NUM_TASKS, 0); + TEST_ASSERT_NOT_EQUAL(NULL, done_sem); + + TaskHandle_t task_handles[configNUM_CORES][TEST_PINNED_NUM_TASKS]; + + // Create test tasks for each core + for (int i = 0; i < configNUM_CORES; i++) { + for (int j = 0; j < TEST_PINNED_NUM_TASKS; j++) { + TEST_ASSERT_EQUAL(pdTRUE, xTaskCreatePinnedToCore(pinned_task, "task", 4096, (void *)done_sem, UNITY_FREERTOS_PRIORITY + 1, &task_handles[i][j], i)); + } + } + + // Start the created tasks simultaneously + for (int i = 0; i < configNUM_CORES; i++) { + for (int j = 0; j < TEST_PINNED_NUM_TASKS; j++) { + xTaskNotifyGive(task_handles[i][j]); + } + } + + // Wait for the tasks to complete + for (int i = 0; i < configNUM_CORES * TEST_PINNED_NUM_TASKS; i++) { + xSemaphoreTake(done_sem, portMAX_DELAY); + } + + // Delete the tasks + for (int i = 0; i < configNUM_CORES; i++) { + for (int j = 0; j < TEST_PINNED_NUM_TASKS; j++) { + vTaskDelete(task_handles[i][j]); + } + } + + vTaskDelay(10); // Short delay to allow idle task to be free task memory and FPU contexts + vSemaphoreDelete(done_sem); +} + +/* ------------------------------------------------------------------------------------------------------------------ */ + +/* +Test FPU usage will pin an unpinned task + +Purpose: + - Test that unpinned tasks are automatically pinned to the current core on the task's first use of the FPU +Procedure: + - Create an unpinned task + - Task disables scheduling/preemption to ensure that it does not switch cores + - Task uses the FPU + - Task checks its core affinity after FPU usage +Expected: + - Task remains unpinned until its first usage of the FPU + - The task becomes pinned to the current core after first use of the FPU +*/ + +#if configNUM_CORES > 1 + +static void unpinned_task(void *arg) +{ + // Disable scheduling to make sure current core ID doesn't change + vTaskSuspendAll(); + + BaseType_t cur_core_num = xPortGetCoreID(); + // Check that the task is unpinned + TEST_ASSERT_EQUAL(tskNO_AFFINITY, xTaskGetAffinity(NULL)); + + /* + Use the FPU + - We test using a calculation that will cause a change in mantissa and exponent for extra thoroughness + - cosf(0.0f) should return 1.0f, thus we are simply doubling test_float every iteration. + - Therefore, we should end up with (0.01) * (2^8) = 2.56 at the end of the loop + */ + volatile float test_float = 0.01f; + for (int i = 0; i < 8; i++) { + test_float = test_float * 2.0f * cosf(0.0f); + } + // We allow a 0.1% delta on the final result in case of any loss of precision from floating point calculations + TEST_ASSERT_FLOAT_WITHIN(0.00256f, 2.56f, test_float); + + TEST_ASSERT_EQUAL(cur_core_num, xTaskGetAffinity(NULL)); + + // Reenable scheduling + xTaskResumeAll(); + + // Indicate done and self delete + xTaskNotifyGive((TaskHandle_t)arg); + vTaskDelete(NULL); +} + +TEST_CASE("FPU: Usage in unpinned task", "[freertos]") +{ + TaskHandle_t unity_task_handle = xTaskGetCurrentTaskHandle(); + // Create unpinned task + xTaskCreate(unpinned_task, "unpin", 4096, (void *)unity_task_handle, UNITY_FREERTOS_PRIORITY + 1, NULL); + // Wait for task to complete + ulTaskNotifyTake(pdTRUE, portMAX_DELAY); + vTaskDelay(10); // Short delay to allow task memory to be freed +} + +#endif // configNUM_CORES > 1 +#endif // SOC_CPU_HAS_FPU