[PATCH 00/16] memory: Stop piggybacking on memory region owners

[PATCH 00/16] memory: Stop piggybacking on memory region owners

[Akihiko Odaki]

Supersedes: https://lore.kernel.org/qemu-devel/20250828-san-v9-0-c0dff4b8a487@rsg.ci.i.u-tokyo.ac.jp/
("[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer errors")

MemoryRegions used to "piggyback" on their owners instead of using their
reference counters due to the circular dependencies between them, which
caused memory leak.

I tried to fix it with "[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer
errors" but it resulted in a lengthy discussion; ultimately it is
attributed to the fact that "piggybacking" is hard to understand and
forces us design trade-offs. It was also insufficient because it only
deals with the container-subregion pattern and did not deal with DMA.

With this series, I remove the "piggyback" hack altogather.
The key insight here is that the owners explicitly call
memory_region_del_subregion() to stop accepting new accesses to
its MemoryRegions when they are no longer needed. I code the fact by 
calling object_unparent() along with it.

While I could write a function like memory_region_unparent() and replace
such memory_region_del_subregion() calls, I used a few other insights to
simplify the code:
- Deletable MemoryRegions are of hotpluggable devices.
- Devices do no longer accept new accesses after unrealization.

So I made the common qdev code call memory_region_del_subregion() and
object_unparent(). In the end, this series makes the code simpler and
semantically robust, and kills the entire class of memory leak.

Patch [1, 2] removes object_unparent() calls in instance_finalize(),
which are incorrect.

Patch 3 makes the qdev code automatically call
memory_region_del_subregion().

Patch [4, 15] removes memory_region_del_subregion() calls that are
obviously no longer needed, demonstrating the benefit of automatic
automatic subregion deletion.

Patch 16 adds the object_unparent() call and stop piggybacking.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Akihiko Odaki (16):
      docs/devel: Do not unparent in instance_finalize
      vfio/pci: Do not unparent in instance_finalize
      qdev: Automatically delete memory subregions
      hw/char/diva-gsp: Do not delete the subregion
      hw/char/serial-pci-multi: Do not delete the subregion
      secondary-vga: Do not delete the subregions
      cmd646: Do not delete the subregions
      hw/ide/piix: Do not delete the subregions
      hw/ide/via: Do not delete the subregions
      hw/nvme: Do not delete the subregion
      pci: Do not delete the subregions
      hw/ppc/spapr_pci: Do not delete the subregions
      hw/usb/hcd-ehci: Do not delete the subregions
      hw/usb/hcd-xhci: Do not delete the subregions
      vfio-user: Do not delete the subregion
      memory: Stop piggybacking on memory region owners

 MAINTAINERS                |  1 +
 docs/devel/memory.rst      | 45 +++++++++++++++++-----------------------
 include/hw/qdev-core.h     |  2 ++
 include/system/memory.h    | 51 +++++++++++++++++++++++-----------------------
 hw/char/diva-gsp.c         |  1 -
 hw/char/serial-pci-multi.c |  1 -
 hw/core/qdev.c             | 29 ++++++++++++++++++++++++++
 hw/display/vga-pci.c       |  8 --------
 hw/ide/cmd646.c            | 12 -----------
 hw/ide/piix.c              | 13 ------------
 hw/ide/via.c               | 12 -----------
 hw/nvme/ctrl.c             |  2 --
 hw/pci/pci.c               | 20 ------------------
 hw/ppc/spapr_pci.c         | 22 --------------------
 hw/usb/hcd-ehci.c          |  4 ----
 hw/usb/hcd-xhci.c          | 10 ---------
 hw/vfio-user/pci.c         |  6 ------
 hw/vfio/pci.c              |  4 ----
 stubs/memory.c             |  9 ++++++++
 system/memory.c            | 11 +++-------
 stubs/meson.build          |  1 +
 21 files changed, 89 insertions(+), 175 deletions(-)
---
base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
change-id: 20250831-mr-d0dc495bad11

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

[PATCH 01/16] 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 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.

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 57fb2aec76e066236d33efe1033d2e73c7f7c295..749f11d8a4ddc80f2d44b66fa41fb12c0fa54006 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 02/16] vfio/pci: Do not unparent in instance_finalize

[Akihiko Odaki]

This reflects a recent change of: docs/devel/memory.rst

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);
     }
 }

-- 
2.51.0

[PATCH 03/16] qdev: Automatically delete memory subregions

[Akihiko Odaki]

A common pattern is that to delete memory subregions during realization
error handling and unrealization. pci automatically automatically
deletes the IO subregions, but the pattern is manually implemented
in other places, which is tedious and error-prone.

Implement the logic to delete subregions in qdev to cover all devices.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/hw/qdev-core.h |  2 ++
 hw/core/qdev.c         | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 530f3da70218df59da72dc7a975dca8265600e00..c2582a4d59b38152a00d066351492c2e2ae0718f 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -226,6 +226,7 @@ typedef QLIST_HEAD(, BusState) BusStateHead;
 struct DeviceState {
     /* private: */
     Object parent_obj;
+
     /* public: */
 
     /**
@@ -526,6 +527,7 @@ bool qdev_realize_and_unref(DeviceState *dev, BusState *bus, Error **errp);
  *  - unrealize any child buses by calling qbus_unrealize()
  *    (this will recursively unrealize any devices on those buses)
  *  - call the unrealize method of @dev
+ *  - remove @dev from memory
  *
  * The device can then be freed by causing its reference count to go
  * to zero.
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f600226176871361d7ff3875f5d06bd4e614be6e..8fdf6774f87ec8424348e8c9652dc4c99a2faeb5 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -451,6 +451,17 @@ static bool check_only_migratable(Object *obj, Error **errp)
     return true;
 }
 
+static int del_memory_region(Object *child, void *opaque)
+{
+    MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
+
+    if (mr && mr->container) {
+        memory_region_del_subregion(mr->container, mr);
+    }
+
+    return 0;
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -582,6 +593,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (dc->unrealize) {
             dc->unrealize(dev);
         }
+        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
     }
@@ -606,6 +618,8 @@ post_realize_fail:
     }
 
 fail:
+    object_child_foreach(OBJECT(dev), del_memory_region, NULL);
+
     error_propagate(errp, local_err);
     if (unattached_parent) {
         /*

-- 
2.51.0

[PATCH 04/16] hw/char/diva-gsp: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/char/diva-gsp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/char/diva-gsp.c b/hw/char/diva-gsp.c
index e1f0713cb794d0442c56935dfe56d784d96949f0..1ae472e879b53555d4751b6a4848354f81c27fee 100644
--- a/hw/char/diva-gsp.c
+++ b/hw/char/diva-gsp.c
@@ -63,7 +63,6 @@ static void diva_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->membar, &s->io);
         g_free(pci->name[i]);
     }
     qemu_free_irqs(pci->irqs, pci->ports);

-- 
2.51.0

[PATCH 05/16] hw/char/serial-pci-multi: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/char/serial-pci-multi.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/char/serial-pci-multi.c b/hw/char/serial-pci-multi.c
index 13df272691a64f14ca3597174815653a01fbd381..3132ca90cebfe411cd3333b62ed75dd01f501067 100644
--- a/hw/char/serial-pci-multi.c
+++ b/hw/char/serial-pci-multi.c
@@ -57,7 +57,6 @@ static void multi_serial_pci_exit(PCIDevice *dev)
     for (i = 0; i < pci->ports; i++) {
         s = pci->state + i;
         qdev_unrealize(DEVICE(s));
-        memory_region_del_subregion(&pci->iobar, &s->io);
         g_free(pci->name[i]);
     }
 }

-- 
2.51.0

[PATCH 06/16] secondary-vga: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/display/vga-pci.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/display/vga-pci.c b/hw/display/vga-pci.c
index b81f7fd2d0fd11913e6b132897028ca490e74b95..90b4545d382135b109e3bbc880bb8fe1a4fc5275 100644
--- a/hw/display/vga-pci.c
+++ b/hw/display/vga-pci.c
@@ -307,14 +307,6 @@ static void pci_secondary_vga_exit(PCIDevice *dev)
     VGACommonState *s = &d->vga;
 
     graphic_console_close(s->con);
-    memory_region_del_subregion(&d->mmio, &d->mrs[0]);
-    memory_region_del_subregion(&d->mmio, &d->mrs[1]);
-    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_QEXT)) {
-        memory_region_del_subregion(&d->mmio, &d->mrs[2]);
-    }
-    if (d->flags & (1 << PCI_VGA_FLAG_ENABLE_EDID)) {
-        memory_region_del_subregion(&d->mmio, &d->mrs[3]);
-    }
 }
 
 static void pci_secondary_vga_init(Object *obj)

-- 
2.51.0

[PATCH 07/16] cmd646: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/cmd646.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 2a59516a9ddbc0a7d40400f521b9cef07006b76a..ea4d501c5e40b0f7f5fcd5c025c113d97d89f5b7 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -302,17 +302,6 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void pci_cmd646_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 static const Property cmd646_ide_properties[] = {
     DEFINE_PROP_UINT32("secondary", PCIIDEState, secondary, 0),
 };
@@ -325,7 +314,6 @@ static void cmd646_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, cmd646_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_cmd646_ide_realize;
-    k->exit = pci_cmd646_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_CMD;
     k->device_id = PCI_DEVICE_ID_CMD_646;
     k->revision = 0x07;

-- 
2.51.0

[PATCH 08/16] hw/ide/piix: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/piix.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/hw/ide/piix.c b/hw/ide/piix.c
index a0f2709c6973420b9e07fc5cc3fa1ef12a8e3d42..138f8e1936b448cb9185018ba5744d3c5445abd9 100644
--- a/hw/ide/piix.c
+++ b/hw/ide/piix.c
@@ -166,17 +166,6 @@ static void pci_piix_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void pci_piix_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < 2; ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 /* NOTE: for the PIIX3, the IRQs and IOports are hardcoded */
 static void piix3_ide_class_init(ObjectClass *klass, const void *data)
 {
@@ -186,7 +175,6 @@ static void piix3_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, piix_ide_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371SB_1;
     k->class_id = PCI_CLASS_STORAGE_IDE;
@@ -209,7 +197,6 @@ static void piix4_ide_class_init(ObjectClass *klass, const void *data)
     device_class_set_legacy_reset(dc, piix_ide_reset);
     dc->vmsd = &vmstate_ide_pci;
     k->realize = pci_piix_ide_realize;
-    k->exit = pci_piix_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_INTEL;
     k->device_id = PCI_DEVICE_ID_INTEL_82371AB;
     k->class_id = PCI_CLASS_STORAGE_IDE;

-- 
2.51.0

[PATCH 09/16] hw/ide/via: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ide/via.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index dedc2674c002f9de8894e6c49cdb3989cb0deccc..cbaf4ad1548b94afa14809930729eba169f1b061 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -234,17 +234,6 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     }
 }
 
-static void via_ide_exitfn(PCIDevice *dev)
-{
-    PCIIDEState *d = PCI_IDE(dev);
-    unsigned i;
-
-    for (i = 0; i < ARRAY_SIZE(d->bmdma); ++i) {
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].extra_io);
-        memory_region_del_subregion(&d->bmdma_bar, &d->bmdma[i].addr_ioport);
-    }
-}
-
 static void via_ide_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -258,7 +247,6 @@ static void via_ide_class_init(ObjectClass *klass, const void *data)
     k->config_read = via_ide_cfg_read;
     k->config_write = via_ide_cfg_write;
     k->realize = via_ide_realize;
-    k->exit = via_ide_exitfn;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_IDE;
     k->revision = 0x06;

-- 
2.51.0

[PATCH 10/16] hw/nvme: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/nvme/ctrl.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/nvme/ctrl.c b/hw/nvme/ctrl.c
index f5ee6bf260f159249204571a366472f3e0d16dea..eebce1f787f464978f535356533294c5a0c7bea8 100644
--- a/hw/nvme/ctrl.c
+++ b/hw/nvme/ctrl.c
@@ -9058,8 +9058,6 @@ static void nvme_exit(PCIDevice *pci_dev)
     } else {
         msix_uninit(pci_dev, &n->bar0, &n->bar0);
     }
-
-    memory_region_del_subregion(&n->bar0, &n->iomem);
 }
 
 static const Property nvme_props[] = {

-- 
2.51.0

[PATCH 11/16] pci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/pci/pci.c | 20 --------------------
 1 file changed, 20 deletions(-)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..1dd577ca20ab204b5f38469c801e421398660718 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1188,10 +1188,6 @@ static void do_pci_unregister_device(PCIDevice *pci_dev)
     if (xen_mode == XEN_EMULATE) {
         xen_evtchn_remove_pci_device(pci_dev);
     }
-    if (memory_region_is_mapped(&pci_dev->bus_master_enable_region)) {
-        memory_region_del_subregion(&pci_dev->bus_master_container_region,
-                                    &pci_dev->bus_master_enable_region);
-    }
     address_space_destroy(&pci_dev->bus_master_as);
 }
 
@@ -1417,27 +1413,11 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev,
     return pci_dev;
 }
 
-static void pci_unregister_io_regions(PCIDevice *pci_dev)
-{
-    PCIIORegion *r;
-    int i;
-
-    for(i = 0; i < PCI_NUM_REGIONS; i++) {
-        r = &pci_dev->io_regions[i];
-        if (!r->size || r->addr == PCI_BAR_UNMAPPED)
-            continue;
-        memory_region_del_subregion(r->address_space, r->memory);
-    }
-
-    pci_unregister_vga(pci_dev);
-}
-
 static void pci_qdev_unrealize(DeviceState *dev)
 {
     PCIDevice *pci_dev = PCI_DEVICE(dev);
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(pci_dev);
 
-    pci_unregister_io_regions(pci_dev);
     pci_del_option_rom(pci_dev);
     pcie_sriov_unregister_device(pci_dev);
 

-- 
2.51.0

[PATCH 12/16] hw/ppc/spapr_pci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/ppc/spapr_pci.c | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 1ac1185825e84ca908fd878f6cbe7e8cacac1d89..b4043ee752c5f9ab2c0f5800dffa809d3c182225 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1735,27 +1735,13 @@ static void spapr_phb_unrealize(DeviceState *dev)
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
     SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(phb);
-    SpaprTceTable *tcet;
     int i;
-    const unsigned windows_supported = spapr_phb_windows_supported(sphb);
 
     if (sphb->msi) {
         g_hash_table_unref(sphb->msi);
         sphb->msi = NULL;
     }
 
-    /*
-     * Remove IO/MMIO subregions and aliases, rest should get cleaned
-     * via PHB's unrealize->object_finalize
-     */
-    for (i = windows_supported - 1; i >= 0; i--) {
-        tcet = spapr_tce_find_by_liobn(sphb->dma_liobn[i]);
-        if (tcet) {
-            memory_region_del_subregion(&sphb->iommu_root,
-                                        spapr_tce_get_iommu(tcet));
-        }
-    }
-
     remove_drcs(sphb, phb->bus);
 
     for (i = PCI_NUM_PINS - 1; i >= 0; i--) {
@@ -1767,8 +1753,6 @@ static void spapr_phb_unrealize(DeviceState *dev)
 
     QLIST_REMOVE(sphb, list);
 
-    memory_region_del_subregion(&sphb->iommu_root, &sphb->msiwindow);
-
     /*
      * An attached PCI device may have memory listeners, eg. VFIO PCI. We have
      * unmapped all sections. Remove the listeners now, before destroying the
@@ -1779,12 +1763,6 @@ static void spapr_phb_unrealize(DeviceState *dev)
 
     qbus_set_hotplug_handler(BUS(phb->bus), NULL);
     pci_unregister_root_bus(phb->bus);
-
-    memory_region_del_subregion(get_system_memory(), &sphb->iowindow);
-    if (sphb->mem64_win_pciaddr != (hwaddr)-1) {
-        memory_region_del_subregion(get_system_memory(), &sphb->mem64window);
-    }
-    memory_region_del_subregion(get_system_memory(), &sphb->mem32window);
 }
 
 static void spapr_phb_destroy_msi(gpointer opaque)

-- 
2.51.0

[PATCH 13/16] hw/usb/hcd-ehci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/usb/hcd-ehci.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index b090f253656ba17c7c6b3b805235a9360334baf5..21c3501455b5705b5e155acf9df2156b653f69bf 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -2557,10 +2557,6 @@ void usb_ehci_unrealize(EHCIState *s, DeviceState *dev)
     ehci_queues_rip_all(s, 0);
     ehci_queues_rip_all(s, 1);
 
-    memory_region_del_subregion(&s->mem, &s->mem_caps);
-    memory_region_del_subregion(&s->mem, &s->mem_opreg);
-    memory_region_del_subregion(&s->mem, &s->mem_ports);
-
     usb_bus_release(&s->bus);
 
     if (s->vmstate) {

-- 
2.51.0

[PATCH 14/16] hw/usb/hcd-xhci: Do not delete the subregions

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/usb/hcd-xhci.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 292c378bfc98bf458f7f760cdb6f1f21f23a7633..b68a2aec317107ec36736f762b9dccd71b53dc3e 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3454,16 +3454,6 @@ static void usb_xhci_unrealize(DeviceState *dev)
         xhci->mfwrap_timer = NULL;
     }
 
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_cap);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_oper);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_runtime);
-    memory_region_del_subregion(&xhci->mem, &xhci->mem_doorbell);
-
-    for (i = 0; i < xhci->numports; i++) {
-        XHCIPort *port = &xhci->ports[i];
-        memory_region_del_subregion(&xhci->mem, &port->mem);
-    }
-
     usb_bus_release(&xhci->bus);
 }
 

-- 
2.51.0

[PATCH 15/16] vfio-user: Do not delete the subregion

[Akihiko Odaki]

It is no longer necessary.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/vfio-user/pci.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/vfio-user/pci.c b/hw/vfio-user/pci.c
index be71c777291f0c68b01b54029612c4dbc6aa86e2..0b6c6a1c5ed327ec53b119a799976a0823e304c3 100644
--- a/hw/vfio-user/pci.c
+++ b/hw/vfio-user/pci.c
@@ -73,12 +73,6 @@ static void vfio_user_msix_setup(VFIOPCIDevice *vdev)
 
 static void vfio_user_msix_teardown(VFIOPCIDevice *vdev)
 {
-    MemoryRegion *mr, *sub;
-
-    mr = vdev->bars[vdev->msix->pba_bar].mr;
-    sub = vdev->msix->pba_region;
-    memory_region_del_subregion(mr, sub);
-
     g_free(vdev->msix->pba_region);
     vdev->msix->pba_region = NULL;
 }

-- 
2.51.0

[PATCH 16/16] memory: Stop piggybacking on memory region owners

[Akihiko Odaki]

MemoryRegions used to "piggyback" on their owners instead of using their
own reference counters due to the circular dependencies between them,
which caused a few problems. Stop piggybacking, showing that the
circular dependencies are actually broken.

Problems with piggybacking
==========================

Piggybacking obscured the fact that a MemoryRegion used memory allocated
to its owner, as it didn't take a reference to the owner.

It also created another circular reference when an owner referred to its
MemoryRegion via memory_region_ref(). This could happen in two
scenarios:

- A subregion and its container had the same owner. In this case, the
  container would use memory_region_ref() to take a reference to the
  subregion.

- An owner called address_space_map() with a guest-controlled address
  that points back to its MemoryRegion. This scenario is similar to what
  MemReentrancyGuard handles.

Avoiding such a circular reference required checking if the referrer is
the owner, complicating the code further.

Insight
=======

This change challenges the underlying assumption that circulare
dependencies exist and can be tolerated. The deletable MemoryRegions are
of hotpluggable devices. A device and its MemoryRegion have the
following dependencies:

QOM tree -> Device
Container -> MemoryRegion
Device <-> MemoryRegion

Techniques like piggybacking or a hypothetical garbage collector can
finalize the device and the MemoryRegion once the QOM tree and container
lose their dependencies. However, these methods are fundamentally
insufficient because the MemoryRegion and the device have finalizers,
and they do not respect the dependencies these objects may have during
finalization.

As long as the object model based on the device and the MemoryRegion is
correct, one of them should lose its dependency on the first,
establishing a valid finalization order. Understanding this allows using
standard reference counting, which is immune to the problems of
piggybacking.

Analysis reveals that the device no longer depends on the
MemoryRegion after unrealization. The device needs the MemoryRegion to
add it to a container and expose it to an AddressSpace. Once unrealized,
the device is hidden from the AddresSpaces and thus no longer needs the
MemoryRegion.

This suggests that the circular dependencies are broken and that the
device should be finalized *after*:

- It is unrealized.
- The MemoryRegion is finalized.

Solution
========

Fortunately, this requirement can be coded with minimal complexity.
Hotpluggable devices already explicitly call
memory_region_del_subregion() to stop accepting new accesses to its
MemoryRegions after unrealization. This change lets object_unparent()
calls follow that, permitting the MemoryRegions to be finalized earlier
than the device.

This enforces a valid finalization order, allows MemoryRegions to rely
on standard reference counting, and eliminates the entire class of
memory leaks caused by piggybacking.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 MAINTAINERS             |  1 +
 docs/devel/memory.rst   | 32 +++++++++++++++----------------
 include/system/memory.h | 51 ++++++++++++++++++++++++-------------------------
 hw/core/qdev.c          | 25 +++++++++++++++++++-----
 stubs/memory.c          |  9 +++++++++
 system/memory.c         | 11 +++--------
 stubs/meson.build       |  1 +
 7 files changed, 75 insertions(+), 55 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 8147fff3523eaa45c4a0d2c21d40b4ade3f419ff..4665f0a4b7a513c5863f6d5227a0173c836505e6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3212,6 +3212,7 @@ F: include/system/memory.h
 F: include/system/ram_addr.h
 F: include/system/ramblock.h
 F: include/system/memory_mapping.h
+F: stubs/memory.c
 F: system/dma-helpers.c
 F: system/ioport.c
 F: system/memory.c
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 749f11d8a4ddc80f2d44b66fa41fb12c0fa54006..c7bb5602e9c20e19dd8dfe4a814e958cc0518598 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -158,30 +158,31 @@ ioeventfd) can be changed during the region lifecycle.  They take effect
 as soon as the region is made visible.  This can be immediately, later,
 or never.
 
-Destruction of a memory region happens automatically when the owner
-object dies.
+You should call object_unparent() if the memory region becomes
+unnecessary for its owner. If its owner is a device, object_unparent()
+will be called automatically after unrealization.
 
-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.
+You must not free 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 allocate or
+free 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:
 
-- the memory region's owner had a reference taken via memory_region_ref
+- the memory region had a reference taken via memory_region_ref
   (for example by address_space_map)
 
-- the region is unparented, and has no owner anymore
+- the region is freed
 
-- when address_space_unmap is called, the reference to the memory region's
-  owner is leaked.
+- when the mapped memory is used, the use of the memory region
+  results in use-after-free.
 
 
-There is an exception to the above rule: it is okay to call
-object_unparent at any time for an alias or a container region.  It is
-therefore also okay to create or destroy alias and container regions
-dynamically during a device's lifetime.
+There is an exception to the above rule: it is okay to free at any time
+for an alias or a container region.  It is therefore also okay to
+allocate or free alias and container regions dynamically during a
+device's lifetime.
 
 This exceptional usage is valid because aliases and containers only help
 QEMU building the guest's memory map; they are never accessed directly.
@@ -191,9 +192,8 @@ 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.  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 free
+regions that have no owner, unless they are aliases or containers.
 
 
 Overlapping regions and priority
diff --git a/include/system/memory.h b/include/system/memory.h
index e2cd6ed126144abaed6f3035e3ef091d747b4c34..1fc773ca49e26d605a4e152b5b41426ad7666912 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1307,7 +1307,7 @@ static inline bool memory_region_section_intersect_range(MemoryRegionSection *s,
  * memory_region_add_subregion() to add subregions.
  *
  * @mr: the #MemoryRegion to be initialized
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region; any subregions beyond this size will be clipped
  */
@@ -1320,14 +1320,14 @@ void memory_region_init(MemoryRegion *mr,
  * memory_region_ref: Add 1 to a memory region's reference count
  *
  * Whenever memory regions are accessed outside the BQL, they need to be
- * preserved against hot-unplug.  MemoryRegions actually do not have their
- * own reference count; they piggyback on a QOM object, their "owner".
- * This function adds a reference to the owner.
+ * preserved against hot-unplug. This function adds a reference to the
+ * memory region.
  *
- * All MemoryRegions must have an owner if they can disappear, even if the
- * device they belong to operates exclusively under the BQL.  This is because
- * the region could be returned at any time by memory_region_find, and this
- * is usually under guest control.
+ * We do not ref/unref memory regions without an owner because it slows
+ * down DMA sensibly. All MemoryRegions must have an owner if they can
+ * disappear, even if the device they belong to operates exclusively
+ * under the BQL.  This is because the region could be returned at any
+ * time by memory_region_find, and this is usually under guest control.
  *
  * @mr: the #MemoryRegion
  */
@@ -1337,9 +1337,8 @@ void memory_region_ref(MemoryRegion *mr);
  * memory_region_unref: Remove 1 to a memory region's reference count
  *
  * Whenever memory regions are accessed outside the BQL, they need to be
- * preserved against hot-unplug.  MemoryRegions actually do not have their
- * own reference count; they piggyback on a QOM object, their "owner".
- * This function removes a reference to the owner and possibly destroys it.
+ * preserved against hot-unplug. This function removes a reference to
+ * the memory and possibly destroys it.
  *
  * @mr: the #MemoryRegion
  */
@@ -1352,7 +1351,7 @@ void memory_region_unref(MemoryRegion *mr);
  * if @size is nonzero, subregions will be clipped to @size.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @ops: a structure containing read and write callbacks to be used when
  *       I/O is performed on the region.
  * @opaque: passed to the read and write callbacks of the @ops structure.
@@ -1372,7 +1371,7 @@ void memory_region_init_io(MemoryRegion *mr,
  *                                    directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1395,7 +1394,7 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
  *                                          modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1425,7 +1424,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
  *                                     canceled.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: used size of the region.
@@ -1454,7 +1453,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
  *                                    mmap-ed backend.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1487,7 +1486,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
  *                                  mmap-ed backend.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: the name of the region.
  * @size: size of the region.
  * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
@@ -1518,7 +1517,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
  *                              region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1546,7 +1545,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
  * skip_dump flag.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: the name of the region.
  * @size: size of the region.
  * @ptr: memory to be mapped; must contain at least @size bytes.
@@ -1566,7 +1565,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
  *                           part of another memory region.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: used for debugging; not visible to the user or ABI
  * @orig: the region to be referenced; @mr will be equivalent to
  *        @orig between @offset and @offset + @size - 1.
@@ -1592,7 +1591,7 @@ void memory_region_init_alias(MemoryRegion *mr,
  * of the caller.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1615,7 +1614,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
  * of the caller.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @ops: callbacks for write access handling (must not be NULL).
  * @opaque: passed to the read and write callbacks of the @ops structure.
  * @name: Region name, becomes part of RAMBlock name used in migration stream
@@ -1651,7 +1650,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
  * @_iommu_mr: the #IOMMUMemoryRegion to be initialized
  * @instance_size: the IOMMUMemoryRegion subclass instance size
  * @mrtypename: the type name of the #IOMMUMemoryRegion
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region.
  */
@@ -1667,7 +1666,7 @@ void memory_region_init_iommu(void *_iommu_mr,
  *                          region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized
- * @owner: the object that tracks the region's reference count (must be
+ * @owner: the object that provides the region's storage (must be
  *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
  * @name: name of the memory region
  * @size: size of the region in bytes
@@ -1713,7 +1712,7 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
  * If you pass a non-NULL non-device @owner then we will assert.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1744,7 +1743,7 @@ bool memory_region_init_rom(MemoryRegion *mr,
  * If you pass a non-NULL non-device @owner then we will assert.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that provides the region's storage
  * @ops: callbacks for write access handling (must not be NULL).
  * @opaque: passed to the read and write callbacks of the @ops structure.
  * @name: Region name, becomes part of RAMBlock name used in migration stream
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 8fdf6774f87ec8424348e8c9652dc4c99a2faeb5..3b1a01cdeec5b6823217fbc2d611ec94ac1ccf51 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -451,17 +451,32 @@ static bool check_only_migratable(Object *obj, Error **errp)
     return true;
 }
 
-static int del_memory_region(Object *child, void *opaque)
+static int collect_memory_region(Object *child, void *opaque)
 {
     MemoryRegion *mr = (MemoryRegion *)object_dynamic_cast(child, TYPE_MEMORY_REGION);
 
-    if (mr && mr->container) {
-        memory_region_del_subregion(mr->container, mr);
+    if (mr) {
+        if (mr->container) {
+            memory_region_del_subregion(mr->container, mr);
+        }
+
+        g_array_append_val(opaque, child);
     }
 
     return 0;
 }
 
+static void del_memory_regions(DeviceState *dev)
+{
+    g_autoptr(GArray) array = g_array_new(FALSE, FALSE, sizeof(Object *));
+
+    object_child_foreach(OBJECT(dev), collect_memory_region, array);
+
+    for (gsize i = 0; i < array->len; i++) {
+        object_unparent(g_array_index(array, Object *, i));
+    }
+}
+
 static void device_set_realized(Object *obj, bool value, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
@@ -593,7 +608,7 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
         if (dc->unrealize) {
             dc->unrealize(dev);
         }
-        object_child_foreach(OBJECT(dev), del_memory_region, NULL);
+        del_memory_regions(dev);
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
     }
@@ -618,7 +633,7 @@ post_realize_fail:
     }
 
 fail:
-    object_child_foreach(OBJECT(dev), del_memory_region, NULL);
+    del_memory_regions(dev);
 
     error_propagate(errp, local_err);
     if (unattached_parent) {
diff --git a/stubs/memory.c b/stubs/memory.c
new file mode 100644
index 0000000000000000000000000000000000000000..9c36531ae542d804dc19ed2a3c657005881a2bca
--- /dev/null
+++ b/stubs/memory.c
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+
+#include "qemu/osdep.h"
+#include "system/memory.h"
+
+void memory_region_del_subregion(MemoryRegion *mr,
+                                 MemoryRegion *subregion)
+{
+}
diff --git a/system/memory.c b/system/memory.c
index 56465479406f4a264bfe13e6a2bc7d9b6565410f..f81c634bcca4c4f32dea6796e5f98f6e2baaa94c 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1222,7 +1222,7 @@ static void memory_region_do_init(MemoryRegion *mr,
         mr->size = int128_2_64();
     }
     mr->name = g_strdup(name);
-    mr->owner = owner;
+    mr->owner = object_ref(owner);
     mr->dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE);
     mr->ram_block = NULL;
 
@@ -1816,6 +1816,7 @@ static void memory_region_finalize(Object *obj)
     memory_region_clear_coalescing(mr);
     g_free((char *)mr->name);
     g_free(mr->ioeventfds);
+    object_unref(mr->owner);
 }
 
 Object *memory_region_owner(MemoryRegion *mr)
@@ -1826,13 +1827,7 @@ Object *memory_region_owner(MemoryRegion *mr)
 
 void memory_region_ref(MemoryRegion *mr)
 {
-    /* MMIO callbacks most likely will access data that belongs
-     * to the owner, hence the need to ref/unref the owner whenever
-     * the memory region is in use.
-     *
-     * The memory region is a child of its owner.  As long as the
-     * owner doesn't call unparent itself on the memory region,
-     * ref-ing the owner will also keep the memory region alive.
+    /*
      * Memory regions without an owner are supposed to never go away;
      * we do not ref/unref them because it slows down DMA sensibly.
      */
diff --git a/stubs/meson.build b/stubs/meson.build
index cef046e6854ddaa9f12714c317a541ea75b8d412..b4df4e60a1af89c9354d5b92449ce5409095b9f1 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -95,5 +95,6 @@ if have_system or have_user
 
   # Also included in have_system for tests/unit/test-qdev-global-props
   stub_ss.add(files('hotplug-stubs.c'))
+  stub_ss.add(files('memory.c'))
   stub_ss.add(files('sysbus.c'))
 endif

-- 
2.51.0

Re: [PATCH 02/16] vfio/pci: Do not unparent in instance_finalize

[Cédric Le Goater]

On 9/1/25 08:10, Akihiko Odaki wrote:
> This reflects a recent change of: docs/devel/memory.rst

This patch could be merged quickly in the VFIO tree but it
would require a better commit log.

Thanks,

C.



  
> 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);
>       }
>   }
> 

Re: [PATCH 00/16] memory: Stop piggybacking on memory region owners

[Peter Maydell]

On Mon, 1 Sept 2025 at 07:11, Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> Supersedes: https://lore.kernel.org/qemu-devel/20250828-san-v9-0-c0dff4b8a487@rsg.ci.i.u-tokyo.ac.jp/
> ("[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer errors")
>
> MemoryRegions used to "piggyback" on their owners instead of using their
> reference counters due to the circular dependencies between them, which
> caused memory leak.
>
> I tried to fix it with "[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer
> errors" but it resulted in a lengthy discussion; ultimately it is
> attributed to the fact that "piggybacking" is hard to understand and
> forces us design trade-offs. It was also insufficient because it only
> deals with the container-subregion pattern and did not deal with DMA.

Unlike Peter Xu's proposed patch and your v9 patch you reference
above, with this series I still see leaks doing a 'make check'
on an ASAN build of the Arm targets. Here's a sample leak
detected during the device-introspect-test:

==3769612==ERROR: LeakSanitizer: detected memory leaks

Too many leaks! Only the first 5000 leaks encountered will be reported.
Direct leak of 120 byte(s) in 8 object(s) allocated from:
    #0 0x61e094196de3 in malloc
(/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-arm+0x21f2de3)
(BuildId: 9b33a0e2d440e084929ae6a2821eacb977772688)
    #1 0x79c9d0e06b09 in g_malloc
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId:
1eb6131419edb83b2178b682829a6913cf682d75)
    #2 0x79c9d0e1c4d8 in g_strdup
(/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x784d8) (BuildId:
1eb6131419edb83b2178b682829a6913cf682d75)
    #3 0x61e0958b6749 in g_strdup_inline
/usr/include/glib-2.0/glib/gstrfuncs.h:321:10
    #4 0x61e0958b6749 in memory_region_do_init
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1224:16
    #5 0x61e0958b6551 in memory_region_init
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1250:5
    #6 0x61e0958bc097 in memory_region_init_io
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1568:5
    #7 0x61e09494b6d0 in stm32l4x5_gpio_init
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/gpio/stm32l4x5_gpio.c:402:5
    #8 0x61e096a36371 in object_init_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:428:9
    #9 0x61e096a1d8db in object_initialize_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:570:5
    #10 0x61e096a1d220 in object_initialize
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:578:5
    #11 0x61e096a1dbdc in object_initialize_child_with_propsv
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:608:5
    #12 0x61e096a1dab7 in object_initialize_child_with_props
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:591:10
    #13 0x61e096a1e607 in object_initialize_child_internal
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:645:5
    #14 0x61e0962c7f9a in stm32l4x5_soc_initfn
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/arm/stm32l4x5_soc.c:150:9
    #15 0x61e096a36371 in object_init_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:428:9
    #16 0x61e096a36242 in object_init_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:424:9
    #17 0x61e096a1d8db in object_initialize_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:570:5
    #18 0x61e096a1f1fd in object_new_with_type
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:774:5
    #19 0x61e096a1efc9 in object_new_with_class
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:782:12
    #20 0x61e09709cec5 in qmp_device_list_properties
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/qom-qmp-cmds.c:206:11
    #21 0x61e09594492c in qdev_device_help
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/qdev-monitor.c:313:17
    #22 0x61e09594ac2c in hmp_device_add
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/qdev-monitor.c:989:9
    #23 0x61e095b17b2d in handle_hmp_command_exec
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/hmp.c:1106:9
    #24 0x61e095b12035 in handle_hmp_command
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/hmp.c:1158:9
    #25 0x61e095b2549d in qmp_human_monitor_command
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/qmp-cmds.c:179:5
    #26 0x61e09720c44a in qmp_marshal_human_monitor_command
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qapi/qapi-commands-misc.c:347:14
    #27 0x61e0973140f1 in do_qmp_dispatch_bh
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qapi/qmp-dispatch.c:128:5
    #28 0x61e0973f01ad in aio_bh_call
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/async.c:172:5
    #29 0x61e0973f0ee6 in aio_bh_poll
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/async.c:219:13
    #30 0x61e09735c8b8 in aio_dispatch
/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/aio-posix.c:436:5

(there are many more after this one)

thanks
-- PMM

Re: [PATCH 00/16] memory: Stop piggybacking on memory region owners

[Peter Maydell]

On Mon, 1 Sept 2025 at 13:35, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 1 Sept 2025 at 07:11, Akihiko Odaki
> <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
> >
> > Supersedes: https://lore.kernel.org/qemu-devel/20250828-san-v9-0-c0dff4b8a487@rsg.ci.i.u-tokyo.ac.jp/
> > ("[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer errors")
> >
> > MemoryRegions used to "piggyback" on their owners instead of using their
> > reference counters due to the circular dependencies between them, which
> > caused memory leak.
> >
> > I tried to fix it with "[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer
> > errors" but it resulted in a lengthy discussion; ultimately it is
> > attributed to the fact that "piggybacking" is hard to understand and
> > forces us design trade-offs. It was also insufficient because it only
> > deals with the container-subregion pattern and did not deal with DMA.
>
> Unlike Peter Xu's proposed patch and your v9 patch you reference
> above, with this series I still see leaks doing a 'make check'
> on an ASAN build of the Arm targets. Here's a sample leak
> detected during the device-introspect-test:

I should mention that I'm using an lsan-suppressions.txt file
with the following entries:

# This is a set of suppressions for LeakSanitizer; you can use it
# by setting
#   LSAN_OPTIONS="suppressions=/path/to/scripts/lsan-suppressions.txt"
# register_init_block API is busted
leak:register_init_block
leak:canfd_populate_regarray
# qtest-only leak, not very important
leak:qemu_irq_intercept_in
# this is maybe a leak caused by g_test_trap_subprocess():
# in the subprocess, the cleanup functions that are supposed to free
# memory don't get run for some reason.
leak:qos_traverse_graph

plus various leak fixes which I've sent out over the past week or two:

[PATCH 0/3] hw: Fix qemu_init_irq() leaks
 https://patchew.org/QEMU/20250821154053.2417090-1-peter.maydell@linaro.org/

[PATCH] hw/char/max78000_uart: Destroy FIFO on deinit
https://patchew.org/QEMU/20250821154358.2417744-1-peter.maydell@linaro.org/

[PATCH] hw/gpio/pca9554: Avoid leak in pca9554_set_pin()
https://patchew.org/QEMU/20250821154459.2417976-1-peter.maydell@linaro.org/

[PATCH 0/2] hw: fix some leaks in xlnx devices
https://patchew.org/QEMU/20250826174956.3010274-1-peter.maydell@linaro.org/

[PATCH] hw/arm/boot: Correctly free the MemoryDeviceInfoList
https://patchew.org/QEMU/20250901102214.3748011-1-peter.maydell@linaro.org/

and with those patches plus the lsan-suppressions file plus either
Peter Xu's patch or your v9 patch I get a clean 'make check' run.

thanks
-- PMM

Re: [PATCH 00/16] memory: Stop piggybacking on memory region owners

[Akihiko Odaki]

On 2025/09/01 21:35, Peter Maydell wrote:
> On Mon, 1 Sept 2025 at 07:11, Akihiko Odaki
> <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>>
>> Supersedes: https://lore.kernel.org/qemu-devel/20250828-san-v9-0-c0dff4b8a487@rsg.ci.i.u-tokyo.ac.jp/
>> ("[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer errors")
>>
>> MemoryRegions used to "piggyback" on their owners instead of using their
>> reference counters due to the circular dependencies between them, which
>> caused memory leak.
>>
>> I tried to fix it with "[PATCH v9 0/2] Fix check-qtest-ppc64 sanitizer
>> errors" but it resulted in a lengthy discussion; ultimately it is
>> attributed to the fact that "piggybacking" is hard to understand and
>> forces us design trade-offs. It was also insufficient because it only
>> deals with the container-subregion pattern and did not deal with DMA.
> 
> Unlike Peter Xu's proposed patch and your v9 patch you reference
> above, with this series I still see leaks doing a 'make check'
> on an ASAN build of the Arm targets. Here's a sample leak
> detected during the device-introspect-test:
> 
> ==3769612==ERROR: LeakSanitizer: detected memory leaks
> 
> Too many leaks! Only the first 5000 leaks encountered will be reported.
> Direct leak of 120 byte(s) in 8 object(s) allocated from:
>      #0 0x61e094196de3 in malloc
> (/mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qemu-system-arm+0x21f2de3)
> (BuildId: 9b33a0e2d440e084929ae6a2821eacb977772688)
>      #1 0x79c9d0e06b09 in g_malloc
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x62b09) (BuildId:
> 1eb6131419edb83b2178b682829a6913cf682d75)
>      #2 0x79c9d0e1c4d8 in g_strdup
> (/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x784d8) (BuildId:
> 1eb6131419edb83b2178b682829a6913cf682d75)
>      #3 0x61e0958b6749 in g_strdup_inline
> /usr/include/glib-2.0/glib/gstrfuncs.h:321:10
>      #4 0x61e0958b6749 in memory_region_do_init
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1224:16
>      #5 0x61e0958b6551 in memory_region_init
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1250:5
>      #6 0x61e0958bc097 in memory_region_init_io
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/memory.c:1568:5
>      #7 0x61e09494b6d0 in stm32l4x5_gpio_init
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/gpio/stm32l4x5_gpio.c:402:5
>      #8 0x61e096a36371 in object_init_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:428:9
>      #9 0x61e096a1d8db in object_initialize_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:570:5
>      #10 0x61e096a1d220 in object_initialize
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:578:5
>      #11 0x61e096a1dbdc in object_initialize_child_with_propsv
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:608:5
>      #12 0x61e096a1dab7 in object_initialize_child_with_props
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:591:10
>      #13 0x61e096a1e607 in object_initialize_child_internal
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:645:5
>      #14 0x61e0962c7f9a in stm32l4x5_soc_initfn
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../hw/arm/stm32l4x5_soc.c:150:9
>      #15 0x61e096a36371 in object_init_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:428:9
>      #16 0x61e096a36242 in object_init_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:424:9
>      #17 0x61e096a1d8db in object_initialize_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:570:5
>      #18 0x61e096a1f1fd in object_new_with_type
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:774:5
>      #19 0x61e096a1efc9 in object_new_with_class
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/object.c:782:12
>      #20 0x61e09709cec5 in qmp_device_list_properties
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qom/qom-qmp-cmds.c:206:11
>      #21 0x61e09594492c in qdev_device_help
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/qdev-monitor.c:313:17
>      #22 0x61e09594ac2c in hmp_device_add
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../system/qdev-monitor.c:989:9
>      #23 0x61e095b17b2d in handle_hmp_command_exec
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/hmp.c:1106:9
>      #24 0x61e095b12035 in handle_hmp_command
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/hmp.c:1158:9
>      #25 0x61e095b2549d in qmp_human_monitor_command
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../monitor/qmp-cmds.c:179:5
>      #26 0x61e09720c44a in qmp_marshal_human_monitor_command
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/qapi/qapi-commands-misc.c:347:14
>      #27 0x61e0973140f1 in do_qmp_dispatch_bh
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../qapi/qmp-dispatch.c:128:5
>      #28 0x61e0973f01ad in aio_bh_call
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/async.c:172:5
>      #29 0x61e0973f0ee6 in aio_bh_poll
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/async.c:219:13
>      #30 0x61e09735c8b8 in aio_dispatch
> /mnt/nvmedisk/linaro/qemu-from-laptop/qemu/build/arm-asan/../../util/aio-posix.c:436:5
> 
> (there are many more after this one)

This approach is clearly not working. The problem here is that there are 
devices that never get realized (so never get unrealized either).

I'm thinking of a solution that fixes all possible circular references 
originated from owners without breaking anything else, but I don't have 
one for now.

Regards,
Akihiko Odaki

Re: [PATCH 00/16] memory: Stop piggybacking on memory region owners

[Peter Maydell]

On Mon, 1 Sept 2025 at 14:28, Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
> This approach is clearly not working. The problem here is that there are
> devices that never get realized (so never get unrealized either).

Yes, the "instance_init -> instance_finalize" lifecycle path
is valid: it gets used when we want to introspect a device
(e.g. find out what properties it has). device-introspect-test
does this for every device compiled into QEMU.

The instance_init -> realize -> unrealize -> instance_finalize path
on the other hand only really needs to work for pluggable devices,
because only those will get unrealized. So we have a lot of
devices where if we ever exercised this path we'd find we had
memory leaks or other bugs. (Many devices for SoC objects and
devices only used in SoC objects don't implement unrealize at all.)

> I'm thinking of a solution that fixes all possible circular references
> originated from owners without breaking anything else, but I don't have
> one for now.

I think overall I like Peter Xu's patch, with the underlying
model being "MemoryRegions are not reference counted objects
that should be considered to have a separate existence from
the device that backs them; unless they're the 'lives forever,
no owner' kind then what you want is to hold a reference to
the owning device".

-- PMM