-
Notifications
You must be signed in to change notification settings - Fork 155
Move and rename some Vm traits #1105
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?
Conversation
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Clean up import paths after file move Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com> Rename hyperv_linux to mshv, hyperv_windows to whp Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
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.
Pull request overview
This PR performs a significant refactoring of VM-related code to improve organization and naming consistency. It moves hypervisor-specific implementation files from the root hypervisor module and the sandbox module into a new hypervisor/vm submodule, renames files to use shorter and more descriptive names (mshv, whp), and renames core abstractions (Hypervisor trait → Vm trait, HyperlightExit → VmExit) to better reflect their purpose.
Key Changes:
- Created new
hypervisor/vmmodule to consolidate VM implementations (KVM, MSHV, WHP) - Renamed
Hypervisortrait toVmandHyperlightExitenum toVmExitfor clearer semantics - Moved hypervisor detection logic from
sandbox::hypervisortohypervisor::vm
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/src/sandbox/mod.rs | Removed hypervisor submodule and moved is_hypervisor_present() to hypervisor/vm module |
| src/hyperlight_host/src/sandbox/hypervisor.rs | Deleted - functionality moved to hypervisor/vm/mod.rs |
| src/hyperlight_host/src/lib.rs | Updated public export to reference hypervisor::vm::is_hypervisor_present |
| src/hyperlight_host/src/hypervisor/vm/mod.rs | New module containing VM trait, VmExit enum, hypervisor detection, and test moved from sandbox |
| src/hyperlight_host/src/hypervisor/vm/whp.rs | Renamed from hyperv_windows.rs; updated trait implementation and imports |
| src/hyperlight_host/src/hypervisor/vm/mshv.rs | Renamed from hyperv_linux.rs; updated trait implementation and imports |
| src/hyperlight_host/src/hypervisor/vm/kvm.rs | Moved to vm submodule; updated trait implementation and imports |
| src/hyperlight_host/src/hypervisor/mod.rs | Removed Hypervisor trait, HyperlightExit enum, and old module declarations |
| src/hyperlight_host/src/hypervisor/hyperlight_vm.rs | Updated imports to reference vm module and new Vm/VmExit names; updated comments |
| src/hyperlight_host/src/hypervisor/gdb/mod.rs | Updated DebuggableVm trait to extend Vm instead of Hypervisor |
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
99037b1 to
6d2f287
Compare
danbugs
left a comment
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 like the change from Hypervisor → Vm. I agree that it is more accurate. Same for VmExit over HyperlightExit.
Though, iirc, in the past, when we considered changing from hyperv_* to mshv/whp, we opted to keep the previous names.
THanks. I think the renaming was discarded in the past due to making a PR too hard to review |
I think Simon also mentioned the |
|
LGTM, I tend to agree that I perfer hyperv_linux and hyperv_windows over mshv and whp. |
|
I like whp (windows hypervisor platform) because it's the name of the API we use https://learn.microsoft.com/en-us/virtualization/api/hypervisor-platform/hypervisor-platform |
Some follow up items from #924. Please review commit by commit
Hypervisortrait toVmtraitHyperlightExittoVmExitNote that this PR does not modify any code (with exception of renames and moves)