-
Notifications
You must be signed in to change notification settings - Fork 132
Refactor Vm trait #517
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?
Refactor Vm trait #517
Conversation
…ll drivers. Remove duplicated GDB code. Signed-off-by: Ludvig Liljenberg <[email protected]>
e697c0f
to
c18f0f1
Compare
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.
LGTM. Awesome work!
Ok(()) | ||
} | ||
} | ||
} |
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.
Maybe @dblnz wants to have a look at this?
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.
This is copied from previous implementation
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.
Given the scope of the changes I think we should have @dblnz review this
Ok(HyperlightExit::MmioRead(addr)) => { | ||
#[cfg(crashdump)] | ||
crashdump::crashdump_to_tempfile(self)?; | ||
|
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.
Currently, on Windows, we have some very useful debug!
s for different HyperlightExit
s that capture VCPU state. I think these were removed from whp.rs
. Would be good to re-add them here, so we have consistent debug!
s for all drivers.
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.
Isn't the crashdump feature enough for this? I can add them back if you prefer though
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 tend to agree with Dan, debug output might be used when crashdump feature is off, however we should probably not do this in release builds since registers might contain sensitive data
unsafe impl Send for WhpVm {} | ||
unsafe impl Sync for WhpVm {} | ||
|
||
#[repr(C, align(16))] |
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.
Maybe add a comment here on why we do this :)
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.
fixed
Signed-off-by: Ludvig Liljenberg <[email protected]>
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 had to give up reviewing this, it is too big, please split this up into smaller sequential commits and a set of smaller PRs to make it reviewable. For example the changes to optimise /dev/kvm and /dev/mshv could easily be a seperate PR
} | ||
|
||
// If any of the B0-B3 flags in DR6 register is set, it means a | ||
// hardware breakpoint triggered the exit | ||
// Check page 19-4 Vol. 3B of Intel 64 and IA-32 | ||
// Architectures Software Developer's Manual | ||
if DR6_HW_BP_FLAGS_MASK & dr6 != 0 && hw_breakpoints.contains(&rip) { | ||
if DR6_HW_BP_FLAGS_MASK & dr6 != 0 { |
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.
why did this logic change?
} | ||
} | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did this if change?
Ok(()) | ||
} | ||
} | ||
} |
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.
Given the scope of the changes I think we should have @dblnz review this
} | ||
|
||
#[derive(Debug)] | ||
pub(crate) struct HyperlightSandbox { |
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 am a bit confused as we now seem to have a new struct called HyperlightSandbox and we already have a Sandbox , think this needs a different name , and probably needs to be in its own module file
} | ||
} | ||
|
||
impl HyperlightVm for HyperlightSandbox { |
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.
#Why are we calling this HyperlightVm? what other Vms are we going to have? Should it be called Hypervisor?
@@ -0,0 +1,470 @@ | |||
/* |
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 think renaming these files is wrong, the old names were more descriptive, but, if we want to rename them then lets do it as a separate PR , it makes it even harder to review
Do you plan on updating this after the |
I won't merge yet, I'm very sorry for delaying your windows PR. Let's get your windows PR merged first if it's ready |
This PR introduces a minimal
Vm
trait. It's implemented byKvmVm
,MshvVm
andWhpVm
. The goal is to reduce code duplication. The existingHypervisor
trait is renamed toHyperlightVm
, and is now only implemented byHyperlightSandbox
(and Inprocess, but not for long since we're getting rid of inproces mode). In addition, theVm
trait also reduces a bunch of code duplication related to GDB debugging.About half of the lines of this PR is introducing common register structs used for all Vms. They Are CommonRegisters, CommonStandardRegisters, and CommonFpu, with tests. The rest should be pretty straight forward, and is mostly just plumbing.
Closes #465
Closes #312
Closes #503
There are some more cleanup to do (like cleaning up some windows surrogate process things, tests, no more need for a
dyn
HyperligtVM, etc.), but figured I'll do that in separate PRs as this one is quite big already.