Re: [Qemu-ppc] [RFC for-2.12 7/8] spapr: Handle VMX/VSX presence as an spapr capability flag

[David Gibson]

On Wed, Dec 13, 2017 at 10:35:32AM +0100, Greg Kurz wrote:
> On Wed, 13 Dec 2017 12:51:37 +1100
> David Gibson <address@hidden> wrote:
> 
> > On Tue, Dec 12, 2017 at 01:54:36PM +0100, Greg Kurz wrote:
> > > On Mon, 11 Dec 2017 18:08:07 +1100
> > > David Gibson <address@hidden> wrote:
> > >   
> > > > We currently have some conditionals in the spapr device tree code to 
> > > > decide
> > > > whether or not to advertise the availability of the VMX (aka Altivec) 
> > > > and
> > > > VSX vector extensions to the guest, based on whether the guest cpu has
> > > > those features.
> > > > 
> > > > This can lead to confusion and subtle failures on migration, since it 
> > > > makes
> > > > a guest visible change based only on host capabilities.  We now have a
> > > > better mechanism for this, in spapr capabilities flags, which explicitly
> > > > depend on user options rather than host capabilities.
> > > > 
> > > > Rework the advertisement of VSX and VMX based on a new VSX capability.  
> > > > We
> > > > no longer bother with a conditional for VMX support, because every CPU
> > > > that's ever been supported by the pseries machine type supports VMX.
> > > > 
> > > > NOTE: Some userspace distributions (e.g. RHEL7.4) already rely on
> > > > availability of VSX in libc, so using cap-vsx=off may lead to a fatal
> > > > SIGILL in init.
> > > > 
> > > > Signed-off-by: David Gibson <address@hidden>
> > > > ---  
> > > 
> > > Just one nit, see below, but anyway:
> > > 
> > > Reviewed-by: Greg Kurz <address@hidden>
> > > 
> > >   
> > > >  hw/ppc/spapr.c         | 18 ++++++++++--------
> > > >  hw/ppc/spapr_caps.c    | 24 ++++++++++++++++++++++++
> > > >  include/hw/ppc/spapr.h |  3 +++
> > > >  3 files changed, 37 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index e206825ed9..6b37511e8f 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -556,14 +556,16 @@ static void spapr_populate_cpu_dt(CPUState *cs, 
> > > > void *fdt, int offset,
> > > >                            segs, sizeof(segs))));
> > > >      }
> > > >  
> > > > -    /* Advertise VMX/VSX (vector extensions) if available
> > > > -     *   0 / no property == no vector extensions
> > > > +    /* Advertise VSX (vector extensions) if available
> > > >       *   1               == VMX / Altivec available
> > > > -     *   2               == VSX available */
> > > > -    if (env->insns_flags & PPC_ALTIVEC) {
> > > > -        uint32_t vmx = (env->insns_flags2 & PPC2_VSX) ? 2 : 1;
> > > > -
> > > > -        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", vmx)));
> > > > +     *   2               == VSX available
> > > > +     *
> > > > +     * Only CPUs for which we create core types in spapr_cpu_core.c
> > > > +     * are possible, and all of those have VMX */
> > > > +    if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > > > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 2)));  
> > > 
> > > Maybe a good opportunity to drop the extra parens since the _FDT() macro
> > > already adds them around its argument.
> > > 
> > > #define _FDT(exp)                                                  \
> > >     do {                                                           \
> > >         int ret = (exp);                                           \  
> > 
> > Uh, yeah, that won't work.  The extra parens are so that the macro
> > considers the whole string inside as a single argument, even if it
> > includes ',' characters, which it usually will.
> 
> Hmm... we have a bunch of other _FDT() call sites where we do without the
> extra parens. No big deal anyway.

Ah, so we do.  I think we're only getting away with that because the
','s happen to be within parens already (as function args).

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

signature.asc
Description: PGP signature