[PATCH v3 0/6] Implement memory_region_new_* functions

[PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

Our documentation says that memory regions are automatically freed
when the owner dies and the reference counting to do this is also
implemented. However this relies on the QOM free funtion that can only
be set by creating objects with object_new but memory API only
provides constructors that call object_initialize which clears the
free function that prevents QOM to manage the memory region lifetime.
Implement corresponding memory_region_new_* functions that do the same
as the memory_region_init_* functions but create the memory region
with object_new so the lifetime can be automatically managed by QOM as
documented. The memory_region_init functions are kept because they are
useful for memory regions embedded in other object or managed
externally and not by QOM for some reason.

v3:
Based-on: <cover.1770753117.git.balaton@eik.bme.hu>
- convert xtfpga from nomigrate as this has no migration compatibility
issue

v2:
- rebase on master
- update documentation
- use these function to fix some leaks (there may be more, e.g. in
hw/pci-host/bonito but I leave that for later and/or others)

BALATON Zoltan (6):
  memory: Add memory_region_new* functions
  memory: Update documentation for memory_region_new*()
  hw/ide/sii3112: Use memory_region_new to avoid leaking regions
  hw/pci-host/articia: Map PCI memory windows in realize
  hw/pci-host/articia: Add variable for common type cast
  hw/xtensa/xtfpga: Fix leaking memory region

 docs/devel/memory.rst   |  21 ++---
 hw/ide/sii3112.c        |  30 +++----
 hw/pci-host/articia.c   |  22 +++--
 hw/ppc/amigaone.c       |  28 ++-----
 hw/ppc/pegasos.c        |  13 ---
 hw/xtensa/xtfpga.c      |   7 +-
 include/system/memory.h | 179 +++++++++++++++++++++++++++++++++++++++
 system/memory.c         | 181 ++++++++++++++++++++++++++++++++++++++++
 8 files changed, 407 insertions(+), 74 deletions(-)

-- 
2.41.3

[PATCH v3 1/6] memory: Add memory_region_new* functions

[BALATON Zoltan]

These are counterparts of similar memory_region_init* functions but
differ in that these allocate the memory region with object_new so the
memory region will be managed by QOM and freed with the owner. This
behaviour was already documented and the reference tracking is
implemented but it could not be used without these functions because
memory_region_init* functions call object_initialize that clears the
free function of the object so it would not be freed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 include/system/memory.h |  81 ++++++++++++++++++
 system/memory.c         | 181 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 262 insertions(+)

diff --git a/include/system/memory.h b/include/system/memory.h
index d4793a08a7..ab76433c54 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1324,6 +1324,10 @@ void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size);
 
+MemoryRegion *memory_region_new(Object *owner,
+                                const char *name,
+                                uint64_t size);
+
 /**
  * memory_region_ref: Add 1 to a memory region's reference count
  *
@@ -1374,6 +1378,12 @@ void memory_region_init_io(MemoryRegion *mr,
                            const char *name,
                            uint64_t size);
 
+MemoryRegion *memory_region_new_io(Object *owner,
+                                   const MemoryRegionOps *ops,
+                                   void *opaque,
+                                   const char *name,
+                                   uint64_t size);
+
 /**
  * memory_region_init_ram_flags_nomigrate:  Initialize RAM memory region.
  *                                          Accesses into the region will
@@ -1400,6 +1410,12 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
                                             uint32_t ram_flags,
                                             Error **errp);
 
+MemoryRegion *memory_region_new_ram_flags_nomigrate(Object *owner,
+                                                    const char *name,
+                                                    uint64_t size,
+                                                    uint32_t ram_flags,
+                                                    Error **errp);
+
 /**
  * memory_region_init_resizeable_ram:  Initialize memory region with resizable
  *                                     RAM.  Accesses into the region will
@@ -1432,6 +1448,16 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                                        uint64_t length,
                                                        void *host),
                                        Error **errp);
+
+MemoryRegion *memory_region_new_resizeable_ram(Object *owner,
+                                               const char *name,
+                                               uint64_t size,
+                                               uint64_t max_size,
+                                               void (*resized)(const char*,
+                                                               uint64_t length,
+                                                               void *host),
+                                               Error **errp);
+
 #ifdef CONFIG_POSIX
 
 /**
@@ -1467,6 +1493,15 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
                                       ram_addr_t offset,
                                       Error **errp);
 
+MemoryRegion *memory_region_new_ram_from_file(Object *owner,
+                                              const char *name,
+                                              uint64_t size,
+                                              uint64_t align,
+                                              uint32_t ram_flags,
+                                              const char *path,
+                                              ram_addr_t offset,
+                                              Error **errp);
+
 /**
  * memory_region_init_ram_from_fd:  Initialize RAM memory region with a
  *                                  mmap-ed backend.
@@ -1495,6 +1530,15 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
                                     int fd,
                                     ram_addr_t offset,
                                     Error **errp);
+
+MemoryRegion *memory_region_new_ram_from_fd(Object *owner,
+                                            const char *name,
+                                            uint64_t size,
+                                            uint32_t ram_flags,
+                                            int fd,
+                                            ram_addr_t offset,
+                                            Error **errp);
+
 #endif
 
 /**
@@ -1518,6 +1562,11 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
                                 uint64_t size,
                                 void *ptr);
 
+MemoryRegion *memory_region_new_ram_ptr(Object *owner,
+                                        const char *name,
+                                        uint64_t size,
+                                        void *ptr);
+
 /**
  * memory_region_init_ram_device_ptr:  Initialize RAM device memory region from
  *                                     a user-provided pointer.
@@ -1546,6 +1595,11 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
                                        uint64_t size,
                                        void *ptr);
 
+MemoryRegion *memory_region_new_ram_device_ptr(Object *owner,
+                                               const char *name,
+                                               uint64_t size,
+                                               void *ptr);
+
 /**
  * memory_region_init_alias: Initialize a memory region that aliases all or a
  *                           part of another memory region.
@@ -1565,6 +1619,12 @@ void memory_region_init_alias(MemoryRegion *mr,
                               hwaddr offset,
                               uint64_t size);
 
+MemoryRegion *memory_region_new_alias(Object *owner,
+                                      const char *name,
+                                      MemoryRegion *orig,
+                                      hwaddr offset,
+                                      uint64_t size);
+
 /**
  * memory_region_init_iommu: Initialize a memory region of a custom type
  * that translates addresses
@@ -1630,6 +1690,16 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
                                         uint64_t size,
                                         Error **errp);
 
+MemoryRegion *memory_region_new_ram(Object *owner,
+                                    const char *name,
+                                    uint64_t size,
+                                    Error **errp);
+
+MemoryRegion *memory_region_new_ram_guest_memfd(Object *owner,
+                                                const char *name,
+                                                uint64_t size,
+                                                Error **errp);
+
 /**
  * memory_region_init_rom: Initialize a ROM memory region.
  *
@@ -1659,6 +1729,11 @@ bool memory_region_init_rom(MemoryRegion *mr,
                             uint64_t size,
                             Error **errp);
 
+MemoryRegion *memory_region_new_rom(Object *owner,
+                                    const char *name,
+                                    uint64_t size,
+                                    Error **errp);
+
 /**
  * memory_region_init_rom_device:  Initialize a ROM memory region.
  *                                 Writes are handled via callbacks.
@@ -1694,6 +1769,12 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
                                    uint64_t size,
                                    Error **errp);
 
+MemoryRegion *memory_region_new_rom_device(Object *owner,
+                                           const MemoryRegionOps *ops,
+                                           void *opaque,
+                                           const char *name,
+                                           uint64_t size,
+                                           Error **errp);
 
 /**
  * memory_region_owner: get a memory region's owner.
diff --git a/system/memory.c b/system/memory.c
index fa3e19e1fd..d125e8102b 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1251,6 +1251,15 @@ void memory_region_init(MemoryRegion *mr,
     memory_region_do_init(mr, owner, name, size);
 }
 
+MemoryRegion *memory_region_new(Object *owner, const char *name, uint64_t size)
+{
+    MemoryRegion *mr = MEMORY_REGION(object_new(TYPE_MEMORY_REGION));
+
+    assert(name); /* mr is attached to owner by name */
+    memory_region_do_init(mr, owner, name, size);
+    return mr;
+}
+
 static void memory_region_get_container(Object *obj, Visitor *v,
                                         const char *name, void *opaque,
                                         Error **errp)
@@ -1576,6 +1585,16 @@ void memory_region_init_io(MemoryRegion *mr, Object *owner,
     memory_region_set_ops(mr, ops, opaque);
 }
 
+MemoryRegion *memory_region_new_io(Object *owner,
+                                   const MemoryRegionOps *ops, void *opaque,
+                                   const char *name, uint64_t size)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+
+    memory_region_set_ops(mr, ops, opaque);
+    return mr;
+}
+
 static void memory_region_setup_ram(MemoryRegion *mr)
 {
     mr->ram = true;
@@ -1607,6 +1626,20 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
     return memory_region_error_propagate(mr, err, errp);
 }
 
+MemoryRegion *memory_region_new_ram_flags_nomigrate(Object *owner,
+                                                    const char *name,
+                                                    uint64_t size,
+                                                    uint32_t ram_flags,
+                                                    Error **errp)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+    Error *err = NULL;
+
+    memory_region_setup_ram(mr);
+    mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+    return memory_region_error_propagate(mr, err, errp) ? mr : NULL;
+}
+
 bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                        Object *owner,
                                        const char *name,
@@ -1626,6 +1659,24 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
     return memory_region_error_propagate(mr, err, errp);
 }
 
+MemoryRegion *memory_region_new_resizeable_ram(Object *owner,
+                                               const char *name,
+                                               uint64_t size,
+                                               uint64_t max_size,
+                                               void (*resized)(const char*,
+                                                               uint64_t length,
+                                                               void *host),
+                                               Error **errp)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+    Error *err = NULL;
+
+    memory_region_setup_ram(mr);
+    mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
+                                              &err);
+    return memory_region_error_propagate(mr, err, errp) ? mr : NULL;
+}
+
 #if defined(CONFIG_POSIX) && !defined(EMSCRIPTEN)
 bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
                                       const char *name, uint64_t size,
@@ -1644,6 +1695,26 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
     return memory_region_error_propagate(mr, err, errp);
 }
 
+MemoryRegion *memory_region_new_ram_from_file(Object *owner,
+                                              const char *name,
+                                              uint64_t size,
+                                              uint64_t align,
+                                              uint32_t ram_flags,
+                                              const char *path,
+                                              ram_addr_t offset,
+                                              Error **errp)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+    Error *err = NULL;
+
+    memory_region_setup_ram(mr);
+    mr->readonly = !!(ram_flags & RAM_READONLY);
+    mr->align = align;
+    mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset,
+                                             &err);
+    return memory_region_error_propagate(mr, err, errp) ? mr : NULL;
+}
+
 bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
                                     const char *name, uint64_t size,
                                     uint32_t ram_flags, int fd,
@@ -1658,6 +1729,21 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
                                            offset, false, &err);
     return memory_region_error_propagate(mr, err, errp);
 }
+
+MemoryRegion *memory_region_new_ram_from_fd(Object *owner,
+                                            const char *name, uint64_t size,
+                                            uint32_t ram_flags, int fd,
+                                            ram_addr_t offset, Error **errp)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+    Error *err = NULL;
+
+    memory_region_setup_ram(mr);
+    mr->readonly = !!(ram_flags & RAM_READONLY);
+    mr->ram_block = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
+                                           offset, false, &err);
+    return memory_region_error_propagate(mr, err, errp) ? mr : NULL;
+}
 #endif
 
 static void memory_region_setup_ram_ptr(MemoryRegion *mr, uint64_t size,
@@ -1676,6 +1762,15 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
     memory_region_setup_ram_ptr(mr, size, ptr);
 }
 
+MemoryRegion *memory_region_new_ram_ptr(Object *owner, const char *name,
+                                        uint64_t size, void *ptr)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+
+    memory_region_setup_ram_ptr(mr, size, ptr);
+    return mr;
+}
+
 void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
                                        const char *name, uint64_t size,
                                        void *ptr)
@@ -1685,6 +1780,17 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
     mr->ram_device = true;
 }
 
+MemoryRegion *memory_region_new_ram_device_ptr(Object *owner, const char *name,
+                                               uint64_t size, void *ptr)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+
+    memory_region_set_ops(mr, &ram_device_mem_ops, mr);
+    memory_region_setup_ram_ptr(mr, size, ptr);
+    mr->ram_device = true;
+    return mr;
+}
+
 void memory_region_init_alias(MemoryRegion *mr, Object *owner,
                               const char *name, MemoryRegion *orig,
                               hwaddr offset, uint64_t size)
@@ -1694,6 +1800,17 @@ void memory_region_init_alias(MemoryRegion *mr, Object *owner,
     mr->alias_offset = offset;
 }
 
+MemoryRegion *memory_region_new_alias(Object *owner,
+                                      const char *name, MemoryRegion *orig,
+                                      hwaddr offset, uint64_t size)
+{
+    MemoryRegion *mr = memory_region_new(owner, name, size);
+
+    mr->alias = orig;
+    mr->alias_offset = offset;
+    return mr;
+}
+
 void memory_region_init_iommu(void *_iommu_mr,
                               size_t instance_size,
                               const char *mrtypename,
@@ -3669,6 +3786,19 @@ bool memory_region_init_ram(MemoryRegion *mr, Object *owner,
     return true;
 }
 
+MemoryRegion *memory_region_new_ram(Object *owner,
+                                    const char *name, uint64_t size,
+                                    Error **errp)
+{
+    MemoryRegion *mr;
+
+    mr = memory_region_new_ram_flags_nomigrate(owner, name, size, 0, errp);
+    if (mr) {
+        memory_region_register_ram(mr, owner);
+    }
+    return mr;
+}
+
 bool memory_region_init_ram_guest_memfd(MemoryRegion *mr, Object *owner,
                                         const char *name, uint64_t size,
                                         Error **errp)
@@ -3681,6 +3811,21 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr, Object *owner,
     return true;
 }
 
+MemoryRegion *memory_region_new_ram_guest_memfd(Object *owner,
+                                                const char *name,
+                                                uint64_t size,
+                                                Error **errp)
+{
+    MemoryRegion *mr;
+
+    mr = memory_region_new_ram_flags_nomigrate(owner, name, size,
+                                               RAM_GUEST_MEMFD, errp);
+    if (mr) {
+        memory_region_register_ram(mr, owner);
+    }
+    return mr;
+}
+
 bool memory_region_init_rom(MemoryRegion *mr, Object *owner,
                             const char *name, uint64_t size,
                             Error **errp)
@@ -3694,6 +3839,20 @@ bool memory_region_init_rom(MemoryRegion *mr, Object *owner,
     return true;
 }
 
+MemoryRegion *memory_region_new_rom(Object *owner,
+                                    const char *name, uint64_t size,
+                                    Error **errp)
+{
+    MemoryRegion *mr;
+
+    mr = memory_region_new_ram_flags_nomigrate(owner, name, size, 0, errp);
+    if (mr) {
+        mr->readonly = true;
+        memory_region_register_ram(mr, owner);
+    }
+    return mr;
+}
+
 bool memory_region_init_rom_device(MemoryRegion *mr, Object *owner,
                                    const MemoryRegionOps *ops, void *opaque,
                                    const char *name, uint64_t size,
@@ -3714,6 +3873,28 @@ bool memory_region_init_rom_device(MemoryRegion *mr, Object *owner,
     return true;
 }
 
+MemoryRegion *memory_region_new_rom_device(Object *owner,
+                                           const MemoryRegionOps *ops,
+                                           void *opaque,
+                                           const char *name, uint64_t size,
+                                           Error **errp)
+{
+    MemoryRegion *mr;
+    Error *err = NULL;
+
+    assert(ops);
+    mr = memory_region_new_io(owner, ops, opaque, name, size);
+    memory_region_setup_ram(mr);
+    mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
+    if (!memory_region_error_propagate(mr, err, errp)) {
+        return NULL;
+    }
+    mr->ram = false;
+    mr->rom_device = true;
+    memory_region_register_ram(mr, owner);
+    return mr;
+}
+
 /*
  * Support system builds with CONFIG_FUZZ using a weak symbol and a stub for
  * the fuzz_dma_read_cb callback
-- 
2.41.3

[PATCH v3 2/6] memory: Update documentation for memory_region_new*()

[BALATON Zoltan]

Clarify the difference between memory_region_new() and
memory_region_init() with regard to region lifecycle.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 docs/devel/memory.rst   | 21 ++++-----
 include/system/memory.h | 98 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 10 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 9083b18f08..988380c632 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -138,7 +138,8 @@ stability.
 Region lifecycle
 ----------------
 
-A region is created by one of the memory_region_init*() functions and
+A region is allocated by one of the memory_region_new*() functions or
+pre-allocated and initialized by memory_region_init*() functions and
 attached to an object, which acts as its owner or parent.  QEMU ensures
 that the owner object remains alive as long as the region is visible to
 the guest, or as long as the region is in use by a virtual CPU or another
@@ -154,16 +155,16 @@ 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.  When there are multiple memory regions under the same owner object,
-the memory API will guarantee all memory regions will be properly detached
-and finalized one by one.  The order in which memory regions will be
-finalized is not guaranteed.
+Destruction of a memory region allocated with memory_region_new*() functions
+happens automatically when the owner object dies.  When there are multiple
+memory regions under the same owner object, the memory API will guarantee all
+memory regions will be properly detached and finalized one by one.  The order
+in which memory regions will be finalized is not guaranteed.
 
-If however the memory region is part of a dynamically allocated data
-structure, you should free the memory region in the instance_finalize
-callback.  For an example see VFIOMSIXInfo and VFIOQuirk in
-hw/vfio/pci.c.
+If however the memory region is part of a separately allocated data structure
+and initialized with one of the memory_region_init*() functions, you may have
+to free the memory region e.g. in an instance_finalize callback.  For an
+example see VFIOMSIXInfo and VFIOQuirk in hw/vfio/pci.c.
 
 You must not destroy a memory region as long as it may be in use by a
 device or CPU.  In order to do this, as a general rule do not create or
diff --git a/include/system/memory.h b/include/system/memory.h
index ab76433c54..017c290f27 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1324,6 +1324,13 @@ void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size);
 
+/**
+ * memory_region_new: Allocate and initialize a memory region
+ *
+ * Like memory_region_init() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new(Object *owner,
                                 const char *name,
                                 uint64_t size);
@@ -1378,6 +1385,13 @@ void memory_region_init_io(MemoryRegion *mr,
                            const char *name,
                            uint64_t size);
 
+/**
+ * memory_region_new_io: Allocate and initialize an I/O memory region.
+ *
+ * Like memory_region_init_io() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_io(Object *owner,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
@@ -1410,6 +1424,15 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
                                             uint32_t ram_flags,
                                             Error **errp);
 
+/**
+ * memory_region_new_ram_flags_nomigrate:  Allocate and initialize RAM memory
+ *                                         region with flags.
+ *
+ * Like memory_region_init_ram_flags_nomigrate() but @mr is allocated and
+ * managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram_flags_nomigrate(Object *owner,
                                                     const char *name,
                                                     uint64_t size,
@@ -1449,6 +1472,15 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                                        void *host),
                                        Error **errp);
 
+/**
+ * memory_region_new_resizeable_ram:  Allocate and initialize memory region
+ *                                    with resizable RAM.
+ *
+ * Like memory_region_init_resizeable_ram() but @mr is allocated and managed
+ * by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_resizeable_ram(Object *owner,
                                                const char *name,
                                                uint64_t size,
@@ -1493,6 +1525,15 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr,
                                       ram_addr_t offset,
                                       Error **errp);
 
+/**
+ * memory_region_new_ram_from_file:  Allocate and initialize RAM memory region
+ *                                   with a mmap-ed backend.
+ *
+ * Like memory_region_init_ram_from_file() but @mr is allocated and managed
+ * by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram_from_file(Object *owner,
                                               const char *name,
                                               uint64_t size,
@@ -1531,6 +1572,15 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr,
                                     ram_addr_t offset,
                                     Error **errp);
 
+/**
+ * memory_region_new_ram_from_fd:  Allocate and initialize RAM memory region
+ *                                 with a mmap-ed backend.
+ *
+ * Like memory_region_init_ram_from_fd() but @mr is allocated and managed
+ * by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram_from_fd(Object *owner,
                                             const char *name,
                                             uint64_t size,
@@ -1562,6 +1612,14 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
                                 uint64_t size,
                                 void *ptr);
 
+/**
+ * memory_region_new_ram_ptr:  Allocate and initialize RAM memory region from a
+ *                             user-provided pointer.
+ *
+ * Like memory_region_init_ram_ptr() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram_ptr(Object *owner,
                                         const char *name,
                                         uint64_t size,
@@ -1595,6 +1653,15 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
                                        uint64_t size,
                                        void *ptr);
 
+/**
+ * memory_region_new_ram_device_ptr:  Allocate and initialize RAM device memory
+ *                                    region from a user-provided pointer.
+ *
+ * Like memory_region_init_ram_device_ptr() but @mr is allocated and managed
+ * by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram_device_ptr(Object *owner,
                                                const char *name,
                                                uint64_t size,
@@ -1619,6 +1686,14 @@ void memory_region_init_alias(MemoryRegion *mr,
                               hwaddr offset,
                               uint64_t size);
 
+/**
+ * memory_region_new_alias: Allocate and initialize a memory region that
+ *                          aliases all or a part of another memory region.
+ *
+ * Like memory_region_init_alias() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_alias(Object *owner,
                                       const char *name,
                                       MemoryRegion *orig,
@@ -1690,6 +1765,13 @@ bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
                                         uint64_t size,
                                         Error **errp);
 
+/**
+ * memory_region_new_ram: Allocate and initialize a RAM memory region.
+ *
+ * Like memory_region_init_ram() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_ram(Object *owner,
                                     const char *name,
                                     uint64_t size,
@@ -1729,6 +1811,13 @@ bool memory_region_init_rom(MemoryRegion *mr,
                             uint64_t size,
                             Error **errp);
 
+/**
+ * memory_region_new_rom: Allocate and initialize a ROM memory region.
+ *
+ * Like memory_region_init_rom() but @mr is allocated and managed by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_rom(Object *owner,
                                     const char *name,
                                     uint64_t size,
@@ -1769,6 +1858,15 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
                                    uint64_t size,
                                    Error **errp);
 
+/**
+ * memory_region_new_rom_device:  Allocate and initialize a ROM memory region.
+ *                                Writes are handled via callbacks.
+ *
+ * Like memory_region_init_rom_device() but @mr is allocated and managed
+ * by QOM.
+ *
+ * Return: Pointer to allocated #MemoryRegion.
+ */
 MemoryRegion *memory_region_new_rom_device(Object *owner,
                                            const MemoryRegionOps *ops,
                                            void *opaque,
-- 
2.41.3

[PATCH v3 3/6] hw/ide/sii3112: Use memory_region_new to avoid leaking regions

[BALATON Zoltan]

Memory regions created with memory_region_init are not freed with
their owner. Use memory_region_new instead to let QOM manage the
lifetime of the memory regions.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/sii3112.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 9b28c691fd..d2dcfc3830 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -31,7 +31,7 @@ typedef struct SiI3112Regs {
 
 struct SiI3112PCIState {
     PCIIDEState i;
-    MemoryRegion mmio;
+
     SiI3112Regs regs[2];
 };
 
@@ -249,39 +249,33 @@ static void sii3112_reset(DeviceState *dev)
 
 static void sii3112_pci_realize(PCIDevice *dev, Error **errp)
 {
-    SiI3112PCIState *d = SII3112_PCI(dev);
     PCIIDEState *s = PCI_IDE(dev);
     DeviceState *ds = DEVICE(dev);
-    MemoryRegion *mr;
-    int i;
+    Object *o = OBJECT(dev);
+    MemoryRegion *mmio, *mr;
 
     pci_config_set_interrupt_pin(dev->config, 1);
     pci_set_byte(dev->config + PCI_CACHE_LINE_SIZE, 8);
 
     /* BAR5 is in PCI memory space */
-    memory_region_init_io(&d->mmio, OBJECT(d), &sii3112_reg_ops, d,
-                         "sii3112.bar5", 0x200);
-    pci_register_bar(dev, 5, PCI_BASE_ADDRESS_SPACE_MEMORY, &d->mmio);
+    mmio = memory_region_new_io(o, &sii3112_reg_ops, SII3112_PCI(dev),
+                                "sii3112.bar5", 0x200);
+    pci_register_bar(dev, 5, PCI_BASE_ADDRESS_SPACE_MEMORY, mmio);
 
     /* BAR0-BAR4 are PCI I/O space aliases into BAR5 */
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar0", &d->mmio, 0x80, 8);
+    mr = memory_region_new_alias(o, "sii3112.bar0", mmio, 0x80, 8);
     pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar1", &d->mmio, 0x88, 4);
+    mr = memory_region_new_alias(o, "sii3112.bar1", mmio, 0x88, 4);
     pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar2", &d->mmio, 0xc0, 8);
+    mr = memory_region_new_alias(o, "sii3112.bar2", mmio, 0xc0, 8);
     pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar3", &d->mmio, 0xc8, 4);
+    mr = memory_region_new_alias(o, "sii3112.bar3", mmio, 0xc8, 4);
     pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(d), "sii3112.bar4", &d->mmio, 0, 16);
+    mr = memory_region_new_alias(o, "sii3112.bar4", mmio, 0, 16);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, mr);
 
     qdev_init_gpio_in(ds, sii3112_set_irq, 2);
-    for (i = 0; i < 2; i++) {
+    for (int i = 0; i < 2; i++) {
         ide_bus_init(&s->bus[i], sizeof(s->bus[i]), ds, i, 1);
         ide_bus_init_output_irq(&s->bus[i], qdev_get_gpio_in(ds, i));
 
-- 
2.41.3

[PATCH v3 4/6] hw/pci-host/articia: Map PCI memory windows in realize

[BALATON Zoltan]

These memory windows are a result of the address decoding in the
Articia S north bridge so better model it there and not in board code.

Suggested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/articia.c | 14 +++++++++++++-
 hw/ppc/amigaone.c     | 28 +++++-----------------------
 hw/ppc/pegasos.c      | 13 -------------
 3 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
index 04623dfd84..203e0646df 100644
--- a/hw/pci-host/articia.c
+++ b/hw/pci-host/articia.c
@@ -22,6 +22,11 @@
  * Most features are missing but those are not needed by firmware and guests.
  */
 
+#define PCI_HIGH_ADDR 0x80000000
+#define PCI_HIGH_SIZE 0x7d000000
+#define PCI_LOW_ADDR  0xfd000000
+#define PCI_LOW_SIZE   0x1000000
+
 OBJECT_DECLARE_SIMPLE_TYPE(ArticiaState, ARTICIA)
 
 OBJECT_DECLARE_SIMPLE_TYPE(ArticiaHostState, ARTICIA_PCI_HOST)
@@ -169,6 +174,7 @@ static void articia_realize(DeviceState *dev, Error **errp)
 {
     ArticiaState *s = ARTICIA(dev);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
+    MemoryRegion *mr;
     PCIDevice *pdev;
 
     bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
@@ -181,6 +187,13 @@ static void articia_realize(DeviceState *dev, Error **errp)
                           TYPE_ARTICIA, 0x1000000);
     memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
 
+    mr = memory_region_new_alias(OBJECT(dev), "pci-mem-low", &s->mem, 0,
+                                 PCI_LOW_SIZE);
+    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
+    mr = memory_region_new_alias(OBJECT(dev), "pci-mem-high", &s->mem,
+                                 PCI_HIGH_ADDR, PCI_HIGH_SIZE);
+    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
+
     /* devfn_min is 8 that matches first PCI slot in AmigaOne */
     h->bus = pci_register_root_bus(dev, NULL, articia_pcihost_set_irq,
                                    amigaone_pcihost_bus0_map_irq, dev, &s->mem,
@@ -191,7 +204,6 @@ static void articia_realize(DeviceState *dev, Error **errp)
     pci_create_simple(h->bus, PCI_DEVFN(0, 1), TYPE_ARTICIA_PCI_BRIDGE);
 
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->reg);
-    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->mem);
     qdev_init_gpio_out(dev, s->irq, ARRAY_SIZE(s->irq));
 }
 
diff --git a/hw/ppc/amigaone.c b/hw/ppc/amigaone.c
index 8074713fbe..17deacab44 100644
--- a/hw/ppc/amigaone.c
+++ b/hw/ppc/amigaone.c
@@ -35,13 +35,6 @@
 #define INITRD_MIN_ADDR 0x600000
 #define INIT_RAM_ADDR 0x40000000
 
-#define PCI_HIGH_ADDR 0x80000000
-#define PCI_HIGH_SIZE 0x7d000000
-#define PCI_LOW_ADDR  0xfd000000
-#define PCI_LOW_SIZE  0xe0000
-
-#define ARTICIA_ADDR 0xfe000000
-
 /*
  * Firmware binary available at
  * https://www.hyperion-entertainment.com/index.php/downloads?view=files&parent=28
@@ -267,7 +260,7 @@ static void amigaone_init(MachineState *machine)
 {
     PowerPCCPU *cpu;
     CPUPPCState *env;
-    MemoryRegion *rom, *pci_mem, *mr;
+    MemoryRegion *rom, *mr;
     ssize_t sz;
     PCIBus *pci_bus;
     Object *via;
@@ -308,8 +301,8 @@ static void amigaone_init(MachineState *machine)
         qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(di));
     }
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    memory_region_add_subregion(get_system_memory(), NVRAM_ADDR,
-                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0));
+    mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
+    memory_region_add_subregion_overlap(get_system_memory(), NVRAM_ADDR, mr, 1);
 
     /* allocate and load firmware */
     rom = g_new(MemoryRegion, 1);
@@ -329,8 +322,8 @@ static void amigaone_init(MachineState *machine)
     }
 
     /* Articia S */
-    dev = sysbus_create_simple(TYPE_ARTICIA, ARTICIA_ADDR, NULL);
-
+    dev = sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL);
+    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
     i2c_bus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));
     if (machine->ram_size > 512 * MiB) {
         spd_data = spd_data_generate(SDR, machine->ram_size / 2);
@@ -343,17 +336,6 @@ static void amigaone_init(MachineState *machine)
         smbus_eeprom_init_one(i2c_bus, 0x52, spd_data);
     }
 
-    pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 1);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-low", pci_mem,
-                             0, PCI_LOW_SIZE);
-    memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
-    mr = g_new(MemoryRegion, 1);
-    memory_region_init_alias(mr, OBJECT(dev), "pci-mem-high", pci_mem,
-                             PCI_HIGH_ADDR, PCI_HIGH_SIZE);
-    memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
-    pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
-
     /* VIA VT82c686B South Bridge (multifunction PCI device) */
     via = OBJECT(pci_create_simple_multifunction(pci_bus, PCI_DEVFN(7, 0),
                                                  TYPE_VT82C686B_ISA));
diff --git a/hw/ppc/pegasos.c b/hw/ppc/pegasos.c
index ac9fc5a654..e0e2e8815d 100644
--- a/hw/ppc/pegasos.c
+++ b/hw/ppc/pegasos.c
@@ -213,23 +213,10 @@ static void pegasos_init(MachineState *machine)
     /* north bridge */
     switch (pm->type) {
     case PEGASOS1:
-    {
-        MemoryRegion *pci_mem, *mr;
-
         /* Articia S */
         pm->nb = DEVICE(sysbus_create_simple(TYPE_ARTICIA, 0xfe000000, NULL));
-        pci_mem = sysbus_mmio_get_region(SYS_BUS_DEVICE(pm->nb), 1);
-        mr = g_new(MemoryRegion, 1);
-        memory_region_init_alias(mr, OBJECT(pm->nb), "pci-mem-low", pci_mem,
-                                 0, 0x1000000);
-        memory_region_add_subregion(get_system_memory(), 0xfd000000, mr);
-        mr = g_new(MemoryRegion, 1);
-        memory_region_init_alias(mr, OBJECT(pm->nb), "pci-mem-high", pci_mem,
-                                 0x80000000, 0x7d000000);
-        memory_region_add_subregion(get_system_memory(), 0x80000000, mr);
         pci_bus = PCI_BUS(qdev_get_child_bus(pm->nb, "pci.0"));
         break;
-    }
     case PEGASOS2:
         /* Marvell Discovery II system controller */
         pm->nb = DEVICE(sysbus_create_simple(TYPE_MV64361, -1,
-- 
2.41.3

[PATCH v3 5/6] hw/pci-host/articia: Add variable for common type cast

[BALATON Zoltan]

We need the device casted to OBJECT often enough in realize to store
it in a local variable that also makes function calls more readable.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/pci-host/articia.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/pci-host/articia.c b/hw/pci-host/articia.c
index 203e0646df..d7676a166f 100644
--- a/hw/pci-host/articia.c
+++ b/hw/pci-host/articia.c
@@ -174,24 +174,24 @@ static void articia_realize(DeviceState *dev, Error **errp)
 {
     ArticiaState *s = ARTICIA(dev);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
+    Object *o = OBJECT(dev);
     MemoryRegion *mr;
     PCIDevice *pdev;
 
     bitbang_i2c_init(&s->smbus, i2c_init_bus(dev, "smbus"));
-    memory_region_init_io(&s->gpio_reg, OBJECT(s), &articia_gpio_ops, s,
+    memory_region_init_io(&s->gpio_reg, o, &articia_gpio_ops, s,
                           TYPE_ARTICIA, 4);
 
-    memory_region_init(&s->mem, OBJECT(dev), "pci-mem", UINT64_MAX);
-    memory_region_init(&s->io, OBJECT(dev), "pci-io", 0xc00000);
-    memory_region_init_io(&s->reg, OBJECT(s), &articia_reg_ops, s,
+    memory_region_init(&s->mem, o, "pci-mem", UINT64_MAX);
+    memory_region_init(&s->io, o, "pci-io", 0xc00000);
+    memory_region_init_io(&s->reg, o, &articia_reg_ops, s,
                           TYPE_ARTICIA, 0x1000000);
     memory_region_add_subregion_overlap(&s->reg, 0, &s->io, 1);
 
-    mr = memory_region_new_alias(OBJECT(dev), "pci-mem-low", &s->mem, 0,
-                                 PCI_LOW_SIZE);
+    mr = memory_region_new_alias(o, "pci-mem-low", &s->mem, 0, PCI_LOW_SIZE);
     memory_region_add_subregion(get_system_memory(), PCI_LOW_ADDR, mr);
-    mr = memory_region_new_alias(OBJECT(dev), "pci-mem-high", &s->mem,
-                                 PCI_HIGH_ADDR, PCI_HIGH_SIZE);
+    mr = memory_region_new_alias(o, "pci-mem-high", &s->mem, PCI_HIGH_ADDR,
+                                 PCI_HIGH_SIZE);
     memory_region_add_subregion(get_system_memory(), PCI_HIGH_ADDR, mr);
 
     /* devfn_min is 8 that matches first PCI slot in AmigaOne */
-- 
2.41.3

[PATCH v3 6/6] hw/xtensa/xtfpga: Fix leaking memory region

[BALATON Zoltan]

Use memory_region_new_ram to avoid leaking memory region. This would
be a migration break but the machines using this don't seem to support
migration and even if they did we make no compatibility guarantee for
these machines.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/xtensa/xtfpga.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index b025cc53a8..b8ae1994d8 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -50,7 +50,6 @@
 #include "xtensa_memory.h"
 #include "hw/xtensa/mx_pic.h"
 #include "exec/cpu-common.h"
-#include "migration/vmstate.h"
 
 typedef struct XtfpgaFlashDesc {
     hwaddr base;
@@ -162,10 +161,8 @@ static void xtfpga_net_init(MemoryRegion *address_space,
     memory_region_add_subregion(address_space, descriptors,
             sysbus_mmio_get_region(s, 1));
 
-    ram = g_malloc(sizeof(*ram));
-    memory_region_init_ram_flags_nomigrate(ram, OBJECT(s), "open_eth.ram",
-                                           16 * KiB, 0, &error_fatal);
-    vmstate_register_ram_global(ram);
+    ram = memory_region_new_ram(OBJECT(s), "open_eth.ram", 16 * KiB,
+                                &error_fatal);
     memory_region_add_subregion(address_space, buffers, ram);
 }
 
-- 
2.41.3

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

On Tue, 10 Feb 2026, BALATON Zoltan wrote:
> Our documentation says that memory regions are automatically freed
> when the owner dies and the reference counting to do this is also
> implemented. However this relies on the QOM free funtion that can only
> be set by creating objects with object_new but memory API only
> provides constructors that call object_initialize which clears the
> free function that prevents QOM to manage the memory region lifetime.
> Implement corresponding memory_region_new_* functions that do the same
> as the memory_region_init_* functions but create the memory region
> with object_new so the lifetime can be automatically managed by QOM as
> documented. The memory_region_init functions are kept because they are
> useful for memory regions embedded in other object or managed
> externally and not by QOM for some reason.
>
> v3:
> Based-on: <cover.1770753117.git.balaton@eik.bme.hu>

I mean
Based-on: <cover.1770253186.git.balaton@eik.bme.hu>
[PATCH v4 0/8] memory: Remove most _nomigrate variants
series.

> - convert xtfpga from nomigrate as this has no migration compatibility
> issue
>
> v2:
> - rebase on master
> - update documentation
> - use these function to fix some leaks (there may be more, e.g. in
> hw/pci-host/bonito but I leave that for later and/or others)
>
> BALATON Zoltan (6):
>  memory: Add memory_region_new* functions
>  memory: Update documentation for memory_region_new*()
>  hw/ide/sii3112: Use memory_region_new to avoid leaking regions
>  hw/pci-host/articia: Map PCI memory windows in realize
>  hw/pci-host/articia: Add variable for common type cast
>  hw/xtensa/xtfpga: Fix leaking memory region
>
> docs/devel/memory.rst   |  21 ++---
> hw/ide/sii3112.c        |  30 +++----
> hw/pci-host/articia.c   |  22 +++--
> hw/ppc/amigaone.c       |  28 ++-----
> hw/ppc/pegasos.c        |  13 ---
> hw/xtensa/xtfpga.c      |   7 +-
> include/system/memory.h | 179 +++++++++++++++++++++++++++++++++++++++
> system/memory.c         | 181 ++++++++++++++++++++++++++++++++++++++++
> 8 files changed, 407 insertions(+), 74 deletions(-)
>
>

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2099 bytes --]

Il mar 10 feb 2026, 21:02 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:

> Our documentation says that memory regions are automatically freed
> when the owner dies and the reference counting to do this is also
> implemented. However this relies on the QOM free funtion that can only
> be set by creating objects with object_new but memory API only
> provides constructors that call object_initialize which clears the
> free function that prevents QOM to manage the memory region lifetime.
> Implement corresponding memory_region_new_* functions that do the same
> as the memory_region_init_* functions but create the memory region
> with object_new so the lifetime can be automatically managed by QOM as
> documented. The memory_region_init functions are kept because they are
> useful for memory regions embedded in other object or managed
> externally and not by QOM for some reason.
>

I think there was pretty much agreement *not* to do this?

Paolo

v3:
> Based-on: <cover.1770753117.git.balaton@eik.bme.hu>
> - convert xtfpga from nomigrate as this has no migration compatibility
> issue
>
> v2:
> - rebase on master
> - update documentation
> - use these function to fix some leaks (there may be more, e.g. in
> hw/pci-host/bonito but I leave that for later and/or others)
>
> BALATON Zoltan (6):
>   memory: Add memory_region_new* functions
>   memory: Update documentation for memory_region_new*()
>   hw/ide/sii3112: Use memory_region_new to avoid leaking regions
>   hw/pci-host/articia: Map PCI memory windows in realize
>   hw/pci-host/articia: Add variable for common type cast
>   hw/xtensa/xtfpga: Fix leaking memory region
>
>  docs/devel/memory.rst   |  21 ++---
>  hw/ide/sii3112.c        |  30 +++----
>  hw/pci-host/articia.c   |  22 +++--
>  hw/ppc/amigaone.c       |  28 ++-----
>  hw/ppc/pegasos.c        |  13 ---
>  hw/xtensa/xtfpga.c      |   7 +-
>  include/system/memory.h | 179 +++++++++++++++++++++++++++++++++++++++
>  system/memory.c         | 181 ++++++++++++++++++++++++++++++++++++++++
>  8 files changed, 407 insertions(+), 74 deletions(-)
>
> --
> 2.41.3
>
>

[-- Attachment #2: Type: text/html, Size: 3025 bytes --]

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

On Wed, 11 Feb 2026, Paolo Bonzini wrote:
> Il mar 10 feb 2026, 21:02 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>
>> Our documentation says that memory regions are automatically freed
>> when the owner dies and the reference counting to do this is also
>> implemented. However this relies on the QOM free funtion that can only
>> be set by creating objects with object_new but memory API only
>> provides constructors that call object_initialize which clears the
>> free function that prevents QOM to manage the memory region lifetime.
>> Implement corresponding memory_region_new_* functions that do the same
>> as the memory_region_init_* functions but create the memory region
>> with object_new so the lifetime can be automatically managed by QOM as
>> documented. The memory_region_init functions are kept because they are
>> useful for memory regions embedded in other object or managed
>> externally and not by QOM for some reason.
>>
>
> I think there was pretty much agreement *not* to do this?

Who agreed on that? So far I got concerns that this was too big (that's 
why I split it from the clean ups to show it's not that big change, less 
than 200 lines plus the docs but the clean ups removed as many lines as 
the doc comments add) and others saying this should replace 
memory_region_init altogether (that's way too much churn and not needed 
IMO) but no clear explanation of why this was *not* needed. You as 
maintainer can say no but then please give a justification for why and 
some solution to the problems this tries to fix.

Regards,
BALATON Zoltan

> Paolo
>
> v3:
>> Based-on: <cover.1770753117.git.balaton@eik.bme.hu>
>> - convert xtfpga from nomigrate as this has no migration compatibility
>> issue
>>
>> v2:
>> - rebase on master
>> - update documentation
>> - use these function to fix some leaks (there may be more, e.g. in
>> hw/pci-host/bonito but I leave that for later and/or others)
>>
>> BALATON Zoltan (6):
>>   memory: Add memory_region_new* functions
>>   memory: Update documentation for memory_region_new*()
>>   hw/ide/sii3112: Use memory_region_new to avoid leaking regions
>>   hw/pci-host/articia: Map PCI memory windows in realize
>>   hw/pci-host/articia: Add variable for common type cast
>>   hw/xtensa/xtfpga: Fix leaking memory region
>>
>>  docs/devel/memory.rst   |  21 ++---
>>  hw/ide/sii3112.c        |  30 +++----
>>  hw/pci-host/articia.c   |  22 +++--
>>  hw/ppc/amigaone.c       |  28 ++-----
>>  hw/ppc/pegasos.c        |  13 ---
>>  hw/xtensa/xtfpga.c      |   7 +-
>>  include/system/memory.h | 179 +++++++++++++++++++++++++++++++++++++++
>>  system/memory.c         | 181 ++++++++++++++++++++++++++++++++++++++++
>>  8 files changed, 407 insertions(+), 74 deletions(-)
>>
>> --
>> 2.41.3
>>
>>
>

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[Peter Xu]

On Wed, Feb 11, 2026 at 12:47:53PM +0100, BALATON Zoltan wrote:
> On Wed, 11 Feb 2026, Paolo Bonzini wrote:
> > Il mar 10 feb 2026, 21:02 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
> > 
> > > Our documentation says that memory regions are automatically freed
> > > when the owner dies and the reference counting to do this is also
> > > implemented. However this relies on the QOM free funtion that can only
> > > be set by creating objects with object_new but memory API only
> > > provides constructors that call object_initialize which clears the
> > > free function that prevents QOM to manage the memory region lifetime.
> > > Implement corresponding memory_region_new_* functions that do the same
> > > as the memory_region_init_* functions but create the memory region
> > > with object_new so the lifetime can be automatically managed by QOM as
> > > documented. The memory_region_init functions are kept because they are
> > > useful for memory regions embedded in other object or managed
> > > externally and not by QOM for some reason.
> > > 
> > 
> > I think there was pretty much agreement *not* to do this?
> 
> Who agreed on that? So far I got concerns that this was too big (that's why
> I split it from the clean ups to show it's not that big change, less than
> 200 lines plus the docs but the clean ups removed as many lines as the doc
> comments add) and others saying this should replace memory_region_init
> altogether (that's way too much churn and not needed IMO) but no clear
> explanation of why this was *not* needed. You as maintainer can say no but
> then please give a justification for why and some solution to the problems
> this tries to fix.

I didn't participate much on this discussion, because I don't think I know
well on this topic.. especially on the Rust implications, comparing to most
of others who should be more familiar with.

However, IMHO, what we really need here is a justification on "why this is
needed", instead of requesting maintainer to say "why it's not needed"..

The rational should be simple to me: by default, we change nothing.  We fix
bugs, but that justifies itself.  If we change something else, we need to
justify the change worthwhile (by the author of the proposal).  If we
change a lot of things, we need serious and strong justifications.

At least I didn't see any Rust mentioned in this cover letter explaining
why we need this change.  I also don't know if the "documentation" issue is
a major flaw to introduce these whole set of new APIs.  For example, could
we fix the doc instead if it is flawed?

Thanks,

-- 
Peter Xu

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

On Wed, 11 Feb 2026, Peter Xu wrote:
> On Wed, Feb 11, 2026 at 12:47:53PM +0100, BALATON Zoltan wrote:
>> On Wed, 11 Feb 2026, Paolo Bonzini wrote:
>>> Il mar 10 feb 2026, 21:02 BALATON Zoltan <balaton@eik.bme.hu> ha scritto:
>>>
>>>> Our documentation says that memory regions are automatically freed
>>>> when the owner dies and the reference counting to do this is also
>>>> implemented. However this relies on the QOM free funtion that can only
>>>> be set by creating objects with object_new but memory API only
>>>> provides constructors that call object_initialize which clears the
>>>> free function that prevents QOM to manage the memory region lifetime.
>>>> Implement corresponding memory_region_new_* functions that do the same
>>>> as the memory_region_init_* functions but create the memory region
>>>> with object_new so the lifetime can be automatically managed by QOM as
>>>> documented. The memory_region_init functions are kept because they are
>>>> useful for memory regions embedded in other object or managed
>>>> externally and not by QOM for some reason.
>>>>
>>>
>>> I think there was pretty much agreement *not* to do this?
>>
>> Who agreed on that? So far I got concerns that this was too big (that's why
>> I split it from the clean ups to show it's not that big change, less than
>> 200 lines plus the docs but the clean ups removed as many lines as the doc
>> comments add) and others saying this should replace memory_region_init
>> altogether (that's way too much churn and not needed IMO) but no clear
>> explanation of why this was *not* needed. You as maintainer can say no but
>> then please give a justification for why and some solution to the problems
>> this tries to fix.
>
> I didn't participate much on this discussion, because I don't think I know
> well on this topic.. especially on the Rust implications, comparing to most
> of others who should be more familiar with.
>
> However, IMHO, what we really need here is a justification on "why this is
> needed", instead of requesting maintainer to say "why it's not needed"..
>
> The rational should be simple to me: by default, we change nothing.  We fix
> bugs, but that justifies itself.  If we change something else, we need to
> justify the change worthwhile (by the author of the proposal).  If we
> change a lot of things, we need serious and strong justifications.

When somebody submits a patch, the cover letter and commit messages 
describe what and why is prpoposed to be changed. I tried to do that and 
also replied to review comments trying to explain why I think the change 
would help. So I think I gave justification. If I get a reply saying that 
the proposed change is broken, won't work or breaks something then it's 
clear why it's not a good idea. If I get a reply that this would work but 
I don't think it's a good idea then I'd like to also get some explanation 
on why it's not a good idea and if there's a better solution for the 
problems that the poropsed patch tries to fix. That's where the maintainer 
should say why it's not needed.

I try to summarise the problem again briefly as it seems I did not manage 
to explain it in previous messages (or I wrote too much and people did not 
even read it). This again got a bit long but I don't know how to explain 
it better with less words.

Memory regions are registered as a child with an owner and when the owner 
is destroyed it unparents the memory region which decreases its ref count 
so it would be freed if there are no other uses of the memory region. This 
is implemented and documented like that but there's currently no way to 
actually create a memory region that would be managed and freed by QOM 
that way and a memory region created with memory_region_init would never 
be freed and the code to manage memory region lifecycle is effectively 
dead. Because of this we have all sorts of hacks to solve problems that 
arise from this such as memory_region_ref/unref not using the ref count of 
the memory region itself but the ref count of the owner and we embed 
memory regions in devices to avoid leaking them.

My immediate motivation was trying to keep device models simple and avoid 
any additional state or boiler plate code as much as possible because I 
think simple device models are easier to write and easier to find bugs in 
if boiler plate code is not obscuring the actual device functionality. 
Therefore the less additional lines we have to add to a device model is 
the better. A typical example is a memory region of a sysbus or PCI device 
where this would be created with the device and registered via 
sysbus_init_mmio or pci_register_bar. The SysBusDevice state has 
mmio[QDEV_MAX_MMIO] with a MemoryRegion *memory pointer in it and 
PCIDevice has PCIIORegion io_regions[PCI_NUM_REGIONS] where PCIIORegion 
has MemoryRegion *memory so the subclass should not need to store or embed 
these memory regions as QOM should be able to store and manage it. All 
that should be needed in the device model is to define memory ops, create 
the memory region and register it with the superclass and forget about it. 
Reading the documentation of memory regions I was under the impression it 
also works like that but when trying to create such memory regions I was 
told this would leak. Then I looked at why.

I've found that QOM lifecycle management relies on a free function that is 
set by object_new so anything not created with object_new won't be freed. 
QOM has two ways to create objects: object_new and object_initialize 
(latter of which zeroes the free function so this will never be freed by 
QOM) and there's no way to set a free function any other way. The 
memory_region_init* functions call object_initialze so they cannot create 
regions that QOM will manage. To understand this better look at: 
system/memory.c::memory_region_init(), qom/object.c::object_finalize(), 
qom/object.c::object_new() and qom/object.c::object_initialize_with_type() 
(note the memset in object_initialize_with_type).

There are two possible ways to fix this: remove object_initialize from 
memory_region_init so callers could do object_new; memory_region_init or 
object_initialize; memory_region_init or add corresponding 
memory_region_new functions. The latter seems to be less churn and more 
convenient to use so that's what this series does.

With this series we can have

struct SiI3112PCIState {
     PCIIDEState i;

     SiI3112Regs regs[2];
};

instead of

struct SiI3112PCIState {
     PCIIDEState i;

     MemoryRegion mmio;
     MemoryRegion bar0;
     MemoryRegion bar1;
     MemoryRegion bar2;
     MemoryRegion bar3;
     MemoryRegion bar4;
     SiI3112Regs regs[2];
};

or drop an unneeded field from PCIHostState as proposed here:

https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/797587b52159d2db79ed924b4962be7e7ba84207.1761232472.git.balaton@eik.bme.hu/

and several similar simplifications in a lot of devices, that would be 
done as follow up patches not all in this series.

We may also be able to untangle memory_region_ref/unref and just use 
object_ref/unref with memory regions that would be more regular QOM usage 
and may be simpler to follow than the current way of piggy-backing on the 
owner which recently was also found to have some issues.

> At least I didn't see any Rust mentioned in this cover letter explaining
> why we need this change.  I also don't know if the "documentation" issue is
> a major flaw to introduce these whole set of new APIs.  For example, could
> we fix the doc instead if it is flawed?

I don't know if this helps Rust or not, that wasn't brought up by me but I 
guess making memory regions normally managed QOM objects would fit more 
with what Rust expects or can handle than the current way of embedding or 
strangely ref counting regions. As described above the main motivation is 
not to fix documentation but to make it possible to use memory regions as 
documented and intended to avoid some hacks that I think arise from not 
being able to create normal ref counted memory regions that would be freed 
by QOM.

If you think the above is not a problem or this is not the right solution 
for it then I'd like to see a justification on why and what would be a 
better solution. So far I got that it might be that people did not get why 
it is a problem and it works good enough as it is so best not touch it but 
I think the porposed change is simple enough and would allow removing some 
hacks so allow simplifying code and as it does not touch existing 
memory_region_init functions it would also not break anything. Again if 
this was not clear above the series does not add any new way of managing 
memory regions, that's already implemented and always was, this just adds 
the functions that allow it to be used.

Regards,
BALATON Zoltan

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

On Thu, 12 Feb 2026, BALATON Zoltan wrote:
> Memory regions are registered as a child with an owner and when the owner is 
> destroyed it unparents the memory region which decreases its ref count so it 
> would be freed if there are no other uses of the memory region. This is 
> implemented and documented like that but there's currently no way to actually 
> create a memory region that would be managed and freed by QOM that way and a 
> memory region created with memory_region_init would never be freed and the 
> code to manage memory region lifecycle is effectively dead. Because of this 
> we have all sorts of hacks to solve problems that arise from this such as 
> memory_region_ref/unref not using the ref count of the memory region itself 
> but the ref count of the owner and we embed memory regions in devices to 
> avoid leaking them.

This is an example of hacks that I think arise from not being able to 
create a memory region that is freed with its parent:

https://lists.nongnu.org/archive/html/qemu-devel/2026-02/msg03876.html

This code does a few strange things meddling with internals of memory 
region trying to handle what the ref counts should already take care of. 
It also abuses the free function for something that I'd expect to be done 
in a finalize or unrealize method. I think this could be simplified with 
QOM managed memory regions that this series allow even without untangling 
memory region ref count from the owner but I don't know that code so not 
trying to do a fix. With the current memory_region_init we need to add 
more hacks to make it work but that's both confusing and fragile.

> or drop an unneeded field from PCIHostState as proposed here:
>
> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/797587b52159d2db79ed924b4962be7e7ba84207.1761232472.git.balaton@eik.bme.hu/

Another point I'd like to clear up here. For the linked patch I was told 
to do this:

struct PREPPCIState {
     PCIHostState parent_obj;

     MemoryRegion mmfg;
}

static void raven_pcihost_init(Object *obj)
{
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);

     memory_region_init_io(&s->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
                           "pci-mmcfg", 0x00400000);
}

static void raven_pcihost_realize(DeviceState *d, Error **errp)
{
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);

     memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);
}

instead of what I think should also work and should be enough:

static void raven_pcihost_realize(DeviceState *d, Error **errp)
{
     PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
     PCIHostState *h = PCI_HOST_BRIDGE(dev);
     MemoryRegion *mr;

     mr = memory_region_new_io(OBJECT(s), &raven_mmcfg_ops, h->bus,
                               "pci-mmcfg", 0x00400000);
     memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);
}

When asking why and if that's really necessary I got referred to some 
obscure conventions that may or may not exist but not documented anywhere 
or just because that's a more canonical way to do this. But is there 
really such convention or any reason to do it the hard way instead of 
allowing simpler device models? I'm all for simplifying devices and be 
able to do it with just the two lines in my second example which works as 
demonstrated by my prep clean up series but was not accepted for reasons I 
could not quite understand. So could this be cleared up too and if there 
are reasons for the first way then explain and document that somewhere?

Regards,
BALATON Zoltan

Re: [PATCH v3 0/6] Implement memory_region_new_* functions

[BALATON Zoltan]

On Sat, 14 Feb 2026, BALATON Zoltan wrote:
> On Thu, 12 Feb 2026, BALATON Zoltan wrote:
>> Memory regions are registered as a child with an owner and when the owner 
>> is destroyed it unparents the memory region which decreases its ref count 
>> so it would be freed if there are no other uses of the memory region. This 
>> is implemented and documented like that but there's currently no way to 
>> actually create a memory region that would be managed and freed by QOM that 
>> way and a memory region created with memory_region_init would never be 
>> freed and the code to manage memory region lifecycle is effectively dead. 
>> Because of this we have all sorts of hacks to solve problems that arise 
>> from this such as memory_region_ref/unref not using the ref count of the 
>> memory region itself but the ref count of the owner and we embed memory 
>> regions in devices to avoid leaking them.
>
> This is an example of hacks that I think arise from not being able to create 
> a memory region that is freed with its parent:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2026-02/msg03876.html
>
> This code does a few strange things meddling with internals of memory region 
> trying to handle what the ref counts should already take care of. It also 
> abuses the free function for something that I'd expect to be done in a 
> finalize or unrealize method. I think this could be simplified with QOM 
> managed memory regions that this series allow even without untangling memory 
> region ref count from the owner but I don't know that code so not trying to 
> do a fix. With the current memory_region_init we need to add more hacks to 
> make it work but that's both confusing and fragile.
>
>> or drop an unneeded field from PCIHostState as proposed here:
>> 
>> https://patchew.org/QEMU/cover.1761232472.git.balaton@eik.bme.hu/797587b52159d2db79ed924b4962be7e7ba84207.1761232472.git.balaton@eik.bme.hu/
>
> Another point I'd like to clear up here. For the linked patch I was told to 
> do this:
>
> struct PREPPCIState {
>    PCIHostState parent_obj;
>
>    MemoryRegion mmfg;
> }
>
> static void raven_pcihost_init(Object *obj)
> {
>    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
>    PCIHostState *h = PCI_HOST_BRIDGE(dev);
>
>    memory_region_init_io(&s->mmcfg, OBJECT(h), &raven_mmcfg_ops, h->bus,
>                          "pci-mmcfg", 0x00400000);
> }
>
> static void raven_pcihost_realize(DeviceState *d, Error **errp)
> {
>    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
>
>    memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);
> }
>
> instead of what I think should also work and should be enough:
>
> static void raven_pcihost_realize(DeviceState *d, Error **errp)
> {
>    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(dev);
>    PCIHostState *h = PCI_HOST_BRIDGE(dev);

dev above is d or obj

>    MemoryRegion *mr;
>
>    mr = memory_region_new_io(OBJECT(s), &raven_mmcfg_ops, h->bus,
>                              "pci-mmcfg", 0x00400000);
>    memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);

and s->mmcfg here is mr (just copy&pasted these) but you get the idea.

> }
>
> When asking why and if that's really necessary I got referred to some obscure 
> conventions that may or may not exist but not documented anywhere or just 
> because that's a more canonical way to do this. But is there really such 
> convention or any reason to do it the hard way instead of allowing simpler 
> device models? I'm all for simplifying devices and be able to do it with just 
> the two lines in my second example which works as demonstrated by my prep 
> clean up series but was not accepted for reasons I could not quite 
> understand. So could this be cleared up too and if there are reasons for the 
> first way then explain and document that somewhere?
>
> Regards,
> BALATON Zoltan
>
>