Fix timer virtualization bugs

This commit resolves two issues with timer virtualization:
1. Fixed a timer delay that was too large, ensuring test success regardless of log level.
2. Corrected a bug where the MPIE value was being overwritten, which for some reason affected only Spike.
Additionally, now firmware cannot delegate MSI and MTI.

firmware/clint_interrupt/main.rs

@@ -8,9 +8,14 @@ use miralis_abi::{failure, setup_binary, success};
 
 setup_binary!(main);
 
+/// This test verifies the general functionality of MTI (Machine Timer Interrupt) virtualization, including:
+/// 1. The timer interrupt is delivered within a reasonable time when the timer is set and interrupts are enabled.
+/// 2. Firmware cannot directly clear the M-mode timer interrupt (MTI) by writing to `vMIP.MTIP`.
+/// 3. Writing to the appropriate memory-mapped register correctly clears the interrupt.
+/// 4. If the virtual interrupt is not cleared, the firmware will trap on each execution cycle.
 fn main() -> ! {
     // Setup a timer deadline
-    set_mtimecmp_future_value(10000);
+    set_mtimecmp_future_value(0);
 
     // Configure trap handler and enable interrupts
     unsafe {
@@ -25,8 +30,7 @@ fn main() -> ! {
     for _ in 1..REPETITIONS {
         unsafe {
             asm!(
-                "csrs mstatus, {mstatus_mie}",  // Enable interrupts (MIE)
-                "nop",
+                "csrs mstatus, {mstatus_mie}",  // Enable global interrupts (MIE), expect to trap immediately
                 mstatus_mie = in(reg) 0x8,
             )
         };
@@ -71,14 +75,12 @@ extern "C" fn trap_handler() {
 
     unsafe {
         asm!(
-            "csrc mstatus, {mstatus_mie}", // Disable interrupts (MIE)
             "csrr {0}, mip",
             "csrr {1}, mcause",
             "csrr {2}, mepc",
             out(reg) mip,
             out(reg) mcause,
             out(reg) mepc,
-            mstatus_mie = in(reg) 0x8,
         );
     }
 

firmware/clint_interrupt_multihart/main.rs

@@ -8,6 +8,16 @@ use miralis_abi::{failure, setup_binary, success};
 
 setup_binary!(main);
 
+/// This test verifies the functionality of virtualized Machine Software Interrupt (MSI) between two harts.
+/// It ensures that one hart can trigger an interrupt on another hart by writing to the target hart's
+/// memory-mapped interrupt register.
+///
+/// Specifically, the test checks:
+/// 1. Hart A can successfully write to Hart B's MSI memory-mapped register.
+/// 2. Hart B correctly receives and handles the MSI triggered by Hart A's write.
+///
+/// This test ensures proper inter-hart communication through software interrupts, confirming that
+/// MSIs can be triggered and handled correctly in a multi-hart environment.
 fn main() -> ! {
     let hart_id: usize;
     unsafe {
@@ -64,10 +74,8 @@ extern "C" fn trap_handler() {
     let mcause: usize;
     unsafe {
         asm!(
-            "csrc mstatus, {mstatus_mie}", // Disable interrupts
             "csrr {0}, mcause",
             out(reg) mcause,
-            mstatus_mie = in(reg) 0x8,
         );
     }
 

firmware/clint_interrupt_priority/main.rs

@@ -8,6 +8,10 @@ use miralis_abi::{failure, setup_binary, success};
 
 setup_binary!(main);
 
+/// This test verifies that the priority of clearing virtualized Machine Software Interrupt (MSI)
+/// and Machine Timer Interrupt (MTI) is correct, with MSI taking precedence over MTI.
+/// It also ensures that both interrupts can be properly cleared by writing to their respective
+/// memory-mapped registers.
 fn main() -> ! {
     // Set up two interrupts simultaniously
     set_msip(0, 1);
@@ -21,26 +25,26 @@ fn main() -> ! {
     // Configure trap handler and enable interrupts
     unsafe {
         asm!(
-            "csrw mtvec, {handler}",       // Setup trap handler
-            "csrc mstatus, {mstatus_mie}", // Enable interrupts
-            "csrs mie, {mtie}",            // Enable MTIE and MSIE
+            "csrw mtvec, {handler}",        // Setup trap handler
+            "csrs mstatus, {mstatus_mie}",  // Enable interrupts
+            "csrs mie, {mie}",              // Enable MTIE and MSIE
             handler = in(reg) _raw_interrupt_trap_handler as usize,
-            mtie = in(reg) 0x88,
+            mie = in(reg) 0x88,
             mstatus_mie = in(reg) 0x8,
         );
     }
 
     // Expect to trap on both interrupts one by one
-    for _ in 1..2 {
+    for _ in 1..10000 {
         unsafe {
-            asm!("wfi");
+            asm!("nop");
         }
     }
 
     let mip: usize;
     unsafe {
         asm!(
-            "csrr {mip}, mip",            // Enable MTIE and MSIE
+            "csrr {mip}, mip",
             mip = out(reg) mip,
         );
     }
@@ -93,12 +97,10 @@ extern "C" fn trap_handler() {
     let mcause: usize;
     unsafe {
         asm!(
-            "csrc mstatus, {mstatus_mie}", // Disable interrupts
             "csrr {0}, mip",
             "csrr {1}, mcause",
             out(reg) mip,
             out(reg) mcause,
-            mstatus_mie = in(reg) 0x8,
         );
     }
     log::trace!("MIP: {:b}", mip);
@@ -123,11 +125,7 @@ extern "C" fn trap_handler() {
     }
 
     unsafe {
-        asm!(
-            "csrs mstatus, {mstatus_mie}",  // Enable interrupts
-            "mret",
-            mstatus_mie = in(reg) 0x8,
-        );
+        asm!("mret",);
     }
 }
 

justfile

@@ -53,7 +53,7 @@ test:
 	cargo run -- run --config {{qemu_virt}} --firmware vectored_mtvec
 	cargo run -- run --config {{qemu_virt}} --firmware device
 	cargo run -- run --config {{qemu_virt}} --firmware clint_interrupt_priority
-	#cargo run -- run --config {{qemu_virt}} --firmware clint_interrupt
+	cargo run -- run --config {{qemu_virt}} --firmware clint_interrupt
 	cargo run -- run --config {{qemu_virt_2harts}} --firmware clint_interrupt_multihart
 	cargo run -- run --config {{qemu_virt_release}} --firmware default
 
@@ -90,6 +90,7 @@ spike-test:
 	cargo run -- run --config {{spike}} --firmware vectored_mtvec
 	cargo run -- run --config {{spike}} --firmware device
 	cargo run -- run --config {{spike}} --firmware default
+	cargo run -- run --config {{spike}} --firmware clint_interrupt
 	cargo run -- run --config {{spike}} --firmware clint_interrupt_priority
 
 	# Testing with Miralis as firmware

src/device/clint.rs

@@ -58,12 +58,14 @@ impl VirtClint {
         let mtimecmp = driver.read_mtimecmp(ctx.hart_id).unwrap();
         let mtime = driver.read_mtime();
 
-        if msip == 0 {
-            ctx.csr.mip &= !(mie::MSIE_FILTER);
+        if (ctx.csr.mip & mie::MSIE_FILTER) != 0 && msip == 0 {
+            // If the vMIP.MSIP bit was set, but MSIP of CLINT is 0, clear the vMIP.MSIP bit
+            ctx.csr.mip &= !mie::MSIE_FILTER;
             log::trace!("vMSIP cleared");
         }
-        if mtime < mtimecmp {
-            ctx.csr.mip &= !(mie::MTIE_FILTER);
+        if (ctx.csr.mip & mie::MTIE_FILTER) != 0 && mtime < mtimecmp {
+            // If the vMIP.MSIP bit was set, but MTIMECMP is in the future, clear the vMIP.MSIP bit
+            ctx.csr.mip &= !mie::MTIE_FILTER;
             log::trace!("vMTIP cleared");
         }
     }

src/virt.rs

@@ -183,7 +183,6 @@ impl VirtContext {
                     Arch::write_csr(Csr::Mie, self.csr.mie);
                 }
 
-                self.update_interrupts(mctx);
                 Arch::wfi();
 
                 self.pc += 4;
@@ -434,9 +433,13 @@ impl VirtContext {
 
     pub fn emulate_jump_trap_handler(&mut self) {
         // We are now emulating a trap, registers need to be updated
-        log::trace!("Emulating jump to trap handler");
+        log::trace!("Emulating jump to trap handler to {:x?}", self.trap_info);
         self.csr.mcause = self.trap_info.mcause;
-        self.csr.mstatus = self.trap_info.mstatus;
+        // MPIE bit should be preserved; otherwise, MPIE would be equal to vMIE
+        // Global MIE bit is 0 inside the trap handler according to the spec, so MIE = 0 = vMIE
+        self.csr.mstatus = (self.trap_info.mstatus & !mstatus::MPIE_FILTER)
+            | ((self.csr.mstatus & mstatus::MIE_FILTER) >> mstatus::MIE_OFFSET
+                << mstatus::MPIE_OFFSET);
         self.csr.mtval = self.trap_info.mtval;
         self.csr.mepc = self.trap_info.mepc;
 
@@ -695,21 +698,49 @@ impl VirtContext {
         };
 
         // MEI is expected to be delegated at almost all times (for S-mode to receive interrupts)
-        // This virtualization approach might not be the most suitable one
+        // This virtualization approach might not be the most suitable one here
         if irq_to_set & mie::MEIE_FILTER != 0 {
             todo!("Add plic device");
         }
 
         if irq_to_set & mie::MTIE_FILTER != 0 {
-            log::trace!("Set up M timer interrupt");
+            log::trace!("Set up M-mode timer interrupt");
+
+            // From my understanding, there are two potential ways to handle re-raising interrupts here:
+            // 1. Hardware-based re-raising: This seems like the more reliable option,
+            //    but there might be some overhead, such as delays in MTI arrival. This could be
+            //    problematic for timing-sensitive systems.
+            // 2. Software-based injection: This approach involves injecting the interrupt directly.
+            //    However, in its current state, it seems a bit unstable and has caused occasional
+            //    RustSBI test failures towards the end.
+
+            // I’m leaving the alternative code below commented out for now, as I'm not entirely
+            // sure if all edge cases are handled correctly.
+
+            // self.trap_info.mcause = MCause::MachineTimerInt as usize;
+            // self.trap_info.mepc = self.pc + 4;
+            // self.trap_info.mip = self.csr.mip;
+            // self.trap_info.mstatus = (self.csr.mstatus & !mstatus::MIE_FILTER)
+            //     | ((self.csr.mstatus & mstatus::MIE_FILTER) >> mstatus::MIE_OFFSET
+            //         << mstatus::MPIE_OFFSET);
+            // self.trap_info.mtval = 0;
+            // self.emulate_jump_trap_handler();
             let mut clint = Plat::get_clint().lock();
             clint
                 .write_mtimecmp(mctx.hw.hart, usize::MIN)
                 .expect("Failed to write mtimecmp");
         }
 
         if irq_to_set & mie::MSIE_FILTER != 0 {
-            log::trace!("Set up M software interrupt");
+            log::trace!("Set up M-mode software interrupt");
+            // self.trap_info.mcause = MCause::MachineSoftInt as usize;
+            // self.trap_info.mepc = self.pc + 4;
+            // self.trap_info.mip = self.csr.mip;
+            // self.trap_info.mstatus = (self.csr.mstatus & !mstatus::MIE_FILTER)
+            //     | ((self.csr.mstatus & mstatus::MIE_FILTER) >> mstatus::MIE_OFFSET
+            //         << mstatus::MPIE_OFFSET);
+            // self.trap_info.mtval = 0;
+            // self.emulate_jump_trap_handler();
             let mut clint = Plat::get_clint().lock();
             clint
                 .write_msip(mctx.hw.hart, 1)
@@ -1161,7 +1192,9 @@ impl HwRegisterContextSetter<Csr> for VirtContext {
             Csr::Mseccfg => self.csr.mseccfg = value,
             Csr::Mconfigptr => (),                    // Read-only
             Csr::Medeleg => self.csr.medeleg = value, //TODO : some values need to be read-only 0
-            Csr::Mideleg => self.csr.mideleg = value & hw.interrupts,
+            Csr::Mideleg => {
+                self.csr.mideleg = value & hw.interrupts & !mie::MSIE_FILTER & !mie::MTIE_FILTER
+            }
             Csr::Mtinst => todo!(), // TODO : Can only be written automatically by the hardware on a trap, this register should not exist in a system without hypervisor extension
             Csr::Mtval2 => todo!(), // TODO : Must be able to hold 0 and may hold an arbitrary number of 2-bit-shifted guest physical addresses, written alongside mtval, this register should not exist in a system without hypervisor extension
             Csr::Tselect => todo!(), // Read-only 0 when no triggers are implemented