Re: [PULL 15/20] include/hw/core: Create struct CPUJumpCache

[Ilya Leoshkevich]

On Thu, Oct 27, 2022 at 04:18:56PM +0200, Ilya Leoshkevich wrote:
> On Tue, Oct 04, 2022 at 12:52:36PM -0700, Richard Henderson wrote:
> > Wrap the bare TranslationBlock pointer into a structure.
> > 
> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  accel/tcg/tb-hash.h       |  1 +
> >  accel/tcg/tb-jmp-cache.h  | 24 ++++++++++++++++++++++++
> >  include/exec/cpu-common.h |  1 +
> >  include/hw/core/cpu.h     | 15 +--------------
> >  include/qemu/typedefs.h   |  1 +
> >  accel/stubs/tcg-stub.c    |  4 ++++
> >  accel/tcg/cpu-exec.c      | 10 +++++++---
> >  accel/tcg/cputlb.c        |  9 +++++----
> >  accel/tcg/translate-all.c | 28 +++++++++++++++++++++++++---
> >  hw/core/cpu-common.c      |  3 +--
> >  plugins/core.c            |  2 +-
> >  trace/control-target.c    |  2 +-
> >  12 files changed, 72 insertions(+), 28 deletions(-)
> >  create mode 100644 accel/tcg/tb-jmp-cache.h
> 
> Hi,
> 
> After this patch, I get:
> 
>     qemu-s390x: qemu/include/qemu/rcu.h:102: rcu_read_unlock: Assertion 
> `p_rcu_reader->depth != 0' failed.
> 
> in one of the wasmtime tests (host=x86_64, guest=s390x).
> GDB shows that the root cause is actually this:
> 
>     Thread 181 "wasi_tokio::pat" received signal SIGSEGV, Segmentation fault.
>     [Switching to Thread 0x7ffff6c54640 (LWP 168352)]
>     0x0000555555626736 in do_tb_phys_invalidate (tb=tb@entry=0x7fffea4b8500 
> <code_gen_buffer+38503635>, rm_from_page_list=rm_from_page_list@entry=true) 
> at qemu/accel/tcg/translate-all.c:1192
>     1192              if (qatomic_read(&jc->array[h].tb) == tb) {
>     (gdb) bt
>     #0  0x0000555555626736 in do_tb_phys_invalidate 
> (tb=tb@entry=0x7fffea4b8500 <code_gen_buffer+38503635>, 
> rm_from_page_list=rm_from_page_list@entry=true) at 
> qemu/accel/tcg/translate-all.c:1192
>     #1  0x0000555555626b98 in tb_phys_invalidate__locked (tb=0x7fffea4b8500 
> <code_gen_buffer+38503635>) at qemu/accel/tcg/translate-all.c:1211
>     #2  tb_invalidate_phys_page_range__locked (p=<optimized out>, 
> start=start@entry=836716683264, end=end@entry=836716687360, retaddr=0, 
> pages=0x0) at qemu/accel/tcg/translate-all.c:1678
>     #3  0x0000555555626dfb in tb_invalidate_phys_range (start=836716683264, 
> start@entry=836716584960, end=end@entry=836716982272) at 
> qemu/accel/tcg/translate-all.c:1753
>     #4  0x0000555555639e43 in target_munmap (start=start@entry=836716584960, 
> len=len@entry=397312) at qemu/linux-user/mmap.c:769
> 
> Let me know if you need more information, I can try to extract a
> minimal reproducer.
> 
> Best regards,
> Ilya

Putting CPUJumpCache inside CPUState made problem go away:

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 18ca701b443..3ea528566c3 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -32,6 +32,7 @@
 #include "qemu/thread.h"
 #include "qemu/plugin.h"
 #include "qom/object.h"
+#include "accel/tcg/tb-jmp-cache.h"
 
 typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,
                                      void *opaque);
@@ -366,7 +367,7 @@ struct CPUState {
     CPUArchState *env_ptr;
     IcountDecr *icount_decr_ptr;
 
-    CPUJumpCache *tb_jmp_cache;
+    CPUJumpCache tb_jmp_cache;
 
     struct GDBRegisterState *gdb_regs;
     int gdb_num_regs;
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 2d7e610ee24..47165fc03e3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -253,7 +253,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, 
target_ulong pc,
     tcg_debug_assert(!(cflags & CF_INVALID));
 
     hash = tb_jmp_cache_hash_func(pc);
-    tb = qatomic_rcu_read(&cpu->tb_jmp_cache->array[hash].tb);
+    tb = qatomic_rcu_read(&cpu->tb_jmp_cache.array[hash].tb);
 
     if (likely(tb &&
                tb->pc == pc &&
@@ -267,7 +267,7 @@ static inline TranslationBlock *tb_lookup(CPUState *cpu, 
target_ulong pc,
     if (tb == NULL) {
         return NULL;
     }
-    qatomic_set(&cpu->tb_jmp_cache->array[hash].tb, tb);
+    qatomic_set(&cpu->tb_jmp_cache.array[hash].tb, tb);
     return tb;
 }
 
@@ -998,7 +998,7 @@ int cpu_exec(CPUState *cpu)
                  * for the fast lookup
                  */
                 h = tb_jmp_cache_hash_func(pc);
-                qatomic_set(&cpu->tb_jmp_cache->array[h].tb, tb);
+                qatomic_set(&cpu->tb_jmp_cache.array[h].tb, tb);
             }
 
 #ifndef CONFIG_USER_ONLY
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 63ecc152366..fffd9cb15f8 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -1188,7 +1188,7 @@ static void do_tb_phys_invalidate(TranslationBlock *tb, 
bool rm_from_page_list)
     /* remove the TB from the hash list */
     h = tb_jmp_cache_hash_func(tb->pc);
     CPU_FOREACH(cpu) {
-        CPUJumpCache *jc = cpu->tb_jmp_cache;
+        CPUJumpCache *jc = &cpu->tb_jmp_cache;
         if (qatomic_read(&jc->array[h].tb) == tb) {
             qatomic_set(&jc->array[h].tb, NULL);
         }
@@ -2445,23 +2445,12 @@ int page_unprotect(target_ulong address, uintptr_t pc)
 }
 #endif /* CONFIG_USER_ONLY */
 
-/*
- * Called by generic code at e.g. cpu reset after cpu creation,
- * therefore we must be prepared to allocate the jump cache.
- */
 void tcg_flush_jmp_cache(CPUState *cpu)
 {
-    CPUJumpCache *jc = cpu->tb_jmp_cache;
+    CPUJumpCache *jc = &cpu->tb_jmp_cache;
 
-    if (likely(jc)) {
-        for (int i = 0; i < TB_JMP_CACHE_SIZE; i++) {
-            qatomic_set(&jc->array[i].tb, NULL);
-        }
-    } else {
-        /* This should happen once during realize, and thus never race. */
-        jc = g_new0(CPUJumpCache, 1);
-        jc = qatomic_xchg(&cpu->tb_jmp_cache, jc);
-        assert(jc == NULL);
+    for (int i = 0; i < TB_JMP_CACHE_SIZE; i++) {
+        qatomic_set(&jc->array[i].tb, NULL);
     }
 }
 
So there must be a race in tcg_flush_jmp_cache() after all?

Best regards,
Ilya