From e2a7c5e2ae5a7da096aff57272293b7f14086784 Mon Sep 17 00:00:00 2001 From: Hadi Orabi Date: Tue, 21 Jul 2026 17:51:50 +0000 Subject: [PATCH 1/3] matt feedback --- .../disk_nvme/nvme_driver/src/driver.rs | 10 +- .../tests/multiarch/openhcl_servicing.rs | 109 +++++++++++------- 2 files changed, 76 insertions(+), 43 deletions(-) diff --git a/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs b/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs index b489dd0040..4a8ad7418a 100644 --- a/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs +++ b/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs @@ -900,12 +900,20 @@ impl NvmeDriver { .admin .as_ref() .map(|a| { + let pending_commands_count = a.handler_data.pending_cmds.commands.len(); tracing::info!( id = a.qid, - pending_commands_count = a.handler_data.pending_cmds.commands.len(), + pending_commands_count, ?pci_id, "restoring admin queue", ); + if fused_keepalive_device && pending_commands_count > 0 { + panic!( + "fused keepalive device {pci_id} restored with a non-empty admin \ + queue ({pending_commands_count} pending commands); fused devices \ + must not issue admin commands after init" + ); + } // Restore memory block for admin queue pair. let mem_block = restored_memory .iter() diff --git a/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs b/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs index 01409ed729..481dd92b9a 100644 --- a/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs +++ b/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs @@ -1322,10 +1322,13 @@ async fn servicing_keepalive_slow_create_io_queue_with_inspect( /// Boots a frontpage OpenHCL VM with two VTL2 NVMe -> VTL0 SCSI relays: one /// controller forced into fused keepalive mode (VendorID=0x1414/DeviceID=0xb111) /// and one normal controller. Verifies the fused device eagerly pre-creates its -/// IO queues at init while the normal device creates them lazily. -#[openvmm_test(openhcl_linux_direct_x64)] -async fn nvme_fused_keepalive_enablement( +/// IO queues at init while the normal device creates them lazily, then services +/// the VM with NVMe keepalive. Re-verifies that the fused device is still fused +/// and the normal device is still non-fused after the service boundary. +#[openvmm_test(openhcl_linux_direct_x64 [LATEST_LINUX_DIRECT_TEST_X64])] +async fn nvme_fused_keepalive_servicing( config: PetriVmBuilder, + (igvm_file,): (ResolvedArtifact,), ) -> Result<(), anyhow::Error> { const VP_COUNT: u32 = 4; const MSIX_COUNT: u16 = 10; @@ -1340,6 +1343,9 @@ async fn nvme_fused_keepalive_enablement( let eager_count = (VP_COUNT as usize).min(MAX_IO_QUEUES as usize); + let mut flags = config.default_servicing_flags(); + flags.enable_nvme_keepalive = true; + let run_vm = async || -> Result, anyhow::Error> { let mut fused_updater = CellUpdater::new(false); // Spoof the vendor and device ID for a device that requires fused keepalive @@ -1460,49 +1466,68 @@ async fn nvme_fused_keepalive_enablement( .collect() }; - let vm = run_vm().await?; - let devices = inspect_devices(&vm).await?; + // Asserts the per-device fused/non-fused and IO-queue invariants. `phase` is + // used only to make failures clear about whether they occurred before or + // after servicing. + let assert_devices = |devices: &[(bool, Vec)], phase: &str| { + assert_eq!(devices.len(), 2, "[{phase}] expected two VTL2 NVMe devices"); + + let (_, fused_io) = devices + .iter() + .find(|(fused, _)| *fused) + .unwrap_or_else(|| panic!("[{phase}] expected one device in fused keepalive mode")); + let (_, normal_io) = devices + .iter() + .find(|(fused, _)| !*fused) + .unwrap_or_else(|| panic!("[{phase}] expected one device in normal (non-fused) mode")); + + assert_eq!( + fused_io.len(), + eager_count, + "[{phase}] fused keepalive device should have min(max_io_queues, vp_count) = \ + {eager_count} IO queues, but found {}", + fused_io.len() + ); + assert!( + fused_io.iter().filter(|&&unmapped| unmapped).count() >= 1, + "[{phase}] fused keepalive device should have eagerly pre-created unmapped IO \ + queues, but all {} queues were mapped", + fused_io.len() + ); - assert_eq!(devices.len(), 2, "expected two VTL2 NVMe devices"); + assert!( + !normal_io.is_empty(), + "[{phase}] the normal NVMe driver should have at least one IO queue" + ); + assert_eq!( + normal_io.iter().filter(|&&unmapped| unmapped).count(), + 0, + "[{phase}] a normal (non-fused) device must never have unmapped IO queues" + ); + assert!( + normal_io.len() < eager_count, + "[{phase}] a normal device creates IO queues lazily, so it should have fewer than \ + the fused eager count ({eager_count}), but found {}", + normal_io.len() + ); + }; - let (_, fused_io) = devices - .iter() - .find(|(fused, _)| *fused) - .expect("expected one device in fused keepalive mode"); - let (_, normal_io) = devices - .iter() - .find(|(fused, _)| !*fused) - .expect("expected one device in normal (non-fused) mode"); + let mut vm = run_vm().await?; - assert_eq!( - fused_io.len(), - eager_count, - "fused keepalive device should eagerly pre-create min(max_io_queues, vp_count) = \ - {eager_count} IO queues at init, but found {}", - fused_io.len() - ); - assert!( - fused_io.iter().filter(|&&unmapped| unmapped).count() >= 1, - "fused keepalive device should have eagerly pre-created unmapped IO queues, \ - but all {} queues were mapped", - fused_io.len() - ); + // Before servicing: the fused device eagerly pre-creates unmapped IO queues + // while the normal device creates them lazily. + let devices = inspect_devices(&vm).await?; + assert_devices(&devices, "pre-servicing"); + + // Exercise servicing with NVMe keepalive enabled. + vm.restart_openhcl(igvm_file, flags).await?; + + // After servicing: the fused flag is recomputed on the restore path, so the + // fused device must still be fused and the normal device must still be + // non-fused, with IO-queue state consistent with each mode. + let devices = inspect_devices(&vm).await?; + assert_devices(&devices, "post-servicing"); - assert!( - !normal_io.is_empty(), - "the normal NVMe driver should have come up with at least one IO queue" - ); - assert_eq!( - normal_io.iter().filter(|&&unmapped| unmapped).count(), - 0, - "a normal (non-fused) device must never eagerly pre-create unmapped IO queues" - ); - assert!( - normal_io.len() < eager_count, - "a normal device creates IO queues lazily, so it should have fewer than the \ - fused eager count ({eager_count}), but found {}", - normal_io.len() - ); Ok(()) } From 09856ecc8d31612a381c4d784acf602c6e7f0da6 Mon Sep 17 00:00:00 2001 From: Hadi Orabi Date: Tue, 21 Jul 2026 18:51:08 +0000 Subject: [PATCH 2/3] perform servicing in test --- .../tests/multiarch/openhcl_servicing.rs | 82 ++++++++++--------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs b/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs index 481dd92b9a..e5a9bd87bd 100644 --- a/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs +++ b/vmm_tests/vmm_tests/tests/tests/multiarch/openhcl_servicing.rs @@ -1466,12 +1466,12 @@ async fn nvme_fused_keepalive_servicing( .collect() }; - // Asserts the per-device fused/non-fused and IO-queue invariants. `phase` is + // Asserts exactly one fused and one non-fused device are present, returning + // their IO-queue `unmapped` vectors as `(fused_io, normal_io)`. `phase` is // used only to make failures clear about whether they occurred before or // after servicing. - let assert_devices = |devices: &[(bool, Vec)], phase: &str| { + let split_fused = |devices: &[(bool, Vec)], phase: &str| -> (Vec, Vec) { assert_eq!(devices.len(), 2, "[{phase}] expected two VTL2 NVMe devices"); - let (_, fused_io) = devices .iter() .find(|(fused, _)| *fused) @@ -1480,53 +1480,55 @@ async fn nvme_fused_keepalive_servicing( .iter() .find(|(fused, _)| !*fused) .unwrap_or_else(|| panic!("[{phase}] expected one device in normal (non-fused) mode")); - - assert_eq!( - fused_io.len(), - eager_count, - "[{phase}] fused keepalive device should have min(max_io_queues, vp_count) = \ - {eager_count} IO queues, but found {}", - fused_io.len() - ); - assert!( - fused_io.iter().filter(|&&unmapped| unmapped).count() >= 1, - "[{phase}] fused keepalive device should have eagerly pre-created unmapped IO \ - queues, but all {} queues were mapped", - fused_io.len() - ); - - assert!( - !normal_io.is_empty(), - "[{phase}] the normal NVMe driver should have at least one IO queue" - ); - assert_eq!( - normal_io.iter().filter(|&&unmapped| unmapped).count(), - 0, - "[{phase}] a normal (non-fused) device must never have unmapped IO queues" - ); - assert!( - normal_io.len() < eager_count, - "[{phase}] a normal device creates IO queues lazily, so it should have fewer than \ - the fused eager count ({eager_count}), but found {}", - normal_io.len() - ); + (fused_io.clone(), normal_io.clone()) }; let mut vm = run_vm().await?; - // Before servicing: the fused device eagerly pre-creates unmapped IO queues - // while the normal device creates them lazily. - let devices = inspect_devices(&vm).await?; - assert_devices(&devices, "pre-servicing"); + // Before servicing: the fused device eagerly pre-creates its full set of + // unmapped IO queues while the normal device creates them lazily. + let (fused_io, normal_io) = split_fused(&inspect_devices(&vm).await?, "pre-servicing"); + assert_eq!( + fused_io.len(), + eager_count, + "[pre-servicing] fused keepalive device should have min(max_io_queues, vp_count) = \ + {eager_count} IO queues, but found {}", + fused_io.len() + ); + assert!( + fused_io.iter().filter(|&&unmapped| unmapped).count() >= 1, + "[pre-servicing] fused keepalive device should have eagerly pre-created unmapped IO \ + queues, but all {} queues were mapped", + fused_io.len() + ); + assert!( + !normal_io.is_empty(), + "[pre-servicing] the normal NVMe driver should have at least one IO queue" + ); + assert_eq!( + normal_io.iter().filter(|&&unmapped| unmapped).count(), + 0, + "[pre-servicing] a normal (non-fused) device must never have unmapped IO queues" + ); + assert!( + normal_io.len() < eager_count, + "[pre-servicing] a normal device creates IO queues lazily, so it should have fewer than \ + the fused eager count ({eager_count}), but found {}", + normal_io.len() + ); // Exercise servicing with NVMe keepalive enabled. vm.restart_openhcl(igvm_file, flags).await?; // After servicing: the fused flag is recomputed on the restore path, so the // fused device must still be fused and the normal device must still be - // non-fused, with IO-queue state consistent with each mode. - let devices = inspect_devices(&vm).await?; - assert_devices(&devices, "post-servicing"); + // non-fused. The `split_fused` call fails if either determination changed. + let (_fused_io, normal_io) = split_fused(&inspect_devices(&vm).await?, "post-servicing"); + assert_eq!( + normal_io.iter().filter(|&&unmapped| unmapped).count(), + 0, + "[post-servicing] a normal (non-fused) device must never have unmapped IO queues" + ); Ok(()) } From 0de33bda762e90350725202acc68c5512ffae13c Mon Sep 17 00:00:00 2001 From: Hadi Orabi Date: Wed, 22 Jul 2026 05:48:13 +0000 Subject: [PATCH 3/3] error propogation in restore path Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: ad2f5a78-7441-43d3-9679-9de10f68a9c3 --- vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs b/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs index 4a8ad7418a..2c20e4e4f0 100644 --- a/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs +++ b/vm/devices/storage/disk_nvme/nvme_driver/src/driver.rs @@ -1117,7 +1117,7 @@ impl NvmeDriver { q.queue_data.qid == 1 || !q.queue_data.handler_data.pending_cmds.commands.is_empty() }) - .flat_map(|q| -> Result, anyhow::Error> { + .map(|q| -> Result, anyhow::Error> { let qid = q.queue_data.qid; let cpu = q.cpu; tracing::info!(qid, cpu, ?pci_id, "restoring queue"); @@ -1172,7 +1172,7 @@ impl NvmeDriver { } Ok(q) }) - .collect(); + .collect::>>()?; // Create prototype entries for any queues that don't currently have // outstanding commands. They will be restored on demand later. @@ -1228,7 +1228,7 @@ impl NvmeDriver { worker.io = sorted_io .into_iter() - .flat_map(|q| -> Result, anyhow::Error> { + .map(|q| -> Result, anyhow::Error> { let qid = q.queue_data.qid; let cpu = q.cpu; tracing::info!(qid, cpu, iv = q.iv, ?pci_id, "restoring queue"); @@ -1288,7 +1288,7 @@ impl NvmeDriver { } Ok(q) }) - .collect(); + .collect::>>()?; } // Update next_ioq_id to avoid reusing qids.