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

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

On 5/1/25 4:35 PM, BALATON Zoltan wrote:
> On Mon, 28 Apr 2025, Pierrick Bouvier wrote:
>> 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.
> 
> You can pick the patch from this series I've just posted:
> https://patchew.org/QEMU/cover.1746139668.git.balaton@eik.bme.hu/ad355178b2a3fe285854ed2e25b288baf0fd6e05.1746139668.git.balaton@eik.bme.hu/
> it is used in patch 12 of that series for an example.
>

Sounds good, thanks.

@Philippe, could you cherry-pick this patch as part of next iteration?

> Regards,
> BALATON Zoltan