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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4206 bytes --]

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.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]