Fix RISC-V direct vectoring and re-enable interrupts HIL test (#4171)

* Fix direct vectoring

* WIP global_asm

* resolve reviews

ugh

* Document `enable_direct` function more, remove misc conversion.

fix

* Make instructions stricter

Author: playfulFence

esp-hal/CHANGELOG.md

@@ -76,6 +76,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ESP32: Enable up to 4M of PSRAM (#3990)
 - I2C error recovery logic issues (#4000)
 - I2S: Fixed RX half-sample bits configuration bug causing microphone noise (#4109)
+- RISC-V: Direct interrupt vectoring (#4171)
 
 ### Removed
 

esp-hal/MIGRATING-1.0.0-rc.0.md

@@ -267,3 +267,18 @@ All RTC clock enums/structs have been moved from `rtc_cntl` to the `clock` modul
 Imports will need to be updated accordingly.
 
 Additionally, enum variant naming violations have been resolved, so the `RtcFastClock` and `RtcSlowClock` prefixes will need to be removed from any variants from these enums.
+
+## Direct vectoring changes
+
+`enable_direct` now requires user to pass handler function to it. 
+
+```diff
+interrupt::enable_direct(
+    Interrupt::FROM_CPU_INTR0,
+    Priority::Priority3,
+    CpuInterrupt::Interrupt20,
++   interrupt_handler,
+)
+.unwrap();
+
+```

esp-hal/src/interrupt/riscv.rs

@@ -210,14 +210,32 @@ pub static RESERVED_INTERRUPTS: &[u32] = PRIORITY_TO_INTERRUPT;
 
 /// Enable an interrupt by directly binding it to a available CPU interrupt
 ///
-/// Unless you are sure, you most likely want to use [`enable`] instead.
+/// ⚠️ This installs a *raw trap handler*, the `handler` user provides is written directly into the
+/// CPU interrupt vector table. That means:
+///
+/// - Provided handler will be used as an actual trap-handler
+/// - It is user's responsibility to:
+///   - Save and restore all registers they use.
+///   - Clear the interrupt source if necessary.
+///   - Return using the `mret` instruction.
+/// - The handler should be declared as naked function. The compiler will not insert a function
+///   prologue/epilogue for the user, normal Rust `fn` will result in an error.
+///
+/// Unless you are sure that you need such low-level control to achieve the lowest possible latency,
+/// you most likely want to use [`enable`] instead.
 ///
 /// Trying using a reserved interrupt from [`RESERVED_INTERRUPTS`] will return
 /// an error.
+///
+/// ## Example
+/// Visit the [interrupt] test to see a proper example of how to use direct vectoring.
+///
+/// [interrupt]: https://github.com/esp-rs/esp-hal/blob/main/hil-test/src/bin/interrupt.rs
 pub fn enable_direct(
     interrupt: Interrupt,
     level: Priority,
     cpu_interrupt: CpuInterrupt,
+    handler: unsafe extern "C" fn(),
 ) -> Result<(), Error> {
     if RESERVED_INTERRUPTS.contains(&(cpu_interrupt as _)) {
         return Err(Error::CpuInterruptReserved);
@@ -228,11 +246,53 @@ pub fn enable_direct(
     unsafe {
         map(Cpu::current(), interrupt, cpu_interrupt);
         set_priority(Cpu::current(), cpu_interrupt, level);
+
+        let mt = mtvec::read();
+
+        assert_eq!(
+            mt.trap_mode().into_usize(),
+            mtvec::TrapMode::Vectored.into_usize()
+        );
+
+        let base_addr = mt.address() as usize;
+
+        let int_slot = base_addr.wrapping_add((cpu_interrupt as usize) * 4);
+
+        let instr = encode_jal_x0(handler as usize, int_slot)?;
+
+        core::ptr::write_volatile(int_slot as *mut u32, instr);
+        core::arch::asm!("fence.i");
+
         enable_cpu_interrupt(cpu_interrupt);
     }
     Ok(())
 }
 
+// helper: returns correctly encoded RISC-V `jal` instruction
+fn encode_jal_x0(target: usize, pc: usize) -> Result<u32, Error> {
+    let offset = (target as isize) - (pc as isize);
+
+    const MIN: isize = -(1isize << 20);
+    const MAX: isize = (1isize << 20) - 1;
+
+    assert!(offset % 2 == 0 && (MIN..=MAX).contains(&offset));
+
+    let imm = offset as u32;
+    let imm20 = (imm >> 20) & 0x1;
+    let imm10_1 = (imm >> 1) & 0x3ff;
+    let imm11 = (imm >> 11) & 0x1;
+    let imm19_12 = (imm >> 12) & 0xff;
+
+    let instr = (imm20 << 31)
+        | (imm19_12 << 12)
+        | (imm11 << 20)
+        | (imm10_1 << 21)
+        // https://lhtin.github.io/01world/app/riscv-isa/?xlen=32&insn_name=jal
+        | 0b1101111u32;
+
+    Ok(instr)
+}
+
 /// Disable the given peripheral interrupt.
 pub fn disable(core: Cpu, interrupt: Interrupt) {
     map_raw(core, interrupt, DISABLED_CPU_INTERRUPT)

hil-test/src/bin/interrupt.rs

@@ -25,36 +25,45 @@ use hil_test as _;
 
 static SWINT0: Mutex<RefCell<Option<SoftwareInterrupt<0>>>> = Mutex::new(RefCell::new(None));
 
-#[allow(unused)] // TODO: Remove attribute when interrupt latency test re-enabled
+#[unsafe(no_mangle)]
+static mut LAST_PERF: u32 = 0;
+
+#[unsafe(no_mangle)]
+static mut SW_TRIGGER_ADDR: *mut u32 = core::ptr::null_mut();
+
 struct Context {
     sw0_trigger_addr: u32,
 }
 
+// We need to place this close to the trap handler for the jump to be resolved properly
+#[unsafe(link_section = ".trap.rust")]
 #[unsafe(no_mangle)]
-fn interrupt20() {
-    unsafe { asm!("csrrwi x0, 0x7e1, 0 #disable timer") }
-    critical_section::with(|cs| {
-        SWINT0.borrow_ref(cs).as_ref().unwrap().reset();
-    });
-
-    let mut perf_counter: u32 = 0;
-    unsafe {
-        asm!(
-            "
-            csrr {x}, 0x7e2
-            ",
-            options(nostack),
-            x = inout(reg) perf_counter,
-        )
-    };
-    defmt::info!("Performance counter:{}", perf_counter);
-    // TODO these values should be adjusted to catch smaller regressions
-    cfg_if::cfg_if! {
-        if #[cfg(any(feature = "esp32c3", feature = "esp32c2"))] {
-            assert!(perf_counter < 1100);
-        } else {
-            assert!(perf_counter < 750);
-        }
+#[unsafe(naked)]
+#[rustfmt::skip]
+unsafe extern "C" fn interrupt_handler() {
+    core::arch::naked_asm! {"
+        # save affected registers
+        addi sp, sp, -16*4 # allocate 16 words for saving regs (will work with just 2, but RISC-V wants it to be aligned by 16)
+        sw t0, 1*4(sp)
+        sw t1, 2*4(sp)
+
+        csrrwi x0, 0x7e1, 0 #disable timer
+
+        lw t0, {sw}  # t0 <- &SW_TRIGGER_ADDR
+        sw x0, 0(t0) # clear (deassert) *SW_TRIGGER_ADDR
+
+        csrr t1, 0x7e2 # read performance counter
+        la t0, {x}
+        sw t1, 0(t0) # store word to LAST_PERF
+
+        # restore affected registers
+        lw t0, 1*4(sp)
+        lw t1, 2*4(sp)
+        addi sp, sp, 16*4
+        mret
+        ",
+        x = sym LAST_PERF,
+        sw = sym SW_TRIGGER_ADDR,
     }
 }
 
@@ -77,47 +86,66 @@ mod tests {
         }
 
         let sw0_trigger_addr = cpu_intr.register_block().cpu_intr_from_cpu(0) as *const _ as u32;
+        unsafe {
+            SW_TRIGGER_ADDR = sw0_trigger_addr as *mut u32;
+        }
 
         critical_section::with(|cs| {
             SWINT0
                 .borrow_ref_mut(cs)
                 .replace(sw_ints.software_interrupt0)
         });
+
         interrupt::enable_direct(
             Interrupt::FROM_CPU_INTR0,
             Priority::Priority3,
             CpuInterrupt::Interrupt20,
+            interrupt_handler,
         )
         .unwrap();
 
         Context { sw0_trigger_addr }
     }
 
+    // Handler function (assigned above) reads cycles count from CSR and stores it into `LAST_PERF`
+    // static, which this test then reads and asserts its value to detect regressions
     #[test]
     #[rustfmt::skip]
-    fn interrupt_latency(_ctx: Context) {
-        // unsafe {
-        //     asm!(
-        //         "
-        // csrrwi x0, 0x7e0, 1 #what to count, for cycles write 1 for instructions write 2
-        // csrrwi x0, 0x7e1, 0 #disable counter
-        // csrrwi x0, 0x7e2, 0 #reset counter
-        // "
-        //     );
-        // }
-
-        // // interrupt is raised from assembly for max timer granularity.
-        // unsafe {
-        //     asm!(
-        //         "
-        // li {bit}, 1                   # Flip flag (bit 0)
-        // csrrwi x0, 0x7e1, 1           # enable timer
-        // sw {bit}, 0({addr})           # trigger FROM_CPU_INTR0
-        // ",
-        //     options(nostack),
-        //     addr = in(reg) ctx.sw0_trigger_addr,
-        //     bit = out(reg) _,
-        //     )
-        // }
+    fn interrupt_latency(ctx: Context) {
+        unsafe {
+            asm!(
+                "
+                    csrrwi x0, 0x7e0, 1 #what to count, for cycles write 1 for instructions write 2
+                    csrrwi x0, 0x7e1, 0 #disable counter
+                    csrrwi x0, 0x7e2, 0 #reset counter
+                "
+            );
+        }
+        
+        // interrupt is raised from assembly for max timer granularity.
+        unsafe {
+            asm!(
+                "
+                    li {bit}, 1                   # Flip flag (bit 0)
+                    csrrwi x0, 0x7e1, 1           # enable timer
+                    sw {bit}, 0({addr})           # trigger FROM_CPU_INTR0
+                ",
+                options(nostack),
+                addr = in(reg) ctx.sw0_trigger_addr,
+                bit = out(reg) _,
+            )
+        }
+
+        let perf_counter = unsafe { LAST_PERF };
+        defmt::info!("Performance counter: {}", perf_counter);
+
+        // TODO c3/c2 values should be adjusted to catch smaller regressions
+        cfg_if::cfg_if! {
+        if #[cfg(any(feature = "esp32c3", feature = "esp32c2"))] {
+            assert!(perf_counter < 400);
+        } else {
+            assert!(perf_counter < 155);
+        }
+    }
     }
 }

qa-test/Cargo.toml

@@ -21,6 +21,7 @@ esp-backtrace = { path = "../esp-backtrace", features = [
     "panic-handler",
     "println",
 ] }
+critical-section   = "1.1.3"
 esp-bootloader-esp-idf = { path = "../esp-bootloader-esp-idf" }
 esp-hal = { path = "../esp-hal", features = ["log-04", "unstable"] }
 esp-hal-embassy = { path = "../esp-hal-embassy" }