Re: [Qemu-devel] [PATCH] pseries: Add device tree properties for VMX/VSX and DFP under kvm

[David Gibson <david@gibson.dropbear.id.au>]

On Wed, Sep 21, 2011 at 09:53:22AM +0200, Alexander Graf wrote:
> 
> On 21.09.2011, at 08:35, David Gibson wrote:
> 
> > Sufficiently recent PAPR specifications define properties "ibm,vmx"
> > and "ibm,dfp" on the CPU node which advertise whether the VMX vector
> > extensions (or the later VSX version) and/or the Decimal Floating
> > Point operations from IBM's recent POWER CPUs are available.
> > 
> > Currently we do not put these in the guest device tree and the guest
> > kernel will consequently assume they are not available.  This is good,
> > because they are not supported under TCG.  VMX is similar enough to
> > Altivec that it might be trivial to support, but VSX and DFP would
> > both require significant work to support in TCG.
> > 
> > However, when running under kvm on a host which supports these
> > instructions, there's no reason not to let the guest use them.  This
> > patch, therefore, checks for the relevant support on the host CPU
> > and, if present, advertises them to the guest as well.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/spapr.c           |   13 ++++++++++
> > target-ppc/kvm.c     |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > target-ppc/kvm_ppc.h |   12 ++++++++++
> > 3 files changed, 85 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/spapr.c b/hw/spapr.c
> > index b118975..573392d 100644
> > --- a/hw/spapr.c
> > +++ b/hw/spapr.c
> > @@ -168,6 +168,9 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> >                            0xffffffff, 0xffffffff};
> >         uint32_t tbfreq = kvm_enabled() ? kvmppc_get_tbfreq() : TIMEBASE_FREQ;
> >         uint32_t cpufreq = kvm_enabled() ? kvmppc_get_clockfreq() : 1000000000;
> > +        /* Currently TCG doesn't implement VMX or DFP instructions */
> > +        uint32_t vmx = kvm_enabled() ? kvmppc_get_vmx() : 0;
> > +        uint32_t dfp = kvm_enabled() ? kvmppc_get_dfp() : 0;
> 
> This should also take the target CPU into account. If the target CPU
> can't do VMX/DFP, it shouldn' be announced. Imagine running a guest
> with the compatibility mode bit set, so we're virtualization the
> previous generation.

Hrm, ok.

> When did dfp get introduced? Was that POWER6 or POWER7?

POWER7, I think, but I'm not certain.

> Also, isn't
> there this twice-as-wide VMX extension too?

Yeah, that's VSX.  It's represented by a 2 instead of a 1 in the
"ibm,vmx" property.

> >         if (asprintf(&nodename, "%s@%x", modelname, index) < 0) {
> >             fprintf(stderr, "Allocation failure\n");
> > @@ -202,6 +205,16 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
> >                                segs, sizeof(segs))));
> >         }
> > 
> > +        /* Advertise VMX/VSX (vector extensions) if available */
> > +        if (vmx) {
> > +            _FDT((fdt_property_cell(fdt, "ibm,vmx", vmx)));
> > +        }
> > +
> > +        /* Advertise DFP (Decimal Floating Point) if available */
> > +        if (dfp) {
> > +            _FDT((fdt_property_cell(fdt, "ibm,dfp", dfp)));
> > +        }
> > +
> >         _FDT((fdt_end_node(fdt)));
> >     }
> > 
> > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> > index 35a6f10..397a803 100644
> > --- a/target-ppc/kvm.c
> > +++ b/target-ppc/kvm.c
> > @@ -673,6 +673,66 @@ uint64_t kvmppc_get_clockfreq(void)
> >     return 0;
> > }
> > 
> > +uint32_t kvmppc_get_vmx(void)
> > +{
> > +    char buf[512];
> > +    uint32_t vmx;
> > +    FILE *f;
> > +    int len;
> > +
> > +    if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
> > +        return 0;
> > +    }
> > +
> > +    strncat(buf, "/ibm,vmx", sizeof(buf) - strlen(buf));
> > +
> > +    f = fopen(buf, "rb");
> > +    if (!f) {
> > +        /* Assume -ENOENT, which indicates that VMX is not available */
> > +        return 0;
> > +    }
> > +
> > +    len = fread(&vmx, sizeof(vmx), 1, f);
> > +    fclose(f);
> > +
> > +    if (len != 1) {
> > +        /* Malformed ibm,vmx property, assume no vmx or vsx */
> > +        return 0;
> > +    }
> > +
> > +    return be32_to_cpu(vmx);
> > +}
> > +
> > +uint32_t kvmppc_get_dfp(void)
> > +{
> > +    char buf[512];
> > +    uint32_t dfp;
> > +    FILE *f;
> > +    int len;
> > +
> > +    if (kvmppc_find_cpu_dt(buf, sizeof(buf))) {
> > +        return 0;
> > +    }
> > +
> > +    strncat(buf, "/ibm,dfp", sizeof(buf) - strlen(buf));
> > +
> > +    f = fopen(buf, "rb");
> > +    if (!f) {
> > +        /* Assume -ENOENT, which indicates that DFP is not available */
> > +        return 0;
> > +    }
> > +
> > +    len = fread(&dfp, sizeof(dfp), 1, f);
> > +    fclose(f);
> > +
> > +    if (len != 1) {
> > +        /* Malformed ibm,dfp property, assume no DFP */
> > +        return 0;
> > +    }
> > +
> > +    return be32_to_cpu(dfp);
> > +}
> 
> Could you please fold those two into a single helper function to
> read out a 32-bit dt property and then just use that? :)
> 
> Please also document somewhere in the code what the return value
> means (0 = unavailable, 1 = available).

Ok.

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