Re: [PATCH 2/2] qom: Manage references to embedded child objects

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Jun 15, 2026 at 02:42:10PM +0100, Mark Cave-Ayland wrote:
> On 15/06/2026 10:28, Peter Maydell wrote:
> 
> > On Mon, 15 Jun 2026 at 09:35, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > 
> > > On Mon, Jun 15, 2026 at 09:31:36AM +0100, Peter Maydell wrote:
> > > > On Mon, 15 Jun 2026 at 09:13, Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > > > Do we know how many examples we have of embedding objects inside
> > > > > another ?
> > > > 
> > > > It is an extremely common pattern for device and SoC model
> > > > implementations; we've been recommending it for years.
> > > > 
> > > > > I would much prefer if we forbid the embedding of objects. It is
> > > > > horrible design practice to have some QOM objects which can be
> > > > > freed via reference count and some which cannot.
> > > > 
> > > > That would be a very large amount of code to rewrite to the
> > > > new paradigm. I don't object inherently ("you have pointers to
> > > > your child objects" works better when they might be implemented
> > > > in Rust and might play better with being able to create
> > > > machines and wire them up on the command line); I'm just
> > > > noting how much work it would be if you wanted to make
> > > > embedding forbidden.
> > > 
> > > Would it be a more tractable problem to "fix" object structure only
> > > incrementally as they gain a need to be managed/reference from
> > > Rust code, or does the Rust usage already implicitly extend too
> > > broadly
> > 
> > At the moment we have exactly 2 Rust devices. You can see the
> > workaround we ended up with for the PL011 in commits 5b87a07e768
> > and cc3d262aa93a4, where we pad out the C device struct and
> > assert on the Rust side that its version isn't any bigger.
> > 
> > For that particular problem we could say "you need to refactor
> > all users of device X to not embed it before creating the Rust
> > version". There is I think only one embedded use of the PL011
> > (I was actually expecting more). This does have the awkward
> > effect that anybody wanting to do a "rewrite device in rust"
> > is now forced into "do a big refactor of some old C code they
> > don't care about". We would also need to actually document and
> > provide some good examples of "this is how we think we should
> > be writing device models that have child objects now".
> 
> I think given the legacy of the QEMU codebase then this will always be an
> issue, but we've solved this in the past with the introduction of
> MemoryRegions so there's no reason we couldn't do it again.
> 
> This is a similar problem to the use of non-class object properties I was
> discussing in the thread with Daniel: it seems the general consensus is that
> they shouldn't exist, but this hasn't been formalised anywhere. If we can
> formalise the decision not to use embedded QOM objects, then we can start by
> ensuring that new submissions don't use them and go from there.
> 
> Question: who currently should make these decisions? Is it restricted to the
> QOM maintainers?

Ultimately it would need a docs update on object.h to declare the
object_initialize_child related methods to be deprecated which the
QOM maintainers would have to queue.

Since this impacts device maintainers in general though, IMHO we need
broad consensus that it is a better approach & we're willing to convert
existing code incrementally.

We have ~150 devices using this for 824 children

$ git grep -l  object_initialize_child  | wc -l
150

$ git grep object_initialize_child | wc -l
824


Where updates are desired, we'd be looking at tedious changes like
thsi:

diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 31fa53e6a4..86757c56e5 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -355,16 +355,23 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
 
     /* USB */
     if (d->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &d->uhci, uhci_type);
-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+        d->uhci = UHCI(object_new_with_props(
+                           uhci_type, OBJECT(d), "uhci", errp, NULL));
+        if (!d->uhci) {
+            return;
+        }
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(d->uhci), BUS(pci_bus), errp)) {
             return;
         }
     }
diff --git a/hw/isa/piix.c b/hw/isa/piix.c
index 31fa53e6a4..86757c56e5 100644
--- a/hw/isa/piix.c
+++ b/hw/isa/piix.c
@@ -355,16 +355,23 @@ static void pci_piix_realize(PCIDevice *dev, const char *uhci_type,
 
     /* USB */
     if (d->has_usb) {
-        object_initialize_child(OBJECT(dev), "uhci", &d->uhci, uhci_type);
-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
-        if (!qdev_realize(DEVICE(&d->uhci), BUS(pci_bus), errp)) {
+        d->uhci = UHCI(object_new_with_props(
+                           uhci_type, OBJECT(d), "uhci", errp, NULL));
+        if (!d->uhci) {
+            return;
+        }
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);
+        if (!qdev_realize(DEVICE(d->uhci), BUS(pci_bus), errp)) {
             return;
         }
     }


Noting that the compiler won't complain if we forget to update any
lines like

-        qdev_prop_set_int32(DEVICE(&d->uhci), "addr", dev->devfn + 2);
+        qdev_prop_set_int32(DEVICE(d->uhci), "addr", dev->devfn + 2);

because the explicit DEVICE(..) cast is hiding the type error we would
otherwise get from using "Device **" instead of "Device *".


With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|