From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56983) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHuNa-0002ky-Un for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:06:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHuNX-0007zj-Om for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:06:54 -0400 Message-ID: <514846C7.1030608@suse.de> Date: Tue, 19 Mar 2013 12:06:47 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 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> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable 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: Alexander Graf Cc: "qemu-ppc@nongnu.org" , qemu-devel , David Gibson 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: >>> >>> On 14.03.2013, at 02:53, David Gibson wrote: >>> >>>> PAPR requires that the device tree's CPU nodes have several properti= es >>>> with information about the L1 cache. We already create two of these >>>> properties, but with incorrect names - "[id]cache-block-size" instea= d >>>> 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 s= et >>>> 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 >>>> --- >>>> 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_dc= ache_size))); >>>> + } else { >>>> + fprintf(stderr, "Warning: Unknown L1 dcache size for cp= u\n"); >>>> + } >>>> + if (env->l1_icache_size) { >>>> + _FDT((fdt_property_cell(fdt, "i-cache-size", env->l1_ic= ache_size))); >>>> + } else { >>>> + fprintf(stderr, "Warning: Unknown L1 icache size for cp= u\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 (o= r >> 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 her= e. >=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. 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 --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg