Refactor SandboxMemoryLayout#1455
Merged
Merged
Conversation
ludfjig
added
the
kind/refactor
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors SandboxMemoryLayout to remove derived/stored offset fields and instead compute layout addresses/offsets on demand, with the goal of simplifying snapshot serialization and reducing layout-related bugs.
Changes:
- Replace stored layout-derived fields (PEB offsets, heap/init offsets, etc.) with accessor methods and store only configuration-derived sizes.
- Switch PEB initialization to build a
HyperlightPEBstruct and serialize it viabytemuckrather than manually writing individualu64fields. - Add
bytemuck::Pod/Zeroablederives forGuestMemoryRegion/HyperlightPEBand add a unit test validating round-trip byte serialization.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/uninitialized_evolve.rs | Update PEB address access to the new peb_address() accessor. |
| src/hyperlight_host/src/mem/mgr.rs | Use new SandboxMemoryLayout size fields (input_data_size/output_data_size) instead of pulling from config. |
| src/hyperlight_host/src/mem/layout.rs | Remove derived fields and compute offsets via methods; refactor PEB writing to struct + bytemuck serialization. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/x86_64.rs | Update test to use peb_address() accessor. |
| src/hyperlight_host/Cargo.toml | Add bytemuck dependency for host-side PEB serialization. |
| src/hyperlight_common/src/mem.rs | Mark PEB-related structs as Pod/Zeroable and add serialization round-trip unit test. |
| Cargo.lock | Lockfile update to include bytemuck in the dependency graph. |
Signed-off-by: Ludvig Liljenberg <[email protected]>
ludfjig
requested review from
andreiltd,
danbugs,
dblnz,
devigned,
jprendes,
jsturtevant,
simongdavies and
syntactically
as code owners
jprendes
approved these changes
May 14, 2026
Contributor
jprendes
left a comment
jprendes
left a comment
There was a problem hiding this comment.
LGTM
Very clean refactor, thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR removes derived fields from SandboxMemoryLayout, with the primary goal to help simplify future snapshot serialization and prevent a whole category of bugs. Note that the added
bytemuckcrate dep is already a transitive dep of hyperlight-common, so I'm not adding anything new. I filed #1454 to fix this, but not in scope for this PR