Msix assertions

src/pci/src/bus.rs

@@ -291,8 +291,8 @@ impl PciConfigIo {
                 u32::from(data[0]) << (offset * 8),
             ),
             2 => (
-                0x0000_ffff << (offset * 16),
-                ((u32::from(data[1]) << 8) | u32::from(data[0])) << (offset * 16),
+                0x0000_ffff << (offset * 8),
+                ((u32::from(data[1]) << 8) | u32::from(data[0])) << (offset * 8),
             ),
             4 => (0xffff_ffff, LittleEndian::read_u32(data)),
             _ => return,
@@ -475,3 +475,119 @@ fn parse_io_config_address(config_address: u32) -> (usize, usize, usize, usize)
         shift_and_mask(config_address, REGISTER_NUMBER_OFFSET, REGISTER_NUMBER_MASK),
     )
 }
+
+#[cfg(test)]
+mod tests {
+    use std::sync::{Arc, Mutex};
+
+    use vm_device::BusDevice;
+
+    use super::{PciBus, PciConfigIo, PciRoot};
+    use crate::DeviceRelocation;
+
+    struct RelocationMock;
+
+    impl DeviceRelocation for RelocationMock {
+        fn move_bar(
+            &self,
+            _old_base: u64,
+            _new_base: u64,
+            _len: u64,
+            _pci_dev: &mut dyn crate::PciDevice,
+            _region_type: crate::PciBarRegionType,
+        ) -> std::result::Result<(), std::io::Error> {
+            Ok(())
+        }
+    }
+
+    #[test]
+    fn test_writing_config_address() {
+        let mock = Arc::new(RelocationMock);
+        let root = PciRoot::new(None);
+        let mut bus = PciConfigIo::new(Arc::new(Mutex::new(PciBus::new(root, mock))));
+
+        assert_eq!(bus.config_address, 0);
+        // Writing more than 32 bits will should fail
+        bus.write(0, 0, &[0x42; 8]);
+        assert_eq!(bus.config_address, 0);
+        // Write all the address at once
+        bus.write(0, 0, &[0x13, 0x12, 0x11, 0x10]);
+        assert_eq!(bus.config_address, 0x10111213);
+        // Not writing 32bits at offset 0 should have no effect
+        bus.write(0, 1, &[0x0; 4]);
+        assert_eq!(bus.config_address, 0x10111213);
+
+        // Write two bytes at a time
+        bus.write(0, 0, &[0x42, 0x42]);
+        assert_eq!(bus.config_address, 0x10114242);
+        bus.write(0, 1, &[0x43, 0x43]);
+        assert_eq!(bus.config_address, 0x10434342);
+        bus.write(0, 2, &[0x44, 0x44]);
+        assert_eq!(bus.config_address, 0x44444342);
+        // Writing two bytes at offset 3 should overflow, so it shouldn't have any effect
+        bus.write(0, 3, &[0x45, 0x45]);
+        assert_eq!(bus.config_address, 0x44444342);
+
+        // Write one byte at a time
+        bus.write(0, 0, &[0x0]);
+        assert_eq!(bus.config_address, 0x44444300);
+        bus.write(0, 1, &[0x0]);
+        assert_eq!(bus.config_address, 0x44440000);
+        bus.write(0, 2, &[0x0]);
+        assert_eq!(bus.config_address, 0x44000000);
+        bus.write(0, 3, &[0x0]);
+        assert_eq!(bus.config_address, 0x00000000);
+        // Writing past 4 bytes should have no effect
+        bus.write(0, 4, &[0x13]);
+        assert_eq!(bus.config_address, 0x0);
+    }
+
+    #[test]
+    fn test_reading_config_address() {
+        let mock = Arc::new(RelocationMock);
+        let root = PciRoot::new(None);
+        let mut bus = PciConfigIo::new(Arc::new(Mutex::new(PciBus::new(root, mock))));
+
+        let mut buffer = [0u8; 4];
+
+        bus.config_address = 0x13121110;
+
+        // First 4 bytes are the config address
+        // Next 4 bytes are the values read from the configuration space.
+        //
+        // Reading past offset 7 should not return nothing (all 1s)
+        bus.read(0, 8, &mut buffer);
+        assert_eq!(buffer, [0xff; 4]);
+
+        // offset + buffer.len() needs to be smaller or equal than 4
+        bus.read(0, 1, &mut buffer);
+        assert_eq!(buffer, [0xff; 4]);
+        bus.read(0, 2, &mut buffer[..3]);
+        assert_eq!(buffer, [0xff; 4]);
+        bus.read(0, 3, &mut buffer[..2]);
+        assert_eq!(buffer, [0xff; 4]);
+
+        // reading one byte at a time
+        bus.read(0, 0, &mut buffer[0..1]);
+        assert_eq!(buffer, [0x10, 0xff, 0xff, 0xff]);
+        bus.read(0, 1, &mut buffer[1..2]);
+        assert_eq!(buffer, [0x10, 0x11, 0xff, 0xff]);
+        bus.read(0, 2, &mut buffer[2..3]);
+        assert_eq!(buffer, [0x10, 0x11, 0x12, 0xff]);
+        bus.read(0, 3, &mut buffer[3..4]);
+        assert_eq!(buffer, [0x10, 0x11, 0x12, 0x13]);
+
+        // reading two bytes at a time
+        bus.config_address = 0x42434445;
+        bus.read(0, 0, &mut buffer[..2]);
+        assert_eq!(buffer, [0x45, 0x44, 0x12, 0x13]);
+        bus.read(0, 1, &mut buffer[..2]);
+        assert_eq!(buffer, [0x44, 0x43, 0x12, 0x13]);
+        bus.read(0, 2, &mut buffer[..2]);
+        assert_eq!(buffer, [0x43, 0x42, 0x12, 0x13]);
+
+        // reading all of it at once
+        bus.read(0, 0, &mut buffer);
+        assert_eq!(buffer, [0x45, 0x44, 0x43, 0x42]);
+    }
+}

src/pci/src/msix.rs

@@ -219,7 +219,7 @@ impl MsixConfig {
     }
 
     pub fn read_table(&self, offset: u64, data: &mut [u8]) {
-        assert!((data.len() == 4 || data.len() == 8));
+        assert!(data.len() <= 8);
 
         let index: usize = (offset / MSIX_TABLE_ENTRIES_MODULO) as usize;
         let modulo_offset = offset % MSIX_TABLE_ENTRIES_MODULO;
@@ -272,7 +272,7 @@ impl MsixConfig {
     }
 
     pub fn write_table(&mut self, offset: u64, data: &[u8]) {
-        assert!((data.len() == 4 || data.len() == 8));
+        assert!(data.len() <= 8);
 
         let index: usize = (offset / MSIX_TABLE_ENTRIES_MODULO) as usize;
         let modulo_offset = offset % MSIX_TABLE_ENTRIES_MODULO;
@@ -368,7 +368,7 @@ impl MsixConfig {
     }
 
     pub fn read_pba(&mut self, offset: u64, data: &mut [u8]) {
-        assert!((data.len() == 4 || data.len() == 8));
+        assert!(data.len() <= 8);
 
         let index: usize = (offset / MSIX_PBA_ENTRIES_MODULO) as usize;
         let modulo_offset = offset % MSIX_PBA_ENTRIES_MODULO;

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

@@ -230,7 +230,19 @@ impl VirtioPciCommonConfig {
             0x12 => queues.len().try_into().unwrap(), // num_queues
             0x16 => self.queue_select,
             0x18 => self.with_queue(queues, |q| q.size).unwrap_or(0),
-            0x1a => self.msix_queues.lock().unwrap()[self.queue_select as usize],
+            // If `queue_select` points to an invalid queue we should return NO_VECTOR.
+            // Reading from here
+            // https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-1280005:
+            //
+            // > The device MUST return vector mapped to a given event, (NO_VECTOR if unmapped) on
+            // > read of config_msix_vector/queue_msix_vector.
+            0x1a => self
+                .msix_queues
+                .lock()
+                .unwrap()
+                .get(self.queue_select as usize)
+                .copied()
+                .unwrap_or(0xffff),
             0x1c => u16::from(self.with_queue(queues, |q| q.ready).unwrap_or(false)),
             0x1e => self.queue_select, // notify_off
             _ => {
@@ -408,8 +420,13 @@ mod tests {
         // 'queue_select' can be read and written.
         regs.write(0x16, &[0xaa, 0x55], dev.clone());
         let mut read_back = vec![0x00, 0x00];
-        regs.read(0x16, &mut read_back, dev);
+        regs.read(0x16, &mut read_back, dev.clone());
         assert_eq!(read_back[0], 0xaa);
         assert_eq!(read_back[1], 0x55);
+
+        // Getting the MSI vector when `queue_select` points to an invalid queue should return
+        // NO_VECTOR (0xffff)
+        regs.read(0x1a, &mut read_back, dev);
+        assert_eq!(read_back, [0xff, 0xff]);
     }
 }

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

@@ -305,7 +305,7 @@ pub struct VirtioPciDeviceState {
     pub pci_configuration_state: PciConfigurationState,
     pub pci_dev_state: VirtioPciCommonConfigState,
     pub msix_state: MsixConfigState,
-    pub msi_vector_group: HashMap<u32, u32>,
+    pub msi_vector_group: Vec<u32>,
     pub bar_configuration: Vec<PciBarConfiguration>,
 }
 

src/vmm/src/vstate/vm.rs

@@ -28,8 +28,8 @@ use vm_device::interrupt::{
 use vmm_sys_util::errno;
 use vmm_sys_util::eventfd::EventFd;
 
-use crate::arch::host_page_size;
 pub use crate::arch::{ArchVm as Vm, ArchVmError, VmState};
+use crate::arch::{GSI_MSI_END, host_page_size};
 use crate::logger::info;
 use crate::persist::CreateSnapshotError;
 use crate::snapshot::Persist;
@@ -110,7 +110,7 @@ impl MsiVector {
 /// MSI interrupts created for a VirtIO device
 pub struct MsiVectorGroup {
     vm: Arc<Vm>,
-    irq_routes: HashMap<u32, MsiVector>,
+    irq_routes: Vec<MsiVector>,
 }
 
 impl MsiVectorGroup {
@@ -123,28 +123,25 @@ impl MsiVectorGroup {
 }
 
 impl<'a> Persist<'a> for MsiVectorGroup {
-    type State = HashMap<u32, u32>;
+    type State = Vec<u32>;
     type ConstructorArgs = Arc<Vm>;
     type Error = InterruptError;
 
     fn save(&self) -> Self::State {
         // We don't save the "enabled" state of the MSI interrupt. PCI devices store the MSI-X
         // configuration and make sure that the vector is enabled during the restore path if it was
         // initially enabled
-        self.irq_routes
-            .iter()
-            .map(|(id, route)| (*id, route.gsi))
-            .collect()
+        self.irq_routes.iter().map(|route| route.gsi).collect()
     }
 
     fn restore(
         constructor_args: Self::ConstructorArgs,
         state: &Self::State,
     ) -> std::result::Result<Self, Self::Error> {
-        let mut irq_routes = HashMap::new();
+        let mut irq_routes = Vec::with_capacity(state.len());
 
-        for (id, gsi) in state {
-            irq_routes.insert(*id, MsiVector::new(*gsi, false)?);
+        for gsi in state {
+            irq_routes.push(MsiVector::new(*gsi, false)?);
         }
 
         Ok(MsiVectorGroup {
@@ -156,15 +153,15 @@ impl<'a> Persist<'a> for MsiVectorGroup {
 
 impl InterruptSourceGroup for MsiVectorGroup {
     fn enable(&self) -> vm_device::interrupt::Result<()> {
-        for route in self.irq_routes.values() {
+        for route in &self.irq_routes {
             route.enable(&self.vm.common.fd)?;
         }
 
         Ok(())
     }
 
     fn disable(&self) -> vm_device::interrupt::Result<()> {
-        for route in self.irq_routes.values() {
+        for route in &self.irq_routes {
             route.disable(&self.vm.common.fd)?;
         }
 
@@ -180,7 +177,9 @@ impl InterruptSourceGroup for MsiVectorGroup {
     }
 
     fn notifier(&self, index: InterruptIndex) -> Option<&EventFd> {
-        self.irq_routes.get(&index).map(|route| &route.event_fd)
+        self.irq_routes
+            .get(index as usize)
+            .map(|route| &route.event_fd)
     }
 
     fn update(
@@ -199,7 +198,7 @@ impl InterruptSourceGroup for MsiVectorGroup {
             InterruptSourceConfig::MsiIrq(config) => config,
         };
 
-        if let Some(route) = self.irq_routes.get(&index) {
+        if let Some(route) = self.irq_routes.get(index as usize) {
             // When an interrupt is masked the GSI will not be passed to KVM through
             // KVM_SET_GSI_ROUTING. So, call [`disable()`] to unregister the interrupt file
             // descriptor before passing the interrupt routes to KVM
@@ -323,7 +322,7 @@ impl Vm {
             fd,
             max_memslots: kvm.max_nr_memslots(),
             guest_memory: GuestMemoryMmap::default(),
-            interrupts: Mutex::new(HashMap::new()),
+            interrupts: Mutex::new(HashMap::with_capacity(GSI_MSI_END as usize + 1)),
             resource_allocator: Mutex::new(ResourceAllocator::new()),
             mmio_bus: Arc::new(vm_device::Bus::new()),
         })
@@ -593,14 +592,13 @@ impl Vm {
     /// Create a group of MSI-X interrupts
     pub fn create_msix_group(vm: Arc<Vm>, count: u16) -> Result<MsiVectorGroup, InterruptError> {
         debug!("Creating new MSI group with {count} vectors");
-        let mut irq_routes = HashMap::with_capacity(count as usize);
-        for (gsi, i) in vm
+        let mut irq_routes = Vec::with_capacity(count as usize);
+        for gsi in vm
             .resource_allocator()
             .allocate_gsi_msi(count as u32)?
             .iter()
-            .zip(0u32..)
         {
-            irq_routes.insert(i, MsiVector::new(*gsi, false)?);
+            irq_routes.push(MsiVector::new(*gsi, false)?);
         }
 
         Ok(MsiVectorGroup { vm, irq_routes })
@@ -821,21 +819,21 @@ pub(crate) mod tests {
         let msix_group = create_msix_group(&vm);
 
         // Initially all vectors are disabled
-        for route in msix_group.irq_routes.values() {
+        for route in &msix_group.irq_routes {
             assert!(!route.enabled.load(Ordering::Acquire))
         }
 
         // Enable works
         msix_group.enable().unwrap();
-        for route in msix_group.irq_routes.values() {
+        for route in &msix_group.irq_routes {
             assert!(route.enabled.load(Ordering::Acquire));
         }
         // Enabling an enabled group doesn't error out
         msix_group.enable().unwrap();
 
         // Disable works
         msix_group.disable().unwrap();
-        for route in msix_group.irq_routes.values() {
+        for route in &msix_group.irq_routes {
             assert!(!route.enabled.load(Ordering::Acquire))
         }
         // Disabling a disabled group doesn't error out
@@ -921,7 +919,7 @@ pub(crate) mod tests {
         }
 
         // All vectors should be disabled
-        for vector in msix_group.irq_routes.values() {
+        for vector in &msix_group.irq_routes {
             assert!(!vector.enabled.load(Ordering::Acquire));
         }
 
@@ -1018,8 +1016,8 @@ pub(crate) mod tests {
         // Even if an MSI group is enabled, we don't save it as such. During restoration, the PCI
         // transport will make sure the correct config is set for the vectors and enable them
         // accordingly.
-        for (id, vector) in msix_group.irq_routes {
-            let new_vector = restored_group.irq_routes.get(&id).unwrap();
+        for (id, vector) in msix_group.irq_routes.iter().enumerate() {
+            let new_vector = &restored_group.irq_routes[id];
             assert_eq!(vector.gsi, new_vector.gsi);
             assert!(!new_vector.enabled.load(Ordering::Acquire));
         }