Re: [PATCH 00/15] hw: expand DEFINE_MACHINE to explicit TypeInfo registration

[Yonggang Luo <luoyonggang@xxxxxxxxx>]

On Tue, Nov 3, 2026 at 9:26 PM BALATON Zoltan <balaton@xxxxxxxxxx> wrote:
>
> On Tue, 3 Nov 2026, BALATON Zoltan wrote:
> > 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.
>
> I noticed there's some redundancy between the arch and the name prefix
> with this change. Basically you're trying to split namestr into name and
> arch where the final machine name will be arch-name. Is there a way to do
> this from the macro somehow so you don't have to specify the arch twice
> once in the name and once as an enum?

There won't be redundancy, the change's intention is to keep the
machine QOM-name not
changed, specifically, drop MR
https://lore.kernel.org/qemu-devel/20261027111216.392-1-luoyonggang@xxxxxxxxx/
so we can keep the QOM name for machines not changed by introduce a
extra targets parameter
to it, then we can use buckets to storage different arch/target, I've
already implemented
that at 
https://gitlab.com/lygstate-qemu/qemu/-/commits/qom-type-buckets?ref_type=heads,
 and it's working well:)

I am trying to get it merged by sending smaller patches.

Basically we always need a "targets" parameter or
"TargetInfo::is_avaiable" attributes.
the targets parameter will be saved into TargetImpl::targets, and not
in TargetInfo, as the default
value for TargetInfo is 0, means no targets. but we want keep
"QEMU_ARCH_ALL" default behavior, that's why
I want it to be a parameter, not a TargetInfo member

>
> Regards,
> BALATON Zoltan



-- 
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo