diff mbox series

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

Message ID 20240503123456.28866-6-philmd@linaro.org
State New
Headers show
Series accel/tcg: Call tcg_flush_jmp_cache() again when creating user-mode cpu | expand

Commit Message

Philippe Mathieu-Daudé May 3, 2024, 12:34 p.m. UTC 
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(-)

Comments

Fiona Ebner May 16, 2024, 11:17 a.m. UTC | #1 
Hi,

Am 03.05.24 um 14:34 schrieb 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.
> 

AFAICT, commit 4e4fa6c12d was already present in v7.2.0, while commit
bb6cf6f016 only later in v8.2.0. So is it really fine to remove the NULL
check?

Best Regards,
Fiona
Michael Tokarev Aug. 11, 2024, 5:43 p.m. UTC | #2 
03.05.2024 15:34, Philippe Mathieu-Daudé wrote:
> 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>

Hi!

Has this change been forgotten, or is it not appropriate anymore?

Thanks,

/mjt
Philippe Mathieu-Daudé Aug. 13, 2024, 3:10 p.m. UTC | #3 
Hi Michael,

On 11/8/24 19:43, Michael Tokarev wrote:
> 03.05.2024 15:34, Philippe Mathieu-Daudé wrote:
>> 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>
> 
> Hi!
> 
> Has this change been forgotten, or is it not appropriate anymore?

Not forgotten and still need to be fixed, however unfortunately
this exposed a bug in user-mode SYS_exit_group when using plugins
(see qemu_plugin_disable_mem_helpers call in qemu_plugin_user_exit).

Pierrick is working on it, and I'll rebase this series once his
work gets merged. Next release :/

Regards,

Phil.
diff mbox series

Patch

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);
     }