Re: [Qemu-devel] [PATCH RFC v1 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag

[Benjamin Herrenschmidt]

On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote:
> > Signed-off-by: Nikunj A Dadhania <address@hidden>
> ---
>  target-ppc/cpu.h         | 1 +
>  target-ppc/helper_regs.h | 2 +-
>  target-ppc/mmu-hash64.c  | 4 ++--
>  target-ppc/mmu_helper.c  | 6 +++---
>  4 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 1e808c8..71111dc 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1009,6 +1009,7 @@ struct CPUPPCState {
>      bool tlb_dirty;   /* Set to non-zero when modifying TLB                  
> */
>      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
>  #endif
>  
>      /* Other registers */
> diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
> index 3d279f1..4457a30 100644
> --- a/target-ppc/helper_regs.h
> +++ b/target-ppc/helper_regs.h
> @@ -157,7 +157,7 @@ static inline int hreg_store_msr(CPUPPCState *env, 
> target_ulong value,
>  static inline void check_tlb_flush(CPUPPCState *env)
>  {
>      CPUState *cs = CPU(ppc_env_get_cpu(env));
> -    if (env->tlb_need_flush) {
> +    if ((env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) == 
> TLB_NEED_LOCAL_FLUSH) {
>          env->tlb_need_flush = 0;
>          tlb_flush(cs, 1);
>      }

No. This should be

        if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
                tlb_flush(cs, 1);
                env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
        }

> diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
> index 8118143..4c7ceef 100644
> --- a/target-ppc/mmu-hash64.c
> +++ b/target-ppc/mmu-hash64.c
> @@ -110,7 +110,7 @@ void helper_slbia(CPUPPCState *env)
>               *      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;
> +            env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
>          }
>      }
>  }

Should be

                env->tlb_need_flush |= TLB_NEED_LOCAL_FLUSH;

> @@ -132,7 +132,7 @@ void helper_slbie(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;
> +        env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
>      }
>  }

ditto.
 
> diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
> index 696bb03..2498888 100644
> --- a/target-ppc/mmu_helper.c
> +++ b/target-ppc/mmu_helper.c
> @@ -1965,7 +1965,7 @@ void ppc_tlb_invalidate_one(CPUPPCState *env, 
> target_ulong addr)
>           * we just mark the TLB to be flushed later (context synchronizing
>           * event or sync instruction on 32-bit).
>           */
> -        env->tlb_need_flush = 1;
> +        env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
>          break;

again.

>  #if defined(TARGET_PPC64)
>      case POWERPC_MMU_64B:
> @@ -1979,7 +1979,7 @@ 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;
> +        env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
>          break;

again.

>  #endif /* defined(TARGET_PPC64) */
>      default:
> @@ -2065,7 +2065,7 @@ void helper_store_sr(CPUPPCState *env, target_ulong 
> srnum, target_ulong value)
>              }
>          }
>  #else
> -        env->tlb_need_flush = 1;
> +        env->tlb_need_flush = TLB_NEED_LOCAL_FLUSH;
>  #endif

and one more.

>      }
>  }