[Qemu-devel] [PATCH v4 1/6] memory: add memory_region_init_ram_may_fail() and memory_region_init_ram_ptr_may_fail()

[Hu Tao]

These two are almost the same as memory_region_init_ram() and
memory_region_init_ram_ptr() except that they have an extra errp
parameter to let callers handle error. The purpose is to fix the bug
described below.

We should have added errp directly to memory_region_ram(), but that
mixes updates to calls to memory_region_ram() and this bug fix and make
it hard to review.

We will rename _may_fail variants later so that we will have two versions
of API: one with errp parameter(memory_region_init_ram(),
memory_region_init_ram_ptr()), one without errp parameter and with
suffix _nofail.

In hostmem-ram.c we call memory_region_init_ram_may_fail() now rather
than memory_region_init_ram() so that error can be handled.  This
solves a problem that qemu just exits when using monitor command
object_add to add a memory backend whose size is way too large.  In the
case we'd better give an error message and keep guest running.

The problem can be reproduced as follows:

1. run qemu
2. (monitor)object_add memory-backend-ram,size=100000G,id=ram0

Signed-off-by: Hu Tao <address@hidden>
---
 backends/hostmem-ram.c  |  4 +--
 exec.c                  | 32 +++++++++++++++++-------
 include/exec/memory.h   | 37 +++++++++++++++++++++++++++
 include/exec/ram_addr.h |  4 +--
 memory.c                | 66 ++++++++++++++++++++++++++++++++++++++++---------
 5 files changed, 119 insertions(+), 24 deletions(-)

diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index d9a8290..d0e92ad 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -26,8 +26,8 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error 
**errp)
     }
 
     path = object_get_canonical_path_component(OBJECT(backend));
-    memory_region_init_ram(&backend->mr, OBJECT(backend), path,
-                           backend->size);
+    memory_region_init_ram_may_fail(&backend->mr, OBJECT(backend), path,
+                                    backend->size, errp);
     g_free(path);
 }
 
diff --git a/exec.c b/exec.c
index 765bd94..7e60a44 100644
--- a/exec.c
+++ b/exec.c
@@ -1224,7 +1224,7 @@ static int memory_try_enable_merging(void *addr, size_t 
len)
     return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
 }
 
-static ram_addr_t ram_block_add(RAMBlock *new_block)
+static ram_addr_t ram_block_add(RAMBlock *new_block, Error **errp)
 {
     RAMBlock *block;
     ram_addr_t old_ram_size, new_ram_size;
@@ -1241,9 +1241,11 @@ static ram_addr_t ram_block_add(RAMBlock *new_block)
         } else {
             new_block->host = phys_mem_alloc(new_block->length);
             if (!new_block->host) {
-                fprintf(stderr, "Cannot set up guest memory '%s': %s\n",
-                        new_block->mr->name, strerror(errno));
-                exit(1);
+                error_setg_errno(errp, errno,
+                                 "cannot set up guest memory '%s'",
+                                 new_block->mr->name);
+                qemu_mutex_unlock_ramlist();
+                return -1;
             }
             memory_try_enable_merging(new_block->host, new_block->length);
         }
@@ -1294,6 +1296,7 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, 
MemoryRegion *mr,
                                     Error **errp)
 {
     RAMBlock *new_block;
+    ram_addr_t addr;
 
     if (xen_enabled()) {
         error_setg(errp, "-mem-path not supported with Xen");
@@ -1323,14 +1326,20 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, 
MemoryRegion *mr,
         return -1;
     }
 
-    return ram_block_add(new_block);
+    addr = ram_block_add(new_block, errp);
+    if (errp && *errp) {
+        g_free(new_block);
+        return -1;
+    }
+    return addr;
 }
 #endif
 
 ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
-                                   MemoryRegion *mr)
+                                   MemoryRegion *mr, Error **errp)
 {
     RAMBlock *new_block;
+    ram_addr_t addr;
 
     size = TARGET_PAGE_ALIGN(size);
     new_block = g_malloc0(sizeof(*new_block));
@@ -1341,12 +1350,17 @@ ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, 
void *host,
     if (host) {
         new_block->flags |= RAM_PREALLOC;
     }
-    return ram_block_add(new_block);
+    addr = ram_block_add(new_block, errp);
+    if (errp && *errp) {
+        g_free(new_block);
+        return -1;
+    }
+    return addr;
 }
 
-ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr)
+ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr, Error **errp)
 {
-    return qemu_ram_alloc_from_ptr(size, NULL, mr);
+    return qemu_ram_alloc_from_ptr(size, NULL, mr, errp);
 }
 
 void qemu_ram_free_from_ptr(ram_addr_t addr)
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e2c8e3e..7ebcea6 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -304,6 +304,23 @@ void memory_region_init_io(MemoryRegion *mr,
                            uint64_t size);
 
 /**
+ * memory_region_init_ram_may_fail:  Initialize RAM memory region.  Accesses
+ *                                   into the region will modify memory
+ *                                   directly.
+ *
+ * @mr: the #MemoryRegion to be initialized.
+ * @owner: the object that tracks the region's reference count
+ * @name: the name of the region.
+ * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
+ */
+void memory_region_init_ram_may_fail(MemoryRegion *mr,
+                                     struct Object *owner,
+                                     const char *name,
+                                     uint64_t size,
+                                     Error **errp);
+
+/**
  * memory_region_init_ram:  Initialize RAM memory region.  Accesses into the
  *                          region will modify memory directly.
  *
@@ -340,6 +357,25 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
 #endif
 
 /**
+ * memory_region_init_ram_ptr_may_fail:  Initialize RAM memory region from a
+ *                                       user-provided pointer.  Accesses into
+ *                                       the region will modify memory 
directly.
+ *
+ * @mr: the #MemoryRegion to be initialized.
+ * @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.
+ * @errp: pointer to Error*, to store an error if it happens.
+ */
+void memory_region_init_ram_ptr_may_fail(MemoryRegion *mr,
+                                         struct Object *owner,
+                                         const char *name,
+                                         uint64_t size,
+                                         void *ptr,
+                                         Error **errp);
+
+/**
  * memory_region_init_ram_ptr:  Initialize RAM memory region from a
  *                              user-provided pointer.  Accesses into the
  *                              region will modify memory directly.
@@ -384,6 +420,7 @@ void memory_region_init_alias(MemoryRegion *mr,
  * @ops: callbacks for write access handling.
  * @name: the name of the region.
  * @size: size of the region.
+ * @errp: pointer to Error*, to store an error if it happens.
  */
 void memory_region_init_rom_device(MemoryRegion *mr,
                                    struct Object *owner,
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 6593be1..cf1d4c7 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -26,8 +26,8 @@ ram_addr_t qemu_ram_alloc_from_file(ram_addr_t size, 
MemoryRegion *mr,
                                     bool share, const char *mem_path,
                                     Error **errp);
 ram_addr_t qemu_ram_alloc_from_ptr(ram_addr_t size, void *host,
-                                   MemoryRegion *mr);
-ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr);
+                                   MemoryRegion *mr, Error **errp);
+ram_addr_t qemu_ram_alloc(ram_addr_t size, MemoryRegion *mr, Error **errp);
 int qemu_get_ram_fd(ram_addr_t addr);
 void *qemu_get_ram_block_host_ptr(ram_addr_t addr);
 void *qemu_get_ram_ptr(ram_addr_t addr);
diff --git a/memory.c b/memory.c
index 64d7176..f55f098 100644
--- a/memory.c
+++ b/memory.c
@@ -25,6 +25,7 @@
 #include "exec/memory-internal.h"
 #include "exec/ram_addr.h"
 #include "sysemu/sysemu.h"
+#include "qemu/error-report.h"
 
 //#define DEBUG_UNASSIGNED
 
@@ -1160,16 +1161,33 @@ void memory_region_init_io(MemoryRegion *mr,
     mr->ram_addr = ~(ram_addr_t)0;
 }
 
-void memory_region_init_ram(MemoryRegion *mr,
-                            Object *owner,
-                            const char *name,
-                            uint64_t size)
+void memory_region_init_ram_may_fail(MemoryRegion *mr,
+                                     Object *owner,
+                                     const char *name,
+                                     uint64_t size,
+                                     Error **errp)
 {
     memory_region_init(mr, owner, name, size);
     mr->ram = true;
     mr->terminates = true;
     mr->destructor = memory_region_destructor_ram;
-    mr->ram_addr = qemu_ram_alloc(size, mr);
+    mr->ram_addr = qemu_ram_alloc(size, mr, errp);
+}
+
+void memory_region_init_ram(MemoryRegion *mr,
+                            Object *owner,
+                            const char *name,
+                            uint64_t size)
+{
+    Error *local_err = NULL;
+
+    memory_region_init_ram_may_fail(mr, owner, name, size, &local_err);
+
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
 }
 
 #ifdef __linux__
@@ -1189,17 +1207,35 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
 }
 #endif
 
+void memory_region_init_ram_ptr_may_fail(MemoryRegion *mr,
+                                         Object *owner,
+                                         const char *name,
+                                         uint64_t size,
+                                         void *ptr,
+                                         Error **errp)
+{
+    memory_region_init(mr, owner, name, size);
+    mr->ram = true;
+    mr->terminates = true;
+    mr->destructor = memory_region_destructor_ram_from_ptr;
+    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr, errp);
+}
+
 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_from_ptr;
-    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr);
+    Error *local_err = NULL;
+
+    memory_region_init_ram_ptr_may_fail(mr, owner, name, size, ptr, 
&local_err);
+
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
 }
 
 void memory_region_init_alias(MemoryRegion *mr,
@@ -1223,13 +1259,21 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    const char *name,
                                    uint64_t size)
 {
+    Error *local_err = NULL;
+
     memory_region_init(mr, owner, name, size);
     mr->ops = ops;
     mr->opaque = opaque;
     mr->terminates = true;
     mr->rom_device = true;
     mr->destructor = memory_region_destructor_rom_device;
-    mr->ram_addr = qemu_ram_alloc(size, mr);
+    mr->ram_addr = qemu_ram_alloc(size, mr, &local_err);
+
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
 }
 
 void memory_region_init_iommu(MemoryRegion *mr,
-- 
1.9.3