Re: [Qemu-ppc] [Qemu-ppc for-2.10] [PATCH V3 2/5] target/ppc: Flush TLB on write to PIDR

[David Gibson]

On Wed, Apr 12, 2017 at 04:42:35PM +1000, Suraj Jitindar Singh wrote:
> On Thu, 2017-04-06 at 15:24 +1000, David Gibson wrote:
> > On Thu, Apr 06, 2017 at 10:37:42AM +1000, Suraj Jitindar Singh wrote:
> > > 
> > > On Thu, 2017-03-30 at 15:34 +1100, David Gibson wrote:
> > > > 
> > > > On Wed, Mar 29, 2017 at 04:43:46PM +1100, Suraj Jitindar Singh
> > > > wrote:
> > > > > 
> > > > > 
> > > > > The PIDR (process id register) is used to store the id of the
> > > > > currently
> > > > > running process, which is used to select the process table
> > > > > entry
> > > > > used to
> > > > > perform address translation. This means that when we write to
> > > > > this
> > > > > register
> > > > > all the translations in the TLB become outdated as they are for
> > > > > a
> > > > > previously running process. Thus when this register is written
> > > > > to
> > > > > we need
> > > > > to invalidate the TLB entries to ensure stale entries aren't
> > > > > used
> > > > > to
> > > > > to perform translation for the new process, which would result
> > > > > in
> > > > > at best
> > > > > segfaults or alternatively just random memory being accessed.
> > > > > 
> > > > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > > > > Reviewed-by: David Gibson <address@hidden>
> > > > > ---
> > > > >  target/ppc/helper.h         | 1 +
> > > > >  target/ppc/misc_helper.c    | 8 ++++++++
> > > > >  target/ppc/translate_init.c | 8 +++++++-
> > > > >  3 files changed, 16 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> > > > > index 6d77661..bb6a94a 100644
> > > > > --- a/target/ppc/helper.h
> > > > > +++ b/target/ppc/helper.h
> > > > > @@ -709,6 +709,7 @@ DEF_HELPER_FLAGS_1(load_601_rtcu,
> > > > > TCG_CALL_NO_RWG, tl, env)
> > > > >  DEF_HELPER_FLAGS_1(load_purr, TCG_CALL_NO_RWG, tl, env)
> > > > >  #endif
> > > > >  DEF_HELPER_2(store_sdr1, void, env, tl)
> > > > > +DEF_HELPER_2(store_pidr, void, env, tl)
> > > > >  DEF_HELPER_FLAGS_2(store_tbl, TCG_CALL_NO_RWG, void, env, tl)
> > > > >  DEF_HELPER_FLAGS_2(store_tbu, TCG_CALL_NO_RWG, void, env, tl)
> > > > >  DEF_HELPER_FLAGS_2(store_atbl, TCG_CALL_NO_RWG, void, env, tl)
> > > > > diff --git a/target/ppc/misc_helper.c
> > > > > b/target/ppc/misc_helper.c
> > > > > index fa573dd..0e42178 100644
> > > > > --- a/target/ppc/misc_helper.c
> > > > > +++ b/target/ppc/misc_helper.c
> > > > > @@ -88,6 +88,14 @@ void helper_store_sdr1(CPUPPCState *env,
> > > > > target_ulong val)
> > > > >      }
> > > > >  }
> > > > >  
> > > > > +void helper_store_pidr(CPUPPCState *env, target_ulong val)
> > > > > +{
> > > > > +    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> > > > > +
> > > > > +    env->spr[SPR_BOOKS_PID] = val;
> > > > > +    tlb_flush(CPU(cpu));
> > > > > +}
> > > > > +
> > > > >  void helper_store_hid0_601(CPUPPCState *env, target_ulong val)
> > > > >  {
> > > > >      target_ulong hid0;
> > > > > diff --git a/target/ppc/translate_init.c
> > > > > b/target/ppc/translate_init.c
> > > > > index aa0c44d..79f17a0 100644
> > > > > --- a/target/ppc/translate_init.c
> > > > > +++ b/target/ppc/translate_init.c
> > > > > @@ -394,6 +394,12 @@ static void spr_write_sdr1 (DisasContext
> > > > > *ctx,
> > > > > int sprn, int gprn)
> > > > >      gen_helper_store_sdr1(cpu_env, cpu_gpr[gprn]);
> > > > >  }
> > > > >  
> > > > > +/* PIDR */
> > > > > +static void spr_write_pidr(DisasContext *ctx, int sprn, int
> > > > > gprn)
> > > > > +{
> > > > > +    gen_helper_store_pidr(cpu_env, cpu_gpr[gprn]);
> > > > > +}
> > > > > +
> > > > >  /* 64 bits PowerPC specific SPRs */
> > > > >  #if defined(TARGET_PPC64)
> > > > >  static void spr_read_hior (DisasContext *ctx, int gprn, int
> > > > > sprn)
> > > > > @@ -8200,7 +8206,7 @@ static void
> > > > > gen_spr_power8_book4(CPUPPCState
> > > > > *env)
> > > > >                       KVM_REG_PPC_ACOP, 0);
> > > > >      spr_register_kvm(env, SPR_BOOKS_PID, "PID",
> > > > >                       SPR_NOACCESS, SPR_NOACCESS,
> > > > > -                     &spr_read_generic, &spr_write_generic,
> > > > > +                     &spr_read_generic, &spr_write_pidr,
> > > > >                       KVM_REG_PPC_PID, 0);
> > > > >      spr_register_kvm(env, SPR_WORT, "WORT",
> > > > >                       SPR_NOACCESS, SPR_NOACCESS,
> > > > Hrm.. I'm a bit confused.  I see PID being registered for both
> > > > POWER8
> > > > and POWER9, but I thought the PID register was new for POWER9
> > > > (for
> > > > BookS).
> > > > 
> > > Yeah so for POWER8 the PIDR only existed for POWER8E.
> > Ok.  While I'm glad you refactored to get rid of most of the explicit
> > model checks, this seems like an appropriate situation for one to add
> > the PIDR when necessary.
> 
> So previously we didn't have any checks and just generated the PIDR for
> any POWER8 processor. I could add a check for POWER8E though.

Yes, that's what I'm suggesting.

> 
> > 
> > > 
> > > But in the init function we don't differentiate between the various
> > > POWER8 processors.
> > But.. what does the PIDR *do* on the POWER8E?
> > 
> 
> Seems to be used in address translation. We never look at it in qemu so
> not sure if we ever actually used it.

Right, I'm pretty certain that whatever it is we don't implement it in
qemu.  I'm just kind of baffled by what it could do on real hardware,
since the pre-POWER9 BookS MMU doesn't really have any concept of PID.

There doesn't seem to be any mention of it in the v2.07 architecture.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature