From b77fdeba2d620273c51d133d6ea8e568fc5391a3 Mon Sep 17 00:00:00 2001
From: Sebastien Boeuf <sebastien.boeuf@intel.com>
Date: Thu, 20 Feb 2020 14:32:29 +0100
Subject: [PATCH] msi/msi-x: Prevent from losing masked interrupts

We want to prevent from losing interrupts while they are masked. The
way they can be lost is due to the internals of how they are connected
through KVM. An eventfd is registered to a specific GSI, and then a
route is associated with this same GSI.

The current code adds/removes a route whenever a mask/unmask action
happens. Problem with this approach, KVM will consume the eventfd but
it won't be able to find an associated route and eventually it won't
be able to deliver the interrupt.

That's why this patch introduces a different way of masking/unmasking
the interrupts, simply by registering/unregistering the eventfd with the
GSI. This way, when the vector is masked, the eventfd is going to be
written but nothing will happen because KVM won't consume the event.
Whenever the unmask happens, the eventfd will be registered with a
specific GSI, and if there's some pending events, KVM will trigger them,
based on the route associated with the GSI.

Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
---
 pci/src/msi.rs       |  2 ++
 pci/src/msix.rs      |  3 ++
 vmm/src/interrupt.rs | 68 ++++++++++++++++++++++++++++++++++----------
 3 files changed, 58 insertions(+), 15 deletions(-)

diff --git a/pci/src/msi.rs b/pci/src/msi.rs
index fda44819b..05494cdac 100644
--- a/pci/src/msi.rs
+++ b/pci/src/msi.rs
@@ -214,6 +214,8 @@ impl MsiConfig {
                     if let Err(e) = self.interrupt_source_group.mask(idx as InterruptIndex) {
                         error!("Failed masking vector: {:?}", e);
                     }
+                } else if let Err(e) = self.interrupt_source_group.unmask(idx as InterruptIndex) {
+                    error!("Failed unmasking vector: {:?}", e);
                 }
             }
 
diff --git a/pci/src/msix.rs b/pci/src/msix.rs
index e8020e00b..a83218dca 100644
--- a/pci/src/msix.rs
+++ b/pci/src/msix.rs
@@ -116,6 +116,9 @@ impl MsixConfig {
                         if let Err(e) = self.interrupt_source_group.mask(idx as InterruptIndex) {
                             error!("Failed masking vector: {:?}", e);
                         }
+                    } else if let Err(e) = self.interrupt_source_group.unmask(idx as InterruptIndex)
+                    {
+                        error!("Failed unmasking vector: {:?}", e);
                     }
                 }
 
diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs
index ee43173f4..fad2ea691 100644
--- a/vmm/src/interrupt.rs
+++ b/vmm/src/interrupt.rs
@@ -9,6 +9,7 @@ use kvm_ioctls::VmFd;
 use std::collections::HashMap;
 use std::io;
 use std::mem::size_of;
+use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Mutex};
 use vm_allocator::SystemAllocator;
 use vm_device::interrupt::{
@@ -52,6 +53,7 @@ pub fn vec_with_array_field<T: Default, F>(count: usize) -> Vec<T> {
 pub struct InterruptRoute {
     pub gsi: u32,
     pub irq_fd: EventFd,
+    registered: AtomicBool,
 }
 
 impl InterruptRoute {
@@ -61,25 +63,43 @@ impl InterruptRoute {
             .allocate_gsi()
             .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Failed allocating new GSI"))?;
 
-        Ok(InterruptRoute { gsi, irq_fd })
+        Ok(InterruptRoute {
+            gsi,
+            irq_fd,
+            registered: AtomicBool::new(false),
+        })
     }
 
     pub fn enable(&self, vm: &Arc<VmFd>) -> Result<()> {
-        vm.register_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
-            io::Error::new(
-                io::ErrorKind::Other,
-                format!("Failed registering irq_fd: {}", e),
-            )
-        })
+        if !self.registered.load(Ordering::SeqCst) {
+            vm.register_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
+                io::Error::new(
+                    io::ErrorKind::Other,
+                    format!("Failed registering irq_fd: {}", e),
+                )
+            })?;
+
+            // Update internals to track the irq_fd as "registered".
+            self.registered.store(true, Ordering::SeqCst);
+        }
+
+        Ok(())
     }
 
     pub fn disable(&self, vm: &Arc<VmFd>) -> Result<()> {
-        vm.unregister_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
-            io::Error::new(
-                io::ErrorKind::Other,
-                format!("Failed unregistering irq_fd: {}", e),
-            )
-        })
+        if self.registered.load(Ordering::SeqCst) {
+            vm.unregister_irqfd(&self.irq_fd, self.gsi).map_err(|e| {
+                io::Error::new(
+                    io::ErrorKind::Other,
+                    format!("Failed unregistering irq_fd: {}", e),
+                )
+            })?;
+
+            // Update internals to track the irq_fd as "unregistered".
+            self.registered.store(false, Ordering::SeqCst);
+        }
+
+        Ok(())
     }
 }
 
@@ -234,11 +254,29 @@ impl InterruptSourceGroup for MsiInterruptGroup {
     }
 
     fn mask(&self, index: InterruptIndex) -> Result<()> {
-        self.mask_kvm_entry(index, true)
+        self.mask_kvm_entry(index, true)?;
+
+        if let Some(route) = self.irq_routes.get(&index) {
+            return route.disable(&self.vm_fd);
+        }
+
+        Err(io::Error::new(
+            io::ErrorKind::Other,
+            format!("mask: Invalid interrupt index {}", index),
+        ))
     }
 
     fn unmask(&self, index: InterruptIndex) -> Result<()> {
-        self.mask_kvm_entry(index, false)
+        self.mask_kvm_entry(index, false)?;
+
+        if let Some(route) = self.irq_routes.get(&index) {
+            return route.enable(&self.vm_fd);
+        }
+
+        Err(io::Error::new(
+            io::ErrorKind::Other,
+            format!("unmask: Invalid interrupt index {}", index),
+        ))
     }
 }