[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] hostmem: detect host backend memory is bein
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] hostmem: detect host backend memory is being used properly |
Date: |
Wed, 13 Jul 2016 09:30:44 +0200 |
On Wed, 13 Jul 2016 12:18:06 +0800
Xiao Guangrong <address@hidden> 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 <address@hidden>
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