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

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

On 4/28/25 6:10 PM, BALATON Zoltan wrote:
> On Mon, 28 Apr 2025, Pierrick Bouvier wrote:
>> On 4/28/25 11:44 AM, BALATON Zoltan wrote:
>>> On Mon, 28 Apr 2025, Pierrick Bouvier wrote:
>>>> On 4/28/25 3:31 AM, BALATON Zoltan wrote:
>>>>> Since you are touching the lines using DEFINE_MACHINE it's a good
>>>>> opportunity to change the macro to be more general to be able to keep
>>>>> using it instead of replacing it with the boiler plate it's supposed to
>>>>> hide. Adding one or two more parameters to the macro is not a big change
>>>>> so I don't see why you don't want to do it. This could be addressed later
>>>>> to revert to use the macro again but in practice it will not be addressed
>>>>> because everybody will be busy doing other things and doing that now
>>>>> would
>>>>> prevent some churn. I too, don't like doing unrelated clean up which is
>>>>> not the main goal, but if it's not much more work then it's not
>>>>> unreasonable to do it. I only oppose to that if it's a lot of work so I
>>>>> would not ask such change but what I asked is not unrelated and quite
>>>>> simple change.
>>>>>
>>>>> That said, I can't stop you so if you still don't want to do it now then
>>>>> you can move on. I don't care that much as long as you stay within
>>>>> hw/arm,
>>>>> but will raise my concern again when you submit a similar patch that
>>>>> touches parts I care more about. If others don't think it's a problem and
>>>>> not bothered by the boiler plate code then it's not so important but
>>>>> otherwise I think I have a valid point. I remember when I started to get
>>>>> to know QEMU it was quite difficult to wade through all the QOM boiler
>>>>> plate just to see what is related to the actual functionality. These
>>>>> macros help to make code more readable and accessible for new people.
>>>>
>>>> Having been through that recently, I agree with you that it can be hard to
>>>> follow at first. Luckily, we have perfect compiler based completion for
>>>> all
>>>> editors those days (I sincerely hope everyone spent 2 hours configuring
>>>> this
>>>> on their own favorite one), and it's easy to see where things are defined
>>>> and
>>>> used, even when code is cryptic.
>>>
>>> It's not about typing but reading it. The verbose struct definitions are
>>> hard to follow and makes board code look more complex than it should be.
>>>
>>>> That said, pushing to someone adding a new field the responsibility of
>>>> cleaning up the whole thing is not a fair request. You can't expect your
>>>> friends to clean your shared house because they brought a cake for dinner.
>>>
>>> I tend to get such requests to clean up unrelated things whenever I try to
>>> change anything in PPC Mac emulation which I also complain about and think
>>> is not reasonable to ask. But I did not ask for unrelated cleanup here and
>>> changing the patch so you don't do this:
>>>
>>> -DEFINE_MACHINE("none", machine_none_machine_init)
>>> +static const TypeInfo null_machine_types[] = {
>>> +    {
>>> +        .name           = MACHINE_TYPE_NAME("none"),
>>> +        .parent         = TYPE_MACHINE,
>>> +        .class_init     = null_machine_class_init,
>>> +    },
>>> +};
>>> +
>>> +DEFINE_TYPES(null_machine_types)
>>>
>>> but instead add the .interfaces field to a variant of DEFINE_MACHINE once
>>> and keep the one line definition is not something unreasonable to ask. I
>>> think you can ask your friends to not make a mess in the shared house
>>> while having a party or at least clean up after that. Adding one more
>>> parameter to the macro is also simple to do so I don't get why you're so
>>> opposed to this.
>>>
>>
>> Maybe there is a misunderstanding on my side, but it seems that what you
>> asked is exactly patch 7, which introduce DEFINE_MACHINE_WITH_INTERFACES.
> 
> Almost but not quite. I don't know why I can't get this through to you. If
> you compare patch 7 to how DO_OBJECT_DEFINE_TYPE_EXTENDED is defined do
> you notice the difference in how .interfaces is set? With the same way as
> in DO_OBJECT_DEFINE_TYPE_EXTENDED you don't need separate InterfaceInfo
> arm_aarch64_machine_interfaces[] definitions or different macros in the
> next patch just list the needed interfaces in the machine definitions.
>

I'm sorry, I don't understand what you want exactly, despite asking 
several times.
I think it would be more clear if you could apply this series on your 
side, write a small patch showing *exactly* what you expect, and 
applying this to one of the board concerned. Then, we can do the change 
you request.

> Regards,
> BALATON Zoltan