Re: [PATCH 2/2] hostmem: detect host backend memory is being used properly

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 13 Jul 2016 12:18:06 +0800
Xiao Guangrong <guangrong.xiao@linux.intel.com> wrote:

> Currently, we use memory_region_is_mapped() to detect if the host
> backend memory is being used. This works if the memory is directly
> mapped into guest's address space, however, it is not true for
> nvdimm as it uses aliased memory region to map the memory. This is
> why this bug can happen:
>    https://bugzilla.redhat.com/show_bug.cgi?id=1352769
> 
> Fix it by introduce a new filed, is_mapped, to HostMemoryBackend,
> we set/clear this filed accordingly when the device link/unlink to
> host backend memory
> 
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
I wonder if it could be cleaner to extend QOM API with

object_get_refcount();

and then add

bool hostmem_is_busy()
{
    return object_get_refcount() > 1;
}

that would work as not used used hostmem would have ref counter == 1
and when front-end starts to use it, it calls
  qdev_prop_allow_set_link_before_realize()
which rises ref counter of backend to 2.

Also see a comment below.

> ---
>  backends/hostmem.c       | 15 +++++++++++----
>  hw/mem/pc-dimm.c         | 18 +++++++++++-------
>  hw/misc/ivshmem.c        | 14 ++++++++++----
>  include/sysemu/hostmem.h |  4 +++-
>  4 files changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 8dede4d..ac80257 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -264,6 +264,16 @@ host_memory_backend_get_memory(HostMemoryBackend *backend, Error **errp)
>      return memory_region_size(&backend->mr) ? &backend->mr : NULL;
>  }
>  
> +void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped)
> +{
> +    backend->is_mapped = mapped;
> +}
> +
> +bool host_memory_backend_is_mapped(HostMemoryBackend *backend)
> +{
> +    return backend->is_mapped;
> +}
> +
>  static void
>  host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>  {
> @@ -341,10 +351,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, Error **errp)
>  static bool
>  host_memory_backend_can_be_deleted(UserCreatable *uc, Error **errp)
>  {
> -    MemoryRegion *mr;
> -
> -    mr = host_memory_backend_get_memory(MEMORY_BACKEND(uc), errp);
> -    if (memory_region_is_mapped(mr)) {
> +    if (host_memory_backend_is_mapped(MEMORY_BACKEND(uc))) {
>          return false;
>      } else {
>          return true;
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 249193a..9e8dab0 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -369,14 +369,9 @@ static void pc_dimm_get_size(Object *obj, Visitor *v, const char *name,
>  static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name,
>                                        Object *val, Error **errp)
>  {
> -    MemoryRegion *mr;
>      Error *local_err = NULL;
>  
> -    mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &local_err);
> -    if (local_err) {
> -        goto out;
> -    }
> -    if (memory_region_is_mapped(mr)) {
> +    if (host_memory_backend_is_mapped(MEMORY_BACKEND(val))) {
>          char *path = object_get_canonical_path_component(val);
>          error_setg(&local_err, "can't use already busy memdev: %s", path);
>          g_free(path);
> @@ -384,7 +379,6 @@ static void pc_dimm_check_memdev_is_busy(Object *obj, const char *name,
>          qdev_prop_allow_set_link_before_realize(obj, name, val, &local_err);
>      }
>  
> -out:
>      error_propagate(errp, local_err);
>  }
>  
> @@ -421,6 +415,15 @@ static void pc_dimm_realize(DeviceState *dev, Error **errp)
>      if (ddc->realize) {
>          ddc->realize(dimm, errp);
>      }
> +
> +    host_memory_backend_set_mapped(dimm->hostmem, true);
> +}
> +
> +static void pc_dimm_unrealize(DeviceState *dev, Error **errp)
> +{
> +    PCDIMMDevice *dimm = PC_DIMM(dev);
> +
> +    host_memory_backend_set_mapped(dimm->hostmem, false);
>  }
>  
>  static MemoryRegion *pc_dimm_get_memory_region(PCDIMMDevice *dimm)
> @@ -439,6 +442,7 @@ static void pc_dimm_class_init(ObjectClass *oc, void *data)
>      PCDIMMDeviceClass *ddc = PC_DIMM_CLASS(oc);
>  
>      dc->realize = pc_dimm_realize;
> +    dc->unrealize = pc_dimm_unrealize;
>      dc->props = pc_dimm_properties;
>      dc->desc = "DIMM memory module";
>  
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index c4dde3a..7e7c843 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -1008,10 +1008,7 @@ static const TypeInfo ivshmem_common_info = {
>  static void ivshmem_check_memdev_is_busy(Object *obj, const char *name,
>                                           Object *val, Error **errp)
this function seems to complete duplicate of pc_dimm_check_memdev_is_busy()

can we generalize it to host_memory_backend_is_busy()?


>  {
> -    MemoryRegion *mr;
> -
> -    mr = host_memory_backend_get_memory(MEMORY_BACKEND(val), &error_abort);
> -    if (memory_region_is_mapped(mr)) {
> +    if (host_memory_backend_is_mapped(MEMORY_BACKEND(val))) {
>          char *path = object_get_canonical_path_component(val);
>          error_setg(errp, "can't use already busy memdev: %s", path);
>          g_free(path);
> @@ -1060,6 +1057,14 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      ivshmem_common_realize(dev, errp);
> +    host_memory_backend_set_mapped(s->hostmem, true);
> +}
> +
> +static void ivshmem_plain_exit(PCIDevice *pci_dev)
> +{
> +    IVShmemState *s = IVSHMEM_COMMON(pci_dev);
> +
> +    host_memory_backend_set_mapped(s->hostmem, false);
>  }
>  
>  static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
> @@ -1068,6 +1073,7 @@ static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
>      k->realize = ivshmem_plain_realize;
> +    k->exit = ivshmem_plain_exit;
>      dc->props = ivshmem_plain_properties;
>      dc->vmsd = &ivshmem_plain_vmsd;
>  }
> diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
> index 4d6617e..c903404 100644
> --- a/include/sysemu/hostmem.h
> +++ b/include/sysemu/hostmem.h
> @@ -53,7 +53,7 @@ struct HostMemoryBackend {
>      /* protected */
>      uint64_t size;
>      bool merge, dump;
> -    bool prealloc, force_prealloc;
> +    bool prealloc, force_prealloc, is_mapped;
>      DECLARE_BITMAP(host_nodes, MAX_NODES + 1);
>      HostMemPolicy policy;
>  
> @@ -63,4 +63,6 @@ struct HostMemoryBackend {
>  MemoryRegion *host_memory_backend_get_memory(HostMemoryBackend *backend,
>                                               Error **errp);
>  
> +void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
> +bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
>  #endif