Re: [Qemu-ppc] [QEMU-ppc for-2.10][PATCH V5 2/6] target/ppc: Flush TLB on write to PIDR

[David Gibson]

On Tue, May 02, 2017 at 04:35:00PM +1000, Suraj Jitindar Singh wrote:
> On Tue, 2017-05-02 at 15:32 +1000, Suraj Jitindar Singh wrote:
> > On Tue, 2017-05-02 at 11:40 +1000, David Gibson wrote:
> > > 
> > > On Fri, Apr 28, 2017 at 04:58:22PM +1000, 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.
> 
> You've already merged this into ppc-for-2.10 David?

Not only that, but it's already gone upstream as 31b2b0f8.  Except for
the fix that belongs in the other patch, of course.

> 
> > > > 
> > > > Signed-off-by: Suraj Jitindar Singh <address@hidden>
> > > > Reviewed-by: David Gibson <address@hidden>
> > > > ---
> > > >  hw/ppc/spapr_hcall.c        | 2 +-
> > > >  target/ppc/helper.h         | 1 +
> > > >  target/ppc/misc_helper.c    | 8 ++++++++
> > > >  target/ppc/translate_init.c | 8 +++++++-
> > > >  4 files changed, 17 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > > > index 2e571cc..28794f7 100644
> > > > --- a/hw/ppc/spapr_hcall.c
> > > > +++ b/hw/ppc/spapr_hcall.c
> > > > @@ -1027,7 +1027,7 @@ static target_ulong
> > > > h_register_process_table(PowerPCCPU *cpu,
> > > >      /* Update the UPRT and GTSE bits in the LPCR for all cpus */
> > > >      ppc_set_spr_cpu_foreach(SPR_LPCR, LPCR_UPRT,
> > > >                              (flags & (FLAG_RADIX |
> > > > FLAG_HASH_PROC_TBL)) ?
> > > > -                            LPRC_UPRT : 0);
> > > > +                            LPCR_UPRT : 0);
> > > >      ppc_set_spr_cpu_foreach(SPR_LPCR, LPCR_GTSE, (flags &
> > > > FLAG_GTSE) ? LPCR_GTSE
> > > >                                                                  
> > > >   
> > > >    : 0);
> > > Uh.. this hunk appears to be a typo fix for the previous patch.
> > Yep, woops...
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > 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..57dc098 100644
> > > > --- a/target/ppc/translate_init.c
> > > > +++ b/target/ppc/translate_init.c
> > > > @@ -408,6 +408,12 @@ static void spr_write_hior (DisasContext
> > > > *ctx,
> > > > int sprn, int gprn)
> > > >      tcg_gen_st_tl(t0, cpu_env, offsetof(CPUPPCState,
> > > > excp_prefix));
> > > >      tcg_temp_free(t0);
> > > >  }
> > > > +
> > > > +/* PIDR */
> > > > +static void spr_write_pidr(DisasContext *ctx, int sprn, int
> > > > gprn)
> > > > +{
> > > > +    gen_helper_store_pidr(cpu_env, cpu_gpr[gprn]);
> > > > +}
> > > >  #endif
> > > >  #endif
> > > >  
> > > > @@ -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,
> 

-- 
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