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

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

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 ?

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/roms/SLOF b/roms/SLOF
--- a/roms/SLOF
+++ b/roms/SLOF
@@ -1 +1 @@
-Subproject commit e3d05727a074619fc12d0a67f05cf2c42c875cce
+Subproject commit e3d05727a074619fc12d0a67f05cf2c42c875cce-dirty
diff --git a/roms/openbios b/roms/openbios
index e79bca6..46ee135 160000
--- a/roms/openbios
+++ b/roms/openbios
@@ -1 +1 @@
-Subproject commit e79bca64838c96ec44fd7acd508879c5284233dd
+Subproject commit 46ee1352c50aa891e3dce9b3e3428ae9a5703fbe-dirty
diff --git a/roms/seabios b/roms/seabios
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit e2fc41e24ee0ada60fc511d60b15a41b294538be
+Subproject commit e2fc41e24ee0ada60fc511d60b15a41b294538be-dirty
diff --git a/roms/vgabios b/roms/vgabios
--- a/roms/vgabios
+++ b/roms/vgabios
@@ -1 +1 @@
-Subproject commit 19ea12c230ded95928ecaef0db47a82231c2e485
+Subproject commit 19ea12c230ded95928ecaef0db47a82231c2e485-dirty
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);
+        }
     }
 }


Cheers,
Ben.