[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [RFC NO-MERGE 05/12] target/ppc: Add ibm, processor-radix
From: |
David Gibson |
Subject: |
Re: [Qemu-ppc] [RFC NO-MERGE 05/12] target/ppc: Add ibm, processor-radix-AP-encodings to cpu device tree node |
Date: |
Mon, 27 Feb 2017 13:07:04 +1100 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Fri, Feb 24, 2017 at 06:03:12PM +1100, Suraj Jitindar Singh wrote:
> On Mon, 2017-02-20 at 12:03 +1100, David Gibson wrote:
> > On Fri, Feb 17, 2017 at 04:08:05PM +1100, Suraj Jitindar Singh wrote:
> > >
> > > The ibm,processor-radix-AP-encodings device tree property of the
> > > cpu node
> > > is used to specify the radix mode supported page sizes of the
> > > processor
> > > to the guest os. Contained in the top 3 bits of the msb is the
> > > actual
> > > page size (AP) encoding associated with the corresponding radix
> > > mode
> > > supported page size.
> > >
> > > As the supported page sizes are implementation dependant, we add a
> > > radix
> > > page size (rps) member to the PowerPCCPUClass struct which can be
> > > assigned
> > > on a per processor basis based on the supported page sizes. Add an
> > > array
> > > for the POWER9 supported page sizes and assign this in the POWER9
> > > cpu
> > > definition accordingly.
> > >
> > > We need a way to add this to the device tree on boot. Add a
> > > CPUPPCState
> > > struct member to contain the rps encodings and assign it if the
> > > member
> > What's the reason for putting the encodings in the CPUPPCState,
> > rather
> > than accessing them directly from the PowerPCCPUClass?
>
> None really, this is just how other stuff was done previously. I'll
> hook this into samb's stuff anyway.
>
> >
> > >
> > > of the cpu class has been assigned, otherwise we ignore this -
> > > older cpus
> > > which don't assign this in the cpu class definition don't support
> > > radix so
> > > they obviously need it, if we do know about radix then linux will
> > > just
> > > assume some defaults so it's fine to leave this blank in the
> > > default case.
> > I'm confused by this. We're just adding the first radix CPU now and
> > we're adding the encoding table. Why would the case of a radix
> > capable CPU without an encodings list ever arise?
>
> I confused myself I think... I'll reword for better english.
>
> >
> > >
> > > When we construct the device tree on cpu init we check the cpu
> > > state for
> > > an array of rps encodings, if we find it then parse the array and
> > > create
> > > the device tree property accordingly.
> > >
> > > The ibm,processor-radix-AP-encodings device tree property is
> > > defined as:
> > > One to n cells in ascending order of radix mode supported page
> > > sizes
> > > encoded as BE ints (32bit on ppc) in the form:
> > > 0bxxxyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> > > - 0bxxx -> AP encoding
> > > - 0byyyyyyyyyyyyyyyyyyyyyyyyyyyyy -> supported page size
> > How is page size encoded? I see from the code below that it's as a
> > shift, but you should state that here.
>
> Will do
>
> >
> > >
> > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > > ---
> > > hw/ppc/spapr.c | 20 ++++++++++++++++++++
> > > target/ppc/cpu-qom.h | 1 +
> > > target/ppc/cpu.h | 1 +
> > > target/ppc/translate_init.c | 20 ++++++++++++++++++++
> > > 4 files changed, 42 insertions(+)
> > >
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 441d39b..3214df6 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -503,6 +503,26 @@ static void spapr_populate_cpu_dt(CPUState
> > > *cs, void *fdt, int offset,
> > > page_sizes_prop,
> > > page_sizes_prop_size)));
> > > }
> > >
> > > + if (env->rps) {
> > > + size_t prop_size = 0;
> > > + int32_t *rps_prop;
> > You have a bunch of tabs here, which breaks qemu coding standard.
> > Also the mixture of tabs and spaces on different lines makes the diff
> > harder to read.
>
> Qemu coding standard breaks me...
>
> >
> > >
> > > + int i = 0;
> > > + /* Calculate size of property array, we know it's null
> > > terminated */
> > > + for (; env->rps[i++]; prop_size += sizeof(*env->rps)) {}
> > > + rps_prop = g_malloc0(prop_size);
> > > + if (!rps_prop) {
> > > + error_report("Could not alloc mem for radix encodings
> > > property\n");
> > > + exit(1);
> > I'm pretty sure g_malloc() will already abort on failure, you don't
> > need this check.
>
> I think that's g_try_malloc()?
No, other way around. As the name suggests g_try_malloc() can return
NULL on failure. g_malloc() will abort on failure.
See
https://developer.gnome.org/glib/2.28/glib-Memory-Allocation.html#g-malloc
>
> >
> > >
> > > + }
> > > + /* Convert to correct endianness */
> > > + for (i = 0; i < (prop_size/sizeof(*rps_prop)); i++) {
> > > + rps_prop[i] = cpu_to_be32(env->rps[i]);
> > > + }
> > > + _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-
> > > encodings",
> > > + rps_prop, prop_size)));
> > > + g_free(rps_prop);
> > > + }
> > > +
> > > spapr_populate_pa_features(env, fdt, offset);
> > >
> > > _FDT((fdt_setprop_cell(fdt, offset, "ibm,chip-id",
> > > diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> > > index 4e3132b..23a2a4e 100644
> > > --- a/target/ppc/cpu-qom.h
> > > +++ b/target/ppc/cpu-qom.h
> > > @@ -195,6 +195,7 @@ typedef struct PowerPCCPUClass {
> > > int bfd_mach;
> > > uint32_t l1_dcache_size, l1_icache_size;
> > > const struct ppc_segment_page_sizes *sps;
> > > + int32_t *rps; /* Radix Page Sizes */
> > > void (*init_proc)(CPUPPCState *env);
> > > int (*check_pow)(CPUPPCState *env);
> > > int (*handle_mmu_fault)(PowerPCCPU *cpu, vaddr eaddr, int rwx,
> > > int mmu_idx);
> > > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > > index 0fd352e..224873d 100644
> > > --- a/target/ppc/cpu.h
> > > +++ b/target/ppc/cpu.h
> > > @@ -1070,6 +1070,7 @@ struct CPUPPCState {
> > > uint64_t insns_flags2;
> > > #if defined(TARGET_PPC64)
> > > struct ppc_segment_page_sizes sps;
> > > + int32_t *rps; /* Radix Page Sizes */
> > > ppc_slb_t vrma_slb;
> > > target_ulong rmls;
> > > bool ci_large_pages;
> > > diff --git a/target/ppc/translate_init.c
> > > b/target/ppc/translate_init.c
> > > index cc8ab1f..66a7f4a 100644
> > > --- a/target/ppc/translate_init.c
> > > +++ b/target/ppc/translate_init.c
> > > @@ -8488,6 +8488,7 @@ static Property
> > > powerpc_servercpu_properties[] = {
> > > };
> > >
> > > #ifdef CONFIG_SOFTMMU
> > > +/* Segment Page Sizes for dt node ibm,segment-page-sizes */
> > > static const struct ppc_segment_page_sizes POWER7_POWER8_sps = {
> > > .sps = {
> > > {
> > > @@ -8515,6 +8516,21 @@ static const struct ppc_segment_page_sizes
> > > POWER7_POWER8_sps = {
> > > },
> > > }
> > > };
> > > +
> > > +/*
> > > + * Radix pg sizes and AP encodings for dt node ibm,processor-
> > > radix-AP-encodings
> > > + * Encoded as array of int_32s in the form:
> > > + * 0bxxxyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
> > > + * x -> AP encoding
> > > + * y -> radix mode supported page size
> > > + */
> > > +static int32_t POWER9_rps[] = {
> > > + 0x0000000c, /* 4K - enc: 0x0 */
> > > + 0xa0000010, /* 64K - enc: 0x5 */
> > > + 0x20000015, /* 2M - enc: 0x1 */
> > > + 0x4000001e, /* 1G - enc: 0x2 */
> > > + 0x0 /* END */
> > > +};
> > > #endif /* CONFIG_SOFTMMU */
> > >
> > > static void init_proc_POWER7 (CPUPPCState *env)
> > > @@ -8877,6 +8893,7 @@ POWERPC_FAMILY(POWER9)(ObjectClass *oc, void
> > > *data)
> > > pcc->handle_mmu_fault = ppc64_v3_handle_mmu_fault;
> > > /* segment page size remain the same */
> > > pcc->sps = &POWER7_POWER8_sps;
> > > + pcc->rps = POWER9_rps;
> > > #endif
> > > pcc->excp_model = POWERPC_EXCP_POWER8;
> > > pcc->bus_model = PPC_FLAGS_INPUT_POWER7;
> > > @@ -10488,6 +10505,9 @@ static void ppc_cpu_initfn(Object *obj)
> > > };
> > > env->sps = (env->mmu_model & POWERPC_MMU_64K) ? defsps_64k
> > > : defsps_4k;
> > > }
> > > + if (pcc->rps) {
> > > + env->rps = pcc->rps;
> > > + } /* Ignore this if not defined in cpu class, kernel will
> > > assume defaults */
> > > #endif /* defined(TARGET_PPC64) */
> > >
> > > if (tcg_enabled()) {
>
--
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
signature.asc
Description: PGP signature