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

[Andreas Färber]

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.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg