pci: virtio: refactor: move MsixConfig interaction to VirtioPciDevice

Following previous commits notion, move MsixConfig handling out of
PciConfiguration into VirtioPciDevice.

Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>

Author: ShadowCurse

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

@@ -246,6 +246,7 @@ pub struct VirtioPciDeviceState {
     pub pci_dev_state: VirtioPciCommonConfigState,
     pub msix_state: MsixConfigState,
     pub bars: Bars,
+    pub msix_config_offset: u16,
 }
 
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
@@ -291,6 +292,8 @@ pub struct VirtioPciDevice {
 
     // BARs region for the device
     pub bars: Bars,
+    pub msix_config_offset: u16,
+    pub msix_config: Arc<Mutex<MsixConfig>>,
 }
 
 impl Debug for VirtioPciDevice {
@@ -330,7 +333,6 @@ impl VirtioPciDevice {
             subclass,
             VIRTIO_PCI_VENDOR_ID,
             pci_device_id,
-            Some(msix_config.clone()),
         )
     }
 
@@ -410,6 +412,8 @@ impl VirtioPciDevice {
             memory,
             cap_pci_cfg_info: VirtioPciCfgCapInfo::default(),
             bars: Bars::default(),
+            msix_config,
+            msix_config_offset: 0,
         };
 
         Ok(virtio_pci_device)
@@ -426,10 +430,7 @@ impl VirtioPciDevice {
         let vectors = msix_config.vectors.clone();
         let msix_config = Arc::new(Mutex::new(msix_config));
 
-        let pci_config = PciConfiguration::type0_from_state(
-            state.pci_configuration_state,
-            Some(msix_config.clone()),
-        );
+        let pci_config = PciConfiguration::type0_from_state(state.pci_configuration_state);
         let virtio_common_config = VirtioPciCommonConfig::new(state.pci_dev_state);
         let cap_pci_cfg_info = VirtioPciCfgCapInfo {
             offset: state.cap_pci_cfg_offset,
@@ -455,6 +456,8 @@ impl VirtioPciDevice {
             memory: vm.guest_memory().clone(),
             cap_pci_cfg_info,
             bars: state.bars,
+            msix_config,
+            msix_config_offset: state.msix_config_offset,
         };
 
         if state.device_activated {
@@ -537,7 +540,10 @@ impl VirtioPciDevice {
                 VIRTIO_BAR_INDEX,
                 MSIX_PBA_BAR_OFFSET.try_into().unwrap(),
             );
-            self.configuration.add_capability(&msix_cap);
+            // The whole Configuration region is 4K, so u16 can address it all
+            #[allow(clippy::cast_possible_truncation)]
+            let offset = self.configuration.add_capability(&msix_cap) as u16;
+            self.msix_config_offset = offset;
         }
     }
 
@@ -640,6 +646,7 @@ impl VirtioPciDevice {
                 .expect("Poisoned lock")
                 .state(),
             bars: self.bars,
+            msix_config_offset: self.msix_config_offset,
         }
     }
 }
@@ -753,10 +760,18 @@ impl PciDevice for VirtioPciDevice {
             self.bars.write(bar_idx, offset, data);
             None
         } else {
-            // Handle the special case where the capability VIRTIO_PCI_CAP_PCI_CFG
-            // is accessed. This capability has a special meaning as it allows the
-            // guest to access other capabilities without mapping the PCI BAR.
+            // Handle access to the header of the MsixCapability. The rest of the
+            // capability is handled by the PciConfiguration.
             let base = reg_idx * 4;
+            if base == self.msix_config_offset as usize {
+                // offset is within a 4-byte PCI config register (0..3).
+                #[allow(clippy::cast_possible_truncation)]
+                let offset = offset as u8;
+                self.msix_config
+                    .lock()
+                    .unwrap()
+                    .write_msg_ctl_register(offset, data);
+            }
             if base + u64_to_usize(offset) >= self.cap_pci_cfg_info.offset
                 && base + u64_to_usize(offset) + data.len()
                     <= self.cap_pci_cfg_info.offset + self.cap_pci_cfg_info.cap.bytes().len()

src/vmm/src/pci/bus.rs

@@ -52,7 +52,6 @@ impl PciRoot {
                     &PciBridgeSubclass::HostBridge,
                     0,
                     0,
-                    None,
                 ),
             }
         }
@@ -492,7 +491,6 @@ mod tests {
                 &PciMassStorageSubclass::SerialScsiController,
                 0x13,
                 0x12,
-                None,
             );
             PciDevMock(config)
         }

src/vmm/src/pci/configuration.rs

@@ -5,14 +5,11 @@
 //
 // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
 
-use std::sync::{Arc, Mutex};
-
 use byteorder::{ByteOrder, LittleEndian};
 use pci::{PciCapabilityId, PciClassCode, PciSubclass};
 use serde::{Deserialize, Serialize};
 use zerocopy::{FromBytes, IntoBytes};
 
-use super::msix::MsixConfig;
 use crate::logger::warn;
 use crate::utils::u64_to_usize;
 
@@ -155,7 +152,6 @@ pub struct PciConfigurationState {
     registers: Vec<u32>,
     writable_bits: Vec<u32>,
     last_capability: Option<(usize, usize)>,
-    msix_cap_reg_idx: Option<usize>,
 }
 
 #[derive(Debug)]
@@ -168,8 +164,6 @@ pub struct PciConfiguration {
     writable_bits: [u32; NUM_CONFIGURATION_REGISTERS], // writable bits for each register.
     // Contains the byte offset and size of the last capability.
     last_capability: Option<(usize, usize)>,
-    msix_cap_reg_idx: Option<usize>,
-    msix_config: Option<Arc<Mutex<MsixConfig>>>,
 }
 
 impl PciConfiguration {
@@ -183,7 +177,6 @@ impl PciConfiguration {
         subclass: &dyn PciSubclass,
         subsystem_vendor_id: u16,
         subsystem_id: u16,
-        msix_config: Option<Arc<Mutex<MsixConfig>>>,
     ) -> Self {
         let mut registers = [0u32; NUM_CONFIGURATION_REGISTERS];
         let mut writable_bits = [0u32; NUM_CONFIGURATION_REGISTERS];
@@ -202,22 +195,15 @@ impl PciConfiguration {
             registers,
             writable_bits,
             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 {
+    pub fn type0_from_state(state: PciConfigurationState) -> Self {
         PciConfiguration {
             registers: state.registers.try_into().unwrap(),
             writable_bits: state.writable_bits.try_into().unwrap(),
             last_capability: state.last_capability,
-            msix_cap_reg_idx: state.msix_cap_reg_idx,
-            msix_config,
         }
     }
 
@@ -227,7 +213,6 @@ impl PciConfiguration {
             registers: self.registers.to_vec(),
             writable_bits: self.writable_bits.to_vec(),
             last_capability: self.last_capability,
-            msix_cap_reg_idx: self.msix_cap_reg_idx,
         }
     }
 
@@ -333,8 +318,7 @@ impl PciConfiguration {
                 self.writable_bits[cap_offset / 4] = MSI_CAPABILITY_REGISTER_MASK;
             }
             PciCapabilityId::MsiX => {
-                self.msix_cap_reg_idx = Some(cap_offset / 4);
-                self.writable_bits[self.msix_cap_reg_idx.unwrap()] = MSIX_CAPABILITY_REGISTER_MASK;
+                self.writable_bits[cap_offset / 4] = MSIX_CAPABILITY_REGISTER_MASK;
             }
             _ => {}
         }
@@ -358,26 +342,6 @@ impl PciConfiguration {
             return;
         }
 
-        // Handle potential write to MSI-X message control register
-        if let Some(msix_cap_reg_idx) = self.msix_cap_reg_idx
-            && let Some(msix_config) = &self.msix_config
-        {
-            if msix_cap_reg_idx == reg_idx && offset == 2 && data.len() == 2 {
-                // 2-bytes write in the Message Control field
-                msix_config
-                    .lock()
-                    .unwrap()
-                    .set_msg_ctl(LittleEndian::read_u16(data));
-            } else if msix_cap_reg_idx == reg_idx && offset == 0 && data.len() == 4 {
-                // 4 bytes write at the beginning. Ignore the first 2 bytes which are the
-                // capability id and next capability pointer
-                msix_config
-                    .lock()
-                    .unwrap()
-                    .set_msg_ctl((LittleEndian::read_u32(data) >> 16) as u16);
-            }
-        }
-
         match data.len() {
             1 => self.write_byte(reg_idx * 4 + u64_to_usize(offset), data[0]),
             2 => self.write_word(
@@ -592,7 +556,6 @@ mod tests {
             &PciMultimediaSubclass::AudioController,
             0xABCD,
             0x2468,
-            None,
         )
     }
 

src/vmm/src/pci/msix.rs

@@ -10,6 +10,7 @@ use byteorder::{ByteOrder, LittleEndian};
 use pci::PciCapabilityId;
 use serde::{Deserialize, Serialize};
 use vm_memory::ByteValued;
+use zerocopy::FromBytes;
 
 use crate::Vm;
 use crate::logger::{debug, error, warn};
@@ -205,6 +206,18 @@ impl MsixConfig {
         }
     }
 
+    /// Write to the Message Control register
+    pub fn write_msg_ctl_register(&mut self, offset: u8, data: &[u8]) {
+        if offset == 2 && data.len() == 2 {
+            // 2-bytes write in the Message Control field
+            self.set_msg_ctl(u16::read_from_bytes(data).unwrap());
+        } else if offset == 0 && data.len() == 4 {
+            // 4 bytes write at the beginning. Ignore the first 2 bytes which are the
+            // capability id and next capability pointer
+            self.set_msg_ctl((u32::read_from_bytes(data).unwrap() >> 16) as u16);
+        }
+    }
+
     /// Read an MSI-X table entry
     pub fn read_table(&self, offset: u64, data: &mut [u8]) {
         assert!(data.len() <= 8);