Re: [PATCH 3/4] target/ppc: Split out helper_dbczl for 970

[Nicholas Piggin]

On Wed Jul 3, 2024 at 9:46 AM AEST, Richard Henderson wrote:
> We can determine at translation time whether the insn is or
> is not dbczl.  We must retain a runtime check against the
> HID5 register, but we can move that to a separate function
> that never affects other ppc models.

Looks right I think. You could go one further and have the
HID bit on 970 a hflag, but that might be overkill without
numbers...

>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  target/ppc/helper.h     |  7 +++++--
>  target/ppc/mem_helper.c | 34 +++++++++++++++++++++-------------
>  target/ppc/translate.c  | 24 ++++++++++++++----------
>  3 files changed, 40 insertions(+), 25 deletions(-)
>
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 76b8f25c77..afc56855ff 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -46,8 +46,11 @@ DEF_HELPER_FLAGS_3(stmw, TCG_CALL_NO_WG, void, env, tl, 
> i32)
>  DEF_HELPER_4(lsw, void, env, tl, i32, i32)
>  DEF_HELPER_5(lswx, void, env, tl, i32, i32, i32)
>  DEF_HELPER_FLAGS_4(stsw, TCG_CALL_NO_WG, void, env, tl, i32, i32)
> -DEF_HELPER_FLAGS_3(dcbz, TCG_CALL_NO_WG, void, env, tl, i32)
> -DEF_HELPER_FLAGS_3(dcbzep, TCG_CALL_NO_WG, void, env, tl, i32)
> +DEF_HELPER_FLAGS_2(dcbz, TCG_CALL_NO_WG, void, env, tl)
> +DEF_HELPER_FLAGS_2(dcbzep, TCG_CALL_NO_WG, void, env, tl)
> +#ifdef TARGET_PPC64
> +DEF_HELPER_FLAGS_2(dcbzl, TCG_CALL_NO_WG, void, env, tl)
> +#endif
>  DEF_HELPER_FLAGS_2(icbi, TCG_CALL_NO_WG, void, env, tl)
>  DEF_HELPER_FLAGS_2(icbiep, TCG_CALL_NO_WG, void, env, tl)
>  DEF_HELPER_5(lscbx, tl, env, tl, i32, i32, i32)
> diff --git a/target/ppc/mem_helper.c b/target/ppc/mem_helper.c
> index 5067919ff8..d4957efd6e 100644
> --- a/target/ppc/mem_helper.c
> +++ b/target/ppc/mem_helper.c
> @@ -296,26 +296,34 @@ static void dcbz_common(CPUPPCState *env, target_ulong 
> addr,
>      }
>  }
>  
> -void helper_dcbz(CPUPPCState *env, target_ulong addr, uint32_t opcode)
> +void helper_dcbz(CPUPPCState *env, target_ulong addr)
>  {
> -    int dcbz_size = env->dcache_line_size;
> -
> -#if defined(TARGET_PPC64)
> -    /* Check for dcbz vs dcbzl on 970 */
> -    if (env->excp_model == POWERPC_EXCP_970 &&
> -        !(opcode & 0x00200000) && ((env->spr[SPR_970_HID5] >> 7) & 0x3) == 
> 1) {
> -        dcbz_size = 32;
> -    }
> -#endif
> -
> -    dcbz_common(env, addr, dcbz_size, ppc_env_mmu_index(env, false), 
> GETPC());
> +    dcbz_common(env, addr, env->dcache_line_size,
> +                ppc_env_mmu_index(env, false), GETPC());
>  }
>  
> -void helper_dcbzep(CPUPPCState *env, target_ulong addr, uint32_t opcode)
> +void helper_dcbzep(CPUPPCState *env, target_ulong addr)
>  {
>      dcbz_common(env, addr, env->dcache_line_size, PPC_TLB_EPID_STORE, 
> GETPC());
>  }
>  
> +#ifdef TARGET_PPC64
> +void helper_dcbzl(CPUPPCState *env, target_ulong addr)
> +{
> +    int dcbz_size = env->dcache_line_size;
> +
> +    /*
> +     * The translator checked for POWERPC_EXCP_970.
> +     * All that's left is to check HID5.
> +     */
> +    if (((env->spr[SPR_970_HID5] >> 7) & 0x3) == 1) {
> +        dcbz_size = 32;
> +    }
> +
> +    dcbz_common(env, addr, dcbz_size, ppc_env_mmu_index(env, false), 
> GETPC());
> +}
> +#endif
> +
>  void helper_icbi(CPUPPCState *env, target_ulong addr)
>  {
>      addr &= ~(env->dcache_line_size - 1);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 0bc16d7251..2664c94522 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -200,6 +200,7 @@ struct DisasContext {
>      uint32_t flags;
>      uint64_t insns_flags;
>      uint64_t insns_flags2;
> +    powerpc_excp_t excp_model;

Should we make this TARGET_PPC64 only? I think so, I can check
it an fold that in.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

Thanks,
Nick

>  };
>  
>  #define DISAS_EXIT         DISAS_TARGET_0  /* exit to main loop, pc updated 
> */
> @@ -4445,27 +4446,29 @@ static void gen_dcblc(DisasContext *ctx)
>  /* dcbz */
>  static void gen_dcbz(DisasContext *ctx)
>  {
> -    TCGv tcgv_addr;
> -    TCGv_i32 tcgv_op;
> +    TCGv tcgv_addr = tcg_temp_new();
>  
>      gen_set_access_type(ctx, ACCESS_CACHE);
> -    tcgv_addr = tcg_temp_new();
> -    tcgv_op = tcg_constant_i32(ctx->opcode & 0x03FF000);
>      gen_addr_reg_index(ctx, tcgv_addr);
> -    gen_helper_dcbz(tcg_env, tcgv_addr, tcgv_op);
> +
> +#ifdef TARGET_PPC64
> +    if (ctx->excp_model == POWERPC_EXCP_970 && !(ctx->opcode & 0x00200000)) {
> +        gen_helper_dcbzl(tcg_env, tcgv_addr);
> +        return;
> +    }
> +#endif
> +
> +    gen_helper_dcbz(tcg_env, tcgv_addr);
>  }
>  
>  /* dcbzep */
>  static void gen_dcbzep(DisasContext *ctx)
>  {
> -    TCGv tcgv_addr;
> -    TCGv_i32 tcgv_op;
> +    TCGv tcgv_addr = tcg_temp_new();
>  
>      gen_set_access_type(ctx, ACCESS_CACHE);
> -    tcgv_addr = tcg_temp_new();
> -    tcgv_op = tcg_constant_i32(ctx->opcode & 0x03FF000);
>      gen_addr_reg_index(ctx, tcgv_addr);
> -    gen_helper_dcbzep(tcg_env, tcgv_addr, tcgv_op);
> +    gen_helper_dcbzep(tcg_env, tcgv_addr);
>  }
>  
>  /* dst / dstt */
> @@ -6480,6 +6483,7 @@ static void ppc_tr_init_disas_context(DisasContextBase 
> *dcbase, CPUState *cs)
>      ctx->hv = (hflags >> HFLAGS_HV) & 1;
>      ctx->insns_flags = env->insns_flags;
>      ctx->insns_flags2 = env->insns_flags2;
> +    ctx->excp_model = env->excp_model;
>      ctx->access_type = -1;
>      ctx->need_access_type = !mmu_is_64bit(env->mmu_model);
>      ctx->le_mode = (hflags >> HFLAGS_LE) & 1;