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

[Pierrick Bouvier <pierrick.bouvier@linaro.org>]

On 4/25/25 02:43, BALATON Zoltan wrote:
> 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.
> 

This is exactly what we have in this series.
Patch 7 introduces DEFINE_MACHINE_WITH_INTERFACES.
I guess Philippe chose a new name to avoid modifying all existing 
DEFINE_MACHINE, and I think it's understandable, as we want those 
changes to impact hw/arm only first. That said, it would be very easy to 
refactor/modify later, so it's not a big deal.

This patch introduces DEFINE_MACHINE_ARM_AARCH64 and DEFINE_MACHINE_AARCH64.

Is the problem with those specialized DEFINE_MACHINE_{ARM, AARCH64} 
definition?
If yes, and if you prefer an explicit 
DEFINE_MACHINE_WITH_INTERFACES(..., arm_aarch64_machine_interfaces), I'm 
sure Philippe would be open to make such a change to satisfy reviews.

Let's just try to decide something, and move on.

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

This macro is not used for any machine definition so far, and 
DEFINE_MACHINE is the "standard" macro used, at least the one most 
commonly used in the codebase. So it makes sense to simply expand the 
latter.

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

If you refer to "DEFINE_MACHINE_EXTENDED(name, parent, initfn, 
interfaces...)", this is almost exactly what patch 7 is introducing with
DEFINE_MACHINE_WITH_INTERFACES(namestr, machine_initfn, ifaces).

> Regards,
> BALATON Zoltan