Re: [Qemu-devel] [PATCH v3 2/3] target-ppc: add flag in chech_tlb_flush()

[Benjamin Herrenschmidt]

On Wed, 2016-09-14 at 13:09 +1000, David Gibson wrote:
> On Mon, Sep 12, 2016 at 11:18:33AM +0530, Nikunj A Dadhania wrote:
> > 
> > The flag will be used to indicate whether broadcast tlb flush is
> > needed
> > or not.
> > 
> > Moreover, BookS does both ptesync and tlbsync, so make that a nop
> > for
> > the server and tlbsync would generate a check flush for BookE
> 
> This commit message needs a bunch more detail.  The flag indicates
> whether a broadcast tlb flush is neede where exactly?  How does the
> information in the parameter differ from the information in the
> existing flags which check_tlb_flush() checks?

The flag to check_tlb_flush() indicates that any pending broadcast
needs to be performed/completed. IE. it's set to 1 as a result of
an instruction (or an H_CALL) defined to synchronize broadcast tlbie's

> > 
> > 
> > Signed-off-by: Nikunj A Dadhania <address@hidden>
> > ---
> >  hw/ppc/spapr_hcall.c     |  4 ++--
> >  target-ppc/excp_helper.c |  4 ++--
> >  target-ppc/helper.h      |  2 +-
> >  target-ppc/helper_regs.h |  4 ++--
> >  target-ppc/mmu_helper.c  |  4 ++--
> >  target-ppc/translate.c   | 20 ++++++++++----------
> >  6 files changed, 19 insertions(+), 19 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 73af112..ef12ea0 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu,
> > sPAPRMachineState *spapr,
> >  
> >      switch (ret) {
> >      case REMOVE_SUCCESS:
> > -        check_tlb_flush(env);
> > +        check_tlb_flush(env, 1);
> >          return H_SUCCESS;
> >  
> >      case REMOVE_NOT_FOUND:
> > @@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU
> > *cpu, sPAPRMachineState *spapr,
> >          }
> >      }
> >   exit:
> > -    check_tlb_flush(env);
> > +    check_tlb_flush(env, 1);
> >  
> >      return rc;
> >  }
> > diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> > index 04ed4da..3b78126 100644
> > --- a/target-ppc/excp_helper.c
> > +++ b/target-ppc/excp_helper.c
> > @@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU
> > *cpu, int excp_model, int excp)
> >      /* Any interrupt is context synchronizing, check if TCG TLB
> >       * needs a delayed flush on ppc64
> >       */
> > -    check_tlb_flush(env);
> > +    check_tlb_flush(env, 0);
> >  }
> >  
> >  void ppc_cpu_do_interrupt(CPUState *cs)
> > @@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env,
> > target_ulong nip, target_ulong msr)
> >      cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
> >  
> >      /* Context synchronizing: check if TCG TLB needs flush */
> > -    check_tlb_flush(env);
> > +    check_tlb_flush(env, 0);
> >  }
> >  
> >  void helper_rfi(CPUPPCState *env)
> > diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> > index e75d070..5ececf1 100644
> > --- a/target-ppc/helper.h
> > +++ b/target-ppc/helper.h
> > @@ -18,7 +18,7 @@ DEF_HELPER_1(rfid, void, env)
> >  DEF_HELPER_1(hrfid, void, env)
> >  DEF_HELPER_2(store_lpcr, void, env, tl)
> >  #endif
> > -DEF_HELPER_1(check_tlb_flush, void, env)
> > +DEF_HELPER_2(check_tlb_flush, void, env, i32)
> >  #endif
> >  
> >  DEF_HELPER_3(lmw, void, env, tl, i32)
> > diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> > index 69204a5..bcf65ce 100644
> > --- a/target-ppc/helper_regs.h
> > +++ b/target-ppc/helper_regs.h
> > @@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState
> > *env, target_ulong value,
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -static inline void check_tlb_flush(CPUPPCState *env)
> > +static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> > global)
> >  {
> >      CPUState *cs = CPU(ppc_env_get_cpu(env));
> >      if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
> > @@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState
> > *env)
> >      }
> >  }
> >  #else
> > -static inline void check_tlb_flush(CPUPPCState *env) { }
> > +static inline void check_tlb_flush(CPUPPCState *env, uint32_t
> > global) { }
> >  #endif
> >  
> >  #endif /* HELPER_REGS_H */
> > diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> > index d59d2f8..bf9f329 100644
> > --- a/target-ppc/mmu_helper.c
> > +++ b/target-ppc/mmu_helper.c
> > @@ -2867,9 +2867,9 @@ void helper_booke206_tlbflush(CPUPPCState
> > *env, target_ulong type)
> >  }
> >  
> >  
> > -void helper_check_tlb_flush(CPUPPCState *env)
> > +void helper_check_tlb_flush(CPUPPCState *env, unsigned int global)
> >  {
> > -    check_tlb_flush(env);
> > +    check_tlb_flush(env, global);
> >  }
> >  
> >  /*****************************************************************
> > ************/
> > diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> > index a27f455..5026804 100644
> > --- a/target-ppc/translate.c
> > +++ b/target-ppc/translate.c
> > @@ -3066,7 +3066,7 @@ static void gen_eieio(DisasContext *ctx)
> >  }
> >  
> >  #if !defined(CONFIG_USER_ONLY)
> > -static inline void gen_check_tlb_flush(DisasContext *ctx)
> > +static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t
> > global)
> >  {
> >      TCGv_i32 t;
> >      TCGLabel *l;
> > @@ -3078,12 +3078,13 @@ static inline void
> > gen_check_tlb_flush(DisasContext *ctx)
> >      t = tcg_temp_new_i32();
> >      tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState,
> > tlb_need_flush));
> >      tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
> > -    gen_helper_check_tlb_flush(cpu_env);
> > +    tcg_gen_movi_i32(t, global);
> > +    gen_helper_check_tlb_flush(cpu_env, t);
> >      gen_set_label(l);
> >      tcg_temp_free_i32(t);
> >  }
> >  #else
> > -static inline void gen_check_tlb_flush(DisasContext *ctx) { }
> > +static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t
> > global) { }
> >  #endif
> >  
> >  /* isync */
> > @@ -3094,7 +3095,7 @@ static void gen_isync(DisasContext *ctx)
> >       * kernel mode however so check MSR_PR
> >       */
> >      if (!ctx->pr) {
> > -        gen_check_tlb_flush(ctx);
> > +        gen_check_tlb_flush(ctx, 0);
> >      }
> >      gen_stop_exception(ctx);
> >  }
> > @@ -3259,7 +3260,7 @@ static void gen_sync(DisasContext *ctx)
> >       * check MSR_PR as well.
> >       */
> >      if (((l == 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) {
> > -        gen_check_tlb_flush(ctx);
> > +        gen_check_tlb_flush(ctx, 1);
> >      }
> >  }
> >  
> > @@ -4468,11 +4469,10 @@ static void gen_tlbsync(DisasContext *ctx)
> >  #else
> >      CHK_HV;
> >  
> > -    /* tlbsync is a nop for server, ptesync handles delayed tlb
> > flush,
> > -     * embedded however needs to deal with tlbsync. We don't try
> > to be
> > -     * fancy and swallow the overhead of checking for both.
> > -     */
> > -    gen_check_tlb_flush(ctx);
> > +    /* BookS does both ptesync and tlbsync make tlbsync a nop for
> > server */
> > +    if (ctx->insns_flags & PPC_BOOKE) {
> > +        gen_check_tlb_flush(ctx, 1);
> > +    }
> >  #endif /* defined(CONFIG_USER_ONLY) */
> >  }
> >  
>