feat: i686 page tables, snapshot compaction, and CoW (standalone)#1385
feat: i686 page tables, snapshot compaction, and CoW (standalone)#1385
Conversation
…eatures Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Co-authored-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Co-authored-by: danbugs <danilochiarlone@gmail.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Co-authored-by: danbugs <danilochiarlone@gmail.com>
- sandbox/snapshot.rs: decode i686 PTEs with `u32::from_le_bytes`
instead of `from_ne_bytes`. Page-table entries are defined as
little-endian by arch spec; `from_ne_bytes` is incorrect on
big-endian hosts and inconsistent with surrounding helpers that
already use `from_le_bytes`.
- hyperlight_common/vmem.rs: guard the `i686-guest` feature with a
`compile_error!` on targets that are neither `x86` (the guest
itself) nor `x86_64` (the host that runs the guest). Previously
enabling the feature on e.g. aarch64 would silently compile but
downstream crates would hit confusing missing-item errors when
they reached for `vmem::i686_guest`.
- mem/mgr.rs, sandbox/snapshot.rs: persist the per-process
PD-roots count in `Snapshot` and rewrite the scratch bookkeeping
area (`SCRATCH_TOP_PD_ROOTS_{COUNT,ARRAY}_OFFSET`) during
`restore_snapshot`. Scratch is zeroed on restore, so without
this a subsequent `snapshot()` call would read count=0 through
`read_pd_roots_from_scratch` and fail. Root `i`'s compacted
GPA is deterministic (`layout.get_pt_base_gpa() + i * PAGE_SIZE`
— same layout `compact_i686_snapshot` used when building the
rebuilt PDs), so we only need to store the count.
Signed-off-by: danbugs <danilochiarlone@gmail.com>
danbugs
requested review from
dblnz,
devigned,
jprendes,
ludfjig,
marosset,
simongdavies and
syntactically
as code owners
danbugs
added
the
kind/enhancement
There was a problem hiding this comment.
Reviewers: Opus, Gemini,ChatGPT
All three reviewers agree this is a well-structured PR that cleanly separates the nanvix-unstable feature into i686-guest and guest-counter, and replaces the TODO-laden real-mode boot with proper 32-bit protected mode + paging. The code quality is generally good — comments are thorough, unsafe blocks are contained, and the architecture is sensible.
However, all three independently flagged critical concerns around the CoW flag logic, potential scratch memory layout overlap, and the complete absence of tests for the i686 code path.
🟢 Good Stuff
- Unified
restore_all_state(): Removing the TODO-ladennanvix-unstablebranch with its "this is probably not correct" comment is a significant improvement. The CR3-based restore is now clean and shared. compile_error!guard for i686-guest on non-x86 — nice defensive programming.- OOB-safe
read_entryimplementations: BothSnapshotandSharedMemoryPageTableBufferreturn 0 (not-present) for OOB addresses rather than panicking. Exactly right for defensively walking guest-controlled page tables. - Clean feature flag split:
i686-guest+guest-counterare genuinely independent concerns. - Thorough doc comments on
Snapshotfields (n_pd_roots,separate_pt_bytes). - Scratch bookkeeping factoring: Extracting
copy_pt_to_scratch()fromupdate_scratch_bookkeeping()is a clean refactor.
🔴 Additional findings (on lines outside the diff)
Snapshot impl of TableReadOps always reads 8-byte entries (snapshot.rs line 131, Flagged by: 2/3)
The impl TableReadOps for Snapshot at line 131 of snapshot.rs unconditionally reads 8 bytes for a PTE. While it appears to only be used in the #[cfg(not(feature = "i686-guest"))] path currently, it's not gated by #[cfg]. If anyone ever calls virt_to_phys with a Snapshot on the i686 path, it will read 8-byte entries from a 4-byte PTE table — reading cross-entry data and producing garbage mappings. Either gate with #[cfg(not(feature = "i686-guest"))] or add a comment + compile_error! guard.
Endianness inconsistency (snapshot.rs line 149, Flagged by: 2/3)
Line 149 of snapshot.rs uses u64::from_ne_bytes() (native endian), while the i686 SharedMemoryPageTableBuffer::read_entry() at line 273 correctly uses u32::from_le_bytes() with a comment noting "Page-table entries are little-endian by arch spec". The x86_64 path happens to work on LE hosts but is technically incorrect by the same reasoning. Should use from_le_bytes consistently.
See inline comments for the remaining findings.
andreiltd
force-pushed
the
nanvix-platform
branch
2 times, most recently
from
cd065b1 to
105974d
Compare
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
Signed-off-by: Tomasz Andrzejak <andreiltd@gmail.com>
andreiltd
force-pushed
the
nanvix-platform
branch
from
327f85b to
8fcb7ea
Compare
There was a problem hiding this comment.
The high-level feature here (supporting x86-32 VA spaces instead of the pretending-to-be-real-mode #[cfg(feature = "nanvix-unstable")]) is great.
It seems like all of the i686 page table code is scattered around in various places and does not follow the same structure or architecture-independent API as the amd64 and aarch64 code. I don't think that's at all maintainable. Please try to use the same structures and APIs as the amd64 code, having i686 implementations in hyperlight_common/src/arch/i686/vmem.rs that implement the architecture-independent re-exports of hyperlight_common/src/vmem.rs and allowing most/all of the downstream code to use the same APIs and patterns. (It may be sensible to extract out the page table iterator code that is currently amd64 only, in which case I would expect it to not require much extra code to support i686 page tables (although, if you do this, please check that inlining the iterators still works, at least in release builds---e.g. vmem::map should inline down to basically the same thing as a handwritten loop in a single function)).
I would expect the changes to src/hyperlight_host/src/sandbox/snapshot.rs in particular to be limited to basically
- Make
Snapshot::newiterate through several different VA spaces worth of mappings, instead of just one (there ought to be no need to do anything special to avoid duplicated backing pages as long as the existingphys_seenmap is kept) - (If necessary) minor changes to support keeping the PTs generated for the new mappings in a separate vec in the snapshot, instead of at the end of the snapshot memory where they are now on all architectures. Ideally, rather than being #[cfg] items directly in the crate, this would be controlled by some constant e.g.
hyperlight_common::vmem::PA_SPACE_IS_SMALL: bool(with the naming reflecting the actual motivation for making this different across architectures).- It occurs to me that this may not even need any changes here---perhaps it could be achieved simply by changing the mem mgr or hypervisor manager code to avoid mapping the end of the snapshot region into the guest PA space on i686.
I have left a few other minor inline comments, but the above issue is the only thing that I think is actually quite important.
I didn't review any of the i686 address space manipulation code yet, because it seems like it will need to be refactored and generalised a little to be able to provide the same APIs that we use on other architectures. I think it is quite important that all the guest architectures implement the same APIs, so that most of the code can be architecture-independent.
| /// page tables in guest memory. The `arch/i686/vmem.rs` module only compiles | ||
| /// for `target_arch = "x86"` (the guest side), so the host-side walker lives | ||
| /// here, gated behind the feature flag. | ||
| #[cfg(feature = "i686-guest")] |
There was a problem hiding this comment.
I think it would make more sense to have the i686/vmem.rs module used when feature = i686-guest---we should understand that the arch in hyperlight_common is always about the guest arch.
| /// | ||
| /// # Safety | ||
| /// The caller must ensure that `op` provides valid page table memory. | ||
| pub unsafe fn virt_to_phys_all<Op: TableReadOps>(op: &Op) -> Vec<Mapping> { |
There was a problem hiding this comment.
Is there a reason this doesn't match the API of the amd64 stuff so that it can be re-exported from the architecture-independent module, like we do on other platforms?
| ExeInfo::Elf(elf) => Offset::from(elf.entrypoint_va()), | ||
| } | ||
| } | ||
| /// Returns the base virtual address of the loaded binary (lowest PT_LOAD p_vaddr). |
There was a problem hiding this comment.
This whole base va sliding is a hack because of some executables we used to have + the lack of virtual memory in the guest. I don't think that any of the usual guests end up computing anything other than 0 here, so I was thinking it was worth getting rid of entirely. Do the nanvix binaries actually end up with something here? If so, can we replace the unpleasant semantics-breaking relocation with just changing the initial page tables slightly to get things mapped to the VAs that they ask for?
(Not relevant for merging this PR---just curious about answers to these questions for the future).
| type T<S: SharedMemory>; | ||
| } | ||
| pub struct SnapshotSharedMemory_; | ||
| impl SnapshotSharedMemoryT for SnapshotSharedMemory_ { |
There was a problem hiding this comment.
I would have expected that with the description of this PR, we could get rid of this and use ReadonlySharedMemory unconditionally, since Nanvix will no longer depend on writing to the snapshot shared memory. Is that not true?
There was a problem hiding this comment.
I think it can be removed, but I'd prefer to defer it to antoher pr
| abort_buffer: Vec::new(), // Guest doesn't need abort buffer | ||
| }; | ||
| host_mgr.update_scratch_bookkeeping()?; | ||
| host_mgr.copy_pt_to_scratch()?; |
There was a problem hiding this comment.
Does this need to make the same differentiation between the i686 and x86-64 sources of the initial page table information?
| /// the i686-guest path so the scratch state mirrors what it | ||
| /// looked like right after the snapshot was taken. | ||
| #[cfg(feature = "i686-guest")] | ||
| fn update_pd_roots_bookkeeping(&mut self, n_roots: usize) -> Result<()> { |
There was a problem hiding this comment.
I am still not a huge fan of this extra table.
If the roots are only updated in this table, how does kernel code notice that a snapshot+restore cycle has happened and update its internal source-of-truth for them? (i.e. I assume there is another copy of these hanging out in various struct task-like structures that need to be updated). If the location of those internal-source-of-truth copies of things could be provided by a hypercall or a root finder callback, it would perhaps simplify things, as well as removing this statically-allocated limit to the number of distinct address spaces in the guest.
| .vm | ||
| .get_root_pt() | ||
| .map_err(|e| HyperlightError::HyperlightVmError(e.into()))?; | ||
| .map_err(|e| HyperlightError::HyperlightVmError(e.into()))?]; |
There was a problem hiding this comment.
If this were a hypercall or a callback into the embedder, it would also avoid this architecture-specific dependency.
| /// CoW resolution map: maps snapshot GPAs to their CoW'd scratch GPAs. | ||
| /// Built by walking the kernel PD to find pages that were CoW'd during boot. | ||
| #[cfg(feature = "i686-guest")] | ||
| cow_map: Option<&'a std::collections::HashMap<u64, u64>>, |
There was a problem hiding this comment.
Why is this information necessary beyond the scope of one address space traversal? And, why is this a useful form of this information?
There was a problem hiding this comment.
it's used in a couple of places and TableReadOps::read_entry takes a &self...
| u64::from_ne_bytes(n) | ||
| let memoff = access_gpa(self.snap, self.scratch, self.layout, addr); | ||
| // For i686 guests, page table entries are 4 bytes; for x86_64 they | ||
| // are 8 bytes. Read the correct size based on the feature flag. |
There was a problem hiding this comment.
This can just use mem::size_of::<hyperlight_common::vmem::PageTableEntry>(). Or, if it can't because of some limitation on the const-ness of that expression, it should just be another constant and/or type synonym defined in the same place.
There should ideally be little-to-no arch-specific code in this module: the hyperlight_common::vmem and related APIs should abstract over the architecture-of-the-guest differences well enough that this code can be more-or-less entirely generic. (If there are problems with the vmem api assuming to much, we should fix them, rather than introduce more #[cfg] here).
There was a problem hiding this comment.
please take another look, I believe it's mostly fixed
| /// The buffer stores one or more page directories (PDs) at the front, | ||
| /// followed by page tables (PTs) that are allocated on demand. All | ||
| /// entries use 4-byte i686 PTEs. | ||
| #[cfg(feature = "i686-guest")] |
There was a problem hiding this comment.
Again, why isn't this just hyperlight_common/arch/i686/vmem.rs and following the same API as the amd64 (and soon to be aarch64) code?
There was a problem hiding this comment.
should be mostly fixed now
Refactor i686 page table code to follow the same architecture-independent API as amd64/aarch64, per reviewer feedback. hyperlight_common changes: - Extract shared page table iterators (modify_ptes, read_pte_if_present, require_pte_exist, write_entry_updating) to vmem.rs with PTE_SHIFT const generic for reuse across architectures. - Implement i686/vmem.rs with proper map()/virt_to_phys() using the shared iterators, replacing stubs and the host-side i686_guest submodule that was in amd64/vmem.rs. - Remove PAGE_USER from pte_for_table (PDEs are supervisor-only by default); user-space PDEs get PAGE_USER via post-processing. - Remove SCRATCH_TOP_PD_ROOTS_* and MAX_PD_ROOTS constants (replaced by set_pt_root_finder callback). hyperlight_host changes: - Unify GuestPageTableBuffer<PTE_BYTES> for both architectures, removing i686_pt::Builder, build_initial_i686_page_tables, and compact_i686_snapshot (~500 lines removed from snapshot.rs). - Add root_offset to GuestPageTableBuffer for targeting per-process PDs, with finalize_multi_root() to replicate kernel PDEs, map scratch, and set PAGE_USER across all roots. - Snapshot::new uses filtered_mappings with root-index tagging for per-process PD isolation in a single pass (no double PT walk). - Add set_pt_root_finder callback on MultiUseSandbox, replacing the scratch-based PD roots bookkeeping. - Add CR0 named constants (CR0_PE, CR0_ET, CR0_WP, CR0_PG). - Add compaction_kind() helper to deduplicate kind conversion. - Add 5 i686 unit tests for map/virt_to_phys. - Detailed comments on separate_pt_bytes explaining the map_file_cow GPA overlap constraint. Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
ludfjig
force-pushed
the
nanvix-platform
branch
from
1213584 to
22af1e7
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds standalone i686 guest support (protected-mode boot, 2-level PT walking, CoW-aware PTE reads) and snapshot compaction, while splitting the old nanvix-unstable gating into i686-guest and guest-counter.
Changes:
- Introduces i686 page-table walking/mapping and updates snapshot creation/restore to support multi-root compaction + CoW resolution.
- Adds an optional
PtRootFindercallback to snapshot multiple page-table roots (e.g., per-process PD roots). - Renames feature gating across host/guest/common (
nanvix-unstable→i686-guest+guest-counter) and updates build tooling.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Updates feature gate for publishing scratch HostSharedMemory for guest counter. |
| src/hyperlight_host/src/sandbox/uninitialized.rs | Renames feature gate and propagates guest-counter gating through GuestCounter + tests. |
| src/hyperlight_host/src/sandbox/snapshot.rs | Implements i686-aware PT reading, CoW resolution, multi-root walking, and separate PT storage for i686 snapshots. |
| src/hyperlight_host/src/sandbox/mod.rs | Re-exports PtRootFinder alongside MultiUseSandbox. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Adds PtRootFinder callback and uses it to supply multiple PT roots to snapshotting. |
| src/hyperlight_host/src/mem/shared_mem.rs | Renames feature gate for test-only HostSharedMemory view. |
| src/hyperlight_host/src/mem/mgr.rs | Generalizes GuestPageTableBuffer over PTE size, adds i686 multi-root finalization, and updates restore PT copying logic. |
| src/hyperlight_host/src/mem/memory_region.rs | Updates cfg attributes to the new i686-guest feature naming. |
| src/hyperlight_host/src/mem/layout.rs | Removes nanvix-unstable-specific base address split; updates cfg attrs to i686-guest. |
| src/hyperlight_host/src/mem/exe.rs | Exposes ELF base VA to compute entrypoint offsets correctly. |
| src/hyperlight_host/src/lib.rs | Re-exports GuestCounter under guest-counter feature. |
| src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs | Updates test gating from nanvix-unstable to i686-guest. |
| src/hyperlight_host/src/hypervisor/virtual_machine/mshv/x86_64.rs | Updates test gating from nanvix-unstable to i686-guest. |
| src/hyperlight_host/src/hypervisor/virtual_machine/mod.rs | Updates XSAVE constant/test gating from nanvix-unstable to i686-guest. |
| src/hyperlight_host/src/hypervisor/virtual_machine/kvm/x86_64.rs | Updates test gating from nanvix-unstable to i686-guest. |
| src/hyperlight_host/src/hypervisor/regs/x86_64/special_regs.rs | Adds i686 protected-mode paging defaults and reorganizes shared constants. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs | Switches to i686 paging defaults when i686-guest is enabled and unifies CR3 handling. |
| src/hyperlight_host/build.rs | Adjusts unshared_snapshot_mem documentation/logic ordering around feature flags. |
| src/hyperlight_host/Cargo.toml | Adds i686-guest and guest-counter features; keeps nanvix-unstable as a compatibility alias. |
| src/hyperlight_guest/src/layout.rs | Renames guest counter gating to guest-counter. |
| src/hyperlight_guest/Cargo.toml | Adds i686-guest and guest-counter features. |
| src/hyperlight_common/src/vmem.rs | Adds shared PT iterator infrastructure, i686 guest module export, and i686-guest target validation. |
| src/hyperlight_common/src/layout.rs | Switches x86_64 layout selection based on i686-guest; renames guest-counter constant gating. |
| src/hyperlight_common/src/arch/i686/vmem.rs | Implements real i686 2-level paging map + walk support (incl. CoW tag via AVL bits). |
| src/hyperlight_common/src/arch/i686/layout.rs | Implements real i686 min scratch size calculation. |
| src/hyperlight_common/src/arch/amd64/vmem.rs | Refactors amd64 PT code to use shared iterator infrastructure + shared UpdateParent logic. |
| src/hyperlight_common/src/arch/aarch64/vmem.rs | Updates TableMovability impls to match new shared trait shape. |
| src/hyperlight_common/Cargo.toml | Adds i686-guest and guest-counter features; keeps nanvix-unstable as alias. |
| Justfile | Updates i686 checks and replaces nanvix-unstable checks with i686-guest where appropriate. |
| use std::collections::HashSet; | ||
| let mut all_mappings = Vec::new(); | ||
| let mut seen_phys = HashSet::new(); |
There was a problem hiding this comment.
Deduplicating by phys_base here can drop valid alias mappings (multiple virt_base values mapping to the same phys_base). That would cause the rebuilt page tables to miss some virtual mappings in the compacted snapshot. A safer approach is to keep all mappings (or at most dedupe by (effective_root, virt_base)), and rely on the later phys_seen compaction map to dedupe page contents.
| if seen_phys.insert(m.phys_base) { | ||
| all_mappings.push((effective_root, m)); | ||
| } |
There was a problem hiding this comment.
Deduplicating by phys_base here can drop valid alias mappings (multiple virt_base values mapping to the same phys_base). That would cause the rebuilt page tables to miss some virtual mappings in the compacted snapshot. A safer approach is to keep all mappings (or at most dedupe by (effective_root, virt_base)), and rely on the later phys_seen compaction map to dedupe page contents.
| #[cfg(feature = "i686-guest")] | ||
| separate_pt_bytes: Vec::new(), |
There was a problem hiding this comment.
On i686-guest, SandboxMemoryManager::restore_snapshot copies page tables from Snapshot::separate_pt_bytes(). Initializing separate_pt_bytes to an empty Vec when constructing a Snapshot risks restoring a snapshot without any page tables copied into scratch. Consider storing the generated PT bytes into separate_pt_bytes (and ensuring restore uses the same source) so every i686 snapshot is self-contained for restore.
| // doesn't make this any more panic-y. | ||
| #[allow(clippy::unwrap_used)] | ||
| let n: [u8; 8] = pte_bytes.try_into().unwrap(); | ||
| u64::from_ne_bytes(n) |
There was a problem hiding this comment.
This still decodes 8-byte page-table entries using from_ne_bytes. Page table entries are little-endian by architecture spec (including x86_64), so this should use u64::from_le_bytes to avoid host-endianness dependence.
| u64::from_ne_bytes(n) | |
| // Page-table entries are little-endian by arch spec; | |
| // use `from_le_bytes` so host endianness doesn't leak in. | |
| u64::from_le_bytes(n) |
| /// For each PTE that maps a VA in [0, MEMORY_SIZE) to a PA in scratch, | ||
| /// record: original_gpa -> scratch_gpa. |
There was a problem hiding this comment.
The comment says the map records original_gpa -> scratch_gpa, but the implementation keys by m.virt_base (a VA from virt_to_phys). Since later lookups in read_entry are done using a physical address (addr), this is easy to misread and can be incorrect if identity-mapping is not guaranteed. Either (a) adjust the doc/comment and naming to explicitly state the identity-mapping assumption (VA==original GPA), or (b) store keys in the same address domain used by the lookup (page-aligned original GPA).
| /// For each PTE that maps a VA in [0, MEMORY_SIZE) to a PA in scratch, | |
| /// record: original_gpa -> scratch_gpa. | |
| /// For each PTE that maps a guest VA in [0, MEMORY_SIZE) to a PA in scratch, | |
| /// record: original_guest_addr -> scratch_gpa, where `original_guest_addr` | |
| /// is the page-table VA (`m.virt_base`). This only corresponds to an | |
| /// "original GPA" if that region is identity-mapped. |
| let page_gpa = addr & 0xFFFFF000; | ||
| if let Some(map) = self.cow_map { | ||
| if let Some(&scratch_gpa) = map.get(&page_gpa) { | ||
| scratch_gpa + (addr & 0xFFF) |
There was a problem hiding this comment.
These hard-coded masks duplicate PAGE_SIZE knowledge. Using PAGE_SIZE-derived masks (e.g., !(PAGE_SIZE as u64 - 1) / (PAGE_SIZE as u64 - 1)) would make the intent clearer and avoid accidental inconsistencies if constants ever change.
| let page_gpa = addr & 0xFFFFF000; | |
| if let Some(map) = self.cow_map { | |
| if let Some(&scratch_gpa) = map.get(&page_gpa) { | |
| scratch_gpa + (addr & 0xFFF) | |
| let page_gpa = addr & !(PAGE_SIZE as u64 - 1); | |
| if let Some(map) = self.cow_map { | |
| if let Some(&scratch_gpa) = map.get(&page_gpa) { | |
| scratch_gpa + (addr & (PAGE_SIZE as u64 - 1)) |
| vm.set_sregs(&CommonSpecialRegisters::standard_32bit_paging_defaults( | ||
| _pml4_addr, | ||
| )) |
There was a problem hiding this comment.
Under i686-guest, _pml4_addr is passed as the 32-bit page directory base. Renaming this variable to something architecture-neutral (e.g., root_pt_addr/root_table_addr) would reduce confusion now that the same path supports both amd64 and i686.
| MappingKind::Unmapped => MappingKind::Unmapped, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new multi-root + dedup/collection logic in filtered_mappings is central to snapshot correctness but isn’t directly covered by the existing tests in this module. Adding a unit test that creates two distinct VAs mapping to the same PA (aliasing) and verifies both VAs remain mapped after compaction would help prevent regressions (especially given the new dedup behavior).
| #[cfg(test)] | |
| fn filtered_mapping_dedup_key(root_index: usize, virt_base: u64) -> (usize, u64) { | |
| (root_index, virt_base) | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| use std::collections::HashMap; | |
| #[test] | |
| fn compaction_dedup_preserves_aliasing_virtual_addresses() { | |
| let root_index = 0usize; | |
| let virt_a = 0x1000_u64; | |
| let virt_b = 0x2000_u64; | |
| let shared_phys = 0x3000_u64; | |
| let kind = MappingKind::Basic(BasicMapping { | |
| readable: true, | |
| writable: true, | |
| executable: false, | |
| }); | |
| let mut collected = HashMap::new(); | |
| collected.insert( | |
| filtered_mapping_dedup_key(root_index, virt_a), | |
| (shared_phys, compaction_kind(&kind)), | |
| ); | |
| collected.insert( | |
| filtered_mapping_dedup_key(root_index, virt_b), | |
| (shared_phys, compaction_kind(&kind)), | |
| ); | |
| assert_eq!(collected.len(), 2); | |
| assert_eq!( | |
| collected | |
| .get(&filtered_mapping_dedup_key(root_index, virt_a)) | |
| .map(|(phys, _)| *phys), | |
| Some(shared_phys) | |
| ); | |
| assert_eq!( | |
| collected | |
| .get(&filtered_mapping_dedup_key(root_index, virt_b)) | |
| .map(|(phys, _)| *phys), | |
| Some(shared_phys) | |
| ); | |
| assert!(matches!( | |
| collected | |
| .get(&filtered_mapping_dedup_key(root_index, virt_a)) | |
| .map(|(_, kind)| kind), | |
| Some(MappingKind::Cow(_)) | |
| )); | |
| assert!(matches!( | |
| collected | |
| .get(&filtered_mapping_dedup_key(root_index, virt_b)) | |
| .map(|(_, kind)| kind), | |
| Some(MappingKind::Cow(_)) | |
| )); | |
| } | |
| #[test] | |
| fn compaction_dedup_preserves_same_virtual_address_across_roots() { | |
| let virt = 0x4000_u64; | |
| let phys = 0x5000_u64; | |
| let kind = MappingKind::Basic(BasicMapping { | |
| readable: true, | |
| writable: false, | |
| executable: false, | |
| }); | |
| let mut collected = HashMap::new(); | |
| collected.insert(filtered_mapping_dedup_key(0, virt), (phys, compaction_kind(&kind))); | |
| collected.insert(filtered_mapping_dedup_key(1, virt), (phys, compaction_kind(&kind))); | |
| assert_eq!(collected.len(), 2); | |
| assert!(collected.contains_key(&filtered_mapping_dedup_key(0, virt))); | |
| assert!(collected.contains_key(&filtered_mapping_dedup_key(1, virt))); | |
| assert!(matches!( | |
| collected.get(&filtered_mapping_dedup_key(0, virt)).map(|(_, kind)| kind), | |
| Some(MappingKind::Basic(BasicMapping { | |
| readable: true, | |
| writable: false, | |
| executable: false, | |
| })) | |
| )); | |
| } | |
| } |
| return None; | ||
| root_pts: &[u64], | ||
| #[cfg(feature = "i686-guest")] cow_map: &std::collections::HashMap<u64, u64>, | ||
| ) -> Vec<(usize, Mapping, &'a [u8])> { |
There was a problem hiding this comment.
The new multi-root + dedup/collection logic in filtered_mappings is central to snapshot correctness but isn’t directly covered by the existing tests in this module. Adding a unit test that creates two distinct VAs mapping to the same PA (aliasing) and verifies both VAs remain mapped after compaction would help prevent regressions (especially given the new dedup behavior).
| // temporarily!) need to use writable/un-shared snapshot | ||
| // memories, and so can't share | ||
| unshared_snapshot_mem: { any(feature = "nanvix-unstable", feature = "gdb") }, | ||
| // gdb needs writable snapshot memory for debug access. |
There was a problem hiding this comment.
The comment mentions only gdb, but the condition also includes nanvix-unstable. Either update the comment to reflect both reasons, or (if intended) migrate this to the new feature split (i686-guest / guest-counter) so the build-time behavior matches the new feature naming.
| // gdb needs writable snapshot memory for debug access. | |
| // Writable snapshot memory is required for gdb debug access and nanvix-unstable builds. |
Summary
The cleaned-up version of #1381: drops the dependencies on #1379 and #1380 so this PR can land on its own.
Commits
refactor: replace nanvix-unstable with i686-guest and guest-counter featuresfeat: i686 protected-mode boot and unified restore pathfeat: i686 page tables, snapshot compaction, and CoW supportfixup: address PR #1381 review commentsWhat this adds
nanvix-unstablegates split intoi686-guest(PT walker, protected-mode boot, compaction) andguest-counter(scratch counter plumbing). No behavior change for consumers using the old feature flag; Cargo.toml / Justfile / build.rs updated to match.What it does not include (vs #1381)
file_mappingsremoval (Remove HyperlightPEB file mappings #1380).Both can land separately; #1381 happened to stack on top of them.
Review items from #1381 addressed here
u32::from_le_bytes(previouslyfrom_ne_bytes) — PTEs are little-endian by arch spec.i686-guestfeature guarded with acompile_error!on targets that are neitherx86(guest) norx86_64(host).restore_snapshotrewrites the scratch PD-roots bookkeeping (SCRATCH_TOP_PD_ROOTS_{COUNT,ARRAY}_OFFSET) so a subsequentsnapshot()doesn't see a stale zero count.Snapshotpersists the root count; root GPAs are deterministic (layout.get_pt_base_gpa() + i * PAGE_SIZE).Verification
Built with
cargo check -p hyperlight-{common,host,guest} --features kvm,mshv3,executable_heap,i686-guest,guest-counter— clean. End-to-end tested against nanvix@e61306676: Hello 5/5 withNANVIX_REPEAT=4through the restore+call loop.