From e67a60c8233918e03b5771b491aa0b9dc72d4bb2 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 22:56:38 -0800 Subject: [PATCH 1/8] refactor(virtio): add WorkerCtl for transports to drive workers The PCI transport and the vhost-user backend each carried a copy of the code that sends a wake event to a device worker and kicks its notifier. Move the event sender and the notifier from VirtioDevice into a new WorkerCtl, which the device shares with its transport, and keep the only copy of that code there. The following changes extend WorkerCtl to track device resets. VuChannel events from the vhost-user backend now also kick the notifier, so that a running worker picks them up right away. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/dev/dev.rs | 69 ++++++++++++++++++++++++++------- alioth/src/virtio/pci.rs | 31 ++++----------- alioth/src/virtio/pci_test.rs | 15 +++---- alioth/src/virtio/vu/backend.rs | 22 ++--------- 4 files changed, 74 insertions(+), 63 deletions(-) diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index 98178333..2753e298 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -114,8 +114,8 @@ pub struct WorkerParam where S: IrqSender, { - /// Receiving end of the wake event channel. The sending end is - /// [`VirtioDevice::event_tx`]. + /// Receiving end of the wake event channel. The sending end is in + /// [`WorkerCtl`]. pub(crate) event_rx: Receiver>, /// Guest memory. pub(crate) memory: Arc, @@ -158,6 +158,52 @@ where backend: B, } +/// The handle a transport uses to drive the worker thread of a virtio +/// device. +#[derive(Debug)] +pub struct WorkerCtl +where + S: IrqSender, +{ + name: Arc, + event_tx: Sender>, + notifier: Arc, +} + +impl WorkerCtl +where + S: IrqSender, +{ + pub(crate) fn new( + name: Arc, + event_tx: Sender>, + notifier: Arc, + ) -> Self { + WorkerCtl { + name, + event_tx, + notifier, + } + } + + /// Sends `event` to the worker and wakes it up. + /// + /// Returns `false` if the worker has exited. + pub fn wake(&self, event: WakeEvent) -> bool { + // A worker accepts `Start` only while it is pending, during which + // it waits on the channel instead of the notifier. + let is_start = matches!(event, WakeEvent::Start { .. }); + if let Err(e) = self.event_tx.send(event) { + log::error!("{}: failed to send event: {e}", self.name); + return false; + } + if !is_start && let Err(e) = self.notifier.notify() { + log::error!("{}: failed to wake up device: {e}", self.name); + } + true + } +} + #[derive(Debug)] pub struct VirtioDevice where @@ -169,8 +215,7 @@ where pub device_feature: u128, pub queue_regs: Arc<[QueueReg]>, pub shared_mem_regions: Option>, - pub notifier: Arc, - pub event_tx: Sender>, + pub ctl: Arc>, pub(crate) worker_handle: Option>, } @@ -178,16 +223,14 @@ impl VirtioDevice where S: IrqSender, { - fn shutdown(&mut self) -> Result<(), Box> { + fn shutdown(&mut self) { let Some(handle) = self.worker_handle.take() else { - return Ok(()); + return; }; - self.event_tx.send(WakeEvent::Shutdown)?; - self.notifier.notify()?; + self.ctl.wake(WakeEvent::Shutdown); if let Err(e) = handle.join() { log::error!("{}: failed to join worker thread: {e:?}", self.name) } - Ok(()) } pub fn new( @@ -228,14 +271,14 @@ where VirtioFeature::from_bits_retain(device_feature & !D::Feature::all().bits()), D::Feature::from_bits_truncate(device_feature) ); + let ctl = Arc::new(WorkerCtl::new(name.clone(), event_tx, notifier)); let virtio_dev = VirtioDevice { name, id, device_feature, queue_regs, worker_handle: Some(handle), - event_tx, - notifier, + ctl, device_config, shared_mem_regions, }; @@ -248,9 +291,7 @@ where S: IrqSender, { fn drop(&mut self) { - if let Err(e) = self.shutdown() { - log::error!("{}: failed to shutdown: {e}", self.name); - } + self.shutdown() } } diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index af8e2bcd..6fd5dadd 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -20,7 +20,6 @@ use std::sync::Arc; use std::sync::atomic::{AtomicU16, Ordering}; use alioth_macros::Layout; -use flume::Sender; use parking_lot::{Mutex, RwLock}; use zerocopy::{FromZeros, Immutable, IntoBytes}; @@ -39,7 +38,7 @@ use crate::pci::config::{ use crate::pci::{self, Pci, PciBar}; use crate::sync::notifier::Notifier; use crate::utils::{get_atomic_high32, get_atomic_low32, set_atomic_high32, set_atomic_low32}; -use crate::virtio::dev::{Register, StartParam, VirtioDevice, WakeEvent}; +use crate::virtio::dev::{Register, StartParam, VirtioDevice, WakeEvent, WorkerCtl}; use crate::virtio::queue::{QUEUE_SIZE_MAX, QueueReg}; use crate::virtio::{DevStatus, DeviceId, IrqSender, Result, VirtioFeature, error}; use crate::{consts, impl_mmio_for_zerocopy, mem}; @@ -193,28 +192,13 @@ where queues: Arc<[QueueReg]>, irq_sender: Arc>, notifiers: Option>, - event_tx: Sender>>, - notifier: Arc, + ctl: Arc>>, } impl VirtioPciRegisterMmio where M: MsiSender, { - fn wake_up_dev(&self, event: WakeEvent>) { - let is_start = matches!(event, WakeEvent::Start { .. }); - if let Err(e) = self.event_tx.send(event) { - log::error!("{}: failed to send event: {e}", self.name); - return; - } - if is_start { - return; - } - if let Err(e) = self.notifier.notify() { - log::error!("{}: failed to wake up device: {e}", self.name); - } - } - fn reset(&self) { let config_msix = &self.irq_sender.msix_vector.config; config_msix.store(VIRTIO_MSI_NO_VECTOR, Ordering::Release); @@ -460,7 +444,7 @@ where if status.is_empty() { self.reset(); if old.contains(DevStatus::DRIVER_OK) { - self.wake_up_dev(WakeEvent::Reset); + self.ctl.wake(WakeEvent::Reset); } } else if !old.contains(DevStatus::DRIVER_OK) && status.contains(DevStatus::DRIVER_OK) @@ -470,7 +454,7 @@ where irq_sender: self.irq_sender.clone(), notifiers: self.notifiers.clone(), }; - self.wake_up_dev(WakeEvent::Start { param }); + self.ctl.wake(WakeEvent::Start { param }); } } VirtioCommonCfg::LAYOUT_QUEUE_SELECT => { @@ -602,7 +586,7 @@ where log::warn!("{}: notifying queue-{q_index} by vm exit!", self.name); } let event = WakeEvent::Notify { q_index }; - self.wake_up_dev(event) + self.ctl.wake(event); } _ => { log::error!( @@ -947,8 +931,7 @@ where device_feature, ..Default::default() }, - event_tx: dev.event_tx.clone(), - notifier: dev.notifier.clone(), + ctl: dev.ctl.clone(), queues: dev.queue_regs.clone(), irq_sender: Arc::new(PciIrqSender { msix_vector, @@ -1015,7 +998,7 @@ where } fn reset(&self) -> pci::Result<()> { - self.registers.wake_up_dev(WakeEvent::Reset); + self.registers.ctl.wake(WakeEvent::Reset); self.registers.reset(); self.registers.reg.status.store(0, Ordering::Release); Ok(()) diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index b1aad476..f55ce30d 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -31,7 +31,7 @@ use crate::pci::cap::{ use crate::pci::config::{BAR_MEM32, BAR_MEM64, BAR_PREFETCHABLE, PciConfigArea}; use crate::pci::{Pci, PciBar}; use crate::sync::notifier::Notifier; -use crate::virtio::dev::{Register, VirtioDevice, WakeEvent}; +use crate::virtio::dev::{Register, VirtioDevice, WakeEvent, WorkerCtl}; use crate::virtio::pci::{ PciIrqSender, VIRTIO_MSI_NO_VECTOR, VirtioCommonCfg, VirtioPciCap, VirtioPciCap64, VirtioPciDevice, VirtioPciMsixVector, VirtioPciNotifyCap, VirtioPciRegister, @@ -70,8 +70,7 @@ fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver) { queues, irq_sender, notifiers: None, - event_tx, - notifier, + ctl: Arc::new(WorkerCtl::new("test-virtio-pci".into(), event_tx, notifier)), }; (mmio, event_rx) } @@ -1017,8 +1016,11 @@ fn test_notify_with_notifiers() { queues, irq_sender, notifiers: Some(Arc::new([Notifier::new().unwrap()])), - event_tx, - notifier, + ctl: Arc::new(WorkerCtl::new( + "test-virtio-pci-notifier".into(), + event_tx, + notifier, + )), }; assert_matches!( @@ -1112,8 +1114,7 @@ where device_feature: (VirtioFeature::VERSION_1 | VirtioFeature::RING_PACKED).bits(), queue_regs, shared_mem_regions: shared_mem, - notifier, - event_tx, + ctl: Arc::new(WorkerCtl::new("test-dev".into(), event_tx, notifier)), worker_handle: None, }; (dev, event_rx) diff --git a/alioth/src/virtio/vu/backend.rs b/alioth/src/virtio/vu/backend.rs index 8a648319..8619ae05 100644 --- a/alioth/src/virtio/vu/backend.rs +++ b/alioth/src/virtio/vu/backend.rs @@ -171,20 +171,6 @@ impl VuBackend { self.dev.name.as_ref() } - fn wake_up_dev(&self, event: WakeEvent) { - let is_start = matches!(event, WakeEvent::Start { .. }); - if let Err(e) = self.dev.event_tx.send(event) { - log::error!("{}: failed to send event: {e}", self.dev.name); - return; - } - if is_start { - return; - } - if let Err(e) = self.dev.notifier.notify() { - log::error!("{}: failed to wake up device: {e}", self.dev.name); - } - } - fn convert_frontend_hva(&self, hva: u64) -> Result { for r in &self.init.regions { if hva >= r.hva && hva < r.hva + r.size { @@ -296,10 +282,10 @@ impl VuBackend { let channel = Arc::new(VuChannel { conn: UnixStream::from(fd), }); - let r = self.dev.event_tx.send(WakeEvent::VuChannel { + let event = WakeEvent::VuChannel { channel: channel.clone(), - }); - if r.is_err() { + }; + if !self.dev.ctl.wake(event) { return error::SendChannel.fail(); } self.channel = Some(channel); @@ -441,7 +427,7 @@ impl VuBackend { } else { WakeEvent::Reset }; - self.wake_up_dev(event); + self.dev.ctl.wake(event); } } (VuFrontMsg::GET_CONFIG, 12..) => { From 2e6bfabc9e234524254eb78c37be2748c88236eb Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 22:59:53 -0800 Subject: [PATCH 2/8] fix(virtio): let workers acknowledge device resets A transport asks the worker of a device to reset it with a Reset event, but has no way to learn when the worker has stopped using the queues. The transport therefore reports a reset as completed right away, while the worker might still write to the rings, e.g. into memory the driver has already reused. Number each Reset event, and have the worker send the number back once it has left its event loop, drained in-flight requests, cleaned up the backend and released the guest memory layout. A worker that has not started acknowledges a reset right away. The worker is the only holder of the sending end, so its exit also completes all pending resets. WorkerCtl offers poll_reset() to transports that must not block, and wait_reset() to those that can. WorkerCtl takes a sequence number and sends the event under one lock, so that the worker sees resets in the order of their numbers. It compares the numbers in serial number arithmetic, which stays correct even if the counter wraps around. So that a reset can still complete after an error, the worker no longer exits when its event loop fails. It cleans up the backend, which also covers a partially activated device, and waits for the next start. For the same reason, vhost-vsock now logs errors during reset instead of panicking. The transports still report resets as completed right away. The following changes make them wait for the acknowledgement. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/dev/dev.rs | 152 ++++++++- alioth/src/virtio/dev/dev_test.rs | 299 +++++++++++++++++- alioth/src/virtio/dev/entropy_test.rs | 1 + alioth/src/virtio/dev/vsock/uds_vsock_test.rs | 1 + alioth/src/virtio/dev/vsock/vhost_vsock.rs | 24 +- alioth/src/virtio/pci.rs | 4 +- alioth/src/virtio/pci_test.rs | 21 +- alioth/src/virtio/virtio.rs | 4 + alioth/src/virtio/vu/backend.rs | 9 +- alioth/src/virtio/worker/io_uring_test.rs | 1 + 10 files changed, 476 insertions(+), 40 deletions(-) diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index 2753e298..a2923c71 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -26,9 +26,11 @@ use std::fmt::Debug; use std::sync::Arc; use std::sync::atomic::{AtomicU8, AtomicU16, AtomicU32, Ordering}; use std::thread::JoinHandle; +use std::time::{Duration, Instant}; use bitflags::Flags; -use flume::{Receiver, Sender}; +use flume::{Receiver, RecvTimeoutError, Sender, TryRecvError}; +use parking_lot::Mutex; use snafu::ResultExt; use crate::mem::emulated::Mmio; @@ -117,6 +119,12 @@ where /// Receiving end of the wake event channel. The sending end is in /// [`WorkerCtl`]. pub(crate) event_rx: Receiver>, + /// Sending end of the reset completion channel. The receiving end is in + /// [`WorkerCtl`]. + /// + /// Only the worker holds it, so the channel disconnects once the worker + /// exits, which completes all pending resets. + pub(crate) reset_tx: Sender, /// Guest memory. pub(crate) memory: Arc, /// Queue registers, shared with the transport. @@ -139,7 +147,11 @@ where Start { param: StartParam, }, - Reset, + /// Resets the device. Once the worker no longer uses the queues, it + /// sends `seq` back through [`WorkerParam::reset_tx`]. + Reset { + seq: u64, + }, } #[derive(Debug, PartialEq, Eq)] @@ -168,6 +180,37 @@ where name: Arc, event_tx: Sender>, notifier: Arc, + /// Sequence number of the last requested reset. + /// + /// The lock is held while sending the reset event, so that the worker + /// sees resets in the order of their sequence numbers. + reset_seq: Mutex, + reset_acks: Mutex, +} + +/// Returns `true` if reset `seq` is not later than reset `done`. +/// +/// Sequence numbers are compared in serial number arithmetic, which stays +/// correct after the counter wraps around, as long as the two are less than +/// 2^63 resets apart. +fn reset_completed(done: u64, seq: u64) -> bool { + (done.wrapping_sub(seq) as i64) >= 0 +} + +/// Resets the worker has reported as completed. +#[derive(Debug)] +struct ResetAcks { + rx: Receiver, + /// Sequence number of the last completed reset. + done: u64, +} + +impl ResetAcks { + fn complete(&mut self, seq: u64) { + if !reset_completed(self.done, seq) { + self.done = seq; + } + } } impl WorkerCtl @@ -178,11 +221,17 @@ where name: Arc, event_tx: Sender>, notifier: Arc, + reset_rx: Receiver, ) -> Self { WorkerCtl { name, event_tx, notifier, + reset_seq: Mutex::new(0), + reset_acks: Mutex::new(ResetAcks { + rx: reset_rx, + done: 0, + }), } } @@ -202,6 +251,52 @@ where } true } + + /// Asks the worker to reset the device. + /// + /// Returns the sequence number of this reset, which + /// [`poll_reset`](Self::poll_reset) and [`wait_reset`](Self::wait_reset) + /// take. + pub fn request_reset(&self) -> u64 { + let mut reset_seq = self.reset_seq.lock(); + let seq = reset_seq.wrapping_add(1); + *reset_seq = seq; + self.wake(WakeEvent::Reset { seq }); + seq + } + + /// Returns `true` if the worker has completed reset `seq`. + /// + /// The worker handles resets in order, so this also means that all + /// earlier resets have completed. + pub fn poll_reset(&self, seq: u64) -> bool { + let mut acks = self.reset_acks.lock(); + loop { + match acks.rx.try_recv() { + Ok(done) => acks.complete(done), + Err(TryRecvError::Empty) => break reset_completed(acks.done, seq), + // The worker has exited, so it no longer uses the queues. + Err(TryRecvError::Disconnected) => break true, + } + } + } + + /// Waits until the worker has completed reset `seq`. + pub fn wait_reset(&self, seq: u64, timeout: Duration) -> Result<()> { + let deadline = Instant::now() + timeout; + let mut acks = self.reset_acks.lock(); + while !reset_completed(acks.done, seq) { + match acks.rx.recv_deadline(deadline) { + Ok(done) => acks.complete(done), + Err(RecvTimeoutError::Disconnected) => break, + Err(RecvTimeoutError::Timeout) => { + let name = self.name.clone(); + return error::ResetTimeout { name, timeout }.fail(); + } + } + } + Ok(()) + } } #[derive(Debug)] @@ -260,8 +355,10 @@ where let shared_mem_regions = dev.shared_mem_regions(); let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); let param = WorkerParam { event_rx, + reset_tx, memory, queue_regs: queue_regs.clone(), }; @@ -271,14 +368,14 @@ where VirtioFeature::from_bits_retain(device_feature & !D::Feature::all().bits()), D::Feature::from_bits_truncate(device_feature) ); - let ctl = Arc::new(WorkerCtl::new(name.clone(), event_tx, notifier)); + let ctl = WorkerCtl::new(name.clone(), event_tx, notifier, reset_rx); let virtio_dev = VirtioDevice { name, id, device_feature, queue_regs, worker_handle: Some(handle), - ctl, + ctl: Arc::new(ctl), device_config, shared_mem_regions, }; @@ -328,6 +425,9 @@ where pub dev: D, param: WorkerParam, pub state: WorkerState, + /// The reset being handled, acknowledged once the worker no longer uses + /// the queues. + reset_seq: Option, } impl Context @@ -335,6 +435,12 @@ where D: Virtio, S: IrqSender, { + fn ack_reset(&self, seq: u64) { + log::debug!("{}: reset {seq} completed", self.dev.name()); + // The transport might be gone, e.g. at VMM shutdown. + let _ = self.param.reset_tx.send(seq); + } + fn handle_wake_events(&mut self, backend: &mut B) -> Result<()> where B: ActiveBackend, @@ -351,9 +457,10 @@ where } #[cfg(target_os = "linux")] WakeEvent::VuChannel { channel } => self.dev.set_vu_channel(channel), - WakeEvent::Reset => { - log::info!("{}: guest requested reset", self.dev.name()); + WakeEvent::Reset { seq } => { + log::info!("{}: reset requested", self.dev.name()); self.state = WorkerState::Pending; + self.reset_seq = Some(seq); break; } } @@ -364,7 +471,8 @@ where fn wait_start(&mut self) -> Option> { for wake_event in self.param.event_rx.iter() { match wake_event { - WakeEvent::Reset => {} + // The device has not started, so there is nothing to reset. + WakeEvent::Reset { seq } => self.ack_reset(seq), WakeEvent::Start { param } => { self.state = WorkerState::Running; return Some(param); @@ -413,6 +521,7 @@ where dev, param, state: WorkerState::Pending, + reset_seq: None, }, backend, }; @@ -439,8 +548,13 @@ where VirtioFeature::from_bits_retain(param.feature & !D::Feature::all().bits()), D::Feature::from_bits_truncate(param.feature) ); - self.backend - .event_loop(ram, &mut self.context, queues, param) + let ret = self + .backend + .event_loop(ram, &mut self.context, queues, param); + // Clean up even if the event loop failed, since the device might + // have been partially activated. + let reset = self.backend.reset(&mut self.context.dev); + ret.and(reset) } fn loop_until_reset(&mut self) -> Result<()> { @@ -461,7 +575,7 @@ where Ok(Some(Queue::new(split_queue, reg, &ram))) }; let queues: Result> = queue_regs.iter().map(new_queue).collect(); - self.event_loop(&mut (queues?), &ram, ¶m)?; + self.event_loop(&mut (queues?), &ram, ¶m) } else { let new_queue = |reg| { let Some(split_queue) = SplitQueue::new(reg, &ram, event_idx)? else { @@ -470,17 +584,23 @@ where Ok(Some(Queue::new(split_queue, reg, &ram))) }; let queues: Result> = queue_regs.iter().map(new_queue).collect(); - self.event_loop(&mut (queues?), &ram, ¶m)?; - }; - self.backend.reset(&mut self.context.dev)?; - Ok(()) + self.event_loop(&mut (queues?), &ram, ¶m) + } } fn do_work(mut self) { while self.context.state != WorkerState::Shutdown { if let Err(e) = self.loop_until_reset() { - log::error!("worker {}: {e:?}", self.context.dev.name(),); - return; + log::error!("worker {}: {e:?}", self.context.dev.name()); + // Keep the worker alive, so that the driver can still reset + // and restart the device. + if self.context.state == WorkerState::Running { + self.context.state = WorkerState::Pending; + } + } + // The memory layout lock is released by now. + if let Some(seq) = self.context.reset_seq.take() { + self.context.ack_reset(seq); } } log::debug!("worker {}: done", self.context.dev.name()) diff --git a/alioth/src/virtio/dev/dev_test.rs b/alioth/src/virtio/dev/dev_test.rs index 677f5082..b0d37408 100644 --- a/alioth/src/virtio/dev/dev_test.rs +++ b/alioth/src/virtio/dev/dev_test.rs @@ -12,9 +12,24 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::sync::atomic::Ordering; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use std::thread::{self, JoinHandle}; +use std::time::Duration; -use super::Register; +use assert_matches::assert_matches; +use mio::Registry; +use mio::event::Event; + +use crate::sync::notifier::Notifier; +use crate::virtio::dev::entropy::{EntropyConfig, EntropyFeature}; +use crate::virtio::dev::{Register, StartParam, Virtio, WakeEvent, WorkerCtl, WorkerParam}; +use crate::virtio::queue::VirtQueue; +use crate::virtio::tests::{FakeIrqSender, fixture_queues, fixture_ram_bus}; +use crate::virtio::worker::mio::{ActiveMio, Mio, VirtioMio}; +use crate::virtio::{DeviceId, Error, IrqSender, Result, VirtioFeature, error}; + +const TIMEOUT: Duration = Duration::from_secs(5); #[test] fn test_virtio_register_get_driver_feature() { @@ -32,3 +47,283 @@ fn test_virtio_register_get_driver_feature() { | ((0x0123_4567u128) << 96); assert_eq!(reg.get_driver_feature(), expected); } + +/// A device that only records what the worker does to it. +/// +/// It borrows the config and feature types of the entropy device, which +/// has neither. +#[derive(Debug, Default)] +struct FakeDev { + /// Makes activation fail, like a backend rejecting the driver setup. + fail_activate: Arc, + /// Counts the calls to [`VirtioMio::reset`]. + resets: Arc, +} + +impl Virtio for FakeDev { + type Config = EntropyConfig; + type Feature = EntropyFeature; + + fn name(&self) -> &str { + "fake" + } + + fn id(&self) -> DeviceId { + DeviceId::ENTROPY + } + + fn num_queues(&self) -> u16 { + 1 + } + + fn config(&self) -> Arc { + Arc::new(EntropyConfig) + } + + fn feature(&self) -> u128 { + 0 + } + + fn spawn_worker(self, param: WorkerParam) -> Result<(JoinHandle<()>, Arc)> + where + S: IrqSender, + { + Mio::spawn_worker(self, param) + } +} + +impl VirtioMio for FakeDev { + fn activate<'m, Q, S>( + &mut self, + _feature: u128, + _active_mio: &mut ActiveMio<'_, '_, 'm, Q, S>, + ) -> Result<()> + where + Q: VirtQueue<'m>, + S: IrqSender, + { + if self.fail_activate.load(Ordering::Acquire) { + return error::InvalidBuffer.fail(); + } + Ok(()) + } + + fn handle_queue<'m, Q, S>( + &mut self, + _index: u16, + _active_mio: &mut ActiveMio<'_, '_, 'm, Q, S>, + ) -> Result<()> + where + Q: VirtQueue<'m>, + S: IrqSender, + { + Ok(()) + } + + fn handle_event<'m, Q, S>( + &mut self, + _event: &Event, + _active_mio: &mut ActiveMio<'_, '_, 'm, Q, S>, + ) -> Result<()> + where + Q: VirtQueue<'m>, + S: IrqSender, + { + Ok(()) + } + + fn reset(&mut self, _registry: &Registry) { + self.resets.fetch_add(1, Ordering::AcqRel); + } +} + +struct TestWorker { + ctl: WorkerCtl, + handle: JoinHandle<()>, + fail_activate: Arc, + resets: Arc, +} + +impl TestWorker { + fn spawn() -> Self { + let dev = FakeDev::default(); + let fail_activate = dev.fail_activate.clone(); + let resets = dev.resets.clone(); + let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); + let param = WorkerParam { + event_rx, + reset_tx, + memory: Arc::new(fixture_ram_bus()), + queue_regs: Arc::from(fixture_queues(1)), + }; + let (handle, notifier) = dev.spawn_worker(param).unwrap(); + TestWorker { + ctl: WorkerCtl::new("fake".into(), event_tx, notifier, reset_rx), + handle, + fail_activate, + resets, + } + } + + fn start(&self) { + let (q_tx, _) = flume::unbounded(); + let param = StartParam { + feature: VirtioFeature::VERSION_1.bits(), + irq_sender: Arc::new(FakeIrqSender { q_tx }), + notifiers: None, + }; + assert!(self.ctl.wake(WakeEvent::Start { param })); + } + + fn reset(&self) { + let seq = self.ctl.request_reset(); + assert_matches!(self.ctl.wait_reset(seq, TIMEOUT), Ok(())); + assert!(self.ctl.poll_reset(seq)); + } + + fn resets(&self) -> u32 { + self.resets.load(Ordering::Acquire) + } + + fn shutdown(self) { + assert!(self.ctl.wake(WakeEvent::Shutdown)); + self.handle.join().unwrap(); + } +} + +#[test] +fn test_worker_reset_before_start() { + let worker = TestWorker::spawn(); + worker.reset(); + // The device has not started, so there was nothing to clean up. + assert_eq!(worker.resets(), 0); + worker.shutdown(); +} + +#[test] +fn test_worker_reset_after_start() { + let worker = TestWorker::spawn(); + worker.start(); + worker.reset(); + // The worker acknowledges a reset only after cleaning up. + assert_eq!(worker.resets(), 1); + + worker.start(); + worker.reset(); + assert_eq!(worker.resets(), 2); + worker.shutdown(); +} + +#[test] +fn test_worker_survives_activation_failure() { + let worker = TestWorker::spawn(); + worker.fail_activate.store(true, Ordering::Release); + worker.start(); + // The worker cleans up the partially activated device, and stays alive + // to complete the reset. + worker.reset(); + assert_eq!(worker.resets(), 1); + + worker.fail_activate.store(false, Ordering::Release); + worker.start(); + worker.reset(); + assert_eq!(worker.resets(), 2); + worker.shutdown(); +} + +#[test] +fn test_worker_exit_completes_reset() { + let worker = TestWorker::spawn(); + worker.start(); + let TestWorker { ctl, handle, .. } = worker; + assert!(ctl.wake(WakeEvent::Shutdown)); + handle.join().unwrap(); + + // An exited worker no longer uses the queues. + let seq = ctl.request_reset(); + assert!(ctl.poll_reset(seq)); + assert_matches!(ctl.wait_reset(seq, TIMEOUT), Ok(())); +} + +#[test] +fn test_wait_reset_timeout() { + let (event_tx, _event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); + let notifier = Arc::new(Notifier::new().unwrap()); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + + let seq1 = ctl.request_reset(); + let seq2 = ctl.request_reset(); + assert!(!ctl.poll_reset(seq1)); + assert_matches!( + ctl.wait_reset(seq1, Duration::from_millis(10)), + Err(Error::ResetTimeout { timeout, .. }) if timeout == Duration::from_millis(10) + ); + + // Completing a reset also completes the earlier ones. + reset_tx.send(seq2).unwrap(); + assert!(ctl.poll_reset(seq1)); + assert_matches!(ctl.wait_reset(seq2, Duration::ZERO), Ok(())); +} + +#[test] +fn test_reset_seq_wraps_around() { + let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); + let notifier = Arc::new(Notifier::new().unwrap()); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + // Pretend that almost all sequence numbers have been used up. + *ctl.reset_seq.lock() = u64::MAX - 1; + ctl.reset_acks.lock().done = u64::MAX - 1; + + let seq1 = ctl.request_reset(); + let seq2 = ctl.request_reset(); + assert_eq!((seq1, seq2), (u64::MAX, 0)); + for seq in [seq1, seq2] { + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { seq: s }) if s == seq); + } + assert!(!ctl.poll_reset(seq1)); + + reset_tx.send(seq1).unwrap(); + assert!(ctl.poll_reset(seq1)); + assert!(!ctl.poll_reset(seq2)); + + reset_tx.send(seq2).unwrap(); + assert!(ctl.poll_reset(seq1)); + assert_matches!(ctl.wait_reset(seq2, Duration::ZERO), Ok(())); +} + +#[test] +fn test_request_reset_in_order() { + let (event_tx, event_rx) = flume::unbounded(); + let notifier = Arc::new(Notifier::new().unwrap()); + let reset_rx = flume::unbounded().1; + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + let ctl = Arc::new(ctl); + + let threads: Vec<_> = (0..4) + .map(|_| { + let ctl = ctl.clone(); + thread::spawn(move || { + for _ in 0..100 { + ctl.request_reset(); + } + }) + }) + .collect(); + for t in threads { + t.join().unwrap(); + } + + // The worker sees the resets in the order of their sequence numbers, + // even when they are requested concurrently. + let seqs: Vec = event_rx + .try_iter() + .map(|event| match event { + WakeEvent::Reset { seq } => seq, + event => panic!("unexpected event: {event:?}"), + }) + .collect(); + assert_eq!(seqs, (1..=400).collect::>()); +} diff --git a/alioth/src/virtio/dev/entropy_test.rs b/alioth/src/virtio/dev/entropy_test.rs index ba613baa..3b91e941 100644 --- a/alioth/src/virtio/dev/entropy_test.rs +++ b/alioth/src/virtio/dev/entropy_test.rs @@ -79,6 +79,7 @@ fn entropy_test() { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, + reset_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs, }) diff --git a/alioth/src/virtio/dev/vsock/uds_vsock_test.rs b/alioth/src/virtio/dev/vsock/uds_vsock_test.rs index 70874ada..534d5594 100644 --- a/alioth/src/virtio/dev/vsock/uds_vsock_test.rs +++ b/alioth/src/virtio/dev/vsock/uds_vsock_test.rs @@ -131,6 +131,7 @@ impl<'m> VsockTest<'m> { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, + reset_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs, }) diff --git a/alioth/src/virtio/dev/vsock/vhost_vsock.rs b/alioth/src/virtio/dev/vsock/vhost_vsock.rs index 9a18e7b3..49c60397 100644 --- a/alioth/src/virtio/dev/vsock/vhost_vsock.rs +++ b/alioth/src/virtio/dev/vsock/vhost_vsock.rs @@ -209,20 +209,24 @@ impl VirtioMio for VhostVsock { } fn reset(&mut self, registry: &Registry) { - self.vhost_dev.vsock_set_running(false).unwrap(); + let name = &*self.name; + if let Err(e) = self.vhost_dev.vsock_set_running(false) { + log::error!("{name}: failed to stop vhost-vsock: {e:?}"); + } for (index, error_fd) in self.error_fds.iter_mut().enumerate() { let Some(err_fd) = error_fd else { continue; }; - self.vhost_dev - .set_virtq_err(&VirtqFile { - index: index as _, - fd: VHOST_FILE_UNBIND, - }) - .unwrap(); - registry - .deregister(&mut SourceFd(&err_fd.as_raw_fd())) - .unwrap(); + let unbind = VirtqFile { + index: index as _, + fd: VHOST_FILE_UNBIND, + }; + if let Err(e) = self.vhost_dev.set_virtq_err(&unbind) { + log::error!("{name}: queue-{index}: failed to unbind error fd: {e:?}"); + } + if let Err(e) = registry.deregister(&mut SourceFd(&err_fd.as_raw_fd())) { + log::error!("{name}: queue-{index}: failed to deregister error fd: {e:?}"); + } *error_fd = None; } } diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index 6fd5dadd..bbeae4e1 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -444,7 +444,7 @@ where if status.is_empty() { self.reset(); if old.contains(DevStatus::DRIVER_OK) { - self.ctl.wake(WakeEvent::Reset); + self.ctl.request_reset(); } } else if !old.contains(DevStatus::DRIVER_OK) && status.contains(DevStatus::DRIVER_OK) @@ -998,7 +998,7 @@ where } fn reset(&self) -> pci::Result<()> { - self.registers.ctl.wake(WakeEvent::Reset); + self.registers.ctl.request_reset(); self.registers.reset(); self.registers.reg.status.store(0, Ordering::Release); Ok(()) diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index f55ce30d..73b95267 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -70,7 +70,12 @@ fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver) { queues, irq_sender, notifiers: None, - ctl: Arc::new(WorkerCtl::new("test-virtio-pci".into(), event_tx, notifier)), + ctl: Arc::new(WorkerCtl::new( + "test-virtio-pci".into(), + event_tx, + notifier, + flume::unbounded().1, + )), }; (mmio, event_rx) } @@ -624,7 +629,7 @@ fn test_device_status_valid_transitions() { mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), Ok(0) ); - assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset)); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); assert!(event_rx.is_empty()); assert!(!queues[0].enabled.load(Ordering::Acquire)); assert_matches!( @@ -903,7 +908,7 @@ fn test_device_reset_queue_registers() { mmio.write(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1, 0), Ok(Action::None) ); - assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset)); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); assert!(event_rx.is_empty()); // Verify all queue registers are reset @@ -1020,6 +1025,7 @@ fn test_notify_with_notifiers() { "test-virtio-pci-notifier".into(), event_tx, notifier, + flume::unbounded().1, )), }; @@ -1114,7 +1120,12 @@ where device_feature: (VirtioFeature::VERSION_1 | VirtioFeature::RING_PACKED).bits(), queue_regs, shared_mem_regions: shared_mem, - ctl: Arc::new(WorkerCtl::new("test-dev".into(), event_tx, notifier)), + ctl: Arc::new(WorkerCtl::new( + "test-dev".into(), + event_tx, + notifier, + flume::unbounded().1, + )), worker_handle: None, }; (dev, event_rx) @@ -1541,7 +1552,7 @@ fn test_virtio_pci_device_pci_reset() { // Queue should be disabled assert!(!pci_dev.registers.queues[0].enabled.load(Ordering::Acquire)); // Reset event sent - assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset)); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); } #[test] diff --git a/alioth/src/virtio/virtio.rs b/alioth/src/virtio/virtio.rs index b566d594..222e46d6 100644 --- a/alioth/src/virtio/virtio.rs +++ b/alioth/src/virtio/virtio.rs @@ -28,6 +28,8 @@ pub mod worker; use std::fmt::Debug; use std::os::fd::BorrowedFd; use std::path::Path; +use std::sync::Arc; +use std::time::Duration; use bitflags::Flags; use snafu::Snafu; @@ -79,6 +81,8 @@ pub enum Error { InvalidMsixVector { vector: u16 }, #[snafu(display("Invalid virtq buffer"))] InvalidBuffer, + #[snafu(display("{name}: device did not complete reset within {timeout:?}"))] + ResetTimeout { name: Arc, timeout: Duration }, #[cfg(target_os = "linux")] #[snafu(display("vhost-user error"), context(false))] Vu { source: Box }, diff --git a/alioth/src/virtio/vu/backend.rs b/alioth/src/virtio/vu/backend.rs index 8619ae05..34cd0e33 100644 --- a/alioth/src/virtio/vu/backend.rs +++ b/alioth/src/virtio/vu/backend.rs @@ -421,13 +421,12 @@ impl VuBackend { self.status = new; log::debug!("{name}: set status: {old:x?} -> {new:x?}"); if (old ^ new).contains(DevStatus::DRIVER_OK) { - let event = if new.contains(DevStatus::DRIVER_OK) { + if new.contains(DevStatus::DRIVER_OK) { let param = self.parse_init()?; - WakeEvent::Start { param } + self.dev.ctl.wake(WakeEvent::Start { param }); } else { - WakeEvent::Reset - }; - self.dev.ctl.wake(event); + self.dev.ctl.request_reset(); + } } } (VuFrontMsg::GET_CONFIG, 12..) => { diff --git a/alioth/src/virtio/worker/io_uring_test.rs b/alioth/src/virtio/worker/io_uring_test.rs index 168afec5..904165df 100644 --- a/alioth/src/virtio/worker/io_uring_test.rs +++ b/alioth/src/virtio/worker/io_uring_test.rs @@ -138,6 +138,7 @@ fn io_uring_drain_test() { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, + reset_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs.clone(), }) From 172dd36df406a86e0b60888063df1d0d2c2f7a11 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:04:55 -0800 Subject: [PATCH 3/8] fix(virtio/pci): complete device resets only after the worker stops When the driver writes 0 to device_status, the transport clears the registers and reports the reset as completed right away, while the worker might still be using the queues. The driver then reuses the ring memory, which the worker can still write into. Pci::reset, which runs when the VM reboots, has the same problem. For a reset requested by the driver, keep all registers, including the non-zero device_status, until the worker acknowledges the reset. The driver must poll device_status until it reads 0 before reinitializing the device (VirtIO spec 1.4, Sec 4.1.4.3.2), so the vCPU does not wait. The transport completes the reset once a later register access finds the acknowledgement, and ignores all writes in the meantime. A device that has not started completes its reset right away. Pci::reset waits for the worker instead, for at most 10 seconds. If the worker does not complete the reset in time, the reboot fails with an error rather than hanging on a stuck worker. Queue notifications do not touch the registers, so they skip the new lock. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/dev/dev.rs | 7 + alioth/src/virtio/pci.rs | 149 +++++++++++++------ alioth/src/virtio/pci_test.rs | 262 +++++++++++++++++++++++++++------- 3 files changed, 326 insertions(+), 92 deletions(-) diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index a2923c71..48d3a76d 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -95,6 +95,13 @@ impl Register { const TOKEN_WARKER: u64 = 1 << 63; +/// How long a transport waits for the worker to reset a device when it +/// cannot leave the waiting to the driver. +/// +/// It is longer than the time the io_uring worker waits for in-flight +/// requests when it leaves its event loop. +pub(crate) const RESET_TIMEOUT: Duration = Duration::from_secs(10); + #[derive(Debug, Clone)] pub struct StartParam where diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index bbeae4e1..09459840 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -18,12 +18,14 @@ use std::mem::size_of; use std::os::fd::{AsFd, AsRawFd, BorrowedFd}; use std::sync::Arc; use std::sync::atomic::{AtomicU16, Ordering}; +use std::time::Duration; use alioth_macros::Layout; use parking_lot::{Mutex, RwLock}; use zerocopy::{FromZeros, Immutable, IntoBytes}; use crate::device::Pause; +use crate::errors::BoxTrace; use crate::hv::{IrqFd, MsiSender, NotifierRegistry}; use crate::mem::emulated::{Action, Mmio}; use crate::mem::{MemRange, MemRegion, MemRegionCallback, MemRegionEntry}; @@ -38,7 +40,7 @@ use crate::pci::config::{ use crate::pci::{self, Pci, PciBar}; use crate::sync::notifier::Notifier; use crate::utils::{get_atomic_high32, get_atomic_low32, set_atomic_high32, set_atomic_low32}; -use crate::virtio::dev::{Register, StartParam, VirtioDevice, WakeEvent, WorkerCtl}; +use crate::virtio::dev::{RESET_TIMEOUT, Register, StartParam, VirtioDevice, WakeEvent, WorkerCtl}; use crate::virtio::queue::{QUEUE_SIZE_MAX, QueueReg}; use crate::virtio::{DevStatus, DeviceId, IrqSender, Result, VirtioFeature, error}; use crate::{consts, impl_mmio_for_zerocopy, mem}; @@ -193,13 +195,22 @@ where irq_sender: Arc>, notifiers: Option>, ctl: Arc>>, + /// Sequence number of the device reset in progress, if any. + /// + /// Writes to the common configuration hold the lock, so that they never + /// interleave with a reset. + reset_seq: Mutex>, } impl VirtioPciRegisterMmio where M: MsiSender, { - fn reset(&self) { + /// Returns the registers to their initial values. + /// + /// The worker must not be using the queues, i.e. it either has not + /// started or has completed the reset. + fn complete_reset(&self) { let config_msix = &self.irq_sender.msix_vector.config; config_msix.store(VIRTIO_MSI_NO_VECTOR, Ordering::Release); for q_vector in self.irq_sender.msix_vector.queues.iter() { @@ -216,6 +227,55 @@ where q.driver.store(0, Ordering::Release); q.device.store(0, Ordering::Release); } + // Clear the status last, since it tells the driver that the reset + // has completed. + self.reg.status.store(0, Ordering::Release); + } + + /// Polls a pending reset and completes it once the worker stops. + /// + /// Returns `true` if no reset is in progress. + fn poll_reset(&self, reset_seq: &mut Option) -> bool { + let Some(seq) = *reset_seq else { + return true; + }; + if !self.ctl.poll_reset(seq) { + return false; + } + self.complete_reset(); + *reset_seq = None; + true + } + + /// Resets the device and waits for the worker to complete the reset. + fn reset_sync(&self, timeout: Duration) -> Result<()> { + let mut reset_seq = self.reset_seq.lock(); + let seq = self.ctl.request_reset(); + self.ctl.wait_reset(seq, timeout)?; + self.complete_reset(); + *reset_seq = None; + Ok(()) + } + + fn notify_queue(&self, offset: usize, size: u8, val: u64) { + let q_index = (offset - VirtioPciRegister::OFFSET_QUEUE_NOTIFY) / size_of::(); + if q_index >= self.queues.len() { + self.log_invalid_write(offset, size, val); + return; + } + if self.notifiers.is_some() { + log::warn!("{}: notifying queue-{q_index} by vm exit!", self.name); + } + let q_index = q_index as u16; + self.ctl.wake(WakeEvent::Notify { q_index }); + } + + fn log_invalid_write(&self, offset: usize, size: u8, val: u64) { + log::error!( + "{}: write 0x{val:0width$x} to invalid register offset = {offset:#x}", + self.name, + width = 2 * size as usize + ); } /// Returns the queue `q_sel` if the driver may still change its ring @@ -294,7 +354,12 @@ where self.irq_sender.msix_vector.config.load(Ordering::Acquire) as u64 } VirtioCommonCfg::LAYOUT_NUM_QUEUES => self.queues.len() as u64, - VirtioCommonCfg::LAYOUT_DEVICE_STATUS => reg.status.load(Ordering::Acquire) as u64, + VirtioCommonCfg::LAYOUT_DEVICE_STATUS => { + // The driver polls device_status until a reset completes. + let mut reset_seq = self.reset_seq.lock(); + self.poll_reset(&mut reset_seq); + reg.status.load(Ordering::Acquire) as u64 + } VirtioCommonCfg::LAYOUT_CONFIG_GENERATION => { 0 // TODO: support device config change at runtime } @@ -391,8 +456,27 @@ where } fn write(&self, offset: u64, size: u8, val: u64) -> mem::Result { + let offset = offset as usize; + // Queue notifications change no registers, so they skip the reset + // lock and stay fast. + if offset >= VirtioPciRegister::OFFSET_QUEUE_NOTIFY { + self.notify_queue(offset, size, val); + return Ok(Action::None); + } + let mut reset_seq = self.reset_seq.lock(); + if !self.poll_reset(&mut reset_seq) { + // The driver must wait for device_status to read 0 before it + // reinitializes the device (VirtIO spec 1.4, Sec 4.1.4.3.2). + if offset != VirtioCommonCfg::OFFSET_DEVICE_STATUS || val != 0 { + log::warn!( + "{}: ignored write {val:#x} to {offset:#x} during reset", + self.name + ); + } + return Ok(Action::None); + } let reg = &self.reg; - match (offset as usize, size as usize) { + match (offset, size as usize) { VirtioCommonCfg::LAYOUT_DEVICE_FEATURE_SELECT => { reg.device_feature_sel.store(val as u32, Ordering::Release); } @@ -440,21 +524,26 @@ where log::warn!("{}: invalid status change: {old:#x} -> {val:#x}", self.name); return Ok(Action::None); } - reg.status.store(status.bits(), Ordering::Release); if status.is_empty() { - self.reset(); if old.contains(DevStatus::DRIVER_OK) { - self.ctl.request_reset(); + // The worker might still be using the queues. Keep + // all registers, including the non-zero status, until + // it completes the reset. + *reset_seq = Some(self.ctl.request_reset()); + } else { + self.complete_reset(); + } + } else { + reg.status.store(status.bits(), Ordering::Release); + if !old.contains(DevStatus::DRIVER_OK) && status.contains(DevStatus::DRIVER_OK) + { + let param = StartParam { + feature: reg.get_driver_feature(), + irq_sender: self.irq_sender.clone(), + notifiers: self.notifiers.clone(), + }; + self.ctl.wake(WakeEvent::Start { param }); } - } else if !old.contains(DevStatus::DRIVER_OK) - && status.contains(DevStatus::DRIVER_OK) - { - let param = StartParam { - feature: reg.get_driver_feature(), - irq_sender: self.irq_sender.clone(), - notifiers: self.notifiers.clone(), - }; - self.ctl.wake(WakeEvent::Start { param }); } } VirtioCommonCfg::LAYOUT_QUEUE_SELECT => { @@ -575,26 +664,7 @@ where log::warn!("{}: queue reset requested but not supported", self.name); } } - (offset, _) - if offset >= VirtioPciRegister::OFFSET_QUEUE_NOTIFY - && offset - < VirtioPciRegister::OFFSET_QUEUE_NOTIFY - + size_of::() * self.queues.len() => - { - let q_index = (offset - VirtioPciRegister::OFFSET_QUEUE_NOTIFY) as u16 / 4; - if self.notifiers.is_some() { - log::warn!("{}: notifying queue-{q_index} by vm exit!", self.name); - } - let event = WakeEvent::Notify { q_index }; - self.ctl.wake(event); - } - _ => { - log::error!( - "{}: write 0x{val:0width$x} to invalid register offset = {offset:#x}", - self.name, - width = 2 * size as usize - ); - } + _ => self.log_invalid_write(offset, size, val), } Ok(Action::None) } @@ -932,6 +1002,7 @@ where ..Default::default() }, ctl: dev.ctl.clone(), + reset_seq: Mutex::new(None), queues: dev.queue_regs.clone(), irq_sender: Arc::new(PciIrqSender { msix_vector, @@ -998,10 +1069,8 @@ where } fn reset(&self) -> pci::Result<()> { - self.registers.ctl.request_reset(); - self.registers.reset(); - self.registers.reg.status.store(0, Ordering::Release); - Ok(()) + let ret = self.registers.reset_sync(RESET_TIMEOUT); + ret.box_trace(pci::error::Reset) } } diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index 73b95267..f05621a4 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -16,9 +16,11 @@ use std::mem::size_of; use std::os::fd::AsRawFd; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU16, AtomicU64, Ordering}; +use std::thread; +use std::time::Duration; use assert_matches::assert_matches; -use flume::Receiver; +use flume::{Receiver, Sender}; use parking_lot::{Mutex, RwLock}; use rstest::rstest; @@ -38,13 +40,16 @@ use crate::virtio::pci::{ VirtioPciRegisterMmio, }; use crate::virtio::queue::{QUEUE_SIZE_MAX, QueueReg}; -use crate::virtio::{DevStatus, DeviceId, IrqSender, VirtioFeature}; +use crate::virtio::{DevStatus, DeviceId, Error, IrqSender, VirtioFeature}; type TestMmio = VirtioPciRegisterMmio; type TestWakeReceiver = flume::Receiver>>; -fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver) { +/// Creates the registers of a device, together with the worker ends of the +/// wake event channel and the reset completion channel. +fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver, Sender) { let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let msi_sender = TestMsiSender::default(); let msix_table = Arc::new(MsixTableMmio { @@ -74,10 +79,11 @@ fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver) { "test-virtio-pci".into(), event_tx, notifier, - flume::unbounded().1, + reset_rx, )), + reset_seq: Mutex::new(None), }; - (mmio, event_rx) + (mmio, event_rx, reset_tx) } #[rstest] @@ -94,7 +100,7 @@ fn test_queue_address_reads(#[case] offset: usize, #[case] expected: u64) { device: AtomicU64::new(0x0123_4567_89ab_cdef), ..Default::default() }]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset as u64, 4), Ok(val) if val == expected); } @@ -123,7 +129,7 @@ fn test_queue_alignment( device: AtomicU64::new(0x3000_0000), ..Default::default() }]); - let (mmio, _event_rx) = create_test_mmio(queues); + let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); if packed { assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_DRIVER_FEATURE_SELECT as u64, 4, 1), @@ -150,7 +156,7 @@ fn test_queue_alignment( #[case(VirtioCommonCfg::OFFSET_QUEUE_ENABLE, 2, 1)] fn test_queue_registers_write_read(#[case] offset: usize, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 0), @@ -170,7 +176,7 @@ fn test_queue_size_write_read(#[case] packed: bool, #[case] input: u64, #[case] size: AtomicU16::new(128), ..Default::default() }]); - let (mmio, _event_rx) = create_test_mmio(queues); + let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); if packed { assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_DRIVER_FEATURE_SELECT as u64, 4, 1), @@ -195,7 +201,7 @@ fn test_queue_size_write_read(#[case] packed: bool, #[case] input: u64, #[case] #[test] fn test_queue_size_locked_when_enabled() { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 0), @@ -219,7 +225,7 @@ fn test_queue_size_locked_when_enabled() { #[test] fn test_queue_addrs_locked_when_enabled() { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues.clone()); + let (mmio, _, _reset_tx) = create_test_mmio(queues.clone()); let offsets = [ VirtioCommonCfg::OFFSET_QUEUE_DESC_LO, @@ -274,7 +280,7 @@ fn test_queue_addrs_locked_when_enabled() { #[case(VirtioCommonCfg::OFFSET_QUEUE_ENABLE, 2, 1)] fn test_out_of_bounds_queue_writes(#[case] offset: usize, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 99), @@ -323,7 +329,7 @@ fn test_common_cfg_queue_reads( ..Default::default() }, ]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, q_sel), @@ -340,7 +346,7 @@ fn test_common_cfg_queue_reads( #[case(VirtioCommonCfg::OFFSET_QUEUE_RESET, 2, 0)] fn test_common_cfg_misc_reads(#[case] offset: usize, #[case] size: u8, #[case] expected: u64) { let queues = Arc::new([QueueReg::default(), QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset as u64, size), Ok(val) if val == expected); } @@ -350,7 +356,7 @@ fn test_common_cfg_misc_reads(#[case] offset: usize, #[case] size: u8, #[case] e #[case(10, 0)] fn test_device_feature_reads(#[case] sel: u64, #[case] expected_feature: u64) { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _) = create_test_mmio(queues); + let (mut mmio, _, _reset_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1111_2222, 0x3333_4444, 0, 0]; assert_matches!( @@ -378,7 +384,7 @@ fn test_device_feature_reads(#[case] sel: u64, #[case] expected_feature: u64) { #[case(10, 0x1234, 0)] fn test_driver_features(#[case] bank: u64, #[case] write_val: u64, #[case] expected: u64) { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _event_rx) = create_test_mmio(queues); + let (mut mmio, _event_rx, _reset_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1234_5678, 0x0000_0005, 0, 0]; assert_matches!( @@ -406,7 +412,7 @@ fn test_driver_features(#[case] bank: u64, #[case] write_val: u64, #[case] expec #[test] fn test_driver_features_locked_after_features_ok() { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _event_rx) = create_test_mmio(queues); + let (mut mmio, _event_rx, _reset_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1234_5678, 0x0000_0005, 0, 0]; assert_matches!( @@ -451,7 +457,7 @@ fn test_driver_features_locked_after_features_ok() { #[test] fn test_device_status_valid_transitions() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); // Initially status is 0 (empty) assert_matches!( @@ -625,12 +631,28 @@ fn test_device_status_valid_transitions() { mmio.write(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1, 0), Ok(Action::None) ); + let Ok(WakeEvent::Reset { seq }) = event_rx.try_recv() else { + panic!("expected a reset event"); + }; + assert!(event_rx.is_empty()); + + // The worker might still be using the queues, so nothing changes until + // it completes the reset. + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(status) if status == needs_reset.bits() as u64 + ); + assert!(queues[0].enabled.load(Ordering::Acquire)); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_CONFIG_MSIX_VECTOR as u64, 2), + Ok(0) + ); + + reset_tx.send(seq).unwrap(); assert_matches!( mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), Ok(0) ); - assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); - assert!(event_rx.is_empty()); assert!(!queues[0].enabled.load(Ordering::Acquire)); assert_matches!( mmio.read(VirtioCommonCfg::OFFSET_CONFIG_MSIX_VECTOR as u64, 2), @@ -638,6 +660,100 @@ fn test_device_status_valid_transitions() { ); } +/// Drives the device to DRIVER_OK with queue 0 enabled, and then asks for a +/// reset. Returns the sequence number of the reset. +fn start_then_reset(mmio: &TestMmio, event_rx: &TestWakeReceiver) -> u64 { + let driver_ok = + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + for (offset, size, val) in [ + (VirtioCommonCfg::OFFSET_QUEUE_SELECT, 2, 0), + (VirtioCommonCfg::OFFSET_QUEUE_ENABLE, 2, 1), + ( + VirtioCommonCfg::OFFSET_DEVICE_STATUS, + 1, + driver_ok.bits() as u64, + ), + (VirtioCommonCfg::OFFSET_DEVICE_STATUS, 1, 0), + ] { + assert_matches!(mmio.write(offset as u64, size, val), Ok(Action::None)); + } + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Start { .. })); + let Ok(WakeEvent::Reset { seq }) = event_rx.try_recv() else { + panic!("expected a reset event"); + }; + seq +} + +#[test] +fn test_device_reset_in_progress() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + let seq = start_then_reset(&mmio, &event_rx); + + // The driver must leave the device alone until the reset completes, so + // all writes are ignored, and writing 0 again does not reset it twice. + for (offset, size, val) in [ + (VirtioCommonCfg::OFFSET_DEVICE_STATUS, 1, 0), + ( + VirtioCommonCfg::OFFSET_DEVICE_STATUS, + 1, + DevStatus::ACK.bits() as u64, + ), + (VirtioCommonCfg::OFFSET_QUEUE_SELECT, 2, 1), + (VirtioCommonCfg::OFFSET_CONFIG_MSIX_VECTOR, 2, 1), + ] { + assert_matches!(mmio.write(offset as u64, size, val), Ok(Action::None)); + } + assert!(event_rx.is_empty()); + let driver_ok = + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(status) if status == driver_ok.bits() as u64 + ); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2), + Ok(0) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_CONFIG_MSIX_VECTOR as u64, 2), + Ok(vector) if vector == VIRTIO_MSI_NO_VECTOR as u64 + ); + assert!(queues[0].enabled.load(Ordering::Acquire)); + + // Once the worker has completed the reset, the driver can set up the + // device again. + reset_tx.send(seq).unwrap(); + assert_matches!( + mmio.write( + VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, + 1, + DevStatus::ACK.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); + assert!(!queues[0].enabled.load(Ordering::Acquire)); +} + +#[test] +fn test_device_reset_worker_exited() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + start_then_reset(&mmio, &event_rx); + + // A worker that has exited no longer uses the queues. + drop(reset_tx); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(0) + ); + assert!(!queues[0].enabled.load(Ordering::Acquire)); +} + #[rstest] // Invalid transitions from empty state (skipping ACK or invalid combinations) #[case(DevStatus::empty(), DevStatus::DRIVER.bits() as u64)] @@ -672,7 +788,7 @@ fn test_device_status_valid_transitions() { )] fn test_device_status_invalid_transitions(#[case] initial: DevStatus, #[case] invalid_write: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues); + let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); if !initial.is_empty() { assert_matches!( @@ -726,7 +842,7 @@ fn test_device_status_multistep_transition( #[case] expect_start: bool, ) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues); + let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); // Set driver features assert_matches!( @@ -792,7 +908,7 @@ fn test_device_status_multistep_transition( #[test] fn test_device_status_reset_without_driver_ok() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, _reset_tx) = create_test_mmio(queues.clone()); // Set status to ACK | DRIVER let ack_driver = DevStatus::ACK | DevStatus::DRIVER; @@ -855,7 +971,7 @@ fn test_device_reset_queue_registers() { ..Default::default() }, ]); - let (mmio, event_rx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); // Initialize MSI-X table with 2 Entry slots *mmio.irq_sender.msix_table.entries.write() = vec![ @@ -908,8 +1024,15 @@ fn test_device_reset_queue_registers() { mmio.write(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1, 0), Ok(Action::None) ); - assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); + let Ok(WakeEvent::Reset { seq }) = event_rx.try_recv() else { + panic!("expected a reset event"); + }; assert!(event_rx.is_empty()); + reset_tx.send(seq).unwrap(); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(0) + ); // Verify all queue registers are reset for (q_index, _) in queues.iter().enumerate() { @@ -949,7 +1072,7 @@ fn test_device_reset_queue_registers() { #[case(VirtioCommonCfg::OFFSET_QUEUE_MSIX_VECTOR, Some(0))] fn test_msix_vector_configuration(#[case] offset: usize, #[case] queue_sel: Option) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); // Initialize MSI-X table with 2 Entry slots *mmio.irq_sender.msix_table.entries.write() = vec![ @@ -986,7 +1109,7 @@ fn test_msix_vector_configuration(#[case] offset: usize, #[case] queue_sel: Opti #[case(VirtioPciRegister::OFFSET_QUEUE_NOTIFY + size_of::(), false)] fn test_queue_notify(#[case] offset: usize, #[case] expect_wake: bool) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues); + let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); assert_matches!(mmio.write(offset as u64, 2, 0), Ok(Action::None)); if expect_wake { @@ -1027,6 +1150,7 @@ fn test_notify_with_notifiers() { notifier, flume::unbounded().1, )), + reset_seq: Mutex::new(None), }; assert_matches!( @@ -1039,7 +1163,7 @@ fn test_notify_with_notifiers() { #[test] fn test_wake_up_dev_channel_error() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx) = create_test_mmio(queues); + let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); // Drop receiver so send will fail drop(event_rx); @@ -1058,7 +1182,7 @@ fn test_wake_up_dev_channel_error() { #[case(0xdead_beef, 4, 0x1234)] fn test_invalid_register_writes(#[case] offset: u64, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!(mmio.write(offset, size, val), Ok(Action::None)); } @@ -1067,7 +1191,7 @@ fn test_invalid_register_writes(#[case] offset: u64, #[case] size: u8, #[case] v #[case(0, 3)] fn test_invalid_register_reads(#[case] offset: u64, #[case] size: u8) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset, size), Ok(0)); } @@ -1076,7 +1200,7 @@ fn test_invalid_register_reads(#[case] offset: u64, #[case] size: u8) { #[case(3, (size_of::() + size_of::() * 4) as u64)] fn test_mmio_size(#[case] num_queues: usize, #[case] expected_size: u64) { let queues = (0..num_queues).map(|_| QueueReg::default()).collect(); - let (mmio, _) = create_test_mmio(queues); + let (mmio, _, _reset_tx) = create_test_mmio(queues); assert_eq!(mmio.size(), expected_size); } @@ -1104,11 +1228,12 @@ fn create_test_virtio_device( config_size: u64, shared_mem: Option>, num_queues: usize, -) -> (VirtioDevice, Receiver>) +) -> (VirtioDevice, Receiver>, Sender) where S: IrqSender, { let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let queue_regs = (0..num_queues) .map(|_| QueueReg::default()) @@ -1124,11 +1249,11 @@ where "test-dev".into(), event_tx, notifier, - flume::unbounded().1, + reset_rx, )), worker_handle: None, }; - (dev, event_rx) + (dev, event_rx, reset_tx) } #[test] @@ -1336,7 +1461,8 @@ fn test_virtio_pci_device_classes( #[case] expected_subclass: u8, #[case] expected_dev_id: u16, ) { - let (dev, _rx) = create_test_virtio_device::>(id, 0, None, 2); + let (dev, _rx, _reset_tx) = + create_test_virtio_device::>(id, 0, None, 2); let pci_dev = VirtioPciDevice::new( dev, TestMsiSender::default(), @@ -1389,7 +1515,7 @@ fn test_virtio_pci_device_with_config_and_shared_memory_prefetchable() { ], callbacks: Mutex::new(vec![]), }); - let (dev, _rx) = create_test_virtio_device::>( + let (dev, _rx, _reset_tx) = create_test_virtio_device::>( DeviceId::FILE_SYSTEM, 32, Some(shared_mem), @@ -1441,7 +1567,7 @@ fn test_virtio_pci_device_shared_memory_non_prefetchable() { }], callbacks: Mutex::new(vec![]), }); - let (dev, _rx) = create_test_virtio_device::>( + let (dev, _rx, _reset_tx) = create_test_virtio_device::>( DeviceId::FILE_SYSTEM, 0, Some(shared_mem), @@ -1460,7 +1586,7 @@ fn test_virtio_pci_device_shared_memory_non_prefetchable() { #[test] fn test_virtio_pci_device_notifier_callback() { - let (dev, _rx) = + let (dev, _rx, _reset_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 2); let registry = TestNotifierRegistry::default(); let registered = registry.registered.clone(); @@ -1505,7 +1631,7 @@ fn test_virtio_pci_device_notifier_callback() { #[test] fn test_virtio_pci_device_notifier_fallback() { - let (dev, _rx) = + let (dev, _rx, _reset_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 1); let pci_dev = VirtioPciDevice::new( dev, @@ -1525,7 +1651,7 @@ fn test_virtio_pci_device_notifier_fallback() { #[test] fn test_virtio_pci_device_pci_reset() { - let (dev, event_rx) = + let (dev, event_rx, reset_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 1); let pci_dev = VirtioPciDevice::new( dev, @@ -1535,24 +1661,56 @@ fn test_virtio_pci_device_pci_reset() { .unwrap(); // Set queue enabled and device status - pci_dev.registers.queues[0] - .enabled - .store(true, Ordering::Release); - pci_dev - .registers - .reg - .status - .store(DevStatus::DRIVER_OK.bits(), Ordering::Release); + let registers = pci_dev.registers.clone(); + registers.queues[0].enabled.store(true, Ordering::Release); + let driver_ok = DevStatus::DRIVER_OK.bits(); + registers.reg.status.store(driver_ok, Ordering::Release); + + // A fake worker checks that nothing is cleared before it completes the + // reset. + let worker = thread::spawn(move || { + let Ok(WakeEvent::Reset { seq }) = event_rx.recv() else { + panic!("expected a reset event"); + }; + assert_eq!(registers.reg.status.load(Ordering::Acquire), driver_ok); + assert!(registers.queues[0].enabled.load(Ordering::Acquire)); + reset_tx.send(seq).unwrap(); + }); - // Call Pci::reset + // Pci::reset returns only after the worker has completed the reset. assert_matches!(pci_dev.reset(), Ok(())); + worker.join().unwrap(); - // Device status should be cleared assert_eq!(pci_dev.registers.reg.status.load(Ordering::Acquire), 0); - // Queue should be disabled assert!(!pci_dev.registers.queues[0].enabled.load(Ordering::Acquire)); - // Reset event sent +} + +#[test] +fn test_virtio_pci_device_pci_reset_timeout() { + let (dev, event_rx, _reset_tx) = + create_test_virtio_device::>(DeviceId::NET, 0, None, 1); + let pci_dev = VirtioPciDevice::new( + dev, + TestMsiSender::default(), + Option::::None, + ) + .unwrap(); + let driver_ok = DevStatus::DRIVER_OK.bits(); + pci_dev + .registers + .reg + .status + .store(driver_ok, Ordering::Release); + + // The worker never completes the reset. + let timeout = Duration::from_millis(10); + assert_matches!( + pci_dev.registers.reset_sync(timeout), + Err(Error::ResetTimeout { timeout: t, .. }) if t == timeout + ); assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Reset { .. })); + let status = pci_dev.registers.reg.status.load(Ordering::Acquire); + assert_eq!(status, driver_ok); } #[test] From ac3648ddc076879ee3452d9e03aa7afe5206a13e Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:07:27 -0800 Subject: [PATCH 4/8] fix(vu): stop vhost-user rings with GET_VRING_BASE on reset To reset a device, the vhost-user frontend disables the rings with SET_VRING_ENABLE and then sends SET_STATUS(0). Per the vhost-user spec, the backend still processes a disabled ring, only without side effects. For example, it still consumes transmitted packets and writes the used ring. The worker could therefore report the reset as completed while the backend still writes guest memory. Stop each ring with GET_VRING_BASE first. Once all rings are stopped, the device is suspended and the backend no longer writes guest memory or sends notifications. Each message waits for its reply, so the rings have stopped by the time the worker acknowledges the reset. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/vu/frontend.rs | 15 +++++ alioth/src/virtio/vu/frontend_test.rs | 86 +++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 alioth/src/virtio/vu/frontend_test.rs diff --git a/alioth/src/virtio/vu/frontend.rs b/alioth/src/virtio/vu/frontend.rs index a4f2de46..67018fed 100644 --- a/alioth/src/virtio/vu/frontend.rs +++ b/alioth/src/virtio/vu/frontend.rs @@ -410,6 +410,17 @@ impl VirtioMio for VuFrontend { fn reset(&mut self, registry: &Registry) { let name = &*self.name; for index in 0..self.num_queues { + // A disabled ring is still processed. Only GET_VRING_BASE stops + // it, and once all rings are stopped, the backend no longer + // touches guest memory or sends notifications. + let virtq_base = VirtqState { + index: index as _, + val: 0, + }; + match self.session.get_virtq_base(&virtq_base) { + Ok(base) => log::trace!("{name}: queue-{index}: stopped at {}", base.val), + Err(e) => log::error!("{name}: failed to stop queue-{index}: {e:?}"), + } let disable = VirtqState { index: index as _, val: 0, @@ -453,3 +464,7 @@ impl DevSpec for VuFrontendSpec { true } } + +#[cfg(test)] +#[path = "frontend_test.rs"] +mod tests; diff --git a/alioth/src/virtio/vu/frontend_test.rs b/alioth/src/virtio/vu/frontend_test.rs new file mode 100644 index 00000000..ccd96314 --- /dev/null +++ b/alioth/src/virtio/vu/frontend_test.rs @@ -0,0 +1,86 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::os::unix::net::UnixStream; +use std::sync::Arc; +use std::thread; + +use mio::Poll; + +use crate::virtio::DeviceId; +use crate::virtio::vu::bindings::{VirtqState, VuFeature, VuFrontMsg}; +use crate::virtio::vu::conn::VuSession; +use crate::virtio::vu::frontend::VuFrontend; +use crate::virtio::worker::mio::VirtioMio; + +#[test] +fn test_vu_frontend_reset_stops_rings() { + let (frontend_conn, backend_conn) = UnixStream::pair().unwrap(); + let backend = VuSession::from(backend_conn); + let backend_thread = thread::spawn(move || { + let mut fds = [const { None }; 8]; + let mut requests = vec![]; + while let Ok(msg) = backend.recv_msg(&mut fds) { + let req = VuFrontMsg::from(msg.request); + match req { + VuFrontMsg::GET_VIRTQ_BASE => { + let mut virtq_base: VirtqState = backend.recv_payload().unwrap(); + requests.push((req, virtq_base.index as u64)); + virtq_base.val = 0; + backend.reply(req, &virtq_base, &[]).unwrap(); + } + VuFrontMsg::SET_VIRTQ_ENABLE => { + let virtq_enable: VirtqState = backend.recv_payload().unwrap(); + assert_eq!(virtq_enable.val, 0); + requests.push((req, virtq_enable.index as u64)); + backend.reply(req, &0u64, &[]).unwrap(); + } + VuFrontMsg::SET_STATUS => { + let status: u64 = backend.recv_payload().unwrap(); + requests.push((req, status)); + backend.reply(req, &0u64, &[]).unwrap(); + } + req => panic!("unexpected request: {req:?}"), + } + } + requests + }); + + let mut frontend = VuFrontend { + name: "vu-frontend".into(), + session: Arc::new(VuSession::from(frontend_conn)), + channel: None, + id: DeviceId::NET, + vu_feature: VuFeature::STATUS, + device_feature: 0, + num_queues: 2, + err_fds: Box::default(), + }; + let poll = Poll::new().unwrap(); + frontend.reset(poll.registry()); + drop(frontend); + + // Every ring is stopped before the device is reset. + let requests = backend_thread.join().unwrap(); + assert_eq!( + requests, + [ + (VuFrontMsg::GET_VIRTQ_BASE, 0), + (VuFrontMsg::SET_VIRTQ_ENABLE, 0), + (VuFrontMsg::GET_VIRTQ_BASE, 1), + (VuFrontMsg::SET_VIRTQ_ENABLE, 1), + (VuFrontMsg::SET_STATUS, 0), + ] + ); +} From 6a62ad7123b61fdcab260409068f923e50fb7b57 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:10:50 -0800 Subject: [PATCH 5/8] fix(vu): stop the device worker before replying to the frontend The vhost-user backend replies to SET_STATUS without DRIVER_OK right after asking the worker to reset, and replies to GET_VRING_BASE without stopping anything. A frontend takes either reply to mean that the rings have stopped, and might then hand the ring memory back to the guest while the worker still uses it. Wait for the worker to complete the reset before replying. Blocking is fine here: in the vhost-user protocol the reply is the only completion signal, and the backend thread serves no one else. GET_VRING_BASE stops the whole device, since one worker serves all queues. Track whether the worker runs separately from the device status, so that a frontend can restart the device after stopping it with GET_VRING_BASE, and can change the memory layout while it is stopped. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/vu/backend.rs | 60 +++++++++---- alioth/src/virtio/vu/backend_test.rs | 121 +++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 14 deletions(-) create mode 100644 alioth/src/virtio/vu/backend_test.rs diff --git a/alioth/src/virtio/vu/backend.rs b/alioth/src/virtio/vu/backend.rs index 34cd0e33..6410cd7d 100644 --- a/alioth/src/virtio/vu/backend.rs +++ b/alioth/src/virtio/vu/backend.rs @@ -28,7 +28,7 @@ use zerocopy::IntoBytes; use crate::errors::DebugTrace; use crate::mem::mapped::{ArcMemPages, RamBus}; use crate::sync::notifier::Notifier; -use crate::virtio::dev::{StartParam, VirtioDevice, WakeEvent}; +use crate::virtio::dev::{RESET_TIMEOUT, StartParam, VirtioDevice, WakeEvent}; use crate::virtio::vu::Error as VuError; use crate::virtio::vu::bindings::{ MAX_CONFIG_SIZE, MemoryRegion, MemorySingleRegion, Message, VirtqAddr, VirtqState, VuFeature, @@ -49,6 +49,8 @@ pub enum Error { Vu { source: Box, }, + #[snafu(display("Virtio device error"), context(false))] + Virtio { source: Box }, #[snafu(display("failed to parse the payload of {req:?}"))] Parse { req: VuFrontMsg }, #[snafu(display("frontend requested invalid queue index: {index}"))] @@ -140,6 +142,11 @@ pub struct VuBackend { session: VuSession, channel: Option>, status: DevStatus, + /// Whether the worker has started and not stopped since. + /// + /// The frontend can stop the worker with GET_VRING_BASE, which does not + /// change the device status. + running: bool, memory: Arc, dev: VirtioDevice, init: VuInit, @@ -159,6 +166,7 @@ impl VuBackend { dev, memory, status: DevStatus::empty(), + running: false, init: VuInit { drv_feat: 0, queues: (0..queue_num).map(|_| VuQueueInit::default()).collect(), @@ -171,6 +179,21 @@ impl VuBackend { self.dev.name.as_ref() } + /// Stops the worker and waits until it no longer uses the queues. + /// + /// The frontend takes the reply to its request as the completion of the + /// stop, so the backend has to wait before replying. + fn stop_worker(&mut self) -> Result<()> { + if !self.running { + return Ok(()); + } + let ctl = &self.dev.ctl; + let seq = ctl.request_reset(); + ctl.wait_reset(seq, RESET_TIMEOUT)?; + self.running = false; + Ok(()) + } + fn convert_frontend_hva(&self, hva: u64) -> Result { for r in &self.init.regions { if hva >= r.hva && hva < r.hva + r.size { @@ -233,7 +256,7 @@ impl VuBackend { } fn handle_msg(&mut self, msg: &mut Message, fds: &mut [Option; 8]) -> Result<()> { - let name = &*self.dev.name; + let name = self.dev.name.clone(); let (req, size) = (VuFrontMsg::from(msg.request), msg.size); match (req, size) { @@ -346,6 +369,13 @@ impl VuBackend { let Some(_q) = self.init.queues.get_mut(index as usize) else { return error::InvalidQueue { index }.fail(); }; + // GET_VRING_BASE stops the ring. One worker serves all the + // queues, so stop all of them. The frontend has no way to + // learn about a failure other than waiting forever for the + // reply, so reply anyway. + if let Err(e) = self.stop_worker() { + log::error!("{name}: queue-{index}: failed to stop: {e:?}"); + } virtq_base.val = 0; self.session.reply(req, &virtq_base, &[])?; msg.flag.set_need_reply(false); @@ -380,7 +410,7 @@ impl VuBackend { return error::MissingFd { req: msg.request }.fail(); }; let region = &single.region; - if self.status.contains(DevStatus::DRIVER_OK) { + if self.running { return error::ChangeMemoryLayout.fail(); } log::debug!("{name}: add mem: {region:x?}, fd: {}", fd.as_raw_fd()); @@ -396,7 +426,7 @@ impl VuBackend { (VuFrontMsg::REM_MEM_REG, 40) => { let single: MemorySingleRegion = self.session.recv_payload()?; let region = &single.region; - if self.status.contains(DevStatus::DRIVER_OK) { + if self.running { return error::ChangeMemoryLayout.fail(); } for (index, r) in self.init.regions.iter().enumerate() { @@ -417,17 +447,15 @@ impl VuBackend { (VuFrontMsg::SET_STATUS, 8) => { let status: u64 = self.session.recv_payload()?; let new = DevStatus::from_bits_retain(status as u8); - let old = self.status; - self.status = new; - log::debug!("{name}: set status: {old:x?} -> {new:x?}"); - if (old ^ new).contains(DevStatus::DRIVER_OK) { - if new.contains(DevStatus::DRIVER_OK) { - let param = self.parse_init()?; - self.dev.ctl.wake(WakeEvent::Start { param }); - } else { - self.dev.ctl.request_reset(); - } + log::debug!("{name}: set status: {:x?} -> {new:x?}", self.status); + if !new.contains(DevStatus::DRIVER_OK) { + self.stop_worker()?; + } else if !self.running { + let param = self.parse_init()?; + self.dev.ctl.wake(WakeEvent::Start { param }); + self.running = true; } + self.status = new; } (VuFrontMsg::GET_CONFIG, 12..) => { let mut region = [0u8; MAX_CONFIG_SIZE]; @@ -493,3 +521,7 @@ impl VuBackend { Ok(()) } } + +#[cfg(test)] +#[path = "backend_test.rs"] +mod tests; diff --git a/alioth/src/virtio/vu/backend_test.rs b/alioth/src/virtio/vu/backend_test.rs new file mode 100644 index 00000000..2e211061 --- /dev/null +++ b/alioth/src/virtio/vu/backend_test.rs @@ -0,0 +1,121 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::os::fd::AsFd; +use std::os::unix::net::UnixStream; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::thread; +use std::time::Duration; + +use crate::mem; +use crate::mem::emulated::{Action, Mmio}; +use crate::mem::mapped::RamBus; +use crate::sync::notifier::Notifier; +use crate::virtio::dev::{VirtioDevice, WakeEvent, WorkerCtl}; +use crate::virtio::queue::QueueReg; +use crate::virtio::vu::backend::VuBackend; +use crate::virtio::vu::bindings::VirtqState; +use crate::virtio::vu::conn::VuSession; +use crate::virtio::{DevStatus, DeviceId}; + +#[derive(Debug)] +struct TestDevConfig; + +impl Mmio for TestDevConfig { + fn size(&self) -> u64 { + 0 + } + + fn read(&self, _offset: u64, _size: u8) -> mem::Result { + Ok(0) + } + + fn write(&self, _offset: u64, _size: u8, _val: u64) -> mem::Result { + Ok(Action::None) + } +} + +#[test] +fn test_vu_backend_stops_worker_before_reply() { + let (event_tx, event_rx) = flume::unbounded(); + let (reset_tx, reset_rx) = flume::unbounded(); + let notifier = Arc::new(Notifier::new().unwrap()); + let dev = VirtioDevice { + name: "vu-backend".into(), + id: DeviceId::NET, + device_config: Arc::new(TestDevConfig), + device_feature: 0, + queue_regs: Arc::new([QueueReg::default()]), + shared_mem_regions: None, + ctl: Arc::new(WorkerCtl::new( + "vu-backend".into(), + event_tx, + notifier, + reset_rx, + )), + worker_handle: None, + }; + + // A fake worker that takes a while to stop. + let stopped = Arc::new(AtomicBool::new(false)); + let worker_stopped = stopped.clone(); + let worker = thread::spawn(move || { + let mut starts = 0; + for event in event_rx.iter() { + match event { + WakeEvent::Start { .. } => starts += 1, + WakeEvent::Reset { seq } => { + thread::sleep(Duration::from_millis(50)); + worker_stopped.store(true, Ordering::Release); + reset_tx.send(seq).unwrap(); + } + event => panic!("unexpected event: {event:?}"), + } + } + starts + }); + + let (frontend_conn, backend_conn) = UnixStream::pair().unwrap(); + let mut backend = VuBackend::new(backend_conn, dev, Arc::new(RamBus::new())).unwrap(); + let backend_thread = thread::spawn(move || backend.run().unwrap()); + + let frontend = VuSession::from(frontend_conn); + let kick = Notifier::new().unwrap(); + let driver_ok = + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + let driver_ok = driver_ok.bits() as u64; + + // The reply to SET_STATUS(0) means that the worker has stopped. + frontend.set_virtq_kick(&0, kick.as_fd()).unwrap(); + frontend.set_status(&driver_ok).unwrap(); + frontend.set_status(&0).unwrap(); + assert!(stopped.swap(false, Ordering::AcqRel)); + + // So does the reply to GET_VRING_BASE. + frontend.set_virtq_kick(&0, kick.as_fd()).unwrap(); + frontend.set_status(&driver_ok).unwrap(); + let virtq_base = VirtqState { index: 0, val: 0 }; + assert_eq!(frontend.get_virtq_base(&virtq_base).unwrap().val, 0); + assert!(stopped.swap(false, Ordering::AcqRel)); + + // The worker has already stopped, so there is nothing left to stop. + frontend.set_status(&0).unwrap(); + assert!(!stopped.load(Ordering::Acquire)); + + drop(frontend); + backend_thread.join().unwrap(); + // Dropping the backend disconnects the worker. + assert_eq!(worker.join().unwrap(), 2); +} From 3cd4dbdebf71e992ef91a2a59d0e65818431ccce Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:13:39 -0800 Subject: [PATCH 6/8] fix(virtio/pci): keep the MSI-X table on device reset A device reset clears the whole MSI-X table, which masks all vectors and zeroes their addresses and data. The table belongs to the PCI function, though, and the virtio spec only requires a device reset to unmap the configuration and queue events from the vectors. A driver that resets the device and then reuses the vectors it has programmed would receive no more interrupts. Linux reprograms the table after each reset, which hides the problem. Keep what the guest has programmed into the table on a device reset, and only release the irqfds that workers set up. Clear the table on a PCI reset instead. Signed-off-by: Changyuan Lyu --- alioth/src/pci/cap.rs | 19 ++++++++++ alioth/src/pci/cap_test.rs | 41 ++++++++++++++++++++++ alioth/src/virtio/pci.rs | 10 ++++-- alioth/src/virtio/pci_test.rs | 66 +++++++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 2 deletions(-) diff --git a/alioth/src/pci/cap.rs b/alioth/src/pci/cap.rs index 833d7573..5365cfab 100644 --- a/alioth/src/pci/cap.rs +++ b/alioth/src/pci/cap.rs @@ -575,6 +575,25 @@ where *entry = MsixTableMmioEntry::default(); } } + + /// Replaces irqfds with plain entries that keep the address, data and + /// mask programmed by the guest. + pub fn release_irqfds(&self) { + let mut entries = self.entries.write(); + for entry in entries.iter_mut() { + let MsixTableMmioEntry::IrqFd(fd) = entry else { + continue; + }; + let mut control = MsixVectorCtrl(0); + control.set_masked(fd.get_masked()); + *entry = MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: fd.get_addr_lo(), + addr_hi: fd.get_addr_hi(), + data: fd.get_data(), + control, + }); + } + } } impl Mmio for MsixTableMmio diff --git a/alioth/src/pci/cap_test.rs b/alioth/src/pci/cap_test.rs index 7fc8adc3..6df28c7f 100644 --- a/alioth/src/pci/cap_test.rs +++ b/alioth/src/pci/cap_test.rs @@ -311,6 +311,47 @@ fn test_msix_table_mmio() { ); } +#[test] +fn test_msix_table_release_irqfds() { + let irqfd = TestIrqFd::default(); + irqfd.set_addr_lo(0xfee0_0000).unwrap(); + irqfd.set_addr_hi(0x1).unwrap(); + irqfd.set_data(0x41).unwrap(); + irqfd.set_masked(false).unwrap(); + let entries = Box::new([ + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_1000, + addr_hi: 0, + data: 0x42, + control: MsixVectorCtrl(1), + }), + MsixTableMmioEntry::IrqFd(irqfd), + ]); + let table = MsixTableMmio { + entries: RwLock::new(entries), + }; + + // Irqfds are gone, but the guest programming stays. + table.release_irqfds(); + assert_matches!( + **table.entries.read(), + [ + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_1000, + addr_hi: 0, + data: 0x42, + control: MsixVectorCtrl(1), + }), + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_0000, + addr_hi: 0x1, + data: 0x41, + control: MsixVectorCtrl(0), + }) + ] + ); +} + #[test] fn test_pci_cap_list() { let caps: Vec> = vec![ diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index 09459840..5a7b7956 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -210,13 +210,16 @@ where /// /// The worker must not be using the queues, i.e. it either has not /// started or has completed the reset. + /// + /// The MSI-X table belongs to the PCI function, so a device reset keeps + /// what the guest has programmed into it. fn complete_reset(&self) { let config_msix = &self.irq_sender.msix_vector.config; config_msix.store(VIRTIO_MSI_NO_VECTOR, Ordering::Release); for q_vector in self.irq_sender.msix_vector.queues.iter() { q_vector.store(VIRTIO_MSI_NO_VECTOR, Ordering::Release); } - self.irq_sender.msix_table.reset(); + self.irq_sender.msix_table.release_irqfds(); for q in self.queues.iter() { // Disable the queue before clearing its ring addresses, so that // a reader that checks `enabled` after loading the addresses @@ -1070,7 +1073,10 @@ where fn reset(&self) -> pci::Result<()> { let ret = self.registers.reset_sync(RESET_TIMEOUT); - ret.box_trace(pci::error::Reset) + ret.box_trace(pci::error::Reset)?; + // Unlike a device reset, a PCI reset also clears the MSI-X table. + self.registers.irq_sender.msix_table.reset(); + Ok(()) } } diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index f05621a4..e2181012 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -24,6 +24,7 @@ use flume::{Receiver, Sender}; use parking_lot::{Mutex, RwLock}; use rstest::rstest; +use crate::hv::IrqFd; use crate::hv::tests::{RegisteredAddr, TestIrqFd, TestMsiSender, TestNotifierRegistry}; use crate::mem::emulated::{Action, Mmio}; use crate::mem::{self, MemRange, MemRegion, MemRegionEntry, MemRegionType}; @@ -959,6 +960,53 @@ fn test_device_status_reset_without_driver_ok() { ); } +#[test] +fn test_device_reset_keeps_msix_table() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + + let irqfd = TestIrqFd::default(); + irqfd.set_addr_lo(0xfee0_0000).unwrap(); + irqfd.set_data(0x41).unwrap(); + irqfd.set_masked(false).unwrap(); + *mmio.irq_sender.msix_table.entries.write() = vec![ + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_1000, + addr_hi: 0, + data: 0x42, + control: MsixVectorCtrl(0), + }), + MsixTableMmioEntry::IrqFd(irqfd), + ] + .into_boxed_slice(); + + assert_matches!( + mmio.write(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1, 0), + Ok(Action::None) + ); + + // The device reset releases the irqfds, but keeps what the guest has + // programmed into the MSI-X table. + let entries = mmio.irq_sender.msix_table.entries.read(); + assert_matches!( + **entries, + [ + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_1000, + data: 0x42, + .. + }), + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_0000, + data: 0x41, + .. + }) + ] + ); + assert!(!entries[0].get_masked()); + assert!(!entries[1].get_masked()); +} + #[test] fn test_device_reset_queue_registers() { let queues = Arc::new([ @@ -1665,6 +1713,13 @@ fn test_virtio_pci_device_pci_reset() { registers.queues[0].enabled.store(true, Ordering::Release); let driver_ok = DevStatus::DRIVER_OK.bits(); registers.reg.status.store(driver_ok, Ordering::Release); + registers.irq_sender.msix_table.entries.write()[0] = + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0xfee0_0000, + addr_hi: 0, + data: 0x41, + control: MsixVectorCtrl(0), + }); // A fake worker checks that nothing is cleared before it completes the // reset. @@ -1683,6 +1738,17 @@ fn test_virtio_pci_device_pci_reset() { assert_eq!(pci_dev.registers.reg.status.load(Ordering::Acquire), 0); assert!(!pci_dev.registers.queues[0].enabled.load(Ordering::Acquire)); + // Unlike a device reset, a PCI reset also clears the MSI-X table. + let entries = pci_dev.registers.irq_sender.msix_table.entries.read(); + assert_matches!( + &entries[0], + MsixTableMmioEntry::Entry(MsixTableEntry { + addr_lo: 0, + data: 0, + .. + }) + ); + assert!(entries[0].get_masked()); } #[test] From 7621fa853494cad2cc5c707cb7b75f291c762d0d Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:17:02 -0800 Subject: [PATCH 7/8] feat(virtio): let workers report failures to the transport A worker that fails while its device is running cleans up and waits for the next start, but the transport cannot tell, and keeps presenting a working device to the driver. Turn the reset acknowledgement channel into a channel for all worker reports, and have the worker report failures there. A failure stands until a reset completes. Reports arrive in order, so a failure reported before the acknowledgement of a reset is gone with that reset, while a later one is not. The following change lets the transports tell the driver. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/dev/dev.rs | 137 ++++++++++++------ alioth/src/virtio/dev/dev_test.rs | 86 ++++++++--- alioth/src/virtio/dev/entropy_test.rs | 2 +- alioth/src/virtio/dev/vsock/uds_vsock_test.rs | 2 +- alioth/src/virtio/pci_test.rs | 98 +++++++------ alioth/src/virtio/vu/backend_test.rs | 8 +- alioth/src/virtio/worker/io_uring_test.rs | 2 +- 7 files changed, 218 insertions(+), 117 deletions(-) diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index 48d3a76d..8355d944 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -126,12 +126,12 @@ where /// Receiving end of the wake event channel. The sending end is in /// [`WorkerCtl`]. pub(crate) event_rx: Receiver>, - /// Sending end of the reset completion channel. The receiving end is in + /// Sending end of the report channel. The receiving end is in /// [`WorkerCtl`]. /// /// Only the worker holds it, so the channel disconnects once the worker /// exits, which completes all pending resets. - pub(crate) reset_tx: Sender, + pub(crate) report_tx: Sender, /// Guest memory. pub(crate) memory: Arc, /// Queue registers, shared with the transport. @@ -155,12 +155,22 @@ where param: StartParam, }, /// Resets the device. Once the worker no longer uses the queues, it - /// sends `seq` back through [`WorkerParam::reset_tx`]. + /// reports [`WorkerReport::ResetDone`] with `seq`. Reset { seq: u64, }, } +/// What a worker reports to the transport. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum WorkerReport { + /// The worker has completed reset `seq`. + ResetDone { seq: u64 }, + /// The worker has failed, and does not serve the queues until the + /// device is reset. + Failed, +} + #[derive(Debug, PartialEq, Eq)] pub enum WorkerState { Pending, @@ -192,7 +202,7 @@ where /// The lock is held while sending the reset event, so that the worker /// sees resets in the order of their sequence numbers. reset_seq: Mutex, - reset_acks: Mutex, + reports: Mutex, } /// Returns `true` if reset `seq` is not later than reset `done`. @@ -204,18 +214,40 @@ fn reset_completed(done: u64, seq: u64) -> bool { (done.wrapping_sub(seq) as i64) >= 0 } -/// Resets the worker has reported as completed. +/// What the worker has reported so far. #[derive(Debug)] -struct ResetAcks { - rx: Receiver, +struct Reports { + rx: Receiver, /// Sequence number of the last completed reset. - done: u64, + reset_done: u64, + /// Whether the worker has failed since the last completed reset. + failed: bool, } -impl ResetAcks { - fn complete(&mut self, seq: u64) { - if !reset_completed(self.done, seq) { - self.done = seq; +impl Reports { + fn handle(&mut self, report: WorkerReport) { + match report { + WorkerReport::ResetDone { seq } => { + if !reset_completed(self.reset_done, seq) { + self.reset_done = seq; + } + // A reset brings the device back into service. + self.failed = false; + } + WorkerReport::Failed => self.failed = true, + } + } + + /// Handles all pending reports. + /// + /// Returns `false` if the worker has exited. + fn drain(&mut self) -> bool { + loop { + match self.rx.try_recv() { + Ok(report) => self.handle(report), + Err(TryRecvError::Empty) => break true, + Err(TryRecvError::Disconnected) => break false, + } } } } @@ -228,16 +260,17 @@ where name: Arc, event_tx: Sender>, notifier: Arc, - reset_rx: Receiver, + report_rx: Receiver, ) -> Self { WorkerCtl { name, event_tx, notifier, reset_seq: Mutex::new(0), - reset_acks: Mutex::new(ResetAcks { - rx: reset_rx, - done: 0, + reports: Mutex::new(Reports { + rx: report_rx, + reset_done: 0, + failed: false, }), } } @@ -277,24 +310,19 @@ where /// The worker handles resets in order, so this also means that all /// earlier resets have completed. pub fn poll_reset(&self, seq: u64) -> bool { - let mut acks = self.reset_acks.lock(); - loop { - match acks.rx.try_recv() { - Ok(done) => acks.complete(done), - Err(TryRecvError::Empty) => break reset_completed(acks.done, seq), - // The worker has exited, so it no longer uses the queues. - Err(TryRecvError::Disconnected) => break true, - } - } + let mut reports = self.reports.lock(); + let alive = reports.drain(); + // An exited worker no longer uses the queues. + !alive || reset_completed(reports.reset_done, seq) } /// Waits until the worker has completed reset `seq`. pub fn wait_reset(&self, seq: u64, timeout: Duration) -> Result<()> { let deadline = Instant::now() + timeout; - let mut acks = self.reset_acks.lock(); - while !reset_completed(acks.done, seq) { - match acks.rx.recv_deadline(deadline) { - Ok(done) => acks.complete(done), + let mut reports = self.reports.lock(); + while !reset_completed(reports.reset_done, seq) { + match reports.rx.recv_deadline(deadline) { + Ok(report) => reports.handle(report), Err(RecvTimeoutError::Disconnected) => break, Err(RecvTimeoutError::Timeout) => { let name = self.name.clone(); @@ -304,6 +332,14 @@ where } Ok(()) } + + /// Returns `true` if the worker has failed, so that the device needs a + /// reset. + pub fn failed(&self) -> bool { + let mut reports = self.reports.lock(); + reports.drain(); + reports.failed + } } #[derive(Debug)] @@ -362,10 +398,10 @@ where let shared_mem_regions = dev.shared_mem_regions(); let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let param = WorkerParam { event_rx, - reset_tx, + report_tx, memory, queue_regs: queue_regs.clone(), }; @@ -375,7 +411,7 @@ where VirtioFeature::from_bits_retain(device_feature & !D::Feature::all().bits()), D::Feature::from_bits_truncate(device_feature) ); - let ctl = WorkerCtl::new(name.clone(), event_tx, notifier, reset_rx); + let ctl = WorkerCtl::new(name.clone(), event_tx, notifier, report_rx); let virtio_dev = VirtioDevice { name, id, @@ -442,10 +478,14 @@ where D: Virtio, S: IrqSender, { + fn report(&self, report: WorkerReport) { + // The transport might be gone, e.g. at VMM shutdown. + let _ = self.param.report_tx.send(report); + } + fn ack_reset(&self, seq: u64) { log::debug!("{}: reset {seq} completed", self.dev.name()); - // The transport might be gone, e.g. at VMM shutdown. - let _ = self.param.reset_tx.send(seq); + self.report(WorkerReport::ResetDone { seq }); } fn handle_wake_events(&mut self, backend: &mut B) -> Result<()> @@ -564,10 +604,7 @@ where ret.and(reset) } - fn loop_until_reset(&mut self) -> Result<()> { - let Some(param) = self.context.wait_start() else { - return Ok(()); - }; + fn run(&mut self, param: &StartParam) -> Result<()> { let memory = self.context.param.memory.clone(); let ram = memory.lock_layout(); let feature = param.feature & !VirtioFeature::ACCESS_PLATFORM.bits(); @@ -582,7 +619,7 @@ where Ok(Some(Queue::new(split_queue, reg, &ram))) }; let queues: Result> = queue_regs.iter().map(new_queue).collect(); - self.event_loop(&mut (queues?), &ram, ¶m) + self.event_loop(&mut (queues?), &ram, param) } else { let new_queue = |reg| { let Some(split_queue) = SplitQueue::new(reg, &ram, event_idx)? else { @@ -591,19 +628,29 @@ where Ok(Some(Queue::new(split_queue, reg, &ram))) }; let queues: Result> = queue_regs.iter().map(new_queue).collect(); - self.event_loop(&mut (queues?), &ram, ¶m) + self.event_loop(&mut (queues?), &ram, param) } } + fn loop_until_reset(&mut self) -> Result<()> { + let Some(param) = self.context.wait_start() else { + return Ok(()); + }; + let ret = self.run(¶m); + // Unless the worker failed in handling a reset or a shutdown, the + // device is out of service until the driver resets it. The worker + // stays alive for that reset. + if ret.is_err() && self.context.state == WorkerState::Running { + self.context.state = WorkerState::Pending; + self.context.report(WorkerReport::Failed); + } + ret + } + fn do_work(mut self) { while self.context.state != WorkerState::Shutdown { if let Err(e) = self.loop_until_reset() { log::error!("worker {}: {e:?}", self.context.dev.name()); - // Keep the worker alive, so that the driver can still reset - // and restart the device. - if self.context.state == WorkerState::Running { - self.context.state = WorkerState::Pending; - } } // The memory layout lock is released by now. if let Some(seq) = self.context.reset_seq.take() { diff --git a/alioth/src/virtio/dev/dev_test.rs b/alioth/src/virtio/dev/dev_test.rs index b0d37408..dd968edb 100644 --- a/alioth/src/virtio/dev/dev_test.rs +++ b/alioth/src/virtio/dev/dev_test.rs @@ -15,7 +15,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::thread::{self, JoinHandle}; -use std::time::Duration; +use std::time::{Duration, Instant}; use assert_matches::assert_matches; use mio::Registry; @@ -23,7 +23,9 @@ use mio::event::Event; use crate::sync::notifier::Notifier; use crate::virtio::dev::entropy::{EntropyConfig, EntropyFeature}; -use crate::virtio::dev::{Register, StartParam, Virtio, WakeEvent, WorkerCtl, WorkerParam}; +use crate::virtio::dev::{ + Register, StartParam, Virtio, WakeEvent, WorkerCtl, WorkerParam, WorkerReport, +}; use crate::virtio::queue::VirtQueue; use crate::virtio::tests::{FakeIrqSender, fixture_queues, fixture_ram_bus}; use crate::virtio::worker::mio::{ActiveMio, Mio, VirtioMio}; @@ -150,16 +152,16 @@ impl TestWorker { let fail_activate = dev.fail_activate.clone(); let resets = dev.resets.clone(); let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let param = WorkerParam { event_rx, - reset_tx, + report_tx, memory: Arc::new(fixture_ram_bus()), queue_regs: Arc::from(fixture_queues(1)), }; let (handle, notifier) = dev.spawn_worker(param).unwrap(); TestWorker { - ctl: WorkerCtl::new("fake".into(), event_tx, notifier, reset_rx), + ctl: WorkerCtl::new("fake".into(), event_tx, notifier, report_rx), handle, fail_activate, resets, @@ -186,6 +188,15 @@ impl TestWorker { self.resets.load(Ordering::Acquire) } + /// Waits until the worker reports a failure. + fn wait_failed(&self) { + let deadline = Instant::now() + TIMEOUT; + while !self.ctl.failed() { + assert!(Instant::now() < deadline, "no failure reported"); + thread::sleep(Duration::from_millis(1)); + } + } + fn shutdown(self) { assert!(self.ctl.wake(WakeEvent::Shutdown)); self.handle.join().unwrap(); @@ -212,6 +223,7 @@ fn test_worker_reset_after_start() { worker.start(); worker.reset(); assert_eq!(worker.resets(), 2); + assert!(!worker.ctl.failed()); worker.shutdown(); } @@ -220,15 +232,21 @@ fn test_worker_survives_activation_failure() { let worker = TestWorker::spawn(); worker.fail_activate.store(true, Ordering::Release); worker.start(); - // The worker cleans up the partially activated device, and stays alive - // to complete the reset. - worker.reset(); + // The worker cleans up the partially activated device before reporting + // the failure. + worker.wait_failed(); assert_eq!(worker.resets(), 1); + // The worker stays alive to complete the reset, which brings the device + // back into service. + worker.reset(); + assert!(!worker.ctl.failed()); + worker.fail_activate.store(false, Ordering::Release); worker.start(); worker.reset(); assert_eq!(worker.resets(), 2); + assert!(!worker.ctl.failed()); worker.shutdown(); } @@ -249,9 +267,9 @@ fn test_worker_exit_completes_reset() { #[test] fn test_wait_reset_timeout() { let (event_tx, _event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); let seq1 = ctl.request_reset(); let seq2 = ctl.request_reset(); @@ -262,7 +280,9 @@ fn test_wait_reset_timeout() { ); // Completing a reset also completes the earlier ones. - reset_tx.send(seq2).unwrap(); + report_tx + .send(WorkerReport::ResetDone { seq: seq2 }) + .unwrap(); assert!(ctl.poll_reset(seq1)); assert_matches!(ctl.wait_reset(seq2, Duration::ZERO), Ok(())); } @@ -270,12 +290,12 @@ fn test_wait_reset_timeout() { #[test] fn test_reset_seq_wraps_around() { let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); // Pretend that almost all sequence numbers have been used up. *ctl.reset_seq.lock() = u64::MAX - 1; - ctl.reset_acks.lock().done = u64::MAX - 1; + ctl.reports.lock().reset_done = u64::MAX - 1; let seq1 = ctl.request_reset(); let seq2 = ctl.request_reset(); @@ -285,11 +305,15 @@ fn test_reset_seq_wraps_around() { } assert!(!ctl.poll_reset(seq1)); - reset_tx.send(seq1).unwrap(); + report_tx + .send(WorkerReport::ResetDone { seq: seq1 }) + .unwrap(); assert!(ctl.poll_reset(seq1)); assert!(!ctl.poll_reset(seq2)); - reset_tx.send(seq2).unwrap(); + report_tx + .send(WorkerReport::ResetDone { seq: seq2 }) + .unwrap(); assert!(ctl.poll_reset(seq1)); assert_matches!(ctl.wait_reset(seq2, Duration::ZERO), Ok(())); } @@ -298,8 +322,8 @@ fn test_reset_seq_wraps_around() { fn test_request_reset_in_order() { let (event_tx, event_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let reset_rx = flume::unbounded().1; - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, reset_rx); + let report_rx = flume::unbounded().1; + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); let ctl = Arc::new(ctl); let threads: Vec<_> = (0..4) @@ -327,3 +351,29 @@ fn test_request_reset_in_order() { .collect(); assert_eq!(seqs, (1..=400).collect::>()); } + +#[test] +fn test_worker_failure_until_reset() { + let (event_tx, _event_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); + let notifier = Arc::new(Notifier::new().unwrap()); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); + assert!(!ctl.failed()); + + // Reports arrive in order. A failure before a reset is gone with the + // reset, while a failure after it is not. + let seq = ctl.request_reset(); + for report in [ + WorkerReport::Failed, + WorkerReport::ResetDone { seq }, + WorkerReport::Failed, + ] { + report_tx.send(report).unwrap(); + } + assert!(ctl.failed()); + assert!(ctl.poll_reset(seq)); + + let seq = ctl.request_reset(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); + assert!(!ctl.failed()); +} diff --git a/alioth/src/virtio/dev/entropy_test.rs b/alioth/src/virtio/dev/entropy_test.rs index 3b91e941..5f809c8d 100644 --- a/alioth/src/virtio/dev/entropy_test.rs +++ b/alioth/src/virtio/dev/entropy_test.rs @@ -79,7 +79,7 @@ fn entropy_test() { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, - reset_tx: flume::unbounded().0, + report_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs, }) diff --git a/alioth/src/virtio/dev/vsock/uds_vsock_test.rs b/alioth/src/virtio/dev/vsock/uds_vsock_test.rs index 534d5594..d6a904f9 100644 --- a/alioth/src/virtio/dev/vsock/uds_vsock_test.rs +++ b/alioth/src/virtio/dev/vsock/uds_vsock_test.rs @@ -131,7 +131,7 @@ impl<'m> VsockTest<'m> { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, - reset_tx: flume::unbounded().0, + report_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs, }) diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index e2181012..95761f59 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -34,7 +34,7 @@ use crate::pci::cap::{ use crate::pci::config::{BAR_MEM32, BAR_MEM64, BAR_PREFETCHABLE, PciConfigArea}; use crate::pci::{Pci, PciBar}; use crate::sync::notifier::Notifier; -use crate::virtio::dev::{Register, VirtioDevice, WakeEvent, WorkerCtl}; +use crate::virtio::dev::{Register, VirtioDevice, WakeEvent, WorkerCtl, WorkerReport}; use crate::virtio::pci::{ PciIrqSender, VIRTIO_MSI_NO_VECTOR, VirtioCommonCfg, VirtioPciCap, VirtioPciCap64, VirtioPciDevice, VirtioPciMsixVector, VirtioPciNotifyCap, VirtioPciRegister, @@ -48,9 +48,9 @@ type TestWakeReceiver = flume::Receiver>>; /// Creates the registers of a device, together with the worker ends of the /// wake event channel and the reset completion channel. -fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver, Sender) { +fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver, Sender) { let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let msi_sender = TestMsiSender::default(); let msix_table = Arc::new(MsixTableMmio { @@ -80,11 +80,11 @@ fn create_test_mmio(queues: Arc<[QueueReg]>) -> (TestMmio, TestWakeReceiver, Sen "test-virtio-pci".into(), event_tx, notifier, - reset_rx, + report_rx, )), reset_seq: Mutex::new(None), }; - (mmio, event_rx, reset_tx) + (mmio, event_rx, report_tx) } #[rstest] @@ -101,7 +101,7 @@ fn test_queue_address_reads(#[case] offset: usize, #[case] expected: u64) { device: AtomicU64::new(0x0123_4567_89ab_cdef), ..Default::default() }]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset as u64, 4), Ok(val) if val == expected); } @@ -130,7 +130,7 @@ fn test_queue_alignment( device: AtomicU64::new(0x3000_0000), ..Default::default() }]); - let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, _event_rx, _report_tx) = create_test_mmio(queues); if packed { assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_DRIVER_FEATURE_SELECT as u64, 4, 1), @@ -157,7 +157,7 @@ fn test_queue_alignment( #[case(VirtioCommonCfg::OFFSET_QUEUE_ENABLE, 2, 1)] fn test_queue_registers_write_read(#[case] offset: usize, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 0), @@ -177,7 +177,7 @@ fn test_queue_size_write_read(#[case] packed: bool, #[case] input: u64, #[case] size: AtomicU16::new(128), ..Default::default() }]); - let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, _event_rx, _report_tx) = create_test_mmio(queues); if packed { assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_DRIVER_FEATURE_SELECT as u64, 4, 1), @@ -202,7 +202,7 @@ fn test_queue_size_write_read(#[case] packed: bool, #[case] input: u64, #[case] #[test] fn test_queue_size_locked_when_enabled() { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 0), @@ -226,7 +226,7 @@ fn test_queue_size_locked_when_enabled() { #[test] fn test_queue_addrs_locked_when_enabled() { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues.clone()); + let (mmio, _, _report_tx) = create_test_mmio(queues.clone()); let offsets = [ VirtioCommonCfg::OFFSET_QUEUE_DESC_LO, @@ -281,7 +281,7 @@ fn test_queue_addrs_locked_when_enabled() { #[case(VirtioCommonCfg::OFFSET_QUEUE_ENABLE, 2, 1)] fn test_out_of_bounds_queue_writes(#[case] offset: usize, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, 99), @@ -330,7 +330,7 @@ fn test_common_cfg_queue_reads( ..Default::default() }, ]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!( mmio.write(VirtioCommonCfg::OFFSET_QUEUE_SELECT as u64, 2, q_sel), @@ -347,7 +347,7 @@ fn test_common_cfg_queue_reads( #[case(VirtioCommonCfg::OFFSET_QUEUE_RESET, 2, 0)] fn test_common_cfg_misc_reads(#[case] offset: usize, #[case] size: u8, #[case] expected: u64) { let queues = Arc::new([QueueReg::default(), QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset as u64, size), Ok(val) if val == expected); } @@ -357,7 +357,7 @@ fn test_common_cfg_misc_reads(#[case] offset: usize, #[case] size: u8, #[case] e #[case(10, 0)] fn test_device_feature_reads(#[case] sel: u64, #[case] expected_feature: u64) { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _, _reset_tx) = create_test_mmio(queues); + let (mut mmio, _, _report_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1111_2222, 0x3333_4444, 0, 0]; assert_matches!( @@ -385,7 +385,7 @@ fn test_device_feature_reads(#[case] sel: u64, #[case] expected_feature: u64) { #[case(10, 0x1234, 0)] fn test_driver_features(#[case] bank: u64, #[case] write_val: u64, #[case] expected: u64) { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + let (mut mmio, _event_rx, _report_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1234_5678, 0x0000_0005, 0, 0]; assert_matches!( @@ -413,7 +413,7 @@ fn test_driver_features(#[case] bank: u64, #[case] write_val: u64, #[case] expec #[test] fn test_driver_features_locked_after_features_ok() { let queues = Arc::new([QueueReg::default()]); - let (mut mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + let (mut mmio, _event_rx, _report_tx) = create_test_mmio(queues); mmio.reg.device_feature = [0x1234_5678, 0x0000_0005, 0, 0]; assert_matches!( @@ -458,7 +458,7 @@ fn test_driver_features_locked_after_features_ok() { #[test] fn test_device_status_valid_transitions() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, report_tx) = create_test_mmio(queues.clone()); // Initially status is 0 (empty) assert_matches!( @@ -649,7 +649,7 @@ fn test_device_status_valid_transitions() { Ok(0) ); - reset_tx.send(seq).unwrap(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); assert_matches!( mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), Ok(0) @@ -688,7 +688,7 @@ fn start_then_reset(mmio: &TestMmio, event_rx: &TestWakeReceiver) -> u64 { #[test] fn test_device_reset_in_progress() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, report_tx) = create_test_mmio(queues.clone()); let seq = start_then_reset(&mmio, &event_rx); // The driver must leave the device alone until the reset completes, so @@ -724,7 +724,7 @@ fn test_device_reset_in_progress() { // Once the worker has completed the reset, the driver can set up the // device again. - reset_tx.send(seq).unwrap(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); assert_matches!( mmio.write( VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, @@ -743,11 +743,11 @@ fn test_device_reset_in_progress() { #[test] fn test_device_reset_worker_exited() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, report_tx) = create_test_mmio(queues.clone()); start_then_reset(&mmio, &event_rx); // A worker that has exited no longer uses the queues. - drop(reset_tx); + drop(report_tx); assert_matches!( mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), Ok(0) @@ -789,7 +789,7 @@ fn test_device_reset_worker_exited() { )] fn test_device_status_invalid_transitions(#[case] initial: DevStatus, #[case] invalid_write: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, event_rx, _report_tx) = create_test_mmio(queues); if !initial.is_empty() { assert_matches!( @@ -843,7 +843,7 @@ fn test_device_status_multistep_transition( #[case] expect_start: bool, ) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, event_rx, _report_tx) = create_test_mmio(queues); // Set driver features assert_matches!( @@ -909,7 +909,7 @@ fn test_device_status_multistep_transition( #[test] fn test_device_status_reset_without_driver_ok() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, _reset_tx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, _report_tx) = create_test_mmio(queues.clone()); // Set status to ACK | DRIVER let ack_driver = DevStatus::ACK | DevStatus::DRIVER; @@ -963,7 +963,7 @@ fn test_device_status_reset_without_driver_ok() { #[test] fn test_device_reset_keeps_msix_table() { let queues = Arc::new([QueueReg::default()]); - let (mmio, _event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, _event_rx, _report_tx) = create_test_mmio(queues); let irqfd = TestIrqFd::default(); irqfd.set_addr_lo(0xfee0_0000).unwrap(); @@ -1019,7 +1019,7 @@ fn test_device_reset_queue_registers() { ..Default::default() }, ]); - let (mmio, event_rx, reset_tx) = create_test_mmio(queues.clone()); + let (mmio, event_rx, report_tx) = create_test_mmio(queues.clone()); // Initialize MSI-X table with 2 Entry slots *mmio.irq_sender.msix_table.entries.write() = vec![ @@ -1076,7 +1076,7 @@ fn test_device_reset_queue_registers() { panic!("expected a reset event"); }; assert!(event_rx.is_empty()); - reset_tx.send(seq).unwrap(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); assert_matches!( mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), Ok(0) @@ -1120,7 +1120,7 @@ fn test_device_reset_queue_registers() { #[case(VirtioCommonCfg::OFFSET_QUEUE_MSIX_VECTOR, Some(0))] fn test_msix_vector_configuration(#[case] offset: usize, #[case] queue_sel: Option) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); // Initialize MSI-X table with 2 Entry slots *mmio.irq_sender.msix_table.entries.write() = vec![ @@ -1157,7 +1157,7 @@ fn test_msix_vector_configuration(#[case] offset: usize, #[case] queue_sel: Opti #[case(VirtioPciRegister::OFFSET_QUEUE_NOTIFY + size_of::(), false)] fn test_queue_notify(#[case] offset: usize, #[case] expect_wake: bool) { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, event_rx, _report_tx) = create_test_mmio(queues); assert_matches!(mmio.write(offset as u64, 2, 0), Ok(Action::None)); if expect_wake { @@ -1211,7 +1211,7 @@ fn test_notify_with_notifiers() { #[test] fn test_wake_up_dev_channel_error() { let queues = Arc::new([QueueReg::default()]); - let (mmio, event_rx, _reset_tx) = create_test_mmio(queues); + let (mmio, event_rx, _report_tx) = create_test_mmio(queues); // Drop receiver so send will fail drop(event_rx); @@ -1230,7 +1230,7 @@ fn test_wake_up_dev_channel_error() { #[case(0xdead_beef, 4, 0x1234)] fn test_invalid_register_writes(#[case] offset: u64, #[case] size: u8, #[case] val: u64) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!(mmio.write(offset, size, val), Ok(Action::None)); } @@ -1239,7 +1239,7 @@ fn test_invalid_register_writes(#[case] offset: u64, #[case] size: u8, #[case] v #[case(0, 3)] fn test_invalid_register_reads(#[case] offset: u64, #[case] size: u8) { let queues = Arc::new([QueueReg::default()]); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_matches!(mmio.read(offset, size), Ok(0)); } @@ -1248,7 +1248,7 @@ fn test_invalid_register_reads(#[case] offset: u64, #[case] size: u8) { #[case(3, (size_of::() + size_of::() * 4) as u64)] fn test_mmio_size(#[case] num_queues: usize, #[case] expected_size: u64) { let queues = (0..num_queues).map(|_| QueueReg::default()).collect(); - let (mmio, _, _reset_tx) = create_test_mmio(queues); + let (mmio, _, _report_tx) = create_test_mmio(queues); assert_eq!(mmio.size(), expected_size); } @@ -1276,12 +1276,16 @@ fn create_test_virtio_device( config_size: u64, shared_mem: Option>, num_queues: usize, -) -> (VirtioDevice, Receiver>, Sender) +) -> ( + VirtioDevice, + Receiver>, + Sender, +) where S: IrqSender, { let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let queue_regs = (0..num_queues) .map(|_| QueueReg::default()) @@ -1297,11 +1301,11 @@ where "test-dev".into(), event_tx, notifier, - reset_rx, + report_rx, )), worker_handle: None, }; - (dev, event_rx, reset_tx) + (dev, event_rx, report_tx) } #[test] @@ -1509,7 +1513,7 @@ fn test_virtio_pci_device_classes( #[case] expected_subclass: u8, #[case] expected_dev_id: u16, ) { - let (dev, _rx, _reset_tx) = + let (dev, _rx, _report_tx) = create_test_virtio_device::>(id, 0, None, 2); let pci_dev = VirtioPciDevice::new( dev, @@ -1563,7 +1567,7 @@ fn test_virtio_pci_device_with_config_and_shared_memory_prefetchable() { ], callbacks: Mutex::new(vec![]), }); - let (dev, _rx, _reset_tx) = create_test_virtio_device::>( + let (dev, _rx, _report_tx) = create_test_virtio_device::>( DeviceId::FILE_SYSTEM, 32, Some(shared_mem), @@ -1615,7 +1619,7 @@ fn test_virtio_pci_device_shared_memory_non_prefetchable() { }], callbacks: Mutex::new(vec![]), }); - let (dev, _rx, _reset_tx) = create_test_virtio_device::>( + let (dev, _rx, _report_tx) = create_test_virtio_device::>( DeviceId::FILE_SYSTEM, 0, Some(shared_mem), @@ -1634,7 +1638,7 @@ fn test_virtio_pci_device_shared_memory_non_prefetchable() { #[test] fn test_virtio_pci_device_notifier_callback() { - let (dev, _rx, _reset_tx) = + let (dev, _rx, _report_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 2); let registry = TestNotifierRegistry::default(); let registered = registry.registered.clone(); @@ -1679,7 +1683,7 @@ fn test_virtio_pci_device_notifier_callback() { #[test] fn test_virtio_pci_device_notifier_fallback() { - let (dev, _rx, _reset_tx) = + let (dev, _rx, _report_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 1); let pci_dev = VirtioPciDevice::new( dev, @@ -1699,7 +1703,7 @@ fn test_virtio_pci_device_notifier_fallback() { #[test] fn test_virtio_pci_device_pci_reset() { - let (dev, event_rx, reset_tx) = + let (dev, event_rx, report_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 1); let pci_dev = VirtioPciDevice::new( dev, @@ -1729,7 +1733,7 @@ fn test_virtio_pci_device_pci_reset() { }; assert_eq!(registers.reg.status.load(Ordering::Acquire), driver_ok); assert!(registers.queues[0].enabled.load(Ordering::Acquire)); - reset_tx.send(seq).unwrap(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); }); // Pci::reset returns only after the worker has completed the reset. @@ -1753,7 +1757,7 @@ fn test_virtio_pci_device_pci_reset() { #[test] fn test_virtio_pci_device_pci_reset_timeout() { - let (dev, event_rx, _reset_tx) = + let (dev, event_rx, _report_tx) = create_test_virtio_device::>(DeviceId::NET, 0, None, 1); let pci_dev = VirtioPciDevice::new( dev, diff --git a/alioth/src/virtio/vu/backend_test.rs b/alioth/src/virtio/vu/backend_test.rs index 2e211061..d3431ea2 100644 --- a/alioth/src/virtio/vu/backend_test.rs +++ b/alioth/src/virtio/vu/backend_test.rs @@ -23,7 +23,7 @@ use crate::mem; use crate::mem::emulated::{Action, Mmio}; use crate::mem::mapped::RamBus; use crate::sync::notifier::Notifier; -use crate::virtio::dev::{VirtioDevice, WakeEvent, WorkerCtl}; +use crate::virtio::dev::{VirtioDevice, WakeEvent, WorkerCtl, WorkerReport}; use crate::virtio::queue::QueueReg; use crate::virtio::vu::backend::VuBackend; use crate::virtio::vu::bindings::VirtqState; @@ -50,7 +50,7 @@ impl Mmio for TestDevConfig { #[test] fn test_vu_backend_stops_worker_before_reply() { let (event_tx, event_rx) = flume::unbounded(); - let (reset_tx, reset_rx) = flume::unbounded(); + let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let dev = VirtioDevice { name: "vu-backend".into(), @@ -63,7 +63,7 @@ fn test_vu_backend_stops_worker_before_reply() { "vu-backend".into(), event_tx, notifier, - reset_rx, + report_rx, )), worker_handle: None, }; @@ -79,7 +79,7 @@ fn test_vu_backend_stops_worker_before_reply() { WakeEvent::Reset { seq } => { thread::sleep(Duration::from_millis(50)); worker_stopped.store(true, Ordering::Release); - reset_tx.send(seq).unwrap(); + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); } event => panic!("unexpected event: {event:?}"), } diff --git a/alioth/src/virtio/worker/io_uring_test.rs b/alioth/src/virtio/worker/io_uring_test.rs index 904165df..694626e5 100644 --- a/alioth/src/virtio/worker/io_uring_test.rs +++ b/alioth/src/virtio/worker/io_uring_test.rs @@ -138,7 +138,7 @@ fn io_uring_drain_test() { let (handle, notifier) = dev .spawn_worker(WorkerParam { event_rx: rx, - reset_tx: flume::unbounded().0, + report_tx: flume::unbounded().0, memory: ram_bus.clone(), queue_regs: regs.clone(), }) From 00eb59b8e9228007679fc66fa0e38fea87f765f8 Mon Sep 17 00:00:00 2001 From: Changyuan Lyu Date: Sat, 7 Nov 2026 23:18:46 -0800 Subject: [PATCH 8/8] feat(virtio): set DEVICE_NEEDS_RESET when a device fails When the worker of a running device fails, e.g. because the driver has set up a queue that the device cannot use, the device silently stops serving the queues. Set DEVICE_NEEDS_RESET and send a configuration change notification, which the virtio spec asks of a device that enters an error state that needs a reset (VirtIO spec 1.4, Sec 2.1.2). The PCI transport sets the bit when the driver reads device_status, and the vhost-user backend when the frontend reads the status. Resetting the device clears it. Signed-off-by: Changyuan Lyu --- alioth/src/virtio/dev/dev.rs | 4 + alioth/src/virtio/dev/dev_test.rs | 53 +++++++++++-- alioth/src/virtio/pci.rs | 14 +++- alioth/src/virtio/pci_test.rs | 50 +++++++++++++ alioth/src/virtio/vu/backend.rs | 4 + alioth/src/virtio/vu/backend_test.rs | 108 ++++++++++++++++++++------- 6 files changed, 197 insertions(+), 36 deletions(-) diff --git a/alioth/src/virtio/dev/dev.rs b/alioth/src/virtio/dev/dev.rs index 8355d944..df8e0d9b 100644 --- a/alioth/src/virtio/dev/dev.rs +++ b/alioth/src/virtio/dev/dev.rs @@ -643,6 +643,10 @@ where if ret.is_err() && self.context.state == WorkerState::Running { self.context.state = WorkerState::Pending; self.context.report(WorkerReport::Failed); + // The transport sets DEVICE_NEEDS_RESET, after which a running + // device must notify the driver of a configuration change + // (VirtIO spec 1.4, Sec 2.1.2). + param.irq_sender.config_irq(); } ret } diff --git a/alioth/src/virtio/dev/dev_test.rs b/alioth/src/virtio/dev/dev_test.rs index dd968edb..fa2a3fff 100644 --- a/alioth/src/virtio/dev/dev_test.rs +++ b/alioth/src/virtio/dev/dev_test.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::os::fd::BorrowedFd; use std::sync::Arc; use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; use std::thread::{self, JoinHandle}; @@ -27,7 +28,7 @@ use crate::virtio::dev::{ Register, StartParam, Virtio, WakeEvent, WorkerCtl, WorkerParam, WorkerReport, }; use crate::virtio::queue::VirtQueue; -use crate::virtio::tests::{FakeIrqSender, fixture_queues, fixture_ram_bus}; +use crate::virtio::tests::{fixture_queues, fixture_ram_bus}; use crate::virtio::worker::mio::{ActiveMio, Mio, VirtioMio}; use crate::virtio::{DeviceId, Error, IrqSender, Result, VirtioFeature, error}; @@ -139,9 +140,38 @@ impl VirtioMio for FakeDev { } } +/// Counts configuration interrupts. +#[derive(Debug, Default)] +struct TestIrqSender { + config_irqs: AtomicU32, +} + +impl IrqSender for TestIrqSender { + fn queue_irq(&self, _idx: u16) {} + + fn config_irq(&self) { + self.config_irqs.fetch_add(1, Ordering::AcqRel); + } + + fn queue_irqfd(&self, _idx: u16, _f: F) -> Result + where + F: FnOnce(BorrowedFd) -> Result, + { + unimplemented!() + } + + fn config_irqfd(&self, _f: F) -> Result + where + F: FnOnce(BorrowedFd) -> Result, + { + unimplemented!() + } +} + struct TestWorker { - ctl: WorkerCtl, + ctl: WorkerCtl, handle: JoinHandle<()>, + irq_sender: Arc, fail_activate: Arc, resets: Arc, } @@ -163,16 +193,16 @@ impl TestWorker { TestWorker { ctl: WorkerCtl::new("fake".into(), event_tx, notifier, report_rx), handle, + irq_sender: Arc::default(), fail_activate, resets, } } fn start(&self) { - let (q_tx, _) = flume::unbounded(); let param = StartParam { feature: VirtioFeature::VERSION_1.bits(), - irq_sender: Arc::new(FakeIrqSender { q_tx }), + irq_sender: self.irq_sender.clone(), notifiers: None, }; assert!(self.ctl.wake(WakeEvent::Start { param })); @@ -188,6 +218,10 @@ impl TestWorker { self.resets.load(Ordering::Acquire) } + fn config_irqs(&self) -> u32 { + self.irq_sender.config_irqs.load(Ordering::Acquire) + } + /// Waits until the worker reports a failure. fn wait_failed(&self) { let deadline = Instant::now() + TIMEOUT; @@ -224,6 +258,7 @@ fn test_worker_reset_after_start() { worker.reset(); assert_eq!(worker.resets(), 2); assert!(!worker.ctl.failed()); + assert_eq!(worker.config_irqs(), 0); worker.shutdown(); } @@ -241,6 +276,8 @@ fn test_worker_survives_activation_failure() { // back into service. worker.reset(); assert!(!worker.ctl.failed()); + // The worker has notified the driver of the failure. + assert_eq!(worker.config_irqs(), 1); worker.fail_activate.store(false, Ordering::Release); worker.start(); @@ -269,7 +306,7 @@ fn test_wait_reset_timeout() { let (event_tx, _event_rx) = flume::unbounded(); let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); let seq1 = ctl.request_reset(); let seq2 = ctl.request_reset(); @@ -292,7 +329,7 @@ fn test_reset_seq_wraps_around() { let (event_tx, event_rx) = flume::unbounded(); let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); // Pretend that almost all sequence numbers have been used up. *ctl.reset_seq.lock() = u64::MAX - 1; ctl.reports.lock().reset_done = u64::MAX - 1; @@ -323,7 +360,7 @@ fn test_request_reset_in_order() { let (event_tx, event_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); let report_rx = flume::unbounded().1; - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); let ctl = Arc::new(ctl); let threads: Vec<_> = (0..4) @@ -357,7 +394,7 @@ fn test_worker_failure_until_reset() { let (event_tx, _event_rx) = flume::unbounded(); let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); - let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); + let ctl = WorkerCtl::::new("fake".into(), event_tx, notifier, report_rx); assert!(!ctl.failed()); // Reports arrive in order. A failure before a reset is gone with the diff --git a/alioth/src/virtio/pci.rs b/alioth/src/virtio/pci.rs index 5a7b7956..bb9d3228 100644 --- a/alioth/src/virtio/pci.rs +++ b/alioth/src/virtio/pci.rs @@ -250,6 +250,16 @@ where true } + /// Tells the driver that the device needs a reset, after the worker of + /// the running device has failed (VirtIO spec 1.4, Sec 2.1.2). + fn set_needs_reset(&self) { + let status = DevStatus::from_bits_retain(self.reg.status.load(Ordering::Acquire)); + if status.contains(DevStatus::DRIVER_OK) { + let status = status | DevStatus::NEEDS_RESET; + self.reg.status.store(status.bits(), Ordering::Release); + } + } + /// Resets the device and waits for the worker to complete the reset. fn reset_sync(&self, timeout: Duration) -> Result<()> { let mut reset_seq = self.reset_seq.lock(); @@ -360,7 +370,9 @@ where VirtioCommonCfg::LAYOUT_DEVICE_STATUS => { // The driver polls device_status until a reset completes. let mut reset_seq = self.reset_seq.lock(); - self.poll_reset(&mut reset_seq); + if self.poll_reset(&mut reset_seq) && self.ctl.failed() { + self.set_needs_reset(); + } reg.status.load(Ordering::Acquire) as u64 } VirtioCommonCfg::LAYOUT_CONFIG_GENERATION => { diff --git a/alioth/src/virtio/pci_test.rs b/alioth/src/virtio/pci_test.rs index 95761f59..b618c8ee 100644 --- a/alioth/src/virtio/pci_test.rs +++ b/alioth/src/virtio/pci_test.rs @@ -740,6 +740,56 @@ fn test_device_reset_in_progress() { assert!(!queues[0].enabled.load(Ordering::Acquire)); } +#[test] +fn test_device_needs_reset_after_worker_failure() { + let queues = Arc::new([QueueReg::default()]); + let (mmio, event_rx, report_tx) = create_test_mmio(queues); + let driver_ok = + DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + assert_matches!( + mmio.write( + VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, + 1, + driver_ok.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!(event_rx.try_recv(), Ok(WakeEvent::Start { .. })); + + report_tx.send(WorkerReport::Failed).unwrap(); + let needs_reset = driver_ok | DevStatus::NEEDS_RESET; + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(status) if status == needs_reset.bits() as u64 + ); + + // Resetting the device brings it back into service. + assert_matches!( + mmio.write(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1, 0), + Ok(Action::None) + ); + let Ok(WakeEvent::Reset { seq }) = event_rx.try_recv() else { + panic!("expected a reset event"); + }; + report_tx.send(WorkerReport::ResetDone { seq }).unwrap(); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(0) + ); + assert_matches!( + mmio.write( + VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, + 1, + DevStatus::ACK.bits() as u64 + ), + Ok(Action::None) + ); + assert_matches!( + mmio.read(VirtioCommonCfg::OFFSET_DEVICE_STATUS as u64, 1), + Ok(status) if status == DevStatus::ACK.bits() as u64 + ); +} + #[test] fn test_device_reset_worker_exited() { let queues = Arc::new([QueueReg::default()]); diff --git a/alioth/src/virtio/vu/backend.rs b/alioth/src/virtio/vu/backend.rs index 6410cd7d..76cdaba2 100644 --- a/alioth/src/virtio/vu/backend.rs +++ b/alioth/src/virtio/vu/backend.rs @@ -439,6 +439,10 @@ impl VuBackend { } } (VuFrontMsg::GET_STATUS, 0) => { + if self.running && self.dev.ctl.failed() { + // VirtIO spec 1.4, Sec 2.1.2 + self.status |= DevStatus::NEEDS_RESET; + } let status = self.status.bits() as u64; self.session.reply(req, &status, &[])?; msg.flag.set_need_reply(false); diff --git a/alioth/src/virtio/vu/backend_test.rs b/alioth/src/virtio/vu/backend_test.rs index d3431ea2..212c7165 100644 --- a/alioth/src/virtio/vu/backend_test.rs +++ b/alioth/src/virtio/vu/backend_test.rs @@ -15,9 +15,11 @@ use std::os::fd::AsFd; use std::os::unix::net::UnixStream; use std::sync::Arc; -use std::sync::atomic::{AtomicBool, Ordering}; -use std::thread; -use std::time::Duration; +use std::sync::atomic::{AtomicBool, AtomicU32, Ordering}; +use std::thread::{self, JoinHandle}; +use std::time::{Duration, Instant}; + +use flume::{Receiver, Sender}; use crate::mem; use crate::mem::emulated::{Action, Mmio}; @@ -25,7 +27,7 @@ use crate::mem::mapped::RamBus; use crate::sync::notifier::Notifier; use crate::virtio::dev::{VirtioDevice, WakeEvent, WorkerCtl, WorkerReport}; use crate::virtio::queue::QueueReg; -use crate::virtio::vu::backend::VuBackend; +use crate::virtio::vu::backend::{VuBackend, VuIrqSender}; use crate::virtio::vu::bindings::VirtqState; use crate::virtio::vu::conn::VuSession; use crate::virtio::{DevStatus, DeviceId}; @@ -47,8 +49,16 @@ impl Mmio for TestDevConfig { } } -#[test] -fn test_vu_backend_stops_worker_before_reply() { +type TestWakeReceiver = Receiver>; + +/// Serves a device with one queue, whose worker is `worker`, on a backend +/// thread. Returns the frontend end of the connection and the thread. +/// +/// The thread joins the worker after the frontend disconnects. +fn spawn_backend(worker: W) -> (VuSession, JoinHandle<()>) +where + W: FnOnce(TestWakeReceiver, Sender) + Send + 'static, +{ let (event_tx, event_rx) = flume::unbounded(); let (report_tx, report_rx) = flume::unbounded(); let notifier = Arc::new(Notifier::new().unwrap()); @@ -67,15 +77,43 @@ fn test_vu_backend_stops_worker_before_reply() { )), worker_handle: None, }; + let worker = thread::spawn(move || worker(event_rx, report_tx)); + + let (frontend_conn, backend_conn) = UnixStream::pair().unwrap(); + let mut backend = VuBackend::new(backend_conn, dev, Arc::new(RamBus::new())).unwrap(); + let backend_thread = thread::spawn(move || { + backend.run().unwrap(); + // Dropping the backend disconnects the worker. + drop(backend); + worker.join().unwrap(); + }); + (VuSession::from(frontend_conn), backend_thread) +} + +fn driver_ok() -> u64 { + let status = DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; + status.bits() as u64 +} + +/// Starts the device. The backend takes the kick fd for each start. +fn start(frontend: &VuSession) { + let kick = Notifier::new().unwrap(); + frontend.set_virtq_kick(&0, kick.as_fd()).unwrap(); + frontend.set_status(&driver_ok()).unwrap(); +} +#[test] +fn test_vu_backend_stops_worker_before_reply() { // A fake worker that takes a while to stop. + let starts = Arc::new(AtomicU32::new(0)); let stopped = Arc::new(AtomicBool::new(false)); - let worker_stopped = stopped.clone(); - let worker = thread::spawn(move || { - let mut starts = 0; + let (worker_starts, worker_stopped) = (starts.clone(), stopped.clone()); + let (frontend, backend_thread) = spawn_backend(move |event_rx, report_tx| { for event in event_rx.iter() { match event { - WakeEvent::Start { .. } => starts += 1, + WakeEvent::Start { .. } => { + worker_starts.fetch_add(1, Ordering::AcqRel); + } WakeEvent::Reset { seq } => { thread::sleep(Duration::from_millis(50)); worker_stopped.store(true, Ordering::Release); @@ -84,28 +122,15 @@ fn test_vu_backend_stops_worker_before_reply() { event => panic!("unexpected event: {event:?}"), } } - starts }); - let (frontend_conn, backend_conn) = UnixStream::pair().unwrap(); - let mut backend = VuBackend::new(backend_conn, dev, Arc::new(RamBus::new())).unwrap(); - let backend_thread = thread::spawn(move || backend.run().unwrap()); - - let frontend = VuSession::from(frontend_conn); - let kick = Notifier::new().unwrap(); - let driver_ok = - DevStatus::ACK | DevStatus::DRIVER | DevStatus::FEATURES_OK | DevStatus::DRIVER_OK; - let driver_ok = driver_ok.bits() as u64; - // The reply to SET_STATUS(0) means that the worker has stopped. - frontend.set_virtq_kick(&0, kick.as_fd()).unwrap(); - frontend.set_status(&driver_ok).unwrap(); + start(&frontend); frontend.set_status(&0).unwrap(); assert!(stopped.swap(false, Ordering::AcqRel)); // So does the reply to GET_VRING_BASE. - frontend.set_virtq_kick(&0, kick.as_fd()).unwrap(); - frontend.set_status(&driver_ok).unwrap(); + start(&frontend); let virtq_base = VirtqState { index: 0, val: 0 }; assert_eq!(frontend.get_virtq_base(&virtq_base).unwrap().val, 0); assert!(stopped.swap(false, Ordering::AcqRel)); @@ -116,6 +141,35 @@ fn test_vu_backend_stops_worker_before_reply() { drop(frontend); backend_thread.join().unwrap(); - // Dropping the backend disconnects the worker. - assert_eq!(worker.join().unwrap(), 2); + assert_eq!(starts.load(Ordering::Acquire), 2); +} + +#[test] +fn test_vu_backend_needs_reset_after_worker_failure() { + // A fake worker that fails as soon as it starts. + let (frontend, backend_thread) = spawn_backend(move |event_rx, report_tx| { + for event in event_rx.iter() { + let report = match event { + WakeEvent::Start { .. } => WorkerReport::Failed, + WakeEvent::Reset { seq } => WorkerReport::ResetDone { seq }, + event => panic!("unexpected event: {event:?}"), + }; + report_tx.send(report).unwrap(); + } + }); + + start(&frontend); + let needs_reset = driver_ok() | DevStatus::NEEDS_RESET.bits() as u64; + let deadline = Instant::now() + Duration::from_secs(5); + while frontend.get_status().unwrap() != needs_reset { + assert!(Instant::now() < deadline, "no DEVICE_NEEDS_RESET"); + thread::sleep(Duration::from_millis(1)); + } + + // Resetting the device brings it back into service. + frontend.set_status(&0).unwrap(); + assert_eq!(frontend.get_status().unwrap(), 0); + + drop(frontend); + backend_thread.join().unwrap(); }