Re: [Qemu-devel] [PATCH for-2.13 13/13] target/ppc: Fold slb_nr into PPCHash64Options

[David Gibson]

On Thu, Apr 05, 2018 at 03:12:55PM +0200, Greg Kurz wrote:
> On Thu,  5 Apr 2018 12:14:37 +1000
> David Gibson <address@hidden> wrote:
> 
> > The env->slb_nr field gives the size of the SLB (Segment Lookaside Buffer).
> > This is another static-after-initialization parameter of the specific
> > version of the 64-bit hash MMU in the CPU.  So, this patch folds the field
> > into PPCHash64Options with the other hash MMU options.
> > 
> > This is a bit more complicated that the things previously put in there,
> > because slb_nr was foolishly included in the migration stream.  So we need
> > some of the usual dance to handle backwards compatible migration.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/pnv.c                |  2 +-
> >  hw/ppc/spapr.c              | 11 ++++++++---
> >  target/ppc/cpu.h            |  3 ++-
> >  target/ppc/kvm.c            |  2 +-
> >  target/ppc/machine.c        | 23 ++++++++++++++++++++---
> >  target/ppc/mmu-hash64.c     | 15 +++++++++------
> >  target/ppc/mmu-hash64.h     |  1 +
> >  target/ppc/translate_init.c | 15 ---------------
> >  8 files changed, 42 insertions(+), 30 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> > index 5905be3f71..53f672afa8 100644
> > --- a/hw/ppc/pnv.c
> > +++ b/hw/ppc/pnv.c
> > @@ -180,7 +180,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, 
> > void *fdt)
> >  
> >      _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> >      _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> > -    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr)));
> > +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", 
> > cpu->hash64_opts->slb_size)));
> >      _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> >      _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> >  
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 60bc8417b6..6021631722 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -547,8 +547,8 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> > *fdt, int offset,
> >  
> >      _FDT((fdt_setprop_cell(fdt, offset, "timebase-frequency", tbfreq)));
> >      _FDT((fdt_setprop_cell(fdt, offset, "clock-frequency", cpufreq)));
> > -    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", env->slb_nr)));
> > -    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", env->slb_nr)));
> > +    _FDT((fdt_setprop_cell(fdt, offset, "slb-size", 
> > cpu->hash64_opts->slb_size)));
> > +    _FDT((fdt_setprop_cell(fdt, offset, "ibm,slb-size", 
> > cpu->hash64_opts->slb_size)));
> >      _FDT((fdt_setprop_string(fdt, offset, "status", "okay")));
> >      _FDT((fdt_setprop(fdt, offset, "64-bit", NULL, 0)));
> >  
> > @@ -4000,7 +4000,12 @@ DEFINE_SPAPR_MACHINE(2_13, "2.13", true);
> >   * pseries-2.12
> >   */
> >  #define SPAPR_COMPAT_2_12                                              \
> > -    HW_COMPAT_2_12
> 
> This hunk doesn't apply on master, nor on your ppc-for-2.13 branch...

Uh.. I think you need to pull the ppc-for-2.13 brnch again, then..

> It looks like a patch to introduce the 2.13 machine type is missing.

..since I added a patch to do exactly that.

> FWIW, Connie has already queued a patch to do so for s390x, that also
> introduces HW_COMPAT_2_12.
> 
> https://github.com/cohuck/qemu/commit/b54cde7350b6681b4349b904e0f9a8a8d58c0951
> 
> Maybe the HW_COMPAT_ macros should be added in a standalone patch ?
> 
> Cc'ing Connie for insights.
> 
> > +    HW_COMPAT_2_12                                                     \
> > +    {                                                                  \
> > +        .driver = TYPE_POWERPC_CPU,                                    \
> > +            .property = "pre-2.13-migration",                          \
> > +            .value    = "on",                                          \
> 
> indentation ?

Oops, adjusted.

> 
> Also, this property must be added to the TYPE_POWERPC_CPU class.
> 
> --- a/target/ppc/translate_init.c
> +++ b/target/ppc/translate_init.c
> @@ -10471,6 +10471,8 @@ static Property ppc_cpu_properties[] = {
>      DEFINE_PROP_BOOL("pre-2.8-migration", PowerPCCPU, pre_2_8_migration, 
> false),
>      DEFINE_PROP_BOOL("pre-2.10-migration", PowerPCCPU, pre_2_10_migration,
>                       false),
> +    DEFINE_PROP_BOOL("pre-2.13-migration", PowerPCCPU, pre_2_13_migration,
> +                     false),
>      DEFINE_PROP_END_OF_LIST(),
>  };

Oops again, adjusted.

> Appart from that, the patch looks good, so:
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> With all the above points addressed, I could successfully migrate from an
> older QEMU and back, so:
> 
> Tested-by: Greg Kurz <address@hidden>
> 
> > +    },
> >  
> >  static void spapr_machine_2_12_instance_options(MachineState *machine)
> >  {
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index c0c44fb91d..8c9e03f54d 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -1025,7 +1025,6 @@ struct CPUPPCState {
> >  #if defined(TARGET_PPC64)
> >      /* PowerPC 64 SLB area */
> >      ppc_slb_t slb[MAX_SLB_ENTRIES];
> > -    int32_t slb_nr;
> >      /* tcg TLB needs flush (deferred slb inval instruction typically) */
> >  #endif
> >      /* segment registers */
> > @@ -1216,6 +1215,8 @@ struct PowerPCCPU {
> >      uint64_t mig_insns_flags2;
> >      uint32_t mig_nb_BATs;
> >      bool pre_2_10_migration;
> > +    bool pre_2_13_migration;
> > +    int32_t mig_slb_nr;
> >  };
> >  
> >  static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index b329cd8173..1bd38c6a90 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -484,7 +484,7 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
> >              break;
> >          }
> >      }
> > -    env->slb_nr = smmu_info.slb_size;
> > +    cpu->hash64_opts->slb_size = smmu_info.slb_size;
> >      if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) {
> >          cpu->hash64_opts->flags &= ~PPC_HASH64_1TSEG;
> >      }
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index 0634cdb295..3d6434a006 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -18,6 +18,9 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> > version_id)
> >      unsigned int i, j;
> >      target_ulong sdr1;
> >      uint32_t fpscr;
> > +#if defined(TARGET_PPC64)
> > +    int32_t slb_nr;
> > +#endif
> >      target_ulong xer;
> >  
> >      for (i = 0; i < 32; i++)
> > @@ -49,7 +52,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int 
> > version_id)
> >      qemu_get_sbe32s(f, &env->access_type);
> >  #if defined(TARGET_PPC64)
> >      qemu_get_betls(f, &env->spr[SPR_ASR]);
> > -    qemu_get_sbe32s(f, &env->slb_nr);
> > +    qemu_get_sbe32s(f, &slb_nr);
> >  #endif
> >      qemu_get_betls(f, &sdr1);
> >      for (i = 0; i < 32; i++)
> > @@ -146,6 +149,15 @@ static bool cpu_pre_2_8_migration(void *opaque, int 
> > version_id)
> >      return cpu->pre_2_8_migration;
> >  }
> >  
> > +#if defined(TARGET_PPC64)
> > +static bool cpu_pre_2_13_migration(void *opaque, int version_id)
> > +{
> > +    PowerPCCPU *cpu = opaque;
> > +
> > +    return cpu->pre_2_13_migration;
> > +}
> > +#endif
> > +
> >  static int cpu_pre_save(void *opaque)
> >  {
> >      PowerPCCPU *cpu = opaque;
> > @@ -203,6 +215,11 @@ static int cpu_pre_save(void *opaque)
> >          cpu->mig_insns_flags2 = env->insns_flags2 & insns_compat_mask2;
> >          cpu->mig_nb_BATs = env->nb_BATs;
> >      }
> > +    if (cpu->pre_2_13_migration) {
> > +        if (cpu->hash64_opts) {
> > +            cpu->mig_slb_nr = cpu->hash64_opts->slb_size;
> > +        }
> > +    }
> >  
> >      return 0;
> >  }
> > @@ -478,7 +495,7 @@ static int slb_post_load(void *opaque, int version_id)
> >  
> >      /* We've pulled in the raw esid and vsid values from the migration
> >       * stream, but we need to recompute the page size pointers */
> > -    for (i = 0; i < env->slb_nr; i++) {
> > +    for (i = 0; i < cpu->hash64_opts->slb_size; i++) {
> >          if (ppc_store_slb(cpu, i, env->slb[i].esid, env->slb[i].vsid) < 0) 
> > {
> >              /* Migration source had bad values in its SLB */
> >              return -1;
> > @@ -495,7 +512,7 @@ static const VMStateDescription vmstate_slb = {
> >      .needed = slb_needed,
> >      .post_load = slb_post_load,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_INT32_EQUAL(env.slb_nr, PowerPCCPU, NULL),
> > +        VMSTATE_INT32_TEST(mig_slb_nr, PowerPCCPU, cpu_pre_2_13_migration),
> >          VMSTATE_SLB_ARRAY(env.slb, PowerPCCPU, MAX_SLB_ENTRIES),
> >          VMSTATE_END_OF_LIST()
> >      }
> > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> > index a5570c8774..7e0adecfd9 100644
> > --- a/target/ppc/mmu-hash64.c
> > +++ b/target/ppc/mmu-hash64.c
> > @@ -52,7 +52,7 @@ static ppc_slb_t *slb_lookup(PowerPCCPU *cpu, 
> > target_ulong eaddr)
> >      esid_256M = (eaddr & SEGMENT_MASK_256M) | SLB_ESID_V;
> >      esid_1T = (eaddr & SEGMENT_MASK_1T) | SLB_ESID_V;
> >  
> > -    for (n = 0; n < env->slb_nr; n++) {
> > +    for (n = 0; n < cpu->hash64_opts->slb_size; n++) {
> >          ppc_slb_t *slb = &env->slb[n];
> >  
> >          LOG_SLB("%s: slot %d %016" PRIx64 " %016"
> > @@ -80,7 +80,7 @@ void dump_slb(FILE *f, fprintf_function cpu_fprintf, 
> > PowerPCCPU *cpu)
> >      cpu_synchronize_state(CPU(cpu));
> >  
> >      cpu_fprintf(f, "SLB\tESID\t\t\tVSID\n");
> > -    for (i = 0; i < env->slb_nr; i++) {
> > +    for (i = 0; i < cpu->hash64_opts->slb_size; i++) {
> >          slbe = env->slb[i].esid;
> >          slbv = env->slb[i].vsid;
> >          if (slbe == 0 && slbv == 0) {
> > @@ -93,10 +93,11 @@ void dump_slb(FILE *f, fprintf_function cpu_fprintf, 
> > PowerPCCPU *cpu)
> >  
> >  void helper_slbia(CPUPPCState *env)
> >  {
> > +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> >      int n;
> >  
> >      /* XXX: Warning: slbia never invalidates the first segment */
> > -    for (n = 1; n < env->slb_nr; n++) {
> > +    for (n = 1; n < cpu->hash64_opts->slb_size; n++) {
> >          ppc_slb_t *slb = &env->slb[n];
> >  
> >          if (slb->esid & SLB_ESID_V) {
> > @@ -151,7 +152,7 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
> >      const PPCHash64SegmentPageSizes *sps = NULL;
> >      int i;
> >  
> > -    if (slot >= env->slb_nr) {
> > +    if (slot >= cpu->hash64_opts->slb_size) {
> >          return -1; /* Bad slot number */
> >      }
> >      if (esid & ~(SLB_ESID_ESID | SLB_ESID_V)) {
> > @@ -202,7 +203,7 @@ static int ppc_load_slb_esid(PowerPCCPU *cpu, 
> > target_ulong rb,
> >      int slot = rb & 0xfff;
> >      ppc_slb_t *slb = &env->slb[slot];
> >  
> > -    if (slot >= env->slb_nr) {
> > +    if (slot >= cpu->hash64_opts->slb_size) {
> >          return -1;
> >      }
> >  
> > @@ -217,7 +218,7 @@ static int ppc_load_slb_vsid(PowerPCCPU *cpu, 
> > target_ulong rb,
> >      int slot = rb & 0xfff;
> >      ppc_slb_t *slb = &env->slb[slot];
> >  
> > -    if (slot >= env->slb_nr) {
> > +    if (slot >= cpu->hash64_opts->slb_size) {
> >          return -1;
> >      }
> >  
> > @@ -1115,6 +1116,7 @@ void ppc_hash64_finalize(PowerPCCPU *cpu)
> >  
> >  const PPCHash64Options ppc_hash64_opts_basic = {
> >      .flags = 0,
> > +    .slb_size = 64,
> >      .sps = {
> >          { .page_shift = 12, /* 4K */
> >            .slb_enc = 0,
> > @@ -1129,6 +1131,7 @@ const PPCHash64Options ppc_hash64_opts_basic = {
> >  
> >  const PPCHash64Options ppc_hash64_opts_POWER7 = {
> >      .flags = PPC_HASH64_1TSEG | PPC_HASH64_AMR | PPC_HASH64_CI_LARGEPAGE,
> > +    .slb_size = 32,
> >      .sps = {
> >          {
> >              .page_shift = 12, /* 4K */
> > diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> > index f1babb0afc..d5fc03441d 100644
> > --- a/target/ppc/mmu-hash64.h
> > +++ b/target/ppc/mmu-hash64.h
> > @@ -157,6 +157,7 @@ struct PPCHash64Options {
> >  #define PPC_HASH64_AMR          0x00002
> >  #define PPC_HASH64_CI_LARGEPAGE 0x00004
> >      unsigned flags;
> > +    unsigned slb_size;
> >      PPCHash64SegmentPageSizes sps[PPC_PAGE_SIZES_MAX_SZ];
> >  };
> >  
> > diff --git a/target/ppc/translate_init.c b/target/ppc/translate_init.c
> > index a925cf5cd3..2f63acd310 100644
> > --- a/target/ppc/translate_init.c
> > +++ b/target/ppc/translate_init.c
> > @@ -8195,9 +8195,6 @@ static void init_proc_970(CPUPPCState *env)
> >      gen_spr_970_dbg(env);
> >  
> >      /* env variables */
> > -#if !defined(CONFIG_USER_ONLY)
> > -    env->slb_nr = 64;
> > -#endif
> >      env->dcache_line_size = 128;
> >      env->icache_line_size = 128;
> >  
> > @@ -8272,9 +8269,6 @@ static void init_proc_power5plus(CPUPPCState *env)
> >      gen_spr_power5p_ear(env);
> >  
> >      /* env variables */
> > -#if !defined(CONFIG_USER_ONLY)
> > -    env->slb_nr = 64;
> > -#endif
> >      env->dcache_line_size = 128;
> >      env->icache_line_size = 128;
> >  
> > @@ -8389,9 +8383,6 @@ static void init_proc_POWER7(CPUPPCState *env)
> >      gen_spr_power7_book4(env);
> >  
> >      /* env variables */
> > -#if !defined(CONFIG_USER_ONLY)
> > -    env->slb_nr = 32;
> > -#endif
> >      env->dcache_line_size = 128;
> >      env->icache_line_size = 128;
> >  
> > @@ -8543,9 +8534,6 @@ static void init_proc_POWER8(CPUPPCState *env)
> >      gen_spr_power8_rpr(env);
> >  
> >      /* env variables */
> > -#if !defined(CONFIG_USER_ONLY)
> > -    env->slb_nr = 32;
> > -#endif
> >      env->dcache_line_size = 128;
> >      env->icache_line_size = 128;
> >  
> > @@ -8743,9 +8731,6 @@ static void init_proc_POWER9(CPUPPCState *env)
> >                          KVM_REG_PPC_PSSCR, 0);
> >  
> >      /* env variables */
> > -#if !defined(CONFIG_USER_ONLY)
> > -    env->slb_nr = 32;
> > -#endif
> >      env->dcache_line_size = 128;
> >      env->icache_line_size = 128;
> >  
> 

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