On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> Children are automatically unparented so manually unparenting is
> unnecessary.
>
> Worse, automatic unparenting happens before the insntance_finalize()
> callback of the parent gets called, so object_unparent() calls in
> the callback will refer to objects that are already unparented, which
> is semantically incorrect.
>
> Signed-off-by: Akihiko Odaki <od...@rsg.ci.i.u-tokyo.ac.jp>
> ---
> hw/vfio/pci.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index
> 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e
> 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> vfio_region_finalize(&bar->region);
> if (bar->mr) {
> assert(bar->size);
> - object_unparent(OBJECT(bar->mr));
> g_free(bar->mr);
> bar->mr = NULL;
> }
> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>
> if (vdev->vga) {
> vfio_vga_quirk_finalize(vdev);
> - for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
> - object_unparent(OBJECT(&vdev->vga->region[i].mem));
> - }
> g_free(vdev->vga);
> }
> }
So the 2nd object_unparent() here should be no-op, seeing empty list of
properties (but shouldn't causing anything severe), is that correct?
I think it still makes some sense to theoretically allow object_unparent()
to happen, at least when it happens before owner's finalize(). IIUC that
was the intention of the doc, pairing the memory_region_init*() operation.
It might depend on two use cases:
1. MRs dynamically created, it'll share the same lifecycle of the owner
after creation (just like an embeded MemoryRegion)
I feel like most, if not all, VFIO's dynamic mrs follows this trend,
that this patch touched.
In this case, IMHO instead of object_unparent(), we could also allow the
owner / device to take ownership of the MR completely, by replacing:
mr = g_new0(MemoryRegion, 1);
with:
mr = object_new(TYPE_MEMORY_REGION);
Then after memory_region_init*(), essentially the owner will be in
charge of the memory, as it'll be g_free()ed when remove the mr from
property list (in owner's finalize() automatically).
With that, the device impl can not only avoid object_unparent(), but
avoid g_free() altogether. That would make some more sense to me,
instead of relying on memory internal to unparent, and rely on device
impl to g_free().
2. MRs dynamically created, and it may be freed even before the owner
finishes its lifecycle
This is the case that I _think_ an object_unparent() should still be
allowed, because when the mr is unparented (aka, not used), we should
still provide a way for the device impl to detach and free the mr
resources on the fly.
There're a bunch of object_unparent() users, I didn't check whether there's
any real user of case (2), though.
AFAIU for such case maybe it's always better to provide real refcounting
for the mr, since the mr can always be address_space_map()ed.. with an
elevated refcount. In that case, the owner shouldn't be the device impl,
but a temp obj that represents the mr (and do refcounts).
Thanks,
--
Peter Xu
