Adding virtualization support for ARMv7 platforms
Needs ReviewPublic

Authored by mihai on Mar 31 2017, 9:10 AM.


Group Reviewers

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).

Diff Detail

rS FreeBSD src repository
Lint Skipped
Unit Tests Skipped
mihai created this revision.Mar 31 2017, 9:10 AM
andrew edited edge metadata.Mar 31 2017, 2:25 PM

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.

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.

26 ↗(On Diff #26860)

Will this break running on Cortex-A8 or A9?


This comment doesn't make sense.


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.


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


I don't think we need these blank lines


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


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


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


I don't think these apply

loos added a subscriber: loos.Mar 31 2017, 6:25 PM
mihai added a comment.Apr 11 2017, 6:57 PM

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.

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?


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


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.


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 added a subscriber: emaste.Apr 13 2017, 5:49 PM
emaste added inline comments.Apr 13 2017, 6:02 PM

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

mihai added inline comments.Apr 13 2017, 8:31 PM

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 added inline comments.Apr 13 2017, 9:39 PM

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

andrew added inline comments.Apr 19 2017, 10:08 AM
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.


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 updated this revision to Diff 31086.Jul 22 2017, 7:28 PM

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.


andrew added a comment.Aug 2 2017, 4:19 PM

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.


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


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 added a comment.Aug 3 2017, 3:06 PM

I've created three new patches containing:

  • sysreg bits
  • generic_timer bits
  • gic bits.

Please review them.

wblock added a subscriber: wblock.Aug 25 2017, 1:38 PM
wblock added inline comments.

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


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 added a comment.Fri, Feb 23, 9:15 AM

Any update on this?

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

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?

Can you extend what means MD/MI?

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

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 added a comment.Mon, Feb 26, 8:24 AM

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)