Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties

[Alexander Graf]

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 <address@hidden>
>>>>>>> ---
>>>>>>> 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