bug fixs for memory backend

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 <[email protected]>

---
 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(-)

backends/hostmem-ram.c +0 -0

index d9a8290..a67a134 100644

exec.c +7 -4

index 5a2a25e..8c2a91d 100644
@@ -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");

exec.c +4 -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));

exec.c +4 -2

+    }
+    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)

hw/block/pflash_cfi01.c +4 -1

index f9507b4..92b8b87 100644
@@ -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);

hw/block/pflash_cfi02.c +4 -1

index 8d4b828..b773f19 100644
@@ -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;

hw/core/loader.c +1 -1

index 2bf6b8f..fdebf86 100644
@@ -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);
 

hw/display/vga.c +1 -1

index 4b089a3..f17a4b4 100644
@@ -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);

hw/display/vmware_vga.c +2 -1

index 591b645..a3738be 100644
@@ -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);
 

hw/i386/kvm/pci-assign.c +5 -4

index de33657..52d0db0 100644
@@ -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);

hw/i386/pc.c +1 -1

index 2cf22b1..f503b0e 100644
@@ -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,

hw/i386/pc_sysfw.c +2 -2

index 75a7ebb..a15963b 100644
@@ -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);

hw/misc/ivshmem.c +5 -4

index 768e528..e3b6e06 100644
@@ -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",

hw/misc/vfio.c +2 -1

index aef4c9c..5bdee3e 100644
@@ -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. */

hw/pci/pci.c +1 -1

index 17ed510..ba39f08 100644
@@ -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);

include/exec/memory.h +29 -3

index e2c8e3e..2e74518 100644
@@ -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

include/exec/ram_addr.h +2 -2

index e9eb831..998ac4f 100644
@@ -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);

memory.c +51 -6

index 64d7176..dc24c53 100644
@@ -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,

numa.c +2 -2

index 2fde740..dabba4f 100644
@@ -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);
 }

Signature · 1 line

1.9.3

Quoted 18 lines · 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.

Quoted 6 lines

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.

Quoted 28 lines

Signed-off-by Hu Tao <[email protected]>

---
 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(-)

backends/hostmem-ram.c +0 -0

index d9a8290..a67a134 100644
@@ -27,7 +27,7 @@ ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
 

backends/hostmem-ram.c ram_backend_memory_alloc(HostMemoryBackend *backend, Error **errp) · line 33

Quoted 7 lines

backends/hostmem-ram.c +1 -1

     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

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)

Quoted 14 lines · 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:

Quoted 3 lines

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.

Quoted 8 lines

Signed-off-by Hu Tao <[email protected]>

---
 exec.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

exec.c static void *file_ram_alloc(RAMBlock *block, · line 1039

Quoted 5 lines

     if (memory < hpagesize) {

exec.c +3 -1

-        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"

Signature · 3 lines

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

Quoted 15 lines · Eric Blake

Quoted 14 lines · 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:

Quoted 6 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 6 lines

 
     if (memory < hpagesize) {

exec.c +3 -1

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

Quoted 2 lines

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.

Quoted 36 lines · Hu Tao

Quoted 15 lines · Eric Blake

Quoted 14 lines · 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:

Quoted 6 lines

Quoted 3 lines

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.

Quoted 9 lines

exec.c static void *file_ram_alloc(RAMBlock *block, · line 1039

Quoted 6 lines

 
     if (memory < hpagesize) {

exec.c +3 -1

-        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"

Quoted 2 lines

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)?

Il 04/07/2014 09:43, Hu Tao ha scritto:

Quoted 4 lines

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.

Quoted 2 lines

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

Quoted 5 lines · Paolo Bonzini

Il 04/07/2014 09:43, Hu Tao ha scritto:

Quoted 4 lines

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.

Quoted 7 lines

Quoted 2 lines

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.