| Message ID | 20200318044135.851716-2-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] target/ppc: Fix slbia TLB invalidation gap | expand |
On 3/18/20 5:41 AM, Nicholas Piggin wrote: > Linux using the hash MMU ("disable_radix" command line) on a POWER9 > machine quickly hits translation bugs due to using v3.0 slbia > features that are not implemented in TCG. Add them. I checked the ISA books and this looks OK but you are also modifying slbie. Thanks, C. > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > --- > target/ppc/helper.h | 2 +- > target/ppc/mmu-hash64.c | 57 ++++++++++++++++++++++++++++++++++++----- > target/ppc/translate.c | 5 +++- > 3 files changed, 55 insertions(+), 9 deletions(-) > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h > index ee1498050d..2dfa1c6942 100644 > --- a/target/ppc/helper.h > +++ b/target/ppc/helper.h > @@ -615,7 +615,7 @@ 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(slbia, TCG_CALL_NO_RWG, void, env, i32) > DEF_HELPER_FLAGS_2(slbie, TCG_CALL_NO_RWG, void, env, tl) > DEF_HELPER_FLAGS_2(slbieg, TCG_CALL_NO_RWG, void, env, tl) > #endif > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > index 373d44de74..deb1c13a66 100644 > --- a/target/ppc/mmu-hash64.c > +++ b/target/ppc/mmu-hash64.c > @@ -95,9 +95,10 @@ void dump_slb(PowerPCCPU *cpu) > } > } > > -void helper_slbia(CPUPPCState *env) > +void helper_slbia(CPUPPCState *env, uint32_t ih) > { > PowerPCCPU *cpu = env_archcpu(env); > + int starting_entry; > int n; > > /* > @@ -111,18 +112,59 @@ void helper_slbia(CPUPPCState *env) > * expected that slbmte is more common than slbia, and slbia is usually > * going to evict valid SLB entries, so that tradeoff is unlikely to be a > * good one. > + * > + * ISA v2.05 introduced IH field with values 0,1,2,6. These all invalidate > + * the same SLB entries (everything but entry 0), but differ in what > + * "lookaside information" is invalidated. TCG can ignore this and flush > + * everything. > + * > + * ISA v3.0 introduced additional values 3,4,7, which change what SLBs are > + * invalidated. > */ > > - /* XXX: Warning: slbia never invalidates the first segment */ > - for (n = 1; n < cpu->hash64_opts->slb_size; n++) { > - ppc_slb_t *slb = &env->slb[n]; > + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > + > + starting_entry = 1; /* default for IH=0,1,2,6 */ > + > + if (env->mmu_model == POWERPC_MMU_3_00) { > + switch (ih) { > + case 0x7: > + /* invalidate no SLBs, but all lookaside information */ > + return; > > - if (slb->esid & SLB_ESID_V) { > - slb->esid &= ~SLB_ESID_V; > + case 0x3: > + case 0x4: > + /* also considers SLB entry 0 */ > + starting_entry = 0; > + break; > + > + case 0x5: > + /* treat undefined values as ih==0, and warn */ > + qemu_log_mask(LOG_GUEST_ERROR, > + "slbia undefined IH field %u.\n", ih); > + break; > + > + default: > + /* 0,1,2,6 */ > + break; > } > } > > - env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > + for (n = starting_entry; n < cpu->hash64_opts->slb_size; n++) { > + ppc_slb_t *slb = &env->slb[n]; > + > + if (!(slb->esid & SLB_ESID_V)) { > + continue; > + } > + if (env->mmu_model == POWERPC_MMU_3_00) { > + if (ih == 0x3 && (slb->vsid & SLB_VSID_C) == 0) { > + /* preserves entries with a class value of 0 */ > + continue; > + } > + } > + > + slb->esid &= ~SLB_ESID_V; > + } > } > > static void __helper_slbie(CPUPPCState *env, target_ulong addr, > @@ -136,6 +178,7 @@ static void __helper_slbie(CPUPPCState *env, target_ulong addr, > return; > } > > + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > if (slb->esid & SLB_ESID_V) { > slb->esid &= ~SLB_ESID_V; > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index eb0ddba850..e514732a09 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -5027,12 +5027,15 @@ static void gen_tlbsync(DisasContext *ctx) > /* slbia */ > static void gen_slbia(DisasContext *ctx) > { > + uint32_t ih = (ctx->opcode >> 21) & 0x7; > + TCGv_i32 t0 = tcg_const_i32(ih); > + > #if defined(CONFIG_USER_ONLY) > GEN_PRIV; > #else > CHK_SV; > > - gen_helper_slbia(cpu_env); > + gen_helper_slbia(cpu_env, t0); > #endif /* defined(CONFIG_USER_ONLY) */ > } >
On Wed, 2020-03-18 at 18:08 +0100, Cédric Le Goater wrote: > On 3/18/20 5:41 AM, Nicholas Piggin wrote: > > Linux using the hash MMU ("disable_radix" command line) on a POWER9 > > machine quickly hits translation bugs due to using v3.0 slbia > > features that are not implemented in TCG. Add them. > > I checked the ISA books and this looks OK but you are also modifying > slbie. For the same reason, I believe slbie needs to invalidate caches even if the entry isn't present. The kernel will under some circumstances overwrite SLB entries without invalidating (because the translation itself isn't invalid, it's just that the SLB is full, so anything cached in the ERAT is still technically ok). However, when those things get really invalidated, they need to be taken out, even if they no longer have a corresponding SLB entry. Cheers, Ben. > Thanks, > > C. > > > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> > > --- > > target/ppc/helper.h | 2 +- > > target/ppc/mmu-hash64.c | 57 ++++++++++++++++++++++++++++++++++++- > > ---- > > target/ppc/translate.c | 5 +++- > > 3 files changed, 55 insertions(+), 9 deletions(-) > > > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h > > index ee1498050d..2dfa1c6942 100644 > > --- a/target/ppc/helper.h > > +++ b/target/ppc/helper.h > > @@ -615,7 +615,7 @@ 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(slbia, TCG_CALL_NO_RWG, void, env, i32) > > DEF_HELPER_FLAGS_2(slbie, TCG_CALL_NO_RWG, void, env, tl) > > DEF_HELPER_FLAGS_2(slbieg, TCG_CALL_NO_RWG, void, env, tl) > > #endif > > diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c > > index 373d44de74..deb1c13a66 100644 > > --- a/target/ppc/mmu-hash64.c > > +++ b/target/ppc/mmu-hash64.c > > @@ -95,9 +95,10 @@ void dump_slb(PowerPCCPU *cpu) > > } > > } > > > > -void helper_slbia(CPUPPCState *env) > > +void helper_slbia(CPUPPCState *env, uint32_t ih) > > { > > PowerPCCPU *cpu = env_archcpu(env); > > + int starting_entry; > > int n; > > > > /* > > @@ -111,18 +112,59 @@ void helper_slbia(CPUPPCState *env) > > * expected that slbmte is more common than slbia, and slbia > > is usually > > * going to evict valid SLB entries, so that tradeoff is > > unlikely to be a > > * good one. > > + * > > + * ISA v2.05 introduced IH field with values 0,1,2,6. These > > all invalidate > > + * the same SLB entries (everything but entry 0), but differ > > in what > > + * "lookaside information" is invalidated. TCG can ignore this > > and flush > > + * everything. > > + * > > + * ISA v3.0 introduced additional values 3,4,7, which change > > what SLBs are > > + * invalidated. > > */ > > > > - /* XXX: Warning: slbia never invalidates the first segment */ > > - for (n = 1; n < cpu->hash64_opts->slb_size; n++) { > > - ppc_slb_t *slb = &env->slb[n]; > > + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > > + > > + starting_entry = 1; /* default for IH=0,1,2,6 */ > > + > > + if (env->mmu_model == POWERPC_MMU_3_00) { > > + switch (ih) { > > + case 0x7: > > + /* invalidate no SLBs, but all lookaside information > > */ > > + return; > > > > - if (slb->esid & SLB_ESID_V) { > > - slb->esid &= ~SLB_ESID_V; > > + case 0x3: > > + case 0x4: > > + /* also considers SLB entry 0 */ > > + starting_entry = 0; > > + break; > > + > > + case 0x5: > > + /* treat undefined values as ih==0, and warn */ > > + qemu_log_mask(LOG_GUEST_ERROR, > > + "slbia undefined IH field %u.\n", ih); > > + break; > > + > > + default: > > + /* 0,1,2,6 */ > > + break; > > } > > } > > > > - env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > > + for (n = starting_entry; n < cpu->hash64_opts->slb_size; n++) > > { > > + ppc_slb_t *slb = &env->slb[n]; > > + > > + if (!(slb->esid & SLB_ESID_V)) { > > + continue; > > + } > > + if (env->mmu_model == POWERPC_MMU_3_00) { > > + if (ih == 0x3 && (slb->vsid & SLB_VSID_C) == 0) { > > + /* preserves entries with a class value of 0 */ > > + continue; > > + } > > + } > > + > > + slb->esid &= ~SLB_ESID_V; > > + } > > } > > > > static void __helper_slbie(CPUPPCState *env, target_ulong addr, > > @@ -136,6 +178,7 @@ static void __helper_slbie(CPUPPCState *env, > > target_ulong addr, > > return; > > } > > > > + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; > > if (slb->esid & SLB_ESID_V) { > > slb->esid &= ~SLB_ESID_V; > > > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > > index eb0ddba850..e514732a09 100644 > > --- a/target/ppc/translate.c > > +++ b/target/ppc/translate.c > > @@ -5027,12 +5027,15 @@ static void gen_tlbsync(DisasContext *ctx) > > /* slbia */ > > static void gen_slbia(DisasContext *ctx) > > { > > + uint32_t ih = (ctx->opcode >> 21) & 0x7; > > + TCGv_i32 t0 = tcg_const_i32(ih); > > + > > #if defined(CONFIG_USER_ONLY) > > GEN_PRIV; > > #else > > CHK_SV; > > > > - gen_helper_slbia(cpu_env); > > + gen_helper_slbia(cpu_env, t0); > > #endif /* defined(CONFIG_USER_ONLY) */ > > } > >
Benjamin Herrenschmidt's on March 19, 2020 6:46 am: > On Wed, 2020-03-18 at 18:08 +0100, Cédric Le Goater wrote: >> On 3/18/20 5:41 AM, Nicholas Piggin wrote: >> > Linux using the hash MMU ("disable_radix" command line) on a POWER9 >> > machine quickly hits translation bugs due to using v3.0 slbia >> > features that are not implemented in TCG. Add them. >> >> I checked the ISA books and this looks OK but you are also modifying >> slbie. That was a mistake that leaked in from debugging the crashes. > For the same reason, I believe slbie needs to invalidate caches even if > the entry isn't present. I don't think it does per the ISA. If we overwrite it then we can only invalidate with slbia. That's why there is that slb insertion cache for pre-POWER9 that lets us use slbies to context switch so long as none have been overwritten. > The kernel will under some circumstances overwrite SLB entries without > invalidating (because the translation itself isn't invalid, it's just > that the SLB is full, so anything cached in the ERAT is still > technically ok). > > However, when those things get really invalidated, they need to be > taken out, even if they no longer have a corresponding SLB entry. Yeah we track that and do slbia in that case. Thanks, Nick
On Thu, Mar 19, 2020 at 07:46:54AM +1100, Benjamin Herrenschmidt wrote: > On Wed, 2020-03-18 at 18:08 +0100, Cédric Le Goater wrote: > > On 3/18/20 5:41 AM, Nicholas Piggin wrote: > > > Linux using the hash MMU ("disable_radix" command line) on a POWER9 > > > machine quickly hits translation bugs due to using v3.0 slbia > > > features that are not implemented in TCG. Add them. > > > > I checked the ISA books and this looks OK but you are also modifying > > slbie. > > For the same reason, I believe slbie needs to invalidate caches even if > the entry isn't present. > > The kernel will under some circumstances overwrite SLB entries without > invalidating (because the translation itself isn't invalid, it's just > that the SLB is full, so anything cached in the ERAT is still > technically ok). > > However, when those things get really invalidated, they need to be > taken out, even if they no longer have a corresponding SLB entry. Right, the slbie change is certainly correct, but it doesn't match what the commit message says this is doing. Nick, can you split that out please.
diff --git a/target/ppc/helper.h b/target/ppc/helper.h index ee1498050d..2dfa1c6942 100644 --- a/target/ppc/helper.h +++ b/target/ppc/helper.h @@ -615,7 +615,7 @@ 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(slbia, TCG_CALL_NO_RWG, void, env, i32) DEF_HELPER_FLAGS_2(slbie, TCG_CALL_NO_RWG, void, env, tl) DEF_HELPER_FLAGS_2(slbieg, TCG_CALL_NO_RWG, void, env, tl) #endif diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c index 373d44de74..deb1c13a66 100644 --- a/target/ppc/mmu-hash64.c +++ b/target/ppc/mmu-hash64.c @@ -95,9 +95,10 @@ void dump_slb(PowerPCCPU *cpu) } } -void helper_slbia(CPUPPCState *env) +void helper_slbia(CPUPPCState *env, uint32_t ih) { PowerPCCPU *cpu = env_archcpu(env); + int starting_entry; int n; /* @@ -111,18 +112,59 @@ void helper_slbia(CPUPPCState *env) * expected that slbmte is more common than slbia, and slbia is usually * going to evict valid SLB entries, so that tradeoff is unlikely to be a * good one. + * + * ISA v2.05 introduced IH field with values 0,1,2,6. These all invalidate + * the same SLB entries (everything but entry 0), but differ in what + * "lookaside information" is invalidated. TCG can ignore this and flush + * everything. + * + * ISA v3.0 introduced additional values 3,4,7, which change what SLBs are + * invalidated. */ - /* XXX: Warning: slbia never invalidates the first segment */ - for (n = 1; n < cpu->hash64_opts->slb_size; n++) { - ppc_slb_t *slb = &env->slb[n]; + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; + + starting_entry = 1; /* default for IH=0,1,2,6 */ + + if (env->mmu_model == POWERPC_MMU_3_00) { + switch (ih) { + case 0x7: + /* invalidate no SLBs, but all lookaside information */ + return; - if (slb->esid & SLB_ESID_V) { - slb->esid &= ~SLB_ESID_V; + case 0x3: + case 0x4: + /* also considers SLB entry 0 */ + starting_entry = 0; + break; + + case 0x5: + /* treat undefined values as ih==0, and warn */ + qemu_log_mask(LOG_GUEST_ERROR, + "slbia undefined IH field %u.\n", ih); + break; + + default: + /* 0,1,2,6 */ + break; } } - env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; + for (n = starting_entry; n < cpu->hash64_opts->slb_size; n++) { + ppc_slb_t *slb = &env->slb[n]; + + if (!(slb->esid & SLB_ESID_V)) { + continue; + } + if (env->mmu_model == POWERPC_MMU_3_00) { + if (ih == 0x3 && (slb->vsid & SLB_VSID_C) == 0) { + /* preserves entries with a class value of 0 */ + continue; + } + } + + slb->esid &= ~SLB_ESID_V; + } } static void __helper_slbie(CPUPPCState *env, target_ulong addr, @@ -136,6 +178,7 @@ static void __helper_slbie(CPUPPCState *env, target_ulong addr, return; } + env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH; if (slb->esid & SLB_ESID_V) { slb->esid &= ~SLB_ESID_V; diff --git a/target/ppc/translate.c b/target/ppc/translate.c index eb0ddba850..e514732a09 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -5027,12 +5027,15 @@ static void gen_tlbsync(DisasContext *ctx) /* slbia */ static void gen_slbia(DisasContext *ctx) { + uint32_t ih = (ctx->opcode >> 21) & 0x7; + TCGv_i32 t0 = tcg_const_i32(ih); + #if defined(CONFIG_USER_ONLY) GEN_PRIV; #else CHK_SV; - gen_helper_slbia(cpu_env); + gen_helper_slbia(cpu_env, t0); #endif /* defined(CONFIG_USER_ONLY) */ }
Linux using the hash MMU ("disable_radix" command line) on a POWER9 machine quickly hits translation bugs due to using v3.0 slbia features that are not implemented in TCG. Add them. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- target/ppc/helper.h | 2 +- target/ppc/mmu-hash64.c | 57 ++++++++++++++++++++++++++++++++++++----- target/ppc/translate.c | 5 +++- 3 files changed, 55 insertions(+), 9 deletions(-)