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

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

[Richard Henderson]

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 <david@gibson.dropbear.id.au>
Reported-by: Ivan Warren <ivan@vmfacility.fr>
Signed-off-by: Richard Henderson <richard.henderson@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;
 }
 
 void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
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->exception = POWERPC_EXCP_NONE;
     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;
 
-    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 >> 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
 
     bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
     ctx->base.max_insns = MIN(ctx->base.max_insns, bound);
-- 
2.25.1

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

[David Gibson]

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

On Sat, Jan 23, 2021 at 05:24:22PM -1000, 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.

My knowledge of TCG isn't great, so I'm pretty much prepared to accept
this is correct on your say so.

But that commit message feels like it's following on from a
conversation that's not here, nor linked.  It'd be great if it
explained how said hflags truncation is happening, because it's
certainly not obvious to someone with only a fair to middling
understanding of TCG.


> Cc: David Gibson <david@gibson.dropbear.id.au>
> Reported-by: Ivan Warren <ivan@vmfacility.fr>
> Signed-off-by: Richard Henderson <richard.henderson@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;
>  }
>  
>  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
> 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->exception = POWERPC_EXCP_NONE;
>      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;
>  
> -    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 >> 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
>  
>      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] target/ppc: Fix truncation of env->hflags

[Ivan Warren]

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

Richard,

Reviewed-by:  Ivan Warren  <ivan@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 <david@gibson.dropbear.id.au>
> Reported-by: Ivan Warren <ivan@vmfacility.fr>
> Signed-off-by: Richard Henderson <richard.henderson@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;
>   }
>   
>   void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
> 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->exception = POWERPC_EXCP_NONE;
>       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;
>   
> -    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 >> 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
>   
>       bound = -(ctx->base.pc_first | TARGET_PAGE_MASK) / 4;
>       ctx->base.max_insns = MIN(ctx->base.max_insns, bound);


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 3997 bytes --]

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

[Richard Henderson]

On 1/23/21 6:46 PM, David Gibson wrote:
> On Sat, Jan 23, 2021 at 05:24:22PM -1000, 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.
> 
> My knowledge of TCG isn't great, so I'm pretty much prepared to accept
> this is correct on your say so.
> 
> But that commit message feels like it's following on from a
> conversation that's not here, nor linked.  It'd be great if it
> explained how said hflags truncation is happening, because it's
> certainly not obvious to someone with only a fair to middling
> understanding of TCG.

Mm, fair.

How about:

The assignment from env->hflags to tb->flags truncates
target_ulong to uint32_t.  This loses important bits from
the top of hflags, which results in incorrect tb selection.

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

In translate, extract most bits from a local hflags variable.
All of the checks vs env->flags are redundant with env->msr_mask
in that msr bits cannot be set when the feature is not available.
Mark several cases where code generation is *not* derived from
data stored within hashed elements of the tb.


r~

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

[Alex Bennée]

Richard Henderson <richard.henderson@linaro.org> writes:

> On 1/23/21 6:46 PM, David Gibson wrote:
>> On Sat, Jan 23, 2021 at 05:24:22PM -1000, 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.
>> 
>> My knowledge of TCG isn't great, so I'm pretty much prepared to accept
>> this is correct on your say so.
>> 
>> But that commit message feels like it's following on from a
>> conversation that's not here, nor linked.  It'd be great if it
>> explained how said hflags truncation is happening, because it's
>> certainly not obvious to someone with only a fair to middling
>> understanding of TCG.
>
> Mm, fair.
>
> How about:
>
> The assignment from env->hflags to tb->flags truncates
> target_ulong to uint32_t.  This loses important bits from
> the top of hflags, which results in incorrect tb selection.

We are just putting off the day we declare tb->flags to be 64 bit or end
up renaming cs_base to
tb->cs_base_or_extra_flag_bits_we_could_not_fit_in_flags. The fact that
cs_base is expressed in terms of target_ulong worries me if there is
ever any hflag state above bit 32 for the ppc32 targets.

>
> Use the cs_base field instead, because it happens to be the
> same size as hflags (and MSR fom which hflags is derived).
>
> In translate, extract most bits from a local hflags variable.
> All of the checks vs env->flags are redundant with env->msr_mask
> in that msr bits cannot be set when the feature is not available.
> Mark several cases where code generation is *not* derived from
> data stored within hashed elements of the tb.
>
>
> r~


-- 
Alex Bennée

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

[David Gibson]

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

On Sun, Jan 24, 2021 at 09:38:04AM -1000, Richard Henderson wrote:
> On 1/23/21 6:46 PM, David Gibson wrote:
> > On Sat, Jan 23, 2021 at 05:24:22PM -1000, 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.
> > 
> > My knowledge of TCG isn't great, so I'm pretty much prepared to accept
> > this is correct on your say so.
> > 
> > But that commit message feels like it's following on from a
> > conversation that's not here, nor linked.  It'd be great if it
> > explained how said hflags truncation is happening, because it's
> > certainly not obvious to someone with only a fair to middling
> > understanding of TCG.
> 
> Mm, fair.
> 
> How about:
> 
> The assignment from env->hflags to tb->flags truncates
> target_ulong to uint32_t.  This loses important bits from
> the top of hflags, which results in incorrect tb selection.
> 
> Use the cs_base field instead, because it happens to be the
> same size as hflags (and MSR fom which hflags is derived).
> 
> In translate, extract most bits from a local hflags variable.
> All of the checks vs env->flags are redundant with env->msr_mask
> in that msr bits cannot be set when the feature is not available.
> Mark several cases where code generation is *not* derived from
> data stored within hashed elements of the tb.

Thanks, I've applied the patch with the updated description.

> 
> 
> r~
> 

-- 
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] target/ppc: Fix truncation of env->hflags

[David Gibson]

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

On Sat, Jan 23, 2021 at 05:24:22PM -1000, 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 <david@gibson.dropbear.id.au>
> Reported-by: Ivan Warren <ivan@vmfacility.fr>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Well, I don't know why, but somehow this patch is breaking one of the
acceptance tests:

 (043/134) tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_ppc64_e500: INTERRUPTED: Test interrupted by SIGTERM\nRunner error occurred: Timeout reached\nOriginal status: ERROR\n{'name': '043-tests/acceptance/boot_linux_console.py:BootLinuxConsole.test_ppc64_e500', 'logdir': '/home/dwg/src/qemu/build/normal/tests/results/job-2021-02-10T15.04... (90.26 s)

From that timeout, I'm guessing something about this is causing the
boot to wedge.

So, I've removed this from my tree for now, I'll need a fixed version
to proceed with.


> ---
>  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;
>  }
>  
>  void QEMU_NORETURN raise_exception(CPUPPCState *env, uint32_t exception);
> 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->exception = POWERPC_EXCP_NONE;
>      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;
>  
> -    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 >> 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
>  
>      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 --]