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

Commit Message

Xiao Guangrong July 13, 2016, 4:18 a.m. UTC

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 <[e-mail]>
---
 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(-)

Comments

Igor Mammedov July 13, 2016, 7:30 a.m. UTC | #1

On Wed, 13 Jul 2016 12:18:06 +0800
Xiao Guangrong <[e-mail]> 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 <[e-mail]>
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

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Bonzini July 13, 2016, 10:49 a.m. UTC | #2

On 13/07/2016 09:30, Igor Mammedov wrote:
> 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.

I disagree---there are many reasons why a refcount can be > 1, and given
that we use RCU the release of the reference can be delayed arbitrarily.

Paolo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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)
 {
-    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