[PATCH 3/5] target/s390x: Restrict WatchPoint API to TCG

Philippe Mathieu-Daudé posted 5 patches 1 month ago
Maintainers: Richard Henderson <richard.henderson@linaro.org>, Paolo Bonzini <pbonzini@redhat.com>, Riku Voipio <riku.voipio@iki.fi>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, Peter Maydell <peter.maydell@linaro.org>, Nicholas Piggin <npiggin@gmail.com>, Chinmay Rath <rathc@linux.ibm.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Ilya Leoshkevich <iii@linux.ibm.com>, David Hildenbrand <david@kernel.org>, Thomas Huth <thuth@redhat.com>, Max Filippov <jcmvbkbc@gmail.com>
[PATCH 3/5] target/s390x: Restrict WatchPoint API to TCG
Posted by Philippe Mathieu-Daudé 1 month ago
By inverting the 'tcg_enabled()' check in s390_cpu_set_psw()
we can let the compiler elide the s390_cpu_recompute_watchpoints()
call when TCG is not available. Move it to a TCG specific
file to avoid compiling dead code on KVM. This restricts the
WatchPoint API calls to TCG.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 target/s390x/s390x-internal.h |  1 -
 target/s390x/tcg/tcg_s390x.h  |  4 +++
 target/s390x/cpu.c            | 30 ++++++++++----------
 target/s390x/helper.c         | 38 -------------------------
 target/s390x/tcg/debug.c      | 53 +++++++++++++++++++++++++++++++++++
 target/s390x/tcg/meson.build  |  3 ++
 6 files changed, 75 insertions(+), 54 deletions(-)
 create mode 100644 target/s390x/tcg/debug.c

diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
index 9691366ec91..c4cd17d4d7d 100644
--- a/target/s390x/s390x-internal.h
+++ b/target/s390x/s390x-internal.h
@@ -317,7 +317,6 @@ void s390_cpu_gdb_init(CPUState *cs);
 void s390_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
 void do_restart_interrupt(CPUS390XState *env);
 #ifndef CONFIG_USER_ONLY
-void s390_cpu_recompute_watchpoints(CPUState *cs);
 void s390x_tod_timer(void *opaque);
 void s390x_cpu_timer(void *opaque);
 void s390_handle_wait(S390CPU *cpu);
diff --git a/target/s390x/tcg/tcg_s390x.h b/target/s390x/tcg/tcg_s390x.h
index 78558912f99..33f26f26c0f 100644
--- a/target/s390x/tcg/tcg_s390x.h
+++ b/target/s390x/tcg/tcg_s390x.h
@@ -21,4 +21,8 @@ G_NORETURN void tcg_s390_data_exception(CPUS390XState *env, uint32_t dxc,
 G_NORETURN void tcg_s390_vector_exception(CPUS390XState *env, uint32_t vxc,
                                           uintptr_t ra);
 
+#ifndef CONFIG_USER_ONLY
+void s390_cpu_recompute_watchpoints(CPUState *cs);
+#endif
+
 #endif /* TCG_S390X_H */
diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
index 6c4198eb1b1..f68b288e364 100644
--- a/target/s390x/cpu.c
+++ b/target/s390x/cpu.c
@@ -40,6 +40,7 @@
 #include "system/reset.h"
 #endif
 #include "hw/s390x/cpu-topology.h"
+#include "tcg/tcg_s390x.h"
 
 #define CR0_RESET       0xE0UL
 #define CR14_RESET      0xC2000000UL;
@@ -74,26 +75,25 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
     env->psw.mask = mask;
 
     /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
-    if (!tcg_enabled()) {
-        return;
-    }
-    env->cc_op = (mask >> 44) & 3;
+    if (tcg_enabled()) {
+        env->cc_op = (mask >> 44) & 3;
 
 #ifndef CONFIG_USER_ONLY
-    if (is_early_exception_psw(mask, addr)) {
-        env->int_pgm_ilen = 0;
-        trigger_pgm_exception(env, PGM_SPECIFICATION);
-        return;
-    }
+        if (is_early_exception_psw(mask, addr)) {
+            env->int_pgm_ilen = 0;
+            trigger_pgm_exception(env, PGM_SPECIFICATION);
+            return;
+        }
 
-    if ((old_mask ^ mask) & PSW_MASK_PER) {
-        s390_cpu_recompute_watchpoints(env_cpu(env));
-    }
+        if ((old_mask ^ mask) & PSW_MASK_PER) {
+            s390_cpu_recompute_watchpoints(env_cpu(env));
+        }
 
-    if (mask & PSW_MASK_WAIT) {
-        s390_handle_wait(env_archcpu(env));
-    }
+        if (mask & PSW_MASK_WAIT) {
+            s390_handle_wait(env_archcpu(env));
+        }
 #endif
+    }
 }
 
 uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
diff --git a/target/s390x/helper.c b/target/s390x/helper.c
index 184428c6d9d..8d1e03f6768 100644
--- a/target/s390x/helper.c
+++ b/target/s390x/helper.c
@@ -143,41 +143,3 @@ void do_restart_interrupt(CPUS390XState *env)
 
     s390_cpu_set_psw(env, mask, addr);
 }
-
-void s390_cpu_recompute_watchpoints(CPUState *cs)
-{
-    const int wp_flags = BP_CPU | BP_MEM_WRITE | BP_STOP_BEFORE_ACCESS;
-    CPUS390XState *env = cpu_env(cs);
-
-    /* We are called when the watchpoints have changed. First
-       remove them all.  */
-    cpu_watchpoint_remove_all(cs, BP_CPU);
-
-    /* Return if PER is not enabled */
-    if (!(env->psw.mask & PSW_MASK_PER)) {
-        return;
-    }
-
-    /* Return if storage-alteration event is not enabled.  */
-    if (!(env->cregs[9] & PER_CR9_EVENT_STORE)) {
-        return;
-    }
-
-    if (env->cregs[10] == 0 && env->cregs[11] == -1LL) {
-        /* We can't create a watchoint spanning the whole memory range, so
-           split it in two parts.   */
-        cpu_watchpoint_insert(cs, 0, 1ULL << 63, wp_flags, NULL);
-        cpu_watchpoint_insert(cs, 1ULL << 63, 1ULL << 63, wp_flags, NULL);
-    } else if (env->cregs[10] > env->cregs[11]) {
-        /* The address range loops, create two watchpoints.  */
-        cpu_watchpoint_insert(cs, env->cregs[10], -env->cregs[10],
-                              wp_flags, NULL);
-        cpu_watchpoint_insert(cs, 0, env->cregs[11] + 1, wp_flags, NULL);
-
-    } else {
-        /* Default case, create a single watchpoint.  */
-        cpu_watchpoint_insert(cs, env->cregs[10],
-                              env->cregs[11] - env->cregs[10] + 1,
-                              wp_flags, NULL);
-    }
-}
diff --git a/target/s390x/tcg/debug.c b/target/s390x/tcg/debug.c
new file mode 100644
index 00000000000..12ae95d4fe8
--- /dev/null
+++ b/target/s390x/tcg/debug.c
@@ -0,0 +1,53 @@
+/*
+ * QEMU S/390 debug routines
+ *
+ * Copyright (c) 2009 Ulrich Hecht
+ * Copyright (c) 2011 Alexander Graf
+ * Copyright (c) 2012 SUSE LINUX Products GmbH
+ * Copyright (c) 2012 IBM Corp.
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "exec/watchpoint.h"
+#include "target/s390x/cpu.h"
+#include "tcg_s390x.h"
+
+void s390_cpu_recompute_watchpoints(CPUState *cs)
+{
+    const int wp_flags = BP_CPU | BP_MEM_WRITE | BP_STOP_BEFORE_ACCESS;
+    CPUS390XState *env = cpu_env(cs);
+
+    /* We are called when the watchpoints have changed. First
+       remove them all.  */
+    cpu_watchpoint_remove_all(cs, BP_CPU);
+
+    /* Return if PER is not enabled */
+    if (!(env->psw.mask & PSW_MASK_PER)) {
+        return;
+    }
+
+    /* Return if storage-alteration event is not enabled.  */
+    if (!(env->cregs[9] & PER_CR9_EVENT_STORE)) {
+        return;
+    }
+
+    if (env->cregs[10] == 0 && env->cregs[11] == -1LL) {
+        /* We can't create a watchoint spanning the whole memory range, so
+           split it in two parts.   */
+        cpu_watchpoint_insert(cs, 0, 1ULL << 63, wp_flags, NULL);
+        cpu_watchpoint_insert(cs, 1ULL << 63, 1ULL << 63, wp_flags, NULL);
+    } else if (env->cregs[10] > env->cregs[11]) {
+        /* The address range loops, create two watchpoints.  */
+        cpu_watchpoint_insert(cs, env->cregs[10], -env->cregs[10],
+                              wp_flags, NULL);
+        cpu_watchpoint_insert(cs, 0, env->cregs[11] + 1, wp_flags, NULL);
+
+    } else {
+        /* Default case, create a single watchpoint.  */
+        cpu_watchpoint_insert(cs, env->cregs[10],
+                              env->cregs[11] - env->cregs[10] + 1,
+                              wp_flags, NULL);
+    }
+}
diff --git a/target/s390x/tcg/meson.build b/target/s390x/tcg/meson.build
index ee4e8fec77c..515cb8b473d 100644
--- a/target/s390x/tcg/meson.build
+++ b/target/s390x/tcg/meson.build
@@ -12,3 +12,6 @@ s390x_ss.add(when: 'CONFIG_TCG', if_true: files(
   'vec_int_helper.c',
   'vec_string_helper.c',
 ))
+s390x_system_ss.add(when: 'CONFIG_TCG', if_true: files(
+  'debug.c',
+))
-- 
2.52.0


Re: [PATCH 3/5] target/s390x: Restrict WatchPoint API to TCG
Posted by Pierrick Bouvier 1 month ago
On 1/6/26 3:19 PM, Philippe Mathieu-Daudé wrote:
> By inverting the 'tcg_enabled()' check in s390_cpu_set_psw()
> we can let the compiler elide the s390_cpu_recompute_watchpoints()
> call when TCG is not available. Move it to a TCG specific
> file to avoid compiling dead code on KVM. This restricts the
> WatchPoint API calls to TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h |  1 -
>   target/s390x/tcg/tcg_s390x.h  |  4 +++
>   target/s390x/cpu.c            | 30 ++++++++++----------
>   target/s390x/helper.c         | 38 -------------------------
>   target/s390x/tcg/debug.c      | 53 +++++++++++++++++++++++++++++++++++
>   target/s390x/tcg/meson.build  |  3 ++
>   6 files changed, 75 insertions(+), 54 deletions(-)
>   create mode 100644 target/s390x/tcg/debug.c
> 

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Re: [PATCH 3/5] target/s390x: Restrict WatchPoint API to TCG
Posted by Thomas Huth 1 month ago
On 07/01/2026 00.19, Philippe Mathieu-Daudé wrote:
> By inverting the 'tcg_enabled()' check in s390_cpu_set_psw()
> we can let the compiler elide the s390_cpu_recompute_watchpoints()
> call when TCG is not available. Move it to a TCG specific
> file to avoid compiling dead code on KVM. This restricts the
> WatchPoint API calls to TCG.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   target/s390x/s390x-internal.h |  1 -
>   target/s390x/tcg/tcg_s390x.h  |  4 +++
>   target/s390x/cpu.c            | 30 ++++++++++----------
>   target/s390x/helper.c         | 38 -------------------------
>   target/s390x/tcg/debug.c      | 53 +++++++++++++++++++++++++++++++++++
>   target/s390x/tcg/meson.build  |  3 ++
>   6 files changed, 75 insertions(+), 54 deletions(-)
>   create mode 100644 target/s390x/tcg/debug.c
> 
> diff --git a/target/s390x/s390x-internal.h b/target/s390x/s390x-internal.h
> index 9691366ec91..c4cd17d4d7d 100644
> --- a/target/s390x/s390x-internal.h
> +++ b/target/s390x/s390x-internal.h
> @@ -317,7 +317,6 @@ void s390_cpu_gdb_init(CPUState *cs);
>   void s390_cpu_dump_state(CPUState *cpu, FILE *f, int flags);
>   void do_restart_interrupt(CPUS390XState *env);
>   #ifndef CONFIG_USER_ONLY
> -void s390_cpu_recompute_watchpoints(CPUState *cs);
>   void s390x_tod_timer(void *opaque);
>   void s390x_cpu_timer(void *opaque);
>   void s390_handle_wait(S390CPU *cpu);
> diff --git a/target/s390x/tcg/tcg_s390x.h b/target/s390x/tcg/tcg_s390x.h
> index 78558912f99..33f26f26c0f 100644
> --- a/target/s390x/tcg/tcg_s390x.h
> +++ b/target/s390x/tcg/tcg_s390x.h
> @@ -21,4 +21,8 @@ G_NORETURN void tcg_s390_data_exception(CPUS390XState *env, uint32_t dxc,
>   G_NORETURN void tcg_s390_vector_exception(CPUS390XState *env, uint32_t vxc,
>                                             uintptr_t ra);
>   
> +#ifndef CONFIG_USER_ONLY
> +void s390_cpu_recompute_watchpoints(CPUState *cs);
> +#endif
> +
>   #endif /* TCG_S390X_H */
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index 6c4198eb1b1..f68b288e364 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -40,6 +40,7 @@
>   #include "system/reset.h"
>   #endif
>   #include "hw/s390x/cpu-topology.h"
> +#include "tcg/tcg_s390x.h"
>   
>   #define CR0_RESET       0xE0UL
>   #define CR14_RESET      0xC2000000UL;
> @@ -74,26 +75,25 @@ void s390_cpu_set_psw(CPUS390XState *env, uint64_t mask, uint64_t addr)
>       env->psw.mask = mask;
>   
>       /* KVM will handle all WAITs and trigger a WAIT exit on disabled_wait */
> -    if (!tcg_enabled()) {
> -        return;
> -    }
> -    env->cc_op = (mask >> 44) & 3;
> +    if (tcg_enabled()) {
> +        env->cc_op = (mask >> 44) & 3;
>   
>   #ifndef CONFIG_USER_ONLY
> -    if (is_early_exception_psw(mask, addr)) {
> -        env->int_pgm_ilen = 0;
> -        trigger_pgm_exception(env, PGM_SPECIFICATION);
> -        return;
> -    }
> +        if (is_early_exception_psw(mask, addr)) {
> +            env->int_pgm_ilen = 0;
> +            trigger_pgm_exception(env, PGM_SPECIFICATION);
> +            return;
> +        }
>   
> -    if ((old_mask ^ mask) & PSW_MASK_PER) {
> -        s390_cpu_recompute_watchpoints(env_cpu(env));
> -    }
> +        if ((old_mask ^ mask) & PSW_MASK_PER) {
> +            s390_cpu_recompute_watchpoints(env_cpu(env));
> +        }
>   
> -    if (mask & PSW_MASK_WAIT) {
> -        s390_handle_wait(env_archcpu(env));
> -    }
> +        if (mask & PSW_MASK_WAIT) {
> +            s390_handle_wait(env_archcpu(env));
> +        }
>   #endif
> +    }
>   }
>   
>   uint64_t s390_cpu_get_psw_mask(CPUS390XState *env)
> diff --git a/target/s390x/helper.c b/target/s390x/helper.c
> index 184428c6d9d..8d1e03f6768 100644
> --- a/target/s390x/helper.c
> +++ b/target/s390x/helper.c
> @@ -143,41 +143,3 @@ void do_restart_interrupt(CPUS390XState *env)
>   
>       s390_cpu_set_psw(env, mask, addr);
>   }
> -
> -void s390_cpu_recompute_watchpoints(CPUState *cs)
> -{
> -    const int wp_flags = BP_CPU | BP_MEM_WRITE | BP_STOP_BEFORE_ACCESS;
> -    CPUS390XState *env = cpu_env(cs);
> -
> -    /* We are called when the watchpoints have changed. First
> -       remove them all.  */
> -    cpu_watchpoint_remove_all(cs, BP_CPU);
> -
> -    /* Return if PER is not enabled */
> -    if (!(env->psw.mask & PSW_MASK_PER)) {
> -        return;
> -    }
> -
> -    /* Return if storage-alteration event is not enabled.  */
> -    if (!(env->cregs[9] & PER_CR9_EVENT_STORE)) {
> -        return;
> -    }
> -
> -    if (env->cregs[10] == 0 && env->cregs[11] == -1LL) {
> -        /* We can't create a watchoint spanning the whole memory range, so
> -           split it in two parts.   */
> -        cpu_watchpoint_insert(cs, 0, 1ULL << 63, wp_flags, NULL);
> -        cpu_watchpoint_insert(cs, 1ULL << 63, 1ULL << 63, wp_flags, NULL);
> -    } else if (env->cregs[10] > env->cregs[11]) {
> -        /* The address range loops, create two watchpoints.  */
> -        cpu_watchpoint_insert(cs, env->cregs[10], -env->cregs[10],
> -                              wp_flags, NULL);
> -        cpu_watchpoint_insert(cs, 0, env->cregs[11] + 1, wp_flags, NULL);
> -
> -    } else {
> -        /* Default case, create a single watchpoint.  */
> -        cpu_watchpoint_insert(cs, env->cregs[10],
> -                              env->cregs[11] - env->cregs[10] + 1,
> -                              wp_flags, NULL);
> -    }
> -}
> diff --git a/target/s390x/tcg/debug.c b/target/s390x/tcg/debug.c
> new file mode 100644
> index 00000000000..12ae95d4fe8
> --- /dev/null
> +++ b/target/s390x/tcg/debug.c
> @@ -0,0 +1,53 @@
> +/*
> + * QEMU S/390 debug routines
> + *
> + * Copyright (c) 2009 Ulrich Hecht
> + * Copyright (c) 2011 Alexander Graf
> + * Copyright (c) 2012 SUSE LINUX Products GmbH
> + * Copyright (c) 2012 IBM Corp.

Looking at "git blame," the code has been introduced in commit 
311918b979c5364c30392c1054ed77d047a83953, so none of the above 
people/companies has been involved.

That original commit also introduced another function called 
s390x_cpu_debug_excp_handler() which now resides in 
target/s390x/tcg/excp_helper.c ... so maybe s390_cpu_recompute_watchpoints() 
should simply be moved there, too, instead?

  Thomas