[PATCH RESEND v2 0/6] target/arm: Add nested virtualization support

Haibo Xu posted 6 patches 2 years, 12 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/cover.1617281290.git.haibo.xu@linaro.org
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Thomas Huth <thuth@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Laurent Vivier <lvivier@redhat.com>
hw/arm/virt.c                      | 19 ++++++++---
hw/intc/arm_gicv3_common.c         |  1 +
hw/intc/arm_gicv3_kvm.c            | 16 +++++++++
include/hw/intc/arm_gicv3_common.h |  1 +
linux-headers/asm-arm64/kvm.h      |  2 ++
linux-headers/linux/kvm.h          |  1 +
target/arm/cpu.c                   | 14 +++++++-
target/arm/cpu.h                   |  4 +++
target/arm/cpu64.c                 | 53 ++++++++++++++++++++++++++++++
target/arm/kvm64.c                 | 15 +++++++++
target/arm/kvm_arm.h               | 13 ++++++++
target/arm/monitor.c               |  2 +-
tests/qtest/arm-cpu-features.c     |  9 +++++
13 files changed, 144 insertions(+), 6 deletions(-)
[PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Haibo Xu 2 years, 12 months ago
v2:
  - Move the NV to a CPU feature flag(Andrea&Andrew)
  - Add CPU feature 'el2' test(Andrew)

Many thanks to Andrea and Andrew for their comments!

This series add support for ARMv8.3/8.4 nested virtualization support
in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
has been tested on a FVP model to run a L2 guest with Qemu. Now the 
feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
starting a VM. 

[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/nv-5.12-WIP

Haibo Xu (6):
  Update linux header with new arm64 NV macro
  target/arm/kvm: Add helper to detect el2 when using KVM
  target/arm/kvm: Add an option to turn on/off el2 support
  hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ
  target/arm/cpu: Enable 'el2' to work with host/max cpu
  target/arm: Add vCPU feature 'el2' test.

 hw/arm/virt.c                      | 19 ++++++++---
 hw/intc/arm_gicv3_common.c         |  1 +
 hw/intc/arm_gicv3_kvm.c            | 16 +++++++++
 include/hw/intc/arm_gicv3_common.h |  1 +
 linux-headers/asm-arm64/kvm.h      |  2 ++
 linux-headers/linux/kvm.h          |  1 +
 target/arm/cpu.c                   | 14 +++++++-
 target/arm/cpu.h                   |  4 +++
 target/arm/cpu64.c                 | 53 ++++++++++++++++++++++++++++++
 target/arm/kvm64.c                 | 15 +++++++++
 target/arm/kvm_arm.h               | 13 ++++++++
 target/arm/monitor.c               |  2 +-
 tests/qtest/arm-cpu-features.c     |  9 +++++
 13 files changed, 144 insertions(+), 6 deletions(-)

-- 
2.17.1


Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Andrea Bolognani 2 years, 12 months ago
On Thu, 2021-04-01 at 12:55 +0000, Haibo Xu wrote:
> v2:
>   - Move the NV to a CPU feature flag(Andrea&Andrew)
>   - Add CPU feature 'el2' test(Andrew)
> 
> Many thanks to Andrea and Andrew for their comments!
> 
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
> has been tested on a FVP model to run a L2 guest with Qemu. Now the 
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM. 

This looks good from the user interface point of view, thanks for
addressing the concerns that were raised!

I'll leave Drew to review the actual code changes :)

-- 
Andrea Bolognani / Red Hat / Virtualization


Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Peter Maydell 2 years, 11 months ago
On Thu, 1 Apr 2021 at 13:55, Haibo Xu <haibo.xu@linaro.org> wrote:
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and
> has been tested on a FVP model to run a L2 guest with Qemu. Now the
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM.

Why are we making the UI for "enable EL2 guest with KVM" different
from that for "enable EL2 guest with TCG" ? Currently an EL2
TCG guest is set up with "-M virt,virtualization=on", which then
does everything it needs to enable virtualization on all the
components on the board including the CPU.

Unless there's a strong technical reason why KVM EL2 has to
be different, I think we should use the same switch.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Andrew Jones 2 years, 11 months ago
On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> On Thu, 1 Apr 2021 at 13:55, Haibo Xu <haibo.xu@linaro.org> wrote:
> > This series add support for ARMv8.3/8.4 nested virtualization support
> > in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and
> > has been tested on a FVP model to run a L2 guest with Qemu. Now the
> > feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> > starting a VM.
> 
> Why are we making the UI for "enable EL2 guest with KVM" different
> from that for "enable EL2 guest with TCG" ? Currently an EL2
> TCG guest is set up with "-M virt,virtualization=on", which then
> does everything it needs to enable virtualization on all the
> components on the board including the CPU.
> 
> Unless there's a strong technical reason why KVM EL2 has to
> be different, I think we should use the same switch.

I agree we should use the same switch, but I think I'd prefer it be the
CPU switch instead of the machine switch, as it's a CPU feature. There are
some board properties too, like the maintenance interrupt, but we tend to
call a feature a CPU feature when it shows up in the CPU manual, e.g. the
PMU is also a CPU feature, even though it has a PPI.

Thanks,
drew


Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Peter Maydell 2 years, 11 months ago
On Tue, 27 Apr 2021 at 10:55, Andrew Jones <drjones@redhat.com> wrote:
>
> On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> > Why are we making the UI for "enable EL2 guest with KVM" different
> > from that for "enable EL2 guest with TCG" ? Currently an EL2
> > TCG guest is set up with "-M virt,virtualization=on", which then
> > does everything it needs to enable virtualization on all the
> > components on the board including the CPU.
> >
> > Unless there's a strong technical reason why KVM EL2 has to
> > be different, I think we should use the same switch.
>
> I agree we should use the same switch, but I think I'd prefer it be the
> CPU switch instead of the machine switch, as it's a CPU feature. There are
> some board properties too, like the maintenance interrupt, but we tend to
> call a feature a CPU feature when it shows up in the CPU manual, e.g. the
> PMU is also a CPU feature, even though it has a PPI.

This is mostly not how we've generally opted to handle this kind of thing on
the virt board where there is something that is not merely a CPU feature
but also has effects on the wider system: look at 'virtualization',
'secure' and 'mte'. Granted, the effects of 'virtualization' on the wider
system are less significant than those of 'secure' or 'mte' -- but I think
we implemented 'virtualization' on the same pattern as 'secure'.

If you want to propose changing how we handle those things, including
a backward-compatibility setup so we don't break existing command lines,
I guess we can have that discussion. But we should either *first* do that
change-of-course and *then* implement KVM EL2 to fit into that, or we should
just implement KVM EL2 to fit into the design we have today (and then do
the redesign later if we decide to do that). I don't think we should add
KVM EL2 support's command line switches using a new design that we haven't
committed to and which leaves it completely out of line with what the TCG
handling of the exact same feature is. And I don't feel strongly enough
that our current approach is wrong that I want to impose a "first do this
big rework" precondition on landing the KVM EL2 support.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Andrew Jones 2 years, 11 months ago
On Tue, Apr 27, 2021 at 01:15:24PM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 10:55, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Tue, Apr 27, 2021 at 10:42:18AM +0100, Peter Maydell wrote:
> > > Why are we making the UI for "enable EL2 guest with KVM" different
> > > from that for "enable EL2 guest with TCG" ? Currently an EL2
> > > TCG guest is set up with "-M virt,virtualization=on", which then
> > > does everything it needs to enable virtualization on all the
> > > components on the board including the CPU.
> > >
> > > Unless there's a strong technical reason why KVM EL2 has to
> > > be different, I think we should use the same switch.
> >
> > I agree we should use the same switch, but I think I'd prefer it be the
> > CPU switch instead of the machine switch, as it's a CPU feature. There are
> > some board properties too, like the maintenance interrupt, but we tend to
> > call a feature a CPU feature when it shows up in the CPU manual, e.g. the
> > PMU is also a CPU feature, even though it has a PPI.
> 
> This is mostly not how we've generally opted to handle this kind of thing on
> the virt board where there is something that is not merely a CPU feature
> but also has effects on the wider system: look at 'virtualization',
> 'secure' and 'mte'. Granted, the effects of 'virtualization' on the wider
> system are less significant than those of 'secure' or 'mte' -- but I think
> we implemented 'virtualization' on the same pattern as 'secure'.
> 
> If you want to propose changing how we handle those things, including
> a backward-compatibility setup so we don't break existing command lines,
> I guess we can have that discussion. But we should either *first* do that
> change-of-course and *then* implement KVM EL2 to fit into that, or we should
> just implement KVM EL2 to fit into the design we have today (and then do
> the redesign later if we decide to do that). I don't think we should add
> KVM EL2 support's command line switches using a new design that we haven't
> committed to and which leaves it completely out of line with what the TCG
> handling of the exact same feature is. And I don't feel strongly enough
> that our current approach is wrong that I want to impose a "first do this
> big rework" precondition on landing the KVM EL2 support.
>

Since these types of features seem to blur the line between being a CPU
and board property, then I think I'd prefer we call them CPU properties,
as they come from the CPU manual.

Also, if we'd rather not rework 'virtualization' to be a CPU property,
then we'll need libvirt to learn how to probe and set it, whereas if
it's a CPU property we just need to add it to
cpu_model_advertised_features[].

Whichever way we go, we should commit to it, at least for the foreseeable
future, otherwise libvirt and users will have to flipflop their approaches
as well (I'm assuming there have been limited users of this feature
without KVM and libvirt support, so less users would flipflop now than
later.)

I suggest we rework 'virtualization' now with this KVM support series and
'mte' with the series that brings its KVM support too. 'secure' doesn't
currently work with KVM, so it can probably wait until its KVM support
series comes along to be reworked.

Thanks,
drew


Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Peter Maydell 2 years, 11 months ago
On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:

> Since these types of features seem to blur the line between being a CPU
> and board property, then I think I'd prefer we call them CPU properties,
> as they come from the CPU manual.

Conversely, I prefer to call them board properties, because that's
the way it works in hardware: the hardware board has the necessary
support for the system-level feature, and part of that is that it
has an SoC or CPU which has been configured to have the properties
that are needed for the board to support the feature. Having a CPU
that nominally supports a feature is useless if the system as a whole
doesn't handle it.

thanks
-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Peter Maydell 2 years, 11 months ago
On Tue, 27 Apr 2021 at 15:58, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
>
> > Since these types of features seem to blur the line between being a CPU
> > and board property, then I think I'd prefer we call them CPU properties,
> > as they come from the CPU manual.
>
> Conversely, I prefer to call them board properties, because that's
> the way it works in hardware: the hardware board has the necessary
> support for the system-level feature, and part of that is that it
> has an SoC or CPU which has been configured to have the properties
> that are needed for the board to support the feature. Having a CPU
> that nominally supports a feature is useless if the system as a whole
> doesn't handle it.

...this also means that we're consistent between boards: some board
models unconditionally have support for a feature (and always set it
on the CPU, GIC, etc), some don't ever support the feature (and always
disable it), and a few offer the user the choice. Having the user
use CPU properties suggests that they can, for instance, plug a
has-el3 CPU into any board model, which in general won't work.

-- PMM

Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Andrew Jones 2 years, 11 months ago
On Tue, Apr 27, 2021 at 04:01:10PM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 15:58, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Tue, 27 Apr 2021 at 15:48, Andrew Jones <drjones@redhat.com> wrote:
> >
> > > Since these types of features seem to blur the line between being a CPU
> > > and board property, then I think I'd prefer we call them CPU properties,
> > > as they come from the CPU manual.
> >
> > Conversely, I prefer to call them board properties, because that's
> > the way it works in hardware: the hardware board has the necessary
> > support for the system-level feature, and part of that is that it
> > has an SoC or CPU which has been configured to have the properties
> > that are needed for the board to support the feature. Having a CPU
> > that nominally supports a feature is useless if the system as a whole
> > doesn't handle it.
> 
> ...this also means that we're consistent between boards: some board
> models unconditionally have support for a feature (and always set it
> on the CPU, GIC, etc), some don't ever support the feature (and always
> disable it), and a few offer the user the choice. Having the user
> use CPU properties suggests that they can, for instance, plug a
> has-el3 CPU into any board model, which in general won't work.
>

I feel like this can be looked at both ways. A board can have a supporting
CPU or a CPU can have a supporting board. While a CPU is physically
mounted in a board, giving it a natural "physical member of" relationship
to the board, a board's design is driven by the features of the CPU, which
leads to the board having a "implements dependencies of" relationship to
the CPU.

I think the way we look at it depends on what we consider our top level
requirements to be. If it's a board specification that we're implementing,
then we clearly look at it from the board perspective. However, for mach-
virt, we don't have much of a board specification. We want it to be a
minimal board that supports VIRTIO and CPU features.

Maybe this is a place where our perspective and interface should diverge
between mach-virt and the emulated board models?

All that said, if we'd rather start thinking about system features, then
should we rework 'pmu' and perhaps other CPU features, which might better
be considered system features? Also, is the '-M virt,\?' type of probing
sufficient? Don't we need some sort of probing that considers both the
board support and the CPU support? 'virt' might support a system feature
that '-M virt -cpu xyz' does not support, right? How do users (libvirt)
know if they need to probe both the board and the CPU for feature support?
How do we probe the CPU's support for the feature if we don't want to
expose the feature as a CPU property? And, if we do expose the feature
as a CPU property, then what should be the response to enabling it without
the board property? Error out or imply its enablement when it's available?

I think we need some sort of system feature document to explain what a
system feature is and how it combines board properties together with CPU
features (which may or may not be exposed as properties). We'll need
to document how to properly do the probing and we'll also need tests to
check all our {board-property, cpu-type, cpu-property} misconfiguration
possibilities for proper error handling.

Thanks,
drew


Re: [PATCH RESEND v2 0/6] target/arm: Add nested virtualization support
Posted by Andrew Jones 2 years, 11 months ago
On Thu, Apr 01, 2021 at 05:55:32AM -0700, Haibo Xu wrote:
> v2:
>   - Move the NV to a CPU feature flag(Andrea&Andrew)
>   - Add CPU feature 'el2' test(Andrew)
> 
> Many thanks to Andrea and Andrew for their comments!
> 
> This series add support for ARMv8.3/8.4 nested virtualization support
> in KVM mode. It's based on Marc Zyngier's kernel KVM patches[1], and 
> has been tested on a FVP model to run a L2 guest with Qemu. Now the 
> feature can be enabled by "-M virt,accel=kvm -cpu host,el2=on" when
> starting a VM. 
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/nv-5.12-WIP
> 
> Haibo Xu (6):
>   Update linux header with new arm64 NV macro
>   target/arm/kvm: Add helper to detect el2 when using KVM
>   target/arm/kvm: Add an option to turn on/off el2 support
>   hw/intc/arm_gicv3: Enable support for setting vGIC maintenance IRQ
>   target/arm/cpu: Enable 'el2' to work with host/max cpu
>   target/arm: Add vCPU feature 'el2' test.

Hi Haibo,

Thank you for this new feature.

Please also update docs/system/arm/cpu-features.rst in the next version of
this patch series.

Thanks,
drew