Re: [EXTERNAL] [PATCH 2/2] target/ppc: Fix ISA v3.0 (POWER9) slbia implementation

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

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) */
> >  }
> >