Re: [RFC PATCH] hw/core: Avoid attaching qdevs to /machine/unattached if they have a bus

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Feb 19, 2026 at 09:49:05AM +0100, Markus Armbruster wrote:
>> Thomas Huth <thuth@redhat.com> writes:
>> 
>> > From: Thomas Huth <thuth@redhat.com>
>> >
>> > We still have a lot of devices that end up in /machine/unattached in
>> > case the caller forgot to use object_property_add_child() to add it
>> > to a proper location in the QOM tree.
>> 
>> This is QOM papering over sloppy modeling.  Predictably, it has enabled
>> us to remain sloppy slobs.
>> 
>> I think the decision to paper over sloppiness to get QOM off the ground
>> quickly was defensible back then.  It's a lot less defensible now that
>> QOM has been off the ground for more than a decade.
>> 
>> I believe people are by and large unaware of the need to add children.
>> This risks further accumulation of technical debt.
>
> IMHO, it is slightly more subtle - people believe they are already
> adding children.
>
> Consider this code.
>
>     port92 = isa_create_simple(isa_bus, TYPE_PORT92);
>
> my reading of that is that I'm creating a "port92" device that is a
> child of "isa_bus". Why would I need to tell QEMU that it is a child
> for a second time ?
>
> If I trace calls through isa_create_simple I get to a call to:
>
>    qdev_realize_and_unref(dev, parent, errp);

Actually

    qdev_realize_and_unref(&dev->parent_obj, &bus->parent_obj, errp)

in isa_realize_and_unref().

> and once I again I'm left wondering why I would need to tell
> QEMU 'dev' is a child of 'parent' a second time.

Beware of confusion around "parent" here.

qdev_realize_and_unref() takes a qdev and optionally a qbus (a
DeviceState * and a BusState *).  qdev_realize() plugs the qdev into the
qbus with qdev_set_parent_bus() if the qbus is non-null.

The qdev concept "parent bus" is distinct from the QOM concept "parent
in the composition tree".  Evidence: the QOM parent of a qdev created
with -device is either "/machine/peripheral" or
"/machine/peripheral-anon".  Its parent qbus is something else.

"info qtree" shows the tree of qdevs and qbuses rooted at the main
system bus.

"info qom-tree <root>" shows the QOM composition tree rooted at <root>
(default "/machine").

> Of course I know the answer. We need to give a name for the
> child and it isn't trivial to "do the right thing" to invent
> an automatic name.

At least not in the general case.

> Still, overall I'm inclined to largely blame our API designs for
> not guiding people into doing the right thing.

Indeed!

> Picking another random unattached set of objects "smbus-eeprom"
> I again find they've being created with qdev_realize_and_unref.
>
> Pick another unattached device 'i8259_init_chip', and again
> we end up calling into qdev_realize_and_unref()
>
>
> It feels like qdev_realize_and_unref() is a common point of
> blame in unattached devices. IMHO it ought to be taking a
> "const char *name" parameter.

The problem are onboard devices without QOM composition tree parents.

Onboard devices need to be realized with qdev_realize().  Often called
via qdev_realize_and_unref().

Currently, we use that chokepoint to make devices without QOM parents
children of /machine/unattached.  That's bad.

Thomas's RFC PATCH changes this for devices that plug into a qbus: make
them children of the qbus (which is a QOM object) instead, with a
made-up name.  This may or may not be the parent we'd pick manually.

I'm curious: are there devices that plug into a qbus with a QOM parent
other than that qbus?

You propose to require callers to pass a name.

I think passing a name makes intent explicit: the qbus is the parent we
want.

> There are 104 calls to qdev_realize_and_unref(), but it is in
> the call path of many more wrapped calls.
>
> A big-ish job to convert them all, so perhaps we need to add
> a parallel
>
>    qdev_realize_and_unref_child(...)
>
> with the new 'name' parameter, and incrementally convert stuff,
> though ideally a conversion that doesn't last "forever" like
> so many of our conversion tasks.

There's just one way to get a feel for how big a job this is: try it.

>> To really put a stop to it, we'd have to mark the existing misuses, then
>> warn or crash on umarked misuse.
>
> While marking / warning / crashing helps surface problems, I think
> that fixing the API designs to guide developers at compile time is
> more important.

It's more helpful, so if we can pull it off...

>> None of the above is an objection to your patch.
>
> I guess even with the patch applied we can still identify broken
> code by looking for any child with an "x-" property name.

Yes.