RISC-V - Interrupts not enabled in xPortStartFirstTask

Kernel
1

Hi,
I’m currently using the RISC-V port of FreeRTOS and I’ve run into an issue in the xPortStartFirstTask function of portASM.S.

Interrupts are supposed to be enabled by restoring the mstatus value saved in the task’s stack with:

load_x  t0, 29 * portWORD_SIZE( sp )	/* mstatus */
csrrw  x0, mstatus, t0					/* Interrupts enabled from here! */

However during the task initialization, the saved value is simply mstatus | 0x1880. But at the time, interrupts are disabled, so the value is always 0x1880 and not 0x1888 which would mean interrupts are enabled. So when it’s restored in xPortStartFirstTask, it means mstatus is just set to 0x1880. Since the function ends with a simple ret instruction, and not mret, mstatus remains at 0x1880 and interrupts remain disabled until the next time portENABLE_INTERRUPTS hopefully gets called.

I think the function should instead set mepc to the correct return value with:
csrw mepc, x1
and end with
mret
just like the function processed_source. This way, the MIE bit of mstatus gets set with the mret instruction.

It could even jump to processed_source like so:

#if( portasmHAS_CLINT != 0 )
	/* If there is a clint then interrupts can branch directly to the FreeRTOS
	trap handler.  Otherwise the interrupt controller will need to be configured
	outside of this file. */
	la t0, freertos_risc_v_trap_handler
	csrw mtvec, t0
#endif /* portasmHAS_CLILNT */
	j processed_source

What do you think?

1 Like
2

Sorry about this - I think it has been fixed already though - please have a look here to see if you agree: https://sourceforge.net/p/freertos/code/HEAD/tree/trunk/FreeRTOS/Source/portable/GCC/RISC-V/portASM.S#l312

3

You’re absolutely right, sorry about that!
Thank you.

4

By the way, just out of curiosity, is there a reason why popping mstatus from the stack happens before all the other registers (in processed_source and xPortStartFirstTask)? Why not do it in the order of their offset from the current sp (0 to 29 or 29 to 0)? As it is now, an interrupt could happen before the first task’s context is popped, which isn’t really a problem, but could seem counter intuitive, while in processed_source, the actual mstatus register isn’t even modified before the mret instruction, so it wouldn’t change the actual behavior.

5

The stack frame is set up at that point, so I think it is safe to do so - let me know if you disagree.

Enabling interrupts requires using a couple of registers, so it could not happen as the very last thing (after the registers it uses have been restored) without corrupting those registers. Arguably that doesn’t matter in xPortStartFirstTask() as the registers have not been used for anything at that point, but sometimes I fill the values that are popped into registers with known values so I can check they are popped into the correct registers.

I could move restoring the interrupt enable bit down so it happens at the last possible time, something like this:

  1. Restores all registers other than those required to re-enable interrupts.
  2. Re-enable interrupts.
  3. Restore the registers used in the line above (when re-enabling interrupts).
  4. Remove stack frame
6

I agree that it’s safe as is.

I think processed_source could stay as is, since the interrupts only become active after the mret instruction anyway. For consistency with the offsets, you could do as you suggested and move the mstatus restoring to the end of the function, like this:

	load_x  x29, 26 * portWORD_SIZE( sp )	/* t4 */
	load_x  x30, 27 * portWORD_SIZE( sp )	/* t5 */
	/* Use t6 to load mstatus with the interrupt enable bits used by the task. */
	load_x  t6, 29 * portWORD_SIZE( sp )
	csrw mstatus, t6		
	/* Restore t6 */			
	load_x  t6, 28 * portWORD_SIZE( sp )	/* t6 */
	addi sp, sp, portCONTEXT_SIZE
	mret

But in xPortStartFirstTask, you could just set the mie bit of mstatus. As it is now, it ends up restoring the mstatus value that was present when the task that ends up being the starting one was created, which could be surprising. But I don’t understand why MPP and MPIE are set in pxPortInitialiseStack since we’re not using mret at the end the function, so maybe I’m missing something.
If I’m not mistaken, you could just do this for xPortStartFirstTask:

	load_x  sp, pxCurrentTCB			/* Load pxCurrentTCB. */
	load_x  sp, 0( sp )				 	/* Read sp from first TCB member. */

	load_x  x1, 0( sp )

	portasmRESTORE_ADDITIONAL_REGISTERS	/* Defined in freertos_risc_v_chip_specific_extensions.h to restore any registers unique to the RISC-V implementation. */

	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 */
	load_x  x9, 6 * portWORD_SIZE( sp )		/* s1 */
	load_x  x10, 7 * portWORD_SIZE( sp )	/* a0 */
	load_x  x11, 8 * portWORD_SIZE( sp )	/* a1 */
	load_x  x12, 9 * portWORD_SIZE( sp )	/* a2 */
	load_x  x13, 10 * portWORD_SIZE( sp )	/* a3 */
	load_x  x14, 11 * portWORD_SIZE( sp )	/* a4 */
	load_x  x15, 12 * portWORD_SIZE( sp )	/* a5 */
	load_x  x16, 13 * portWORD_SIZE( sp )	/* a6 */
	load_x  x17, 14 * portWORD_SIZE( sp )	/* a7 */
	load_x  x18, 15 * portWORD_SIZE( sp )	/* s2 */
	load_x  x19, 16 * portWORD_SIZE( sp )	/* s3 */
	load_x  x20, 17 * portWORD_SIZE( sp )	/* s4 */
	load_x  x21, 18 * portWORD_SIZE( sp )	/* s5 */
	load_x  x22, 19 * portWORD_SIZE( sp )	/* s6 */
	load_x  x23, 20 * portWORD_SIZE( sp )	/* s7 */
	load_x  x24, 21 * portWORD_SIZE( sp )	/* s8 */
	load_x  x25, 22 * portWORD_SIZE( sp )	/* s9 */
	load_x  x26, 23 * portWORD_SIZE( sp )	/* s10 */
	load_x  x27, 24 * portWORD_SIZE( sp )	/* s11 */
	load_x  x28, 25 * portWORD_SIZE( sp )	/* t3 */
	load_x  x29, 26 * portWORD_SIZE( sp )	/* t4 */
	load_x  x30, 27 * portWORD_SIZE( sp )	/* t5 */
	load_x  x31, 28 * portWORD_SIZE( sp )	/* t6 */
	addi	sp, sp, portCONTEXT_SIZE
	csrrs  x0, mstatus, 0x8					/* Interrupts enabled from here! */
	ret

What do you think?

7

Only ready your post briefly - so forgive me if I missed something: The initial stack given to a task might be started by xPortStartFirstTask (if it is the first task to run) - in which case mret is not called at the end of the function - or by the trap handler (in every other case) where mret is called at the end of the function. The initial stack therefore has to work in both cases.

8

That explains why MPP and MPIE are set in pxPortInitialiseStack. Thank you, I hadn’t thought of that.
However, wouldn’t you agree that inside xPortStartFirstTask, only the MIE bit of mstatus needs to be set? At this point, I think there’s no need to restore the mstatus value currently in the task’s stack.