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

[Alexander Graf]

On 18.12.2012, at 00:00, David Gibson wrote:

> On Mon, Dec 17, 2012 at 11:10:12AM +0100, Alexander Graf wrote:
>> 
>> 
>> 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.
> 
> Hrm.  Ok.
> 
> So, the remaining difficulty I have with that is that for -cpu HOST we
> should still take the cache sizes from the host, but that can't easily
> be done because they're only stored in the env, not the cpu_def.

Can we set a bit somewhere that allows us to do a sanity check later? After 
all, the values coming from the host and the values in the populated env really 
should just be identical for -cpu host. Every time they're not, it's simply a 
bug that needs to be reported.

> 
>> 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.
> 
> So.. this is kind of an aside at this point but POWER6/POWER7/POWER8
> compatibility is more complicated than that.  For TCG or KVM-PR, -cpu
> POWER6 should be possible, and pretty much identical to a real POWER6.
> For KVM HV, the best we can do is POWER7's POWER6 compatibility mode,
> which is kinda-similar to POWER6, but still a fair way from identical
> (for starters the POWER7 PVR value is still visible).  It should be
> close enough to run (PAPR compliant)  OSes that know about POWER6 but
> not POWER7 - but only if they use the device tree the way they're
> supposed to (which includes amongst other things a special "virtual
> PVR" value for this quasi-POWER6).  And as long as they don't use the
> performance counters.

Sounds pretty broken. But I'm sort of getting used to that with HV mode in 
POWER :).

So if we made things properly transparent and well working, could we use PR KVM 
as fallback for HV migration? That'd get rid of the whole category of problems, 
no?

But really, for now -cpu host on a POWER7 box == -cpu POWER7. So the cache 
sizes will automatically match.


Alex

> As far as (HV) KVM is concerned our plan is to trigger the POWER6
> compatibility mode by setting the relevant virtual PVR value in
> SET_SREGS.  For qemu it's a bit less clear - we could add a cputable
> entry for the virtual PVR - -cpu POWER7_POWER6 or something.  Or
> alternatively we could treat the compatibility mode as that - a
> special mode bit in the env.  The latter has the advantage that we can
> use the PAPR defined CAS system to see what processors the guest OS
> advertises support for and automatically switch on the compat mode
> when necessary.
> 
> -- 
> 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