Repository navigation
Conversation
ludfjig
added
kind/enhancement
ludfjig
added
the
kind/enhancement
There was a problem hiding this comment.
🔵 Needs a closer look
The VMM memory boundary changes substantially, and OCI loading maps oversized blobs before enforcing resource limits.
1 open finding
What changed in this PR
Adds layered incremental snapshots that share immutable memory, persist OCI data and page-table layers, and optimize restore mappings.
Changes:
- Adds shared snapshot layers with incremental capture and installation.
- Introduces snapshot ABI 6 and OCI config v4.
- Optimizes page-table walks, KVM invalidation, tests, and benchmarks.
| File | Description |
|---|---|
src/tests/rust_guests/simpleguest/src/main.rs |
Adds zero-init mapping test support. |
src/hyperlight_host/tests/snapshot_goldens/goldens_version.rs |
Bumps golden snapshots to v6. |
src/hyperlight_host/src/sandbox/uninitialized.rs |
Documents capture behavior and uses GPA spans. |
src/hyperlight_host/src/sandbox/trace/mem_profile.rs |
Reads layered memory by GPA. |
src/hyperlight_host/src/sandbox/snapshot/tripwires.rs |
Pins ABI 6 media types. |
src/hyperlight_host/src/sandbox/snapshot/memory/mod.rs |
Defines layered snapshot memory. |
src/hyperlight_host/src/sandbox/snapshot/memory/backing.rs |
Maps and accesses snapshot layers. |
src/hyperlight_host/src/sandbox/snapshot/file/mod.rs |
Saves and loads layered OCI snapshots. |
src/hyperlight_host/src/sandbox/snapshot/file/media_types.rs |
Adds config v4 and new layer media types. |
src/hyperlight_host/src/sandbox/snapshot/file/config.rs |
Defines the layered snapshot schema. |
src/hyperlight_host/src/sandbox/snapshot/file_tests.rs |
Tests layered persistence and validation. |
src/hyperlight_host/src/sandbox/mod.rs |
Exports SnapshotMemoryReader. |
src/hyperlight_host/src/sandbox/initialized.rs |
Installs captures and updates snapshot APIs. |
src/hyperlight_host/src/mem/virtq/tests.rs |
Updates scratch transport tests. |
src/hyperlight_host/src/mem/shared_mem.rs |
Adds immutable memory and range mappings. |
src/hyperlight_host/src/mem/layout.rs |
Removes flat snapshot layout state. |
src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs |
Disables the slot-wide KVM flush quirk. |
src/hyperlight_host/src/hypervisor/virtual_machine/hvf/mod.rs |
Uses range-based snapshot mappings. |
src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs |
Adopts layered VM memory. |
src/hyperlight_host/src/hypervisor/hyperlight_vm/test_support.rs |
Updates mapping-state fault tests. |
src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs |
Manages multiple snapshot mappings. |
src/hyperlight_host/src/hypervisor/hyperlight_vm/aarch64.rs |
Adopts layered VM memory. |
src/hyperlight_host/src/hypervisor/gdb/mod.rs |
Resolves layered debug memory. |
src/hyperlight_host/build.rs |
Removes obsolete memory-read configuration. |
src/hyperlight_host/benches/benchmarks.rs |
Adds incremental snapshot benchmarks. |
src/hyperlight_common/src/vmem.rs |
Coalesces adjacent mappings. |
src/hyperlight_common/src/arch/amd64/vmem.rs |
Applies mapping coalescing on AMD64. |
src/hyperlight_common/src/arch/aarch64/vmem.rs |
Applies mapping coalescing on AArch64. |
proposals/0003-incremental-snapshots/README.md |
Updates the snapshot design. |
docs/snapshot-versioning.md |
Documents ABI 6 versioning. |
docs/snapshot-oci-format.md |
Documents layered OCI storage. |
docs/paging-development-notes.md |
Describes incremental capture behavior. |
CHANGELOG.md |
Records breaking API and format changes. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This comment has been minimized.
This comment has been minimized.
67c5cc8 to
7ca7b5e
Compare
This comment has been minimized.
This comment has been minimized.
7ca7b5e to
67561dc
Compare
This comment has been minimized.
This comment has been minimized.
| .checked_add(chunk_len) | ||
| .ok_or_else(|| new_error!("crash-dump GVA range overflows"))?; | ||
|
|
||
| if !try_coalesce_region(&mut regions, virt_base, virt_end, host_region.start, flags) |
There was a problem hiding this comment.
I think this could also use the same coalescing pattern:
resolved_regions
.coalesce(CrashDumpRegion::try_merge)
.collect()There was a problem hiding this comment.
since skiping the above coalescing, skipping this too
There was a problem hiding this comment.
I think because virt_to_phys now does the coalescing, maybe there is no need for this to coalesce at all?
There was a problem hiding this comment.
virt_to_phys doesn't coalesce, only walk_va_spaces does. And the crash dump resolves the backing per page anyway, since adjacent GPAs can be in different layers, scratch or a dynamic region. So without this we'd get one region per page in the core dump.
67561dc to
fd719b9
Compare
This comment has been minimized.
This comment has been minimized.
fd719b9 to
c739623
Compare
This comment has been minimized.
This comment has been minimized.
simongdavies
left a comment
simongdavies
left a comment
There was a problem hiding this comment.
I don't believe this breaking change should be absolute - Sandbox::snapshot removes map_region ... Pages of these regions the guest has not mapped are lost.
We should make this optional so that a snapshot may optionally capture the file(s)
| ) | ||
| .read(gpa, data) | ||
| .map(|_| ()) | ||
| .map_err(|e| DebugMemoryAccessError::CopyFailed(Box::new(e))) |
There was a problem hiding this comment.
The reader now classifies every failure as CopyFailed, including an address with no host backing. Previously, that case returned TranslateGuestAddress, which the debugger treats as a recoverable memory-request error. CopyFailed escapes the handler and fails the current guest execution.
For example, a guest PTE can still reference a dynamically mapped region after the host unmaps it. Inspecting that address at a breakpoint should reject the read and leave debugging usable, but this path aborts execution.
c739623 to
fd20c46
Compare
To be more precise about the breaking change, snapshots capture the same pages as before. main only captures the
Capturing whole files isn't supported on main, so I think it makes sense as a follow-up to see if we really want this |
@simongdavies: to clarify, this is already the behaviour on snapshot. What actually changes in this PR is that taking a snapshot and then calling a guest function now works identically to taking a snapshot, restoring the same snapshot, and then calling a guest function---which it did not in the past. I think that is straightforwardly a good change from an API/semantics point of view, although it probably comes with a small perf hit (but is necessary to have the nice semantics of snapshot+snapshot produce a layered snapshot). I do not think there is any strong argument to be made to capture the pages which are not mapped to a VA, as this is not really the memory model of hyperlight. We always map to a VA right after doing the map file cow, and the general design philosophy of guest-CoW is that only VAs matter---PAs never do except for certain communication related concerns that are specially privileged and fundamental in a way that these pages are not. It is also hard/impossible to do in full generality, since the PA layout of the sandbox fundamentally changes on snapshot/restore, so things may overlap the old addresses. (In the longer term, I would like to get rid of the room for the slight break in the model that map_file_cow presently represents and remove it entirely, in favour of simply being a way to optimise VecBytes transfers in much the same way that bytechunks could be...) |
syntactically
left a comment
syntactically
left a comment
There was a problem hiding this comment.
This mostly looks sensible (some minor nits/questions below); my main concern is with the way that the memory access code uses raw pointers to read/write from a HostSharedMemory, which does not seem like it upholds the usual shared memory invariants.
| internal_walk_va_spaces(&op, roots.iter().cloned(), true, address, len) | ||
| .map(|(id, mappings)| (id, mappings.collect::<Vec<_>>())) | ||
| .map(|(id, mappings)| { | ||
| let mut collected = Vec::new(); |
There was a problem hiding this comment.
Do you have any idea how this compares on release optimisations to a fold / a "coalesce adjacent mappings" wrapper iterator? My immediate intuition is that this is less likely to get nicely inlined/optimised down to a tight loop, but I could be totally wrong.
There was a problem hiding this comment.
I tried a coalesce wrapper in #1926 (comment). The walk took about 2x as long, and snapshots/create/large was 34% slower (x86 KVM, release). It looks like coalesce drives the nested flat_map through try_fold, which compiles to a bunch of out-of-line closures, while the for loop compiles to one loop. Didn't try a plain fold.
| let captured = VirtqSnapshot::capture(&layout, &case.scratch).unwrap(); | ||
| let restored = host_scratch(); | ||
| let allocator = layout.get_first_free_scratch_gpa(); | ||
| let allocator = layout.get_pt_base_gpa(); |
There was a problem hiding this comment.
SandboxMemoryLayout::get_first_free_scratch_gpa doesn't exist anymore. It returned pt base + pt size, and pt size now lives on the snapshot, since each snapshot has its own page tables. This layout has no page tables, so the old call returned just the pt base, which is what get_pt_base_gpa() returns.
| } | ||
|
|
||
| /// Installs memory produced by a capture without restoring runtime state. | ||
| pub(crate) fn install_captured_snapshot( |
There was a problem hiding this comment.
Why do we need this, rather than use a normal restore?
There was a problem hiding this comment.
A restore does a lot more than we need. I'm not exactly sure what you mean
| .checked_add(chunk_len) | ||
| .ok_or_else(|| new_error!("crash-dump GVA range overflows"))?; | ||
|
|
||
| if !try_coalesce_region(&mut regions, virt_base, virt_end, host_region.start, flags) |
There was a problem hiding this comment.
I think because virt_to_phys now does the coalescing, maybe there is no need for this to coalesce at all?
| /// CPU vendor captured at snapshot time. Checked on load. | ||
| pub(super) cpu_vendor: CpuVendor, | ||
| /// Host page size at capture. Blob storage is padded to it. | ||
| pub(super) host_page_size: usize, |
There was a problem hiding this comment.
I think the padding should be to max(host page size, guest page size)? And maybe it will be useful to add a config knob. So maybe there is some better word here---maximum restorable page size or something?
There was a problem hiding this comment.
The padding already used max(host, guest) through host_page_size(), but the field stored the raw host page size. Renamed it to layer_alignment, and it now stores the real alignment, in adaa260. I didn't go with max restorable page size since load still requires an exact match. I think we can add a knob for a bigger alignment in a follow up if we want
| memory: Arc<SnapshotMemory>, | ||
| /// One writable copy for each layer in `memory`. | ||
| #[cfg(unshared_snapshot_mem)] | ||
| backings: Box<[S]>, |
There was a problem hiding this comment.
Maybe instead of keeping separate readable and writable copies, it is easier to do what was done before and push the S: SharedMemory type variable all the way down to where the ReadonlySharedMemory ultimately ends up in SnapshotBlob, with sufficient eager copying (when cfg(unshared_snapshot_mem)) that there is not excessive sharing?
There was a problem hiding this comment.
The Arc is the snapshot's own memory, not a copy. The only extra memory is the gdb writable copy, which we needed on main too. Making SnapshotBlob/SnapshotMemory generic would get rid of this type, but then capture, validation and restore all need to handle the gdb case. Since unshared snapshot mem is meant to be temporary, I'd rather keep that in this one file.
| // contract keeps them valid and unmodified for the sandbox's | ||
| // lifetime. The checks above bound this read. | ||
| unsafe { | ||
| std::ptr::copy( |
There was a problem hiding this comment.
Hm, I think this loses the lock that ensures there is no concurrent sandbox writer. Until/unless we switch to some other typestate/lifecycle-based approach to ensure that, I think this is not quite right. (I do agree that it is a bit complicated to avoid lots of lock acquires/releases---that is why previously with_exclusivity/with_contents were used to acquire a slice upfront that was then used with the other functions---maybe the same is possible here by changing self.scratch: &'a [8] and replacing the constructors with methods that run a closure with access to the memory reader and acquire that via scratch.with_contents?
There was a problem hiding this comment.
Good catch. I did what you suggested. GuestPhysicalMemoryView now holds scratch as a &[u8] taken under the write lock, and callers get the view through a closure (with_guest_memory). I added a &self version of with_contents on HostSharedMemory so the gdb path doesn't need &mut. Fixed in 3710955
| Box<dyn Fn(&SnapshotMemoryReader<'_>, &[u8], u64) -> Result<Vec<u64>> + Send>; | ||
|
|
||
| fn normalize_page_table_roots(roots: Vec<u64>, active_root: u64) -> Vec<u64> { | ||
| let mut seen = BTreeSet::from([active_root]); |
There was a problem hiding this comment.
Is this equivalent to
BTreeSet::from([active_root]).extend(roots.iter()).into_iter().collect::<Vec<_>>()?
There was a problem hiding this comment.
No extend can put things first, consider
let mut set = BTreeSet::from([0x5000]); // active root
set.extend([0x3000]); // other root
set.into_iter().collect::<Vec<_>>() // [0x3000, 0x5000]| .resolve(gpa, data.len()) | ||
| .ok_or(DebugMemoryAccessError::TranslateGuestAddress(gpa))?; | ||
|
|
||
| // We can only safely write (without causing UB in the host |
There was a problem hiding this comment.
I think this comment is not correct
call_and_create times a guest call plus snapshot creation, so the call pays for memory remapping done by the previous snapshot. restore_alternate restores two snapshots in turn, so each restore targets memory that is not current. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Share immutable data blobs between related snapshots. Each blob holds guest data followed by the page tables of the snapshot that created it. Persist layered snapshots as OCI image layouts. Capture copies only pages that changed since the snapshot the sandbox was created from or last restored to. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Snapshot capture switches the running sandbox to the captured snapshot. The next capture reuses its layers and copies only pages changed after it. Scratch pages used before the capture become free again. They are zeroed so guest allocations return zeroed pages, as after restore. Capture removes map_region and map_file_cow regions. Pages of these regions that the guest has not mapped are lost. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
walk_va_spaces merges mappings that have contiguous physical and virtual addresses and the same kind. Snapshot capture classifies and rebuilds each merged mapping once. Criterion snapshots/create: * MSHV: medium 1.18 ms to 950 us (-20%), large 3.56 ms to 2.85 ms (-20%) * KVM: medium 1.34 ms to 1.04 ms (-22%), large 3.59 ms to 2.91 ms (-19%) Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
A page-table walk reads every entry through GuestPhysicalMemoryView, which resolves the backing and copies 8 bytes through a locked volatile copy. The reader keeps a copy of the last table page it read, so each table costs one copy. Snapshot capture and crash dumps use the reader. Criterion snapshots/create: * MSHV: default 344 us to 248 us (-28%), medium 950 us to 594 us (-37%) * KVM: default 499 us to 379 us (-24%), medium 1.04 ms to 746 us (-28%) Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
By default, deleting a memslot flushes the guest mappings of every memslot. KVM keeps that flush as a workaround for VMs with assigned GPUs. Hyperlight assigns no devices and disables the quirk when the kernel supports it (Linux 6.12 and later). Deleting a memslot then flushes only that memslot's mappings. Snapshot capture deletes a memslot when a layer drops out of the snapshot. Restore to another snapshot and region unmapping also delete memslots. Criterion on KVM (Linux 6.17): * snapshots/call_and_create: 24% to 58% faster * snapshots/restore_alternate: 32% to 52% faster Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
* Iterate snapshot mappings in reverse and test membership with contains. * Rename GuestVirtualMemoryReader::read and the public SnapshotMemoryReader::read to read_exact. * Move the snapshot memory test helpers into the tests module. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
* Hold the scratch lock for a whole guest memory access. GuestPhysicalMemoryView borrows scratch as a slice under the write lock, and callers get the view through a closure. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
* Rename the snapshot config field host_page_size to layer_alignment and store the alignment the layers use: the larger of the host and guest page sizes. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
* Correct the gdb write comment. Snapshot layers are writable copies in gdb builds, and only dynamic regions are refused. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
fd20c46 to
cfaa18c
Compare
the only raw pointer read left is for map_region/map_file_cow memory, which isn't a HostSharedMemory, and the host has to keep it unmodified anyway. Main read it with from_raw_parts too. Scratch is now locked for the whole access, see this. |
Benchmark ResultsMeasured commit: kvm / amd (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
kvm / intel (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
mshv3 / amd (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
mshv3 / intel (Linux) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
hyperv-ws2025 / amd (Windows) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
hyperv-ws2025 / intel (Windows) (➖ stable)No benchmark improved or regressed. Benchmark Resultsfunction_call_codec
payload_allocation
sandboxes
slot_pool
snapshot_files
virtq_readonly
virtq_readwrite
Reported by |
Summary
Snapshots share immutable memory layers. A capture copies only the pages changed since its parent snapshot, and
Sandbox::snapshotswitches the sandbox to the capture so the next one is incremental too. This makessnapshot()much faster for larger sandboxes. It also improves memory density: each snapshot holds only its changed pages and shares the rest with its parent, wheremainholds a full copy of guest memory per snapshot. See HIP 0003.Deviations from the merged HIP 0003 draft (the HIP is updated):
main. A restore copies the newest page tables into scratch.Sandbox::snapshotinstalls the capture, because otherwise every later capture copies all pages changed since the last restore again. Installing removesmap_regionandmap_file_cowregions, because the snapshot holds their mapped pages and later blobs may reuse their addresses.Changes
Sandbox::snapshotinstalls the capture into the running sandbox. Scratch pages it frees are zeroed.KVM_X86_QUIRK_SLOT_ZAP_ALLis disabled where supported, so deleting a memory slot flushes only that slot.snapshots/call_and_createandsnapshots/restore_alternatecover the cost moved into the next call and restores to a non-current snapshot.Breaking changes
Sandbox::snapshotremovesmap_regionandmap_file_cowregions from the sandbox, which onlyrestoredid before. Region pages the guest has not mapped become unreachable at the snapshot instead of at the next restore. Snapshots hold the same pages as before.PtRootFinderreceives aSnapshotMemoryReaderand returnsResult<Vec<u64>>.Commits
call_and_createandrestore_alternatesnapshot benchmarks. Lands first so the benchmarks run before and after.walk_va_spaces.PageTableReader.KVM_X86_QUIRK_SLOT_ZAP_ALL.Benchmarks
Compared to
main.restore,restore_alternateandcall_and_createcompare to the first commit, which adds the last two.Highlights
snapshots/create/defaultsnapshots/create/largesnapshots/call_and_create/mediumsnapshots/restore_alternate/largesnapshots/restore/largeGuest calls show no consistent change.
Regressions
snapshot_files/load_snapshot_unverified/defaultsnapshot_files/load_snapshot/defaultsnapshot_files/save_snapshot/defaultsnapshot_files/cold_start_via_snapshot/defaultload_snapshotsandboxes/sandbox_from_snapshot/smalldefault(+11%)Kernels before 6.12 cannot disable
KVM_X86_QUIRK_SLOT_ZAP_ALL, so each memory slot change flushes all guest mappings. WHP and aarch64 were not benchmarked.A saved snapshot stores each layer in full. Base layer pages that a newer layer replaced stay in the file, which adds 88 KiB for
default.Closes #1956
Closes #1551
Full results
All benchmarks on both hosts (click to expand)
Before → after (change). Negative is faster. Bold is a regression. ~ is no consistent change. A change needs agreement between a 0.5 s and a 3 s run, or a 3 s run alone.
snapshots
call_and_createcall_and_createcall_and_createcall_and_createcreatecreatecreatecreaterestorerestorerestorerestorerestore_alternaterestore_alternaterestore_alternaterestore_alternateguest_calls
callcallcallcallcall_with_host_functioncall_with_host_functioncall_with_host_functioncall_with_host_functioncall_with_restorecall_with_restorecall_with_restorecall_with_restoredifferent_threadinterrupt_latencyguest_functions_with_large_parameters
guest_call_with_large_parameterssample_workloads
24K_in_8K_out_c24K_in_8K_out_rustsandboxes
create_initializedcreate_initializedcreate_initializedcreate_initializedcreate_initialized_and_dropcreate_initialized_and_dropcreate_initialized_and_dropcreate_initialized_and_dropsandbox_from_snapshotsandbox_from_snapshotsandbox_from_snapshotsandbox_from_snapshotsnapshot_files
cold_start_via_evolvecold_start_via_evolvecold_start_via_evolvecold_start_via_evolvecold_start_via_snapshotcold_start_via_snapshotcold_start_via_snapshotcold_start_via_snapshotcold_start_via_snapshot_unverifiedcold_start_via_snapshot_unverifiedcold_start_via_snapshot_unverifiedcold_start_via_snapshot_unverifiedload_snapshotload_snapshotload_snapshotload_snapshotload_snapshot_unverifiedload_snapshot_unverifiedload_snapshot_unverifiedload_snapshot_unverifiedsave_snapshotsave_snapshotsave_snapshotsave_snapshot