From adf6e9ee0c8f72cd4147c0cbcfd577b147643913 Mon Sep 17 00:00:00 2001 From: RichardBarry Date: Fri, 1 May 2020 13:43:51 -0700 Subject: [PATCH] fix: CLEAR MIE BIT IN INITIAL RISC-V MSTATUS VALUE The MIE bit in the RISC-V MSTATUS register is used to globally enable or disable interrupts. It is copied into the MPIE bit and cleared on entry to an interrupt, and then copied back from the MPIE bit on exit from an interrupt. When a task is created it is given an initial MSTATUS value that is derived from the current MSTATUS value with the MPIE bit force to 1, but the MIE bit is not forced into any state. This change forces the MIE bit to 0 (interrupts disabled). Why: If a task is created before the scheduler is started the MIE bit will happen to be 0 (interrupts disabled), which is fine. If a task is created after the scheduler has been started the MIE bit is set (interrupts enabled), causing interrupts to unintentionally become enabled inside the interrupt in which the task is first moved to the running state - effectively breaking a critical section which in turn could cause a crash if enabling interrupts causes interrupts to nest. It is only an issue when starting a newly created task that was created after the scheduler was started. Related Issues: https://forums.freertos.org/t/risc-v-port-pxportinitialisestack-issue-about-mstatus-value-onto-the-stack/9622 --- portable/GCC/RISC-V/portASM.S | 12 +++++++----- portable/IAR/RISC-V/portASM.s | 11 ++++++----- queue.c | 2 +- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/portable/GCC/RISC-V/portASM.S b/portable/GCC/RISC-V/portASM.S index b9c56a693..f758674b8 100644 --- a/portable/GCC/RISC-V/portASM.S +++ b/portable/GCC/RISC-V/portASM.S @@ -313,11 +313,6 @@ xPortStartFirstTask: portasmRESTORE_ADDITIONAL_REGISTERS /* Defined in freertos_risc_v_chip_specific_extensions.h to restore any registers unique to the RISC-V implementation. */ - load_x t0, 29 * portWORD_SIZE( sp ) /* mstatus */ - addi t0, t0, 0x08 /* Set MIE bit so the first task starts with interrupts enabled - required as returns with ret not eret. */ - csrrw x0, mstatus, t0 /* Interrupts enabled from here! */ - - load_x x5, 2 * portWORD_SIZE( sp ) /* t0 */ load_x x6, 3 * portWORD_SIZE( sp ) /* t1 */ load_x x7, 4 * portWORD_SIZE( sp ) /* t2 */ load_x x8, 5 * portWORD_SIZE( sp ) /* s0/fp */ @@ -344,6 +339,12 @@ xPortStartFirstTask: load_x x29, 26 * portWORD_SIZE( sp ) /* t4 */ load_x x30, 27 * portWORD_SIZE( sp ) /* t5 */ load_x x31, 28 * portWORD_SIZE( sp ) /* t6 */ + + load_x x5, 29 * portWORD_SIZE( sp ) /* Initial mstatus into x5 (t0) */ + addi x5, x5, 0x08 /* Set MIE bit so the first task starts with interrupts enabled - required as returns with ret not eret. */ + csrrw x0, mstatus, x5 /* Interrupts enabled from here! */ + load_x x5, 2 * portWORD_SIZE( sp ) /* Initial x5 (t0) value. */ + addi sp, sp, portCONTEXT_SIZE ret .endfunc @@ -416,6 +417,7 @@ xPortStartFirstTask: pxPortInitialiseStack: csrr t0, mstatus /* Obtain current mstatus value. */ + andi t0, t0, ~0x8 /* Ensure interrupts are disabled when the stack is restored within an ISR. Required when a task is created after the schedulre has been started, otherwise interrupts would be disabled anyway. */ addi t1, x0, 0x188 /* Generate the value 0x1880, which are the MPIE and MPP bits to set in mstatus. */ slli t1, t1, 4 or t0, t0, t1 /* Set MPIE and MPP bits in mstatus value. */ diff --git a/portable/IAR/RISC-V/portASM.s b/portable/IAR/RISC-V/portASM.s index a33009977..050e63bc6 100644 --- a/portable/IAR/RISC-V/portASM.s +++ b/portable/IAR/RISC-V/portASM.s @@ -320,11 +320,6 @@ xPortStartFirstTask: portasmRESTORE_ADDITIONAL_REGISTERS /* Defined in freertos_risc_v_chip_specific_extensions.h to restore any registers unique to the RISC-V implementation. */ - load_x t0, 29 * portWORD_SIZE( sp ) /* mstatus */ - addi t0, t0, 0x08 /* Set MIE bit so the first task starts with interrupts enabled - required as returns with ret not eret. */ - csrrw x0, CSR_MSTATUS, t0 /* Interrupts enabled from here! */ - - load_x x5, 2 * portWORD_SIZE( sp ) /* t0 */ load_x x6, 3 * portWORD_SIZE( sp ) /* t1 */ load_x x7, 4 * portWORD_SIZE( sp ) /* t2 */ load_x x8, 5 * portWORD_SIZE( sp ) /* s0/fp */ @@ -351,6 +346,11 @@ xPortStartFirstTask: load_x x29, 26 * portWORD_SIZE( sp ) /* t4 */ load_x x30, 27 * portWORD_SIZE( sp ) /* t5 */ load_x x31, 28 * portWORD_SIZE( sp ) /* t6 */ + + load_x x5, 29 * portWORD_SIZE( sp ) /* Initial mstatus into x5 (t0) */ + addi x5, x5, 0x08 /* Set MIE bit so the first task starts with interrupts enabled - required as returns with ret not eret. */ + csrrw x0, CSR_MSTATUS, x5 /* Interrupts enabled from here! */ + load_x x5, 2 * portWORD_SIZE( sp ) /* Initial x5 (t0) value. */ addi sp, sp, portCONTEXT_SIZE ret @@ -421,6 +421,7 @@ xPortStartFirstTask: pxPortInitialiseStack: csrr t0, CSR_MSTATUS /* Obtain current mstatus value. */ + andi t0, t0, ~0x8 /* Ensure interrupts are disabled when the stack is restored within an ISR. Required when a task is created after the schedulre has been started, otherwise interrupts would be disabled anyway. */ addi t1, x0, 0x188 /* Generate the value 0x1880, which are the MPIE and MPP bits to set in mstatus. */ slli t1, t1, 4 or t0, t0, t1 /* Set MPIE and MPP bits in mstatus value. */ diff --git a/queue.c b/queue.c index 14ad01ec9..8b46da06a 100644 --- a/queue.c +++ b/queue.c @@ -1082,7 +1082,7 @@ Queue_t * const pxQueue = xQueue; { mtCOVERAGE_TEST_MARKER(); } - + /* Not used in this path. */ ( void ) uxPreviousMessagesWaiting; }