Patchwork [3/5] pseries: Fixes and enhancements to L1 cache properties

login
register
mail settings
Submitter David Gibson
Date March 14, 2013, 1:53 a.m.
Message ID <1363226008-26639-4-git-send-email-david@gibson.dropbear.id.au>
Download mbox | patch
Permalink /patch/227424/
State New
Headers show

Comments

David Gibson - March 14, 2013, 1:53 a.m.
PAPR requires that the device tree's CPU nodes have several properties
with information about the L1 cache.  We already create 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.

Adding the cache sizes requires some extra infrastructure in the
general target-ppc code to (optionally) set the cache sizes for
various CPUs.  The CPU family descriptions in translate_init.c can set
these sizes - this patch adds correct information for POWER7, I'm
leaving other CPU types to people who have a physical example to
verify against.  In addition, for -cpu host we take the values
advertised by the host (if available) and use those to override the
information based on PVR.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr.c              |   20 ++++++++++++++++++--
 target-ppc/cpu.h            |    1 +
 target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
 target-ppc/translate_init.c |    4 ++++
 4 files changed, 62 insertions(+), 2 deletions(-)
Alexander Graf - March 15, 2013, 12:27 p.m.
On 14.03.2013, at 02:53, David Gibson wrote:

> PAPR requires that the device tree's CPU nodes have several properties
> with information about the L1 cache.  We already create 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.
> 
> Adding the cache sizes requires some extra infrastructure in the
> general target-ppc code to (optionally) set the cache sizes for
> various CPUs.  The CPU family descriptions in translate_init.c can set
> these sizes - this patch adds correct information for POWER7, I'm
> leaving other CPU types to people who have a physical example to
> verify against.  In addition, for -cpu host we take the values
> advertised by the host (if available) and use those to override the
> information based on PVR.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> target-ppc/cpu.h            |    1 +
> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> target-ppc/translate_init.c |    4 ++++
> 4 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 9a13697..7293082 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> +        } else {
> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> +        }
> +        if (env->l1_icache_size) {
> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> +        } else {
> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> +        }

The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?


Alex

> +
>         _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 0b2d0b9..793ae0f 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -994,6 +994,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 f5710b7..eaa11a1 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1612,14 +1612,49 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
>     }
> }
> 
> +typedef struct PowerPCHostCPUClass {
> +    /*< private >*/
> +    PowerPCCPUClass parent_class;
> +    /*< public >*/
> +
> +    DeviceRealize parent_realize;
> +} PowerPCHostCPUClass;
> +
> +#define POWERPC_HOST_CPU_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU)
> +#define POWERPC_HOST_CPU_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU)
> +
> static void kvmppc_host_cpu_initfn(Object *obj)
> {
>     assert(kvm_enabled());
> }
> 
> +static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(dev);
> +    CPUPPCState *env = &cpu->env;
> +    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev);
> +    uint32_t val;
> +
> +    (*hpcc->parent_realize)(dev, errp);
> +
> +    val = kvmppc_read_int_cpu_dt("d-cache-size");
> +    if (val != -1) {
> +        env->l1_dcache_size = val;
> +    }
> +
> +    val = kvmppc_read_int_cpu_dt("i-cache-size");
> +    if (val != -1) {
> +        env->l1_icache_size = val;
> +    }
> +}
> +
> static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> {
> +    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc);
>     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +    DeviceClass *dc = DEVICE_CLASS(oc);
>     uint32_t vmx = kvmppc_get_vmx();
>     uint32_t dfp = kvmppc_get_dfp();
> 
> @@ -1634,6 +1669,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
>         /* Only override when we know what the host supports */
>         alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
>     }
> +
> +    hpcc->parent_realize = dc->realize;
> +    dc->realize = kvmppc_host_cpu_realizefn;
> }
> 
> int kvmppc_fixup_cpu(PowerPCCPU *cpu)
> @@ -1654,6 +1692,7 @@ static int kvm_ppc_register_host_cpu_type(void)
>     TypeInfo type_info = {
>         .name = TYPE_HOST_POWERPC_CPU,
>         .instance_init = kvmppc_host_cpu_initfn,
> +        .class_size = sizeof(PowerPCHostCPUClass),
>         .class_init = kvmppc_host_cpu_class_init,
>     };
>     uint32_t host_pvr = mfpvr();
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 3cf440e..594053e 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -6989,6 +6989,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 - March 16, 2013, 7:10 a.m.
On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
> > PAPR requires that the device tree's CPU nodes have several properties
> > with information about the L1 cache.  We already create 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.
> > 
> > Adding the cache sizes requires some extra infrastructure in the
> > general target-ppc code to (optionally) set the cache sizes for
> > various CPUs.  The CPU family descriptions in translate_init.c can set
> > these sizes - this patch adds correct information for POWER7, I'm
> > leaving other CPU types to people who have a physical example to
> > verify against.  In addition, for -cpu host we take the values
> > advertised by the host (if available) and use those to override the
> > information based on PVR.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> > target-ppc/cpu.h            |    1 +
> > target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> > target-ppc/translate_init.c |    4 ++++
> > 4 files changed, 62 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 9a13697..7293082 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
> > +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> > +        } else {
> > +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> > +        }
> > +        if (env->l1_icache_size) {
> > +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> > +        } else {
> > +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> > +        }
> 
> The L1 sizes should come from the class, not env, right? Andreas,
> any ideas on this?

Well.. initially I was going to put them in class.  But then it
occurred to me that the class represents a family of similar CPUs, not
a single precise CPU model.  Total cache sizes are the sort of thing
that could easily vary between minor revisions, although I don't know
if they have in practice.  So I thought it was safer to put these in
env.
Andreas Färber - March 18, 2013, 10:54 a.m.
Am 15.03.2013 13:27, schrieb Alexander Graf:
> 
> On 14.03.2013, at 02:53, David Gibson wrote:
> 
>> PAPR requires that the device tree's CPU nodes have several properties
>> with information about the L1 cache.  We already create 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.
>>
>> Adding the cache sizes requires some extra infrastructure in the
>> general target-ppc code to (optionally) set the cache sizes for
>> various CPUs.  The CPU family descriptions in translate_init.c can set
>> these sizes - this patch adds correct information for POWER7, I'm
>> leaving other CPU types to people who have a physical example to
>> verify against.  In addition, for -cpu host we take the values
>> advertised by the host (if available) and use those to override the
>> information based on PVR.
>>
>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>> ---
>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>> target-ppc/cpu.h            |    1 +
>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>> target-ppc/translate_init.c |    4 ++++
>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index 9a13697..7293082 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>> +        } else {
>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>> +        }
>> +        if (env->l1_icache_size) {
>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>> +        } else {
>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>> +        }
> 
> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?

Generally speaking,

CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
for legacy grouping reasons).

PowerPCCPU: If you ever intend to let the user override this value
(per-instance) from the command line.

PowerPCCPUClass: If the value is always constant at runtime.

I can't tell from a brief look at this patch which may be the case here.

Andreas
Alexander Graf - March 18, 2013, 11:05 a.m.
On 18.03.2013, at 11:54, Andreas Färber wrote:

> Am 15.03.2013 13:27, schrieb Alexander Graf:
>> 
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> PAPR requires that the device tree's CPU nodes have several properties
>>> with information about the L1 cache.  We already create 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.
>>> 
>>> Adding the cache sizes requires some extra infrastructure in the
>>> general target-ppc code to (optionally) set the cache sizes for
>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>> these sizes - this patch adds correct information for POWER7, I'm
>>> leaving other CPU types to people who have a physical example to
>>> verify against.  In addition, for -cpu host we take the values
>>> advertised by the host (if available) and use those to override the
>>> information based on PVR.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>> ---
>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>> target-ppc/cpu.h            |    1 +
>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>> target-ppc/translate_init.c |    4 ++++
>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>> index 9a13697..7293082 100644
>>> --- a/hw/ppc/spapr.c
>>> +++ b/hw/ppc/spapr.c
>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>> +        } else {
>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>> +        }
>>> +        if (env->l1_icache_size) {
>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>> +        } else {
>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>> +        }
>> 
>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
> 
> Generally speaking,
> 
> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
> for legacy grouping reasons).
> 
> PowerPCCPU: If you ever intend to let the user override this value
> (per-instance) from the command line.
> 
> PowerPCCPUClass: If the value is always constant at runtime.
> 
> I can't tell from a brief look at this patch which may be the case here.

Imagine the following:

  PPC
    `- POWER7
        |- POWER7_v10
        `- POWER7_v20

Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.


Alex
Andreas Färber - March 18, 2013, 11:10 a.m.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 16.03.2013 08:10, schrieb David Gibson:
> On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
>> On 14.03.2013, at 02:53, David Gibson wrote:
>> 
>>> PAPR requires that the device tree's CPU nodes have several
>>> properties with information about the L1 cache.  We already
>>> create 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.
>>> 
>>> Adding the cache sizes requires some extra infrastructure in
>>> the general target-ppc code to (optionally) set the cache sizes
>>> for various CPUs.  The CPU family descriptions in
>>> translate_init.c can set these sizes - this patch adds correct
>>> information for POWER7, I'm leaving other CPU types to people
>>> who have a physical example to verify against.  In addition,
>>> for -cpu host we take the values advertised by the host (if
>>> available) and use those to override the information based on
>>> PVR.
>>> 
>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- 
>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++-- 
>>> target-ppc/cpu.h            |    1 + target-ppc/kvm.c
>>> |   39 +++++++++++++++++++++++++++++++++++++++ 
>>> target-ppc/translate_init.c |    4 ++++ 4 files changed, 62
>>> insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index
>>> 9a13697..7293082 100644 --- a/hw/ppc/spapr.c +++
>>> b/hw/ppc/spapr.c @@ -333,10 +333,26 @@ 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
>>> (env->l1_dcache_size) { +
>>> _FDT((fdt_property_cell(fdt, "d-cache-size",
>>> env->l1_dcache_size))); +        } else { +
>>> fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); +
>>> } +        if (env->l1_icache_size) { +
>>> _FDT((fdt_property_cell(fdt, "i-cache-size",
>>> env->l1_icache_size))); +        } else { +
>>> fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); +
>>> }
>> 
>> The L1 sizes should come from the class, not env, right?
>> Andreas, any ideas on this?
> 
> Well.. initially I was going to put them in class.  But then it 
> occurred to me that the class represents a family of similar CPUs,
> not a single precise CPU model.  Total cache sizes are the sort of
> thing that could easily vary between minor revisions, although I
> don't know if they have in practice.

Actually that is irrelevant: As it stands, we can do neither
model-specific instance_init nor class_init due to the old
POWERPC_DEF_SVR() macros. Adding support for either seems equally
invasive.

As I just replied to Alex, the question to ask is whether you want the
user to fiddle with this value or not.

Andreas

> So I thought it was safer to put these in env.

- -- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nrnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imend￶rffer; HRB 16746 AG Nrnberg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (GNU/Linux)

iQIcBAEBAgAGBQJRRvYUAAoJEPou0S0+fgE/TuUP/2ZqXQw9klNYkkKfeA8lml8R
jX6NsdIGDxAh8BQMea5YEVfTUUqHa5ygO4HMSBT42GvWR4gY7lmDO08d2FrtO3Oz
ybWf1OubcNm3rp9vwtGKKWxWAiMBSjv9KqAR2gYd305e15ADr3wzRPRPyUtOXO6r
ute+snXu3rI49wCngAF3zBhQnq+HLyUcLpMn/ss/p0086e5LaPhPjEjUDRoKLED6
xqjG3x65rknpWdRPWtpZJHEhPfuDBKpZTS0XdPUkBQ48F2D0yZ3SbRHfIjnikEtV
G43RXBYAUK449HmJY0/hz5W51Nm81f1Sg04uXDLsG9K7mBNUcw/TxgjzW+7ByVyk
+zjCHsC0aimX7owIgMPNd8wtWXmxs4JxdC7icjDtZUNKUSFHC6plMPInlZdIkTU5
brJq02I16lQCYHfnFkFdUM+pwV7r4B0FtymirEX3G8l3n5ddoFmj4tUmLPdjxcHL
Dzn5LMOGNsvL0hqf5eW5JCcq4FMGIYKkeo839+p2u4nnoYSpNIB7mseZzzTTGEYH
HLMTXRijpX9Po65fMgPkiyv9Mv3w2N7UjjP2E9a2VJk2oRsEMO0GXrV3OUJBD64U
3qmB+xvjLgijrDvlYTWvZtOhqIRuqjtS+La2qxtbWE8Mf3YYZ1UxVFPd82qQf0zw
luXozSihIlcx55xtw9Lr
=ejJS
-----END PGP SIGNATURE-----
David Gibson - March 19, 2013, 12:52 a.m.
On Mon, Mar 18, 2013 at 12:10:12PM +0100, Andreas Färber wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Am 16.03.2013 08:10, schrieb David Gibson:
> > On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote:
> >> On 14.03.2013, at 02:53, David Gibson wrote:
> >> 
> >>> PAPR requires that the device tree's CPU nodes have several
> >>> properties with information about the L1 cache.  We already
> >>> create 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.
> >>> 
> >>> Adding the cache sizes requires some extra infrastructure in
> >>> the general target-ppc code to (optionally) set the cache sizes
> >>> for various CPUs.  The CPU family descriptions in
> >>> translate_init.c can set these sizes - this patch adds correct
> >>> information for POWER7, I'm leaving other CPU types to people
> >>> who have a physical example to verify against.  In addition,
> >>> for -cpu host we take the values advertised by the host (if
> >>> available) and use those to override the information based on
> >>> PVR.
> >>> 
> >>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- 
> >>> hw/ppc/spapr.c              |   20 ++++++++++++++++++-- 
> >>> target-ppc/cpu.h            |    1 + target-ppc/kvm.c
> >>> |   39 +++++++++++++++++++++++++++++++++++++++ 
> >>> target-ppc/translate_init.c |    4 ++++ 4 files changed, 62
> >>> insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index
> >>> 9a13697..7293082 100644 --- a/hw/ppc/spapr.c +++
> >>> b/hw/ppc/spapr.c @@ -333,10 +333,26 @@ 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
> >>> (env->l1_dcache_size) { +
> >>> _FDT((fdt_property_cell(fdt, "d-cache-size",
> >>> env->l1_dcache_size))); +        } else { +
> >>> fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n"); +
> >>> } +        if (env->l1_icache_size) { +
> >>> _FDT((fdt_property_cell(fdt, "i-cache-size",
> >>> env->l1_icache_size))); +        } else { +
> >>> fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n"); +
> >>> }
> >> 
> >> The L1 sizes should come from the class, not env, right?
> >> Andreas, any ideas on this?
> > 
> > Well.. initially I was going to put them in class.  But then it 
> > occurred to me that the class represents a family of similar CPUs,
> > not a single precise CPU model.  Total cache sizes are the sort of
> > thing that could easily vary between minor revisions, although I
> > don't know if they have in practice.
> 
> Actually that is irrelevant: As it stands, we can do neither
> model-specific instance_init nor class_init due to the old
> POWERPC_DEF_SVR() macros. Adding support for either seems equally
> invasive.
> 
> As I just replied to Alex, the question to ask is whether you want the
> user to fiddle with this value or not.

User to fiddle, probably not.  But I was thinking of submodel specific
conditionals in the init_proc function.
David Gibson - March 19, 2013, 1 a.m.
On Mon, Mar 18, 2013 at 11:54:05AM +0100, Andreas Färber wrote:
> Am 15.03.2013 13:27, schrieb Alexander Graf:
> > 
> > On 14.03.2013, at 02:53, David Gibson wrote:
> > 
> >> PAPR requires that the device tree's CPU nodes have several properties
> >> with information about the L1 cache.  We already create 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.
> >>
> >> Adding the cache sizes requires some extra infrastructure in the
> >> general target-ppc code to (optionally) set the cache sizes for
> >> various CPUs.  The CPU family descriptions in translate_init.c can set
> >> these sizes - this patch adds correct information for POWER7, I'm
> >> leaving other CPU types to people who have a physical example to
> >> verify against.  In addition, for -cpu host we take the values
> >> advertised by the host (if available) and use those to override the
> >> information based on PVR.
> >>
> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >> ---
> >> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
> >> target-ppc/cpu.h            |    1 +
> >> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
> >> target-ppc/translate_init.c |    4 ++++
> >> 4 files changed, 62 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> >> index 9a13697..7293082 100644
> >> --- a/hw/ppc/spapr.c
> >> +++ b/hw/ppc/spapr.c
> >> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
> >> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
> >> +        } else {
> >> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
> >> +        }
> >> +        if (env->l1_icache_size) {
> >> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
> >> +        } else {
> >> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
> >> +        }
> > 
> > The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
> 
> Generally speaking,
> 
> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
> for legacy grouping reasons).
> 
> PowerPCCPU: If you ever intend to let the user override this value
> (per-instance) from the command line.
> 
> PowerPCCPUClass: If the value is always constant at runtime.
> 
> I can't tell from a brief look at this patch which may be the case here.

Ok, on that rationale I'll rework to move it to the class.
Alexander Graf - March 19, 2013, 10:10 a.m.
On 19.03.2013, at 02:00, David Gibson wrote:

> On Mon, Mar 18, 2013 at 11:54:05AM +0100, Andreas Färber wrote:
>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>> 
>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>> 
>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>> with information about the L1 cache.  We already create 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.
>>>> 
>>>> Adding the cache sizes requires some extra infrastructure in the
>>>> general target-ppc code to (optionally) set the cache sizes for
>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>> leaving other CPU types to people who have a physical example to
>>>> verify against.  In addition, for -cpu host we take the values
>>>> advertised by the host (if available) and use those to override the
>>>> information based on PVR.
>>>> 
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>> target-ppc/cpu.h            |    1 +
>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>> target-ppc/translate_init.c |    4 ++++
>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 9a13697..7293082 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>> +        }
>>>> +        if (env->l1_icache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>> +        }
>>> 
>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>> 
>> Generally speaking,
>> 
>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>> for legacy grouping reasons).
>> 
>> PowerPCCPU: If you ever intend to let the user override this value
>> (per-instance) from the command line.
>> 
>> PowerPCCPUClass: If the value is always constant at runtime.
>> 
>> I can't tell from a brief look at this patch which may be the case here.
> 
> Ok, on that rationale I'll rework to move it to the class.

Thanks :). I think we decided a while ago that subversions should also be individual (sub)classes, so that all of this makes sense again ;).


Alex
Andreas Färber - March 19, 2013, 11:06 a.m.
Am 18.03.2013 12:05, schrieb Alexander Graf:
> 
> On 18.03.2013, at 11:54, Andreas Färber wrote:
> 
>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>
>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>
>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>> with information about the L1 cache.  We already create 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.
>>>>
>>>> Adding the cache sizes requires some extra infrastructure in the
>>>> general target-ppc code to (optionally) set the cache sizes for
>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>> leaving other CPU types to people who have a physical example to
>>>> verify against.  In addition, for -cpu host we take the values
>>>> advertised by the host (if available) and use those to override the
>>>> information based on PVR.
>>>>
>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>> ---
>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>> target-ppc/cpu.h            |    1 +
>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>> target-ppc/translate_init.c |    4 ++++
>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>> index 9a13697..7293082 100644
>>>> --- a/hw/ppc/spapr.c
>>>> +++ b/hw/ppc/spapr.c
>>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>> +        }
>>>> +        if (env->l1_icache_size) {
>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>> +        } else {
>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>> +        }
>>>
>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>
>> Generally speaking,
>>
>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>> for legacy grouping reasons).
>>
>> PowerPCCPU: If you ever intend to let the user override this value
>> (per-instance) from the command line.
>>
>> PowerPCCPUClass: If the value is always constant at runtime.
>>
>> I can't tell from a brief look at this patch which may be the case here.
> 
> Imagine the following:
> 
>   PPC
>     `- POWER7
>         |- POWER7_v10
>         `- POWER7_v20
> 
> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.

Alex, you asked for an answer here, but I don't spot a question. :-)

I'm guessing requirements like these mean that we need to introduce a
new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
macro to put additional code into class_init and instance_init
respectively and let _DEF() and _DEF_SVR() pass nothing there...
Possibly add specific macros wrapping the above to keep the model list
readable.

Either way there's a trade-off between keeping easy macros hiding QOM
boilerplate code vs. having high flexibility of what code to put in
there - that's why I resisted your requests to hide too much in macros
at the family level.

Andreas
Alexander Graf - March 19, 2013, 11:09 a.m.
On 19.03.2013, at 12:06, Andreas Färber wrote:

> Am 18.03.2013 12:05, schrieb Alexander Graf:
>> 
>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>> 
>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>> 
>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>> 
>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>> with information about the L1 cache.  We already create 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.
>>>>> 
>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>> leaving other CPU types to people who have a physical example to
>>>>> verify against.  In addition, for -cpu host we take the values
>>>>> advertised by the host (if available) and use those to override the
>>>>> information based on PVR.
>>>>> 
>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>> ---
>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>> target-ppc/cpu.h            |    1 +
>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>> target-ppc/translate_init.c |    4 ++++
>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>> index 9a13697..7293082 100644
>>>>> --- a/hw/ppc/spapr.c
>>>>> +++ b/hw/ppc/spapr.c
>>>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>> +        } else {
>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>> +        }
>>>>> +        if (env->l1_icache_size) {
>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>> +        } else {
>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>> +        }
>>>> 
>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>> 
>>> Generally speaking,
>>> 
>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>> for legacy grouping reasons).
>>> 
>>> PowerPCCPU: If you ever intend to let the user override this value
>>> (per-instance) from the command line.
>>> 
>>> PowerPCCPUClass: If the value is always constant at runtime.
>>> 
>>> I can't tell from a brief look at this patch which may be the case here.
>> 
>> Imagine the following:
>> 
>>  PPC
>>    `- POWER7
>>        |- POWER7_v10
>>        `- POWER7_v20
>> 
>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
> 
> Alex, you asked for an answer here, but I don't spot a question. :-)
> 
> I'm guessing requirements like these mean that we need to introduce a
> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
> macro to put additional code into class_init and instance_init
> respectively and let _DEF() and _DEF_SVR() pass nothing there...
> Possibly add specific macros wrapping the above to keep the model list
> readable.
> 
> Either way there's a trade-off between keeping easy macros hiding QOM
> boilerplate code vs. having high flexibility of what code to put in
> there - that's why I resisted your requests to hide too much in macros
> at the family level.

Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)


Alex
Andreas Färber - March 19, 2013, 11:16 a.m.
Am 19.03.2013 12:09, schrieb Alexander Graf:
> 
> On 19.03.2013, at 12:06, Andreas Färber wrote:
> 
>> Am 18.03.2013 12:05, schrieb Alexander Graf:
>>>
>>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>>>
>>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>>>
>>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>>>
>>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>>> with information about the L1 cache.  We already create 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.
>>>>>>
>>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>>> leaving other CPU types to people who have a physical example to
>>>>>> verify against.  In addition, for -cpu host we take the values
>>>>>> advertised by the host (if available) and use those to override the
>>>>>> information based on PVR.
>>>>>>
>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>> ---
>>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>>> target-ppc/cpu.h            |    1 +
>>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>>> target-ppc/translate_init.c |    4 ++++
>>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>> index 9a13697..7293082 100644
>>>>>> --- a/hw/ppc/spapr.c
>>>>>> +++ b/hw/ppc/spapr.c
>>>>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>>> +        } else {
>>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>>> +        }
>>>>>> +        if (env->l1_icache_size) {
>>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>>> +        } else {
>>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>>> +        }
>>>>>
>>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>>>
>>>> Generally speaking,
>>>>
>>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>>> for legacy grouping reasons).
>>>>
>>>> PowerPCCPU: If you ever intend to let the user override this value
>>>> (per-instance) from the command line.
>>>>
>>>> PowerPCCPUClass: If the value is always constant at runtime.
>>>>
>>>> I can't tell from a brief look at this patch which may be the case here.
>>>
>>> Imagine the following:
>>>
>>>  PPC
>>>    `- POWER7
>>>        |- POWER7_v10
>>>        `- POWER7_v20
>>>
>>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
>>
>> Alex, you asked for an answer here, but I don't spot a question. :-)
>>
>> I'm guessing requirements like these mean that we need to introduce a
>> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
>> macro to put additional code into class_init and instance_init
>> respectively and let _DEF() and _DEF_SVR() pass nothing there...
>> Possibly add specific macros wrapping the above to keep the model list
>> readable.
>>
>> Either way there's a trade-off between keeping easy macros hiding QOM
>> boilerplate code vs. having high flexibility of what code to put in
>> there - that's why I resisted your requests to hide too much in macros
>> at the family level.
> 
> Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)

A function would work as well, yes. But unless you've reworked the list
you do need the SVR in the base macro.
For the case at hand you only need code for class_init but I was
thinking ahead to also prepare the instance_init equivalent while at it. ;)

Andreas
Alexander Graf - March 19, 2013, 11:37 a.m.
On 19.03.2013, at 12:16, Andreas Färber wrote:

> Am 19.03.2013 12:09, schrieb Alexander Graf:
>> 
>> On 19.03.2013, at 12:06, Andreas Färber wrote:
>> 
>>> Am 18.03.2013 12:05, schrieb Alexander Graf:
>>>> 
>>>> On 18.03.2013, at 11:54, Andreas Färber wrote:
>>>> 
>>>>> Am 15.03.2013 13:27, schrieb Alexander Graf:
>>>>>> 
>>>>>> On 14.03.2013, at 02:53, David Gibson wrote:
>>>>>> 
>>>>>>> PAPR requires that the device tree's CPU nodes have several properties
>>>>>>> with information about the L1 cache.  We already create 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.
>>>>>>> 
>>>>>>> Adding the cache sizes requires some extra infrastructure in the
>>>>>>> general target-ppc code to (optionally) set the cache sizes for
>>>>>>> various CPUs.  The CPU family descriptions in translate_init.c can set
>>>>>>> these sizes - this patch adds correct information for POWER7, I'm
>>>>>>> leaving other CPU types to people who have a physical example to
>>>>>>> verify against.  In addition, for -cpu host we take the values
>>>>>>> advertised by the host (if available) and use those to override the
>>>>>>> information based on PVR.
>>>>>>> 
>>>>>>> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>>>>>>> ---
>>>>>>> hw/ppc/spapr.c              |   20 ++++++++++++++++++--
>>>>>>> target-ppc/cpu.h            |    1 +
>>>>>>> target-ppc/kvm.c            |   39 +++++++++++++++++++++++++++++++++++++++
>>>>>>> target-ppc/translate_init.c |    4 ++++
>>>>>>> 4 files changed, 62 insertions(+), 2 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>> index 9a13697..7293082 100644
>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>> @@ -333,10 +333,26 @@ 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 (env->l1_dcache_size) {
>>>>>>> +            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
>>>>>>> +        } else {
>>>>>>> +            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
>>>>>>> +        }
>>>>>>> +        if (env->l1_icache_size) {
>>>>>>> +            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
>>>>>>> +        } else {
>>>>>>> +            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
>>>>>>> +        }
>>>>>> 
>>>>>> The L1 sizes should come from the class, not env, right? Andreas, any ideas on this?
>>>>> 
>>>>> Generally speaking,
>>>>> 
>>>>> CPUPPCState: Only if this is used for TCG with an offset from AREG0 (or
>>>>> for legacy grouping reasons).
>>>>> 
>>>>> PowerPCCPU: If you ever intend to let the user override this value
>>>>> (per-instance) from the command line.
>>>>> 
>>>>> PowerPCCPUClass: If the value is always constant at runtime.
>>>>> 
>>>>> I can't tell from a brief look at this patch which may be the case here.
>>>> 
>>>> Imagine the following:
>>>> 
>>>> PPC
>>>>   `- POWER7
>>>>       |- POWER7_v10
>>>>       `- POWER7_v20
>>>> 
>>>> Version 1.0 has L1 cache size of x MB. Version 2.0 has L1 cache size of y MB. The user should never override the value (except with -cpu host). It is constant during the lifetime of a VM.
>>> 
>>> Alex, you asked for an answer here, but I don't spot a question. :-)
>>> 
>>> I'm guessing requirements like these mean that we need to introduce a
>>> new POWERPC_DEF_SVR_CLS_INST(name, pvr, svr, type, snippet1, snippet2)
>>> macro to put additional code into class_init and instance_init
>>> respectively and let _DEF() and _DEF_SVR() pass nothing there...
>>> Possibly add specific macros wrapping the above to keep the model list
>>> readable.
>>> 
>>> Either way there's a trade-off between keeping easy macros hiding QOM
>>> boilerplate code vs. having high flexibility of what code to put in
>>> there - that's why I resisted your requests to hide too much in macros
>>> at the family level.
>> 
>> Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which would pass in a model specific initialization function in addition to the easy to read defaults? :)
> 
> A function would work as well, yes. But unless you've reworked the list
> you do need the SVR in the base macro.
> For the case at hand you only need code for class_init but I was
> thinking ahead to also prepare the instance_init equivalent while at it. ;)

What does that do? Populate env variables with defaults depending on the class?


Alex

Patch

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 9a13697..7293082 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -333,10 +333,26 @@  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 (env->l1_dcache_size) {
+            _FDT((fdt_property_cell(fdt, "d-cache-size", env->l1_dcache_size)));
+        } else {
+            fprintf(stderr, "Warning: Unknown L1 dcache size for cpu\n");
+        }
+        if (env->l1_icache_size) {
+            _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_icache_size)));
+        } else {
+            fprintf(stderr, "Warning: Unknown L1 icache size for cpu\n");
+        }
+
         _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 0b2d0b9..793ae0f 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -994,6 +994,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 f5710b7..eaa11a1 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1612,14 +1612,49 @@  static void alter_insns(uint64_t *word, uint64_t flags, bool on)
     }
 }
 
+typedef struct PowerPCHostCPUClass {
+    /*< private >*/
+    PowerPCCPUClass parent_class;
+    /*< public >*/
+
+    DeviceRealize parent_realize;
+} PowerPCHostCPUClass;
+
+#define POWERPC_HOST_CPU_CLASS(klass) \
+    OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU)
+#define POWERPC_HOST_CPU_GET_CLASS(obj) \
+    OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU)
+
 static void kvmppc_host_cpu_initfn(Object *obj)
 {
     assert(kvm_enabled());
 }
 
+static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(dev);
+    CPUPPCState *env = &cpu->env;
+    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev);
+    uint32_t val;
+
+    (*hpcc->parent_realize)(dev, errp);
+
+    val = kvmppc_read_int_cpu_dt("d-cache-size");
+    if (val != -1) {
+        env->l1_dcache_size = val;
+    }
+
+    val = kvmppc_read_int_cpu_dt("i-cache-size");
+    if (val != -1) {
+        env->l1_icache_size = val;
+    }
+}
+
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
 {
+    PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc);
     PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    DeviceClass *dc = DEVICE_CLASS(oc);
     uint32_t vmx = kvmppc_get_vmx();
     uint32_t dfp = kvmppc_get_dfp();
 
@@ -1634,6 +1669,9 @@  static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
         /* Only override when we know what the host supports */
         alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp);
     }
+
+    hpcc->parent_realize = dc->realize;
+    dc->realize = kvmppc_host_cpu_realizefn;
 }
 
 int kvmppc_fixup_cpu(PowerPCCPU *cpu)
@@ -1654,6 +1692,7 @@  static int kvm_ppc_register_host_cpu_type(void)
     TypeInfo type_info = {
         .name = TYPE_HOST_POWERPC_CPU,
         .instance_init = kvmppc_host_cpu_initfn,
+        .class_size = sizeof(PowerPCHostCPUClass),
         .class_init = kvmppc_host_cpu_class_init,
     };
     uint32_t host_pvr = mfpvr();
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 3cf440e..594053e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -6989,6 +6989,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