pci: Refine pci topology

PciConfigIo is a legacy pci bus dispatcher, which manages all pci
devices including a pci root bridge. However, it is unnecessary to
design a complex hierarchy which redirects every access by PciRoot.

Since pci root bridge is also a pci device instance, and only contains
easy config space read/write, and PciConfigIo actually acts as a pci bus
to dispatch resource based resolving when VMExit, we re-arrange to make
the pci hierarchy clean.

Signed-off-by: Jing Liu <jing2.liu@linux.intel.com>

pci/src/bus.rs

@@ -26,25 +26,20 @@ pub enum PciRootError {
 }
 pub type Result<T> = std::result::Result<T, PciRootError>;
 
-/// Emulates the PCI Root bridge.
+/// Emulates the PCI Root bridge device.
 pub struct PciRoot {
-    /// Bus configuration for the root device.
-    configuration: PciConfiguration,
-    /// Devices attached to this bridge.
-    devices: Vec<Arc<Mutex<dyn PciDevice>>>,
+    /// Configuration space.
+    config: PciConfiguration,
 }
 
 impl PciRoot {
-    /// Create an empty PCI root bus.
-    pub fn new(configuration: Option<PciConfiguration>) -> Self {
-        if let Some(config) = configuration {
-            PciRoot {
-                configuration: config,
-                devices: Vec::new(),
-            }
+    /// Create an empty PCI root bridge.
+    pub fn new(config: Option<PciConfiguration>) -> Self {
+        if let Some(config) = config {
+            PciRoot { config }
         } else {
             PciRoot {
-                configuration: PciConfiguration::new(
+                config: PciConfiguration::new(
                     VENDOR_ID_INTEL,
                     DEVICE_ID_INTEL_VIRT_PCIE_HOST,
                     PciClassCode::BridgeDevice,
@@ -55,126 +50,102 @@ impl PciRoot {
                     0,
                     None,
                 ),
-                devices: Vec::new(),
-            }
-        }
-    }
-
-    /// Add a `device` to this root PCI bus.
-    pub fn add_device(&mut self, pci_device: Arc<Mutex<dyn PciDevice>>) -> Result<()> {
-        self.devices.push(pci_device);
-        Ok(())
-    }
-
-    /// Register Guest Address mapping of a `device` to IO bus.
-    pub fn register_mapping(
-        &self,
-        device: Arc<Mutex<dyn BusDevice>>,
-        bus: &mut devices::Bus,
-        bars: Vec<(GuestAddress, GuestUsize)>,
-    ) -> Result<()> {
-        for (address, size) in bars {
-            bus.insert(device.clone(), address.raw_value(), size)
-                .map_err(PciRootError::MmioInsert)?;
-        }
-        Ok(())
-    }
-    pub fn config_space_read(
-        &self,
-        bus: usize,
-        device: usize,
-        _function: usize,
-        register: usize,
-    ) -> u32 {
-        // Only support one bus.
-        if bus != 0 {
-            return 0xffff_ffff;
-        }
-
-        match device {
-            0 => {
-                // If bus and device are both zero, then read from the root config.
-                self.configuration.read_config_register(register)
-            }
-            dev_num => self.devices.get(dev_num - 1).map_or(0xffff_ffff, |d| {
-                d.lock().unwrap().read_config_register(register)
-            }),
-        }
-    }
-
-    pub fn config_space_write(
-        &mut self,
-        bus: usize,
-        device: usize,
-        _function: usize,
-        register: usize,
-        offset: u64,
-        data: &[u8],
-    ) {
-        if offset as usize + data.len() > 4 {
-            return;
-        }
-
-        // Only support one bus.
-        if bus != 0 {
-            return;
-        }
-
-        match device {
-            0 => {
-                // If bus and device are both zero, then read from the root config.
-                self.configuration
-                    .write_config_register(register, offset, data);
-            }
-            dev_num => {
-                if let Some(d) = self.devices.get(dev_num - 1) {
-                    d.lock()
-                        .unwrap()
-                        .write_config_register(register, offset, data);
-                }
             }
         }
     }
 }
 
-/// Emulates PCI configuration access mechanism #1 (I/O ports 0xcf8 and 0xcfc).
+impl BusDevice for PciRoot {}
+
+impl PciDevice for PciRoot {
+    fn write_config_register(&mut self, reg_idx: usize, offset: u64, data: &[u8]) {
+        self.config.write_config_register(reg_idx, offset, data);
+    }
+
+    fn read_config_register(&self, reg_idx: usize) -> u32 {
+        self.config.read_reg(reg_idx)
+    }
+}
+
 pub struct PciConfigIo {
-    /// PCI root bridge.
-    pci_root: PciRoot,
-    /// Current address to read/write from (0xcf8 register, litte endian).
+    /// Devices attached to this bus.
+    /// Device 0 is host bridge.
+    devices: Vec<Arc<Mutex<dyn PciDevice>>>,
+    /// Config space register.
     config_address: u32,
 }
 
 impl PciConfigIo {
     pub fn new(pci_root: PciRoot) -> Self {
+        let mut devices: Vec<Arc<Mutex<dyn PciDevice>>> = Vec::new();
+        devices.push(Arc::new(Mutex::new(pci_root)));
+
         PciConfigIo {
-            pci_root,
+            devices,
             config_address: 0,
         }
     }
 
-    fn config_space_read(&self) -> u32 {
+    pub fn register_mapping(
+        &self,
+        dev: Arc<Mutex<dyn BusDevice>>,
+        bus: &mut devices::Bus,
+        bars: Vec<(GuestAddress, GuestUsize)>,
+    ) -> Result<()> {
+        for (address, size) in bars {
+            bus.insert(dev.clone(), address.raw_value(), size)
+                .map_err(PciRootError::MmioInsert)?;
+        }
+        Ok(())
+    }
+
+    pub fn add_device(&mut self, device: Arc<Mutex<dyn PciDevice>>) -> Result<()> {
+        self.devices.push(device);
+        Ok(())
+    }
+
+    pub fn config_space_read(&self) -> u32 {
         let enabled = (self.config_address & 0x8000_0000) != 0;
         if !enabled {
             return 0xffff_ffff;
         }
 
-        let (bus, device, function, register) =
+        let (bus, device, _function, register) =
             parse_config_address(self.config_address & !0x8000_0000);
-        self.pci_root
-            .config_space_read(bus, device, function, register)
+
+        // Only support one bus.
+        if bus != 0 {
+            return 0xffff_ffff;
+        }
+
+        self.devices.get(device).map_or(0xffff_ffff, |d| {
+            d.lock().unwrap().read_config_register(register)
+        })
     }
 
-    fn config_space_write(&mut self, offset: u64, data: &[u8]) {
+    pub fn config_space_write(&mut self, offset: u64, data: &[u8]) {
+        if offset as usize + data.len() > 4 {
+            return;
+        }
+
         let enabled = (self.config_address & 0x8000_0000) != 0;
         if !enabled {
             return;
         }
 
-        let (bus, device, function, register) =
+        let (bus, device, _function, register) =
             parse_config_address(self.config_address & !0x8000_0000);
-        self.pci_root
-            .config_space_write(bus, device, function, register, offset, data)
+
+        // Only support one bus.
+        if bus != 0 {
+            return;
+        }
+
+        if let Some(d) = self.devices.get(device) {
+            d.lock()
+                .unwrap()
+                .write_config_register(register, offset, data);
+        }
     }
 
     fn set_config_address(&mut self, offset: u64, data: &[u8]) {
@@ -232,25 +203,49 @@ impl BusDevice for PciConfigIo {
 
 /// Emulates PCI memory-mapped configuration access mechanism.
 pub struct PciConfigMmio {
-    /// PCI root bridge.
-    pci_root: PciRoot,
+    /// Devices attached to this bus.
+    /// Device 0 is host bridge.
+    devices: Vec<Arc<Mutex<dyn PciDevice>>>,
 }
 
 impl PciConfigMmio {
     pub fn new(pci_root: PciRoot) -> Self {
-        PciConfigMmio { pci_root }
+        let mut devices: Vec<Arc<Mutex<dyn PciDevice>>> = Vec::new();
+
+        devices.push(Arc::new(Mutex::new(pci_root)));
+        PciConfigMmio { devices }
     }
 
     fn config_space_read(&self, config_address: u32) -> u32 {
-        let (bus, device, function, register) = parse_config_address(config_address);
-        self.pci_root
-            .config_space_read(bus, device, function, register)
+        let (bus, device, _function, register) = parse_config_address(config_address);
+
+        // Only support one bus.
+        if bus != 0 {
+            return 0xffff_ffff;
+        }
+
+        self.devices.get(device).map_or(0xffff_ffff, |d| {
+            d.lock().unwrap().read_config_register(register)
+        })
     }
 
     fn config_space_write(&mut self, config_address: u32, offset: u64, data: &[u8]) {
-        let (bus, device, function, register) = parse_config_address(config_address);
-        self.pci_root
-            .config_space_write(bus, device, function, register, offset, data)
+        if offset as usize + data.len() > 4 {
+            return;
+        }
+
+        let (bus, device, _function, register) = parse_config_address(config_address);
+
+        // Only support one bus.
+        if bus != 0 {
+            return;
+        }
+
+        if let Some(d) = self.devices.get(device) {
+            d.lock()
+                .unwrap()
+                .write_config_register(register, offset, data);
+        }
     }
 }
 

pci/src/device.rs

@@ -90,11 +90,11 @@ pub trait PciDevice: BusDevice {
     /// Reads from a BAR region mapped in to the device.
     /// * `addr` - The guest address inside the BAR.
     /// * `data` - Filled with the data from `addr`.
-    fn read_bar(&mut self, base: u64, offset: u64, data: &mut [u8]);
+    fn read_bar(&mut self, _base: u64, _offset: u64, _data: &mut [u8]) {}
     /// Writes to a BAR region mapped in to the device.
     /// * `addr` - The guest address inside the BAR.
     /// * `data` - The data to write.
-    fn write_bar(&mut self, base: u64, offset: u64, data: &[u8]);
+    fn write_bar(&mut self, _base: u64, _offset: u64, _data: &[u8]) {}
     /// Invoked when the device is sandboxed.
     fn on_device_sandboxed(&mut self) {}
 }

pci/src/lib.rs

@@ -10,20 +10,21 @@ extern crate kvm_ioctls;
 extern crate vm_memory;
 extern crate vmm_sys_util;
 
+mod bus;
 mod configuration;
 mod device;
 mod msix;
-mod root;
 
+pub use self::bus::{PciConfigIo, PciConfigMmio, PciRoot, PciRootError};
 pub use self::configuration::{
     PciBarConfiguration, PciBarPrefetchable, PciBarRegionType, PciCapability, PciCapabilityID,
     PciClassCode, PciConfiguration, PciHeaderType, PciMassStorageSubclass,
     PciNetworkControllerSubclass, PciProgrammingInterface, PciSerialBusSubClass, PciSubclass,
 };
-pub use self::device::Error as PciDeviceError;
-pub use self::device::{InterruptDelivery, InterruptParameters, PciDevice};
+pub use self::device::{
+    Error as PciDeviceError, InterruptDelivery, InterruptParameters, PciDevice,
+};
 pub use self::msix::{MsixCap, MsixConfig, MsixTableEntry};
-pub use self::root::{PciConfigIo, PciConfigMmio, PciRoot, PciRootError};
 
 /// PCI has four interrupt pins A->D.
 #[derive(Copy, Clone)]

vmm/src/vm.rs

@@ -525,7 +525,8 @@ impl DeviceManager {
             exit_evt.try_clone().map_err(DeviceManagerError::EventFd)?,
         )));
 
-        let mut pci_root = PciRoot::new(None);
+        let pci_root = PciRoot::new(None);
+        let mut pci = PciConfigIo::new(pci_root);
 
         for disk_cfg in &vm_cfg.disks {
             // Open block device path
@@ -559,7 +560,7 @@ impl DeviceManager {
                 memory.clone(),
                 allocator,
                 vm_fd,
-                &mut pci_root,
+                &mut pci,
                 &mut mmio_bus,
                 &interrupt_info,
             )?;
@@ -584,7 +585,7 @@ impl DeviceManager {
                 memory.clone(),
                 allocator,
                 vm_fd,
-                &mut pci_root,
+                &mut pci,
                 &mut mmio_bus,
                 &interrupt_info,
             )?;
@@ -600,7 +601,7 @@ impl DeviceManager {
                 memory.clone(),
                 allocator,
                 vm_fd,
-                &mut pci_root,
+                &mut pci,
                 &mut mmio_bus,
                 &interrupt_info,
             )?;
@@ -623,7 +624,7 @@ impl DeviceManager {
                         memory.clone(),
                         allocator,
                         vm_fd,
-                        &mut pci_root,
+                        &mut pci,
                         &mut mmio_bus,
                         &interrupt_info,
                     )?;
@@ -688,14 +689,14 @@ impl DeviceManager {
                     memory.clone(),
                     allocator,
                     vm_fd,
-                    &mut pci_root,
+                    &mut pci,
                     &mut mmio_bus,
                     &interrupt_info,
                 )?;
             }
         }
 
-        let pci = Arc::new(Mutex::new(PciConfigIo::new(pci_root)));
+        let pci = Arc::new(Mutex::new(pci));
 
         Ok(DeviceManager {
             io_bus,
@@ -713,7 +714,7 @@ impl DeviceManager {
         memory: GuestMemoryMmap,
         allocator: &mut SystemAllocator,
         vm_fd: &Arc<VmFd>,
-        pci_root: &mut PciRoot,
+        pci: &mut PciConfigIo,
         mmio_bus: &mut devices::Bus,
         interrupt_info: &InterruptInfo,
     ) -> DeviceManagerResult<()> {
@@ -804,12 +805,10 @@ impl DeviceManager {
 
         let virtio_pci_device = Arc::new(Mutex::new(virtio_pci_device));
 
-        pci_root
-            .add_device(virtio_pci_device.clone())
+        pci.add_device(virtio_pci_device.clone())
             .map_err(DeviceManagerError::AddPciDevice)?;
 
-        pci_root
-            .register_mapping(virtio_pci_device.clone(), mmio_bus, bars)
+        pci.register_mapping(virtio_pci_device.clone(), mmio_bus, bars)
             .map_err(DeviceManagerError::AddPciDevice)?;
 
         Ok(())