Paths

Table of Contentst

Adding virtualization support for ARMv7 platforms
Needs ReviewPublic
Actions

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

Details

Reviewers

andrew
grehan
rgrimes
mmacy
tychon

Group Reviewers

ARM
manpages
bhyve

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

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

mihai created this revision.Mar 31 2017, 9:10 AM

Herald added subscribers: andrew, imp. · View Herald TranscriptMar 31 2017, 9:10 AM

mihai added a reviewer: andrew.Mar 31 2017, 9:10 AM

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

loos added a subscriber: loos.Mar 31 2017, 6:25 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.

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.

mihai added a reviewer: grehan.Apr 11 2017, 6:58 PM

emaste added a subscriber: emaste.Apr 13 2017, 5:49 PM

emaste added inline comments.Apr 13 2017, 6:02 PM

lib/Makefile
177–180	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

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

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

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 added subscribers: alexnivan_gmail.com, alexandru.elisei_gmail.com, darius.mihaim_gmail.com.Jul 22 2017, 7:21 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.

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

Herald added a reviewer: manpages. · View Herald TranscriptJul 22 2017, 7:28 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.

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.

I've created three new patches containing:

sysreg bits
generic_timer bits
gic bits.

Please review them.

For reference, D11846 D11847 D11848

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

wblock added inline comments.

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.

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.

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)

mgrooms_shrew.net added a subscriber: mgrooms_shrew.net.Mar 23 2018, 2:43 PM

Herald added a reviewer: rgrimes. · View Herald TranscriptMar 23 2018, 2:43 PM

rgrimes added a reviewer: bhyve.Mar 23 2018, 3:10 PM

olevole_olevole.ru added a subscriber: olevole_olevole.ru.Apr 18 2018, 10:25 AM

mmacy added reviewers: mmacy, tychon.May 8 2018, 7:21 AM

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

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

MI/MD refactoring of libvmmapi

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.

In D10213#323447, @mmacy wrote:

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

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

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.

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.

araujo added a subscriber: araujo.Oct 5 2018, 9:15 AM