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é <[email protected]>
> > 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é <[email protected]>
> > > > 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 :|
