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 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