Re: [PATCH v3 4/4] spapr: Forbid nested KVM-HV in pre-power9 compat mode

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 3749 bytes --]

On Mon, 15 Jun 2020 11:20:31 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Sat, 13 Jun 2020 17:18:04 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Thu, Jun 11, 2020 at 03:40:33PM +0200, Greg Kurz wrote:
> > > Nested KVM-HV only works on POWER9.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> > 
> > Hrm.  I have mixed feelings about this.  It does bring forward an
> > error that we'd otherwise only discover when we try to load the kvm
> > module in the guest.
> > 
> > On the other hand, it's kind of a layering violation - really it's
> > KVM's business to report what it can and can't do, rather than having
> > qemu anticipate it.
> > 
> 
> Agreed and it seems that we can probably get KVM to report that
> already. I'll have closer look.
> 

Checking the KVM_CAP_PPC_NESTED_HV extension only reports what the host
supports. It can't reasonably take into account that we're going to
switch vCPUs in some compat mode later on. KVM could possibly check
that it has a vCPU in pre-power9 compat mode when we try to enable
the capability and fail... but it would be a layering violation all
the same. The KVM that doesn't like pre-power9 CPUs isn't the one in
the host, it is the one in the guest, and it's not even directly
related to the CPU type but to the MMU mode currently in use:

long kvmhv_nested_init(void)
{
	long int ptb_order;
	unsigned long ptcr;
	long rc;

	if (!kvmhv_on_pseries())
		return 0;
==>	if (!radix_enabled())
		return -ENODEV;

We cannot know either for sure the MMU mode the guest will run in
when we enable the nested cap during the initial machine reset.
So it seems we cannot do anything better than denylisting well
known broken setups, in which case QEMU seems a better fit than
KVM.

Makes sense ?

> > Allowing POWER8 compat for an L2 is something we hope to have in the
> > fairly near future.
> 
> Ok but I guess we don't want to start an L2 in compat POWER8 mode
> with cap-nested-hv=on, do we ?
> 
> >  Allowing POWER8 compat for L1, which is what this
> > covers, is, I'll admit, likely to never happen.
> > 
> > 
> > > ---
> > >  HW/ppc/spapr_caps.c |   10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > index 27cf2b38af27..dfe3b419daaa 100644
> > > --- a/hw/ppc/spapr_caps.c
> > > +++ b/hw/ppc/spapr_caps.c
> > > @@ -391,6 +391,8 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> > >                                      uint8_t val, Error **errp)
> > >  {
> > >      ERRP_AUTO_PROPAGATE();
> > > +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > +
> > >      if (!val) {
> > >          /* capability disabled by default */
> > >          return;
> > > @@ -400,6 +402,14 @@ static void cap_nested_kvm_hv_apply(SpaprMachineState *spapr,
> > >          error_setg(errp, "No Nested KVM-HV support in TCG");
> > >          error_append_hint(errp, "Try appending -machine cap-nested-hv=off\n");
> > >      } else if (kvm_enabled()) {
> > > +        if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_3_00, 0,
> > > +                              spapr->max_compat_pvr)) {
> > > +            error_setg(errp, "Nested KVM-HV only supported on POWER9");
> > > +            error_append_hint(errp,
> > > +                              "Try appending -machine max-cpu-compat=power9\n");
> > > +            return;
> > > +        }
> > > +
> > >          if (!kvmppc_has_cap_nested_kvm_hv()) {
> > >              error_setg(errp,
> > >                         "KVM implementation does not support Nested KVM-HV");
> > > 
> > > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]