From bb712d649f82f623d9d2ed25f9ab758fa4343e19 Mon Sep 17 00:00:00 2001 From: Daniel Verkamp Date: Tue, 19 Nov 2019 09:47:33 -0800 Subject: 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 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/1925169 Reviewed-by: Zide Chen Reviewed-by: Dylan Reid Tested-by: kokoro --- devices/src/pci/msix.rs | 5 ++ devices/src/virtio/block.rs | 5 -- devices/src/virtio/fs/mod.rs | 4 -- devices/src/virtio/net.rs | 5 -- devices/src/virtio/vhost/net.rs | 7 +-- devices/src/virtio/vhost/vsock.rs | 7 +-- devices/src/virtio/virtio_device.rs | 5 -- devices/src/virtio/virtio_pci_device.rs | 104 ++++++++++++-------------------- 8 files changed, 46 insertions(+), 96 deletions(-) (limited to 'devices') 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> { workers_kill_evt: Option, @@ -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, @@ -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, mem: Option, settings_bar: u8, - msix_config: Option>>, + msix_config: Arc>, msix_cap_reg_idx: Option, common_config: VirtioPciCommonConfig, } @@ -177,7 +178,7 @@ impl VirtioPciDevice { pub fn new( mem: GuestMemory, device: Box, - msi_device_socket: Option, + msi_device_socket: VmIrqRequestSocket, ) -> Result { 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, ); -- cgit 1.4.1