refactor(pci): simplify MSI-X interrupts implementation

Our implementation of MSI-X was taken from the equivalent
Cloud Hypervisor one. Cloud Hypervisor supports non-KVM hypervisors,
hence they introduced a few abstractions to cater for that. More
specifically, they are using a trait (`InterruptSourceGroup`) to
describe the underlying hypervisor-specific mechanism used to drive
MSI-X interrupts from the device to the guest.

Moreover, they use a couple of types to distinguish between MSI, MSI-X
and legacy IRQ interrupts and how these can be configured. This is
especially useful for MSI/MSI-X interrupts where the interrupt
configuration is provided by the guest over PCI bus MMIO accesses.
Finally, in order to allow for transparent error handling, `Result`
types returned by related trait methods was returning `std::io::Error`
types as the error type, which is not very intuitive.

We don't need all of that. We clearly distinguish between legacy
interrupts and MSI-X (we don't use at all MSI), so we can simply plug in
all the definitions directly where we need them without any dynamic
dispatch indirection.

Moreover, various MSI-X components were stored all over the place
creating a lot of unnecessary duplication.

This commit refactors our implementation to get rid of the unnecessary
bit of dynamic dispatch. We still keep the `VirtioInterrupt` trait which
we use to abstract the actual interrupt type (IRQ or MSI-X) from VirtIO
devices. Moreover, it removes the duplication of MSI-X related types
across various types. We only duplicate the MSI-X configuration type
(which is behind an Arc) in a few places to avoid needing to
unnecessarily take a lock(). Moreover, we introduce a single interrupt
related error type and get rid of the `std::io::Error` usage.

Finally, it introduces a couple of metrics for interrupts that are
useful information by themselves, but also help us maintain some unit
testing assertions for which we were relying on a mock type that
implemented `InterruptSourceGroup`.

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

Author: bchalios

src/vmm/src/device_manager/pci_mngr.rs

@@ -34,8 +34,8 @@ use crate::pci::bus::PciRootError;
 use crate::resources::VmResources;
 use crate::snapshot::Persist;
 use crate::vmm_config::mmds::MmdsConfigError;
+use crate::vstate::interrupts::InterruptError;
 use crate::vstate::memory::GuestMemoryMmap;
-use crate::vstate::vm::{InterruptError, MsiVectorGroup};
 use crate::{EventManager, Vm};
 
 #[derive(Debug, Default)]
@@ -121,11 +121,16 @@ impl PciDevices {
         let msix_num =
             u16::try_from(device.lock().expect("Poisoned lock").queues().len() + 1).unwrap();
 
-        let msix_vectors = Arc::new(Vm::create_msix_group(vm.clone(), msix_num)?);
+        let msix_vectors = Vm::create_msix_group(vm.clone(), msix_num)?;
 
         // Create the transport
-        let mut virtio_device =
-            VirtioPciDevice::new(id.clone(), mem, device, msix_vectors, pci_device_bdf.into())?;
+        let mut virtio_device = VirtioPciDevice::new(
+            id.clone(),
+            mem,
+            device,
+            Arc::new(msix_vectors),
+            pci_device_bdf.into(),
+        )?;
 
         // Allocate bars
         let mut resource_allocator_lock = vm.resource_allocator();
@@ -162,17 +167,12 @@ impl PciDevices {
     ) -> Result<(), PciManagerError> {
         // We should only be reaching this point if PCI is enabled
         let pci_segment = self.pci_segment.as_ref().unwrap();
-        let msi_vector_group = Arc::new(MsiVectorGroup::restore(
-            vm.clone(),
-            &transport_state.msi_vector_group,
-        )?);
         let device_type: u32 = device.lock().expect("Poisoned lock").device_type();
 
         let virtio_device = Arc::new(Mutex::new(VirtioPciDevice::new_from_state(
             device_id.to_string(),
-            vm.guest_memory().clone(),
+            vm,
             device.clone(),
-            msi_vector_group,
             transport_state.clone(),
         )?));
 

src/vmm/src/devices/mod.rs

@@ -23,6 +23,7 @@ use crate::devices::virtio::net::metrics::NetDeviceMetrics;
 use crate::devices::virtio::queue::{InvalidAvailIdx, QueueError};
 use crate::devices::virtio::vsock::VsockError;
 use crate::logger::IncMetric;
+use crate::vstate::interrupts::InterruptError;
 
 // Function used for reporting error in terms of logging
 // but also in terms of metrics of net event fails.
@@ -41,7 +42,7 @@ pub enum DeviceError {
     /// Failed to read from the TAP device.
     FailedReadTap,
     /// Failed to signal irq: {0}
-    FailedSignalingIrq(io::Error),
+    FailedSignalingIrq(#[from] InterruptError),
     /// IO error: {0}
     IoError(io::Error),
     /// Device received malformed payload.

src/vmm/src/devices/virtio/balloon/mod.rs

@@ -17,6 +17,7 @@ use super::queue::{InvalidAvailIdx, QueueError};
 use crate::devices::virtio::balloon::metrics::METRICS;
 use crate::devices::virtio::queue::FIRECRACKER_MAX_QUEUE_SIZE;
 use crate::logger::IncMetric;
+use crate::vstate::interrupts::InterruptError;
 
 /// Device ID used in MMIO device identification.
 /// Because Balloon is unique per-vm, this ID can be hardcoded.
@@ -72,7 +73,7 @@ pub enum BalloonError {
     /// EventFd error: {0}
     EventFd(std::io::Error),
     /// Received error while sending an interrupt: {0}
-    InterruptError(std::io::Error),
+    InterruptError(InterruptError),
     /// Guest gave us a malformed descriptor.
     MalformedDescriptor,
     /// Guest gave us a malformed payload.

src/vmm/src/devices/virtio/block/vhost_user/mod.rs

@@ -7,6 +7,7 @@ pub mod persist;
 
 use self::device::VhostUserBlock;
 use crate::devices::virtio::vhost_user::VhostUserError;
+use crate::vstate::interrupts::InterruptError;
 
 /// Number of queues for the vhost-user block device.
 pub const NUM_QUEUES: u64 = 1;
@@ -28,5 +29,5 @@ pub enum VhostUserBlockError {
     /// Error opening eventfd: {0}
     EventFd(std::io::Error),
     /// Error creating irqfd: {0}
-    Interrupt(std::io::Error),
+    Interrupt(InterruptError),
 }

src/vmm/src/devices/virtio/transport/mmio.rs

@@ -15,8 +15,9 @@ use super::{VirtioInterrupt, VirtioInterruptType};
 use crate::devices::virtio::device::VirtioDevice;
 use crate::devices::virtio::device_status;
 use crate::devices::virtio::queue::Queue;
-use crate::logger::{error, warn};
+use crate::logger::{IncMetric, METRICS, error, warn};
 use crate::utils::byte_order;
+use crate::vstate::interrupts::InterruptError;
 use crate::vstate::memory::{GuestAddress, GuestMemoryMmap};
 
 // TODO crosvm uses 0 here, but IIRC virtio specified some other vendor id that should be used
@@ -399,17 +400,19 @@ impl Default for IrqTrigger {
 }
 
 impl VirtioInterrupt for IrqTrigger {
-    fn trigger(&self, interrupt_type: VirtioInterruptType) -> Result<(), std::io::Error> {
+    fn trigger(&self, interrupt_type: VirtioInterruptType) -> Result<(), InterruptError> {
+        METRICS.interrupts.triggers.inc();
         match interrupt_type {
             VirtioInterruptType::Config => self.trigger_irq(IrqType::Config),
             VirtioInterruptType::Queue(_) => self.trigger_irq(IrqType::Vring),
         }
     }
 
-    fn trigger_queues(&self, queues: &[u16]) -> Result<(), std::io::Error> {
+    fn trigger_queues(&self, queues: &[u16]) -> Result<(), InterruptError> {
         if queues.is_empty() {
             Ok(())
         } else {
+            METRICS.interrupts.triggers.inc();
             self.trigger_irq(IrqType::Vring)
         }
     }
@@ -449,7 +452,7 @@ impl IrqTrigger {
         }
     }
 
-    fn trigger_irq(&self, irq_type: IrqType) -> Result<(), std::io::Error> {
+    fn trigger_irq(&self, irq_type: IrqType) -> Result<(), InterruptError> {
         let irq = match irq_type {
             IrqType::Config => VIRTIO_MMIO_INT_CONFIG,
             IrqType::Vring => VIRTIO_MMIO_INT_VRING,

src/vmm/src/devices/virtio/transport/mod.rs

@@ -6,6 +6,8 @@ use std::sync::atomic::AtomicU32;
 
 use vmm_sys_util::eventfd::EventFd;
 
+use crate::vstate::interrupts::InterruptError;
+
 /// MMIO transport for VirtIO devices
 pub mod mmio;
 /// PCI transport for VirtIO devices
@@ -23,15 +25,15 @@ pub enum VirtioInterruptType {
 /// API of interrupt types used by VirtIO devices
 pub trait VirtioInterrupt: std::fmt::Debug + Send + Sync {
     /// Trigger a VirtIO interrupt.
-    fn trigger(&self, interrupt_type: VirtioInterruptType) -> Result<(), std::io::Error>;
+    fn trigger(&self, interrupt_type: VirtioInterruptType) -> Result<(), InterruptError>;
 
     /// Trigger multiple Virtio interrupts for selected queues.
     /// The caller needs to ensure that [`queues`] does not include duplicate entries to
     /// avoid sending multiple interrupts for the same queue.
     /// This is to allow sending a single interrupt for implementations that don't
     /// distinguish different queues, like IrqTrigger, instead of sending multiple same
     /// interrupts.
-    fn trigger_queues(&self, queues: &[u16]) -> Result<(), std::io::Error> {
+    fn trigger_queues(&self, queues: &[u16]) -> Result<(), InterruptError> {
         queues
             .iter()
             .try_for_each(|&qidx| self.trigger(VirtioInterruptType::Queue(qidx)))