[Qemu-devel] [PATCH v6 0/3] ppc: handle broadcast tlb flush

[Qemu-devel] [PATCH v6 0/3] ppc: handle broadcast tlb flush

[Nikunj A Dadhania]

PowerPC failed to handle broadcast TLB flush operations. Executing 
instructions that are defined architecturally as synchronizing global TLB 
should have a global effect.

* tlbie on BookS
* tlbivax on BookE
* H_CALLs (H_REMOVE, H_BULK_REMOVE and H_PROTECT) in case of pseries, 
  since they contain a tlbie on a real hypervisor.

The implementation provides a single point that can be used in MTTCG for 
doing async-flushes.

The patchset introduces bit-flags in CPUPPCState::tlb_need_flush:

  TLB_NEED_LOCAL_FLUSH (0x1) - Flush local tlb
  TLB_NEED_GLOBAL_FLUSH (0x2) - Flush tlb on other cpus.

Note: Tested patches on commit ebc231d(Merge remote-tracking branch 
'remotes/awilliam/tags/vfio-fixes-20160915.0' into staging), as ppc64 
system emulation is broken after that.

Changelog
v5:
* Added check_tlb_flush_[local, global] helpers
* Changed the variable type to bool

v4: 
* Updated commit message for patch 1/3

v3:
* Updated commit messages and cover letter(benh)


Nikunj A Dadhania (3):
  target-ppc: add TLB_NEED_LOCAL_FLUSH flag
  target-ppc: add flag in chech_tlb_flush()
  target-ppc: tlbie/tlbivax should have global effect

 hw/ppc/spapr_hcall.c     |  6 ++++--
 target-ppc/cpu.h         |  2 ++
 target-ppc/excp_helper.c |  4 ++--
 target-ppc/helper.h      |  3 ++-
 target-ppc/helper_regs.h | 25 +++++++++++++++++++++----
 target-ppc/mmu-hash64.c  |  6 +++---
 target-ppc/mmu_helper.c  | 25 +++++++++++++++++--------
 target-ppc/translate.c   | 29 +++++++++++++++++++----------
 8 files changed, 70 insertions(+), 30 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v6 1/3] target-ppc: add TLB_NEED_LOCAL_FLUSH flag

[Nikunj A Dadhania]

Introduces bit-flag in CPUPPCState::tlb_need_flush:

  TLB_NEED_LOCAL_FLUSH (0x1) - Flush local tlb

This would indicate a pending local tlb flush (isync instructions,
interrupts, ...)

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 target-ppc/cpu.h         | 1 +
 target-ppc/helper_regs.h | 4 ++--
 target-ppc/mmu-hash64.c  | 4 ++--
 target-ppc/mmu_helper.c  | 6 +++---
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 9617481..96d2def 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..69204a5 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -157,9 +157,9 @@ 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) {
-        env->tlb_need_flush = 0;
+    if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
         tlb_flush(cs, 1);
+        env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
     }
 }
 #else
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 8118143..1f52b64 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..d59d2f8 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
     }
 }
-- 
2.7.4

[Qemu-devel] [PATCH v6 2/3] target-ppc: add flag in chech_tlb_flush()

[Nikunj A Dadhania]

We flush the qemu TLB lazily. check_tlb_flush is called whenever we hit
a context synchronizing event or instruction that requires a pending
flush to be performed.

However, we fail to handle broadcast TLB flush operations. In order to
fix that efficiently, we want to differenciate whether check_tlb_flush()
needs to only apply pending local flushes (isync instructions,
interrupts, ...) or also global pending flush operations. The latter is
only needed when executing instructions that are defined architecturally
as synchronizing global TLB flush operations. This in our case is
ptesync on BookS and tlbsync on BookE along with the paravirtualized
hypervisor calls.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/ppc/spapr_hcall.c     |  4 ++--
 target-ppc/excp_helper.c |  4 ++--
 target-ppc/helper.h      |  3 ++-
 target-ppc/helper_regs.h |  4 ++--
 target-ppc/mmu_helper.c  |  9 +++++++--
 target-ppc/translate.c   | 23 +++++++++++++----------
 6 files changed, 28 insertions(+), 19 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 73af112..0884e3e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -201,7 +201,7 @@ static target_ulong h_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
 
     switch (ret) {
     case REMOVE_SUCCESS:
-        check_tlb_flush(env);
+        check_tlb_flush(env, true);
         return H_SUCCESS;
 
     case REMOVE_NOT_FOUND:
@@ -282,7 +282,7 @@ static target_ulong h_bulk_remove(PowerPCCPU *cpu, sPAPRMachineState *spapr,
         }
     }
  exit:
-    check_tlb_flush(env);
+    check_tlb_flush(env, true);
 
     return rc;
 }
diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
index 04ed4da..921c39d 100644
--- a/target-ppc/excp_helper.c
+++ b/target-ppc/excp_helper.c
@@ -711,7 +711,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int excp_model, int excp)
     /* Any interrupt is context synchronizing, check if TCG TLB
      * needs a delayed flush on ppc64
      */
-    check_tlb_flush(env);
+    check_tlb_flush(env, false);
 }
 
 void ppc_cpu_do_interrupt(CPUState *cs)
@@ -973,7 +973,7 @@ static inline void do_rfi(CPUPPCState *env, target_ulong nip, target_ulong msr)
     cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
 
     /* Context synchronizing: check if TCG TLB needs flush */
-    check_tlb_flush(env);
+    check_tlb_flush(env, false);
 }
 
 void helper_rfi(CPUPPCState *env)
diff --git a/target-ppc/helper.h b/target-ppc/helper.h
index e75d070..9641e5e 100644
--- a/target-ppc/helper.h
+++ b/target-ppc/helper.h
@@ -18,7 +18,8 @@ DEF_HELPER_1(rfid, void, env)
 DEF_HELPER_1(hrfid, void, env)
 DEF_HELPER_2(store_lpcr, void, env, tl)
 #endif
-DEF_HELPER_1(check_tlb_flush, void, env)
+DEF_HELPER_1(check_tlb_flush_local, void, env)
+DEF_HELPER_1(check_tlb_flush_global, void, env)
 #endif
 
 DEF_HELPER_3(lmw, void, env, tl, i32)
diff --git a/target-ppc/helper_regs.h b/target-ppc/helper_regs.h
index 69204a5..dd85fc5 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -154,7 +154,7 @@ static inline int hreg_store_msr(CPUPPCState *env, target_ulong value,
 }
 
 #if !defined(CONFIG_USER_ONLY)
-static inline void check_tlb_flush(CPUPPCState *env)
+static inline void check_tlb_flush(CPUPPCState *env, bool global)
 {
     CPUState *cs = CPU(ppc_env_get_cpu(env));
     if (env->tlb_need_flush & TLB_NEED_LOCAL_FLUSH) {
@@ -163,7 +163,7 @@ static inline void check_tlb_flush(CPUPPCState *env)
     }
 }
 #else
-static inline void check_tlb_flush(CPUPPCState *env) { }
+static inline void check_tlb_flush(CPUPPCState *env, bool global) { }
 #endif
 
 #endif /* HELPER_REGS_H */
diff --git a/target-ppc/mmu_helper.c b/target-ppc/mmu_helper.c
index d59d2f8..0124150 100644
--- a/target-ppc/mmu_helper.c
+++ b/target-ppc/mmu_helper.c
@@ -2867,9 +2867,14 @@ void helper_booke206_tlbflush(CPUPPCState *env, target_ulong type)
 }
 
 
-void helper_check_tlb_flush(CPUPPCState *env)
+void helper_check_tlb_flush_local(CPUPPCState *env)
 {
-    check_tlb_flush(env);
+    check_tlb_flush(env, false);
+}
+
+void helper_check_tlb_flush_global(CPUPPCState *env)
+{
+    check_tlb_flush(env, true);
 }
 
 /*****************************************************************************/
diff --git a/target-ppc/translate.c b/target-ppc/translate.c
index e747c1f..df63b4f 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -3022,7 +3022,7 @@ static void gen_eieio(DisasContext *ctx)
 }
 
 #if !defined(CONFIG_USER_ONLY)
-static inline void gen_check_tlb_flush(DisasContext *ctx)
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global)
 {
     TCGv_i32 t;
     TCGLabel *l;
@@ -3034,12 +3034,16 @@ static inline void gen_check_tlb_flush(DisasContext *ctx)
     t = tcg_temp_new_i32();
     tcg_gen_ld_i32(t, cpu_env, offsetof(CPUPPCState, tlb_need_flush));
     tcg_gen_brcondi_i32(TCG_COND_EQ, t, 0, l);
-    gen_helper_check_tlb_flush(cpu_env);
+    if (global) {
+        gen_helper_check_tlb_flush_global(cpu_env);
+    } else {
+        gen_helper_check_tlb_flush_local(cpu_env);
+    }
     gen_set_label(l);
     tcg_temp_free_i32(t);
 }
 #else
-static inline void gen_check_tlb_flush(DisasContext *ctx) { }
+static inline void gen_check_tlb_flush(DisasContext *ctx, uint32_t global) { }
 #endif
 
 /* isync */
@@ -3050,7 +3054,7 @@ static void gen_isync(DisasContext *ctx)
      * kernel mode however so check MSR_PR
      */
     if (!ctx->pr) {
-        gen_check_tlb_flush(ctx);
+        gen_check_tlb_flush(ctx, 0);
     }
     gen_stop_exception(ctx);
 }
@@ -3230,7 +3234,7 @@ static void gen_sync(DisasContext *ctx)
      * check MSR_PR as well.
      */
     if (((l == 2) || !(ctx->insns_flags & PPC_64B)) && !ctx->pr) {
-        gen_check_tlb_flush(ctx);
+        gen_check_tlb_flush(ctx, 1);
     }
 }
 
@@ -4439,11 +4443,10 @@ static void gen_tlbsync(DisasContext *ctx)
 #else
     CHK_HV;
 
-    /* tlbsync is a nop for server, ptesync handles delayed tlb flush,
-     * embedded however needs to deal with tlbsync. We don't try to be
-     * fancy and swallow the overhead of checking for both.
-     */
-    gen_check_tlb_flush(ctx);
+    /* BookS does both ptesync and tlbsync make tlbsync a nop for server */
+    if (ctx->insns_flags & PPC_BOOKE) {
+        gen_check_tlb_flush(ctx, 1);
+    }
 #endif /* defined(CONFIG_USER_ONLY) */
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v6 3/3] target-ppc: tlbie/tlbivax should have global effect

[Nikunj A Dadhania]

tlbie (BookS) and tlbivax (BookE) plus the H_CALLs(pseries) should have
a global effect.

Introduces TLB_NEED_GLOBAL_FLUSH flag. During lazy tlb flush, after
taking care of pending local flushes, check broadcast flush(at context
synchronizing event ptesync/tlbsync, etc) is needed. Depending on the
bitmask state of the tlb_need_flush, tlb is flushed from other cpus if
needed and the flags are cleared.

Suggested-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_hcall.c     |  2 ++
 target-ppc/cpu.h         |  1 +
 target-ppc/helper_regs.h | 17 +++++++++++++++++
 target-ppc/mmu-hash64.c  |  2 +-
 target-ppc/mmu_helper.c  | 10 +++++++---
 target-ppc/translate.c   |  6 ++++++
 6 files changed, 34 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 0884e3e..c56a9de 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -319,6 +319,8 @@ static target_ulong h_protect(PowerPCCPU *cpu, sPAPRMachineState *spapr,
     ppc_hash64_store_hpte(cpu, pte_index,
                           (v & ~HPTE64_V_VALID) | HPTE64_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 96d2def..1c90adb 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 dd85fc5..bb9ce60 100644
--- a/target-ppc/helper_regs.h
+++ b/target-ppc/helper_regs.h
@@ -161,6 +161,23 @@ static inline void check_tlb_flush(CPUPPCState *env, bool global)
         tlb_flush(cs, 1);
         env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
     }
+
+    /* Propagate TLB invalidations to other CPUs when the guest uses broadcast
+     * TLB invalidation instructions.
+     */
+    if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
+        CPUState *other_cs;
+        CPU_FOREACH(other_cs) {
+            if (other_cs != cs) {
+                PowerPCCPU *cpu = POWERPC_CPU(other_cs);
+                CPUPPCState *other_env = &cpu->env;
+
+                other_env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
+                tlb_flush(other_cs, 1);
+            }
+        }
+        env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
+    }
 }
 #else
 static inline void check_tlb_flush(CPUPPCState *env, bool global) { }
diff --git a/target-ppc/mmu-hash64.c b/target-ppc/mmu-hash64.c
index 1f52b64..fdb7a78 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 0124150..d09fc0a 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 df63b4f..bfadb3b 100644
--- a/target-ppc/translate.c
+++ b/target-ppc/translate.c
@@ -4422,6 +4422,7 @@ static void gen_tlbie(DisasContext *ctx)
 #if defined(CONFIG_USER_ONLY)
     GEN_PRIV;
 #else
+    TCGv_i32 t1;
     CHK_HV;
 
     if (NARROW_MODE(ctx)) {
@@ -4432,6 +4433,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) */
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v6 0/3] ppc: handle broadcast tlb flush

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1186 bytes --]

On Tue, Sep 20, 2016 at 10:04:58PM +0530, Nikunj A Dadhania wrote:
> PowerPC failed to handle broadcast TLB flush operations. Executing 
> instructions that are defined architecturally as synchronizing global TLB 
> should have a global effect.
> 
> * tlbie on BookS
> * tlbivax on BookE
> * H_CALLs (H_REMOVE, H_BULK_REMOVE and H_PROTECT) in case of pseries, 
>   since they contain a tlbie on a real hypervisor.
> 
> The implementation provides a single point that can be used in MTTCG for 
> doing async-flushes.
> 
> The patchset introduces bit-flags in CPUPPCState::tlb_need_flush:
> 
>   TLB_NEED_LOCAL_FLUSH (0x1) - Flush local tlb
>   TLB_NEED_GLOBAL_FLUSH (0x2) - Flush tlb on other cpus.
> 
> Note: Tested patches on commit ebc231d(Merge remote-tracking branch 
> 'remotes/awilliam/tags/vfio-fixes-20160915.0' into staging), as ppc64 
> system emulation is broken after that.

Applied to ppc-for-2.8.  I did make a couple of small tweaks to
patches 2 & 3.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]