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

[Alexander Graf]

On 17.12.2012, at 03:32, David Gibson <address@hidden> wrote:

> 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

Well, either we want to be compatible or we don't :). If we run with -cpu 
POWER6 we want to generate the same dt as we did on a POWER6 system itself.

Look at it from one step ahead. Take POWER8 and POWER7. I want to be able to 
live migrate from a POWER7 system to a POWER8 system. On my POWER8 box, the 
generated dt should look like it did on my POWER7 box.

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

I disagree. We can add a sanity check that -cpu host has to find the same cache 
size as is in the cpu table. We could even make that sanity check take the host 
value if the cpu table value is 0 and issue a warning. But I don't want to have 
host information take precedence over user controlled settings unless we have 
to (tb freq).

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

Well, then today we are at the same level as x86. And with proper cpu table 
cache size info we are even better than x86. :)


Alex

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