[Qemu-devel] [PATCH for 2.1 0/2] bug fixs for memory backend

[Qemu-devel] [PATCH for 2.1 0/2] bug fixs for memory backend

[Hu Tao]

This series includes two patches to fix bugs of memory backend. See each
patch for the bugs and how to reproduce them.

Hu Tao (2):
  memory: introduce memory_region_init_ram_nofail() and
    memory_region_init_ram_ptr_nofail()
  memory-backend-file: improve error handling

 backends/hostmem-ram.c   |  2 +-
 exec.c                   | 46 ++++++++++++++++++++++++--------------
 hw/block/pflash_cfi01.c  |  5 ++++-
 hw/block/pflash_cfi02.c  |  5 ++++-
 hw/core/loader.c         |  2 +-
 hw/display/vga.c         |  2 +-
 hw/display/vmware_vga.c  |  3 ++-
 hw/i386/kvm/pci-assign.c |  9 ++++----
 hw/i386/pc.c             |  2 +-
 hw/i386/pc_sysfw.c       |  4 ++--
 hw/misc/ivshmem.c        |  9 ++++----
 hw/misc/vfio.c           |  3 ++-
 hw/pci/pci.c             |  2 +-
 include/exec/memory.h    | 32 ++++++++++++++++++++++++---
 include/exec/ram_addr.h  |  4 ++--
 memory.c                 | 57 +++++++++++++++++++++++++++++++++++++++++++-----
 numa.c                   |  4 ++--
 17 files changed, 143 insertions(+), 48 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH for 2.1 1/2] memory: introduce memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail()

[Hu Tao]

Introduce memory_region_init_ram_nofail() and
memory_region_init_ram_ptr_nofail(), which are the same as
memory_region_init_ram() and memory_region_init_ram_ptr()
respectively. They will exit qemu if there is an error, this is the
behaviour of old memory_region_init_ram() and
memory_region_init_ram_ptr().

All existing calls to memory_region_init_ram() and
memory_region_init_ram_ptr() are replaced with
memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail().

memory_region_init_ram() and memory_region_init_ram_ptr() are added an
extra parameter errp to let callers handle the error.

This patch 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.

How to reproduce:

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


Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 backends/hostmem-ram.c   |  2 +-
 exec.c                   | 30 +++++++++++++++++--------
 hw/block/pflash_cfi01.c  |  5 ++++-
 hw/block/pflash_cfi02.c  |  5 ++++-
 hw/core/loader.c         |  2 +-
 hw/display/vga.c         |  2 +-
 hw/display/vmware_vga.c  |  3 ++-
 hw/i386/kvm/pci-assign.c |  9 ++++----
 hw/i386/pc.c             |  2 +-
 hw/i386/pc_sysfw.c       |  4 ++--
 hw/misc/ivshmem.c        |  9 ++++----
 hw/misc/vfio.c           |  3 ++-
 hw/pci/pci.c             |  2 +-
 include/exec/memory.h    | 32 ++++++++++++++++++++++++---
 include/exec/ram_addr.h  |  4 ++--
 memory.c                 | 57 +++++++++++++++++++++++++++++++++++++++++++-----
 numa.c                   |  4 ++--
 17 files changed, 134 insertions(+), 41 deletions(-)

diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
index d9a8290..a67a134 100644
--- a/backends/hostmem-ram.c
+++ b/backends/hostmem-ram.c
@@ -27,7 +27,7 @@ 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);
+                           backend->size, errp);
     g_free(path);
 }
 
diff --git a/exec.c b/exec.c
index 5a2a25e..8c2a91d 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,19 @@ 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 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 +1349,16 @@ 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 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/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index f9507b4..92b8b87 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -770,7 +770,10 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
     memory_region_init_rom_device(
         &pfl->mem, OBJECT(dev),
         pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
-        pfl->name, total_len);
+        pfl->name, total_len, errp);
+    if (errp && *errp) {
+        return;
+    }
     vmstate_register_ram(&pfl->mem, DEVICE(pfl));
     pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
     sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 8d4b828..b773f19 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -608,7 +608,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
 
     memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
                                   &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
-                                  pfl, pfl->name, chip_len);
+                                  pfl, pfl->name, chip_len, errp);
+    if (errp && *errp) {
+        return;
+    }
     vmstate_register_ram(&pfl->orig_mem, DEVICE(pfl));
     pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
     pfl->chip_len = chip_len;
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2bf6b8f..fdebf86 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -632,7 +632,7 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
     void *data;
 
     rom->mr = g_malloc(sizeof(*rom->mr));
-    memory_region_init_ram(rom->mr, owner, name, rom->datasize);
+    memory_region_init_ram_nofail(rom->mr, owner, name, rom->datasize);
     memory_region_set_readonly(rom->mr, true);
     vmstate_register_ram_global(rom->mr);
 
diff --git a/hw/display/vga.c b/hw/display/vga.c
index 4b089a3..f17a4b4 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -2291,7 +2291,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
     s->vram_size_mb = s->vram_size >> 20;
 
     s->is_vbe_vmstate = 1;
-    memory_region_init_ram(&s->vram, obj, "vga.vram", s->vram_size);
+    memory_region_init_ram_nofail(&s->vram, obj, "vga.vram", s->vram_size);
     vmstate_register_ram(&s->vram, global_vmstate ? NULL : DEVICE(obj));
     xen_register_framebuffer(&s->vram);
     s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
index 591b645..a3738be 100644
--- a/hw/display/vmware_vga.c
+++ b/hw/display/vmware_vga.c
@@ -1201,7 +1201,8 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
     s->vga.con = graphic_console_init(dev, 0, &vmsvga_ops, s);
 
     s->fifo_size = SVGA_FIFO_SIZE;
-    memory_region_init_ram(&s->fifo_ram, NULL, "vmsvga.fifo", s->fifo_size);
+    memory_region_init_ram_nofail(&s->fifo_ram, NULL, "vmsvga.fifo",
+                                  s->fifo_size);
     vmstate_register_ram_global(&s->fifo_ram);
     s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
 
diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
index de33657..52d0db0 100644
--- a/hw/i386/kvm/pci-assign.c
+++ b/hw/i386/kvm/pci-assign.c
@@ -454,9 +454,10 @@ static void assigned_dev_register_regions(PCIRegion *io_regions,
                 char name[32];
                 snprintf(name, sizeof(name), "%s.bar%d",
                          object_get_typename(OBJECT(pci_dev)), i);
-                memory_region_init_ram_ptr(&pci_dev->v_addrs[i].real_iomem,
-                                           OBJECT(pci_dev), name,
-                                           cur_region->size, virtbase);
+                memory_region_init_ram_ptr_nofail(
+                                              &pci_dev->v_addrs[i].real_iomem,
+                                              OBJECT(pci_dev), name,
+                                              cur_region->size, virtbase);
                 vmstate_register_ram(&pci_dev->v_addrs[i].real_iomem,
                                      &pci_dev->dev.qdev);
             }
@@ -1943,7 +1944,7 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
 
     snprintf(name, sizeof(name), "%s.rom",
             object_get_typename(OBJECT(dev)));
-    memory_region_init_ram(&dev->dev.rom, OBJECT(dev), name, st.st_size);
+    memory_region_init_ram_nofail(&dev->dev.rom, OBJECT(dev), name, st.st_size);
     vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
     ptr = memory_region_get_ram_ptr(&dev->dev.rom);
     memset(ptr, 0xff, st.st_size);
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 2cf22b1..f503b0e 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1272,7 +1272,7 @@ FWCfgState *pc_memory_init(MachineState *machine,
     pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
 
     option_rom_mr = g_malloc(sizeof(*option_rom_mr));
-    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
+    memory_region_init_ram_nofail(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
     vmstate_register_ram_global(option_rom_mr);
     memory_region_add_subregion_overlap(rom_memory,
                                         PC_ROM_MIN_VGA,
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 75a7ebb..a15963b 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -55,7 +55,7 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = MIN(flash_size, 128 * 1024);
     isa_bios = g_malloc(sizeof(*isa_bios));
-    memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size);
+    memory_region_init_ram_nofail(isa_bios, NULL, "isa-bios", isa_bios_size);
     vmstate_register_ram_global(isa_bios);
     memory_region_add_subregion_overlap(rom_memory,
                                         0x100000 - isa_bios_size,
@@ -192,7 +192,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
         goto bios_error;
     }
     bios = g_malloc(sizeof(*bios));
-    memory_region_init_ram(bios, NULL, "pc.bios", bios_size);
+    memory_region_init_ram_nofail(bios, NULL, "pc.bios", bios_size);
     vmstate_register_ram_global(bios);
     if (!isapc_ram_fw) {
         memory_region_set_readonly(bios, true);
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 768e528..e3b6e06 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -347,8 +347,8 @@ static void create_shared_memory_BAR(IVShmemState *s, int fd) {
 
     ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
 
-    memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
-                               s->ivshmem_size, ptr);
+    memory_region_init_ram_ptr_nofail(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
+                                      s->ivshmem_size, ptr);
     vmstate_register_ram(&s->ivshmem, DEVICE(s));
     memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
 
@@ -475,8 +475,9 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
         /* mmap the region and map into the BAR2 */
         map_ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED,
                                                             incoming_fd, 0);
-        memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s),
-                                   "ivshmem.bar2", s->ivshmem_size, map_ptr);
+        memory_region_init_ram_ptr_nofail(&s->ivshmem, OBJECT(s),
+                                          "ivshmem.bar2", s->ivshmem_size,
+                                          map_ptr);
         vmstate_register_ram(&s->ivshmem, DEVICE(s));
 
         IVSHMEM_DPRINTF("guest h/w addr = %" PRIu64 ", size = %" PRIu64 "\n",
diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
index aef4c9c..5bdee3e 100644
--- a/hw/misc/vfio.c
+++ b/hw/misc/vfio.c
@@ -2894,7 +2894,8 @@ static int vfio_mmap_bar(VFIODevice *vdev, VFIOBAR *bar,
             goto empty_region;
         }
 
-        memory_region_init_ram_ptr(submem, OBJECT(vdev), name, size, *map);
+        memory_region_init_ram_ptr_nofail(submem, OBJECT(vdev), name, size,
+                                          *map);
     } else {
 empty_region:
         /* Create a zero sized sub-region to make cleanup easy. */
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 17ed510..ba39f08 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1974,7 +1974,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
         snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev)));
     }
     pdev->has_rom = true;
-    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size);
+    memory_region_init_ram_nofail(&pdev->rom, OBJECT(pdev), name, size);
     vmstate_register_ram(&pdev->rom, &pdev->qdev);
     ptr = memory_region_get_ram_ptr(&pdev->rom);
     load_image(path, ptr);
diff --git a/include/exec/memory.h b/include/exec/memory.h
index e2c8e3e..2e74518 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -311,11 +311,27 @@ void memory_region_init_io(MemoryRegion *mr,
  * @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(MemoryRegion *mr,
                             struct Object *owner,
                             const char *name,
-                            uint64_t size);
+                            uint64_t size,
+                            Error **errp);
+
+/**
+ * memory_region_init_ram_nofail: like memory_region_init_ram but won't
+ * fail
+ *
+ * @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.
+ */
+void memory_region_init_ram_nofail(MemoryRegion *mr,
+                                   struct Object *owner,
+                                   const char *name,
+                                   uint64_t size);
 
 #ifdef __linux__
 /**
@@ -349,12 +365,20 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
  * @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(MemoryRegion *mr,
                                 struct Object *owner,
                                 const char *name,
                                 uint64_t size,
-                                void *ptr);
+                                void *ptr,
+                                Error **errp);
+
+void memory_region_init_ram_ptr_nofail(MemoryRegion *mr,
+                                       struct Object *owner,
+                                       const char *name,
+                                       uint64_t size,
+                                       void *ptr);
 
 /**
  * memory_region_init_alias: Initialize a memory region that aliases all or a
@@ -384,13 +408,15 @@ 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,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
                                    const char *name,
-                                   uint64_t size);
+                                   uint64_t size,
+                                   Error **errp);
 
 /**
  * memory_region_init_reservation: Initialize a memory region that reserves
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index e9eb831..998ac4f 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..dc24c53 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
 
@@ -1163,13 +1164,34 @@ void memory_region_init_io(MemoryRegion *mr,
 void memory_region_init_ram(MemoryRegion *mr,
                             Object *owner,
                             const char *name,
-                            uint64_t size)
+                            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_nofail(MemoryRegion *mr,
+                                   Object *owner,
+                                   const char *name,
+                                   uint64_t size)
+{
+    Error *local_err = NULL;
+
+    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, &local_err);
+
+    if (local_err) {
+        error_report("%s", error_get_pretty(local_err));
+        error_free(local_err);
+        exit(EXIT_FAILURE);
+    }
 }
 
 #ifdef __linux__
@@ -1193,13 +1215,35 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
                                 Object *owner,
                                 const char *name,
                                 uint64_t size,
-                                void *ptr)
+                                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);
+    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr, errp);
+}
+
+void memory_region_init_ram_ptr_nofail(MemoryRegion *mr,
+                                       Object *owner,
+                                       const char *name,
+                                       uint64_t size,
+                                       void *ptr)
+{
+    Error *local_err = NULL;
+
+    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, &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,
@@ -1221,7 +1265,8 @@ void memory_region_init_rom_device(MemoryRegion *mr,
                                    const MemoryRegionOps *ops,
                                    void *opaque,
                                    const char *name,
-                                   uint64_t size)
+                                   uint64_t size,
+                                   Error **errp)
 {
     memory_region_init(mr, owner, name, size);
     mr->ops = ops;
@@ -1229,7 +1274,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
     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, errp);
 }
 
 void memory_region_init_iommu(MemoryRegion *mr,
diff --git a/numa.c b/numa.c
index 2fde740..dabba4f 100644
--- a/numa.c
+++ b/numa.c
@@ -263,14 +263,14 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
         if (err) {
             qerror_report_err(err);
             error_free(err);
-            memory_region_init_ram(mr, owner, name, ram_size);
+            memory_region_init_ram_nofail(mr, owner, name, ram_size);
         }
 #else
         fprintf(stderr, "-mem-path not supported on this host\n");
         exit(1);
 #endif
     } else {
-        memory_region_init_ram(mr, owner, name, ram_size);
+        memory_region_init_ram_nofail(mr, owner, name, ram_size);
     }
     vmstate_register_ram_global(mr);
 }
-- 
1.9.3

[Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Hu Tao]

This patch fixes two problems of memory-backend-file:

1. If user adds a memory-backend-file object using object_add command,
   specifying a non-existing directory for property mem-path, qemu
   will core dump with message:

     /nonexistingdir: No such file or directory
     Bad ram offset fffffffffffff000
     Aborted (core dumped)

2. If user adds a memory-backend-file object using object_add command,
   specifying a size that is less than huge page size, qemu
   will core dump with message:

     Bad ram offset fffffffffffff000
     Aborted (core dumped)

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 exec.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/exec.c b/exec.c
index 8c2a91d..35c2dcb 100644
--- a/exec.c
+++ b/exec.c
@@ -996,7 +996,7 @@ void qemu_mutex_unlock_ramlist(void)
 
 #define HUGETLBFS_MAGIC       0x958458f6
 
-static long gethugepagesize(const char *path)
+static long gethugepagesize(const char *path, Error **errp)
 {
     struct statfs fs;
     int ret;
@@ -1006,7 +1006,7 @@ static long gethugepagesize(const char *path)
     } while (ret != 0 && errno == EINTR);
 
     if (ret != 0) {
-        perror(path);
+        error_setg_errno(errp, errno, "failed to stat file %s", path);
         return 0;
     }
 
@@ -1024,17 +1024,19 @@ static void *file_ram_alloc(RAMBlock *block,
     char *filename;
     char *sanitized_name;
     char *c;
-    void *area;
+    void *area = NULL;
     int fd;
     unsigned long hpagesize;
 
-    hpagesize = gethugepagesize(path);
+    hpagesize = gethugepagesize(path, errp);
     if (!hpagesize) {
         goto error;
     }
 
     if (memory < hpagesize) {
-        return NULL;
+        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger "
+                   "than huge page size 0x%" PRIx64, memory, hpagesize);
+        goto error;
     }
 
     if (kvm_enabled() && !kvm_has_sync_mmu()) {
@@ -1094,8 +1096,8 @@ static void *file_ram_alloc(RAMBlock *block,
     return area;
 
 error:
-    if (mem_prealloc) {
-        exit(1);
+    if (area && area != MAP_FAILED) {
+        munmap(area, memory);
     }
     return NULL;
 }
-- 
1.9.3

Re: [Qemu-devel] [PATCH for 2.1 1/2] memory: introduce memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail()

[Michael S. Tsirkin]

On Thu, Jul 03, 2014 at 02:10:55PM +0800, Hu Tao wrote:
> Introduce memory_region_init_ram_nofail() and
> memory_region_init_ram_ptr_nofail(), which are the same as
> memory_region_init_ram() and memory_region_init_ram_ptr()
> respectively. They will exit qemu if there is an error, this is the
> behaviour of old memory_region_init_ram() and
> memory_region_init_ram_ptr().
> 
> All existing calls to memory_region_init_ram() and
> memory_region_init_ram_ptr() are replaced with
> memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail().
> 
> memory_region_init_ram() and memory_region_init_ram_ptr() are added an
> extra parameter errp to let callers handle the error.
> 
> This patch 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.
> 
> How to reproduce:
> 
> 1. run qemu
> 2. (monitor)object_add memory-backend-ram,size=100000G,id=ram0
> 
> 

Don't put two empty lines in a row please.

> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  backends/hostmem-ram.c   |  2 +-
>  exec.c                   | 30 +++++++++++++++++--------
>  hw/block/pflash_cfi01.c  |  5 ++++-
>  hw/block/pflash_cfi02.c  |  5 ++++-
>  hw/core/loader.c         |  2 +-
>  hw/display/vga.c         |  2 +-
>  hw/display/vmware_vga.c  |  3 ++-
>  hw/i386/kvm/pci-assign.c |  9 ++++----
>  hw/i386/pc.c             |  2 +-
>  hw/i386/pc_sysfw.c       |  4 ++--
>  hw/misc/ivshmem.c        |  9 ++++----
>  hw/misc/vfio.c           |  3 ++-
>  hw/pci/pci.c             |  2 +-
>  include/exec/memory.h    | 32 ++++++++++++++++++++++++---
>  include/exec/ram_addr.h  |  4 ++--
>  memory.c                 | 57 +++++++++++++++++++++++++++++++++++++++++++-----
>  numa.c                   |  4 ++--
>  17 files changed, 134 insertions(+), 41 deletions(-)
> 
> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> index d9a8290..a67a134 100644
> --- a/backends/hostmem-ram.c
> +++ b/backends/hostmem-ram.c
> @@ -27,7 +27,7 @@ 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);
> +                           backend->size, errp);
>      g_free(path);
>  }
>  

Sigh.  So you are still mixing a huge mechanical rename with
a bugfix.  I'm not merging this, please split up the patch:
1. rename existing functions and convert all users to _nofail
2. add parameter to qemu_ram_alloc variants,
   add new function and use in hostmem-ram


> diff --git a/exec.c b/exec.c
> index 5a2a25e..8c2a91d 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,19 @@ 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);

You want return -1 here. Don't rely on ram_block_add to 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 +1349,16 @@ 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);

same

> +    }
> +    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/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index f9507b4..92b8b87 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -770,7 +770,10 @@ static void pflash_cfi01_realize(DeviceState *dev, Error **errp)
>      memory_region_init_rom_device(
>          &pfl->mem, OBJECT(dev),
>          pfl->be ? &pflash_cfi01_ops_be : &pflash_cfi01_ops_le, pfl,
> -        pfl->name, total_len);
> +        pfl->name, total_len, errp);
> +    if (errp && *errp) {
> +        return;
> +    }
>      vmstate_register_ram(&pfl->mem, DEVICE(pfl));
>      pfl->storage = memory_region_get_ram_ptr(&pfl->mem);
>      sysbus_init_mmio(SYS_BUS_DEVICE(dev), &pfl->mem);
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 8d4b828..b773f19 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -608,7 +608,10 @@ static void pflash_cfi02_realize(DeviceState *dev, Error **errp)
>  
>      memory_region_init_rom_device(&pfl->orig_mem, OBJECT(pfl), pfl->be ?
>                                    &pflash_cfi02_ops_be : &pflash_cfi02_ops_le,
> -                                  pfl, pfl->name, chip_len);
> +                                  pfl, pfl->name, chip_len, errp);
> +    if (errp && *errp) {
> +        return;
> +    }
>      vmstate_register_ram(&pfl->orig_mem, DEVICE(pfl));
>      pfl->storage = memory_region_get_ram_ptr(&pfl->orig_mem);
>      pfl->chip_len = chip_len;
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2bf6b8f..fdebf86 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -632,7 +632,7 @@ static void *rom_set_mr(Rom *rom, Object *owner, const char *name)
>      void *data;
>  
>      rom->mr = g_malloc(sizeof(*rom->mr));
> -    memory_region_init_ram(rom->mr, owner, name, rom->datasize);
> +    memory_region_init_ram_nofail(rom->mr, owner, name, rom->datasize);
>      memory_region_set_readonly(rom->mr, true);
>      vmstate_register_ram_global(rom->mr);
>  
> diff --git a/hw/display/vga.c b/hw/display/vga.c
> index 4b089a3..f17a4b4 100644
> --- a/hw/display/vga.c
> +++ b/hw/display/vga.c
> @@ -2291,7 +2291,7 @@ void vga_common_init(VGACommonState *s, Object *obj, bool global_vmstate)
>      s->vram_size_mb = s->vram_size >> 20;
>  
>      s->is_vbe_vmstate = 1;
> -    memory_region_init_ram(&s->vram, obj, "vga.vram", s->vram_size);
> +    memory_region_init_ram_nofail(&s->vram, obj, "vga.vram", s->vram_size);
>      vmstate_register_ram(&s->vram, global_vmstate ? NULL : DEVICE(obj));
>      xen_register_framebuffer(&s->vram);
>      s->vram_ptr = memory_region_get_ram_ptr(&s->vram);
> diff --git a/hw/display/vmware_vga.c b/hw/display/vmware_vga.c
> index 591b645..a3738be 100644
> --- a/hw/display/vmware_vga.c
> +++ b/hw/display/vmware_vga.c
> @@ -1201,7 +1201,8 @@ static void vmsvga_init(DeviceState *dev, struct vmsvga_state_s *s,
>      s->vga.con = graphic_console_init(dev, 0, &vmsvga_ops, s);
>  
>      s->fifo_size = SVGA_FIFO_SIZE;
> -    memory_region_init_ram(&s->fifo_ram, NULL, "vmsvga.fifo", s->fifo_size);
> +    memory_region_init_ram_nofail(&s->fifo_ram, NULL, "vmsvga.fifo",
> +                                  s->fifo_size);
>      vmstate_register_ram_global(&s->fifo_ram);
>      s->fifo_ptr = memory_region_get_ram_ptr(&s->fifo_ram);
>  
> diff --git a/hw/i386/kvm/pci-assign.c b/hw/i386/kvm/pci-assign.c
> index de33657..52d0db0 100644
> --- a/hw/i386/kvm/pci-assign.c
> +++ b/hw/i386/kvm/pci-assign.c
> @@ -454,9 +454,10 @@ static void assigned_dev_register_regions(PCIRegion *io_regions,
>                  char name[32];
>                  snprintf(name, sizeof(name), "%s.bar%d",
>                           object_get_typename(OBJECT(pci_dev)), i);
> -                memory_region_init_ram_ptr(&pci_dev->v_addrs[i].real_iomem,
> -                                           OBJECT(pci_dev), name,
> -                                           cur_region->size, virtbase);
> +                memory_region_init_ram_ptr_nofail(
> +                                              &pci_dev->v_addrs[i].real_iomem,
> +                                              OBJECT(pci_dev), name,
> +                                              cur_region->size, virtbase);
>                  vmstate_register_ram(&pci_dev->v_addrs[i].real_iomem,
>                                       &pci_dev->dev.qdev);
>              }
> @@ -1943,7 +1944,7 @@ static void assigned_dev_load_option_rom(AssignedDevice *dev)
>  
>      snprintf(name, sizeof(name), "%s.rom",
>              object_get_typename(OBJECT(dev)));
> -    memory_region_init_ram(&dev->dev.rom, OBJECT(dev), name, st.st_size);
> +    memory_region_init_ram_nofail(&dev->dev.rom, OBJECT(dev), name, st.st_size);
>      vmstate_register_ram(&dev->dev.rom, &dev->dev.qdev);
>      ptr = memory_region_get_ram_ptr(&dev->dev.rom);
>      memset(ptr, 0xff, st.st_size);
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index 2cf22b1..f503b0e 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1272,7 +1272,7 @@ FWCfgState *pc_memory_init(MachineState *machine,
>      pc_system_firmware_init(rom_memory, guest_info->isapc_ram_fw);
>  
>      option_rom_mr = g_malloc(sizeof(*option_rom_mr));
> -    memory_region_init_ram(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
> +    memory_region_init_ram_nofail(option_rom_mr, NULL, "pc.rom", PC_ROM_SIZE);
>      vmstate_register_ram_global(option_rom_mr);
>      memory_region_add_subregion_overlap(rom_memory,
>                                          PC_ROM_MIN_VGA,
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index 75a7ebb..a15963b 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -55,7 +55,7 @@ static void pc_isa_bios_init(MemoryRegion *rom_memory,
>      /* map the last 128KB of the BIOS in ISA space */
>      isa_bios_size = MIN(flash_size, 128 * 1024);
>      isa_bios = g_malloc(sizeof(*isa_bios));
> -    memory_region_init_ram(isa_bios, NULL, "isa-bios", isa_bios_size);
> +    memory_region_init_ram_nofail(isa_bios, NULL, "isa-bios", isa_bios_size);
>      vmstate_register_ram_global(isa_bios);
>      memory_region_add_subregion_overlap(rom_memory,
>                                          0x100000 - isa_bios_size,
> @@ -192,7 +192,7 @@ static void old_pc_system_rom_init(MemoryRegion *rom_memory, bool isapc_ram_fw)
>          goto bios_error;
>      }
>      bios = g_malloc(sizeof(*bios));
> -    memory_region_init_ram(bios, NULL, "pc.bios", bios_size);
> +    memory_region_init_ram_nofail(bios, NULL, "pc.bios", bios_size);
>      vmstate_register_ram_global(bios);
>      if (!isapc_ram_fw) {
>          memory_region_set_readonly(bios, true);
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 768e528..e3b6e06 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -347,8 +347,8 @@ static void create_shared_memory_BAR(IVShmemState *s, int fd) {
>  
>      ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
>  
> -    memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
> -                               s->ivshmem_size, ptr);
> +    memory_region_init_ram_ptr_nofail(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
> +                                      s->ivshmem_size, ptr);
>      vmstate_register_ram(&s->ivshmem, DEVICE(s));
>      memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
>  
> @@ -475,8 +475,9 @@ static void ivshmem_read(void *opaque, const uint8_t * buf, int flags)
>          /* mmap the region and map into the BAR2 */
>          map_ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED,
>                                                              incoming_fd, 0);
> -        memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s),
> -                                   "ivshmem.bar2", s->ivshmem_size, map_ptr);
> +        memory_region_init_ram_ptr_nofail(&s->ivshmem, OBJECT(s),
> +                                          "ivshmem.bar2", s->ivshmem_size,
> +                                          map_ptr);
>          vmstate_register_ram(&s->ivshmem, DEVICE(s));
>  
>          IVSHMEM_DPRINTF("guest h/w addr = %" PRIu64 ", size = %" PRIu64 "\n",
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index aef4c9c..5bdee3e 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -2894,7 +2894,8 @@ static int vfio_mmap_bar(VFIODevice *vdev, VFIOBAR *bar,
>              goto empty_region;
>          }
>  
> -        memory_region_init_ram_ptr(submem, OBJECT(vdev), name, size, *map);
> +        memory_region_init_ram_ptr_nofail(submem, OBJECT(vdev), name, size,
> +                                          *map);
>      } else {
>  empty_region:
>          /* Create a zero sized sub-region to make cleanup easy. */
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index 17ed510..ba39f08 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -1974,7 +1974,7 @@ static int pci_add_option_rom(PCIDevice *pdev, bool is_default_rom)
>          snprintf(name, sizeof(name), "%s.rom", object_get_typename(OBJECT(pdev)));
>      }
>      pdev->has_rom = true;
> -    memory_region_init_ram(&pdev->rom, OBJECT(pdev), name, size);
> +    memory_region_init_ram_nofail(&pdev->rom, OBJECT(pdev), name, size);
>      vmstate_register_ram(&pdev->rom, &pdev->qdev);
>      ptr = memory_region_get_ram_ptr(&pdev->rom);
>      load_image(path, ptr);
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index e2c8e3e..2e74518 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -311,11 +311,27 @@ void memory_region_init_io(MemoryRegion *mr,
>   * @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(MemoryRegion *mr,
>                              struct Object *owner,
>                              const char *name,
> -                            uint64_t size);
> +                            uint64_t size,
> +                            Error **errp);
> +
> +/**
> + * memory_region_init_ram_nofail: like memory_region_init_ram but won't
> + * fail
> + *
> + * @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.
> + */
> +void memory_region_init_ram_nofail(MemoryRegion *mr,
> +                                   struct Object *owner,
> +                                   const char *name,
> +                                   uint64_t size);
>  
>  #ifdef __linux__
>  /**
> @@ -349,12 +365,20 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
>   * @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(MemoryRegion *mr,
>                                  struct Object *owner,
>                                  const char *name,
>                                  uint64_t size,
> -                                void *ptr);
> +                                void *ptr,
> +                                Error **errp);
> +
> +void memory_region_init_ram_ptr_nofail(MemoryRegion *mr,
> +                                       struct Object *owner,
> +                                       const char *name,
> +                                       uint64_t size,
> +                                       void *ptr);
>  
>  /**
>   * memory_region_init_alias: Initialize a memory region that aliases all or a
> @@ -384,13 +408,15 @@ 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,
>                                     const MemoryRegionOps *ops,
>                                     void *opaque,
>                                     const char *name,
> -                                   uint64_t size);
> +                                   uint64_t size,
> +                                   Error **errp);
>  
>  /**
>   * memory_region_init_reservation: Initialize a memory region that reserves
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index e9eb831..998ac4f 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..dc24c53 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
>  
> @@ -1163,13 +1164,34 @@ void memory_region_init_io(MemoryRegion *mr,
>  void memory_region_init_ram(MemoryRegion *mr,
>                              Object *owner,
>                              const char *name,
> -                            uint64_t size)
> +                            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_nofail(MemoryRegion *mr,
> +                                   Object *owner,
> +                                   const char *name,
> +                                   uint64_t size)
> +{
> +    Error *local_err = NULL;
> +
> +    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, &local_err);
> +
> +    if (local_err) {
> +        error_report("%s", error_get_pretty(local_err));
> +        error_free(local_err);
> +        exit(EXIT_FAILURE);
> +    }
>  }
>  
>  #ifdef __linux__
> @@ -1193,13 +1215,35 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
>                                  Object *owner,
>                                  const char *name,
>                                  uint64_t size,
> -                                void *ptr)
> +                                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);
> +    mr->ram_addr = qemu_ram_alloc_from_ptr(size, ptr, mr, errp);
> +}
> +
> +void memory_region_init_ram_ptr_nofail(MemoryRegion *mr,
> +                                       Object *owner,
> +                                       const char *name,
> +                                       uint64_t size,
> +                                       void *ptr)
> +{
> +    Error *local_err = NULL;
> +
> +    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, &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,
> @@ -1221,7 +1265,8 @@ void memory_region_init_rom_device(MemoryRegion *mr,
>                                     const MemoryRegionOps *ops,
>                                     void *opaque,
>                                     const char *name,
> -                                   uint64_t size)
> +                                   uint64_t size,
> +                                   Error **errp)
>  {
>      memory_region_init(mr, owner, name, size);
>      mr->ops = ops;
> @@ -1229,7 +1274,7 @@ void memory_region_init_rom_device(MemoryRegion *mr,
>      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, errp);
>  }
>  
>  void memory_region_init_iommu(MemoryRegion *mr,
> diff --git a/numa.c b/numa.c
> index 2fde740..dabba4f 100644
> --- a/numa.c
> +++ b/numa.c
> @@ -263,14 +263,14 @@ static void allocate_system_memory_nonnuma(MemoryRegion *mr, Object *owner,
>          if (err) {
>              qerror_report_err(err);
>              error_free(err);
> -            memory_region_init_ram(mr, owner, name, ram_size);
> +            memory_region_init_ram_nofail(mr, owner, name, ram_size);
>          }
>  #else
>          fprintf(stderr, "-mem-path not supported on this host\n");
>          exit(1);
>  #endif
>      } else {
> -        memory_region_init_ram(mr, owner, name, ram_size);
> +        memory_region_init_ram_nofail(mr, owner, name, ram_size);
>      }
>      vmstate_register_ram_global(mr);
>  }
> -- 
> 1.9.3

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Eric Blake]

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

On 07/03/2014 12:10 AM, Hu Tao wrote:
> This patch fixes two problems of memory-backend-file:
> 
> 1. If user adds a memory-backend-file object using object_add command,
>    specifying a non-existing directory for property mem-path, qemu
>    will core dump with message:
> 
>      /nonexistingdir: No such file or directory
>      Bad ram offset fffffffffffff000
>      Aborted (core dumped)
> 
> 2. If user adds a memory-backend-file object using object_add command,
>    specifying a size that is less than huge page size, qemu
>    will core dump with message:
> 
>      Bad ram offset fffffffffffff000
>      Aborted (core dumped)
> 

Might be nice if the commit message also shows the new message issued
for the same cases after the patch is applied.

> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  exec.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 

>  
>      if (memory < hpagesize) {
> -        return NULL;
> +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger "
> +                   "than huge page size 0x%" PRIx64, memory, hpagesize);
> +        goto error;

Isn't exactly equal also allowed?  Maybe a better wording is "should be
a multiple of the huge page size"

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for 2.1 1/2] memory: introduce memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail()

[Hu Tao]

On Thu, Jul 03, 2014 at 09:51:13AM +0300, Michael S. Tsirkin wrote:
> On Thu, Jul 03, 2014 at 02:10:55PM +0800, Hu Tao wrote:
> > Introduce memory_region_init_ram_nofail() and
> > memory_region_init_ram_ptr_nofail(), which are the same as
> > memory_region_init_ram() and memory_region_init_ram_ptr()
> > respectively. They will exit qemu if there is an error, this is the
> > behaviour of old memory_region_init_ram() and
> > memory_region_init_ram_ptr().
> > 
> > All existing calls to memory_region_init_ram() and
> > memory_region_init_ram_ptr() are replaced with
> > memory_region_init_ram_nofail() and memory_region_init_ram_ptr_nofail().
> > 
> > memory_region_init_ram() and memory_region_init_ram_ptr() are added an
> > extra parameter errp to let callers handle the error.
> > 
> > This patch 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.
> > 
> > How to reproduce:
> > 
> > 1. run qemu
> > 2. (monitor)object_add memory-backend-ram,size=100000G,id=ram0
> > 
> > 
> 
> Don't put two empty lines in a row please.

I'll fix it.

> 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  backends/hostmem-ram.c   |  2 +-
> >  exec.c                   | 30 +++++++++++++++++--------
> >  hw/block/pflash_cfi01.c  |  5 ++++-
> >  hw/block/pflash_cfi02.c  |  5 ++++-
> >  hw/core/loader.c         |  2 +-
> >  hw/display/vga.c         |  2 +-
> >  hw/display/vmware_vga.c  |  3 ++-
> >  hw/i386/kvm/pci-assign.c |  9 ++++----
> >  hw/i386/pc.c             |  2 +-
> >  hw/i386/pc_sysfw.c       |  4 ++--
> >  hw/misc/ivshmem.c        |  9 ++++----
> >  hw/misc/vfio.c           |  3 ++-
> >  hw/pci/pci.c             |  2 +-
> >  include/exec/memory.h    | 32 ++++++++++++++++++++++++---
> >  include/exec/ram_addr.h  |  4 ++--
> >  memory.c                 | 57 +++++++++++++++++++++++++++++++++++++++++++-----
> >  numa.c                   |  4 ++--
> >  17 files changed, 134 insertions(+), 41 deletions(-)
> > 
> > diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> > index d9a8290..a67a134 100644
> > --- a/backends/hostmem-ram.c
> > +++ b/backends/hostmem-ram.c
> > @@ -27,7 +27,7 @@ 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);
> > +                           backend->size, errp);
> >      g_free(path);
> >  }
> >  
> 
> Sigh.  So you are still mixing a huge mechanical rename with
> a bugfix.  I'm not merging this, please split up the patch:
> 1. rename existing functions and convert all users to _nofail
> 2. add parameter to qemu_ram_alloc variants,
>    add new function and use in hostmem-ram

Sure. Thanks for review!


Regards,
Hu

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Hu Tao]

On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote:
> On 07/03/2014 12:10 AM, Hu Tao wrote:
> > This patch fixes two problems of memory-backend-file:
> > 
> > 1. If user adds a memory-backend-file object using object_add command,
> >    specifying a non-existing directory for property mem-path, qemu
> >    will core dump with message:
> > 
> >      /nonexistingdir: No such file or directory
> >      Bad ram offset fffffffffffff000
> >      Aborted (core dumped)
> > 
> > 2. If user adds a memory-backend-file object using object_add command,
> >    specifying a size that is less than huge page size, qemu
> >    will core dump with message:
> > 
> >      Bad ram offset fffffffffffff000
> >      Aborted (core dumped)
> > 
> 
> Might be nice if the commit message also shows the new message issued
> for the same cases after the patch is applied.

OK.

> 
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  exec.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> 
> >  
> >      if (memory < hpagesize) {
> > -        return NULL;
> > +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger "
> > +                   "than huge page size 0x%" PRIx64, memory, hpagesize);
> > +        goto error;
> 
> Isn't exactly equal also allowed?  Maybe a better wording is "should be
> a multiple of the huge page size"

Yes. I'll change the error message.

Thanks,
Hu

> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Eric Blake]

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

On 07/03/2014 08:56 PM, Hu Tao wrote:
> On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote:
>> On 07/03/2014 12:10 AM, Hu Tao wrote:
>>> This patch fixes two problems of memory-backend-file:
>>>
>>> 1. If user adds a memory-backend-file object using object_add command,
>>>    specifying a non-existing directory for property mem-path, qemu
>>>    will core dump with message:
>>>
>>>      /nonexistingdir: No such file or directory
>>>      Bad ram offset fffffffffffff000
>>>      Aborted (core dumped)
>>>
>>> 2. If user adds a memory-backend-file object using object_add command,
>>>    specifying a size that is less than huge page size, qemu
>>>    will core dump with message:
>>>
>>>      Bad ram offset fffffffffffff000
>>>      Aborted (core dumped)
>>>
>>
>> Might be nice if the commit message also shows the new message issued
>> for the same cases after the patch is applied.
> 
> OK.
> 

>>>  
>>>      if (memory < hpagesize) {
>>> -        return NULL;
>>> +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger "
>>> +                   "than huge page size 0x%" PRIx64, memory, hpagesize);
>>> +        goto error;
>>
>> Isn't exactly equal also allowed?  Maybe a better wording is "should be
>> a multiple of the huge page size"
> 
> Yes. I'll change the error message.
> 

Thinking about it more, should you also enforce that it is a multiple?

As in: if (!memory || memory % hpageszie) { error... }

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Hu Tao]

On Thu, Jul 03, 2014 at 10:05:56PM -0600, Eric Blake wrote:
> On 07/03/2014 08:56 PM, Hu Tao wrote:
> > On Thu, Jul 03, 2014 at 06:33:28AM -0600, Eric Blake wrote:
> >> On 07/03/2014 12:10 AM, Hu Tao wrote:
> >>> This patch fixes two problems of memory-backend-file:
> >>>
> >>> 1. If user adds a memory-backend-file object using object_add command,
> >>>    specifying a non-existing directory for property mem-path, qemu
> >>>    will core dump with message:
> >>>
> >>>      /nonexistingdir: No such file or directory
> >>>      Bad ram offset fffffffffffff000
> >>>      Aborted (core dumped)
> >>>
> >>> 2. If user adds a memory-backend-file object using object_add command,
> >>>    specifying a size that is less than huge page size, qemu
> >>>    will core dump with message:
> >>>
> >>>      Bad ram offset fffffffffffff000
> >>>      Aborted (core dumped)
> >>>
> >>
> >> Might be nice if the commit message also shows the new message issued
> >> for the same cases after the patch is applied.
> > 
> > OK.
> > 
> 
> >>>  
> >>>      if (memory < hpagesize) {
> >>> -        return NULL;
> >>> +        error_setg(errp, "memory size 0x" RAM_ADDR_FMT " should be larger "
> >>> +                   "than huge page size 0x%" PRIx64, memory, hpagesize);
> >>> +        goto error;
> >>
> >> Isn't exactly equal also allowed?  Maybe a better wording is "should be
> >> a multiple of the huge page size"
> > 
> > Yes. I'll change the error message.
> > 
> 
> Thinking about it more, should you also enforce that it is a multiple?
> 
> As in: if (!memory || memory % hpageszie) { error... }

The memory size is rounded up to hpagesize when allocating memory, we
can waste at most hpagesize-1 bytes memory. I don't think it's a
problem.

In the other side, do you think we should give an error in the case like
-object memory-backend-ram,size=1111M(given 2M hpagesize)?

Regards,
Hu

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Paolo Bonzini]

Il 04/07/2014 09:43, Hu Tao ha scritto:
> The memory size is rounded up to hpagesize when allocating memory, we
> can waste at most hpagesize-1 bytes memory. I don't think it's a
> problem.
>
> In the other side, do you think we should give an error in the case like
> -object memory-backend-ram,size=1111M(given 2M hpagesize)?

I think for gb pages it can make sense to waste a little memory (or even 
not so little, like if you have a 2.5 GB guest).

Paolo

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Michael S. Tsirkin]

On Fri, Jul 04, 2014 at 09:47:43AM +0200, Paolo Bonzini wrote:
> Il 04/07/2014 09:43, Hu Tao ha scritto:
> >The memory size is rounded up to hpagesize when allocating memory, we
> >can waste at most hpagesize-1 bytes memory. I don't think it's a
> >problem.
> >
> >In the other side, do you think we should give an error in the case like
> >-object memory-backend-ram,size=1111M(given 2M hpagesize)?
> 
> I think for gb pages it can make sense to waste a little memory (or even not
> so little, like if you have a 2.5 GB guest).
> 
> Paolo

For 2.1, all we can take at this point is obvious bugfixes.
Propagating error up the stack instead of aborting
might be fine if it's ready before rc1, that is Monday at the latest.

Afterwards, I'll only merge high priority bugfixes, improving
handling of user errors doesn't count as there's
an easy workaround.

Whether rounding up to page size is a good idea or not,
I'm inclined to say this is 2.2 material.

-- 
MST

Re: [Qemu-devel] [PATCH for 2.1 2/2] memory-backend-file: improve error handling

[Hu Tao]

On Sun, Jul 06, 2014 at 09:42:42AM +0300, Michael S. Tsirkin wrote:
> On Fri, Jul 04, 2014 at 09:47:43AM +0200, Paolo Bonzini wrote:
> > Il 04/07/2014 09:43, Hu Tao ha scritto:
> > >The memory size is rounded up to hpagesize when allocating memory, we
> > >can waste at most hpagesize-1 bytes memory. I don't think it's a
> > >problem.
> > >
> > >In the other side, do you think we should give an error in the case like
> > >-object memory-backend-ram,size=1111M(given 2M hpagesize)?
> > 
> > I think for gb pages it can make sense to waste a little memory (or even not
> > so little, like if you have a 2.5 GB guest).
> > 
> > Paolo
> 
> For 2.1, all we can take at this point is obvious bugfixes.
> Propagating error up the stack instead of aborting
> might be fine if it's ready before rc1, that is Monday at the latest.
> 
> Afterwards, I'll only merge high priority bugfixes, improving
> handling of user errors doesn't count as there's
> an easy workaround.
> 
> Whether rounding up to page size is a good idea or not,
> I'm inclined to say this is 2.2 material.

I'll send patches not including this part today.

Regards,
Hu