[PATCH 2/6] memory: Factor out common ram region initialization

BALATON Zoltan posted 6 patches 1 month, 2 weeks ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@kernel.org>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
There is a newer version of this series
[PATCH 2/6] memory: Factor out common ram region initialization
Posted by BALATON Zoltan 1 month, 2 weeks ago
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 a003095632..87c8e78662 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
Re: [PATCH 2/6] memory: Factor out common ram region initialization
Posted by Peter Xu 1 month, 2 weeks ago
On Tue, Dec 23, 2025 at 10:49:58PM +0100, BALATON Zoltan wrote:
> -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 this is moved out, the err below will never be set.  Maybe this helper
then doesn't need errp at all.

>      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;
>  }

-- 
Peter Xu
Re: [PATCH 2/6] memory: Factor out common ram region initialization
Posted by BALATON Zoltan 1 week, 5 days ago
On Wed, 24 Dec 2025, Peter Xu wrote:
> On Tue, Dec 23, 2025 at 10:49:58PM +0100, BALATON Zoltan wrote:
>> -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 this is moved out, the err below will never be set.  Maybe this helper
> then doesn't need errp at all.

err is an input parameter to this function as it consolidates common error 
handling so it does not have to be repeated at every caller.

Regards,
BALATON Zoltan

>>      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;
>>  }
>
>