[PATCH v2 0/3] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

[PATCH v2 0/3] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

[Richard Henderson]

Supercedes: 20250404235624.67816-1-philmd@linaro.org
("[PATCH-for-10.1 0/2] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps::has_precise_smc field")

Amusingly, I was already working on this before Phil posted his patch,
as part of trying to compile tb-maint.c twice.  The major difference
between our two patches is I don't introduce a global variable; I read
from cpu->cc->tcg_ops->precice_smc when needed.


r~


Richard Henderson (3):
  accel/tcg: Add CPUState argument to page_unprotect
  accel/tcg: Add CPUState argument to tb_invalidate_phys_page_unwind
  accel/tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

 accel/tcg/tb-internal.h        |  3 ++-
 include/accel/tcg/cpu-ops.h    |  6 ++++++
 include/exec/poison.h          |  1 -
 include/user/page-protection.h |  2 +-
 target/i386/cpu.h              |  4 ----
 target/s390x/cpu.h             |  2 --
 accel/tcg/tb-maint.c           | 34 ++++++++++++++--------------------
 accel/tcg/user-exec.c          | 13 ++++++++-----
 linux-user/elfload.c           |  2 +-
 target/i386/tcg/tcg-cpu.c      |  1 +
 target/s390x/cpu.c             |  1 +
 11 files changed, 34 insertions(+), 35 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] accel/tcg: Add CPUState argument to page_unprotect

[Richard Henderson]

In the next patch, page_unprotect will need to pass
the CPUState to tb_invalidate_phys_page_unwind.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/user/page-protection.h | 2 +-
 accel/tcg/user-exec.c          | 8 +++++---
 linux-user/elfload.c           | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/include/user/page-protection.h b/include/user/page-protection.h
index d5c8748d49..1de72e31e6 100644
--- a/include/user/page-protection.h
+++ b/include/user/page-protection.h
@@ -16,7 +16,7 @@
 #include "exec/target_long.h"
 #include "exec/translation-block.h"
 
-int page_unprotect(tb_page_addr_t address, uintptr_t pc);
+int page_unprotect(CPUState *cpu, tb_page_addr_t address, uintptr_t pc);
 
 int page_get_flags(target_ulong address);
 
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 5eef8e7f18..90b345a0cf 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -128,7 +128,7 @@ MMUAccessType adjust_signal_pc(uintptr_t *pc, bool is_write)
 bool handle_sigsegv_accerr_write(CPUState *cpu, sigset_t *old_set,
                                  uintptr_t host_pc, abi_ptr guest_addr)
 {
-    switch (page_unprotect(guest_addr, host_pc)) {
+    switch (page_unprotect(cpu, guest_addr, host_pc)) {
     case 0:
         /*
          * Fault not caused by a page marked unwritable to protect
@@ -584,7 +584,7 @@ bool page_check_range(target_ulong start, target_ulong len, int flags)
                 break;
             }
             /* Asking about writable, but has been protected: undo. */
-            if (!page_unprotect(start, 0)) {
+            if (!page_unprotect(NULL, start, 0)) {
                 ret = false;
                 break;
             }
@@ -704,11 +704,13 @@ void tb_lock_page0(tb_page_addr_t address)
  * immediately exited. (We can only return 2 if the 'pc' argument is
  * non-zero.)
  */
-int page_unprotect(tb_page_addr_t address, uintptr_t pc)
+int page_unprotect(CPUState *cpu, tb_page_addr_t address, uintptr_t pc)
 {
     PageFlagsNode *p;
     bool current_tb_invalidated;
 
+    assert((cpu == NULL) == (pc == 0));
+
     /*
      * Technically this isn't safe inside a signal handler.  However we
      * know this only ever happens in a synchronous SEGV handler, so in
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 99811af5e7..7519b6bcda 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -4245,7 +4245,7 @@ static int wmr_page_unprotect_regions(void *opaque, target_ulong start,
         size_t step = MAX(TARGET_PAGE_SIZE, qemu_real_host_page_size());
 
         while (1) {
-            page_unprotect(start, 0);
+            page_unprotect(NULL, start, 0);
             if (end - start <= step) {
                 break;
             }
-- 
2.43.0

Re: [PATCH v2 1/3] accel/tcg: Add CPUState argument to page_unprotect

[Philippe Mathieu-Daudé]

On 5/4/25 17:50, Richard Henderson wrote:
> In the next patch, page_unprotect will need to pass
> the CPUState to tb_invalidate_phys_page_unwind.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/user/page-protection.h | 2 +-
>   accel/tcg/user-exec.c          | 8 +++++---
>   linux-user/elfload.c           | 2 +-
>   3 files changed, 7 insertions(+), 5 deletions(-)

I was not sure this would be acceptable, but this is what we need for
heterogeneous emulation :)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 2/3] accel/tcg: Add CPUState argument to tb_invalidate_phys_page_unwind

[Richard Henderson]

Replace existing usage of current_cpu.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 accel/tcg/tb-internal.h | 3 ++-
 accel/tcg/tb-maint.c    | 8 ++++----
 accel/tcg/user-exec.c   | 5 +++--
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/accel/tcg/tb-internal.h b/accel/tcg/tb-internal.h
index 08538e2896..1078de6c99 100644
--- a/accel/tcg/tb-internal.h
+++ b/accel/tcg/tb-internal.h
@@ -50,6 +50,7 @@ void tb_invalidate_phys_range_fast(ram_addr_t ram_addr,
                                    uintptr_t retaddr);
 #endif /* CONFIG_SOFTMMU */
 
-bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc);
+bool tb_invalidate_phys_page_unwind(CPUState *cpu, tb_page_addr_t addr,
+                                    uintptr_t pc);
 
 #endif
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index d479f53ae0..714dcaedc9 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -1045,7 +1045,8 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr)
  * TB (because it was modified by this store and the guest CPU has
  * precise-SMC semantics).
  */
-bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
+bool tb_invalidate_phys_page_unwind(CPUState *cpu, tb_page_addr_t addr,
+                                    uintptr_t pc)
 {
     TranslationBlock *current_tb;
     bool current_tb_modified;
@@ -1083,15 +1084,14 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
              * the CPU state.
              */
             current_tb_modified = true;
-            cpu_restore_state_from_tb(current_cpu, current_tb, pc);
+            cpu_restore_state_from_tb(cpu, current_tb, pc);
         }
         tb_phys_invalidate__locked(tb);
     }
 
     if (current_tb_modified) {
         /* Force execution of one insn next time.  */
-        CPUState *cpu = current_cpu;
-        cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(current_cpu);
+        cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(cpu);
         return true;
     }
     return false;
diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 90b345a0cf..39b76d9654 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -749,7 +749,8 @@ int page_unprotect(CPUState *cpu, tb_page_addr_t address, uintptr_t pc)
             len = TARGET_PAGE_SIZE;
             prot = p->flags | PAGE_WRITE;
             pageflags_set_clear(start, start + len - 1, PAGE_WRITE, 0);
-            current_tb_invalidated = tb_invalidate_phys_page_unwind(start, pc);
+            current_tb_invalidated =
+                tb_invalidate_phys_page_unwind(cpu, start, pc);
         } else {
             start = address & -host_page_size;
             len = host_page_size;
@@ -772,7 +773,7 @@ int page_unprotect(CPUState *cpu, tb_page_addr_t address, uintptr_t pc)
                  * the corresponding translated code.
                  */
                 current_tb_invalidated |=
-                    tb_invalidate_phys_page_unwind(addr, pc);
+                    tb_invalidate_phys_page_unwind(cpu, addr, pc);
             }
         }
         if (prot & PAGE_EXEC) {
-- 
2.43.0

Re: [PATCH v2 2/3] accel/tcg: Add CPUState argument to tb_invalidate_phys_page_unwind

[Philippe Mathieu-Daudé]

On 5/4/25 17:50, Richard Henderson wrote:
> Replace existing usage of current_cpu.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   accel/tcg/tb-internal.h | 3 ++-
>   accel/tcg/tb-maint.c    | 8 ++++----
>   accel/tcg/user-exec.c   | 5 +++--
>   3 files changed, 9 insertions(+), 7 deletions(-)

Excellent.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH v2 3/3] accel/tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

[Richard Henderson]

Instead of having a compile-time TARGET_HAS_PRECISE_SMC definition,
have each target set the 'precise_smc' field in the TCGCPUOps
structure.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/accel/tcg/cpu-ops.h |  6 ++++++
 include/exec/poison.h       |  1 -
 target/i386/cpu.h           |  4 ----
 target/s390x/cpu.h          |  2 --
 accel/tcg/tb-maint.c        | 26 ++++++++++----------------
 target/i386/tcg/tcg-cpu.c   |  1 +
 target/s390x/cpu.c          |  1 +
 7 files changed, 18 insertions(+), 23 deletions(-)

diff --git a/include/accel/tcg/cpu-ops.h b/include/accel/tcg/cpu-ops.h
index a4932fc5d7..303753c43e 100644
--- a/include/accel/tcg/cpu-ops.h
+++ b/include/accel/tcg/cpu-ops.h
@@ -19,6 +19,12 @@
 #include "tcg/tcg-mo.h"
 
 struct TCGCPUOps {
+    /**
+     * @precise_smc: Stores which modify code within the current TB force
+     *               the TB to exit; the next executed instruction will see
+     *               the result of the store.
+     */
+    bool precise_smc;
 
     /**
      * @guest_default_memory_order: default barrier that is required
diff --git a/include/exec/poison.h b/include/exec/poison.h
index a09e0c1263..1593a56489 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -38,7 +38,6 @@
 #pragma GCC poison TARGET_SUPPORTS_MTTCG
 #pragma GCC poison TARGET_BIG_ENDIAN
 #pragma GCC poison TCG_GUEST_DEFAULT_MO
-#pragma GCC poison TARGET_HAS_PRECISE_SMC
 
 #pragma GCC poison TARGET_LONG_BITS
 #pragma GCC poison TARGET_FMT_lx
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 16d76df34b..5a2e4a8103 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -35,10 +35,6 @@
 
 #define XEN_NR_VIRQS 24
 
-/* support for self modifying code even if the modified instruction is
-   close to the modifying instruction */
-#define TARGET_HAS_PRECISE_SMC
-
 #ifdef TARGET_X86_64
 #define I386_ELF_MACHINE  EM_X86_64
 #define ELF_MACHINE_UNAME "x86_64"
diff --git a/target/s390x/cpu.h b/target/s390x/cpu.h
index 90f64ee20c..ee59039879 100644
--- a/target/s390x/cpu.h
+++ b/target/s390x/cpu.h
@@ -35,8 +35,6 @@
 
 #define ELF_MACHINE_UNAME "S390X"
 
-#define TARGET_HAS_PRECISE_SMC
-
 #define MMU_USER_IDX 0
 
 #define S390_MAX_CPUS 248
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index 714dcaedc9..2af6c6d451 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -28,6 +28,7 @@
 #include "exec/mmap-lock.h"
 #include "exec/tb-flush.h"
 #include "exec/target_page.h"
+#include "accel/tcg/cpu-ops.h"
 #include "tb-internal.h"
 #include "system/tcg.h"
 #include "tcg/tcg.h"
@@ -1041,9 +1042,7 @@ static void tb_invalidate_phys_page(tb_page_addr_t addr)
 /*
  * Called with mmap_lock held. If pc is not 0 then it indicates the
  * host PC of the faulting store instruction that caused this invalidate.
- * Returns true if the caller needs to abort execution of the current
- * TB (because it was modified by this store and the guest CPU has
- * precise-SMC semantics).
+ * Returns true if the caller needs to abort execution of the current TB.
  */
 bool tb_invalidate_phys_page_unwind(CPUState *cpu, tb_page_addr_t addr,
                                     uintptr_t pc)
@@ -1058,10 +1057,7 @@ bool tb_invalidate_phys_page_unwind(CPUState *cpu, tb_page_addr_t addr,
      * Without precise smc semantics, or when outside of a TB,
      * we can skip to invalidate.
      */
-#ifndef TARGET_HAS_PRECISE_SMC
-    pc = 0;
-#endif
-    if (!pc) {
+    if (!pc || !cpu->cc->tcg_ops->precise_smc) {
         tb_invalidate_phys_page(addr);
         return false;
     }
@@ -1109,14 +1105,16 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
 {
     TranslationBlock *tb;
     PageForEachNext n;
-#ifdef TARGET_HAS_PRECISE_SMC
     bool current_tb_modified = false;
-    TranslationBlock *current_tb = retaddr ? tcg_tb_lookup(retaddr) : NULL;
-#endif /* TARGET_HAS_PRECISE_SMC */
+    TranslationBlock *current_tb = NULL;
 
     /* Range may not cross a page. */
     tcg_debug_assert(((start ^ last) & TARGET_PAGE_MASK) == 0);
 
+    if (retaddr && current_cpu->cc->tcg_ops->precise_smc) {
+        current_tb = tcg_tb_lookup(retaddr);
+    }
+
     /*
      * We remove all the TBs in the range [start, last].
      * XXX: see if in some cases it could be faster to invalidate all the code
@@ -1134,8 +1132,7 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
             tb_last = tb_start + (tb_last & ~TARGET_PAGE_MASK);
         }
         if (!(tb_last < start || tb_start > last)) {
-#ifdef TARGET_HAS_PRECISE_SMC
-            if (current_tb == tb &&
+            if (unlikely(current_tb == tb) &&
                 (tb_cflags(current_tb) & CF_COUNT_MASK) != 1) {
                 /*
                  * If we are modifying the current TB, we must stop
@@ -1147,7 +1144,6 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
                 current_tb_modified = true;
                 cpu_restore_state_from_tb(current_cpu, current_tb, retaddr);
             }
-#endif /* TARGET_HAS_PRECISE_SMC */
             tb_phys_invalidate__locked(tb);
         }
     }
@@ -1157,15 +1153,13 @@ tb_invalidate_phys_page_range__locked(struct page_collection *pages,
         tlb_unprotect_code(start);
     }
 
-#ifdef TARGET_HAS_PRECISE_SMC
-    if (current_tb_modified) {
+    if (unlikely(current_tb_modified)) {
         page_collection_unlock(pages);
         /* Force execution of one insn next time.  */
         current_cpu->cflags_next_tb = 1 | CF_NOIRQ | curr_cflags(current_cpu);
         mmap_unlock();
         cpu_loop_exit_noexc(current_cpu);
     }
-#endif
 }
 
 /*
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index d941df0956..8e8ebe8aa2 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -125,6 +125,7 @@ static bool x86_debug_check_breakpoint(CPUState *cs)
 #include "accel/tcg/cpu-ops.h"
 
 static const TCGCPUOps x86_tcg_ops = {
+    .precise_smc = true,
     /*
      * The x86 has a strong memory model with some store-after-load re-ordering
      */
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 1e101b5afe..f8f859cd30 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -345,6 +345,7 @@ void cpu_get_tb_cpu_state(CPUS390XState *env, vaddr *pc,
 }
 
 static const TCGCPUOps s390_tcg_ops = {
+    .precise_smc = true,
     /*
      * The z/Architecture has a strong memory model with some
      * store-after-load re-ordering.
-- 
2.43.0

Re: [PATCH v2 3/3] accel/tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

[Philippe Mathieu-Daudé]

On 5/4/25 17:50, Richard Henderson wrote:
> Instead of having a compile-time TARGET_HAS_PRECISE_SMC definition,
> have each target set the 'precise_smc' field in the TCGCPUOps
> structure.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/accel/tcg/cpu-ops.h |  6 ++++++
>   include/exec/poison.h       |  1 -
>   target/i386/cpu.h           |  4 ----
>   target/s390x/cpu.h          |  2 --
>   accel/tcg/tb-maint.c        | 26 ++++++++++----------------
>   target/i386/tcg/tcg-cpu.c   |  1 +
>   target/s390x/cpu.c          |  1 +
>   7 files changed, 18 insertions(+), 23 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2 3/3] accel/tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps.precise_smc

[Richard Henderson]

On 4/5/25 08:50, Richard Henderson wrote:
> Instead of having a compile-time TARGET_HAS_PRECISE_SMC definition,
> have each target set the 'precise_smc' field in the TCGCPUOps
> structure.
> 
> Signed-off-by: Richard Henderson<richard.henderson@linaro.org>
> ---
>   include/accel/tcg/cpu-ops.h |  6 ++++++
>   include/exec/poison.h       |  1 -
>   target/i386/cpu.h           |  4 ----
>   target/s390x/cpu.h          |  2 --
>   accel/tcg/tb-maint.c        | 26 ++++++++++----------------
>   target/i386/tcg/tcg-cpu.c   |  1 +
>   target/s390x/cpu.c          |  1 +
>   7 files changed, 18 insertions(+), 23 deletions(-)

Bah, I missed adding user-exec.c to the commit.


r~