From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:37143) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHuqs-0004I2-Uo for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:37:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHuqo-0002JQ-EZ for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:37:10 -0400 Mime-Version: 1.0 (Apple Message framework v1278) Content-Type: text/plain; charset=iso-8859-1 From: Alexander Graf In-Reply-To: <5148490C.6060905@suse.de> Date: Tue, 19 Mar 2013 12:37:03 +0100 Content-Transfer-Encoding: quoted-printable Message-Id: <6F892025-470B-4696-AD1B-29F7B9754513@suse.de> References: <1363226008-26639-1-git-send-email-david@gibson.dropbear.id.au> <1363226008-26639-4-git-send-email-david@gibson.dropbear.id.au> <5146F24D.7090003@suse.de> <514846C7.1030608@suse.de> <3C479A24-2168-4349-B3E0-E7F5E32ADA0D@suse.de> <5148490C.6060905@suse.de> Subject: Re: [Qemu-devel] [PATCH 3/5] pseries: Fixes and enhancements to L1 cache properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?iso-8859-1?Q?Andreas_F=E4rber?= Cc: "qemu-ppc@nongnu.org" , qemu-devel , David Gibson On 19.03.2013, at 12:16, Andreas F=E4rber wrote: > Am 19.03.2013 12:09, schrieb Alexander Graf: >>=20 >> On 19.03.2013, at 12:06, Andreas F=E4rber wrote: >>=20 >>> Am 18.03.2013 12:05, schrieb Alexander Graf: >>>>=20 >>>> On 18.03.2013, at 11:54, Andreas F=E4rber wrote: >>>>=20 >>>>> Am 15.03.2013 13:27, schrieb Alexander Graf: >>>>>>=20 >>>>>> On 14.03.2013, at 02:53, David Gibson wrote: >>>>>>=20 >>>>>>> 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). >>>>>>>=20 >>>>>>> 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. >>>>>>>=20 >>>>>>> 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. >>>>>>>=20 >>>>>>> Signed-off-by: David Gibson >>>>>>> --- >>>>>>> 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(-) >>>>>>>=20 >>>>>>> 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"))); >>>>>>>=20 >>>>>>> _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"); >>>>>>> + } >>>>>>=20 >>>>>> The L1 sizes should come from the class, not env, right? Andreas, = any ideas on this? >>>>>=20 >>>>> Generally speaking, >>>>>=20 >>>>> CPUPPCState: Only if this is used for TCG with an offset from = AREG0 (or >>>>> for legacy grouping reasons). >>>>>=20 >>>>> PowerPCCPU: If you ever intend to let the user override this value >>>>> (per-instance) from the command line. >>>>>=20 >>>>> PowerPCCPUClass: If the value is always constant at runtime. >>>>>=20 >>>>> I can't tell from a brief look at this patch which may be the case = here. >>>>=20 >>>> Imagine the following: >>>>=20 >>>> PPC >>>> `- POWER7 >>>> |- POWER7_v10 >>>> `- POWER7_v20 >>>>=20 >>>> 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. >>>=20 >>> Alex, you asked for an answer here, but I don't spot a question. :-) >>>=20 >>> 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. >>>=20 >>> 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. >>=20 >> 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? :) >=20 > 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