From b1934dc18bf7afc0782976194729d6358d0c3400 Mon Sep 17 00:00:00 2001 From: Sofia <70857676+Wesdcv@users.noreply.github.com> Date: Mon, 30 Sep 2024 07:53:36 +0300 Subject: [PATCH] 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 | 12 +++--- firmware/clint_interrupt_multihart/main.rs | 12 +++++- firmware/clint_interrupt_priority/main.rs | 26 ++++++------ justfile | 3 +- src/device/clint.rs | 10 +++-- src/virt.rs | 47 ++++++++++++++++++---- 6 files changed, 77 insertions(+), 33 deletions(-) diff --git a/firmware/clint_interrupt/main.rs b/firmware/clint_interrupt/main.rs index 06a3f636..c4563667 100644 --- a/firmware/clint_interrupt/main.rs +++ b/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, ); } diff --git a/firmware/clint_interrupt_multihart/main.rs b/firmware/clint_interrupt_multihart/main.rs index 50934a8c..9251611b 100644 --- a/firmware/clint_interrupt_multihart/main.rs +++ b/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, ); } diff --git a/firmware/clint_interrupt_priority/main.rs b/firmware/clint_interrupt_priority/main.rs index 8f2cd737..f0da4bce 100644 --- a/firmware/clint_interrupt_priority/main.rs +++ b/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",); } } diff --git a/justfile b/justfile index d4465a33..9220e0c4 100644 --- a/justfile +++ b/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 diff --git a/src/device/clint.rs b/src/device/clint.rs index 8b237d58..d08c03ff 100644 --- a/src/device/clint.rs +++ b/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"); } } diff --git a/src/virt.rs b/src/virt.rs index 9333bf98..37c04b32 100644 --- a/src/virt.rs +++ b/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,13 +698,33 @@ 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) @@ -709,7 +732,15 @@ impl VirtContext { } 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 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