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

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

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.

Regards,
BALATON Zoltan