Re: [PATCH 25/37] common-user: Split out watchpoint-stub.c

2025-03-15 Thread Pierrick Bouvier 

On 3/14/25 09:37, Richard Henderson wrote:

On 3/13/25 03:39, Philippe Mathieu-Daudé wrote:

--- /dev/null
+++ b/common-user/watchpoint-stub.c
@@ -0,0 +1,28 @@
+/*
+ * CPU watchpoint stubs
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+  int flags, CPUWatchpoint **watchpoint)
+{
+    return -ENOSYS;
+}
+
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, int flags)
+{
+    return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *wp)
+{


Again, can this be elide? Otherwise better use g_assert_not_reached().


We can, including:

-- >8 --
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index dba1b3ffef..54d3879c56 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7545,4 +7545,6 @@ static void x86_cpu_reset_hold(Object *obj, ResetType 
type)
   env->dr[7] = DR7_FIXED_1;
+#ifndef CONFIG_USER_ONLY
   cpu_breakpoint_remove_all(cs, BP_CPU);
   cpu_watchpoint_remove_all(cs, BP_CPU);
+#endif


But do we really want to add all those ifdefs?



I agree with Richard, trading ifdefs is not a win in the end.
Here, we replace header stubs (which are a problem, because they rely on 
ifdef by design) to different compilation units, which can be selected 
at link time.


In the end, we might even have to unify some stubs and their 
implementations, to have a single definition of those symbols.




r~

Re: [PATCH 25/37] common-user: Split out watchpoint-stub.c

2025-03-14 Thread Richard Henderson 

On 3/13/25 03:39, Philippe Mathieu-Daudé wrote:

--- /dev/null
+++ b/common-user/watchpoint-stub.c
@@ -0,0 +1,28 @@
+/*
+ * CPU watchpoint stubs
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+  int flags, CPUWatchpoint **watchpoint)
+{
+    return -ENOSYS;
+}
+
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, int flags)
+{
+    return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *wp)
+{


Again, can this be elide? Otherwise better use g_assert_not_reached().


We can, including:

-- >8 --
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index dba1b3ffef..54d3879c56 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7545,4 +7545,6 @@ static void x86_cpu_reset_hold(Object *obj, ResetType 
type)
  env->dr[7] = DR7_FIXED_1;
+#ifndef CONFIG_USER_ONLY
  cpu_breakpoint_remove_all(cs, BP_CPU);
  cpu_watchpoint_remove_all(cs, BP_CPU);
+#endif


But do we really want to add all those ifdefs?


r~

Re: [PATCH 25/37] common-user: Split out watchpoint-stub.c

2025-03-13 Thread Pierrick Bouvier 

On 3/12/25 20:45, Richard Henderson wrote:

Uninline the user-only stubs from hw/core/cpu.h.

Signed-off-by: Richard Henderson 
---
  include/hw/core/cpu.h | 23 ---
  common-user/watchpoint-stub.c | 28 
  common-user/meson.build   |  1 +
  3 files changed, 29 insertions(+), 23 deletions(-)
  create mode 100644 common-user/watchpoint-stub.c

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556..2fdb115b19 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1109,35 +1109,12 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, 
vaddr pc, int mask)
  return false;
  }
  
-#if defined(CONFIG_USER_ONLY)

-static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
-int flags, CPUWatchpoint **watchpoint)
-{
-return -ENOSYS;
-}
-
-static inline int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
-vaddr len, int flags)
-{
-return -ENOSYS;
-}
-
-static inline void cpu_watchpoint_remove_by_ref(CPUState *cpu,
-CPUWatchpoint *wp)
-{
-}
-
-static inline void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
-{
-}
-#else
  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
int flags, CPUWatchpoint **watchpoint);
  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
vaddr len, int flags);
  void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint);
  void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
-#endif
  
  /**

   * cpu_get_address_space:
diff --git a/common-user/watchpoint-stub.c b/common-user/watchpoint-stub.c
new file mode 100644
index 00..2489fca4f3
--- /dev/null
+++ b/common-user/watchpoint-stub.c
@@ -0,0 +1,28 @@
+/*
+ * CPU watchpoint stubs
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+  int flags, CPUWatchpoint **watchpoint)
+{
+return -ENOSYS;
+}
+
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, int flags)
+{
+return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *wp)
+{
+}
+
+void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
+{
+}
diff --git a/common-user/meson.build b/common-user/meson.build
index ac9de5b9e3..4dba482863 100644
--- a/common-user/meson.build
+++ b/common-user/meson.build
@@ -7,4 +7,5 @@ common_user_inc += include_directories('host/' / host_arch)
  user_ss.add(files(
'safe-syscall.S',
'safe-syscall-error.c',
+  'watchpoint-stub.c',
  ))


Reviewed-by: Pierrick Bouvier

Re: [PATCH 25/37] common-user: Split out watchpoint-stub.c

2025-03-13 Thread Philippe Mathieu-Daudé 

On 13/3/25 11:07, Philippe Mathieu-Daudé wrote:

On 13/3/25 04:45, Richard Henderson wrote:

Uninline the user-only stubs from hw/core/cpu.h.

Signed-off-by: Richard Henderson 
---
  include/hw/core/cpu.h | 23 ---
  common-user/watchpoint-stub.c | 28 
  common-user/meson.build   |  1 +
  3 files changed, 29 insertions(+), 23 deletions(-)
  create mode 100644 common-user/watchpoint-stub.c

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556..2fdb115b19 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1109,35 +1109,12 @@ static inline bool 
cpu_breakpoint_test(CPUState *cpu, vaddr pc, int mask)

  return false;
  }
-#if defined(CONFIG_USER_ONLY)
-static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, 
vaddr len,
-    int flags, CPUWatchpoint 
**watchpoint)

-{
-    return -ENOSYS;
-}
-
-static inline int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
-    vaddr len, int flags)
-{
-    return -ENOSYS;
-}
-
-static inline void cpu_watchpoint_remove_by_ref(CPUState *cpu,
-    CPUWatchpoint *wp)
-{
-}
-
-static inline void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
-{
-}
-#else
  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
    int flags, CPUWatchpoint **watchpoint);
  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
    vaddr len, int flags);
  void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint 
*watchpoint);

  void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
-#endif
  /**
   * cpu_get_address_space:
diff --git a/common-user/watchpoint-stub.c b/common-user/watchpoint- 
stub.c

new file mode 100644
index 00..2489fca4f3
--- /dev/null
+++ b/common-user/watchpoint-stub.c
@@ -0,0 +1,28 @@
+/*
+ * CPU watchpoint stubs
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+  int flags, CPUWatchpoint **watchpoint)
+{
+    return -ENOSYS;
+}
+
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, int 
flags)

+{
+    return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *wp)
+{


Again, can this be elide? Otherwise better use g_assert_not_reached().


We can, including:

-- >8 --
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index dba1b3ffef..54d3879c56 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7545,4 +7545,6 @@ static void x86_cpu_reset_hold(Object *obj, 
ResetType type)

 env->dr[7] = DR7_FIXED_1;
+#ifndef CONFIG_USER_ONLY
 cpu_breakpoint_remove_all(cs, BP_CPU);
 cpu_watchpoint_remove_all(cs, BP_CPU);
+#endif

diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c
index a9a619ba6b..f798569de7 100644
--- a/target/arm/debug_helper.c
+++ b/target/arm/debug_helper.c
@@ -368,2 +368,3 @@ static bool check_watchpoints(ARMCPU *cpu)

+#ifndef CONFIG_USER_ONLY
 for (n = 0; n < ARRAY_SIZE(env->cpu_watchpoint); n++) {
@@ -373,2 +374,3 @@ static bool check_watchpoints(ARMCPU *cpu)
 }
+#endif
 return false;
@@ -555,2 +557,3 @@ void hw_watchpoint_update(ARMCPU *cpu, int n)

+#ifndef CONFIG_USER_ONLY
 if (env->cpu_watchpoint[n]) {
@@ -559,2 +562,3 @@ void hw_watchpoint_update(ARMCPU *cpu, int n)
 }
+#endif

@@ -631,4 +635,6 @@ void hw_watchpoint_update(ARMCPU *cpu, int n)

+#ifndef CONFIG_USER_ONLY
 cpu_watchpoint_insert(CPU(cpu), wvr, len, flags,
   &env->cpu_watchpoint[n]);
+#endif
 }
@@ -637,3 +643,3 @@ void hw_watchpoint_update_all(ARMCPU *cpu)
 {
-int i;
+#ifndef CONFIG_USER_ONLY
 CPUARMState *env = &cpu->env;
@@ -646,4 +652,5 @@ void hw_watchpoint_update_all(ARMCPU *cpu)
 memset(env->cpu_watchpoint, 0, sizeof(env->cpu_watchpoint));
+#endif

-for (i = 0; i < ARRAY_SIZE(cpu->env.cpu_watchpoint); i++) {
+for (unsigned i = 0; i < ARRAY_SIZE(cpu->env.cpu_watchpoint); i++) {
 hw_watchpoint_update(cpu, i);
---

Re: [PATCH 25/37] common-user: Split out watchpoint-stub.c

2025-03-13 Thread Philippe Mathieu-Daudé 

On 13/3/25 04:45, Richard Henderson wrote:

Uninline the user-only stubs from hw/core/cpu.h.

Signed-off-by: Richard Henderson 
---
  include/hw/core/cpu.h | 23 ---
  common-user/watchpoint-stub.c | 28 
  common-user/meson.build   |  1 +
  3 files changed, 29 insertions(+), 23 deletions(-)
  create mode 100644 common-user/watchpoint-stub.c

diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 5d11d26556..2fdb115b19 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -1109,35 +1109,12 @@ static inline bool cpu_breakpoint_test(CPUState *cpu, 
vaddr pc, int mask)
  return false;
  }
  
-#if defined(CONFIG_USER_ONLY)

-static inline int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
-int flags, CPUWatchpoint **watchpoint)
-{
-return -ENOSYS;
-}
-
-static inline int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
-vaddr len, int flags)
-{
-return -ENOSYS;
-}
-
-static inline void cpu_watchpoint_remove_by_ref(CPUState *cpu,
-CPUWatchpoint *wp)
-{
-}
-
-static inline void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
-{
-}
-#else
  int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
int flags, CPUWatchpoint **watchpoint);
  int cpu_watchpoint_remove(CPUState *cpu, vaddr addr,
vaddr len, int flags);
  void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *watchpoint);
  void cpu_watchpoint_remove_all(CPUState *cpu, int mask);
-#endif
  
  /**

   * cpu_get_address_space:
diff --git a/common-user/watchpoint-stub.c b/common-user/watchpoint-stub.c
new file mode 100644
index 00..2489fca4f3
--- /dev/null
+++ b/common-user/watchpoint-stub.c
@@ -0,0 +1,28 @@
+/*
+ * CPU watchpoint stubs
+ *
+ * Copyright (c) 2003 Fabrice Bellard
+ * SPDX-License-Identifier: LGPL-2.1-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu.h"
+
+int cpu_watchpoint_insert(CPUState *cpu, vaddr addr, vaddr len,
+  int flags, CPUWatchpoint **watchpoint)
+{
+return -ENOSYS;
+}
+
+int cpu_watchpoint_remove(CPUState *cpu, vaddr addr, vaddr len, int flags)
+{
+return -ENOSYS;
+}
+
+void cpu_watchpoint_remove_by_ref(CPUState *cpu, CPUWatchpoint *wp)
+{


Again, can this be elide? Otherwise better use g_assert_not_reached().


+}
+
+void cpu_watchpoint_remove_all(CPUState *cpu, int mask)
+{
+}