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

[Greg Kurz]

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.

> >   
> > > +    } else {
> > > +        _FDT((fdt_setprop_cell(fdt, offset, "ibm,vmx", 1)));
> > >      }
> > >  
> > >      /* Advertise DFP (Decimal Floating Point) if available
> > > @@ -3829,7 +3831,7 @@ static void spapr_machine_class_init(ObjectClass 
> > > *oc, void *data)
> > >       */
> > >      mc->numa_mem_align_shift = 28;
> > >  
> > > -    smc->default_caps = spapr_caps(SPAPR_CAP_HTM);
> > > +    smc->default_caps = spapr_caps(SPAPR_CAP_HTM | SPAPR_CAP_VSX);
> > >      spapr_capability_properties(smc, &error_abort);
> > >  }
> > >  
> > > diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> > > index 6bba04d60e..b19a9f4417 100644
> > > --- a/hw/ppc/spapr_caps.c
> > > +++ b/hw/ppc/spapr_caps.c
> > > @@ -44,6 +44,11 @@ static sPAPRCapabilityInfo capability_table[] = {
> > >          .description = "Allow Hardware Transactional Memory (HTM)",
> > >          .bit = SPAPR_CAP_HTM,
> > >      },
> > > +    {
> > > +        .name = "vsx",
> > > +        .description = "Allow Vector Scalar Extensions (VSX)",
> > > +        .bit = SPAPR_CAP_VSX,
> > > +    },
> > >  };
> > >  
> > >  static sPAPRCapabilities default_caps_with_cpu(sPAPRMachineState *spapr, 
> > > CPUState *cs)
> > > @@ -59,6 +64,11 @@ static sPAPRCapabilities 
> > > default_caps_with_cpu(sPAPRMachineState *spapr, CPUStat
> > >          caps.mask &= ~SPAPR_CAP_HTM;
> > >      }
> > >  
> > > +    if (!ppc_check_compat(cpu, CPU_POWERPC_LOGICAL_2_06,
> > > +                          0, spapr->max_compat_pvr)) {
> > > +        caps.mask &= ~SPAPR_CAP_VSX;
> > > +    }
> > > +
> > >      return caps;
> > >  }
> > >  
> > > @@ -149,6 +159,8 @@ const VMStateDescription vmstate_spapr_caps = {
> > >  
> > >  static void spapr_allow_caps(sPAPRMachineState *spapr, Error **errp)
> > >  {
> > > +    PowerPCCPU *cpu = POWERPC_CPU(first_cpu);
> > > +    CPUPPCState *env = &cpu->env;
> > >      Error *err = NULL;
> > >  
> > >      if (spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > > @@ -164,6 +176,14 @@ static void spapr_allow_caps(sPAPRMachineState 
> > > *spapr, Error **errp)
> > >          }
> > >      }
> > >  
> > > +    g_assert(env->insns_flags & PPC_ALTIVEC);
> > > +    if (spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > > +        if (!(env->insns_flags2 & PPC2_VSX)) {
> > > +            error_setg(&err, "VSX support not available, try 
> > > cap-vsx=off");
> > > +            goto out;
> > > +        }
> > > +    }
> > > +
> > >  out:
> > >      if (err) {
> > >          error_propagate(errp, err);
> > > @@ -175,6 +195,10 @@ static void spapr_enforce_caps(sPAPRMachineState 
> > > *spapr, Error **errp)
> > >      if (!spapr_has_cap(spapr, SPAPR_CAP_HTM)) {
> > >          /* TODO: Tell KVM not to allow HTM instructions */
> > >      }
> > > +
> > > +    if (!spapr_has_cap(spapr, SPAPR_CAP_VSX)) {
> > > +        /* TODO: Any way to disable VSX? */
> > > +    }
> > >  }
> > >  
> > >  void spapr_caps_reset(sPAPRMachineState *spapr)
> > > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> > > index 9b80fe72ed..4c173cb40c 100644
> > > --- a/include/hw/ppc/spapr.h
> > > +++ b/include/hw/ppc/spapr.h
> > > @@ -59,6 +59,9 @@ typedef enum {
> > >  /* Hardware Transactional Memory */
> > >  #define SPAPR_CAP_HTM               0x0000000000000001ULL
> > >  
> > > +/* Vector Scalar Extensions */
> > > +#define SPAPR_CAP_VSX               0x0000000000000002ULL
> > > +
> > >  typedef struct sPAPRCapabilities sPAPRCapabilities;
> > >  struct sPAPRCapabilities {
> > >      uint64_t mask;  
> >   
>

pgp_x1XODYfW_.pgp
Description: OpenPGP digital signature