From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:59569) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UHuX2-0006tE-Fo for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:16:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UHuWu-00038T-1p for qemu-devel@nongnu.org; Tue, 19 Mar 2013 07:16:40 -0400 Message-ID: <5148490C.6060905@suse.de> Date: Tue, 19 Mar 2013 12:16:28 +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> <514846C7.1030608@suse.de> <3C479A24-2168-4349-B3E0-E7F5E32ADA0D@suse.de> In-Reply-To: <3C479A24-2168-4349-B3E0-E7F5E32ADA0D@suse.de> 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 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: >>> >>> On 18.03.2013, at 11:54, Andreas F=E4rber 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 proper= ties >>>>>> with information about the L1 cache. We already create two of the= se >>>>>> properties, but with incorrect names - "[id]cache-block-size" inst= ead >>>>>> 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 sam= e >>>>>> values as the block size properties in all current cases). We als= o >>>>>> 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 th= e >>>>>> 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 cha= r *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, a= ny 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 h= ere. >>> >>> 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. >=20 > Can't we just do a POWERPC_DEF_FUNC(name, pvr, type, init_func) which w= ould 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 --=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