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 08:39:38 +0300 [thread overview]
Message-ID: <20140707053938.GA17980@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.
>
> 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);
> + }
This is not the best way to do it.
It's better to call memory_region_init_ram from
memory_region_init_ram_nofail.
> }
>
> #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 6:37 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
2014-07-07 5:39 ` Michael S. Tsirkin [this message]
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=20140707053938.GA17980@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).