qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V2 for 2.1 2/3] memory: add errp parameter to me


From: Michael S. Tsirkin
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

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 <address@hidden>
> ---
>  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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]