Message ID | 20250313034524.3069690-26-richard.henderson@linaro.org |
---|---|
State | New |
Headers | show |
Series | accel/tcg, codebase: Build once patches | expand |
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) > +{ > +}
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); ---
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>
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~
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 --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', ))
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