Re: [Qemu-devel] [PATCH for-2.11.2] spapr: make pseries-2.11 the default machine type

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Jun 19, 2018 at 01:11:28PM +0200, Greg Kurz wrote:
> On Mon, 18 Jun 2018 21:04:38 -0500
> Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 
> > Quoting Greg Kurz (2018-05-22 12:17:28)
> > > The spapr capability framework was introduced in QEMU 2.12. It allows
> > > to have an explicit control on how host features are exposed to the
> > > guest. This is especially needed to handle migration between hetero-
> > > geneous hosts (eg, POWER8 to POWER9). It is also used to expose fixes/
> > > workarounds against speculative execution vulnerabilities to guests.
> > > The framework was hence backported to QEMU 2.11.1, especially these
> > > commits:
> > > 
> > > 0fac4aa93074 spapr: Add pseries-2.12 machine type
> > > 9070f408f491 spapr: Treat Hardware Transactional Memory (HTM) as an
> > >  optional capability
> > > 
> > > 0fac4aa93074 has the confusing effect of making pseries-2.12 the default
> > > machine type for QEMU 2.11.1, instead of the expected pseries-2.11. This
> > > patch changes the default machine back to pseries-2.11.
> > > 
> > > Unfortunately, 9070f408f491 enforces the HTM capability for pseries-2.11.
> > > This isn't supported by TCG and breaks 'make check'. So this patch also
> > > adds a hack to turn HTM off when using TCG.  
> > 
> > I noticed this ends up breaking TCG migration for 2.11.2 -> 2.12, I
> > get this on the target side even when specifying -machine
> > pseries-2.11,cap-htm=off for both ends:
> > 
> >   qemu-system-ppc64: cap-htm higher level (1) in incoming stream than on destination (0)
> >   qemu-system-ppc64: error while loading state for instance 0x0 of device 'spapr'
> >   qemu-system-ppc64: load of migration failed: Invalid argument
> > 
> > I'm not sure we care all that much about it but it's a regression from 2.11.1
> > at least. The main issue seems to be the default caps for 2.11.2 for TCG are
> > now different from 2.11 and 2.12+, but spapr_cap_##cap##_needed still assumes
> > everything is the same across all these versions and as such opts not to
> > migrate cap-htm=off, since that's the default for 2.11.2. This results in the
> > target assuming the source was using the default, which is cap-htm=on,
> > and since that disagrees with the spapr->eff we get a failure.
> > 
> > It seems spapr_cap_##cap##_needed needs to be fixed up to address that,
> > but I'm not sure how best to deal with backward compatibility in that case.
> > Any ideas? If it ends up being a trade-off I think forward compatibility is
> > more important.
> > 
> 
> Yeah, we shouldn't change the default since it affects the migration logic :-\
> 
> The motivation behind this hack is to fix TCG based 'make check', because
> it doesn't pass cap-htm=off, and thus can't run with pseries-2.11.
> 
> Another possibility is to let the default as is, and to disable HTM after the
> default caps have been applied.
> 
> Something like that squashed into this patch:
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index 82043e60e78b..26e6be043b18 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -285,11 +285,6 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
>  
>      caps = smc->default_caps;
>  
> -    /* HACK for 2.11.2: fix make check */
> -    if (tcg_enabled()) {
> -        caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> -    }
> -
>      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
>                            0, spapr->max_compat_pvr)) {
>          caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> @@ -405,6 +400,11 @@ void spapr_caps_reset(sPAPRMachineState *spapr)
>          }
>      }
>  
> +    /* HACK for 2.11.2: fix make check */
> +    if (tcg_enabled()) {
> +        spapr->eff.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> +    }
> +
>      /* .. then apply those caps to the virtual hardware */
>  
>      for (i = 0; i < SPAPR_CAP_NUM; i++) {
> ---------------------

Nooooo!  The whole point of the caps stuff is to stop changing guest
visible behaviours based on host side configuration like the
accelerator.  So, really, let's not put it back in.

The correct fix is to add cap-htm=off to the testcases.  Gross, but
necessary.


> 
> This allows:
> - TCG 'make check' to be happy with pseries-2.11
> - 2.11.2 --> 2.12 migration and backward
> 
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/spapr.c      |    4 ++--
> > >  hw/ppc/spapr_caps.c |    5 +++++
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 1a2dd1f597d9..6499a867520f 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -3820,7 +3820,7 @@ static void spapr_machine_2_12_class_options(MachineClass *mc)
> > >      /* Defaults for the latest behaviour inherited from the base class */
> > >  }
> > > 
> > > -DEFINE_SPAPR_MACHINE(2_12, "2.12", true);
> > > +DEFINE_SPAPR_MACHINE(2_12, "2.12", false);
> > > 
> > >  /*
> > >   * pseries-2.11
> > > @@ -3842,7 +3842,7 @@ static void spapr_machine_2_11_class_options(MachineClass *mc)
> > >      SET_MACHINE_COMPAT(mc, SPAPR_COMPAT_2_11);
> > >  }
> > > 
> > > -DEFINE_SPAPR_MACHINE(2_11, "2.11", false);
> > > +DEFINE_SPAPR_MACHINE(2_11, "2.11", true);
> > > 
> > >  /*
> > >   * pseries-2.10
> > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > index 7b229517be38..82043e60e78b 100644
> > > --- a/hw/ppc/spapr_caps.c
> > > +++ b/hw/ppc/spapr_caps.c
> > > @@ -285,6 +285,11 @@ static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr,
> > > 
> > >      caps = smc->default_caps;
> > > 
> > > +    /* HACK for 2.11.2: fix make check */
> > > +    if (tcg_enabled()) {
> > > +        caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> > > +    }
> > > +
> > >      if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_07,
> > >                            0, spapr->max_compat_pvr)) {
> > >          caps.caps[SPAPR_CAP_HTM] = SPAPR_CAP_OFF;
> > >   
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

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