| Message ID | 20250305161248.54901-3-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm: Register target-specific QOM types at runtime | expand |
On 3/5/25 08:12, Philippe Mathieu-Daudé wrote: > For legacy ARM binaries, legacy_binary_is_64bit() is > equivalent of the compile time TARGET_AARCH64 definition. > I'm not sure where this function comes from. Anyway, to completely replace TARGET_AARCH64, what we want is something like: target_is_aarch64(), because all the objects in this file should not be enabled for 64 bits targets who are not arm based. So it's more easy to introduce a specific function *per* target, and enable objects on a per need basis. Some will be enabled for all targets, some for only one, some for a specific selection. > Use it as TypeInfo::registerable() callback to dynamically > add Aarch64 specific types in qemu-system-aarch64 binary, > removing the need of TARGET_AARCH64 #ifdef'ry. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/arm/bcm2836.c | 6 ++---- > hw/arm/raspi.c | 7 +++---- > 2 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 95e16806fa1..88a32e5fc20 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -12,6 +12,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/module.h" > +#include "qemu/legacy_binary_info.h" > #include "hw/arm/bcm2836.h" > #include "hw/arm/raspi_platform.h" > #include "hw/sysbus.h" > @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data) > dc->realize = bcm2836_realize; > }; > > -#ifdef TARGET_AARCH64 > static void bcm2837_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void *data) > bc->clusterid = 0x0; > dc->realize = bcm2836_realize; > }; > -#endif > > static const TypeInfo bcm283x_types[] = { > { > @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { > .name = TYPE_BCM2836, > .parent = TYPE_BCM283X, > .class_init = bcm2836_class_init, > -#ifdef TARGET_AARCH64 > }, { > .name = TYPE_BCM2837, > .parent = TYPE_BCM283X, > + .registerable = legacy_binary_is_64bit, > .class_init = bcm2837_class_init, > -#endif > }, { > .name = TYPE_BCM283X, > .parent = TYPE_BCM283X_BASE, > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index dce35ca11aa..f7e647a9cbf 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -15,6 +15,7 @@ > #include "qemu/osdep.h" > #include "qemu/units.h" > #include "qemu/cutils.h" > +#include "qemu/legacy_binary_info.h" > #include "qapi/error.h" > #include "hw/arm/boot.h" > #include "hw/arm/bcm2836.h" > @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass *oc, void *data) > raspi_machine_class_init(mc, rmc->board_rev); > }; > > -#ifdef TARGET_AARCH64 > static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass *oc, void *data) > rmc->board_rev = 0xa02082; > raspi_machine_class_init(mc, rmc->board_rev); > }; > -#endif /* TARGET_AARCH64 */ > > static const TypeInfo raspi_machine_types[] = { > { > @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { > .name = MACHINE_TYPE_NAME("raspi2b"), > .parent = TYPE_RASPI_MACHINE, > .class_init = raspi2b_machine_class_init, > -#ifdef TARGET_AARCH64 > }, { > .name = MACHINE_TYPE_NAME("raspi3ap"), > .parent = TYPE_RASPI_MACHINE, > + .registerable = legacy_binary_is_64bit, > .class_init = raspi3ap_machine_class_init, > }, { > .name = MACHINE_TYPE_NAME("raspi3b"), > .parent = TYPE_RASPI_MACHINE, > + .registerable = legacy_binary_is_64bit, > .class_init = raspi3b_machine_class_init, > -#endif > }, { > .name = TYPE_RASPI_MACHINE, > .parent = TYPE_RASPI_BASE_MACHINE,
On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: > For legacy ARM binaries, legacy_binary_is_64bit() is > equivalent of the compile time TARGET_AARCH64 definition. > > Use it as TypeInfo::registerable() callback to dynamically > add Aarch64 specific types in qemu-system-aarch64 binary, > removing the need of TARGET_AARCH64 #ifdef'ry. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> > --- > hw/arm/bcm2836.c | 6 ++---- > hw/arm/raspi.c | 7 +++---- > 2 files changed, 5 insertions(+), 8 deletions(-) > > diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c > index 95e16806fa1..88a32e5fc20 100644 > --- a/hw/arm/bcm2836.c > +++ b/hw/arm/bcm2836.c > @@ -12,6 +12,7 @@ > #include "qemu/osdep.h" > #include "qapi/error.h" > #include "qemu/module.h" > +#include "qemu/legacy_binary_info.h" > #include "hw/arm/bcm2836.h" > #include "hw/arm/raspi_platform.h" > #include "hw/sysbus.h" > @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data) > dc->realize = bcm2836_realize; > }; > > -#ifdef TARGET_AARCH64 > static void bcm2837_class_init(ObjectClass *oc, void *data) > { > DeviceClass *dc = DEVICE_CLASS(oc); > @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void *data) > bc->clusterid = 0x0; > dc->realize = bcm2836_realize; > }; > -#endif > > static const TypeInfo bcm283x_types[] = { > { > @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { > .name = TYPE_BCM2836, > .parent = TYPE_BCM283X, > .class_init = bcm2836_class_init, > -#ifdef TARGET_AARCH64 > }, { > .name = TYPE_BCM2837, > .parent = TYPE_BCM283X, > + .registerable = legacy_binary_is_64bit, > .class_init = bcm2837_class_init, > -#endif > }, { > .name = TYPE_BCM283X, > .parent = TYPE_BCM283X_BASE, > diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c > index dce35ca11aa..f7e647a9cbf 100644 > --- a/hw/arm/raspi.c > +++ b/hw/arm/raspi.c > @@ -15,6 +15,7 @@ > #include "qemu/osdep.h" > #include "qemu/units.h" > #include "qemu/cutils.h" > +#include "qemu/legacy_binary_info.h" > #include "qapi/error.h" > #include "hw/arm/boot.h" > #include "hw/arm/bcm2836.h" > @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass *oc, void *data) > raspi_machine_class_init(mc, rmc->board_rev); > }; > > -#ifdef TARGET_AARCH64 > static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) > { > MachineClass *mc = MACHINE_CLASS(oc); > @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass *oc, void *data) > rmc->board_rev = 0xa02082; > raspi_machine_class_init(mc, rmc->board_rev); > }; > -#endif /* TARGET_AARCH64 */ > > static const TypeInfo raspi_machine_types[] = { > { > @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { > .name = MACHINE_TYPE_NAME("raspi2b"), > .parent = TYPE_RASPI_MACHINE, > .class_init = raspi2b_machine_class_init, > -#ifdef TARGET_AARCH64 > }, { > .name = MACHINE_TYPE_NAME("raspi3ap"), > .parent = TYPE_RASPI_MACHINE, > + .registerable = legacy_binary_is_64bit, > .class_init = raspi3ap_machine_class_init, > }, { > .name = MACHINE_TYPE_NAME("raspi3b"), > .parent = TYPE_RASPI_MACHINE, > + .registerable = legacy_binary_is_64bit, > .class_init = raspi3b_machine_class_init, > -#endif > }, { > .name = TYPE_RASPI_MACHINE, > .parent = TYPE_RASPI_BASE_MACHINE, Uh, this (together with patch 1) looks very cumbersome. Why don't you simply split the array into two, one for 32-bit and one for 64-bit, and then use a simply "if (legacy_binary_is_64bit())" in the type_init function instead? Thomas
On 3/5/25 18:40, Thomas Huth wrote: > On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: >> For legacy ARM binaries, legacy_binary_is_64bit() is >> equivalent of the compile time TARGET_AARCH64 definition. >> >> Use it as TypeInfo::registerable() callback to dynamically >> add Aarch64 specific types in qemu-system-aarch64 binary, >> removing the need of TARGET_AARCH64 #ifdef'ry. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> --- >> hw/arm/bcm2836.c | 6 ++---- >> hw/arm/raspi.c | 7 +++---- >> 2 files changed, 5 insertions(+), 8 deletions(-) >> >> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >> index 95e16806fa1..88a32e5fc20 100644 >> --- a/hw/arm/bcm2836.c >> +++ b/hw/arm/bcm2836.c >> @@ -12,6 +12,7 @@ >> #include "qemu/osdep.h" >> #include "qapi/error.h" >> #include "qemu/module.h" >> +#include "qemu/legacy_binary_info.h" >> #include "hw/arm/bcm2836.h" >> #include "hw/arm/raspi_platform.h" >> #include "hw/sysbus.h" >> @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data) >> dc->realize = bcm2836_realize; >> }; >> -#ifdef TARGET_AARCH64 >> static void bcm2837_class_init(ObjectClass *oc, void *data) >> { >> DeviceClass *dc = DEVICE_CLASS(oc); >> @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void *data) >> bc->clusterid = 0x0; >> dc->realize = bcm2836_realize; >> }; >> -#endif >> static const TypeInfo bcm283x_types[] = { >> { >> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { >> .name = TYPE_BCM2836, >> .parent = TYPE_BCM283X, >> .class_init = bcm2836_class_init, >> -#ifdef TARGET_AARCH64 >> }, { >> .name = TYPE_BCM2837, >> .parent = TYPE_BCM283X, >> + .registerable = legacy_binary_is_64bit, >> .class_init = bcm2837_class_init, >> -#endif >> }, { >> .name = TYPE_BCM283X, >> .parent = TYPE_BCM283X_BASE, >> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >> index dce35ca11aa..f7e647a9cbf 100644 >> --- a/hw/arm/raspi.c >> +++ b/hw/arm/raspi.c >> @@ -15,6 +15,7 @@ >> #include "qemu/osdep.h" >> #include "qemu/units.h" >> #include "qemu/cutils.h" >> +#include "qemu/legacy_binary_info.h" >> #include "qapi/error.h" >> #include "hw/arm/boot.h" >> #include "hw/arm/bcm2836.h" >> @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass *oc, void *data) >> raspi_machine_class_init(mc, rmc->board_rev); >> }; >> -#ifdef TARGET_AARCH64 >> static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) >> { >> MachineClass *mc = MACHINE_CLASS(oc); >> @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass *oc, void *data) >> rmc->board_rev = 0xa02082; >> raspi_machine_class_init(mc, rmc->board_rev); >> }; >> -#endif /* TARGET_AARCH64 */ >> static const TypeInfo raspi_machine_types[] = { >> { >> @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { >> .name = MACHINE_TYPE_NAME("raspi2b"), >> .parent = TYPE_RASPI_MACHINE, >> .class_init = raspi2b_machine_class_init, >> -#ifdef TARGET_AARCH64 >> }, { >> .name = MACHINE_TYPE_NAME("raspi3ap"), >> .parent = TYPE_RASPI_MACHINE, >> + .registerable = legacy_binary_is_64bit, >> .class_init = raspi3ap_machine_class_init, >> }, { >> .name = MACHINE_TYPE_NAME("raspi3b"), >> .parent = TYPE_RASPI_MACHINE, >> + .registerable = legacy_binary_is_64bit, >> .class_init = raspi3b_machine_class_init, >> -#endif >> }, { >> .name = TYPE_RASPI_MACHINE, >> .parent = TYPE_RASPI_BASE_MACHINE, > > Uh, this (together with patch 1) looks very cumbersome. Why don't you simply split the array into two, one for 32-bit and one for 64-bit, and then use a simply "if (legacy_binary_is_64bit())" in the type_init function instead? Sounds like a good idea. So we would have DEFINE_TYPES() and DEFINE_TYPES64() macros ? C.
On 05/03/2025 19.12, Cédric Le Goater wrote: > On 3/5/25 18:40, Thomas Huth wrote: >> On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: >>> For legacy ARM binaries, legacy_binary_is_64bit() is >>> equivalent of the compile time TARGET_AARCH64 definition. >>> >>> Use it as TypeInfo::registerable() callback to dynamically >>> add Aarch64 specific types in qemu-system-aarch64 binary, >>> removing the need of TARGET_AARCH64 #ifdef'ry. >>> >>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> --- >>> hw/arm/bcm2836.c | 6 ++---- >>> hw/arm/raspi.c | 7 +++---- >>> 2 files changed, 5 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >>> index 95e16806fa1..88a32e5fc20 100644 >>> --- a/hw/arm/bcm2836.c >>> +++ b/hw/arm/bcm2836.c >>> @@ -12,6 +12,7 @@ >>> #include "qemu/osdep.h" >>> #include "qapi/error.h" >>> #include "qemu/module.h" >>> +#include "qemu/legacy_binary_info.h" >>> #include "hw/arm/bcm2836.h" >>> #include "hw/arm/raspi_platform.h" >>> #include "hw/sysbus.h" >>> @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void >>> *data) >>> dc->realize = bcm2836_realize; >>> }; >>> -#ifdef TARGET_AARCH64 >>> static void bcm2837_class_init(ObjectClass *oc, void *data) >>> { >>> DeviceClass *dc = DEVICE_CLASS(oc); >>> @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void >>> *data) >>> bc->clusterid = 0x0; >>> dc->realize = bcm2836_realize; >>> }; >>> -#endif >>> static const TypeInfo bcm283x_types[] = { >>> { >>> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { >>> .name = TYPE_BCM2836, >>> .parent = TYPE_BCM283X, >>> .class_init = bcm2836_class_init, >>> -#ifdef TARGET_AARCH64 >>> }, { >>> .name = TYPE_BCM2837, >>> .parent = TYPE_BCM283X, >>> + .registerable = legacy_binary_is_64bit, >>> .class_init = bcm2837_class_init, >>> -#endif >>> }, { >>> .name = TYPE_BCM283X, >>> .parent = TYPE_BCM283X_BASE, >>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >>> index dce35ca11aa..f7e647a9cbf 100644 >>> --- a/hw/arm/raspi.c >>> +++ b/hw/arm/raspi.c >>> @@ -15,6 +15,7 @@ >>> #include "qemu/osdep.h" >>> #include "qemu/units.h" >>> #include "qemu/cutils.h" >>> +#include "qemu/legacy_binary_info.h" >>> #include "qapi/error.h" >>> #include "hw/arm/boot.h" >>> #include "hw/arm/bcm2836.h" >>> @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass >>> *oc, void *data) >>> raspi_machine_class_init(mc, rmc->board_rev); >>> }; >>> -#ifdef TARGET_AARCH64 >>> static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) >>> { >>> MachineClass *mc = MACHINE_CLASS(oc); >>> @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass >>> *oc, void *data) >>> rmc->board_rev = 0xa02082; >>> raspi_machine_class_init(mc, rmc->board_rev); >>> }; >>> -#endif /* TARGET_AARCH64 */ >>> static const TypeInfo raspi_machine_types[] = { >>> { >>> @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { >>> .name = MACHINE_TYPE_NAME("raspi2b"), >>> .parent = TYPE_RASPI_MACHINE, >>> .class_init = raspi2b_machine_class_init, >>> -#ifdef TARGET_AARCH64 >>> }, { >>> .name = MACHINE_TYPE_NAME("raspi3ap"), >>> .parent = TYPE_RASPI_MACHINE, >>> + .registerable = legacy_binary_is_64bit, >>> .class_init = raspi3ap_machine_class_init, >>> }, { >>> .name = MACHINE_TYPE_NAME("raspi3b"), >>> .parent = TYPE_RASPI_MACHINE, >>> + .registerable = legacy_binary_is_64bit, >>> .class_init = raspi3b_machine_class_init, >>> -#endif >>> }, { >>> .name = TYPE_RASPI_MACHINE, >>> .parent = TYPE_RASPI_BASE_MACHINE, >> >> Uh, this (together with patch 1) looks very cumbersome. Why don't you >> simply split the array into two, one for 32-bit and one for 64-bit, and >> then use a simply "if (legacy_binary_is_64bit())" in the type_init >> function instead? > > Sounds like a good idea. > > So we would have DEFINE_TYPES() and DEFINE_TYPES64() macros ? Either that - or simply use type_init() directly here for the time being. Thomas
On 5/3/25 19:35, Thomas Huth wrote: > On 05/03/2025 19.12, Cédric Le Goater wrote: >> On 3/5/25 18:40, Thomas Huth wrote: >>> On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: >>>> For legacy ARM binaries, legacy_binary_is_64bit() is >>>> equivalent of the compile time TARGET_AARCH64 definition. >>>> >>>> Use it as TypeInfo::registerable() callback to dynamically >>>> add Aarch64 specific types in qemu-system-aarch64 binary, >>>> removing the need of TARGET_AARCH64 #ifdef'ry. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>> --- >>>> hw/arm/bcm2836.c | 6 ++---- >>>> hw/arm/raspi.c | 7 +++---- >>>> 2 files changed, 5 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >>>> index 95e16806fa1..88a32e5fc20 100644 >>>> --- a/hw/arm/bcm2836.c >>>> +++ b/hw/arm/bcm2836.c >>>> @@ -12,6 +12,7 @@ >>>> #include "qemu/osdep.h" >>>> #include "qapi/error.h" >>>> #include "qemu/module.h" >>>> +#include "qemu/legacy_binary_info.h" >>>> #include "hw/arm/bcm2836.h" >>>> #include "hw/arm/raspi_platform.h" >>>> #include "hw/sysbus.h" >>>> @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, >>>> void *data) >>>> dc->realize = bcm2836_realize; >>>> }; >>>> -#ifdef TARGET_AARCH64 >>>> static void bcm2837_class_init(ObjectClass *oc, void *data) >>>> { >>>> DeviceClass *dc = DEVICE_CLASS(oc); >>>> @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, >>>> void *data) >>>> bc->clusterid = 0x0; >>>> dc->realize = bcm2836_realize; >>>> }; >>>> -#endif >>>> static const TypeInfo bcm283x_types[] = { >>>> { >>>> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { >>>> .name = TYPE_BCM2836, >>>> .parent = TYPE_BCM283X, >>>> .class_init = bcm2836_class_init, >>>> -#ifdef TARGET_AARCH64 >>>> }, { >>>> .name = TYPE_BCM2837, >>>> .parent = TYPE_BCM283X, >>>> + .registerable = legacy_binary_is_64bit, >>>> .class_init = bcm2837_class_init, >>>> -#endif >>>> }, { >>>> .name = TYPE_BCM283X, >>>> .parent = TYPE_BCM283X_BASE, >>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >>>> index dce35ca11aa..f7e647a9cbf 100644 >>>> --- a/hw/arm/raspi.c >>>> +++ b/hw/arm/raspi.c >>>> @@ -15,6 +15,7 @@ >>>> #include "qemu/osdep.h" >>>> #include "qemu/units.h" >>>> #include "qemu/cutils.h" >>>> +#include "qemu/legacy_binary_info.h" >>>> #include "qapi/error.h" >>>> #include "hw/arm/boot.h" >>>> #include "hw/arm/bcm2836.h" >>>> @@ -367,7 +368,6 @@ static void >>>> raspi2b_machine_class_init(ObjectClass *oc, void *data) >>>> raspi_machine_class_init(mc, rmc->board_rev); >>>> }; >>>> -#ifdef TARGET_AARCH64 >>>> static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) >>>> { >>>> MachineClass *mc = MACHINE_CLASS(oc); >>>> @@ -387,7 +387,6 @@ static void >>>> raspi3b_machine_class_init(ObjectClass *oc, void *data) >>>> rmc->board_rev = 0xa02082; >>>> raspi_machine_class_init(mc, rmc->board_rev); >>>> }; >>>> -#endif /* TARGET_AARCH64 */ >>>> static const TypeInfo raspi_machine_types[] = { >>>> { >>>> @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { >>>> .name = MACHINE_TYPE_NAME("raspi2b"), >>>> .parent = TYPE_RASPI_MACHINE, >>>> .class_init = raspi2b_machine_class_init, >>>> -#ifdef TARGET_AARCH64 >>>> }, { >>>> .name = MACHINE_TYPE_NAME("raspi3ap"), >>>> .parent = TYPE_RASPI_MACHINE, >>>> + .registerable = legacy_binary_is_64bit, >>>> .class_init = raspi3ap_machine_class_init, >>>> }, { >>>> .name = MACHINE_TYPE_NAME("raspi3b"), >>>> .parent = TYPE_RASPI_MACHINE, >>>> + .registerable = legacy_binary_is_64bit, >>>> .class_init = raspi3b_machine_class_init, >>>> -#endif >>>> }, { >>>> .name = TYPE_RASPI_MACHINE, >>>> .parent = TYPE_RASPI_BASE_MACHINE, >>> >>> Uh, this (together with patch 1) looks very cumbersome. Why don't you >>> simply split the array into two, one for 32-bit and one for 64-bit, >>> and then use a simply "if (legacy_binary_is_64bit())" in the >>> type_init function instead? >> >> Sounds like a good idea. >> >> So we would have DEFINE_TYPES() and DEFINE_TYPES64() macros ? > > Either that - or simply use type_init() directly here for the time being. As Pierrick noted on private chat, my approach doesn't scale, I should use smth in the lines of: }, { .name = MACHINE_TYPE_NAME("raspi2b"), .parent = TYPE_RASPI_MACHINE, .registerable = qemu_binary_has_target_arm, .class_init = raspi2b_machine_class_init, }, { .name = MACHINE_TYPE_NAME("raspi3ap"), .parent = TYPE_RASPI_MACHINE, .registerable = qemu_binary_has_target_aarch64, .class_init = raspi3ap_machine_class_init, }, { Having: bool qemu_binary_has_target_arm(void) { return qemu_arch_available(QEMU_ARCH_ARM); } Now back to Thomas suggestion, we could define 2 TypeInfo arrays, but I foresee lot of code churn when devices has to be made available on different setup combinations; so with that in mind the QOM registerable() callback appears a bit more future proof.
On Wed, 5 Mar 2025, Philippe Mathieu-Daudé wrote: > On 5/3/25 19:35, Thomas Huth wrote: >> On 05/03/2025 19.12, Cédric Le Goater wrote: >>> On 3/5/25 18:40, Thomas Huth wrote: >>>> On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: >>>>> For legacy ARM binaries, legacy_binary_is_64bit() is >>>>> equivalent of the compile time TARGET_AARCH64 definition. >>>>> >>>>> Use it as TypeInfo::registerable() callback to dynamically >>>>> add Aarch64 specific types in qemu-system-aarch64 binary, >>>>> removing the need of TARGET_AARCH64 #ifdef'ry. >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> hw/arm/bcm2836.c | 6 ++---- >>>>> hw/arm/raspi.c | 7 +++---- >>>>> 2 files changed, 5 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >>>>> index 95e16806fa1..88a32e5fc20 100644 >>>>> --- a/hw/arm/bcm2836.c >>>>> +++ b/hw/arm/bcm2836.c >>>>> @@ -12,6 +12,7 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qapi/error.h" >>>>> #include "qemu/module.h" >>>>> +#include "qemu/legacy_binary_info.h" >>>>> #include "hw/arm/bcm2836.h" >>>>> #include "hw/arm/raspi_platform.h" >>>>> #include "hw/sysbus.h" >>>>> @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void >>>>> *data) >>>>> dc->realize = bcm2836_realize; >>>>> }; >>>>> -#ifdef TARGET_AARCH64 >>>>> static void bcm2837_class_init(ObjectClass *oc, void *data) >>>>> { >>>>> DeviceClass *dc = DEVICE_CLASS(oc); >>>>> @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void >>>>> *data) >>>>> bc->clusterid = 0x0; >>>>> dc->realize = bcm2836_realize; >>>>> }; >>>>> -#endif >>>>> static const TypeInfo bcm283x_types[] = { >>>>> { >>>>> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { >>>>> .name = TYPE_BCM2836, >>>>> .parent = TYPE_BCM283X, >>>>> .class_init = bcm2836_class_init, >>>>> -#ifdef TARGET_AARCH64 >>>>> }, { >>>>> .name = TYPE_BCM2837, >>>>> .parent = TYPE_BCM283X, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = bcm2837_class_init, >>>>> -#endif >>>>> }, { >>>>> .name = TYPE_BCM283X, >>>>> .parent = TYPE_BCM283X_BASE, >>>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >>>>> index dce35ca11aa..f7e647a9cbf 100644 >>>>> --- a/hw/arm/raspi.c >>>>> +++ b/hw/arm/raspi.c >>>>> @@ -15,6 +15,7 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qemu/units.h" >>>>> #include "qemu/cutils.h" >>>>> +#include "qemu/legacy_binary_info.h" >>>>> #include "qapi/error.h" >>>>> #include "hw/arm/boot.h" >>>>> #include "hw/arm/bcm2836.h" >>>>> @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass >>>>> *oc, void *data) >>>>> raspi_machine_class_init(mc, rmc->board_rev); >>>>> }; >>>>> -#ifdef TARGET_AARCH64 >>>>> static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) >>>>> { >>>>> MachineClass *mc = MACHINE_CLASS(oc); >>>>> @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass >>>>> *oc, void *data) >>>>> rmc->board_rev = 0xa02082; >>>>> raspi_machine_class_init(mc, rmc->board_rev); >>>>> }; >>>>> -#endif /* TARGET_AARCH64 */ >>>>> static const TypeInfo raspi_machine_types[] = { >>>>> { >>>>> @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { >>>>> .name = MACHINE_TYPE_NAME("raspi2b"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> .class_init = raspi2b_machine_class_init, >>>>> -#ifdef TARGET_AARCH64 >>>>> }, { >>>>> .name = MACHINE_TYPE_NAME("raspi3ap"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = raspi3ap_machine_class_init, >>>>> }, { >>>>> .name = MACHINE_TYPE_NAME("raspi3b"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = raspi3b_machine_class_init, >>>>> -#endif >>>>> }, { >>>>> .name = TYPE_RASPI_MACHINE, >>>>> .parent = TYPE_RASPI_BASE_MACHINE, >>>> >>>> Uh, this (together with patch 1) looks very cumbersome. Why don't you >>>> simply split the array into two, one for 32-bit and one for 64-bit, and >>>> then use a simply "if (legacy_binary_is_64bit())" in the type_init >>>> function instead? >>> >>> Sounds like a good idea. >>> >>> So we would have DEFINE_TYPES() and DEFINE_TYPES64() macros ? >> >> Either that - or simply use type_init() directly here for the time being. > > As Pierrick noted on private chat, my approach doesn't scale, I should > use smth in the lines of: > > }, { > .name = MACHINE_TYPE_NAME("raspi2b"), > .parent = TYPE_RASPI_MACHINE, > .registerable = qemu_binary_has_target_arm, > .class_init = raspi2b_machine_class_init, > }, { > .name = MACHINE_TYPE_NAME("raspi3ap"), > .parent = TYPE_RASPI_MACHINE, > .registerable = qemu_binary_has_target_aarch64, > .class_init = raspi3ap_machine_class_init, > }, { > > Having: > > bool qemu_binary_has_target_arm(void) > { > return qemu_arch_available(QEMU_ARCH_ARM); > } I don't know where this is going and what are the details here but why add yet another one line function that calls another one that's identical to a third one. Why not put in TypeInfo .arch = QEMU_ARCH_ARM then compare to that when needed? Although it's questionable if arch belongs to QOM TypeInfo and not in Device or Machine instead this may be needed if you have to decide on this when registering types. I'm not sure about what .registerable means here but more common spelling might be .registrable (which suggests this could be problematic in practice so maybe try to find a better name for this). Regards, BALATON Zoltan > Now back to Thomas suggestion, we could define 2 TypeInfo arrays, > but I foresee lot of code churn when devices has to be made > available on different setup combinations; so with that in mind > the QOM registerable() callback appears a bit more future proof. > >
On 05/03/2025 20.07, Philippe Mathieu-Daudé wrote: > On 5/3/25 19:35, Thomas Huth wrote: >> On 05/03/2025 19.12, Cédric Le Goater wrote: >>> On 3/5/25 18:40, Thomas Huth wrote: >>>> On 05/03/2025 17.12, Philippe Mathieu-Daudé wrote: >>>>> For legacy ARM binaries, legacy_binary_is_64bit() is >>>>> equivalent of the compile time TARGET_AARCH64 definition. >>>>> >>>>> Use it as TypeInfo::registerable() callback to dynamically >>>>> add Aarch64 specific types in qemu-system-aarch64 binary, >>>>> removing the need of TARGET_AARCH64 #ifdef'ry. >>>>> >>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>>>> --- >>>>> hw/arm/bcm2836.c | 6 ++---- >>>>> hw/arm/raspi.c | 7 +++---- >>>>> 2 files changed, 5 insertions(+), 8 deletions(-) >>>>> >>>>> diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c >>>>> index 95e16806fa1..88a32e5fc20 100644 >>>>> --- a/hw/arm/bcm2836.c >>>>> +++ b/hw/arm/bcm2836.c >>>>> @@ -12,6 +12,7 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qapi/error.h" >>>>> #include "qemu/module.h" >>>>> +#include "qemu/legacy_binary_info.h" >>>>> #include "hw/arm/bcm2836.h" >>>>> #include "hw/arm/raspi_platform.h" >>>>> #include "hw/sysbus.h" >>>>> @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, >>>>> void *data) >>>>> dc->realize = bcm2836_realize; >>>>> }; >>>>> -#ifdef TARGET_AARCH64 >>>>> static void bcm2837_class_init(ObjectClass *oc, void *data) >>>>> { >>>>> DeviceClass *dc = DEVICE_CLASS(oc); >>>>> @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, >>>>> void *data) >>>>> bc->clusterid = 0x0; >>>>> dc->realize = bcm2836_realize; >>>>> }; >>>>> -#endif >>>>> static const TypeInfo bcm283x_types[] = { >>>>> { >>>>> @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { >>>>> .name = TYPE_BCM2836, >>>>> .parent = TYPE_BCM283X, >>>>> .class_init = bcm2836_class_init, >>>>> -#ifdef TARGET_AARCH64 >>>>> }, { >>>>> .name = TYPE_BCM2837, >>>>> .parent = TYPE_BCM283X, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = bcm2837_class_init, >>>>> -#endif >>>>> }, { >>>>> .name = TYPE_BCM283X, >>>>> .parent = TYPE_BCM283X_BASE, >>>>> diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c >>>>> index dce35ca11aa..f7e647a9cbf 100644 >>>>> --- a/hw/arm/raspi.c >>>>> +++ b/hw/arm/raspi.c >>>>> @@ -15,6 +15,7 @@ >>>>> #include "qemu/osdep.h" >>>>> #include "qemu/units.h" >>>>> #include "qemu/cutils.h" >>>>> +#include "qemu/legacy_binary_info.h" >>>>> #include "qapi/error.h" >>>>> #include "hw/arm/boot.h" >>>>> #include "hw/arm/bcm2836.h" >>>>> @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass >>>>> *oc, void *data) >>>>> raspi_machine_class_init(mc, rmc->board_rev); >>>>> }; >>>>> -#ifdef TARGET_AARCH64 >>>>> static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) >>>>> { >>>>> MachineClass *mc = MACHINE_CLASS(oc); >>>>> @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass >>>>> *oc, void *data) >>>>> rmc->board_rev = 0xa02082; >>>>> raspi_machine_class_init(mc, rmc->board_rev); >>>>> }; >>>>> -#endif /* TARGET_AARCH64 */ >>>>> static const TypeInfo raspi_machine_types[] = { >>>>> { >>>>> @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { >>>>> .name = MACHINE_TYPE_NAME("raspi2b"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> .class_init = raspi2b_machine_class_init, >>>>> -#ifdef TARGET_AARCH64 >>>>> }, { >>>>> .name = MACHINE_TYPE_NAME("raspi3ap"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = raspi3ap_machine_class_init, >>>>> }, { >>>>> .name = MACHINE_TYPE_NAME("raspi3b"), >>>>> .parent = TYPE_RASPI_MACHINE, >>>>> + .registerable = legacy_binary_is_64bit, >>>>> .class_init = raspi3b_machine_class_init, >>>>> -#endif >>>>> }, { >>>>> .name = TYPE_RASPI_MACHINE, >>>>> .parent = TYPE_RASPI_BASE_MACHINE, >>>> >>>> Uh, this (together with patch 1) looks very cumbersome. Why don't you >>>> simply split the array into two, one for 32-bit and one for 64-bit, and >>>> then use a simply "if (legacy_binary_is_64bit())" in the type_init >>>> function instead? >>> >>> Sounds like a good idea. >>> >>> So we would have DEFINE_TYPES() and DEFINE_TYPES64() macros ? >> >> Either that - or simply use type_init() directly here for the time being. > > As Pierrick noted on private chat, my approach doesn't scale, I should > use smth in the lines of: > > }, { > .name = MACHINE_TYPE_NAME("raspi2b"), > .parent = TYPE_RASPI_MACHINE, > .registerable = qemu_binary_has_target_arm, > .class_init = raspi2b_machine_class_init, > }, { > .name = MACHINE_TYPE_NAME("raspi3ap"), > .parent = TYPE_RASPI_MACHINE, > .registerable = qemu_binary_has_target_aarch64, > .class_init = raspi3ap_machine_class_init, > }, { > > Having: > > bool qemu_binary_has_target_arm(void) > { > return qemu_arch_available(QEMU_ARCH_ARM); > } > > Now back to Thomas suggestion, we could define 2 TypeInfo arrays, > but I foresee lot of code churn when devices has to be made > available on different setup combinations; so with that in mind > the QOM registerable() callback appears a bit more future proof. Honestly, in this case, I'd rather prefer some code churn now instead of having a unnecessary callback interface until forever! Just my 0.02 €. Thomas
diff --git a/hw/arm/bcm2836.c b/hw/arm/bcm2836.c index 95e16806fa1..88a32e5fc20 100644 --- a/hw/arm/bcm2836.c +++ b/hw/arm/bcm2836.c @@ -12,6 +12,7 @@ #include "qemu/osdep.h" #include "qapi/error.h" #include "qemu/module.h" +#include "qemu/legacy_binary_info.h" #include "hw/arm/bcm2836.h" #include "hw/arm/raspi_platform.h" #include "hw/sysbus.h" @@ -195,7 +196,6 @@ static void bcm2836_class_init(ObjectClass *oc, void *data) dc->realize = bcm2836_realize; }; -#ifdef TARGET_AARCH64 static void bcm2837_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); @@ -208,7 +208,6 @@ static void bcm2837_class_init(ObjectClass *oc, void *data) bc->clusterid = 0x0; dc->realize = bcm2836_realize; }; -#endif static const TypeInfo bcm283x_types[] = { { @@ -219,12 +218,11 @@ static const TypeInfo bcm283x_types[] = { .name = TYPE_BCM2836, .parent = TYPE_BCM283X, .class_init = bcm2836_class_init, -#ifdef TARGET_AARCH64 }, { .name = TYPE_BCM2837, .parent = TYPE_BCM283X, + .registerable = legacy_binary_is_64bit, .class_init = bcm2837_class_init, -#endif }, { .name = TYPE_BCM283X, .parent = TYPE_BCM283X_BASE, diff --git a/hw/arm/raspi.c b/hw/arm/raspi.c index dce35ca11aa..f7e647a9cbf 100644 --- a/hw/arm/raspi.c +++ b/hw/arm/raspi.c @@ -15,6 +15,7 @@ #include "qemu/osdep.h" #include "qemu/units.h" #include "qemu/cutils.h" +#include "qemu/legacy_binary_info.h" #include "qapi/error.h" #include "hw/arm/boot.h" #include "hw/arm/bcm2836.h" @@ -367,7 +368,6 @@ static void raspi2b_machine_class_init(ObjectClass *oc, void *data) raspi_machine_class_init(mc, rmc->board_rev); }; -#ifdef TARGET_AARCH64 static void raspi3ap_machine_class_init(ObjectClass *oc, void *data) { MachineClass *mc = MACHINE_CLASS(oc); @@ -387,7 +387,6 @@ static void raspi3b_machine_class_init(ObjectClass *oc, void *data) rmc->board_rev = 0xa02082; raspi_machine_class_init(mc, rmc->board_rev); }; -#endif /* TARGET_AARCH64 */ static const TypeInfo raspi_machine_types[] = { { @@ -402,16 +401,16 @@ static const TypeInfo raspi_machine_types[] = { .name = MACHINE_TYPE_NAME("raspi2b"), .parent = TYPE_RASPI_MACHINE, .class_init = raspi2b_machine_class_init, -#ifdef TARGET_AARCH64 }, { .name = MACHINE_TYPE_NAME("raspi3ap"), .parent = TYPE_RASPI_MACHINE, + .registerable = legacy_binary_is_64bit, .class_init = raspi3ap_machine_class_init, }, { .name = MACHINE_TYPE_NAME("raspi3b"), .parent = TYPE_RASPI_MACHINE, + .registerable = legacy_binary_is_64bit, .class_init = raspi3b_machine_class_init, -#endif }, { .name = TYPE_RASPI_MACHINE, .parent = TYPE_RASPI_BASE_MACHINE,
For legacy ARM binaries, legacy_binary_is_64bit() is equivalent of the compile time TARGET_AARCH64 definition. Use it as TypeInfo::registerable() callback to dynamically add Aarch64 specific types in qemu-system-aarch64 binary, removing the need of TARGET_AARCH64 #ifdef'ry. Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> --- hw/arm/bcm2836.c | 6 ++---- hw/arm/raspi.c | 7 +++---- 2 files changed, 5 insertions(+), 8 deletions(-)