Re: [RFC PATCH 2/4] hw/arm/raspi: Replace TARGET_AARCH64 by legacy_binary_is_64bit()

[Thomas Huth <thuth@redhat.com>]

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