| Message ID | 1363226008-26639-4-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
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 <david@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? Alex > + > _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq))); > _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq))); > _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr))); > diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h > index 0b2d0b9..793ae0f 100644 > --- a/target-ppc/cpu.h > +++ b/target-ppc/cpu.h > @@ -994,6 +994,7 @@ struct CPUPPCState { > > int dcache_line_size; > int icache_line_size; > + int l1_dcache_size, l1_icache_size; > > /* Those resources are used during exception processing */ > /* CPU model definition */ > diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c > index f5710b7..eaa11a1 100644 > --- a/target-ppc/kvm.c > +++ b/target-ppc/kvm.c > @@ -1612,14 +1612,49 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on) > } > } > > +typedef struct PowerPCHostCPUClass { > + /*< private >*/ > + PowerPCCPUClass parent_class; > + /*< public >*/ > + > + DeviceRealize parent_realize; > +} PowerPCHostCPUClass; > + > +#define POWERPC_HOST_CPU_CLASS(klass) \ > + OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU) > +#define POWERPC_HOST_CPU_GET_CLASS(obj) \ > + OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU) > + > static void kvmppc_host_cpu_initfn(Object *obj) > { > assert(kvm_enabled()); > } > > +static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp) > +{ > + PowerPCCPU *cpu = POWERPC_CPU(dev); > + CPUPPCState *env = &cpu->env; > + PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev); > + uint32_t val; > + > + (*hpcc->parent_realize)(dev, errp); > + > + val = kvmppc_read_int_cpu_dt("d-cache-size"); > + if (val != -1) { > + env->l1_dcache_size = val; > + } > + > + val = kvmppc_read_int_cpu_dt("i-cache-size"); > + if (val != -1) { > + env->l1_icache_size = val; > + } > +} > + > static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) > { > + PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc); > PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); > + DeviceClass *dc = DEVICE_CLASS(oc); > uint32_t vmx = kvmppc_get_vmx(); > uint32_t dfp = kvmppc_get_dfp(); > > @@ -1634,6 +1669,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) > /* Only override when we know what the host supports */ > alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp); > } > + > + hpcc->parent_realize = dc->realize; > + dc->realize = kvmppc_host_cpu_realizefn; > } > > int kvmppc_fixup_cpu(PowerPCCPU *cpu) > @@ -1654,6 +1692,7 @@ static int kvm_ppc_register_host_cpu_type(void) > TypeInfo type_info = { > .name = TYPE_HOST_POWERPC_CPU, > .instance_init = kvmppc_host_cpu_initfn, > + .class_size = sizeof(PowerPCHostCPUClass), > .class_init = kvmppc_host_cpu_class_init, > }; > uint32_t host_pvr = mfpvr(); > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c > index 3cf440e..594053e 100644 > --- a/target-ppc/translate_init.c > +++ b/target-ppc/translate_init.c > @@ -6989,6 +6989,10 @@ static void init_proc_POWER7 (CPUPPCState *env) > init_excp_POWER7(env); > env->dcache_line_size = 128; > env->icache_line_size = 128; > + > + env->l1_dcache_size = 0x8000; > + env->l1_icache_size = 0x8000; > + > /* Allocate hardware IRQ controller */ > ppcPOWER7_irq_init(env); > /* Can't find information on what this should be on reset. This > -- > 1.7.10.4 >
On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote: > 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 <david@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? Well.. initially I was going to put them in class. But then it occurred to me that the class represents a family of similar CPUs, not a single precise CPU model. Total cache sizes are the sort of thing that could easily vary between minor revisions, although I don't know if they have in practice. So I thought it was safer to put these in env.
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 <david@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. Andreas
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 <david@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
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 16.03.2013 08:10, schrieb David Gibson: > On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote: >> 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 <david@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? > > Well.. initially I was going to put them in class. But then it > occurred to me that the class represents a family of similar CPUs, > not a single precise CPU model. Total cache sizes are the sort of > thing that could easily vary between minor revisions, although I > don't know if they have in practice. Actually that is irrelevant: As it stands, we can do neither model-specific instance_init nor class_init due to the old POWERPC_DEF_SVR() macros. Adding support for either seems equally invasive. As I just replied to Alex, the question to ask is whether you want the user to fiddle with this value or not. Andreas > So I thought it was safer to put these in env. - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendrffer; HRB 16746 AG Nrnberg -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.19 (GNU/Linux) iQIcBAEBAgAGBQJRRvYUAAoJEPou0S0+fgE/TuUP/2ZqXQw9klNYkkKfeA8lml8R jX6NsdIGDxAh8BQMea5YEVfTUUqHa5ygO4HMSBT42GvWR4gY7lmDO08d2FrtO3Oz ybWf1OubcNm3rp9vwtGKKWxWAiMBSjv9KqAR2gYd305e15ADr3wzRPRPyUtOXO6r ute+snXu3rI49wCngAF3zBhQnq+HLyUcLpMn/ss/p0086e5LaPhPjEjUDRoKLED6 xqjG3x65rknpWdRPWtpZJHEhPfuDBKpZTS0XdPUkBQ48F2D0yZ3SbRHfIjnikEtV G43RXBYAUK449HmJY0/hz5W51Nm81f1Sg04uXDLsG9K7mBNUcw/TxgjzW+7ByVyk +zjCHsC0aimX7owIgMPNd8wtWXmxs4JxdC7icjDtZUNKUSFHC6plMPInlZdIkTU5 brJq02I16lQCYHfnFkFdUM+pwV7r4B0FtymirEX3G8l3n5ddoFmj4tUmLPdjxcHL Dzn5LMOGNsvL0hqf5eW5JCcq4FMGIYKkeo839+p2u4nnoYSpNIB7mseZzzTTGEYH HLMTXRijpX9Po65fMgPkiyv9Mv3w2N7UjjP2E9a2VJk2oRsEMO0GXrV3OUJBD64U 3qmB+xvjLgijrDvlYTWvZtOhqIRuqjtS+La2qxtbWE8Mf3YYZ1UxVFPd82qQf0zw luXozSihIlcx55xtw9Lr =ejJS -----END PGP SIGNATURE-----
On Mon, Mar 18, 2013 at 12:10:12PM +0100, Andreas Färber wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > Am 16.03.2013 08:10, schrieb David Gibson: > > On Fri, Mar 15, 2013 at 01:27:09PM +0100, Alexander Graf wrote: > >> 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 <david@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? > > > > Well.. initially I was going to put them in class. But then it > > occurred to me that the class represents a family of similar CPUs, > > not a single precise CPU model. Total cache sizes are the sort of > > thing that could easily vary between minor revisions, although I > > don't know if they have in practice. > > Actually that is irrelevant: As it stands, we can do neither > model-specific instance_init nor class_init due to the old > POWERPC_DEF_SVR() macros. Adding support for either seems equally > invasive. > > As I just replied to Alex, the question to ask is whether you want the > user to fiddle with this value or not. User to fiddle, probably not. But I was thinking of submodel specific conditionals in the init_proc function.
On Mon, Mar 18, 2013 at 11:54:05AM +0100, 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 <david@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. Ok, on that rationale I'll rework to move it to the class.
On 19.03.2013, at 02:00, David Gibson wrote: > On Mon, Mar 18, 2013 at 11:54:05AM +0100, 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 <david@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. > > Ok, on that rationale I'll rework to move it to the class. Thanks :). I think we decided a while ago that subversions should also be individual (sub)classes, so that all of this makes sense again ;). Alex
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 <david@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. Andreas
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 <david@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
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 <david@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? :) 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
On 19.03.2013, at 12:16, Andreas Färber wrote: > 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 <david@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? :) > > 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
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"); + } + _FDT((fdt_property_cell(fdt, "timebase-frequency", tbfreq))); _FDT((fdt_property_cell(fdt, "clock-frequency", cpufreq))); _FDT((fdt_property_cell(fdt, "ibm,slb-size", env->slb_nr))); diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h index 0b2d0b9..793ae0f 100644 --- a/target-ppc/cpu.h +++ b/target-ppc/cpu.h @@ -994,6 +994,7 @@ struct CPUPPCState { int dcache_line_size; int icache_line_size; + int l1_dcache_size, l1_icache_size; /* Those resources are used during exception processing */ /* CPU model definition */ diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c index f5710b7..eaa11a1 100644 --- a/target-ppc/kvm.c +++ b/target-ppc/kvm.c @@ -1612,14 +1612,49 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on) } } +typedef struct PowerPCHostCPUClass { + /*< private >*/ + PowerPCCPUClass parent_class; + /*< public >*/ + + DeviceRealize parent_realize; +} PowerPCHostCPUClass; + +#define POWERPC_HOST_CPU_CLASS(klass) \ + OBJECT_CLASS_CHECK(PowerPCHostCPUClass, (klass), TYPE_HOST_POWERPC_CPU) +#define POWERPC_HOST_CPU_GET_CLASS(obj) \ + OBJECT_GET_CLASS(PowerPCHostCPUClass, (obj), TYPE_HOST_POWERPC_CPU) + static void kvmppc_host_cpu_initfn(Object *obj) { assert(kvm_enabled()); } +static void kvmppc_host_cpu_realizefn(DeviceState *dev, Error **errp) +{ + PowerPCCPU *cpu = POWERPC_CPU(dev); + CPUPPCState *env = &cpu->env; + PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_GET_CLASS(dev); + uint32_t val; + + (*hpcc->parent_realize)(dev, errp); + + val = kvmppc_read_int_cpu_dt("d-cache-size"); + if (val != -1) { + env->l1_dcache_size = val; + } + + val = kvmppc_read_int_cpu_dt("i-cache-size"); + if (val != -1) { + env->l1_icache_size = val; + } +} + static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) { + PowerPCHostCPUClass *hpcc = POWERPC_HOST_CPU_CLASS(oc); PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc); + DeviceClass *dc = DEVICE_CLASS(oc); uint32_t vmx = kvmppc_get_vmx(); uint32_t dfp = kvmppc_get_dfp(); @@ -1634,6 +1669,9 @@ static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data) /* Only override when we know what the host supports */ alter_insns(&pcc->insns_flags2, PPC2_DFP, dfp); } + + hpcc->parent_realize = dc->realize; + dc->realize = kvmppc_host_cpu_realizefn; } int kvmppc_fixup_cpu(PowerPCCPU *cpu) @@ -1654,6 +1692,7 @@ static int kvm_ppc_register_host_cpu_type(void) TypeInfo type_info = { .name = TYPE_HOST_POWERPC_CPU, .instance_init = kvmppc_host_cpu_initfn, + .class_size = sizeof(PowerPCHostCPUClass), .class_init = kvmppc_host_cpu_class_init, }; uint32_t host_pvr = mfpvr(); diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c index 3cf440e..594053e 100644 --- a/target-ppc/translate_init.c +++ b/target-ppc/translate_init.c @@ -6989,6 +6989,10 @@ static void init_proc_POWER7 (CPUPPCState *env) init_excp_POWER7(env); env->dcache_line_size = 128; env->icache_line_size = 128; + + env->l1_dcache_size = 0x8000; + env->l1_icache_size = 0x8000; + /* Allocate hardware IRQ controller */ ppcPOWER7_irq_init(env); /* Can't find information on what this should be on reset. This
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 <david@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(-)