[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V3 for 2.1 1/2] memory: add memory_region_init_r
|
From: |
Hu Tao |
|
Subject: |
Re: [Qemu-devel] [PATCH V3 for 2.1 1/2] memory: add memory_region_init_ram_may_fail() and memory_region_init_ram_ptr_may_fail() |
|
Date: |
Fri, 11 Jul 2014 16:40:03 +0800 |
|
User-agent: |
Mutt/1.5.21 (2010-09-15) |
On Mon, Jul 07, 2014 at 03:27:35PM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 07, 2014 at 06:55:27PM +0800, Hu Tao wrote:
> > 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.
> >
> > 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 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.
> >
> > 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 ++++++++++++++++++--------
> > hw/block/pflash_cfi01.c | 5 +++-
> > hw/block/pflash_cfi02.c | 5 +++-
>
> Why do we need to patch pflash?
> We can't trigger it with object_add, can we?
pflash calls memory_region_init_rom_device which calls qemu_ram_alloc
to which an errp is added. so I added an errp to
memory_region_init_rom_device too. The change is small so I don't think
it is worth splitting it into a single patch.
Or we can make memory_region_init_rom_device nofail thus avoid patching
pflash. Even if later we decide to add errp to
memory_region_init_rom_device the patch won't be big since there is
little users.
>
> > include/exec/memory.h | 40 +++++++++++++++++++++++++++++++-
> > include/exec/ram_addr.h | 4 ++--
> > memory.c | 61
> > +++++++++++++++++++++++++++++++++++++++----------
> > 7 files changed, 123 insertions(+), 28 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 5a2a25e..ca7741b 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/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/include/exec/memory.h b/include/exec/memory.h
> > index e2c8e3e..c720d43 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,13 +420,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..6b1f6c3 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,
> > @@ -1221,7 +1257,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 +1266,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,
> > --
> > 1.9.3