Skip to content
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

RFC: enable openhcl to run in VTL0, use nested virtualization #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jstarks
Copy link
Member

@jstarks jstarks commented Nov 8, 2024

This change enables OpenHCL to be launched in a non-VSM capable partition (i.e., in VTL0) and to use nested virtualization to run the target guest OS.

To achieve this, we build the existing virt_kvm backend into the paravisor openvmm build. So this has all the same limitations as virt_kvm has on the host.

With this, it becomes possible to do some basic OpenHCL testing on a Linux platform, with no Windows dependencies. Of course, since we are using nested virtualization, there are some limitations:

  • The host is not aware of the L2 guest, so it cannot offer vmbus or assigned devices directly to it. All devices must be emulated in the paravisor. There is no support for an interactive graphics console or anything else we haven't built a relay for yet.

  • Performance is pretty bad in my testing. Of course, I tested this with 3x nested virtualization, so that is not too surprising. I expect this to be much better on a native Linux host.

Currently, we still depend on the GET and other Hyper-V HCL devices, so this can really only be used in conjunction with openvmm on the host. Future changes can support alternate configuration mechanisms so that this would be able to run in qemu or another VMM.

Also, virt_whp and virt_hvf do not yet support nested virtualization (even though WHP and HVF do), so even with openvmm, this only works on Linux.

Finally, I only built and tested this on x64, because I do not have a KVM ARM64 environment that supports nested virtualization.

@jstarks jstarks requested review from a team as code owners November 8, 2024 20:50
@@ -54,11 +54,20 @@ pub enum IgvmManifestPath {
LocalOnlyCustom(PathBuf),
}

#[derive(Serialize, Deserialize, Clone, Debug)]
pub enum InitrdRootfsPath {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: here and elsewhere, I would suggest using the slightly more verbose name InitrdRootfsConfigPath, to make it marginally more clear we're talking about the rootfs.config files here.

it took me a sec to realize this wasn't related to the pre-packaged initrd layers themselves

dir /lib/modules/000 0755 0 0
dir /lib/modules/001 0755 0 0
dir /lib/modules/999 0755 0 0
dir /lib/modules/auto 0755 0 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the "auto" here for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything in auto will be automatically loaded. Modules elsewhere need explicit code to load.

I'll add a comment.

Comment on lines +8 to +9
//! it at some point. Right now, there are enough differences in requirements
//! that this is not practical.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what sort of differences are we talking about here?

} else if vendor.is_amd_compatible() {
Some("amd")
} else {
None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this have a louder error path?

@mattkur
Copy link
Contributor

mattkur commented Nov 9, 2024

You got an example of how one would run this?

@smmalis37
Copy link
Contributor

Can we add some sort of basic vmm test for this? I can work with you on petri & macro support for whatever's needed (haven't looked too in depth yet).

@jstarks
Copy link
Member Author

jstarks commented Nov 9, 2024

Can we add some sort of basic vmm test for this? I can work with you on petri & macro support for whatever's needed (haven't looked too in depth yet).

Yeah. Shouldn't be too bad. Linux x64 host only for now, until we add nested support to virt_whp.

@jstarks
Copy link
Member Author

jstarks commented Nov 9, 2024

You got an example of how one would run this?

I'll add one, once the kernel updates are in. But basically it's similar to how you run openhcl within openvmm today, except you don't pass --vtl2.

/// exit flowing through the paravisor.
///
/// This is best effort. Exits for this range may still flow through the paravisor.
fn register_host_io_port_fast_path(&self, range: RangeInclusive<u16>) -> Box<dyn Send>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this return value useful for?

@@ -20,6 +20,11 @@ openssl-vendored = ["underhill_attestation/openssl-vendored"]
# Enable VPCI device support
vpci = []

# Enable support for running the guest OS via nested virtualization with KVM.
# (Note that the virt_kvm crate is always a dependency just to avoid build
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a binary size concern?

}
}

impl OpenhclPartition for virt_kvm::KvmPartition {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth mentioning in a comment that this is not a shipping configuration, hence all the stubs?

@@ -292,6 +291,8 @@ pub struct UnderhillEnvCfg {
pub no_sidecar_hotplug: bool,
/// Enables the GDB stub for debugging the guest.
pub gdbstub: bool,
/// Use KVM instead of mshv_vtl.
pub kvm: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

enum Backend? Presumably we may have more than just mshv_vtl and kvm in the future?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could also allow carrying the ProtoPartition on an enum, instead of having to do all the if lets on it. Like it feels a little off to me that every branch where we don't have a proto partition assumes kvm, an enum could make the link clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants