riscv-rt: Support for vectored mode interrupt handling

[romancardenas]

I am working on providing support for interrupt dispatching in vectored mode. This is a first draft, and I could not test yet if it works. However, I wanted to share it with you so I can start getting some feedback and suggestions about different design decisions. So, the main changes are:

• New v-trap feature to enable vectored mode. If this feature is enabled, the assembly code will add a trap vector with j instructions to a different trap per interrupt source. Some targets do not implement the standard interrupt convention (e.g., ESP32C3). That is why I left the table as weak. PACs will be able to replace this table accordingly.
• New interrupt procedural macro to implement interrupt handlers. Currently, in direct mode, this macro is quite dummy. However, I plan to add some parameters to the macro that will, eventually, allow us to "check" at compile time that the interrupt source name is an interrupt source of the target. Additionally, if v-trap feature is enabled, this macro will generate the trap handler to make sure that user registers are properly restored after executing the interrupt handler.

Also, I've been careful regarding how traps are generated to ease shortly the compatibility with the E extension (see #178).

This PR is still WIP, and I appreciate the feedback. Especially from the esp-rs folks (@MabezDev , @jessebraham) and @hegza , who already implemented a solution for this.

Closes #158

[hegza]

For sure I can at least test this on our HW & sims, compare it with mine and see how it fits into the current workflow. I'll be onto this.

[romancardenas]

I successfully ran an example on QEMU using MachineTimer and MachineSoft interrupts in vectored mode.

For each interrupt source, an independent trap handler is added in the final binary:

20010150 <_start_MachineSoft_trap>:
20010150:      	addi	sp, sp, -64                           // allocate space for trap frame
20010152:      	sw	a0, 32(sp)                            // store trap partially (only register a0)
20010154:      	auipc	a0, 1
20010158:      	addi	a0, a0, -476                          // load MachineSoft address into a0
2001015c:      	j	0x20010210 <_continue_interrupt_trap> // jump to common interrupt handler

20010160 <_start_MachineTimer_trap>:
20010160:      	addi	sp, sp, -64                           // allocate space for trap frame
20010162:      	sw	a0, 32(sp)                            // store trap partially (only register a0)
20010164:      	auipc	a0, 1
20010168:      	addi	a0, a0, 1250                          // load MachineTimer address into a0
2001016c:      	j	0x20010210 <_continue_interrupt_trap> // jump to common interrupt handler

Additionally, a common interrupt trap does the rest of the work and jumps to the corresponding handler:

20010210 <_continue_interrupt_trap>:
// store all the registers of the trap excluding a0 (which points to interrupt handler)
20010210:      	sw	ra, 0(sp)
20010214:      	sw	t0, 4(sp)
20010218:      	sw	t1, 8(sp)
2001021c:      	sw	t2, 12(sp)
20010220:      	sw	t3, 16(sp)
20010224:      	sw	t4, 20(sp)
20010228:      	sw	t5, 24(sp)
2001022c:      	sw	t6, 28(sp)
20010230:      	sw	a1, 36(sp)  // skip a0, already stored
20010234:      	sw	a2, 40(sp)
20010238:      	sw	a3, 44(sp)
2001023c:      	sw	a4, 48(sp)
20010240:      	sw	a5, 52(sp)
20010244:      	sw	a6, 56(sp)
20010248:      	sw	a7, 60(sp)
2001024c:      	jalr	a0           // jump to interrupt handler in a0
// load registers from trap
20010250:      	lw	ra, 0(sp)
20010254:      	lw	t0, 4(sp)
20010258:      	lw	t1, 8(sp)
2001025c:      	lw	t2, 12(sp)
20010260:      	lw	t3, 16(sp)
20010264:      	lw	t4, 20(sp)
20010268:      	lw	t5, 24(sp)
2001026c:      	lw	t6, 28(sp)
20010270:      	lw	a0, 32(sp) // we now include a0
20010274:      	lw	a1, 36(sp)
20010278:      	lw	a2, 40(sp)
2001027c:      	lw	a3, 44(sp)
20010280:      	lw	a4, 48(sp)
20010284:      	lw	a5, 52(sp)
20010288:      	lw	a6, 56(sp)
2001028c:      	lw	a7, 60(sp)
20010290:      	addi	sp, sp, 64 // free stack and return from interrupt
20010294:      	mret

We also preserve the original _start_trap, as it is the default implementation for all those interrupts that are not defined using the macro. Also, it is required for dealing with exceptions:

20010200 <_start_trap>:
20010200:      	addi	sp, sp, -64
20010204:      	sw	ra, 0(sp)
20010208:      	sw	t0, 4(sp)
2001020c:      	sw	t1, 8(sp)
20010210:      	sw	t2, 12(sp)
20010214:      	sw	t3, 16(sp)
20010218:      	sw	t4, 20(sp)
2001021c:      	sw	t5, 24(sp)
20010220:      	sw	t6, 28(sp)
20010224:      	sw	a0, 32(sp)
20010228:      	sw	a1, 36(sp)
2001022c:      	sw	a2, 40(sp)
20010230:      	sw	a3, 44(sp)
20010234:      	sw	a4, 48(sp)
20010238:      	sw	a5, 52(sp)
2001023c:      	sw	a6, 56(sp)
20010240:      	sw	a7, 60(sp)
20010244:      	add	a0, sp, zero // store pointer to trap in a0
20010248:      	jal	0x2001042a <_start_trap_rust>
2001024c:      	lw	ra, 0(sp)
20010250:      	lw	t0, 4(sp)
20010254:      	lw	t1, 8(sp)
20010258:      	lw	t2, 12(sp)
2001025c:      	lw	t3, 16(sp)
20010260:      	lw	t4, 20(sp)
20010264:      	lw	t5, 24(sp)
20010268:      	lw	t6, 28(sp)
2001026c:      	lw	a0, 32(sp)
20010270:      	lw	a1, 36(sp)
20010274:      	lw	a2, 40(sp)
20010278:      	lw	a3, 44(sp)
2001027c:      	lw	a4, 48(sp)
20010280:      	lw	a5, 52(sp)
20010284:      	lw	a6, 56(sp)
20010288:      	lw	a7, 60(sp)
2001028c:      	addi	sp, sp, 64
20010290:      	mret

Next, the vector table is filled as expected:

20010300 <_vector_table>:
20010300:      	j	0x20010200 <_start_trap>
20010304:      	j	0x20010200 <_start_trap>
20010308:      	j	0x20010200 <_start_trap>
2001030c:      	j	0x20010150 <_start_MachineSoft_trap>
20010310:      	j	0x20010200 <_start_trap>
20010314:      	j	0x20010200 <_start_trap>
20010318:      	j	0x20010200 <_start_trap>
2001031c:      	j	0x200101a0 <_start_MachineTimer_trap>
20010320:      	j	0x20010200 <_start_trap>
20010324:      	j	0x20010200 <_start_trap>
20010328:      	j	0x20010200 <_start_trap>
2001032c:      	j	0x20010200 <_start_trap>

I used GDB to monitor which trap handler is used and works as expected. The program does not use the _start_trap handler at all :D

[hegza]

In my understanding, we only need _continue_interrupt_trap to be a valid target for j instruction. j has LSB set to 0 so we'd need to make sure this is align(2) even if there's multiple items placed in .trap section. I can't see why it would ever need to be 4 or 8 byte aligned. I could be wrong.

[hegza]

The proc-macro trap handler is somewhat more intimidating than the previous trap_handler macro. I guess it becomes worth it as the vectored handler needs a more customizable code flow anyway.

Looks OK other than that. I'll try out the user experience on a sim.

[hegza]

Works as expected on Renode simulated generic 64-bit RISC-V using a CLINT for interrupt dispatch. I tested MachineSoft & MachineTimer as well. GDB shows the same vectoring behavior. Renode VP example for reference: soc-hub-fi/headsail-vp@main...tmp/vectored-rt

I wonder how it meshes with the nested interrupts we use on the FPGA implementation of the real-time RVE. I'll try adapting that runtime to this one and see if it raises any issue.

[romancardenas]

I tested this with my SLIC crate, which exploits nested MachineSoft interrupts to have an interrupt-driven program (e.g., RTIC). So hopefully you will face no issues.

[romancardenas]

TO DO LIST

• I would love to skip _start_trap at all and leave it only for exceptions. We could use the DefaultHandler to do this:

  riscv/riscv-rt/src/lib.rs

  Line 478 in aca6410

  fn DefaultHandler();

  )
• use cargo flags to simplify _start_rust_trap if vectored mode is enabled? maybe the array of __INTERRUPTS is not necessary in this mode, as only exceptions are expected to fall here.

Next, another challenge is supporting heterogeneous interrupt sources (e.g., target-specific additional interrupt sources or non-standard interrupt sources. In order to achieve this, I would do as follows:

• Modify _start_rust_trap. When an interrupt is detected, it relies on _handle_interrupts function, which, by default, behaves as this part.
• Provide a new macro to 1) define core interrupt enums 2) define the appropriate _vector_table

Essentially, with these changes, we would start moving towards supporting target-specific interrupt sources (see #146)

[romancardenas]

New changes to riscv-rt:

• Now, when an interrupt is detected, _start_trap_rust calls _dispatch_interrupt(interrupt_number). This does not really change anything from previous versions, but we are now closer to accepting custom interrupt sources (I will leave this to a future PR, it needs additional changes).
• When v-trap feature is enabled, the binary will contain a _vector_table with j instructions to trap handlers. The first j instruction points to _start_trap, as it is reserved to exceptions. The rest points to _start_INTERRUPT_trap, where INTERRUPT is the ID of the corresponding interrupt name (e.g., MachineSoft). By default, all _start_INTERRUPT_trap are weak aliases of _start_DefaultHandler_trap.
• Interrupt handlers are divided into two stages:
  • _start_INTERRUPT_trap: saves space in stack, stores a0 register, loads the interrupt handler in a0, and jumps to _continue_interrupt_trap.
  • _continue_interrupt_trap: saves the rest of the registers, calls the interrupt handler stored in a0, restores all registers, and returns.
• To help users to define interrupt traps together with their interrupt handler, the new riscv_rt::interrupt macro also generates the _start_INTERRUPT_trap routine.

[romancardenas]

The PR is good to go. @rust-embedded/riscv please take a look.

[rmsyn]

Did an initial review, just some minor style nits.

I'll do a more in-depth review, and test against hardware soon.

[rmsyn]

This looks correct to me, from the RISC-V Privileged Spec entry for mtvec:

The value in the BASE field must always be aligned on a 4-byte boundary,
and the MODE setting may impose additional alignment constraints on
the value in the BASE field.

MODE field set to 1 for vectored mode:

Asynchronous interrupts set pc to BASE+4×cause.

[hegza]

The CLIC specification may require _vector_table to be 64-byte aligned so this will have to be overridden but I think there's not much we can or should do in riscv-rt about this. CLIC based implementations might want to define a separate vector for CLIC interrupts anyway; I don't know for sure, there's not many implementations like this in the wild 🙂

[romancardenas]

Also, if I remember well, CLIC interrupt table is not comprised of j instructions, but the address of the trap itself. So, once we start integrating vectored CLIC mode, we might use a different methodology.

[hegza]

This is correct. CLIC uses a vector table of addresses instead of jumps (at least our implementation 🙂).

[MabezDev]

Sorry for not taking a look at this sooner. I will hopefully have some time to review at the weekend :)

[rmsyn]

Changes LGTM. I'll work on testing these changes on hardware soon.

[MabezDev]

This LGTM, I hope in the some day we can ditch esp-riscv-rt and go back to using riscv-rt and friends :)

[MabezDev]

One interesting data point, which might not be required here, is that in esp-riscv-rt, with high opt level + lto = 'fat' we found instances where weak assembly symbols were duplicated by the linker. To fix this we had to sprinkle some weird llvm specific attributes before the weak definitions:

https://github.com/esp-rs/esp-hal/blob/3c057595561a7a5dba68ae4df09c9960217c33fc/esp-riscv-rt/src/lib.rs#L830-L835

probably not required here, but if you run into issue you'll know what to do :D.

[romancardenas]

Thank you! This is my goal. If riscv-rt is also suitable for ESP32 Cx, then it is flexible enough for any RISC-V target. The issue you point out is interesting, we will monitor it in case it appears in future versions