From 2fa5c2e31e5d5f87b3d97ea2255c24a8d223e90f Mon Sep 17 00:00:00 2001 From: bitranox Date: Fri, 10 Jul 2026 13:01:38 +0200 Subject: [PATCH] pcie, virtio, nvme: preserve and quiesce emulated PCIe devices across guest reset On a guest reboot, emulated PCIe devices either vanished from the guest or corrupted the rebooted guest's memory. Three fixes: pcie: a VM reset resets device config space, which clears the downstream-port slot-status Presence Detect bit. The connected device (the port's link) survives the reset, so the slot is still occupied, but the guest's pciehp driver reads the port after reboot, sees no presence, and removes the still-attached device: an emulated disk or NIC disappears on every reboot until a manual PCI rescan. Re-derive presence from the link on reset, for both root-complex root ports and switch downstream ports. virtio: the reset/disable teardown drained each queue with stop_queue, which posts completions to the used ring. A disk read submitted before the reset completes asynchronously and writes into guest memory the rebooted guest has already reused. Add VirtioDevice::abort_queue, used by the teardown path: it awaits the in-flight IO to completion, so it finishes while the guest is stopped, then discards the completion without posting. The default falls back to stop_queue; virtio-blk overrides it. The suspend/resume path still posts. nvme: a shadow-doorbell mapping installed by Doorbell Buffer Config survived controller reset, so after a reboot the controller wrote shadow event_idx into the stale guest address the rebooted guest had reused. Reset the doorbell store to its power-on host-backed state on controller reset. Tested by rebooting a Linux guest with virtio-blk, nvme, and virtio-net attached in a loop: before, guest memory was corrupted on the first reboot (random anon_vma / slab / rbtree oopses); after, it survives repeated reboots with the devices intact. --- vm/devices/pci/pcie/src/port.rs | 84 +++++++++++++++++++ vm/devices/pci/pcie/src/root.rs | 5 +- vm/devices/pci/pcie/src/switch.rs | 61 +++++++++++++- vm/devices/storage/nvme/src/queue.rs | 11 +++ .../storage/nvme/src/workers/coordinator.rs | 10 +++ vm/devices/virtio/virtio/src/device.rs | 26 ++++++ .../virtio/virtio/src/transport/task.rs | 8 +- vm/devices/virtio/virtio_blk/src/lib.rs | 37 ++++++++ 8 files changed, 238 insertions(+), 4 deletions(-) diff --git a/vm/devices/pci/pcie/src/port.rs b/vm/devices/pci/pcie/src/port.rs index fb2363005a..f08c57e32b 100644 --- a/vm/devices/pci/pcie/src/port.rs +++ b/vm/devices/pci/pcie/src/port.rs @@ -791,6 +791,24 @@ impl PcieDownstreamPort { bail!("port name does not match") } + /// Reset the port's config space, preserving presence for a still-connected + /// device. + /// + /// A VM reset resets the config space, which clears the PCIe slot-status + /// Presence Detect bit. But `self.link` (the connected device) survives the + /// reset, so the slot is still occupied. Without re-asserting presence here, + /// the guest's pciehp driver reads the port after reboot, sees Presence + /// Detect clear ("card not present"), and removes the still-attached device + /// even though its link is up - so an emulated PCIe disk/NIC disappears on + /// every guest reboot until a manual PCI rescan. Re-derive presence from the + /// link so a boot-present device stays present across reset. + pub fn reset(&mut self) { + self.cfg_space.reset(); + if self.link.is_some() { + self.cfg_space.set_presence_detect_state(true); + } + } + /// Hot-add a device to this port at runtime. /// /// Unlike `add_pcie_device`, this method verifies the port is hotplug-capable @@ -1046,6 +1064,72 @@ mod tests { ); } + #[test] + fn test_reset_preserves_presence_detect_for_connected_device() { + use pci_core::spec::hwid::{ClassCode, ProgrammingInterface, Subclass}; + + let hardware_ids = HardwareIds { + vendor_id: 0x1234, + device_id: 0x5678, + revision_id: 0, + prog_if: ProgrammingInterface::NONE, + sub_class: Subclass::BRIDGE_PCI_TO_PCI, + base_class: ClassCode::BRIDGE, + type0_sub_vendor_id: 0, + type0_sub_system_id: 0, + }; + + let msi_conn = pci_core::msi::MsiConnection::new(); + let mut port = PcieDownstreamPort::new( + "test-port", + hardware_ids, + DevicePortType::RootPort, + false, + Some(1), // hotplug slot 1 + &msi_conn.target(), + PciePortSettings::default(), + None, + None, + ); + + // presence_detect_state is bit 6 of slot status at cap offset 0x18. + fn presence(port: &mut PcieDownstreamPort) -> u32 { + let mut v = 0u32; + assert!(matches!( + port.cfg_space.read_u32(0x58, &mut v), + IoResult::Ok + )); + (v >> 22) & 0x1 + } + + // An empty port reports no presence after reset. + port.reset(); + assert_eq!( + presence(&mut port), + 0, + "empty port: no presence after reset" + ); + + // Connect a device (presence asserted), then reset as a guest reboot does. + port.add_pcie_device("test-port", "mock-device", Box::new(MockDevice)) + .unwrap(); + assert_eq!( + presence(&mut port), + 1, + "presence asserted after connecting device" + ); + + // Regression: the connected device (self.link) survives the reset, so + // presence must remain set. If it clears, the guest's pciehp evicts the + // still-attached device on reboot (the PCIe-devices-vanish-on-reboot bug). + port.reset(); + assert_eq!( + presence(&mut port), + 1, + "presence must survive reset while a device is connected" + ); + } + #[test] fn test_add_pcie_device_without_hotplug() { use pci_core::spec::hwid::{ClassCode, ProgrammingInterface, Subclass}; diff --git a/vm/devices/pci/pcie/src/root.rs b/vm/devices/pci/pcie/src/root.rs index fc04516972..1a00740cc2 100644 --- a/vm/devices/pci/pcie/src/root.rs +++ b/vm/devices/pci/pcie/src/root.rs @@ -575,7 +575,10 @@ impl ChangeDeviceState for GenericPcieRootComplex { async fn reset(&mut self) { for (_, d) in self.devices.iter_mut() { if let BusDevice::RootPort { port, .. } = d { - port.port.cfg_space.reset(); + // Reset via the port so presence detect is re-asserted for a + // still-connected device; a bare cfg_space.reset() would clear + // it and the guest's pciehp would evict the device on reboot. + port.port.reset(); } } } diff --git a/vm/devices/pci/pcie/src/switch.rs b/vm/devices/pci/pcie/src/switch.rs index 0ba7395bdf..f4c20ec3dd 100644 --- a/vm/devices/pci/pcie/src/switch.rs +++ b/vm/devices/pci/pcie/src/switch.rs @@ -346,9 +346,12 @@ impl ChangeDeviceState for GenericPcieSwitch { // Reset the upstream port configuration space self.upstream_port.cfg_space.reset(); - // Reset all downstream port configuration spaces + // Reset each downstream port via the port (not a bare cfg_space.reset), + // so presence detect is re-asserted for a still-connected device; a bare + // cfg_space.reset() would clear it and the guest's pciehp would evict a + // device attached behind the switch on reboot. for (_, _, downstream_port) in self.downstream_ports.iter_mut() { - downstream_port.port.cfg_space.reset(); + downstream_port.port.reset(); } } } @@ -887,6 +890,60 @@ mod tests { assert!(port_numbers.contains(&2)); } + #[pal_async::async_test] + async fn test_reset_preserves_presence_detect_behind_switch() { + use crate::test_helpers::TestPcieEndpoint; + use chipset_device::io::IoResult; + + let mut switch = build(switch_def( + "test-switch", + 2, + true, // hotplug-capable downstream ports + PciePortSettings::default(), + MsiTarget::disconnected(), + )); + + // Presence Detect is bit 6 of the PCIe slot-status register (cap offset + // 0x18), i.e. bit 22 of the dword at config offset 0x58 on a downstream + // port. + fn presence(switch: &GenericPcieSwitch, idx: usize) -> u32 { + let mut v = 0u32; + assert!(matches!( + switch.downstream_ports[idx] + .2 + .cfg_space() + .read_u32(0x58, &mut v), + IoResult::Ok + )); + (v >> 22) & 0x1 + } + + // Connect a device to the first downstream port; presence asserts. + let devfn = switch.downstream_ports[0].0; + switch + .add_pcie_device( + devfn, + "downstream-dev", + Box::new(TestPcieEndpoint::new( + |_, _| Some(IoResult::Ok), + |_, _| Some(IoResult::Ok), + )), + ) + .unwrap(); + assert_eq!(presence(&switch, 0), 1, "presence asserted after connect"); + + // A guest reboot resets the switch; the connected device's link survives, + // so presence must remain set - otherwise the guest's pciehp evicts a + // device attached behind the switch on reboot (the same bug this fixes for + // root ports). + switch.reset().await; + assert_eq!( + presence(&switch, 0), + 1, + "presence must survive switch reset while a device is connected" + ); + } + #[test] fn test_switch_device_connections() { use crate::test_helpers::TestPcieEndpoint; diff --git a/vm/devices/storage/nvme/src/queue.rs b/vm/devices/storage/nvme/src/queue.rs index 7b46ae9266..37d0b77b6a 100644 --- a/vm/devices/storage/nvme/src/queue.rs +++ b/vm/devices/storage/nvme/src/queue.rs @@ -60,6 +60,17 @@ impl DoorbellMemory { Ok(()) } + /// Restore the doorbell store to its power-on state: a fresh internal + /// host-side buffer with no guest-backed shadow-doorbell mapping. + /// + /// Called on controller reset. A shadow-doorbell mapping installed by + /// Doorbell Buffer Config otherwise survives the reset, so after the guest + /// tears the controller down and reboots, the controller writes shadow + /// event_idx into a guest address the rebooted guest has already reused. + pub fn reset(&mut self) { + *self = Self::new(self.wakers.len() as u16); + } + pub fn try_write(&self, db_id: u16, value: u32) -> Result<(), InvalidDoorbell> { if (db_id as usize) >= self.wakers.len() { return Err(InvalidDoorbell); diff --git a/vm/devices/storage/nvme/src/workers/coordinator.rs b/vm/devices/storage/nvme/src/workers/coordinator.rs index 365ec9cf7b..c0edd2cff4 100644 --- a/vm/devices/storage/nvme/src/workers/coordinator.rs +++ b/vm/devices/storage/nvme/src/workers/coordinator.rs @@ -82,6 +82,7 @@ impl NvmeWorkers { driver: driver.clone(), admin: TaskControl::new(handler), reset: None, + doorbells: doorbells.clone(), }; let (send, recv) = mesh::mpsc_channel(); let task = driver.spawn("nvme-coord", coordinator.run(recv)); @@ -208,6 +209,10 @@ struct Coordinator { admin: TaskControl, #[inspect(with = "Option::is_some")] reset: Option>, + // Cleared on every controller reset so a stale shadow-doorbell mapping + // cannot outlive the reset and corrupt a rebooted guest's reused memory. + #[inspect(skip)] + doorbells: Arc>, } enum CoordinatorRequest { @@ -240,6 +245,11 @@ impl Coordinator { state.drain().await; self.admin.remove(); } + // Drop any shadow-doorbell mapping installed by Doorbell + // Buffer Config. Otherwise its guest-memory address survives + // the reset and the controller writes shadow event_idx into a + // page the rebooted guest has reused. See DoorbellMemory::reset. + self.doorbells.write().reset(); } else { pending().await } diff --git a/vm/devices/virtio/virtio/src/device.rs b/vm/devices/virtio/virtio/src/device.rs index 598e513204..3887006b06 100644 --- a/vm/devices/virtio/virtio/src/device.rs +++ b/vm/devices/virtio/virtio/src/device.rs @@ -97,6 +97,24 @@ pub trait VirtioDevice: InspectMut + Send { /// iterating all queue indices, not just active ones. fn stop_queue(&mut self, idx: u16) -> impl Future> + Send; + /// Tear down a single queue on the reset/disable path. + /// + /// The queue is being torn down and the guest no longer owns the ring + /// memory, so a device with in-flight host IO must not publish its + /// completions to the used ring: publishing there (as `stop_queue` does) + /// writes into pages a rebooted guest has already reused. Such a device + /// overrides this to discard the completions, and must also ensure no + /// in-flight host IO writes guest memory after this returns (it cancels the + /// IO, or awaits it so the write lands while the guest is still stopped). + /// + /// The default forwards to `stop_queue`, which is correct only for a device + /// with no in-flight host IO to discard. + fn abort_queue(&mut self, idx: u16) -> impl Future + Send { + async move { + let _ = self.stop_queue(idx).await; + } + } + /// Reset device-internal state to initial values. /// /// Called after all queues have been stopped on guest-initiated reset. @@ -161,6 +179,10 @@ pub trait DynVirtioDevice: InspectMut + Send { idx: u16, ) -> Pin> + Send + '_>>; + /// Tear down a single queue on the reset/disable path (object-safe mirror + /// of `VirtioDevice::abort_queue`). + fn abort_queue(&mut self, idx: u16) -> Pin + Send + '_>>; + /// Reset device-internal state. fn reset(&mut self) -> Pin + Send + '_>>; @@ -222,6 +244,10 @@ impl DynVirtioDevice for T { Box::pin(VirtioDevice::stop_queue(self, idx)) } + fn abort_queue(&mut self, idx: u16) -> Pin + Send + '_>> { + Box::pin(VirtioDevice::abort_queue(self, idx)) + } + fn reset(&mut self) -> Pin + Send + '_>> { Box::pin(VirtioDevice::reset(self)) } diff --git a/vm/devices/virtio/virtio/src/transport/task.rs b/vm/devices/virtio/virtio/src/transport/task.rs index e3f71fbb04..e5459f505e 100644 --- a/vm/devices/virtio/virtio/src/transport/task.rs +++ b/vm/devices/virtio/virtio/src/transport/task.rs @@ -231,8 +231,14 @@ impl DeviceTask { } async fn stop_all_queues(&mut self) { + // Reset/disable path: call `abort_queue`, which a device with in-flight + // host IO overrides to discard those completions rather than publish + // them (as `stop_queue` does). The queue is being torn down, so + // publishing a completion into its guest memory can corrupt a rebooted + // guest that has reused those pages. Suspend uses `stop()` instead, + // which drains and completes so the resumed guest sees its completions. for idx in 0..self.max_queues { - self.device.stop_queue(idx).await; + self.device.abort_queue(idx).await; } } } diff --git a/vm/devices/virtio/virtio_blk/src/lib.rs b/vm/devices/virtio/virtio_blk/src/lib.rs index 3adf2f67f7..8cc91f6d68 100644 --- a/vm/devices/virtio/virtio_blk/src/lib.rs +++ b/vm/devices/virtio/virtio_blk/src/lib.rs @@ -160,6 +160,27 @@ impl BlkWorker { } } } + + /// Await all in-flight IOs to completion, then drop their completions + /// without posting to the used ring. + /// + /// Used on the reset/disable teardown path. The IOs must be awaited (not + /// just dropped): dropping a pending disk read does not cancel the + /// underlying io_uring op, so it would complete asynchronously later and + /// write its disk data into a page a rebooted guest has already reused. + /// Awaiting lets that write land while the guest is stopped (memory still + /// valid). Each awaited IO still writes its data and status byte into the + /// guest's descriptor buffers; only its completion is dropped, so no + /// used-ring entry is posted into the torn-down queue's memory. + fn poll_drain_discard(&mut self, cx: &mut Context<'_>) -> Poll<()> { + loop { + match self.ios.poll_next_unpin(cx) { + Poll::Ready(Some(_completion)) => {} + Poll::Ready(None) => return Poll::Ready(()), + Poll::Pending => return Poll::Pending, + } + } + } } impl AsyncRun for BlkWorker { @@ -408,6 +429,22 @@ impl VirtioDevice for VirtioBlkDevice { Some(state) } + async fn abort_queue(&mut self, idx: u16) { + assert_eq!(idx, 0); + if !self.worker.has_state() { + return; + } + // Reset/disable teardown: await the in-flight IOs to completion, then + // discard them without posting to the used ring (unlike stop_queue). The + // reads must be awaited, not dropped: dropping a pending disk read does + // not cancel the io_uring op, which would then write into a page the + // rebooted guest has reused. See poll_drain_discard. + self.worker.stop().await; + let (worker, _state) = self.worker.get_mut(); + poll_fn(|cx| worker.poll_drain_discard(cx)).await; + self.worker.remove(); + } + fn supports_save_restore(&self) -> bool { true }