From 52800a871a9172674ae60760f33c5d3aaacad55c Mon Sep 17 00:00:00 2001
From: Sebastien Boeuf <sebastien.boeuf@intel.com>
Date: Wed, 22 Jan 2020 18:39:46 +0100
Subject: [PATCH] vmm: Create an InterruptManager dedicated to IOAPIC

By introducing a new InterruptManager dedicated to the IOAPIC, we don't
have to solve the chicken and eggs problem about which of the
InterruptManager or the Ioapic should be created first. It's also
totally fine to have two interrupt manager instances as they both share
the same list of GSI routes and the same allocator.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
---
 vmm/src/device_manager.rs | 38 ++++++++++++++++++++++++++++++++------
 vmm/src/interrupt.rs      | 19 +++++++++++++------
 2 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs
index 57c17a00a..d86aa056f 100644
--- a/vmm/src/device_manager.rs
+++ b/vmm/src/device_manager.rs
@@ -423,8 +423,6 @@ impl DeviceManager {
             vm_fd: vm_info.vm_fd.clone(),
         });
 
-        let ioapic = DeviceManager::add_ioapic(vm_info, &address_manager)?;
-
         // Create a shared list of GSI that can be shared through all PCI
         // devices. This way, we can maintain the full list of used GSI,
         // preventing one device from overriding interrupts setting from
@@ -432,11 +430,38 @@ impl DeviceManager {
         let kvm_gsi_msi_routes: Arc<Mutex<HashMap<u32, KvmRoutingEntry>>> =
             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<dyn InterruptManager> =
+            Arc::new(KvmInterruptManager::new(
+                Arc::clone(&address_manager.allocator),
+                Arc::clone(&vm_info.vm_fd),
+                Arc::clone(&kvm_gsi_msi_routes),
+                None,
+            ));
+
+        let ioapic =
+            DeviceManager::add_ioapic(vm_info, &address_manager, ioapic_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<dyn InterruptManager> = Arc::new(KvmInterruptManager::new(
-            address_manager.allocator.clone(),
-            vm_info.vm_fd.clone(),
-            kvm_gsi_msi_routes,
-            ioapic.clone(),
+            Arc::clone(&address_manager.allocator),
+            Arc::clone(&vm_info.vm_fd),
+            Arc::clone(&kvm_gsi_msi_routes),
+            Some(ioapic.clone()),
         ));
 
         let console = DeviceManager::add_console_device(
@@ -672,6 +697,7 @@ impl DeviceManager {
     fn add_ioapic(
         vm_info: &VmInfo,
         address_manager: &Arc<AddressManager>,
+        _ioapic_interrupt_manager: Arc<dyn InterruptManager>,
     ) -> DeviceManagerResult<Arc<Mutex<ioapic::Ioapic>>> {
         // Create IOAPIC
         let ioapic = Arc::new(Mutex::new(ioapic::Ioapic::new(
diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs
index ea51dc81b..115277b02 100644
--- a/vmm/src/interrupt.rs
+++ b/vmm/src/interrupt.rs
@@ -276,7 +276,7 @@ pub struct KvmInterruptManager {
     allocator: Arc<Mutex<SystemAllocator>>,
     vm_fd: Arc<VmFd>,
     gsi_msi_routes: Arc<Mutex<HashMap<u32, KvmRoutingEntry>>>,
-    ioapic: Arc<Mutex<ioapic::Ioapic>>,
+    ioapic: Option<Arc<Mutex<ioapic::Ioapic>>>,
 }
 
 impl KvmInterruptManager {
@@ -284,7 +284,7 @@ impl KvmInterruptManager {
         allocator: Arc<Mutex<SystemAllocator>>,
         vm_fd: Arc<VmFd>,
         gsi_msi_routes: Arc<Mutex<HashMap<u32, KvmRoutingEntry>>>,
-        ioapic: Arc<Mutex<ioapic::Ioapic>>,
+        ioapic: Option<Arc<Mutex<ioapic::Ioapic>>>,
     ) -> Self {
         KvmInterruptManager {
             allocator,
@@ -311,10 +311,17 @@ impl InterruptManager for KvmInterruptManager {
                     ));
                 }
 
-                Arc::new(Box::new(LegacyUserspaceInterruptGroup::new(
-                    self.ioapic.clone(),
-                    base as u32,
-                )))
+                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",
+                    ));
+                }
             }
             PCI_MSI_IRQ => {
                 let mut allocator = self.allocator.lock().unwrap();