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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.