[PATCH v4 0/7] Do not unparent in instance_finalize()

[PATCH v4 0/7] Do not unparent in instance_finalize()

[Akihiko Odaki]

Supersedes: <20240829-memory-v1-1-ac07af2f4fa5@daynix.com>
("[PATCH] docs/devel: Prohibit calling object_unparent() for memory region")

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
---
Changes in v4:
- Removed Based-on:.
- Restored the examples of VFIOMSIXInfo and VFIOQuirk in
  docs/devel/memory.rst.
- Ensured that the timing to call object_unparent() and free memory
  regions is mentioned once for each.
- Link to v3: https://lore.kernel.org/qemu-devel/20250917-use-v3-0-72c2a6887c6c@rsg.ci.i.u-tokyo.ac.jp

Changes in v3:
- Added patches to remove other object_unparent() calls in
  instance_finalize().
- Dropped patch "qdev: Automatically delete memory subregions" and the
  succeeding patches to avoid Ccing many.
- Link to v2: https://lore.kernel.org/qemu-devel/20250915-use-v2-0-f4c7ff13bfe9@rsg.ci.i.u-tokyo.ac.jp

Changes in v2:
- Added a reference to "[PATCH] docs/devel: Prohibit calling
  object_unparent() for memory region", which does something similar to
  patch "docs/devel: Do not unparent in instance_finalize()" but I
  forgot I sent it in the past.
- Fixed a typo in patch
  "docs/devel: Do not unparent in instance_finalize()" and
  "[PATCH 02/22] vfio/pci: Do not unparent in instance_finalize()".
- Dropped patches to move address_space_init() calls; I intend to
  QOM-ify to fix memory leaks automatically as discussed in the
  following thread:
  https://lore.kernel.org/qemu-devel/cd21698f-db77-eb75-6966-d559fdcab835@eik.bme.hu/
  But the QOM-ification will be big so I'll send it as a separate
  series.
- Rebased on top of "[PATCH v2 00/14] hw/pci-host/raven clean ups".
  https://lore.kernel.org/qemu-devel/cover.1751493467.git.balaton@eik.bme.hu/
- Link to v1: https://lore.kernel.org/qemu-devel/20250906-use-v1-0-c51caafd1eb7@rsg.ci.i.u-tokyo.ac.jp

---
Akihiko Odaki (7):
      docs/devel: Do not unparent in instance_finalize()
      vfio/pci: Do not unparent in instance_finalize()
      hw/core/register: Do not unparent in instance_finalize()
      hv-balloon: hw/core/register: Do not unparent in instance_finalize()
      hw/sd/sdhci: Do not unparent in instance_finalize()
      vfio: Do not unparent in instance_finalize()
      hw/xen: Do not unparent in instance_finalize()

 docs/devel/memory.rst  | 17 ++++++-----------
 hw/core/register.c     |  1 -
 hw/hyperv/hv-balloon.c | 12 +-----------
 hw/sd/sdhci.c          |  4 ----
 hw/vfio/pci-quirks.c   |  9 +--------
 hw/vfio/pci.c          |  4 ----
 hw/vfio/region.c       |  3 ---
 hw/xen/xen_pt_msi.c    | 11 +----------
 8 files changed, 9 insertions(+), 52 deletions(-)
---
base-commit: ab8008b231e758e03c87c1c483c03afdd9c02e19
change-id: 20250906-use-37ecc903a9e0

Best regards,
--  
Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>

[PATCH v4 1/7] docs/devel: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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.

Remove the instruction to call object_unparent(), and the exception
of the "do not call object_unparent()" rule for instance_finalize().

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 docs/devel/memory.rst | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 42d3ca29c431..11858543f256 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -165,17 +165,14 @@ and finalized one by one.  The order in which memory regions will be
 finalized is not guaranteed.
 
 If however the memory region is part of a dynamically allocated data
-structure, you should call object_unparent() to destroy the memory region
-before the data structure is freed.  For an example see VFIOMSIXInfo
-and VFIOQuirk in hw/vfio/pci.c.
+structure, you should free the memory region in the instance_finalize
+callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
+hw/vfio/pci.c.
 
 You must not destroy a memory region as long as it may be in use by a
 device or CPU.  In order to do this, as a general rule do not create or
-destroy memory regions dynamically during a device's lifetime, and only
-call object_unparent() in the memory region owner's instance_finalize
-callback.  The dynamically allocated data structure that contains the
-memory region then should obviously be freed in the instance_finalize
-callback as well.
+destroy memory regions dynamically during a device's lifetime, and must
+never call object_unparent().
 
 If you break this rule, the following situation can happen:
 
@@ -201,9 +198,7 @@ this exception is rarely necessary, and therefore it is discouraged,
 but nevertheless it is used in a few places.
 
 For regions that "have no owner" (NULL is passed at creation time), the
-machine object is actually used as the owner.  Since instance_finalize is
-never called for the machine object, you must never call object_unparent
-on regions that have no owner, unless they are aliases or containers.
+machine object is actually used as the owner.
 
 
 Overlapping regions and priority

-- 
2.51.0

[PATCH v4 2/7] vfio/pci: Do not unparent in instance_finalize()

[Akihiko Odaki]

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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/vfio/pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index d14e96b2f82d..bc0b4c4d562b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2025,7 +2025,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;
         }
@@ -2033,9 +2032,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);
     }
 }

-- 
2.51.0

[PATCH v4 3/7] hw/core/register: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/core/register.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/core/register.c b/hw/core/register.c
index 8f63d9f227c4..3340df70b06e 100644
--- a/hw/core/register.c
+++ b/hw/core/register.c
@@ -314,7 +314,6 @@ RegisterInfoArray *register_init_block64(DeviceState *owner,
 
 void register_finalize_block(RegisterInfoArray *r_array)
 {
-    object_unparent(OBJECT(&r_array->mem));
     g_free(r_array->r);
     g_free(r_array);
 }

-- 
2.51.0

[PATCH v4 4/7] hv-balloon: hw/core/register: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/hyperv/hv-balloon.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/hyperv/hv-balloon.c b/hw/hyperv/hv-balloon.c
index 6dbcb2d9a29d..2d6d7db4ee0e 100644
--- a/hw/hyperv/hv-balloon.c
+++ b/hw/hyperv/hv-balloon.c
@@ -1475,16 +1475,6 @@ static void hv_balloon_ensure_mr(HvBalloon *balloon)
     balloon->mr->align = memory_region_get_alignment(hostmem_mr);
 }
 
-static void hv_balloon_free_mr(HvBalloon *balloon)
-{
-    if (!balloon->mr) {
-        return;
-    }
-
-    object_unparent(OBJECT(balloon->mr));
-    g_clear_pointer(&balloon->mr, g_free);
-}
-
 static void hv_balloon_vmdev_realize(VMBusDevice *vdev, Error **errp)
 {
     ERRP_GUARD();
@@ -1580,7 +1570,7 @@ static void hv_balloon_vmdev_reset(VMBusDevice *vdev)
  */
 static void hv_balloon_unrealize_finalize_common(HvBalloon *balloon)
 {
-    hv_balloon_free_mr(balloon);
+    g_clear_pointer(&balloon->mr, g_free);
     balloon->addr = 0;
 
     balloon->memslot_count = 0;

-- 
2.51.0

[PATCH v4 5/7] hw/sd/sdhci: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/sd/sdhci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 3c897e54b721..89b595ce4a5a 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1578,10 +1578,6 @@ static void sdhci_sysbus_finalize(Object *obj)
 {
     SDHCIState *s = SYSBUS_SDHCI(obj);
 
-    if (s->dma_mr) {
-        object_unparent(OBJECT(s->dma_mr));
-    }
-
     sdhci_uninitfn(s);
 }
 

-- 
2.51.0

[PATCH v4 6/7] vfio: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/vfio/pci-quirks.c | 9 +--------
 hw/vfio/region.c     | 3 ---
 2 files changed, 1 insertion(+), 11 deletions(-)

diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index c97606dbf194..b5da6afbf5b0 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1159,15 +1159,12 @@ void vfio_vga_quirk_exit(VFIOPCIDevice *vdev)
 
 void vfio_vga_quirk_finalize(VFIOPCIDevice *vdev)
 {
-    int i, j;
+    int i;
 
     for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
         while (!QLIST_EMPTY(&vdev->vga->region[i].quirks)) {
             VFIOQuirk *quirk = QLIST_FIRST(&vdev->vga->region[i].quirks);
             QLIST_REMOVE(quirk, next);
-            for (j = 0; j < quirk->nr_mem; j++) {
-                object_unparent(OBJECT(&quirk->mem[j]));
-            }
             g_free(quirk->mem);
             g_free(quirk->data);
             g_free(quirk);
@@ -1207,14 +1204,10 @@ void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr)
 void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr)
 {
     VFIOBAR *bar = &vdev->bars[nr];
-    int i;
 
     while (!QLIST_EMPTY(&bar->quirks)) {
         VFIOQuirk *quirk = QLIST_FIRST(&bar->quirks);
         QLIST_REMOVE(quirk, next);
-        for (i = 0; i < quirk->nr_mem; i++) {
-            object_unparent(OBJECT(&quirk->mem[i]));
-        }
         g_free(quirk->mem);
         g_free(quirk->data);
         g_free(quirk);
diff --git a/hw/vfio/region.c b/hw/vfio/region.c
index d04c57db630f..b165ab0b9378 100644
--- a/hw/vfio/region.c
+++ b/hw/vfio/region.c
@@ -365,12 +365,9 @@ void vfio_region_finalize(VFIORegion *region)
     for (i = 0; i < region->nr_mmaps; i++) {
         if (region->mmaps[i].mmap) {
             munmap(region->mmaps[i].mmap, region->mmaps[i].size);
-            object_unparent(OBJECT(&region->mmaps[i].mem));
         }
     }
 
-    object_unparent(OBJECT(region->mem));
-
     g_free(region->mem);
     g_free(region->mmaps);
 

-- 
2.51.0

[PATCH v4 7/7] hw/xen: Do not unparent in instance_finalize()

[Akihiko Odaki]

Children are automatically unparented so manually unparenting is
unnecessary.

Worse, automatic unparenting happens before the instance_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>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
---
 hw/xen/xen_pt_msi.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 09cca4eecb1c..e9ba17317aba 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -637,14 +637,5 @@ void xen_pt_msix_unmap(XenPCIPassthroughState *s)
 
 void xen_pt_msix_delete(XenPCIPassthroughState *s)
 {
-    XenPTMSIX *msix = s->msix;
-
-    if (!msix) {
-        return;
-    }
-
-    object_unparent(OBJECT(&msix->mmio));
-
-    g_free(s->msix);
-    s->msix = NULL;
+    g_clear_pointer(&s->msix, g_free);
 }

-- 
2.51.0

Re: [PATCH v4 2/7] vfio/pci: Do not unparent in instance_finalize()

[Cédric Le Goater]

On 9/24/25 06:37, 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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   hw/vfio/pci.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index d14e96b2f82d..bc0b4c4d562b 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2025,7 +2025,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;
>           }
> @@ -2033,9 +2032,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);
>       }
>   }
> 



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

Re: [PATCH v4 6/7] vfio: Do not unparent in instance_finalize()

[Cédric Le Goater]

On 9/24/25 06:37, Akihiko Odaki wrote:
> Children are automatically unparented so manually unparenting is
> unnecessary.
> 
> Worse, automatic unparenting happens before the instance_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>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   hw/vfio/pci-quirks.c | 9 +--------
>   hw/vfio/region.c     | 3 ---
>   2 files changed, 1 insertion(+), 11 deletions(-)
> 
> diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
> index c97606dbf194..b5da6afbf5b0 100644
> --- a/hw/vfio/pci-quirks.c
> +++ b/hw/vfio/pci-quirks.c
> @@ -1159,15 +1159,12 @@ void vfio_vga_quirk_exit(VFIOPCIDevice *vdev)
>   
>   void vfio_vga_quirk_finalize(VFIOPCIDevice *vdev)
>   {
> -    int i, j;
> +    int i;
>   
>       for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
>           while (!QLIST_EMPTY(&vdev->vga->region[i].quirks)) {
>               VFIOQuirk *quirk = QLIST_FIRST(&vdev->vga->region[i].quirks);
>               QLIST_REMOVE(quirk, next);
> -            for (j = 0; j < quirk->nr_mem; j++) {
> -                object_unparent(OBJECT(&quirk->mem[j]));
> -            }
>               g_free(quirk->mem);
>               g_free(quirk->data);
>               g_free(quirk);
> @@ -1207,14 +1204,10 @@ void vfio_bar_quirk_exit(VFIOPCIDevice *vdev, int nr)
>   void vfio_bar_quirk_finalize(VFIOPCIDevice *vdev, int nr)
>   {
>       VFIOBAR *bar = &vdev->bars[nr];
> -    int i;
>   
>       while (!QLIST_EMPTY(&bar->quirks)) {
>           VFIOQuirk *quirk = QLIST_FIRST(&bar->quirks);
>           QLIST_REMOVE(quirk, next);
> -        for (i = 0; i < quirk->nr_mem; i++) {
> -            object_unparent(OBJECT(&quirk->mem[i]));
> -        }
>           g_free(quirk->mem);
>           g_free(quirk->data);
>           g_free(quirk);
> diff --git a/hw/vfio/region.c b/hw/vfio/region.c
> index d04c57db630f..b165ab0b9378 100644
> --- a/hw/vfio/region.c
> +++ b/hw/vfio/region.c
> @@ -365,12 +365,9 @@ void vfio_region_finalize(VFIORegion *region)
>       for (i = 0; i < region->nr_mmaps; i++) {
>           if (region->mmaps[i].mmap) {
>               munmap(region->mmaps[i].mmap, region->mmaps[i].size);
> -            object_unparent(OBJECT(&region->mmaps[i].mem));
>           }
>       }
>   
> -    object_unparent(OBJECT(region->mem));
> -
>       g_free(region->mem);
>       g_free(region->mmaps);
>   
> 

What about vfio_subregion_unmap() calling object_unparent() too ?

C.