[PATCH-for-10.1 0/2] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps::has_precise_smc field

[PATCH-for-10.1 0/2] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps::has_precise_smc field

[Philippe Mathieu-Daudé]

This series is similar to the TARGET_SUPPORTS_MTTCG replacement
to a 'mttcg_supported' field in TCGCPUOps, but doing it for
TARGET_HAS_PRECISE_SMC, adding the 'has_precise_smc' field.

Based on tcg-next tree.

Philippe Mathieu-Daudé (2):
  tcg: Introduce and use target_has_precise_smc() runtime helper
  tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps::has_precise_smc
    field

 accel/tcg/tb-internal.h     |  3 +++
 include/accel/tcg/cpu-ops.h |  8 ++++++++
 include/exec/poison.h       |  1 -
 target/i386/cpu.h           |  4 ----
 target/s390x/cpu.h          |  2 --
 accel/tcg/cpu-exec.c        | 12 ++++++++++--
 accel/tcg/tb-maint.c        | 18 +++++-------------
 accel/tcg/user-exec.c       | 10 +++++-----
 target/i386/tcg/tcg-cpu.c   |  1 +
 target/s390x/cpu.c          |  1 +
 10 files changed, 33 insertions(+), 27 deletions(-)

-- 
2.47.1

[PATCH-for-10.1 1/2] tcg: Introduce and use target_has_precise_smc() runtime helper

[Philippe Mathieu-Daudé]

target_has_precise_smc() returns the value of the
TARGET_HAS_PRECISE_SMC definition at runtime.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 accel/tcg/tb-internal.h |  3 +++
 accel/tcg/cpu-exec.c    |  9 +++++++++
 accel/tcg/tb-maint.c    | 18 +++++-------------
 accel/tcg/user-exec.c   | 10 +++++-----
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/accel/tcg/tb-internal.h b/accel/tcg/tb-internal.h
index 08538e2896b..a844709bbb2 100644
--- a/accel/tcg/tb-internal.h
+++ b/accel/tcg/tb-internal.h
@@ -44,6 +44,9 @@ void tb_unlock_page1(tb_page_addr_t, tb_page_addr_t);
 void tb_unlock_pages(TranslationBlock *);
 #endif
 
+/* Whether the target supports implicit self modifying code */
+bool target_has_precise_smc(void);
+
 #ifdef CONFIG_SOFTMMU
 void tb_invalidate_phys_range_fast(ram_addr_t ram_addr,
                                    unsigned size,
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index b00f046b29f..cfe3b93e1e3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1065,6 +1065,15 @@ int cpu_exec(CPUState *cpu)
     return ret;
 }
 
+bool target_has_precise_smc(void)
+{
+#ifdef TARGET_HAS_PRECISE_SMC
+    return true;
+#else
+    return false;
+#endif
+}
+
 bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
 {
     static bool tcg_target_initialized;
diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
index d479f53ae02..ae12ad2d867 100644
--- a/accel/tcg/tb-maint.c
+++ b/accel/tcg/tb-maint.c
@@ -1057,10 +1057,7 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
      * 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 (!target_has_precise_smc() || !pc) {
         tb_invalidate_phys_page(addr);
         return false;
     }
@@ -1109,10 +1106,9 @@ 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 = (target_has_precise_smc() && retaddr)
+                                   ? tcg_tb_lookup(retaddr) : NULL;
 
     /* Range may not cross a page. */
     tcg_debug_assert(((start ^ last) & TARGET_PAGE_MASK) == 0);
@@ -1134,8 +1130,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 (target_has_precise_smc() && current_tb == tb &&
                 (tb_cflags(current_tb) & CF_COUNT_MASK) != 1) {
                 /*
                  * If we are modifying the current TB, we must stop
@@ -1147,7 +1142,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 +1151,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 (target_has_precise_smc() && 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/accel/tcg/user-exec.c b/accel/tcg/user-exec.c
index 5eef8e7f186..135c54980a2 100644
--- a/accel/tcg/user-exec.c
+++ b/accel/tcg/user-exec.c
@@ -731,12 +731,12 @@ int page_unprotect(tb_page_addr_t address, uintptr_t pc)
          * this thread raced with another one which got here first and
          * set the page to PAGE_WRITE and did the TB invalidate for us.
          */
-#ifdef TARGET_HAS_PRECISE_SMC
-        TranslationBlock *current_tb = tcg_tb_lookup(pc);
-        if (current_tb) {
-            current_tb_invalidated = tb_cflags(current_tb) & CF_INVALID;
+        if (target_has_precise_smc()) {
+            TranslationBlock *current_tb = tcg_tb_lookup(pc);
+            if (current_tb) {
+                current_tb_invalidated = tb_cflags(current_tb) & CF_INVALID;
+            }
         }
-#endif
     } else {
         int host_page_size = qemu_real_host_page_size();
         target_ulong start, len, i;
-- 
2.47.1

[PATCH-for-10.1 2/2] tcg: Convert TARGET_HAS_PRECISE_SMC to TCGCPUOps::has_precise_smc field

[Philippe Mathieu-Daudé]

Instead of having a compile-time TARGET_HAS_PRECISE_SMC definition,
have targets set the 'has_precise_smc' field in the TCGCPUOps
structure.

Since so far we only emulate one target architecture at a time,
add a static 'tcg_target_has_precise_smc' variable, initialized
just after calling TCGCPUOps::initialize() in tcg_exec_realizefn().

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 include/accel/tcg/cpu-ops.h |  8 ++++++++
 include/exec/poison.h       |  1 -
 target/i386/cpu.h           |  4 ----
 target/s390x/cpu.h          |  2 --
 accel/tcg/cpu-exec.c        | 13 ++++++-------
 target/i386/tcg/tcg-cpu.c   |  1 +
 target/s390x/cpu.c          |  1 +
 7 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/accel/tcg/cpu-ops.h b/include/accel/tcg/cpu-ops.h
index 0e4352513d1..a76cfe49df8 100644
--- a/include/accel/tcg/cpu-ops.h
+++ b/include/accel/tcg/cpu-ops.h
@@ -28,6 +28,14 @@ struct TCGCPUOps {
      */
     bool mttcg_supported;
 
+    /**
+     * has_precise_smc: guest CPU has precise-SMC semantics
+     *
+     * Guest support for precise self modifying code even if the
+     * modified instruction is close to the modifying instruction.
+     */
+    bool has_precise_smc;
+
     /**
      * @guest_default_memory_order: default barrier that is required
      *                              for the guest memory ordering.
diff --git a/include/exec/poison.h b/include/exec/poison.h
index 413dfd16f24..011aa2378d7 100644
--- a/include/exec/poison.h
+++ b/include/exec/poison.h
@@ -36,7 +36,6 @@
 #pragma GCC poison TARGET_HAS_BFLT
 #pragma GCC poison TARGET_NAME
 #pragma GCC poison TARGET_BIG_ENDIAN
-#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 16d76df34b2..5a2e4a8103f 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 90f64ee20cc..ee59039879b 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/cpu-exec.c b/accel/tcg/cpu-exec.c
index cfe3b93e1e3..d410a4780b3 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -1065,19 +1065,17 @@ int cpu_exec(CPUState *cpu)
     return ret;
 }
 
+static bool tcg_target_initialized;
+static bool tcg_target_has_precise_smc;
+
 bool target_has_precise_smc(void)
 {
-#ifdef TARGET_HAS_PRECISE_SMC
-    return true;
-#else
-    return false;
-#endif
+    assert(tcg_target_initialized);
+    return tcg_target_has_precise_smc;
 }
 
 bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
 {
-    static bool tcg_target_initialized;
-
     if (!tcg_target_initialized) {
         /* Check mandatory TCGCPUOps handlers */
         const TCGCPUOps *tcg_ops = cpu->cc->tcg_ops;
@@ -1088,6 +1086,7 @@ bool tcg_exec_realizefn(CPUState *cpu, Error **errp)
         assert(tcg_ops->translate_code);
         assert(tcg_ops->mmu_index);
         tcg_ops->initialize();
+        tcg_target_has_precise_smc = tcg_ops->has_precise_smc;
         tcg_target_initialized = true;
     }
 
diff --git a/target/i386/tcg/tcg-cpu.c b/target/i386/tcg/tcg-cpu.c
index a0258f4739e..2254fc2d739 100644
--- a/target/i386/tcg/tcg-cpu.c
+++ b/target/i386/tcg/tcg-cpu.c
@@ -130,6 +130,7 @@ static const TCGCPUOps x86_tcg_ops = {
      * The x86 has a strong memory model with some store-after-load re-ordering
      */
     .guest_default_memory_order = TCG_MO_ALL & ~TCG_MO_ST_LD,
+    .has_precise_smc = true,
     .initialize = tcg_x86_init,
     .translate_code = x86_translate_code,
     .synchronize_from_tb = x86_cpu_synchronize_from_tb,
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 41cccc1e692..845b2515aeb 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -351,6 +351,7 @@ static const TCGCPUOps s390_tcg_ops = {
      * store-after-load re-ordering.
      */
     .guest_default_memory_order = TCG_MO_ALL & ~TCG_MO_ST_LD,
+    .has_precise_smc = true,
 
     .initialize = s390x_translate_init,
     .translate_code = s390x_translate_code,
-- 
2.47.1

Re: [PATCH-for-10.1 1/2] tcg: Introduce and use target_has_precise_smc() runtime helper

[Richard Henderson]

On 4/4/25 16:56, Philippe Mathieu-Daudé wrote:
> diff --git a/accel/tcg/tb-maint.c b/accel/tcg/tb-maint.c
> index d479f53ae02..ae12ad2d867 100644
> --- a/accel/tcg/tb-maint.c
> +++ b/accel/tcg/tb-maint.c
> @@ -1057,10 +1057,7 @@ bool tb_invalidate_phys_page_unwind(tb_page_addr_t addr, uintptr_t pc)
>        * 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 (!target_has_precise_smc() || !pc) {
>           tb_invalidate_phys_page(addr);
>           return false;
>       }

For the record, in my v2 I reverse these tests, since !pc is simpler.

> @@ -1109,10 +1106,9 @@ 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 = (target_has_precise_smc() && retaddr)
> +                                   ? tcg_tb_lookup(retaddr) : NULL;
>   
>       /* Range may not cross a page. */
>       tcg_debug_assert(((start ^ last) & TARGET_PAGE_MASK) == 0);
> @@ -1134,8 +1130,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 (target_has_precise_smc() && current_tb == tb &&
>                   (tb_cflags(current_tb) & CF_COUNT_MASK) != 1) {

For the record, we can eliminate the target_has_precise_smc() test here, because we've set 
current_tb == NULL, and thus the current_tb == tb test always fails ...

> @@ -1157,15 +1151,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 (target_has_precise_smc() && current_tb_modified) {

... which in turn means that current_tb_modified is never set.

Thus only the one runtime test at the top of the function suffices.


r~