| Message ID | 1473417926-14263-1-git-send-email-nikunj@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2016-09-09 at 16:15 +0530, Nikunj A Dadhania wrote: > > 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); > } 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. > } > }
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? > 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
Alex Bennée <alex.bennee@linaro.org> writes: > 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? Sure will do that. > >> 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. In the next patches I am adding TLB_NEED_GLOBAL_FLUSH, that is for broadcast flush for other cpus. TLB_NEED_LOCAL_FLUSH = 0x1 TLB_NEED_GLOBAL_FLUSH = 0x2 Regards Nikunj
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
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; } } } @@ -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 } }
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(-)