-
Notifications
You must be signed in to change notification settings - Fork 155
Unify page table manipulation code between the guest and the host #1093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,217 @@ | ||||||||||||||||||
| /* | ||||||||||||||||||
| Copyright 2025 The Hyperlight Authors. | ||||||||||||||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||
| you may not use this file except in compliance with the License. | ||||||||||||||||||
| You may obtain a copy of the License at | ||||||||||||||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||
| Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||
| See the License for the specific language governing permissions and | ||||||||||||||||||
| limitations under the License. | ||||||||||||||||||
| */ | ||||||||||||||||||
|
|
||||||||||||||||||
| use crate::vm::{Mapping, MappingKind, TableOps}; | ||||||||||||||||||
|
|
||||||||||||||||||
| #[inline(always)] | ||||||||||||||||||
| /// Utility function to extract an (inclusive on both ends) bit range | ||||||||||||||||||
| /// from a quadword. | ||||||||||||||||||
| fn bits<const HIGH_BIT: u8, const LOW_BIT: u8>(x: u64) -> u64 { | ||||||||||||||||||
| (x & ((1 << (HIGH_BIT + 1)) - 1)) >> LOW_BIT | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A helper structure indicating a mapping operation that needs to be | ||||||||||||||||||
| /// performed | ||||||||||||||||||
| struct MapRequest<T> { | ||||||||||||||||||
| table_base: T, | ||||||||||||||||||
| vmin: VirtAddr, | ||||||||||||||||||
| len: u64, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// A helper structure indicating that a particular PTE needs to be | ||||||||||||||||||
| /// modified | ||||||||||||||||||
| struct MapResponse<T> { | ||||||||||||||||||
| entry_ptr: T, | ||||||||||||||||||
| vmin: VirtAddr, | ||||||||||||||||||
| len: u64, | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| struct ModifyPteIterator<const HIGH_BIT: u8, const LOW_BIT: u8, Op: TableOps> { | ||||||||||||||||||
| request: MapRequest<Op::TableAddr>, | ||||||||||||||||||
| n: u64, | ||||||||||||||||||
| } | ||||||||||||||||||
| impl<const HIGH_BIT: u8, const LOW_BIT: u8, Op: TableOps> Iterator | ||||||||||||||||||
| for ModifyPteIterator<HIGH_BIT, LOW_BIT, Op> | ||||||||||||||||||
| { | ||||||||||||||||||
| type Item = MapResponse<Op::TableAddr>; | ||||||||||||||||||
| fn next(&mut self) -> Option<Self::Item> { | ||||||||||||||||||
| if (self.n << LOW_BIT) >= self.request.len { | ||||||||||||||||||
| return None; | ||||||||||||||||||
| } | ||||||||||||||||||
| // next stage parameters | ||||||||||||||||||
| let mut next_vmin = self.request.vmin + (self.n << LOW_BIT); | ||||||||||||||||||
| let lower_bits_mask = (1 << LOW_BIT) - 1; | ||||||||||||||||||
| if self.n > 0 { | ||||||||||||||||||
| next_vmin &= !lower_bits_mask; | ||||||||||||||||||
| } | ||||||||||||||||||
| let entry_ptr = Op::entry_addr( | ||||||||||||||||||
| self.request.table_base, | ||||||||||||||||||
| bits::<HIGH_BIT, LOW_BIT>(next_vmin) << 3, | ||||||||||||||||||
| ); | ||||||||||||||||||
| let len_from_here = self.request.len - (next_vmin - self.request.vmin); | ||||||||||||||||||
| let max_len = (1 << LOW_BIT) - (next_vmin & lower_bits_mask); | ||||||||||||||||||
| let next_len = core::cmp::min(len_from_here, max_len); | ||||||||||||||||||
|
Comment on lines
+54
to
+66
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not too sure what's going on here. Maybe some comments would help |
||||||||||||||||||
|
|
||||||||||||||||||
| // update our state | ||||||||||||||||||
| self.n += 1; | ||||||||||||||||||
|
|
||||||||||||||||||
| Some(MapResponse { | ||||||||||||||||||
| entry_ptr, | ||||||||||||||||||
| vmin: next_vmin, | ||||||||||||||||||
| len: next_len, | ||||||||||||||||||
| }) | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
| fn modify_ptes<const HIGH_BIT: u8, const LOW_BIT: u8, Op: TableOps>( | ||||||||||||||||||
| r: MapRequest<Op::TableAddr>, | ||||||||||||||||||
| ) -> ModifyPteIterator<HIGH_BIT, LOW_BIT, Op> { | ||||||||||||||||||
| ModifyPteIterator { request: r, n: 0 } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Page-mapping callback to allocate a next-level page table if necessary. | ||||||||||||||||||
| /// # Safety | ||||||||||||||||||
| /// This function modifies page table data structures, and should not be called concurrently | ||||||||||||||||||
| /// with any other operations that modify the page tables. | ||||||||||||||||||
| unsafe fn alloc_pte_if_needed<Op: TableOps>( | ||||||||||||||||||
| op: &Op, | ||||||||||||||||||
| x: MapResponse<Op::TableAddr>, | ||||||||||||||||||
| ) -> MapRequest<Op::TableAddr> { | ||||||||||||||||||
| let pte = unsafe { op.read_entry(x.entry_ptr) }; | ||||||||||||||||||
| let present = pte & 0x1; | ||||||||||||||||||
| if present != 0 { | ||||||||||||||||||
|
Comment on lines
+93
to
+94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
or even better
Suggested change
|
||||||||||||||||||
| return MapRequest { | ||||||||||||||||||
| table_base: Op::from_phys(pte & !0xfff), | ||||||||||||||||||
| vmin: x.vmin, | ||||||||||||||||||
| len: x.len, | ||||||||||||||||||
| }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| let page_addr = unsafe { op.alloc_table() }; | ||||||||||||||||||
|
|
||||||||||||||||||
| #[allow(clippy::identity_op)] | ||||||||||||||||||
| #[allow(clippy::precedence)] | ||||||||||||||||||
| let pte = Op::to_phys(page_addr) | | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we do |
||||||||||||||||||
| 1 << 5 | // A - we don't track accesses at table level | ||||||||||||||||||
| 0 << 4 | // PCD - leave caching enabled | ||||||||||||||||||
| 0 << 3 | // PWT - write-back | ||||||||||||||||||
| 1 << 2 | // U/S - allow user access to everything (for now) | ||||||||||||||||||
| 1 << 1 | // R/W - we don't use block-level permissions | ||||||||||||||||||
| 1 << 0; // P - this entry is present | ||||||||||||||||||
| unsafe { op.write_entry(x.entry_ptr, pte) }; | ||||||||||||||||||
| MapRequest { | ||||||||||||||||||
| table_base: page_addr, | ||||||||||||||||||
| vmin: x.vmin, | ||||||||||||||||||
| len: x.len, | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| /// Map a normal memory page | ||||||||||||||||||
| /// # Safety | ||||||||||||||||||
| /// This function modifies page table data structures, and should not be called concurrently | ||||||||||||||||||
| /// with any other operations that modify the page tables. | ||||||||||||||||||
| #[allow(clippy::identity_op)] | ||||||||||||||||||
| #[allow(clippy::precedence)] | ||||||||||||||||||
| unsafe fn map_page<Op: TableOps>(op: &Op, mapping: &Mapping, r: MapResponse<Op::TableAddr>) { | ||||||||||||||||||
| let pte = match &mapping.kind { | ||||||||||||||||||
| MappingKind::BasicMapping(bm) => | ||||||||||||||||||
| // TODO: Support not readable | ||||||||||||||||||
|
||||||||||||||||||
| // TODO: Support not readable | |
| // NOTE: On x86-64, there is no separate "readable" bit in the page table entry. | |
| // This means that pages cannot be made write-only or execute-only without also being readable. | |
| // All pages that are mapped as writable or executable are also implicitly readable. | |
| // If support for "not readable" mappings is required in the future, it would need to be | |
| // implemented using additional mechanisms (e.g., page-fault handling or memory protection keys), | |
| // but for now, this architectural limitation is accepted. |
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting bit 7 (PAT bit) to 1 is incorrect. According to the Intel and AMD manuals, bit 7 in a page table entry is the PAT (Page Attribute Table) bit, not a reserved bit that must be 1. Setting this bit unconditionally changes the memory type of all mapped pages to use PAT entry 1 instead of the default entry 0, which could cause unexpected caching behavior.
Unless there's a specific reason to use a non-default PAT entry, this bit should be set to 0 to match the PCD=0, PWT=0 settings (which together select PAT entry 0 for normal write-back cached memory).
| 1 << 7 | // 1 - RES1 according to manual | |
| 0 << 7 | // PAT=0 (default write-back caching) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is code that was just moved around, but it would be nice if there was some comments explaining what's going on here.
Copilot
AI
Dec 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new page table manipulation code in hyperlight_common lacks unit tests. This is critical code that handles memory mapping with complex logic including bit manipulation, iterator state management, and unsafe operations. Given that similar modules in the codebase have comprehensive test coverage, tests should be added to verify:
- Correct handling of page-aligned and unaligned virtual addresses
- Proper calculation of entry indices at each page table level
- Correct PTE flag generation for different mapping types
- Edge cases like zero-length mappings or boundaries crossing page table entries
This is especially important given the bugs found in the host's TableOps implementation, which would have been caught by tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| let present = pte & 0x1; | |
| if present == 0 { | |
| let present = bits<0,0>(pte) != 0; | |
| if !present { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this might have changed behavious, is that intentional?
This used to panic, now it retuns None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I king of liked better the previous name dbg_print_address_pte, what is vtop?
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,131 @@ | ||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||
| Copyright 2025 The Hyperlight Authors. | ||||||||||||||||||||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||||||||||||||||||||
| you may not use this file except in compliance with the License. | ||||||||||||||||||||||||
| You may obtain a copy of the License at | ||||||||||||||||||||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||||||||||||||||||||
| Unless required by applicable law or agreed to in writing, software | ||||||||||||||||||||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||||||||||||||||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||||||||||||||||||||
| See the License for the specific language governing permissions and | ||||||||||||||||||||||||
| limitations under the License. | ||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| #[cfg_attr(target_arch = "x86_64", path = "arch/amd64/vm.rs")] | ||||||||||||||||||||||||
| mod arch; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| pub use arch::{PAGE_SIZE, PAGE_TABLE_SIZE, PageTableEntry, PhysAddr, VirtAddr}; | ||||||||||||||||||||||||
| pub const PAGE_TABLE_ENTRIES_PER_TABLE: usize = | ||||||||||||||||||||||||
| PAGE_TABLE_SIZE / core::mem::size_of::<PageTableEntry>(); | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// The operations used to actually access the page table structures, | ||||||||||||||||||||||||
| /// used to allow the same code to be used in the host and the guest | ||||||||||||||||||||||||
| /// for page table setup | ||||||||||||||||||||||||
| pub trait TableOps { | ||||||||||||||||||||||||
| /// The type of table addresses | ||||||||||||||||||||||||
| type TableAddr: Copy; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Allocate a zeroed table | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// # Safety | ||||||||||||||||||||||||
| /// The current implementations of this function are not | ||||||||||||||||||||||||
| /// inherently unsafe, but the guest implementation will likely | ||||||||||||||||||||||||
| /// become so in the future when a real physical page allocator is | ||||||||||||||||||||||||
| /// implemented. | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// Currently, callers should take care not to call this on | ||||||||||||||||||||||||
| /// multiple threads at the same time. | ||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||
| /// # Panics | ||||||||||||||||||||||||
| /// This function may panic if: | ||||||||||||||||||||||||
| /// - The Layout creation fails | ||||||||||||||||||||||||
| /// - Memory allocation fails | ||||||||||||||||||||||||
| unsafe fn alloc_table(&self) -> Self::TableAddr; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| /// Offset the table address by the u64 entry offset | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| /// Offset the table address by the u64 entry offset | |
| /// Offset the table address by the given offset in bytes. | |
| /// | |
| /// # Parameters | |
| /// - `addr`: The base address of the table. | |
| /// - `entry_offset`: The offset in **bytes** within the page table. This is | |
| /// not an entry index; callers must multiply the entry index by the size | |
| /// of a page table entry (typically 8 bytes) to obtain the correct byte offset. | |
| /// | |
| /// # Returns | |
| /// The address of the entry at the given byte offset from the base address. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| unsafe fn write_entry(&self, addr: Self::TableAddr, x: PageTableEntry); | |
| unsafe fn write_entry(&self, addr: Self::TableAddr, entry: PageTableEntry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these comments correct? or are they swapped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feature is not initializaing paging, but providing data structures for managing virtual memory.
I would make the data structures unconditionally available (i.e., no feature needed), as they have no side-effects (no extra dependencies, no runtime side effects, I wouldn't feature gate it here, but would feature gate it in hl-host / hl-guest)
Alternatively, how about calling it "virtual-memory"?