From e11ff541da7a520a02acf2128ba84e63da80d895 Mon Sep 17 00:00:00 2001 From: Rob Bradford Date: Sun, 26 Apr 2026 10:57:49 +0100 Subject: [PATCH] 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 --- virtio-devices/src/balloon.rs | 19 ++++-------- virtio-devices/src/block.rs | 15 ++++------ virtio-devices/src/console.rs | 5 ++-- virtio-devices/src/device.rs | 30 ++++++++++--------- virtio-devices/src/iommu.rs | 5 ++-- virtio-devices/src/mem.rs | 23 +++++--------- virtio-devices/src/net.rs | 5 ++-- virtio-devices/src/pmem.rs | 5 ++-- virtio-devices/src/rng.rs | 5 ++-- virtio-devices/src/transport/pci_device.rs | 11 +++---- virtio-devices/src/vdpa.rs | 7 ++--- virtio-devices/src/vhost_user/blk.rs | 6 ++-- virtio-devices/src/vhost_user/fs.rs | 6 ++-- .../src/vhost_user/generic_vhost_user.rs | 6 ++-- virtio-devices/src/vhost_user/mod.rs | 19 +++++------- virtio-devices/src/vhost_user/net.rs | 6 ++-- virtio-devices/src/vsock/device.rs | 5 ++-- virtio-devices/src/watchdog.rs | 5 ++-- 18 files changed, 73 insertions(+), 110 deletions(-) diff --git a/virtio-devices/src/balloon.rs b/virtio-devices/src/balloon.rs index 37ebb056f..74b83dff2 100644 --- a/virtio-devices/src/balloon.rs +++ b/virtio-devices/src/balloon.rs @@ -458,7 +458,6 @@ pub struct Balloon { config: VirtioBalloonConfig, seccomp_action: SeccompAction, exit_evt: EventFd, - interrupt_cb: Option>, } impl Balloon { @@ -523,20 +522,15 @@ impl Balloon { config, seccomp_action, exit_evt, - interrupt_cb: None, }) } pub fn resize(&mut self, size: u64) -> Result<(), Error> { self.config.num_pages = (size >> VIRTIO_BALLOON_PFN_SHIFT) as u32; - if let Some(interrupt_cb) = &self.interrupt_cb { - interrupt_cb - .trigger(VirtioInterruptType::Config) - .map_err(Error::FailedSignal) - } else { - Ok(()) - } + self.common + .trigger_interrupt(VirtioInterruptType::Config) + .map_err(Error::FailedSignal) } // Get the actual size of the virtio-balloon. @@ -647,8 +641,6 @@ impl VirtioDevice for Balloon { None }; - self.interrupt_cb = Some(interrupt_cb.clone()); - let mut handler = BalloonEpollHandler { mem, queues: virtqueues, @@ -688,10 +680,9 @@ impl VirtioDevice for Balloon { self.common.access_platform() } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } } diff --git a/virtio-devices/src/block.rs b/virtio-devices/src/block.rs index 5765d5a39..d37db6cd8 100644 --- a/virtio-devices/src/block.rs +++ b/virtio-devices/src/block.rs @@ -1025,13 +1025,9 @@ impl Block { self.common.resume().map_err(Error::ResumeVcpus)?; - if let Some(interrupt_cb) = self.common.interrupt_cb.as_ref() { - interrupt_cb - .trigger(VirtioInterruptType::Config) - .map_err(Error::ConfigChange) - } else { - Ok(()) - } + self.common + .trigger_interrupt(VirtioInterruptType::Config) + .map_err(Error::ConfigChange) } #[cfg(fuzzing)] @@ -1178,11 +1174,10 @@ impl VirtioDevice for Block { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); self.set_writeback_mode(true); event!("virtio-device", "reset", "id", &self.id); - result } fn counters(&self) -> Option>> { diff --git a/virtio-devices/src/console.rs b/virtio-devices/src/console.rs index 0a4cf65cf..180be8dfa 100644 --- a/virtio-devices/src/console.rs +++ b/virtio-devices/src/console.rs @@ -773,10 +773,9 @@ impl VirtioDevice for Console { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } fn set_access_platform(&mut self, access_platform: Arc) { diff --git a/virtio-devices/src/device.rs b/virtio-devices/src/device.rs index 84f064259..2349db6b5 100644 --- a/virtio-devices/src/device.rs +++ b/virtio-devices/src/device.rs @@ -123,11 +123,8 @@ pub trait VirtioDevice: Send { /// Activates this device for real usage. fn activate(&mut self, context: ActivationContext) -> ActivateResult; - /// Optionally deactivates this device and returns ownership of the guest memory map, interrupt - /// event, and queue events. - fn reset(&mut self) -> Option> { - None - } + /// Optionally deactivates this device. + fn reset(&mut self) {} /// Returns the list of shared memory regions required by the device. fn get_shm_regions(&self) -> Option { @@ -290,7 +287,7 @@ impl VirtioCommon { Ok(()) } - pub fn reset(&mut self) -> Option> { + pub fn reset(&mut self) { self.queue_evts.clear(); // Resume the virtio thread if it was paused. Reset must always @@ -315,8 +312,16 @@ impl VirtioCommon { } } - // Return the interrupt - Some(self.interrupt_cb.take().unwrap()) + // Drop the interrupt callback clone + 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 @@ -406,12 +411,9 @@ impl Pausable for VirtioCommon { } // 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() { - interrupt_cb - .trigger(crate::VirtioInterruptType::Queue(i as u16)) - .ok(); - } + for i in 0..self.queue_evts.len() { + self.trigger_interrupt(crate::VirtioInterruptType::Queue(i as u16)) + .ok(); } Ok(()) diff --git a/virtio-devices/src/iommu.rs b/virtio-devices/src/iommu.rs index 513d510b5..bc03e6775 100644 --- a/virtio-devices/src/iommu.rs +++ b/virtio-devices/src/iommu.rs @@ -1120,10 +1120,9 @@ impl VirtioDevice for Iommu { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } } diff --git a/virtio-devices/src/mem.rs b/virtio-devices/src/mem.rs index 727fd72c0..afc168a6e 100644 --- a/virtio-devices/src/mem.rs +++ b/virtio-devices/src/mem.rs @@ -737,7 +737,6 @@ pub struct Mem { dma_mapping_handlers: Arc>>>, blocks_state: Arc>, exit_evt: EventFd, - interrupt_cb: Option>, } impl Mem { @@ -830,7 +829,6 @@ impl Mem { dma_mapping_handlers: Arc::new(Mutex::new(BTreeMap::new())), blocks_state, exit_evt, - interrupt_cb: None, }) } @@ -844,15 +842,11 @@ impl Mem { Error::ResizeError(anyhow!("Failed to update virtio configuration: {e:?}")) })?; - if let Some(interrupt_cb) = self.interrupt_cb.as_ref() { - interrupt_cb - .trigger(VirtioInterruptType::Config) - .map_err(|e| { - Error::ResizeError(anyhow!("Failed to signal the guest about resize: {e:?}")) - }) - } else { - Ok(()) - } + self.common + .trigger_interrupt(VirtioInterruptType::Config) + .map_err(|e| { + Error::ResizeError(anyhow!("Failed to signal the guest about resize: {e:?}")) + }) } pub fn add_dma_mapping_handler( @@ -966,8 +960,6 @@ impl VirtioDevice for Mem { let (_, queue, queue_evt) = queues.remove(0); - self.interrupt_cb = Some(interrupt_cb.clone()); - let mut handler = MemEpollHandler { mem, region: self.region.clone(), @@ -1016,10 +1008,9 @@ impl VirtioDevice for Mem { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } } diff --git a/virtio-devices/src/net.rs b/virtio-devices/src/net.rs index d7e1d1f36..13c64b270 100644 --- a/virtio-devices/src/net.rs +++ b/virtio-devices/src/net.rs @@ -868,10 +868,9 @@ impl VirtioDevice for Net { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } fn counters(&self) -> Option>> { diff --git a/virtio-devices/src/pmem.rs b/virtio-devices/src/pmem.rs index 80d41ad5d..e0ec26531 100644 --- a/virtio-devices/src/pmem.rs +++ b/virtio-devices/src/pmem.rs @@ -438,10 +438,9 @@ impl VirtioDevice for Pmem { Err(ActivateError::BadActivate) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } fn userspace_mappings(&self) -> Vec { diff --git a/virtio-devices/src/rng.rs b/virtio-devices/src/rng.rs index 9d86cf7d8..b639cc192 100644 --- a/virtio-devices/src/rng.rs +++ b/virtio-devices/src/rng.rs @@ -311,10 +311,9 @@ impl VirtioDevice for Rng { Err(ActivateError::BadActivate) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } fn set_access_platform(&mut self, access_platform: Arc) { diff --git a/virtio-devices/src/transport/pci_device.rs b/virtio-devices/src/transport/pci_device.rs index fe3cf7105..41743e419 100644 --- a/virtio-devices/src/transport/pci_device.rs +++ b/virtio-devices/src/transport/pci_device.rs @@ -300,7 +300,7 @@ pub struct VirtioPciDeviceState { } pub struct VirtioPciDeviceActivator { - interrupt: Option>, + interrupt: Arc, memory: Option>, device: Arc>, device_activated: Arc, @@ -315,7 +315,7 @@ impl VirtioPciDeviceActivator { let mut locked_device = self.device.lock().unwrap(); locked_device.activate(crate::device::ActivationContext { mem: self.memory.take().unwrap(), - interrupt_cb: self.interrupt.take().unwrap(), + interrupt_cb: self.interrupt, queues: self.queues.take().unwrap(), device_status: self.status, })?; @@ -822,7 +822,7 @@ impl VirtioPciDevice { } VirtioPciDeviceActivator { - interrupt: self.virtio_interrupt.take(), + interrupt: self.virtio_interrupt.as_ref().unwrap().clone(), memory: Some(self.memory.clone()), device: self.device.clone(), queues: Some(queues), @@ -1250,10 +1250,7 @@ impl PciDevice for VirtioPciDevice { if self.is_driver_init() { if self.device_activated.swap(false, Ordering::SeqCst) { let mut device = self.device.lock().unwrap(); - if let Some(virtio_interrupt) = device.reset() { - // Upon reset the device returns its interrupt EventFD - self.virtio_interrupt = Some(virtio_interrupt); - } + device.reset(); } // Reset queue readiness and the common configuration diff --git a/virtio-devices/src/vdpa.rs b/virtio-devices/src/vdpa.rs index e1cf48d9a..44b5cfde8 100644 --- a/virtio-devices/src/vdpa.rs +++ b/virtio-devices/src/vdpa.rs @@ -443,14 +443,13 @@ impl VirtioDevice for Vdpa { self.activate_vdpa(&mem.memory(), virtio_interrupt.as_ref(), &queues) .map_err(ActivateError::ActivateVdpa)?; - // Store the virtio interrupt handler as we need to return it on reset self.common.interrupt_cb = Some(virtio_interrupt); event!("vdpa", "activated", "id", &self.id); Ok(()) } - fn reset(&mut self) -> Option> { + fn reset(&mut self) { // Backend reset failures are logged but don't skip local cleanup: // reset must converge to fresh state regardless of backend state. if let Err(e) = self.reset_vdpa() { @@ -459,8 +458,8 @@ impl VirtioDevice for Vdpa { event!("vdpa", "reset", "id", &self.id); - // Return the virtio interrupt handler - self.common.interrupt_cb.take() + // Drop the interrupt callback clone + self.common.interrupt_cb = None; } fn set_access_platform(&mut self, access_platform: Arc) { diff --git a/virtio-devices/src/vhost_user/blk.rs b/virtio-devices/src/vhost_user/blk.rs index 7e434dd9f..5b64bbb5a 100644 --- a/virtio-devices/src/vhost_user/blk.rs +++ b/virtio-devices/src/vhost_user/blk.rs @@ -28,7 +28,7 @@ use super::{DEFAULT_VIRTIO_FEATURES, Error, Result}; use crate::seccomp_filters::Thread; use crate::thread_helper::spawn_virtio_thread; 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; @@ -307,8 +307,8 @@ impl VirtioDevice for Blk { Ok(()) } - fn reset(&mut self) -> Option> { - self.vu_common.reset(&self.id) + fn reset(&mut self) { + self.vu_common.reset(&self.id); } fn shutdown(&mut self) { diff --git a/virtio-devices/src/vhost_user/fs.rs b/virtio-devices/src/vhost_user/fs.rs index 60a60a86c..37c7c420e 100644 --- a/virtio-devices/src/vhost_user/fs.rs +++ b/virtio-devices/src/vhost_user/fs.rs @@ -25,7 +25,7 @@ use crate::thread_helper::spawn_virtio_thread; use crate::vhost_user::{VhostUserCommon, VhostUserState}; use crate::{ ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM, - VirtioCommon, VirtioDevice, VirtioDeviceType, VirtioInterrupt, VirtioSharedMemoryList, + VirtioCommon, VirtioDevice, VirtioDeviceType, VirtioSharedMemoryList, }; const NUM_QUEUE_OFFSET: usize = 1; @@ -285,8 +285,8 @@ impl VirtioDevice for Fs { Ok(()) } - fn reset(&mut self) -> Option> { - self.vu_common.reset(&self.id) + fn reset(&mut self) { + self.vu_common.reset(&self.id); } fn shutdown(&mut self) { diff --git a/virtio-devices/src/vhost_user/generic_vhost_user.rs b/virtio-devices/src/vhost_user/generic_vhost_user.rs index 0d2aaf682..3ac01406b 100644 --- a/virtio-devices/src/vhost_user/generic_vhost_user.rs +++ b/virtio-devices/src/vhost_user/generic_vhost_user.rs @@ -26,7 +26,7 @@ use crate::thread_helper::spawn_virtio_thread; use crate::vhost_user::{VhostUserCommon, VhostUserState}; use crate::{ ActivateResult, GuestMemoryMmap, GuestRegionMmap, MmapRegion, VIRTIO_F_ACCESS_PLATFORM, - VirtioCommon, VirtioDevice, VirtioInterrupt, VirtioSharedMemoryList, + VirtioCommon, VirtioDevice, VirtioSharedMemoryList, }; pub type State = VhostUserState<()>; @@ -308,8 +308,8 @@ impl VirtioDevice for GenericVhostUser { Ok(()) } - fn reset(&mut self) -> Option> { - self.vu_common.reset(&self.id) + fn reset(&mut self) { + self.vu_common.reset(&self.id); } fn shutdown(&mut self) { diff --git a/virtio-devices/src/vhost_user/mod.rs b/virtio-devices/src/vhost_user/mod.rs index 07c8d4555..9dd3d7f79 100644 --- a/virtio-devices/src/vhost_user/mod.rs +++ b/virtio-devices/src/vhost_user/mod.rs @@ -347,7 +347,6 @@ pub struct VhostUserCommon { pub vu_num_queues: usize, pub migration_started: bool, pub server: bool, - pub interrupt_cb: Option>, pub vring_bases: Option>, pub epoll_thread: Option>, } @@ -395,8 +394,6 @@ impl VhostUserCommon { ) .map_err(ActivateError::VhostUserSetup)?; - self.interrupt_cb = Some(interrupt_cb.clone()); - Ok(VhostUserEpollHandler { vu: vu.clone(), mem, @@ -428,7 +425,7 @@ impl VhostUserCommon { Ok(()) } - pub fn reset(&mut self, id: &str) -> Option> { + pub fn reset(&mut self, id: &str) { // Resume the virtio thread if it was paused. Reset must always // converge to fresh state, so backend resume / reset failures are // logged but don't skip the rest of the teardown. @@ -454,8 +451,8 @@ impl VhostUserCommon { event!("virtio-device", "reset", "id", id); - // Return the interrupt - Some(self.virtio_common.interrupt_cb.take().unwrap()) + // Drop the interrupt callback clone + self.virtio_common.interrupt_cb = None; } pub fn shutdown(&mut self) { @@ -525,12 +522,10 @@ impl VhostUserCommon { 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 { - interrupt_cb - .trigger(crate::VirtioInterruptType::Queue(i as u16)) - .ok(); - } + for i in 0..self.vu_num_queues { + self.virtio_common + .trigger_interrupt(crate::VirtioInterruptType::Queue(i as u16)) + .ok(); } Ok(()) } diff --git a/virtio-devices/src/vhost_user/net.rs b/virtio-devices/src/vhost_user/net.rs index 8563a2124..d77c80a51 100644 --- a/virtio-devices/src/vhost_user/net.rs +++ b/virtio-devices/src/vhost_user/net.rs @@ -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::{ 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; @@ -364,8 +364,8 @@ impl VirtioDevice for Net { Ok(()) } - fn reset(&mut self) -> Option> { - self.vu_common.reset(&self.id) + fn reset(&mut self) { + self.vu_common.reset(&self.id); } fn shutdown(&mut self) { diff --git a/virtio-devices/src/vsock/device.rs b/virtio-devices/src/vsock/device.rs index c20288c2d..1d5985037 100644 --- a/virtio-devices/src/vsock/device.rs +++ b/virtio-devices/src/vsock/device.rs @@ -488,10 +488,9 @@ where Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } fn shutdown(&mut self) { diff --git a/virtio-devices/src/watchdog.rs b/virtio-devices/src/watchdog.rs index 742a2e024..e362532e6 100644 --- a/virtio-devices/src/watchdog.rs +++ b/virtio-devices/src/watchdog.rs @@ -379,10 +379,9 @@ impl VirtioDevice for Watchdog { Ok(()) } - fn reset(&mut self) -> Option> { - let result = self.common.reset(); + fn reset(&mut self) { + self.common.reset(); event!("virtio-device", "reset", "id", &self.id); - result } }