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

[Greg Kurz <groug@kaod.org>]

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

On Wed, 20 Jun 2018 19:06:01 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jun 20, 2018 at 09:27:27AM +0200, Greg Kurz wrote:
> > On Wed, 20 Jun 2018 11:22:40 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > 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.
> > >   
> > 
> > Yes I'm aware of all the work we're doing to hide host details.
> > I was just thinking this small violation would be an acceptable
> > tradeoff.  
> 
> The reason I wrote the caps stuff was that I kept finding that
> supposedly small violations of this still kept biting us in the arse.
> 

Fair enough.

> > > The correct fix is to add cap-htm=off to the testcases.  Gross, but
> > > necessary.
> > >   
> > 
> > Ok, then I'll try to come up with patches ASAP so that Mike can release
> > 2.11.2.  
> 
> After I sent that I did think of some alternatives.  We can't change
> the behaviour based on the accelerator, but we might be able to change
> it based on qtest_enabled().
> 

No. qtest is the accelerator used by qtest based testcases, and the
failing testcases aren't qtest:
- tests/boot-serial-test.c
- tests/migration-test.c
- tests/prom-env-test.c
- tests/pxe-test.c

These all start QEMU with TCG on non-ppc64 hosts, and the modifications
to add cap-htm=off aren't that gross in the end. I'll do some more
testing and post a v2.

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