[PATCH v5 00/10] target/ppc: Fix truncation of env->hflags

[PATCH v5 00/10] target/ppc: Fix truncation of env->hflags

[Richard Henderson]

Changes for v5:
 * Rebase on david's ppc-for-6.0 branch (fcc83aa7359d)
 * Extract post_load_update_msr (clg)
 * Validate MSR_FOO == HFLAGS_FOO when necessary (david)

Changes for v4:
 * Use hregs_recompute_hflags for hw/ppc/ reset.
   -- Incorporate Cedric's feedback.

Changes for v3:
 * Fixes for linux-user, signal handling and startup.
   -- Oops, the directory in which I did testing for v2
      had a reduced set of targets.

Changes for v2:
 * Do not put tcg internal state into migration, except to
   retain backward compatibility.
 * Do not touch anything in env in ppc_tr_init_disas_context.
 * Do make sure that hflags contains everything that it should.
 * Do verify that hflags is properly updated.


r~


Richard Henderson (10):
  target/ppc: Extract post_load_update_msr
  target/ppc: Disconnect hflags from MSR
  target/ppc: Reduce env->hflags to uint32_t
  target/ppc: Put dbcr0 single-step bits into hflags
  target/ppc: Create helper_scv
  target/ppc: Put LPCR[GTSE] in hflags
  target/ppc: Remove MSR_SA and MSR_AP from hflags
  target/ppc: Remove env->immu_idx and env->dmmu_idx
  linux-user/ppc: Fix msr updates for signal handling
  target/ppc: Validate hflags with CONFIG_DEBUG_TCG

 target/ppc/cpu.h          |  43 +++++++++++--
 target/ppc/helper.h       |   1 +
 target/ppc/helper_regs.h  |   1 -
 linux-user/ppc/cpu_loop.c |   5 +-
 linux-user/ppc/signal.c   |  23 ++++---
 target/ppc/excp_helper.c  |   9 +++
 target/ppc/helper_regs.c  | 128 ++++++++++++++++++++++++++++++--------
 target/ppc/machine.c      |  30 +++++----
 target/ppc/mem_helper.c   |   2 +-
 target/ppc/misc_helper.c  |   5 +-
 target/ppc/mmu-hash64.c   |   3 +
 target/ppc/translate.c    |  98 +++++++++--------------------
 12 files changed, 215 insertions(+), 133 deletions(-)

-- 
2.25.1

[PATCH v5 01/10] target/ppc: Extract post_load_update_msr

[Richard Henderson]

Extract post_load_update_msr to share between cpu_load_old
and cpu_post_load in updating the msr.

Suggested-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/machine.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 1f7a353c78..09c5765a87 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -10,6 +10,18 @@
 #include "kvm_ppc.h"
 #include "exec/helper-proto.h"
 
+static void post_load_update_msr(CPUPPCState *env)
+{
+    target_ulong msr = env->msr;
+
+    /*
+     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
+     * before restoring.  Note that this recomputes hflags and mem_idx.
+     */
+    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
+    ppc_store_msr(env, msr);
+}
+
 static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
 {
     PowerPCCPU *cpu = opaque;
@@ -21,7 +33,6 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
     int32_t slb_nr;
 #endif
     target_ulong xer;
-    target_ulong msr;
 
     for (i = 0; i < 32; i++) {
         qemu_get_betls(f, &env->gpr[i]);
@@ -117,13 +128,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
     qemu_get_sbe32(f); /* Discard unused mmu_idx */
     qemu_get_sbe32(f); /* Discard unused power_mode */
 
-    /*
-     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
-     * before restoring.  Note that this recomputes hflags and mem_idx.
-     */
-    msr = env->msr;
-    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
-    ppc_store_msr(env, msr);
+    post_load_update_msr(env);
 
     return 0;
 }
@@ -343,7 +348,6 @@ static int cpu_post_load(void *opaque, int version_id)
     PowerPCCPU *cpu = opaque;
     CPUPPCState *env = &cpu->env;
     int i;
-    target_ulong msr;
 
     /*
      * If we're operating in compat mode, we should be ok as long as
@@ -417,13 +421,7 @@ static int cpu_post_load(void *opaque, int version_id)
         ppc_store_sdr1(env, env->spr[SPR_SDR1]);
     }
 
-    /*
-     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
-     * before restoring.  Note that this recomputes hflags and mem_idx.
-     */
-    msr = env->msr;
-    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
-    ppc_store_msr(env, msr);
+    post_load_update_msr(env);
 
     return 0;
 }
-- 
2.25.1

Re: [PATCH v5 01/10] target/ppc: Extract post_load_update_msr

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]

On Tue, Mar 23, 2021 at 12:43:31PM -0600, Richard Henderson wrote:
> Extract post_load_update_msr to share between cpu_load_old
> and cpu_post_load in updating the msr.
> 
> Suggested-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/machine.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 1f7a353c78..09c5765a87 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -10,6 +10,18 @@
>  #include "kvm_ppc.h"
>  #include "exec/helper-proto.h"
>  
> +static void post_load_update_msr(CPUPPCState *env)
> +{
> +    target_ulong msr = env->msr;
> +
> +    /*
> +     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> +     * before restoring.  Note that this recomputes hflags and mem_idx.
> +     */
> +    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> +    ppc_store_msr(env, msr);
> +}
> +
>  static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>  {
>      PowerPCCPU *cpu = opaque;
> @@ -21,7 +33,6 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>      int32_t slb_nr;
>  #endif
>      target_ulong xer;
> -    target_ulong msr;
>  
>      for (i = 0; i < 32; i++) {
>          qemu_get_betls(f, &env->gpr[i]);
> @@ -117,13 +128,7 @@ static int cpu_load_old(QEMUFile *f, void *opaque, int version_id)
>      qemu_get_sbe32(f); /* Discard unused mmu_idx */
>      qemu_get_sbe32(f); /* Discard unused power_mode */
>  
> -    /*
> -     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> -     * before restoring.  Note that this recomputes hflags and mem_idx.
> -     */
> -    msr = env->msr;
> -    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> -    ppc_store_msr(env, msr);
> +    post_load_update_msr(env);
>  
>      return 0;
>  }
> @@ -343,7 +348,6 @@ static int cpu_post_load(void *opaque, int version_id)
>      PowerPCCPU *cpu = opaque;
>      CPUPPCState *env = &cpu->env;
>      int i;
> -    target_ulong msr;
>  
>      /*
>       * If we're operating in compat mode, we should be ok as long as
> @@ -417,13 +421,7 @@ static int cpu_post_load(void *opaque, int version_id)
>          ppc_store_sdr1(env, env->spr[SPR_SDR1]);
>      }
>  
> -    /*
> -     * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> -     * before restoring.  Note that this recomputes hflags and mem_idx.
> -     */
> -    msr = env->msr;
> -    env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
> -    ppc_store_msr(env, msr);
> +    post_load_update_msr(env);
>  
>      return 0;
>  }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Richard Henderson]

Copying flags directly from msr has drawbacks: (1) msr bits
mean different things per cpu, (2) msr has 64 bits on 64 cpus
while tb->flags has only 32 bits.

Create a enum to define these bits.  Document the origin of each bit
and validate those bits that must match MSR.  This fixes the
truncation of env->hflags to tb->flags, because we no longer
have hflags bits set above bit 31.

Most of the code in ppc_tr_init_disas_context is moved over to
hreg_compute_hflags.  Some of it is simple extractions from msr,
some requires examining other cpu flags.  Anything that is moved
becomes a simple extract from hflags in ppc_tr_init_disas_context.

Several existing bugs are left in ppc_tr_init_disas_context, where
additional changes are required -- to be addressed in future patches.

Remove a broken #if 0 block.

Reported-by: Ivan Warren <ivan@vmfacility.fr>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         | 25 ++++++++++++++++
 target/ppc/helper_regs.c | 65 +++++++++++++++++++++++++++++++++-------
 target/ppc/translate.c   | 55 ++++++++++------------------------
 3 files changed, 95 insertions(+), 50 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index fd13489dce..fe6c3f815d 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -585,6 +585,31 @@ enum {
     POWERPC_FLAG_HID0_LE  = 0x00400000,
 };
 
+/*
+ * Bits for env->hflags.
+ *
+ * Most of these bits overlap with corresponding bits in MSR,
+ * but some come from other sources.  Those that do come from
+ * the MSR are validated in hreg_compute_hflags.
+ */
+enum {
+    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
+    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
+    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
+    HFLAGS_DR = 4,   /* MSR_DR */
+    HFLAGS_IR = 5,   /* MSR_IR */
+    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
+    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
+    HFLAGS_TM = 8,   /* computed from MSR_TM */
+    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
+    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
+    HFLAGS_FP = 13,  /* MSR_FP */
+    HFLAGS_PR = 14,  /* MSR_PR */
+    HFLAGS_SA = 22,  /* MSR_SA */
+    HFLAGS_AP = 23,  /* MSR_AP */
+    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
+};
+
 /*****************************************************************************/
 /* Floating point status and control register                                */
 #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index a87e354ca2..df9673b90f 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "cpu.h"
 #include "qemu/main-loop.h"
 #include "exec/exec-all.h"
 #include "sysemu/kvm.h"
@@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)
 
 void hreg_compute_hflags(CPUPPCState *env)
 {
-    target_ulong hflags_mask;
+    target_ulong msr = env->msr;
+    uint32_t ppc_flags = env->flags;
+    uint32_t hflags = 0;
+    uint32_t msr_mask;
 
-    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
-    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
-        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
-        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
-    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
-    hreg_compute_mem_idx(env);
-    env->hflags = env->msr & hflags_mask;
+    /* Some bits come straight across from MSR. */
+    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
+    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
+    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
+    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
+    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
+    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
+    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
+    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
+                (1 << MSR_DR) | (1 << MSR_IR) |
+                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
 
-    if (env->flags & POWERPC_FLAG_HID0_LE) {
+    if (ppc_flags & POWERPC_FLAG_HID0_LE) {
         /*
          * Note that MSR_LE is not set in env->msr_mask for this cpu,
-         * and so will never be set in msr or hflags at this point.
+         * and so will never be set in msr.
          */
         uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
-        env->hflags |= le << MSR_LE;
+        hflags |= le << MSR_LE;
     }
+
+    if (ppc_flags & POWERPC_FLAG_BE) {
+        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
+        msr_mask |= 1 << MSR_BE;
+    }
+    if (ppc_flags & POWERPC_FLAG_SE) {
+        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
+        msr_mask |= 1 << MSR_SE;
+    }
+
+    if (msr_is_64bit(env, msr)) {
+        hflags |= 1 << HFLAGS_64;
+    }
+    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {
+        hflags |= 1 << HFLAGS_SPE;
+    }
+    if (ppc_flags & POWERPC_FLAG_VRE) {
+        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
+        msr_mask |= 1 << MSR_VR;
+    }
+    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
+        hflags |= 1 << HFLAGS_VSX;
+    }
+    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
+        hflags |= 1 << HFLAGS_TM;
+    }
+
+#ifndef CONFIG_USER_ONLY
+    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
+        hflags |= 1 << HFLAGS_HV;
+    }
+#endif
+
+    env->hflags = hflags | (msr & msr_mask);
+    hreg_compute_mem_idx(env);
 }
 
 void cpu_interrupt_exittb(CPUState *cs)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0984ce637b..a9325a12e5 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7879,67 +7879,48 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
 {
     DisasContext *ctx = container_of(dcbase, DisasContext, base);
     CPUPPCState *env = cs->env_ptr;
+    uint32_t hflags = ctx->base.tb->flags;
     int bound;
 
     ctx->exception = POWERPC_EXCP_NONE;
     ctx->spr_cb = env->spr_cb;
-    ctx->pr = msr_pr;
+    ctx->pr = (hflags >> HFLAGS_PR) & 1;
     ctx->mem_idx = env->dmmu_idx;
-    ctx->dr = msr_dr;
-#if !defined(CONFIG_USER_ONLY)
-    ctx->hv = msr_hv || !env->has_hv_mode;
-#endif
+    ctx->dr = (hflags >> HFLAGS_DR) & 1;
+    ctx->hv = (hflags >> HFLAGS_HV) & 1;
     ctx->insns_flags = env->insns_flags;
     ctx->insns_flags2 = env->insns_flags2;
     ctx->access_type = -1;
     ctx->need_access_type = !mmu_is_64bit(env->mmu_model);
-    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
+    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;
     ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;
     ctx->flags = env->flags;
 #if defined(TARGET_PPC64)
-    ctx->sf_mode = msr_is_64bit(env, env->msr);
+    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;
     ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);
 #endif
     ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B
         || env->mmu_model == POWERPC_MMU_601
         || env->mmu_model & POWERPC_MMU_64;
 
-    ctx->fpu_enabled = !!msr_fp;
-    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {
-        ctx->spe_enabled = !!msr_spe;
-    } else {
-        ctx->spe_enabled = false;
-    }
-    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {
-        ctx->altivec_enabled = !!msr_vr;
-    } else {
-        ctx->altivec_enabled = false;
-    }
-    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {
-        ctx->vsx_enabled = !!msr_vsx;
-    } else {
-        ctx->vsx_enabled = false;
-    }
+    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;
+    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
+    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
+    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
     if ((env->flags & POWERPC_FLAG_SCV)
         && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
         ctx->scv_enabled = true;
     } else {
         ctx->scv_enabled = false;
     }
-#if defined(TARGET_PPC64)
-    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {
-        ctx->tm_enabled = !!msr_tm;
-    } else {
-        ctx->tm_enabled = false;
-    }
-#endif
+    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
-    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {
-        ctx->singlestep_enabled = CPU_SINGLE_STEP;
-    } else {
-        ctx->singlestep_enabled = 0;
+
+    ctx->singlestep_enabled = 0;
+    if ((hflags >> HFLAGS_SE) & 1) {
+        ctx->singlestep_enabled |= CPU_SINGLE_STEP;
     }
-    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {
+    if ((hflags >> HFLAGS_BE) & 1) {
         ctx->singlestep_enabled |= CPU_BRANCH_STEP;
     }
     if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
@@ -7956,10 +7937,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     if (unlikely(ctx->base.singlestep_enabled)) {
         ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
     }
-#if defined(DO_SINGLE_STEP) && 0
-    /* Single step trace mode */
-    msr_se = 1;
-#endif
 
     bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);
-- 
2.25.1

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10343 bytes --]

On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
> Copying flags directly from msr has drawbacks: (1) msr bits
> mean different things per cpu, (2) msr has 64 bits on 64 cpus
> while tb->flags has only 32 bits.
> 
> Create a enum to define these bits.  Document the origin of each bit
> and validate those bits that must match MSR.  This fixes the
> truncation of env->hflags to tb->flags, because we no longer
> have hflags bits set above bit 31.
> 
> Most of the code in ppc_tr_init_disas_context is moved over to
> hreg_compute_hflags.  Some of it is simple extractions from msr,
> some requires examining other cpu flags.  Anything that is moved
> becomes a simple extract from hflags in ppc_tr_init_disas_context.
> 
> Several existing bugs are left in ppc_tr_init_disas_context, where
> additional changes are required -- to be addressed in future patches.
> 
> Remove a broken #if 0 block.
> 
> Reported-by: Ivan Warren <ivan@vmfacility.fr>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/cpu.h         | 25 ++++++++++++++++
>  target/ppc/helper_regs.c | 65 +++++++++++++++++++++++++++++++++-------
>  target/ppc/translate.c   | 55 ++++++++++------------------------
>  3 files changed, 95 insertions(+), 50 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index fd13489dce..fe6c3f815d 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -585,6 +585,31 @@ enum {
>      POWERPC_FLAG_HID0_LE  = 0x00400000,
>  };
>  
> +/*
> + * Bits for env->hflags.
> + *
> + * Most of these bits overlap with corresponding bits in MSR,
> + * but some come from other sources.  Those that do come from
> + * the MSR are validated in hreg_compute_hflags.
> + */
> +enum {
> +    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
> +    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
> +    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
> +    HFLAGS_DR = 4,   /* MSR_DR */
> +    HFLAGS_IR = 5,   /* MSR_IR */
> +    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
> +    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
> +    HFLAGS_TM = 8,   /* computed from MSR_TM */
> +    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> +    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> +    HFLAGS_FP = 13,  /* MSR_FP */
> +    HFLAGS_PR = 14,  /* MSR_PR */
> +    HFLAGS_SA = 22,  /* MSR_SA */
> +    HFLAGS_AP = 23,  /* MSR_AP */
> +    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> +};
> +
>  /*****************************************************************************/
>  /* Floating point status and control register                                */
>  #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index a87e354ca2..df9673b90f 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -18,6 +18,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "cpu.h"
>  #include "qemu/main-loop.h"
>  #include "exec/exec-all.h"
>  #include "sysemu/kvm.h"
> @@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)
>  
>  void hreg_compute_hflags(CPUPPCState *env)
>  {
> -    target_ulong hflags_mask;
> +    target_ulong msr = env->msr;
> +    uint32_t ppc_flags = env->flags;
> +    uint32_t hflags = 0;
> +    uint32_t msr_mask;
>  
> -    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
> -    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
> -        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
> -        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
> -    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
> -    hreg_compute_mem_idx(env);
> -    env->hflags = env->msr & hflags_mask;
> +    /* Some bits come straight across from MSR. */
> +    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
> +    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
> +    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
> +    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
> +    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
> +    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
> +    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
> +    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> +                (1 << MSR_DR) | (1 << MSR_IR) |
> +                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
>  
> -    if (env->flags & POWERPC_FLAG_HID0_LE) {
> +    if (ppc_flags & POWERPC_FLAG_HID0_LE) {
>          /*
>           * Note that MSR_LE is not set in env->msr_mask for this cpu,
> -         * and so will never be set in msr or hflags at this point.
> +         * and so will never be set in msr.
>           */
>          uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
> -        env->hflags |= le << MSR_LE;
> +        hflags |= le << MSR_LE;
>      }
> +
> +    if (ppc_flags & POWERPC_FLAG_BE) {
> +        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> +        msr_mask |= 1 << MSR_BE;
> +    }
> +    if (ppc_flags & POWERPC_FLAG_SE) {
> +        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> +        msr_mask |= 1 << MSR_SE;
> +    }
> +
> +    if (msr_is_64bit(env, msr)) {
> +        hflags |= 1 << HFLAGS_64;
> +    }
> +    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {
> +        hflags |= 1 << HFLAGS_SPE;
> +    }
> +    if (ppc_flags & POWERPC_FLAG_VRE) {
> +        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
> +        msr_mask |= 1 << MSR_VR;
> +    }
> +    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
> +        hflags |= 1 << HFLAGS_VSX;
> +    }
> +    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
> +        hflags |= 1 << HFLAGS_TM;
> +    }
> +
> +#ifndef CONFIG_USER_ONLY
> +    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> +        hflags |= 1 << HFLAGS_HV;
> +    }
> +#endif
> +
> +    env->hflags = hflags | (msr & msr_mask);
> +    hreg_compute_mem_idx(env);
>  }
>  
>  void cpu_interrupt_exittb(CPUState *cs)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0984ce637b..a9325a12e5 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7879,67 +7879,48 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>  {
>      DisasContext *ctx = container_of(dcbase, DisasContext, base);
>      CPUPPCState *env = cs->env_ptr;
> +    uint32_t hflags = ctx->base.tb->flags;
>      int bound;
>  
>      ctx->exception = POWERPC_EXCP_NONE;
>      ctx->spr_cb = env->spr_cb;
> -    ctx->pr = msr_pr;
> +    ctx->pr = (hflags >> HFLAGS_PR) & 1;
>      ctx->mem_idx = env->dmmu_idx;
> -    ctx->dr = msr_dr;
> -#if !defined(CONFIG_USER_ONLY)
> -    ctx->hv = msr_hv || !env->has_hv_mode;
> -#endif
> +    ctx->dr = (hflags >> HFLAGS_DR) & 1;
> +    ctx->hv = (hflags >> HFLAGS_HV) & 1;
>      ctx->insns_flags = env->insns_flags;
>      ctx->insns_flags2 = env->insns_flags2;
>      ctx->access_type = -1;
>      ctx->need_access_type = !mmu_is_64bit(env->mmu_model);
> -    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
> +    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;
>      ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;
>      ctx->flags = env->flags;
>  #if defined(TARGET_PPC64)
> -    ctx->sf_mode = msr_is_64bit(env, env->msr);
> +    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;
>      ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);
>  #endif
>      ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B
>          || env->mmu_model == POWERPC_MMU_601
>          || env->mmu_model & POWERPC_MMU_64;
>  
> -    ctx->fpu_enabled = !!msr_fp;
> -    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {
> -        ctx->spe_enabled = !!msr_spe;
> -    } else {
> -        ctx->spe_enabled = false;
> -    }
> -    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {
> -        ctx->altivec_enabled = !!msr_vr;
> -    } else {
> -        ctx->altivec_enabled = false;
> -    }
> -    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {
> -        ctx->vsx_enabled = !!msr_vsx;
> -    } else {
> -        ctx->vsx_enabled = false;
> -    }
> +    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;
> +    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
> +    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
> +    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>      if ((env->flags & POWERPC_FLAG_SCV)
>          && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
>          ctx->scv_enabled = true;
>      } else {
>          ctx->scv_enabled = false;
>      }
> -#if defined(TARGET_PPC64)
> -    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {
> -        ctx->tm_enabled = !!msr_tm;
> -    } else {
> -        ctx->tm_enabled = false;
> -    }
> -#endif
> +    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>      ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
> -    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {
> -        ctx->singlestep_enabled = CPU_SINGLE_STEP;
> -    } else {
> -        ctx->singlestep_enabled = 0;
> +
> +    ctx->singlestep_enabled = 0;
> +    if ((hflags >> HFLAGS_SE) & 1) {
> +        ctx->singlestep_enabled |= CPU_SINGLE_STEP;
>      }
> -    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {
> +    if ((hflags >> HFLAGS_BE) & 1) {
>          ctx->singlestep_enabled |= CPU_BRANCH_STEP;
>      }
>      if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
> @@ -7956,10 +7937,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      if (unlikely(ctx->base.singlestep_enabled)) {
>          ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
>      }
> -#if defined(DO_SINGLE_STEP) && 0
> -    /* Single step trace mode */
> -    msr_se = 1;
> -#endif
>  
>      bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
>      ctx->base.max_insns = MIN(ctx->base.max_insns, bound);

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 11255 bytes --]

On Wed, 24 Mar 2021 11:03:02 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
> > Copying flags directly from msr has drawbacks: (1) msr bits
> > mean different things per cpu, (2) msr has 64 bits on 64 cpus
> > while tb->flags has only 32 bits.
> > 
> > Create a enum to define these bits.  Document the origin of each bit
> > and validate those bits that must match MSR.  This fixes the
> > truncation of env->hflags to tb->flags, because we no longer
> > have hflags bits set above bit 31.
> > 
> > Most of the code in ppc_tr_init_disas_context is moved over to
> > hreg_compute_hflags.  Some of it is simple extractions from msr,
> > some requires examining other cpu flags.  Anything that is moved
> > becomes a simple extract from hflags in ppc_tr_init_disas_context.
> > 
> > Several existing bugs are left in ppc_tr_init_disas_context, where
> > additional changes are required -- to be addressed in future patches.
> > 
> > Remove a broken #if 0 block.
> > 
> > Reported-by: Ivan Warren <ivan@vmfacility.fr>
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Applied to ppc-for-6.0.
> 

FYI I can consistently reproduce locally on my laptop an error I'm also
seeing in CI.

$ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg
...
  TEST    threadcount on ppc64abi32
qemu: uncaught target signal 11 (Segmentation fault) - core dumped

Bisect led to this commit in ppc-for-6.0 

> > ---
> >  target/ppc/cpu.h         | 25 ++++++++++++++++
> >  target/ppc/helper_regs.c | 65 +++++++++++++++++++++++++++++++++-------
> >  target/ppc/translate.c   | 55 ++++++++++------------------------
> >  3 files changed, 95 insertions(+), 50 deletions(-)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index fd13489dce..fe6c3f815d 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -585,6 +585,31 @@ enum {
> >      POWERPC_FLAG_HID0_LE  = 0x00400000,$ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg

> >  };
> >  
> > +/*
> > + * Bits for env->hflags.
> > + *
> > + * Most of these bits overlap with corresponding bits in MSR,
> > + * but some come from other sources.  Those that do come from
> > + * the MSR are validated in hreg_compute_hflags.
> > + */
> > +enum {
> > +    HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
> > +    HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
> > +    HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
> > +    HFLAGS_DR = 4,   /* MSR_DR */
> > +    HFLAGS_IR = 5,   /* MSR_IR */
> > +    HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
> > +    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
> > +    HFLAGS_TM = 8,   /* computed from MSR_TM */
> > +    HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> > +    HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
> > +    HFLAGS_FP = 13,  /* MSR_FP */
> > +    HFLAGS_PR = 14,  /* MSR_PR */
> > +    HFLAGS_SA = 22,  /* MSR_SA */
> > +    HFLAGS_AP = 23,  /* MSR_AP */
> > +    HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> > +};
> > +
> >  /*****************************************************************************/
> >  /* Floating point status and control register                                */
> >  #define FPSCR_DRN2   34 /* Decimal Floating-Point rounding control           */
> > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > index a87e354ca2..df9673b90f 100644
> > --- a/target/ppc/helper_regs.c
> > +++ b/target/ppc/helper_regs.c
> > @@ -18,6 +18,7 @@
> >   */
> >  
> >  #include "qemu/osdep.h"
> > +#include "cpu.h"
> >  #include "qemu/main-loop.h"
> >  #include "exec/exec-all.h"
> >  #include "sysemu/kvm.h"
> > @@ -87,24 +88,66 @@ void hreg_compute_mem_idx(CPUPPCState *env)
> >  
> >  void hreg_compute_hflags(CPUPPCState *env)
> >  {
> > -    target_ulong hflags_mask;
> > +    target_ulong msr = env->msr;
> > +    uint32_t ppc_flags = env->flags;
> > +    uint32_t hflags = 0;
> > +    uint32_t msr_mask;
> >  $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg

> > -    /* We 'forget' FE0 & FE1: we'll never generate imprecise exceptions */
> > -    hflags_mask = (1 << MSR_VR) | (1 << MSR_AP) | (1 << MSR_SA) |
> > -        (1 << MSR_PR) | (1 << MSR_FP) | (1 << MSR_SE) | (1 << MSR_BE) |
> > -        (1 << MSR_LE) | (1 << MSR_VSX) | (1 << MSR_IR) | (1 << MSR_DR);
> > -    hflags_mask |= (1ULL << MSR_CM) | (1ULL << MSR_SF) | MSR_HVB;
> > -    hreg_compute_mem_idx(env);
> > -    env->hflags = env->msr & hflags_mask;
> > +    /* Some bits come straight across from MSR. */
> > +    QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
> > +    QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
> > +    QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
> > +    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
> > +    QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
> > +    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
> > +    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
> > +    msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> > +                (1 << MSR_DR) | (1 << MSR_IR) |
> > +                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
> >  
> > -    if (env->flags & POWERPC_FLAG_HID0_LE) {
> > +    if (ppc_flags & POWERPC_FLAG_HID0_LE) {
> >          /*
> >           * Note that MSR_LE is not set in env->msr_mask for this cpu,
> > -         * and so will never be set in msr or hflags at this point.
> > +         * and so will never be set in msr.
> >           */
> >          uint32_t le = extract32(env->spr[SPR_HID0], 3, 1);
> > -        env->hflags |= le << MSR_LE;
> > +        hflags |= le << MSR_LE;
> >      }
> > +
> > +    if (ppc_flags & POWERPC_FLAG_BE) {
> > +        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> > +        msr_mask |= 1 << MSR_BE;
> > +    }
> > +    if (ppc_flags & POWERPC_FLAG_SE) {
> > +        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> > +        msr_mask |= 1 << MSR_SE;
> > +    }
> > +
> > +    if (msr_is_64bit(env, msr)) {
> > +        hflags |= 1 << HFLAGS_64;
> > +    }
> > +    if ((ppc_flags & POWERPC_FLAG_SPE) && (msr & (1 << MSR_SPE))) {
> > +        hflags |= 1 << HFLAGS_SPE;
> > +    }
> > +    if (ppc_flags & POWERPC_FLAG_VRE) {
> > +        QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
> > +        msr_mask |= 1 << MSR_VR;
> > +    }
> > +    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
> > +        hflags |= 1 << HFLAGS_VSX;
> > +    }
> > +    if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
> > +        hflags |= 1 << HFLAGS_TM;
> > +    }
> > +
> > +#ifndef CONFIG_USER_ONLY
> > +    if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> > +        hflags |= 1 << HFLAGS_HV;
> > +    }
> > +#endif
> > +
> > +    env->hflags = hflags | (msr & msr_mask);
> > +    hreg_compute_mem_idx(env);
> >  }
> >  
> >  void cpu_interrupt_exittb(CPUState *cs)
> > diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> > index 0984ce637b..a9325a12e5 100644
> > --- a/target/ppc/translate.c
> > +++ b/target/ppc/translate.c
> > @@ -7879,67 +7879,48 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >  {
> >      DisasContext *ctx = container_of(dcbase, DisasContext, base);
> >      CPUPPCState *env = cs->env_ptr;
> > +    uint32_t hflags = ctx->base.tb->flags;
> >      int bound;
> >  
> >      ctx->exception = POWERPC_EXCP_NONE;
> >      ctx->spr_cb = env->spr_cb;
> > -    ctx->pr = msr_pr;
> > +    ctx->pr = (hflags >> HFLAGS_PR) & 1;
> >      ctx->mem_idx = env->dmmu_idx;
> > -    ctx->dr = msr_dr;
> > -#if !defined(CONFIG_USER_ONLY)
> > -    ctx->hv = msr_hv || !env->has_hv_mode;
> > -#endif
> > +    ctx->dr = (hflags >> HFLAGS_DR) & 1;
> > +    ctx->hv = (hflags >> HFLAGS_HV) & 1;
> >      ctx->insns_flags = env->insns_flags;
> >      ctx->insns_flags2 = env->insns_flags2;
> >      ctx->access_type = -1;
> >      ctx->need_access_type = !mmu_is_64bit(env->mmu_model);
> > -    ctx->le_mode = !!(env->hflags & (1 << MSR_LE));
> > +    ctx->le_mode = (hflags >> HFLAGS_LE) & 1;
> >      ctx->default_tcg_memop_mask = ctx->le_mode ? MO_LE : MO_BE;
> >      ctx->flags = env->flags;
> >  #if defined(TARGET_PPC64)
> > -    ctx->sf_mode = msr_is_64bit(env, env->msr);
> > +    ctx->sf_mode = (hflags >> HFLAGS_64) & 1;
> >      ctx->has_cfar = !!(env->flags & POWERPC_FLAG_CFAR);
> >  #endif
> >      ctx->lazy_tlb_flush = env->mmu_model == POWERPC_MMU_32B
> >          || env->mmu_model == POWERPC_MMU_601
> >          || env->mmu_model & POWERPC_MMU_64;
> >  
> > -    ctx->fpu_enabled = !!msr_fp;
> > -    if ((env->flags & POWERPC_FLAG_SPE) && msr_spe) {
> > -        ctx->spe_enabled = !!msr_spe;
> > -    } else {
> > -        ctx->spe_enabled = false;
> > -    }
> > -    if ((env->flags & POWERPC_FLAG_VRE) && msr_vr) {
> > -        ctx->altivec_enabled = !!msr_vr;
> > -    } else {
> > -        ctx->altivec_enabled = false;
> > -    }
> > -    if ((env->flags & POWERPC_FLAG_VSX) && msr_vsx) {
> > -        ctx->vsx_enabled = !!msr_vsx;
> > -    } else {
> > -        ctx->vsx_enabled = false;
> > -    }
> > +    ctx->fpu_enabled = (hflags >> HFLAGS_FP) & 1;
> > +    ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
> > +    ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
> > +    ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
> >      if ((env->flags & POWERPC_FLAG_SCV)
> >          && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
> >          ctx->scv_enabled = true;
> >      } else {
> >          ctx->scv_enabled = false;
> >      }
> > -#if defined(TARGET_PPC64)
> > -    if ((env->flags & POWERPC_FLAG_TM) && msr_tm) {
> > -        ctx->tm_enabled = !!msr_tm;
> > -    } else {
> > -        ctx->tm_enabled = false;
> > -    }
> > -#endif
> > +    ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> >      ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
> > -    if ((env->flags & POWERPC_FLAG_SE) && msr_se) {
> > -        ctx->singlestep_enabled = CPU_SINGLE_STEP;
> > -    } else {
> > -        ctx->singlestep_enabled = 0;
> > +
> > +    ctx->singlestep_enabled = 0;
> > +    if ((hflags >> HFLAGS_SE) & 1) {
> > +        ctx->singlestep_enabled |= CPU_SINGLE_STEP;
> >      }
> > -    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {
> > +    if ((hflags >> HFLAGS_BE) & 1) {
> >          ctx->singlestep_enabled |= CPU_BRANCH_STEP;
> >      }
> >      if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
> > @@ -7956,10 +7937,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
> >      if (unlikely(ctx->base.singlestep_enabled)) {
> >          ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
> >      }
> > -#if defined(DO_SINGLE_STEP) && 0
> > -    /* Single step trace mode */
> > -    msr_se = 1;
> > -#endif
> >  
> >      bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
> >      ctx->base.max_insns = MIN(ctx->base.max_insns, bound);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Richard Henderson]

On 3/29/21 7:05 AM, Greg Kurz wrote:
> On Wed, 24 Mar 2021 11:03:02 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
>> On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
>>> Copying flags directly from msr has drawbacks: (1) msr bits
>>> mean different things per cpu, (2) msr has 64 bits on 64 cpus
>>> while tb->flags has only 32 bits.
>>>
>>> Create a enum to define these bits.  Document the origin of each bit
>>> and validate those bits that must match MSR.  This fixes the
>>> truncation of env->hflags to tb->flags, because we no longer
>>> have hflags bits set above bit 31.
>>>
>>> Most of the code in ppc_tr_init_disas_context is moved over to
>>> hreg_compute_hflags.  Some of it is simple extractions from msr,
>>> some requires examining other cpu flags.  Anything that is moved
>>> becomes a simple extract from hflags in ppc_tr_init_disas_context.
>>>
>>> Several existing bugs are left in ppc_tr_init_disas_context, where
>>> additional changes are required -- to be addressed in future patches.
>>>
>>> Remove a broken #if 0 block.
>>>
>>> Reported-by: Ivan Warren <ivan@vmfacility.fr>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Applied to ppc-for-6.0.
>>
> 
> FYI I can consistently reproduce locally on my laptop an error I'm also
> seeing in CI.
> 
> $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg
> ...
>    TEST    threadcount on ppc64abi32
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> 
> Bisect led to this commit in ppc-for-6.0

Any more details?  Because this works for me.


r~

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2904 bytes --]

On Mon, Mar 29, 2021 at 10:26:02AM -0600, Richard Henderson wrote:
> On 3/29/21 7:05 AM, Greg Kurz wrote:
> > On Wed, 24 Mar 2021 11:03:02 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Tue, Mar 23, 2021 at 12:43:32PM -0600, Richard Henderson wrote:
> > > > Copying flags directly from msr has drawbacks: (1) msr bits
> > > > mean different things per cpu, (2) msr has 64 bits on 64 cpus
> > > > while tb->flags has only 32 bits.
> > > > 
> > > > Create a enum to define these bits.  Document the origin of each bit
> > > > and validate those bits that must match MSR.  This fixes the
> > > > truncation of env->hflags to tb->flags, because we no longer
> > > > have hflags bits set above bit 31.
> > > > 
> > > > Most of the code in ppc_tr_init_disas_context is moved over to
> > > > hreg_compute_hflags.  Some of it is simple extractions from msr,
> > > > some requires examining other cpu flags.  Anything that is moved
> > > > becomes a simple extract from hflags in ppc_tr_init_disas_context.
> > > > 
> > > > Several existing bugs are left in ppc_tr_init_disas_context, where
> > > > additional changes are required -- to be addressed in future patches.
> > > > 
> > > > Remove a broken #if 0 block.
> > > > 
> > > > Reported-by: Ivan Warren <ivan@vmfacility.fr>
> > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > 
> > > Applied to ppc-for-6.0.
> > > 
> > 
> > FYI I can consistently reproduce locally on my laptop an error I'm also
> > seeing in CI.
> > 
> > $ ./configure --target-list=ppc64abi32-linux-user && make -j all && make check-tcg
> > ...
> >    TEST    threadcount on ppc64abi32
> > qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> > 
> > Bisect led to this commit in ppc-for-6.0
> 
> Any more details?  Because this works for me.

Yeah, I haven't gotten this to fail locally either.

But wait... it gets even weirder.  With gitlab, I can get similar
looking failures with

  A) Just the non-hflags patches from my tree
     https://gitlab.com/dgibson/qemu/-/pipelines/278542370

  B) Just the hflags patches from my / Richard's tree
     https://gitlab.com/dgibson/qemu/-/pipelines/278497244

But I haven't managed to get the same failures with (C) their common
ancestor (i.e. the upstream master at the time I split made the (A)
and (B) branches).
    https://gitlab.com/dgibson/qemu/-/pipelines/278497233

With (A) and (B) the build-user and build-user-static tests don't
always fail, but they generally seem to fail within 2-4 attempts.
I've made a bunch of retries on (C) and haven't managed to get either
build-user or build-user-static to fail.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Richard Henderson]

On 3/29/21 10:54 PM, David Gibson wrote:
>    B) Just the hflags patches from my / Richard's tree
>       https://gitlab.com/dgibson/qemu/-/pipelines/278497244

Look closer at this one -- it's an s390x test that's failing:

make: *** [/builds/dgibson/qemu/tests/Makefile.include:63: 
run-tcg-tests-s390x-linux-user] Error 2


r~

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 792 bytes --]

On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:
> On 3/29/21 10:54 PM, David Gibson wrote:
> >    B) Just the hflags patches from my / Richard's tree
> >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244
> 
> Look closer at this one -- it's an s390x test that's failing:
> 
> make: *** [/builds/dgibson/qemu/tests/Makefile.include:63:
> run-tcg-tests-s390x-linux-user] Error 2

Goldangit!  Good point.  Now I'm even more baffled as to how I wasn't
able to reproduce on master, but was on different variants of my tree.
A whole heap of bad luck, I guess.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 891 bytes --]

On Wed, 31 Mar 2021 11:09:06 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:
> > On 3/29/21 10:54 PM, David Gibson wrote:
> > >    B) Just the hflags patches from my / Richard's tree
> > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244
> > 
> > Look closer at this one -- it's an s390x test that's failing:
> > 

I've been seeing errors with s390x as well in CI but I couldn't
reproduce locally... and of course, now it seems I cannot
reproduce locally with ppc64abi32 either :-\

> > make: *** [/builds/dgibson/qemu/tests/Makefile.include:63:
> > run-tcg-tests-s390x-linux-user] Error 2
> 
> Goldangit!  Good point.  Now I'm even more baffled as to how I wasn't
> able to reproduce on master, but was on different variants of my tree.
> A whole heap of bad luck, I guess.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1880 bytes --]

On Wed, Mar 31, 2021 at 06:04:27AM +0200, Greg Kurz wrote:
> On Wed, 31 Mar 2021 11:09:06 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:
> > > On 3/29/21 10:54 PM, David Gibson wrote:
> > > >    B) Just the hflags patches from my / Richard's tree
> > > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244
> > > 
> > > Look closer at this one -- it's an s390x test that's failing:
> > > 
> 
> I've been seeing errors with s390x as well in CI but I couldn't
> reproduce locally... and of course, now it seems I cannot
> reproduce locally with ppc64abi32 either :-\

Huh.  Well supporting the idea that the issues I've seen on gitlab
were just bad luck, I've now gotten a clean check with the hflags
patches... bug only on my ppc-for-6.1 branch.

The ppc64 bug that Greg was seeing still makes me nervous, as does the
failures which we saw at one point which showed that new hflags assert
explicitly failing.

Since the hflags stuff is of moderate complexity and is a bug fix,
it's not a regression fix.  So, I'm going to postpone that until
ppc-for-6.1, and move ahead with this PR without it.

Richard - the remaining possible problem with the hflags stuff seems
to manifest with the assert failing in the last patch.  However, I'm
guess that's just exposing some more subtle problem introduced by an
earlier patch.  Any chance you could re-order the series to insert the
assert near the beginning, which might give us a better way of
bisecting if this shows up again.

Greg, if this shows up again for you locally, can you please try to
track it down.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Richard Henderson]

On 3/30/21 10:47 PM, David Gibson wrote:
> Richard - the remaining possible problem with the hflags stuff seems
> to manifest with the assert failing in the last patch.  However, I'm
> guess that's just exposing some more subtle problem introduced by an
> earlier patch.  Any chance you could re-order the series to insert the
> assert near the beginning, which might give us a better way of
> bisecting if this shows up again.

No, we begin in a very sorry state.  Every one of the patches reduces the 
number of bugs with hflags.  If we put the assert anywhere but the end, it 
*will* fail.

If you can get the assert to trigger again, we can work out where the next 
problem is hiding.


r~

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1040 bytes --]

On Tue, Mar 30, 2021 at 11:31:25PM -0700, Richard Henderson wrote:
> On 3/30/21 10:47 PM, David Gibson wrote:
> > Richard - the remaining possible problem with the hflags stuff seems
> > to manifest with the assert failing in the last patch.  However, I'm
> > guess that's just exposing some more subtle problem introduced by an
> > earlier patch.  Any chance you could re-order the series to insert the
> > assert near the beginning, which might give us a better way of
> > bisecting if this shows up again.
> 
> No, we begin in a very sorry state.  Every one of the patches reduces the
> number of bugs with hflags.  If we put the assert anywhere but the end, it
> *will* fail.

Heh, ok, understood.

> If you can get the assert to trigger again, we can work out where the next
> problem is hiding.

Ok.  *fingers crossed*

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 02/10] target/ppc: Disconnect hflags from MSR

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 1886 bytes --]

On Wed, 31 Mar 2021 15:47:26 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Mar 31, 2021 at 06:04:27AM +0200, Greg Kurz wrote:
> > On Wed, 31 Mar 2021 11:09:06 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> > 
> > > On Tue, Mar 30, 2021 at 08:01:13AM -0700, Richard Henderson wrote:
> > > > On 3/29/21 10:54 PM, David Gibson wrote:
> > > > >    B) Just the hflags patches from my / Richard's tree
> > > > >       https://gitlab.com/dgibson/qemu/-/pipelines/278497244
> > > > 
> > > > Look closer at this one -- it's an s390x test that's failing:
> > > > 
> > 
> > I've been seeing errors with s390x as well in CI but I couldn't
> > reproduce locally... and of course, now it seems I cannot
> > reproduce locally with ppc64abi32 either :-\
> 
> Huh.  Well supporting the idea that the issues I've seen on gitlab
> were just bad luck, I've now gotten a clean check with the hflags
> patches... bug only on my ppc-for-6.1 branch.
> 
> The ppc64 bug that Greg was seeing still makes me nervous, as does the
> failures which we saw at one point which showed that new hflags assert
> explicitly failing.
> 
> Since the hflags stuff is of moderate complexity and is a bug fix,
> it's not a regression fix.  So, I'm going to postpone that until
> ppc-for-6.1, and move ahead with this PR without it.
> 
> Richard - the remaining possible problem with the hflags stuff seems
> to manifest with the assert failing in the last patch.  However, I'm
> guess that's just exposing some more subtle problem introduced by an
> earlier patch.  Any chance you could re-order the series to insert the
> assert near the beginning, which might give us a better way of
> bisecting if this shows up again.
> 
> Greg, if this shows up again for you locally, can you please try to
> track it down.
> 

Will do.

Cheers,

--
Greg

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 03/10] target/ppc: Reduce env->hflags to uint32_t

[Richard Henderson]

It will be stored in tb->flags, which is also uint32_t,
so let's use the correct size.

Reviewed-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         | 4 ++--
 target/ppc/misc_helper.c | 2 +-
 target/ppc/translate.c   | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index fe6c3f815d..d5f362506a 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -1129,8 +1129,8 @@ struct CPUPPCState {
     bool resume_as_sreset;
 #endif
 
-    /* These resources are used only in QEMU core */
-    target_ulong hflags;
+    /* These resources are used only in TCG */
+    uint32_t hflags;
     target_ulong hflags_compat_nmsr; /* for migration compatibility */
     int immu_idx;     /* precomputed MMU index to speed up insn accesses */
     int dmmu_idx;     /* precomputed MMU index to speed up data accesses */
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index 63e3147eb4..b04b4d7c6e 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -199,7 +199,7 @@ void helper_store_hid0_601(CPUPPCState *env, target_ulong val)
     if ((val ^ hid0) & 0x00000008) {
         /* Change current endianness */
         hreg_compute_hflags(env);
-        qemu_log("%s: set endianness to %c => " TARGET_FMT_lx "\n", __func__,
+        qemu_log("%s: set endianness to %c => %08x\n", __func__,
                  val & 0x8 ? 'l' : 'b', env->hflags);
     }
 }
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index a9325a12e5..a85b890bb0 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7657,7 +7657,7 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
                  env->nip, env->lr, env->ctr, cpu_read_xer(env),
                  cs->cpu_index);
     qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
-                 TARGET_FMT_lx " iidx %d didx %d\n",
+                 "%08x iidx %d didx %d\n",
                  env->msr, env->spr[SPR_HID0],
                  env->hflags, env->immu_idx, env->dmmu_idx);
 #if !defined(NO_TIMER_DUMP)
-- 
2.25.1

Re: [PATCH v5 03/10] target/ppc: Reduce env->hflags to uint32_t

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

On Tue, Mar 23, 2021 at 12:43:33PM -0600, Richard Henderson wrote:
> It will be stored in tb->flags, which is also uint32_t,
> so let's use the correct size.
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.


> ---
>  target/ppc/cpu.h         | 4 ++--
>  target/ppc/misc_helper.c | 2 +-
>  target/ppc/translate.c   | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index fe6c3f815d..d5f362506a 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1129,8 +1129,8 @@ struct CPUPPCState {
>      bool resume_as_sreset;
>  #endif
>  
> -    /* These resources are used only in QEMU core */
> -    target_ulong hflags;
> +    /* These resources are used only in TCG */
> +    uint32_t hflags;
>      target_ulong hflags_compat_nmsr; /* for migration compatibility */
>      int immu_idx;     /* precomputed MMU index to speed up insn accesses */
>      int dmmu_idx;     /* precomputed MMU index to speed up data accesses */
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index 63e3147eb4..b04b4d7c6e 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -199,7 +199,7 @@ void helper_store_hid0_601(CPUPPCState *env, target_ulong val)
>      if ((val ^ hid0) & 0x00000008) {
>          /* Change current endianness */
>          hreg_compute_hflags(env);
> -        qemu_log("%s: set endianness to %c => " TARGET_FMT_lx "\n", __func__,
> +        qemu_log("%s: set endianness to %c => %08x\n", __func__,
>                   val & 0x8 ? 'l' : 'b', env->hflags);
>      }
>  }
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a9325a12e5..a85b890bb0 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7657,7 +7657,7 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>                   env->nip, env->lr, env->ctr, cpu_read_xer(env),
>                   cs->cpu_index);
>      qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
> -                 TARGET_FMT_lx " iidx %d didx %d\n",
> +                 "%08x iidx %d didx %d\n",
>                   env->msr, env->spr[SPR_HID0],
>                   env->hflags, env->immu_idx, env->dmmu_idx);
>  #if !defined(NO_TIMER_DUMP)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 04/10] target/ppc: Put dbcr0 single-step bits into hflags

[Richard Henderson]

Because these bits were not in hflags, the code generated
for single-stepping on BookE was essentially random.
Recompute hflags when storing to dbcr0.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper_regs.c | 24 +++++++++++++++++-------
 target/ppc/misc_helper.c |  3 +++
 target/ppc/translate.c   | 11 -----------
 3 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index df9673b90f..e345966b6b 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -114,13 +114,23 @@ void hreg_compute_hflags(CPUPPCState *env)
         hflags |= le << MSR_LE;
     }
 
-    if (ppc_flags & POWERPC_FLAG_BE) {
-        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
-        msr_mask |= 1 << MSR_BE;
-    }
-    if (ppc_flags & POWERPC_FLAG_SE) {
-        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
-        msr_mask |= 1 << MSR_SE;
+    if (ppc_flags & POWERPC_FLAG_DE) {
+        target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
+        if (dbcr0 & DBCR0_ICMP) {
+            hflags |= 1 << HFLAGS_SE;
+        }
+        if (dbcr0 & DBCR0_BRT) {
+            hflags |= 1 << HFLAGS_BE;
+        }
+    } else {
+        if (ppc_flags & POWERPC_FLAG_BE) {
+            QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
+            msr_mask |= 1 << MSR_BE;
+        }
+        if (ppc_flags & POWERPC_FLAG_SE) {
+            QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
+            msr_mask |= 1 << MSR_SE;
+        }
     }
 
     if (msr_is_64bit(env, msr)) {
diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
index b04b4d7c6e..002958be26 100644
--- a/target/ppc/misc_helper.c
+++ b/target/ppc/misc_helper.c
@@ -215,6 +215,9 @@ void helper_store_403_pbr(CPUPPCState *env, uint32_t num, target_ulong value)
 
 void helper_store_40x_dbcr0(CPUPPCState *env, target_ulong val)
 {
+    /* Bits 26 & 27 affect single-stepping. */
+    hreg_compute_hflags(env);
+    /* Bits 28 & 29 affect reset or shutdown. */
     store_40x_dbcr0(env, val);
 }
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index a85b890bb0..7912495f28 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7923,17 +7923,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     if ((hflags >> HFLAGS_BE) & 1) {
         ctx->singlestep_enabled |= CPU_BRANCH_STEP;
     }
-    if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
-        ctx->singlestep_enabled = 0;
-        target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
-        if (dbcr0 & DBCR0_ICMP) {
-            ctx->singlestep_enabled |= CPU_SINGLE_STEP;
-        }
-        if (dbcr0 & DBCR0_BRT) {
-            ctx->singlestep_enabled |= CPU_BRANCH_STEP;
-        }
-
-    }
     if (unlikely(ctx->base.singlestep_enabled)) {
         ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
     }
-- 
2.25.1

Re: [PATCH v5 04/10] target/ppc: Put dbcr0 single-step bits into hflags

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3472 bytes --]

On Tue, Mar 23, 2021 at 12:43:34PM -0600, Richard Henderson wrote:
> Because these bits were not in hflags, the code generated
> for single-stepping on BookE was essentially random.
> Recompute hflags when storing to dbcr0.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/helper_regs.c | 24 +++++++++++++++++-------
>  target/ppc/misc_helper.c |  3 +++
>  target/ppc/translate.c   | 11 -----------
>  3 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index df9673b90f..e345966b6b 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -114,13 +114,23 @@ void hreg_compute_hflags(CPUPPCState *env)
>          hflags |= le << MSR_LE;
>      }
>  
> -    if (ppc_flags & POWERPC_FLAG_BE) {
> -        QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> -        msr_mask |= 1 << MSR_BE;
> -    }
> -    if (ppc_flags & POWERPC_FLAG_SE) {
> -        QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> -        msr_mask |= 1 << MSR_SE;
> +    if (ppc_flags & POWERPC_FLAG_DE) {
> +        target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> +        if (dbcr0 & DBCR0_ICMP) {
> +            hflags |= 1 << HFLAGS_SE;
> +        }
> +        if (dbcr0 & DBCR0_BRT) {
> +            hflags |= 1 << HFLAGS_BE;
> +        }
> +    } else {
> +        if (ppc_flags & POWERPC_FLAG_BE) {
> +            QEMU_BUILD_BUG_ON(MSR_BE != HFLAGS_BE);
> +            msr_mask |= 1 << MSR_BE;
> +        }
> +        if (ppc_flags & POWERPC_FLAG_SE) {
> +            QEMU_BUILD_BUG_ON(MSR_SE != HFLAGS_SE);
> +            msr_mask |= 1 << MSR_SE;
> +        }
>      }
>  
>      if (msr_is_64bit(env, msr)) {
> diff --git a/target/ppc/misc_helper.c b/target/ppc/misc_helper.c
> index b04b4d7c6e..002958be26 100644
> --- a/target/ppc/misc_helper.c
> +++ b/target/ppc/misc_helper.c
> @@ -215,6 +215,9 @@ void helper_store_403_pbr(CPUPPCState *env, uint32_t num, target_ulong value)
>  
>  void helper_store_40x_dbcr0(CPUPPCState *env, target_ulong val)
>  {
> +    /* Bits 26 & 27 affect single-stepping. */
> +    hreg_compute_hflags(env);
> +    /* Bits 28 & 29 affect reset or shutdown. */
>      store_40x_dbcr0(env, val);
>  }
>  
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index a85b890bb0..7912495f28 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7923,17 +7923,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      if ((hflags >> HFLAGS_BE) & 1) {
>          ctx->singlestep_enabled |= CPU_BRANCH_STEP;
>      }
> -    if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
> -        ctx->singlestep_enabled = 0;
> -        target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
> -        if (dbcr0 & DBCR0_ICMP) {
> -            ctx->singlestep_enabled |= CPU_SINGLE_STEP;
> -        }
> -        if (dbcr0 & DBCR0_BRT) {
> -            ctx->singlestep_enabled |= CPU_BRANCH_STEP;
> -        }
> -
> -    }
>      if (unlikely(ctx->base.singlestep_enabled)) {
>          ctx->singlestep_enabled |= GDBSTUB_SINGLE_STEP;
>      }

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 05/10] target/ppc: Create helper_scv

[Richard Henderson]

Perform the test against FSCR_SCV at runtime, in the helper.

This means we can remove the incorrect set against SCV in
ppc_tr_init_disas_context and do not need to add an HFLAGS bit.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/helper.h      |  1 +
 target/ppc/excp_helper.c |  9 +++++++++
 target/ppc/translate.c   | 20 +++++++-------------
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/target/ppc/helper.h b/target/ppc/helper.h
index 6a4dccf70c..513066d54d 100644
--- a/target/ppc/helper.h
+++ b/target/ppc/helper.h
@@ -13,6 +13,7 @@ DEF_HELPER_1(rfci, void, env)
 DEF_HELPER_1(rfdi, void, env)
 DEF_HELPER_1(rfmci, void, env)
 #if defined(TARGET_PPC64)
+DEF_HELPER_2(scv, noreturn, env, i32)
 DEF_HELPER_2(pminsn, void, env, i32)
 DEF_HELPER_1(rfid, void, env)
 DEF_HELPER_1(rfscv, void, env)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 85de7e6c90..5c95e0c103 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1130,6 +1130,15 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
 }
 
 #if defined(TARGET_PPC64)
+void helper_scv(CPUPPCState *env, uint32_t lev)
+{
+    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
+        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
+    } else {
+        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
+    }
+}
+
 void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
 {
     CPUState *cs;
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 7912495f28..d48c554290 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -173,7 +173,6 @@ struct DisasContext {
     bool vsx_enabled;
     bool spe_enabled;
     bool tm_enabled;
-    bool scv_enabled;
     bool gtse;
     ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
     int singlestep_enabled;
@@ -4081,15 +4080,16 @@ static void gen_sc(DisasContext *ctx)
 #if !defined(CONFIG_USER_ONLY)
 static void gen_scv(DisasContext *ctx)
 {
-    uint32_t lev;
+    uint32_t lev = (ctx->opcode >> 5) & 0x7F;
 
-    if (unlikely(!ctx->scv_enabled)) {
-        gen_exception_err(ctx, POWERPC_EXCP_FU, FSCR_IC_SCV);
-        return;
+    /* Set the PC back to the faulting instruction. */
+    if (ctx->exception == POWERPC_EXCP_NONE) {
+        gen_update_nip(ctx, ctx->base.pc_next - 4);
     }
+    gen_helper_scv(cpu_env, tcg_constant_i32(lev));
 
-    lev = (ctx->opcode >> 5) & 0x7F;
-    gen_exception_err(ctx, POWERPC_SYSCALL_VECTORED, lev);
+    /* This need not be exact, just not POWERPC_EXCP_NONE */
+    ctx->exception = POWERPC_SYSCALL_VECTORED;
 }
 #endif
 #endif
@@ -7907,12 +7907,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
     ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
     ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
-    if ((env->flags & POWERPC_FLAG_SCV)
-        && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
-        ctx->scv_enabled = true;
-    } else {
-        ctx->scv_enabled = false;
-    }
     ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
     ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
 
-- 
2.25.1

Re: [PATCH v5 05/10] target/ppc: Create helper_scv

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3774 bytes --]

On Tue, Mar 23, 2021 at 12:43:35PM -0600, Richard Henderson wrote:
> Perform the test against FSCR_SCV at runtime, in the helper.
> 
> This means we can remove the incorrect set against SCV in
> ppc_tr_init_disas_context and do not need to add an HFLAGS bit.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/helper.h      |  1 +
>  target/ppc/excp_helper.c |  9 +++++++++
>  target/ppc/translate.c   | 20 +++++++-------------
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 6a4dccf70c..513066d54d 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -13,6 +13,7 @@ DEF_HELPER_1(rfci, void, env)
>  DEF_HELPER_1(rfdi, void, env)
>  DEF_HELPER_1(rfmci, void, env)
>  #if defined(TARGET_PPC64)
> +DEF_HELPER_2(scv, noreturn, env, i32)
>  DEF_HELPER_2(pminsn, void, env, i32)
>  DEF_HELPER_1(rfid, void, env)
>  DEF_HELPER_1(rfscv, void, env)
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 85de7e6c90..5c95e0c103 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1130,6 +1130,15 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
>  }
>  
>  #if defined(TARGET_PPC64)
> +void helper_scv(CPUPPCState *env, uint32_t lev)
> +{
> +    if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> +        raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> +    } else {
> +        raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> +    }
> +}
> +
>  void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>  {
>      CPUState *cs;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 7912495f28..d48c554290 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -173,7 +173,6 @@ struct DisasContext {
>      bool vsx_enabled;
>      bool spe_enabled;
>      bool tm_enabled;
> -    bool scv_enabled;
>      bool gtse;
>      ppc_spr_t *spr_cb; /* Needed to check rights for mfspr/mtspr */
>      int singlestep_enabled;
> @@ -4081,15 +4080,16 @@ static void gen_sc(DisasContext *ctx)
>  #if !defined(CONFIG_USER_ONLY)
>  static void gen_scv(DisasContext *ctx)
>  {
> -    uint32_t lev;
> +    uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>  
> -    if (unlikely(!ctx->scv_enabled)) {
> -        gen_exception_err(ctx, POWERPC_EXCP_FU, FSCR_IC_SCV);
> -        return;
> +    /* Set the PC back to the faulting instruction. */
> +    if (ctx->exception == POWERPC_EXCP_NONE) {
> +        gen_update_nip(ctx, ctx->base.pc_next - 4);
>      }
> +    gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>  
> -    lev = (ctx->opcode >> 5) & 0x7F;
> -    gen_exception_err(ctx, POWERPC_SYSCALL_VECTORED, lev);
> +    /* This need not be exact, just not POWERPC_EXCP_NONE */
> +    ctx->exception = POWERPC_SYSCALL_VECTORED;
>  }
>  #endif
>  #endif
> @@ -7907,12 +7907,6 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->spe_enabled = (hflags >> HFLAGS_SPE) & 1;
>      ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
>      ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
> -    if ((env->flags & POWERPC_FLAG_SCV)
> -        && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
> -        ctx->scv_enabled = true;
> -    } else {
> -        ctx->scv_enabled = false;
> -    }
>      ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
>      ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 06/10] target/ppc: Put LPCR[GTSE] in hflags

[Richard Henderson]

Because this bit was not in hflags, the privilege check
for tlb instructions was essentially random.
Recompute hflags when storing to LPCR.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         | 1 +
 target/ppc/helper_regs.c | 3 +++
 target/ppc/mmu-hash64.c  | 3 +++
 target/ppc/translate.c   | 2 +-
 4 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index d5f362506a..3c28ddb331 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -596,6 +596,7 @@ enum {
     HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
     HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
     HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
+    HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
     HFLAGS_DR = 4,   /* MSR_DR */
     HFLAGS_IR = 5,   /* MSR_IR */
     HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index e345966b6b..f85bb14d1d 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -149,6 +149,9 @@ void hreg_compute_hflags(CPUPPCState *env)
     if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
         hflags |= 1 << HFLAGS_TM;
     }
+    if (env->spr[SPR_LPCR] & LPCR_GTSE) {
+        hflags |= 1 << HFLAGS_GTSE;
+    }
 
 #ifndef CONFIG_USER_ONLY
     if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index 0fabc10302..d517a99832 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -30,6 +30,7 @@
 #include "exec/log.h"
 #include "hw/hw.h"
 #include "mmu-book3s-v3.h"
+#include "helper_regs.h"
 
 /* #define DEBUG_SLB */
 
@@ -1125,6 +1126,8 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
     CPUPPCState *env = &cpu->env;
 
     env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
+    /* The gtse bit affects hflags */
+    hreg_compute_hflags(env);
 }
 
 void helper_store_lpcr(CPUPPCState *env, target_ulong val)
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d48c554290..5e629291d3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7908,7 +7908,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
     ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
     ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
-    ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
+    ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
 
     ctx->singlestep_enabled = 0;
     if ((hflags >> HFLAGS_SE) & 1) {
-- 
2.25.1

Re: [PATCH v5 06/10] target/ppc: Put LPCR[GTSE] in hflags

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3221 bytes --]

On Tue, Mar 23, 2021 at 12:43:36PM -0600, Richard Henderson wrote:
> Because this bit was not in hflags, the privilege check
> for tlb instructions was essentially random.
> Recompute hflags when storing to LPCR.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/cpu.h         | 1 +
>  target/ppc/helper_regs.c | 3 +++
>  target/ppc/mmu-hash64.c  | 3 +++
>  target/ppc/translate.c   | 2 +-
>  4 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index d5f362506a..3c28ddb331 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -596,6 +596,7 @@ enum {
>      HFLAGS_LE = 0,   /* MSR_LE -- comes from elsewhere on 601 */
>      HFLAGS_HV = 1,   /* computed from MSR_HV and other state */
>      HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
> +    HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
>      HFLAGS_DR = 4,   /* MSR_DR */
>      HFLAGS_IR = 5,   /* MSR_IR */
>      HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index e345966b6b..f85bb14d1d 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -149,6 +149,9 @@ void hreg_compute_hflags(CPUPPCState *env)
>      if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
>          hflags |= 1 << HFLAGS_TM;
>      }
> +    if (env->spr[SPR_LPCR] & LPCR_GTSE) {
> +        hflags |= 1 << HFLAGS_GTSE;
> +    }
>  
>  #ifndef CONFIG_USER_ONLY
>      if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index 0fabc10302..d517a99832 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -30,6 +30,7 @@
>  #include "exec/log.h"
>  #include "hw/hw.h"
>  #include "mmu-book3s-v3.h"
> +#include "helper_regs.h"
>  
>  /* #define DEBUG_SLB */
>  
> @@ -1125,6 +1126,8 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>      CPUPPCState *env = &cpu->env;
>  
>      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
> +    /* The gtse bit affects hflags */
> +    hreg_compute_hflags(env);
>  }
>  
>  void helper_store_lpcr(CPUPPCState *env, target_ulong val)
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index d48c554290..5e629291d3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7908,7 +7908,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->altivec_enabled = (hflags >> HFLAGS_VR) & 1;
>      ctx->vsx_enabled = (hflags >> HFLAGS_VSX) & 1;
>      ctx->tm_enabled = (hflags >> HFLAGS_TM) & 1;
> -    ctx->gtse = !!(env->spr[SPR_LPCR] & LPCR_GTSE);
> +    ctx->gtse = (hflags >> HFLAGS_GTSE) & 1;
>  
>      ctx->singlestep_enabled = 0;
>      if ((hflags >> HFLAGS_SE) & 1) {

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 07/10] target/ppc: Remove MSR_SA and MSR_AP from hflags

[Richard Henderson]

Nothing within the translator -- or anywhere else for that
matter -- checks MSR_SA or MSR_AP on the 602.  This may be
a mistake.  However, for the moment, we need not record these
bits in hflags.

This allows us to simplify HFLAGS_VSX computation by moving
it to overlap with MSR_VSX.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         |  4 +---
 target/ppc/helper_regs.c | 10 ++++------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 3c28ddb331..2f72f83ee3 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -600,14 +600,12 @@ enum {
     HFLAGS_DR = 4,   /* MSR_DR */
     HFLAGS_IR = 5,   /* MSR_IR */
     HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
-    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
     HFLAGS_TM = 8,   /* computed from MSR_TM */
     HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
     HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
     HFLAGS_FP = 13,  /* MSR_FP */
     HFLAGS_PR = 14,  /* MSR_PR */
-    HFLAGS_SA = 22,  /* MSR_SA */
-    HFLAGS_AP = 23,  /* MSR_AP */
+    HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
     HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
 };
 
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index f85bb14d1d..dd3cd770a3 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -99,11 +99,8 @@ void hreg_compute_hflags(CPUPPCState *env)
     QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
     QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
     QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
-    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
-    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
     msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
-                (1 << MSR_DR) | (1 << MSR_IR) |
-                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
+                (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
 
     if (ppc_flags & POWERPC_FLAG_HID0_LE) {
         /*
@@ -143,8 +140,9 @@ void hreg_compute_hflags(CPUPPCState *env)
         QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
         msr_mask |= 1 << MSR_VR;
     }
-    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
-        hflags |= 1 << HFLAGS_VSX;
+    if (ppc_flags & POWERPC_FLAG_VSX) {
+        QEMU_BUILD_BUG_ON(MSR_VSX != HFLAGS_VSX);
+        msr_mask |= 1 << MSR_VSX;
     }
     if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
         hflags |= 1 << HFLAGS_TM;
-- 
2.25.1

Re: [PATCH v5 07/10] target/ppc: Remove MSR_SA and MSR_AP from hflags

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3200 bytes --]

On Tue, Mar 23, 2021 at 12:43:37PM -0600, Richard Henderson wrote:
> Nothing within the translator -- or anywhere else for that
> matter -- checks MSR_SA or MSR_AP on the 602.  This may be
> a mistake.  However, for the moment, we need not record these
> bits in hflags.

And frankly, even if it's wrong, I suspect the chances of someone
caring enough to fix 602 logic are pretty slim.

> This allows us to simplify HFLAGS_VSX computation by moving
> it to overlap with MSR_VSX.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/cpu.h         |  4 +---
>  target/ppc/helper_regs.c | 10 ++++------
>  2 files changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 3c28ddb331..2f72f83ee3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -600,14 +600,12 @@ enum {
>      HFLAGS_DR = 4,   /* MSR_DR */
>      HFLAGS_IR = 5,   /* MSR_IR */
>      HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
> -    HFLAGS_VSX = 7,  /* from MSR_VSX if cpu has VSX; avoid overlap w/ MSR_AP */
>      HFLAGS_TM = 8,   /* computed from MSR_TM */
>      HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
>      HFLAGS_SE = 10,  /* MSR_SE -- from elsewhere on embedded ppc */
>      HFLAGS_FP = 13,  /* MSR_FP */
>      HFLAGS_PR = 14,  /* MSR_PR */
> -    HFLAGS_SA = 22,  /* MSR_SA */
> -    HFLAGS_AP = 23,  /* MSR_AP */
> +    HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>      HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
>  };
>  
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index f85bb14d1d..dd3cd770a3 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -99,11 +99,8 @@ void hreg_compute_hflags(CPUPPCState *env)
>      QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
>      QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
>      QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
> -    QEMU_BUILD_BUG_ON(MSR_SA != HFLAGS_SA);
> -    QEMU_BUILD_BUG_ON(MSR_AP != HFLAGS_AP);
>      msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> -                (1 << MSR_DR) | (1 << MSR_IR) |
> -                (1 << MSR_FP) | (1 << MSR_SA) | (1 << MSR_AP));
> +                (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
>  
>      if (ppc_flags & POWERPC_FLAG_HID0_LE) {
>          /*
> @@ -143,8 +140,9 @@ void hreg_compute_hflags(CPUPPCState *env)
>          QEMU_BUILD_BUG_ON(MSR_VR != HFLAGS_VR);
>          msr_mask |= 1 << MSR_VR;
>      }
> -    if ((ppc_flags & POWERPC_FLAG_VSX) && (msr & (1 << MSR_VSX))) {
> -        hflags |= 1 << HFLAGS_VSX;
> +    if (ppc_flags & POWERPC_FLAG_VSX) {
> +        QEMU_BUILD_BUG_ON(MSR_VSX != HFLAGS_VSX);
> +        msr_mask |= 1 << MSR_VSX;
>      }
>      if ((ppc_flags & POWERPC_FLAG_TM) && (msr & (1ull << MSR_TM))) {
>          hflags |= 1 << HFLAGS_TM;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 08/10] target/ppc: Remove env->immu_idx and env->dmmu_idx

[Richard Henderson]

We weren't recording MSR_GS in hflags, which means that BookE
memory accesses were essentially random vs Guest State.

Instead of adding this bit directly, record the completed mmu
indexes instead.  This makes it obvious that we are recording
exactly the information that we need.

This also means that we can stop directly recording MSR_IR.

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         | 12 ++++--
 target/ppc/helper_regs.h |  1 -
 target/ppc/helper_regs.c | 89 +++++++++++++++++++---------------------
 target/ppc/machine.c     |  2 +-
 target/ppc/mem_helper.c  |  2 +-
 target/ppc/translate.c   |  6 +--
 6 files changed, 56 insertions(+), 56 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2f72f83ee3..3d021f61f3 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -598,7 +598,6 @@ enum {
     HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
     HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
     HFLAGS_DR = 4,   /* MSR_DR */
-    HFLAGS_IR = 5,   /* MSR_IR */
     HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
     HFLAGS_TM = 8,   /* computed from MSR_TM */
     HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
@@ -607,6 +606,9 @@ enum {
     HFLAGS_PR = 14,  /* MSR_PR */
     HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
     HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
+
+    HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */
+    HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */
 };
 
 /*****************************************************************************/
@@ -1131,8 +1133,6 @@ struct CPUPPCState {
     /* These resources are used only in TCG */
     uint32_t hflags;
     target_ulong hflags_compat_nmsr; /* for migration compatibility */
-    int immu_idx;     /* precomputed MMU index to speed up insn accesses */
-    int dmmu_idx;     /* precomputed MMU index to speed up data accesses */
 
     /* Power management */
     int (*check_pow)(CPUPPCState *env);
@@ -1368,7 +1368,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
 #define MMU_USER_IDX 0
 static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch)
 {
-    return ifetch ? env->immu_idx : env->dmmu_idx;
+#ifdef CONFIG_USER_ONLY
+    return MMU_USER_IDX;
+#else
+    return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7;
+#endif
 }
 
 /* Compatibility modes */
diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
index 4148a442b3..42f26870b9 100644
--- a/target/ppc/helper_regs.h
+++ b/target/ppc/helper_regs.h
@@ -21,7 +21,6 @@
 #define HELPER_REGS_H
 
 void hreg_swap_gpr_tgpr(CPUPPCState *env);
-void hreg_compute_mem_idx(CPUPPCState *env);
 void hreg_compute_hflags(CPUPPCState *env);
 void cpu_interrupt_exittb(CPUState *cs);
 int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index dd3cd770a3..5411a67e9a 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
     env->tgpr[3] = tmp;
 }
 
-void hreg_compute_mem_idx(CPUPPCState *env)
-{
-    /*
-     * This is our encoding for server processors. The architecture
-     * specifies that there is no such thing as userspace with
-     * translation off, however it appears that MacOS does it and some
-     * 32-bit CPUs support it. Weird...
-     *
-     *   0 = Guest User space virtual mode
-     *   1 = Guest Kernel space virtual mode
-     *   2 = Guest User space real mode
-     *   3 = Guest Kernel space real mode
-     *   4 = HV User space virtual mode
-     *   5 = HV Kernel space virtual mode
-     *   6 = HV User space real mode
-     *   7 = HV Kernel space real mode
-     *
-     * For BookE, we need 8 MMU modes as follow:
-     *
-     *  0 = AS 0 HV User space
-     *  1 = AS 0 HV Kernel space
-     *  2 = AS 1 HV User space
-     *  3 = AS 1 HV Kernel space
-     *  4 = AS 0 Guest User space
-     *  5 = AS 0 Guest Kernel space
-     *  6 = AS 1 Guest User space
-     *  7 = AS 1 Guest Kernel space
-     */
-    if (env->mmu_model & POWERPC_MMU_BOOKE) {
-        env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
-        env->immu_idx += msr_is ? 2 : 0;
-        env->dmmu_idx += msr_ds ? 2 : 0;
-        env->immu_idx += msr_gs ? 4 : 0;
-        env->dmmu_idx += msr_gs ? 4 : 0;
-    } else {
-        env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
-        env->immu_idx += msr_ir ? 0 : 2;
-        env->dmmu_idx += msr_dr ? 0 : 2;
-        env->immu_idx += msr_hv ? 4 : 0;
-        env->dmmu_idx += msr_hv ? 4 : 0;
-    }
-}
-
 void hreg_compute_hflags(CPUPPCState *env)
 {
     target_ulong msr = env->msr;
@@ -97,10 +54,9 @@ void hreg_compute_hflags(CPUPPCState *env)
     QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
     QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
     QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
-    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
     QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
     msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
-                (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
+                (1 << MSR_DR) | (1 << MSR_FP));
 
     if (ppc_flags & POWERPC_FLAG_HID0_LE) {
         /*
@@ -155,10 +111,51 @@ void hreg_compute_hflags(CPUPPCState *env)
     if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
         hflags |= 1 << HFLAGS_HV;
     }
+
+    /*
+     * This is our encoding for server processors. The architecture
+     * specifies that there is no such thing as userspace with
+     * translation off, however it appears that MacOS does it and some
+     * 32-bit CPUs support it. Weird...
+     *
+     *   0 = Guest User space virtual mode
+     *   1 = Guest Kernel space virtual mode
+     *   2 = Guest User space real mode
+     *   3 = Guest Kernel space real mode
+     *   4 = HV User space virtual mode
+     *   5 = HV Kernel space virtual mode
+     *   6 = HV User space real mode
+     *   7 = HV Kernel space real mode
+     *
+     * For BookE, we need 8 MMU modes as follow:
+     *
+     *  0 = AS 0 HV User space
+     *  1 = AS 0 HV Kernel space
+     *  2 = AS 1 HV User space
+     *  3 = AS 1 HV Kernel space
+     *  4 = AS 0 Guest User space
+     *  5 = AS 0 Guest Kernel space
+     *  6 = AS 1 Guest User space
+     *  7 = AS 1 Guest Kernel space
+     */
+    unsigned immu_idx, dmmu_idx;
+    dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1;
+    if (env->mmu_model & POWERPC_MMU_BOOKE) {
+        dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0;
+        immu_idx = dmmu_idx;
+        immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
+        dmmu_idx |= msr & (1 << MSR_DS) ? 2 : 0;
+    } else {
+        dmmu_idx |= msr & (1ull << MSR_HV) ? 4 : 0;
+        immu_idx = dmmu_idx;
+        immu_idx |= msr & (1 << MSR_IR) ? 0 : 2;
+        dmmu_idx |= msr & (1 << MSR_DR) ? 0 : 2;
+    }
+    hflags |= immu_idx << HFLAGS_IMMU_IDX;
+    hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
 #endif
 
     env->hflags = hflags | (msr & msr_mask);
-    hreg_compute_mem_idx(env);
 }
 
 void cpu_interrupt_exittb(CPUState *cs)
diff --git a/target/ppc/machine.c b/target/ppc/machine.c
index 09c5765a87..e5bffbe365 100644
--- a/target/ppc/machine.c
+++ b/target/ppc/machine.c
@@ -16,7 +16,7 @@ static void post_load_update_msr(CPUPPCState *env)
 
     /*
      * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
-     * before restoring.  Note that this recomputes hflags and mem_idx.
+     * before restoring.  Note that this recomputes hflags.
      */
     env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
     ppc_store_msr(env, msr);
diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
index f4f7e730de..444b2a30ef 100644
--- a/target/ppc/mem_helper.c
+++ b/target/ppc/mem_helper.c
@@ -278,7 +278,7 @@ static void dcbz_common(CPUPPCState *env, target_ulong addr,
     target_ulong mask, dcbz_size = env->dcache_line_size;
     uint32_t i;
     void *haddr;
-    int mmu_idx = epid ? PPC_TLB_EPID_STORE : env->dmmu_idx;
+    int mmu_idx = epid ? PPC_TLB_EPID_STORE : cpu_mmu_index(env, false);
 
 #if defined(TARGET_PPC64)
     /* Check for dcbz vs dcbzl on 970 */
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 5e629291d3..a53463b9b8 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7658,8 +7658,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
                  cs->cpu_index);
     qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
                  "%08x iidx %d didx %d\n",
-                 env->msr, env->spr[SPR_HID0],
-                 env->hflags, env->immu_idx, env->dmmu_idx);
+                 env->msr, env->spr[SPR_HID0], env->hflags,
+                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
 #if !defined(NO_TIMER_DUMP)
     qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
 #if !defined(CONFIG_USER_ONLY)
@@ -7885,7 +7885,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
     ctx->exception = POWERPC_EXCP_NONE;
     ctx->spr_cb = env->spr_cb;
     ctx->pr = (hflags >> HFLAGS_PR) & 1;
-    ctx->mem_idx = env->dmmu_idx;
+    ctx->mem_idx = (hflags >> HFLAGS_DMMU_IDX) & 7;
     ctx->dr = (hflags >> HFLAGS_DR) & 1;
     ctx->hv = (hflags >> HFLAGS_HV) & 1;
     ctx->insns_flags = env->insns_flags;
-- 
2.25.1

Re: [PATCH v5 08/10] target/ppc: Remove env->immu_idx and env->dmmu_idx

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10448 bytes --]

On Tue, Mar 23, 2021 at 12:43:38PM -0600, Richard Henderson wrote:
> We weren't recording MSR_GS in hflags, which means that BookE
> memory accesses were essentially random vs Guest State.
> 
> Instead of adding this bit directly, record the completed mmu
> indexes instead.  This makes it obvious that we are recording
> exactly the information that we need.
> 
> This also means that we can stop directly recording MSR_IR.
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  target/ppc/cpu.h         | 12 ++++--
>  target/ppc/helper_regs.h |  1 -
>  target/ppc/helper_regs.c | 89 +++++++++++++++++++---------------------
>  target/ppc/machine.c     |  2 +-
>  target/ppc/mem_helper.c  |  2 +-
>  target/ppc/translate.c   |  6 +--
>  6 files changed, 56 insertions(+), 56 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 2f72f83ee3..3d021f61f3 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -598,7 +598,6 @@ enum {
>      HFLAGS_64 = 2,   /* computed from MSR_CE and MSR_SF */
>      HFLAGS_GTSE = 3, /* computed from SPR_LPCR[GTSE] */
>      HFLAGS_DR = 4,   /* MSR_DR */
> -    HFLAGS_IR = 5,   /* MSR_IR */
>      HFLAGS_SPE = 6,  /* from MSR_SPE if cpu has SPE; avoid overlap w/ MSR_VR */
>      HFLAGS_TM = 8,   /* computed from MSR_TM */
>      HFLAGS_BE = 9,   /* MSR_BE -- from elsewhere on embedded ppc */
> @@ -607,6 +606,9 @@ enum {
>      HFLAGS_PR = 14,  /* MSR_PR */
>      HFLAGS_VSX = 23, /* MSR_VSX if cpu has VSX */
>      HFLAGS_VR = 25,  /* MSR_VR if cpu has VRE */
> +
> +    HFLAGS_IMMU_IDX = 26, /* 26..28 -- the composite immu_idx */
> +    HFLAGS_DMMU_IDX = 29, /* 29..31 -- the composite dmmu_idx */
>  };
>  
>  /*****************************************************************************/
> @@ -1131,8 +1133,6 @@ struct CPUPPCState {
>      /* These resources are used only in TCG */
>      uint32_t hflags;
>      target_ulong hflags_compat_nmsr; /* for migration compatibility */
> -    int immu_idx;     /* precomputed MMU index to speed up insn accesses */
> -    int dmmu_idx;     /* precomputed MMU index to speed up data accesses */
>  
>      /* Power management */
>      int (*check_pow)(CPUPPCState *env);
> @@ -1368,7 +1368,11 @@ int ppc_dcr_write(ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
>  #define MMU_USER_IDX 0
>  static inline int cpu_mmu_index(CPUPPCState *env, bool ifetch)
>  {
> -    return ifetch ? env->immu_idx : env->dmmu_idx;
> +#ifdef CONFIG_USER_ONLY
> +    return MMU_USER_IDX;
> +#else
> +    return (env->hflags >> (ifetch ? HFLAGS_IMMU_IDX : HFLAGS_DMMU_IDX)) & 7;
> +#endif
>  }
>  
>  /* Compatibility modes */
> diff --git a/target/ppc/helper_regs.h b/target/ppc/helper_regs.h
> index 4148a442b3..42f26870b9 100644
> --- a/target/ppc/helper_regs.h
> +++ b/target/ppc/helper_regs.h
> @@ -21,7 +21,6 @@
>  #define HELPER_REGS_H
>  
>  void hreg_swap_gpr_tgpr(CPUPPCState *env);
> -void hreg_compute_mem_idx(CPUPPCState *env);
>  void hreg_compute_hflags(CPUPPCState *env);
>  void cpu_interrupt_exittb(CPUState *cs);
>  int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index dd3cd770a3..5411a67e9a 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -43,49 +43,6 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
>      env->tgpr[3] = tmp;
>  }
>  
> -void hreg_compute_mem_idx(CPUPPCState *env)
> -{
> -    /*
> -     * This is our encoding for server processors. The architecture
> -     * specifies that there is no such thing as userspace with
> -     * translation off, however it appears that MacOS does it and some
> -     * 32-bit CPUs support it. Weird...
> -     *
> -     *   0 = Guest User space virtual mode
> -     *   1 = Guest Kernel space virtual mode
> -     *   2 = Guest User space real mode
> -     *   3 = Guest Kernel space real mode
> -     *   4 = HV User space virtual mode
> -     *   5 = HV Kernel space virtual mode
> -     *   6 = HV User space real mode
> -     *   7 = HV Kernel space real mode
> -     *
> -     * For BookE, we need 8 MMU modes as follow:
> -     *
> -     *  0 = AS 0 HV User space
> -     *  1 = AS 0 HV Kernel space
> -     *  2 = AS 1 HV User space
> -     *  3 = AS 1 HV Kernel space
> -     *  4 = AS 0 Guest User space
> -     *  5 = AS 0 Guest Kernel space
> -     *  6 = AS 1 Guest User space
> -     *  7 = AS 1 Guest Kernel space
> -     */
> -    if (env->mmu_model & POWERPC_MMU_BOOKE) {
> -        env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -        env->immu_idx += msr_is ? 2 : 0;
> -        env->dmmu_idx += msr_ds ? 2 : 0;
> -        env->immu_idx += msr_gs ? 4 : 0;
> -        env->dmmu_idx += msr_gs ? 4 : 0;
> -    } else {
> -        env->immu_idx = env->dmmu_idx = msr_pr ? 0 : 1;
> -        env->immu_idx += msr_ir ? 0 : 2;
> -        env->dmmu_idx += msr_dr ? 0 : 2;
> -        env->immu_idx += msr_hv ? 4 : 0;
> -        env->dmmu_idx += msr_hv ? 4 : 0;
> -    }
> -}
> -
>  void hreg_compute_hflags(CPUPPCState *env)
>  {
>      target_ulong msr = env->msr;
> @@ -97,10 +54,9 @@ void hreg_compute_hflags(CPUPPCState *env)
>      QEMU_BUILD_BUG_ON(MSR_LE != HFLAGS_LE);
>      QEMU_BUILD_BUG_ON(MSR_PR != HFLAGS_PR);
>      QEMU_BUILD_BUG_ON(MSR_DR != HFLAGS_DR);
> -    QEMU_BUILD_BUG_ON(MSR_IR != HFLAGS_IR);
>      QEMU_BUILD_BUG_ON(MSR_FP != HFLAGS_FP);
>      msr_mask = ((1 << MSR_LE) | (1 << MSR_PR) |
> -                (1 << MSR_DR) | (1 << MSR_IR) | (1 << MSR_FP));
> +                (1 << MSR_DR) | (1 << MSR_FP));
>  
>      if (ppc_flags & POWERPC_FLAG_HID0_LE) {
>          /*
> @@ -155,10 +111,51 @@ void hreg_compute_hflags(CPUPPCState *env)
>      if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
>          hflags |= 1 << HFLAGS_HV;
>      }
> +
> +    /*
> +     * This is our encoding for server processors. The architecture
> +     * specifies that there is no such thing as userspace with
> +     * translation off, however it appears that MacOS does it and some
> +     * 32-bit CPUs support it. Weird...
> +     *
> +     *   0 = Guest User space virtual mode
> +     *   1 = Guest Kernel space virtual mode
> +     *   2 = Guest User space real mode
> +     *   3 = Guest Kernel space real mode
> +     *   4 = HV User space virtual mode
> +     *   5 = HV Kernel space virtual mode
> +     *   6 = HV User space real mode
> +     *   7 = HV Kernel space real mode
> +     *
> +     * For BookE, we need 8 MMU modes as follow:
> +     *
> +     *  0 = AS 0 HV User space
> +     *  1 = AS 0 HV Kernel space
> +     *  2 = AS 1 HV User space
> +     *  3 = AS 1 HV Kernel space
> +     *  4 = AS 0 Guest User space
> +     *  5 = AS 0 Guest Kernel space
> +     *  6 = AS 1 Guest User space
> +     *  7 = AS 1 Guest Kernel space
> +     */
> +    unsigned immu_idx, dmmu_idx;
> +    dmmu_idx = msr & (1 << MSR_PR) ? 0 : 1;
> +    if (env->mmu_model & POWERPC_MMU_BOOKE) {
> +        dmmu_idx |= msr & (1 << MSR_GS) ? 4 : 0;
> +        immu_idx = dmmu_idx;
> +        immu_idx |= msr & (1 << MSR_IS) ? 2 : 0;
> +        dmmu_idx |= msr & (1 << MSR_DS) ? 2 : 0;
> +    } else {
> +        dmmu_idx |= msr & (1ull << MSR_HV) ? 4 : 0;
> +        immu_idx = dmmu_idx;
> +        immu_idx |= msr & (1 << MSR_IR) ? 0 : 2;
> +        dmmu_idx |= msr & (1 << MSR_DR) ? 0 : 2;
> +    }
> +    hflags |= immu_idx << HFLAGS_IMMU_IDX;
> +    hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
>  #endif
>  
>      env->hflags = hflags | (msr & msr_mask);
> -    hreg_compute_mem_idx(env);
>  }
>  
>  void cpu_interrupt_exittb(CPUState *cs)
> diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> index 09c5765a87..e5bffbe365 100644
> --- a/target/ppc/machine.c
> +++ b/target/ppc/machine.c
> @@ -16,7 +16,7 @@ static void post_load_update_msr(CPUPPCState *env)
>  
>      /*
>       * Invalidate all supported msr bits except MSR_TGPR/MSR_HVB
> -     * before restoring.  Note that this recomputes hflags and mem_idx.
> +     * before restoring.  Note that this recomputes hflags.
>       */
>      env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
>      ppc_store_msr(env, msr);
> diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> index f4f7e730de..444b2a30ef 100644
> --- a/target/ppc/mem_helper.c
> +++ b/target/ppc/mem_helper.c
> @@ -278,7 +278,7 @@ static void dcbz_common(CPUPPCState *env, target_ulong addr,
>      target_ulong mask, dcbz_size = env->dcache_line_size;
>      uint32_t i;
>      void *haddr;
> -    int mmu_idx = epid ? PPC_TLB_EPID_STORE : env->dmmu_idx;
> +    int mmu_idx = epid ? PPC_TLB_EPID_STORE : cpu_mmu_index(env, false);
>  
>  #if defined(TARGET_PPC64)
>      /* Check for dcbz vs dcbzl on 970 */
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 5e629291d3..a53463b9b8 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -7658,8 +7658,8 @@ void ppc_cpu_dump_state(CPUState *cs, FILE *f, int flags)
>                   cs->cpu_index);
>      qemu_fprintf(f, "MSR " TARGET_FMT_lx " HID0 " TARGET_FMT_lx "  HF "
>                   "%08x iidx %d didx %d\n",
> -                 env->msr, env->spr[SPR_HID0],
> -                 env->hflags, env->immu_idx, env->dmmu_idx);
> +                 env->msr, env->spr[SPR_HID0], env->hflags,
> +                 cpu_mmu_index(env, true), cpu_mmu_index(env, false));
>  #if !defined(NO_TIMER_DUMP)
>      qemu_fprintf(f, "TB %08" PRIu32 " %08" PRIu64
>  #if !defined(CONFIG_USER_ONLY)
> @@ -7885,7 +7885,7 @@ static void ppc_tr_init_disas_context(DisasContextBase *dcbase, CPUState *cs)
>      ctx->exception = POWERPC_EXCP_NONE;
>      ctx->spr_cb = env->spr_cb;
>      ctx->pr = (hflags >> HFLAGS_PR) & 1;
> -    ctx->mem_idx = env->dmmu_idx;
> +    ctx->mem_idx = (hflags >> HFLAGS_DMMU_IDX) & 7;
>      ctx->dr = (hflags >> HFLAGS_DR) & 1;
>      ctx->hv = (hflags >> HFLAGS_HV) & 1;
>      ctx->insns_flags = env->insns_flags;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 09/10] linux-user/ppc: Fix msr updates for signal handling

[Richard Henderson]

In save_user_regs, there are two bugs where we OR in a bit number
instead of the bit, clobbering the low bits of MSR.  However:

The MSR_VR and MSR_SPE bits control the availability of the insns.
If the bits were not already set in MSR, then any attempt to access
those registers would result in SIGILL.

For linux-user, we always initialize MSR to the capabilities
of the cpu.  We *could* add checks vs MSR where we currently
check insn_flags and insn_flags2, but we know they match.

Also, there's a stray cut-and-paste comment in restore.

Then, do not force little-endian binaries into big-endian mode.

Finally, use ppc_store_msr for the update to affect hflags.
Which is the reason none of these bugs were previously noticed.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/ppc/cpu_loop.c |  5 +++--
 linux-user/ppc/signal.c   | 23 +++++++++++------------
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
index df71e15a25..4a0f6c8dc2 100644
--- a/linux-user/ppc/cpu_loop.c
+++ b/linux-user/ppc/cpu_loop.c
@@ -492,11 +492,12 @@ void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
 #if defined(TARGET_PPC64)
     int flag = (env->insns_flags2 & PPC2_BOOKE206) ? MSR_CM : MSR_SF;
 #if defined(TARGET_ABI32)
-    env->msr &= ~((target_ulong)1 << flag);
+    ppc_store_msr(env, env->msr & ~((target_ulong)1 << flag));
 #else
-    env->msr |= (target_ulong)1 << flag;
+    ppc_store_msr(env, env->msr | (target_ulong)1 << flag);
 #endif
 #endif
+
     env->nip = regs->nip;
     for(i = 0; i < 32; i++) {
         env->gpr[i] = regs->gpr[i];
diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
index b78613f7c8..bad38f8ed9 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -261,9 +261,6 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
             __put_user(avr->u64[PPC_VEC_HI], &vreg->u64[0]);
             __put_user(avr->u64[PPC_VEC_LO], &vreg->u64[1]);
         }
-        /* Set MSR_VR in the saved MSR value to indicate that
-           frame->mc_vregs contains valid data.  */
-        msr |= MSR_VR;
 #if defined(TARGET_PPC64)
         vrsave = (uint32_t *)&frame->mc_vregs.altivec[33];
         /* 64-bit needs to put a pointer to the vectors in the frame */
@@ -300,9 +297,6 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
         for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
             __put_user(env->gprh[i], &frame->mc_vregs.spe[i]);
         }
-        /* Set MSR_SPE in the saved MSR value to indicate that
-           frame->mc_vregs contains valid data.  */
-        msr |= MSR_SPE;
         __put_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
     }
 #endif
@@ -354,8 +348,10 @@ static void restore_user_regs(CPUPPCState *env,
     __get_user(msr, &frame->mc_gregs[TARGET_PT_MSR]);
 
     /* If doing signal return, restore the previous little-endian mode.  */
-    if (sig)
-        env->msr = (env->msr & ~(1ull << MSR_LE)) | (msr & (1ull << MSR_LE));
+    if (sig) {
+        ppc_store_msr(env, ((env->msr & ~(1ull << MSR_LE)) |
+                            (msr & (1ull << MSR_LE))));
+    }
 
     /* Restore Altivec registers if necessary.  */
     if (env->insns_flags & PPC_ALTIVEC) {
@@ -376,8 +372,6 @@ static void restore_user_regs(CPUPPCState *env,
             __get_user(avr->u64[PPC_VEC_HI], &vreg->u64[0]);
             __get_user(avr->u64[PPC_VEC_LO], &vreg->u64[1]);
         }
-        /* Set MSR_VEC in the saved MSR value to indicate that
-           frame->mc_vregs contains valid data.  */
 #if defined(TARGET_PPC64)
         vrsave = (uint32_t *)&v_regs[33];
 #else
@@ -468,7 +462,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
     env->nip = (target_ulong) ka->_sa_handler;
 
     /* Signal handlers are entered in big-endian mode.  */
-    env->msr &= ~(1ull << MSR_LE);
+    ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
 
     unlock_user_struct(frame, frame_addr, 1);
     return;
@@ -563,8 +557,13 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
     env->nip = (target_ulong) ka->_sa_handler;
 #endif
 
+#ifdef TARGET_WORDS_BIGENDIAN
     /* Signal handlers are entered in big-endian mode.  */
-    env->msr &= ~(1ull << MSR_LE);
+    ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
+#else
+    /* Signal handlers are entered in little-endian mode.  */
+    ppc_store_msr(env, env->msr | (1ull << MSR_LE));
+#endif
 
     unlock_user_struct(rt_sf, rt_sf_addr, 1);
     return;
-- 
2.25.1

Re: [PATCH v5 09/10] linux-user/ppc: Fix msr updates for signal handling

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5218 bytes --]

On Tue, Mar 23, 2021 at 12:43:39PM -0600, Richard Henderson wrote:
> In save_user_regs, there are two bugs where we OR in a bit number
> instead of the bit, clobbering the low bits of MSR.  However:
> 
> The MSR_VR and MSR_SPE bits control the availability of the insns.
> If the bits were not already set in MSR, then any attempt to access
> those registers would result in SIGILL.
> 
> For linux-user, we always initialize MSR to the capabilities
> of the cpu.  We *could* add checks vs MSR where we currently
> check insn_flags and insn_flags2, but we know they match.
> 
> Also, there's a stray cut-and-paste comment in restore.
> 
> Then, do not force little-endian binaries into big-endian mode.
> 
> Finally, use ppc_store_msr for the update to affect hflags.
> Which is the reason none of these bugs were previously noticed.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0.

> ---
>  linux-user/ppc/cpu_loop.c |  5 +++--
>  linux-user/ppc/signal.c   | 23 +++++++++++------------
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
> index df71e15a25..4a0f6c8dc2 100644
> --- a/linux-user/ppc/cpu_loop.c
> +++ b/linux-user/ppc/cpu_loop.c
> @@ -492,11 +492,12 @@ void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs)
>  #if defined(TARGET_PPC64)
>      int flag = (env->insns_flags2 & PPC2_BOOKE206) ? MSR_CM : MSR_SF;
>  #if defined(TARGET_ABI32)
> -    env->msr &= ~((target_ulong)1 << flag);
> +    ppc_store_msr(env, env->msr & ~((target_ulong)1 << flag));
>  #else
> -    env->msr |= (target_ulong)1 << flag;
> +    ppc_store_msr(env, env->msr | (target_ulong)1 << flag);
>  #endif
>  #endif
> +
>      env->nip = regs->nip;
>      for(i = 0; i < 32; i++) {
>          env->gpr[i] = regs->gpr[i];
> diff --git a/linux-user/ppc/signal.c b/linux-user/ppc/signal.c
> index b78613f7c8..bad38f8ed9 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -261,9 +261,6 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
>              __put_user(avr->u64[PPC_VEC_HI], &vreg->u64[0]);
>              __put_user(avr->u64[PPC_VEC_LO], &vreg->u64[1]);
>          }
> -        /* Set MSR_VR in the saved MSR value to indicate that
> -           frame->mc_vregs contains valid data.  */
> -        msr |= MSR_VR;
>  #if defined(TARGET_PPC64)
>          vrsave = (uint32_t *)&frame->mc_vregs.altivec[33];
>          /* 64-bit needs to put a pointer to the vectors in the frame */
> @@ -300,9 +297,6 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
>          for (i = 0; i < ARRAY_SIZE(env->gprh); i++) {
>              __put_user(env->gprh[i], &frame->mc_vregs.spe[i]);
>          }
> -        /* Set MSR_SPE in the saved MSR value to indicate that
> -           frame->mc_vregs contains valid data.  */
> -        msr |= MSR_SPE;
>          __put_user(env->spe_fscr, &frame->mc_vregs.spe[32]);
>      }
>  #endif
> @@ -354,8 +348,10 @@ static void restore_user_regs(CPUPPCState *env,
>      __get_user(msr, &frame->mc_gregs[TARGET_PT_MSR]);
>  
>      /* If doing signal return, restore the previous little-endian mode.  */
> -    if (sig)
> -        env->msr = (env->msr & ~(1ull << MSR_LE)) | (msr & (1ull << MSR_LE));
> +    if (sig) {
> +        ppc_store_msr(env, ((env->msr & ~(1ull << MSR_LE)) |
> +                            (msr & (1ull << MSR_LE))));
> +    }
>  
>      /* Restore Altivec registers if necessary.  */
>      if (env->insns_flags & PPC_ALTIVEC) {
> @@ -376,8 +372,6 @@ static void restore_user_regs(CPUPPCState *env,
>              __get_user(avr->u64[PPC_VEC_HI], &vreg->u64[0]);
>              __get_user(avr->u64[PPC_VEC_LO], &vreg->u64[1]);
>          }
> -        /* Set MSR_VEC in the saved MSR value to indicate that
> -           frame->mc_vregs contains valid data.  */
>  #if defined(TARGET_PPC64)
>          vrsave = (uint32_t *)&v_regs[33];
>  #else
> @@ -468,7 +462,7 @@ void setup_frame(int sig, struct target_sigaction *ka,
>      env->nip = (target_ulong) ka->_sa_handler;
>  
>      /* Signal handlers are entered in big-endian mode.  */
> -    env->msr &= ~(1ull << MSR_LE);
> +    ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
>  
>      unlock_user_struct(frame, frame_addr, 1);
>      return;
> @@ -563,8 +557,13 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>      env->nip = (target_ulong) ka->_sa_handler;
>  #endif
>  
> +#ifdef TARGET_WORDS_BIGENDIAN
>      /* Signal handlers are entered in big-endian mode.  */
> -    env->msr &= ~(1ull << MSR_LE);
> +    ppc_store_msr(env, env->msr & ~(1ull << MSR_LE));
> +#else
> +    /* Signal handlers are entered in little-endian mode.  */
> +    ppc_store_msr(env, env->msr | (1ull << MSR_LE));
> +#endif
>  
>      unlock_user_struct(rt_sf, rt_sf_addr, 1);
>      return;

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[Richard Henderson]

Verify that hflags was updated correctly whenever we change
cpu state that is used by hflags.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/ppc/cpu.h         |  5 +++++
 target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 3d021f61f3..69fc9a2831 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);
  */
 #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
 
+#ifdef CONFIG_DEBUG_TCG
+void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
+                          target_ulong *cs_base, uint32_t *flags);
+#else
 static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
                                         target_ulong *cs_base, uint32_t *flags)
 {
@@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
     *cs_base = 0;
     *flags = env->hflags;
 }
+#endif
 
 void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
 void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,
diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 5411a67e9a..3723872aa6 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
     env->tgpr[3] = tmp;
 }
 
-void hreg_compute_hflags(CPUPPCState *env)
+static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
 {
     target_ulong msr = env->msr;
     uint32_t ppc_flags = env->flags;
@@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)
     hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
 #endif
 
-    env->hflags = hflags | (msr & msr_mask);
+    return hflags | (msr & msr_mask);
 }
 
+void hreg_compute_hflags(CPUPPCState *env)
+{
+    env->hflags = hreg_compute_hflags_value(env);
+}
+
+#ifdef CONFIG_DEBUG_TCG
+void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
+                          target_ulong *cs_base, uint32_t *flags)
+{
+    uint32_t hflags_current = env->hflags;
+    uint32_t hflags_rebuilt;
+
+    *pc = env->nip;
+    *cs_base = 0;
+    *flags = hflags_current;
+
+    hflags_rebuilt = hreg_compute_hflags_value(env);
+    if (unlikely(hflags_current != hflags_rebuilt)) {
+        cpu_abort(env_cpu(env),
+                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
+                  hflags_current, hflags_rebuilt);
+    }
+}
+#endif
+
 void cpu_interrupt_exittb(CPUState *cs)
 {
     if (!kvm_enabled()) {
-- 
2.25.1

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]

On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
> Verify that hflags was updated correctly whenever we change
> cpu state that is used by hflags.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Applied to ppc-for-6.0, thanks.

> ---
>  target/ppc/cpu.h         |  5 +++++
>  target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 3d021f61f3..69fc9a2831 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);
>   */
>  #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
>  
> +#ifdef CONFIG_DEBUG_TCG
> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> +                          target_ulong *cs_base, uint32_t *flags);
> +#else
>  static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
>                                          target_ulong *cs_base, uint32_t *flags)
>  {
> @@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
>      *cs_base = 0;
>      *flags = env->hflags;
>  }
> +#endif
>  
>  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
>  void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 5411a67e9a..3723872aa6 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
>      env->tgpr[3] = tmp;
>  }
>  
> -void hreg_compute_hflags(CPUPPCState *env)
> +static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
>  {
>      target_ulong msr = env->msr;
>      uint32_t ppc_flags = env->flags;
> @@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)
>      hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
>  #endif
>  
> -    env->hflags = hflags | (msr & msr_mask);
> +    return hflags | (msr & msr_mask);
>  }
>  
> +void hreg_compute_hflags(CPUPPCState *env)
> +{
> +    env->hflags = hreg_compute_hflags_value(env);
> +}
> +
> +#ifdef CONFIG_DEBUG_TCG
> +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> +                          target_ulong *cs_base, uint32_t *flags)
> +{
> +    uint32_t hflags_current = env->hflags;
> +    uint32_t hflags_rebuilt;
> +
> +    *pc = env->nip;
> +    *cs_base = 0;
> +    *flags = hflags_current;
> +
> +    hflags_rebuilt = hreg_compute_hflags_value(env);
> +    if (unlikely(hflags_current != hflags_rebuilt)) {
> +        cpu_abort(env_cpu(env),
> +                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
> +                  hflags_current, hflags_rebuilt);
> +    }
> +}
> +#endif
> +
>  void cpu_interrupt_exittb(CPUState *cs)
>  {
>      if (!kvm_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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3597 bytes --]

On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:
> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
> > Verify that hflags was updated correctly whenever we change
> > cpu state that is used by hflags.
> > 
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Applied to ppc-for-6.0, thanks.

Alas, this appears to cause a failure in the 'build-user' test on
gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
sure what's going on.

> 
> > ---
> >  target/ppc/cpu.h         |  5 +++++
> >  target/ppc/helper_regs.c | 29 +++++++++++++++++++++++++++--
> >  2 files changed, 32 insertions(+), 2 deletions(-)
> > 
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index 3d021f61f3..69fc9a2831 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -2425,6 +2425,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer);
> >   */
> >  #define is_book3s_arch2x(ctx) (!!((ctx)->insns_flags & PPC_SEGMENT_64B))
> >  
> > +#ifdef CONFIG_DEBUG_TCG
> > +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> > +                          target_ulong *cs_base, uint32_t *flags);
> > +#else
> >  static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> >                                          target_ulong *cs_base, uint32_t *flags)
> >  {
> > @@ -2432,6 +2436,7 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> >      *cs_base = 0;
> >      *flags = env->hflags;
> >  }
> > +#endif
> >  
> >  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
> >  void QEMU_NORETURN raise_exception_ra(CPUPPCState *env, uint32_t exception,
> > diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> > index 5411a67e9a..3723872aa6 100644
> > --- a/target/ppc/helper_regs.c
> > +++ b/target/ppc/helper_regs.c
> > @@ -43,7 +43,7 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
> >      env->tgpr[3] = tmp;
> >  }
> >  
> > -void hreg_compute_hflags(CPUPPCState *env)
> > +static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
> >  {
> >      target_ulong msr = env->msr;
> >      uint32_t ppc_flags = env->flags;
> > @@ -155,9 +155,34 @@ void hreg_compute_hflags(CPUPPCState *env)
> >      hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
> >  #endif
> >  
> > -    env->hflags = hflags | (msr & msr_mask);
> > +    return hflags | (msr & msr_mask);
> >  }
> >  
> > +void hreg_compute_hflags(CPUPPCState *env)
> > +{
> > +    env->hflags = hreg_compute_hflags_value(env);
> > +}
> > +
> > +#ifdef CONFIG_DEBUG_TCG
> > +void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
> > +                          target_ulong *cs_base, uint32_t *flags)
> > +{
> > +    uint32_t hflags_current = env->hflags;
> > +    uint32_t hflags_rebuilt;
> > +
> > +    *pc = env->nip;
> > +    *cs_base = 0;
> > +    *flags = hflags_current;
> > +
> > +    hflags_rebuilt = hreg_compute_hflags_value(env);
> > +    if (unlikely(hflags_current != hflags_rebuilt)) {
> > +        cpu_abort(env_cpu(env),
> > +                  "TCG hflags mismatch (current:0x%08x rebuilt:0x%08x)\n",
> > +                  hflags_current, hflags_rebuilt);
> > +    }
> > +}
> > +#endif
> > +
> >  void cpu_interrupt_exittb(CPUState *cs)
> >  {
> >      if (!kvm_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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[Richard Henderson]

On 3/25/21 2:47 AM, David Gibson wrote:
> On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:
>> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
>>> Verify that hflags was updated correctly whenever we change
>>> cpu state that is used by hflags.
>>>
>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>
>> Applied to ppc-for-6.0, thanks.
> 
> Alas, this appears to cause a failure in the 'build-user' test on
> gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
> sure what's going on.

I guess you mean e.g.

https://gitlab.com/dgibson/qemu/-/jobs/1126364147

?  I'll have a look.


r~

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[Richard Henderson]

On 3/26/21 6:41 AM, Richard Henderson wrote:
> On 3/25/21 2:47 AM, David Gibson wrote:
>> On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:
>>> On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
>>>> Verify that hflags was updated correctly whenever we change
>>>> cpu state that is used by hflags.
>>>>
>>>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>>>
>>> Applied to ppc-for-6.0, thanks.
>>
>> Alas, this appears to cause a failure in the 'build-user' test on
>> gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
>> sure what's going on.
> 
> I guess you mean e.g.
> 
> https://gitlab.com/dgibson/qemu/-/jobs/1126364147
> 
> ?  I'll have a look.

I haven't been able to repo locally, or on gitlab:

https://gitlab.com/rth7680/qemu/-/pipelines/277073704

Have you tried simply re-running that job?


r~

Re: [PATCH v5 10/10] target/ppc: Validate hflags with CONFIG_DEBUG_TCG

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

On Sat, Mar 27, 2021 at 06:46:15AM -0600, Richard Henderson wrote:
> On 3/26/21 6:41 AM, Richard Henderson wrote:
> > On 3/25/21 2:47 AM, David Gibson wrote:
> > > On Wed, Mar 24, 2021 at 11:12:20AM +1100, David Gibson wrote:
> > > > On Tue, Mar 23, 2021 at 12:43:40PM -0600, Richard Henderson wrote:
> > > > > Verify that hflags was updated correctly whenever we change
> > > > > cpu state that is used by hflags.
> > > > > 
> > > > > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > > > 
> > > > Applied to ppc-for-6.0, thanks.
> > > 
> > > Alas, this appears to cause a failure in the 'build-user' test on
> > > gitlab CI :(.  I haven't managed to reproduce it locally, so I'm not
> > > sure what's going on.
> > 
> > I guess you mean e.g.
> > 
> > https://gitlab.com/dgibson/qemu/-/jobs/1126364147

Yes, sorry I meant to give you a link.

> > 
> > ?  I'll have a look.
> 
> I haven't been able to repo locally, or on gitlab:
> 
> https://gitlab.com/rth7680/qemu/-/pipelines/277073704

Huh..

> Have you tried simply re-running that job?

Several times :/

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]