[PATCH v6 0/8] memory: Remove most _nomigrate variants

[PATCH v6 0/8] memory: Remove most _nomigrate variants

[BALATON Zoltan]

v6:
- keep the last two uses of memory_region_init_ram_nomigrate in vga and xtfpga for now
- added R-b tags

v5:
- convert Sun machines and their display devices from global vmstate

v4:
- separate patch converting Sun machines from memory_region_init_ram_nomigrate
- split helper to init ram into two functions: setup and error_propagate
- also use memory_region_init_io in memory_region_init_ram_device_ptr

v3:
- rebased on master after some patches were merged
- drop some more line from memory-region-housekeeping.cocci
- added comment to explain what factored out helper does
- some more clean ups included

BALATON Zoltan (8):
  hw/display/{cg3.tcx}: Do not use memory_region_init_rom_nomigrate()
  memory: Remove memory_region_init_rom_nomigrate()
  sun4m,sun4u,tcx: Do not use memory_region_init_ram_nomigrate()
  system/memory: Reduce memory_region_init_ram_nomigrate() usage
  memory: Factor out common ram region initialization
  memory: Add internal memory_region_register_ram function
  memory: Shorten memory_region_init_ram_device_ptr and
    memory_region_init_rom_device
  memory: Factor out more common ram region initialization

 docs/devel/memory.rst                         |   9 +-
 hw/display/cg3.c                              |   5 +-
 hw/display/tcx.c                              |   8 +-
 hw/sparc/sun4m.c                              |  19 +-
 hw/sparc64/sun4u.c                            |  10 +-
 include/system/memory.h                       |  26 --
 .../memory-region-housekeeping.cocci          |  47 ----
 system/memory.c                               | 259 ++++++------------
 8 files changed, 104 insertions(+), 279 deletions(-)

-- 
2.41.3

[PATCH v6 1/8] hw/display/{cg3.tcx}: Do not use memory_region_init_rom_nomigrate()

[BALATON Zoltan]

Use memory_region_init_rom() instead which is what other devices do.
This breaks migration but these devices are only used by sparc Sun
machines which have no migration compatibility guarantee.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/cg3.c | 5 ++---
 hw/display/tcx.c | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/display/cg3.c b/hw/display/cg3.c
index 568d6048a6..61bdb0552e 100644
--- a/hw/display/cg3.c
+++ b/hw/display/cg3.c
@@ -282,8 +282,8 @@ static void cg3_initfn(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     CG3State *s = CG3(obj);
 
-    memory_region_init_rom_nomigrate(&s->rom, obj, "cg3.prom",
-                                     FCODE_MAX_ROM_SIZE, &error_fatal);
+    memory_region_init_rom(&s->rom, obj, "cg3.prom", FCODE_MAX_ROM_SIZE,
+                           &error_fatal);
     sysbus_init_mmio(sbd, &s->rom);
 
     memory_region_init_io(&s->reg, obj, &cg3_reg_ops, s, "cg3.reg",
@@ -299,7 +299,6 @@ static void cg3_realizefn(DeviceState *dev, Error **errp)
     char *fcode_filename;
 
     /* FCode ROM */
-    vmstate_register_ram_global(&s->rom);
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, CG3_ROM_FILE);
     if (fcode_filename) {
         ret = load_image_mr(fcode_filename, &s->rom);
diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 36cad82abd..16114b9bb8 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -756,8 +756,8 @@ static void tcx_initfn(Object *obj)
     SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
     TCXState *s = TCX(obj);
 
-    memory_region_init_rom_nomigrate(&s->rom, obj, "tcx.prom",
-                                     FCODE_MAX_ROM_SIZE, &error_fatal);
+    memory_region_init_rom(&s->rom, obj, "tcx.prom", FCODE_MAX_ROM_SIZE,
+                           &error_fatal);
     sysbus_init_mmio(sbd, &s->rom);
 
     /* 2/STIP : Stippler */
@@ -822,7 +822,6 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     vram_base = memory_region_get_ram_ptr(&s->vram_mem);
 
     /* 10/ROM : FCode ROM */
-    vmstate_register_ram_global(&s->rom);
     fcode_filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, TCX_ROM_FILE);
     if (fcode_filename) {
         ret = load_image_mr(fcode_filename, &s->rom);
-- 
2.41.3

[PATCH v6 2/8] memory: Remove memory_region_init_rom_nomigrate()

[BALATON Zoltan]

All users were converted so no longer needed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 docs/devel/memory.rst                         |  1 -
 include/system/memory.h                       | 26 -----------------
 .../memory-region-housekeeping.cocci          | 28 -------------------
 system/memory.c                               | 19 ++-----------
 4 files changed, 3 insertions(+), 71 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 8558f70a42..0bb5acab21 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -113,7 +113,6 @@ have a special case where you need to manage the migration of
 the backing memory yourself, you can call the functions:
 
 - memory_region_init_ram_nomigrate()
-- memory_region_init_rom_nomigrate()
 
 which only initialize the MemoryRegion and leave handling
 migration to the caller.
diff --git a/include/system/memory.h b/include/system/memory.h
index 0562af3136..7117699b10 100644
--- a/include/system/memory.h
+++ b/include/system/memory.h
@@ -1588,32 +1588,6 @@ void memory_region_init_alias(MemoryRegion *mr,
                               hwaddr offset,
                               uint64_t size);
 
-/**
- * memory_region_init_rom_nomigrate: Initialize a ROM memory region.
- *
- * This has the same effect as calling memory_region_init_ram_nomigrate()
- * and then marking the resulting region read-only with
- * memory_region_set_readonly().
- *
- * 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.
- *
- * @mr: the #MemoryRegion to be initialized.
- * @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: true on success, else false setting @errp with error.
- */
-bool memory_region_init_rom_nomigrate(MemoryRegion *mr,
-                                      Object *owner,
-                                      const char *name,
-                                      uint64_t size,
-                                      Error **errp);
-
 /**
  * memory_region_init_iommu: Initialize a memory region of a custom type
  * that translates addresses
diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci b/scripts/coccinelle/memory-region-housekeeping.cocci
index 7f89e9712e..e45703141a 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -16,17 +16,10 @@
 expression E1, E2, E3, E4, E5;
 symbol true;
 @@
-(
 - memory_region_init_ram(E1, E2, E3, E4, E5);
 + memory_region_init_rom(E1, E2, E3, E4, E5);
   ... WHEN != E1
 - memory_region_set_readonly(E1, true);
-|
-- memory_region_init_ram_nomigrate(E1, E2, E3, E4, E5);
-+ memory_region_init_rom_nomigrate(E1, E2, E3, E4, E5);
-  ... WHEN != E1
-- memory_region_set_readonly(E1, true);
-)
 
 
 @possible_memory_region_init_rom@
@@ -53,11 +46,7 @@ cocci.print_main("potential use of memory_region_init_rom*() in ", p)
 expression ROM, E1, E2, E3, E4;
 expression ALIAS, E5, E6, E7, E8;
 @@
-(
   memory_region_init_rom(ROM, E1, E2, E3, E4);
-|
-  memory_region_init_rom_nomigrate(ROM, E1, E2, E3, E4);
-)
   ...
    memory_region_init_alias(ALIAS, E5, E6, ROM, E7, E8);
 -  memory_region_set_readonly(ALIAS, true);
@@ -80,23 +69,6 @@ expression ERRP;
  ...
 -vmstate_register_ram_global(MR);
 @@
-expression MR;
-expression NAME;
-expression SIZE;
-expression ERRP;
-@@
--memory_region_init_rom_nomigrate(MR, NULL, NAME, SIZE, ERRP);
-+memory_region_init_rom(MR, NULL, NAME, SIZE, ERRP);
- ...
--vmstate_register_ram_global(MR);
-@@
-expression MR;
-expression OPS;
-expression OPAQUE;
-expression NAME;
-expression SIZE;
-expression ERRP;
-@@
 typedef DeviceState;
 identifier device_fn, dev, obj;
 expression E1, E2, E3, E4, E5;
diff --git a/system/memory.c b/system/memory.c
index c51d0798a8..65042bd9fa 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1738,21 +1738,6 @@ void memory_region_init_alias(MemoryRegion *mr,
     mr->alias_offset = offset;
 }
 
-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;
-    }
-    mr->readonly = true;
-
-    return true;
-}
-
 void memory_region_init_iommu(void *_iommu_mr,
                               size_t instance_size,
                               const char *mrtypename,
@@ -3757,9 +3742,11 @@ bool memory_region_init_rom(MemoryRegion *mr,
 {
     DeviceState *owner_dev;
 
-    if (!memory_region_init_rom_nomigrate(mr, owner, name, size, errp)) {
+    if (!memory_region_init_ram_flags_nomigrate(mr, owner, name,
+                                                size, 0, errp)) {
         return false;
     }
+    mr->readonly = true;
     /* 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
-- 
2.41.3

[PATCH v6 3/8] sun4m,sun4u,tcx: Do not use memory_region_init_ram_nomigrate()

[BALATON Zoltan]

Convert to use memory_region_init_{ram,rom} instead. This breaks
migration but these machines have no migration compatibility guarantee
and this removes most remaining usages of this nomigrate variant.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/display/tcx.c   |  3 +--
 hw/sparc/sun4m.c   | 19 +++++--------------
 hw/sparc64/sun4u.c | 10 +++-------
 3 files changed, 9 insertions(+), 23 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 16114b9bb8..c8a4ac21ca 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -815,9 +815,8 @@ static void tcx_realizefn(DeviceState *dev, Error **errp)
     uint8_t *vram_base;
     char *fcode_filename;
 
-    memory_region_init_ram_nomigrate(&s->vram_mem, OBJECT(s), "tcx.vram",
+    memory_region_init_ram(&s->vram_mem, OBJECT(s), "tcx.vram",
                            s->vram_size * (1 + 4 + 4), &error_fatal);
-    vmstate_register_ram_global(&s->vram_mem);
     memory_region_set_log(&s->vram_mem, true, DIRTY_MEMORY_VGA);
     vram_base = memory_region_get_ram_ptr(&s->vram_mem);
 
diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
index a17bdb3692..47853e3f76 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -587,13 +587,10 @@ static void idreg_realize(DeviceState *ds, Error **errp)
     IDRegState *s = MACIO_ID_REGISTER(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
 
-    if (!memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.idreg",
-                                          sizeof(idreg_data), errp)) {
+    if (!memory_region_init_rom(&s->mem, OBJECT(ds), "sun4m.idreg",
+                                sizeof(idreg_data), errp)) {
         return;
     }
-
-    vmstate_register_ram_global(&s->mem);
-    memory_region_set_readonly(&s->mem, true);
     sysbus_init_mmio(dev, &s->mem);
 }
 
@@ -638,12 +635,9 @@ static void afx_realize(DeviceState *ds, Error **errp)
     AFXState *s = TCX_AFX(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
 
-    if (!memory_region_init_ram_nomigrate(&s->mem, OBJECT(ds), "sun4m.afx",
-                                          4, errp)) {
+    if (!memory_region_init_ram(&s->mem, OBJECT(ds), "sun4m.afx", 4, errp)) {
         return;
     }
-
-    vmstate_register_ram_global(&s->mem);
     sysbus_init_mmio(dev, &s->mem);
 }
 
@@ -719,13 +713,10 @@ static void prom_realize(DeviceState *ds, Error **errp)
     PROMState *s = OPENPROM(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
 
-    if (!memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4m.prom",
-                                          PROM_SIZE_MAX, errp)) {
+    if (!memory_region_init_rom(&s->prom, OBJECT(ds), "sun4m.prom",
+                                PROM_SIZE_MAX, errp)) {
         return;
     }
-
-    vmstate_register_ram_global(&s->prom);
-    memory_region_set_readonly(&s->prom, true);
     sysbus_init_mmio(dev, &s->prom);
 }
 
diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
index b8bda1eb81..2e41785b78 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -455,13 +455,10 @@ static void prom_realize(DeviceState *ds, Error **errp)
     PROMState *s = OPENPROM(ds);
     SysBusDevice *dev = SYS_BUS_DEVICE(ds);
 
-    if (!memory_region_init_ram_nomigrate(&s->prom, OBJECT(ds), "sun4u.prom",
-                                          PROM_SIZE_MAX, errp)) {
+    if (!memory_region_init_rom(&s->prom, OBJECT(ds), "sun4u.prom",
+                                PROM_SIZE_MAX, errp)) {
         return;
     }
-
-    vmstate_register_ram_global(&s->prom);
-    memory_region_set_readonly(&s->prom, true);
     sysbus_init_mmio(dev, &s->prom);
 }
 
@@ -498,9 +495,8 @@ static void ram_realize(DeviceState *dev, Error **errp)
     RamDevice *d = SUN4U_RAM(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
 
-    memory_region_init_ram_nomigrate(&d->ram, OBJECT(d), "sun4u.ram", d->size,
+    memory_region_init_ram(&d->ram, OBJECT(d), "sun4u.ram", d->size,
                            &error_fatal);
-    vmstate_register_ram_global(&d->ram);
     sysbus_init_mmio(sbd, &d->ram);
 }
 
-- 
2.41.3

[PATCH v6 4/8] system/memory: Reduce memory_region_init_ram_nomigrate() usage

[BALATON Zoltan]

This function is kept until the last two usages can be removed but
convert the last usage in memory.c and remove it from the
documentation.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 docs/devel/memory.rst                         |  8 +++-----
 .../memory-region-housekeeping.cocci          | 19 -------------------
 system/memory.c                               |  3 ++-
 3 files changed, 5 insertions(+), 25 deletions(-)

diff --git a/docs/devel/memory.rst b/docs/devel/memory.rst
index 0bb5acab21..9083b18f08 100644
--- a/docs/devel/memory.rst
+++ b/docs/devel/memory.rst
@@ -110,11 +110,9 @@ migrated:
 
 For most devices and boards this is the correct thing. If you
 have a special case where you need to manage the migration of
-the backing memory yourself, you can call the functions:
-
-- memory_region_init_ram_nomigrate()
-
-which only initialize the MemoryRegion and leave handling
+the backing memory yourself, you can call the function
+memory_region_init_ram_flags_nomigrate()
+which only initializes the MemoryRegion and leaves handling
 migration to the caller.
 
 The functions:
diff --git a/scripts/coccinelle/memory-region-housekeeping.cocci b/scripts/coccinelle/memory-region-housekeeping.cocci
index e45703141a..b23647a3d8 100644
--- a/scripts/coccinelle/memory-region-housekeeping.cocci
+++ b/scripts/coccinelle/memory-region-housekeeping.cocci
@@ -26,15 +26,9 @@ symbol true;
 expression E1, E2, E3, E4, E5;
 position p;
 @@
-(
   memory_region_init_ram@p(E1, E2, E3, E4, E5);
   ...
   memory_region_set_readonly(E1, true);
-|
-  memory_region_init_ram_nomigrate@p(E1, E2, E3, E4, E5);
-  ...
-  memory_region_set_readonly(E1, true);
-)
 @script:python@
 p << possible_memory_region_init_rom.p;
 @@
@@ -52,23 +46,10 @@ expression ALIAS, E5, E6, E7, E8;
 -  memory_region_set_readonly(ALIAS, true);
 
 
-// Replace by-hand memory_region_init_ram_nomigrate/vmstate_register_ram
-// code sequences with use of the new memory_region_init_ram function.
-// Similarly for the _rom and _rom_device functions.
 // We don't try to replace sequences with a non-NULL owner, because
 // there are none in the tree that can be automatically converted
 // (and only a handful that can be manually converted).
 @@
-expression MR;
-expression NAME;
-expression SIZE;
-expression ERRP;
-@@
--memory_region_init_ram_nomigrate(MR, NULL, NAME, SIZE, ERRP);
-+memory_region_init_ram(MR, NULL, NAME, SIZE, ERRP);
- ...
--vmstate_register_ram_global(MR);
-@@
 typedef DeviceState;
 identifier device_fn, dev, obj;
 expression E1, E2, E3, E4, E5;
diff --git a/system/memory.c b/system/memory.c
index 65042bd9fa..e8c4912a60 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3695,7 +3695,8 @@ bool memory_region_init_ram(MemoryRegion *mr,
 {
     DeviceState *owner_dev;
 
-    if (!memory_region_init_ram_nomigrate(mr, owner, name, size, errp)) {
+    if (!memory_region_init_ram_flags_nomigrate(mr, owner, name,
+                                                size, 0, errp)) {
         return false;
     }
     /* This will assert if owner is neither NULL nor a DeviceState.
-- 
2.41.3

[PATCH v6 5/8] memory: Factor out common ram region initialization

[BALATON Zoltan]

Introduce internal helper functions to remove duplicated code from
different memory_region_init_*ram functions.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 system/memory.c | 132 ++++++++++++++++++------------------------------
 1 file changed, 50 insertions(+), 82 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index e8c4912a60..1b26c6b5a5 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1589,19 +1589,16 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
                                                   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 void memory_region_setup_ram(MemoryRegion *mr)
 {
-    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);
+}
+
+static bool memory_region_error_propagate(MemoryRegion *mr,
+                                          Error *err, Error **errp)
+{
     if (err) {
         mr->size = int128_zero();
         object_unparent(OBJECT(mr));
@@ -1611,6 +1608,18 @@ 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);
+    memory_region_setup_ram(mr);
+    mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
+    return memory_region_error_propagate(mr, err, errp);
+}
+
 bool memory_region_init_resizeable_ram(MemoryRegion *mr,
                                        Object *owner,
                                        const char *name,
@@ -1622,108 +1631,69 @@ 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;
+    memory_region_setup_ram(mr);
+    mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
+                                              &err);
+    return memory_region_error_propagate(mr, err, errp);
 }
 
 #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;
+    memory_region_setup_ram(mr);
     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_error_propagate(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;
+    memory_region_setup_ram(mr);
     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_error_propagate(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;
-
+    memory_region_setup_ram(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);
 }
 
-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;
-
+    memory_region_setup_ram(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);
+    mr->ram_device = true;
 }
 
 void memory_region_init_alias(MemoryRegion *mr,
@@ -3774,15 +3744,13 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
     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;
+    memory_region_setup_ram(mr);
     mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
-    if (err) {
-        mr->size = int128_zero();
-        object_unparent(OBJECT(mr));
-        error_propagate(errp, err);
+    if (!memory_region_error_propagate(mr, err, errp)) {
         return false;
     }
+    mr->ram = false;
+    mr->rom_device = true;
     /* 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
-- 
2.41.3

[PATCH v6 6/8] memory: Add internal memory_region_register_ram function

[BALATON Zoltan]

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 | 76 ++++++++++++++-----------------------------------
 1 file changed, 21 insertions(+), 55 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 1b26c6b5a5..f75ca015a9 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3657,18 +3657,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_flags_nomigrate(mr, owner, name,
-                                                size, 0, 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
@@ -3677,68 +3669,50 @@ 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_flags_nomigrate(mr, owner, name, size,
+                                                0, 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_ram_flags_nomigrate(mr, owner, name,
                                                 size, 0, errp)) {
         return false;
     }
     mr->readonly = true;
-    /* 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;
     Error *err = NULL;
 
     assert(ops);
@@ -3751,15 +3725,7 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
     }
     mr->ram = false;
     mr->rom_device = true;
-    /* 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

[PATCH v6 7/8] memory: Shorten memory_region_init_ram_device_ptr and memory_region_init_rom_device

[BALATON Zoltan]

We can reuse memory_region_init_io in these functions. Also shorten
some other function prototypes.

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

diff --git a/system/memory.c b/system/memory.c
index f75ca015a9..5c6514b985 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);
@@ -1687,8 +1684,7 @@ 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);
+    memory_region_init_io(mr, owner, &ram_device_mem_ops, mr, name, size);
     memory_region_setup_ram(mr);
     /* qemu_ram_alloc_from_ptr cannot fail with ptr != NULL.  */
     assert(ptr != NULL);
@@ -1696,12 +1692,9 @@ 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;
@@ -3716,8 +3709,7 @@ bool memory_region_init_rom_device(MemoryRegion *mr, Object *owner,
     Error *err = NULL;
 
     assert(ops);
-    memory_region_init(mr, owner, name, size);
-    memory_region_set_ops(mr, ops, opaque);
+    memory_region_init_io(mr, owner, ops, opaque, name, size);
     memory_region_setup_ram(mr);
     mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
     if (!memory_region_error_propagate(mr, err, errp)) {
-- 
2.41.3

[PATCH v6 8/8] memory: Factor out more common ram region initialization

[BALATON Zoltan]

Introduce internal memory_region_setup_ram_ptr() function to remove
duplicated code from different memory_region_init_ram_*ptr functions.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 system/memory.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 5c6514b985..aecd85b45b 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1670,25 +1670,28 @@ 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_setup_ram_ptr(MemoryRegion *mr, uint64_t size,
+                                        void *ptr)
 {
-    memory_region_init(mr, owner, name, size);
     memory_region_setup_ram(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);
 }
 
+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_setup_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_io(mr, owner, &ram_device_mem_ops, mr, name, size);
-    memory_region_setup_ram(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_setup_ram_ptr(mr, size, ptr);
     mr->ram_device = true;
 }
 
-- 
2.41.3

Re: [PATCH v6 5/8] memory: Factor out common ram region initialization

[Akihiko Odaki]

On 2026/03/04 6:47, BALATON Zoltan wrote:
> Introduce internal helper functions to remove duplicated code from
> different memory_region_init_*ram functions.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   system/memory.c | 132 ++++++++++++++++++------------------------------
>   1 file changed, 50 insertions(+), 82 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index e8c4912a60..1b26c6b5a5 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1589,19 +1589,16 @@ bool memory_region_init_ram_nomigrate(MemoryRegion *mr,
>                                                     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 void memory_region_setup_ram(MemoryRegion *mr)
>   {
> -    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);
> +}
> +
> +static bool memory_region_error_propagate(MemoryRegion *mr,
> +                                          Error *err, Error **errp)
> +{
>       if (err) {
>           mr->size = int128_zero();
>           object_unparent(OBJECT(mr));
> @@ -1611,6 +1608,18 @@ bool memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>       return true;
>   }

I think the optimal way to factor out error propagation is to use 
ERRP_GUARD().

Regards,
Akihiko Odaki

>   
> +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);
> +    memory_region_setup_ram(mr);
> +    mr->ram_block = qemu_ram_alloc(size, ram_flags, mr, &err);
> +    return memory_region_error_propagate(mr, err, errp);
> +}
> +
>   bool memory_region_init_resizeable_ram(MemoryRegion *mr,
>                                          Object *owner,
>                                          const char *name,
> @@ -1622,108 +1631,69 @@ 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;
> +    memory_region_setup_ram(mr);
> +    mr->ram_block = qemu_ram_alloc_resizeable(size, max_size, resized, mr,
> +                                              &err);
> +    return memory_region_error_propagate(mr, err, errp);
>   }
>   
>   #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;
> +    memory_region_setup_ram(mr);
>       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_error_propagate(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;
> +    memory_region_setup_ram(mr);
>       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_error_propagate(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;
> -
> +    memory_region_setup_ram(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);
>   }
>   
> -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;
> -
> +    memory_region_setup_ram(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);
> +    mr->ram_device = true;
>   }
>   
>   void memory_region_init_alias(MemoryRegion *mr,
> @@ -3774,15 +3744,13 @@ bool memory_region_init_rom_device(MemoryRegion *mr,
>       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;
> +    memory_region_setup_ram(mr);
>       mr->ram_block = qemu_ram_alloc(size, 0, mr, &err);
> -    if (err) {
> -        mr->size = int128_zero();
> -        object_unparent(OBJECT(mr));
> -        error_propagate(errp, err);
> +    if (!memory_region_error_propagate(mr, err, errp)) {
>           return false;
>       }
> +    mr->ram = false;
> +    mr->rom_device = true;
>       /* 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

Re: [PATCH v6 5/8] memory: Factor out common ram region initialization

[BALATON Zoltan]

On Wed, 4 Mar 2026, Akihiko Odaki wrote:
> On 2026/03/04 6:47, BALATON Zoltan wrote:
>> Introduce internal helper functions to remove duplicated code from
>> different memory_region_init_*ram functions.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   system/memory.c | 132 ++++++++++++++++++------------------------------
>>   1 file changed, 50 insertions(+), 82 deletions(-)
>> 
>> diff --git a/system/memory.c b/system/memory.c
>> index e8c4912a60..1b26c6b5a5 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -1589,19 +1589,16 @@ bool memory_region_init_ram_nomigrate(MemoryRegion 
>> *mr,
>>                                                     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 void memory_region_setup_ram(MemoryRegion *mr)
>>   {
>> -    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);
>> +}
>> +
>> +static bool memory_region_error_propagate(MemoryRegion *mr,
>> +                                          Error *err, Error **errp)
>> +{
>>       if (err) {
>>           mr->size = int128_zero();
>>           object_unparent(OBJECT(mr));
>> @@ -1611,6 +1608,18 @@ bool 
>> memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>>       return true;
>>   }
>
> I think the optimal way to factor out error propagation is to use 
> ERRP_GUARD().

I don't think ERRP_GUARD applies here as we do not dereference errp and 
this does more than just propagate the error so I don't see how ERRP_GUARD 
could help here.

Regards,
BALATON Zoltan

Re: [PATCH v6 0/8] memory: Remove most _nomigrate variants

[Peter Xu]

On Tue, Mar 03, 2026 at 10:47:49PM +0100, BALATON Zoltan wrote:
> BALATON Zoltan (8):
>   hw/display/{cg3.tcx}: Do not use memory_region_init_rom_nomigrate()
>   memory: Remove memory_region_init_rom_nomigrate()
>   sun4m,sun4u,tcx: Do not use memory_region_init_ram_nomigrate()

If no objections, I can queue these three patch (1-3) for 11.0-rc0 for now.

Thanks,

-- 
Peter Xu

Re: [PATCH v6 0/8] memory: Remove most _nomigrate variants

[BALATON Zoltan]

On Wed, 4 Mar 2026, Peter Xu wrote:
> On Tue, Mar 03, 2026 at 10:47:49PM +0100, BALATON Zoltan wrote:
>> BALATON Zoltan (8):
>>   hw/display/{cg3.tcx}: Do not use memory_region_init_rom_nomigrate()
>>   memory: Remove memory_region_init_rom_nomigrate()
>>   sun4m,sun4u,tcx: Do not use memory_region_init_ram_nomigrate()
>
> If no objections, I can queue these three patch (1-3) for 11.0-rc0 for now.

What about the rest of this series? These are all simple clean ups so 
should not be hard to review. Patches 4-5 and 7-8 need review. Anybody?

Regards,
BALATON Zoltan

Re: [PATCH v6 5/8] memory: Factor out common ram region initialization

[Akihiko Odaki]

On 2026/03/04 20:38, BALATON Zoltan wrote:
> On Wed, 4 Mar 2026, Akihiko Odaki wrote:
>> On 2026/03/04 6:47, BALATON Zoltan wrote:
>>> Introduce internal helper functions to remove duplicated code from
>>> different memory_region_init_*ram functions.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>   system/memory.c | 132 ++++++++++++++++++------------------------------
>>>   1 file changed, 50 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/system/memory.c b/system/memory.c
>>> index e8c4912a60..1b26c6b5a5 100644
>>> --- a/system/memory.c
>>> +++ b/system/memory.c
>>> @@ -1589,19 +1589,16 @@ bool 
>>> memory_region_init_ram_nomigrate(MemoryRegion *mr,
>>>                                                     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 void memory_region_setup_ram(MemoryRegion *mr)
>>>   {
>>> -    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);
>>> +}
>>> +
>>> +static bool memory_region_error_propagate(MemoryRegion *mr,
>>> +                                          Error *err, Error **errp)
>>> +{
>>>       if (err) {
>>>           mr->size = int128_zero();
>>>           object_unparent(OBJECT(mr));
>>> @@ -1611,6 +1608,18 @@ bool 
>>> memory_region_init_ram_flags_nomigrate(MemoryRegion *mr,
>>>       return true;
>>>   }
>>
>> I think the optimal way to factor out error propagation is to use 
>> ERRP_GUARD().
> 
> I don't think ERRP_GUARD applies here as we do not dereference errp and 
> this does more than just propagate the error so I don't see how 
> ERRP_GUARD could help here.

include/qapi/error.h has an extensive documentation describing how 
errors should be passed around:

 > Call a function, receive an error from it, and pass it to the caller
 > - when the function returns a value that indicates failure, say
 >   false:
 >     if (!foo(arg, errp)) {
 >         handle the error...
 >     }
 > - when it does not, say because it is a void function:
 >     ERRP_GUARD();
 >     foo(arg, errp);
 >     if (*errp) {
 >         handle the error...
 >     }
 > More on ERRP_GUARD() below.
 >
 > Code predating ERRP_GUARD() still exists, and looks like this:
 >     Error *err = NULL;
 >     foo(arg, &err);
 >     if (err) {
 >         handle the error...
 >         error_propagate(errp, err); // deprecated
 >     }
 > Avoid in new code.  Do *not* "optimize" it to
 >     foo(arg, errp);
 >     if (*errp) { // WRONG!
 >         handle the error...
 >     }
 > because errp may be NULL without the ERRP_GUARD() guard.
 >
 > But when all you do with the error is pass it on, please use
 >     foo(arg, errp);
 > for readability.
 >
 > Receive an error, and handle it locally
 > - when the function returns a value that indicates failure, say
 >   false:
 >     Error *err = NULL;
 >     if (!foo(arg, &err)) {
 >         handle the error...
 >     }
 > - when it does not, say because it is a void function:
 >     Error *err = NULL;
 >     foo(arg, &err);
 >     if (err) {
 >         handle the error...
 >     }
 >
 > Pass an existing error to the caller:
 >     error_propagate(errp, err);
 > This is rarely needed.  When @err is a local variable, use of
 > ERRP_GUARD() commonly results in more readable code.

But looking at the code, the functions generating errors (e.g., 
qemu_ram_alloc()) return values that indicate failures (NULL), so I now 
think we should use the first pattern I cited (i.e., check the returned 
value instead of err) and remove the err variable and error_propagate() 
altogether instead of factoring them out with 
memory_region_error_propagate() or ERRP_GUARD().

Regards,
Akihiko Odaki

Re: [PATCH v6 5/8] memory: Factor out common ram region initialization

[Peter Xu]

On Thu, Mar 05, 2026 at 10:57:19AM +0900, Akihiko Odaki wrote:
> But looking at the code, the functions generating errors (e.g.,
> qemu_ram_alloc()) return values that indicate failures (NULL), so I now
> think we should use the first pattern I cited (i.e., check the returned
> value instead of err) and remove the err variable and error_propagate()
> altogether instead of factoring them out with
> memory_region_error_propagate() or ERRP_GUARD().

I agree, this looks better.

-- 
Peter Xu