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

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

> 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.

> 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.

> +     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.

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