Re: [Qemu-ppc] [RFC PATCH 3/9] spapr: Add ibm, processor-radix-AP-encodings to the device tree

[David Gibson]

On Tue, Feb 07, 2017 at 01:56:46PM +1100, Sam Bobroff wrote:
> Use the new ioctl, KVM_PPC_GET_RMMU_INFO, to fetch radix MMU
> information from KVM and present the page encodings in the device tree
> under ibm,processor-radix-AP-encodings. This provides page size
> information to the guest which is necessary for it to use radix mode.
> 
> Signed-off-by: Sam Bobroff <address@hidden>
> ---
>  hw/ppc/spapr.c   |  7 +++++++
>  target/ppc/cpu.h |  5 +++++
>  target/ppc/kvm.c | 32 +++++++++++++++++++++++++++++++-
>  3 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index a642e663d4..d629e2630c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -496,6 +496,13 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>  
>      _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
>                                  ppc_get_compat_smt_threads(cpu)));
> +
> +    if (env->radix_page_info.count) {
> +        _FDT((fdt_setprop(fdt, offset, "ibm,processor-radix-AP-encodings",
> +                          env->radix_page_info.entries,
> +                          env->radix_page_info.count *
> +                          sizeof(env->radix_page_info.entries[0]))));
> +    }
>  }
>  
>  static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index afb7ddbdd0..5a96d98b6f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -914,6 +914,10 @@ struct ppc_segment_page_sizes {
>      struct ppc_one_seg_page_size sps[PPC_PAGE_SIZES_MAX_SZ];
>  };
>  
> +struct ppc_radix_page_info {
> +    uint32_t count;
> +    uint32_t entries[PPC_PAGE_SIZES_MAX_SZ];

IIUC this info is ready for direct inclusion in the device tree:
i.e. it's big-endian.  That absolutely needs a comment in the
structure.  I'm also not sure it's a good idea, since I assume the TCG
POWER9 code will eventually need to access this information directly
to implement the radix MMU.

Might be clearer to make this data structure native and do the BE
conversion when generating the device tree.

> +};
>  
>  
> /*****************************************************************************/
>  /* The whole PowerPC CPU context */
> @@ -1053,6 +1057,7 @@ struct CPUPPCState {
>      ppc_slb_t vrma_slb;
>      target_ulong rmls;
>      bool ci_large_pages;
> +    struct ppc_radix_page_info radix_page_info;

I'm not sure this belongs in CPUPPCState.  New fields should generally
be added to PowerPCCPU ("cpu") rather than to CPUPPCState ("env")
unless they need to be directly accessed from TCG generated code.

But even more, AFAICT this should vary only with the CPU type/model,
not with the individual CPU instance.  So this information could
probably go into the CPU class structure instead of the instance.

Yes, there are a lot of things that don't obey those rules already -
that's because a lot of stuff hasn't been converted since the new QOM
stuff was introduced.  But we shouldn't make it any worse with new
patches.

>  #endif
>  
>  #if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index ec92c64159..390d6342cc 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -328,6 +328,18 @@ static void kvm_get_smmu_info(PowerPCCPU *cpu, struct 
> kvm_ppc_smmu_info *info)
>      kvm_get_fallback_smmu_info(cpu, info);
>  }
>  
> +static bool kvm_get_rmmu_info(PowerPCCPU *cpu, struct kvm_ppc_rmmu_info 
> *info)
> +{
> +    CPUState *cs = CPU(cpu);
> +    int ret;
> +
> +    if (kvm_check_extension(cs->kvm_state, KVM_CAP_PPC_MMU_RADIX)) {
> +        ret = kvm_vm_ioctl(cs->kvm_state, KVM_PPC_GET_RMMU_INFO, info);
> +        return (ret == 0);
> +    }
> +    return false;
> +}
> +
>  static long gethugepagesize(const char *mem_path)
>  {
>      struct statfs fs;
> @@ -441,9 +453,11 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>  {
>      static struct kvm_ppc_smmu_info smmu_info;
>      static bool has_smmu_info;
> +    static struct kvm_ppc_rmmu_info rmmu_info;
> +    static bool has_rmmu_info;
>      CPUPPCState *env = &cpu->env;
>      long rampagesize;
> -    int iq, ik, jq, jk;
> +    int iq, ik, jq, jk, i;
>      bool has_64k_pages = false;
>  
>      /* We only handle page sizes for 64-bit server guests for now */
> @@ -508,6 +522,22 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>      if (!has_64k_pages) {
>          env->mmu_model &= ~POWERPC_MMU_64K;
>      }
> +
> +    /* Collect radix page info from kernel if not already */
> +    if (!has_rmmu_info) {

Putting the data in the class would avoid this ugly messing with a
static local, for one thing.

> +        env->radix_page_info.count = 0;
> +        if (kvm_get_rmmu_info(cpu, &rmmu_info)) {
> +            for (i = 0; i < 8; i++) {

s/8/PPC_PAGE_SIZES_MAX_SZ/ ?

> +                if (rmmu_info.ap_encodings[i]) {
> +                    env->radix_page_info.entries[i] =
> +                        cpu_to_be32(rmmu_info.ap_encodings[i]);
> +                    env->radix_page_info.count++;
> +                }
> +            }
> +        }
> +        has_rmmu_info = true;
> +    }
> +
>  }
>  #else /* defined (TARGET_PPC64) */
>  

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