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 V2 for 2.1 2/3] memory: add errp parameter to memory_region_init_ram() and memory_region_init_ram_ptr()
Date: Mon, 7 Jul 2014 06:20:11 +0300 [thread overview]
Message-ID: <20140707032011.GA4589@redhat.com> (raw)
In-Reply-To: <b7a181d8374914abbeb394067fca42797d1bc94e.1404701006.git.hutao@cn.fujitsu.com>
On Mon, Jul 07, 2014 at 10:58:07AM +0800, Hu Tao wrote:
> This patch reintroduces memory_region_init_ram() and
> memory_region_init_ram_ptr() which are almost the same as the renamed
> ones in the previous patch, except that an errp parameter is introduced
> to let callers handle error.
>
> In hostmem-ram.c we call memory_region_init_ram() now rather than
> memory_region_init_ram_nofail() so that error can be handled.
s/error/errors/ above.
I fixed it up.
> 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 ++++-
> include/exec/memory.h | 39 +++++++++++++++++++++++++++++++++++-
> include/exec/ram_addr.h | 4 ++--
> memory.c | 53 +++++++++++++++++++++++++++++++++++++++++++++----
> 7 files changed, 122 insertions(+), 20 deletions(-)
>
> diff --git a/backends/hostmem-ram.c b/backends/hostmem-ram.c
> index 9d4960b..a67a134 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_nofail(&backend->mr, OBJECT(backend), path,
> - backend->size);
> + memory_region_init_ram(&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 28c86ee..279239d 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -304,6 +304,22 @@ void memory_region_init_io(MemoryRegion *mr,
> uint64_t size);
>
> /**
> + * memory_region_init_ram: 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(MemoryRegion *mr,
> + struct Object *owner,
> + const char *name,
> + uint64_t size,
> + Error **errp);
> +
> +/**
> * memory_region_init_ram_nofail: Initialize RAM memory region. Accesses into
> * the region will modify memory directly.
> *
> @@ -340,6 +356,25 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
> #endif
>
> /**
> + * memory_region_init_ram_ptr: 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(MemoryRegion *mr,
> + struct Object *owner,
> + const char *name,
> + uint64_t size,
> + void *ptr,
> + Error **errp);
> +
> +/**
> * memory_region_init_ram_ptr_nofail: Initialize RAM memory region from a
> * user-provided pointer. Accesses into the
> * region will modify memory directly.
> @@ -384,13 +419,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 5f8a9f5..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
>
> @@ -1160,16 +1161,37 @@ 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,
> + 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, 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);
> + 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__
> @@ -1189,17 +1211,39 @@ void memory_region_init_ram_from_file(MemoryRegion *mr,
> }
> #endif
>
> +void memory_region_init_ram_ptr(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_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);
> + 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,
> --
> 1.9.3
next prev parent reply other threads:[~2014-07-07 4:18 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-07-07 2:58 [Qemu-devel] [PATCH V2 for 2.1 0/3] bug fixs for memory backend Hu Tao
2014-07-07 2:58 ` [Qemu-devel] [PATCH V2 for 2.1 1/3] memory: rename memory_region_init_ram() and memory_region_init_ram_ptr() Hu Tao
2014-07-07 2:58 ` [Qemu-devel] [PATCH V2 for 2.1 2/3] memory: add errp parameter to " Hu Tao
2014-07-07 3:20 ` Michael S. Tsirkin [this message]
2014-07-07 5:39 ` Michael S. Tsirkin
2014-07-07 2:58 ` [Qemu-devel] [PATCH V2 for 2.1 3/3] exec: improve error handling and reporting in file_ram_alloc() and gethugepagesize() Hu Tao
2014-07-07 3:24 ` Michael S. Tsirkin
2014-07-07 3:24 ` [Qemu-devel] [PATCH V2 for 2.1 0/3] bug fixs for memory backend Michael S. Tsirkin
2014-07-07 3:35 ` Michael S. Tsirkin
2014-07-07 8:17 ` Hu Tao
2014-07-07 9:08 ` 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=20140707032011.GA4589@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.