Re: [Qemu-devel] [PATCH RFC v1 3/3] target-ppc: tlbie should have global effect

[Benjamin Herrenschmidt]

On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote:
> 
> +    env->tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
> check_tlb_flush(env th, 1);
> 

Hrm... how did that work bore ? IE. check_tlb_flush won't do anything
if tlb_need_flush is 0, isn't it already set elsewhere ? If not, that's a
bug in the existing bug that I'd suggest you fix separately. If it is, then
this only needs to OR-in TLB_NEED_GLOBAL_FLUSH and only if the hash entry
was found, it, at the point where tlb_need_flush was originally set

>      return r
> @@ -319,6 +320,8 @@ static target_ulong h_protect(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>      ppc_hash64_store_hpte(cpu, pte_index,
>                            (v & ~HPTE64_V_VALID) | H.PTE64_V_HPTE_DIRTY, 0);
>      ppc_hash64_tlb_flush_hpte(cpu, pte_index, v, r);
> +    /* Flush the tlb */
> +    check_tlb_flush(env, 1);
>      /* Don't need a memory barrier, due to qemu's global lock */
>      ppc_hash64_store_hpte(cpu, pte_index, v | HPTE64_V_HPTE_DIRTY, r);
>      return H_SUCCESS;
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 71111dc..50fe0f5 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1010,6 +1010,7 @@ struct CPUPPCState {
>      bool kvm_sw_tlb;  /* non-zero if KVM SW TLB API is active                
> */
>      uint32_t tlb_need_flush; /* Delayed flush needed */
>  #define TLB_NEED_LOCAL_FLUSH   0x1
> +#define TLB_NEED_GLOBAL_FLUSH  0x2
>  #endif
>  
>      /* Other registers */
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index fe20870..f18b2ff 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -154,13 +154,34 @@ static inline int hreg_store_msr(CPUPPCState *env, 
> target_ulong value,
>  }
>  
>  #if !defined(CONFIG_USER_ONLY)
> +static inline void tlb_clear_flag(CPUState *cs)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +
> +    env->tlb_need_flush = 0;
> +}
> +
>  static inline void check_tlb_flush(CPUPPCState *env, uint32_t global)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
> +    CPUState *other_cs;
> +
>      if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) == 
> TLB_NEED_LOCAL_FLUSH) {
> -        env->tlb_need_flush = 0;
>          tlb_flush(cs, 1);
>      }
> +
> +    if (global &&
> +        (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH) == 
> TLB_NEED_GLOBAL_FLUSH)
> +    {
> +        CPU_FOREACH(other_cs) {
> +            if (other_cs != cs) {
> +                tlb_clear_flag(other_cs);
> +                tlb_flush(other_cs, 1);
> +            }
> +        }
> +    }
> +    env->tlb_need_flush = 0;
>  }
>  #else
>  static inline void check_tlb_flush(CPUPPCState *env, uint32_t global) { }
> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 4c7ceef..f582efe 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -912,7 +912,7 @@ void ppc_hash64_tlb_flush_hpte(PowerPCCPU *cpu,
>       * 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);
> +    cpu->env.tlb_need_flush = TLB_NEED_GLOBAL_FLUSH | TLB_NEED_LOCAL_FLUSH;
>  }
>  
>  void ppc_hash64_update_rmls(CPUPPCState *env)
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 59dea8f..aee272f 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2757,7 +2757,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 +2774,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 5f12c41..d3cfbc8 100644
> --- a/target-ppc/translate.c
> +++ b/target-ppc/translate.c
> @@ -4443,6 +4443,7 @@ static void gen_tlbie(DisasContext *ctx)
>  #if defined(CONFIG_USER_ONLY)
>      GEN_PRIV;
>  #else
> +    TCGv_i32 t1;
>      CHK_HV;
>  
>      if (NARROW_MODE(ctx)) {
> @@ -4453,6 +4454,11 @@ static void gen_tlbie(DisasContext *ctx)
>      } else {
>          gen_helper_tlbie(cpu_env, cpu_gpr[rB(ctx->opcode)]);
>      }
> +    t1 = tcg_temp_new_i32();
> +    tcg_gen_ld_i32(t1, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
> +    tcg_gen_ori_i32(t1, t1, TLB_NEED_GLOBAL_FLUSH);
> +    tcg_gen_st_i32(t1, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
> +    tcg_temp_free_i32(t1);
>  #endif /* defined(CONFIG_USER_ONLY) */
>  }
>