Unify register representation across hypervisors #907
Conversation
ludfjig
requested review from
danbugs,
dblnz,
devigned,
jprendes,
marosset,
simongdavies and
syntactically
as code owners
ludfjig
added
the
kind/enhancement
ludfjig
force-pushed
the
common_regs
branch
4 times, most recently
from
c1ada25 to
a523458
Compare
| /// This is a default implementation that works for all hypervisors | ||
| fn setup_initial_sregs(&mut self, _pml4_addr: u64) -> Result<()> { | ||
| #[cfg(feature = "init-paging")] | ||
| let sregs = CommonSpecialRegisters { |
There was a problem hiding this comment.
Having registers in one location seems like a create improvement as we move forward with adding new features.
ludfjig
force-pushed
the
common_regs
branch
4 times, most recently
from
5950288 to
bcbe77f
Compare
| #[allow(dead_code)] | ||
| fn sregs(&self) -> Result<CommonSpecialRegisters>; | ||
| /// Set special regs | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
Are this allow(dead_code) really needed?
There was a problem hiding this comment.
yes. In next PR I will consolidate everything
| @@ -0,0 +1,466 @@ | |||
| /* | |||
| Copyright 2024 The Hyperlight Authors. | |||
There was a problem hiding this comment.
I am not sure what the guidance is here, but shouldn't we use 2025 if we added the file this year? I'm asking for myself also, to know how I do it.
There was a problem hiding this comment.
For new files , I think definitely, I think i we update files we should probably update it as well?
There was a problem hiding this comment.
Co-pilot seems to agree 😄
There was a problem hiding this comment.
I had done something similar but only for this tracing scope.
The unification you've done is more comprehensive
There was a problem hiding this comment.
Pull Request Overview
This PR introduces unified register representations across hypervisor platforms (KVM, MSHV, and WHP) through new CommonRegisters, CommonSpecialRegisters, and CommonFpu abstractions. This unification is the first part of a larger VM trait refactoring effort and addresses UB issues with WHP by ensuring proper 16-byte alignment for register values.
Key changes:
- Introduces common register abstractions that abstract over hypervisor-specific register formats
- Implements alignment wrapper (
Align16) to fix undefined behavior in Windows Hypervisor Platform register handling - Refactors existing register manipulation code to use the new common interfaces
Reviewed Changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
typos.toml |
Added "fpr" to typo exceptions for floating-point register field names |
src/hyperlight_host/src/sandbox/outb.rs |
Updated to use common register interfaces instead of platform-specific trace register reads |
src/hyperlight_host/src/hypervisor/regs/ |
New module containing common register abstractions and conversions |
src/hyperlight_host/src/hypervisor/mod.rs |
Added common register trait methods and default special register setup |
src/hyperlight_host/src/hypervisor/wrappers.rs |
Removed platform-specific register structs now replaced by common abstractions |
| Various hypervisor implementations | Updated to implement new register trait methods and use common register types |
| @@ -0,0 +1,466 @@ | |||
| /* | |||
| Copyright 2024 The Hyperlight Authors. | |||
There was a problem hiding this comment.
The copyright year should be 2025 to match the other files in this PR. Other files in this change use 2025.
| Copyright 2024 The Hyperlight Authors. | |
| Copyright 2025 The Hyperlight Authors. |
Copilot uses AI. Check for mistakes.
| WHvX64RegisterRip, | ||
| WHvX64RegisterRflags, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Nit: might be good to add a compile time check that the length of the array matches WHP_REGS_NAMES_LEN?
#[cfg(target_os = "windows")]
const _: () = assert!(WHP_REGS_NAMES.len() == WHP_REGS_NAMES_LEN);
There was a problem hiding this comment.
That should already be type-checked in the type declaration pub(crate) const WHP_REGS_NAMES: [WHV_REGISTER_NAME; WHP_REGS_NAMES_LEN] = ....
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
5 participants
This PR is part of my bigger effort to refactor VM trait (PR 1/3). This PR introduces registers structs that abstract over differences between kvm, mshv, and whp, for general registers, special registers, and fpu registers. It also fixes some UB where whp requires register values to be 16-aligned.
The introduced abstractions are not too useful when just looking at this PR in isolation, but future PR will refactor and deduplicate a bunch of code, where this will be needed.
Closes #908
Closes #312