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

@@ -233,6 +233,7 @@ pub struct VirtioPciDeviceState {
     pub pci_dev_state: VirtioPciCommonConfigState,
     pub msix_state: MsixConfigState,
     pub bars: Bars,
+    pub msix_config_cap_offset: u16,
 }
 
 #[derive(Debug, thiserror::Error, displaydoc::Display)]
@@ -280,6 +281,8 @@ pub struct VirtioPciDevice {
 
     // BARs region for the device
     pub bars: Bars,
+    pub msix_config_cap_offset: u16,
+    pub msix_config: Arc<Mutex<MsixConfig>>,
 }
 
 impl Debug for VirtioPciDevice {
@@ -319,7 +322,6 @@ impl VirtioPciDevice {
             subclass,
             VIRTIO_PCI_VENDOR_ID,
             pci_device_id,
-            Some(msix_config.clone()),
         )
     }
 
@@ -396,6 +398,8 @@ impl VirtioPciDevice {
             memory,
             cap_pci_cfg_info: VirtioPciCfgCapInfo::default(),
             bars: Bars::default(),
+            msix_config,
+            msix_config_cap_offset: 0,
         };
 
         Ok(virtio_pci_device)
@@ -411,10 +415,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,
@@ -442,6 +443,8 @@ impl VirtioPciDevice {
             memory: vm.guest_memory().clone(),
             cap_pci_cfg_info,
             bars: state.bars,
+            msix_config,
+            msix_config_cap_offset: state.msix_config_cap_offset,
         };
 
         if state.device_activated {
@@ -523,7 +526,10 @@ impl VirtioPciDevice {
                 VIRTIO_BAR_INDEX,
                 MSIX_PBA_BAR_OFFSET,
             );
-            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_cap_offset = offset;
         }
     }
 
@@ -626,6 +632,7 @@ impl VirtioPciDevice {
                 .expect("Poisoned lock")
                 .state(),
             bars: self.bars,
+            msix_config_cap_offset: self.msix_config_cap_offset,
         }
     }
 }
@@ -736,16 +743,21 @@ 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.
-            let base = reg_idx as usize * 4;
-            if base + offset as usize >= self.cap_pci_cfg_info.offset as usize
-                && base + offset as usize + data.len()
+            // 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_cap_offset {
+                self.msix_config
+                    .lock()
+                    .unwrap()
+                    .write_msg_ctl_register(offset, data);
+            }
+            if base + u16::from(offset) >= self.cap_pci_cfg_info.offset
+                && (base + u16::from(offset)) as usize + data.len()
                     <= self.cap_pci_cfg_info.offset as usize
                         + self.cap_pci_cfg_info.cap.bytes().len()
             {
-                let offset = base + offset as usize - self.cap_pci_cfg_info.offset as usize;
+                let offset = (base + u16::from(offset) - self.cap_pci_cfg_info.offset) as usize;
                 self.write_cap_pci_cfg(offset, data)
             } else {
                 self.configuration

src/vmm/src/pci/bus.rs

@@ -51,7 +51,6 @@ impl PciRoot {
                     PciBridgeSubclass::HostBridge as u8,
                     0,
                     0,
-                    None,
                 ),
             }
         }
@@ -518,7 +517,6 @@ mod tests {
                 PciMassStorageSubclass::SerialScsiController as u8,
                 0x13,
                 0x12,
-                None,
             );
             PciDevMock(config)
         }

src/vmm/src/pci/configuration.rs

@@ -5,13 +5,10 @@
 //
 // SPDX-License-Identifier: Apache-2.0 AND BSD-3-Clause
 
-use std::sync::{Arc, Mutex};
-
 use byteorder::{ByteOrder, LittleEndian};
 use serde::{Deserialize, Serialize};
 use zerocopy::{FromBytes, IntoBytes};
 
-use super::msix::MsixConfig;
 use crate::logger::warn;
 use crate::pci::{PciCapabilityId, PciClassCode};
 
@@ -175,7 +172,6 @@ pub struct PciConfigurationState {
     registers: Vec<u32>,
     writable_bits: Vec<u32>,
     last_capability: Option<(u8, u8)>,
-    msix_cap_reg_idx: Option<u16>,
 }
 
 #[derive(Debug)]
@@ -188,8 +184,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<(u8, u8)>,
-    msix_cap_reg_idx: Option<u16>,
-    msix_config: Option<Arc<Mutex<MsixConfig>>>,
 }
 
 impl PciConfiguration {
@@ -203,7 +197,6 @@ impl PciConfiguration {
         subclass: u8,
         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];
@@ -222,16 +215,11 @@ 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>>>,
-    ) -> Result<Self, PciConfigurationError> {
+    pub fn type0_from_state(state: PciConfigurationState) -> Result<Self, PciConfigurationError> {
         let reg_len = state.registers.len();
         let registers = state
             .registers
@@ -248,8 +236,6 @@ impl PciConfiguration {
             registers,
             writable_bits,
             last_capability: state.last_capability,
-            msix_cap_reg_idx: state.msix_cap_reg_idx,
-            msix_config,
         })
     }
 
@@ -259,7 +245,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,
         }
     }
 
@@ -373,9 +358,7 @@ impl PciConfiguration {
                 self.writable_bits[(cap_offset / 4) as usize] = MSI_CAPABILITY_REGISTER_MASK;
             }
             PciCapabilityId::MsiX => {
-                self.msix_cap_reg_idx = Some(u16::from(cap_offset) / 4);
-                self.writable_bits[self.msix_cap_reg_idx.unwrap() as usize] =
-                    MSIX_CAPABILITY_REGISTER_MASK;
+                self.writable_bits[(cap_offset / 4) as usize] = MSIX_CAPABILITY_REGISTER_MASK;
             }
             _ => {}
         }
@@ -399,26 +382,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 + offset as u16, data[0]),
             2 => self.write_word(
@@ -636,7 +599,6 @@ mod tests {
             PciMultimediaSubclass::AudioController as u8,
             0xABCD,
             0x2468,
-            None,
         )
     }
 

src/vmm/src/pci/msix.rs

@@ -9,6 +9,7 @@ use std::sync::Arc;
 use byteorder::{ByteOrder, LittleEndian};
 use serde::{Deserialize, Serialize};
 use vm_memory::ByteValued;
+use zerocopy::FromBytes;
 
 use crate::Vm;
 use crate::logger::{debug, error, warn};
@@ -209,6 +210,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);