vm-device: interrupt: Remove InterruptType dependencies and definitions

Having the InterruptManager trait depend on an InterruptType forces
implementations into supporting potentially very different kind of
interrupts from the same code base. What we're defining through the
current, interrupt type based create_group() method is a need for having
different interrupt managers for different kind of interrupts.

By associating the InterruptManager trait to an interrupt group
configuration type, we create a cleaner design to support that need as
we're basically saying that one interrupt manager should have the single
responsibility of supporting one kind of interrupt (defined through its
configuration).

Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>

devices/src/ioapic.rs

@@ -16,7 +16,7 @@ use std::result;
 use std::sync::Arc;
 use vm_device::interrupt::{
     InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup,
-    MsiIrqSourceConfig, PCI_MSI_IRQ,
+    MsiIrqGroupConfig, MsiIrqSourceConfig,
 };
 use vm_memory::GuestAddress;
 
@@ -211,14 +211,13 @@ impl BusDevice for Ioapic {
 impl Ioapic {
     pub fn new(
         apic_address: GuestAddress,
-        interrupt_manager: Arc<dyn InterruptManager>,
+        interrupt_manager: Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> Result<Ioapic> {
         let interrupt_source_group = interrupt_manager
-            .create_group(
+            .create_group(MsiIrqGroupConfig {
-                PCI_MSI_IRQ,
+                base: 0 as InterruptIndex,
-                0 as InterruptIndex,
+                count: NUM_IOAPIC_PINS as InterruptIndex,
-                NUM_IOAPIC_PINS as InterruptIndex,
+            })
-            )
             .map_err(Error::CreateInterruptSourceGroup)?;
 
         interrupt_source_group

vfio/src/vfio_pci.rs

@@ -24,7 +24,9 @@ use std::sync::Arc;
 use std::{fmt, io, result};
 use vfio_bindings::bindings::vfio::*;
 use vm_allocator::SystemAllocator;
-use vm_device::interrupt::{InterruptIndex, InterruptManager, InterruptSourceGroup, PCI_MSI_IRQ};
+use vm_device::interrupt::{
+    InterruptIndex, InterruptManager, InterruptSourceGroup, MsiIrqGroupConfig,
+};
 use vm_memory::{Address, GuestAddress, GuestUsize};
 use vmm_sys_util::eventfd::EventFd;
 
@@ -286,7 +288,7 @@ impl VfioPciDevice {
     pub fn new(
         vm_fd: &Arc<VmFd>,
         device: VfioDevice,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> Result<Self> {
         let device = Arc::new(device);
         device.reset();
@@ -322,7 +324,11 @@ impl VfioPciDevice {
         Ok(vfio_pci_device)
     }
 
-    fn parse_msix_capabilities(&mut self, cap: u8, interrupt_manager: &Arc<dyn InterruptManager>) {
+    fn parse_msix_capabilities(
+        &mut self,
+        cap: u8,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
+    ) {
         let msg_ctl = self
             .vfio_pci_configuration
             .read_config_word((cap + 2).into());
@@ -342,7 +348,10 @@ impl VfioPciDevice {
         };
 
         let interrupt_source_group = interrupt_manager
-            .create_group(PCI_MSI_IRQ, 0, msix_cap.table_size() as InterruptIndex)
+            .create_group(MsiIrqGroupConfig {
+                base: 0,
+                count: msix_cap.table_size() as InterruptIndex,
+            })
             .unwrap();
 
         let msix_config = MsixConfig::new(msix_cap.table_size(), interrupt_source_group.clone());
@@ -355,17 +364,20 @@ impl VfioPciDevice {
         });
     }
 
-    fn parse_msi_capabilities(&mut self, cap: u8, interrupt_manager: &Arc<dyn InterruptManager>) {
+    fn parse_msi_capabilities(
+        &mut self,
+        cap: u8,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
+    ) {
         let msg_ctl = self
             .vfio_pci_configuration
             .read_config_word((cap + 2).into());
 
         let interrupt_source_group = interrupt_manager
-            .create_group(
+            .create_group(MsiIrqGroupConfig {
-                PCI_MSI_IRQ,
+                base: 0,
-                0,
+                count: msi_num_enabled_vectors(msg_ctl) as InterruptIndex,
-                msi_num_enabled_vectors(msg_ctl) as InterruptIndex,
+            })
-            )
             .unwrap();
 
         let msi_config = MsiConfig::new(msg_ctl, interrupt_source_group.clone());
@@ -377,7 +389,10 @@ impl VfioPciDevice {
         });
     }
 
-    fn parse_capabilities(&mut self, interrupt_manager: &Arc<dyn InterruptManager>) {
+    fn parse_capabilities(
+        &mut self,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
+    ) {
         let mut cap_next = self
             .vfio_pci_configuration
             .read_config_byte(PCI_CONFIG_CAPABILITY_OFFSET);

vm-device/src/interrupt/mod.rs

@@ -66,12 +66,6 @@ pub type Result<T> = std::io::Result<T>;
 /// Data type to store an interrupt source identifier.
 pub type InterruptIndex = u32;
 
-/// Data type to store an interrupt source type.
-///
-/// The interrupt source type is a slim wrapper so that the `InterruptManager`
-/// can be implemented in external, non rust-vmm crates.
-pub type InterruptType = u32;
-
 /// Configuration data for legacy interrupts.
 ///
 /// On x86 platforms, legacy interrupts means those interrupts routed through PICs or IOAPICs.
@@ -100,14 +94,33 @@ pub enum InterruptSourceConfig {
     MsiIrq(MsiIrqSourceConfig),
 }
 
-pub const PIN_IRQ: InterruptType = 0;
+/// Configuration data for legacy, pin based interrupt groups.
-pub const PCI_MSI_IRQ: InterruptType = 1;
+///
+/// A legacy interrupt group only takes one irq number as its configuration.
+#[derive(Copy, Clone, Debug)]
+pub struct LegacyIrqGroupConfig {
+    /// Legacy irq number.
+    pub irq: InterruptIndex,
+}
+
+/// Configuration data for MSI/MSI-X interrupt groups
+///
+/// MSI/MSI-X interrupt groups are basically a set of vectors.
+#[derive(Copy, Clone, Debug)]
+pub struct MsiIrqGroupConfig {
+    /// First index of the MSI/MSI-X interrupt vectors
+    pub base: InterruptIndex,
+    /// Number of vectors in the MSI/MSI-X group.
+    pub count: InterruptIndex,
+}
 
 /// Trait to manage interrupt sources for virtual device backends.
 ///
 /// The InterruptManager implementations should protect itself from concurrent accesses internally,
 /// so it could be invoked from multi-threaded context.
 pub trait InterruptManager {
+    type GroupConfig;
+
     /// Create an [InterruptSourceGroup](trait.InterruptSourceGroup.html) object to manage
     /// interrupt sources for a virtual device
     ///
@@ -118,12 +131,8 @@ pub trait InterruptManager {
     /// * interrupt_type: type of interrupt source.
     /// * base: base Interrupt Source ID to be managed by the group object.
     /// * count: number of Interrupt Sources to be managed by the group object.
-    fn create_group(
+    fn create_group(&self, config: Self::GroupConfig)
-        &self,
+        -> Result<Arc<Box<dyn InterruptSourceGroup>>>;
-        interrupt_type: InterruptType,
-        base: InterruptIndex,
-        count: InterruptIndex,
-    ) -> Result<Arc<Box<dyn InterruptSourceGroup>>>;
 
     /// Destroy an [InterruptSourceGroup](trait.InterruptSourceGroup.html) object created by
     /// [create_group()](trait.InterruptManager.html#tymethod.create_group).

vm-virtio/src/transport/pci_device.rs

@@ -35,7 +35,9 @@ use std::result;
 use std::sync::atomic::{AtomicU16, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use vm_allocator::SystemAllocator;
-use vm_device::interrupt::{InterruptIndex, InterruptManager, InterruptSourceGroup, PCI_MSI_IRQ};
+use vm_device::interrupt::{
+    InterruptIndex, InterruptManager, InterruptSourceGroup, MsiIrqGroupConfig,
+};
 use vm_device::{Migratable, MigratableError, Pausable, Snapshotable};
 use vm_memory::{Address, ByteValued, GuestAddress, GuestMemoryMmap, GuestUsize, Le32};
 use vmm_sys_util::{errno::Result, eventfd::EventFd};
@@ -305,7 +307,7 @@ impl VirtioPciDevice {
         device: Arc<Mutex<dyn VirtioDevice>>,
         msix_num: u16,
         iommu_mapping_cb: Option<Arc<VirtioIommuRemapping>>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> Result<Self> {
         let device_clone = device.clone();
         let locked_device = device_clone.lock().unwrap();
@@ -325,8 +327,10 @@ impl VirtioPciDevice {
 
         let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + locked_device.device_type() as u16;
 
-        let interrupt_source_group =
+        let interrupt_source_group = interrupt_manager.create_group(MsiIrqGroupConfig {
-            interrupt_manager.create_group(PCI_MSI_IRQ, 0, msix_num as InterruptIndex)?;
+            base: 0,
+            count: msix_num as InterruptIndex,
+        })?;
 
         let (msix_config, msix_config_clone) = if msix_num > 0 {
             let msix_config = Arc::new(Mutex::new(MsixConfig::new(

vmm/src/device_manager.rs

@@ -42,8 +42,9 @@ use std::sync::{Arc, Mutex};
 #[cfg(feature = "pci_support")]
 use vfio::{VfioDevice, VfioDmaMapping, VfioPciDevice, VfioPciError};
 use vm_allocator::SystemAllocator;
-use vm_device::interrupt::InterruptManager;
+use vm_device::interrupt::{
-use vm_device::interrupt::{InterruptIndex, PIN_IRQ};
+    InterruptIndex, InterruptManager, LegacyIrqGroupConfig, MsiIrqGroupConfig,
+};
 use vm_device::{Migratable, MigratableError, Pausable, Snapshotable};
 use vm_memory::guest_memory::FileOffset;
 use vm_memory::{Address, GuestAddress, GuestUsize, MmapRegion};
@@ -436,7 +437,7 @@ impl DeviceManager {
         // and then the legacy interrupt manager needs an IOAPIC. So we're
         // handling a linear dependency chain:
         // msi_interrupt_manager <- IOAPIC <- legacy_interrupt_manager.
-        let msi_interrupt_manager: Arc<dyn InterruptManager> =
+        let msi_interrupt_manager: Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>> =
             Arc::new(KvmMsiInterruptManager::new(
                 Arc::clone(&address_manager.allocator),
                 vm_fd,
@@ -448,8 +449,9 @@ impl DeviceManager {
 
         // Now we can create the legacy interrupt manager, which needs the freshly
         // formed IOAPIC device.
-        let legacy_interrupt_manager: Arc<dyn InterruptManager> =
+        let legacy_interrupt_manager: Arc<
-            Arc::new(KvmLegacyUserspaceInterruptManager::new(ioapic.clone()));
+            dyn InterruptManager<GroupConfig = LegacyIrqGroupConfig>,
+        > = Arc::new(KvmLegacyUserspaceInterruptManager::new(ioapic.clone()));
 
         #[cfg(feature = "acpi")]
         address_manager
@@ -509,7 +511,7 @@ impl DeviceManager {
     fn add_pci_devices(
         &mut self,
         virtio_devices: Vec<(Arc<Mutex<dyn vm_virtio::VirtioDevice>>, bool)>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> DeviceManagerResult<()> {
         #[cfg(feature = "pci_support")]
         {
@@ -587,7 +589,7 @@ impl DeviceManager {
     fn add_mmio_devices(
         &mut self,
         virtio_devices: Vec<(Arc<Mutex<dyn vm_virtio::VirtioDevice>>, bool)>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = LegacyIrqGroupConfig>>,
     ) -> DeviceManagerResult<()> {
         #[cfg(feature = "mmio_support")]
         {
@@ -611,7 +613,7 @@ impl DeviceManager {
 
     fn add_ioapic(
         address_manager: &Arc<AddressManager>,
-        interrupt_manager: Arc<dyn InterruptManager>,
+        interrupt_manager: Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> DeviceManagerResult<Arc<Mutex<ioapic::Ioapic>>> {
         // Create IOAPIC
         let ioapic = Arc::new(Mutex::new(
@@ -630,7 +632,7 @@ impl DeviceManager {
     #[cfg(feature = "acpi")]
     fn add_acpi_devices(
         &mut self,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = LegacyIrqGroupConfig>>,
         reset_evt: EventFd,
         exit_evt: EventFd,
     ) -> DeviceManagerResult<Option<Arc<Mutex<devices::AcpiGEDDevice>>>> {
@@ -659,7 +661,9 @@ impl DeviceManager {
             .unwrap();
 
         let interrupt_group = interrupt_manager
-            .create_group(PIN_IRQ, ged_irq as InterruptIndex, 1 as InterruptIndex)
+            .create_group(LegacyIrqGroupConfig {
+                irq: ged_irq as InterruptIndex,
+            })
             .map_err(DeviceManagerError::CreateInterruptGroup)?;
 
         let ged_device = Arc::new(Mutex::new(devices::AcpiGEDDevice::new(
@@ -721,7 +725,7 @@ impl DeviceManager {
 
     fn add_console_device(
         &mut self,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = LegacyIrqGroupConfig>>,
         virtio_devices: &mut Vec<(Arc<Mutex<dyn vm_virtio::VirtioDevice>>, bool)>,
     ) -> DeviceManagerResult<Arc<Console>> {
         let serial_config = self.config.lock().unwrap().serial.clone();
@@ -738,7 +742,9 @@ impl DeviceManager {
             let serial_irq = 4;
 
             let interrupt_group = interrupt_manager
-                .create_group(PIN_IRQ, serial_irq as InterruptIndex, 1 as InterruptIndex)
+                .create_group(LegacyIrqGroupConfig {
+                    irq: serial_irq as InterruptIndex,
+                })
                 .map_err(DeviceManagerError::CreateInterruptGroup)?;
 
             let serial = Arc::new(Mutex::new(devices::legacy::Serial::new(
@@ -1262,7 +1268,7 @@ impl DeviceManager {
         &mut self,
         pci: &mut PciBus,
         iommu_device: &mut Option<vm_virtio::Iommu>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> DeviceManagerResult<Vec<u32>> {
         let mut mem_slot = self
             .memory_manager
@@ -1340,7 +1346,7 @@ impl DeviceManager {
         virtio_device: Arc<Mutex<dyn vm_virtio::VirtioDevice>>,
         pci: &mut PciBus,
         iommu_mapping: &Option<Arc<IommuMapping>>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = MsiIrqGroupConfig>>,
     ) -> DeviceManagerResult<Option<u32>> {
         // Allows support for one MSI-X vector per queue. It also adds 1
         // as we need to take into account the dedicated vector to notify
@@ -1427,7 +1433,7 @@ impl DeviceManager {
     fn add_virtio_mmio_device(
         &mut self,
         virtio_device: Arc<Mutex<dyn vm_virtio::VirtioDevice>>,
-        interrupt_manager: &Arc<dyn InterruptManager>,
+        interrupt_manager: &Arc<dyn InterruptManager<GroupConfig = LegacyIrqGroupConfig>>,
         mmio_base: GuestAddress,
     ) -> DeviceManagerResult<()> {
         let memory = self.memory_manager.lock().unwrap().guest_memory();
@@ -1451,7 +1457,9 @@ impl DeviceManager {
             .ok_or(DeviceManagerError::AllocateIrq)?;
 
         let interrupt_group = interrupt_manager
-            .create_group(PIN_IRQ, irq_num as InterruptIndex, 1 as InterruptIndex)
+            .create_group(LegacyIrqGroupConfig {
+                irq: irq_num as InterruptIndex,
+            })
             .map_err(DeviceManagerError::CreateInterruptGroup)?;
 
         mmio_device.assign_interrupt(interrupt_group);

vmm/src/interrupt.rs

@@ -12,8 +12,8 @@ use std::mem::size_of;
 use std::sync::{Arc, Mutex};
 use vm_allocator::SystemAllocator;
 use vm_device::interrupt::{
-    InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup, InterruptType,
+    InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup,
-    PCI_MSI_IRQ, PIN_IRQ,
+    LegacyIrqGroupConfig, MsiIrqGroupConfig,
 };
 use vmm_sys_util::eventfd::EventFd;
 
@@ -303,35 +303,16 @@ impl KvmMsiInterruptManager {
 }
 
 impl InterruptManager for KvmLegacyUserspaceInterruptManager {
+    type GroupConfig = LegacyIrqGroupConfig;
+
     fn create_group(
         &self,
-        interrupt_type: InterruptType,
+        config: Self::GroupConfig,
-        base: InterruptIndex,
-        count: InterruptIndex,
     ) -> Result<Arc<Box<dyn InterruptSourceGroup>>> {
-        let interrupt_source_group: Arc<Box<dyn InterruptSourceGroup>> = match interrupt_type {
+        Ok(Arc::new(Box::new(LegacyUserspaceInterruptGroup::new(
-            PIN_IRQ => {
+            self.ioapic.clone(),
-                if count > 1 {
+            config.irq as u32,
-                    return Err(io::Error::new(
+        ))))
-                        io::ErrorKind::Other,
-                        "Legacy cannot support more than one interrupt",
-                    ));
-                }
-
-                Arc::new(Box::new(LegacyUserspaceInterruptGroup::new(
-                    self.ioapic.clone(),
-                    base as u32,
-                )))
-            }
-            _ => {
-                return Err(io::Error::new(
-                    io::ErrorKind::Other,
-                    "Interrupt type not supported",
-                ))
-            }
-        };
-
-        Ok(interrupt_source_group)
     }
 
     fn destroy_group(&self, _group: Arc<Box<dyn InterruptSourceGroup>>) -> Result<()> {
@@ -340,36 +321,24 @@ impl InterruptManager for KvmLegacyUserspaceInterruptManager {
 }
 
 impl InterruptManager for KvmMsiInterruptManager {
+    type GroupConfig = MsiIrqGroupConfig;
+
     fn create_group(
         &self,
-        interrupt_type: InterruptType,
+        config: Self::GroupConfig,
-        base: InterruptIndex,
-        count: InterruptIndex,
     ) -> Result<Arc<Box<dyn InterruptSourceGroup>>> {
-        let interrupt_source_group: Arc<Box<dyn InterruptSourceGroup>> = match interrupt_type {
+        let mut allocator = self.allocator.lock().unwrap();
-            PCI_MSI_IRQ => {
+        let mut irq_routes: HashMap<InterruptIndex, InterruptRoute> =
-                let mut allocator = self.allocator.lock().unwrap();
+            HashMap::with_capacity(config.count as usize);
-                let mut irq_routes: HashMap<InterruptIndex, InterruptRoute> =
+        for i in config.base..config.base + config.count {
-                    HashMap::with_capacity(count as usize);
+            irq_routes.insert(i, InterruptRoute::new(&mut allocator)?);
-                for i in base..base + count {
+        }
-                    irq_routes.insert(i, InterruptRoute::new(&mut allocator)?);
-                }
 
-                Arc::new(Box::new(MsiInterruptGroup::new(
+        Ok(Arc::new(Box::new(MsiInterruptGroup::new(
-                    self.vm_fd.clone(),
+            self.vm_fd.clone(),
-                    self.gsi_msi_routes.clone(),
+            self.gsi_msi_routes.clone(),
-                    irq_routes,
+            irq_routes,
-                )))
+        ))))
-            }
-            _ => {
-                return Err(io::Error::new(
-                    io::ErrorKind::Other,
-                    "Interrupt type not supported",
-                ))
-            }
-        };
-
-        Ok(interrupt_source_group)
     }
 
     fn destroy_group(&self, _group: Arc<Box<dyn InterruptSourceGroup>>) -> Result<()> {