devices: virtio: enable MSI-X for all devices

All virtio devices can use the same generic calculation for number of
MSI-X vectors required: number of queues plus one for configuration
changes.  Move this calculation to the VirtioPciDevice implementation
and remove the Option to unconditionally enable MSI-X support for all
PCI virtio devices.

BUG=chromium:854765
TEST=Verify all virtio interrupts in /proc/interrupts are PCI-MSI

Change-Id: I5905ab52840e7617b0b342ec6ca3f75dccd16e4d
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1925169
Reviewed-by: Zide Chen <zide.chen@intel.com>
Reviewed-by: Dylan Reid <dgreid@chromium.org>
Tested-by: kokoro <noreply+kokoro@google.com>

8 files changed, 46 insertions, 96 deletions

diff --git a/devices/src/pci/msix.rs b/devices/src/pci/msix.rs
index eab4398..282771f 100644
--- a/devices/src/pci/msix.rs
+++ b/devices/src/pci/msix.rs
@@ -80,6 +80,11 @@ impl MsixConfig {
         }
     }
 
+    /// Get the number of MSI-X vectors in this configuration.
+    pub fn num_vectors(&self) -> u16 {
+        self.msix_num
+    }
+
     /// Check whether the Function Mask bit in Message Control word in set or not.
     /// if 1, all of the vectors associated with the function are masked,
     /// regardless of their per-vector Mask bit states.
diff --git a/devices/src/virtio/block.rs b/devices/src/virtio/block.rs
index 1fc69a0..3dab8b9 100644
--- a/devices/src/virtio/block.rs
+++ b/devices/src/virtio/block.rs
@@ -29,7 +29,6 @@ use super::{
 
 const QUEUE_SIZE: u16 = 256;
 const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE];
-const NUM_MSIX_VECTORS: u16 = 2;
 const SECTOR_SHIFT: u8 = 9;
 const SECTOR_SIZE: u64 = 0x01 << SECTOR_SHIFT;
 const MAX_DISCARD_SECTORS: u32 = u32::MAX;
@@ -710,10 +709,6 @@ impl VirtioDevice for Block {
         TYPE_BLOCK
     }
 
-    fn msix_vectors(&self) -> u16 {
-        NUM_MSIX_VECTORS
-    }
-
     fn queue_max_sizes(&self) -> &[u16] {
         QUEUE_SIZES
     }
diff --git a/devices/src/virtio/fs/mod.rs b/devices/src/virtio/fs/mod.rs
index 0442bad..a1f8868 100644
--- a/devices/src/virtio/fs/mod.rs
+++ b/devices/src/virtio/fs/mod.rs
@@ -192,10 +192,6 @@ impl VirtioDevice for Fs {
         TYPE_FS
     }
 
-    fn msix_vectors(&self) -> u16 {
-        self.queue_sizes.len() as u16
-    }
-
     fn queue_max_sizes(&self) -> &[u16] {
         &self.queue_sizes
     }
diff --git a/devices/src/virtio/net.rs b/devices/src/virtio/net.rs
index b439238..a2b1058 100644
--- a/devices/src/virtio/net.rs
+++ b/devices/src/virtio/net.rs
@@ -25,7 +25,6 @@ use super::{Interrupt, Queue, Reader, VirtioDevice, Writer, TYPE_NET};
 const QUEUE_SIZE: u16 = 256;
 const NUM_QUEUES: usize = 2;
 const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE, QUEUE_SIZE];
-const NUM_MSIX_VECTORS: u16 = NUM_QUEUES as u16;
 
 #[derive(Debug)]
 pub enum NetError {
@@ -407,10 +406,6 @@ where
         TYPE_NET
     }
 
-    fn msix_vectors(&self) -> u16 {
-        NUM_MSIX_VECTORS
-    }
-
     fn queue_max_sizes(&self) -> &[u16] {
         QUEUE_SIZES
     }
diff --git a/devices/src/virtio/vhost/net.rs b/devices/src/virtio/vhost/net.rs
index 5834aa7..2082a0f 100644
--- a/devices/src/virtio/vhost/net.rs
+++ b/devices/src/virtio/vhost/net.rs
@@ -21,7 +21,6 @@ use crate::virtio::{Interrupt, Queue, VirtioDevice, TYPE_NET};
 const QUEUE_SIZE: u16 = 256;
 const NUM_QUEUES: usize = 2;
 const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE; NUM_QUEUES];
-const NUM_MSIX_VECTORS: u16 = NUM_QUEUES as u16;
 
 pub struct Net<T: TapT, U: VhostNetT<T>> {
     workers_kill_evt: Option<EventFd>,
@@ -82,7 +81,7 @@ where
             | 1 << virtio_sys::vhost::VIRTIO_F_VERSION_1;
 
         let mut vhost_interrupt = Vec::new();
-        for _ in 0..NUM_MSIX_VECTORS {
+        for _ in 0..NUM_QUEUES {
             vhost_interrupt.push(EventFd::new().map_err(Error::VhostIrqCreate)?);
         }
 
@@ -150,10 +149,6 @@ where
         TYPE_NET
     }
 
-    fn msix_vectors(&self) -> u16 {
-        NUM_MSIX_VECTORS
-    }
-
     fn queue_max_sizes(&self) -> &[u16] {
         QUEUE_SIZES
     }
diff --git a/devices/src/virtio/vhost/vsock.rs b/devices/src/virtio/vhost/vsock.rs
index 231e134..32f2c99 100644
--- a/devices/src/virtio/vhost/vsock.rs
+++ b/devices/src/virtio/vhost/vsock.rs
@@ -17,7 +17,6 @@ use crate::virtio::{copy_config, Interrupt, Queue, VirtioDevice, TYPE_VSOCK};
 const QUEUE_SIZE: u16 = 256;
 const NUM_QUEUES: usize = 3;
 const QUEUE_SIZES: &[u16] = &[QUEUE_SIZE; NUM_QUEUES];
-const NUM_MSIX_VECTORS: u16 = NUM_QUEUES as u16;
 
 pub struct Vsock {
     worker_kill_evt: Option<EventFd>,
@@ -43,7 +42,7 @@ impl Vsock {
             | 1 << virtio_sys::vhost::VIRTIO_F_VERSION_1;
 
         let mut interrupts = Vec::new();
-        for _ in 0..NUM_MSIX_VECTORS {
+        for _ in 0..NUM_QUEUES {
             interrupts.push(EventFd::new().map_err(Error::VhostIrqCreate)?);
         }
 
@@ -112,10 +111,6 @@ impl VirtioDevice for Vsock {
         TYPE_VSOCK
     }
 
-    fn msix_vectors(&self) -> u16 {
-        NUM_MSIX_VECTORS
-    }
-
     fn queue_max_sizes(&self) -> &[u16] {
         QUEUE_SIZES
     }
diff --git a/devices/src/virtio/virtio_device.rs b/devices/src/virtio/virtio_device.rs
index 281e098..b61245b 100644
--- a/devices/src/virtio/virtio_device.rs
+++ b/devices/src/virtio/virtio_device.rs
@@ -32,11 +32,6 @@ pub trait VirtioDevice: Send {
     /// The virtio device type.
     fn device_type(&self) -> u32;
 
-    /// number of MSI-X vectors. 0 means MSI-X not supported.
-    fn msix_vectors(&self) -> u16 {
-        0
-    }
-
     /// The maximum size of each queue that this device supports.
     fn queue_max_sizes(&self) -> &[u16];
 
diff --git a/devices/src/virtio/virtio_pci_device.rs b/devices/src/virtio/virtio_pci_device.rs
index d31821e..9ace795 100644
--- a/devices/src/virtio/virtio_pci_device.rs
+++ b/devices/src/virtio/virtio_pci_device.rs
@@ -10,6 +10,7 @@ use sync::Mutex;
 
 use data_model::{DataInit, Le32};
 use kvm::Datamatch;
+use libc::ERANGE;
 use resources::{Alloc, MmioType, SystemAllocator};
 use sys_util::{EventFd, GuestMemory, Result};
 
@@ -167,7 +168,7 @@ pub struct VirtioPciDevice {
     queue_evts: Vec<EventFd>,
     mem: Option<GuestMemory>,
     settings_bar: u8,
-    msix_config: Option<Arc<Mutex<MsixConfig>>>,
+    msix_config: Arc<Mutex<MsixConfig>>,
     msix_cap_reg_idx: Option<usize>,
     common_config: VirtioPciCommonConfig,
 }
@@ -177,7 +178,7 @@ impl VirtioPciDevice {
     pub fn new(
         mem: GuestMemory,
         device: Box<dyn VirtioDevice>,
-        msi_device_socket: Option<VmIrqRequestSocket>,
+        msi_device_socket: VmIrqRequestSocket,
     ) -> Result<Self> {
         let mut queue_evts = Vec::new();
         for _ in device.queue_max_sizes() {
@@ -191,16 +192,11 @@ impl VirtioPciDevice {
 
         let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + device.device_type() as u16;
 
-        let msix_num = device.msix_vectors();
-        let msix_config = if msix_num > 0 && msi_device_socket.is_some() {
-            let msix_config = Arc::new(Mutex::new(MsixConfig::new(
-                msix_num,
-                msi_device_socket.unwrap(),
-            )));
-            Some(msix_config)
-        } else {
-            None
-        };
+        let num_queues = device.queue_max_sizes().len();
+
+        // One MSI-X vector per queue plus one for configuration changes.
+        let msix_num = u16::try_from(num_queues + 1).map_err(|_| sys_util::Error::new(ERANGE))?;
+        let msix_config = Arc::new(Mutex::new(MsixConfig::new(msix_num, msi_device_socket)));
 
         let config_regs = PciConfiguration::new(
             VIRTIO_PCI_VENDOR_ID,
@@ -306,20 +302,18 @@ impl VirtioPciDevice {
             .add_capability(&configuration_cap)
             .map_err(PciDeviceError::CapabilitiesSetup)?;
 
-        if self.msix_config.is_some() && self.device.msix_vectors() > 0 {
-            let msix_cap = MsixCap::new(
-                settings_bar,
-                self.device.msix_vectors(),
-                MSIX_TABLE_BAR_OFFSET as u32,
-                settings_bar,
-                MSIX_PBA_BAR_OFFSET as u32,
-            );
-            let msix_offset = self
-                .config_regs
-                .add_capability(&msix_cap)
-                .map_err(PciDeviceError::CapabilitiesSetup)?;
-            self.msix_cap_reg_idx = Some(msix_offset / 4);
-        }
+        let msix_cap = MsixCap::new(
+            settings_bar,
+            self.msix_config.lock().num_vectors(),
+            MSIX_TABLE_BAR_OFFSET as u32,
+            settings_bar,
+            MSIX_PBA_BAR_OFFSET as u32,
+        );
+        let msix_offset = self
+            .config_regs
+            .add_capability(&msix_cap)
+            .map_err(PciDeviceError::CapabilitiesSetup)?;
+        self.msix_cap_reg_idx = Some(msix_offset / 4);
 
         self.settings_bar = settings_bar;
         Ok(())
@@ -343,10 +337,8 @@ impl PciDevice for VirtioPciDevice {
         if let Some(interrupt_resample_evt) = &self.interrupt_resample_evt {
             fds.push(interrupt_resample_evt.as_raw_fd());
         }
-        if let Some(msix_config) = &self.msix_config {
-            let fd = msix_config.lock().get_msi_socket();
-            fds.push(fd);
-        }
+        let fd = self.msix_config.lock().get_msi_socket();
+        fds.push(fd);
         fds
     }
 
@@ -464,10 +456,8 @@ impl PciDevice for VirtioPciDevice {
     fn read_config_register(&self, reg_idx: usize) -> u32 {
         let mut data: u32 = self.config_regs.read_reg(reg_idx);
         if let Some(msix_cap_reg_idx) = self.msix_cap_reg_idx {
-            if let Some(msix_config) = &self.msix_config {
-                if msix_cap_reg_idx == reg_idx {
-                    data = msix_config.lock().read_msix_capability(data);
-                }
+            if msix_cap_reg_idx == reg_idx {
+                data = self.msix_config.lock().read_msix_capability(data);
             }
         }
 
@@ -476,10 +466,8 @@ impl PciDevice for VirtioPciDevice {
 
     fn write_config_register(&mut self, reg_idx: usize, offset: u64, data: &[u8]) {
         if let Some(msix_cap_reg_idx) = self.msix_cap_reg_idx {
-            if let Some(msix_config) = &self.msix_config {
-                if msix_cap_reg_idx == reg_idx {
-                    msix_config.lock().write_msix_capability(offset, data);
-                }
+            if msix_cap_reg_idx == reg_idx {
+                self.msix_config.lock().write_msix_capability(offset, data);
             }
         }
 
@@ -523,19 +511,15 @@ impl PciDevice for VirtioPciDevice {
             }
 
             o if MSIX_TABLE_BAR_OFFSET <= o && o < MSIX_TABLE_BAR_OFFSET + MSIX_TABLE_SIZE => {
-                if let Some(msix_config) = &self.msix_config {
-                    msix_config
-                        .lock()
-                        .read_msix_table(o - MSIX_TABLE_BAR_OFFSET, data);
-                }
+                self.msix_config
+                    .lock()
+                    .read_msix_table(o - MSIX_TABLE_BAR_OFFSET, data);
             }
 
             o if MSIX_PBA_BAR_OFFSET <= o && o < MSIX_PBA_BAR_OFFSET + MSIX_PBA_SIZE => {
-                if let Some(msix_config) = &self.msix_config {
-                    msix_config
-                        .lock()
-                        .read_pba_entries(o - MSIX_PBA_BAR_OFFSET, data);
-                }
+                self.msix_config
+                    .lock()
+                    .read_pba_entries(o - MSIX_PBA_BAR_OFFSET, data);
             }
 
             _ => (),
@@ -574,18 +558,14 @@ impl PciDevice for VirtioPciDevice {
                 // Handled with ioeventfds.
             }
             o if MSIX_TABLE_BAR_OFFSET <= o && o < MSIX_TABLE_BAR_OFFSET + MSIX_TABLE_SIZE => {
-                if let Some(msix_config) = &self.msix_config {
-                    msix_config
-                        .lock()
-                        .write_msix_table(o - MSIX_TABLE_BAR_OFFSET, data);
-                }
+                self.msix_config
+                    .lock()
+                    .write_msix_table(o - MSIX_TABLE_BAR_OFFSET, data);
             }
             o if MSIX_PBA_BAR_OFFSET <= o && o < MSIX_PBA_BAR_OFFSET + MSIX_PBA_SIZE => {
-                if let Some(msix_config) = &self.msix_config {
-                    msix_config
-                        .lock()
-                        .write_pba_entries(o - MSIX_PBA_BAR_OFFSET, data);
-                }
+                self.msix_config
+                    .lock()
+                    .write_pba_entries(o - MSIX_PBA_BAR_OFFSET, data);
             }
 
             _ => (),
@@ -595,17 +575,11 @@ impl PciDevice for VirtioPciDevice {
             if let Some(interrupt_evt) = self.interrupt_evt.take() {
                 if let Some(interrupt_resample_evt) = self.interrupt_resample_evt.take() {
                     if let Some(mem) = self.mem.take() {
-                        let msix_config = if let Some(msix_config) = &self.msix_config {
-                            Some(msix_config.clone())
-                        } else {
-                            None
-                        };
-
                         let interrupt = Interrupt::new(
                             self.interrupt_status.clone(),
                             interrupt_evt,
                             interrupt_resample_evt,
-                            msix_config,
+                            Some(self.msix_config.clone()),
                             self.common_config.msix_config,
                         );