Re: [Qemu-ppc] [RFC NO-MERGE 05/12] target/ppc: Add ibm, processor-radix-AP-encodings to cpu device tree node

[David Gibson]

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