From 954f3dd057a61ec84107d0e893565f66167f6bd2 Mon Sep 17 00:00:00 2001 From: Yuanchu Xie Date: Tue, 19 Mar 2024 18:03:56 -0700 Subject: [PATCH] vm-device: generalize BusDevice to use a shared reference BusDevice trait functions currently holds a mutable reference to self, and exclusive access is guaranteed by taking a Mutex when dispatched by the Bus object. However, this prevents individual devices from serving accesses that do not require an mutable reference or is better served with different synchronization primitives. We switch Bus to dispatch via BusDeviceSync, which holds a shared reference, and delegate locking to the BusDeviceSync trait implementation for Mutex. Other changes are made to make use of the dyn BusDeviceSync trait object. Signed-off-by: Yuanchu Xie --- pci/src/bus.rs | 4 +-- vm-device/src/bus.rs | 61 +++++++++++++++++++++++++-------------- vm-device/src/lib.rs | 2 +- vmm/src/device_manager.rs | 48 +++++++++++++++--------------- vmm/src/pci_segment.rs | 4 +-- 5 files changed, 69 insertions(+), 50 deletions(-) diff --git a/pci/src/bus.rs b/pci/src/bus.rs index a809fbc22..8166d9b39 100644 --- a/pci/src/bus.rs +++ b/pci/src/bus.rs @@ -14,7 +14,7 @@ use std::any::Any; use std::collections::HashMap; use std::ops::DerefMut; use std::sync::{Arc, Barrier, Mutex}; -use vm_device::{Bus, BusDevice}; +use vm_device::{Bus, BusDevice, BusDeviceSync}; const VENDOR_ID_INTEL: u16 = 0x8086; const DEVICE_ID_INTEL_VIRT_PCIE_HOST: u16 = 0x0d57; @@ -122,7 +122,7 @@ impl PciBus { pub fn register_mapping( &self, - dev: Arc>, + dev: Arc, #[cfg(target_arch = "x86_64")] io_bus: &Bus, mmio_bus: &Bus, bars: Vec, diff --git a/vm-device/src/bus.rs b/vm-device/src/bus.rs index 081cf6b8e..7e1f87c80 100644 --- a/vm-device/src/bus.rs +++ b/vm-device/src/bus.rs @@ -26,6 +26,31 @@ pub trait BusDevice: Send { } } +#[allow(unused_variables)] +pub trait BusDeviceSync: Send + Sync { + /// Reads at `offset` from this device + fn read(&self, base: u64, offset: u64, data: &mut [u8]) {} + /// Writes at `offset` into this device + fn write(&self, base: u64, offset: u64, data: &[u8]) -> Option> { + None + } +} + +impl BusDeviceSync for Mutex { + /// Reads at `offset` from this device + fn read(&self, base: u64, offset: u64, data: &mut [u8]) { + self.lock() + .expect("Failed to acquire device lock") + .read(base, offset, data) + } + /// Writes at `offset` into this device + fn write(&self, base: u64, offset: u64, data: &[u8]) -> Option> { + self.lock() + .expect("Failed to acquire device lock") + .write(base, offset, data) + } +} + #[derive(Debug)] pub enum Error { /// The insertion failed because the new device overlapped with an old device. @@ -95,7 +120,7 @@ impl PartialOrd for BusRange { /// only restriction is that no two devices can overlap in this address space. #[derive(Default)] pub struct Bus { - devices: RwLock>>>, + devices: RwLock>>, } impl Bus { @@ -106,7 +131,7 @@ impl Bus { } } - fn first_before(&self, addr: u64) -> Option<(BusRange, Arc>)> { + fn first_before(&self, addr: u64) -> Option<(BusRange, Arc)> { let devices = self.devices.read().unwrap(); let (range, dev) = devices .range(..=BusRange { base: addr, len: 1 }) @@ -115,7 +140,7 @@ impl Bus { } #[allow(clippy::type_complexity)] - pub fn resolve(&self, addr: u64) -> Option<(u64, u64, Arc>)> { + fn resolve(&self, addr: u64) -> Option<(u64, u64, Arc)> { if let Some((range, dev)) = self.first_before(addr) { let offset = addr - range.base; if offset < range.len { @@ -125,8 +150,7 @@ impl Bus { None } - /// Puts the given device at the given address space. - pub fn insert(&self, device: Arc>, base: u64, len: u64) -> Result<()> { + pub fn insert(&self, device: Arc, base: u64, len: u64) -> Result<()> { if len == 0 { return Err(Error::ZeroSizedRange); } @@ -171,7 +195,7 @@ impl Bus { } /// Removes all entries referencing the given device. - pub fn remove_by_device(&self, device: &Arc>) -> Result<()> { + pub fn remove_by_device(&self, device: &Arc) -> Result<()> { let mut device_list = self.devices.write().unwrap(); let mut remove_key_list = Vec::new(); @@ -216,9 +240,7 @@ impl Bus { pub fn read(&self, addr: u64, data: &mut [u8]) -> Result<()> { if let Some((base, offset, dev)) = self.resolve(addr) { // OK to unwrap as lock() failing is a serious error condition and should panic. - dev.lock() - .expect("Failed to acquire device lock") - .read(base, offset, data); + dev.read(base, offset, data); Ok(()) } else { Err(Error::MissingAddressRange) @@ -231,10 +253,7 @@ impl Bus { pub fn write(&self, addr: u64, data: &[u8]) -> Result>> { if let Some((base, offset, dev)) = self.resolve(addr) { // OK to unwrap as lock() failing is a serious error condition and should panic. - Ok(dev - .lock() - .expect("Failed to acquire device lock") - .write(base, offset, data)) + Ok(dev.write(base, offset, data)) } else { Err(Error::MissingAddressRange) } @@ -246,17 +265,17 @@ mod tests { use super::*; struct DummyDevice; - impl BusDevice for DummyDevice {} + impl BusDeviceSync for DummyDevice {} struct ConstantDevice; - impl BusDevice for ConstantDevice { - fn read(&mut self, _base: u64, offset: u64, data: &mut [u8]) { + impl BusDeviceSync for ConstantDevice { + fn read(&self, _base: u64, offset: u64, data: &mut [u8]) { for (i, v) in data.iter_mut().enumerate() { *v = (offset as u8) + (i as u8); } } - fn write(&mut self, _base: u64, offset: u64, data: &[u8]) -> Option> { + fn write(&self, _base: u64, offset: u64, data: &[u8]) -> Option> { for (i, v) in data.iter().enumerate() { assert_eq!(*v, (offset as u8) + (i as u8)) } @@ -268,7 +287,7 @@ mod tests { #[test] fn bus_insert() { let bus = Bus::new(); - let dummy = Arc::new(Mutex::new(DummyDevice)); + let dummy = Arc::new(DummyDevice); assert!(bus.insert(dummy.clone(), 0x10, 0).is_err()); assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok()); @@ -290,7 +309,7 @@ mod tests { #[allow(clippy::redundant_clone)] fn bus_read_write() { let bus = Bus::new(); - let dummy = Arc::new(Mutex::new(DummyDevice)); + let dummy = Arc::new(DummyDevice); assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok()); assert!(bus.read(0x10, &mut [0, 0, 0, 0]).is_ok()); assert!(bus.write(0x10, &[0, 0, 0, 0]).is_ok()); @@ -308,7 +327,7 @@ mod tests { #[allow(clippy::redundant_clone)] fn bus_read_write_values() { let bus = Bus::new(); - let dummy = Arc::new(Mutex::new(ConstantDevice)); + let dummy = Arc::new(ConstantDevice); assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok()); let mut values = [0, 1, 2, 3]; @@ -334,7 +353,7 @@ mod tests { let bus = Bus::new(); let mut data = [1, 2, 3, 4]; - let device = Arc::new(Mutex::new(DummyDevice)); + let device = Arc::new(DummyDevice); assert!(bus.insert(device.clone(), 0x10, 0x10).is_ok()); assert!(bus.write(0x10, &data).is_ok()); assert!(bus.read(0x10, &mut data).is_ok()); diff --git a/vm-device/src/lib.rs b/vm-device/src/lib.rs index c16d0ec4e..c10731ea9 100644 --- a/vm-device/src/lib.rs +++ b/vm-device/src/lib.rs @@ -9,7 +9,7 @@ mod bus; pub mod dma_mapping; pub mod interrupt; -pub use self::bus::{Bus, BusDevice, Error as BusError}; +pub use self::bus::{Bus, BusDevice, BusDeviceSync, Error as BusError}; /// Type of Message Signalled Interrupt #[derive(Copy, Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index d6fc1b94f..3b9b24e39 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -89,7 +89,7 @@ use vm_device::dma_mapping::ExternalDmaMapping; use vm_device::interrupt::{ InterruptIndex, InterruptManager, LegacyIrqGroupConfig, MsiIrqGroupConfig, }; -use vm_device::{Bus, BusDevice, Resource}; +use vm_device::{Bus, BusDevice, BusDeviceSync, Resource}; use vm_memory::guest_memory::FileOffset; use vm_memory::GuestMemoryRegion; use vm_memory::{Address, GuestAddress, GuestUsize, MmapRegion}; @@ -818,7 +818,7 @@ pub struct DeviceManager { // Let the DeviceManager keep strong references to the BusDevice devices. // This allows the IO and MMIO buses to be provided with Weak references, // which prevents cyclic dependencies. - bus_devices: Vec>>, + bus_devices: Vec>, // Counter to keep track of the consumed device IDs. device_id_cnt: Wrapping, @@ -1183,7 +1183,7 @@ impl DeviceManager { address_manager .mmio_bus .insert( - Arc::clone(&device_manager) as Arc>, + Arc::clone(&device_manager) as Arc, acpi_address.0, DEVICE_MANAGER_ACPI_SIZE as u64, ) @@ -1226,7 +1226,7 @@ impl DeviceManager { self.address_manager .mmio_bus .insert( - Arc::clone(&self.memory_manager) as Arc>, + Arc::clone(&self.memory_manager) as Arc, acpi_address.0, MEMORY_MANAGER_ACPI_SIZE as u64, ) @@ -1268,7 +1268,7 @@ impl DeviceManager { if let Some(tpm) = self.config.clone().lock().unwrap().tpm.as_ref() { let tpm_dev = self.add_tpm_device(tpm.socket.clone())?; self.bus_devices - .push(Arc::clone(&tpm_dev) as Arc>) + .push(Arc::clone(&tpm_dev) as Arc) } self.legacy_interrupt_manager = Some(legacy_interrupt_manager); @@ -1400,11 +1400,11 @@ impl DeviceManager { #[cfg(target_arch = "x86_64")] if let Some(pci_config_io) = segment.pci_config_io.as_ref() { self.bus_devices - .push(Arc::clone(pci_config_io) as Arc>); + .push(Arc::clone(pci_config_io) as Arc); } self.bus_devices - .push(Arc::clone(&segment.pci_config_mmio) as Arc>); + .push(Arc::clone(&segment.pci_config_mmio) as Arc); } Ok(()) @@ -1489,7 +1489,7 @@ impl DeviceManager { .map_err(DeviceManagerError::BusError)?; self.bus_devices - .push(Arc::clone(&interrupt_controller) as Arc>); + .push(Arc::clone(&interrupt_controller) as Arc); // Fill the device tree with a new node. In case of restore, we // know there is nothing to do, so we can simply override the @@ -1521,7 +1521,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&shutdown_device) as Arc>); + .push(Arc::clone(&shutdown_device) as Arc); #[cfg(target_arch = "x86_64")] { @@ -1584,12 +1584,12 @@ impl DeviceManager { ) .map_err(DeviceManagerError::BusError)?; self.bus_devices - .push(Arc::clone(&ged_device) as Arc>); + .push(Arc::clone(&ged_device) as Arc); let pm_timer_device = Arc::new(Mutex::new(devices::AcpiPmTimerDevice::new())); self.bus_devices - .push(Arc::clone(&pm_timer_device) as Arc>); + .push(Arc::clone(&pm_timer_device) as Arc); #[cfg(target_arch = "x86_64")] { @@ -1629,7 +1629,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&i8042) as Arc>); + .push(Arc::clone(&i8042) as Arc); self.address_manager .io_bus @@ -1657,7 +1657,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&cmos) as Arc>); + .push(Arc::clone(&cmos) as Arc); self.address_manager .io_bus @@ -1667,7 +1667,7 @@ impl DeviceManager { let fwdebug = Arc::new(Mutex::new(devices::legacy::FwDebugDevice::new())); self.bus_devices - .push(Arc::clone(&fwdebug) as Arc>); + .push(Arc::clone(&fwdebug) as Arc); self.address_manager .io_bus @@ -1678,7 +1678,7 @@ impl DeviceManager { // 0x80 debug port let debug_port = Arc::new(Mutex::new(devices::legacy::DebugPort::new(self.timestamp))); self.bus_devices - .push(Arc::clone(&debug_port) as Arc>); + .push(Arc::clone(&debug_port) as Arc); self.address_manager .io_bus .insert(debug_port, 0x80, 0x1) @@ -1710,7 +1710,7 @@ impl DeviceManager { let rtc_device = Arc::new(Mutex::new(devices::legacy::Rtc::new(interrupt_group))); self.bus_devices - .push(Arc::clone(&rtc_device) as Arc>); + .push(Arc::clone(&rtc_device) as Arc); let addr = arch::layout::LEGACY_RTC_MAPPED_IO_START; @@ -1752,7 +1752,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&gpio_device) as Arc>); + .push(Arc::clone(&gpio_device) as Arc); let addr = arch::layout::LEGACY_GPIO_MAPPED_IO_START; @@ -1802,7 +1802,7 @@ impl DeviceManager { .unwrap_or(debug_console::DEFAULT_PORT); self.bus_devices - .push(Arc::clone(&debug_console) as Arc>); + .push(Arc::clone(&debug_console) as Arc); self.address_manager .allocator @@ -1853,7 +1853,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&serial) as Arc>); + .push(Arc::clone(&serial) as Arc); self.address_manager .allocator @@ -1910,7 +1910,7 @@ impl DeviceManager { ))); self.bus_devices - .push(Arc::clone(&serial) as Arc>); + .push(Arc::clone(&serial) as Arc); let addr = arch::layout::LEGACY_SERIAL_MAPPED_IO_START; @@ -3412,7 +3412,7 @@ impl DeviceManager { fn add_pci_device( &mut self, - bus_device: Arc>, + bus_device: Arc, pci_device: Arc>, segment_id: u16, bdf: PciBdf, @@ -4076,7 +4076,7 @@ impl DeviceManager { ( Arc::clone(&vfio_pci_device) as Arc>, - Arc::clone(&vfio_pci_device) as Arc>, + Arc::clone(&vfio_pci_device) as Arc, None as Option>>, false, ) @@ -4108,7 +4108,7 @@ impl DeviceManager { ( Arc::clone(&virtio_pci_device) as Arc>, - Arc::clone(&virtio_pci_device) as Arc>, + Arc::clone(&virtio_pci_device) as Arc, Some(dev.virtio_device()), dev.dma_handler().is_some() && !iommu_attached, ) @@ -4124,7 +4124,7 @@ impl DeviceManager { ( Arc::clone(&vfio_user_pci_device) as Arc>, - Arc::clone(&vfio_user_pci_device) as Arc>, + Arc::clone(&vfio_user_pci_device) as Arc, None as Option>>, true, ) diff --git a/vmm/src/pci_segment.rs b/vmm/src/pci_segment.rs index 0b9b7f019..556a44aba 100644 --- a/vmm/src/pci_segment.rs +++ b/vmm/src/pci_segment.rs @@ -18,7 +18,7 @@ use pci::{PciConfigIo, PCI_CONFIG_IO_PORT, PCI_CONFIG_IO_PORT_SIZE}; use std::sync::{Arc, Mutex}; use uuid::Uuid; use vm_allocator::AddressAllocator; -use vm_device::BusDevice; +use vm_device::BusDeviceSync; pub(crate) struct PciSegment { pub(crate) id: u16, @@ -70,7 +70,7 @@ impl PciSegment { address_manager .mmio_bus .insert( - Arc::clone(&pci_config_mmio) as Arc>, + Arc::clone(&pci_config_mmio) as Arc, mmio_config_address, layout::PCI_MMIO_CONFIG_SIZE_PER_SEGMENT, )