diff mbox

[11/13] pseries: Fixes and enhancements to L1 cache properties

Message ID 1354588937-27122-12-git-send-email-david@gibson.dropbear.id.au
State New
Headers show

Commit Message

David Gibson Dec. 4, 2012, 2:42 a.m. UTC
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 <david@gibson.dropbear.id.au>
---
 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(-)

Comments

Alexander Graf Dec. 13, 2012, 12:50 p.m. UTC | #1
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 <david@gibson.dropbear.id.au>
> ---
> 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.

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?

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.


Alex

>         uint32_t page_sizes_prop[64];
>         size_t page_sizes_prop_size;
> 
> @@ -335,10 +339,22 @@ static void *spapr_create_fdt_skel(const char *cpu_model,
>         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
> 
>         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
> -        _FDT((fdt_property_cell(fdt, "dcache-block-size",
> +        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
>                                 env->dcache_line_size)));
> -        _FDT((fdt_property_cell(fdt, "icache-block-size",
> +        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
> +                                env->dcache_line_size)));
> +        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
> +                                env->icache_line_size)));
> +        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
>                                 env->icache_line_size)));
> +
> +        if (dcache_size) {
> +            _FDT((fdt_property_cell(fdt, "d-cache-size", dcache_size)));
> +        }
> +        if (icache_size) {
> +            _FDT((fdt_property_cell(fdt, "i-cache-size", icache_size)));
> +        }
> +
>         _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
>         _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
>         _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 742d4f8..7a42ce0 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1031,6 +1031,7 @@ struct CPUPPCState {
> 
>     int dcache_line_size;
>     int icache_line_size;
> +    int l1_dcache_size, l1_icache_size;
> 
>     /* Those resources are used during exception processing */
>     /* CPU model definition */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 3f5df57..b6f69e5 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -967,6 +967,16 @@ uint32_t kvmppc_get_dfp(void)
>     return kvmppc_read_int_cpu_dt("ibm,dfp");
> }
> 
> +int kvmppc_get_icache_size(void)
> +{
> +    return kvmppc_read_int_cpu_dt("i-cache-size");
> +}
> +
> +int kvmppc_get_dcache_size(void)
> +{
> +    return kvmppc_read_int_cpu_dt("d-cache-size");
> +}
> +
> int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
> {
>     uint32_t *hc = (uint32_t*)buf;
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index baad6eb..17b8b23 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -19,6 +19,8 @@ uint32_t kvmppc_get_tbfreq(void);
> uint64_t kvmppc_get_clockfreq(void);
> uint32_t kvmppc_get_vmx(void);
> uint32_t kvmppc_get_dfp(void);
> +int kvmppc_get_icache_size(void);
> +int kvmppc_get_dcache_size(void);
> int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len);
> int kvmppc_set_interrupt(CPUPPCState *env, int irq, int level);
> void kvmppc_set_papr(CPUPPCState *env);
> @@ -55,6 +57,16 @@ static inline uint32_t kvmppc_get_dfp(void)
>     return 0;
> }
> 
> +static inline int kvmppc_get_icache_size(void)
> +{
> +    return 0;
> +}
> +
> +static inline int kvmppc_get_dcache_size(void)
> +{
> +    return 0;
> +}
> +
> static inline int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
> {
>     return -1;
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index e63627c..dba572f 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -6753,6 +6753,10 @@ static void init_proc_POWER7 (CPUPPCState *env)
>     init_excp_POWER7(env);
>     env->dcache_line_size = 128;
>     env->icache_line_size = 128;
> +
> +    env->l1_dcache_size = 0x8000;
> +    env->l1_icache_size = 0x8000;
> +
>     /* Allocate hardware IRQ controller */
>     ppcPOWER7_irq_init(env);
>     /* Can't find information on what this should be on reset.  This
> -- 
> 1.7.10.4
>
David Gibson Dec. 17, 2012, 2:32 a.m. UTC | #2
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 <david@gibson.dropbear.id.au>
> > ---
> > 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.
Alexander Graf Dec. 17, 2012, 10:10 a.m. UTC | #3
On 17.12.2012, at 03:32, David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
>>> ---
>>> 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
David Gibson Dec. 17, 2012, 11 p.m. UTC | #4
On Mon, Dec 17, 2012 at 11:10:12AM +0100, Alexander Graf wrote:
> 
> 
> On 17.12.2012, at 03:32, David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> >>> ---
> >>> 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.

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

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.
Alexander Graf Dec. 17, 2012, 11:49 p.m. UTC | #5
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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
>>>>> ---
>>>>> 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
David Gibson Dec. 19, 2012, 4:34 a.m. UTC | #6
On Tue, Dec 18, 2012 at 12:49:02AM +0100, Alexander Graf wrote:
> 
> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> >>>>> ---
> >>>>> 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.

That works.  Although it's not obvious where to put the check and
fixup.  kvmppc_fixup_cpu() seems like the obvious place, but that's no
good because it's called before the per-cpu-type init function, which
is what populates the expected cachesize values.
Alexander Graf Dec. 19, 2012, 10:40 p.m. UTC | #7
On 19.12.2012, at 05:34, David Gibson wrote:

> On Tue, Dec 18, 2012 at 12:49:02AM +0100, Alexander Graf wrote:
>> 
>> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
>>>>>>> ---
>>>>>>> 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.
> 
> That works.  Although it's not obvious where to put the check and
> fixup.  kvmppc_fixup_cpu() seems like the obvious place, but that's no
> good because it's called before the per-cpu-type init function, which
> is what populates the expected cachesize values.

That's a real shame. Any reason we don't run it after the init function? Fixup indicates that it fixes things up after they happened, not before :).


Alex
David Gibson Dec. 20, 2012, 3:38 a.m. UTC | #8
On Wed, Dec 19, 2012 at 11:40:09PM +0100, Alexander Graf wrote:
> 
> On 19.12.2012, at 05:34, David Gibson wrote:
> 
> > On Tue, Dec 18, 2012 at 12:49:02AM +0100, Alexander Graf wrote:
> >> 
> >> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> >>>>>>> ---
> >>>>>>> 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.
> > 
> > That works.  Although it's not obvious where to put the check and
> > fixup.  kvmppc_fixup_cpu() seems like the obvious place, but that's no
> > good because it's called before the per-cpu-type init function, which
> > is what populates the expected cachesize values.
> 
> That's a real shame. Any reason we don't run it after the init
> function? Fixup indicates that it fixes things up after they
> happened, not before :).

Well, the very first thing that kvmppc_fixup_cpu() did, when I first
implemented it is to adjust the cpu numbers so  we get the right SMT
behaviour under KVM.  I was concerned that later parts of the
initialization might use the cpu_index.
Alexander Graf Dec. 20, 2012, 9:32 a.m. UTC | #9
On 20.12.2012, at 04:38, David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Dec 19, 2012 at 11:40:09PM +0100, Alexander Graf wrote:
>> 
>> On 19.12.2012, at 05:34, David Gibson wrote:
>> 
>>> On Tue, Dec 18, 2012 at 12:49:02AM +0100, Alexander Graf wrote:
>>>> 
>>>> 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 <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
>>>>>>>>> ---
>>>>>>>>> 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.
>>> 
>>> That works.  Although it's not obvious where to put the check and
>>> fixup.  kvmppc_fixup_cpu() seems like the obvious place, but that's no
>>> good because it's called before the per-cpu-type init function, which
>>> is what populates the expected cachesize values.
>> 
>> That's a real shame. Any reason we don't run it after the init
>> function? Fixup indicates that it fixes things up after they
>> happened, not before :).
> 
> Well, the very first thing that kvmppc_fixup_cpu() did, when I first
> implemented it is to adjust the cpu numbers so  we get the right SMT
> behaviour under KVM.  I was concerned that later parts of the
> initialization might use the cpu_index.

Then create 2 fixups. One before and one after init ;).

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

Patch

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;
         uint32_t page_sizes_prop[64];
         size_t page_sizes_prop_size;
 
@@ -335,10 +339,22 @@  static void *spapr_create_fdt_skel(const char *cpu_model,
         _FDT((fdt_property_string(fdt, "device_type", "cpu")));
 
         _FDT((fdt_property_cell(fdt, "cpu-version", env->spr[SPR_PVR])));
-        _FDT((fdt_property_cell(fdt, "dcache-block-size",
+        _FDT((fdt_property_cell(fdt, "d-cache-block-size",
                                 env->dcache_line_size)));
-        _FDT((fdt_property_cell(fdt, "icache-block-size",
+        _FDT((fdt_property_cell(fdt, "d-cache-line-size",
+                                env->dcache_line_size)));
+        _FDT((fdt_property_cell(fdt, "i-cache-block-size",
+                                env->icache_line_size)));
+        _FDT((fdt_property_cell(fdt, "i-cache-line-size",
                                 env->icache_line_size)));
+
+        if (dcache_size) {
+            _FDT((fdt_property_cell(fdt, "d-cache-size", dcache_size)));
+        }
+        if (icache_size) {
+            _FDT((fdt_property_cell(fdt, "i-cache-size", icache_size)));
+        }
+
         _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq)));
         _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq)));
         _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr)));
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 742d4f8..7a42ce0 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1031,6 +1031,7 @@  struct CPUPPCState {
 
     int dcache_line_size;
     int icache_line_size;
+    int l1_dcache_size, l1_icache_size;
 
     /* Those resources are used during exception processing */
     /* CPU model definition */
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 3f5df57..b6f69e5 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -967,6 +967,16 @@  uint32_t kvmppc_get_dfp(void)
     return kvmppc_read_int_cpu_dt("ibm,dfp");
 }
 
+int kvmppc_get_icache_size(void)
+{
+    return kvmppc_read_int_cpu_dt("i-cache-size");
+}
+
+int kvmppc_get_dcache_size(void)
+{
+    return kvmppc_read_int_cpu_dt("d-cache-size");
+}
+
 int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
 {
     uint32_t *hc = (uint32_t*)buf;
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index baad6eb..17b8b23 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -19,6 +19,8 @@  uint32_t kvmppc_get_tbfreq(void);
 uint64_t kvmppc_get_clockfreq(void);
 uint32_t kvmppc_get_vmx(void);
 uint32_t kvmppc_get_dfp(void);
+int kvmppc_get_icache_size(void);
+int kvmppc_get_dcache_size(void);
 int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len);
 int kvmppc_set_interrupt(CPUPPCState *env, int irq, int level);
 void kvmppc_set_papr(CPUPPCState *env);
@@ -55,6 +57,16 @@  static inline uint32_t kvmppc_get_dfp(void)
     return 0;
 }
 
+static inline int kvmppc_get_icache_size(void)
+{
+    return 0;
+}
+
+static inline int kvmppc_get_dcache_size(void)
+{
+    return 0;
+}
+
 static inline int kvmppc_get_hypercall(CPUPPCState *env, uint8_t *buf, int buf_len)
 {
     return -1;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e63627c..dba572f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -6753,6 +6753,10 @@  static void init_proc_POWER7 (CPUPPCState *env)
     init_excp_POWER7(env);
     env->dcache_line_size = 128;
     env->icache_line_size = 128;
+
+    env->l1_dcache_size = 0x8000;
+    env->l1_icache_size = 0x8000;
+
     /* Allocate hardware IRQ controller */
     ppcPOWER7_irq_init(env);
     /* Can't find information on what this should be on reset.  This