Skip to content

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Refactor Vm trait #517

wants to merge 2 commits into from

Conversation

ludfjig
Copy link
Contributor

@ludfjig ludfjig commented May 21, 2025

This PR introduces a minimal Vm trait. It's implemented by KvmVm, MshvVm and WhpVm. The goal is to reduce code duplication. The existing Hypervisor trait is renamed to HyperlightVm, and is now only implemented by HyperlightSandbox (and Inprocess, but not for long since we're getting rid of inproces mode). In addition, the Vm 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.

@ludfjig ludfjig added kind/refactor For PRs that restructure or remove code without adding new functionality. area/API Related to the API or public interface labels May 21, 2025
…ll drivers. Remove duplicated GDB code.

Signed-off-by: Ludvig Liljenberg <[email protected]>
@ludfjig ludfjig force-pushed the vm_trait2 branch 3 times, most recently from e697c0f to c18f0f1 Compare May 21, 2025 06:01
danbugs
danbugs previously approved these changes May 21, 2025
Copy link
Contributor

@danbugs danbugs left a 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(())
}
}
}
Copy link
Contributor

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?

Copy link
Contributor Author

@ludfjig ludfjig May 21, 2025

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

Copy link
Contributor

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)?;

Copy link
Contributor

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 HyperlightExits 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.

Copy link
Contributor Author

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

Copy link
Contributor

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))]
Copy link
Contributor

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 :)

Copy link
Contributor Author

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]>
Copy link
Contributor

@simongdavies simongdavies left a 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 {
Copy link
Contributor

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 {
Copy link
Contributor

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(())
}
}
}
Copy link
Contributor

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 {
Copy link
Contributor

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 {
Copy link
Contributor

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 @@
/*
Copy link
Contributor

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

@dblnz
Copy link
Contributor

dblnz commented Jun 3, 2025

Do you plan on updating this after the hypervisor_handler change?
I have partially reviewed the previous draft of this #469 .
Let me know when to have a look on this

@ludfjig
Copy link
Contributor Author

ludfjig commented Jun 4, 2025

Do you plan on updating this after the hypervisor_handler change? I have partially reviewed the previous draft of this #469 . Let me know when to have a look on this

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

@ludfjig ludfjig marked this pull request as draft June 6, 2025 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/API Related to the API or public interface kind/refactor For PRs that restructure or remove code without adding new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid reopening /dev/kvm or /dev/mshv for every new sandbox Rethink driver API Unify register representations across all Hypervisor implementations
4 participants