Page MenuHomeFreeBSD

Table driven hypervisor detection and addition of VirtualBox.
ClosedPublic

Authored by stevek on Jul 17 2018, 7:32 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 1:31 AM
Unknown Object (File)
Tue, Nov 12, 1:25 AM
Unknown Object (File)
Fri, Nov 1, 9:47 AM
Unknown Object (File)
Mon, Oct 28, 1:21 PM
Unknown Object (File)
Oct 18 2024, 4:13 AM
Unknown Object (File)
Oct 8 2024, 7:11 PM
Unknown Object (File)
Oct 1 2024, 6:45 AM
Unknown Object (File)
Oct 1 2024, 5:56 AM

Details

Summary

Instead of individual conditional statements to look for each hypervisor
type, use a table to make it easier to add more in the future, if needed.

Add VirtualBox detection to the table ("VBoxVBoxVBox" is the hypervisor
vendor string to look for.) Also add VM_GUEST_VBOX to the VM_GUEST
enumeration to indicate VirtualBox.

Save the CPUID base for the hypervisor entry that we detected. Driver code
may need to know about it in order to obtain additional CPUID features.

Test Plan

Booted with qemu/KVM and VirtualBox and saw the hypervisors were correctly
detected. Would like additional testers for bhyve, VMware, Hyper-V, and Xen.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

sys/x86/x86/identcpu.c
1363 ↗(On Diff #45425)

It looks like this block of comments got copied above so I think this block can be removed.

Removed duplicate comment and replaced it with a more appropriate one
that explains if CPUID2_HV is set, we are running in a hypervisor environment.

bryanv added inline comments.
sys/x86/x86/identcpu.c
1267 ↗(On Diff #45560)

You copied this correctly from the if/else below but it appears the existing bhyve cpuid string is incorrect.

root@bhyvedev1:~ # uname -a
FreeBSD bhyvedev1 12.0-CURRENT FreeBSD 12.0-CURRENT #0 r335317: Mon Jun 18 16:21:17 UTC 2018 root@releng3.nyi.freebsd.org:/usr/obj/usr/src/amd64.amd64/sys/GENERIC amd64
root@bhyvedev1:~ # sysctl kern.vm_guest
kern.vm_guest: generic

It should be "bhyve bhyve " (note the trailing space) https://github.com/freebsd/freebsd/blob/master/sys/amd64/vmm/x86.c#L57.
Adding the space to this diff fixed bhyve detection.

bhyve detection was added in D11090 by @araujo . It is probably easiest to just fix it in this patch, assuming @stevek have plans to MFC this to 11.

sys/x86/x86/identcpu.c
1267 ↗(On Diff #45560)

@bryanv Do you mean instead to use "bhyve bhyve" we use "bhyvebhyve"?

sys/x86/x86/identcpu.c
1267 ↗(On Diff #45560)

I think would be great to fix that in this patch too and then MFC it to 11 if possible.

Fixed bhyve detection string, as pointed out by bryanv

There is a subtle change here you aren't documenting in the commit message which is that you are now iterating over the list of possible leafs to find a "compatible" hypervisor in case we don't recognize the "real" hypervisor. I think that's actually a fairly major change. If we don't find a hypervisor we recognize we should probably expose the "initial" string in hv_vendor? (I suggested a way to do that above).

sys/x86/x86/identcpu.c
1303 ↗(On Diff #45843)

Perhaps add a comment here in the function or as part of the first block comment above the for to note that we are searching for the first hypervisor we recognize.

1331 ↗(On Diff #45843)

This should be 'regs[0] >= leaf' I think.

1346 ↗(On Diff #45843)

I feel like we should always expose this string, not just known strings. Perhaps we should alter the if to:

if (vm_guest != VM_GUEST_VM || leaf == 0x40000000)

This would always store the first one in hv_vendor[] on the first pass and then only overwrite it if we later find a matching hypervisor.

sys/x86/x86/identcpu.c
1303 ↗(On Diff #45843)

Sure, makes sense, I'll update that.

1331 ↗(On Diff #45843)

Yes, I think you are correct here. I'll change that.

1346 ↗(On Diff #45843)

Okay, I can see that.

Addressed review comments - added additional comments and save the first
hypervisor we found so we have some information even if we cannot find an
exact match.

This change is fine as-is. It (now) occurs to me that what we may really want eventually is to change code that is currently using 'hv_base' and 'vm_guest' directly to check for certain features to instead do a search if a desired VMM is found. That is, suppose I want to check for KVM clock, maybe what I really want is something like this:

int base, high;

if (find_hypervisor_leaf(VM_GUEST_KVM, &base, &high)) {
    /* Look for KVM feature using CPUID nodes at [base, high]. */
}

This would mean that we could always kind KVM clock no matter what the "first" leaf advertised was.

This revision is now accepted and ready to land.Oct 22 2018, 7:42 PM
This revision was automatically updated to reflect the committed changes.