Adding virtualization support for ARMv7 platforms
Needs ReviewPublic

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

Details

Reviewers
andrew
grehan
Group Reviewers
ARM
manpages
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
Lint
Lint Skipped
Unit
Unit Tests Skipped
mihai created this revision.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
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.

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

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

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.

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