From d609410b8ba5c1e44ec10697bb48aebddc36b47b Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 11 Feb 2026 20:08:45 -0500 Subject: [PATCH] pci: Support injecting interrupts from externally-provided irqfds The virtio vhost-user device backend prefers to use externally-provided eventfds as irqfds. This allows the frontend VM to notify the backend VM directly, without the need for a userspace proxy process. Since the frontend can provide irqfds at any time, the backend needs to register and unregister irqfds dynamically. This is tricky because the functions that access the irqfd table all take `&self`, not `&mut self`. The obvious solution to this problem is to wrap the table in a mutex. Most of these functions are not called on hot paths, but `.notifier()` is called whenever Cloud Hypervisor needs to inject an interrupt into a guest. Most devices don't need to register irqfds at runtime, and for them, slowing down interrupt injection would be wasteful. Instead, require devices to opt-in to irqfd registration. The irqfd table now comes in two forms: one that contains a mutex and one that does not. The one containing a mutex can be mutated freely, while attempting to mutate the one that does not will panic. Right now, no code registeres irqfds at runtime, but this will change in subsequent commits. Signed-off-by: Demi Marie Obenour --- pci/src/lib.rs | 5 +- pci/src/msix.rs | 62 ++++++++++++++++++++-- pci/src/vfio.rs | 4 +- virtio-devices/src/device.rs | 12 +++++ virtio-devices/src/transport/pci_device.rs | 52 ++++++++++-------- vm-device/src/interrupt/mod.rs | 42 ++++++++++++++- vmm/src/device_manager.rs | 4 +- vmm/src/interrupt.rs | 46 +++++++++++++++- 8 files changed, 194 insertions(+), 33 deletions(-) diff --git a/pci/src/lib.rs b/pci/src/lib.rs index 5ab87cf19..17c3ab723 100644 --- a/pci/src/lib.rs +++ b/pci/src/lib.rs @@ -32,7 +32,10 @@ pub use self::device::{ BarReprogrammingParams, DeviceRelocation, Error as PciDeviceError, PciDevice, }; pub use self::msi::{MsiCap, MsiConfig, msi_num_enabled_vectors}; -pub use self::msix::{MSIX_CONFIG_ID, MSIX_TABLE_ENTRY_SIZE, MsixCap, MsixConfig, MsixTableEntry}; +pub use self::msix::{ + MSIX_CONFIG_ID, MSIX_TABLE_ENTRY_SIZE, MaybeMutInterruptSourceGroup, MsixCap, MsixConfig, + MsixTableEntry, +}; pub use self::vfio::{MmioRegion, VfioDmaMapping, VfioPciDevice, VfioPciError}; pub use self::vfio_user::{VfioUserDmaMapping, VfioUserPciDevice, VfioUserPciDeviceError}; diff --git a/pci/src/msix.rs b/pci/src/msix.rs index 9bc5e63f3..49b379b02 100644 --- a/pci/src/msix.rs +++ b/pci/src/msix.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 OR BSD-3-Clause // -use std::sync::Arc; +use std::sync::{Arc, Mutex}; use std::{io, result}; use byteorder::{ByteOrder, LittleEndian}; @@ -15,6 +15,7 @@ use vm_device::interrupt::{ }; use vm_memory::ByteValued; use vm_migration::{MigratableError, Pausable, Snapshot, Snapshottable}; +use vmm_sys_util::eventfd::EventFd; use crate::{PciCapability, PciCapabilityId}; @@ -72,11 +73,66 @@ pub struct MsixConfigState { enabled: bool, } +#[derive(Clone)] +pub enum MaybeMutInterruptSourceGroup { + Immutable(Arc), + Mutable(Arc>), +} + +macro_rules! impl_method { + ($( + fn $i: ident(&self $(,$index:ident : $InterruptIndex:ty)*$(,)?) -> $r: ty; + )*) => { + $( + fn $i(&self $(,$index: $InterruptIndex)*) -> $r { + match self { + Self::Immutable(source) => source.$i($($index),*), + Self::Mutable(source) => source.lock().unwrap().$i($($index),*), + } + } + )* + }; +} + +impl InterruptSourceGroup for MaybeMutInterruptSourceGroup { + impl_method! { + fn trigger(&self, index: InterruptIndex) -> vm_device::interrupt::Result<()>; + + fn notifier(&self, index: InterruptIndex) -> Option; + + fn update( + &self, + index: InterruptIndex, + config: InterruptSourceConfig, + masked: bool, + set_gsi: bool, + ) -> vm_device::interrupt::Result<()>; + + fn set_gsi(&self) -> vm_device::interrupt::Result<()>; + } +} + +impl MaybeMutInterruptSourceGroup { + pub fn set_notifier( + &self, + index: InterruptIndex, + eventfd: Option, + vm: &dyn hypervisor::Vm, + ) -> std::io::Result<()> { + match self { + Self::Immutable(_) => panic!( + "Attempted to set a notifier of an immutable source. You must mark your device as needing a mutable source by having sets_irqfd() return true." + ), + Self::Mutable(source) => source.lock().unwrap().set_notifier(index, eventfd, vm), + } + } +} + pub struct MsixConfig { pub table_entries: Vec, pub pba_entries: Vec, pub devid: u32, - interrupt_source_group: Arc, + interrupt_source_group: MaybeMutInterruptSourceGroup, masked: bool, enabled: bool, } @@ -84,7 +140,7 @@ pub struct MsixConfig { impl MsixConfig { pub fn new( msix_vectors: u16, - interrupt_source_group: Arc, + interrupt_source_group: MaybeMutInterruptSourceGroup, devid: u32, state: Option, ) -> result::Result { diff --git a/pci/src/vfio.rs b/pci/src/vfio.rs index 9e8e7e316..e46e276aa 100644 --- a/pci/src/vfio.rs +++ b/pci/src/vfio.rs @@ -37,7 +37,7 @@ use vmm_sys_util::eventfd::EventFd; use crate::mmap::MmapRegion; use crate::msi::{MSI_CONFIG_ID, MsiConfigState}; -use crate::msix::MsixConfigState; +use crate::msix::{MaybeMutInterruptSourceGroup, MsixConfigState}; use crate::{ BarReprogrammingParams, MSIX_CONFIG_ID, MSIX_TABLE_ENTRY_SIZE, MsiCap, MsiConfig, MsixCap, MsixConfig, PCI_CONFIGURATION_ID, PciBarConfiguration, PciBarPrefetchable, PciBarRegionType, @@ -863,7 +863,7 @@ impl VfioCommon { let msix_config = MsixConfig::new( msix_cap.table_size(), - interrupt_source_group.clone(), + MaybeMutInterruptSourceGroup::Immutable(interrupt_source_group.clone()), bdf.into(), state, ) diff --git a/virtio-devices/src/device.rs b/virtio-devices/src/device.rs index 91b742a0b..f0673f561 100644 --- a/virtio-devices/src/device.rs +++ b/virtio-devices/src/device.rs @@ -80,6 +80,18 @@ pub trait VirtioDevice: Send { /// The maximum size of each queue that this device supports. fn queue_max_sizes(&self) -> &[u16]; + /// Whether the device needs to register extra irqfds at runtime + /// from external sources. + /// The default is false. If this is true, locking is required for + /// most operations involving interrupts (but not for sending) + /// interrupts from external irqfds). + /// + /// If the device claims to not need to register irqfds, but + /// attempts to do so, a panic will ensue. + fn interrupt_source_mutable(&self) -> bool { + false + } + /// The set of feature bits that this device supports. fn features(&self) -> u64 { 0 diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index 70e03d028..3e2a96ccd 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -17,9 +17,10 @@ use anyhow::anyhow; use libc::EFD_NONBLOCK; use log::{error, info}; use pci::{ - BarReprogrammingParams, MsixCap, MsixConfig, PciBarConfiguration, PciBarRegionType, - PciCapability, PciCapabilityId, PciClassCode, PciConfiguration, PciDevice, PciDeviceError, - PciHeaderType, PciMassStorageSubclass, PciNetworkControllerSubclass, PciSubclass, + BarReprogrammingParams, MaybeMutInterruptSourceGroup, MsixCap, MsixConfig, PciBarConfiguration, + PciBarRegionType, PciCapability, PciCapabilityId, PciClassCode, PciConfiguration, PciDevice, + PciDeviceError, PciHeaderType, PciMassStorageSubclass, PciNetworkControllerSubclass, + PciSubclass, }; use serde::{Deserialize, Serialize}; use thiserror::Error; @@ -359,7 +360,7 @@ pub struct VirtioPciDevice { // PCI interrupts. interrupt_status: Arc, virtio_interrupt: Option>, - interrupt_source_group: Arc, + interrupt_source_group: MaybeMutInterruptSourceGroup, // virtio queues queues: Vec, @@ -433,17 +434,26 @@ impl VirtioPciDevice { let pci_device_id = VIRTIO_PCI_DEVICE_ID_BASE + locked_device.device_type() as u16; - let interrupt_source_group = interrupt_manager - .create_group(MsiIrqGroupConfig { + let interrupt_source_group: MaybeMutInterruptSourceGroup = { + let config = MsiIrqGroupConfig { base: 0, count: msix_num as InterruptIndex, + }; + (if locked_device.interrupt_source_mutable() { + interrupt_manager + .create_group_mut(config) + .map(MaybeMutInterruptSourceGroup::Mutable) + } else { + interrupt_manager + .create_group(config) + .map(MaybeMutInterruptSourceGroup::Immutable) }) .map_err(|e| { VirtioPciDeviceError::CreateVirtioPciDevice(anyhow!( "Failed creating MSI interrupt group: {e}" )) - })?; - + })? + }; let msix_state = vm_migration::state_from_id(snapshot, pci::MSIX_CONFIG_ID).map_err(|e| { VirtioPciDeviceError::CreateVirtioPciDevice(anyhow!( @@ -452,14 +462,11 @@ impl VirtioPciDevice { })?; let (msix_config, msix_config_clone) = if msix_num > 0 { + let interrupt_source_group: MaybeMutInterruptSourceGroup = + interrupt_source_group.clone(); let msix_config = Arc::new(Mutex::new( - MsixConfig::new( - msix_num, - interrupt_source_group.clone(), - pci_device_bdf, - msix_state, - ) - .unwrap(), + MsixConfig::new(msix_num, interrupt_source_group, pci_device_bdf, msix_state) + .unwrap(), )); let msix_config_clone = msix_config.clone(); (Some(msix_config), Some(msix_config_clone)) @@ -598,7 +605,7 @@ impl VirtioPciDevice { memory, settings_bar: 0, use_64bit_bar, - interrupt_source_group, + interrupt_source_group: interrupt_source_group.clone(), cap_pci_cfg_info, bar_regions: vec![], activate_evt, @@ -855,7 +862,7 @@ pub struct VirtioInterruptMsix { msix_config: Arc>, config_vector: Arc, queues_vectors: Arc>>, - interrupt_source_group: Arc, + interrupt_source_group: MaybeMutInterruptSourceGroup, } impl VirtioInterruptMsix { @@ -863,7 +870,7 @@ impl VirtioInterruptMsix { msix_config: Arc>, config_vector: Arc, queues_vectors: Arc>>, - interrupt_source_group: Arc, + interrupt_source_group: MaybeMutInterruptSourceGroup, ) -> Self { VirtioInterruptMsix { msix_config, @@ -917,11 +924,12 @@ impl VirtioInterrupt for VirtioInterruptMsix { fn set_notifier( &self, - _interrupt: u32, - _eventfd: Option, - _vm: &dyn hypervisor::Vm, + interrupt: u32, + eventfd: Option, + vm: &dyn hypervisor::Vm, ) -> std::io::Result<()> { - unimplemented!() + self.interrupt_source_group + .set_notifier(interrupt, eventfd, vm) } } diff --git a/vm-device/src/interrupt/mod.rs b/vm-device/src/interrupt/mod.rs index 342cbe063..e9b0180d2 100644 --- a/vm-device/src/interrupt/mod.rs +++ b/vm-device/src/interrupt/mod.rs @@ -57,7 +57,8 @@ //! * The virtual device backend requests the interrupt manager to create an interrupt group //! according to guest configuration information -use std::sync::Arc; +use std::io::{Error, ErrorKind}; +use std::sync::{Arc, Mutex}; pub use hypervisor::{InterruptSourceConfig, LegacyIrqSourceConfig, MsiIrqSourceConfig}; use vmm_sys_util::eventfd::EventFd; @@ -107,6 +108,30 @@ pub trait InterruptManager: Send + Sync { /// * count: number of Interrupt Sources to be managed by the group object. fn create_group(&self, config: Self::GroupConfig) -> Result>; + /// Create an [InterruptSourceGroup](trait.InterruptSourceGroup.html) object to manage + /// interrupt sources for a virtual device + /// + /// An [InterruptSourceGroup](trait.InterruptSourceGroup.html) object manages all interrupt + /// sources of the same type for a virtual device. + /// + /// This is the same as [`Self::create_group`], except that the returned + /// [`InterruptSourceGroup`] allows setting the irqfd used as notifier via + /// [`InterruptSourceGroup::set_notifier`]. + /// + /// # Arguments + /// * interrupt_type: type of interrupt source. + /// * base: base Interrupt Source ID to be managed by the group object. + /// * count: number of Interrupt Sources to be managed by the group object. + fn create_group_mut( + &self, + _config: Self::GroupConfig, + ) -> Result>> { + Err(Error::new( + ErrorKind::Unsupported, + "setting notifiers not supported", + )) + } + /// Destroy an [InterruptSourceGroup](trait.InterruptSourceGroup.html) object created by /// [create_group()](trait.InterruptManager.html#tymethod.create_group). /// @@ -137,7 +162,7 @@ pub trait InterruptSourceGroup: Send + Sync { /// Returns an interrupt notifier from this interrupt. /// /// An interrupt notifier allows for external components and processes - /// to inject interrupts into a guest, by writing to the file returned + /// to inject interrupts into a guest, by writing to the [`EventFd`] returned /// by this method. #[allow(unused_variables)] fn notifier(&self, index: InterruptIndex) -> Option; @@ -159,4 +184,17 @@ pub trait InterruptSourceGroup: Send + Sync { /// Set the interrupt group GSI routing table. fn set_gsi(&self) -> Result<()>; + + /// Sets the [`EventFd`] used to trigger interrupts. + fn set_notifier( + &mut self, + _index: InterruptIndex, + _eventfd: Option, + _vm: &dyn hypervisor::Vm, + ) -> Result<()> { + Err(Error::new( + ErrorKind::Unsupported, + "setting notifiers not supported", + )) + } } diff --git a/vmm/src/device_manager.rs b/vmm/src/device_manager.rs index e560b02d7..958d3086b 100644 --- a/vmm/src/device_manager.rs +++ b/vmm/src/device_manager.rs @@ -4201,8 +4201,8 @@ impl DeviceManager { return Err(DeviceManagerError::MissingNode); } - // Allows support for one MSI-X vector per queue. It also adds 1 - // as we need to take into account the dedicated vector to notify + // Allows support for one MSI-X vector per interrupt needed by the device. + // It also adds 1 as we need to take into account the dedicated vector to notify // about a virtio config change. let msix_num = (virtio_device.lock().unwrap().queue_max_sizes().len() + 1) as u16; diff --git a/vmm/src/interrupt.rs b/vmm/src/interrupt.rs index 07727d1ad..0995d8356 100644 --- a/vmm/src/interrupt.rs +++ b/vmm/src/interrupt.rs @@ -86,6 +86,9 @@ impl InterruptRoute { ) } + // This is currently not used, but the upcoming vhost-guest feature + // will use it. Use #[allow(dead_code)] to suppress a compiler + // warning. #[allow(dead_code)] pub fn set_notifier( &mut self, @@ -96,7 +99,7 @@ impl InterruptRoute { if self.registered { if let Some(ref irq_fd) = self.irq_fd { vm.register_irqfd(irq_fd, self.gsi) - .map_err(|e| io::Error::other(format!("Failed registering irq_fd: {e}")))? + .map_err(|e| io::Error::other(format!("Failed registering irq_fd: {e}")))?; } // If the irqfd cannot be unregistered, what to do? Spin? // Returning an error isn't helpful as the new irqfd is already registered. @@ -235,6 +238,19 @@ impl InterruptSourceGroup for MsiInterruptGroup { let routes = self.gsi_msi_routes.lock().unwrap(); self.set_gsi_routes(&routes) } + + fn set_notifier( + &mut self, + index: InterruptIndex, + eventfd: Option, + vm: &dyn hypervisor::Vm, + ) -> Result<()> { + if let Some(route) = self.irq_routes.get(&index) { + return route.lock().unwrap().set_notifier(eventfd, vm); + } + + Ok(()) + } } pub struct LegacyUserspaceInterruptGroup { @@ -323,6 +339,26 @@ impl InterruptManager for LegacyUserspaceInterruptManager { } } +impl MsiInterruptManager { + fn create_group_raw( + &self, + config: ::GroupConfig, + ) -> Result { + let mut allocator = self.allocator.lock().unwrap(); + let mut irq_routes: HashMap> = + HashMap::with_capacity(config.count as usize); + for i in config.base..config.base + config.count { + irq_routes.insert(i, Mutex::new(InterruptRoute::new(&mut allocator)?)); + } + + Ok(MsiInterruptGroup::new( + self.vm.clone(), + self.gsi_msi_routes.clone(), + irq_routes, + )) + } +} + impl InterruptManager for MsiInterruptManager { type GroupConfig = MsiIrqGroupConfig; @@ -341,6 +377,14 @@ impl InterruptManager for MsiInterruptManager { ))) } + fn create_group_mut( + &self, + config: Self::GroupConfig, + ) -> vm_device::interrupt::Result>> { + let r = self.create_group_raw(config)?; + Ok(Arc::new(Mutex::new(r))) + } + fn destroy_group(&self, _group: Arc) -> Result<()> { Ok(()) }