Re: [PATCH] target/ppc: Fix truncation of env->hflags

[Ivan Warren]

Richard,

Reviewed-by:  Ivan Warren  <i...@vmfacility.fr>

Thanks a million !

--Ivan

On 1/24/2021 4:24 AM, Richard Henderson wrote:

Use the cs_base field, because it happens to be the same
size as hflags (and MSR, from which hflags is derived).

In translate, extract most bits from a local hflags variable.
Mark several cases where code generation is *not* derived from
data stored within the hashed elements of the TranslationBlock.

Cc: David Gibson <da...@gibson.dropbear.id.au>
Reported-by: Ivan Warren <i...@vmfacility.fr>
Signed-off-by: Richard Henderson <richard.hender...@linaro.org>
---
  target/ppc/cpu.h       |  4 +--
  target/ppc/translate.c | 64 ++++++++++++++++--------------------------
  2 files changed, 26 insertions(+), 42 deletions(-)

diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 2609e4082e..4a05e4e544 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2396,8 +2396,8 @@ static inline void cpu_get_tb_cpu_state(CPUPPCState *env, 
target_ulong *pc,
                                          target_ulong *cs_base, uint32_t 
*flags)
  {
      *pc = env->nip;
-    *cs_base = 0;
-    *flags = env->hflags;
+    *cs_base = env->hflags;
+    *flags = 0;
  }

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 0984ce637b..1eb2e1b0c6 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -7879,47 +7879,37 @@ static void ppc_tr_init_disas_context(DisasContextBase 
*dcbase, CPUState *cs)
  {
      DisasContext *ctx = container_of(dcbase, DisasContext, base);
      CPUPPCState *env = cs->env_ptr;
+    target_ulong hflags = ctx->base.tb->cs_base;
      int bound;

      ctx->spr_cb = env->spr_cb;
-    ctx->pr = msr_pr;
+    ctx->pr = (hflags >> MSR_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;
+    ctx->dr = (hflags >> MSR_DR) & 1;
+#if defined(TARGET_PPC64) && !defined(CONFIG_USER_ONLY)
+    ctx->hv = (hflags >> MSR_HV) & 1;
  #endif
      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 >> MSR_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 >> MSR_SF) & 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;

-    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 >> MSR_FP) & 1;
+    ctx->spe_enabled = (hflags >> MSR_SPE) & 1;
+    ctx->altivec_enabled = (hflags >> MSR_VR) & 1;
+    ctx->vsx_enabled = (hflags >> MSR_VSX) & 1;
+    /* FIXME: This needs to be stored in env->hflags_nmsr. */
      if ((env->flags & POWERPC_FLAG_SCV)
          && (env->spr[SPR_FSCR] & (1ull << FSCR_SCV))) {
          ctx->scv_enabled = true;
@@ -7927,23 +7917,21 @@ static void ppc_tr_init_disas_context(DisasContextBase 
*dcbase, CPUState *cs)
          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;
-    }
+    ctx->tm_enabled = (hflags >> MSR_TM) & 1;
  #endif
+    /* FIXME: This needs to be stored in env->hflags_nmsr. */
      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;
-    }
-    if ((env->flags & POWERPC_FLAG_BE) && msr_be) {
-        ctx->singlestep_enabled |= CPU_BRANCH_STEP;
-    }
-    if ((env->flags & POWERPC_FLAG_DE) && msr_de) {
+
+    ctx->singlestep_enabled = ((hflags >> MSR_SE) & 1 ? CPU_SINGLE_STEP : 0)
+                            | ((hflags >> MSR_BE) & 1 ? CPU_BRANCH_STEP : 0);
+
+    if ((hflags >> MSR_DE) & 1) {
          ctx->singlestep_enabled = 0;
+        /*
+         * FIXME: This needs to be stored in env->hflags_nmsr,
+         * probably overlapping MSR_SE/MSR_BE like we do for
+         * MSR_LE and the ppc 601.
+         */
          target_ulong dbcr0 = env->spr[SPR_BOOKE_DBCR0];
          if (dbcr0 & DBCR0_ICMP) {
              ctx->singlestep_enabled |= CPU_SINGLE_STEP;
@@ -7956,10 +7944,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

      ctx->base.max_insns = MIN(ctx->base.max_insns, bound);

void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception); ctx->exception = POWERPC_EXCP_NONE; - ctx->fpu_enabled = !!msr_fp; bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;

smime.p7s
Description: S/MIME Cryptographic Signature