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

[Alexander Graf]

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.

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
>