Re: [Qemu-devel] [Qemu-ppc] [PATCH 8/9] ppc: Add missing slbfee. instruction on ppc64 BookS processors

[Cédric Le Goater]

On 06/07/2016 04:50 AM, Benjamin Herrenschmidt wrote:
> Used to lookup SLB entries by address, for some reason it was missing.
> 
> Signed-off-by: Benjamin Herrenschmidt <address@hidden>
> ---
>  target-ppc/helper.h     |  1 +
>  target-ppc/mmu-hash64.c | 30 ++++++++++++++++++++++++++++++
>  target-ppc/translate.c  | 26 ++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
> 
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index 0526322..f4410a8 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -550,6 +550,7 @@ DEF_HELPER_FLAGS_2(tlbiva, TCG_CALL_NO_RWG, void, env, tl)
>  DEF_HELPER_FLAGS_3(store_slb, TCG_CALL_NO_RWG, void, env, tl, tl)
>  DEF_HELPER_2(load_slb_esid, tl, env, tl)
>  DEF_HELPER_2(load_slb_vsid, tl, env, tl)
> +DEF_HELPER_2(find_slb_vsid, tl, env, tl)
>  DEF_HELPER_FLAGS_1(slbia, TCG_CALL_NO_RWG, void, env)
>  DEF_HELPER_FLAGS_2(slbie, TCG_CALL_NO_RWG, void, env, tl)
>  #endif
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index ea6e99a..668da5e 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -219,6 +219,24 @@ static int ppc_load_slb_vsid(PowerPCCPU *cpu, 
> target_ulong rb,
>      return 0;
>  }
> 
> +static int ppc_find_slb_vsid(PowerPCCPU *cpu, target_ulong rb,
> +                             target_ulong *rt)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    ppc_slb_t *slb;
> +
> +    if (!msr_is_64bit(env, env->msr)) {
> +        rb &= 0xffffffff;
> +    }
> +    slb = slb_lookup(cpu, rb);
> +    if (slb == NULL) {
> +        *rt = (target_ulong)-1ul;


So, I was trying today to reconciliate the powernv patchset with 
the current HEAD of qemu when I bumped into the old version of this 
patch. I checked the specs and when no slb are found, rt should 
just be 0. The machine check is only generated when multiple matching 
entries are found. So the above probably needs a fix, at least for 
the NULL case ? 

C.

> +    } else {
> +        *rt = slb->vsid;
> +    }
> +    return 0;
> +}
> +
>  void helper_store_slb(CPUPPCState *env, target_ulong rb, target_ulong rs)
>  {
>      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> @@ -241,6 +259,18 @@ target_ulong helper_load_slb_esid(CPUPPCState *env, 
> target_ulong rb)
>      return rt;
>  }
> 
> +target_ulong helper_find_slb_vsid(CPUPPCState *env, target_ulong rb)
> +{
> +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    target_ulong rt = 0;
> +
> +    if (ppc_find_slb_vsid(cpu, rb, &rt) < 0) {
> +        helper_raise_exception_err(env, POWERPC_EXCP_PROGRAM,
> +                                   POWERPC_EXCP_INVAL);
> +    }
> +    return rt;
> +}
> +
>  target_ulong helper_load_slb_vsid(CPUPPCState *env, target_ulong rb)
>  {
>      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 33a9223..a3de142 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4847,6 +4847,31 @@ static void gen_slbmfev(DisasContext *ctx)
>                               cpu_gpr[rB(ctx->opcode)]);
>  #endif
>  }
> +
> +static void gen_slbfee_(DisasContext *ctx)
> +{
> +#if defined(CONFIG_USER_ONLY)
> +    gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> +#else
> +    TCGLabel *l1, *l2;
> +
> +    if (unlikely(ctx->pr)) {
> +        gen_inval_exception(ctx, POWERPC_EXCP_PRIV_REG);
> +        return;
> +    }
> +    gen_helper_find_slb_vsid(cpu_gpr[rS(ctx->opcode)], cpu_env,
> +                             cpu_gpr[rB(ctx->opcode)]);
> +    l1 = gen_new_label();
> +    l2 = gen_new_label();
> +    tcg_gen_trunc_tl_i32(cpu_crf[0], cpu_so);
> +    tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_gpr[rS(ctx->opcode)], -1, l1);
> +    tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], 1 << CRF_EQ);
> +    tcg_gen_br(l2);
> +    gen_set_label(l1);
> +    tcg_gen_movi_tl(cpu_gpr[rS(ctx->opcode)], 0);
> +    gen_set_label(l2);
> +#endif
> +}
>  #endif /* defined(TARGET_PPC64) */
> 
>  /***                      Lookaside buffer management                      
> ***/
> @@ -9972,6 +9997,7 @@ GEN_HANDLER2(mtsrin_64b, "mtsrin", 0x1F, 0x12, 0x07, 
> 0x001F0001,
>  GEN_HANDLER2(slbmte, "slbmte", 0x1F, 0x12, 0x0C, 0x001F0001, 
> PPC_SEGMENT_64B),
>  GEN_HANDLER2(slbmfee, "slbmfee", 0x1F, 0x13, 0x1C, 0x001F0001, 
> PPC_SEGMENT_64B),
>  GEN_HANDLER2(slbmfev, "slbmfev", 0x1F, 0x13, 0x1A, 0x001F0001, 
> PPC_SEGMENT_64B),
> +GEN_HANDLER2(slbfee_, "slbfee.", 0x1F, 0x13, 0x1E, 0x001F0000, 
> PPC_SEGMENT_64B),
>  #endif
>  GEN_HANDLER(tlbia, 0x1F, 0x12, 0x0B, 0x03FFFC01, PPC_MEM_TLBIA),
>  /* XXX Those instructions will need to be handled differently for
>