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

[Suraj Jitindar Singh]

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()?

> 
> > 
> > +        }
> > +        /* 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()) {