| Message ID | 20250422145502.70770-15-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | single-binary: Make hw/arm/ common | expand |
On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé<philmd@linaro.org> > --- > include/qemu/target-info-impl.h | 4 ++++ > configs/targets/aarch64-softmmu.c | 1 + > configs/targets/arm-softmmu.c | 1 + > target-info-stub.c | 1 + > 4 files changed, 7 insertions(+) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > include/qemu/target-info-impl.h | 4 ++++ > configs/targets/aarch64-softmmu.c | 1 + > configs/targets/arm-softmmu.c | 1 + > target-info-stub.c | 1 + > 4 files changed, 7 insertions(+) > > diff --git a/include/qemu/target-info-impl.h b/include/qemu/target-info-impl.h > index 4ef54c5136a..e5cd169b49a 100644 > --- a/include/qemu/target-info-impl.h > +++ b/include/qemu/target-info-impl.h > @@ -10,12 +10,16 @@ > #define QEMU_TARGET_INFO_IMPL_H > > #include "qemu/target-info.h" > +#include "qapi/qapi-types-machine.h" > > typedef struct TargetInfo { > > /* runtime equivalent of TARGET_NAME definition */ > const char *const target_name; > > + /* related to TARGET_ARCH definition */ > + SysEmuTarget target_arch; > + > /* QOM typename machines for this binary must implement */ > const char *const machine_typename; > > diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/aarch64-softmmu.c > index 375e6fa0b7b..ff89401ea34 100644 > --- a/configs/targets/aarch64-softmmu.c > +++ b/configs/targets/aarch64-softmmu.c > @@ -13,6 +13,7 @@ > > static const TargetInfo target_info_aarch64_system = { > .target_name = "aarch64", > + .target_arch = SYS_EMU_TARGET_AARCH64, > .machine_typename = TYPE_TARGET_AARCH64_MACHINE, > }; > > diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm-softmmu.c > index d4acdae64f3..22ec9e4faa3 100644 > --- a/configs/targets/arm-softmmu.c > +++ b/configs/targets/arm-softmmu.c > @@ -13,6 +13,7 @@ > > static const TargetInfo target_info_arm_system = { > .target_name = "arm", > + .target_arch = SYS_EMU_TARGET_ARM, > .machine_typename = TYPE_TARGET_ARM_MACHINE, > }; > > diff --git a/target-info-stub.c b/target-info-stub.c > index 218e5898e7f..e573f5c1975 100644 > --- a/target-info-stub.c > +++ b/target-info-stub.c > @@ -12,6 +12,7 @@ > > static const TargetInfo target_info_stub = { > .target_name = TARGET_NAME, > + .target_arch = -1, I think we should have a full ifdef ladder here, to handle all architectures. Setting -1 is not a safe default. > .machine_typename = TYPE_MACHINE, > }; >
On 22/4/25 20:20, Pierrick Bouvier wrote: > On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> include/qemu/target-info-impl.h | 4 ++++ >> configs/targets/aarch64-softmmu.c | 1 + >> configs/targets/arm-softmmu.c | 1 + >> target-info-stub.c | 1 + >> 4 files changed, 7 insertions(+) >> >> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >> info-impl.h >> index 4ef54c5136a..e5cd169b49a 100644 >> --- a/include/qemu/target-info-impl.h >> +++ b/include/qemu/target-info-impl.h >> @@ -10,12 +10,16 @@ >> #define QEMU_TARGET_INFO_IMPL_H >> #include "qemu/target-info.h" >> +#include "qapi/qapi-types-machine.h" >> typedef struct TargetInfo { >> /* runtime equivalent of TARGET_NAME definition */ >> const char *const target_name; >> + /* related to TARGET_ARCH definition */ >> + SysEmuTarget target_arch; >> + >> /* QOM typename machines for this binary must implement */ >> const char *const machine_typename; >> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >> aarch64-softmmu.c >> index 375e6fa0b7b..ff89401ea34 100644 >> --- a/configs/targets/aarch64-softmmu.c >> +++ b/configs/targets/aarch64-softmmu.c >> @@ -13,6 +13,7 @@ >> static const TargetInfo target_info_aarch64_system = { >> .target_name = "aarch64", >> + .target_arch = SYS_EMU_TARGET_AARCH64, >> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >> }; >> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >> softmmu.c >> index d4acdae64f3..22ec9e4faa3 100644 >> --- a/configs/targets/arm-softmmu.c >> +++ b/configs/targets/arm-softmmu.c >> @@ -13,6 +13,7 @@ >> static const TargetInfo target_info_arm_system = { >> .target_name = "arm", >> + .target_arch = SYS_EMU_TARGET_ARM, >> .machine_typename = TYPE_TARGET_ARM_MACHINE, >> }; >> diff --git a/target-info-stub.c b/target-info-stub.c >> index 218e5898e7f..e573f5c1975 100644 >> --- a/target-info-stub.c >> +++ b/target-info-stub.c >> @@ -12,6 +12,7 @@ >> static const TargetInfo target_info_stub = { >> .target_name = TARGET_NAME, >> + .target_arch = -1, > > I think we should have a full ifdef ladder here, to handle all > architectures. Setting -1 is not a safe default. TargetInfo definition is internal to "qemu/target-info-impl.h", otherwise its type is forward-declared as opaque. > >> .machine_typename = TYPE_MACHINE, >> }; >
On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: > On 22/4/25 20:20, Pierrick Bouvier wrote: >> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> include/qemu/target-info-impl.h | 4 ++++ >>> configs/targets/aarch64-softmmu.c | 1 + >>> configs/targets/arm-softmmu.c | 1 + >>> target-info-stub.c | 1 + >>> 4 files changed, 7 insertions(+) >>> >>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>> info-impl.h >>> index 4ef54c5136a..e5cd169b49a 100644 >>> --- a/include/qemu/target-info-impl.h >>> +++ b/include/qemu/target-info-impl.h >>> @@ -10,12 +10,16 @@ >>> #define QEMU_TARGET_INFO_IMPL_H >>> #include "qemu/target-info.h" >>> +#include "qapi/qapi-types-machine.h" >>> typedef struct TargetInfo { >>> /* runtime equivalent of TARGET_NAME definition */ >>> const char *const target_name; >>> + /* related to TARGET_ARCH definition */ >>> + SysEmuTarget target_arch; >>> + >>> /* QOM typename machines for this binary must implement */ >>> const char *const machine_typename; >>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>> aarch64-softmmu.c >>> index 375e6fa0b7b..ff89401ea34 100644 >>> --- a/configs/targets/aarch64-softmmu.c >>> +++ b/configs/targets/aarch64-softmmu.c >>> @@ -13,6 +13,7 @@ >>> static const TargetInfo target_info_aarch64_system = { >>> .target_name = "aarch64", >>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>> }; >>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>> softmmu.c >>> index d4acdae64f3..22ec9e4faa3 100644 >>> --- a/configs/targets/arm-softmmu.c >>> +++ b/configs/targets/arm-softmmu.c >>> @@ -13,6 +13,7 @@ >>> static const TargetInfo target_info_arm_system = { >>> .target_name = "arm", >>> + .target_arch = SYS_EMU_TARGET_ARM, >>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>> }; >>> diff --git a/target-info-stub.c b/target-info-stub.c >>> index 218e5898e7f..e573f5c1975 100644 >>> --- a/target-info-stub.c >>> +++ b/target-info-stub.c >>> @@ -12,6 +12,7 @@ >>> static const TargetInfo target_info_stub = { >>> .target_name = TARGET_NAME, >>> + .target_arch = -1, >> >> I think we should have a full ifdef ladder here, to handle all >> architectures. Setting -1 is not a safe default. > > TargetInfo definition is internal to "qemu/target-info-impl.h", > otherwise its type is forward-declared as opaque. > Fine, but we need to be able to access to target_arch(), which returns the enum value, without having to deal with -1 situation, which is not a proper enum value. switch (target_arch()) { case SYS_EMU_TARGET_ARM: case SYS_EMU_TARGET_AARCH64: ... default: break; } >> >>> .machine_typename = TYPE_MACHINE, >>> }; >> >
On 22/4/25 20:30, Pierrick Bouvier wrote: > On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: >> On 22/4/25 20:20, Pierrick Bouvier wrote: >>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> --- >>>> include/qemu/target-info-impl.h | 4 ++++ >>>> configs/targets/aarch64-softmmu.c | 1 + >>>> configs/targets/arm-softmmu.c | 1 + >>>> target-info-stub.c | 1 + >>>> 4 files changed, 7 insertions(+) >>>> >>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>>> info-impl.h >>>> index 4ef54c5136a..e5cd169b49a 100644 >>>> --- a/include/qemu/target-info-impl.h >>>> +++ b/include/qemu/target-info-impl.h >>>> @@ -10,12 +10,16 @@ >>>> #define QEMU_TARGET_INFO_IMPL_H >>>> #include "qemu/target-info.h" >>>> +#include "qapi/qapi-types-machine.h" >>>> typedef struct TargetInfo { >>>> /* runtime equivalent of TARGET_NAME definition */ >>>> const char *const target_name; >>>> + /* related to TARGET_ARCH definition */ >>>> + SysEmuTarget target_arch; >>>> + >>>> /* QOM typename machines for this binary must implement */ >>>> const char *const machine_typename; >>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>>> aarch64-softmmu.c >>>> index 375e6fa0b7b..ff89401ea34 100644 >>>> --- a/configs/targets/aarch64-softmmu.c >>>> +++ b/configs/targets/aarch64-softmmu.c >>>> @@ -13,6 +13,7 @@ >>>> static const TargetInfo target_info_aarch64_system = { >>>> .target_name = "aarch64", >>>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>>> }; >>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>>> softmmu.c >>>> index d4acdae64f3..22ec9e4faa3 100644 >>>> --- a/configs/targets/arm-softmmu.c >>>> +++ b/configs/targets/arm-softmmu.c >>>> @@ -13,6 +13,7 @@ >>>> static const TargetInfo target_info_arm_system = { >>>> .target_name = "arm", >>>> + .target_arch = SYS_EMU_TARGET_ARM, >>>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>>> }; >>>> diff --git a/target-info-stub.c b/target-info-stub.c >>>> index 218e5898e7f..e573f5c1975 100644 >>>> --- a/target-info-stub.c >>>> +++ b/target-info-stub.c >>>> @@ -12,6 +12,7 @@ >>>> static const TargetInfo target_info_stub = { >>>> .target_name = TARGET_NAME, >>>> + .target_arch = -1, >>> >>> I think we should have a full ifdef ladder here, to handle all >>> architectures. Setting -1 is not a safe default. >> >> TargetInfo definition is internal to "qemu/target-info-impl.h", >> otherwise its type is forward-declared as opaque. >> > > Fine, but we need to be able to access to target_arch(), which returns > the enum value, without having to deal with -1 situation, which is not a > proper enum value. > > switch (target_arch()) { > case SYS_EMU_TARGET_ARM: > case SYS_EMU_TARGET_AARCH64: > ... > default: > break; > } I didn't mentioned that because in https://lore.kernel.org/qemu-devel/3242cee6-7485-4958-a198-38d0fc68e8cd@linaro.org/ you said: At this point, I would like to focus on having a first version of TargetInfo API, and not reviewing any other changes, as things may be modified, and they would need to be reviewed again. It's hard to follow the same abstraction done multiple times in multiple series. What is your "full ifdef ladder" suggestion to avoid -1?
On 4/22/25 22:34, Philippe Mathieu-Daudé wrote: > On 22/4/25 20:30, Pierrick Bouvier wrote: >> On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: >>> On 22/4/25 20:20, Pierrick Bouvier wrote: >>>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> include/qemu/target-info-impl.h | 4 ++++ >>>>> configs/targets/aarch64-softmmu.c | 1 + >>>>> configs/targets/arm-softmmu.c | 1 + >>>>> target-info-stub.c | 1 + >>>>> 4 files changed, 7 insertions(+) >>>>> >>>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>>>> info-impl.h >>>>> index 4ef54c5136a..e5cd169b49a 100644 >>>>> --- a/include/qemu/target-info-impl.h >>>>> +++ b/include/qemu/target-info-impl.h >>>>> @@ -10,12 +10,16 @@ >>>>> #define QEMU_TARGET_INFO_IMPL_H >>>>> #include "qemu/target-info.h" >>>>> +#include "qapi/qapi-types-machine.h" >>>>> typedef struct TargetInfo { >>>>> /* runtime equivalent of TARGET_NAME definition */ >>>>> const char *const target_name; >>>>> + /* related to TARGET_ARCH definition */ >>>>> + SysEmuTarget target_arch; >>>>> + >>>>> /* QOM typename machines for this binary must implement */ >>>>> const char *const machine_typename; >>>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>>>> aarch64-softmmu.c >>>>> index 375e6fa0b7b..ff89401ea34 100644 >>>>> --- a/configs/targets/aarch64-softmmu.c >>>>> +++ b/configs/targets/aarch64-softmmu.c >>>>> @@ -13,6 +13,7 @@ >>>>> static const TargetInfo target_info_aarch64_system = { >>>>> .target_name = "aarch64", >>>>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>>>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>>>> }; >>>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>>>> softmmu.c >>>>> index d4acdae64f3..22ec9e4faa3 100644 >>>>> --- a/configs/targets/arm-softmmu.c >>>>> +++ b/configs/targets/arm-softmmu.c >>>>> @@ -13,6 +13,7 @@ >>>>> static const TargetInfo target_info_arm_system = { >>>>> .target_name = "arm", >>>>> + .target_arch = SYS_EMU_TARGET_ARM, >>>>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>>>> }; >>>>> diff --git a/target-info-stub.c b/target-info-stub.c >>>>> index 218e5898e7f..e573f5c1975 100644 >>>>> --- a/target-info-stub.c >>>>> +++ b/target-info-stub.c >>>>> @@ -12,6 +12,7 @@ >>>>> static const TargetInfo target_info_stub = { >>>>> .target_name = TARGET_NAME, >>>>> + .target_arch = -1, >>>> >>>> I think we should have a full ifdef ladder here, to handle all >>>> architectures. Setting -1 is not a safe default. >>> >>> TargetInfo definition is internal to "qemu/target-info-impl.h", >>> otherwise its type is forward-declared as opaque. >>> >> >> Fine, but we need to be able to access to target_arch(), which returns >> the enum value, without having to deal with -1 situation, which is not a >> proper enum value. >> >> switch (target_arch()) { >> case SYS_EMU_TARGET_ARM: >> case SYS_EMU_TARGET_AARCH64: >> ... >> default: >> break; >> } > > I didn't mentioned that because in > https://lore.kernel.org/qemu-devel/3242cee6-7485-4958-a198-38d0fc68e8cd@linaro.org/ > you said: > > At this point, I would like to focus on having a first version of > TargetInfo API, and not reviewing any other changes, as things may > be modified, and they would need to be reviewed again. It's hard > to follow the same abstraction done multiple times in multiple series. > > What is your "full ifdef ladder" suggestion to avoid -1? #ifdef TARGET_AARCH64 # define TARGET_ARCH SYS_EMU_TARGET_AARCH64 #elif TARGET_ARCH_ALPHA # define TARGET_ARCH SYS_EMU_TARGET_ALPHA ... #else #error Target architecture can't be detected #endif static const TargetInfo target_info_stub = { ... .target_arch = TARGET_ARCH; ... } One important stuff is to make sure we treat correctly bitness variants of a given arch: TARGET_AARCH64 should be tested *before* TARGET_ARM, and same for other base architectures. Besides that, it's straightforward, and we can easily integrate that in this series.
On 4/22/25 23:24, Pierrick Bouvier wrote: > On 4/22/25 22:34, Philippe Mathieu-Daudé wrote: >> On 22/4/25 20:30, Pierrick Bouvier wrote: >>> On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: >>>> On 22/4/25 20:20, Pierrick Bouvier wrote: >>>>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>> --- >>>>>> include/qemu/target-info-impl.h | 4 ++++ >>>>>> configs/targets/aarch64-softmmu.c | 1 + >>>>>> configs/targets/arm-softmmu.c | 1 + >>>>>> target-info-stub.c | 1 + >>>>>> 4 files changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>>>>> info-impl.h >>>>>> index 4ef54c5136a..e5cd169b49a 100644 >>>>>> --- a/include/qemu/target-info-impl.h >>>>>> +++ b/include/qemu/target-info-impl.h >>>>>> @@ -10,12 +10,16 @@ >>>>>> #define QEMU_TARGET_INFO_IMPL_H >>>>>> #include "qemu/target-info.h" >>>>>> +#include "qapi/qapi-types-machine.h" >>>>>> typedef struct TargetInfo { >>>>>> /* runtime equivalent of TARGET_NAME definition */ >>>>>> const char *const target_name; >>>>>> + /* related to TARGET_ARCH definition */ >>>>>> + SysEmuTarget target_arch; >>>>>> + >>>>>> /* QOM typename machines for this binary must implement */ >>>>>> const char *const machine_typename; >>>>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>>>>> aarch64-softmmu.c >>>>>> index 375e6fa0b7b..ff89401ea34 100644 >>>>>> --- a/configs/targets/aarch64-softmmu.c >>>>>> +++ b/configs/targets/aarch64-softmmu.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> static const TargetInfo target_info_aarch64_system = { >>>>>> .target_name = "aarch64", >>>>>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>>>>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>>>>> }; >>>>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>>>>> softmmu.c >>>>>> index d4acdae64f3..22ec9e4faa3 100644 >>>>>> --- a/configs/targets/arm-softmmu.c >>>>>> +++ b/configs/targets/arm-softmmu.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> static const TargetInfo target_info_arm_system = { >>>>>> .target_name = "arm", >>>>>> + .target_arch = SYS_EMU_TARGET_ARM, >>>>>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>>>>> }; >>>>>> diff --git a/target-info-stub.c b/target-info-stub.c >>>>>> index 218e5898e7f..e573f5c1975 100644 >>>>>> --- a/target-info-stub.c >>>>>> +++ b/target-info-stub.c >>>>>> @@ -12,6 +12,7 @@ >>>>>> static const TargetInfo target_info_stub = { >>>>>> .target_name = TARGET_NAME, >>>>>> + .target_arch = -1, >>>>> >>>>> I think we should have a full ifdef ladder here, to handle all >>>>> architectures. Setting -1 is not a safe default. >>>> >>>> TargetInfo definition is internal to "qemu/target-info-impl.h", >>>> otherwise its type is forward-declared as opaque. >>>> >>> >>> Fine, but we need to be able to access to target_arch(), which returns >>> the enum value, without having to deal with -1 situation, which is not a >>> proper enum value. >>> >>> switch (target_arch()) { >>> case SYS_EMU_TARGET_ARM: >>> case SYS_EMU_TARGET_AARCH64: >>> ... >>> default: >>> break; >>> } >> >> I didn't mentioned that because in >> https://lore.kernel.org/qemu-devel/3242cee6-7485-4958-a198-38d0fc68e8cd@linaro.org/ >> you said: >> >> At this point, I would like to focus on having a first version of >> TargetInfo API, and not reviewing any other changes, as things may >> be modified, and they would need to be reviewed again. It's hard >> to follow the same abstraction done multiple times in multiple series. >> >> What is your "full ifdef ladder" suggestion to avoid -1? > > #ifdef TARGET_AARCH64 > # define TARGET_ARCH SYS_EMU_TARGET_AARCH64 > #elif TARGET_ARCH_ALPHA > # define TARGET_ARCH SYS_EMU_TARGET_ALPHA > ... > #else > #error Target architecture can't be detected > #endif > > static const TargetInfo target_info_stub = { > ... > .target_arch = TARGET_ARCH; > ... > } > To be complete, we should also add: static inline SysEmuTarget target_arch(void) { return target_info()->target_arch; } so it can be used by code later (QAPI generated files will need that, and virtio devices as you noticed). To make it used through the series, target_aarch64() can be rewritten: static inline bool target_aarch64(void) { return target_arch() == SYS_EMU_TARGET_AARCH64; } > One important stuff is to make sure we treat correctly bitness variants > of a given arch: TARGET_AARCH64 should be tested *before* TARGET_ARM, > and same for other base architectures. > Besides that, it's straightforward, and we can easily integrate that in > this series.
On 23/4/25 08:24, Pierrick Bouvier wrote: > On 4/22/25 22:34, Philippe Mathieu-Daudé wrote: >> On 22/4/25 20:30, Pierrick Bouvier wrote: >>> On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: >>>> On 22/4/25 20:20, Pierrick Bouvier wrote: >>>>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>> --- >>>>>> include/qemu/target-info-impl.h | 4 ++++ >>>>>> configs/targets/aarch64-softmmu.c | 1 + >>>>>> configs/targets/arm-softmmu.c | 1 + >>>>>> target-info-stub.c | 1 + >>>>>> 4 files changed, 7 insertions(+) >>>>>> >>>>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>>>>> info-impl.h >>>>>> index 4ef54c5136a..e5cd169b49a 100644 >>>>>> --- a/include/qemu/target-info-impl.h >>>>>> +++ b/include/qemu/target-info-impl.h >>>>>> @@ -10,12 +10,16 @@ >>>>>> #define QEMU_TARGET_INFO_IMPL_H >>>>>> #include "qemu/target-info.h" >>>>>> +#include "qapi/qapi-types-machine.h" >>>>>> typedef struct TargetInfo { >>>>>> /* runtime equivalent of TARGET_NAME definition */ >>>>>> const char *const target_name; >>>>>> + /* related to TARGET_ARCH definition */ >>>>>> + SysEmuTarget target_arch; >>>>>> + >>>>>> /* QOM typename machines for this binary must implement */ >>>>>> const char *const machine_typename; >>>>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>>>>> aarch64-softmmu.c >>>>>> index 375e6fa0b7b..ff89401ea34 100644 >>>>>> --- a/configs/targets/aarch64-softmmu.c >>>>>> +++ b/configs/targets/aarch64-softmmu.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> static const TargetInfo target_info_aarch64_system = { >>>>>> .target_name = "aarch64", >>>>>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>>>>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>>>>> }; >>>>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>>>>> softmmu.c >>>>>> index d4acdae64f3..22ec9e4faa3 100644 >>>>>> --- a/configs/targets/arm-softmmu.c >>>>>> +++ b/configs/targets/arm-softmmu.c >>>>>> @@ -13,6 +13,7 @@ >>>>>> static const TargetInfo target_info_arm_system = { >>>>>> .target_name = "arm", >>>>>> + .target_arch = SYS_EMU_TARGET_ARM, >>>>>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>>>>> }; >>>>>> diff --git a/target-info-stub.c b/target-info-stub.c >>>>>> index 218e5898e7f..e573f5c1975 100644 >>>>>> --- a/target-info-stub.c >>>>>> +++ b/target-info-stub.c >>>>>> @@ -12,6 +12,7 @@ >>>>>> static const TargetInfo target_info_stub = { >>>>>> .target_name = TARGET_NAME, >>>>>> + .target_arch = -1, >>>>> >>>>> I think we should have a full ifdef ladder here, to handle all >>>>> architectures. Setting -1 is not a safe default. >>>> >>>> TargetInfo definition is internal to "qemu/target-info-impl.h", >>>> otherwise its type is forward-declared as opaque. >>>> >>> >>> Fine, but we need to be able to access to target_arch(), which returns >>> the enum value, without having to deal with -1 situation, which is not a >>> proper enum value. >>> >>> switch (target_arch()) { >>> case SYS_EMU_TARGET_ARM: >>> case SYS_EMU_TARGET_AARCH64: >>> ... >>> default: >>> break; >>> } >> >> I didn't mentioned that because in >> https://lore.kernel.org/qemu-devel/3242cee6-7485-4958- >> a198-38d0fc68e8cd@linaro.org/ >> you said: >> >> At this point, I would like to focus on having a first version of >> TargetInfo API, and not reviewing any other changes, as things may >> be modified, and they would need to be reviewed again. It's hard >> to follow the same abstraction done multiple times in multiple >> series. >> >> What is your "full ifdef ladder" suggestion to avoid -1? > > #ifdef TARGET_AARCH64 > # define TARGET_ARCH SYS_EMU_TARGET_AARCH64 > #elif TARGET_ARCH_ALPHA > # define TARGET_ARCH SYS_EMU_TARGET_ALPHA > ... > #else > #error Target architecture can't be detected I'm sorry but I don't understand what we gain doing that. > #endif > > static const TargetInfo target_info_stub = { > ... > .target_arch = TARGET_ARCH; > ... > } > > One important stuff is to make sure we treat correctly bitness variants > of a given arch: TARGET_AARCH64 should be tested *before* TARGET_ARM, > and same for other base architectures. Thankfully we don't check for TARGET_ARM, so we don't have to implement a confusing target_arm() helper for the single-binary effort. When we get to heterogeneous emulation, I expect system ARM/Aarch64 to be merged as a single Aarch64 target (32-bit ARM being a CPU state), so the target_aarch64() should still be enough. > Besides that, it's straightforward, and we can easily integrate that in > this series.
On 4/23/25 02:14, Philippe Mathieu-Daudé wrote: > On 23/4/25 08:24, Pierrick Bouvier wrote: >> On 4/22/25 22:34, Philippe Mathieu-Daudé wrote: >>> On 22/4/25 20:30, Pierrick Bouvier wrote: >>>> On 4/22/25 11:24, Philippe Mathieu-Daudé wrote: >>>>> On 22/4/25 20:20, Pierrick Bouvier wrote: >>>>>> On 4/22/25 07:54, Philippe Mathieu-Daudé wrote: >>>>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>>>> --- >>>>>>> include/qemu/target-info-impl.h | 4 ++++ >>>>>>> configs/targets/aarch64-softmmu.c | 1 + >>>>>>> configs/targets/arm-softmmu.c | 1 + >>>>>>> target-info-stub.c | 1 + >>>>>>> 4 files changed, 7 insertions(+) >>>>>>> >>>>>>> diff --git a/include/qemu/target-info-impl.h b/include/qemu/target- >>>>>>> info-impl.h >>>>>>> index 4ef54c5136a..e5cd169b49a 100644 >>>>>>> --- a/include/qemu/target-info-impl.h >>>>>>> +++ b/include/qemu/target-info-impl.h >>>>>>> @@ -10,12 +10,16 @@ >>>>>>> #define QEMU_TARGET_INFO_IMPL_H >>>>>>> #include "qemu/target-info.h" >>>>>>> +#include "qapi/qapi-types-machine.h" >>>>>>> typedef struct TargetInfo { >>>>>>> /* runtime equivalent of TARGET_NAME definition */ >>>>>>> const char *const target_name; >>>>>>> + /* related to TARGET_ARCH definition */ >>>>>>> + SysEmuTarget target_arch; >>>>>>> + >>>>>>> /* QOM typename machines for this binary must implement */ >>>>>>> const char *const machine_typename; >>>>>>> diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/ >>>>>>> aarch64-softmmu.c >>>>>>> index 375e6fa0b7b..ff89401ea34 100644 >>>>>>> --- a/configs/targets/aarch64-softmmu.c >>>>>>> +++ b/configs/targets/aarch64-softmmu.c >>>>>>> @@ -13,6 +13,7 @@ >>>>>>> static const TargetInfo target_info_aarch64_system = { >>>>>>> .target_name = "aarch64", >>>>>>> + .target_arch = SYS_EMU_TARGET_AARCH64, >>>>>>> .machine_typename = TYPE_TARGET_AARCH64_MACHINE, >>>>>>> }; >>>>>>> diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm- >>>>>>> softmmu.c >>>>>>> index d4acdae64f3..22ec9e4faa3 100644 >>>>>>> --- a/configs/targets/arm-softmmu.c >>>>>>> +++ b/configs/targets/arm-softmmu.c >>>>>>> @@ -13,6 +13,7 @@ >>>>>>> static const TargetInfo target_info_arm_system = { >>>>>>> .target_name = "arm", >>>>>>> + .target_arch = SYS_EMU_TARGET_ARM, >>>>>>> .machine_typename = TYPE_TARGET_ARM_MACHINE, >>>>>>> }; >>>>>>> diff --git a/target-info-stub.c b/target-info-stub.c >>>>>>> index 218e5898e7f..e573f5c1975 100644 >>>>>>> --- a/target-info-stub.c >>>>>>> +++ b/target-info-stub.c >>>>>>> @@ -12,6 +12,7 @@ >>>>>>> static const TargetInfo target_info_stub = { >>>>>>> .target_name = TARGET_NAME, >>>>>>> + .target_arch = -1, >>>>>> >>>>>> I think we should have a full ifdef ladder here, to handle all >>>>>> architectures. Setting -1 is not a safe default. >>>>> >>>>> TargetInfo definition is internal to "qemu/target-info-impl.h", >>>>> otherwise its type is forward-declared as opaque. >>>>> >>>> >>>> Fine, but we need to be able to access to target_arch(), which returns >>>> the enum value, without having to deal with -1 situation, which is not a >>>> proper enum value. >>>> >>>> switch (target_arch()) { >>>> case SYS_EMU_TARGET_ARM: >>>> case SYS_EMU_TARGET_AARCH64: >>>> ... >>>> default: >>>> break; >>>> } >>> >>> I didn't mentioned that because in >>> https://lore.kernel.org/qemu-devel/3242cee6-7485-4958- >>> a198-38d0fc68e8cd@linaro.org/ >>> you said: >>> >>> At this point, I would like to focus on having a first version of >>> TargetInfo API, and not reviewing any other changes, as things may >>> be modified, and they would need to be reviewed again. It's hard >>> to follow the same abstraction done multiple times in multiple >>> series. >>> >>> What is your "full ifdef ladder" suggestion to avoid -1? >> >> #ifdef TARGET_AARCH64 >> # define TARGET_ARCH SYS_EMU_TARGET_AARCH64 >> #elif TARGET_ARCH_ALPHA >> # define TARGET_ARCH SYS_EMU_TARGET_ALPHA >> ... >> #else >> #error Target architecture can't be detected > > I'm sorry but I don't understand what we gain doing that. > For QAPI generated files, we'll need a various bunch of TARGET_X runtime conversion, to enable some commands only for targets. Some of the concerned targets are different from the one we focus on for single binary. My hope was to include target_arch() function in this series, so it's not a debate in next series that will touch to QAPI. In short, move the noise here and now compared to later with another unrelated topic, that will have a lot of heated opinions already. List of needed (target_()) for QAPI: TARGET_S390X TARGET_PPC TARGET_ARM TARGET_I386 TARGET_LOONGARCH64 TARGET_MIPS TARGET_RISCV TARGET_I386 If it's too much work on your side, would you be open that I provide this patch on top of your series, and that we include it with it? Thanks, Pierrick
diff --git a/include/qemu/target-info-impl.h b/include/qemu/target-info-impl.h index 4ef54c5136a..e5cd169b49a 100644 --- a/include/qemu/target-info-impl.h +++ b/include/qemu/target-info-impl.h @@ -10,12 +10,16 @@ #define QEMU_TARGET_INFO_IMPL_H #include "qemu/target-info.h" +#include "qapi/qapi-types-machine.h" typedef struct TargetInfo { /* runtime equivalent of TARGET_NAME definition */ const char *const target_name; + /* related to TARGET_ARCH definition */ + SysEmuTarget target_arch; + /* QOM typename machines for this binary must implement */ const char *const machine_typename; diff --git a/configs/targets/aarch64-softmmu.c b/configs/targets/aarch64-softmmu.c index 375e6fa0b7b..ff89401ea34 100644 --- a/configs/targets/aarch64-softmmu.c +++ b/configs/targets/aarch64-softmmu.c @@ -13,6 +13,7 @@ static const TargetInfo target_info_aarch64_system = { .target_name = "aarch64", + .target_arch = SYS_EMU_TARGET_AARCH64, .machine_typename = TYPE_TARGET_AARCH64_MACHINE, }; diff --git a/configs/targets/arm-softmmu.c b/configs/targets/arm-softmmu.c index d4acdae64f3..22ec9e4faa3 100644 --- a/configs/targets/arm-softmmu.c +++ b/configs/targets/arm-softmmu.c @@ -13,6 +13,7 @@ static const TargetInfo target_info_arm_system = { .target_name = "arm", + .target_arch = SYS_EMU_TARGET_ARM, .machine_typename = TYPE_TARGET_ARM_MACHINE, }; diff --git a/target-info-stub.c b/target-info-stub.c index 218e5898e7f..e573f5c1975 100644 --- a/target-info-stub.c +++ b/target-info-stub.c @@ -12,6 +12,7 @@ static const TargetInfo target_info_stub = { .target_name = TARGET_NAME, + .target_arch = -1, .machine_typename = TYPE_MACHINE, };
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- include/qemu/target-info-impl.h | 4 ++++ configs/targets/aarch64-softmmu.c | 1 + configs/targets/arm-softmmu.c | 1 + target-info-stub.c | 1 + 4 files changed, 7 insertions(+)