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

[David Gibson]

On Mon, Sep 19, 2016 at 11:52:11AM +0530, Nikunj A Dadhania wrote:
> We flush the qemu TLB lazily. check_tlb_flush is called whenever we hit
> a context synchronizing event or instruction that requires a pending
> flush to be performed.
> 
> However, we fail to handle broadcast TLB flush operations. In order to
> fix that efficiently, we want to differenciate whether check_tlb_flush()
> needs to only apply pending local flushes (isync instructions,
> interrupts, ...) or also global pending flush operations. The latter is
> only needed when executing instructions that are defined architecturally
> as synchronizing global TLB flush operations. This in our case is
> ptesync on BookS and tlbsync on BookE along with the paravirtualized
> hypervisor calls.
> 
> Signed-off-by: Nikunj A Dadhania <address@hidden>

Sorry, there's still a couple of changes I'd like to this.

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

Please use a bool here..

>  {
>      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);

..which you can convert to from the i32 here.

Also.. your helper definition says i32, and I'm not sure that's
guaranteed to match unsigned int on all platforms.  I think you want
an explicit uint32_t here.


Or.. you could have separate tlb_flush_local() and tlb_flush_global helpers..

>  }
>  
>  
> /*****************************************************************************/
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index e747c1f..1ed5868 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -3022,7 +3022,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;
> @@ -3034,12 +3034,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);

.. which would save you a TCG op here.

> +    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 */
> @@ -3050,7 +3051,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);
>  }
> @@ -3230,7 +3231,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);
>      }
>  }
>  
> @@ -4439,11 +4440,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) */
>  }
>  

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