Page MenuHomeFreeBSD

Table driven hypervisor detection and addition of VirtualBox.
ClosedPublic

Authored by stevek on Jul 17 2018, 7:32 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

stevek created this revision.Jul 17 2018, 7:32 PM
bryanv added inline comments.Jul 19 2018, 1:49 PM
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.

stevek updated this revision to Diff 45560.Jul 19 2018, 8:50 PM

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 a subscriber: araujo.Jul 23 2018, 4:27 AM
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.

araujo added inline comments.Jul 23 2018, 6:24 AM
sys/x86/x86/identcpu.c
1267 ↗(On Diff #45560)

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

araujo added inline comments.Jul 23 2018, 6:27 AM
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.

swills added a subscriber: swills.Jul 24 2018, 2:27 PM
stevek updated this revision to Diff 45843.Jul 25 2018, 6:09 PM

Fixed bhyve detection string, as pointed out by bryanv

jhb added a comment.Jul 25 2018, 6:32 PM

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.

stevek added inline comments.Jul 31 2018, 8:39 PM
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.

stevek updated this revision to Diff 49324.Oct 19 2018, 11:00 PM

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.

jhb accepted this revision.Oct 22 2018, 7:42 PM

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
bryanv accepted this revision.Oct 22 2018, 7:57 PM
This revision was automatically updated to reflect the committed changes.