From 8d785bbd5f63c41475a059ff9030cee9dc34915a Mon Sep 17 00:00:00 2001 From: Sebastien Boeuf Date: Thu, 12 Mar 2020 18:12:06 +0100 Subject: [PATCH] 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 --- pci/src/bus.rs | 25 +++++++++++++++---------- vmm/src/device_manager.rs | 12 ++++++------ 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/pci/src/bus.rs b/pci/src/bus.rs index e5c423e18..2909c4a45 100644 --- a/pci/src/bus.rs +++ b/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>>, + devices: HashMap>>, device_reloc: Arc, device_ids: Vec, } impl PciBus { pub fn new(pci_root: PciRoot, device_reloc: Arc) -> Self { - let mut devices: Vec>> = Vec::new(); + let mut devices: HashMap>> = HashMap::new(); let mut device_ids: Vec = 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>) -> Result<()> { - self.devices.push(device); + pub fn add_device( + &mut self, + pci_device_bdf: u32, + device: Arc>, + ) -> Result<()> { + self.devices.insert(pci_device_bdf >> 3, device); Ok(()) } pub fn remove_by_device(&mut self, device: &Arc>) -> 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 diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index e12f57a26..14cdbab62 100644 --- a/vmm/src/device_manager.rs +++ b/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>); let ret = if iommu_mapping.is_some() { - Some(dev_id) + Some(pci_device_bdf) } else { None };