Re: [RFC PATCH v5 08/21] hw/arm: Add DEFINE_MACHINE_[ARM_]AARCH64() macros

[BALATON Zoltan <balaton@eik.bme.hu>]

[-- Attachment #1: Type: text/plain, Size: 3982 bytes --]

On Thu, 24 Apr 2025, Pierrick Bouvier wrote:
> On 4/24/25 17:16, BALATON Zoltan wrote:
>> On Fri, 25 Apr 2025, Philippe Mathieu-Daudé wrote:
>>> A machine defined with the DEFINE_MACHINE_ARM_AARCH64() macro
>>> will be available on qemu-system-arm and qemu-system-aarch64
>>> binaries.
>>> 
>>> One defined with DEFINE_MACHINE_AARCH64() will only be available
>>> in the qemu-system-aarch64 binary.
>>> 
>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> ---
>>> include/hw/arm/machines-qom.h | 13 +++++++++++++
>>> target/arm/machine.c          | 12 ++++++++++++
>>> 2 files changed, 25 insertions(+)
>>> 
>>> diff --git a/include/hw/arm/machines-qom.h b/include/hw/arm/machines-qom.h
>>> index a17225f5f92..6277ee986d9 100644
>>> --- a/include/hw/arm/machines-qom.h
>>> +++ b/include/hw/arm/machines-qom.h
>>> @@ -9,10 +9,23 @@
>>> #ifndef HW_ARM_MACHINES_QOM_H
>>> #define HW_ARM_MACHINES_QOM_H
>>> 
>>> +#include "hw/boards.h"
>>> +
>>> #define TYPE_TARGET_ARM_MACHINE \
>>>          "target-info-arm-machine"
>>> 
>>> #define TYPE_TARGET_AARCH64_MACHINE \
>>>          "target-info-aarch64-machine"
>>> 
>>> +extern InterfaceInfo arm_aarch64_machine_interfaces[];
>>> +extern InterfaceInfo aarch64_machine_interfaces[];
>>> +
>>> +#define DEFINE_MACHINE_ARM_AARCH64(namestr, machine_initfn) \
>>> +        DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \
>>> +                                       arm_aarch64_machine_interfaces)
>>> +
>>> +#define DEFINE_MACHINE_AARCH64(namestr, machine_initfn) \
>>> +        DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, \
>>> +                                       aarch64_machine_interfaces)
>>> +
>>> #endif
>>> diff --git a/target/arm/machine.c b/target/arm/machine.c
>>> index 978249fb71b..193c7a9cff0 100644
>>> --- a/target/arm/machine.c
>>> +++ b/target/arm/machine.c
>>> @@ -8,6 +8,7 @@
>>> #include "cpu-features.h"
>>> #include "migration/cpu.h"
>>> #include "target/arm/gtimer.h"
>>> +#include "hw/arm/machines-qom.h"
>>> 
>>> static bool vfp_needed(void *opaque)
>>> {
>>> @@ -1111,3 +1112,14 @@ const VMStateDescription vmstate_arm_cpu = {
>>>          NULL
>>>      }
>>> };
>>> +
>>> +InterfaceInfo arm_aarch64_machine_interfaces[] = {
>>> +    { TYPE_TARGET_ARM_MACHINE },
>>> +    { TYPE_TARGET_AARCH64_MACHINE },
>>> +    { }
>>> +};
>>> +
>>> +InterfaceInfo aarch64_machine_interfaces[] = {
>>> +    { TYPE_TARGET_AARCH64_MACHINE },
>>> +    { }
>>> +};
>> 
>> Why do you need these? If you define DEFINE_MACHINE_WITH_INTERFACES as
>> OBJECT_DEFINE_TYPE_WITH_INTERFACES then you can write:
>> 
>
> This was requested in v4 by Richard to remove anonymous array duplication in 
> .data.
>
>> DEFINE_MACHINE_WITH_INTERFACES(name, initfn, { TYPE_TARGET_ARM_MACHINE },
>>       { TYPE_TARGET_AARCH64_MACHINE }, { })
>> 
>> and no more macros needed. Ideally those places that are now blown up
>> should use DEFINE_MACHINE too. Maybe they don't yet because the parent
>> type  is hardcoded so we should really have
>> 
>
> Not sure what you mean by "no more macros needed".

No other specialised macros needed for each machine type other than 
DEFINE_MACHINE_WITH_INTERFACES or DEFINE_MACHINE_EXTENDED. So I suggested 
to keep DEFINE_MACHINE by making it more general so it can cover the new 
uses instead of bringing back the boiler plate and losing the clarity 
hinding these behind the macros.

> arm_aarch64_machine_interfaces or aarch64_machine_interfaces are arrays 
> (defined only once), which are passed as a parameter to 
> DEFINE_MACHINE_WITH_INTERFACES, or manually set with ".interfaces =".

Look at how OBJECT_DEFINE_TYPE_WITH_INTERFACES is defined.

>> DEFINE_MACHINE_EXTENDED(name, parent, initfn, interfaces...)
>> 
>> and remove more bolier plate that way?
>> 
>
> Could you can share a concrete example of what you expect, with the new 
> macros to add, and how to use them for a given board?

I tried to do that in this message you replied to.

Regards,
BALATON Zoltan