From 84fc807bc60825f53033259a37abd802ad6069f7 Mon Sep 17 00:00:00 2001 From: Samuel Ortiz Date: Tue, 4 Feb 2020 10:59:53 +0100 Subject: [PATCH] interrupt: Interrupt manager split We create 2 different interrupt managers for separately handling creation of legacy and MSI interrupt groups. Doing so allows us to have a cleaner interrupt manager and IOAPIC initialization path. It also prepares for an InterruptManager trait design improvement where we remove the interrupt source type dependency by associating an interrupt configuration type to the trait. Signed-off-by: Samuel Ortiz --- vmm/src/device_manager.rs | 54 ++++++++++++++--------------------- vmm/src/interrupt.rs | 60 +++++++++++++++++++++++++++------------ 2 files changed, 64 insertions(+), 50 deletions(-) diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index 6bd41ddb5..6a95a51d3 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -13,7 +13,9 @@ extern crate vm_device; use crate::config::ConsoleOutputMode; use crate::config::VmConfig; -use crate::interrupt::{KvmInterruptManager, KvmRoutingEntry}; +use crate::interrupt::{ + KvmLegacyUserspaceInterruptManager, KvmMsiInterruptManager, KvmRoutingEntry, +}; use crate::memory_manager::{Error as MemoryManagerError, MemoryManager}; #[cfg(feature = "acpi")] use acpi_tables::{aml, aml::Aml}; @@ -428,38 +430,26 @@ impl DeviceManager { let kvm_gsi_msi_routes: Arc>> = Arc::new(Mutex::new(HashMap::new())); - // Here we create a first interrupt manager that will be directly - // passed down to the Ioapic. The reason we need this extra interrupt - // manager is because the more global one will need a handler onto the - // Ioapic itself. We didn't want to solve this problem by adding some - // setter to the KvmInterruptManager as this would have required the - // interrupt manager to be mutable. - // - // One thing to note, it is safe to create two interrupt managers - // without risking some concurrency between the two since the list - // of GSI routes is shared and protected by a Mutex. - let ioapic_interrupt_manager: Arc = - Arc::new(KvmInterruptManager::new( + // First we create the MSI interrupt manager, the legacy one is created + // later, after the IOAPIC device creation. + // The reason we create the MSI one first is because the IOAPIC needs it, + // 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 = + Arc::new(KvmMsiInterruptManager::new( Arc::clone(&address_manager.allocator), - vm_fd.clone(), + vm_fd, Arc::clone(&kvm_gsi_msi_routes), - None, )); - let ioapic = DeviceManager::add_ioapic(&address_manager, ioapic_interrupt_manager)?; + let ioapic = + DeviceManager::add_ioapic(&address_manager, Arc::clone(&msi_interrupt_manager))?; - // Creation of the global interrupt manager, which can take a hold onto - // the brand new Ioapic. - // - // Note the list of GSI routes is Arc cloned, the same way it was Arc - // cloned for the interrupt manager dedicated to the Ioapic. That's how - // both interrupt managers are going to share the list correctly. - let interrupt_manager: Arc = Arc::new(KvmInterruptManager::new( - Arc::clone(&address_manager.allocator), - vm_fd, - Arc::clone(&kvm_gsi_msi_routes), - Some(ioapic.clone()), - )); + // 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())); #[cfg(feature = "acpi")] address_manager @@ -494,22 +484,22 @@ impl DeviceManager { #[cfg(feature = "acpi")] { device_manager.ged_notification_device = device_manager.add_acpi_devices( - &interrupt_manager, + &legacy_interrupt_manager, reset_evt.try_clone().map_err(DeviceManagerError::EventFd)?, _exit_evt.try_clone().map_err(DeviceManagerError::EventFd)?, )?; } device_manager.console = - device_manager.add_console_device(&interrupt_manager, &mut virtio_devices)?; + device_manager.add_console_device(&legacy_interrupt_manager, &mut virtio_devices)?; #[cfg(any(feature = "pci_support", feature = "mmio_support"))] virtio_devices.append(&mut device_manager.make_virtio_devices()?); if cfg!(feature = "pci_support") { - device_manager.add_pci_devices(virtio_devices, &interrupt_manager)?; + device_manager.add_pci_devices(virtio_devices, &msi_interrupt_manager)?; } else if cfg!(feature = "mmio_support") { - device_manager.add_mmio_devices(virtio_devices, &interrupt_manager)?; + device_manager.add_mmio_devices(virtio_devices, &legacy_interrupt_manager)?; } Ok(device_manager) diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index 115277b02..e8a690900 100644 --- a/vmm/src/interrupt.rs +++ b/vmm/src/interrupt.rs @@ -272,30 +272,37 @@ impl InterruptSourceGroup for LegacyUserspaceInterruptGroup { } } -pub struct KvmInterruptManager { +pub struct KvmLegacyUserspaceInterruptManager { + ioapic: Arc>, +} + +pub struct KvmMsiInterruptManager { allocator: Arc>, vm_fd: Arc, gsi_msi_routes: Arc>>, - ioapic: Option>>, } -impl KvmInterruptManager { +impl KvmLegacyUserspaceInterruptManager { + pub fn new(ioapic: Arc>) -> Self { + KvmLegacyUserspaceInterruptManager { ioapic } + } +} + +impl KvmMsiInterruptManager { pub fn new( allocator: Arc>, vm_fd: Arc, gsi_msi_routes: Arc>>, - ioapic: Option>>, ) -> Self { - KvmInterruptManager { + KvmMsiInterruptManager { allocator, vm_fd, gsi_msi_routes, - ioapic, } } } -impl InterruptManager for KvmInterruptManager { +impl InterruptManager for KvmLegacyUserspaceInterruptManager { fn create_group( &self, interrupt_type: InterruptType, @@ -311,18 +318,35 @@ impl InterruptManager for KvmInterruptManager { )); } - if let Some(ioapic) = &self.ioapic { - Arc::new(Box::new(LegacyUserspaceInterruptGroup::new( - ioapic.clone(), - base as u32, - ))) - } else { - return Err(io::Error::new( - io::ErrorKind::Other, - "No IOAPIC configured, cannot create legacy interrupt group", - )); - } + 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>) -> Result<()> { + Ok(()) + } +} + +impl InterruptManager for KvmMsiInterruptManager { + fn create_group( + &self, + interrupt_type: InterruptType, + base: InterruptIndex, + count: InterruptIndex, + ) -> Result>> { + let interrupt_source_group: Arc> = match interrupt_type { PCI_MSI_IRQ => { let mut allocator = self.allocator.lock().unwrap(); let mut irq_routes: HashMap =