devices: Lock the BtreeMap inside to avoid deadlocks

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>

devices/src/bus.rs

@@ -10,7 +10,7 @@
 use std::cmp::{Ord, Ordering, PartialEq, PartialOrd};
 use std::collections::btree_map::BTreeMap;
 use std::result;
-use std::sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex, RwLock};
 
 /// Trait for devices that respond to reads or writes in an arbitrary address space.
 ///
@@ -79,30 +79,30 @@ impl PartialOrd for BusRange {
 ///
 /// This doesn't have any restrictions on what kind of device or address space this applies to. The
 /// only restriction is that no two devices can overlap in this address space.
-#[derive(Clone, Default)]
+#[derive(Default)]
 pub struct Bus {
-    devices: BTreeMap<BusRange, Arc<Mutex<dyn BusDevice>>>,
+    devices: RwLock<BTreeMap<BusRange, Arc<Mutex<dyn BusDevice>>>>,
 }
 
 impl Bus {
     /// Constructs an a bus with an empty address space.
     pub fn new() -> Bus {
         Bus {
-            devices: BTreeMap::new(),
+            devices: RwLock::new(BTreeMap::new()),
         }
     }
 
-    fn first_before(&self, addr: u64) -> Option<(BusRange, &Arc<Mutex<dyn BusDevice>>)> {
+    fn first_before(&self, addr: u64) -> Option<(BusRange, Arc<Mutex<dyn BusDevice>>)> {
-        let (range, dev) = self
+        let devices = self.devices.read().unwrap();
-            .devices
+        let (range, dev) = devices
             .range(..=BusRange { base: addr, len: 1 })
             .rev()
             .next()?;
-        Some((*range, dev))
+        Some((*range, dev.clone()))
     }
 
     #[allow(clippy::type_complexity)]
-    pub fn resolve(&self, addr: u64) -> Option<(u64, u64, &Arc<Mutex<dyn BusDevice>>)> {
+    pub fn resolve(&self, addr: u64) -> Option<(u64, u64, Arc<Mutex<dyn BusDevice>>)> {
         if let Some((range, dev)) = self.first_before(addr) {
             let offset = addr - range.base;
             if offset < range.len {
@@ -113,7 +113,7 @@ impl Bus {
     }
 
     /// Puts the given device at the given address space.
-    pub fn insert(&mut self, device: Arc<Mutex<dyn BusDevice>>, base: u64, len: u64) -> Result<()> {
+    pub fn insert(&self, device: Arc<Mutex<dyn BusDevice>>, base: u64, len: u64) -> Result<()> {
         if len == 0 {
             return Err(Error::ZeroSizedRange);
         }
@@ -121,6 +121,8 @@ impl Bus {
         // Reject all cases where the new device's range overlaps with an existing device.
         if self
             .devices
+            .read()
+            .unwrap()
             .iter()
             .any(|(range, _dev)| range.overlaps(base, len))
         {
@@ -129,6 +131,8 @@ impl Bus {
 
         if self
             .devices
+            .write()
+            .unwrap()
             .insert(BusRange { base, len }, device)
             .is_some()
         {
@@ -139,14 +143,14 @@ impl Bus {
     }
 
     /// Removes the device at the given address space range.
-    pub fn remove(&mut self, base: u64, len: u64) -> Result<()> {
+    pub fn remove(&self, base: u64, len: u64) -> Result<()> {
         if len == 0 {
             return Err(Error::ZeroSizedRange);
         }
 
         let bus_range = BusRange { base, len };
 
-        if self.devices.remove(&bus_range).is_none() {
+        if self.devices.write().unwrap().remove(&bus_range).is_none() {
             return Err(Error::MissingAddressRange);
         }
 
@@ -155,7 +159,7 @@ impl Bus {
 
     /// Updates the address range for an existing device.
     pub fn update_range(
-        &mut self,
+        &self,
         old_base: u64,
         old_len: u64,
         new_base: u64,
@@ -230,7 +234,7 @@ mod tests {
 
     #[test]
     fn bus_insert() {
-        let mut bus = Bus::new();
+        let bus = Bus::new();
         let dummy = Arc::new(Mutex::new(DummyDevice));
         assert!(bus.insert(dummy.clone(), 0x10, 0).is_err());
         assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok());
@@ -251,7 +255,7 @@ mod tests {
 
     #[test]
     fn bus_read_write() {
-        let mut bus = Bus::new();
+        let bus = Bus::new();
         let dummy = Arc::new(Mutex::new(DummyDevice));
         assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok());
         assert!(bus.read(0x10, &mut [0, 0, 0, 0]));
@@ -268,7 +272,7 @@ mod tests {
 
     #[test]
     fn bus_read_write_values() {
-        let mut bus = Bus::new();
+        let bus = Bus::new();
         let dummy = Arc::new(Mutex::new(ConstantDevice));
         assert!(bus.insert(dummy.clone(), 0x10, 0x10).is_ok());
 
@@ -282,7 +286,7 @@ mod tests {
     }
 
     #[test]
-    fn busrange_cmp_and_clone() {
+    fn busrange_cmp() {
         let range = BusRange { base: 0x10, len: 2 };
         assert_eq!(range, BusRange { base: 0x10, len: 3 });
         assert_eq!(range, BusRange { base: 0x10, len: 2 });
@@ -292,17 +296,14 @@ mod tests {
 
         assert_eq!(range, range.clone());
 
-        let mut bus = Bus::new();
+        let bus = Bus::new();
         let mut data = [1, 2, 3, 4];
         assert!(bus
             .insert(Arc::new(Mutex::new(DummyDevice)), 0x10, 0x10)
             .is_ok());
         assert!(bus.write(0x10, &mut data));
-        let bus_clone = bus.clone();
         assert!(bus.read(0x10, &mut data));
         assert_eq!(data, [1, 2, 3, 4]);
-        assert!(bus_clone.read(0x10, &mut data));
-        assert_eq!(data, [1, 2, 3, 4]);
     }
 
     #[test]

pci/src/bus.rs

@@ -89,8 +89,8 @@ impl PciBus {
     pub fn register_mapping(
         &self,
         dev: Arc<Mutex<dyn BusDevice>>,
-        io_bus: &mut devices::Bus,
+        io_bus: &devices::Bus,
-        mmio_bus: &mut devices::Bus,
+        mmio_bus: &devices::Bus,
         bars: Vec<(GuestAddress, GuestUsize, PciBarRegionType)>,
     ) -> Result<()> {
         for (address, size, type_) in bars {

vmm/src/device_manager.rs

@@ -168,8 +168,8 @@ pub enum DeviceManagerError {
 pub type DeviceManagerResult<T> = result::Result<T, DeviceManagerError>;
 
 struct BusInfo<'a> {
-    io: &'a mut devices::Bus,
+    io: &'a Arc<devices::Bus>,
-    mmio: &'a mut devices::Bus,
+    mmio: &'a Arc<devices::Bus>,
 }
 
 struct InterruptInfo<'a> {
@@ -276,8 +276,8 @@ impl Console {
 }
 
 pub struct DeviceManager {
-    io_bus: devices::Bus,
+    io_bus: Arc<devices::Bus>,
-    mmio_bus: devices::Bus,
+    mmio_bus: Arc<devices::Bus>,
 
     // Console abstraction
     console: Arc<Console>,
@@ -306,8 +306,8 @@ impl DeviceManager {
         _exit_evt: &EventFd,
         reset_evt: &EventFd,
     ) -> DeviceManagerResult<Self> {
-        let mut io_bus = devices::Bus::new();
+        let mut io_bus = Arc::new(devices::Bus::new());
-        let mut mmio_bus = devices::Bus::new();
+        let mut mmio_bus = Arc::new(devices::Bus::new());
 
         let mut buses = BusInfo {
             io: &mut io_bus,
@@ -1035,8 +1035,13 @@ impl DeviceManager {
                 pci.add_device(vfio_pci_device.clone())
                     .map_err(DeviceManagerError::AddPciDevice)?;
 
-                pci.register_mapping(vfio_pci_device.clone(), buses.io, buses.mmio, bars)
+                pci.register_mapping(
-                    .map_err(DeviceManagerError::AddPciDevice)?;
+                    vfio_pci_device.clone(),
+                    buses.io.as_ref(),
+                    buses.mmio.as_ref(),
+                    bars,
+                )
+                .map_err(DeviceManagerError::AddPciDevice)?;
             }
         }
         Ok(iommu_attached_device_ids)
@@ -1178,8 +1183,8 @@ impl DeviceManager {
 
         pci.register_mapping(
             virtio_pci_device.clone(),
-            &mut buses.io,
+            buses.io.as_ref(),
-            &mut buses.mmio,
+            buses.mmio.as_ref(),
             bars,
         )
         .map_err(DeviceManagerError::AddPciDevice)?;
@@ -1250,11 +1255,11 @@ impl DeviceManager {
         Ok(())
     }
 
-    pub fn io_bus(&self) -> &devices::Bus {
+    pub fn io_bus(&self) -> &Arc<devices::Bus> {
         &self.io_bus
     }
 
-    pub fn mmio_bus(&self) -> &devices::Bus {
+    pub fn mmio_bus(&self) -> &Arc<devices::Bus> {
         &self.mmio_bus
     }
 

vmm/src/vm.rs

@@ -304,8 +304,8 @@ impl CpuidPatch {
 pub struct Vcpu {
     fd: VcpuFd,
     id: u8,
-    io_bus: devices::Bus,
+    io_bus: Arc<devices::Bus>,
-    mmio_bus: devices::Bus,
+    mmio_bus: Arc<devices::Bus>,
     ioapic: Option<Arc<Mutex<ioapic::Ioapic>>>,
     vm_ts: std::time::Instant,
 }
@@ -320,8 +320,8 @@ impl Vcpu {
     pub fn new(
         id: u8,
         vm: &Vm,
-        io_bus: devices::Bus,
+        io_bus: Arc<devices::Bus>,
-        mmio_bus: devices::Bus,
+        mmio_bus: Arc<devices::Bus>,
         ioapic: Option<Arc<Mutex<ioapic::Ioapic>>>,
     ) -> Result<Self> {
         let kvm_vcpu = vm.fd.create_vcpu(id).map_err(Error::VcpuFd)?;