pci: Fix the PciBus using HashMap instead of Vec

By using a Vec to hold the list of devices on the PciBus, there's a
problem when we use unplug. Indeed, the vector of devices gets reduced
and if the unplugged device was not the last one from the list, every
other device after this one is shifted on the bus.

To solve this problem, a HashMap is used. This allows to keep track of
the exact place where each device stands on the bus.

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

pci/src/bus.rs

@@ -10,6 +10,7 @@ use byteorder::{ByteOrder, LittleEndian};
 use devices::BusDevice;
 use std;
 use std::any::Any;
+use std::collections::HashMap;
 use std::ops::DerefMut;
 use std::sync::{Arc, Mutex};
 use vm_memory::{Address, GuestAddress, GuestUsize};
@@ -84,17 +85,17 @@ impl PciDevice for PciRoot {
 pub struct PciBus {
     /// Devices attached to this bus.
     /// Device 0 is host bridge.
-    devices: Vec<Arc<Mutex<dyn PciDevice>>>,
+    devices: HashMap<u32, Arc<Mutex<dyn PciDevice>>>,
     device_reloc: Arc<dyn DeviceRelocation>,
     device_ids: Vec<bool>,
 }
 
 impl PciBus {
     pub fn new(pci_root: PciRoot, device_reloc: Arc<dyn DeviceRelocation>) -> Self {
-        let mut devices: Vec<Arc<Mutex<dyn PciDevice>>> = Vec::new();
+        let mut devices: HashMap<u32, Arc<Mutex<dyn PciDevice>>> = HashMap::new();
         let mut device_ids: Vec<bool> = vec![false; NUM_DEVICE_IDS];
 
-        devices.push(Arc::new(Mutex::new(pci_root)));
+        devices.insert(0, Arc::new(Mutex::new(pci_root)));
         device_ids[0] = true;
 
         PciBus {
@@ -128,13 +129,17 @@ impl PciBus {
         Ok(())
     }
 
-    pub fn add_device(&mut self, device: Arc<Mutex<dyn PciDevice>>) -> Result<()> {
-        self.devices.push(device);
+    pub fn add_device(
+        &mut self,
+        pci_device_bdf: u32,
+        device: Arc<Mutex<dyn PciDevice>>,
+    ) -> Result<()> {
+        self.devices.insert(pci_device_bdf >> 3, device);
         Ok(())
     }
 
     pub fn remove_by_device(&mut self, device: &Arc<Mutex<dyn PciDevice>>) -> Result<()> {
-        self.devices.retain(|dev| !Arc::ptr_eq(dev, device));
+        self.devices.retain(|_, dev| !Arc::ptr_eq(dev, device));
         Ok(())
     }
 
@@ -196,7 +201,7 @@ impl PciConfigIo {
             .lock()
             .unwrap()
             .devices
-            .get(device)
+            .get(&(device as u32))
             .map_or(0xffff_ffff, |d| {
                 d.lock().unwrap().read_config_register(register)
             })
@@ -221,7 +226,7 @@ impl PciConfigIo {
         }
 
         let pci_bus = self.pci_bus.lock().unwrap();
-        if let Some(d) = pci_bus.devices.get(device) {
+        if let Some(d) = pci_bus.devices.get(&(device as u32)) {
             let mut device = d.lock().unwrap();
 
             // Find out if one of the device's BAR is being reprogrammed, and
@@ -318,7 +323,7 @@ impl PciConfigMmio {
             .lock()
             .unwrap()
             .devices
-            .get(device)
+            .get(&(device as u32))
             .map_or(0xffff_ffff, |d| {
                 d.lock().unwrap().read_config_register(register)
             })
@@ -337,7 +342,7 @@ impl PciConfigMmio {
         }
 
         let pci_bus = self.pci_bus.lock().unwrap();
-        if let Some(d) = pci_bus.devices.get(device) {
+        if let Some(d) = pci_bus.devices.get(&(device as u32)) {
             let mut device = d.lock().unwrap();
 
             // Find out if one of the device's BAR is being reprogrammed, and

vmm/src/device_manager.rs

@@ -1539,7 +1539,7 @@ impl DeviceManager {
 
         let vfio_pci_device = Arc::new(Mutex::new(vfio_pci_device));
 
-        pci.add_device(vfio_pci_device.clone())
+        pci.add_device(pci_device_bdf, vfio_pci_device.clone())
             .map_err(DeviceManagerError::AddPciDevice)?;
 
         self.pci_devices.insert(
@@ -1620,7 +1620,7 @@ impl DeviceManager {
         // to the PCI function, and we know we don't do multifunction.
         // Also, because we only support one PCI bus, the bus 0, we don't need
         // to add anything to the global device ID.
-        let dev_id = pci
+        let pci_device_bdf = pci
             .next_device_id()
             .map_err(DeviceManagerError::NextPciDeviceId)?
             << 3;
@@ -1632,12 +1632,12 @@ impl DeviceManager {
             if let Some(mapping) = iommu_mapping {
                 let mapping_clone = mapping.clone();
                 Some(Arc::new(Box::new(move |addr: u64| {
-                    mapping_clone.translate(dev_id, addr).map_err(|e| {
+                    mapping_clone.translate(pci_device_bdf, addr).map_err(|e| {
                         std::io::Error::new(
                             std::io::ErrorKind::Other,
                             format!(
                                 "failed to translate addr 0x{:x} for device 00:{:02x}.0 {}",
-                                addr, dev_id, e
+                                addr, pci_device_bdf, e
                             ),
                         )
                     })
@@ -1672,7 +1672,7 @@ impl DeviceManager {
 
         let virtio_pci_device = Arc::new(Mutex::new(virtio_pci_device));
 
-        pci.add_device(virtio_pci_device.clone())
+        pci.add_device(pci_device_bdf, virtio_pci_device.clone())
             .map_err(DeviceManagerError::AddPciDevice)?;
 
         self.bus_devices
@@ -1690,7 +1690,7 @@ impl DeviceManager {
             .push(Arc::clone(&virtio_pci_device) as Arc<Mutex<dyn Migratable>>);
 
         let ret = if iommu_mapping.is_some() {
-            Some(dev_id)
+            Some(pci_device_bdf)
         } else {
             None
         };