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

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

[Akihiko Odaki]

Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
("[PATCH v2 00/14] hw/pci-host/raven clean ups")

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 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  | 19 ++++++-------------
 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(+), 54 deletions(-)
---
base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
change-id: 20250906-use-37ecc903a9e0

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

[PATCH v3 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>
---
 docs/devel/memory.rst | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 57fb2aec76e0..749f11d8a4dd 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -161,18 +161,11 @@ or never.
 Destruction of a memory region happens automatically when the owner
 object dies.
 
-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.
-
 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.
+The dynamically allocated data structure that contains the
+memory region should be freed in the instance_finalize callback.
 
 If you break this rule, the following situation can happen:
 
@@ -198,9 +191,9 @@ 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.  You must never call
+object_unparent on regions that have no owner, unless they are aliases
+or containers.
 
 
 Overlapping regions and priority

-- 
2.51.0

[PATCH v3 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>
---
 hw/vfio/pci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 07257d0fa049..2e909c190f86 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);
     }
 }

-- 
2.51.0

[PATCH v3 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>
---
 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 v3 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>
---
 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 v3 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>
---
 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 v3 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>
---
 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 3f002252acfb..83419b1ab58d 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -1150,15 +1150,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);
@@ -1198,14 +1195,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 v3 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>
---
 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 v3 0/7] Do not unparent in instance_finalize()

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> 
> 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.

Where is automatic unparenting you're referring to being done ?

> 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.

IIUC, object_property_add_child will acquire a reference on
the child, and object_property_del_child (and thus
object_unparent) will release that reference.

The 'object_finalize' method, and thus 'instance_finalize'
callback, won't be invoked until the last reference is
dropped on the object in question.

IOW, it should be impossible for 'object_finalize' to ever
run, as long as the child has a parent set.

So if we're in the 'finalize' then 'object_unparent' must
be a no-op as the child must already have no references
held and thus no parent.

IOW, the reason to remove 'object_unparent' calls from
finalize is surely because they do nothing at all,
rather than this talk about callbacks being run at the
wrong time ?

> 
> Signed-off-by: Akihiko Odaki <odaki@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  | 19 ++++++-------------
>  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(+), 54 deletions(-)
> ---
> base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
> change-id: 20250906-use-37ecc903a9e0
> 
> Best regards,
> --  
> Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Akihiko Odaki]

On 2025/09/17 20:57, Daniel P. Berrangé wrote:
> On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
>> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>>
>> 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.
> 
> Where is automatic unparenting you're referring to being done ?
> 
>> 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.
> 
> IIUC, object_property_add_child will acquire a reference on
> the child, and object_property_del_child (and thus
> object_unparent) will release that reference.
> 
> The 'object_finalize' method, and thus 'instance_finalize'
> callback, won't be invoked until the last reference is
> dropped on the object in question.
> 
> IOW, it should be impossible for 'object_finalize' to ever
> run, as long as the child has a parent set.
> 
> So if we're in the 'finalize' then 'object_unparent' must
> be a no-op as the child must already have no references
> held and thus no parent.
> 
> IOW, the reason to remove 'object_unparent' calls from
> finalize is surely because they do nothing at all,
> rather than this talk about callbacks being run at the
> wrong time ?

This patch series deals with the situation where the parent calls 
object_unparent() in its instance_finalize() callback. The process of 
finalization looks like as follows:

1. The parent's reference count reaches to zero. Please note that there 
can be remaining children that are referenced by the parent at this point.

2. object_finalize() is called.

2a. object_property_del_all() is called and the parent releases 
references to its children. This is what I referred as "automatic 
unparenting". The children without any other references will be 
finalized here.

2b. instance_finalize() is called. Past children may be already 
finalized, and calling object_unparent() here will cause dereferencing 
finalized objects in that case, which should be avoided.

Regards,
Akihiko Odaki

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 09:24:04PM +0900, Akihiko Odaki wrote:
> On 2025/09/17 20:57, Daniel P. Berrangé wrote:
> > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > 
> > > 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.
> > 
> > Where is automatic unparenting you're referring to being done ?
> > 
> > > 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.
> > 
> > IIUC, object_property_add_child will acquire a reference on
> > the child, and object_property_del_child (and thus
> > object_unparent) will release that reference.
> > 
> > The 'object_finalize' method, and thus 'instance_finalize'
> > callback, won't be invoked until the last reference is
> > dropped on the object in question.
> > 
> > IOW, it should be impossible for 'object_finalize' to ever
> > run, as long as the child has a parent set.
> > 
> > So if we're in the 'finalize' then 'object_unparent' must
> > be a no-op as the child must already have no references
> > held and thus no parent.
> > 
> > IOW, the reason to remove 'object_unparent' calls from
> > finalize is surely because they do nothing at all,
> > rather than this talk about callbacks being run at the
> > wrong time ?
> 
> This patch series deals with the situation where the parent calls
> object_unparent() in its instance_finalize() callback. The process of
> finalization looks like as follows:
> 
> 1. The parent's reference count reaches to zero. Please note that there can
> be remaining children that are referenced by the parent at this point.
> 
> 2. object_finalize() is called.
> 
> 2a. object_property_del_all() is called and the parent releases references
> to its children. This is what I referred as "automatic unparenting". The
> children without any other references will be finalized here.
> 
> 2b. instance_finalize() is called. Past children may be already finalized,
> and calling object_unparent() here will cause dereferencing finalized
> objects in that case, which should be avoided.

Oh, so these object_unparent calls run by the parent, against the child
in fact use-after-free flaws.

This is driven by the parent keeping hold of explicit pointers to the
child (MemoryRegion), without also holding its own reference, and these
pointers are invalidated when the parent<->child property is deleted.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 02:17:35PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 17, 2025 at 09:24:04PM +0900, Akihiko Odaki wrote:
> > On 2025/09/17 20:57, Daniel P. Berrangé wrote:
> > > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > > 
> > > > 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.
> > > 
> > > Where is automatic unparenting you're referring to being done ?
> > > 
> > > > 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.
> > > 
> > > IIUC, object_property_add_child will acquire a reference on
> > > the child, and object_property_del_child (and thus
> > > object_unparent) will release that reference.
> > > 
> > > The 'object_finalize' method, and thus 'instance_finalize'
> > > callback, won't be invoked until the last reference is
> > > dropped on the object in question.
> > > 
> > > IOW, it should be impossible for 'object_finalize' to ever
> > > run, as long as the child has a parent set.
> > > 
> > > So if we're in the 'finalize' then 'object_unparent' must
> > > be a no-op as the child must already have no references
> > > held and thus no parent.
> > > 
> > > IOW, the reason to remove 'object_unparent' calls from
> > > finalize is surely because they do nothing at all,
> > > rather than this talk about callbacks being run at the
> > > wrong time ?
> > 
> > This patch series deals with the situation where the parent calls
> > object_unparent() in its instance_finalize() callback. The process of
> > finalization looks like as follows:
> > 
> > 1. The parent's reference count reaches to zero. Please note that there can
> > be remaining children that are referenced by the parent at this point.
> > 
> > 2. object_finalize() is called.
> > 
> > 2a. object_property_del_all() is called and the parent releases references
> > to its children. This is what I referred as "automatic unparenting". The
> > children without any other references will be finalized here.
> > 
> > 2b. instance_finalize() is called. Past children may be already finalized,
> > and calling object_unparent() here will cause dereferencing finalized
> > objects in that case, which should be avoided.
> 
> Oh, so these object_unparent calls run by the parent, against the child
> in fact use-after-free flaws.

Oh actually not a use-after-free since memory regions aren't directly
freed by object_finalize.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:26PM +0900, 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.
> 
> 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>
> ---
>  docs/devel/memory.rst | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:27PM +0900, 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:28PM +0900, 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>
> ---
>  hw/core/register.c | 1 -
>  1 file changed, 1 deletion(-)
>

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:29PM +0900, 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>
> ---
>  hw/hyperv/hv-balloon.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:30PM +0900, 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>
> ---
>  hw/sd/sdhci.c | 4 ----
>  1 file changed, 4 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:31PM +0900, 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>
> ---
>  hw/vfio/pci-quirks.c | 9 +--------
>  hw/vfio/region.c     | 3 ---
>  2 files changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Daniel P. Berrangé]

On Wed, Sep 17, 2025 at 07:13:32PM +0900, 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>
> ---
>  hw/xen/xen_pt_msi.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Peter Xu]

On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")

Could I ask why this is a dependency?

-- 
Peter Xu

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[BALATON Zoltan]

On Thu, 18 Sep 2025, Peter Xu wrote:
> On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
>> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>
> Could I ask why this is a dependency?

It removes an address_space usage from raven so this series does not have 
to change that and I don't have to rebase that series. Otherwise these are 
not related. I'll check the problem reported about my series and send an 
updated one.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Peter Xu]

On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
> On Thu, 18 Sep 2025, Peter Xu wrote:
> > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > 
> > Could I ask why this is a dependency?
> 
> It removes an address_space usage from raven so this series does not have to
> change that and I don't have to rebase that series. Otherwise these are not
> related. I'll check the problem reported about my series and send an updated
> one.

This series should be a split of a previous mixed up series that may
contain address space changes while this one doesn't.  It also doesn't
touch raven.c and ppc/.

Can I then understand it as the dependency is simply not needed?

Thanks,

-- 
Peter Xu

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Peter Xu]

On Thu, Sep 18, 2025 at 12:20:49PM -0400, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
> > On Thu, 18 Sep 2025, Peter Xu wrote:
> > > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > 
> > > Could I ask why this is a dependency?
> > 
> > It removes an address_space usage from raven so this series does not have to
> > change that and I don't have to rebase that series. Otherwise these are not
> > related. I'll check the problem reported about my series and send an updated
> > one.
> 
> This series should be a split of a previous mixed up series that may
> contain address space changes while this one doesn't.  It also doesn't
> touch raven.c and ppc/.
> 
> Can I then understand it as the dependency is simply not needed?

I meant, it seems we don't need to wait for the other series to merge this
one, hence the there is no real dependency.

I didn't mean to drop that series for sure.. if it was confusing before..

Thanks,

-- 
Peter Xu

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Peter Xu]

On Wed, Sep 17, 2025 at 02:23:35PM +0100, Daniel P. Berrangé wrote:
> On Wed, Sep 17, 2025 at 02:17:35PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Sep 17, 2025 at 09:24:04PM +0900, Akihiko Odaki wrote:
> > > On 2025/09/17 20:57, Daniel P. Berrangé wrote:
> > > > On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
> > > > > Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
> > > > > ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
> > > > > 
> > > > > 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.
> > > > 
> > > > Where is automatic unparenting you're referring to being done ?
> > > > 
> > > > > 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.
> > > > 
> > > > IIUC, object_property_add_child will acquire a reference on
> > > > the child, and object_property_del_child (and thus
> > > > object_unparent) will release that reference.
> > > > 
> > > > The 'object_finalize' method, and thus 'instance_finalize'
> > > > callback, won't be invoked until the last reference is
> > > > dropped on the object in question.
> > > > 
> > > > IOW, it should be impossible for 'object_finalize' to ever
> > > > run, as long as the child has a parent set.
> > > > 
> > > > So if we're in the 'finalize' then 'object_unparent' must
> > > > be a no-op as the child must already have no references
> > > > held and thus no parent.
> > > > 
> > > > IOW, the reason to remove 'object_unparent' calls from
> > > > finalize is surely because they do nothing at all,
> > > > rather than this talk about callbacks being run at the
> > > > wrong time ?
> > > 
> > > This patch series deals with the situation where the parent calls
> > > object_unparent() in its instance_finalize() callback. The process of
> > > finalization looks like as follows:
> > > 
> > > 1. The parent's reference count reaches to zero. Please note that there can
> > > be remaining children that are referenced by the parent at this point.
> > > 
> > > 2. object_finalize() is called.
> > > 
> > > 2a. object_property_del_all() is called and the parent releases references
> > > to its children. This is what I referred as "automatic unparenting". The
> > > children without any other references will be finalized here.
> > > 
> > > 2b. instance_finalize() is called. Past children may be already finalized,
> > > and calling object_unparent() here will cause dereferencing finalized
> > > objects in that case, which should be avoided.
> > 
> > Oh, so these object_unparent calls run by the parent, against the child
> > in fact use-after-free flaws.
> 
> Oh actually not a use-after-free since memory regions aren't directly
> freed by object_finalize.

We discussed this previously, I think so far it's 100% safe to call
object_unparent() twice, because step (2a) will reset child->parent=NULL.
Then at (2b) calling object_unparent() will be 100% safe because it's no-op
for an object that is orphaned.

So the series looks good, but it's kind of a cleanup, as object_unparent()
is just unnecessary for these MRs, same to the memory.rst doc suggestions
which can be avoided.

Thanks,

-- 
Peter Xu

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

[Peter Xu]

On Wed, Sep 17, 2025 at 07:13:26PM +0900, 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.
> 
> 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>
> ---
>  docs/devel/memory.rst | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 57fb2aec76e0..749f11d8a4dd 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -161,18 +161,11 @@ or never.
>  Destruction of a memory region happens automatically when the owner
>  object dies.
>  
> -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.

Should we still keep some of these examples?  After the series they'll be
doing the right things.  Dynamic MRs are still slightly tricky, I think
it's still good to have some references.

> -
>  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.
> +The dynamically allocated data structure that contains the
> +memory region should be freed in the instance_finalize callback.
>  
>  If you break this rule, the following situation can happen:
>  
> @@ -198,9 +191,9 @@ 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.  You must never call
> +object_unparent on regions that have no owner, unless they are aliases
> +or containers.

This looks like a completely separate change.  So we start to allow
machines to be finalized now?  I'm not familiar with machine object
lifecycles.  Maybe split it out even if it's true?

-- 
Peter Xu

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

[Peter Xu]

On Thu, Sep 18, 2025 at 04:03:49PM -0400, Peter Xu wrote:
> On Wed, Sep 17, 2025 at 07:13:26PM +0900, 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.
> > 
> > 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>
> > ---
> >  docs/devel/memory.rst | 19 ++++++-------------
> >  1 file changed, 6 insertions(+), 13 deletions(-)
> > 
> > diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> > index 57fb2aec76e0..749f11d8a4dd 100644
> > --- a/docs/devel/memory.rst
> > +++ b/docs/devel/memory.rst
> > @@ -161,18 +161,11 @@ or never.
> >  Destruction of a memory region happens automatically when the owner
> >  object dies.
> >  
> > -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.
> 
> Should we still keep some of these examples?  After the series they'll be
> doing the right things.  Dynamic MRs are still slightly tricky, I think
> it's still good to have some references.
> 
> > -
> >  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.
> > +The dynamically allocated data structure that contains the
> > +memory region should be freed in the instance_finalize callback.
> >  
> >  If you break this rule, the following situation can happen:
> >  
> > @@ -198,9 +191,9 @@ 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.  You must never call
> > +object_unparent on regions that have no owner, unless they are aliases
> > +or containers.
> 
> This looks like a completely separate change.  So we start to allow
> machines to be finalized now?  I'm not familiar with machine object
> lifecycles.  Maybe split it out even if it's true?

I didn't see anything elsewhere.  If you agree with above, I can queue this
series with above touched up, then no need to repost.

Thanks,

-- 
Peter Xu

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

[Akihiko Odaki]

On 2025/09/19 5:11, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 04:03:49PM -0400, Peter Xu wrote:
>> On Wed, Sep 17, 2025 at 07:13:26PM +0900, 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.
>>>
>>> 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>
>>> ---
>>>   docs/devel/memory.rst | 19 ++++++-------------
>>>   1 file changed, 6 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
>>> index 57fb2aec76e0..749f11d8a4dd 100644
>>> --- a/docs/devel/memory.rst
>>> +++ b/docs/devel/memory.rst
>>> @@ -161,18 +161,11 @@ or never.
>>>   Destruction of a memory region happens automatically when the owner
>>>   object dies.
>>>   
>>> -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.
>>
>> Should we still keep some of these examples?  After the series they'll be
>> doing the right things.  Dynamic MRs are still slightly tricky, I think
>> it's still good to have some references.

I agree. I'll restore it with the next version.

>>
>>> -
>>>   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.
>>> +The dynamically allocated data structure that contains the
>>> +memory region should be freed in the instance_finalize callback.
>>>   
>>>   If you break this rule, the following situation can happen:
>>>   
>>> @@ -198,9 +191,9 @@ 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.  You must never call
>>> +object_unparent on regions that have no owner, unless they are aliases
>>> +or containers.
>>
>> This looks like a completely separate change.  So we start to allow
>> machines to be finalized now?  I'm not familiar with machine object
>> lifecycles.  Maybe split it out even if it's true?

I intended to remove phrase "since instance_finalize is never called for 
the machine object" because whether instance_finalize is called or not 
is no longer relevant, and thus object_unparent is always prohibited, 
whether regions have owners or not, unless they are aliases or containers.

The statement still mentions "regions that have no owner"; the 
restriction of object_unparent is enforced whether the regions have 
owners, so it is a bit misleading.

> 
> I didn't see anything elsewhere.  If you agree with above, I can queue this
> series with above touched up, then no need to repost.

I guess I will rewrite this patch, restoring the VFIOQuirk example, and 
re-check if this whole section is structured logically and consistently.

Regards,
Akihiko Odaki

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Akihiko Odaki]

On 2025/09/19 3:23, Peter Xu wrote:
> On Thu, Sep 18, 2025 at 12:20:49PM -0400, Peter Xu wrote:
>> On Thu, Sep 18, 2025 at 05:29:34PM +0200, BALATON Zoltan wrote:
>>> On Thu, 18 Sep 2025, Peter Xu wrote:
>>>> On Wed, Sep 17, 2025 at 07:13:25PM +0900, Akihiko Odaki wrote:
>>>>> Based-on: <cover.1751493467.git.balaton@eik.bme.hu>
>>>>> ("[PATCH v2 00/14] hw/pci-host/raven clean ups")
>>>>
>>>> Could I ask why this is a dependency?
>>>
>>> It removes an address_space usage from raven so this series does not have to
>>> change that and I don't have to rebase that series. Otherwise these are not
>>> related. I'll check the problem reported about my series and send an updated
>>> one.
>>
>> This series should be a split of a previous mixed up series that may
>> contain address space changes while this one doesn't.  It also doesn't
>> touch raven.c and ppc/.
>>
>> Can I then understand it as the dependency is simply not needed?
> 
> I meant, it seems we don't need to wait for the other series to merge this
> one, hence the there is no real dependency.

You are right. This series can be merged without the raven clean ups.

Regards,
Akihiko Odaki

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Paolo Bonzini]

Queued, thanks.

Paolo

Re: [PATCH v3 0/7] Do not unparent in instance_finalize()

[Akihiko Odaki]

On 2025/09/23 17:08, Paolo Bonzini wrote:
> Queued, thanks.
> 
> Paolo
> 

I sent v4 to address comments in the following thread:
https://lore.kernel.org/qemu-devel/aMxlpfp_LSgiIk9Z@x1.local/

Regards,
Akihiko Odaki