From da2b3c92d39a570e10314358eed56ffc315a694c Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Tue, 4 Feb 2020 12:04:10 +0100 Subject: [PATCH] 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 --- devices/src/ioapic.rs | 13 +++-- vfio/src/vfio_pci.rs | 37 +++++++++---- vm-device/src/interrupt/mod.rs | 37 ++++++++----- vm-virtio/src/transport/pci_device.rs | 12 +++-- vmm/src/device_manager.rs | 40 ++++++++------ vmm/src/interrupt.rs | 77 ++++++++------------------- 6 files changed, 110 insertions(+), 106 deletions(-) diff --git a/devices/src/ioapic.rs b/devices/src/ioapic.rs index f84c46446..393e71750 100644 --- a/devices/src/ioapic.rs +++ b/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, + interrupt_manager: Arc>, ) -> Result { let interrupt_source_group = interrupt_manager - .create_group( - PCI_MSI_IRQ, - 0 as InterruptIndex, - NUM_IOAPIC_PINS as InterruptIndex, - ) + .create_group(MsiIrqGroupConfig { + base: 0 as InterruptIndex, + count: NUM_IOAPIC_PINS as InterruptIndex, + }) .map_err(Error::CreateInterruptSourceGroup)?; interrupt_source_group diff --git a/vfio/src/vfio_pci.rs b/vfio/src/vfio_pci.rs index 280cc9191..f394ebc36 100644 --- a/vfio/src/vfio_pci.rs +++ b/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, device: VfioDevice, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> Result { 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) { + fn parse_msix_capabilities( + &mut self, + cap: u8, + interrupt_manager: &Arc>, + ) { 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) { + fn parse_msi_capabilities( + &mut self, + cap: u8, + interrupt_manager: &Arc>, + ) { let msg_ctl = self .vfio_pci_configuration .read_config_word((cap + 2).into()); let interrupt_source_group = interrupt_manager - .create_group( - PCI_MSI_IRQ, - 0, - msi_num_enabled_vectors(msg_ctl) as InterruptIndex, - ) + .create_group(MsiIrqGroupConfig { + base: 0, + count: 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) { + fn parse_capabilities( + &mut self, + interrupt_manager: &Arc>, + ) { let mut cap_next = self .vfio_pci_configuration .read_config_byte(PCI_CONFIG_CAPABILITY_OFFSET); diff --git a/vm-device/src/interrupt/mod.rs b/vm-device/src/interrupt/mod.rs index fb4d9d9e4..51121aca9 100644 --- a/vm-device/src/interrupt/mod.rs +++ b/vm-device/src/interrupt/mod.rs @@ -66,12 +66,6 @@ pub type Result = std::io::Result; /// 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; -pub const PCI_MSI_IRQ: InterruptType = 1; +/// Configuration data for legacy, pin based interrupt groups. +/// +/// 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( - &self, - interrupt_type: InterruptType, - base: InterruptIndex, - count: InterruptIndex, - ) -> Result>>; + fn create_group(&self, config: Self::GroupConfig) + -> Result>>; /// Destroy an [InterruptSourceGroup](trait.InterruptSourceGroup.html) object created by /// [create_group()](trait.InterruptManager.html#tymethod.create_group). diff --git a/vm-virtio/src/transport/pci_device.rs b/vm-virtio/src/transport/pci_device.rs index d31958be8..2697065d2 100755 --- a/vm-virtio/src/transport/pci_device.rs +++ b/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>, msix_num: u16, iommu_mapping_cb: Option>, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> Result { 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 = - interrupt_manager.create_group(PCI_MSI_IRQ, 0, msix_num as InterruptIndex)?; + let interrupt_source_group = interrupt_manager.create_group(MsiIrqGroupConfig { + 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( diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 6a95a51d3..1fe145974 100644 --- a/vmm/src/device_manager.rs +++ b/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::{InterruptIndex, PIN_IRQ}; +use vm_device::interrupt::{ + 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 = + let msi_interrupt_manager: Arc> = 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 = - Arc::new(KvmLegacyUserspaceInterruptManager::new(ioapic.clone())); + let legacy_interrupt_manager: Arc< + dyn InterruptManager, + > = 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>, bool)>, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> DeviceManagerResult<()> { #[cfg(feature = "pci_support")] { @@ -587,7 +589,7 @@ impl DeviceManager { fn add_mmio_devices( &mut self, virtio_devices: Vec<(Arc>, bool)>, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> DeviceManagerResult<()> { #[cfg(feature = "mmio_support")] { @@ -611,7 +613,7 @@ impl DeviceManager { fn add_ioapic( address_manager: &Arc, - interrupt_manager: Arc, + interrupt_manager: Arc>, ) -> DeviceManagerResult>> { // 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, + interrupt_manager: &Arc>, reset_evt: EventFd, exit_evt: EventFd, ) -> DeviceManagerResult>>> { @@ -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, + interrupt_manager: &Arc>, virtio_devices: &mut Vec<(Arc>, bool)>, ) -> DeviceManagerResult> { 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, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> DeviceManagerResult> { let mut mem_slot = self .memory_manager @@ -1340,7 +1346,7 @@ impl DeviceManager { virtio_device: Arc>, pci: &mut PciBus, iommu_mapping: &Option>, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, ) -> DeviceManagerResult> { // 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>, - interrupt_manager: &Arc, + interrupt_manager: &Arc>, 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); diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index e8a690900..ee43173f4 100644 --- a/vmm/src/interrupt.rs +++ b/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, - PCI_MSI_IRQ, PIN_IRQ, + InterruptIndex, InterruptManager, InterruptSourceConfig, InterruptSourceGroup, + 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, - base: InterruptIndex, - count: InterruptIndex, + config: Self::GroupConfig, ) -> Result>> { - let interrupt_source_group: Arc> = match interrupt_type { - PIN_IRQ => { - if count > 1 { - 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) + Ok(Arc::new(Box::new(LegacyUserspaceInterruptGroup::new( + self.ioapic.clone(), + config.irq as u32, + )))) } fn destroy_group(&self, _group: Arc>) -> Result<()> { @@ -340,36 +321,24 @@ impl InterruptManager for KvmLegacyUserspaceInterruptManager { } impl InterruptManager for KvmMsiInterruptManager { + type GroupConfig = MsiIrqGroupConfig; + fn create_group( &self, - interrupt_type: InterruptType, - base: InterruptIndex, - count: InterruptIndex, + config: Self::GroupConfig, ) -> Result>> { - let interrupt_source_group: Arc> = match interrupt_type { - PCI_MSI_IRQ => { - let mut allocator = self.allocator.lock().unwrap(); - let mut irq_routes: HashMap = - HashMap::with_capacity(count as usize); - for i in base..base + count { - irq_routes.insert(i, InterruptRoute::new(&mut allocator)?); - } + let mut allocator = self.allocator.lock().unwrap(); + let mut irq_routes: HashMap = + HashMap::with_capacity(config.count as usize); + for i in config.base..config.base + config.count { + irq_routes.insert(i, InterruptRoute::new(&mut allocator)?); + } - Arc::new(Box::new(MsiInterruptGroup::new( - self.vm_fd.clone(), - self.gsi_msi_routes.clone(), - irq_routes, - ))) - } - _ => { - return Err(io::Error::new( - io::ErrorKind::Other, - "Interrupt type not supported", - )) - } - }; - - Ok(interrupt_source_group) + Ok(Arc::new(Box::new(MsiInterruptGroup::new( + self.vm_fd.clone(), + self.gsi_msi_routes.clone(), + irq_routes, + )))) } fn destroy_group(&self, _group: Arc>) -> Result<()> {