From: "Michael S. Tsirkin" <mst@redhat.com>
To: Hu Tao <hutao@cn.fujitsu.com>
Cc: Yasunori Goto <y-goto@jp.fujitsu.com>,
Paolo Bonzini <pbonzini@redhat.com>,
qemu-devel@nongnu.org, Igor Mammedov <imammedo@redhat.com>
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: Mon, 7 Jul 2014 15:27:35 +0300 [thread overview]
Message-ID: <20140707122735.GA20927@redhat.com> (raw)
In-Reply-To: <cea4a2005b2deea4833a30bdd074e1b111121de0.1404730357.git.hutao@cn.fujitsu.com>
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 <hutao@cn.fujitsu.com>
> ---
> 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?
> 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
next prev parent reply other threads:[~2014-07-07 12:25 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-07 10:55 [Qemu-devel] [PATCH V3 for 2.1 0/2] bug fixs for memory backend Hu Tao
2014-07-07 10:55 ` [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() Hu Tao
2014-07-07 12:27 ` Michael S. Tsirkin [this message]
2014-07-11 8:40 ` Hu Tao
2014-07-07 10:55 ` [Qemu-devel] [PATCH V3 for 2.1 2/2] exec: improve error handling and reporting in file_ram_alloc() and gethugepagesize() Hu Tao
2014-07-07 12:38 ` Michael S. Tsirkin
2014-07-07 12:39 ` [Qemu-devel] [PATCH V3 for 2.1 0/2] bug fixs for memory backend Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20140707122735.GA20927@redhat.com \
--to=mst@redhat.com \
--cc=hutao@cn.fujitsu.com \
--cc=imammedo@redhat.com \
--cc=pbonzini@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=y-goto@jp.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.