[Top] [All Lists]
[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 Tue, 3 Nov 2026, Yonggang Luo wrote:

On Mon, Nov 2, 2026 at 10:56 PM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:


On Mon, 2 Nov 2026, Yonggang Luo wrote:

On Mon, Nov 2, 2026 at 4:57 AM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:


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 a


That'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 Zoltan


This 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 of


I 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 these


That'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_TYPES


My 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 what I am facing now. And the explicit
static const TypeInfo xxx_machine_typeinfo = {
   .name       = MACHINE_TYPE_NAME("xxx"),
   .parent     = TYPE_MACHINE,
   .class_init = xxx_machine_class_init,
};

Will help me avoid the issue. And I think this is a little boilerplate
and very explicit.


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


At the end, only "none" machine do not have targets parameter or
(QEMU_ARCH_ALL), as that's the sole
machine that supports all targets.


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.


Another option is keep DEFINE_MACHINE and introduce DEFINE_MACHINE_TARGETS
and remove DEFINE_MACHINE_EXTENDED, and we only use DEFINE_MACHINE for
simple case,
that is parent is TYPE_MACHINE and have no custom type and no more
attributes assigned.
DEFINE_MACHINE_EXTENDED is too verbose, when calling it, it less clear
than calling to DEFINE_MACHINE.
And if we want to set more attributes to TypeInfo, just expand it to
use type_init or DEFINE_TYPES, how about this way.

This will be something like:

#define DEFINE_MACHINE_TARGETS(namestr, machine_initfn, targets) \
   static void machine_initfn##_class_init(ObjectClass *oc, const void *data) \
   { \
       MachineClass *mc = MACHINE_CLASS(oc); \
       machine_initfn(mc); \
   } \
   static const TypeInfo machine_initfn##_typeinfo = { \
       .name       = MACHINE_TYPE_NAME(namestr), \
       .parent     = TYPE_MACHINE, \
       .class_init = machine_initfn##_class_init, \
   }; \
   static void machine_initfn##_register_types(void) \
   { \
       type_register_targets(&machine_initfn##_typeinfo, targets); \
   } \
   type_init(machine_initfn##_register_types)

/* Implicitly insecure */
#define DEFINE_MACHINE(namestr, machine_initfn) \
   DEFINE_MACHINE_TARGETS(namestr, machine_initfn, QEMU_ARCH_ALL) \

As the many call is to DEFINE_MACHINE.

And after all conversion, all existing usage of DEFINE_MACHINE will be
converted to
DEFINE_MACHINE_TARGETS.
DEFINE_MACHINE will be called for ('none') machine or future heterogeneous
machines


Either way, these patches will be dropped. We will introduce
DEFINE_MACHINE_TARGETS first.


I think I'm lost and cannot follow what you want. As far as I understood
you want to add a new field "target" to machine TypeInfo but instead of
adding that straight away you proposed to first add is_available then
later replace that with target. I asked why do you need is_available and
not add target right away?

Then if this target parameter will be needed for all machines why can't
you add that to all DEFINE_MACHINE macros as a required parameter and set
it for all machines passing it through to DEFINE_MACHINE_EXTENDED which
sets in the TypeInfo. That way no machines need changing other than adding
the target parameter to the existing DEFINE_MACHINE calls.

What am I missing?

Or sorry, I did not explain well.

I means reshape DEFINE_MACHINE like

https://lore.kernel.org/qemu-devel/20261102194826.1298-1-luoyonggang@xxxxxxxxx/T/#u

basically every call to DEFINE_MACHINE needs the targets parameter.
So we can always passing targets parameter for DEFINE_MACHINE.
That's OK and is what I suggested, the part I did not get is why do you need an intermediate is_available step instead of just adding the target parameter to all DEFINE_MACHINE* macros. 


And I think we can remove DEFINE_MACHINE_EXTENDED, as the call to it is verbose
You can't (without bringing back boiler plate) because some machines need finer control. That's why we have DEFINE_MACHINE_EXTENDED with all possible parameters and convenience wrappers around it for simpler cases. It may be possible to reorganise the macros dropping some unused DEFINE_MACHINE variants and introducing new ones that better suit the needs of machines currently using DEFINE_MACHINE_EXTENDED but just expanding all boiler plate for more complex machines not using DEFINE_MACHINE is what I'd like to avoid. 

Regards,
BALATON Zoltan

 

©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.

Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.