Re: [Qemu-ppc] [RFC for-2.12 6/8] target/ppc: Clean up probing of VMX, VSX and DFP availability on KVM

[David Gibson]

On Tue, Dec 12, 2017 at 09:37:10AM +0100, Greg Kurz wrote:
> On Mon, 11 Dec 2017 18:08:06 +1100
> David Gibson <address@hidden> wrote:
> 
> > When constructing the "host" cpu class we modify whether the VMX and VSX
> > vector extensions and DFP (Decimal Floating Point) are available
> > based on whether KVM can support those instructions.  This can depend on
> > policy in the host kernel as well as on the actual host cpu capabilities.
> > 
> > However, the way we probe for this is not very nice: we explicitly check
> > the host's device tree.  That works in practice, but it's not really
> > correct, since the device tree is a property of the host kernel's platform
> > which we don't really know about.  We get away with it because the only
> > modern POWER platforms happen to encode VMX, VSX and DFP availability in
> > the device tree in the same way.
> > 
> > Arguably we should have an explicit KVM capability for this, but we haven't
> > needed one so far.  Barring specific KVM policies which don't yet exist,
> > each of these instruction classes will be available in the guest if and
> > only if they're available in the qemu userspace process.  We can determine
> > that from the ELF AUX vector we're supplied with.
> > 
> > Once reworked like this, there are no more callers for kvmppc_get_vmx() and
> > kvmppc_get_dfp() so remove them.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> 
> Nice improvement indeed. Just one nit, see below, but anyway:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> >  target/ppc/kvm.c     | 27 ++++++---------------------
> >  target/ppc/kvm_ppc.h |  2 --
> >  2 files changed, 6 insertions(+), 23 deletions(-)
> > 
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 9d57debf0e..81d9bd56c7 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -2014,16 +2014,6 @@ uint64_t kvmppc_get_clockfreq(void)
> >      return kvmppc_read_int_cpu_dt("clock-frequency");
> >  }
> >  
> > -uint32_t kvmppc_get_vmx(void)
> > -{
> > -    return kvmppc_read_int_cpu_dt("ibm,vmx");
> > -}
> > -
> > -uint32_t kvmppc_get_dfp(void)
> > -{
> > -    return kvmppc_read_int_cpu_dt("ibm,dfp");
> > -}
> > -
> >  static int kvmppc_get_pvinfo(CPUPPCState *env, struct kvm_ppc_pvinfo 
> > *pvinfo)
> >   {
> >       PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > @@ -2407,23 +2397,18 @@ static void alter_insns(uint64_t *word, uint64_t 
> > flags, bool on)
> >  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> >  {
> >      PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> > -    uint32_t vmx = kvmppc_get_vmx();
> > -    uint32_t dfp = kvmppc_get_dfp();
> >      uint32_t dcache_size = kvmppc_read_int_cpu_dt("d-cache-size");
> >      uint32_t icache_size = kvmppc_read_int_cpu_dt("i-cache-size");
> >  
> >      /* Now fix up the class with information we can query from the host */
> >      pcc->pvr = mfpvr();
> >  
> > -    if (vmx != -1) {
> > -        /* Only override when we know what the host supports */
> > -        alter_insns(&pcc->insns_flags, PPC_ALTIVEC, vmx > 0);
> > -        alter_insns(&pcc->insns_flags2, PPC2_VSX, vmx > 1);
> > -    }
> > -    if (dfp != -1) {
> > -        /* Only override when we know what the host supports */
> > -        alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
> > -    }
> > +    alter_insns(&pcc->insns_flags, PPC_ALTIVEC,
> > +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_ALTIVEC);
> 
> The third argument of alter_insns() has bool type, so you should
> probably use !! or != 0.

Hrm.  Passing an integer to a bool parameter should already perform an
equivalent coercion, no?  Isn't that the point of the bool type.

> > +    alter_insns(&pcc->insns_flags2, PPC2_VSX,
> > +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_VSX);
> > +    alter_insns(&pcc->insns_flags2, PPC2_DFP,
> > +                qemu_getauxval(AT_HWCAP) & PPC_FEATURE_HAS_DFP);
> >  
> >      if (dcache_size != -1) {
> >          pcc->l1_dcache_size = dcache_size;
> > diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> > index d6be38ecaf..ecb55493cc 100644
> > --- a/target/ppc/kvm_ppc.h
> > +++ b/target/ppc/kvm_ppc.h
> > @@ -15,8 +15,6 @@
> >  
> >  uint32_t kvmppc_get_tbfreq(void);
> >  uint64_t kvmppc_get_clockfreq(void);
> > -uint32_t kvmppc_get_vmx(void);
> > -uint32_t kvmppc_get_dfp(void);
> >  bool kvmppc_get_host_model(char **buf);
> >  bool kvmppc_get_host_serial(char **buf);
> >  int kvmppc_get_hasidle(CPUPPCState *env);
> 

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