feat(whp): support no-surrogate mode via HYPERLIGHT_MAX_SURROGATES=0#1578
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional Windows-only WHP mode (whp-no-surrogate) that bypasses the surrogate process for GPA mapping, intended for single-partition-per-process scenarios, and refactors shared-memory allocation to share validation/guard-page setup across allocation paths.
Changes:
- Introduces a new
whp-no-surrogatefeature flag inhyperlight-host. - Adds a
VirtualAlloc-backed shared memory allocation path (vsCreateFileMappingA) and maps GPAs viaWHvMapGpaRange(vs dynamically-loadedWHvMapGpaRange2through the surrogate). - Refactors Windows shared memory creation to reuse
validated_total_size()andset_guard_pages()helpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/hyperlight_host/src/mem/shared_mem.rs |
Adds DirectAlloc mapping mode + VirtualAlloc allocation path and refactors guard-page/size validation helpers. |
src/hyperlight_host/src/hypervisor/virtual_machine/whp.rs |
Adds feature-gated mapping path using WHvMapGpaRange and removes surrogate-process usage when enabled. |
src/hyperlight_host/Cargo.toml |
Declares the new whp-no-surrogate feature flag. |
ludfjig
left a comment
There was a problem hiding this comment.
Have you considered making this a runtime option instead, for example on SandboxConfiguration? If possible I think I would prefer it
35aaabf to
820c953
Compare
Had a chat w/ @simongdavies and modified this PR to integrate w/ |
simongdavies
left a comment
There was a problem hiding this comment.
LGTM, prefer this to having a feature.
b6d3d16 to
16e5f94
Compare
Added testing. Opted to not re-run the full CI to save time and also because some tests just don't make sense (e.g., full tests validate surrogate process machinery). |
427a949 to
a991a2e
Compare
There was a problem hiding this comment.
I think there's a leak possible. And I don't think any tests exercises the new memory apis still. Can we at least run a subset of existing tests to make sure guest calls/hostcall and snapshotting/persist snapshotting still works as expected?
We should also add this to changelog, up to you if you want to do it in this PR or future PR though
beaf570 to
4559074
Compare
Expanded the CI no-surrogate step to run
Will update the changelog in a follow-up before the release. cc: @ludfjig |
Looks good but I don't think snapshotting nor saving/loading snapshot from/to disk is covered by these added tests. And minor nit is that renaming these tests will not fail ci, it will just report running 0 tests, maybe we can grep for them in output to make sure they ran and passed. I fear that we might rename the tests later and forget updating the ci |
4559074 to
4a0c1cd
Compare
4a0c1cd to
91fcb14
Compare
Current coverage:
Added a rename guard for this. |
91fcb14 to
79c8e94
Compare
Tests look perfect thanks! For the guard, why not have the guard itself set the static boolean when created (or fail if already created), and have the drop unset it, and store on the vm as a field. I think this would be simpler (and no need to std::mem::forget) |
When HYPERLIGHT_MAX_SURROGATES=0, bypass surrogate processes entirely: - Use WHvMapGpaRange (host VA) instead of WHvMapGpaRange2 (surrogate) - Reuse existing WindowsMapping::Anonymous allocation path - RAII guard (stored as WhpVm field) enforces single-VM-per-process - Add dedicated unit test and run existing integration/snapshot tests in CI with per-test verification against silent renames - Add just test-no-surrogate recipe mirroring the CI step Signed-off-by: danbugs <danilochiarlone@gmail.com>
79c8e94 to
212f20a
Compare
Much better idea, thanks. Actually, with this change, the surrogate guard and the surrogate process have the exact same lifetime and exactly one of them is used. So maybe you could even make the SurrogateProcess an enum or a dyn Trait and push the difference in behaviour all the way down to that level, so whp.rs does not need to know or care whether there is a surrogate process. To unify the |
ludfjig
left a comment
There was a problem hiding this comment.
LGTM (nit last bullet on pr description is stale)
HYPERLIGHT_MAX_SURROGATES=0, skip surrogate process creation entirely and useVirtualAlloc+WHvMapGpaRangeinstead ofCreateFileMappingA+ surrogate +WHvMapGpaRange2WHvMapGpaRangereturnsERROR_VID_PARTITION_ALREADY_EXISTSwhen called from multiple partitions in the same process)compute_surrogate_counts()now accepts 0 as a valid minimum, andsurrogates_disabled()checks the env var at runtimeWhpVm::surrogate_processis nowOption<SurrogateProcess>, withmap_memory/unmap_memorybranching at runtimeExclusiveSharedMemory::new()usesVirtualAlloc(via newDirectAllocationRAII type) when surrogates are disabled,CreateFileMappingAotherwise