qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Andreas Färber
Subject: Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties
Date: Tue, 19 Mar 2013 12:16:28 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130307 Thunderbird/17.0.4

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. ;)

Andreas

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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]