Repository navigation
Fix cross-process UEFI snapshot restore + Windows save fsync, add regression test - #4147
ayusharora221204 wants to merge 9 commits into
Conversation
…egression test Bug 1 (Windows save): handle_save_snapshot fsynced the memory backing file via a read-only handle. On Windows sync_all maps to FlushFileBuffers, which requires write access, so save failed with "Access is denied" (os error 5). Open the file writable before sync_all. Bug 2 (UEFI restore): restore forces load_mode = None, so UEFI platform resolvers were never registered and restoring a UEFI snapshot onto a fresh worker panicked with "no resolver for uefi_logger:platform". Detect a restored UEFI VM from the saved state's "uefi" state unit and register the resolvers in that case too. Add petri integration test uefi_cross_process_restore (gated unstable) that boots a worker, saves, tears it down, and restores onto a fresh worker -- the only path that reproduces Bug 2. Supporting plumbing: Worker::launch takes optional saved_state; run_restore forces load_mode = None and skips the in-process pulse; save_state_protobuf; Clone on the PetriVmConfig recipe (incl. manual Clone for BootImageConfig<T>). Add mock unit tests for Bug 1.
There was a problem hiding this comment.
Pull request overview
Fixes snapshot save/restore correctness across two critical scenarios: Windows snapshot save (fsync) and cross-process UEFI snapshot restore into a fresh worker (mirroring openvmm --restore-snapshot). It also adds a Petri integration regression test to ensure the UEFI resolver registration bug can’t regress.
Changes:
- Fix Windows snapshot save by opening the memory backing file with write access before
sync_all. - Fix cross-process UEFI snapshot restore by registering UEFI platform resolvers when restored state indicates a UEFI VM (even when
load_mode = None). - Add Petri cross-process restore plumbing + a new unstable integration test that reproduces the previously-missed restore path.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vmm_tests/vmm_tests/tests/tests/multiarch/save_restore.rs | Adds an unstable Petri integration test for cross-process UEFI snapshot restore. |
| vmm_tests/vmm_tests/tests/tests/multiarch.rs | Wires the new save_restore test module into the multiarch suite. |
| petri/src/worker.rs | Extends Worker::launch to accept an optional saved-state payload. |
| petri/src/vm/openvmm/start.rs | Adds a “restore on fresh worker” startup path and skips in-process save/restore pulse during restore. |
| petri/src/vm/openvmm/runtime.rs | Exposes a raw ProtobufMessage save-state API to avoid encode/decode roundtrips. |
| petri/src/vm/openvmm/mod.rs | Implements PetriVmBuilder::verify_openvmm_cross_process_restore helper used by the new test. |
| petri/src/vm/mod.rs | Enables cloning of VM recipe/config types needed to launch two identical workers. |
| openvmm/openvmm_helpers/src/snapshot.rs | Adds unit tests documenting the Windows sync_all read-only-handle failure and the fixed behavior. |
| openvmm/openvmm_entry/src/vm_controller.rs | Fixes the Windows fsync bug by opening the memory backing file writable before sync_all. |
| openvmm/openvmm_core/src/worker/dispatch.rs | Detects restored UEFI state from saved state and registers UEFI platform resolvers accordingly. |
| // Fresh UEFI boot, or a restored UEFI snapshot: both need the UEFI | ||
| // platform resolvers, else restore hits "no resolver for | ||
| // uefi_logger:platform". | ||
| _ if matches!(cfg.load_mode, LoadMode::Uefi { .. }) || restoring_uefi_state => { |
| use crate::PetriLogFile; | ||
| use crate::PetriVmConfig; | ||
| use crate::PetriVmResources; | ||
| use crate::PetriVmRuntime; | ||
| use crate::PetriVmRuntimeConfig; |
|
|
||
| #[cfg_attr(not(guest_arch = "x86_64"), expect(unused_mut))] | ||
| let mut deps_hyperv_firmware_pcat = None; | ||
| // Restore forces `load_mode = None`, so detect a restored UEFI VM from |
There was a problem hiding this comment.
Maybe we want to have a new LoadMode::Restore or something along those lines...
Replace LoadMode::None with an explicit restore wrapper around the complete Linux, UEFI, PCAT, or IGVM boot recipe. Build restored VMs from the normal platform configuration, delegate later operations through that recipe, and extend restore lifecycle tests and documentation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 00e9e201-7d6b-4903-a2ae-52f53a1a4994
Keep boot-recipe retention and UEFI/Linux-direct restore coverage in this change. Move the UEFI NVRAM mutation fix to a follow-up and tighten the restore documentation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
openvmm/openvmm_core/src/worker/dispatch.rs:333
- The validation enforces ‘restore mode requires saved state’, but not the converse (‘saved state requires restore mode’). As-is, a caller could provide
saved_state: Some(...)whileload_modeis notRestore, producing ambiguous semantics (part restore, part cold boot) that are hard to reason about and debug. Consider adding a symmetric check to rejectsaved_state.is_some()when!cfg.load_mode.is_restore()(or explicitly define and document what that combination should mean).
let saved_state = parameters
.saved_state
.map(|m| m.parse())
.transpose()
.context("failed to decode saved state")?;
if parameters.cfg.load_mode.has_nested_restore() {
anyhow::bail!("restore load mode cannot contain another restore load mode");
}
if parameters.cfg.load_mode.is_restore() && saved_state.is_none() {
anyhow::bail!("restore load mode requires saved state");
}
openvmm/openvmm_helpers/src/snapshot.rs:209
- The comment claims this is a ‘page-sized file’, but the test hard-codes 4096 bytes, which is not universally page-sized on all platforms/configurations. Since the test only needs a non-empty regular file, consider rewording the comment (e.g., ‘a small file’) or computing the host page size if the distinction matters.
// A page-sized file, matching the real memory-backing-file shape.
std::fs::write(&mem_path, vec![0u8; 4096]).unwrap();
|
|
||
| if vtl2_only { | ||
| assert!(matches!(self.load_mode, LoadMode::Igvm { .. })); | ||
| assert!(self.load_mode.is_igvm_platform()); |
| page_imports: initial_page_imports, | ||
| } = match &self.load_mode { | ||
| LoadMode::None => return Ok(()), | ||
| } = match self.load_mode.boot_recipe() { |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (4)
openvmm/openvmm_core/src/worker/dispatch.rs:333
- The validation currently rejects
load_mode=Restorewithoutsaved_state, but it allows the inverse (havingsaved_statepresent whileload_modeis notRestore). That mismatch can lead to confusing/unsafe behavior (e.g., applying saved state while also treating the VM as a normal boot-recipe load). Consider adding a symmetric check (or automatically wrapping) sosaved_state.is_some()impliescfg.load_mode.is_restore().
let saved_state = parameters
.saved_state
.map(|m| m.parse())
.transpose()
.context("failed to decode saved state")?;
if parameters.cfg.load_mode.has_nested_restore() {
anyhow::bail!("restore load mode cannot contain another restore load mode");
}
if parameters.cfg.load_mode.is_restore() && saved_state.is_none() {
anyhow::bail!("restore load mode requires saved state");
}
petri/src/vm/openvmm/start.rs:48
- The
restoringflag is derived only from the presence ofsaved_state, but restore-ness is also representable viaconfig.load_mode.is_restore(). Derivingrestoringfrom both (and/or explicitly rejecting mismatches) would make this API harder to misuse and ensures the later pulse-skip logic is consistent with the worker-side restore validation.
let restoring = saved_state.is_some();
openvmm/openvmm_helpers/src/snapshot.rs:240
- This test hard-codes the Windows error code
5(ERROR_ACCESS_DENIED). To avoid a magic number and improve readability/robustness, consider asserting onstd::io::ErrorKind::PermissionDeniedand/or using a named constant forERROR_ACCESS_DENIED(if available in existing dependencies).
assert_eq!(
err.raw_os_error(),
Some(5),
"expected ERROR_ACCESS_DENIED (5), got: {err}"
);
openvmm/openvmm_entry/src/lib.rs:2038
- The restore-snapshot handling now only wraps
cfg.load_modeand no longer forceswith_hv = true(which previously happened in the restore-snapshot branch). If dropping the implicit Hyper-V enablement is intentional, it would be good to call that out in the PR description as a behavior change; if it’s required for compatibility with existing snapshots, consider restoring the previous behavior or validating/derivingwith_hvfrom the saved manifest/config.
storage.build_config(&mut cfg, &mut resources, opt.scsi_sub_channels)?;
if opt.restore_snapshot.is_some() {
cfg.load_mode = cfg.load_mode.into_restore();
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated no new comments.
Suppressed comments (1)
openvmm/openvmm_core/src/worker/dispatch.rs:333
VmWorker::newvalidates thatLoadMode::Restorerequiressaved_state, but it doesn’t enforce the inverse (a non-restoreload_modewithsaved_state). That mismatch can lead to restore-only behavior being skipped in the future if other code keys offload_mode.is_restore(). Consider validating thatsaved_state.is_some()andcfg.load_mode.is_restore()are provided together.
if parameters.cfg.load_mode.has_nested_restore() {
anyhow::bail!("restore load mode cannot contain another restore load mode");
}
if parameters.cfg.load_mode.is_restore() && saved_state.is_none() {
anyhow::bail!("restore load mode requires saved state");
| _type: core::marker::PhantomData<T>, | ||
| } | ||
|
|
||
| impl<T: boot_image_type::BootImageType> Clone for BootImageConfig<T> { |
There was a problem hiding this comment.
Why can't this be a derive?
There was a problem hiding this comment.
I initially implemented this manually to avoid the T: Clone bound added by derive. Since T is sealed to the empty Vhd and Iso marker types, they can also derive Clone . I’ll update them and use derive here.
| /// bytes. The VM should be paused before calling this. | ||
| pub async fn save_state(&mut self) -> anyhow::Result<Vec<u8>> | ||
| ); | ||
| petri_vm_fn!( |
There was a problem hiding this comment.
Why is this needed when we already have save_state right above?
There was a problem hiding this comment.
I added this because the fresh-worker launch path accepts a ProtobufMessage , while the existing save_state() immediately serializes that message. Returning the message directly avoided an encode-then-decode round trip. However, this test is intended to model restoring a persisted snapshot, so crossing the serialization boundary is desirable. I only added it to avoid the encode decode round trip.I’ll reuse save_state() and decode its bytes before launching the restore worker.
| if supports_save_restore && !is_minimal { | ||
| // Run the basic save/restore pulse if supported. Skip on the restore | ||
| // path (the worker is already restoring a snapshot). | ||
| if supports_save_restore && !is_minimal && !restoring { |
There was a problem hiding this comment.
We should still pulse save/restore after a restore, just in case something in restoring again breaks.
There was a problem hiding this comment.
Agreed. Restored VMs should still verify that they can be saved and restored again.
| /// restore its snapshot on a fresh worker #2. This is the only path that | ||
| /// reproduces fresh-worker restore issues, which the in-process pulse | ||
| /// cannot reach because it restores into the same worker. | ||
| pub async fn verify_openvmm_cross_process_restore(self) -> anyhow::Result<()> { |
There was a problem hiding this comment.
This feels like it should live with the tests, not petri itself.
There was a problem hiding this comment.
That makes sense. I moved the three worker lifecycle and its assertions into tests . I initially placed it in Petri because the test receives a single PetriVmBuilder , and run(self) consumes it and provides no saved-state input. This test must launch multiple fresh workers using the same recipe and memory backing. Petri now exposes only the minimal reusable launch/restore configuration; all scenario-specific sequencing and assertions live in the test
…save-restore-snapshot # Conflicts: # openvmm/openvmm_core/src/worker/dispatch.rs # openvmm/openvmm_entry/src/lib.rs
| /// Mock regression test for the Windows save-snapshot fsync bug. | ||
| /// | ||
| /// `handle_save_snapshot` (in `openvmm_entry`) fsyncs the memory backing | ||
| /// file before capturing it. On Windows `sync_all` maps to | ||
| /// `FlushFileBuffers`, which requires a handle opened with **write** |
| // Fsync the memory backing file. Open it for write: on Windows | ||
| // `FlushFileBuffers` (which backs `sync_all`) requires a handle with | ||
| // write access, so a read-only handle fails with "Access is denied". | ||
| let memory_file = fs_err::OpenOptions::new() | ||
| .write(true) |
Summary
Replace
LoadMode::NonewithLoadMode::Restore { boot }so snapshot restoreretains the original Linux, UEFI, PCAT, or IGVM boot recipe.
Initial restore constructs the correct platform but applies saved state instead
of running the boot loader. Later operations such as machine reset use the
retained recipe.
This also preserves the Windows snapshot fix that opens the RAM backing file
with write access before flushing it.
Changes
mutations.
Testing
then reset and UEFI boot.
loading, then reset and kernel boot.