diff mbox series

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

Message ID 20250313034524.3069690-26-richard.henderson@linaro.org
State New
Headers show
Series accel/tcg, codebase: Build once patches | expand

Commit Message

Richard Henderson March 13, 2025, 3:45 a.m. UTC 
Uninline the user-only stubs from hw/core/cpu.h.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 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

Comments

Philippe Mathieu-Daudé March 13, 2025, 10:07 a.m. UTC | #1 
On 13/3/25 04:45, Richard Henderson wrote:
> Uninline the user-only stubs from hw/core/cpu.h.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   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 0000000000..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)
> +{
> +}
Philippe Mathieu-Daudé March 13, 2025, 10:39 a.m. UTC | #2 
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 <richard.henderson@linaro.org>
>> ---
>>   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 0000000000..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);
---
Pierrick Bouvier March 13, 2025, 8:57 p.m. UTC | #3 
On 3/12/25 20:45, Richard Henderson wrote:
> Uninline the user-only stubs from hw/core/cpu.h.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   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 0000000000..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 <pierrick.bouvier@linaro.org>
Richard Henderson March 14, 2025, 4:37 p.m. UTC | #4 
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~
Pierrick Bouvier March 14, 2025, 4:53 p.m. UTC | #5 
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~
diff mbox series

Patch

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 0000000000..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',
 ))