Re: [Qemu-ppc] [PATCH RFC] target-ppc: tlbie should have global effect

[Benjamin Herrenschmidt]

On Fri, 2016-09-09 at 09:53 +0530, Nikunj A Dadhania wrote:
> tlbie (and H_REMOVE for pseries) should have a global effect. This is
> achieved by iterating and setting tlb_need_flush in all the CPUs.
> 
> Suggested-by: Benjamin Herrenschmidt <address@hidden>
> Signed-off-by: Nikunj A Dadhania <address@hidden>
> 
> --
> 
> Note: Haven't changed following POWERPC_MMU_32B and POWERPC_MMU_601
> yet.
> As I am not sure about it.

604 and 7400 can do SMP.

That said, I think the approach in your patch is going to be a bit big
of a hammer. We should have a separate flag indicating that we need to
broadcast a flush and only set it on tlbie (non-l) and tlbivax (on
BookE) so that we don't end up doing expensive broadcasts on things
like context switches.

We keep the existing logic to flush locally. We additionally replace
the one in ptesync (BookS) or tlbsync (BookE) to test for the broadcast
flag, and flush the "other" CPUs if set.

That also means you have a nice spot to do the more complex MT-TCG
broadcast only when needed in the future.

Cheers,
Ben.

> ---
>  target-ppc/cpu.h         |  3 ++-
>  target-ppc/helper.h      |  1 +
>  target-ppc/helper_regs.h | 13 +++++++++----
>  target-ppc/mmu-hash64.c  | 16 ++++++++++------
>  target-ppc/mmu_helper.c  | 29 +++++++++++++++++++++++------
>  target-ppc/translate.c   |  2 +-
>  6 files changed, 46 insertions(+), 18 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1e808c8..59889f8 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1251,7 +1251,8 @@ void store_40x_sler (CPUPPCState *env, uint32_t
> val);
>  void store_booke_tcr (CPUPPCState *env, target_ulong val);
>  void store_booke_tsr (CPUPPCState *env, target_ulong val);
>  void ppc_tlb_invalidate_all (CPUPPCState *env);
> -void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
> +void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr,
> +                            unsigned int global);
>  void cpu_ppc_set_papr(PowerPCCPU *cpu);
>  #endif
>  #endif
> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
> index dcf3f95..79fb688 100644
> --- a/target-ppc/helper.h
> +++ b/target-ppc/helper.h
> @@ -561,6 +561,7 @@ DEF_HELPER_2(74xx_tlbd, void, env, tl)
>  DEF_HELPER_2(74xx_tlbi, void, env, tl)
>  DEF_HELPER_FLAGS_1(tlbia, TCG_CALL_NO_RWG, void, env)
>  DEF_HELPER_FLAGS_2(tlbie, TCG_CALL_NO_RWG, void, env, tl)
> +DEF_HELPER_FLAGS_2(tlbiel, TCG_CALL_NO_RWG, void, env, tl)
>  DEF_HELPER_FLAGS_2(tlbiva, TCG_CALL_NO_RWG, void, env, tl)
>  #if defined(TARGET_PPC64)
>  DEF_HELPER_FLAGS_3(store_slb, TCG_CALL_NO_RWG, void, env, tl, tl)
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 3d279f1..f3eb21d 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -156,10 +156,15 @@ static inline int hreg_store_msr(CPUPPCState
> *env, target_ulong value,
>  #if !defined(CONFIG_USER_ONLY)
>  static inline void check_tlb_flush(CPUPPCState *env)
>  {
> -    CPUState *cs = CPU(ppc_env_get_cpu(env));
> -    if (env->tlb_need_flush) {
> -        env->tlb_need_flush = 0;
> -        tlb_flush(cs, 1);
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> +        CPUPPCState *env = &cpu->env;
> +        if (env->tlb_need_flush) {
> +            env->tlb_need_flush = 0;
> +            tlb_flush(cs, 1);
> +        }
>      }
>  }
>  #else
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 8118143..a76c92b 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -907,12 +907,16 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>                                 target_ulong pte_index,
>                                 target_ulong pte0, target_ulong pte1)
>  {
> -    /*
> -     * XXX: given the fact that there are too many segments to
> -     * invalidate, and we still don't have a tlb_flush_mask(env, n,
> -     * mask) in QEMU, we just invalidate all TLBs
> -     */
> -    tlb_flush(CPU(cpu), 1);
> +    CPUState *cs;
> +
> +    CPU_FOREACH(cs) {
> +        /*
> +         * XXX: given the fact that there are too many segments to
> +         * invalidate, and we still don't have a tlb_flush_mask(env,
> n,
> +         * mask) in QEMU, we just invalidate all TLBs
> +         */
> +        tlb_flush(cs, 1);
> +    }
>  }
>  
>  void ppc_hash64_update_rmls(CPUPPCState *env)
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 696bb03..1923f1b 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1946,7 +1946,8 @@ void ppc_tlb_invalidate_all(CPUPPCState *env)
>      }
>  }
>  
> -void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr)
> +void ppc_tlb_invalidate_one(CPUPPCState *env, target_ulong addr,
> +                            unsigned int global)
>  {
>  #if !defined(FLUSH_ALL_TLBS)
>      addr &= TARGET_PAGE_MASK;
> @@ -1979,7 +1980,14 @@ void ppc_tlb_invalidate_one(CPUPPCState *env,
> target_ulong addr)
>           *      and we still don't have a tlb_flush_mask(env, n,
> mask) in QEMU,
>           *      we just invalidate all TLBs
>           */
> -        env->tlb_need_flush = 1;
> +        if (!global) {
> +            env->tlb_need_flush = 1;
> +        } else {
> +            CPUState *other_cs;
> +            CPU_FOREACH(other_cs) {
> +                env->tlb_need_flush = 1;
> +            }
> +        }
>          break;
>  #endif /* defined(TARGET_PPC64) */
>      default:
> @@ -2078,7 +2086,12 @@ void helper_tlbia(CPUPPCState *env)
>  
>  void helper_tlbie(CPUPPCState *env, target_ulong addr)
>  {
> -    ppc_tlb_invalidate_one(env, addr);
> +    ppc_tlb_invalidate_one(env, addr, 1);
> +}
> +
> +void helper_tlbiel(CPUPPCState *env, target_ulong addr)
> +{
> +    ppc_tlb_invalidate_one(env, addr, 0);
>  }
>  
>  void helper_tlbiva(CPUPPCState *env, target_ulong addr)
> @@ -2757,7 +2770,7 @@ static inline void
> booke206_invalidate_ea_tlb(CPUPPCState *env, int tlbn,
>  
>  void helper_booke206_tlbivax(CPUPPCState *env, target_ulong address)
>  {
> -    PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    CPUState *cs;
>  
>      if (address & 0x4) {
>          /* flush all entries */
> @@ -2774,11 +2787,15 @@ void helper_booke206_tlbivax(CPUPPCState
> *env, target_ulong address)
>      if (address & 0x8) {
>          /* flush TLB1 entries */
>          booke206_invalidate_ea_tlb(env, 1, address);
> -        tlb_flush(CPU(cpu), 1);
> +        CPU_FOREACH(cs) {
> +            tlb_flush(cs, 1);
> +        }
>      } else {
>          /* flush TLB0 entries */
>          booke206_invalidate_ea_tlb(env, 0, address);
> -        tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
> +        CPU_FOREACH(cs) {
> +            tlb_flush_page(cs, address & MAS2_EPN_MASK);
> +        }
>      }
>  }
>  
> diff --git a/target-ppc/translate.c b/target-ppc/translate.c
> index 618334a..86a8fb6 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4432,7 +4432,7 @@ static void gen_tlbiel(DisasContext *ctx)
>  #else
>      CHK_SV;
>  
> -    gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
> +    gen_helper_tlbiel(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>