| Message ID | 20250418005059.4436-3-philmd@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | single-binary: Make hw/arm/ common | expand |
On 4/17/25 17:50, Philippe Mathieu-Daudé wrote: > Have target_name() be a target-agnostic method, dispatching > to a per-target TargetInfo singleton structure. > By default a stub singleton is used. No logical change > expected. > > Inspired-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> > --- > meson.build | 3 +++ > include/hw/core/cpu.h | 2 -- > include/qemu/target_info-impl.h | 23 +++++++++++++++++++++++ > include/qemu/target_info.h | 19 +++++++++++++++++++ > cpu-target.c | 5 ----- > hw/core/machine-qmp-cmds.c | 1 + > plugins/loader.c | 2 +- > system/vl.c | 2 +- > target_info-stub.c | 19 +++++++++++++++++++ > target_info.c | 16 ++++++++++++++++ > 10 files changed, 83 insertions(+), 9 deletions(-) > create mode 100644 include/qemu/target_info-impl.h > create mode 100644 include/qemu/target_info.h > create mode 100644 target_info-stub.c > create mode 100644 target_info.c > > diff --git a/meson.build b/meson.build > index bcb9d39a387..49a050a28a3 100644 > --- a/meson.build > +++ b/meson.build > @@ -3807,6 +3807,9 @@ endif > common_ss.add(pagevary) > specific_ss.add(files('page-target.c', 'page-vary-target.c')) > > +common_ss.add(files('target_info.c')) > +specific_ss.add(files('target_info-stub.c')) > + > subdir('backends') > subdir('disas') > subdir('migration') > diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h > index 5b645df59f5..9d9448341d1 100644 > --- a/include/hw/core/cpu.h > +++ b/include/hw/core/cpu.h > @@ -1115,8 +1115,6 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp); > void cpu_exec_unrealizefn(CPUState *cpu); > void cpu_exec_reset_hold(CPUState *cpu); > > -const char *target_name(void); > - > #ifdef COMPILING_PER_TARGET > > extern const VMStateDescription vmstate_cpu_common; > diff --git a/include/qemu/target_info-impl.h b/include/qemu/target_info-impl.h > new file mode 100644 > index 00000000000..d5c94ed5296 > --- /dev/null > +++ b/include/qemu/target_info-impl.h > @@ -0,0 +1,23 @@ > +/* > + * QEMU binary/target API ... > + * > + * Copyright (c) Linaro > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#ifndef QEMU_TARGET_INFO_IMPL_H > +#define QEMU_TARGET_INFO_IMPL_H > + > +#include "qemu/target_info.h" > + > +typedef struct TargetInfo { > + > + /* runtime equivalent of TARGET_NAME definition */ > + const char *const name; > + > +} TargetInfo; > + > +const TargetInfo *target_info(void); > + > +#endif > diff --git a/include/qemu/target_info.h b/include/qemu/target_info.h > new file mode 100644 > index 00000000000..3f6cbbbd300 > --- /dev/null > +++ b/include/qemu/target_info.h > @@ -0,0 +1,19 @@ > +/* > + * QEMU binary/target API > + * > + * Copyright (c) Linaro > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#ifndef QEMU_TARGET_INFO_H > +#define QEMU_TARGET_INFO_H > + > +/** > + * target_name: > + * > + * Returns: Canonical target name (i.e. "i386"). > + */ > +const char *target_name(void); > + > +#endif > diff --git a/cpu-target.c b/cpu-target.c > index c99d208a7c4..3f82d3ea444 100644 > --- a/cpu-target.c > +++ b/cpu-target.c > @@ -165,8 +165,3 @@ bool target_words_bigendian(void) > { > return TARGET_BIG_ENDIAN; > } > - > -const char *target_name(void) > -{ > - return TARGET_NAME; > -} > diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c > index 408994b67d7..b317aec234f 100644 > --- a/hw/core/machine-qmp-cmds.c > +++ b/hw/core/machine-qmp-cmds.c > @@ -19,6 +19,7 @@ > #include "qapi/qobject-input-visitor.h" > #include "qapi/type-helpers.h" > #include "qemu/uuid.h" > +#include "qemu/target_info.h" > #include "qom/qom-qobject.h" > #include "system/hostmem.h" > #include "system/hw_accel.h" > diff --git a/plugins/loader.c b/plugins/loader.c > index 7523d554f03..36a4e88d4db 100644 > --- a/plugins/loader.c > +++ b/plugins/loader.c > @@ -29,7 +29,7 @@ > #include "qemu/xxhash.h" > #include "qemu/plugin.h" > #include "qemu/memalign.h" > -#include "hw/core/cpu.h" > +#include "qemu/target_info.h" > #include "exec/tb-flush.h" > > #include "plugin.h" > diff --git a/system/vl.c b/system/vl.c > index c17945c4939..d8a0fe713c9 100644 > --- a/system/vl.c > +++ b/system/vl.c > @@ -40,6 +40,7 @@ > #include "qemu/help_option.h" > #include "qemu/hw-version.h" > #include "qemu/uuid.h" > +#include "qemu/target_info.h" > #include "system/reset.h" > #include "system/runstate.h" > #include "system/runstate-action.h" > @@ -79,7 +80,6 @@ > #include "hw/block/block.h" > #include "hw/i386/x86.h" > #include "hw/i386/pc.h" > -#include "hw/core/cpu.h" > #include "migration/cpr.h" > #include "migration/misc.h" > #include "migration/snapshot.h" > diff --git a/target_info-stub.c b/target_info-stub.c > new file mode 100644 > index 00000000000..1e44bb6f6fb > --- /dev/null > +++ b/target_info-stub.c > @@ -0,0 +1,19 @@ > +/* > + * QEMU target info stubs > + * > + * Copyright (c) Linaro > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/target_info-impl.h" > + > +static const TargetInfo target_info_stub = { > + .name = TARGET_NAME, > +}; > + > +const TargetInfo *target_info(void) > +{ > + return &target_info_stub; > +} > diff --git a/target_info.c b/target_info.c > new file mode 100644 > index 00000000000..877a6a15014 > --- /dev/null > +++ b/target_info.c > @@ -0,0 +1,16 @@ > +/* > + * QEMU binary/target helpers > + * > + * Copyright (c) Linaro > + * > + * SPDX-License-Identifier: GPL-2.0-or-later > + */ > + > +#include "qemu/osdep.h" > +#include "qemu/target_info-impl.h" > +#include "qemu/target_info.h" > + > +const char *target_name(void) > +{ > + return target_info()->name; > +} What is the benefit to have two different files (common and specific)? target_name() can be inline in the same header, returning the matching field in existing target_info, which does not need any specialization per target.
On 18/4/25 05:01, Pierrick Bouvier wrote: > On 4/17/25 17:50, Philippe Mathieu-Daudé wrote: >> Have target_name() be a target-agnostic method, dispatching >> to a per-target TargetInfo singleton structure. >> By default a stub singleton is used. No logical change >> expected. >> >> Inspired-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >> --- >> meson.build | 3 +++ >> include/hw/core/cpu.h | 2 -- >> include/qemu/target_info-impl.h | 23 +++++++++++++++++++++++ >> include/qemu/target_info.h | 19 +++++++++++++++++++ >> cpu-target.c | 5 ----- >> hw/core/machine-qmp-cmds.c | 1 + >> plugins/loader.c | 2 +- >> system/vl.c | 2 +- >> target_info-stub.c | 19 +++++++++++++++++++ >> target_info.c | 16 ++++++++++++++++ >> 10 files changed, 83 insertions(+), 9 deletions(-) >> create mode 100644 include/qemu/target_info-impl.h >> create mode 100644 include/qemu/target_info.h >> create mode 100644 target_info-stub.c >> create mode 100644 target_info.c >> diff --git a/target_info-stub.c b/target_info-stub.c >> new file mode 100644 >> index 00000000000..1e44bb6f6fb >> --- /dev/null >> +++ b/target_info-stub.c >> @@ -0,0 +1,19 @@ >> +/* >> + * QEMU target info stubs >> + * >> + * Copyright (c) Linaro >> + * >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/target_info-impl.h" >> + >> +static const TargetInfo target_info_stub = { >> + .name = TARGET_NAME, >> +}; >> + >> +const TargetInfo *target_info(void) >> +{ >> + return &target_info_stub; >> +} >> diff --git a/target_info.c b/target_info.c >> new file mode 100644 >> index 00000000000..877a6a15014 >> --- /dev/null >> +++ b/target_info.c >> @@ -0,0 +1,16 @@ >> +/* >> + * QEMU binary/target helpers >> + * >> + * Copyright (c) Linaro >> + * >> + * SPDX-License-Identifier: GPL-2.0-or-later >> + */ >> + >> +#include "qemu/osdep.h" >> +#include "qemu/target_info-impl.h" >> +#include "qemu/target_info.h" >> + >> +const char *target_name(void) >> +{ >> + return target_info()->name; >> +} > > What is the benefit to have two different files (common and specific)? > target_name() can be inline in the same header, returning the matching > field in existing target_info, which does not need any specialization > per target. common interface exposed target-agnostic, dispatching to target-specific implementation (providing a stub we'll remove once all targets converted). What would you suggest?
On 4/18/25 07:02, Philippe Mathieu-Daudé wrote: > On 18/4/25 05:01, Pierrick Bouvier wrote: >> On 4/17/25 17:50, Philippe Mathieu-Daudé wrote: >>> Have target_name() be a target-agnostic method, dispatching >>> to a per-target TargetInfo singleton structure. >>> By default a stub singleton is used. No logical change >>> expected. >>> >>> Inspired-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>> --- >>> meson.build | 3 +++ >>> include/hw/core/cpu.h | 2 -- >>> include/qemu/target_info-impl.h | 23 +++++++++++++++++++++++ >>> include/qemu/target_info.h | 19 +++++++++++++++++++ >>> cpu-target.c | 5 ----- >>> hw/core/machine-qmp-cmds.c | 1 + >>> plugins/loader.c | 2 +- >>> system/vl.c | 2 +- >>> target_info-stub.c | 19 +++++++++++++++++++ >>> target_info.c | 16 ++++++++++++++++ >>> 10 files changed, 83 insertions(+), 9 deletions(-) >>> create mode 100644 include/qemu/target_info-impl.h >>> create mode 100644 include/qemu/target_info.h >>> create mode 100644 target_info-stub.c >>> create mode 100644 target_info.c > > >>> diff --git a/target_info-stub.c b/target_info-stub.c >>> new file mode 100644 >>> index 00000000000..1e44bb6f6fb >>> --- /dev/null >>> +++ b/target_info-stub.c >>> @@ -0,0 +1,19 @@ >>> +/* >>> + * QEMU target info stubs >>> + * >>> + * Copyright (c) Linaro >>> + * >>> + * SPDX-License-Identifier: GPL-2.0-or-later >>> + */ >>> + >>> +#include "qemu/osdep.h" >>> +#include "qemu/target_info-impl.h" >>> + >>> +static const TargetInfo target_info_stub = { >>> + .name = TARGET_NAME, >>> +}; >>> + >>> +const TargetInfo *target_info(void) >>> +{ >>> + return &target_info_stub; >>> +} >>> diff --git a/target_info.c b/target_info.c >>> new file mode 100644 >>> index 00000000000..877a6a15014 >>> --- /dev/null >>> +++ b/target_info.c >>> @@ -0,0 +1,16 @@ >>> +/* >>> + * QEMU binary/target helpers >>> + * >>> + * Copyright (c) Linaro >>> + * >>> + * SPDX-License-Identifier: GPL-2.0-or-later >>> + */ >>> + >>> +#include "qemu/osdep.h" >>> +#include "qemu/target_info-impl.h" >>> +#include "qemu/target_info.h" >>> + >>> +const char *target_name(void) >>> +{ >>> + return target_info()->name; >>> +} >> >> What is the benefit to have two different files (common and specific)? >> target_name() can be inline in the same header, returning the matching >> field in existing target_info, which does not need any specialization >> per target. > > common interface exposed target-agnostic, dispatching to target-specific > implementation (providing a stub we'll remove once all targets converted). > > What would you suggest? To remove target_info.c and target_info-impl.h, and implement target_name() as a static inline in target_info.h. This way, target_info.h can still be included from common code, the structure is directly accessed, and we have a single header where we can add new sugar functions and associated fields later.
On 18/4/25 18:23, Pierrick Bouvier wrote: > On 4/18/25 07:02, Philippe Mathieu-Daudé wrote: >> On 18/4/25 05:01, Pierrick Bouvier wrote: >>> On 4/17/25 17:50, Philippe Mathieu-Daudé wrote: >>>> Have target_name() be a target-agnostic method, dispatching >>>> to a per-target TargetInfo singleton structure. >>>> By default a stub singleton is used. No logical change >>>> expected. >>>> >>>> Inspired-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org> >>>> --- >>>> meson.build | 3 +++ >>>> include/hw/core/cpu.h | 2 -- >>>> include/qemu/target_info-impl.h | 23 +++++++++++++++++++++++ >>>> include/qemu/target_info.h | 19 +++++++++++++++++++ >>>> cpu-target.c | 5 ----- >>>> hw/core/machine-qmp-cmds.c | 1 + >>>> plugins/loader.c | 2 +- >>>> system/vl.c | 2 +- >>>> target_info-stub.c | 19 +++++++++++++++++++ >>>> target_info.c | 16 ++++++++++++++++ >>>> 10 files changed, 83 insertions(+), 9 deletions(-) >>>> create mode 100644 include/qemu/target_info-impl.h >>>> create mode 100644 include/qemu/target_info.h >>>> create mode 100644 target_info-stub.c >>>> create mode 100644 target_info.c >> >> >>>> diff --git a/target_info-stub.c b/target_info-stub.c >>>> new file mode 100644 >>>> index 00000000000..1e44bb6f6fb >>>> --- /dev/null >>>> +++ b/target_info-stub.c >>>> @@ -0,0 +1,19 @@ >>>> +/* >>>> + * QEMU target info stubs >>>> + * >>>> + * Copyright (c) Linaro >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0-or-later >>>> + */ >>>> + >>>> +#include "qemu/osdep.h" >>>> +#include "qemu/target_info-impl.h" >>>> + >>>> +static const TargetInfo target_info_stub = { >>>> + .name = TARGET_NAME, >>>> +}; >>>> + >>>> +const TargetInfo *target_info(void) >>>> +{ >>>> + return &target_info_stub; >>>> +} >>>> diff --git a/target_info.c b/target_info.c >>>> new file mode 100644 >>>> index 00000000000..877a6a15014 >>>> --- /dev/null >>>> +++ b/target_info.c >>>> @@ -0,0 +1,16 @@ >>>> +/* >>>> + * QEMU binary/target helpers >>>> + * >>>> + * Copyright (c) Linaro >>>> + * >>>> + * SPDX-License-Identifier: GPL-2.0-or-later >>>> + */ >>>> + >>>> +#include "qemu/osdep.h" >>>> +#include "qemu/target_info-impl.h" >>>> +#include "qemu/target_info.h" >>>> + >>>> +const char *target_name(void) >>>> +{ >>>> + return target_info()->name; >>>> +} >>> >>> What is the benefit to have two different files (common and specific)? >>> target_name() can be inline in the same header, returning the matching >>> field in existing target_info, which does not need any specialization >>> per target. >> >> common interface exposed target-agnostic, dispatching to target-specific >> implementation (providing a stub we'll remove once all targets >> converted). >> >> What would you suggest? > > To remove target_info.c and target_info-impl.h, and implement > target_name() as a static inline in target_info.h. > > This way, target_info.h can still be included from common code, the > structure is directly accessed, and we have a single header where we can > add new sugar functions and associated fields later. OK. As I'm about to post a good-enough v3, I'll not implement this now, but will consider for v4.
diff --git a/meson.build b/meson.build index bcb9d39a387..49a050a28a3 100644 --- a/meson.build +++ b/meson.build @@ -3807,6 +3807,9 @@ endif common_ss.add(pagevary) specific_ss.add(files('page-target.c', 'page-vary-target.c')) +common_ss.add(files('target_info.c')) +specific_ss.add(files('target_info-stub.c')) + subdir('backends') subdir('disas') subdir('migration') diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 5b645df59f5..9d9448341d1 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -1115,8 +1115,6 @@ bool cpu_exec_realizefn(CPUState *cpu, Error **errp); void cpu_exec_unrealizefn(CPUState *cpu); void cpu_exec_reset_hold(CPUState *cpu); -const char *target_name(void); - #ifdef COMPILING_PER_TARGET extern const VMStateDescription vmstate_cpu_common; diff --git a/include/qemu/target_info-impl.h b/include/qemu/target_info-impl.h new file mode 100644 index 00000000000..d5c94ed5296 --- /dev/null +++ b/include/qemu/target_info-impl.h @@ -0,0 +1,23 @@ +/* + * QEMU binary/target API ... + * + * Copyright (c) Linaro + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef QEMU_TARGET_INFO_IMPL_H +#define QEMU_TARGET_INFO_IMPL_H + +#include "qemu/target_info.h" + +typedef struct TargetInfo { + + /* runtime equivalent of TARGET_NAME definition */ + const char *const name; + +} TargetInfo; + +const TargetInfo *target_info(void); + +#endif diff --git a/include/qemu/target_info.h b/include/qemu/target_info.h new file mode 100644 index 00000000000..3f6cbbbd300 --- /dev/null +++ b/include/qemu/target_info.h @@ -0,0 +1,19 @@ +/* + * QEMU binary/target API + * + * Copyright (c) Linaro + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef QEMU_TARGET_INFO_H +#define QEMU_TARGET_INFO_H + +/** + * target_name: + * + * Returns: Canonical target name (i.e. "i386"). + */ +const char *target_name(void); + +#endif diff --git a/cpu-target.c b/cpu-target.c index c99d208a7c4..3f82d3ea444 100644 --- a/cpu-target.c +++ b/cpu-target.c @@ -165,8 +165,3 @@ bool target_words_bigendian(void) { return TARGET_BIG_ENDIAN; } - -const char *target_name(void) -{ - return TARGET_NAME; -} diff --git a/hw/core/machine-qmp-cmds.c b/hw/core/machine-qmp-cmds.c index 408994b67d7..b317aec234f 100644 --- a/hw/core/machine-qmp-cmds.c +++ b/hw/core/machine-qmp-cmds.c @@ -19,6 +19,7 @@ #include "qapi/qobject-input-visitor.h" #include "qapi/type-helpers.h" #include "qemu/uuid.h" +#include "qemu/target_info.h" #include "qom/qom-qobject.h" #include "system/hostmem.h" #include "system/hw_accel.h" diff --git a/plugins/loader.c b/plugins/loader.c index 7523d554f03..36a4e88d4db 100644 --- a/plugins/loader.c +++ b/plugins/loader.c @@ -29,7 +29,7 @@ #include "qemu/xxhash.h" #include "qemu/plugin.h" #include "qemu/memalign.h" -#include "hw/core/cpu.h" +#include "qemu/target_info.h" #include "exec/tb-flush.h" #include "plugin.h" diff --git a/system/vl.c b/system/vl.c index c17945c4939..d8a0fe713c9 100644 --- a/system/vl.c +++ b/system/vl.c @@ -40,6 +40,7 @@ #include "qemu/help_option.h" #include "qemu/hw-version.h" #include "qemu/uuid.h" +#include "qemu/target_info.h" #include "system/reset.h" #include "system/runstate.h" #include "system/runstate-action.h" @@ -79,7 +80,6 @@ #include "hw/block/block.h" #include "hw/i386/x86.h" #include "hw/i386/pc.h" -#include "hw/core/cpu.h" #include "migration/cpr.h" #include "migration/misc.h" #include "migration/snapshot.h" diff --git a/target_info-stub.c b/target_info-stub.c new file mode 100644 index 00000000000..1e44bb6f6fb --- /dev/null +++ b/target_info-stub.c @@ -0,0 +1,19 @@ +/* + * QEMU target info stubs + * + * Copyright (c) Linaro + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "qemu/target_info-impl.h" + +static const TargetInfo target_info_stub = { + .name = TARGET_NAME, +}; + +const TargetInfo *target_info(void) +{ + return &target_info_stub; +} diff --git a/target_info.c b/target_info.c new file mode 100644 index 00000000000..877a6a15014 --- /dev/null +++ b/target_info.c @@ -0,0 +1,16 @@ +/* + * QEMU binary/target helpers + * + * Copyright (c) Linaro + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#include "qemu/osdep.h" +#include "qemu/target_info-impl.h" +#include "qemu/target_info.h" + +const char *target_name(void) +{ + return target_info()->name; +}