| Message ID | 1465267828-10326-8-git-send-email-benh@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
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 <benh@kernel.crashing.org> > --- > 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 >
On Tue, 2016-07-05 at 19:23 +0200, Cédric Le Goater wrote: > > > 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 ? Yes the old patch was broken but I wrote (and already merged) a better one since then. Check upstream qemu :-) The -1 result is now handled in the JITed code to do the right thing (well, afaik). Cheers, Ben.
On 07/06/2016 12:10 AM, Benjamin Herrenschmidt wrote: > On Tue, 2016-07-05 at 19:23 +0200, Cédric Le Goater wrote: >> >> >> 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 ? > > Yes the old patch was broken but I wrote (and already merged) a better > one since then. Check upstream qemu :-) Yes much better :) I saw that when trying to apply the original powernv patchset on top of current qemu. > The -1 result is now handled in the JITed code to do the right thing > (well, afaik). well, no. It should be a 0 when the slb is not found, and thus no machine check. That is how I understand : The SLB is searched for an entry that matches the effective address specified by register RB. The search is performed as if it were being performed for purposes of address translation. That is, in order for a given entry to satisfy the search, the entry must be valid (V=1), (RB)0:63-s must equal SLBE[ESID0:63-s] (where 2s is the segment size selected by the B field in the entry), and RB39 must be equal to SLBTA. If exactly one matching entry is found, the contents of the B, VSID, Ks, Kp, N, L, C, TA, and LP fields of the entry are placed into register RT. If no matching entry is found, register RT is set to 0. If more than one matching entry is found, either one of the matching entries is used, as if it were the only matching entry, or a Machine Check occurs. If a Machine Check occurs, register RT, CR Field 0, and, in tags active mode, the FXCC are set to undefined values, and the description below of how this register and these fields are set does not apply. but do we care that much in qemu ? May be the machine check is better to have. Cheers, C.
On Wed, 2016-07-06 at 08:57 +0200, Cédric Le Goater wrote: > > > The -1 result is now handled in the JITed code to do the right > thing > > (well, afaik). > > well, no. It should be a 0 when the slb is not found, and thus no > machine check. That is how I understand : Right, which is afaik what the current qemu code does no ? The -1 isn't the function return, it's the pointer-argument return, which goes into rT. This is then handled in the generated code: 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); We clear CR (except so) tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_gpr[rS(ctx->opcode)], -1, l1); We branch to l1 if rT is -1 tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], 1 << CRF_EQ); We set EQ if we didn't branch tcg_gen_br(l2); Then go to l2 (skip the next bit) gen_set_label(l1); tcg_gen_movi_tl(cpu_gpr[rS(ctx->opcode)], 0); We clear rS if it was -1 gen_set_label(l2);
On 07/06/2016 09:24 AM, Benjamin Herrenschmidt wrote: > On Wed, 2016-07-06 at 08:57 +0200, Cédric Le Goater wrote: >> >>> The -1 result is now handled in the JITed code to do the right >> thing >>> (well, afaik). >> >> well, no. It should be a 0 when the slb is not found, and thus no >> machine check. That is how I understand : > > Right, which is afaik what the current qemu code does no ? > > The -1 isn't the function return, it's the pointer-argument > return, which goes into rT. ah yes. the : *rt = (target_ulong)-1ul; confused me. > This is then handled in the generated code: > > 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); > > We clear CR (except so) > > tcg_gen_brcondi_tl(TCG_COND_EQ, cpu_gpr[rS(ctx->opcode)], -1, l1); > > We branch to l1 if rT is -1 > > tcg_gen_ori_i32(cpu_crf[0], cpu_crf[0], 1 << CRF_EQ); > > We set EQ if we didn't branch > > tcg_gen_br(l2); > > Then go to l2 (skip the next bit) > > gen_set_label(l1); > tcg_gen_movi_tl(cpu_gpr[rS(ctx->opcode)], 0); > > We clear rS if it was -1 > > gen_set_label(l2); > Thanks for the details, sorry for the noise :/ Are there any OSes using it ? C.
On Wed, 2016-07-06 at 09:53 +0200, Cédric Le Goater wrote: > > Thanks for the details, sorry for the noise :/ > > Are there any OSes using it ? I initially found the problem with trying to run some AIX diag CD images that were floating around ;-) I think I re-found it at some point with some version of Darwin. Cheers, Ben.
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; + } 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
Used to lookup SLB entries by address, for some reason it was missing. Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- target-ppc/helper.h | 1 + target-ppc/mmu-hash64.c | 30 ++++++++++++++++++++++++++++++ target-ppc/translate.c | 26 ++++++++++++++++++++++++++ 3 files changed, 57 insertions(+)