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

[Sam Bobroff <sam.bobroff@au1.ibm.com>]

On Thu, Feb 09, 2017 at 01:14:08PM +1100, David Gibson wrote:
> 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 <sam.bobroff@au1.ibm.com>
> > ---
> >  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.

Sounds good, I'll try it.

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

Agreed, I'll move it.

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

Yeah, that will be nice :-)

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

Yes!

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