Re: [PATCH 02/22] vfio/pci: Do not unparent in instance_finalize()

[Peter Xu <peterx@redhat.com>]

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 <odaki@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