Re: [Qemu-ppc] [PATCH RFC 4/4] target-ppc: flush tlb from all the cpu

[Nikunj A Dadhania]

Benjamin Herrenschmidt <address@hidden> writes:

> On Sun, 2016-09-04 at 18:00 +0100, Alex Bennée wrote:
>
>> When is the synchronisation point? On ARM we end the basic block on
>> system instructions that mess with the cache. As a result the flush
>> is done as soon as we exit the run loop on the next instruction.
>
> Talking o this... Nikunj, I notice, all our TLB flushing is only ever
> done on the "current" CPU. I mean today, without MT-TCG. That looks
> broken already isn't it ?

Without MT-TCG, there was only one cpu, so I think we never hit that
issue.

>
> Looking at ARM, they do this:
>
> /* IS variants of TLB operations must affect all cores */
> static void tlbiall_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
>                              uint64_t value)
> {
>     CPUState *other_cs;
>
>     CPU_FOREACH(other_cs) {
>         tlb_flush(other_cs, 1);
>     }
> }
>
> I wonder if we should audit all tlb_flush() calls in target-ppc to
> do the right thing as well ?
>
> Something like the (untested, not even compiled as I have to run) patch
> below.
>
> Now to do things a bit better, we could split the check_tlb_flush() helper
> (or pass an argument) to tell it whether to check/flush other CPUs or not.
>
> All the slb operations and tlbiel only need to affect the current CPU, but
> broadcast tlbie's (and thus H_REMOVE) should have a global affect. We could
> add another flag to the env in addition to the tlb_need_flush, something like
> tlb_need_global_flush which is set on tlbie instructions to inform
> check_tlb_flush what to do.
>
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 0ee0e5a..f2302ec 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -959,8 +959,13 @@ static inline void do_rfi(CPUPPCState *env, target_ulong 
> nip, target_ulong msr)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
>
> -    /* MSR:POW cannot be set by any form of rfi */
> -    msr &= ~(1ULL << MSR_POW);
> +    /* These bits cannot be set by RFI on non-BookE systems and so must
> +     * be filtered out. 6xx and 7xxx with SW TLB management will put
> +     * TLB related junk in there among other things.
> +     */
> +    if (!(env->excp_model & POWERPC_EXCP_BOOKE)) {
> +            msr &= ~(target_ulong)0xf0000;
> +    }
>
>  #if defined(TARGET_PPC64)
>      /* Switching to 32-bit ? Crop the nip */
> @@ -990,7 +995,6 @@ void helper_rfi(CPUPPCState *env)
>      do_rfi(env, env->spr[SPR_SRR0], env->spr[SPR_SRR1] & 0xfffffffful);
>  }
>
> -#define MSR_BOOK3S_MASK
>  #if defined(TARGET_PPC64)
>  void helper_rfid(CPUPPCState *env)
>  {
> 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..1d84fc4 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -2758,6 +2758,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 *other_cs;
>
>      if (address & 0x4) {
>          /* flush all entries */
> @@ -2774,11 +2775,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(other_cs) {
> +            tlb_flush(other_cs, 1);
> +        }
>      } else {
>          /* flush TLB0 entries */
>          booke206_invalidate_ea_tlb(env, 0, address);
> -        tlb_flush_page(CPU(cpu), address & MAS2_EPN_MASK);
> +        CPU_FOREACH(other_cs) {
> +            tlb_flush_page(other_cs, address & MAS2_EPN_MASK);
> +        }
>      }
>  }

Sure, will have a round of testing.

Regards
Nikunj