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 }