[PATCH v8 0/2] Fix check-qtest-ppc64 sanitizer errors

[PATCH v8 0/2] Fix check-qtest-ppc64 sanitizer errors

[Akihiko Odaki]

I saw various sanitizer errors when running check-qtest-ppc64. While
I could just turn off sanitizers, I decided to tackle them this time.

Unfortunately, GLib versions older than 2.81.0 do not free test data in
some cases so some sanitizer errors remain. All sanitizer errors will be
gone with this patch series combined with the following change for GLib:
https://gitlab.gnome.org/GNOME/glib/-/merge_requests/4120

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v8:
- Clarified that the memory region should be passed to object_ref()
  when creating a reference internal to owner.
- Link to v7: https://lore.kernel.org/r/20250109-san-v7-0-93c432a73024@daynix.com

Changes in v7:
- Don't open code memory_region_ref(). (Peter Xu)
- Link to v6: https://lore.kernel.org/r/20250105-san-v6-0-11fc859b99b7@daynix.com

Changes in v6:
- Avoid referring owner as "the object that tracks the region's
  reference count".
- Noted that memroy_region_ref() and memroy_region_unref() do nothing
  if the owner is not present.
- Explicitly stated that memory_region_unref() may destroy the owner
  along with the memory region itself.
- Link to v5: https://lore.kernel.org/r/20250104-san-v5-0-8b430457b09d@daynix.com

Changes in v5:
- Rebased.
- Merged four patches to update inline documentation into one
- Link to v4: https://lore.kernel.org/r/20240823-san-v4-0-a24c6dfa4ceb@daynix.com

Changes in v4:
- Changed to create a reference to the subregion instead of its owner
  when its owner equals to the container's owner.
- Dropped R-b from patch "memory: Do not create circular reference with
  subregion".
- Rebased.
- Link to v3: https://lore.kernel.org/r/20240708-san-v3-0-b03f671c40c6@daynix.com

Changes in v3:
- Added patch "memory: Clarify that we use owner's reference count".
- Added patch "memory: Refer to docs/devel/memory.rst for 'owner'".
- Fixed the message of patch
  "memory: Do not create circular reference with subregion".
- Dropped patch "cpu: Free cpu_ases" in favor of:
  https://lore.kernel.org/r/20240607115649.214622-7-salil.mehta@huawei.com/
  ("[PATCH V13 6/8] physmem: Add helper function to destroy CPU
  AddressSpace")
- Dropped patches "hw/ide: Convert macio ide_irq into GPIO line" and
  "hw/ide: Remove internal DMA qemu_irq" in favor of commit efb359346c7a
  ("hw/ide/macio: switch from using qemu_allocate_irq() to qdev input
  GPIOs")
- Dropped patch "hw/isa/vt82c686: Define a GPIO line between vt82c686
  and i8259" in favor of:
  https://patchew.org/QEMU/20240704205854.18537-1-shentey@gmail.com/
  ("[PATCH 0/3] Resolve vt82c686 and piix4 qemu_irq memory leaks")
- Dropped pulled patches.
- Link to v2: https://lore.kernel.org/r/20240627-san-v2-0-750bb0946dbd@daynix.com

Changes in v2:
- Rebased to "[PATCH] cpu: fix memleak of 'halt_cond' and 'thread'".
  (Philippe Mathieu-Daudé)
- Converted IRQs into GPIO lines and removed one qemu_irq usage.
  (Peter Maydell)
- s/suppresses/fixes/ (Michael S. Tsirkin)
- Corrected title of patch "hw/virtio: Free vqs after vhost_dev_cleanup()"
  (was "hw/virtio: Free vqs before vhost_dev_cleanup()")
- Link to v1: https://lore.kernel.org/r/20240626-san-v1-0-f3cc42302189@daynix.com

---
Akihiko Odaki (2):
      memory: Update inline documentation
      memory: Do not create circular reference with subregion

 include/exec/memory.h | 59 ++++++++++++++++++++++++---------------------------
 system/memory.c       | 24 +++++++++++++++++++--
 2 files changed, 50 insertions(+), 33 deletions(-)
---
base-commit: 38d0939b86e2eef6f6a622c6f1f7befda0146595
change-id: 20240625-san-097afaf4f1c2

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

[PATCH v8 1/2] memory: Update inline documentation

[Akihiko Odaki]

Do not refer to "memory region's reference count"
-------------------------------------------------

Now MemoryRegions do have their own reference counts, but they will not
be used when their owners are not themselves. However, the documentation
of memory_region_ref() says it adds "1 to a memory region's reference
count", which is confusing. Avoid referring to "memory region's
reference count" and just say: "Add a reference to a memory region".
Make a similar change to memory_region_unref() too.

Refer to docs/devel/memory.rst for "owner"
------------------------------------------

memory_region_ref() and memory_region_unref() used to have their own
descriptions of "owner", but they are somewhat out-of-date and
misleading.

In particular, they say "whenever memory regions are accessed outside
the BQL, they need to be preserved against hot-unplug", but protecting
against hot-unplug is not mandatory if it is known that they will never
be hot-unplugged. They also say "MemoryRegions actually do not have
their own reference count", but they actually do. They just will not be
used unless their owners are not themselves.

Refer to docs/devel/memory.rst as the single source of truth instead of
maintaining duplicate descriptions of "owner".

Clarify that owner may be missing

---------------------------------
A memory region may not have an owner, and memory_region_ref() and
memory_region_unref() do nothing for such.

memory: Clarify owner must not call memory_region_ref()
--------------------------------------------------------

The owner must not call this function as it results in a circular
reference.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 include/exec/memory.h | 59 ++++++++++++++++++++++++---------------------------
 1 file changed, 28 insertions(+), 31 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 9458e2801d50..c3a1f106c9bf 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s);
  * memory_region_add_subregion() to add subregions.
  *
  * @mr: the #MemoryRegion to be initialized
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region; any subregions beyond this size will be clipped
  */
@@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr,
                         uint64_t size);
 
 /**
- * memory_region_ref: Add 1 to a memory region's reference count
+ * memory_region_ref: Add a reference to the owner of a memory region
  *
- * Whenever memory regions are accessed outside the BQL, they need to be
- * preserved against hot-unplug.  MemoryRegions actually do not have their
- * own reference count; they piggyback on a QOM object, their "owner".
- * This function adds a reference to the owner.
- *
- * All MemoryRegions must have an owner if they can disappear, even if the
- * device they belong to operates exclusively under the BQL.  This is because
- * the region could be returned at any time by memory_region_find, and this
- * is usually under guest control.
+ * This function adds a reference to the owner of a memory region to keep the
+ * memory region alive. It does nothing if the owner is not present as a memory
+ * region without owner will never die.
+ * For references internal to the owner, pass the memory region to object_ref()
+ * instead of using this function to avoid a circular reference.
+ * See docs/devel/memory.rst to know about owner.
  *
  * @mr: the #MemoryRegion
  */
 void memory_region_ref(MemoryRegion *mr);
 
 /**
- * memory_region_unref: Remove 1 to a memory region's reference count
+ * memory_region_unref: Remove a reference to the memory region of the owner
  *
- * Whenever memory regions are accessed outside the BQL, they need to be
- * preserved against hot-unplug.  MemoryRegions actually do not have their
- * own reference count; they piggyback on a QOM object, their "owner".
- * This function removes a reference to the owner and possibly destroys it.
+ * This function removes a reference to the owner of a memory region and
+ * possibly destroys the owner along with the memory region. It does nothing if
+ * the owner is not present.
+ * See docs/devel/memory.rst to know about owner.
  *
  * @mr: the #MemoryRegion
  */
@@ -1255,7 +1252,7 @@ void memory_region_unref(MemoryRegion *mr);
  * if @size is nonzero, subregions will be clipped to @size.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @ops: a structure containing read and write callbacks to be used when
  *       I/O is performed on the region.
  * @opaque: passed to the read and write callbacks of the @ops structure.
@@ -1275,7 +1272,7 @@ void memory_region_init_io(MemoryRegion *mr,
  *                                    directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1298,7 +1295,7 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
  *                                          modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1328,7 +1325,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
  *                                     canceled.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: used size of the region.
@@ -1357,7 +1354,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
  *                                    mmap-ed backend.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1390,7 +1387,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
  *                                  mmap-ed backend.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: the name of the region.
  * @size: size of the region.
  * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
@@ -1421,7 +1418,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
  *                              region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1449,7 +1446,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
  * skip_dump flag.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: the name of the region.
  * @size: size of the region.
  * @ptr: memory to be mapped; must contain at least @size bytes.
@@ -1469,7 +1466,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
  *                           part of another memory region.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: used for debugging; not visible to the user or ABI
  * @orig: the region to be referenced; @mr will be equivalent to
  *        @orig between @offset and @offset + @size - 1.
@@ -1495,7 +1492,7 @@ void memory_region_init_alias(MemoryRegion *mr,
  * of the caller.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1518,7 +1515,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
  * of the caller.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @ops: callbacks for write access handling (must not be NULL).
  * @opaque: passed to the read and write callbacks of the @ops structure.
  * @name: Region name, becomes part of RAMBlock name used in migration stream
@@ -1554,7 +1551,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
  * @_iommu_mr: the #IOMMUMemoryRegion to be initialized
  * @instance_size: the IOMMUMemoryRegion subclass instance size
  * @mrtypename: the type name of the #IOMMUMemoryRegion
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: used for debugging; not visible to the user or ABI
  * @size: size of the region.
  */
@@ -1570,7 +1567,7 @@ void memory_region_init_iommu(void *_iommu_mr,
  *                          region will modify memory directly.
  *
  * @mr: the #MemoryRegion to be initialized
- * @owner: the object that tracks the region's reference count (must be
+ * @owner: the object that keeps the region alive (must be
  *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
  * @name: name of the memory region
  * @size: size of the region in bytes
@@ -1616,7 +1613,7 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
  * If you pass a non-NULL non-device @owner then we will assert.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @name: Region name, becomes part of RAMBlock name used in migration stream
  *        must be unique within any device
  * @size: size of the region.
@@ -1647,7 +1644,7 @@ bool memory_region_init_rom(MemoryRegion *mr,
  * If you pass a non-NULL non-device @owner then we will assert.
  *
  * @mr: the #MemoryRegion to be initialized.
- * @owner: the object that tracks the region's reference count
+ * @owner: the object that keeps the region alive
  * @ops: callbacks for write access handling (must not be NULL).
  * @opaque: passed to the read and write callbacks of the @ops structure.
  * @name: Region name, becomes part of RAMBlock name used in migration stream

-- 
2.47.1

[PATCH v8 2/2] memory: Do not create circular reference with subregion

[Akihiko Odaki]

memory_region_update_container_subregions() used to call
memory_region_ref(), which creates a reference to the owner of the
subregion, on behalf of the owner of the container. This results in a
circular reference if the subregion and container have the same owner.

memory_region_ref() creates a reference to the owner instead of the
memory region to match the lifetime of the owner and memory region. We
do not need such a hack if the subregion and container have the same
owner because the owner will be alive as long as the container is.
Therefore, create a reference to the subregion itself instead ot its
owner in such a case; the reference to the subregion is still necessary
to ensure that the subregion gets finalized after the container.

Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 system/memory.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 78e17e0efa83..972617b0192d 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1855,6 +1855,24 @@ void memory_region_unref(MemoryRegion *mr)
     }
 }
 
+static void memory_region_ref_subregion(MemoryRegion *subregion)
+{
+    if (subregion->container->owner == subregion->owner) {
+        object_ref(OBJECT(subregion));
+    } else {
+        memory_region_ref(subregion);
+    }
+}
+
+static void memory_region_unref_subregion(MemoryRegion *subregion)
+{
+    if (subregion->container->owner == subregion->owner) {
+        object_unref(OBJECT(subregion));
+     } else {
+        memory_region_unref(subregion);
+     }
+}
+
 uint64_t memory_region_size(MemoryRegion *mr)
 {
     if (int128_eq(mr->size, int128_2_64())) {
@@ -2644,7 +2662,8 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
 
     memory_region_transaction_begin();
 
-    memory_region_ref(subregion);
+    memory_region_ref_subregion(subregion);
+
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2702,7 +2721,8 @@ void memory_region_del_subregion(MemoryRegion *mr,
         assert(alias->mapped_via_alias >= 0);
     }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_unref(subregion);
+    memory_region_unref_subregion(subregion);
+
     memory_region_update_pending |= mr->enabled && subregion->enabled;
     memory_region_transaction_commit();
 }

-- 
2.47.1

Re: [PATCH v8 1/2] memory: Update inline documentation

[BALATON Zoltan]

On Fri, 10 Jan 2025, Akihiko Odaki wrote:
> Do not refer to "memory region's reference count"
> -------------------------------------------------
>
> Now MemoryRegions do have their own reference counts, but they will not
> be used when their owners are not themselves. However, the documentation
> of memory_region_ref() says it adds "1 to a memory region's reference
> count", which is confusing. Avoid referring to "memory region's
> reference count" and just say: "Add a reference to a memory region".
> Make a similar change to memory_region_unref() too.
>
> Refer to docs/devel/memory.rst for "owner"
> ------------------------------------------
>
> memory_region_ref() and memory_region_unref() used to have their own
> descriptions of "owner", but they are somewhat out-of-date and
> misleading.
>
> In particular, they say "whenever memory regions are accessed outside
> the BQL, they need to be preserved against hot-unplug", but protecting
> against hot-unplug is not mandatory if it is known that they will never
> be hot-unplugged. They also say "MemoryRegions actually do not have
> their own reference count", but they actually do. They just will not be
> used unless their owners are not themselves.
>
> Refer to docs/devel/memory.rst as the single source of truth instead of
> maintaining duplicate descriptions of "owner".
>
> Clarify that owner may be missing
>
> ---------------------------------
> A memory region may not have an owner, and memory_region_ref() and
> memory_region_unref() do nothing for such.
>
> memory: Clarify owner must not call memory_region_ref()
> --------------------------------------------------------
>
> The owner must not call this function as it results in a circular
> reference.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
> include/exec/memory.h | 59 ++++++++++++++++++++++++---------------------------
> 1 file changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 9458e2801d50..c3a1f106c9bf 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1210,7 +1210,7 @@ void memory_region_section_free_copy(MemoryRegionSection *s);
>  * memory_region_add_subregion() to add subregions.
>  *
>  * @mr: the #MemoryRegion to be initialized
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: used for debugging; not visible to the user or ABI
>  * @size: size of the region; any subregions beyond this size will be clipped
>  */
> @@ -1220,29 +1220,26 @@ void memory_region_init(MemoryRegion *mr,
>                         uint64_t size);
>
> /**
> - * memory_region_ref: Add 1 to a memory region's reference count
> + * memory_region_ref: Add a reference to the owner of a memory region
>  *
> - * Whenever memory regions are accessed outside the BQL, they need to be
> - * preserved against hot-unplug.  MemoryRegions actually do not have their
> - * own reference count; they piggyback on a QOM object, their "owner".
> - * This function adds a reference to the owner.
> - *
> - * All MemoryRegions must have an owner if they can disappear, even if the
> - * device they belong to operates exclusively under the BQL.  This is because
> - * the region could be returned at any time by memory_region_find, and this
> - * is usually under guest control.
> + * This function adds a reference to the owner of a memory region to keep the
> + * memory region alive. It does nothing if the owner is not present as a memory
> + * region without owner will never die.
> + * For references internal to the owner, pass the memory region to object_ref()
> + * instead of using this function to avoid a circular reference.

Thanks, I think this is clear now.

> + * See docs/devel/memory.rst to know about owner.

Minor nit: I think 'to know more about the owner' or 'learn more about the 
owner' or 'See docs/devel/memory.rst for further information' might be 
better but if native speakers think it's OK as it is then this might not 
worth another respin.

Regards,
BALATON Zoltan

>  *
>  * @mr: the #MemoryRegion
>  */
> void memory_region_ref(MemoryRegion *mr);
>
> /**
> - * memory_region_unref: Remove 1 to a memory region's reference count
> + * memory_region_unref: Remove a reference to the memory region of the owner
>  *
> - * Whenever memory regions are accessed outside the BQL, they need to be
> - * preserved against hot-unplug.  MemoryRegions actually do not have their
> - * own reference count; they piggyback on a QOM object, their "owner".
> - * This function removes a reference to the owner and possibly destroys it.
> + * This function removes a reference to the owner of a memory region and
> + * possibly destroys the owner along with the memory region. It does nothing if
> + * the owner is not present.
> + * See docs/devel/memory.rst to know about owner.
>  *
>  * @mr: the #MemoryRegion
>  */
> @@ -1255,7 +1252,7 @@ void memory_region_unref(MemoryRegion *mr);
>  * if @size is nonzero, subregions will be clipped to @size.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @ops: a structure containing read and write callbacks to be used when
>  *       I/O is performed on the region.
>  * @opaque: passed to the read and write callbacks of the @ops structure.
> @@ -1275,7 +1272,7 @@ void memory_region_init_io(MemoryRegion *mr,
>  *                                    directly.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1298,7 +1295,7 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
>  *                                          modify memory directly.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1328,7 +1325,7 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>  *                                     canceled.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: used size of the region.
> @@ -1357,7 +1354,7 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
>  *                                    mmap-ed backend.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1390,7 +1387,7 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
>  *                                  mmap-ed backend.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: the name of the region.
>  * @size: size of the region.
>  * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
> @@ -1421,7 +1418,7 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
>  *                              region will modify memory directly.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1449,7 +1446,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>  * skip_dump flag.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: the name of the region.
>  * @size: size of the region.
>  * @ptr: memory to be mapped; must contain at least @size bytes.
> @@ -1469,7 +1466,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
>  *                           part of another memory region.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: used for debugging; not visible to the user or ABI
>  * @orig: the region to be referenced; @mr will be equivalent to
>  *        @orig between @offset and @offset + @size - 1.
> @@ -1495,7 +1492,7 @@ void memory_region_init_alias(MemoryRegion *mr,
>  * of the caller.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1518,7 +1515,7 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
>  * of the caller.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @ops: callbacks for write access handling (must not be NULL).
>  * @opaque: passed to the read and write callbacks of the @ops structure.
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
> @@ -1554,7 +1551,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
>  * @_iommu_mr: the #IOMMUMemoryRegion to be initialized
>  * @instance_size: the IOMMUMemoryRegion subclass instance size
>  * @mrtypename: the type name of the #IOMMUMemoryRegion
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: used for debugging; not visible to the user or ABI
>  * @size: size of the region.
>  */
> @@ -1570,7 +1567,7 @@ void memory_region_init_iommu(void *_iommu_mr,
>  *                          region will modify memory directly.
>  *
>  * @mr: the #MemoryRegion to be initialized
> - * @owner: the object that tracks the region's reference count (must be
> + * @owner: the object that keeps the region alive (must be
>  *         TYPE_DEVICE or a subclass of TYPE_DEVICE, or NULL)
>  * @name: name of the memory region
>  * @size: size of the region in bytes
> @@ -1616,7 +1613,7 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
>  * If you pass a non-NULL non-device @owner then we will assert.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>  *        must be unique within any device
>  * @size: size of the region.
> @@ -1647,7 +1644,7 @@ bool memory_region_init_rom(MemoryRegion *mr,
>  * If you pass a non-NULL non-device @owner then we will assert.
>  *
>  * @mr: the #MemoryRegion to be initialized.
> - * @owner: the object that tracks the region's reference count
> + * @owner: the object that keeps the region alive
>  * @ops: callbacks for write access handling (must not be NULL).
>  * @opaque: passed to the read and write callbacks of the @ops structure.
>  * @name: Region name, becomes part of RAMBlock name used in migration stream
>
>

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Maydell]

On Fri, 10 Jan 2025 at 09:20, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> memory_region_update_container_subregions() used to call
> memory_region_ref(), which creates a reference to the owner of the
> subregion, on behalf of the owner of the container. This results in a
> circular reference if the subregion and container have the same owner.
>
> memory_region_ref() creates a reference to the owner instead of the
> memory region to match the lifetime of the owner and memory region. We
> do not need such a hack if the subregion and container have the same
> owner because the owner will be alive as long as the container is.
> Therefore, create a reference to the subregion itself instead ot its
> owner in such a case; the reference to the subregion is still necessary
> to ensure that the subregion gets finalized after the container.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Hi; I came back to this patchset because it was never applied,
and so we still have various leaks of MemoryRegion objects
in devices which use the "device owns a container MemoryRegion C
and puts another MemoryRegion X which it owns into that container"
pattern.

Unfortunately, with this patch applied, I see a segfault when running
the device-introspect-test for Arm:

$ (cd build/x86 && QTEST_QEMU_BINARY=./qemu-system-arm
./tests/qtest/device-introspect-test -p
/arm/device/introspect/concrete/defaults/none)
[...]
# Testing device 'imx7.analog'
Broken pipe
../../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
signal 11 (Segmentation fault) (core dumped)

Here's a backtrace collected with gdb:

$ (cd build/x86 && QTEST_QEMU_BINARY="xterm -e gdb --tty $(tty) --args
./qemu-system-arm" ./tests/qtest/device-introspect-test -p
/arm/device/introspect/concrete/defaults/none)
[hit 'r' in the gdb prompt in the new window]

Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
memory_region_unref_subregion (subregion=0x555557d58370) at
../../system/memory.c:1862
1862        if (subregion->container->owner == subregion->owner) {
(gdb) bt
#0  memory_region_unref_subregion (subregion=0x555557d58370) at
../../system/memory.c:1862
#1  0x0000555555d5f97b in memory_region_del_subregion
(mr=0x555557d58150, subregion=0x555557d58370)
    at ../../system/memory.c:2705
#2  0x0000555555d5cc1c in memory_region_finalize (obj=0x555557d58150)
at ../../system/memory.c:1811
#3  0x0000555556197595 in object_deinit (obj=0x555557d58150,
type=0x5555579d2ea0) at ../../qom/object.c:715
#4  0x0000555556197613 in object_finalize (data=0x555557d58150) at
../../qom/object.c:729
#5  0x00005555561988f9 in object_unref (objptr=0x555557d58150) at
../../qom/object.c:1232
#6  0x000055555619a3d6 in object_finalize_child_property
    (obj=0x555557d57e20, name=0x555557c59810 "imx7.analog[0]",
opaque=0x555557d58150) at ../../qom/object.c:1818
#7  0x0000555556197344 in object_property_del_all (obj=0x555557d57e20)
at ../../qom/object.c:667
#8  0x0000555556197600 in object_finalize (data=0x555557d57e20) at
../../qom/object.c:728
#9  0x00005555561988f9 in object_unref (objptr=0x555557d57e20) at
../../qom/object.c:1232
#10 0x00005555562f4600 in qmp_device_list_properties
(typename=0x555557d3ee70 "imx7.analog", errp=0x7fffffffdc00)
    at ../../qom/qom-qmp-cmds.c:237
#11 0x00005555563981ba in qmp_marshal_device_list_properties
(args=0x7fffe00031f0, ret=0x7fffef6afda8, errp=0x7fffef6afda0)
    at qapi/qapi-commands-qdev.c:65
#12 0x00005555563bcb53 in do_qmp_dispatch_bh (opaque=0x7fffef6afe40)
at ../../qapi/qmp-dispatch.c:128
#13 0x00005555563ed87f in aio_bh_call (bh=0x555557d3f540) at
../../util/async.c:172
#14 0x00005555563ed9cb in aio_bh_poll (ctx=0x555557a00770) at
../../util/async.c:219
#15 0x00005555563cd517 in aio_dispatch (ctx=0x555557a00770) at
../../util/aio-posix.c:436
#16 0x00005555563edebe in aio_ctx_dispatch (source=0x555557a00770,
callback=0x0, user_data=0x0) at ../../util/async.c:361
#17 0x00007ffff6e965c5 in ??? () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
#18 0x00007ffff6e96710 in g_main_context_dispatch () at
/lib/x86_64-linux-gnu/libglib-2.0.so.0
#19 0x00005555563ef89e in glib_pollfds_poll () at ../../util/main-loop.c:287
#20 0x00005555563ef930 in os_host_main_loop_wait (timeout=0) at
../../util/main-loop.c:310
#21 0x00005555563efa6b in main_loop_wait (nonblocking=0) at
../../util/main-loop.c:589
#22 0x0000555555d7d181 in qemu_main_loop () at ../../system/runstate.c:905
#23 0x00005555562f94c7 in qemu_default_main (opaque=0x0) at
../../system/main.c:50
#24 0x00005555562f9585 in main (argc=18, argv=0x7fffffffe078) at
../../system/main.c:93


In memory_region_unref_subregion(), subregion->container is NULL.

This is because in memory_region_del_subregion() we do:

    subregion->container = NULL;

and then after that we call
    memory_region_unref_subregion(subregion);
which dereferences subregion->container.

Won't this always SEGV ?

thanks
-- PMM

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Maydell]

(updated cc list with Akihiko's new email address)

On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 10 Jan 2025 at 09:20, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >
> > memory_region_update_container_subregions() used to call
> > memory_region_ref(), which creates a reference to the owner of the
> > subregion, on behalf of the owner of the container. This results in a
> > circular reference if the subregion and container have the same owner.
> >
> > memory_region_ref() creates a reference to the owner instead of the
> > memory region to match the lifetime of the owner and memory region. We
> > do not need such a hack if the subregion and container have the same
> > owner because the owner will be alive as long as the container is.
> > Therefore, create a reference to the subregion itself instead ot its
> > owner in such a case; the reference to the subregion is still necessary
> > to ensure that the subregion gets finalized after the container.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Hi; I came back to this patchset because it was never applied,
> and so we still have various leaks of MemoryRegion objects
> in devices which use the "device owns a container MemoryRegion C
> and puts another MemoryRegion X which it owns into that container"
> pattern.
>
> Unfortunately, with this patch applied, I see a segfault when running
> the device-introspect-test for Arm:
>
> $ (cd build/x86 && QTEST_QEMU_BINARY=./qemu-system-arm
> ./tests/qtest/device-introspect-test -p
> /arm/device/introspect/concrete/defaults/none)
> [...]
> # Testing device 'imx7.analog'
> Broken pipe
> ../../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> signal 11 (Segmentation fault) (core dumped)
>
> Here's a backtrace collected with gdb:
>
> $ (cd build/x86 && QTEST_QEMU_BINARY="xterm -e gdb --tty $(tty) --args
> ./qemu-system-arm" ./tests/qtest/device-introspect-test -p
> /arm/device/introspect/concrete/defaults/none)
> [hit 'r' in the gdb prompt in the new window]
>
> Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
> memory_region_unref_subregion (subregion=0x555557d58370) at
> ../../system/memory.c:1862
> 1862        if (subregion->container->owner == subregion->owner) {
> (gdb) bt
> #0  memory_region_unref_subregion (subregion=0x555557d58370) at
> ../../system/memory.c:1862
> #1  0x0000555555d5f97b in memory_region_del_subregion
> (mr=0x555557d58150, subregion=0x555557d58370)
>     at ../../system/memory.c:2705
> #2  0x0000555555d5cc1c in memory_region_finalize (obj=0x555557d58150)
> at ../../system/memory.c:1811
> #3  0x0000555556197595 in object_deinit (obj=0x555557d58150,
> type=0x5555579d2ea0) at ../../qom/object.c:715
> #4  0x0000555556197613 in object_finalize (data=0x555557d58150) at
> ../../qom/object.c:729
> #5  0x00005555561988f9 in object_unref (objptr=0x555557d58150) at
> ../../qom/object.c:1232
> #6  0x000055555619a3d6 in object_finalize_child_property
>     (obj=0x555557d57e20, name=0x555557c59810 "imx7.analog[0]",
> opaque=0x555557d58150) at ../../qom/object.c:1818
> #7  0x0000555556197344 in object_property_del_all (obj=0x555557d57e20)
> at ../../qom/object.c:667
> #8  0x0000555556197600 in object_finalize (data=0x555557d57e20) at
> ../../qom/object.c:728
> #9  0x00005555561988f9 in object_unref (objptr=0x555557d57e20) at
> ../../qom/object.c:1232
> #10 0x00005555562f4600 in qmp_device_list_properties
> (typename=0x555557d3ee70 "imx7.analog", errp=0x7fffffffdc00)
>     at ../../qom/qom-qmp-cmds.c:237
> #11 0x00005555563981ba in qmp_marshal_device_list_properties
> (args=0x7fffe00031f0, ret=0x7fffef6afda8, errp=0x7fffef6afda0)
>     at qapi/qapi-commands-qdev.c:65
> #12 0x00005555563bcb53 in do_qmp_dispatch_bh (opaque=0x7fffef6afe40)
> at ../../qapi/qmp-dispatch.c:128
> #13 0x00005555563ed87f in aio_bh_call (bh=0x555557d3f540) at
> ../../util/async.c:172
> #14 0x00005555563ed9cb in aio_bh_poll (ctx=0x555557a00770) at
> ../../util/async.c:219
> #15 0x00005555563cd517 in aio_dispatch (ctx=0x555557a00770) at
> ../../util/aio-posix.c:436
> #16 0x00005555563edebe in aio_ctx_dispatch (source=0x555557a00770,
> callback=0x0, user_data=0x0) at ../../util/async.c:361
> #17 0x00007ffff6e965c5 in ??? () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #18 0x00007ffff6e96710 in g_main_context_dispatch () at
> /lib/x86_64-linux-gnu/libglib-2.0.so.0
> #19 0x00005555563ef89e in glib_pollfds_poll () at ../../util/main-loop.c:287
> #20 0x00005555563ef930 in os_host_main_loop_wait (timeout=0) at
> ../../util/main-loop.c:310
> #21 0x00005555563efa6b in main_loop_wait (nonblocking=0) at
> ../../util/main-loop.c:589
> #22 0x0000555555d7d181 in qemu_main_loop () at ../../system/runstate.c:905
> #23 0x00005555562f94c7 in qemu_default_main (opaque=0x0) at
> ../../system/main.c:50
> #24 0x00005555562f9585 in main (argc=18, argv=0x7fffffffe078) at
> ../../system/main.c:93
>
>
> In memory_region_unref_subregion(), subregion->container is NULL.
>
> This is because in memory_region_del_subregion() we do:
>
>     subregion->container = NULL;
>
> and then after that we call
>     memory_region_unref_subregion(subregion);
> which dereferences subregion->container.
>
> Won't this always SEGV ?
>
> thanks
> -- PMM

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Xu]

On Thu, Aug 21, 2025 at 01:45:20PM +0100, Peter Maydell wrote:
> (updated cc list with Akihiko's new email address)
> 
> On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > On Fri, 10 Jan 2025 at 09:20, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> > >
> > > memory_region_update_container_subregions() used to call
> > > memory_region_ref(), which creates a reference to the owner of the
> > > subregion, on behalf of the owner of the container. This results in a
> > > circular reference if the subregion and container have the same owner.
> > >
> > > memory_region_ref() creates a reference to the owner instead of the
> > > memory region to match the lifetime of the owner and memory region. We
> > > do not need such a hack if the subregion and container have the same
> > > owner because the owner will be alive as long as the container is.
> > > Therefore, create a reference to the subregion itself instead ot its
> > > owner in such a case; the reference to the subregion is still necessary
> > > to ensure that the subregion gets finalized after the container.
> > >
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > Hi; I came back to this patchset because it was never applied,
> > and so we still have various leaks of MemoryRegion objects
> > in devices which use the "device owns a container MemoryRegion C
> > and puts another MemoryRegion X which it owns into that container"
> > pattern.
> >
> > Unfortunately, with this patch applied, I see a segfault when running
> > the device-introspect-test for Arm:
> >
> > $ (cd build/x86 && QTEST_QEMU_BINARY=./qemu-system-arm
> > ./tests/qtest/device-introspect-test -p
> > /arm/device/introspect/concrete/defaults/none)
> > [...]
> > # Testing device 'imx7.analog'
> > Broken pipe
> > ../../tests/qtest/libqtest.c:208: kill_qemu() detected QEMU death from
> > signal 11 (Segmentation fault) (core dumped)
> >
> > Here's a backtrace collected with gdb:
> >
> > $ (cd build/x86 && QTEST_QEMU_BINARY="xterm -e gdb --tty $(tty) --args
> > ./qemu-system-arm" ./tests/qtest/device-introspect-test -p
> > /arm/device/introspect/concrete/defaults/none)
> > [hit 'r' in the gdb prompt in the new window]
> >
> > Thread 1 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
> > memory_region_unref_subregion (subregion=0x555557d58370) at
> > ../../system/memory.c:1862
> > 1862        if (subregion->container->owner == subregion->owner) {
> > (gdb) bt
> > #0  memory_region_unref_subregion (subregion=0x555557d58370) at
> > ../../system/memory.c:1862
> > #1  0x0000555555d5f97b in memory_region_del_subregion
> > (mr=0x555557d58150, subregion=0x555557d58370)
> >     at ../../system/memory.c:2705
> > #2  0x0000555555d5cc1c in memory_region_finalize (obj=0x555557d58150)
> > at ../../system/memory.c:1811
> > #3  0x0000555556197595 in object_deinit (obj=0x555557d58150,
> > type=0x5555579d2ea0) at ../../qom/object.c:715
> > #4  0x0000555556197613 in object_finalize (data=0x555557d58150) at
> > ../../qom/object.c:729
> > #5  0x00005555561988f9 in object_unref (objptr=0x555557d58150) at
> > ../../qom/object.c:1232
> > #6  0x000055555619a3d6 in object_finalize_child_property
> >     (obj=0x555557d57e20, name=0x555557c59810 "imx7.analog[0]",
> > opaque=0x555557d58150) at ../../qom/object.c:1818
> > #7  0x0000555556197344 in object_property_del_all (obj=0x555557d57e20)
> > at ../../qom/object.c:667
> > #8  0x0000555556197600 in object_finalize (data=0x555557d57e20) at
> > ../../qom/object.c:728
> > #9  0x00005555561988f9 in object_unref (objptr=0x555557d57e20) at
> > ../../qom/object.c:1232
> > #10 0x00005555562f4600 in qmp_device_list_properties
> > (typename=0x555557d3ee70 "imx7.analog", errp=0x7fffffffdc00)
> >     at ../../qom/qom-qmp-cmds.c:237
> > #11 0x00005555563981ba in qmp_marshal_device_list_properties
> > (args=0x7fffe00031f0, ret=0x7fffef6afda8, errp=0x7fffef6afda0)
> >     at qapi/qapi-commands-qdev.c:65
> > #12 0x00005555563bcb53 in do_qmp_dispatch_bh (opaque=0x7fffef6afe40)
> > at ../../qapi/qmp-dispatch.c:128
> > #13 0x00005555563ed87f in aio_bh_call (bh=0x555557d3f540) at
> > ../../util/async.c:172
> > #14 0x00005555563ed9cb in aio_bh_poll (ctx=0x555557a00770) at
> > ../../util/async.c:219
> > #15 0x00005555563cd517 in aio_dispatch (ctx=0x555557a00770) at
> > ../../util/aio-posix.c:436
> > #16 0x00005555563edebe in aio_ctx_dispatch (source=0x555557a00770,
> > callback=0x0, user_data=0x0) at ../../util/async.c:361
> > #17 0x00007ffff6e965c5 in ??? () at /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #18 0x00007ffff6e96710 in g_main_context_dispatch () at
> > /lib/x86_64-linux-gnu/libglib-2.0.so.0
> > #19 0x00005555563ef89e in glib_pollfds_poll () at ../../util/main-loop.c:287
> > #20 0x00005555563ef930 in os_host_main_loop_wait (timeout=0) at
> > ../../util/main-loop.c:310
> > #21 0x00005555563efa6b in main_loop_wait (nonblocking=0) at
> > ../../util/main-loop.c:589
> > #22 0x0000555555d7d181 in qemu_main_loop () at ../../system/runstate.c:905
> > #23 0x00005555562f94c7 in qemu_default_main (opaque=0x0) at
> > ../../system/main.c:50
> > #24 0x00005555562f9585 in main (argc=18, argv=0x7fffffffe078) at
> > ../../system/main.c:93
> >
> >
> > In memory_region_unref_subregion(), subregion->container is NULL.
> >
> > This is because in memory_region_del_subregion() we do:
> >
> >     subregion->container = NULL;
> >
> > and then after that we call
> >     memory_region_unref_subregion(subregion);
> > which dereferences subregion->container.
> >
> > Won't this always SEGV ?
> >
> > thanks
> > -- PMM
> 

Peter, could you try the v3 version patch 8/9 instead?

https://lore.kernel.org/all/20240708-san-v3-8-b03f671c40c6@daynix.com/

I still prefer that one, and I hope that one doesn't have this issue.

If no one objects, IMHO we could merge v3's patch 8/9, and v8's patch 1 (I
can touch up some commit messages and details, e.g. v3's 8/9 commit
message is a bit too simple to me).

Thanks,

-- 
Peter Xu

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Maydell]

On Thu, 21 Aug 2025 at 15:28, Peter Xu <peterx@redhat.com> wrote:
> > On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > In memory_region_unref_subregion(), subregion->container is NULL.
> > >
> > > This is because in memory_region_del_subregion() we do:
> > >
> > >     subregion->container = NULL;
> > >
> > > and then after that we call
> > >     memory_region_unref_subregion(subregion);
> > > which dereferences subregion->container.
> > >
> > > Won't this always SEGV ?

> Peter, could you try the v3 version patch 8/9 instead?
>
> https://lore.kernel.org/all/20240708-san-v3-8-b03f671c40c6@daynix.com/
>
> I still prefer that one, and I hope that one doesn't have this issue.

That one fails like this:
qemu-system-arm: ../../system/memory.c:1799: memory_region_finalize:
Assertion `!mr->container' failed.

See the discussion on v2 (which was the same for this patch):
https://lore.kernel.org/all/CAFEAcA9KTSjwF1rABpM5nv9UFuKqZZk6+Qo4PEF4+rTirNi5fQ@mail.gmail.com/

thanks
-- PMM

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Xu]

On Thu, Aug 21, 2025 at 03:47:06PM +0100, Peter Maydell wrote:
> On Thu, 21 Aug 2025 at 15:28, Peter Xu <peterx@redhat.com> wrote:
> > > On Thu, 21 Aug 2025 at 13:40, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > In memory_region_unref_subregion(), subregion->container is NULL.
> > > >
> > > > This is because in memory_region_del_subregion() we do:
> > > >
> > > >     subregion->container = NULL;
> > > >
> > > > and then after that we call
> > > >     memory_region_unref_subregion(subregion);
> > > > which dereferences subregion->container.
> > > >
> > > > Won't this always SEGV ?
> 
> > Peter, could you try the v3 version patch 8/9 instead?
> >
> > https://lore.kernel.org/all/20240708-san-v3-8-b03f671c40c6@daynix.com/
> >
> > I still prefer that one, and I hope that one doesn't have this issue.
> 
> That one fails like this:
> qemu-system-arm: ../../system/memory.c:1799: memory_region_finalize:
> Assertion `!mr->container' failed.
> 
> See the discussion on v2 (which was the same for this patch):
> https://lore.kernel.org/all/CAFEAcA9KTSjwF1rABpM5nv9UFuKqZZk6+Qo4PEF4+rTirNi5fQ@mail.gmail.com/

My apologies, it was a long discussion and I totally forgot that..

I remember I provided a draft somewhere during the discussion, anyway.. I
redid it, and attached a formal patch below that will contain the removal
of the mr->container check (plus auto-detach when it happens).  The hope is
this should work.. and comparing to the v8 of Akihiko's, it won't make MR's
own refcount any more complicated.

If necessary, I can send a formal patch.

Thanks,

===8<===
From f985c54af3e276b175bcb02725a5fb996c3f5fe5 Mon Sep 17 00:00:00 2001
From: Peter Xu <peterx@redhat.com>
Date: Thu, 21 Aug 2025 12:59:02 -0400
Subject: [PATCH] memory: Fix leaks due to owner-shared MRs circular references

Currently, QEMU refcounts the MR by always taking it from the owner.

It should be non-issue if all the owners of MRs will properly detach all
the MRs from their parents by memory_region_del_subregion() when the owner
will be freed.  However, it turns out many of the device emulations do not
do that properly.  It might be a challenge to fix all of such occurances.

Without fixing it, QEMU faces circular reference issue when an owner can
contain more than one MRs, then when they are linked within each other in
form of subregions, those links keep the owner alive forever, causing
memory leaks even if all the external refcounts are released for the owner.

To fix that, teach memory API to stop refcount on MRs that share the same
owner because if they share the lifecycle of the owner, then they share the
same lifecycle between themselves.

Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
against its container, as long as they belong to the same owner.

The latter is needed because now it's possible to have MR finalize() happen
in any order when they exactly share the same lifespan.  In this case, we
should allow finalize() to happen in any order of either the parent or
child MR, however it should be guaranteed that the mr->container shares the
same owner of this MR to be finalized.

Proper document this behavior in code.

This patch is heavily based on the work done by Akihiko Odaki:

https://lore.kernel.org/r/CAFEAcA8DV40fGsci76r4yeP1P-SP_QjNRDD2OzPxjx5wRs0GEg@mail.gmail.com

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 docs/devel/memory.rst |  9 +++++++--
 system/memory.c       | 45 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 57fb2aec76..1367c7caf7 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -158,8 +158,13 @@ ioeventfd) can be changed during the region lifecycle.  They take effect
 as soon as the region is made visible.  This can be immediately, later,
 or never.
 
-Destruction of a memory region happens automatically when the owner
-object dies.
+Destruction of a memory region happens automatically when the owner object
+dies.  Note that the MRs under the same owner can be destroyed in any order
+when the owner object dies.  It's because the MRs will share the same
+lifespan of the owner, no matter if its a container or child MR.  It's
+suggested to always cleanly detach the MRs under the owner object when the
+owner object is going to be destroyed, however it is not required, as the
+memory core will automatically detach the links when MRs are destroyed.
 
 If however the memory region is part of a dynamically allocated data
 structure, you should call object_unparent() to destroy the memory region
diff --git a/system/memory.c b/system/memory.c
index 5646547940..d7f6ad9be2 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1796,16 +1796,36 @@ static void memory_region_finalize(Object *obj)
 {
     MemoryRegion *mr = MEMORY_REGION(obj);
 
-    assert(!mr->container);
-
-    /* We know the region is not visible in any address space (it
-     * does not have a container and cannot be a root either because
-     * it has no references, so we can blindly clear mr->enabled.
-     * memory_region_set_enabled instead could trigger a transaction
-     * and cause an infinite loop.
+    /*
+     * Each memory region (that can be dynamically freed..) must has an
+     * owner, and it always has the same lifecycle of its owner.  It means
+     * when reaching here, the memory region's owner refcount is zero.
+     *
+     * Here it is possible that the MR has:
+     *
+     * (1) mr->container set, which means this MR can be a subregion of a
+     *     container MR, in this case it must share the same owner
+     *     (otherwise the container should have kept a refcount of this
+     *     MR's owner), or,
+     *
+     * (2) mr->subregions non-empty, which means this MR can be a container
+     *     of other MRs (share the owner or not).
+     *
+     * We know the MR, or any MR that is attached to this one as either
+     * container or children, is not visible in any address space, because
+     * otherwise the address space should have taken at least one refcount
+     * of this MR's owner.  So we can blindly clear mr->enabled.
+     *
+     * memory_region_set_enabled instead could trigger a transaction and
+     * cause an infinite loop.
      */
     mr->enabled = false;
     memory_region_transaction_begin();
+    if (mr->container) {
+        /* Must share the owner; see above comments */
+        assert(mr->container->owner == mr->owner);
+        memory_region_del_subregion(mr->container, mr);
+    }
     while (!QTAILQ_EMPTY(&mr->subregions)) {
         MemoryRegion *subregion = QTAILQ_FIRST(&mr->subregions);
         memory_region_del_subregion(mr, subregion);
@@ -2625,7 +2645,10 @@ static void memory_region_update_container_subregions(MemoryRegion *subregion)
 
     memory_region_transaction_begin();
 
-    memory_region_ref(subregion);
+    if (mr->owner != subregion->owner) {
+        memory_region_ref(subregion);
+    }
+
     QTAILQ_FOREACH(other, &mr->subregions, subregions_link) {
         if (subregion->priority >= other->priority) {
             QTAILQ_INSERT_BEFORE(other, subregion, subregions_link);
@@ -2683,7 +2706,11 @@ void memory_region_del_subregion(MemoryRegion *mr,
         assert(alias->mapped_via_alias >= 0);
     }
     QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link);
-    memory_region_unref(subregion);
+
+    if (mr->owner != subregion->owner) {
+        memory_region_unref(subregion);
+    }
+
     memory_region_update_pending |= mr->enabled && subregion->enabled;
     memory_region_transaction_commit();
 }
-- 
2.50.1


-- 
Peter Xu

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Maydell]

On Thu, 21 Aug 2025 at 19:18, Peter Xu <peterx@redhat.com> wrote:
> I remember I provided a draft somewhere during the discussion, anyway.. I
> redid it, and attached a formal patch below that will contain the removal
> of the mr->container check (plus auto-detach when it happens).  The hope is
> this should work.. and comparing to the v8 of Akihiko's, it won't make MR's
> own refcount any more complicated.
>
> If necessary, I can send a formal patch.

This patch seems to work, in that it fixes the memory-region
related leaks I previously was seeing. Review comments below
(which are only about the commit message and docs).

I also can't remember the details of the previous discussions about
these patches, so I'm reviewing the one below essentially from
scratch. Apologies in advance if we end up going back around
a conversation loop that we've already had...

> Thanks,
>
> ===8<===
> From f985c54af3e276b175bcb02725a5fb996c3f5fe5 Mon Sep 17 00:00:00 2001
> From: Peter Xu <peterx@redhat.com>
> Date: Thu, 21 Aug 2025 12:59:02 -0400
> Subject: [PATCH] memory: Fix leaks due to owner-shared MRs circular references
>
> Currently, QEMU refcounts the MR by always taking it from the owner.
>
> It should be non-issue if all the owners of MRs will properly detach all
> the MRs from their parents by memory_region_del_subregion() when the owner
> will be freed.  However, it turns out many of the device emulations do not
> do that properly.  It might be a challenge to fix all of such occurances.

I think that it's not really right to cast this as "some devices
don't do this right". The pattern of "a device has a container MR C
and some other MRs A, B... which it puts into C" is a legitimate one.
If you do this then (with the current code in QEMU) there is *no*
place where a device emulation can do a manual "remove A, B.. from
the container C so the refcounts go down": the place where devices
undo what they did in instance_init is instance_deinit, but we
will never call instance_deinit because the refcount of the owning
device never goes to 0.

Further, even if we had some place where devices could somehow
undo the "put A, B... in the container so the refcounts go down
correctly", it is better API design to have the memory.c code
automatically handle this situation. "This just works" is more
reliable than "this works if you do cleanup step X", because it's
impossible to have the bug where you forget to write the code to
do the cleanup step.

> Without fixing it, QEMU faces circular reference issue when an owner can
> contain more than one MRs, then when they are linked within each other in
> form of subregions, those links keep the owner alive forever, causing
> memory leaks even if all the external refcounts are released for the owner.
>
> To fix that, teach memory API to stop refcount on MRs that share the same
> owner because if they share the lifecycle of the owner, then they share the
> same lifecycle between themselves.
>
> Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
> against its container, as long as they belong to the same owner.
>
> The latter is needed because now it's possible to have MR finalize() happen
> in any order when they exactly share the same lifespan.  In this case, we
> should allow finalize() to happen in any order of either the parent or
> child MR, however it should be guaranteed that the mr->container shares the
> same owner of this MR to be finalized.
>
> Proper document this behavior in code.
>
> This patch is heavily based on the work done by Akihiko Odaki:
>
> https://lore.kernel.org/r/CAFEAcA8DV40fGsci76r4yeP1P-SP_QjNRDD2OzPxjx5wRs0GEg@mail.gmail.com
>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  docs/devel/memory.rst |  9 +++++++--
>  system/memory.c       | 45 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 43 insertions(+), 11 deletions(-)
>
> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> index 57fb2aec76..1367c7caf7 100644
> --- a/docs/devel/memory.rst
> +++ b/docs/devel/memory.rst
> @@ -158,8 +158,13 @@ ioeventfd) can be changed during the region lifecycle.  They take effect
>  as soon as the region is made visible.  This can be immediately, later,
>  or never.
>
> -Destruction of a memory region happens automatically when the owner
> -object dies.
> +Destruction of a memory region happens automatically when the owner object
> +dies.  Note that the MRs under the same owner can be destroyed in any order
> +when the owner object dies.  It's because the MRs will share the same
> +lifespan of the owner, no matter if its a container or child MR.  It's
> +suggested to always cleanly detach the MRs under the owner object when the
> +owner object is going to be destroyed, however it is not required, as the
> +memory core will automatically detach the links when MRs are destroyed.

I think we should not say "we suggest you always cleanly detach the MRs":
instead we should say "you can rely on this happening, so you don't
need to write manual code to do it".

The actual code changes look OK to me.

thanks
-- PMM

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Xu]

On Tue, Aug 26, 2025 at 06:45:43PM +0100, Peter Maydell wrote:
> On Thu, 21 Aug 2025 at 19:18, Peter Xu <peterx@redhat.com> wrote:
> > I remember I provided a draft somewhere during the discussion, anyway.. I
> > redid it, and attached a formal patch below that will contain the removal
> > of the mr->container check (plus auto-detach when it happens).  The hope is
> > this should work.. and comparing to the v8 of Akihiko's, it won't make MR's
> > own refcount any more complicated.
> >
> > If necessary, I can send a formal patch.
> 
> This patch seems to work, in that it fixes the memory-region
> related leaks I previously was seeing. Review comments below
> (which are only about the commit message and docs).
> 
> I also can't remember the details of the previous discussions about
> these patches, so I'm reviewing the one below essentially from
> scratch. Apologies in advance if we end up going back around
> a conversation loop that we've already had...
> 
> > Thanks,
> >
> > ===8<===
> > From f985c54af3e276b175bcb02725a5fb996c3f5fe5 Mon Sep 17 00:00:00 2001
> > From: Peter Xu <peterx@redhat.com>
> > Date: Thu, 21 Aug 2025 12:59:02 -0400
> > Subject: [PATCH] memory: Fix leaks due to owner-shared MRs circular references
> >
> > Currently, QEMU refcounts the MR by always taking it from the owner.
> >
> > It should be non-issue if all the owners of MRs will properly detach all
> > the MRs from their parents by memory_region_del_subregion() when the owner
> > will be freed.  However, it turns out many of the device emulations do not
> > do that properly.  It might be a challenge to fix all of such occurances.
> 
> I think that it's not really right to cast this as "some devices
> don't do this right". The pattern of "a device has a container MR C
> and some other MRs A, B... which it puts into C" is a legitimate one.
> If you do this then (with the current code in QEMU) there is *no*
> place where a device emulation can do a manual "remove A, B.. from
> the container C so the refcounts go down": the place where devices
> undo what they did in instance_init is instance_deinit, but we
> will never call instance_deinit because the refcount of the owning
> device never goes to 0.

It should still be able to reach zero if we skip the refcount of internal
MR subregions like what this patch does.

Said that, I think you're right..  the problem is we have object_deinit()
after object_property_del_all() (in object_finalize()), which means
memory_region_finalize() will be invoked before object_deinit()..  Then it
looks wrong now to delete subregions in a deinit() even if reachable,
because the MRs should have been freed..

> 
> Further, even if we had some place where devices could somehow
> undo the "put A, B... in the container so the refcounts go down
> correctly", it is better API design to have the memory.c code
> automatically handle this situation. "This just works" is more
> reliable than "this works if you do cleanup step X", because it's
> impossible to have the bug where you forget to write the code to
> do the cleanup step.

Fair enough.

> 
> > Without fixing it, QEMU faces circular reference issue when an owner can
> > contain more than one MRs, then when they are linked within each other in
> > form of subregions, those links keep the owner alive forever, causing
> > memory leaks even if all the external refcounts are released for the owner.
> >
> > To fix that, teach memory API to stop refcount on MRs that share the same
> > owner because if they share the lifecycle of the owner, then they share the
> > same lifecycle between themselves.
> >
> > Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
> > against its container, as long as they belong to the same owner.
> >
> > The latter is needed because now it's possible to have MR finalize() happen
> > in any order when they exactly share the same lifespan.  In this case, we
> > should allow finalize() to happen in any order of either the parent or
> > child MR, however it should be guaranteed that the mr->container shares the
> > same owner of this MR to be finalized.
> >
> > Proper document this behavior in code.
> >
> > This patch is heavily based on the work done by Akihiko Odaki:
> >
> > https://lore.kernel.org/r/CAFEAcA8DV40fGsci76r4yeP1P-SP_QjNRDD2OzPxjx5wRs0GEg@mail.gmail.com
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  docs/devel/memory.rst |  9 +++++++--
> >  system/memory.c       | 45 ++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 43 insertions(+), 11 deletions(-)
> >
> > diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
> > index 57fb2aec76..1367c7caf7 100644
> > --- a/docs/devel/memory.rst
> > +++ b/docs/devel/memory.rst
> > @@ -158,8 +158,13 @@ ioeventfd) can be changed during the region lifecycle.  They take effect
> >  as soon as the region is made visible.  This can be immediately, later,
> >  or never.
> >
> > -Destruction of a memory region happens automatically when the owner
> > -object dies.
> > +Destruction of a memory region happens automatically when the owner object
> > +dies.  Note that the MRs under the same owner can be destroyed in any order
> > +when the owner object dies.  It's because the MRs will share the same
> > +lifespan of the owner, no matter if its a container or child MR.  It's
> > +suggested to always cleanly detach the MRs under the owner object when the
> > +owner object is going to be destroyed, however it is not required, as the
> > +memory core will automatically detach the links when MRs are destroyed.
> 
> I think we should not say "we suggest you always cleanly detach the MRs":
> instead we should say "you can rely on this happening, so you don't
> need to write manual code to do it".
> 
> The actual code changes look OK to me.

I'll send the patch out officially for review, with above point taken.

Akihiko, let me know anytime when you want to either add your SoB or take
over the ownership of the patch.  I'll be OK with it.

Thanks,

-- 
Peter Xu

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2025/08/27 6:57, Peter Xu wrote:
> On Tue, Aug 26, 2025 at 06:45:43PM +0100, Peter Maydell wrote:
>> On Thu, 21 Aug 2025 at 19:18, Peter Xu <peterx@redhat.com> wrote:
>>> I remember I provided a draft somewhere during the discussion, anyway.. I
>>> redid it, and attached a formal patch below that will contain the removal
>>> of the mr->container check (plus auto-detach when it happens).  The hope is
>>> this should work.. and comparing to the v8 of Akihiko's, it won't make MR's
>>> own refcount any more complicated.
>>>
>>> If necessary, I can send a formal patch.
>>
>> This patch seems to work, in that it fixes the memory-region
>> related leaks I previously was seeing. Review comments below
>> (which are only about the commit message and docs).
>>
>> I also can't remember the details of the previous discussions about
>> these patches, so I'm reviewing the one below essentially from
>> scratch. Apologies in advance if we end up going back around
>> a conversation loop that we've already had...
>>
>>> Thanks,
>>>
>>> ===8<===
>>>  From f985c54af3e276b175bcb02725a5fb996c3f5fe5 Mon Sep 17 00:00:00 2001
>>> From: Peter Xu <peterx@redhat.com>
>>> Date: Thu, 21 Aug 2025 12:59:02 -0400
>>> Subject: [PATCH] memory: Fix leaks due to owner-shared MRs circular references
>>>
>>> Currently, QEMU refcounts the MR by always taking it from the owner.
>>>
>>> It should be non-issue if all the owners of MRs will properly detach all
>>> the MRs from their parents by memory_region_del_subregion() when the owner
>>> will be freed.  However, it turns out many of the device emulations do not
>>> do that properly.  It might be a challenge to fix all of such occurances.
>>
>> I think that it's not really right to cast this as "some devices
>> don't do this right". The pattern of "a device has a container MR C
>> and some other MRs A, B... which it puts into C" is a legitimate one.
>> If you do this then (with the current code in QEMU) there is *no*
>> place where a device emulation can do a manual "remove A, B.. from
>> the container C so the refcounts go down": the place where devices
>> undo what they did in instance_init is instance_deinit, but we
>> will never call instance_deinit because the refcount of the owning
>> device never goes to 0.
> 
> It should still be able to reach zero if we skip the refcount of internal
> MR subregions like what this patch does.
> 
> Said that, I think you're right..  the problem is we have object_deinit()
> after object_property_del_all() (in object_finalize()), which means
> memory_region_finalize() will be invoked before object_deinit()..  Then it
> looks wrong now to delete subregions in a deinit() even if reachable,
> because the MRs should have been freed..
> 
>>
>> Further, even if we had some place where devices could somehow
>> undo the "put A, B... in the container so the refcounts go down
>> correctly", it is better API design to have the memory.c code
>> automatically handle this situation. "This just works" is more
>> reliable than "this works if you do cleanup step X", because it's
>> impossible to have the bug where you forget to write the code to
>> do the cleanup step.
> 
> Fair enough.
> 
>>
>>> Without fixing it, QEMU faces circular reference issue when an owner can
>>> contain more than one MRs, then when they are linked within each other in
>>> form of subregions, those links keep the owner alive forever, causing
>>> memory leaks even if all the external refcounts are released for the owner.
>>>
>>> To fix that, teach memory API to stop refcount on MRs that share the same
>>> owner because if they share the lifecycle of the owner, then they share the
>>> same lifecycle between themselves.
>>>
>>> Meanwhile, allow auto-detachments of MRs during finalize() of MRs even
>>> against its container, as long as they belong to the same owner.
>>>
>>> The latter is needed because now it's possible to have MR finalize() happen
>>> in any order when they exactly share the same lifespan.  In this case, we
>>> should allow finalize() to happen in any order of either the parent or
>>> child MR, however it should be guaranteed that the mr->container shares the
>>> same owner of this MR to be finalized.
>>>
>>> Proper document this behavior in code.
>>>
>>> This patch is heavily based on the work done by Akihiko Odaki:
>>>
>>> https://lore.kernel.org/r/CAFEAcA8DV40fGsci76r4yeP1P-SP_QjNRDD2OzPxjx5wRs0GEg@mail.gmail.com
>>>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>   docs/devel/memory.rst |  9 +++++++--
>>>   system/memory.c       | 45 ++++++++++++++++++++++++++++++++++---------
>>>   2 files changed, 43 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
>>> index 57fb2aec76..1367c7caf7 100644
>>> --- a/docs/devel/memory.rst
>>> +++ b/docs/devel/memory.rst
>>> @@ -158,8 +158,13 @@ ioeventfd) can be changed during the region lifecycle.  They take effect
>>>   as soon as the region is made visible.  This can be immediately, later,
>>>   or never.
>>>
>>> -Destruction of a memory region happens automatically when the owner
>>> -object dies.
>>> +Destruction of a memory region happens automatically when the owner object
>>> +dies.  Note that the MRs under the same owner can be destroyed in any order
>>> +when the owner object dies.  It's because the MRs will share the same
>>> +lifespan of the owner, no matter if its a container or child MR.  It's
>>> +suggested to always cleanly detach the MRs under the owner object when the
>>> +owner object is going to be destroyed, however it is not required, as the
>>> +memory core will automatically detach the links when MRs are destroyed.
>>
>> I think we should not say "we suggest you always cleanly detach the MRs":
>> instead we should say "you can rely on this happening, so you don't
>> need to write manual code to do it".
>>
>> The actual code changes look OK to me.
> 
> I'll send the patch out officially for review, with above point taken.
> 
> Akihiko, let me know anytime when you want to either add your SoB or take
> over the ownership of the patch.  I'll be OK with it.

I'm not in favor of the change. There was a long discussion in the past, 
but I think the following email is the most comprehensive description of 
my point and includes comparison between the two approaches:
https://lore.kernel.org/qemu-devel/2fe8b128-dda1-40af-89ac-e86ba53138f5@daynix.com/

So please check the email and reply it.

Regards,
Akihiko Odaki

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Peter Maydell]

On Thu, 28 Aug 2025 at 02:13, Akihiko Odaki
<odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
> I'm not in favor of the change. There was a long discussion in the past,
> but I think the following email is the most comprehensive description of
> my point and includes comparison between the two approaches:
> https://lore.kernel.org/qemu-devel/2fe8b128-dda1-40af-89ac-e86ba53138f5@daynix.com/
>
> So please check the email and reply it.

Do you have a version of your patch which works (i.e. doesn't
segfault) ?

-- PMM

Re: [PATCH v8 2/2] memory: Do not create circular reference with subregion

[Akihiko Odaki]

On 2025/08/28 18:54, Peter Maydell wrote:
> On Thu, 28 Aug 2025 at 02:13, Akihiko Odaki
> <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>> I'm not in favor of the change. There was a long discussion in the past,
>> but I think the following email is the most comprehensive description of
>> my point and includes comparison between the two approaches:
>> https://lore.kernel.org/qemu-devel/2fe8b128-dda1-40af-89ac-e86ba53138f5@daynix.com/
>>
>> So please check the email and reply it.
> 
> Do you have a version of your patch which works (i.e. doesn't
> segfault) ?

Sorry, I missed the context. And indeed v7 and v8 do not work; It seems 
I failed to test v7.

I posted v9, which fixes the NULL dereference.

Regards,
Akihiko Odaki