Adding virtualization support for ARMv7 platforms

[mihai]

Summary

This commit is adding virtualization support to ARMv7 platforms. It is composed of multiple modules:

• kernel module vmm-arm - implements the kernel-side of the hypervisor
• user space library libvmmapiarm - interface to the kernel module for userspace utilities
• user space utility bhyveloadarm - the VM boot-loader
• user space utility bhyvearm - the VM running loop

The VMM interface was preserved as much as we could compared to the x86 one. In the future, we can merge them and create one generic API for the VMM module.

Test Plan

The code was tested on FVP Cortex A15 emulator (with 1 processor) and test is in progress on real hardware (Cubieboard2 - AllwinnerA20).

The sys/arm/vmm/vgic.c is still in progress (there is a glitch in restoring the distributor state).

[andrew]

I've done a quick pass through the code. You have quite a few long lines in this patch, you should put function names on a new line, and there are a few debugging printf calls that may not be appropriate in svn.

sys/arm/arm/bitops.c

1 ↗

(On Diff #26860)

Is this file needed? We already have setbit, clrbit, isset, and isclr in sys/param.h, and more bit manipulation in sys/bitset.h.

sys/arm/conf/GENERIC

26 ↗

(On Diff #26860)

Will this break running on Cortex-A8 or A9?

sys/arm/vmm/arm.c

142

This comment doesn't make sense.

252

What if we don't have a SP804 or it's not at this address?

Would it be better to use the ARM generic timer? It's designed for this & should be available when the CPU supports the virtualisation extensions.

sys/arm/vmm/hyp.S

56

Can you add these (here and below) to sys/arm/include/sysreg.h then use the macro.

sys/arm/vmm/hyp_genassym.c

116–145

I don't think we need these blank lines

sys/arm/vmm/vgic.c

928–932

These should be moved into the gic driver & __BUS_ACCESSOR functions should be added to read them.

942–943

Is this needed? The handler doesn't seem to do anything useful.

sys/arm/vmm/vmm_stat.c

144–149

Some of these look like they're not applicable on ARM

sys/arm/vmm/vmm_stat.h

37–38

I don't think these apply

[mihai]

I've put some questions into the comments. As soon as I get answers to those, I will solve them and UPDATE the commit diff.

sys/arm/conf/GENERIC

26 ↗

(On Diff #26860)

Yes this will break the others. If I don't put the mcpu flag, the compiler doesn't recognize the virtualization instruction. Where should we include these? Or what other flag should I use in order for the compiler to recognize the virtualization instructions?

sys/arm/vmm/vgic.c

928–932

Can you give me an example of best practice or how should I do this?

942–943

The handler only takes the interrupt. In the VM enter/exit logic we verify the GICH_MISR to see if there was any maintenance interrupt and act accordingly.

sys/arm/vmm/vmm_stat.c

144–149

The reason we preserve all flags is that at a future time the VMM interfaces will merge and then will be easy to aggregate them. I will remove them from this commit.

[emaste]

lib/Makefile

177–180

why do we have a separate libvmmapiarm, not just libvmmapi that has the appropriate content per arch?

[mihai]

lib/Makefile

177–180

As stated in the description above, VMM API contained some x86 specific parameters and could not be used as it was for ARM. It needed some modifications. At the time of development it was very hard to ensure that we didn't brake anything on x86 and make the interface compatible with ARM. After some talks with Peter Grehan, we came to the conclusion that we need to duplicate the whole VMM infrastructure (bhyve, libvmmapi, bhyveload, vmm) for ARM and preserve as much as we can from the x86 interface and as a separate project we will merge the VMM API. If it's not an acceptable approach we can rethink the steps before merging.

[grehan]

lib/Makefile

177–180

I think the merge is mandatory before this could be committed to CURRENT. However, it's probably a fine candidate for a branch.

[andrew]

sys/arm/conf/GENERIC

26 ↗

(On Diff #26860)

Which files? In .S files you should use .arch_extension virt, for .c files you will need to create a wrapper file build with the needed cflags in files.arm and the module Makefile.

sys/arm/vmm/vgic.c

928–932

Add new ivars to arm_gic_read_ivar in gic.c, then add a __BUS_ACCESSOR macro to gic_common.h. There are two examples there already. These will create static inline functions, so the hw_rev accessor creates gic_get_hw_rev and gic_set_hw_rev. The former returns a u_int, the latter takes one in, however as there is no ivar write function it doesn't write anything.

[mihai]

This new version of the patch fixes comments from Andrew Turner and Peter Grehan:

• create platform dependent code for bhyve (no more bhyvearm/libvmmapiarm duplicate directory)
• create a device for the vgic in order to user the getter methods from the gic device
• added timer virtualization
• completed vGIC virtualization

Please note that we DID NOT include any configuration files for the CUBIE2 or FVP (Fixed Virtual Platform) emulator. If you want to test bhyvearm you need those too. Please use the github repo [1]. We switched to github because is more easy to do code management.

bhyvearm works at this moment. The FreeBSD virtual machines has a shell console, but is moving slowly due to some issues with the timer interrupts.

Please review and propose what would be the next steps. It's a huge code base and it gets harder to maintain externally.

[1] https://github.com/FreeBSD-UPB/freebsd/tree/projects/bhyvearm

[andrew]

Can you pull out the sys/arm/arm/gic* and sys/arm/arm/generic_timer.c changes into two reviews? They are mostly independent of the rest of the review.

sys/arm/arm/generic_timer.c

341–350

This won't work. The softc hasn't been initialised at this point.

sys/arm/vmm/hyp.S

110

Can you create a new review to add all the coprocessor registers to sys/arm/include/sysreg.h, then use the macros here? Making it as a new review will mean I can just commit the sysreg.h parts.

[mihai]

I've created three new patches containing:

• sysreg bits
• generic_timer bits
• gic bits.

Please review them.

[emaste]

For reference, D11846 D11847 D11848

[wblock]

sys/arm/arm/gic.c

146

There is a tab after a space here, after SYS_RES_IRQ,

sys/arm/include/bitops.h

32

Tab after space here. There are some others, too. Install textproc/igor and run igor -R * | less -RS to find these and other errors.

[mihai]

Any update on this?

Projects like bhyve-armv8 and bhyve-arm-virtio are approaching to the end and all these are prerequisites for them.

[emaste]

I'd suggest we commit this incrementally; I'm looking at the MD/MI movement atm.

I'm curious about changes to amd64 subdir, like usr.sbin/bhyve/amd64/virtio.c - I would expect functionality like this to be MI?

[mihai]

Can you extend what means MD/MI?

Also let's not discuss about virtio.c here. That is another commit that will be done.

[emaste]

MI - Machine Independent
MD - Machine Dependent (i.e., architecture-specific)

Wrt virtio, it was just an example, I mean in general changes like this from this review: usr.sbin/bhyve/{ → amd64}/virtio.c. I'm curious why these files would become amd64-specific.

[mihai]

We don't have any implementation for them in arm and most of them have some x86 specific bits (like registers). This is why we choose to only make what we have machine independent. And when new features appear for ARM we would take care to make them as general as possible (e.g. virtio.c -> which is WIP)

[mmacy]

@mihai Before this can proceed this review needs to be broken up in to at _least_ 3 pieces

1. MI/MD refactoring of sys/amd64/vmm - you need to identify all the common interfaces that are shared and move them out of amd64, this means you need to define a clean shared API where possible, e.g. starting / stopping guest / setting nvcpus / etc

2. MI/MD refactoring of libvmmapi

3. adding ARM specific changes.
   
   Note that drivers that you're not using do _not_ get moved to being architecture specific. All of the virtio drivers are arch independent. If you don't support a driver yet, or don't plan to just don't compile it in. You also don't have good grounds for moving usr.sbin/bhyve wholesale in to something arch specific. I expect there to need to be refactoring, but that should be largely isolated to not compiling things like PCI emulation and changes to libvmmapi.

[mihai]

In D10213#323447, @mmacy wrote:

@mihai Before this can proceed this review needs to be broken up in to at _least_ 3 pieces

1. MI/MD refactoring of sys/amd64/vmm - you need to identify all the common interfaces that are shared and move them out of amd64, this means you need to define a clean shared API where possible, e.g. starting / stopping guest / setting nvcpus / etc

2. MI/MD refactoring of libvmmapi

We tried to do this before, but we analyzed the interfaces, we saw that they are x86 dependent (memseg set, etc). We would end up with a common interface only for starting/stopping/running.

My proposal was to accept these changes like this, to be in different platforms and another project would merge these.

If you all want to go this way, I will ask Darius to create a review only with MD/MI interface. Currently we are having conflicts with the rebasing anyway.

3. adding ARM specific changes.
   
   Note that drivers that you're not using do _not_ get moved to being architecture specific. All of the virtio drivers are arch independent. If you don't support a driver yet, or don't plan to just don't compile it in. You also don't have good grounds for moving usr.sbin/bhyve wholesale in to something arch specific. I expect there to need to be refactoring, but that should be largely isolated to not compiling things like PCI emulation and changes to libvmmapi.

Drivers (e.g. virtio) are another work that are not yet at review. They contain the refactoring you are talking about.

In usr.sbin/bhyve there is a lot of x86 dependent code and logic and this is why we moved it into x86.