Re: [Qemu-devel] [Qemu-ppc] [PATCH 11/13] pseries: Fixes and enhancements to L1 cache properties

[David Gibson]

On Thu, Dec 13, 2012 at 01:50:25PM +0100, Alexander Graf wrote:
> 
> On 04.12.2012, at 03:42, David Gibson wrote:
> 
> > PAPR requires that the device tree's CPU nodes have several properties
> > with information about the L1 cache.  We created two of these
> > properties, but with incorrect names - "[id]cache-block-size" instead
> > of "[id]-cache-block-size" (note the extra hyphen).
> > 
> > We were also missing some of the required cache properties.  This
> > patch adds the [id]-cache-line-size properties (which have the same
> > values as the block size properties in all current cases).  We also
> > add the [id]-cache-size properties.  The latter requires some extra
> > infrastructure in the general target-ppc code to (optionally) set the
> > cache sizes for various CPUs.  We obtain the published values either
> > from there, or from the host when KVM is in use.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> > hw/spapr.c                  |   20 ++++++++++++++++++--
> > target-ppc/cpu.h            |    1 +
> > target-ppc/kvm.c            |   10 ++++++++++
> > target-ppc/kvm_ppc.h        |   12 ++++++++++++
> > target-ppc/translate_init.c |    4 ++++
> > 5 files changed, 45 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/spapr.c b/hw/spapr.c
> > index d23aa9d..3bacf2f 100644
> > --- a/hw/spapr.c
> > +++ b/hw/spapr.c
> > @@ -315,6 +315,10 @@ 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;
> > +        int dcache_size = kvm_enabled() ? kvmppc_get_dcache_size()
> > +            : env->l1_dcache_size;
> > +        int icache_size = kvm_enabled() ? kvmppc_get_icache_size()
> > +            : env->l1_icache_size;
> 
> By default with KVM we use -cpu host, right? So we already should
> get the correct cache sizes for the CPU you're on.

Um.. sort of.  The first problem with that is that I only just added
the cache size information to qemu, so only a few CPUs currently
populate that information.  Using the host info means we can get the
right information even for CPUs that don't yet have cache info in
qemu.

> Imagine we would support the compatibility feature where you could
> run with -cpu POWER6 on a POWER7 machine. Would exposing the POWER6
> cache size rather than the host's make any real difference to the
> guest? Or would it work nevertheless?

The second problem is that there may be circumstances where the
cache size is altered from the normal size for the cpu.  Running in
POWER6 compat mode is one example, but I'm not sure there aren't
others.  e.g. the hypervisor limiting the amount of cache available to
a partition, or portions of the cache disabled due to a chicken switch
or the like.  In short, when we have the cache size information
directly available from the host, I'd really prefer to use that, over
getting the host PVR and assuming what we have in our table is
correct.

> If it would still work just fine, I'd say ditch the "ask the host"
> bit and always use the sizes from the cpu definition.

So, if the cache size is wrong it will probably work for most
purposes.  In fact as far as I know, it will always work from a strict
functionality point of view.  But if something in the guest uses the
cache size information to optimize its cache blocking (I'm thinking of
BLAS) it could have a big effect on performance.

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