virtio-devices: Simplify interrupt handling

Previously the interrupt was created in VirtioPciDevice, moved via the
Option::take() to the VirtioPciDeviceActivator and then moved to the
VirtioDevice upon activation. On reset it would be moved back ready for
reactivation.

Since this already an Arc type remove the wrapping Option and instead
refcount it such that the VirtioPciDevice can continue to hold onto it
for later activations.

This significantly simplifies the reset() logic as there is no need to
hand back the interrupt.

A few devices used whether the interrupt was Some to make triggering an
interrupt a no-op. However the MSI-X interrupt routing already drops the
interrupt if the driver hasn't yet configured the vector so it is safe
to trigger the interrupt before device activation (e.g. balloon resize
request before driver loaded).

VirtioCommon still retains an Option<..> for the interrupt as the
interrupt is not known until activation time (after this has been
created). A helper VirtioCommon::trigger_interrupt() has been added to
handle this.

Signed-off-by: Rob Bradford <rbradford@meta.com>
This commit is contained in:
Rob Bradford
2026-04-26 10:57:49 +01:00
parent fe6bcd0376
commit e11ff541da
18 changed files with 73 additions and 110 deletions

View File

@@ -458,7 +458,6 @@ pub struct Balloon {
config: VirtioBalloonConfig, config: VirtioBalloonConfig,
seccomp_action: SeccompAction, seccomp_action: SeccompAction,
exit_evt: EventFd, exit_evt: EventFd,
interrupt_cb: Option<Arc<dyn VirtioInterrupt>>,
} }
impl Balloon { impl Balloon {
@@ -523,20 +522,15 @@ impl Balloon {
config, config,
seccomp_action, seccomp_action,
exit_evt, exit_evt,
interrupt_cb: None,
}) })
} }
pub fn resize(&mut self, size: u64) -> Result<(), Error> { pub fn resize(&mut self, size: u64) -> Result<(), Error> {
self.config.num_pages = (size >> VIRTIO_BALLOON_PFN_SHIFT) as u32; self.config.num_pages = (size >> VIRTIO_BALLOON_PFN_SHIFT) as u32;
if let Some(interrupt_cb) = &self.interrupt_cb { self.common
interrupt_cb .trigger_interrupt(VirtioInterruptType::Config)
.trigger(VirtioInterruptType::Config) .map_err(Error::FailedSignal)
.map_err(Error::FailedSignal)
} else {
Ok(())
}
} }
// Get the actual size of the virtio-balloon. // Get the actual size of the virtio-balloon.
@@ -647,8 +641,6 @@ impl VirtioDevice for Balloon {
None None
}; };
self.interrupt_cb = Some(interrupt_cb.clone());
let mut handler = BalloonEpollHandler { let mut handler = BalloonEpollHandler {
mem, mem,
queues: virtqueues, queues: virtqueues,
@@ -688,10 +680,9 @@ impl VirtioDevice for Balloon {
self.common.access_platform() self.common.access_platform()
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
} }

View File

@@ -1025,13 +1025,9 @@ impl Block {
self.common.resume().map_err(Error::ResumeVcpus)?; self.common.resume().map_err(Error::ResumeVcpus)?;
if let Some(interrupt_cb) = self.common.interrupt_cb.as_ref() { self.common
interrupt_cb .trigger_interrupt(VirtioInterruptType::Config)
.trigger(VirtioInterruptType::Config) .map_err(Error::ConfigChange)
.map_err(Error::ConfigChange)
} else {
Ok(())
}
} }
#[cfg(fuzzing)] #[cfg(fuzzing)]
@@ -1178,11 +1174,10 @@ impl VirtioDevice for Block {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
self.set_writeback_mode(true); self.set_writeback_mode(true);
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn counters(&self) -> Option<HashMap<&'static str, Wrapping<u64>>> { fn counters(&self) -> Option<HashMap<&'static str, Wrapping<u64>>> {

View File

@@ -773,10 +773,9 @@ impl VirtioDevice for Console {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) { fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) {

View File

@@ -123,11 +123,8 @@ pub trait VirtioDevice: Send {
/// Activates this device for real usage. /// Activates this device for real usage.
fn activate(&mut self, context: ActivationContext) -> ActivateResult; fn activate(&mut self, context: ActivationContext) -> ActivateResult;
/// Optionally deactivates this device and returns ownership of the guest memory map, interrupt /// Optionally deactivates this device.
/// event, and queue events. fn reset(&mut self) {}
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> {
None
}
/// Returns the list of shared memory regions required by the device. /// Returns the list of shared memory regions required by the device.
fn get_shm_regions(&self) -> Option<VirtioSharedMemoryList> { fn get_shm_regions(&self) -> Option<VirtioSharedMemoryList> {
@@ -290,7 +287,7 @@ impl VirtioCommon {
Ok(()) Ok(())
} }
pub fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { pub fn reset(&mut self) {
self.queue_evts.clear(); self.queue_evts.clear();
// Resume the virtio thread if it was paused. Reset must always // Resume the virtio thread if it was paused. Reset must always
@@ -315,8 +312,16 @@ impl VirtioCommon {
} }
} }
// Return the interrupt // Drop the interrupt callback clone
Some(self.interrupt_cb.take().unwrap()) self.interrupt_cb = None;
}
pub fn trigger_interrupt(&self, int_type: VirtioInterruptType) -> std::io::Result<()> {
if let Some(interrupt_cb) = &self.interrupt_cb {
interrupt_cb.trigger(int_type)
} else {
Ok(())
}
} }
// Wait for the worker thread to finish and return // Wait for the worker thread to finish and return
@@ -406,12 +411,9 @@ impl Pausable for VirtioCommon {
} }
// Also trigger interrupts into the guest to wake up the driver to avoid a "livelock" // Also trigger interrupts into the guest to wake up the driver to avoid a "livelock"
if let Some(interrupt_cb) = &self.interrupt_cb { for i in 0..self.queue_evts.len() {
for i in 0..self.queue_evts.len() { self.trigger_interrupt(crate::VirtioInterruptType::Queue(i as u16))
interrupt_cb .ok();
.trigger(crate::VirtioInterruptType::Queue(i as u16))
.ok();
}
} }
Ok(()) Ok(())

View File

@@ -1120,10 +1120,9 @@ impl VirtioDevice for Iommu {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
} }

View File

@@ -737,7 +737,6 @@ pub struct Mem {
dma_mapping_handlers: Arc<Mutex<BTreeMap<VirtioMemMappingSource, Arc<dyn ExternalDmaMapping>>>>, dma_mapping_handlers: Arc<Mutex<BTreeMap<VirtioMemMappingSource, Arc<dyn ExternalDmaMapping>>>>,
blocks_state: Arc<Mutex<BlocksState>>, blocks_state: Arc<Mutex<BlocksState>>,
exit_evt: EventFd, exit_evt: EventFd,
interrupt_cb: Option<Arc<dyn VirtioInterrupt>>,
} }
impl Mem { impl Mem {
@@ -830,7 +829,6 @@ impl Mem {
dma_mapping_handlers: Arc::new(Mutex::new(BTreeMap::new())), dma_mapping_handlers: Arc::new(Mutex::new(BTreeMap::new())),
blocks_state, blocks_state,
exit_evt, exit_evt,
interrupt_cb: None,
}) })
} }
@@ -844,15 +842,11 @@ impl Mem {
Error::ResizeError(anyhow!("Failed to update virtio configuration: {e:?}")) Error::ResizeError(anyhow!("Failed to update virtio configuration: {e:?}"))
})?; })?;
if let Some(interrupt_cb) = self.interrupt_cb.as_ref() { self.common
interrupt_cb .trigger_interrupt(VirtioInterruptType::Config)
.trigger(VirtioInterruptType::Config) .map_err(|e| {
.map_err(|e| { Error::ResizeError(anyhow!("Failed to signal the guest about resize: {e:?}"))
Error::ResizeError(anyhow!("Failed to signal the guest about resize: {e:?}")) })
})
} else {
Ok(())
}
} }
pub fn add_dma_mapping_handler( pub fn add_dma_mapping_handler(
@@ -966,8 +960,6 @@ impl VirtioDevice for Mem {
let (_, queue, queue_evt) = queues.remove(0); let (_, queue, queue_evt) = queues.remove(0);
self.interrupt_cb = Some(interrupt_cb.clone());
let mut handler = MemEpollHandler { let mut handler = MemEpollHandler {
mem, mem,
region: self.region.clone(), region: self.region.clone(),
@@ -1016,10 +1008,9 @@ impl VirtioDevice for Mem {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
} }

View File

@@ -868,10 +868,9 @@ impl VirtioDevice for Net {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn counters(&self) -> Option<HashMap<&'static str, Wrapping<u64>>> { fn counters(&self) -> Option<HashMap<&'static str, Wrapping<u64>>> {

View File

@@ -438,10 +438,9 @@ impl VirtioDevice for Pmem {
Err(ActivateError::BadActivate) Err(ActivateError::BadActivate)
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn userspace_mappings(&self) -> Vec<UserspaceMapping> { fn userspace_mappings(&self) -> Vec<UserspaceMapping> {

View File

@@ -311,10 +311,9 @@ impl VirtioDevice for Rng {
Err(ActivateError::BadActivate) Err(ActivateError::BadActivate)
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) { fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) {

View File

@@ -300,7 +300,7 @@ pub struct VirtioPciDeviceState {
} }
pub struct VirtioPciDeviceActivator { pub struct VirtioPciDeviceActivator {
interrupt: Option<Arc<dyn VirtioInterrupt>>, interrupt: Arc<dyn VirtioInterrupt>,
memory: Option<GuestMemoryAtomic<GuestMemoryMmap>>, memory: Option<GuestMemoryAtomic<GuestMemoryMmap>>,
device: Arc<Mutex<dyn VirtioDevice>>, device: Arc<Mutex<dyn VirtioDevice>>,
device_activated: Arc<AtomicBool>, device_activated: Arc<AtomicBool>,
@@ -315,7 +315,7 @@ impl VirtioPciDeviceActivator {
let mut locked_device = self.device.lock().unwrap(); let mut locked_device = self.device.lock().unwrap();
locked_device.activate(crate::device::ActivationContext { locked_device.activate(crate::device::ActivationContext {
mem: self.memory.take().unwrap(), mem: self.memory.take().unwrap(),
interrupt_cb: self.interrupt.take().unwrap(), interrupt_cb: self.interrupt,
queues: self.queues.take().unwrap(), queues: self.queues.take().unwrap(),
device_status: self.status, device_status: self.status,
})?; })?;
@@ -822,7 +822,7 @@ impl VirtioPciDevice {
} }
VirtioPciDeviceActivator { VirtioPciDeviceActivator {
interrupt: self.virtio_interrupt.take(), interrupt: self.virtio_interrupt.as_ref().unwrap().clone(),
memory: Some(self.memory.clone()), memory: Some(self.memory.clone()),
device: self.device.clone(), device: self.device.clone(),
queues: Some(queues), queues: Some(queues),
@@ -1250,10 +1250,7 @@ impl PciDevice for VirtioPciDevice {
if self.is_driver_init() { if self.is_driver_init() {
if self.device_activated.swap(false, Ordering::SeqCst) { if self.device_activated.swap(false, Ordering::SeqCst) {
let mut device = self.device.lock().unwrap(); let mut device = self.device.lock().unwrap();
if let Some(virtio_interrupt) = device.reset() { device.reset();
// Upon reset the device returns its interrupt EventFD
self.virtio_interrupt = Some(virtio_interrupt);
}
} }
// Reset queue readiness and the common configuration // Reset queue readiness and the common configuration

View File

@@ -443,14 +443,13 @@ impl VirtioDevice for Vdpa {
self.activate_vdpa(&mem.memory(), virtio_interrupt.as_ref(), &queues) self.activate_vdpa(&mem.memory(), virtio_interrupt.as_ref(), &queues)
.map_err(ActivateError::ActivateVdpa)?; .map_err(ActivateError::ActivateVdpa)?;
// Store the virtio interrupt handler as we need to return it on reset
self.common.interrupt_cb = Some(virtio_interrupt); self.common.interrupt_cb = Some(virtio_interrupt);
event!("vdpa", "activated", "id", &self.id); event!("vdpa", "activated", "id", &self.id);
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
// Backend reset failures are logged but don't skip local cleanup: // Backend reset failures are logged but don't skip local cleanup:
// reset must converge to fresh state regardless of backend state. // reset must converge to fresh state regardless of backend state.
if let Err(e) = self.reset_vdpa() { if let Err(e) = self.reset_vdpa() {
@@ -459,8 +458,8 @@ impl VirtioDevice for Vdpa {
event!("vdpa", "reset", "id", &self.id); event!("vdpa", "reset", "id", &self.id);
// Return the virtio interrupt handler // Drop the interrupt callback clone
self.common.interrupt_cb.take() self.common.interrupt_cb = None;
} }
fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) { fn set_access_platform(&mut self, access_platform: Arc<dyn AccessPlatform>) {

View File

@@ -28,7 +28,7 @@ use super::{DEFAULT_VIRTIO_FEATURES, Error, Result};
use crate::seccomp_filters::Thread; use crate::seccomp_filters::Thread;
use crate::thread_helper::spawn_virtio_thread; use crate::thread_helper::spawn_virtio_thread;
use crate::vhost_user::{VhostUserCommon, VhostUserState}; use crate::vhost_user::{VhostUserCommon, VhostUserState};
use crate::{GuestMemoryMmap, GuestRegionMmap, VIRTIO_F_ACCESS_PLATFORM, VirtioInterrupt}; use crate::{GuestMemoryMmap, GuestRegionMmap, VIRTIO_F_ACCESS_PLATFORM};
const DEFAULT_QUEUE_NUMBER: usize = 1; const DEFAULT_QUEUE_NUMBER: usize = 1;
@@ -307,8 +307,8 @@ impl VirtioDevice for Blk {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
self.vu_common.reset(&self.id) self.vu_common.reset(&self.id);
} }
fn shutdown(&mut self) { fn shutdown(&mut self) {

View File

@@ -25,7 +25,7 @@ use crate::thread_helper::spawn_virtio_thread;
use crate::vhost_user::{VhostUserCommon, VhostUserState}; use crate::vhost_user::{VhostUserCommon, VhostUserState};
use crate::{ use crate::{
ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM, ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM,
VirtioCommon, VirtioDevice, VirtioDeviceType, VirtioInterrupt, VirtioSharedMemoryList, VirtioCommon, VirtioDevice, VirtioDeviceType, VirtioSharedMemoryList,
}; };
const NUM_QUEUE_OFFSET: usize = 1; const NUM_QUEUE_OFFSET: usize = 1;
@@ -285,8 +285,8 @@ impl VirtioDevice for Fs {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
self.vu_common.reset(&self.id) self.vu_common.reset(&self.id);
} }
fn shutdown(&mut self) { fn shutdown(&mut self) {

View File

@@ -26,7 +26,7 @@ use crate::thread_helper::spawn_virtio_thread;
use crate::vhost_user::{VhostUserCommon, VhostUserState}; use crate::vhost_user::{VhostUserCommon, VhostUserState};
use crate::{ use crate::{
ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM, ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM,
VirtioCommon, VirtioDevice, VirtioInterrupt, VirtioSharedMemoryList, VirtioCommon, VirtioDevice, VirtioSharedMemoryList,
}; };
pub type State = VhostUserState<()>; pub type State = VhostUserState<()>;
@@ -308,8 +308,8 @@ impl VirtioDevice for GenericVhostUser {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
self.vu_common.reset(&self.id) self.vu_common.reset(&self.id);
} }
fn shutdown(&mut self) { fn shutdown(&mut self) {

View File

@@ -347,7 +347,6 @@ pub struct VhostUserCommon {
pub vu_num_queues: usize, pub vu_num_queues: usize,
pub migration_started: bool, pub migration_started: bool,
pub server: bool, pub server: bool,
pub interrupt_cb: Option<Arc<dyn VirtioInterrupt>>,
pub vring_bases: Option<Vec<u64>>, pub vring_bases: Option<Vec<u64>>,
pub epoll_thread: Option<thread::JoinHandle<()>>, pub epoll_thread: Option<thread::JoinHandle<()>>,
} }
@@ -395,8 +394,6 @@ impl VhostUserCommon {
) )
.map_err(ActivateError::VhostUserSetup)?; .map_err(ActivateError::VhostUserSetup)?;
self.interrupt_cb = Some(interrupt_cb.clone());
Ok(VhostUserEpollHandler { Ok(VhostUserEpollHandler {
vu: vu.clone(), vu: vu.clone(),
mem, mem,
@@ -428,7 +425,7 @@ impl VhostUserCommon {
Ok(()) Ok(())
} }
pub fn reset(&mut self, id: &str) -> Option<Arc<dyn VirtioInterrupt>> { pub fn reset(&mut self, id: &str) {
// Resume the virtio thread if it was paused. Reset must always // Resume the virtio thread if it was paused. Reset must always
// converge to fresh state, so backend resume / reset failures are // converge to fresh state, so backend resume / reset failures are
// logged but don't skip the rest of the teardown. // logged but don't skip the rest of the teardown.
@@ -454,8 +451,8 @@ impl VhostUserCommon {
event!("virtio-device", "reset", "id", id); event!("virtio-device", "reset", "id", id);
// Return the interrupt // Drop the interrupt callback clone
Some(self.virtio_common.interrupt_cb.take().unwrap()) self.virtio_common.interrupt_cb = None;
} }
pub fn shutdown(&mut self) { pub fn shutdown(&mut self) {
@@ -525,12 +522,10 @@ impl VhostUserCommon {
MigratableError::Resume(anyhow!("Error resuming vhost-user backend: {e:?}")) MigratableError::Resume(anyhow!("Error resuming vhost-user backend: {e:?}"))
})?; })?;
} }
if let Some(interrupt_cb) = &self.interrupt_cb { for i in 0..self.vu_num_queues {
for i in 0..self.vu_num_queues { self.virtio_common
interrupt_cb .trigger_interrupt(crate::VirtioInterruptType::Queue(i as u16))
.trigger(crate::VirtioInterruptType::Queue(i as u16)) .ok();
.ok();
}
} }
Ok(()) Ok(())
} }

View File

@@ -29,7 +29,7 @@ use crate::vhost_user::vu_common_ctrl::{VhostUserConfig, VhostUserHandle};
use crate::vhost_user::{DEFAULT_VIRTIO_FEATURES, Error, Result, VhostUserCommon, VhostUserState}; use crate::vhost_user::{DEFAULT_VIRTIO_FEATURES, Error, Result, VhostUserCommon, VhostUserState};
use crate::{ use crate::{
ActivateResult, GuestMemoryMmap, GuestRegionMmap, NetCtrlEpollHandler, ActivateResult, GuestMemoryMmap, GuestRegionMmap, NetCtrlEpollHandler,
VIRTIO_F_ACCESS_PLATFORM, VirtioCommon, VirtioDevice, VirtioDeviceType, VirtioInterrupt, VIRTIO_F_ACCESS_PLATFORM, VirtioCommon, VirtioDevice, VirtioDeviceType,
}; };
const DEFAULT_QUEUE_NUMBER: usize = 2; const DEFAULT_QUEUE_NUMBER: usize = 2;
@@ -364,8 +364,8 @@ impl VirtioDevice for Net {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
self.vu_common.reset(&self.id) self.vu_common.reset(&self.id);
} }
fn shutdown(&mut self) { fn shutdown(&mut self) {

View File

@@ -488,10 +488,9 @@ where
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
fn shutdown(&mut self) { fn shutdown(&mut self) {

View File

@@ -379,10 +379,9 @@ impl VirtioDevice for Watchdog {
Ok(()) Ok(())
} }
fn reset(&mut self) -> Option<Arc<dyn VirtioInterrupt>> { fn reset(&mut self) {
let result = self.common.reset(); self.common.reset();
event!("virtio-device", "reset", "id", &self.id); event!("virtio-device", "reset", "id", &self.id);
result
} }
} }