* [PATCH v2 01/10] memory: Add internal memory_region_set_ops helper function
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
@ 2026-01-25 17:50 ` BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 02/10] memory: Factor out common ram region initialization BALATON Zoltan
` (9 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:50 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
This is a common operation used at multiple places, add a helper
function for it.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
system/memory.c | 21 ++++++++++++---------
1 file changed, 12 insertions(+), 9 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 4bf00d82bc..86742557a1 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1559,6 +1559,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
}
}
+static void memory_region_set_ops(MemoryRegion *mr,
+ const MemoryRegionOps *ops,
+ void *opaque)
+{
+ mr->ops = ops ?: &unassigned_mem_ops;
+ mr->opaque = opaque;
+ mr->terminates = true;
+}
+
void memory_region_init_io(MemoryRegion *mr,
Object *owner,
const MemoryRegionOps *ops,
@@ -1567,9 +1576,7 @@ void memory_region_init_io(MemoryRegion *mr,
uint64_t size)
{
memory_region_init(mr, owner, name, size);
- mr->ops = ops ? ops : &unassigned_mem_ops;
- mr->opaque = opaque;
- mr->terminates = true;
+ memory_region_set_ops(mr, ops, opaque);
}
bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
@@ -1710,10 +1717,8 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr,
{
memory_region_init(mr, owner, name, size);
mr->ram = true;
- mr->terminates = true;
mr->ram_device = true;
- mr->ops = &ram_device_mem_ops;
- mr->opaque = mr;
+ memory_region_set_ops(mr, &ram_device_mem_ops, mr);
mr->destructor = memory_region_destructor_ram;
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
@@ -1759,9 +1764,7 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
Error *err = NULL;
assert(ops);
memory_region_init(mr, owner, name, size);
- mr->ops = ops;
- mr->opaque = opaque;
- mr->terminates = true;
+ memory_region_set_ops(mr, ops, opaque);
mr->rom_device = true;
mr->destructor = memory_region_destructor_ram;
mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 02/10] memory: Factor out common ram region initialization
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 01/10] memory: Add internal memory_region_set_ops helper function BALATON Zoltan
@ 2026-01-25 17:50 ` BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 03/10] memory: Factor out more " BALATON Zoltan
` (8 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:50 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
Introduce internal memory_region_do_init_ram() function to remove
duplicated code from different memory_region_init_*ram functions.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
system/memory.c | 147 +++++++++++++++++-------------------------------
1 file changed, 53 insertions(+), 94 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 86742557a1..7267333e11 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1579,29 +1579,12 @@ void memory_region_init_io(MemoryRegion *mr,
memory_region_set_ops(mr, ops, opaque);
}
-bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- Error **errp)
-{
- return memory_region_init_ram_flags_nomigrate(mr, owner, name,
- size, 0, errp);
-}
-
-bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- uint32_t ram_flags,
- Error **errp)
+static bool memory_region_do_init_ram(MemoryRegion *mr,
+ Error *err, Error **errp)
{
- Error *err = NULL;
- memory_region_init(mr, owner, name, size);
mr->ram = true;
mr->terminates = true;
mr->destructor = memory_region_destructor_ram;
- mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
if (err) {
mr->size = int128_zero();
object_unparent(OBJECT(mr));
@@ -1611,6 +1594,25 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
return true;
}
+bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ uint32_t ram_flags, Error **errp)
+{
+ Error *err = NULL;
+
+ memory_region_init(mr, owner, name, size);
+ mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+ return memory_region_do_init_ram(mr, err, errp);
+}
+
+bool memory_region_init_ram_nomigrate(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ Error **errp)
+{
+ return memory_region_init_ram_flags_nomigrate(mr, owner, name, size, 0,
+ errp);
+}
+
bool memory_region_init_resizeable_ram(MemoryRegion *mr,
Object *owner,
const char *name,
@@ -1622,108 +1624,66 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
Error **errp)
{
Error *err = NULL;
+
memory_region_init(mr, owner, name, size);
- mr->ram = true;
- mr->terminates = true;
- mr->destructor = memory_region_destructor_ram;
- mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized,
- mr, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
- return false;
- }
- return true;
+ mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
+ &err);
+ return memory_region_do_init_ram(mr, err, errp);
}
#if defined(CONFIG_POSIX) && !defined(EMSCRIPTEN)
-bool memory_region_init_ram_from_file(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- uint64_t align,
- uint32_t ram_flags,
- const char *path,
- ram_addr_t offset,
+bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ uint64_t align, uint32_t ram_flags,
+ const char *path, ram_addr_t offset,
Error **errp)
{
Error *err = NULL;
+
memory_region_init(mr, owner, name, size);
- mr->ram = true;
mr->readonly = !!(ram_flags & RAM_READONLY);
- mr->terminates = true;
- mr->destructor = memory_region_destructor_ram;
mr->align = align;
- mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path,
- offset, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
- return false;
- }
- return true;
+ mr->ram_block = qemu_ram_alloc_from_file(size, mr, ram_flags, path, offset,
+ &err);
+ return memory_region_do_init_ram(mr, err, errp);
}
-bool memory_region_init_ram_from_fd(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- uint32_t ram_flags,
- int fd,
- ram_addr_t offset,
- Error **errp)
+bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ uint32_t ram_flags, int fd,
+ ram_addr_t offset, Error **errp)
{
Error *err = NULL;
+
memory_region_init(mr, owner, name, size);
- mr->ram = true;
mr->readonly = !!(ram_flags & RAM_READONLY);
- mr->terminates = true;
- mr->destructor = memory_region_destructor_ram;
mr->ram_block = qemu_ram_alloc_from_fd(size, size, NULL, mr, ram_flags, fd,
offset, false, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
- return false;
- }
- return true;
+ return memory_region_do_init_ram(mr, err, errp);
}
#endif
-void memory_region_init_ram_ptr(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- void *ptr)
+void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size, void *ptr)
{
memory_region_init(mr, owner, name, size);
- mr->ram = true;
- mr->terminates = true;
- mr->destructor = memory_region_destructor_ram;
-
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
assert(ptr != NULL);
mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
+ memory_region_do_init_ram(mr, NULL, NULL);
}
-void memory_region_init_ram_device_ptr(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
+void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
void *ptr)
{
memory_region_init(mr, owner, name, size);
- mr->ram = true;
- mr->ram_device = true;
memory_region_set_ops(mr, &ram_device_mem_ops, mr);
- mr->destructor = memory_region_destructor_ram;
-
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
assert(ptr != NULL);
mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
+ memory_region_do_init_ram(mr, NULL, NULL);
+ mr->ram_device = true;
}
void memory_region_init_alias(MemoryRegion *mr,
@@ -1762,19 +1722,18 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
Error **errp)
{
Error *err = NULL;
+ bool ret;
+
assert(ops);
memory_region_init(mr, owner, name, size);
memory_region_set_ops(mr, ops, opaque);
- mr->rom_device = true;
- mr->destructor = memory_region_destructor_ram;
mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
- if (err) {
- mr->size = int128_zero();
- object_unparent(OBJECT(mr));
- error_propagate(errp, err);
- return false;
+ ret = memory_region_do_init_ram(mr, err, errp);
+ if (ret) {
+ mr->ram = false;
+ mr->rom_device = true;
}
- return true;
+ return ret;
}
void memory_region_init_iommu(void *_iommu_mr,
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 03/10] memory: Factor out more common ram region initialization
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 01/10] memory: Add internal memory_region_set_ops helper function BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 02/10] memory: Factor out common ram region initialization BALATON Zoltan
@ 2026-01-25 17:50 ` BALATON Zoltan
2026-01-25 17:50 ` [PATCH v2 04/10] memory: Shorten memory_region_init_rom_nomigrate BALATON Zoltan
` (7 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:50 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
Introduce internal memory_region_do_init_ram_ptr() function to remove
duplicated code from different memory_region_init_ram_*ptr functions.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
system/memory.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 7267333e11..68dc95b9b1 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1663,26 +1663,29 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
}
#endif
-void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
- const char *name, uint64_t size, void *ptr)
+static void memory_region_do_init_ram_ptr(MemoryRegion *mr, uint64_t size,
+ void *ptr)
{
- memory_region_init(mr, owner, name, size);
/* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
assert(ptr != NULL);
mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
memory_region_do_init_ram(mr, NULL, NULL);
}
+void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size, void *ptr)
+{
+ memory_region_init(mr, owner, name, size);
+ memory_region_do_init_ram_ptr(mr, size, ptr);
+}
+
void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
const char *name, uint64_t size,
void *ptr)
{
memory_region_init(mr, owner, name, size);
memory_region_set_ops(mr, &ram_device_mem_ops, mr);
- /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL. */
- assert(ptr != NULL);
- mr->ram_block = qemu_ram_alloc_from_ptr(size, ptr, mr, &error_abort);
- memory_region_do_init_ram(mr, NULL, NULL);
+ memory_region_do_init_ram_ptr(mr, size, ptr);
mr->ram_device = true;
}
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 04/10] memory: Shorten memory_region_init_rom_nomigrate
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (2 preceding siblings ...)
2026-01-25 17:50 ` [PATCH v2 03/10] memory: Factor out more " BALATON Zoltan
@ 2026-01-25 17:50 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 05/10] memory: Add internal memory_region_register_ram function BALATON Zoltan
` (6 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:50 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
Use the shortcut memory_region_init_ram_nomigrate as we have no flags
to pass and remove some extra line breaks to make the function occupy
less lines. Also shorten some other function prototypes.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
system/memory.c | 30 ++++++++++--------------------
1 file changed, 10 insertions(+), 20 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 68dc95b9b1..015b445ebf 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1568,12 +1568,9 @@ static void memory_region_set_ops(MemoryRegion *mr,
mr->terminates = true;
}
-void memory_region_init_io(MemoryRegion *mr,
- Object *owner,
- const MemoryRegionOps *ops,
- void *opaque,
- const char *name,
- uint64_t size)
+void memory_region_init_io(MemoryRegion *mr, Object *owner,
+ const MemoryRegionOps *ops, void *opaque,
+ const char *name, uint64_t size)
{
memory_region_init(mr, owner, name, size);
memory_region_set_ops(mr, ops, opaque);
@@ -1689,30 +1686,23 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, Object *owner,
mr->ram_device = true;
}
-void memory_region_init_alias(MemoryRegion *mr,
- Object *owner,
- const char *name,
- MemoryRegion *orig,
- hwaddr offset,
- uint64_t size)
+void memory_region_init_alias(MemoryRegion *mr, Object *owner,
+ const char *name, MemoryRegion *orig,
+ hwaddr offset, uint64_t size)
{
memory_region_init(mr, owner, name, size);
mr->alias = orig;
mr->alias_offset = offset;
}
-bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
+bool memory_region_init_rom_nomigrate(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
Error **errp)
{
- if (!memory_region_init_ram_flags_nomigrate(mr, owner, name,
- size, 0, errp)) {
- return false;
+ if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
+ return false;
}
mr->readonly = true;
-
return true;
}
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 05/10] memory: Add internal memory_region_register_ram function
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (3 preceding siblings ...)
2026-01-25 17:50 ` [PATCH v2 04/10] memory: Shorten memory_region_init_rom_nomigrate BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 06/10] memory: Add memory_region_new* functions BALATON Zoltan
` (5 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
Factor out common operation from memory_region_init_{ram,rom}
functions to register the region for migration. This avoids
duplicating the long comment in several functions.
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
system/memory.c | 75 +++++++++++++------------------------------------
1 file changed, 20 insertions(+), 55 deletions(-)
diff --git a/system/memory.c b/system/memory.c
index 015b445ebf..618d5d7f4d 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3678,17 +3678,10 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
}
}
-bool memory_region_init_ram(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
- Error **errp)
+static void memory_region_register_ram(MemoryRegion *mr, Object *owner)
{
DeviceState *owner_dev;
- if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
- return false;
- }
/* This will assert if owner is neither NULL nor a DeviceState.
* We only want the owner here for the purposes of defining a
* unique name for migration. TODO: Ideally we should implement
@@ -3697,80 +3690,52 @@ bool memory_region_init_ram(MemoryRegion *mr,
*/
owner_dev = DEVICE(owner);
vmstate_register_ram(mr, owner_dev);
+}
+bool memory_region_init_ram(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
+ Error **errp)
+{
+ if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
+ return false;
+ }
+ memory_region_register_ram(mr, owner);
return true;
}
-bool memory_region_init_ram_guest_memfd(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
+bool memory_region_init_ram_guest_memfd(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
Error **errp)
{
- DeviceState *owner_dev;
-
if (!memory_region_init_ram_flags_nomigrate(mr, owner, name, size,
RAM_GUEST_MEMFD, errp)) {
return false;
}
- /* This will assert if owner is neither NULL nor a DeviceState.
- * We only want the owner here for the purposes of defining a
- * unique name for migration. TODO: Ideally we should implement
- * a naming scheme for Objects which are not DeviceStates, in
- * which case we can relax this restriction.
- */
- owner_dev = DEVICE(owner);
- vmstate_register_ram(mr, owner_dev);
-
+ memory_region_register_ram(mr, owner);
return true;
}
-bool memory_region_init_rom(MemoryRegion *mr,
- Object *owner,
- const char *name,
- uint64_t size,
+bool memory_region_init_rom(MemoryRegion *mr, Object *owner,
+ const char *name, uint64_t size,
Error **errp)
{
- DeviceState *owner_dev;
-
if (!memory_region_init_rom_nomigrate(mr, owner, name, size, errp)) {
return false;
}
- /* This will assert if owner is neither NULL nor a DeviceState.
- * We only want the owner here for the purposes of defining a
- * unique name for migration. TODO: Ideally we should implement
- * a naming scheme for Objects which are not DeviceStates, in
- * which case we can relax this restriction.
- */
- owner_dev = DEVICE(owner);
- vmstate_register_ram(mr, owner_dev);
-
+ memory_region_register_ram(mr, owner);
return true;
}
-bool memory_region_init_rom_device(MemoryRegion *mr,
- Object *owner,
- const MemoryRegionOps *ops,
- void *opaque,
- const char *name,
- uint64_t size,
+bool memory_region_init_rom_device(MemoryRegion *mr, Object *owner,
+ const MemoryRegionOps *ops, void *opaque,
+ const char *name, uint64_t size,
Error **errp)
{
- DeviceState *owner_dev;
-
if (!memory_region_init_rom_device_nomigrate(mr, owner, ops, opaque,
name, size, errp)) {
return false;
}
- /* This will assert if owner is neither NULL nor a DeviceState.
- * We only want the owner here for the purposes of defining a
- * unique name for migration. TODO: Ideally we should implement
- * a naming scheme for Objects which are not DeviceStates, in
- * which case we can relax this restriction.
- */
- owner_dev = DEVICE(owner);
- vmstate_register_ram(mr, owner_dev);
-
+ memory_region_register_ram(mr, owner);
return true;
}
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 06/10] memory: Add memory_region_new* functions
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (4 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 05/10] memory: Add internal memory_region_register_ram function BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 07/10] memory: Update documentation for memory_region_new*() BALATON Zoltan
` (4 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
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 resets 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 | 360 ++++++++++++++++++++++++++++++++++++++++
system/memory.c | 212 +++++++++++++++++++++++
2 files changed, 572 insertions(+)
diff --git a/include/system/memory.h b/include/system/memory.h
index 8f8725ea2d..58f66adc8f 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1770,6 +1770,366 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
uint64_t size,
Error **errp);
+/**
+ * memory_region_new: Allocate and initialize a memory region
+ *
+ * This is like memory_region_init() but allocates the #MemoryRegion and
+ * attaches it to the owner to free it when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: used for debugging; not visible to the user or ABI
+ * @size: size of the region; any subregions beyond this size will be clipped
+ *
+ * Return: Pointer to the allocated #MemoryRegion.
+ */
+MemoryRegion *memory_region_new(Object *owner,
+ const char *name, uint64_t size);
+
+/**
+ * memory_region_new_io: Allocate and initialize an I/O memory region.
+ *
+ * This is like memory_region_init_io() but allocates the #MemoryRegion and
+ * attaches it to the owner to free it when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @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.
+ * @name: used for debugging; not visible to the user or ABI
+ * @size: size of the region.
+ *
+ * Return: Pointer to the allocated #MemoryRegion.
+ */
+MemoryRegion *memory_region_new_io(Object *owner,
+ const MemoryRegionOps *ops, void *opaque,
+ const char *name, uint64_t size);
+
+/**
+ * memory_region_new_ram_nomigrate: Allocate and initialize RAM memory region.
+ *
+ * This is like memory_region_init_ram_nomigrate() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+MemoryRegion *memory_region_new_ram_nomigrate(Object *owner,
+ const char *name, uint64_t size,
+ Error **errp);
+
+/**
+ * memory_region_new_ram_flags_nomigrate: Allocata and initialize RAM memory
+ * region with flags.
+ *
+ * This is like memory_region_init_ram_flags_nomigrate() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_NORESERVE,
+ * RAM_GUEST_MEMFD.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+MemoryRegion *memory_region_new_ram_flags_nomigrate(Object *owner,
+ const char *name,
+ uint64_t size,
+ uint32_t ram_flags,
+ Error **errp);
+
+/**
+ * memory_region_new_resizeable_ram: Allocate and initialize memory region
+ * with resizable RAM.
+ *
+ * This is like memory_region_init_resizable_ram() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: used size of the region.
+ * @max_size: max size of the region.
+ * @resized: callback to notify owner about used size change.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+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);
+
+/**
+ * memory_region_new_ram_from_file: Allocate and initialize RAM memory region
+ * with a mmap-ed backend.
+ *
+ * This is like memory_region_init_ram_from_file() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @align: alignment of the region base address; if 0, the default alignment
+ * (getpagesize()) will be used.
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
+ * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
+ * RAM_READONLY_FD, RAM_GUEST_MEMFD
+ * @path: the path in which to allocate the RAM.
+ * @offset: offset within the file referenced by path
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+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_new_ram_from_fd: Allocate and initialize RAM memory region
+ * with a mmap-ed backend.
+ *
+ * This is like memory_region_init_ram_from_fd() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: the name of the region.
+ * @size: size of the region.
+ * @ram_flags: RamBlock flags. Supported flags: RAM_SHARED, RAM_PMEM,
+ * RAM_NORESERVE, RAM_PROTECTED, RAM_NAMED_FILE, RAM_READONLY,
+ * RAM_READONLY_FD, RAM_GUEST_MEMFD
+ * @fd: the fd to mmap.
+ * @offset: offset within the file referenced by fd
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+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);
+
+/**
+ * memory_region_new_ram_ptr: Allocate and initialize RAM memory region from a
+ * user-provided pointer.
+ *
+ * This is like memory_region_init_ram_ptr() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @ptr: memory to be mapped; must contain at least @size bytes.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion
+ */
+MemoryRegion *memory_region_new_ram_ptr(Object *owner, const char *name,
+ uint64_t size, void *ptr);
+
+/**
+ * memory_region_new_ram_device_ptr: Allocate and initialize RAM device memory
+ * region from a user-provided pointer.
+ *
+ * This is like memory_region_init_ram_device_ptr() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: the name of the region.
+ * @size: size of the region.
+ * @ptr: memory to be mapped; must contain at least @size bytes.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM memory region to be migrated; that is the responsibility of the caller.
+ * (For RAM device memory regions, migrating the contents rarely makes sense.)
+ *
+ * Return: Pointer to the allocated #MemoryRegion
+ */
+MemoryRegion *memory_region_new_ram_device_ptr(Object *owner, const char *name,
+ uint64_t size, void *ptr);
+
+/**
+ * memory_region_new_alias: Allocate and initialize a memory region that
+ * aliases all or a part of another memory region.
+ *
+ * This is like memory_region_init_alias() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @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.
+ * @offset: start of the section in @orig to be referenced.
+ * @size: size of the region.
+ *
+ * Return: Pointer to the allocated #MemoryRegion
+ */
+MemoryRegion *memory_region_new_alias(Object *owner,
+ const char *name, MemoryRegion *orig,
+ hwaddr offset, uint64_t size);
+
+/**
+ * memory_region_new_rom_nomigrate: Allocate and initialize a ROM memory
+ * region.
+ *
+ * This is like memory_region_init_rom_nomigrate() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+MemoryRegion *memory_region_new_rom_nomigrate(Object *owner,
+ const char *name, uint64_t size,
+ Error **errp);
+
+/**
+ * memory_region_new_rom_device_nomigrate: Allocate and initialize a ROM
+ * device memory region.
+ *
+ * This is like memory_region_init_rom_device_nomigrate() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @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
+ * must be unique within any device
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Note that this function does not do anything to cause the data in the
+ * RAM side of the memory region to be migrated; that is the responsibility
+ * of the caller.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+MemoryRegion *memory_region_new_rom_device_nomigrate(Object *owner,
+ const MemoryRegionOps *ops,
+ void *opaque,
+ const char *name,
+ uint64_t size,
+ Error **errp);
+/**
+ * memory_region_new_ram - Allocate and initialize RAM memory region.
+ *
+ * This is like memory_region_init_ram() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * @owner: the object that tracks the region's reference count (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
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * TODO: Currently we restrict @owner to being either NULL (for
+ * global RAM regions with no owner) or devices, so that we can
+ * give the RAM block a unique name for migration purposes.
+ * We should lift this restriction and allow arbitrary Objects.
+ * If you pass a non-NULL non-device @owner then we will assert.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+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_new_rom: Allocate and initialize a ROM memory region.
+ *
+ * This is like memory_region_init_rom() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * TODO: Currently we restrict @owner to being either NULL (for
+ * global RAM regions with no owner) or devices, so that we can
+ * give the RAM block a unique name for migration purposes.
+ * We should lift this restriction and allow arbitrary Objects.
+ * If you pass a non-NULL non-device @owner then we will assert.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @name: Region name, becomes part of RAMBlock name used in migration stream
+ * must be unique within any device
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+MemoryRegion *memory_region_new_rom(Object *owner,
+ const char *name, uint64_t size,
+ Error **errp);
+
+/**
+ * memory_region_new_rom_device: Allocate and initialize a ROM device memory
+ * region.
+ *
+ * This is like memory_region_init_rom_device() but allocates the
+ * #MemoryRegion and attaches is to the owner to free when the owner is freed.
+ *
+ * TODO: Currently we restrict @owner to being either NULL (for
+ * global RAM regions with no owner) or devices, so that we can
+ * give the RAM block a unique name for migration purposes.
+ * We should lift this restriction and allow arbitrary Objects.
+ * If you pass a non-NULL non-device @owner then we will assert.
+ *
+ * @owner: the object that tracks the region's reference count
+ * @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
+ * must be unique within any device
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ *
+ * Return: Pointer to the allocated #MemoryRegion or NULL on error.
+ */
+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 618d5d7f4d..b4d3ba50b9 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 bool memory_region_do_init_ram(MemoryRegion *mr,
Error *err, Error **errp)
{
@@ -1610,6 +1629,26 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr, Object *owner,
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;
+
+ mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+ return memory_region_do_init_ram(mr, err, errp) ? mr : NULL;
+}
+
+MemoryRegion *memory_region_new_ram_nomigrate(Object *owner,
+ const char *name, uint64_t size,
+ Error **errp)
+{
+ return memory_region_new_ram_flags_nomigrate(owner, name, size, 0, errp);
+}
+
bool memory_region_init_resizeable_ram(MemoryRegion *mr,
Object *owner,
const char *name,
@@ -1628,6 +1667,23 @@ bool memory_region_init_resizeable_ram(MemoryRegion *mr,
return memory_region_do_init_ram(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;
+
+ mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
+ &err);
+ return memory_region_do_init_ram(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,
@@ -1645,6 +1701,25 @@ bool memory_region_init_ram_from_file(MemoryRegion *mr, Object *owner,
return memory_region_do_init_ram(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;
+
+ 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_do_init_ram(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 +1733,20 @@ bool memory_region_init_ram_from_fd(MemoryRegion *mr, Object *owner,
offset, false, &err);
return memory_region_do_init_ram(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;
+
+ 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_do_init_ram(mr, err, errp) ? mr : NULL;
+}
#endif
static void memory_region_do_init_ram_ptr(MemoryRegion *mr, uint64_t size,
@@ -1676,6 +1765,15 @@ void memory_region_init_ram_ptr(MemoryRegion *mr, Object *owner,
memory_region_do_init_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_do_init_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)
@@ -1686,6 +1784,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_do_init_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)
@@ -1695,6 +1804,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;
+}
+
bool memory_region_init_rom_nomigrate(MemoryRegion *mr, Object *owner,
const char *name, uint64_t size,
Error **errp)
@@ -1706,6 +1826,20 @@ bool memory_region_init_rom_nomigrate(MemoryRegion *mr, Object *owner,
return true;
}
+MemoryRegion *memory_region_new_rom_nomigrate(Object *owner,
+ const char *name, uint64_t size,
+ Error **errp)
+{
+ MemoryRegion *mr;
+
+ mr = memory_region_new_ram_nomigrate(owner, name, size, errp);
+ if (!mr) {
+ return NULL;
+ }
+ mr->readonly = true;
+ return mr;
+}
+
bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
Object *owner,
const MemoryRegionOps *ops,
@@ -1729,6 +1863,27 @@ bool memory_region_init_rom_device_nomigrate(MemoryRegion *mr,
return ret;
}
+MemoryRegion *memory_region_new_rom_device_nomigrate(Object *owner,
+ const MemoryRegionOps *ops,
+ void *opaque,
+ const char *name,
+ uint64_t size,
+ Error **errp)
+{
+ MemoryRegion *mr = memory_region_new(owner, name, size);
+ Error *err = NULL;
+
+ assert(ops);
+ memory_region_set_ops(mr, ops, opaque);
+ mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
+ if (memory_region_do_init_ram(mr, err, errp)) {
+ mr->ram = false;
+ mr->rom_device = true;
+ return mr;
+ }
+ return NULL;
+}
+
void memory_region_init_iommu(void *_iommu_mr,
size_t instance_size,
const char *mrtypename,
@@ -3703,6 +3858,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_nomigrate(owner, name, size, 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)
@@ -3715,6 +3883,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)
@@ -3726,6 +3909,19 @@ 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_rom_nomigrate(owner, name, size, errp);
+ if (mr) {
+ 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,
@@ -3739,6 +3935,22 @@ 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;
+
+ mr = memory_region_new_rom_device_nomigrate(owner, ops, opaque, name, size,
+ errp);
+ if (mr) {
+ 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 07/10] memory: Update documentation for memory_region_new*()
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (5 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 06/10] memory: Add memory_region_new* functions BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 08/10] hw/ide/sii3112: Use memory_region_new to avoid leaking regions BALATON Zoltan
` (3 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
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 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index f22146e56c..60eaea8df7 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -142,7 +142,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
@@ -158,16 +159,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
--
2.41.3
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 08/10] hw/ide/sii3112: Use memory_region_new to avoid leaking regions
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (6 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 07/10] memory: Update documentation for memory_region_new*() BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 09/10] hw/pci-host/articia: Map PCI memory windows in realize BALATON Zoltan
` (2 subsequent siblings)
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 09/10] hw/pci-host/articia: Map PCI memory windows in realize
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (7 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 08/10] hw/ide/sii3112: Use memory_region_new to avoid leaking regions BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-25 17:51 ` [PATCH v2 10/10] hw/pci-host/articia: Add variable for common type cast BALATON Zoltan
2026-01-27 13:13 ` [PATCH v2 00/10] Implement memory_region_new_* functions Peter Maydell
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
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 1881e03d58..6e14604311 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v2 10/10] hw/pci-host/articia: Add variable for common type cast
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (8 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 09/10] hw/pci-host/articia: Map PCI memory windows in realize BALATON Zoltan
@ 2026-01-25 17:51 ` BALATON Zoltan
2026-01-27 13:13 ` [PATCH v2 00/10] Implement memory_region_new_* functions Peter Maydell
10 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-25 17:51 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Xu, Akihiko Odaki, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
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 6e14604311..e418d3dac3 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-25 17:50 [PATCH v2 00/10] Implement memory_region_new_* functions BALATON Zoltan
` (9 preceding siblings ...)
2026-01-25 17:51 ` [PATCH v2 10/10] hw/pci-host/articia: Add variable for common type cast BALATON Zoltan
@ 2026-01-27 13:13 ` Peter Maydell
2026-01-27 14:10 ` BALATON Zoltan
10 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2026-01-27 13:13 UTC (permalink / raw)
To: BALATON Zoltan
Cc: qemu-devel, Peter Xu, Akihiko Odaki, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé
On Sun, 25 Jan 2026 at 18:36, BALATON Zoltan <balaton@eik.bme.hu> 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.
If we have a problem with embedded MemoryRegions being leaked,
we need to fix that. I'm very reluctant to create a whole new
set of APIs which handles MRs in a non-standard way (i.e.
"QOM object has a pointer to MR rather than embedding it")
and leaves the existing ones alone. QEMU already has way too
many situations where we have multiple different APIs that can
be used to do something and incomplete transitions from the
old one to the new one.
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-27 13:13 ` [PATCH v2 00/10] Implement memory_region_new_* functions Peter Maydell
@ 2026-01-27 14:10 ` BALATON Zoltan
2026-01-27 19:51 ` Peter Xu
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-27 14:10 UTC (permalink / raw)
To: Peter Maydell
Cc: qemu-devel, Peter Xu, Akihiko Odaki, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé
On Tue, 27 Jan 2026, Peter Maydell wrote:
> On Sun, 25 Jan 2026 at 18:36, BALATON Zoltan <balaton@eik.bme.hu> 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.
>
> If we have a problem with embedded MemoryRegions being leaked,
> we need to fix that.
I have a problem with embedded memory regions (and embedded objects in
general) because:
- they are making state structs unnecessarily crowded obscuring what
should really be there
- leaving the fields to store these memory regions in the parent classes
and the QOM way of lifetime management unused
- exposes internal object implementation to the outside
which is against encapsulation and reuse of object oriented design.
Therefore, I consider embedded memory regions as a hack to solve leaks
because the intended and documented way of using reference counting to
manage the lifetime of these regions cannot be used due to missing API to
instantiate them in a way so it works. This series tries to resolve that
not add new principle or standard.
> I'm very reluctant to create a whole new
> set of APIs which handles MRs in a non-standard way (i.e.
> "QOM object has a pointer to MR rather than embedding it")
But this is the standard and documented way if you read the life cycle
section of the memory docs and the embedded regions became more used
instead of resolving this problem in the way it was originaly intended.
The QOM subclass does not store a pointer to these as it's not needed,
they are managed by QOM so they are created and can be forgotten about
without littering the state struct with fields never used in the object
methods just to manage the memory that QOM could already manage. (The
parent sysbus or PCI class keeps a pointer when registering them as
regions and Object stores them as a child but that's already there and
needed for those classes not the subclass so we can avoid duplicating it
in the subclass.)
> and leaves the existing ones alone. QEMU already has way too
> many situations where we have multiple different APIs that can
> be used to do something and incomplete transitions from the
> old one to the new one.
This series does not convert everything but opens the way to do that and
simplify objects by only storing things in the state that are really
needed by the object and let QOM manage what it can. I'd really like to
avoid having a list of unused stuff in the state struct just because I
can't use something that works and documented just no API to actually use
it. Cf. sii3112 state after this series:
struct SiI3112PCIState {
PCIIDEState i;
SiI3112Regs regs[2];
};
vs. your standard:
struct SiI3112PCIState {
PCIIDEState i;
MemoryRegion mmio;
MemoryRegion bar0;
MemoryRegion bar1;
MemoryRegion bar2;
MemoryRegion bar3;
MemoryRegion bar4;
SiI3112Regs regs[2];
};
First I thought we could use object_new then memory_region_init but it
turns out that does not work because memory_region_init calls
object_initialize which removes the free function installed by object_new
and since there's no other way to set it other than by calling object_new
a split init like in macOS (i.e. alloc+init) is also not possible so a new
set of memory_region_new functions seem to be the easiest way to use it as
intended and avoid changing every user of memory_region_init functions.
As mentioned in the docs and cover letter both set of functions are useful
in different situations and they are otherwise the same so it would not
make the API much more complex as these are basically variants of one set
of functions only differing in dynamically allocating object or
initialising pre-allocated object that are not managed by QOM (which is
what the embedded objects do: take object out from QOM and let the
subclass manage it instead of using the infrastructure that's already
there for it). If you go the embedded way then the registering memory
regions as children should be removed and everything converted to use
embedded regions but I don't think that would be an improvement.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-27 14:10 ` BALATON Zoltan
@ 2026-01-27 19:51 ` Peter Xu
2026-01-27 20:22 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Peter Xu @ 2026-01-27 19:51 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Peter Maydell, qemu-devel, Akihiko Odaki, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé
On Tue, Jan 27, 2026 at 03:10:28PM +0100, BALATON Zoltan wrote:
> This series does not convert everything but opens the way to do that and
> simplify objects by only storing things in the state that are really needed
> by the object and let QOM manage what it can. I'd really like to avoid
> having a list of unused stuff in the state struct just because I can't use
> something that works and documented just no API to actually use it. Cf.
> sii3112 state after this series:
>
> struct SiI3112PCIState {
> PCIIDEState i;
>
> SiI3112Regs regs[2];
> };
>
> vs. your standard:
>
> struct SiI3112PCIState {
> PCIIDEState i;
>
> MemoryRegion mmio;
> MemoryRegion bar0;
> MemoryRegion bar1;
> MemoryRegion bar2;
> MemoryRegion bar3;
> MemoryRegion bar4;
> SiI3112Regs regs[2];
> };
The latter solution is fine at least to me.
I respect your preference on how it can be done, but IMHO it is subjective,
and I tend to agree with Peter that we should try to stick with one way of
solving problems, unless very necessary.
In the last three patches the MRs do not look need dynamic deallocation to
me at all.. Introducing fully dynamic MR allocations only for those is an
overkill to me. I'm not fully convinced we need to merge 600+ new LOCs for
new APIs just for them.
We have real but rare users of dynamically allocated MRs, currently AFAIU
we're leaning towards having an object wrapping the MRs needed, when one
wants to provide a complete lifecycle for the MR so the memory backing the
MRs can be freed completely separately from the parent object.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-27 19:51 ` Peter Xu
@ 2026-01-27 20:22 ` BALATON Zoltan
2026-01-28 10:05 ` Akihiko Odaki
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-27 20:22 UTC (permalink / raw)
To: Peter Xu
Cc: Peter Maydell, qemu-devel, Akihiko Odaki, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé
On Tue, 27 Jan 2026, Peter Xu wrote:
> On Tue, Jan 27, 2026 at 03:10:28PM +0100, BALATON Zoltan wrote:
>> This series does not convert everything but opens the way to do that and
>> simplify objects by only storing things in the state that are really needed
>> by the object and let QOM manage what it can. I'd really like to avoid
>> having a list of unused stuff in the state struct just because I can't use
>> something that works and documented just no API to actually use it. Cf.
>> sii3112 state after this series:
>>
>> struct SiI3112PCIState {
>> PCIIDEState i;
>>
>> SiI3112Regs regs[2];
>> };
>>
>> vs. your standard:
>>
>> struct SiI3112PCIState {
>> PCIIDEState i;
>>
>> MemoryRegion mmio;
>> MemoryRegion bar0;
>> MemoryRegion bar1;
>> MemoryRegion bar2;
>> MemoryRegion bar3;
>> MemoryRegion bar4;
>> SiI3112Regs regs[2];
>> };
>
> The latter solution is fine at least to me.
>
> I respect your preference on how it can be done, but IMHO it is subjective,
> and I tend to agree with Peter that we should try to stick with one way of
> solving problems, unless very necessary.
>
> In the last three patches the MRs do not look need dynamic deallocation to
> me at all.. Introducing fully dynamic MR allocations only for those is an
> overkill to me. I'm not fully convinced we need to merge 600+ new LOCs for
> new APIs just for them.
It's not a new API in the sense that it's the same as existing
memory_region_init functions just a variant of it to allow using QOM to
manage the lifetime of the memory region which the current API does not
allow. Also this series does not introduce this dynamic lifetime
management, it's already implemented and documented just there's currently
no way to use it. The actual change is much less, most of the lines are
just the documentation comments. If it's a problem to increase
documentation size I could make it much shorter not duplicating a separate
doc comment for these just mention in the corresponding memory_region_init
doc comment that there's also an analogous memory_region_new variant of
it. That also shows there's no new API just two ways of using the same
API.
> We have real but rare users of dynamically allocated MRs, currently AFAIU
Since it's already implemented why not use it if it makes the object state
simpler? Adding additional fields that would already be managed by QOM
seems to be unnecessary duplication and complication to me so I'd like to
avoid that and keep the subclass simple.
Regards,
BALATON Zoltan
> we're leaning towards having an object wrapping the MRs needed, when one
> wants to provide a complete lifecycle for the MR so the memory backing the
> MRs can be freed completely separately from the parent object.
>
> Thanks,
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-27 20:22 ` BALATON Zoltan
@ 2026-01-28 10:05 ` Akihiko Odaki
2026-01-28 11:40 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Akihiko Odaki @ 2026-01-28 10:05 UTC (permalink / raw)
To: BALATON Zoltan, Peter Xu
Cc: Peter Maydell, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé
On 2026/01/28 5:22, BALATON Zoltan wrote:
> On Tue, 27 Jan 2026, Peter Xu wrote:
>> On Tue, Jan 27, 2026 at 03:10:28PM +0100, BALATON Zoltan wrote:
>>> This series does not convert everything but opens the way to do that and
>>> simplify objects by only storing things in the state that are really
>>> needed
>>> by the object and let QOM manage what it can. I'd really like to avoid
>>> having a list of unused stuff in the state struct just because I
>>> can't use
>>> something that works and documented just no API to actually use it. Cf.
>>> sii3112 state after this series:
>>>
>>> struct SiI3112PCIState {
>>> PCIIDEState i;
>>>
>>> SiI3112Regs regs[2];
>>> };
>>>
>>> vs. your standard:
>>>
>>> struct SiI3112PCIState {
>>> PCIIDEState i;
>>>
>>> MemoryRegion mmio;
>>> MemoryRegion bar0;
>>> MemoryRegion bar1;
>>> MemoryRegion bar2;
>>> MemoryRegion bar3;
>>> MemoryRegion bar4;
>>> SiI3112Regs regs[2];
>>> };
>>
>> The latter solution is fine at least to me.
>>
>> I respect your preference on how it can be done, but IMHO it is
>> subjective,
>> and I tend to agree with Peter that we should try to stick with one
>> way of
>> solving problems, unless very necessary.
>>
>> In the last three patches the MRs do not look need dynamic
>> deallocation to
>> me at all.. Introducing fully dynamic MR allocations only for those
>> is an
>> overkill to me. I'm not fully convinced we need to merge 600+ new
>> LOCs for
>> new APIs just for them.
>
> It's not a new API in the sense that it's the same as existing
> memory_region_init functions just a variant of it to allow using QOM to
> manage the lifetime of the memory region which the current API does not
> allow. Also this series does not introduce this dynamic lifetime
> management, it's already implemented and documented just there's
> currently no way to use it. The actual change is much less, most of the
> lines are just the documentation comments. If it's a problem to increase
> documentation size I could make it much shorter not duplicating a
> separate doc comment for these just mention in the corresponding
> memory_region_init doc comment that there's also an analogous
> memory_region_new variant of it. That also shows there's no new API just
> two ways of using the same API.
>
>> We have real but rare users of dynamically allocated MRs, currently AFAIU
>
> Since it's already implemented why not use it if it makes the object
> state simpler? Adding additional fields that would already be managed by
> QOM seems to be unnecessary duplication and complication to me so I'd
> like to avoid that and keep the subclass simple.
I feel the discussion is going a wrong direction. This thread and the
cover letter talks different things as the motivation of this series;
The cover letter says the documentation mismatches with the
implementation and that is the motivation for this series. This thread
talks about duplicate memory management and extra fields, which is not
mentioned in the cover letter.
Besides, the "extra fields" problem should have been already discussed
in [1] and I don't see a new argument here. So, in summary, for a
constructive discussion I think:
- There should be a consistent goal of discussion or patch series
(documentation mismatch or extra fields) and
- A new point needs to be made; reminding the points of past discussion
may be worthwhile but is insufficient.
[1]
https://lore.kernel.org/qemu-devel/ceda4c28887c40e1c8eae3f561ee381ca98b0484.1761346145.git.balaton@eik.bme.hu/
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-28 10:05 ` Akihiko Odaki
@ 2026-01-28 11:40 ` BALATON Zoltan
2026-01-28 13:47 ` Peter Maydell
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-28 11:40 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Peter Xu, Peter Maydell, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé,
Daniel P. Berrangé, Eduardo Habkost
[-- Attachment #1: Type: text/plain, Size: 6104 bytes --]
On Wed, 28 Jan 2026, Akihiko Odaki wrote:
> On 2026/01/28 5:22, BALATON Zoltan wrote:
>> On Tue, 27 Jan 2026, Peter Xu wrote:
>>> On Tue, Jan 27, 2026 at 03:10:28PM +0100, BALATON Zoltan wrote:
>>>> This series does not convert everything but opens the way to do that and
>>>> simplify objects by only storing things in the state that are really
>>>> needed
>>>> by the object and let QOM manage what it can. I'd really like to avoid
>>>> having a list of unused stuff in the state struct just because I can't
>>>> use
>>>> something that works and documented just no API to actually use it. Cf.
>>>> sii3112 state after this series:
>>>>
>>>> struct SiI3112PCIState {
>>>> PCIIDEState i;
>>>>
>>>> SiI3112Regs regs[2];
>>>> };
>>>>
>>>> vs. your standard:
>>>>
>>>> struct SiI3112PCIState {
>>>> PCIIDEState i;
>>>>
>>>> MemoryRegion mmio;
>>>> MemoryRegion bar0;
>>>> MemoryRegion bar1;
>>>> MemoryRegion bar2;
>>>> MemoryRegion bar3;
>>>> MemoryRegion bar4;
>>>> SiI3112Regs regs[2];
>>>> };
>>>
>>> The latter solution is fine at least to me.
>>>
>>> I respect your preference on how it can be done, but IMHO it is
>>> subjective,
>>> and I tend to agree with Peter that we should try to stick with one way of
>>> solving problems, unless very necessary.
>>>
>>> In the last three patches the MRs do not look need dynamic deallocation to
>>> me at all.. Introducing fully dynamic MR allocations only for those is an
>>> overkill to me. I'm not fully convinced we need to merge 600+ new LOCs
>>> for
>>> new APIs just for them.
>>
>> It's not a new API in the sense that it's the same as existing
>> memory_region_init functions just a variant of it to allow using QOM to
>> manage the lifetime of the memory region which the current API does not
>> allow. Also this series does not introduce this dynamic lifetime
>> management, it's already implemented and documented just there's currently
>> no way to use it. The actual change is much less, most of the lines are
>> just the documentation comments. If it's a problem to increase
>> documentation size I could make it much shorter not duplicating a separate
>> doc comment for these just mention in the corresponding memory_region_init
>> doc comment that there's also an analogous memory_region_new variant of it.
>> That also shows there's no new API just two ways of using the same API.
>>
>>> We have real but rare users of dynamically allocated MRs, currently AFAIU
>>
>> Since it's already implemented why not use it if it makes the object state
>> simpler? Adding additional fields that would already be managed by QOM
>> seems to be unnecessary duplication and complication to me so I'd like to
>> avoid that and keep the subclass simple.
>
> I feel the discussion is going a wrong direction. This thread and the cover
> letter talks different things as the motivation of this series; The cover
> letter says the documentation mismatches with the implementation and that is
> the motivation for this series. This thread talks about duplicate memory
> management and extra fields, which is not mentioned in the cover letter.
>
> Besides, the "extra fields" problem should have been already discussed in [1]
> and I don't see a new argument here. So, in summary, for a constructive
> discussion I think:
> - There should be a consistent goal of discussion or patch series
> (documentation mismatch or extra fields) and
> - A new point needs to be made; reminding the points of past discussion
> may be worthwhile but is insufficient.
>
> [1]
> https://lore.kernel.org/qemu-devel/ceda4c28887c40e1c8eae3f561ee381ca98b0484.1761346145.git.balaton@eik.bme.hu/
Adding some QOM people in case they have something to add.
OK I try to summarise the motivation again:
1. Documentation in docs/devel/memory.rst says that memory regions'
lifecycle is managed by QOM and they are freed with their owner or when
nothing else uses them. This is also already implemented for a long time
as described but cannot be used because the only constructors available
kill this feature when calling object_initialize that clears the free
function added by object_new. (The life time management is implemented
through adding memory regions as children to the owner and unparenting
them on freeing the owner which decreases ref count of the memory region
and will free it when nothing else references it as far as I can tell.)
2. This also allows to keep subclasses' state simpler as demonstrated by
the last two patches by allowing QOM to manage memory regions as
originally intended (based on the documentation and implementation) and
not needing to embed memory regions not otherwise needed in the subclass
as they are already handled by super classes (e.g. registering as PCI
regions or sysbus mmio regions and their lifetime managed by QOM; this way
the subclass only contains what it has to add to the superclass and kept
simpler as expected in an object oriented design).
So the motivation is to match documentation and allow using already
implemented memory region lifecycle management to make device models
simpler.
These are my motivation for this change. What is the motivation for using
embedded memory regions instead and against this change? If that is found
to be a better way then the lifecycle management as implementated and
documentated is likely no longer needed as it cannot and will not be used
with embedded memory regions that are initialsed with object_initialize
and thus could be removed. But I think that would lead to more complex
device models not using what QOM and QDev was meant to help with. (More
complex device models is a problem because they are harder to understand
and find bugs in them having additional things that disctract from actual
functionality of the subclass so I'd like to make them as simple as
possible only containing things that are related to the subclass. So
less QOM boilet plate, less additional static fields in the state is the
better.)
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-28 11:40 ` BALATON Zoltan
@ 2026-01-28 13:47 ` Peter Maydell
2026-01-28 15:46 ` BALATON Zoltan
0 siblings, 1 reply; 24+ messages in thread
From: Peter Maydell @ 2026-01-28 13:47 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Akihiko Odaki, Peter Xu, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé,
Daniel P. Berrangé, Eduardo Habkost
On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <balaton@eik.bme.hu> wrote:
> OK I try to summarise the motivation again:
>
> 1. Documentation in docs/devel/memory.rst says that memory regions'
> lifecycle is managed by QOM and they are freed with their owner or when
> nothing else uses them. This is also already implemented for a long time
> as described but cannot be used because the only constructors available
> kill this feature when calling object_initialize that clears the free
> function added by object_new. (The life time management is implemented
> through adding memory regions as children to the owner and unparenting
> them on freeing the owner which decreases ref count of the memory region
> and will free it when nothing else references it as far as I can tell.)
If we have leaks because of our very common pattern of "embed
a MemoryRegion struct in the device state struct" then we must
fix those, because there's no way we're going to convert all
that existing code to a new set of APIs. But I was under the
impression we had already dealt with those, because MRs track
their owner's refcount, and don't have their own independent one ?
> These are my motivation for this change. What is the motivation for using
> embedded memory regions instead and against this change?
Simply that it's a consistent pattern we use in a lot of the codebase:
the device embeds a lot of the structs it uses, rather than allocating
memory for them and keeping pointers to that allocated memory. We
still have also various older device models that use the previous
pattern of "allocate memory and have pointers" too, but most new
code doesn't do that. I think we should for preference write code
in one pattern, not two, and "embed structs" seems to be what
we have mostly settled on for new code.
There is an argument to be made that the pointer model would
fit better with a possible future world of "the user can wire
configurably wire up their own board model from devices", and
that it works better in a part-Rust-part-C world where the two
different languages don't have convenient access to the exact
size of structs defined in the other language. But that future
model is not something anybody has yet really fleshed out in any
detail, so it's still a bit speculative.
I'm not actually opposed to the idea of making a design decision
that this struct-embedding is no longer what we want to do, and defining
that something else is our new best practice for how to write devices.
But I think we would need to start by reaching a consensus that that
*is* what we want to do, and documenting that "best practice" somewhere
in docs/devel/. Then we can examine proposed new APIs and all be
on the same page about the design patterns we want and it will
be clearer to reviewers whether the new APIs fit into those
patterns or not.
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-28 13:47 ` Peter Maydell
@ 2026-01-28 15:46 ` BALATON Zoltan
2026-01-29 4:41 ` Akihiko Odaki
0 siblings, 1 reply; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-28 15:46 UTC (permalink / raw)
To: Peter Maydell
Cc: Akihiko Odaki, Peter Xu, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé,
Daniel P. Berrangé, Eduardo Habkost
On Wed, 28 Jan 2026, Peter Maydell wrote:
> On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>> OK I try to summarise the motivation again:
>>
>> 1. Documentation in docs/devel/memory.rst says that memory regions'
>> lifecycle is managed by QOM and they are freed with their owner or when
>> nothing else uses them. This is also already implemented for a long time
>> as described but cannot be used because the only constructors available
>> kill this feature when calling object_initialize that clears the free
>> function added by object_new. (The life time management is implemented
>> through adding memory regions as children to the owner and unparenting
>> them on freeing the owner which decreases ref count of the memory region
>> and will free it when nothing else references it as far as I can tell.)
>
> If we have leaks because of our very common pattern of "embed
> a MemoryRegion struct in the device state struct" then we must
> fix those, because there's no way we're going to convert all
> that existing code to a new set of APIs. But I was under the
> impression we had already dealt with those, because MRs track
> their owner's refcount, and don't have their own independent one ?
I'm not sure if all those leaks are resolved as there were some patches
and discussion about this recently but I think that problem or the need to
use the owner's ref count to circumwent it instead of using the memory
region's own ref count may also come from that there's currently no way to
allocate memory regions that are ref counted and automatically freed as it
should work with QOM and the documentation implies. (Only the constuctor
is missing that is all this series adds, the mechanism is already there
and implemented.) There may still be a problem with circular references if
the memory region needs the owner so the owner can't be freed until the
memory region is also freed but the memory region is not freed until the
owner is freed but if both the owner and memory region used their own ref
count things may become a bit less confusing and could be easier to find a
way to break circular reference (e.g. by owner unparent child regions on
unrealize but isn't freed until memory regions unref owner in their free
method).
>> These are my motivation for this change. What is the motivation for using
>> embedded memory regions instead and against this change?
>
> Simply that it's a consistent pattern we use in a lot of the codebase:
> the device embeds a lot of the structs it uses, rather than allocating
> memory for them and keeping pointers to that allocated memory. We
You mix in the issue of SoCs and complex devices using other devices in
which case the recommendation was to embed those in the parent device so
they don't have to be freed or kept track of by a pointer but won't be
leaked. This series does not mean to change that, it's only limited to
memory regions. (Although that problem may also stem from similar issue
with object_initialize_child not allowing creating reference counted
objects only initializing preallocated instances but that's not
something this series touches.)
We can say that memory regions are like other embedded objects but they
are often used for sysbus and PCI devices only to be registered in the
parent device that already has pointers in their state to track these so
there's no need to keep track of them in the subclass if we can rely on
QOM freeing them when not needed any more and this is already implemented
and documented that way. So even if we keep embedding other child devices
into complex parent devices that I think does not directly apply to memory
regions and we could use what the documentation and implementation
already allows and says for memory regions at least.
> still have also various older device models that use the previous
> pattern of "allocate memory and have pointers" too, but most new
> code doesn't do that. I think we should for preference write code
> in one pattern, not two, and "embed structs" seems to be what
> we have mostly settled on for new code.
>
> There is an argument to be made that the pointer model would
> fit better with a possible future world of "the user can wire
> configurably wire up their own board model from devices", and
> that it works better in a part-Rust-part-C world where the two
> different languages don't have convenient access to the exact
> size of structs defined in the other language. But that future
> model is not something anybody has yet really fleshed out in any
> detail, so it's still a bit speculative.
You keep mentioning pointers but the point of ref counts and regisrering
memory region as child of an owner is to avoid needing a pointer or
embedding it in the subclass state as the relationship and lifecycle
management are then handled by QOM. If we don't use that we could remove
this from QOM and memory regions to simplify it but if it's already there
and makes the device state simpler I think we better use it.
> I'm not actually opposed to the idea of making a design decision
> that this struct-embedding is no longer what we want to do, and defining
> that something else is our new best practice for how to write devices.
> But I think we would need to start by reaching a consensus that that
> *is* what we want to do, and documenting that "best practice" somewhere
> in docs/devel/. Then we can examine proposed new APIs and all be
> on the same page about the design patterns we want and it will
> be clearer to reviewers whether the new APIs fit into those
> patterns or not.
I think we're in that discussion now in this thread. I don't propose to
change the struct-embedding for sub devices used in SoC or south bridge or
other complex devices but only propose to not embed memory regions that
are already documented as and handled by QOM and simply allocate them and
let QOM handle them so we only need to reference them in the devices state
unless they are needed for some reason by the device methods which is
rarely the case. So this is limited to memory regions and the series only
seems to add a lot of lines because of the extensive documentation
comments. The actual change is just factoring out actual memory region
init from memory_region_init functions then add a memory_region_new
variant that does object_new; do_init and keep the memory_region_init do
object_initializel do_init. Nothing else is changed, the way to manage and
free regions based on ref counting is already there this series just
enables them to be actually used becase currently despite what the docs
say memory regions are either leaked or must be embedded.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-28 15:46 ` BALATON Zoltan
@ 2026-01-29 4:41 ` Akihiko Odaki
2026-01-29 18:06 ` Paolo Bonzini
2026-01-30 20:08 ` Mark Cave-Ayland
0 siblings, 2 replies; 24+ messages in thread
From: Akihiko Odaki @ 2026-01-29 4:41 UTC (permalink / raw)
To: BALATON Zoltan, Peter Maydell
Cc: Peter Xu, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé, Daniel P. Berrangé,
Eduardo Habkost
On 2026/01/29 0:46, BALATON Zoltan wrote:
> On Wed, 28 Jan 2026, Peter Maydell wrote:
>> On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>> OK I try to summarise the motivation again:
>>>
>>> 1. Documentation in docs/devel/memory.rst says that memory regions'
>>> lifecycle is managed by QOM and they are freed with their owner or when
>>> nothing else uses them. This is also already implemented for a long time
>>> as described but cannot be used because the only constructors available
>>> kill this feature when calling object_initialize that clears the free
>>> function added by object_new. (The life time management is implemented
>>> through adding memory regions as children to the owner and unparenting
>>> them on freeing the owner which decreases ref count of the memory region
>>> and will free it when nothing else references it as far as I can tell.)
>>
>> If we have leaks because of our very common pattern of "embed
>> a MemoryRegion struct in the device state struct" then we must
>> fix those, because there's no way we're going to convert all
>> that existing code to a new set of APIs. But I was under the
>> impression we had already dealt with those, because MRs track
>> their owner's refcount, and don't have their own independent one ?
>
> I'm not sure if all those leaks are resolved as there were some patches
> and discussion about this recently but I think that problem or the need
> to use the owner's ref count to circumwent it instead of using the
> memory region's own ref count may also come from that there's currently
> no way to allocate memory regions that are ref counted and automatically
> freed as it should work with QOM and the documentation implies. (Only
> the constuctor is missing that is all this series adds, the mechanism is
> already there and implemented.) There may still be a problem with
> circular references if the memory region needs the owner so the owner
> can't be freed until the memory region is also freed but the memory
> region is not freed until the owner is freed but if both the owner and
> memory region used their own ref count things may become a bit less
> confusing and could be easier to find a way to break circular reference
> (e.g. by owner unparent child regions on unrealize but isn't freed until
> memory regions unref owner in their free method).
>
>>> These are my motivation for this change. What is the motivation for
>>> using
>>> embedded memory regions instead and against this change?
>>
>> Simply that it's a consistent pattern we use in a lot of the codebase:
>> the device embeds a lot of the structs it uses, rather than allocating
>> memory for them and keeping pointers to that allocated memory. We
>
> You mix in the issue of SoCs and complex devices using other devices in
> which case the recommendation was to embed those in the parent device so
> they don't have to be freed or kept track of by a pointer but won't be
> leaked. This series does not mean to change that, it's only limited to
> memory regions. (Although that problem may also stem from similar issue
> with object_initialize_child not allowing creating reference counted
> objects only initializing preallocated instances but that's not
> something this series touches.)
>
> We can say that memory regions are like other embedded objects but they
> are often used for sysbus and PCI devices only to be registered in the
> parent device that already has pointers in their state to track these so
> there's no need to keep track of them in the subclass if we can rely on
> QOM freeing them when not needed any more and this is already
> implemented and documented that way. So even if we keep embedding other
> child devices into complex parent devices that I think does not directly
> apply to memory regions and we could use what the documentation and
> implementation already allows and says for memory regions at least.
>
>> still have also various older device models that use the previous
>> pattern of "allocate memory and have pointers" too, but most new
>> code doesn't do that. I think we should for preference write code
>> in one pattern, not two, and "embed structs" seems to be what
>> we have mostly settled on for new code.
>>
>> There is an argument to be made that the pointer model would
>> fit better with a possible future world of "the user can wire
>> configurably wire up their own board model from devices", and
>> that it works better in a part-Rust-part-C world where the two
>> different languages don't have convenient access to the exact
>> size of structs defined in the other language. But that future
>> model is not something anybody has yet really fleshed out in any
>> detail, so it's still a bit speculative.
>
> You keep mentioning pointers but the point of ref counts and regisrering
> memory region as child of an owner is to avoid needing a pointer or
> embedding it in the subclass state as the relationship and lifecycle
> management are then handled by QOM. If we don't use that we could remove
> this from QOM and memory regions to simplify it but if it's already
> there and makes the device state simpler I think we better use it.
>
>> I'm not actually opposed to the idea of making a design decision
>> that this struct-embedding is no longer what we want to do, and defining
>> that something else is our new best practice for how to write devices.
>> But I think we would need to start by reaching a consensus that that
>> *is* what we want to do, and documenting that "best practice" somewhere
>> in docs/devel/. Then we can examine proposed new APIs and all be
>> on the same page about the design patterns we want and it will
>> be clearer to reviewers whether the new APIs fit into those
>> patterns or not.
>
> I think we're in that discussion now in this thread. I don't propose to
> change the struct-embedding for sub devices used in SoC or south bridge
> or other complex devices but only propose to not embed memory regions
> that are already documented as and handled by QOM and simply allocate
> them and let QOM handle them so we only need to reference them in the
> devices state unless they are needed for some reason by the device
> methods which is rarely the case. So this is limited to memory regions
> and the series only seems to add a lot of lines because of the extensive
> documentation comments. The actual change is just factoring out actual
> memory region init from memory_region_init functions then add a
> memory_region_new variant that does object_new; do_init and keep the
> memory_region_init do object_initializel do_init. Nothing else is
> changed, the way to manage and free regions based on ref counting is
> already there this series just enables them to be actually used becase
> currently despite what the docs say memory regions are either leaked or
> must be embedded.
I actually think deprecating struct-embedding for all QOM objects is a
good idea.
The problem initially stated in this thread is that embedding requires
having extra field, but people see the benefit is too small. There is no
real logic involved in having such fields so it does not reduce code
complexity much; it saves some lines and that's it.
However, I see another problem in struct embedding; it breaks
object_ref(). When embedding, the child object effectively takes the
reference to the storage of the parent object, but this reference is not
counted, so use-after-free can happen if someone takes a reference to
the child object with object_ref(). That is why the wrapper of
object_ref() in rust/qom/src/qom.rs needs to be marked unsafe. Memory
regions workaround this with memory_region_ref(), but it's not perfect
since it relies on object_ref() in the end.
For this reason I think object_initialize(), object_initialize_child(),
and the like are better to be noted as deprecated in
include/qom/object.h. Then memory_region_init() can be deprecated
referring to them.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-29 4:41 ` Akihiko Odaki
@ 2026-01-29 18:06 ` Paolo Bonzini
2026-01-30 6:13 ` Akihiko Odaki
2026-01-30 20:08 ` Mark Cave-Ayland
1 sibling, 1 reply; 24+ messages in thread
From: Paolo Bonzini @ 2026-01-29 18:06 UTC (permalink / raw)
To: Akihiko Odaki, BALATON Zoltan, Peter Maydell
Cc: Peter Xu, qemu-devel, Michael S. Tsirkin,
Philippe Mathieu-Daudé, Daniel P. Berrangé,
Eduardo Habkost
On 1/29/26 05:41, Akihiko Odaki wrote:
> However, I see another problem in struct embedding; it breaks
> object_ref(). When embedding, the child object effectively takes the
> reference to the storage of the parent object, but this reference is not
> counted, so use-after-free can happen if someone takes a reference to
> the child object with object_ref(). That is why the wrapper of
> object_ref() in rust/qom/src/qom.rs needs to be marked unsafe. Memory
> regions workaround this with memory_region_ref(), but it's not perfect
> since it relies on object_ref() in the end.
Yes, and in Rust the idea was to have (in addition to Owned<T> which is
for an allocated object) another smart pointer Child<'a, T>: an embedded
object that is owned (the parent has a reference and a field of type
Child releases that reference when the parent is finalized) but cannot
be cloned.
> For this reason I think object_initialize(), object_initialize_child(),
> and the like are better to be noted as deprecated in
> include/qom/object.h. Then memory_region_init() can be deprecated
> referring to them.
This would be huge and I don't think it's feasible.
It also only provides the appearance of safety. If you have a backwards
pointer (such as the memory region's owner), you still have either a
leak or the risk of a use-after-free.
Paolo
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-29 18:06 ` Paolo Bonzini
@ 2026-01-30 6:13 ` Akihiko Odaki
0 siblings, 0 replies; 24+ messages in thread
From: Akihiko Odaki @ 2026-01-30 6:13 UTC (permalink / raw)
To: Paolo Bonzini, BALATON Zoltan, Peter Maydell
Cc: Peter Xu, qemu-devel, Michael S. Tsirkin,
Philippe Mathieu-Daudé, Daniel P. Berrangé,
Eduardo Habkost
On 2026/01/30 3:06, Paolo Bonzini wrote:
> On 1/29/26 05:41, Akihiko Odaki wrote:
>> However, I see another problem in struct embedding; it breaks
>> object_ref(). When embedding, the child object effectively takes the
>> reference to the storage of the parent object, but this reference is
>> not counted, so use-after-free can happen if someone takes a reference
>> to the child object with object_ref(). That is why the wrapper of
>> object_ref() in rust/qom/src/qom.rs needs to be marked unsafe. Memory
>> regions workaround this with memory_region_ref(), but it's not perfect
>> since it relies on object_ref() in the end.
>
> Yes, and in Rust the idea was to have (in addition to Owned<T> which is
> for an allocated object) another smart pointer Child<'a, T>: an embedded
> object that is owned (the parent has a reference and a field of type
> Child releases that reference when the parent is finalized) but cannot
> be cloned.
>
>> For this reason I think object_initialize(),
>> object_initialize_child(), and the like are better to be noted as
>> deprecated in
>> include/qom/object.h. Then memory_region_init() can be deprecated
>> referring to them.
>
> This would be huge and I don't think it's feasible.
>
> It also only provides the appearance of safety. If you have a backwards
> pointer (such as the memory region's owner), you still have either a
> leak or the risk of a use-after-free.
Now I'm starting to recover my memory on the situation of this matter. I
have actually written patches to make struct embedding safe and also
deal with memory region's owner. My tree that include them can be found at:
https://gitlab.com/akihiko.odaki/qemu/-/tree/51066314fcff96303c1ad5814c3d74027855a7cb
Making struct embedding safe is done by introducing another reference
counter for storage. So the reference graph will be looked like the
following:
Owner -+-> Owner's storage
MR ----+
Memory region's backward references to their owning devices are
invalidated when unrealizing the devices. The reasoning here is that the
corner cases where devices are accessed after unrealization are unlikely
to be handled properly, so they are better to be forbidden.
Users other than memory regions may still have backward pointers that
are not properly tracked, so the Rust wrapper of object_ref() is still
marked unsafe.
Regards,
Akihiko Odaki
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-29 4:41 ` Akihiko Odaki
2026-01-29 18:06 ` Paolo Bonzini
@ 2026-01-30 20:08 ` Mark Cave-Ayland
2026-01-30 21:31 ` BALATON Zoltan
1 sibling, 1 reply; 24+ messages in thread
From: Mark Cave-Ayland @ 2026-01-30 20:08 UTC (permalink / raw)
To: Akihiko Odaki, BALATON Zoltan, Peter Maydell
Cc: Peter Xu, qemu-devel, Paolo Bonzini, Michael S. Tsirkin,
Philippe Mathieu-Daudé, Daniel P. Berrangé,
Eduardo Habkost
On 29/01/2026 04:41, Akihiko Odaki wrote:
> On 2026/01/29 0:46, BALATON Zoltan wrote:
>> On Wed, 28 Jan 2026, Peter Maydell wrote:
>>> On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>> OK I try to summarise the motivation again:
>>>>
>>>> 1. Documentation in docs/devel/memory.rst says that memory regions'
>>>> lifecycle is managed by QOM and they are freed with their owner or when
>>>> nothing else uses them. This is also already implemented for a long time
>>>> as described but cannot be used because the only constructors available
>>>> kill this feature when calling object_initialize that clears the free
>>>> function added by object_new. (The life time management is implemented
>>>> through adding memory regions as children to the owner and unparenting
>>>> them on freeing the owner which decreases ref count of the memory region
>>>> and will free it when nothing else references it as far as I can tell.)
>>>
>>> If we have leaks because of our very common pattern of "embed
>>> a MemoryRegion struct in the device state struct" then we must
>>> fix those, because there's no way we're going to convert all
>>> that existing code to a new set of APIs. But I was under the
>>> impression we had already dealt with those, because MRs track
>>> their owner's refcount, and don't have their own independent one ?
>>
>> I'm not sure if all those leaks are resolved as there were some patches and
>> discussion about this recently but I think that problem or the need to use the
>> owner's ref count to circumwent it instead of using the memory region's own ref
>> count may also come from that there's currently no way to allocate memory regions
>> that are ref counted and automatically freed as it should work with QOM and the
>> documentation implies. (Only the constuctor is missing that is all this series
>> adds, the mechanism is already there and implemented.) There may still be a problem
>> with circular references if the memory region needs the owner so the owner can't be
>> freed until the memory region is also freed but the memory region is not freed
>> until the owner is freed but if both the owner and memory region used their own ref
>> count things may become a bit less confusing and could be easier to find a way to
>> break circular reference (e.g. by owner unparent child regions on unrealize but
>> isn't freed until memory regions unref owner in their free method).
>>
>>>> These are my motivation for this change. What is the motivation for using
>>>> embedded memory regions instead and against this change?
>>>
>>> Simply that it's a consistent pattern we use in a lot of the codebase:
>>> the device embeds a lot of the structs it uses, rather than allocating
>>> memory for them and keeping pointers to that allocated memory. We
>>
>> You mix in the issue of SoCs and complex devices using other devices in which case
>> the recommendation was to embed those in the parent device so they don't have to be
>> freed or kept track of by a pointer but won't be leaked. This series does not mean
>> to change that, it's only limited to memory regions. (Although that problem may
>> also stem from similar issue with object_initialize_child not allowing creating
>> reference counted objects only initializing preallocated instances but that's not
>> something this series touches.)
>>
>> We can say that memory regions are like other embedded objects but they are often
>> used for sysbus and PCI devices only to be registered in the parent device that
>> already has pointers in their state to track these so there's no need to keep track
>> of them in the subclass if we can rely on QOM freeing them when not needed any more
>> and this is already implemented and documented that way. So even if we keep
>> embedding other child devices into complex parent devices that I think does not
>> directly apply to memory regions and we could use what the documentation and
>> implementation already allows and says for memory regions at least.
>>
>>> still have also various older device models that use the previous
>>> pattern of "allocate memory and have pointers" too, but most new
>>> code doesn't do that. I think we should for preference write code
>>> in one pattern, not two, and "embed structs" seems to be what
>>> we have mostly settled on for new code.
>>>
>>> There is an argument to be made that the pointer model would
>>> fit better with a possible future world of "the user can wire
>>> configurably wire up their own board model from devices", and
>>> that it works better in a part-Rust-part-C world where the two
>>> different languages don't have convenient access to the exact
>>> size of structs defined in the other language. But that future
>>> model is not something anybody has yet really fleshed out in any
>>> detail, so it's still a bit speculative.
>>
>> You keep mentioning pointers but the point of ref counts and regisrering memory
>> region as child of an owner is to avoid needing a pointer or embedding it in the
>> subclass state as the relationship and lifecycle management are then handled by
>> QOM. If we don't use that we could remove this from QOM and memory regions to
>> simplify it but if it's already there and makes the device state simpler I think we
>> better use it.
>>
>>> I'm not actually opposed to the idea of making a design decision
>>> that this struct-embedding is no longer what we want to do, and defining
>>> that something else is our new best practice for how to write devices.
>>> But I think we would need to start by reaching a consensus that that
>>> *is* what we want to do, and documenting that "best practice" somewhere
>>> in docs/devel/. Then we can examine proposed new APIs and all be
>>> on the same page about the design patterns we want and it will
>>> be clearer to reviewers whether the new APIs fit into those
>>> patterns or not.
>>
>> I think we're in that discussion now in this thread. I don't propose to change the
>> struct-embedding for sub devices used in SoC or south bridge or other complex
>> devices but only propose to not embed memory regions that are already documented as
>> and handled by QOM and simply allocate them and let QOM handle them so we only need
>> to reference them in the devices state unless they are needed for some reason by
>> the device methods which is rarely the case. So this is limited to memory regions
>> and the series only seems to add a lot of lines because of the extensive
>> documentation comments. The actual change is just factoring out actual memory
>> region init from memory_region_init functions then add a memory_region_new variant
>> that does object_new; do_init and keep the memory_region_init do object_initializel
>> do_init. Nothing else is changed, the way to manage and free regions based on ref
>> counting is already there this series just enables them to be actually used becase
>> currently despite what the docs say memory regions are either leaked or must be
>> embedded.
>
> I actually think deprecating struct-embedding for all QOM objects is a good idea.
>
> The problem initially stated in this thread is that embedding requires having extra
> field, but people see the benefit is too small. There is no real logic involved in
> having such fields so it does not reduce code complexity much; it saves some lines
> and that's it.
>
> However, I see another problem in struct embedding; it breaks object_ref(). When
> embedding, the child object effectively takes the reference to the storage of the
> parent object, but this reference is not counted, so use-after-free can happen if
> someone takes a reference to the child object with object_ref(). That is why the
> wrapper of object_ref() in rust/qom/src/qom.rs needs to be marked unsafe. Memory
> regions workaround this with memory_region_ref(), but it's not perfect since it
> relies on object_ref() in the end.
>
> For this reason I think object_initialize(), object_initialize_child(), and the like
> are better to be noted as deprecated in
> include/qom/object.h. Then memory_region_init() can be deprecated referring to them.
FWIW this is something I've been thinking about for some time: possibly I had a chat
with Phil about it at some point? Once example could be if you want to have a
reference to a parent type like PCIDevice that can change at runtime e.g. it could be
PCINE2000State or PCIPCNetState then you can never embed it, because you don't know
the size at compile time. So then why not use object_property_add_child() everywhere
so there is just a single way of doing things?
For memory regions it's a bit trickier because as per the virtio-gpu issues you've
been looking at, it is possible for the memory region to exist outside of its parent
device until it is destroyed later by the RCU thread. Is this something that can be
solved by manipulating the refcount?
I do agree with Peter too that we don't want to add yet another way of doing things:
if we decide to change the way memory regions work, we should go all-in and update
all callers to reflect the new API and remove the old one. Having one easily
understood way of modelling things makes life much easier for contributors and
reviewers alike.
ATB,
Mark.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v2 00/10] Implement memory_region_new_* functions
2026-01-30 20:08 ` Mark Cave-Ayland
@ 2026-01-30 21:31 ` BALATON Zoltan
0 siblings, 0 replies; 24+ messages in thread
From: BALATON Zoltan @ 2026-01-30 21:31 UTC (permalink / raw)
To: Mark Cave-Ayland
Cc: Akihiko Odaki, Peter Maydell, Peter Xu, qemu-devel, Paolo Bonzini,
Michael S. Tsirkin, Philippe Mathieu-Daudé,
Daniel P. Berrangé, Eduardo Habkost
On Fri, 30 Jan 2026, Mark Cave-Ayland wrote:
> On 29/01/2026 04:41, Akihiko Odaki wrote:
>> On 2026/01/29 0:46, BALATON Zoltan wrote:
>>> On Wed, 28 Jan 2026, Peter Maydell wrote:
>>>> On Wed, 28 Jan 2026 at 11:40, BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>>>> OK I try to summarise the motivation again:
>>>>>
>>>>> 1. Documentation in docs/devel/memory.rst says that memory regions'
>>>>> lifecycle is managed by QOM and they are freed with their owner or when
>>>>> nothing else uses them. This is also already implemented for a long time
>>>>> as described but cannot be used because the only constructors available
>>>>> kill this feature when calling object_initialize that clears the free
>>>>> function added by object_new. (The life time management is implemented
>>>>> through adding memory regions as children to the owner and unparenting
>>>>> them on freeing the owner which decreases ref count of the memory region
>>>>> and will free it when nothing else references it as far as I can tell.)
>>>>
>>>> If we have leaks because of our very common pattern of "embed
>>>> a MemoryRegion struct in the device state struct" then we must
>>>> fix those, because there's no way we're going to convert all
>>>> that existing code to a new set of APIs. But I was under the
>>>> impression we had already dealt with those, because MRs track
>>>> their owner's refcount, and don't have their own independent one ?
>>>
>>> I'm not sure if all those leaks are resolved as there were some patches
>>> and discussion about this recently but I think that problem or the need to
>>> use the owner's ref count to circumwent it instead of using the memory
>>> region's own ref count may also come from that there's currently no way to
>>> allocate memory regions that are ref counted and automatically freed as it
>>> should work with QOM and the documentation implies. (Only the constuctor
>>> is missing that is all this series adds, the mechanism is already there
>>> and implemented.) There may still be a problem with circular references if
>>> the memory region needs the owner so the owner can't be freed until the
>>> memory region is also freed but the memory region is not freed until the
>>> owner is freed but if both the owner and memory region used their own ref
>>> count things may become a bit less confusing and could be easier to find a
>>> way to break circular reference (e.g. by owner unparent child regions on
>>> unrealize but isn't freed until memory regions unref owner in their free
>>> method).
>>>
>>>>> These are my motivation for this change. What is the motivation for
>>>>> using
>>>>> embedded memory regions instead and against this change?
>>>>
>>>> Simply that it's a consistent pattern we use in a lot of the codebase:
>>>> the device embeds a lot of the structs it uses, rather than allocating
>>>> memory for them and keeping pointers to that allocated memory. We
>>>
>>> You mix in the issue of SoCs and complex devices using other devices in
>>> which case the recommendation was to embed those in the parent device so
>>> they don't have to be freed or kept track of by a pointer but won't be
>>> leaked. This series does not mean to change that, it's only limited to
>>> memory regions. (Although that problem may also stem from similar issue
>>> with object_initialize_child not allowing creating reference counted
>>> objects only initializing preallocated instances but that's not something
>>> this series touches.)
>>>
>>> We can say that memory regions are like other embedded objects but they
>>> are often used for sysbus and PCI devices only to be registered in the
>>> parent device that already has pointers in their state to track these so
>>> there's no need to keep track of them in the subclass if we can rely on
>>> QOM freeing them when not needed any more and this is already implemented
>>> and documented that way. So even if we keep embedding other child devices
>>> into complex parent devices that I think does not directly apply to memory
>>> regions and we could use what the documentation and implementation already
>>> allows and says for memory regions at least.
>>>
>>>> still have also various older device models that use the previous
>>>> pattern of "allocate memory and have pointers" too, but most new
>>>> code doesn't do that. I think we should for preference write code
>>>> in one pattern, not two, and "embed structs" seems to be what
>>>> we have mostly settled on for new code.
>>>>
>>>> There is an argument to be made that the pointer model would
>>>> fit better with a possible future world of "the user can wire
>>>> configurably wire up their own board model from devices", and
>>>> that it works better in a part-Rust-part-C world where the two
>>>> different languages don't have convenient access to the exact
>>>> size of structs defined in the other language. But that future
>>>> model is not something anybody has yet really fleshed out in any
>>>> detail, so it's still a bit speculative.
>>>
>>> You keep mentioning pointers but the point of ref counts and regisrering
>>> memory region as child of an owner is to avoid needing a pointer or
>>> embedding it in the subclass state as the relationship and lifecycle
>>> management are then handled by QOM. If we don't use that we could remove
>>> this from QOM and memory regions to simplify it but if it's already there
>>> and makes the device state simpler I think we better use it.
>>>
>>>> I'm not actually opposed to the idea of making a design decision
>>>> that this struct-embedding is no longer what we want to do, and defining
>>>> that something else is our new best practice for how to write devices.
>>>> But I think we would need to start by reaching a consensus that that
>>>> *is* what we want to do, and documenting that "best practice" somewhere
>>>> in docs/devel/. Then we can examine proposed new APIs and all be
>>>> on the same page about the design patterns we want and it will
>>>> be clearer to reviewers whether the new APIs fit into those
>>>> patterns or not.
>>>
>>> I think we're in that discussion now in this thread. I don't propose to
>>> change the struct-embedding for sub devices used in SoC or south bridge or
>>> other complex devices but only propose to not embed memory regions that
>>> are already documented as and handled by QOM and simply allocate them and
>>> let QOM handle them so we only need to reference them in the devices state
>>> unless they are needed for some reason by the device methods which is
>>> rarely the case. So this is limited to memory regions and the series only
>>> seems to add a lot of lines because of the extensive documentation
>>> comments. The actual change is just factoring out actual memory region
>>> init from memory_region_init functions then add a memory_region_new
>>> variant that does object_new; do_init and keep the memory_region_init do
>>> object_initializel do_init. Nothing else is changed, the way to manage and
>>> free regions based on ref counting is already there this series just
>>> enables them to be actually used becase currently despite what the docs
>>> say memory regions are either leaked or must be embedded.
>>
>> I actually think deprecating struct-embedding for all QOM objects is a good
>> idea.
>>
>> The problem initially stated in this thread is that embedding requires
>> having extra field, but people see the benefit is too small. There is no
>> real logic involved in having such fields so it does not reduce code
>> complexity much; it saves some lines and that's it.
>>
>> However, I see another problem in struct embedding; it breaks object_ref().
>> When embedding, the child object effectively takes the reference to the
>> storage of the parent object, but this reference is not counted, so
>> use-after-free can happen if someone takes a reference to the child object
>> with object_ref(). That is why the wrapper of object_ref() in
>> rust/qom/src/qom.rs needs to be marked unsafe. Memory regions workaround
>> this with memory_region_ref(), but it's not perfect since it relies on
>> object_ref() in the end.
>>
>> For this reason I think object_initialize(), object_initialize_child(), and
>> the like are better to be noted as deprecated in
>> include/qom/object.h. Then memory_region_init() can be deprecated referring
>> to them.
>
> FWIW this is something I've been thinking about for some time: possibly I had
> a chat with Phil about it at some point? Once example could be if you want to
> have a reference to a parent type like PCIDevice that can change at runtime
> e.g. it could be PCINE2000State or PCIPCNetState then you can never embed it,
> because you don't know the size at compile time. So then why not use
> object_property_add_child() everywhere so there is just a single way of doing
> things?
>
> For memory regions it's a bit trickier because as per the virtio-gpu issues
> you've been looking at, it is possible for the memory region to exist outside
> of its parent device until it is destroyed later by the RCU thread. Is this
> something that can be solved by manipulating the refcount?
I think if memory regions could use their own ref count instead of their
parents' this could become simpler and these memory_region_new functions
allow us to create memory regions that are ref counted and free'd by QOM
so this could help but I don't know the details of that problem.
> I do agree with Peter too that we don't want to add yet another way of doing
> things: if we decide to change the way memory regions work, we should go
> all-in and update all callers to reflect the new API and remove the old one.
> Having one easily understood way of modelling things makes life much easier
> for contributors and reviewers alike.
That can be a goal but this would happen gradually and not all in this
series. I want to use memory_region_new as much as I can in devices I
maintain and a few more I come across but I certainly won't change every
device and that should not be required as it's not feasible for me to
change stuff I don't even know anything about and do all that in my free
time.
I'm not proposing a completely new API and don't change how memory regions
work. All I propose is to allow QOM to manage memory regions as is already
documented by adding memory_region_new functions corresponding to
memory_region_init functions that work exactly the same with the only
difference that memory regions created with memory_region_init have to be
managed by the caller while memory regions allocated with
memory_region_new is managed by QOM and will be automatically freed when
the last reference goes away. That's all, this should not be that hard to
understand for contributors and reviewers.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 24+ messages in thread