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