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

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2016-09-09 at 15:07 +0100, Alex Bennée wrote:
> Nikunj A Dadhania <nikunj@linux.vnet.ibm.com> writes:
> 
> I think we need a little more detail here. In fact when you post the
> next version of the series could you please include a cover letter to
> cover what the series is trying to achieve?

In the meantime, for the readers, this is about fixing a problem
on TCG today (without MT-TCG) where we fail to properly propagate
TLB invalidations to other CPUs when we should (when the guest uses
boradcast TLB invalidation instructions).

The implementation also provides some ground work to make it easier to
plumb in the necessary MT-TCG additions.

> 
> > 
> > Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> > ---
> >  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);
> >      }
> > 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;
> 
> I'm not sure what we gain here versus just using a straight bool for
> the flag.
> 
> > 
> >          }
> >      }
> >  }
> > @@ -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;
> >      }
> >  }
> > 
> > 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;
> >  #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;
> >  #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
> >      }
> >  }
> 
> 
> --
> Alex Bennée