Re: [Qemu-devel] [PATCH for-2.13 09/13] target/ppc: Move 1T segment and AMR options to PPCHash64Options

[Greg Kurz <groug@kaod.org>]

On Thu,  5 Apr 2018 12:14:33 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently env->mmu_model is a bit of an unholy mess of an enum of distinct
> MMU types, with various flag bits as well.  This makes which bits of the
> field should be compared pretty confusing.
> 
> Make a start on cleaning that up by moving two of the flags bits -
> POWERPC_MMU_1TSEG and POWERPC_MMU_AMR - which are specific to the 64-bit
> hash MMU into a new flags field in PPCHash64Options structure.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> ---

While investigating a migration failure from an older QEMU, I realized
this patch has a problem. The *cpu->hash64_opts structure is zeroed in
kvm_fixup_page_sizes(), which has now the unwanted effect of clearing
the cpu->hash64_opts->flags as well.

We only need to zero the segment page sizes actually. The following
fixes migration:

@@ -442,7 +442,7 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
     }
 
     /* Convert to QEMU form */
-    memset(cpu->hash64_opts, 0, sizeof(*cpu->hash64_opts));
+    memset(&cpu->hash64_opts->sps, 0, sizeof(cpu->hash64_opts->sps));
 
     /* If we have HV KVM, we need to forbid CI large pages if our
      * host page size is smaller than 64K.

>  hw/ppc/pnv.c            |  3 ++-
>  hw/ppc/spapr.c          |  2 +-
>  target/ppc/cpu-qom.h    | 11 +++--------
>  target/ppc/kvm.c        |  4 ++--
>  target/ppc/mmu-hash64.c |  6 ++++--
>  target/ppc/mmu-hash64.h |  8 ++++++++
>  6 files changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
> index 5a79b24828..5905be3f71 100644
> --- a/hw/ppc/pnv.c
> +++ b/hw/ppc/pnv.c
> @@ -36,6 +36,7 @@
>  #include "monitor/monitor.h"
>  #include "hw/intc/intc.h"
>  #include "hw/ipmi/ipmi.h"
> +#include "target/ppc/mmu-hash64.h"
>  
>  #include "hw/ppc/xics.h"
>  #include "hw/ppc/pnv_xscom.h"
> @@ -187,7 +188,7 @@ static void pnv_dt_core(PnvChip *chip, PnvCore *pc, void *fdt)
>          _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
>      }
>  
> -    if (env->mmu_model & POWERPC_MMU_1TSEG) {
> +    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
>          _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
>                             segs, sizeof(segs))));
>      }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 14c31f82fa..f86cb09080 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -557,7 +557,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void *fdt, int offset,
>          _FDT((fdt_setprop(fdt, offset, "ibm,purr", NULL, 0)));
>      }
>  
> -    if (env->mmu_model & POWERPC_MMU_1TSEG) {
> +    if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
>          _FDT((fdt_setprop(fdt, offset, "ibm,processor-segment-sizes",
>                            segs, sizeof(segs))));
>      }
> diff --git a/target/ppc/cpu-qom.h b/target/ppc/cpu-qom.h
> index 3e5ef7375f..2bd58b2a84 100644
> --- a/target/ppc/cpu-qom.h
> +++ b/target/ppc/cpu-qom.h
> @@ -68,22 +68,17 @@ enum powerpc_mmu_t {
>      /* PowerPC 601 MMU model (specific BATs format)            */
>      POWERPC_MMU_601        = 0x0000000A,
>  #define POWERPC_MMU_64       0x00010000
> -#define POWERPC_MMU_1TSEG    0x00020000
> -#define POWERPC_MMU_AMR      0x00040000
>  #define POWERPC_MMU_V3       0x00100000 /* ISA V3.00 MMU Support */
>      /* 64 bits PowerPC MMU                                     */
>      POWERPC_MMU_64B        = POWERPC_MMU_64 | 0x00000001,
>      /* Architecture 2.03 and later (has LPCR) */
>      POWERPC_MMU_2_03       = POWERPC_MMU_64 | 0x00000002,
>      /* Architecture 2.06 variant                               */
> -    POWERPC_MMU_2_06       = POWERPC_MMU_64 | POWERPC_MMU_1TSEG
> -                             | POWERPC_MMU_AMR | 0x00000003,
> +    POWERPC_MMU_2_06       = POWERPC_MMU_64 | 0x00000003,
>      /* Architecture 2.07 variant                               */
> -    POWERPC_MMU_2_07       = POWERPC_MMU_64 | POWERPC_MMU_1TSEG
> -                             | POWERPC_MMU_AMR | 0x00000004,
> +    POWERPC_MMU_2_07       = POWERPC_MMU_64 | 0x00000004,
>      /* Architecture 3.00 variant                               */
> -    POWERPC_MMU_3_00       = POWERPC_MMU_64 | POWERPC_MMU_1TSEG
> -                             | POWERPC_MMU_AMR | POWERPC_MMU_V3
> +    POWERPC_MMU_3_00       = POWERPC_MMU_64 | POWERPC_MMU_V3
>                               | 0x00000005,
>  };
>  #define POWERPC_MMU_VER(x) ((x) & (POWERPC_MMU_64 | 0xFFFF))
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index bc6d0a8314..22487cef06 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -302,7 +302,7 @@ static void kvm_get_fallback_smmu_info(PowerPCCPU *cpu,
>          /* HV KVM has backing store size restrictions */
>          info->flags = KVM_PPC_PAGE_SIZES_REAL;
>  
> -        if (env->mmu_model & POWERPC_MMU_1TSEG) {
> +        if (ppc_hash64_has(cpu, PPC_HASH64_1TSEG)) {
>              info->flags |= KVM_PPC_1T_SEGMENTS;
>          }
>  
> @@ -482,7 +482,7 @@ static void kvm_fixup_page_sizes(PowerPCCPU *cpu)
>      }
>      env->slb_nr = smmu_info.slb_size;
>      if (!(smmu_info.flags & KVM_PPC_1T_SEGMENTS)) {
> -        env->mmu_model &= ~POWERPC_MMU_1TSEG;
> +        cpu->hash64_opts->flags &= ~PPC_HASH64_1TSEG;
>      }
>  }
>  
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 2809c31170..c9ee55e1ea 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -160,7 +160,7 @@ int ppc_store_slb(PowerPCCPU *cpu, target_ulong slot,
>      if (vsid & (SLB_VSID_B & ~SLB_VSID_B_1T)) {
>          return -1; /* Bad segment size */
>      }
> -    if ((vsid & SLB_VSID_B) && !(env->mmu_model & POWERPC_MMU_1TSEG)) {
> +    if ((vsid & SLB_VSID_B) && !(ppc_hash64_has(cpu, PPC_HASH64_1TSEG))) {
>          return -1; /* 1T segment on MMU that doesn't support it */
>      }
>  
> @@ -369,7 +369,7 @@ static int ppc_hash64_amr_prot(PowerPCCPU *cpu, ppc_hash_pte64_t pte)
>      int prot = PAGE_READ | PAGE_WRITE | PAGE_EXEC;
>  
>      /* Only recent MMUs implement Virtual Page Class Key Protection */
> -    if (!(env->mmu_model & POWERPC_MMU_AMR)) {
> +    if (!ppc_hash64_has(cpu, PPC_HASH64_AMR)) {
>          return prot;
>      }
>  
> @@ -1114,6 +1114,7 @@ void ppc_hash64_finalize(PowerPCCPU *cpu)
>  }
>  
>  const PPCHash64Options ppc_hash64_opts_basic = {
> +    .flags = 0,
>      .sps = {
>          { .page_shift = 12, /* 4K */
>            .slb_enc = 0,
> @@ -1127,6 +1128,7 @@ const PPCHash64Options ppc_hash64_opts_basic = {
>  };
>  
>  const PPCHash64Options ppc_hash64_opts_POWER7 = {
> +    .flags = PPC_HASH64_1TSEG | PPC_HASH64_AMR,
>      .sps = {
>          {
>              .page_shift = 12, /* 4K */
> diff --git a/target/ppc/mmu-hash64.h b/target/ppc/mmu-hash64.h
> index 341c1524c2..b2b5d25238 100644
> --- a/target/ppc/mmu-hash64.h
> +++ b/target/ppc/mmu-hash64.h
> @@ -153,12 +153,20 @@ struct PPCHash64SegmentPageSizes {
>  };
>  
>  struct PPCHash64Options {
> +#define PPC_HASH64_1TSEG        0x00001
> +#define PPC_HASH64_AMR          0x00002
> +    unsigned flags;
>      PPCHash64SegmentPageSizes sps[PPC_PAGE_SIZES_MAX_SZ];
>  };
>  
>  extern const PPCHash64Options ppc_hash64_opts_basic;
>  extern const PPCHash64Options ppc_hash64_opts_POWER7;
>  
> +static inline bool ppc_hash64_has(PowerPCCPU *cpu, unsigned feature)
> +{
> +    return !!(cpu->hash64_opts->flags & feature);
> +}
> +
>  #endif /* CONFIG_USER_ONLY */
>  
>  #if defined(CONFIG_USER_ONLY) || !defined(TARGET_PPC64)