[PATCH 00/22] Fix memory region leaks and use-after-finalization

[PATCH 00/22] Fix memory region leaks and use-after-finalization

[Akihiko Odaki]

When developing the next version of "[PATCH 00/16] memory: Stop
piggybacking on memory region owners*", I faced multiple memory region
leaks and use-after-finalization. This series extracts their fixes so
that the number of Cc: won't explode.

Patch "qdev: Automatically delete memory subregions" and the succeeding
patches are for refactoring, but patch "vfio-user: Do not delete the
subregion" does fix use-after-finalization.

* https://lore.kernel.org/qemu-devel/20250901-mr-v1-0-dd7cb6b1480b@rsg.ci.i.u-tokyo.ac.jp/

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
Akihiko Odaki (22):
      docs/devel: Do not unparent in instance_finalize()
      vfio/pci: Do not unparent in instance_finalize()
      hw/pci-bridge: Do not assume immediate MemoryRegion finalization
      target/mips: Fix AddressSpace exposure timing
      target/xtensa: Fix AddressSpace exposure timing
      auxbus: Fix AddressSpace exposure timing
      hw/pci-host/raven: Fix AddressSpace exposure timing
      sun4m: Fix AddressSpace exposure timing
      sun4u: Fix AddressSpace exposure timing
      qdev: Automatically delete memory subregions
      vfio-user: Do not delete the subregion
      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

 MAINTAINERS                |  1 +
 docs/devel/memory.rst      | 19 +++------
 include/hw/pci/pci.h       |  1 +
 include/hw/qdev-core.h     |  1 +
 hw/char/diva-gsp.c         |  1 -
 hw/char/serial-pci-multi.c |  1 -
 hw/core/qdev.c             | 14 +++++++
 hw/display/vga-pci.c       |  8 ----
 hw/ide/cmd646.c            | 12 ------
 hw/ide/piix.c              | 13 -------
 hw/ide/via.c               | 12 ------
 hw/misc/auxbus.c           |  2 +-
 hw/nvme/ctrl.c             |  2 -
 hw/pci-host/raven.c        | 27 ++++++-------
 hw/pci/pci.c               | 22 +----------
 hw/pci/pci_bridge.c        | 96 +++++++++++++++++++++++++---------------------
 hw/ppc/spapr_pci.c         | 22 -----------
 hw/sparc/sun4m_iommu.c     |  9 ++++-
 hw/sparc64/sun4u_iommu.c   |  9 ++++-
 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 +++++
 target/mips/cpu.c          |  9 ++++-
 target/xtensa/cpu.c        |  8 ++--
 stubs/meson.build          |  1 +
 27 files changed, 129 insertions(+), 194 deletions(-)
---
base-commit: e101d33792530093fa0b0a6e5f43e4d8cfe4581e
change-id: 20250906-use-37ecc903a9e0

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

[PATCH 01/22] 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/22] 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 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/22] hw/pci-bridge: Do not assume immediate MemoryRegion finalization

[Akihiko Odaki]

When updating memory mappings, pci_bridge_update_mappings() performed
the following operations:
1. Start a transaction
2. Delete the subregions from the container
3. Unparent the subregions
4. Initialize the subregions
5. End the transaction

This assumes the old subregion instances are finalized immediately after
3, but it is not true; the finalization is delayed until 5. Remove the
assumption by using functions to dynamically update MemoryRegions.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 include/hw/pci/pci.h |  1 +
 hw/pci/pci.c         |  2 +-
 hw/pci/pci_bridge.c  | 96 ++++++++++++++++++++++++++++------------------------
 3 files changed, 54 insertions(+), 45 deletions(-)

diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 6b7d3ac8a3611d00d1c1c949c260f3dd44d36cc4..d0bd214b4e11eccc085f7ebe421d170f37552ac3 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -256,6 +256,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
                       uint8_t attr, MemoryRegion *memory);
 void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
                       MemoryRegion *io_lo, MemoryRegion *io_hi);
+void pci_update_vga(PCIDevice *pci_dev);
 void pci_unregister_vga(PCIDevice *pci_dev);
 pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num);
 
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index c70b5ceebaf1f2b10768bd030526cbb518da2b8d..516029f66cda6705bded15322cb6f7eb3d42f82c 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1521,7 +1521,7 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
     }
 }
 
-static void pci_update_vga(PCIDevice *pci_dev)
+void pci_update_vga(PCIDevice *pci_dev)
 {
     uint16_t cmd;
 
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 76255c4cd8929cf9f350af4be1a8448791d52afa..240d0f904de9771f81a392ad26be6ba38a41e4f6 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -145,11 +145,10 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type)
     return limit;
 }
 
-static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
-                                  uint8_t type, const char *name,
-                                  MemoryRegion *space,
-                                  MemoryRegion *parent_space,
-                                  bool enabled)
+static void pci_bridge_update_alias(PCIBridge *bridge, bool init,
+                                    MemoryRegion *alias, uint8_t type,
+                                    const char *name, MemoryRegion *space,
+                                    MemoryRegion *parent_space, bool enabled)
 {
     PCIDevice *bridge_dev = PCI_DEVICE(bridge);
     pcibus_t base = pci_bridge_get_base(bridge_dev, type);
@@ -158,25 +157,37 @@ static void pci_bridge_init_alias(PCIBridge *bridge, MemoryRegion *alias,
      * Apparently no way to do this with existing memory APIs. */
     pcibus_t size = enabled && limit >= base ? limit + 1 - base : 0;
 
-    memory_region_init_alias(alias, OBJECT(bridge), name, space, base, size);
+    if (init) {
+        memory_region_init_alias(alias, OBJECT(bridge), name, space, base, size);
+    } else {
+        memory_region_set_size(alias, size);
+        memory_region_set_alias_offset(alias, base);
+    }
+
     memory_region_add_subregion_overlap(parent_space, base, alias, 1);
 }
 
-static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent,
-                                        MemoryRegion *alias_vga)
+static void pci_bridge_update_vga_aliases(PCIBridge *br, bool init,
+                                          PCIBus *parent,
+                                          MemoryRegion *alias_vga)
 {
     PCIDevice *pd = PCI_DEVICE(br);
     uint16_t brctl = pci_get_word(pd->config + PCI_BRIDGE_CONTROL);
 
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO], OBJECT(br),
-                             "pci_bridge_vga_io_lo", &br->address_space_io,
-                             QEMU_PCI_VGA_IO_LO_BASE, QEMU_PCI_VGA_IO_LO_SIZE);
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI], OBJECT(br),
-                             "pci_bridge_vga_io_hi", &br->address_space_io,
-                             QEMU_PCI_VGA_IO_HI_BASE, QEMU_PCI_VGA_IO_HI_SIZE);
-    memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM], OBJECT(br),
-                             "pci_bridge_vga_mem", &br->address_space_mem,
-                             QEMU_PCI_VGA_MEM_BASE, QEMU_PCI_VGA_MEM_SIZE);
+    if (init) {
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_LO], OBJECT(br),
+                                 "pci_bridge_vga_io_lo", &br->address_space_io,
+                                 QEMU_PCI_VGA_IO_LO_BASE,
+                                 QEMU_PCI_VGA_IO_LO_SIZE);
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_IO_HI], OBJECT(br),
+                                 "pci_bridge_vga_io_hi", &br->address_space_io,
+                                 QEMU_PCI_VGA_IO_HI_BASE,
+                                 QEMU_PCI_VGA_IO_HI_SIZE);
+        memory_region_init_alias(&alias_vga[QEMU_PCI_VGA_MEM], OBJECT(br),
+                                 "pci_bridge_vga_mem", &br->address_space_mem,
+                                 QEMU_PCI_VGA_MEM_BASE,
+                                 QEMU_PCI_VGA_MEM_SIZE);
+    }
 
     if (brctl & PCI_BRIDGE_CTL_VGA) {
         pci_register_vga(pd, &alias_vga[QEMU_PCI_VGA_MEM],
@@ -185,33 +196,33 @@ static void pci_bridge_init_vga_aliases(PCIBridge *br, PCIBus *parent,
     }
 }
 
-static void pci_bridge_region_init(PCIBridge *br)
+static void pci_bridge_region_update(PCIBridge *br, bool init)
 {
     PCIDevice *pd = PCI_DEVICE(br);
     PCIBus *parent = pci_get_bus(pd);
     PCIBridgeWindows *w = &br->windows;
     uint16_t cmd = pci_get_word(pd->config + PCI_COMMAND);
 
-    pci_bridge_init_alias(br, &w->alias_pref_mem,
-                          PCI_BASE_ADDRESS_MEM_PREFETCH,
-                          "pci_bridge_pref_mem",
-                          &br->address_space_mem,
-                          parent->address_space_mem,
-                          cmd & PCI_COMMAND_MEMORY);
-    pci_bridge_init_alias(br, &w->alias_mem,
-                          PCI_BASE_ADDRESS_SPACE_MEMORY,
-                          "pci_bridge_mem",
-                          &br->address_space_mem,
-                          parent->address_space_mem,
-                          cmd & PCI_COMMAND_MEMORY);
-    pci_bridge_init_alias(br, &w->alias_io,
-                          PCI_BASE_ADDRESS_SPACE_IO,
-                          "pci_bridge_io",
-                          &br->address_space_io,
-                          parent->address_space_io,
-                          cmd & PCI_COMMAND_IO);
-
-    pci_bridge_init_vga_aliases(br, parent, w->alias_vga);
+    pci_bridge_update_alias(br, init, &w->alias_pref_mem,
+                            PCI_BASE_ADDRESS_MEM_PREFETCH,
+                            "pci_bridge_pref_mem",
+                            &br->address_space_mem,
+                            parent->address_space_mem,
+                            cmd & PCI_COMMAND_MEMORY);
+    pci_bridge_update_alias(br, init, &w->alias_mem,
+                            PCI_BASE_ADDRESS_SPACE_MEMORY,
+                            "pci_bridge_mem",
+                            &br->address_space_mem,
+                            parent->address_space_mem,
+                            cmd & PCI_COMMAND_MEMORY);
+    pci_bridge_update_alias(br, init, &w->alias_io,
+                            PCI_BASE_ADDRESS_SPACE_IO,
+                            "pci_bridge_io",
+                            &br->address_space_io,
+                            parent->address_space_io,
+                            cmd & PCI_COMMAND_IO);
+
+    pci_bridge_update_vga_aliases(br, init, parent, w->alias_vga);
 }
 
 static void pci_bridge_region_del(PCIBridge *br, PCIBridgeWindows *w)
@@ -237,14 +248,11 @@ static void pci_bridge_region_cleanup(PCIBridge *br, PCIBridgeWindows *w)
 
 void pci_bridge_update_mappings(PCIBridge *br)
 {
-    PCIBridgeWindows *w = &br->windows;
-
     /* Make updates atomic to: handle the case of one VCPU updating the bridge
      * while another accesses an unaffected region. */
     memory_region_transaction_begin();
-    pci_bridge_region_del(br, w);
-    pci_bridge_region_cleanup(br, w);
-    pci_bridge_region_init(br);
+    pci_bridge_region_del(br, &br->windows);
+    pci_bridge_region_update(br, false);
     memory_region_transaction_commit();
 }
 
@@ -386,7 +394,7 @@ void pci_bridge_initfn(PCIDevice *dev, const char *typename)
     memory_region_init(&br->address_space_io, OBJECT(br), "pci_bridge_io",
                        4 * GiB);
     address_space_init(&br->as_io, &br->address_space_io, "pci_bridge_pci_io");
-    pci_bridge_region_init(br);
+    pci_bridge_region_update(br, true);
     QLIST_INIT(&sec_bus->child);
     QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling);
 

-- 
2.51.0

[PATCH 04/22] target/mips: Fix AddressSpace exposure timing

[Akihiko Odaki]

mips-cpu is not hotpluggable but its instance can still be created and
finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 target/mips/cpu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/target/mips/cpu.c b/target/mips/cpu.c
index 1f6c41fd3401e88637c2c0d8fe3fcf4a38370288..e166b570984a40fba2ef417799a267eac9bff7d3 100644
--- a/target/mips/cpu.c
+++ b/target/mips/cpu.c
@@ -457,6 +457,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
     MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
     Error *local_err = NULL;
 
+#ifndef CONFIG_USER_ONLY
+    if (mcc->cpu_def->lcsr_cpucfg2 & (1 << CPUCFG2_LCSRP)) {
+        address_space_init(&env->iocsr.as,
+                            &env->iocsr.mr, "IOCSR");
+    }
+#endif
+
     if (!clock_get(cpu->clock)) {
 #ifndef CONFIG_USER_ONLY
         if (!qtest_enabled()) {
@@ -505,8 +512,6 @@ static void mips_cpu_initfn(Object *obj)
     if (mcc->cpu_def->lcsr_cpucfg2 & (1 << CPUCFG2_LCSRP)) {
         memory_region_init_io(&env->iocsr.mr, OBJECT(cpu), NULL,
                                 env, "iocsr", UINT64_MAX);
-        address_space_init(&env->iocsr.as,
-                            &env->iocsr.mr, "IOCSR");
     }
 #endif
 }

-- 
2.51.0

[PATCH 05/22] target/xtensa: Fix AddressSpace exposure timing

[Akihiko Odaki]

xtensa-cpu is not hotpluggable but its instance can still be created and
finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 target/xtensa/cpu.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
index ea9b6df3aa24178c8e6a88b02afda5db659199da..63edc3a5b2778c8379a30125481f65361655fe1c 100644
--- a/target/xtensa/cpu.c
+++ b/target/xtensa/cpu.c
@@ -243,7 +243,11 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
     Error *local_err = NULL;
 
 #ifndef CONFIG_USER_ONLY
-    xtensa_irq_init(&XTENSA_CPU(dev)->env);
+    CPUXtensaState *env = &XTENSA_CPU(dev)->env;
+
+    env->address_space_er = g_malloc(sizeof(*env->address_space_er));
+    address_space_init(env->address_space_er, env->system_er, "ER");
+    xtensa_irq_init(env);
 #endif
 
     cpu_exec_realizefn(cs, &local_err);
@@ -268,11 +272,9 @@ static void xtensa_cpu_initfn(Object *obj)
     env->config = xcc->config;
 
 #ifndef CONFIG_USER_ONLY
-    env->address_space_er = g_malloc(sizeof(*env->address_space_er));
     env->system_er = g_malloc(sizeof(*env->system_er));
     memory_region_init_io(env->system_er, obj, NULL, env, "er",
                           UINT64_C(0x100000000));
-    address_space_init(env->address_space_er, env->system_er, "ER");
 
     cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk-in", NULL, cpu, 0);
     clock_set_hz(cpu->clock, env->config->clock_freq_khz * 1000);

-- 
2.51.0

[PATCH 06/22] auxbus: Fix AddressSpace exposure timing

[Akihiko Odaki]

aux-bus is not hotpluggable but its instance can still be created and
finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/misc/auxbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
index 877f34560626f0ef741f00bb6c7272135d264399..c47db4da985d8e81f9eb49542279499c931aac6c 100644
--- a/hw/misc/auxbus.c
+++ b/hw/misc/auxbus.c
@@ -74,12 +74,12 @@ AUXBus *aux_bus_init(DeviceState *parent, const char *name)
     /* Memory related. */
     bus->aux_io = g_malloc(sizeof(*bus->aux_io));
     memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", 1 * MiB);
-    address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
     return bus;
 }
 
 void aux_bus_realize(AUXBus *bus)
 {
+    address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
     qdev_realize(DEVICE(bus->bridge), BUS(bus), &error_fatal);
 }
 

-- 
2.51.0

[PATCH 07/22] hw/pci-host/raven: Fix AddressSpace exposure timing

[Akihiko Odaki]

raven-pcihost is not hotpluggable but its instance can still be created
and finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/pci-host/raven.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index f8c0be5d21c351305742a696a65f70f87b546b0c..82f245c91cf267cdc6a518765a8c31d06eac7228 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -233,6 +233,20 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
     MemoryRegion *address_space_mem = get_system_memory();
     int i;
 
+    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
+
+    /* CPU address space */
+    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
+                                &s->pci_io);
+    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
+                                        &s->pci_io_non_contiguous, 1);
+    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
+    pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(d), NULL,
+                      &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
+
+    address_space_init(&s->bm_as, &s->bm, "raven-bm");
+    pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
+
     /*
      * According to PReP specification section 6.1.6 "System Interrupt
      * Assignments", all PCI interrupts are routed via IRQ 15
@@ -276,14 +290,12 @@ static void raven_pcihost_initfn(Object *obj)
 {
     PCIHostState *h = PCI_HOST_BRIDGE(obj);
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
-    MemoryRegion *address_space_mem = get_system_memory();
     DeviceState *pci_dev;
 
     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
                           "pci-io-non-contiguous", 0x00800000);
     memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
-    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
 
     /*
      * Raven's raven_io_ops use the address-space API to access pci-conf-idx
@@ -292,15 +304,6 @@ static void raven_pcihost_initfn(Object *obj)
      */
     s->pci_io_non_contiguous.disable_reentrancy_guard = true;
 
-    /* CPU address space */
-    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
-                                &s->pci_io);
-    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
-                                        &s->pci_io_non_contiguous, 1);
-    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
-    pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
-                      &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
-
     /* Bus master address space */
     memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
     memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
@@ -310,8 +313,6 @@ static void raven_pcihost_initfn(Object *obj)
                              get_system_memory(), 0, 0x80000000);
     memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
     memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
-    address_space_init(&s->bm_as, &s->bm, "raven-bm");
-    pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
 
     h->bus = &s->pci_bus;
 

-- 
2.51.0

[PATCH 08/22] sun4m: Fix AddressSpace exposure timing

[Akihiko Odaki]

sun4m-iommu is not hotpluggable but its instance can still be created
and finalized when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/sparc/sun4m_iommu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/sparc/sun4m_iommu.c b/hw/sparc/sun4m_iommu.c
index a7ff36ee78c1d6295efea6499dffc2a481022167..0997f29ccb97d3dec4e3d34db49f2e51b6807a1a 100644
--- a/hw/sparc/sun4m_iommu.c
+++ b/hw/sparc/sun4m_iommu.c
@@ -359,7 +359,6 @@ static void iommu_init(Object *obj)
     memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
                              TYPE_SUN4M_IOMMU_MEMORY_REGION, OBJECT(dev),
                              "iommu-sun4m", UINT64_MAX);
-    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
 
     sysbus_init_irq(dev, &s->irq);
 
@@ -368,6 +367,13 @@ static void iommu_init(Object *obj)
     sysbus_init_mmio(dev, &s->iomem);
 }
 
+static void iommu_realize(DeviceState *dev, Error **errp)
+{
+    IOMMUState *s = SUN4M_IOMMU(dev);
+
+    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
+}
+
 static const Property iommu_properties[] = {
     DEFINE_PROP_UINT32("version", IOMMUState, version, 0),
 };
@@ -377,6 +383,7 @@ static void iommu_class_init(ObjectClass *klass, const void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     device_class_set_legacy_reset(dc, iommu_reset);
+    dc->realize = iommu_realize;
     dc->vmsd = &vmstate_iommu;
     device_class_set_props(dc, iommu_properties);
 }

-- 
2.51.0

[PATCH 09/22] sun4u: Fix AddressSpace exposure timing

[Akihiko Odaki]

sun4u-iommu is not hotpluggable but its instance can still be created
and destroyed when processing the device-list-properties QMP command.
Exposing such a temporary instance to AddressSpace should be
avoided because it leaks the instance.

Expose instances to the AddressSpace at their realization time so that
it won't happen for the temporary instances.

Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
---
 hw/sparc64/sun4u_iommu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/sparc64/sun4u_iommu.c b/hw/sparc64/sun4u_iommu.c
index 14645f475a09ed3a6bef77f8d8b4b6b4b36ae40a..b6568551935610116d33481ae8d9fc08f02ecf7b 100644
--- a/hw/sparc64/sun4u_iommu.c
+++ b/hw/sparc64/sun4u_iommu.c
@@ -298,18 +298,25 @@ static void iommu_init(Object *obj)
     memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
                              TYPE_SUN4U_IOMMU_MEMORY_REGION, OBJECT(s),
                              "iommu-sun4u", UINT64_MAX);
-    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
 
     memory_region_init_io(&s->iomem, obj, &iommu_mem_ops, s, "iommu",
                           IOMMU_NREGS * sizeof(uint64_t));
     sysbus_init_mmio(sbd, &s->iomem);
 }
 
+static void iommu_realize(DeviceState *dev, Error **errp)
+{
+    IOMMUState *s = SUN4U_IOMMU(dev);
+
+    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
+}
+
 static void iommu_class_init(ObjectClass *klass, const void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     device_class_set_legacy_reset(dc, iommu_reset);
+    dc->realize = iommu_realize;
 }
 
 static const TypeInfo iommu_info = {

-- 
2.51.0

[PATCH 10/22] 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>
---
 MAINTAINERS            |  1 +
 include/hw/qdev-core.h |  1 +
 hw/core/qdev.c         | 14 ++++++++++++++
 stubs/memory.c         |  9 +++++++++
 stubs/meson.build      |  1 +
 5 files changed, 26 insertions(+)

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/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 530f3da70218df59da72dc7a975dca8265600e00..8f443d5f8ea5f31d69181cc1ec53a0b022eb71cc 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -526,6 +526,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) {
         /*
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/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

[PATCH 11/22] 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 12/22] 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 13/22] 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 14/22] 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 15/22] 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 16/22] 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 17/22] 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 18/22] 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 19/22] 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 516029f66cda6705bded15322cb6f7eb3d42f82c..2b408c7ec336df08086f1be9a5bd2555e2e906b7 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 20/22] 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 21/22] 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 22/22] 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

Re: [PATCH 07/22] hw/pci-host/raven: Fix AddressSpace exposure timing

[BALATON Zoltan]

On Sat, 6 Sep 2025, Akihiko Odaki wrote:
> raven-pcihost is not hotpluggable but its instance can still be created
> and finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.

This may suggest there's an issue with freeing address spaces that should 
be fixed there at one place if possible rather than adding a new rule to 
remember not to create address spaces in init methods. Should adress space 
be a child of the device too so it's freed with it? This series simplifies 
some devices removing the rule to remember to unparent memory regions but 
this now adds another rule to remember avoid creating address spaces in 
init so overall we're in the same situation and still have to work around 
issues. This trades one workaround for another so still cannot fix all 
issues with freeing all created objects.

> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.

I had a series here: 
https://patchew.org/QEMU/cover.1751493467.git.balaton@eik.bme.hu/
that changes this for raven (among other clean ups) but I could not get 
that merged in the last devel cycle because of PPC being a bit 
unmaintained. I'd prefer that series to be taken first instead of this 
patch so I don't have to rebase that.

Regards,
BALATON Zoltan

> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
> hw/pci-host/raven.c | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
> index f8c0be5d21c351305742a696a65f70f87b546b0c..82f245c91cf267cdc6a518765a8c31d06eac7228 100644
> --- a/hw/pci-host/raven.c
> +++ b/hw/pci-host/raven.c
> @@ -233,6 +233,20 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
>     MemoryRegion *address_space_mem = get_system_memory();
>     int i;
>
> +    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
> +
> +    /* CPU address space */
> +    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
> +                                &s->pci_io);
> +    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
> +                                        &s->pci_io_non_contiguous, 1);
> +    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> +    pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(d), NULL,
> +                      &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
> +
> +    address_space_init(&s->bm_as, &s->bm, "raven-bm");
> +    pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
> +
>     /*
>      * According to PReP specification section 6.1.6 "System Interrupt
>      * Assignments", all PCI interrupts are routed via IRQ 15
> @@ -276,14 +290,12 @@ static void raven_pcihost_initfn(Object *obj)
> {
>     PCIHostState *h = PCI_HOST_BRIDGE(obj);
>     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
> -    MemoryRegion *address_space_mem = get_system_memory();
>     DeviceState *pci_dev;
>
>     memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
>     memory_region_init_io(&s->pci_io_non_contiguous, obj, &raven_io_ops, s,
>                           "pci-io-non-contiguous", 0x00800000);
>     memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
> -    address_space_init(&s->pci_io_as, &s->pci_io, "raven-io");
>
>     /*
>      * Raven's raven_io_ops use the address-space API to access pci-conf-idx
> @@ -292,15 +304,6 @@ static void raven_pcihost_initfn(Object *obj)
>      */
>     s->pci_io_non_contiguous.disable_reentrancy_guard = true;
>
> -    /* CPU address space */
> -    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
> -                                &s->pci_io);
> -    memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,
> -                                        &s->pci_io_non_contiguous, 1);
> -    memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);
> -    pci_root_bus_init(&s->pci_bus, sizeof(s->pci_bus), DEVICE(obj), NULL,
> -                      &s->pci_memory, &s->pci_io, 0, TYPE_PCI_BUS);
> -
>     /* Bus master address space */
>     memory_region_init(&s->bm, obj, "bm-raven", 4 * GiB);
>     memory_region_init_alias(&s->bm_pci_memory_alias, obj, "bm-pci-memory",
> @@ -310,8 +313,6 @@ static void raven_pcihost_initfn(Object *obj)
>                              get_system_memory(), 0, 0x80000000);
>     memory_region_add_subregion(&s->bm, 0         , &s->bm_pci_memory_alias);
>     memory_region_add_subregion(&s->bm, 0x80000000, &s->bm_ram_alias);
> -    address_space_init(&s->bm_as, &s->bm, "raven-bm");
> -    pci_setup_iommu(&s->pci_bus, &raven_iommu_ops, s);
>
>     h->bus = &s->pci_bus;
>
>
>

Re: [PATCH 07/22] hw/pci-host/raven: Fix AddressSpace exposure timing

[Akihiko Odaki]

On 2025/09/06 18:03, BALATON Zoltan wrote:
> On Sat, 6 Sep 2025, Akihiko Odaki wrote:
>> raven-pcihost is not hotpluggable but its instance can still be created
>> and finalized when processing the device-list-properties QMP command.
>> Exposing such a temporary instance to AddressSpace should be
>> avoided because it leaks the instance.
> 
> This may suggest there's an issue with freeing address spaces that 
> should be fixed there at one place if possible rather than adding a new 
> rule to remember not to create address spaces in init methods. Should 
> adress space be a child of the device too so it's freed with it? This 
> series simplifies some devices removing the rule to remember to unparent 
> memory regions but this now adds another rule to remember avoid creating 
> address spaces in init so overall we're in the same situation and still 
> have to work around issues. This trades one workaround for another so 
> still cannot fix all issues with freeing all created objects.

Making address spaces children will fix memory leaks, but it still 
leaves external side effects in init methods. Avoiding external side 
effect in init methods is necessary because devices can be created and 
removed at any time for introspection and it is not a new rule.

It is not necessary to remember to avoid creating address spaces in 
init; roughly speaking, int methods should only add properties and 
anything else should be done during realization.

"[PATCH v2 0/3] memory: Stop piggybacking on memory region owners" will 
cause an assertion failure if someone still manages to create an address 
space accidentally in init, so it is practically not possible to make 
such a mistake.

This series do not remove the rule to remember to unparent memory 
regions; it instead removes the rule to remember to delete memory 
regions from containers, which are distinct operations. And you still 
cannot add memory regions to containers during initialization; you 
should instead add them during realization just as like address space 
creation.

> 
>> Expose instances to the AddressSpace at their realization time so that
>> it won't happen for the temporary instances.
> 
> I had a series here: https://patchew.org/QEMU/ 
> cover.1751493467.git.balaton@eik.bme.hu/
> that changes this for raven (among other clean ups) but I could not get 
> that merged in the last devel cycle because of PPC being a bit 
> unmaintained. I'd prefer that series to be taken first instead of this 
> patch so I don't have to rebase that.

Looking at the series, "[PATCH v2 13/14] hw/pci-host/raven: Do not map 
regions in init method" moves memory_region_init() and 
memory_region_init_io() from raven_pcihost_initfn() to 
raven_pcihost_realize(). This should be avoided because these function 
calls add memory regions as child properties, which may be introspected 
without realization. Perhaps you may drop the patch and rebase your 
series on top of this patch, or let me rebase this patch on that series 
without it.

I wrote a documentation update to reflect my understanding of 
initialization and realization so please check it out too.
https://patchew.org/QEMU/20250908-qdev-v1-1-df236f7ce5bd@rsg.ci.i.u-tokyo.ac.jp/

Regards,
Akihiko Odaki

Re: [PATCH 07/22] hw/pci-host/raven: Fix AddressSpace exposure timing

[Peter Maydell]

On Mon, 8 Sept 2025 at 16:21, Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> On 2025/09/06 18:03, BALATON Zoltan wrote:
> > I had a series here: https://patchew.org/QEMU/
> > cover.1751493467.git.balaton@eik.bme.hu/
> > that changes this for raven (among other clean ups) but I could not get
> > that merged in the last devel cycle because of PPC being a bit
> > unmaintained. I'd prefer that series to be taken first instead of this
> > patch so I don't have to rebase that.
>
> Looking at the series, "[PATCH v2 13/14] hw/pci-host/raven: Do not map
> regions in init method" moves memory_region_init() and
> memory_region_init_io() from raven_pcihost_initfn() to
> raven_pcihost_realize(). This should be avoided because these function
> calls add memory regions as child properties, which may be introspected
> without realization. Perhaps you may drop the patch and rebase your
> series on top of this patch, or let me rebase this patch on that series
> without it.

I think that patch is fine. It's OK to init MRs either in
instance_init or in realize, whichever is convenient for the
device. Initing an MR adds a child QOM property but they are
not intended for generic introspection. A MemoryRegion is not
part of the public interface to a device unless you call
sysbus_init_mmio() to declare it so.

This is part of a general issue we have with QOM, where we
use QOM properties both for "this is part of the public-facing
interface to this device" and also for "this is internal and
really we're only using a QOM child property for convenience
of having things automatically cleaned up".

thanks
-- PMM

Re: [PATCH 04/22] target/mips: Fix AddressSpace exposure timing

[Thomas Huth]

On 06/09/2025 04.11, Akihiko Odaki wrote:
> mips-cpu is not hotpluggable but its instance can still be created and
> finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   target/mips/cpu.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/target/mips/cpu.c b/target/mips/cpu.c
> index 1f6c41fd3401e88637c2c0d8fe3fcf4a38370288..e166b570984a40fba2ef417799a267eac9bff7d3 100644
> --- a/target/mips/cpu.c
> +++ b/target/mips/cpu.c
> @@ -457,6 +457,13 @@ static void mips_cpu_realizefn(DeviceState *dev, Error **errp)
>       MIPSCPUClass *mcc = MIPS_CPU_GET_CLASS(dev);
>       Error *local_err = NULL;
>   
> +#ifndef CONFIG_USER_ONLY
> +    if (mcc->cpu_def->lcsr_cpucfg2 & (1 << CPUCFG2_LCSRP)) {
> +        address_space_init(&env->iocsr.as,
> +                            &env->iocsr.mr, "IOCSR");
> +    }
> +#endif
> +
>       if (!clock_get(cpu->clock)) {
>   #ifndef CONFIG_USER_ONLY
>           if (!qtest_enabled()) {
> @@ -505,8 +512,6 @@ static void mips_cpu_initfn(Object *obj)
>       if (mcc->cpu_def->lcsr_cpucfg2 & (1 << CPUCFG2_LCSRP)) {
>           memory_region_init_io(&env->iocsr.mr, OBJECT(cpu), NULL,
>                                   env, "iocsr", UINT64_MAX);
> -        address_space_init(&env->iocsr.as,
> -                            &env->iocsr.mr, "IOCSR");
>       }
>   #endif
>   }
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 05/22] target/xtensa: Fix AddressSpace exposure timing

[Thomas Huth]

On 06/09/2025 04.11, Akihiko Odaki wrote:
> xtensa-cpu is not hotpluggable but its instance can still be created and
> finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   target/xtensa/cpu.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/target/xtensa/cpu.c b/target/xtensa/cpu.c
> index ea9b6df3aa24178c8e6a88b02afda5db659199da..63edc3a5b2778c8379a30125481f65361655fe1c 100644
> --- a/target/xtensa/cpu.c
> +++ b/target/xtensa/cpu.c
> @@ -243,7 +243,11 @@ static void xtensa_cpu_realizefn(DeviceState *dev, Error **errp)
>       Error *local_err = NULL;
>   
>   #ifndef CONFIG_USER_ONLY
> -    xtensa_irq_init(&XTENSA_CPU(dev)->env);
> +    CPUXtensaState *env = &XTENSA_CPU(dev)->env;
> +
> +    env->address_space_er = g_malloc(sizeof(*env->address_space_er));
> +    address_space_init(env->address_space_er, env->system_er, "ER");
> +    xtensa_irq_init(env);
>   #endif
>   
>       cpu_exec_realizefn(cs, &local_err);
> @@ -268,11 +272,9 @@ static void xtensa_cpu_initfn(Object *obj)
>       env->config = xcc->config;
>   
>   #ifndef CONFIG_USER_ONLY
> -    env->address_space_er = g_malloc(sizeof(*env->address_space_er));
>       env->system_er = g_malloc(sizeof(*env->system_er));
>       memory_region_init_io(env->system_er, obj, NULL, env, "er",
>                             UINT64_C(0x100000000));
> -    address_space_init(env->address_space_er, env->system_er, "ER");
>   
>       cpu->clock = qdev_init_clock_in(DEVICE(obj), "clk-in", NULL, cpu, 0);
>       clock_set_hz(cpu->clock, env->config->clock_freq_khz * 1000);
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 06/22] auxbus: Fix AddressSpace exposure timing

[Thomas Huth]

On 06/09/2025 04.11, Akihiko Odaki wrote:
> aux-bus is not hotpluggable but its instance can still be created and
> finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/misc/auxbus.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/auxbus.c b/hw/misc/auxbus.c
> index 877f34560626f0ef741f00bb6c7272135d264399..c47db4da985d8e81f9eb49542279499c931aac6c 100644
> --- a/hw/misc/auxbus.c
> +++ b/hw/misc/auxbus.c
> @@ -74,12 +74,12 @@ AUXBus *aux_bus_init(DeviceState *parent, const char *name)
>       /* Memory related. */
>       bus->aux_io = g_malloc(sizeof(*bus->aux_io));
>       memory_region_init(bus->aux_io, OBJECT(bus), "aux-io", 1 * MiB);
> -    address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
>       return bus;
>   }
>   
>   void aux_bus_realize(AUXBus *bus)
>   {
> +    address_space_init(&bus->aux_addr_space, bus->aux_io, "aux-io");
>       qdev_realize(DEVICE(bus->bridge), BUS(bus), &error_fatal);
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 08/22] sun4m: Fix AddressSpace exposure timing

[Thomas Huth]

On 06/09/2025 04.11, Akihiko Odaki wrote:
> sun4m-iommu is not hotpluggable but its instance can still be created
> and finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/sparc/sun4m_iommu.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc/sun4m_iommu.c b/hw/sparc/sun4m_iommu.c
> index a7ff36ee78c1d6295efea6499dffc2a481022167..0997f29ccb97d3dec4e3d34db49f2e51b6807a1a 100644
> --- a/hw/sparc/sun4m_iommu.c
> +++ b/hw/sparc/sun4m_iommu.c
> @@ -359,7 +359,6 @@ static void iommu_init(Object *obj)
>       memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
>                                TYPE_SUN4M_IOMMU_MEMORY_REGION, OBJECT(dev),
>                                "iommu-sun4m", UINT64_MAX);
> -    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
>   
>       sysbus_init_irq(dev, &s->irq);
>   
> @@ -368,6 +367,13 @@ static void iommu_init(Object *obj)
>       sysbus_init_mmio(dev, &s->iomem);
>   }
>   
> +static void iommu_realize(DeviceState *dev, Error **errp)
> +{
> +    IOMMUState *s = SUN4M_IOMMU(dev);
> +
> +    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
> +}
> +
>   static const Property iommu_properties[] = {
>       DEFINE_PROP_UINT32("version", IOMMUState, version, 0),
>   };
> @@ -377,6 +383,7 @@ static void iommu_class_init(ObjectClass *klass, const void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       device_class_set_legacy_reset(dc, iommu_reset);
> +    dc->realize = iommu_realize;
>       dc->vmsd = &vmstate_iommu;
>       device_class_set_props(dc, iommu_properties);
>   }

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 09/22] sun4u: Fix AddressSpace exposure timing

[Thomas Huth]

On 06/09/2025 04.11, Akihiko Odaki wrote:
> sun4u-iommu is not hotpluggable but its instance can still be created
> and destroyed when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/sparc64/sun4u_iommu.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc64/sun4u_iommu.c b/hw/sparc64/sun4u_iommu.c
> index 14645f475a09ed3a6bef77f8d8b4b6b4b36ae40a..b6568551935610116d33481ae8d9fc08f02ecf7b 100644
> --- a/hw/sparc64/sun4u_iommu.c
> +++ b/hw/sparc64/sun4u_iommu.c
> @@ -298,18 +298,25 @@ static void iommu_init(Object *obj)
>       memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
>                                TYPE_SUN4U_IOMMU_MEMORY_REGION, OBJECT(s),
>                                "iommu-sun4u", UINT64_MAX);
> -    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
>   
>       memory_region_init_io(&s->iomem, obj, &iommu_mem_ops, s, "iommu",
>                             IOMMU_NREGS * sizeof(uint64_t));
>       sysbus_init_mmio(sbd, &s->iomem);
>   }
>   
> +static void iommu_realize(DeviceState *dev, Error **errp)
> +{
> +    IOMMUState *s = SUN4U_IOMMU(dev);
> +
> +    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
> +}
> +
>   static void iommu_class_init(ObjectClass *klass, const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       device_class_set_legacy_reset(dc, iommu_reset);
> +    dc->realize = iommu_realize;
>   }
>   
>   static const TypeInfo iommu_info = {

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 08/22] sun4m: Fix AddressSpace exposure timing

[Mark Cave-Ayland]

On 06/09/2025 03:11, Akihiko Odaki wrote:

> sun4m-iommu is not hotpluggable but its instance can still be created
> and finalized when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/sparc/sun4m_iommu.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc/sun4m_iommu.c b/hw/sparc/sun4m_iommu.c
> index a7ff36ee78c1d6295efea6499dffc2a481022167..0997f29ccb97d3dec4e3d34db49f2e51b6807a1a 100644
> --- a/hw/sparc/sun4m_iommu.c
> +++ b/hw/sparc/sun4m_iommu.c
> @@ -359,7 +359,6 @@ static void iommu_init(Object *obj)
>       memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
>                                TYPE_SUN4M_IOMMU_MEMORY_REGION, OBJECT(dev),
>                                "iommu-sun4m", UINT64_MAX);
> -    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
>   
>       sysbus_init_irq(dev, &s->irq);
>   
> @@ -368,6 +367,13 @@ static void iommu_init(Object *obj)
>       sysbus_init_mmio(dev, &s->iomem);
>   }
>   
> +static void iommu_realize(DeviceState *dev, Error **errp)
> +{
> +    IOMMUState *s = SUN4M_IOMMU(dev);
> +
> +    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
> +}
> +
>   static const Property iommu_properties[] = {
>       DEFINE_PROP_UINT32("version", IOMMUState, version, 0),
>   };
> @@ -377,6 +383,7 @@ static void iommu_class_init(ObjectClass *klass, const void *data)
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       device_class_set_legacy_reset(dc, iommu_reset);
> +    dc->realize = iommu_realize;
>       dc->vmsd = &vmstate_iommu;
>       device_class_set_props(dc, iommu_properties);
>   }

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH 09/22] sun4u: Fix AddressSpace exposure timing

[Mark Cave-Ayland]

On 06/09/2025 03:11, Akihiko Odaki wrote:

> sun4u-iommu is not hotpluggable but its instance can still be created
> and destroyed when processing the device-list-properties QMP command.
> Exposing such a temporary instance to AddressSpace should be
> avoided because it leaks the instance.
> 
> Expose instances to the AddressSpace at their realization time so that
> it won't happen for the temporary instances.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>   hw/sparc64/sun4u_iommu.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sparc64/sun4u_iommu.c b/hw/sparc64/sun4u_iommu.c
> index 14645f475a09ed3a6bef77f8d8b4b6b4b36ae40a..b6568551935610116d33481ae8d9fc08f02ecf7b 100644
> --- a/hw/sparc64/sun4u_iommu.c
> +++ b/hw/sparc64/sun4u_iommu.c
> @@ -298,18 +298,25 @@ static void iommu_init(Object *obj)
>       memory_region_init_iommu(&s->iommu, sizeof(s->iommu),
>                                TYPE_SUN4U_IOMMU_MEMORY_REGION, OBJECT(s),
>                                "iommu-sun4u", UINT64_MAX);
> -    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
>   
>       memory_region_init_io(&s->iomem, obj, &iommu_mem_ops, s, "iommu",
>                             IOMMU_NREGS * sizeof(uint64_t));
>       sysbus_init_mmio(sbd, &s->iomem);
>   }
>   
> +static void iommu_realize(DeviceState *dev, Error **errp)
> +{
> +    IOMMUState *s = SUN4U_IOMMU(dev);
> +
> +    address_space_init(&s->iommu_as, MEMORY_REGION(&s->iommu), "iommu-as");
> +}
> +
>   static void iommu_class_init(ObjectClass *klass, const void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
>   
>       device_class_set_legacy_reset(dc, iommu_reset);
> +    dc->realize = iommu_realize;
>   }
>   
>   static const TypeInfo iommu_info = {

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

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

[Peter Xu]

On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> Children are automatically unparented so manually unparenting is
> unnecessary.
> 
> Worse, automatic unparenting happens before the insntance_finalize()
> callback of the parent gets called, so object_unparent() calls in
> the callback will refer to objects that are already unparented, which
> is semantically incorrect.
> 
> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> ---
>  hw/vfio/pci.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>          vfio_region_finalize(&bar->region);
>          if (bar->mr) {
>              assert(bar->size);
> -            object_unparent(OBJECT(bar->mr));
>              g_free(bar->mr);
>              bar->mr = NULL;
>          }
> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>  
>      if (vdev->vga) {
>          vfio_vga_quirk_finalize(vdev);
> -        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
> -            object_unparent(OBJECT(&vdev->vga->region[i].mem));
> -        }
>          g_free(vdev->vga);
>      }
>  }

So the 2nd object_unparent() here should be no-op, seeing empty list of
properties (but shouldn't causing anything severe), is that correct?

I think it still makes some sense to theoretically allow object_unparent()
to happen, at least when it happens before owner's finalize().  IIUC that
was the intention of the doc, pairing the memory_region_init*() operation.

It might depend on two use cases:

1. MRs dynamically created, it'll share the same lifecycle of the owner
   after creation (just like an embeded MemoryRegion)

   I feel like most, if not all, VFIO's dynamic mrs follows this trend,
   that this patch touched.

   In this case, IMHO instead of object_unparent(), we could also allow the
   owner / device to take ownership of the MR completely, by replacing:

       mr = g_new0(MemoryRegion, 1);

   with:

       mr = object_new(TYPE_MEMORY_REGION);

   Then after memory_region_init*(), essentially the owner will be in
   charge of the memory, as it'll be g_free()ed when remove the mr from
   property list (in owner's finalize() automatically).

   With that, the device impl can not only avoid object_unparent(), but
   avoid g_free() altogether.  That would make some more sense to me,
   instead of relying on memory internal to unparent, and rely on device
   impl to g_free().

2. MRs dynamically created, and it may be freed even before the owner
   finishes its lifecycle

   This is the case that I _think_ an object_unparent() should still be
   allowed, because when the mr is unparented (aka, not used), we should
   still provide a way for the device impl to detach and free the mr
   resources on the fly.

There're a bunch of object_unparent() users, I didn't check whether there's
any real user of case (2), though.

AFAIU for such case maybe it's always better to provide real refcounting
for the mr, since the mr can always be address_space_map()ed.. with an
elevated refcount. In that case, the owner shouldn't be the device impl,
but a temp obj that represents the mr (and do refcounts).

Thanks,

-- 
Peter Xu

Re: [PATCH 10/22] qdev: Automatically delete memory subregions

[Peter Xu]

On Sat, Sep 06, 2025 at 04:11:19AM +0200, Akihiko Odaki wrote:
> 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.

I don't think they're the same?  What is the ultimate goal of this change?

PCI core only detachs all the BARs from the address space registered from
pci_register_bar() explicitly.  It's not an object_dynamic_cast() detaching
every MR not matter what it is..

The other thing it does is detaching the DMA root memory region.

I'm not fluent with pci, but IMHO it's good to keep those explicit.

> 
> 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>
> ---
>  MAINTAINERS            |  1 +
>  include/hw/qdev-core.h |  1 +
>  hw/core/qdev.c         | 14 ++++++++++++++
>  stubs/memory.c         |  9 +++++++++
>  stubs/meson.build      |  1 +
>  5 files changed, 26 insertions(+)
> 
> 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/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index 530f3da70218df59da72dc7a975dca8265600e00..8f443d5f8ea5f31d69181cc1ec53a0b022eb71cc 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -526,6 +526,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) {
>          /*
> 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/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
> 

-- 
Peter Xu

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

[Akihiko Odaki]

On 2025/09/11 5:41, Peter Xu wrote:
> On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
>> Children are automatically unparented so manually unparenting is
>> unnecessary.
>>
>> Worse, automatic unparenting happens before the insntance_finalize()
>> callback of the parent gets called, so object_unparent() calls in
>> the callback will refer to objects that are already unparented, which
>> is semantically incorrect.
>>
>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>> ---
>>   hw/vfio/pci.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>           vfio_region_finalize(&bar->region);
>>           if (bar->mr) {
>>               assert(bar->size);
>> -            object_unparent(OBJECT(bar->mr));
>>               g_free(bar->mr);
>>               bar->mr = NULL;
>>           }
>> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>   
>>       if (vdev->vga) {
>>           vfio_vga_quirk_finalize(vdev);
>> -        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
>> -            object_unparent(OBJECT(&vdev->vga->region[i].mem));
>> -        }
>>           g_free(vdev->vga);
>>       }
>>   }
> 
> So the 2nd object_unparent() here should be no-op, seeing empty list of
> properties (but shouldn't causing anything severe), is that correct?

No. The object is finalized with the first object_unparent() if there is 
no referrer other than the parent. The second object_unparent() will 
access the finalized, invalid object in that case.

> 
> I think it still makes some sense to theoretically allow object_unparent()
> to happen, at least when it happens before owner's finalize().  IIUC that
> was the intention of the doc, pairing the memory_region_init*() operation.

Perhaps so, but this patch is only about the case where 
object_unparent() is called in finalize().

Regards,
Akihiko Odaki

Re: [PATCH 10/22] qdev: Automatically delete memory subregions

[Akihiko Odaki]

On 2025/09/11 6:10, Peter Xu wrote:
> On Sat, Sep 06, 2025 at 04:11:19AM +0200, Akihiko Odaki wrote:
>> 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.
> 
> I don't think they're the same?  What is the ultimate goal of this change?

Covering all devices in the common code is less tedious and error-prone 
because the same logic will not be duplicated by the subclasses.

> 
> PCI core only detachs all the BARs from the address space registered from
> pci_register_bar() explicitly.  It's not an object_dynamic_cast() detaching
> every MR not matter what it is..

All devices share one semantics: they are exposed to the guest only when 
they are realized. Therefore, every MRs should be detached after 
unrealization, no matter what it is.

> 
> The other thing it does is detaching the DMA root memory region.
> 
> I'm not fluent with pci, but IMHO it's good to keep those explicit.

Explicit detaching is "tedious and error-prone"; more concretely, there 
are the following two error situations:
1. Forget the detachment and cause a memory leak.
2. Perform the detachment after finalization, which could have happened 
with a manual detachment in "[PATCH 11/22] vfio-user: Do not delete the 
subregion".

Regards,
Akihiko Odaki

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

[Peter Xu]

On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
> On 2025/09/11 5:41, Peter Xu wrote:
> > On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
> > > Children are automatically unparented so manually unparenting is
> > > unnecessary.
> > > 
> > > Worse, automatic unparenting happens before the insntance_finalize()
> > > callback of the parent gets called, so object_unparent() calls in
> > > the callback will refer to objects that are already unparented, which
> > > is semantically incorrect.
> > > 
> > > Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
> > > ---
> > >   hw/vfio/pci.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > > index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
> > > --- a/hw/vfio/pci.c
> > > +++ b/hw/vfio/pci.c
> > > @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >           vfio_region_finalize(&bar->region);
> > >           if (bar->mr) {
> > >               assert(bar->size);
> > > -            object_unparent(OBJECT(bar->mr));
> > >               g_free(bar->mr);
> > >               bar->mr = NULL;
> > >           }
> > > @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
> > >       if (vdev->vga) {
> > >           vfio_vga_quirk_finalize(vdev);
> > > -        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
> > > -            object_unparent(OBJECT(&vdev->vga->region[i].mem));
> > > -        }
> > >           g_free(vdev->vga);
> > >       }
> > >   }
> > 
> > So the 2nd object_unparent() here should be no-op, seeing empty list of
> > properties (but shouldn't causing anything severe), is that correct?
> 
> No. The object is finalized with the first object_unparent() if there is no
> referrer other than the parent. The second object_unparent() will access the
> finalized, invalid object in that case.

Yes, it's logically wrong.  I was trying to understand the impact when it's
invoked.  In this specific case of above two changes, I believe both MR
structs are still available, so it does look fine, e.g. nothing would crash.

For example, I think it doesn't need to copy stable if there's no real
functional issue involved.

> 
> > 
> > I think it still makes some sense to theoretically allow object_unparent()
> > to happen, at least when it happens before owner's finalize().  IIUC that
> > was the intention of the doc, pairing the memory_region_init*() operation.
> 
> Perhaps so, but this patch is only about the case where object_unparent() is
> called in finalize().

You ignored my other comment.  That (using object_new() on MRs) was what I
was thinking might be better than a split model you discussed here, so
that's also a comment for patch 1 of your series.

Btw, this patch also didn't change all occurances of such for VFIO?
E.g. there's at least vfio_vga_quirk_finalize().  I didn't check the rest.

-- 
Peter Xu

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

[Akihiko Odaki]

On 2025/09/12 6:37, Peter Xu wrote:
> On Thu, Sep 11, 2025 at 12:47:24PM +0900, Akihiko Odaki wrote:
>> On 2025/09/11 5:41, Peter Xu wrote:
>>> On Sat, Sep 06, 2025 at 04:11:11AM +0200, Akihiko Odaki wrote:
>>>> Children are automatically unparented so manually unparenting is
>>>> unnecessary.
>>>>
>>>> Worse, automatic unparenting happens before the insntance_finalize()
>>>> callback of the parent gets called, so object_unparent() calls in
>>>> the callback will refer to objects that are already unparented, which
>>>> is semantically incorrect.
>>>>
>>>> Signed-off-by: Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>
>>>> ---
>>>>    hw/vfio/pci.c | 4 ----
>>>>    1 file changed, 4 deletions(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>>> index 07257d0fa049b09fc296ac2279a6fafbdf93d277..2e909c190f86a722e1022fa7c45a96d2dde8d58e 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -2000,7 +2000,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>>>            vfio_region_finalize(&bar->region);
>>>>            if (bar->mr) {
>>>>                assert(bar->size);
>>>> -            object_unparent(OBJECT(bar->mr));
>>>>                g_free(bar->mr);
>>>>                bar->mr = NULL;
>>>>            }
>>>> @@ -2008,9 +2007,6 @@ static void vfio_bars_finalize(VFIOPCIDevice *vdev)
>>>>        if (vdev->vga) {
>>>>            vfio_vga_quirk_finalize(vdev);
>>>> -        for (i = 0; i < ARRAY_SIZE(vdev->vga->region); i++) {
>>>> -            object_unparent(OBJECT(&vdev->vga->region[i].mem));
>>>> -        }
>>>>            g_free(vdev->vga);
>>>>        }
>>>>    }
>>>
>>> So the 2nd object_unparent() here should be no-op, seeing empty list of
>>> properties (but shouldn't causing anything severe), is that correct?
>>
>> No. The object is finalized with the first object_unparent() if there is no
>> referrer other than the parent. The second object_unparent() will access the
>> finalized, invalid object in that case.
> 
> Yes, it's logically wrong.  I was trying to understand the impact when it's
> invoked.  In this specific case of above two changes, I believe both MR
> structs are still available, so it does look fine, e.g. nothing would crash.
> 
> For example, I think it doesn't need to copy stable if there's no real
> functional issue involved.

You are right. Cc: stable is unnecessary.

> 
>>
>>>
>>> I think it still makes some sense to theoretically allow object_unparent()
>>> to happen, at least when it happens before owner's finalize().  IIUC that
>>> was the intention of the doc, pairing the memory_region_init*() operation.
>>
>> Perhaps so, but this patch is only about the case where object_unparent() is
>> called in finalize().
> 
> You ignored my other comment.  That (using object_new() on MRs) was what I
> was thinking might be better than a split model you discussed here, so
> that's also a comment for patch 1 of your series.

I'm not sure what you mean by the "split model".

This change removes object_unparent() in vfio_bars_finalize(). 
object_new() will allow removing even g_free(), but we can do these 
changes incrementally:
1. remove object_unparent() in finalize(),
    which fixes a semantic problem (this patch)
2. allow object_new() for MRs and remove g_free() in finalize()
    as a refactoring.

So I suggest focusing on object_unparent() in finalize() to keep this 
patch and review concise.

> 
> Btw, this patch also didn't change all occurances of such for VFIO?
> E.g. there's at least vfio_vga_quirk_finalize().  I didn't check the rest.
> 

Indeed. I only removed object_unparent() calls hw/vfio/pci.c because it 
was mentioned in the documentation. I suspect you will find more cases 
that subregions are used in instance_finalize() in general if you check 
the code base; "[PATCH 11/22] vfio-user: Do not delete the subregion" 
also removes memory_region_del_subregion() during finalization, for example.

Regards,
Akihiko Odaki

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

[Peter Xu]

On Fri, Sep 12, 2025 at 11:09:51AM +0900, Akihiko Odaki wrote:
> > > > I think it still makes some sense to theoretically allow object_unparent()
> > > > to happen, at least when it happens before owner's finalize().  IIUC that
> > > > was the intention of the doc, pairing the memory_region_init*() operation.
> > > 
> > > Perhaps so, but this patch is only about the case where object_unparent() is
> > > called in finalize().
> > 
> > You ignored my other comment.  That (using object_new() on MRs) was what I
> > was thinking might be better than a split model you discussed here, so
> > that's also a comment for patch 1 of your series.
> 
> I'm not sure what you mean by the "split model".

I meant after similar change as what this patch proposes, (a) "owner of the
MR lifecycle" (aka, who decides to finalize() the MR) is not the same as
(b) "owner of the memory" (aka, who decides to free() the memory backing
the MR struct), so the ownership model itself is more or less "split".

Now it's very hard to tell who owns the MR, because each owns only part of
it.

IMHO it'll be slightly better to have the instance lifecycle and the memory
allocation of the MR struct be managed by the same object, no matter
automatically by the memory core, or manually by the device (in the case of
the current doc, it went with the latter, even though I agree with you it
looks wrong).

> 
> This change removes object_unparent() in vfio_bars_finalize(). object_new()
> will allow removing even g_free(), but we can do these changes
> incrementally:
> 1. remove object_unparent() in finalize(),
>    which fixes a semantic problem (this patch)
> 2. allow object_new() for MRs and remove g_free() in finalize()
>    as a refactoring.
> 
> So I suggest focusing on object_unparent() in finalize() to keep this patch
> and review concise.

I agree that is the minimal change, but this is a common pattern.  It's not
high risk, so I think we could still discuss it thoroughly.

I further analyzed the risk here, it turns out object_unparent() in
finalize() is still very safe so the current code is actually bug-free if
it all works similarly like the vfio code: The object_property_del_all()
(on top of the owner device) would do prop->release(), and here MR will
kickoff object_finalize_child_property(), which resets mr->parent to NULL.

So the 2nd object_unparent() will already see obj->parent==NULL.

Directly dropping object_unparent() should work, but leads to confusion as
"split ownership model" as I discussed above.

Thanks to all recent discussions, IMHO we have much clearer picture of how
MRs can be used.  I discussed it almostly in the first reply:

https://lore.kernel.org/all/aMHidDl1tdx-2G4e@x1.local/

I suspect we don't really have 2nd user I mentioned, because if so it'll
likely require strict mr refcounting due to address_space_map(), in which
case we should go the "create a temp obj as the owner of MR" idea, that you
used to fix the virgl issue in patch 2 of your other series.

For the current issue, I'd suggest as simple as below (I observed at least
the current VFIO use case only uses MMIO memory regions, so we only need
one such helper):

/*
 * Unlike memory_region_init_io(), @memory_region_alloc_io allocates an IO
 * memory region object and returns.
 *
 * After the function returns, the MemoryRegion object will share the same
 * lifecycle of the owner object.  If owner is not specified, the MR will
 * never be released.
 *
 * The caller doesn't need to either detach or unref/free the MR object.
 * It will be automatically detached and returned when the owner finalize.
 * The caller can cache the MR object pointer, but it's only valid to
 * operate before the owner finalizes.
 */
MemoryRegion *
memory_region_alloc_io(MemoryRegion *mr,
                       Object *owner,
                       const MemoryRegionOps *ops,
                       void *opaque,
                       const char *name,
                       uint64_t size)
{
    MemoryRegion = object_new(TYPE_MEMORY_REGION);
    memory_region_do_init(mr, owner, name, size);
    mr->ops = ops ? ops : &unassigned_mem_ops;
    mr->opaque = opaque;
    mr->terminates = true;
}

Here, IIUC if we can allocate the MR using memory_region_alloc_io() here,
then the ownership of both (a)+(b) above will be done automatically by the
memory core / object core code.  The device impl doesn't need to care about
either removal of subregions, or free of MR struct, anymore.  Then we can
drop not only the object_unparent(), but also g_free(), altogether.

Would that sound like a better approach in general?

Again, I don't think this is anything urgent, so we can take time to think
it through.

Thanks,

-- 
Peter Xu

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

[Akihiko Odaki]

On 2025/09/13 6:26, Peter Xu wrote:
> On Fri, Sep 12, 2025 at 11:09:51AM +0900, Akihiko Odaki wrote:
>>>>> I think it still makes some sense to theoretically allow object_unparent()
>>>>> to happen, at least when it happens before owner's finalize().  IIUC that
>>>>> was the intention of the doc, pairing the memory_region_init*() operation.
>>>>
>>>> Perhaps so, but this patch is only about the case where object_unparent() is
>>>> called in finalize().
>>>
>>> You ignored my other comment.  That (using object_new() on MRs) was what I
>>> was thinking might be better than a split model you discussed here, so
>>> that's also a comment for patch 1 of your series.
>>
>> I'm not sure what you mean by the "split model".
> 
> I meant after similar change as what this patch proposes, (a) "owner of the
> MR lifecycle" (aka, who decides to finalize() the MR) is not the same as
> (b) "owner of the memory" (aka, who decides to free() the memory backing
> the MR struct), so the ownership model itself is more or less "split".
> 
> Now it's very hard to tell who owns the MR, because each owns only part of
> it.
> 
> IMHO it'll be slightly better to have the instance lifecycle and the memory
> allocation of the MR struct be managed by the same object, no matter
> automatically by the memory core, or manually by the device (in the case of
> the current doc, it went with the latter, even though I agree with you it
> looks wrong).

The instance lifecycle and the memory allocation is managed by the same 
object (i.e., the owner). When the owner is being finalized, the owner 
performs the following operations in order in object_finalize():

1. Unparent all children.
2. Call the instance_finalize() callback.

We can say the timing is "split", but the split timing exists even 
without this patch; unparenting happens before instance_finalize(), 
which calls g_free(), with or without this patch. Fixing the split 
timing can be done later.

> 
>>
>> This change removes object_unparent() in vfio_bars_finalize(). object_new()
>> will allow removing even g_free(), but we can do these changes
>> incrementally:
>> 1. remove object_unparent() in finalize(),
>>     which fixes a semantic problem (this patch)
>> 2. allow object_new() for MRs and remove g_free() in finalize()
>>     as a refactoring.
>>
>> So I suggest focusing on object_unparent() in finalize() to keep this patch
>> and review concise.
> 
> I agree that is the minimal change, but this is a common pattern.  It's not
> high risk, so I think we could still discuss it thoroughly.
> 
> I further analyzed the risk here, it turns out object_unparent() in
> finalize() is still very safe so the current code is actually bug-free if
> it all works similarly like the vfio code: The object_property_del_all()
> (on top of the owner device) would do prop->release(), and here MR will
> kickoff object_finalize_child_property(), which resets mr->parent to NULL.
> 
> So the 2nd object_unparent() will already see obj->parent==NULL.
> 
> Directly dropping object_unparent() should work, but leads to confusion as
> "split ownership model" as I discussed above.
> 
> Thanks to all recent discussions, IMHO we have much clearer picture of how
> MRs can be used.  I discussed it almostly in the first reply:
> 
> https://lore.kernel.org/all/aMHidDl1tdx-2G4e@x1.local/
> 
> I suspect we don't really have 2nd user I mentioned, because if so it'll
> likely require strict mr refcounting due to address_space_map(), in which
> case we should go the "create a temp obj as the owner of MR" idea, that you
> used to fix the virgl issue in patch 2 of your other series.
> 
> For the current issue, I'd suggest as simple as below (I observed at least
> the current VFIO use case only uses MMIO memory regions, so we only need
> one such helper):
> 
> /*
>   * Unlike memory_region_init_io(), @memory_region_alloc_io allocates an IO
>   * memory region object and returns.
>   *
>   * After the function returns, the MemoryRegion object will share the same
>   * lifecycle of the owner object.  If owner is not specified, the MR will
>   * never be released.
>   *
>   * The caller doesn't need to either detach or unref/free the MR object.
>   * It will be automatically detached and returned when the owner finalize.
>   * The caller can cache the MR object pointer, but it's only valid to
>   * operate before the owner finalizes.
>   */
> MemoryRegion *
> memory_region_alloc_io(MemoryRegion *mr,
>                         Object *owner,
>                         const MemoryRegionOps *ops,
>                         void *opaque,
>                         const char *name,
>                         uint64_t size)
> {
>      MemoryRegion = object_new(TYPE_MEMORY_REGION);
>      memory_region_do_init(mr, owner, name, size);
>      mr->ops = ops ? ops : &unassigned_mem_ops;
>      mr->opaque = opaque;
>      mr->terminates = true;
> }
> 
> Here, IIUC if we can allocate the MR using memory_region_alloc_io() here,
> then the ownership of both (a)+(b) above will be done automatically by the
> memory core / object core code.  The device impl doesn't need to care about
> either removal of subregions, or free of MR struct, anymore.  Then we can
> drop not only the object_unparent(), but also g_free(), altogether.
> 
> Would that sound like a better approach in general?
> 
> Again, I don't think this is anything urgent, so we can take time to think
> it through.
It makes sense to have a through review, but my argument here is the 
de-duplication of object_unparent() and the replacement of g_free() with 
object_new() are logically distinct and should be split into distinct 
patches. Each patch can independently have through review, be 
applied/backported, or be reverted in case of regression.

Regards,
Akihiko Odaki

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

[Peter Xu]

On Sun, Sep 14, 2025 at 06:06:44PM +0900, Akihiko Odaki wrote:
> It makes sense to have a through review, but my argument here is the
> de-duplication of object_unparent() and the replacement of g_free() with
> object_new() are logically distinct and should be split into distinct
> patches. Each patch can independently have through review, be
> applied/backported, or be reverted in case of regression.

We're discussing a change in the memory.rst on suggested way to use dynamic
MRs, so I think we can do it in one shot rather than making it confusing.
It's not a huge change even in one go.

It's fine.  You're right we can remove the object_unparent() first when
it's always a no-op.  We'll update the doc twice, though I assume it's fine.

If you would, please consider sending this part as a separate series.

The subject should be something like "remove unnecessary object_unparent()
for dynamic MRs" or something like that.  It has nothing to do with
memleaks on this part.

Please cover tests as much as possible and if we touch the doc we need to
convert everything that uses dynamic MRs, including the missing ones in
VFIO, and also the rest occurances.

Thanks,

-- 
Peter Xu

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

[Akihiko Odaki]

On 2025/09/16 5:30, Peter Xu wrote:
> On Sun, Sep 14, 2025 at 06:06:44PM +0900, Akihiko Odaki wrote:
>> It makes sense to have a through review, but my argument here is the
>> de-duplication of object_unparent() and the replacement of g_free() with
>> object_new() are logically distinct and should be split into distinct
>> patches. Each patch can independently have through review, be
>> applied/backported, or be reverted in case of regression.
> 
> We're discussing a change in the memory.rst on suggested way to use dynamic
> MRs, so I think we can do it in one shot rather than making it confusing.
> It's not a huge change even in one go.
 > > It's fine.  You're right we can remove the object_unparent() first when
> it's always a no-op.  We'll update the doc twice, though I assume it's fine.
> 
> If you would, please consider sending this part as a separate series.
> 
> The subject should be something like "remove unnecessary object_unparent()
> for dynamic MRs" or something like that.  It has nothing to do with
> memleaks on this part.

I think "Do not unparent in instance_finalize()" is good enough.

"unnecessary" sounds it is correct but only extraneous; in reality it is 
semantically problematic.

"In instance_finalize()" is more appropriate to represent the scope of 
the change than "for dynamic MRs". Unparenting objects when finalizing 
their parents is wrong, whether they are MRs or not. On the other hand, 
unparenting MRs before instance_finalize() is still allowed.

In v2, I dropped patches to fix memory leaks so the series only contains 
ones to fix use-after-finalization. I'll rename the series accordingly.

> 
> Please cover tests as much as possible and if we touch the doc we need to
> convert everything that uses dynamic MRs, including the missing ones in
> VFIO, and also the rest occurances.

I'll search for object_unparent() called from instance_finalize().

Regards,
Akihiko Odaki