diff mbox series

[1/3] target/ppc: Fix broadcast tlbie synchronisation

Message ID 20240328053131.2604454-2-npiggin@gmail.com
State New
Headers show
Series target/ppc: fix tlb flushing race | expand

Commit Message

Nicholas Piggin March 28, 2024, 5:31 a.m. UTC
With mttcg, broadcast tlbie instructions do not wait until other vCPUs
have been kicked out of TCG execution before they complete (including
necessary subsequent tlbsync, etc., instructions). This is contrary to
the ISA, and it permits other vCPUs to use translations after the TLB
flush. For example:

   CPU0
   // *memP is initially 0, memV maps to memP with *pte
   *pte = 0;
   ptesync ; tlbie ; eieio ; tlbsync ; ptesync
   *memP = 1;

   CPU1
   assert(*memV == 0);

It is possible for the assertion to fail because CPU1 translates memV
using the TLB after CPU0 has stored 1 to the underlying memory. This
race was observed with a careful test case where CPU1 checks run in a
very large expensive TB so it can run for the entire CPU0 period between
clearing the pte and storing the memory. It's normally very difficult to
hit, but preemption of host vCPU threads could trigger the race
anywhere.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 target/ppc/helper_regs.c | 2 +-
 target/ppc/mmu_helper.c  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Philippe Mathieu-Daudé March 28, 2024, 1:18 p.m. UTC | #1
On 28/3/24 06:31, Nicholas Piggin wrote:
> With mttcg, broadcast tlbie instructions do not wait until other vCPUs
> have been kicked out of TCG execution before they complete (including
> necessary subsequent tlbsync, etc., instructions). This is contrary to
> the ISA, and it permits other vCPUs to use translations after the TLB
> flush. For example:
> 
>     CPU0
>     // *memP is initially 0, memV maps to memP with *pte
>     *pte = 0;
>     ptesync ; tlbie ; eieio ; tlbsync ; ptesync
>     *memP = 1;
> 
>     CPU1
>     assert(*memV == 0);
> 
> It is possible for the assertion to fail because CPU1 translates memV
> using the TLB after CPU0 has stored 1 to the underlying memory. This
> race was observed with a careful test case where CPU1 checks run in a
> very large expensive TB so it can run for the entire CPU0 period between
> clearing the pte and storing the memory. It's normally very difficult to
> hit, but preemption of host vCPU threads could trigger the race
> anywhere.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   target/ppc/helper_regs.c | 2 +-
>   target/ppc/mmu_helper.c  | 2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)

To the best of my knowledge,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff mbox series

Patch

diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
index 25258986e3..9094ae5004 100644
--- a/target/ppc/helper_regs.c
+++ b/target/ppc/helper_regs.c
@@ -334,7 +334,7 @@  void check_tlb_flush(CPUPPCState *env, bool global)
     if (global && (env->tlb_need_flush & TLB_NEED_GLOBAL_FLUSH)) {
         env->tlb_need_flush &= ~TLB_NEED_GLOBAL_FLUSH;
         env->tlb_need_flush &= ~TLB_NEED_LOCAL_FLUSH;
-        tlb_flush_all_cpus(cs);
+        tlb_flush_all_cpus_synced(cs);
         return;
     }
 
diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index c071b4d5e2..aaa5bfc62a 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -533,7 +533,7 @@  void helper_tlbie_isa300(CPUPPCState *env, target_ulong rb, target_ulong rs,
     if (local) {
         tlb_flush_page(env_cpu(env), addr);
     } else {
-        tlb_flush_page_all_cpus(env_cpu(env), addr);
+        tlb_flush_page_all_cpus_synced(env_cpu(env), addr);
     }
     return;