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

[Greg Kurz]

On Wed, 13 Dec 2017 12:52:11 +1100
David Gibson <address@hidden> wrote:

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

Yes of course, I was just suggesting to make it explicit when looking at the
alter_insns() call sites only. Question of taste.

> > > +    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);  
> >   
>

pgp6mhKcsD0Q2.pgp
Description: OpenPGP digital signature