Introduce new Vm trait #924
Conversation
ludfjig
force-pushed
the
vm_trait_new
branch
3 times, most recently
from
81f0d54 to
62fad87
Compare
ludfjig
added
the
kind/refactor
ludfjig
force-pushed
the
vm_trait_new
branch
17 times, most recently
from
1562f26 to
edc7f00
Compare
ludfjig
force-pushed
the
vm_trait_new
branch
4 times, most recently
from
f6337f9 to
350b8e0
Compare
ludfjig
requested review from
jprendes,
marosset,
simongdavies and
syntactically
as code owners
ludfjig
force-pushed
the
vm_trait_new
branch
5 times, most recently
from
d2e6bf8 to
afa720d
Compare
jsturtevant
left a comment
jsturtevant
left a comment
There was a problem hiding this comment.
I spent some time looking at these changes. There seem to be alot of really good things in here. My main concern is that it is a very big change that encompases new features, changes, and simplifications across multiple implementations.
My initial thought would be break this down in to small chunks: Add a VM trait, and implement for a single platform. Then move each platform over, this would keep the set of changes smaller. I would also think that we could keep things simplier to review by not doing updates to implementations like the changes for "Simplifies and refactors some cancellation stuff" and have those as seperate change sets
Signed-off-by: Ludvig Liljenberg <[email protected]>
Signed-off-by: Ludvig Liljenberg <[email protected]>
dblnz
left a comment
dblnz
left a comment
There was a problem hiding this comment.
Good work!
I like that we get rid of duplicated code, and we now have a clear separation of common Vm code and specific Vm logic.
It is definitely the right direction we want to go.
I left some small comments.
I need to emphasize the fact that being a big change, it is difficult to follow where code was moved.
| ) | ||
| )] | ||
| Retry(), | ||
| #[cfg(gdb)] |
There was a problem hiding this comment.
Nit: Invert the comment and #[cfg..
| #[cfg(gdb)] | |
| /// The vCPU has exited due to a debug event (usually breakpoint) | |
| #[cfg(gdb)] |
| use crate::sandbox::uninitialized::SandboxRuntimeConfig; | ||
| use crate::{HyperlightError, Result, log_then_return, new_error}; | ||
|
|
||
| pub(crate) struct HyperlightVm { |
There was a problem hiding this comment.
Some comments here explaining what this struct wants to achieve and why we need it would be helpful.
I assume this replaces the old Hypervisor
| // Architectures Software Developer's Manual | ||
| if dr6 & DR6_BS_FLAG_MASK != 0 && single_step { | ||
| return VcpuStopReason::DoneStep; | ||
| if dr6 & DR6_BS_FLAG_MASK != 0 { |
There was a problem hiding this comment.
I think it is fine to remove the single_step variable here. It was only an additional state for verifying the correct flag was in sync with internal state of debugging
|
|
||
| if BP_EX_ID == exception && sw_breakpoints.contains_key(&rip) { | ||
| return VcpuStopReason::SwBp; | ||
| if BP_EX_ID == exception { |
There was a problem hiding this comment.
I think this might remove an option here.
If the vCPU stops because of an issue, not a SW/HW breakpoint set by the debugger, it will not be transmitted as an unknown breakpoint.
I am not sure though if this means that the debugger won't show it as an exception.
There was a problem hiding this comment.
I think #BP is only ever raised by INT3, so checking sw breakpoints is redundant?
| // Mark that a guest function call is now active | ||
| // (This also increments the generation counter internally) | ||
| // The guard will automatically clear call_active when dropped | ||
| let _guard = CallActiveGuard::new(self.vm.interrupt_handle())?; |
There was a problem hiding this comment.
Where did this go to? Is call_active still being used? What are the other changes related to this?
There was a problem hiding this comment.
Nope it's all gone. I was not a big fan of the original logic but we still merged it in favor of velocity. All logic related to cancellation is now in HyperlightVm::run. It's been greatly simplified
3 participants
This is PR 2/3 in a bigger effort to remove duplicate code across drivers.
depends on #907 which must be merged first, will mark this PR as ready then
This PR introduces
Vmtrait. It's a minimal trait for common functionality of a minimal Vm. It abstracts over differences in kvm, mshv, whp. This traits only knows things like set/get registers, run, but nothing about guest functions or hyperlight specifics.HyperlightVmstruct. This is a struct that contains thedyn Vmabove, as well as things like guest_ptr, rsp, memory-regions, gdb connections, etc. You can think of this as replacing the previous Hypervisor trait (but now it's just 1 struct to avoid duplicate code). HyperlightVm knows about initialization, dispatching guest calls, gdb-debugging etc, guest-tracing, whichVmtrait doesn't.kill()without changing behaviorWhen reviewing, new file hyperlight_vm.rs should be compared against old kvm.rs, hyperv_linux.rs, hyperv_windows.rs
Closes #465, #904