[PATCH 0/5] accel/tcg: Call tcg_flush_jmp_cache() again when creating user-mode cpu

[PATCH 0/5] accel/tcg: Call tcg_flush_jmp_cache() again when creating user-mode cpu

[Philippe Mathieu-Daudé]

Fix a bug introduced during a refactor in commit bb6cf6f016
("accel/tcg: Factor tcg_cpu_reset_hold() out"), affecting
user emulation. (cpu_reset is called during thread creation,
after cpu_realize).

This issue was noticed by Ilya when running WASM:
https://lore.kernel.org/qemu-devel/20221027141856.w5umjgklawgu7pqv@heavy/

Philippe Mathieu-Daudé (5):
  accel/tcg: Simplify meson.build using subdir_done()
  accel/tcg: Move system emulation files under sysemu/ subdirectory
  accel/tcg: Do not define cpu_exec_reset_hold() as stub
  accel/tcg: Introduce common tcg_exec_cpu_reset_hold() method
  accel/tcg: Always call tcg_flush_jmp_cache() on reset

 accel/tcg/internal-common.h                   |  2 ++
 accel/tcg/{ => sysemu}/tcg-accel-ops-icount.h |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.h  |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-rr.h     |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops.h        |  0
 accel/tcg/cpu-exec.c                          |  6 ++++
 accel/tcg/{ => sysemu}/icount-common.c        |  0
 accel/tcg/{ => sysemu}/monitor.c              |  4 +--
 accel/tcg/{ => sysemu}/tcg-accel-ops-icount.c |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.c  |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-rr.c     |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops.c        |  5 ++--
 accel/tcg/translate-all.c                     |  5 ----
 accel/tcg/user-exec-stub.c                    |  4 ---
 accel/tcg/user-exec.c                         |  5 ++++
 accel/tcg/meson.build                         | 30 +++++++------------
 accel/tcg/sysemu/meson.build                  | 11 +++++++
 17 files changed, 39 insertions(+), 33 deletions(-)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-icount.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-rr.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops.h (100%)
 rename accel/tcg/{ => sysemu}/icount-common.c (100%)
 rename accel/tcg/{ => sysemu}/monitor.c (99%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-icount.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-rr.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops.c (99%)
 create mode 100644 accel/tcg/sysemu/meson.build

-- 
2.41.0

[PATCH 1/5] accel/tcg: Simplify meson.build using subdir_done()

[Philippe Mathieu-Daudé]

If CONFIG_TCG is not defined, skip this directory calling
subdir_done(). Then since we know CONFIG_TCG is defined,
we don't need to check for it.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/meson.build | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index aef80de967..f40959436e 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -1,8 +1,11 @@
-common_ss.add(when: 'CONFIG_TCG', if_true: files(
+if not config_all_accel.has_key('CONFIG_TCG')
+   subdir_done()
+endif
+
+common_ss.add(files(
   'cpu-exec-common.c',
 ))
-tcg_specific_ss = ss.source_set()
-tcg_specific_ss.add(files(
+specific_ss.add(files(
   'tcg-all.c',
   'cpu-exec.c',
   'tb-maint.c',
@@ -11,24 +14,23 @@ tcg_specific_ss.add(files(
   'translate-all.c',
   'translator.c',
 ))
-tcg_specific_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
-tcg_specific_ss.add(when: 'CONFIG_SYSTEM_ONLY', if_false: files('user-exec-stub.c'))
+specific_ss.add(when: 'CONFIG_USER_ONLY', if_true: files('user-exec.c'))
+specific_ss.add(when: 'CONFIG_SYSTEM_ONLY', if_false: files('user-exec-stub.c'))
 if get_option('plugins')
-  tcg_specific_ss.add(files('plugin-gen.c'))
+  specific_ss.add(files('plugin-gen.c'))
 endif
-specific_ss.add_all(when: 'CONFIG_TCG', if_true: tcg_specific_ss)
 
-specific_ss.add(when: ['CONFIG_SYSTEM_ONLY', 'CONFIG_TCG'], if_true: files(
+specific_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files(
   'cputlb.c',
   'watchpoint.c',
 ))
 
-system_ss.add(when: ['CONFIG_TCG'], if_true: files(
+system_ss.add(files(
   'icount-common.c',
   'monitor.c',
 ))
 
-tcg_module_ss.add(when: ['CONFIG_SYSTEM_ONLY', 'CONFIG_TCG'], if_true: files(
+tcg_module_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files(
   'tcg-accel-ops.c',
   'tcg-accel-ops-mttcg.c',
   'tcg-accel-ops-icount.c',
-- 
2.41.0

[PATCH 2/5] accel/tcg: Move system emulation files under sysemu/ subdirectory

[Philippe Mathieu-Daudé]

Some files are specific to system emulation. Move them under
their own sysemu/ directory. This might help to notice what
is affected (user, system or both) when doing global refactors.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/{ => sysemu}/tcg-accel-ops-icount.h |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.h  |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-rr.h     |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops.h        |  0
 accel/tcg/{ => sysemu}/icount-common.c        |  0
 accel/tcg/{ => sysemu}/monitor.c              |  4 ++--
 accel/tcg/{ => sysemu}/tcg-accel-ops-icount.c |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.c  |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops-rr.c     |  0
 accel/tcg/{ => sysemu}/tcg-accel-ops.c        |  0
 accel/tcg/meson.build                         | 14 ++------------
 accel/tcg/sysemu/meson.build                  | 11 +++++++++++
 12 files changed, 15 insertions(+), 14 deletions(-)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-icount.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-rr.h (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops.h (100%)
 rename accel/tcg/{ => sysemu}/icount-common.c (100%)
 rename accel/tcg/{ => sysemu}/monitor.c (99%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-icount.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-mttcg.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops-rr.c (100%)
 rename accel/tcg/{ => sysemu}/tcg-accel-ops.c (100%)
 create mode 100644 accel/tcg/sysemu/meson.build

diff --git a/accel/tcg/tcg-accel-ops-icount.h b/accel/tcg/sysemu/tcg-accel-ops-icount.h
similarity index 100%
rename from accel/tcg/tcg-accel-ops-icount.h
rename to accel/tcg/sysemu/tcg-accel-ops-icount.h
diff --git a/accel/tcg/tcg-accel-ops-mttcg.h b/accel/tcg/sysemu/tcg-accel-ops-mttcg.h
similarity index 100%
rename from accel/tcg/tcg-accel-ops-mttcg.h
rename to accel/tcg/sysemu/tcg-accel-ops-mttcg.h
diff --git a/accel/tcg/tcg-accel-ops-rr.h b/accel/tcg/sysemu/tcg-accel-ops-rr.h
similarity index 100%
rename from accel/tcg/tcg-accel-ops-rr.h
rename to accel/tcg/sysemu/tcg-accel-ops-rr.h
diff --git a/accel/tcg/tcg-accel-ops.h b/accel/tcg/sysemu/tcg-accel-ops.h
similarity index 100%
rename from accel/tcg/tcg-accel-ops.h
rename to accel/tcg/sysemu/tcg-accel-ops.h
diff --git a/accel/tcg/icount-common.c b/accel/tcg/sysemu/icount-common.c
similarity index 100%
rename from accel/tcg/icount-common.c
rename to accel/tcg/sysemu/icount-common.c
diff --git a/accel/tcg/monitor.c b/accel/tcg/sysemu/monitor.c
similarity index 99%
rename from accel/tcg/monitor.c
rename to accel/tcg/sysemu/monitor.c
index 093efe9714..620c18d267 100644
--- a/accel/tcg/monitor.c
+++ b/accel/tcg/sysemu/monitor.c
@@ -17,8 +17,8 @@
 #include "sysemu/cpu-timers.h"
 #include "sysemu/tcg.h"
 #include "tcg/tcg.h"
-#include "internal-common.h"
-#include "tb-context.h"
+#include "../internal-common.h"
+#include "../tb-context.h"
 
 
 static void dump_drift_info(GString *buf)
diff --git a/accel/tcg/tcg-accel-ops-icount.c b/accel/tcg/sysemu/tcg-accel-ops-icount.c
similarity index 100%
rename from accel/tcg/tcg-accel-ops-icount.c
rename to accel/tcg/sysemu/tcg-accel-ops-icount.c
diff --git a/accel/tcg/tcg-accel-ops-mttcg.c b/accel/tcg/sysemu/tcg-accel-ops-mttcg.c
similarity index 100%
rename from accel/tcg/tcg-accel-ops-mttcg.c
rename to accel/tcg/sysemu/tcg-accel-ops-mttcg.c
diff --git a/accel/tcg/tcg-accel-ops-rr.c b/accel/tcg/sysemu/tcg-accel-ops-rr.c
similarity index 100%
rename from accel/tcg/tcg-accel-ops-rr.c
rename to accel/tcg/sysemu/tcg-accel-ops-rr.c
diff --git a/accel/tcg/tcg-accel-ops.c b/accel/tcg/sysemu/tcg-accel-ops.c
similarity index 100%
rename from accel/tcg/tcg-accel-ops.c
rename to accel/tcg/sysemu/tcg-accel-ops.c
diff --git a/accel/tcg/meson.build b/accel/tcg/meson.build
index f40959436e..8d263a71cd 100644
--- a/accel/tcg/meson.build
+++ b/accel/tcg/meson.build
@@ -20,19 +20,9 @@ if get_option('plugins')
   specific_ss.add(files('plugin-gen.c'))
 endif
 
-specific_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files(
+specific_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files( # SOFTMMU
   'cputlb.c',
   'watchpoint.c',
 ))
 
-system_ss.add(files(
-  'icount-common.c',
-  'monitor.c',
-))
-
-tcg_module_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files(
-  'tcg-accel-ops.c',
-  'tcg-accel-ops-mttcg.c',
-  'tcg-accel-ops-icount.c',
-  'tcg-accel-ops-rr.c',
-))
+subdir('sysemu')
diff --git a/accel/tcg/sysemu/meson.build b/accel/tcg/sysemu/meson.build
new file mode 100644
index 0000000000..d0b4939ba9
--- /dev/null
+++ b/accel/tcg/sysemu/meson.build
@@ -0,0 +1,11 @@
+system_ss.add(files(
+  'icount-common.c',
+  'monitor.c',
+))
+
+tcg_module_ss.add(when: ['CONFIG_SYSTEM_ONLY'], if_true: files(
+  'tcg-accel-ops.c',
+  'tcg-accel-ops-mttcg.c',
+  'tcg-accel-ops-icount.c',
+  'tcg-accel-ops-rr.c',
+))
-- 
2.41.0

[PATCH 3/5] accel/tcg: Do not define cpu_exec_reset_hold() as stub

[Philippe Mathieu-Daudé]

In commit 1b5120d74b ("accel: Introduce cpu_exec_reset_hold()")
we defined the user emulation backend cpu_exec_reset_hold() as
a stub. This was a mistake, since common fields are set in
CPU reset, and user emulation needs these fields to be resetted.
Move cpu_exec_reset_hold() back.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/user-exec-stub.c | 4 ----
 accel/tcg/user-exec.c      | 4 ++++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel/tcg/user-exec-stub.c b/accel/tcg/user-exec-stub.c
index 4fbe2dbdc8..2dc6fd9c4e 100644
--- a/accel/tcg/user-exec-stub.c
+++ b/accel/tcg/user-exec-stub.c
@@ -14,10 +14,6 @@ void qemu_init_vcpu(CPUState *cpu)
 {
 }
 
-void cpu_exec_reset_hold(CPUState *cpu)
-{
-}
-
 /* User mode emulation does not support record/replay yet.  */
 
 bool replay_exception(void)
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 1c621477ad..6a108eb212 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -37,6 +37,10 @@ __thread uintptr_t helper_retaddr;
 
 //#define DEBUG_SIGNAL
 
+void cpu_exec_reset_hold(CPUState *cpu)
+{
+}
+
 /*
  * Adjust the pc to pass to cpu_restore_state; return the memop type.
  */
-- 
2.41.0

[PATCH 4/5] accel/tcg: Introduce common tcg_exec_cpu_reset_hold() method

[Philippe Mathieu-Daudé]

Introduce a method called by cpu_reset(), common
to both system and user emulation.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/internal-common.h      | 2 ++
 accel/tcg/cpu-exec.c             | 4 ++++
 accel/tcg/sysemu/tcg-accel-ops.c | 3 ++-
 accel/tcg/user-exec.c            | 1 +
 4 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/accel/tcg/internal-common.h b/accel/tcg/internal-common.h
index edefd0dcb7..b85285e2ff 100644
--- a/accel/tcg/internal-common.h
+++ b/accel/tcg/internal-common.h
@@ -23,4 +23,6 @@ static inline bool cpu_in_serial_context(CPUState *cs)
     return !(cs->tcg_cflags & CF_PARALLEL) || cpu_in_exclusive_context(cs);
 }
 
+void tcg_exec_cpu_reset_hold(CPUState *cpu);
+
 #endif
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 225e5fbd3e..1bf85c324d 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1096,3 +1096,7 @@ void tcg_exec_unrealizefn(CPUState *cpu)
     tlb_destroy(cpu);
     g_free_rcu(cpu->tb_jmp_cache, rcu);
 }
+
+void tcg_exec_cpu_reset_hold(CPUState *cpu)
+{
+}
diff --git a/accel/tcg/sysemu/tcg-accel-ops.c b/accel/tcg/sysemu/tcg-accel-ops.c
index 2c7b0cc09e..82c8368f87 100644
--- a/accel/tcg/sysemu/tcg-accel-ops.c
+++ b/accel/tcg/sysemu/tcg-accel-ops.c
@@ -36,7 +36,7 @@
 #include "exec/hwaddr.h"
 #include "exec/tb-flush.h"
 #include "exec/gdbstub.h"
-
+#include "../internal-common.h"
 #include "hw/core/cpu.h"
 
 #include "tcg-accel-ops.h"
@@ -82,6 +82,7 @@ int tcg_cpu_exec(CPUState *cpu)
 
 static void tcg_cpu_reset_hold(CPUState *cpu)
 {
+    tcg_exec_cpu_reset_hold(cpu);
     tcg_flush_jmp_cache(cpu);
 
     tlb_flush(cpu);
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 6a108eb212..85e92ec8f2 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -39,6 +39,7 @@ __thread uintptr_t helper_retaddr;
 
 void cpu_exec_reset_hold(CPUState *cpu)
 {
+    tcg_exec_cpu_reset_hold(cpu);
 }
 
 /*
-- 
2.41.0

[PATCH 5/5] accel/tcg: Always call tcg_flush_jmp_cache() on reset

[Philippe Mathieu-Daudé]

In commit bb6cf6f016 ("accel/tcg: Factor tcg_cpu_reset_hold() out")
we unfortunately restricted the tcg_flush_jmp_cache() to system
emulation. Move it to the common tcg_exec_cpu_reset_hold() handler
so user emulation gets the jmp_cache initialized when threads
are created.

Remove the NULL check in tcg_flush_jmp_cache() from commit 4e4fa6c12d
("accel/tcg: Complete cpu initialization before registration") which
was a band-aid fix for incorrect commit bb6cf6f016.

Cc: qemu-stable@nongnu.org
Fixes: bb6cf6f016 ("accel/tcg: Factor tcg_cpu_reset_hold() out")
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/cpu-exec.c             | 2 ++
 accel/tcg/sysemu/tcg-accel-ops.c | 2 --
 accel/tcg/translate-all.c        | 5 -----
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 1bf85c324d..7e04df2902 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -36,6 +36,7 @@
 #include "exec/replay-core.h"
 #include "sysemu/tcg.h"
 #include "exec/helper-proto-common.h"
+#include "exec/tb-flush.h"
 #include "tb-jmp-cache.h"
 #include "tb-hash.h"
 #include "tb-context.h"
@@ -1099,4 +1100,5 @@ void tcg_exec_unrealizefn(CPUState *cpu)
 
 void tcg_exec_cpu_reset_hold(CPUState *cpu)
 {
+    tcg_flush_jmp_cache(cpu);
 }
diff --git a/accel/tcg/sysemu/tcg-accel-ops.c b/accel/tcg/sysemu/tcg-accel-ops.c
index 82c8368f87..13e450c088 100644
--- a/accel/tcg/sysemu/tcg-accel-ops.c
+++ b/accel/tcg/sysemu/tcg-accel-ops.c
@@ -34,7 +34,6 @@
 #include "qemu/timer.h"
 #include "exec/exec-all.h"
 #include "exec/hwaddr.h"
-#include "exec/tb-flush.h"
 #include "exec/gdbstub.h"
 #include "../internal-common.h"
 #include "hw/core/cpu.h"
@@ -83,7 +82,6 @@ int tcg_cpu_exec(CPUState *cpu)
 static void tcg_cpu_reset_hold(CPUState *cpu)
 {
     tcg_exec_cpu_reset_hold(cpu);
-    tcg_flush_jmp_cache(cpu);
 
     tlb_flush(cpu);
 }
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 83cc14fbde..93202fa3c1 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -663,11 +663,6 @@ void tcg_flush_jmp_cache(CPUState *cpu)
 {
     CPUJumpCache *jc = cpu->tb_jmp_cache;
 
-    /* During early initialization, the cache may not yet be allocated. */
-    if (unlikely(jc == NULL)) {
-        return;
-    }
-
     for (int i = 0; i < TB_JMP_CACHE_SIZE; i++) {
         qatomic_set(&jc->array[i].tb, NULL);
     }
-- 
2.41.0

Re: [PATCH 1/5] accel/tcg: Simplify meson.build using subdir_done()

[Paolo Bonzini]

On Fri, May 3, 2024 at 11:17 AM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> If CONFIG_TCG is not defined, skip this directory calling
> subdir_done(). Then since we know CONFIG_TCG is defined,
> we don't need to check for it.

You can only remove the check if you assume that TCG (unlike e.g. KVM)
is enabled for all targets. Of course this assumption is true right
now, but in principle it does not have to be - a long time ago,
qemu-kvm had ia64 as a KVM-only target for example.

So I'm not sure this patch is a good idea. A lot of it is just
replacing tcg_specific_ss with specific_ss.

Paolo

Re: [PATCH 1/5] accel/tcg: Simplify meson.build using subdir_done()

[Philippe Mathieu-Daudé]

On 3/5/24 13:16, Paolo Bonzini wrote:
> On Fri, May 3, 2024 at 11:17 AM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> If CONFIG_TCG is not defined, skip this directory calling
>> subdir_done(). Then since we know CONFIG_TCG is defined,
>> we don't need to check for it.
> 
> You can only remove the check if you assume that TCG (unlike e.g. KVM)
> is enabled for all targets. Of course this assumption is true right
> now, but in principle it does not have to be - a long time ago,
> qemu-kvm had ia64 as a KVM-only target for example.

Got it, thanks.

> So I'm not sure this patch is a good idea. A lot of it is just
> replacing tcg_specific_ss with specific_ss.
> 
> Paolo
>