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

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

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