refactor(pci): separate functions for new and restored types

The code we inherited from Cloud Hypervisor was using a single `new`
method for creating objects both for booted and snapshot resumed VMs.
The `new` method had an optional `state` argument. When this argument
was `Some(state)` the construction of the object happened through
copying values from `state`.

Split this functionality, for various types, in two distinct methods
`new` & `from_state` which are only called for booted and snapshotted
microVMs respectively. This is similar to the pattern we're using
throughout Firecracker, reduces if/else branches, as well as the
arguments of distinct methods.

Signed-off-by: Babis Chalios <[email protected]>

Author: bchalios

src/vmm/src/devices/virtio/transport/pci/device.rs

@@ -312,7 +312,6 @@ impl VirtioPciDevice {
     fn pci_configuration(
         virtio_device_type: u32,
         msix_config: &Arc<Mutex<MsixConfig>>,
-        pci_config_state: Option<PciConfigurationState>,
     ) -> PciConfiguration {
         let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + u16::try_from(virtio_device_type).unwrap();
         let (class, subclass) = match virtio_device_type {
@@ -339,25 +338,9 @@ impl VirtioPciDevice {
             VIRTIO_PCI_VENDOR_ID,
             pci_device_id,
             Some(msix_config.clone()),
-            pci_config_state,
         )
     }
 
-    fn msix_config(
-        pci_device_bdf: u32,
-        msix_vectors: Arc<MsiVectorGroup>,
-        msix_config_state: Option<MsixConfigState>,
-    ) -> Result<Arc<Mutex<MsixConfig>>> {
-        let msix_config = Arc::new(Mutex::new(MsixConfig::new(
-            msix_vectors.num_vectors(),
-            msix_vectors,
-            pci_device_bdf,
-            msix_config_state,
-        )?));
-
-        Ok(msix_config)
-    }
-
     /// Allocate the PCI BAR for the VirtIO device and its associated capabilities.
     ///
     /// This must happen only during the creation of a brand new VM. When a VM is restored from a
@@ -399,11 +382,14 @@ impl VirtioPciDevice {
     ) -> Result<Self> {
         let num_queues = device.lock().expect("Poisoned lock").queues().len();
 
-        let msix_config = Self::msix_config(pci_device_bdf, msi_vectors.clone(), None)?;
+        let msix_config = Arc::new(Mutex::new(MsixConfig::new(
+            msi_vectors.num_vectors(),
+            msi_vectors.clone(),
+            pci_device_bdf,
+        )?));
         let pci_config = Self::pci_configuration(
             device.lock().expect("Poisoned lock").device_type(),
             &msix_config,
-            None,
         );
 
         let virtio_common_config = VirtioPciCommonConfig::new(VirtioPciCommonConfigState {
@@ -448,16 +434,15 @@ impl VirtioPciDevice {
         msi_vectors: Arc<MsiVectorGroup>,
         state: VirtioPciDeviceState,
     ) -> Result<Self> {
-        let msix_config = Self::msix_config(
+        let msix_config = Arc::new(Mutex::new(MsixConfig::from_state(
+            state.msix_state,
             state.pci_device_bdf.into(),
             msi_vectors.clone(),
-            Some(state.msix_state),
-        )?;
+        )?));
 
-        let pci_config = Self::pci_configuration(
-            device.lock().expect("Poisoned lock").device_type(),
-            &msix_config,
-            Some(state.pci_configuration_state),
+        let pci_config = PciConfiguration::type0_from_state(
+            state.pci_configuration_state,
+            Some(msix_config.clone()),
         );
         let virtio_common_config = VirtioPciCommonConfig::new(state.pci_dev_state);
         let cap_pci_cfg_info = VirtioPciCfgCapInfo {

src/vmm/src/pci/bus.rs

@@ -53,7 +53,6 @@ impl PciRoot {
                     0,
                     0,
                     None,
-                    None,
                 ),
             }
         }
@@ -500,7 +499,6 @@ mod tests {
                 0x13,
                 0x12,
                 None,
-                None,
             );
 
             config.add_pci_bar(0, 0x1000, 0x1000);

src/vmm/src/pci/configuration.rs

@@ -102,46 +102,41 @@ impl PciConfiguration {
         subsystem_vendor_id: u16,
         subsystem_id: u16,
         msix_config: Option<Arc<Mutex<MsixConfig>>>,
-        state: Option<PciConfigurationState>,
     ) -> Self {
-        let (registers, writable_bits, bars, last_capability, msix_cap_reg_idx) =
-            if let Some(state) = state {
-                (
-                    state.registers.try_into().unwrap(),
-                    state.writable_bits.try_into().unwrap(),
-                    state.bars.try_into().unwrap(),
-                    state.last_capability,
-                    state.msix_cap_reg_idx,
-                )
-            } else {
-                let mut registers = [0u32; NUM_CONFIGURATION_REGISTERS];
-                let mut writable_bits = [0u32; NUM_CONFIGURATION_REGISTERS];
-                registers[0] = (u32::from(device_id) << 16) | u32::from(vendor_id);
-                // TODO(dverkamp): Status should be write-1-to-clear
-                writable_bits[1] = 0x0000_ffff; // Status (r/o), command (r/w)
-                registers[2] = (u32::from(class_code.get_register_value()) << 24)
-                    | (u32::from(subclass.get_register_value()) << 16)
-                    | u32::from(revision_id);
-                writable_bits[3] = 0x0000_00ff; // Cacheline size (r/w)
-                registers[3] = 0x0000_0000; // Header type 0 (device)
-                writable_bits[15] = 0x0000_00ff; // IRQ line (r/w)
-                registers[11] = (u32::from(subsystem_id) << 16) | u32::from(subsystem_vendor_id);
-
-                (
-                    registers,
-                    writable_bits,
-                    [PciBar::default(); NUM_BAR_REGS],
-                    None,
-                    None,
-                )
-            };
+        let mut registers = [0u32; NUM_CONFIGURATION_REGISTERS];
+        let mut writable_bits = [0u32; NUM_CONFIGURATION_REGISTERS];
+        registers[0] = (u32::from(device_id) << 16) | u32::from(vendor_id);
+        // TODO(dverkamp): Status should be write-1-to-clear
+        writable_bits[1] = 0x0000_ffff; // Status (r/o), command (r/w)
+        registers[2] = (u32::from(class_code.get_register_value()) << 24)
+            | (u32::from(subclass.get_register_value()) << 16)
+            | u32::from(revision_id);
+        writable_bits[3] = 0x0000_00ff; // Cacheline size (r/w)
+        registers[3] = 0x0000_0000; // Header type 0 (device)
+        writable_bits[15] = 0x0000_00ff; // IRQ line (r/w)
+        registers[11] = (u32::from(subsystem_id) << 16) | u32::from(subsystem_vendor_id);
 
         PciConfiguration {
             registers,
             writable_bits,
-            bars,
-            last_capability,
-            msix_cap_reg_idx,
+            bars: [PciBar::default(); NUM_BAR_REGS],
+            last_capability: None,
+            msix_cap_reg_idx: None,
+            msix_config,
+        }
+    }
+
+    /// Create a type 0 PCI configuration from snapshot state
+    pub fn type0_from_state(
+        state: PciConfigurationState,
+        msix_config: Option<Arc<Mutex<MsixConfig>>>,
+    ) -> Self {
+        PciConfiguration {
+            registers: state.registers.try_into().unwrap(),
+            writable_bits: state.writable_bits.try_into().unwrap(),
+            bars: state.bars.try_into().unwrap(),
+            last_capability: state.last_capability,
+            msix_cap_reg_idx: state.msix_cap_reg_idx,
             msix_config,
         }
     }
@@ -654,7 +649,6 @@ mod tests {
             0xABCD,
             0x2468,
             None,
-            None,
         )
     }