Page MenuHomeFreeBSD

libvmmapi: Separate MI from MD code
Needs ReviewPublic

Authored by alexandru.elisei_gmail.com on Nov 6 2018, 7:20 PM.

Details

Reviewers
rgrimes
Group Reviewers
bhyve
Summary

Separate machine-independent from machine-dependent code in libvmmapi to prepare for architectures other than x86.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

alexandru.elisei_gmail.com marked 2 inline comments as done.Nov 6 2018, 7:28 PM
alexandru.elisei_gmail.com added inline comments.
lib/libvmmapi/amd64/vmmapi_amd64.c
747

Why doesn't it use a system call? That way it could be treated like a machine-independent function.

Nitpicking: it doesn't follow the same naming convention as all the other functions in libvmmapi (doesn't have a vm_ prefix).

lib/libvmmapi/vmmapi.h
59

struct vmctx memory representation is very x86-centric. There is no lowmem on arm64, no highmem on arm. Because of this I have added a MD member of type struct vmmem to struct vmctx.

This means that now struct vmctx is public, instead of being opaque. One other approach would be to keep struct vmctx opaque and have each architecture define it, but that makes all functions that access struct vmctx (like vm_get_device_fd) machine-dependent.

araujo added a reviewer: bhyve.Nov 7 2018, 1:51 AM

A M lib/libvmmapi/amd64/vmmapi_amd64.h (143 lines)
A M lib/libvmmapi/amd64/vmmapi_amd64.c (1035 lines)
These 2 files are showing up as "added" when clearly they are copied/moved from some place else. This may be an artifact of either git or phabricator and hopefully the svn commit shows the proper logic.

V lib/libvmmapi/{amd64 ← }/vmmapi_freebsd.c
This file shows up as a move, rename properly (rename in same directory?)

I stopped my review as it appears that much of the diff is caused by re-ordering and/or white space changes to the code making it very difficult to sort out what is functional change and what is simply diff noise. If we need to do some code clean up lets do that in place to the current code in a seperate review.

lib/libvmmapi/Makefile
14

Please put the SRCS, INCS, WARNS, LIBADD, CFLAGS back in the original order so that the diff is minimized and made clear as to what is or is not changing. White space and line ordering changes should almost always be done in seperate commits and doing them in a code change review increase the work load on the reviewers trying to sort out what is just noise and what is actual changes.

lib/libvmmapi/vmmapi.h
69

I am unclear why this block was being moved around in the file?

araujo requested changes to this revision.Nov 7 2018, 6:35 AM
araujo added a subscriber: araujo.

Also there are lots of style(9) issues!
@alexandru.elisei_gmail.com could you please first address @rgrimes comment and then we can do the review including the style(9) issues that need to be fixed too?

Tks,

This revision now requires changes to proceed.Nov 7 2018, 6:35 AM
alexandru.elisei_gmail.com edited the summary of this revision. (Show Details)

I've rewritten the patch to make the diff readable.

alexandru.elisei_gmail.com marked 2 inline comments as done.Nov 20 2018, 9:16 PM

The only test I did for the patch was to compile the source code. When the patch code is stabilized I will try to do a proper test by running a virtual machine with bhyve.

lib/libvmmapi/amd64/vmmapi_amd64.h
124

I did a grep on the entire FreeBSD source code and in the ports directory. There are two consumers for this function: usr.sbin/bhyve/bhyverun.c and usr.sbin/bhyve/spinup_ap.c.

After this patch gets merged, I will change the name to vm_vcpu_reset for consistency with the rest of the API and move the prototype to lib/libvmmapi/vmmapi.h, which will make the prototype machine independent, but the implementation machine dependent.

lib/libvmmapi/vmmapi.h
146–147

struct vmctx is now machine dependent, and I needed an accessor for the VM name to be used by vm_destroy().

The only test I did for the patch was to compile the source code. When the patch code is stabilized I will try to do a proper test by running a virtual machine with bhyve.

How we gonna approve something that was not tested? Smoke test is not a real test!
I would suggest you have in your test bed at least 3 different guest flavors: FreeBSD, Linux and Windows.

Also, I do believe you will need to make different tests with different devices, just to assure that nothing is broken.

Best,

araujo added inline comments.Nov 20 2018, 10:02 PM
lib/libvmmapi/amd64/vmmapi_amd64.h
124

OK, this seems reasonable to don't mix two different changes.

Tested using the following guests:

  1. FreeBSD without UEFI - installation and then booting from disk file.
  2. Fedora 29 Workstation with grub2-bhyve - installation and then booting from disk file.
  3. FreeBSD with UEFI - installation and then booting from disk file.
  4. Fedora 29 Workstation with UEFI - installation and then booting from disk file.

Commands used for testing can be found at github.

Not tested:

  1. Windows guest - I don't have an iso image for Windows.
  2. PCI passthrough - not possible on my laptop.

Tested using the following guests:

  1. FreeBSD without UEFI - installation and then booting from disk file.
  2. Fedora 29 Workstation with grub2-bhyve - installation and then booting from disk file.
  3. FreeBSD with UEFI - installation and then booting from disk file.
  4. Fedora 29 Workstation with UEFI - installation and then booting from disk file.

Commands used for testing can be found at github.
Not tested:

  1. Windows guest - I don't have an iso image for Windows.
  2. PCI passthrough - not possible on my laptop.

Hi,

For the Windows guest, you can download a trial ISO that works for 90 days.
Usually I test Windows using these ISOs and it works great.

Tested using the following guests:

  1. FreeBSD without UEFI - installation and then booting from disk file.
  2. Fedora 29 Workstation with grub2-bhyve - installation and then booting from disk file.
  3. FreeBSD with UEFI - installation and then booting from disk file.
  4. Fedora 29 Workstation with UEFI - installation and then booting from disk file.

Commands used for testing can be found at github.
Not tested:

  1. Windows guest - I don't have an iso image for Windows.
  2. PCI passthrough - not possible on my laptop.

Hi,
For the Windows guest, you can download a trial ISO that works for 90 days.
Usually I test Windows using these ISOs and it works great.

One more question, did you test it with AMD and Intel CPUs?
I can help to test with Intel, unfortunately my Ryzen is gone.

One more question, did you test it with AMD and Intel CPUs?
I can help to test with Intel, unfortunately my Ryzen is gone.

At the moment I only have access to Intel CPUs.

jhb added a subscriber: jhb.Dec 11 2018, 5:50 PM
jhb added inline comments.
lib/libvmmapi/Makefile
19

I think it would be fine to just use the ARCH_SUBDIR variants needed for arm, arm64, and amd64 for now. Does MACHINE_ARCH work for all of those?

lib/libvmmapi/amd64/vmmapi_amd64.c
83

This function doesn't seem MD. It doesn't need to be in vmmapi.h, but it could be in an internal MI header in lib/libvmmapi.

lib/libvmmapi/amd64/vmmapi_amd64.h
32

I would still put guards. We still put guards on nested headers.

lib/libvmmapi/vmmapi.h
146–147

We could still expose 'struct vmctx' in the library sources itself via an internal header and then you wouldn't need the accessor functions for 'fd' and 'name' for use in the library. If consumers of the library need the accessors we can still add them, but if they aren't currently needed we can wait to add them until they are needed.

148

I would put the nested include at the top of the file just after the include guards as that is the normal style for nested includes in FreeBSD.

usr.sbin/bhyve/Makefile
6

You shouldn't need this change. We might need to ensure that vmmapi_amd64.h gets installed to /usr/include instead, but this change shouldn't be needed. (likewise for the other binary Makefiles)

alexandru.elisei_gmail.com marked 6 inline comments as done.Dec 12 2018, 7:35 PM
alexandru.elisei_gmail.com added inline comments.
lib/libvmmapi/Makefile
19

After looking at Makefile.inc1, MACHINE_ARCH can be i386 or amd64 when MACHINE is amd64, or aarch64 when MACHINE is arm64.

I'll try to use MACHINE instead.

lib/libvmmapi/amd64/vmmapi_amd64.c
83

I'll can do that.

lib/libvmmapi/amd64/vmmapi_amd64.h
32

Sure, I'll do that.

lib/libvmmapi/vmmapi.h
146–147

get_device_fd() was already present in vmmapi.h. My changes were to add vm_get_name() and use get_device_fd() instead of accessing the fd directly when doing an ioctl.

struct vmctx is needs to be MD because the memory representation is amd64 centric. I will add a internal header file in the lib/vmmapi/amd64/ directory and I'll include it vmmapi.c.

148

Sure, I'll do that.

usr.sbin/bhyve/Makefile
6

This is needed in for compiling the bhyveload/bhyve/bhyvectl binaries. When the libvmmapi.h header is included, that path is needed for finding /lib/libvmmapi/amd64/libvmmapi_amd64.h.

Without that part I get this error when compiling:

In file included from /path/to/freebsd/usr.sbin/bhyveload/bhyveload.c:86:
/path/to/objdirprefix/path/to/freebsd/amd64.amd64/tmp/usr/include/vmmapi.h:146:10: fatal error: 'amd64/vmmapi_amd64.h' file not found
#include <amd64/vmmapi_amd64.h>

Changes:

  • vmmapi.h: Include MD header at the top of the file
  • amd64/vmmapi_amd64.h: Add header guards
  • Makefile: Use MACHINE for the MD directory name
  • Add vmmapi_internal.{c, h} for internal MI functions
  • Add amd64 internal header with struct vmctx definition
alexandru.elisei_gmail.com marked an inline comment as done.Dec 12 2018, 7:50 PM
alexandru.elisei_gmail.com added inline comments.
lib/libvmmapi/amd64/vmctx_amd64.h
1

I am not really sure about the name - maybe something like amd64_internal.h is better?

araujo removed a reviewer: araujo.Feb 22 2019, 1:20 AM
araujo removed a subscriber: araujo.