Fix cross-process UEFI snapshot restore + Windows save fsync, add regression test - #4147
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.
|
ayusharora221204 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
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...
Summary
Fixes two bugs surfaced by cross-process snapshot restore (
openvmm --restore-snapshotinto a fresh worker process), and adds a petri integration test that gates the UEFI regression.Bug 1 — Windows save fsync
handle_save_snapshotfsynced the memory backing file via a read-only handle. On Windowssync_allmaps toFlushFileBuffers, which requires a handle opened with write access, so save failed withAccess is denied(os error 5). Fix: open the file writable beforesync_all. Adds mock unit tests documenting the bug.Bug 2 — UEFI restore resolver
Restore forces
load_mode = None, so the UEFI platform resolvers were never registered, and restoring a UEFI snapshot onto a fresh worker panicked withno resolver for uefi_logger:platform. Fix: detect a restored UEFI VM from the saved state's"uefi"state unit and register the resolvers in that case too.Test + plumbing
uefi_cross_process_restore(gatedunstable, Alpine UEFI x64): boots a worker, saves, tears it down, then restores the snapshot on a fresh worker — the only path that reproduces Bug 2 (the in-process pulse restores into the same worker and can't hit it).Worker::launchtakes an optionalsaved_state;run_restoreforcesload_mode = Noneand skips the in-process pulse;save_state_protobuf;Cloneon thePetriVmConfigrecipe (incl. a manualCloneforBootImageConfig<T>).Testing
PASS uefi_cross_process_restore.no resolver for uefi_logger:platform, confirming it gates the regression.rustfmt --checkclean; mock unit tests for Bug 1 pass.