|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 00/15] hw: expand DEFINE_MACHINE to explicit TypeInfo registration
On Sun, 1 Nov 2026, Yonggang Luo wrote: On Sun, Nov 1, 2026 at 7:29 AM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:On Sun, 1 Nov 2026, Yonggang Luo wrote:On Sun, Nov 1, 2026 at 1:06 AM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:On Sat, 31 Oct 2026, Yonggang Luo wrote:On Sat, Oct 31, 2026 at 8:42 PM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:On Fri, 30 Oct 2026, Pierrick Bouvier wrote:On 10/30/2026 4:09 PM, BALATON Zoltan wrote:On Fri, 30 Oct 2026, Yonggang Luo wrote:DEFINE_MACHINE and DEFINE_MACHINE_EXTENDED hide the TypeInfo and the type_init function that calls type_register_static(). A later change needs to pass aThat's the point of the macro to hide the boiler plate code.target mask into that registration, and the macros have no parameter for it.Couldn't you just change the macros to take that parameter instead of expanding it everywhere?Problem of macros is that they don't have default parameters, so you need variants for everything: DEFINE_MACHINE DEFINE_MACHINE_EXTENDED DEFINE_MACHINE_WITH_INTERFACES DEFINE_MACHINE_WITH_INTERFACE_ARRAY and now would need a new variant, with variant for other existing ones. In the end, it's just simpler to remove this and add the boilerplate. It's written once and never modified, so using a macro just save typing those lines once.Regards, BALATON ZoltanThis series expands every caller first, with no behavior change, so the follow-up only replaces the type_register_static() call. Each machine file gains a class_init function, a TypeInfo, and a type_init function that calls type_register_static(). The last patch removes DEFINE_MACHINE* and DEFINE_SECURE_MACHINE* from include/hw/core/boards.h.Macros help reading the code not saving typing. If the actual functionality is buried in a lot of boiler plate it's harder to understand and spot bugs so putting the unintresting boiler plate behind a macro is better even if you need a separate version for different cases. Then the code that uses the macro will be much more obvious and not just a lot ofI think mainly because DEFINE_MACHINE* and DEFINE_SECURE_MACHINE* are not defined properly that causes them to not be extended easily. DEFINE_TYPES is another example, use them can extend easily, how about always use DEFINE_TYPES? Or add a DEFINE_TYPE to replace DEFINE_MACHINE* and DEFINE_SECURE_MACHINE*?Maybe I'm missing the problem but DEFINE_MACHINE macros are just convenience wrappers around DEFINE_MACHINE_EXTENDED. So why can't you add the needed parameter to DEFINE_MACHINE_EXTENDED and use that for machines that need it, or if it's needed by all machines then make the new parameter required and add it to all convenience wrappers too so you can keep using DEFINE_MACHINE* and just need to add one parameter to it in machines?That's the issue, add parameter to DEFINE_MACHINE_EXTENDED means all calls to it also need to be changed, that means touch many $target_arch files. currently it's 13. But after theseThat's not different from expanding the macro which would touch the same files.changes, there will be much more usage on it, and if we want to add more parameters or attributes to it, it will be a new tree-wide change.Do you expect to need more parameters? Can't you add them at once then?And DEFINE_TYPES do not suffer from such issues. As the TypeInfo[] is explicit defined for DEFINE_TYPESMy point is that keeping the boiler plate as little as possible makes the code more readable. DEFINE_TYPES hides some of the boiler plate, DEFINE_MACHINE hides even more so converting to DEFINE_TYPES would expose some even if it's better than no macro at all. What exactly are the parameters missing from DEFINE_MACHINE_EXTENDED that need to be added?Hi BALATON the things we faced is we first is to add TypeInfo::is_available parameter to DEFINE_MACHINE_EXTENDED, and have more usage of it. Then we will add targets:uint64_t parameter to DEFINE_MACHINE_EXTENDED, this is a prepare to remove TypeInfo::is_available parameter from DEFINE_MACHINE_EXTENDED, And finally, once all usage of TypeInfo::is_available can be replaced with targets:uint64_t, then we finally need to remove TypeInfo::is_available parameter from DEFINE_MACHINE_EXTENDED, Is there a way to avoid the temporary is_available churn and introduce the targets:uint64_t right away? (I'm not following this so I don't really know what you're talking about just looks like there's some churn here which might be good to awoid anyway independent of these macros.) That's not all DEFINE_MACHINE hides though. It's longer with .abstract, .secure and .iterfaces plus the machine initfn that would also need to be expanded if you used DEFINE_TYPES. Is this conversion needed for all machines using all DEFINE_MACHINE variants or only some of them? If the latter why not convert those to DEFINE_MACHINE_EXTENDED and then only that macro needs to be changed with other wrappers passing a default value without needing a new parameter. That does not work if all machines need to do this but even then, keeping the macro and adding a new required parameter is better in my opinion as it keeps boiler plate hidden. Regards, BALATON Zoltan And the .instance_size = sizeof(MachineState), is indeed not needed, they sharing TYPE_MACHINE's sizeof(MachineState) is enough. Even though DEFINE_MACHINE expands to .instance_size = sizeof(MachineState), and it's redundant. So I think convert to DEFINE_TYPES would be a good alternative to this issue, less boilerplate(not none) And dealt with the issue we faced. What do you think about this idea, BALATONRegards, BALATON Zoltan
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |