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

[David Gibson <david@gibson.dropbear.id.au>]

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