refactor(axdevice): unify Device model with indexed dispatch and conflict detect

os/axvisor/src/shell/command/vm.rs

@@ -917,12 +917,8 @@ fn show_vm_basic_details(vm_id: usize, show_config: bool, show_stats: bool) {
             println!();
             println!("Device Summary:");
             println!(
-                "  MMIO Devices:   {}",
-                vm.get_devices().iter_mmio_dev().count()
-            );
-            println!(
-                "  SysReg Devices: {}",
-                vm.get_devices().iter_sys_reg_dev().count()
+                "  Registered Devices: {}",
+                vm.get_devices().devices().count()
             );
         }
 
@@ -1147,11 +1143,9 @@ fn show_vm_full_details(vm_id: usize) {
 
         // Devices
         println!();
-        let mmio_dev_count = vm.get_devices().iter_mmio_dev().count();
-        let sysreg_dev_count = vm.get_devices().iter_sys_reg_dev().count();
+        let device_count = vm.get_devices().devices().count();
         println!("Devices:");
-        println!("  MMIO Devices:   {}", mmio_dev_count);
-        println!("  SysReg Devices: {}", sysreg_dev_count);
+        println!("  Devices:        {}", device_count);
 
         // Additional Statistics
         println!();

virtualization/arm_vgic/src/v3/gits.rs

@@ -14,14 +14,14 @@
 
 //! WARNING: Identical mapping only!!
 
-use core::{cell::UnsafeCell, ptr};
+use core::ptr;
 
-use ax_kspin::SpinNoIrq as Mutex;
+use ax_kspin::SpinNoIrq;
 use ax_memory_addr::PhysAddr;
 use axdevice_base::{AccessWidth, BaseDeviceOps};
 use axvm_types::{GuestPhysAddr, GuestPhysAddrRange, HostPhysAddr};
-use log::{debug, trace};
-use spin::Once;
+use log::{debug, trace, warn};
+use spin::{Mutex, Once};
 
 use super::{
     registers::*,
@@ -61,17 +61,22 @@ pub struct Gits {
     pub is_root_vm: bool,
 
     /// Virtual GITS registers.
-    pub regs: UnsafeCell<VirtualGitsRegs>,
+    pub regs: SpinNoIrq<VirtualGitsRegs>,
+
+    /// Serialises CWRITER command-queue submission so that only one
+    /// vCPU can process the queue at a time, preventing races where
+    /// two vCPUs see the same old `creadr` and re-process already-
+    /// handled commands.
+    pub submission_lock: SpinNoIrq<()>,
 }
 
 impl Gits {
-    fn regs(&self) -> &VirtualGitsRegs {
-        unsafe { &*self.regs.get() }
+    fn with_regs<R>(&self, f: impl FnOnce(&VirtualGitsRegs) -> R) -> R {
+        f(&self.regs.lock())
     }
 
-    #[allow(clippy::mut_from_ref)]
-    fn regs_mut(&self) -> &mut VirtualGitsRegs {
-        unsafe { &mut *self.regs.get() }
+    fn with_regs_mut<R>(&self, f: impl FnOnce(&mut VirtualGitsRegs) -> R) -> R {
+        f(&mut self.regs.lock())
     }
 
     /// Creates a new GITS instance.
@@ -82,7 +87,7 @@ impl Gits {
         is_root_vm: bool,
     ) -> Self {
         let size = size.unwrap_or(DEFAULT_GITS_SIZE); // 4K
-        let regs = UnsafeCell::new(VirtualGitsRegs::default());
+        let regs = SpinNoIrq::new(VirtualGitsRegs::default());
 
         // ensure cmdq and lpi prop table is initialized before VMs are up
         let _ = get_cmdq(host_gits_base);
@@ -98,6 +103,7 @@ impl Gits {
             host_gits_base,
             is_root_vm,
             regs,
+            submission_lock: SpinNoIrq::new(()),
         }
     }
 }
@@ -132,29 +138,25 @@ impl BaseDeviceOps<GuestPhysAddrRange> for Gits {
         // mmio_perform_access(gits_base, mmio);
         match reg {
             GITS_CTRL => perform_mmio_read(gits_base + reg, width),
-            GITS_CBASER => Ok(self.regs().cbaser),
+            GITS_CBASER => Ok(self.with_regs(|r| r.cbaser)),
             GITS_DT_BASER => {
                 if self.is_root_vm {
                     perform_mmio_read(gits_base + reg, width)
                 } else {
-                    Ok(
-                        (self.regs().dt_baser)
-                            & (1usize.unbounded_shl(width.size() as u32 * 8) - 1),
-                    )
+                    Ok((self.with_regs(|r| r.dt_baser))
+                        & (1usize.unbounded_shl(width.size() as u32 * 8) - 1))
                 }
             }
             GITS_CT_BASER => {
                 if self.is_root_vm {
                     perform_mmio_read(gits_base + reg, width)
                 } else {
-                    Ok(
-                        (self.regs().ct_baser)
-                            & (1usize.unbounded_shl(width.size() as u32 * 8) - 1),
-                    )
+                    Ok((self.with_regs(|r| r.ct_baser))
+                        & (1usize.unbounded_shl(width.size() as u32 * 8) - 1))
                 }
             }
-            GITS_CWRITER => Ok(self.regs().cwriter),
-            GITS_CREADR => Ok(self.regs().creadr),
+            GITS_CWRITER => Ok(self.with_regs(|r| r.cwriter)),
+            GITS_CREADR => Ok(self.with_regs(|r| r.creadr)),
             GITS_TYPER => perform_mmio_read(gits_base + reg, width),
             _ => perform_mmio_read(gits_base + reg, width),
         }
@@ -187,41 +189,47 @@ impl BaseDeviceOps<GuestPhysAddrRange> for Gits {
                     perform_mmio_write(gits_base + reg, width, val)?;
                 }
 
-                self.regs_mut().cbaser = val;
+                self.with_regs_mut(|r| r.cbaser = val);
                 Ok(())
             }
             GITS_DT_BASER => {
                 if self.is_root_vm {
                     perform_mmio_write(gits_base + reg, width, val)
                 } else {
-                    self.regs_mut().dt_baser = val;
+                    self.with_regs_mut(|r| r.dt_baser = val);
                     Ok(())
                 }
             }
             GITS_CT_BASER => {
                 if self.is_root_vm {
                     perform_mmio_write(gits_base + reg, width, val)
                 } else {
-                    self.regs_mut().ct_baser = val;
+                    self.with_regs_mut(|r| r.ct_baser = val);
                     Ok(())
                 }
             }
             GITS_CWRITER => {
-                self.regs_mut().cwriter = val;
+                // Serialise concurrent command-queue submissions: two
+                // vCPUs writing CWRITER must not see the same old
+                // creadr, or one may re-process commands already
+                // handled by the other.
+                let _submission = self.submission_lock.lock();
+                self.with_regs_mut(|r| r.cwriter = val);
 
                 if val != 0 {
-                    let regs = self.regs();
-                    let cbaser = regs.cbaser;
-                    let creadr = regs.creadr;
+                    let (cbaser, creadr) = self.with_regs(|r| (r.cbaser, r.creadr));
 
                     let mut cmdq = get_cmdq(self.host_gits_base).lock();
-                    self.regs_mut().creadr = cmdq.insert_cmd(cbaser, creadr, val);
+                    let new_creadr = cmdq.insert_cmd(cbaser, creadr, val);
+                    self.with_regs_mut(|r| r.creadr = new_creadr);
+                    drop(cmdq);
                 }
 
                 Ok(())
             }
             GITS_CREADR => {
-                panic!("GITS_CREADR should not be written by guest!");
+                warn!("GITS_CREADR: guest write ignored (read-only register)");
+                Ok(())
             }
             GITS_TYPER => perform_mmio_write(gits_base + reg, width, val),
             _ => perform_mmio_write(gits_base + reg, width, val),
@@ -413,7 +421,10 @@ impl Cmdq {
         let origin_vm_readr = vm_creadr;
 
         // todo: handle wrap around
-        let cmd_size = vm_writer - origin_vm_readr;
+        let cmd_size = vm_writer.wrapping_sub(origin_vm_readr);
+        if cmd_size == 0 || cmd_size > 0x10000 {
+            return vm_writer;
+        }
         let cmd_num = cmd_size / BYTES_PER_CMD;
 
         trace!(

virtualization/arm_vgic/src/v3/vgicd.rs

@@ -12,9 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use core::cell::UnsafeCell;
-
 use ax_errno::AxResult;
+use ax_kspin::SpinNoIrq;
 use axdevice_base::{AccessWidth, BaseDeviceOps, EmuDeviceType};
 use axvm_types::{GuestPhysAddr, GuestPhysAddrRange, HostPhysAddr};
 use bitmaps::Bitmap;
@@ -39,7 +38,7 @@ pub struct VGicD {
     pub size: usize,
 
     /// IRQs assigned to this VGicD.
-    pub assigned_irqs: UnsafeCell<Bitmap<{ MAX_IRQ_V3 }>>,
+    pub assigned_irqs: SpinNoIrq<Bitmap<{ MAX_IRQ_V3 }>>,
 
     /// The host physical address of the VGicD.
     ///
@@ -55,7 +54,7 @@ impl VGicD {
         Self {
             addr,
             size,
-            assigned_irqs: UnsafeCell::new(Bitmap::new()),
+            assigned_irqs: SpinNoIrq::new(Bitmap::new()),
             host_gicd_addr: crate::api_reexp::get_host_gicd_base(),
         }
     }
@@ -70,9 +69,7 @@ impl VGicD {
         if irq >= MAX_IRQ_V3 as u32 {
             panic!("IRQ {} is out of range for VGicD", irq);
         }
-        unsafe {
-            (*self.assigned_irqs.get()).set(irq as usize, true);
-        }
+        self.assigned_irqs.lock().set(irq as usize, true);
 
         // TODO: update host GICD_ITARGETSR and GICD_IROUTER registers
         let gicd_itargetsr_paddr = self.host_gicd_addr + GICD_ITARGETSR + irq as usize;
@@ -253,7 +250,7 @@ impl BaseDeviceOps<GuestPhysAddrRange> for VGicD {
 impl VGicD {
     /// Checks if an IRQ is assigned to this VGicD.
     pub fn is_irq_assigned(&self, irq: u32) -> bool {
-        unsafe { (*self.assigned_irqs.get()).get(irq as usize) }
+        self.assigned_irqs.lock().get(irq as usize)
     }
 
     /// Checks if an IRQ is a Software Generated Interrupt (SGI).

virtualization/arm_vgic/src/v3/vgicr.rs

@@ -12,9 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use core::{cell::UnsafeCell, ptr};
+use core::ptr;
 
-use ax_kspin::SpinNoIrq as Mutex;
+use ax_kspin::SpinNoIrq;
 use ax_memory_addr::PhysAddr;
 use axdevice_base::{AccessWidth, BaseDeviceOps};
 use axvm_types::{GuestPhysAddr, GuestPhysAddrRange, HostPhysAddr};
@@ -49,19 +49,18 @@ pub struct VGicR {
     pub host_gicr_base_this_cpu: HostPhysAddr,
 
     /// Virtual GICR registers.
-    pub regs: UnsafeCell<VGicRRegs>,
+    pub regs: SpinNoIrq<VGicRRegs>,
 }
 
 impl VGicR {
-    /// Gets a reference to the registers.
-    pub fn regs(&self) -> &VGicRRegs {
-        unsafe { &*self.regs.get() }
+    /// Accesses the registers immutably under the internal lock.
+    fn with_regs<R>(&self, f: impl FnOnce(&VGicRRegs) -> R) -> R {
+        f(&self.regs.lock())
     }
 
-    /// Gets a mutable reference to the registers.
-    #[allow(clippy::mut_from_ref)]
-    pub fn regs_mut(&self) -> &mut VGicRRegs {
-        unsafe { &mut *self.regs.get() }
+    /// Accesses the registers mutably under the internal lock.
+    fn with_regs_mut<R>(&self, f: impl FnOnce(&mut VGicRRegs) -> R) -> R {
+        f(&mut self.regs.lock())
     }
 
     /// Creates a new VGicR instance.
@@ -74,7 +73,7 @@ impl VGicR {
             size,
             cpu_id,
             host_gicr_base_this_cpu,
-            regs: UnsafeCell::new(VGicRRegs { propbaser: 0 }),
+            regs: SpinNoIrq::new(VGicRRegs { propbaser: 0 }),
         }
     }
 }
@@ -128,7 +127,7 @@ impl BaseDeviceOps<GuestPhysAddrRange> for VGicR {
                 // all the redist share one prop tbl
                 // mmio_perform_access(gicr_base, mmio);
 
-                Ok(self.regs().propbaser)
+                Ok(self.with_regs(|r| r.propbaser))
             }
             GICR_SYNCR => {
                 // always return 0 for synchronization register
@@ -181,7 +180,7 @@ impl BaseDeviceOps<GuestPhysAddrRange> for VGicR {
             }
             GICR_PROPBASER => {
                 // all the redist share one prop tbl
-                self.regs_mut().propbaser = value;
+                self.with_regs_mut(|r| r.propbaser = value);
                 Ok(())
             }
             GICR_SETLPIR | GICR_CLRLPIR | GICR_INVALLR => {
@@ -284,17 +283,17 @@ impl LpiPropTable {
 }
 
 /// Global LPI property table instance.
-pub static LPT: Once<Mutex<LpiPropTable>> = Once::new();
+pub static LPT: Once<spin::Mutex<LpiPropTable>> = Once::new();
 
 /// Gets or initializes the global LPI property table.
 pub fn get_lpt(
     host_gicd_typer: u32,
     host_gicr_base: HostPhysAddr,
     size_per_gicr: Option<usize>,
-) -> &'static Mutex<LpiPropTable> {
+) -> &'static spin::Mutex<LpiPropTable> {
     if !LPT.is_completed() {
         LPT.call_once(|| {
-            Mutex::new(LpiPropTable::new(
+            spin::Mutex::new(LpiPropTable::new(
                 host_gicd_typer,
                 host_gicr_base,
                 size_per_gicr,

virtualization/axdevice/Cargo.toml

@@ -30,3 +30,12 @@ arm_vgic = { workspace = true, features = ["vgicv3"] }
 riscv_vplic = { workspace = true }
 [target.'cfg(target_arch = "x86_64")'.dependencies]
 x86_vlapic = { workspace = true }
+
+[[bench]]
+name = "lookup_AxVmDevices"
+harness = false
+
+[dev-dependencies]
+divan = "0.1.21"
+paste = "1.0"
+rand = { version = "0.10", features = ["std_rng"] }