* [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-13 4:18 ` Xiao Guangrong
0 siblings, 0 replies; 17+ messages in thread
From: Xiao Guangrong @ 2016-07-13 4:18 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel,
Xiao Guangrong
'info memdev' crashes QEMU:
(qemu) info memdev
Unexpected error in parse_str() at qapi/string-input-visitor.c:111:
Parameter 'null' expects an int64 value or range
It is caused by null uint16List is returned if 'host-nodes' is the default
value
Return MAX_NODES under this case to fix this bug
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
backends/hostmem.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 6e28be1..8dede4d 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -64,6 +64,14 @@ out:
error_propagate(errp, local_err);
}
+static uint16List **host_memory_append_node(uint16List **node,
+ unsigned long value)
+{
+ *node = g_malloc0(sizeof(**node));
+ (*node)->value = value;
+ return &(*node)->next;
+}
+
static void
host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
@@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
unsigned long value;
value = find_first_bit(backend->host_nodes, MAX_NODES);
+
+ node = host_memory_append_node(node, value);
+
if (value == MAX_NODES) {
- return;
+ goto out;
}
- *node = g_malloc0(sizeof(**node));
- (*node)->value = value;
- node = &(*node)->next;
-
do {
value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
if (value == MAX_NODES) {
break;
}
- *node = g_malloc0(sizeof(**node));
- (*node)->value = value;
- node = &(*node)->next;
+ node = host_memory_append_node(node, value);
} while (true);
+out:
visit_type_uint16List(v, name, &host_nodes, errp);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-13 4:18 ` Xiao Guangrong
0 siblings, 0 replies; 17+ messages in thread
From: Xiao Guangrong @ 2016-07-13 4:18 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel,
Xiao Guangrong
'info memdev' crashes QEMU:
(qemu) info memdev
Unexpected error in parse_str() at qapi/string-input-visitor.c:111:
Parameter 'null' expects an int64 value or range
It is caused by null uint16List is returned if 'host-nodes' is the default
value
Return MAX_NODES under this case to fix this bug
Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
backends/hostmem.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/backends/hostmem.c b/backends/hostmem.c
index 6e28be1..8dede4d 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -64,6 +64,14 @@ out:
error_propagate(errp, local_err);
}
+static uint16List **host_memory_append_node(uint16List **node,
+ unsigned long value)
+{
+ *node = g_malloc0(sizeof(**node));
+ (*node)->value = value;
+ return &(*node)->next;
+}
+
static void
host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
void *opaque, Error **errp)
@@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
unsigned long value;
value = find_first_bit(backend->host_nodes, MAX_NODES);
+
+ node = host_memory_append_node(node, value);
+
if (value == MAX_NODES) {
- return;
+ goto out;
}
- *node = g_malloc0(sizeof(**node));
- (*node)->value = value;
- node = &(*node)->next;
-
do {
value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
if (value == MAX_NODES) {
break;
}
- *node = g_malloc0(sizeof(**node));
- (*node)->value = value;
- node = &(*node)->next;
+ node = host_memory_append_node(node, value);
} while (true);
+out:
visit_type_uint16List(v, name, &host_nodes, errp);
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] hostmem: detect host backend memory is being used properly
2016-07-13 4:18 ` [Qemu-devel] " Xiao Guangrong
@ 2016-07-13 4:18 ` Xiao Guangrong
-1 siblings, 0 replies; 17+ messages in thread
From: Xiao Guangrong @ 2016-07-13 4:18 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel,
Xiao Guangrong
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>
---
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)
{
- 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
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH 2/2] hostmem: detect host backend memory is being used properly
@ 2016-07-13 4:18 ` Xiao Guangrong
0 siblings, 0 replies; 17+ messages in thread
From: Xiao Guangrong @ 2016-07-13 4:18 UTC (permalink / raw)
To: pbonzini, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel,
Xiao Guangrong
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>
---
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)
{
- 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
--
1.8.3.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hostmem: detect host backend memory is being used properly
2016-07-13 4:18 ` [Qemu-devel] " Xiao Guangrong
@ 2016-07-13 7:30 ` Igor Mammedov
-1 siblings, 0 replies; 17+ messages in thread
From: Igor Mammedov @ 2016-07-13 7:30 UTC (permalink / raw)
To: Xiao Guangrong
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha, pbonzini, rth
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hostmem: detect host backend memory is being used properly
@ 2016-07-13 7:30 ` Igor Mammedov
0 siblings, 0 replies; 17+ messages in thread
From: Igor Mammedov @ 2016-07-13 7:30 UTC (permalink / raw)
To: Xiao Guangrong
Cc: pbonzini, gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
2016-07-13 4:18 ` [Qemu-devel] " Xiao Guangrong
@ 2016-07-13 10:45 ` Paolo Bonzini
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 10:45 UTC (permalink / raw)
To: Xiao Guangrong, imammedo
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
Markus Armbruster, stefanha, rth
On 13/07/2016 06:18, Xiao Guangrong wrote:
>
> Return MAX_NODES under this case to fix this bug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> backends/hostmem.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 6e28be1..8dede4d 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -64,6 +64,14 @@ out:
> error_propagate(errp, local_err);
> }
>
> +static uint16List **host_memory_append_node(uint16List **node,
> + unsigned long value)
> +{
> + *node = g_malloc0(sizeof(**node));
> + (*node)->value = value;
> + return &(*node)->next;
> +}
> +
> static void
> host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
> unsigned long value;
>
> value = find_first_bit(backend->host_nodes, MAX_NODES);
> +
> + node = host_memory_append_node(node, value);
> +
> if (value == MAX_NODES) {
> - return;
> + goto out;
> }
>
> - *node = g_malloc0(sizeof(**node));
> - (*node)->value = value;
> - node = &(*node)->next;
> -
> do {
> value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
> if (value == MAX_NODES) {
> break;
> }
>
> - *node = g_malloc0(sizeof(**node));
> - (*node)->value = value;
> - node = &(*node)->next;
> + node = host_memory_append_node(node, value);
> } while (true);
>
> +out:
> visit_type_uint16List(v, name, &host_nodes, errp);
This function is leaking host_nodes, so you need a
qapi_free_uint16List(head);
here (and saving the head pointer on the first call to
host_memory_append_node). The bug is preexisting.
I'm curious about one thing. Eric/Markus, it would be nice to open code
the visit of the list with
visit_start_list(v, name, NULL, 0, &err);
if (err) {
goto out;
}
...
visit_type_uint16(v, name, &value, &err);
visit_next_list(v, NULL, 0);
...
visit_end_list(v, NULL);
We know here that on the other side there is an output visitor.
However, it doesn't work because visit_next_list asserts that tail ==
NULL. Would it be easy to support this idiom, and would it make sense
to extend it to other kinds of visitor?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-13 10:45 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 10:45 UTC (permalink / raw)
To: Xiao Guangrong, imammedo
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel,
Eric Blake, Markus Armbruster
On 13/07/2016 06:18, Xiao Guangrong wrote:
>
> Return MAX_NODES under this case to fix this bug
>
> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
> ---
> backends/hostmem.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/backends/hostmem.c b/backends/hostmem.c
> index 6e28be1..8dede4d 100644
> --- a/backends/hostmem.c
> +++ b/backends/hostmem.c
> @@ -64,6 +64,14 @@ out:
> error_propagate(errp, local_err);
> }
>
> +static uint16List **host_memory_append_node(uint16List **node,
> + unsigned long value)
> +{
> + *node = g_malloc0(sizeof(**node));
> + (*node)->value = value;
> + return &(*node)->next;
> +}
> +
> static void
> host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
> void *opaque, Error **errp)
> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
> unsigned long value;
>
> value = find_first_bit(backend->host_nodes, MAX_NODES);
> +
> + node = host_memory_append_node(node, value);
> +
> if (value == MAX_NODES) {
> - return;
> + goto out;
> }
>
> - *node = g_malloc0(sizeof(**node));
> - (*node)->value = value;
> - node = &(*node)->next;
> -
> do {
> value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
> if (value == MAX_NODES) {
> break;
> }
>
> - *node = g_malloc0(sizeof(**node));
> - (*node)->value = value;
> - node = &(*node)->next;
> + node = host_memory_append_node(node, value);
> } while (true);
>
> +out:
> visit_type_uint16List(v, name, &host_nodes, errp);
This function is leaking host_nodes, so you need a
qapi_free_uint16List(head);
here (and saving the head pointer on the first call to
host_memory_append_node). The bug is preexisting.
I'm curious about one thing. Eric/Markus, it would be nice to open code
the visit of the list with
visit_start_list(v, name, NULL, 0, &err);
if (err) {
goto out;
}
...
visit_type_uint16(v, name, &value, &err);
visit_next_list(v, NULL, 0);
...
visit_end_list(v, NULL);
We know here that on the other side there is an output visitor.
However, it doesn't work because visit_next_list asserts that tail ==
NULL. Would it be easy to support this idiom, and would it make sense
to extend it to other kinds of visitor?
Thanks,
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] hostmem: detect host backend memory is being used properly
2016-07-13 7:30 ` [Qemu-devel] " Igor Mammedov
@ 2016-07-13 10:49 ` Paolo Bonzini
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 10:49 UTC (permalink / raw)
To: Igor Mammedov, Xiao Guangrong
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha, rth
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] hostmem: detect host backend memory is being used properly
@ 2016-07-13 10:49 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 10:49 UTC (permalink / raw)
To: Igor Mammedov, Xiao Guangrong
Cc: gleb, mtosatti, stefanha, mst, rth, ehabkost, kvm, qemu-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
2016-07-13 10:45 ` [Qemu-devel] " Paolo Bonzini
@ 2016-07-13 11:29 ` Markus Armbruster
-1 siblings, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2016-07-13 11:29 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Xiao Guangrong, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, imammedo, rth
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 13/07/2016 06:18, Xiao Guangrong wrote:
>>
>> Return MAX_NODES under this case to fix this bug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> backends/hostmem.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6e28be1..8dede4d 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -64,6 +64,14 @@ out:
>> error_propagate(errp, local_err);
>> }
>>
>> +static uint16List **host_memory_append_node(uint16List **node,
>> + unsigned long value)
>> +{
>> + *node = g_malloc0(sizeof(**node));
>> + (*node)->value = value;
>> + return &(*node)->next;
>> +}
>> +
>> static void
>> host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> unsigned long value;
>>
>> value = find_first_bit(backend->host_nodes, MAX_NODES);
>> +
>> + node = host_memory_append_node(node, value);
>> +
>> if (value == MAX_NODES) {
>> - return;
>> + goto out;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> -
>> do {
>> value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
>> if (value == MAX_NODES) {
>> break;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> + node = host_memory_append_node(node, value);
>> } while (true);
>>
>> +out:
>> visit_type_uint16List(v, name, &host_nodes, errp);
>
> This function is leaking host_nodes, so you need a
>
> qapi_free_uint16List(head);
>
> here (and saving the head pointer on the first call to
> host_memory_append_node). The bug is preexisting.
>
> I'm curious about one thing. Eric/Markus, it would be nice to open code
> the visit of the list with
>
> visit_start_list(v, name, NULL, 0, &err);
> if (err) {
> goto out;
> }
> ...
> visit_type_uint16(v, name, &value, &err);
> visit_next_list(v, NULL, 0);
> ...
> visit_end_list(v, NULL);
>
> We know here that on the other side there is an output visitor.
> However, it doesn't work because visit_next_list asserts that tail ==
> NULL. Would it be easy to support this idiom, and would it make sense
> to extend it to other kinds of visitor?
visit_next_list() asserts tail != NULL because to protect the
next_list() method. qmp_output_next_list() dereferences tail.
Note that you don't have to call visit_next_list() in a virtual visit.
For an example, see prop_get_fdt(). Good enough already?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-13 11:29 ` Markus Armbruster
0 siblings, 0 replies; 17+ messages in thread
From: Markus Armbruster @ 2016-07-13 11:29 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Xiao Guangrong, imammedo, ehabkost, kvm, mst, gleb, mtosatti,
qemu-devel, stefanha, rth
Paolo Bonzini <pbonzini@redhat.com> writes:
> On 13/07/2016 06:18, Xiao Guangrong wrote:
>>
>> Return MAX_NODES under this case to fix this bug
>>
>> Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> ---
>> backends/hostmem.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/backends/hostmem.c b/backends/hostmem.c
>> index 6e28be1..8dede4d 100644
>> --- a/backends/hostmem.c
>> +++ b/backends/hostmem.c
>> @@ -64,6 +64,14 @@ out:
>> error_propagate(errp, local_err);
>> }
>>
>> +static uint16List **host_memory_append_node(uint16List **node,
>> + unsigned long value)
>> +{
>> + *node = g_malloc0(sizeof(**node));
>> + (*node)->value = value;
>> + return &(*node)->next;
>> +}
>> +
>> static void
>> host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> void *opaque, Error **errp)
>> @@ -74,25 +82,23 @@ host_memory_backend_get_host_nodes(Object *obj, Visitor *v, const char *name,
>> unsigned long value;
>>
>> value = find_first_bit(backend->host_nodes, MAX_NODES);
>> +
>> + node = host_memory_append_node(node, value);
>> +
>> if (value == MAX_NODES) {
>> - return;
>> + goto out;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> -
>> do {
>> value = find_next_bit(backend->host_nodes, MAX_NODES, value + 1);
>> if (value == MAX_NODES) {
>> break;
>> }
>>
>> - *node = g_malloc0(sizeof(**node));
>> - (*node)->value = value;
>> - node = &(*node)->next;
>> + node = host_memory_append_node(node, value);
>> } while (true);
>>
>> +out:
>> visit_type_uint16List(v, name, &host_nodes, errp);
>
> This function is leaking host_nodes, so you need a
>
> qapi_free_uint16List(head);
>
> here (and saving the head pointer on the first call to
> host_memory_append_node). The bug is preexisting.
>
> I'm curious about one thing. Eric/Markus, it would be nice to open code
> the visit of the list with
>
> visit_start_list(v, name, NULL, 0, &err);
> if (err) {
> goto out;
> }
> ...
> visit_type_uint16(v, name, &value, &err);
> visit_next_list(v, NULL, 0);
> ...
> visit_end_list(v, NULL);
>
> We know here that on the other side there is an output visitor.
> However, it doesn't work because visit_next_list asserts that tail ==
> NULL. Would it be easy to support this idiom, and would it make sense
> to extend it to other kinds of visitor?
visit_next_list() asserts tail != NULL because to protect the
next_list() method. qmp_output_next_list() dereferences tail.
Note that you don't have to call visit_next_list() in a virtual visit.
For an example, see prop_get_fdt(). Good enough already?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
2016-07-13 11:29 ` [Qemu-devel] " Markus Armbruster
@ 2016-07-13 11:37 ` Paolo Bonzini
-1 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 11:37 UTC (permalink / raw)
To: Markus Armbruster
Cc: Xiao Guangrong, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel,
stefanha, imammedo, rth
On 13/07/2016 13:29, Markus Armbruster wrote:
>> > I'm curious about one thing. Eric/Markus, it would be nice to open code
>> > the visit of the list with
>> >
>> > visit_start_list(v, name, NULL, 0, &err);
>> > if (err) {
>> > goto out;
>> > }
>> > ...
>> > visit_type_uint16(v, name, &value, &err);
>> > visit_next_list(v, NULL, 0);
>> > ...
>> > visit_end_list(v, NULL);
>> >
>> > We know here that on the other side there is an output visitor.
>> > However, it doesn't work because visit_next_list asserts that tail ==
>> > NULL. Would it be easy to support this idiom, and would it make sense
>> > to extend it to other kinds of visitor?
> visit_next_list() asserts tail != NULL because to protect the
> next_list() method. qmp_output_next_list() dereferences tail.
>
> Note that you don't have to call visit_next_list() in a virtual visit.
> For an example, see prop_get_fdt(). Good enough already?
Yes, definitely! I'm queueing Guangrong's patch because it fixes a
crash and the leak existed before, but without next_list we can indeed
visit a "virtual" list and fix the leak. It can be done during the -rc
period.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-13 11:37 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-07-13 11:37 UTC (permalink / raw)
To: Markus Armbruster
Cc: Xiao Guangrong, imammedo, ehabkost, kvm, mst, gleb, mtosatti,
qemu-devel, stefanha, rth
On 13/07/2016 13:29, Markus Armbruster wrote:
>> > I'm curious about one thing. Eric/Markus, it would be nice to open code
>> > the visit of the list with
>> >
>> > visit_start_list(v, name, NULL, 0, &err);
>> > if (err) {
>> > goto out;
>> > }
>> > ...
>> > visit_type_uint16(v, name, &value, &err);
>> > visit_next_list(v, NULL, 0);
>> > ...
>> > visit_end_list(v, NULL);
>> >
>> > We know here that on the other side there is an output visitor.
>> > However, it doesn't work because visit_next_list asserts that tail ==
>> > NULL. Would it be easy to support this idiom, and would it make sense
>> > to extend it to other kinds of visitor?
> visit_next_list() asserts tail != NULL because to protect the
> next_list() method. qmp_output_next_list() dereferences tail.
>
> Note that you don't have to call visit_next_list() in a virtual visit.
> For an example, see prop_get_fdt(). Good enough already?
Yes, definitely! I'm queueing Guangrong's patch because it fixes a
crash and the leak existed before, but without next_list we can indeed
visit a "virtual" list and fix the leak. It can be done during the -rc
period.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
2016-07-13 11:37 ` [Qemu-devel] " Paolo Bonzini
(?)
@ 2016-07-15 6:56 ` Xiao Guangrong
2016-07-15 17:16 ` Eric Blake
-1 siblings, 1 reply; 17+ messages in thread
From: Xiao Guangrong @ 2016-07-15 6:56 UTC (permalink / raw)
To: Paolo Bonzini, Markus Armbruster
Cc: imammedo, ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha, rth
On 07/13/2016 07:37 PM, Paolo Bonzini wrote:
>
>
> On 13/07/2016 13:29, Markus Armbruster wrote:
>>>> I'm curious about one thing. Eric/Markus, it would be nice to open code
>>>> the visit of the list with
>>>>
>>>> visit_start_list(v, name, NULL, 0, &err);
>>>> if (err) {
>>>> goto out;
>>>> }
>>>> ...
>>>> visit_type_uint16(v, name, &value, &err);
>>>> visit_next_list(v, NULL, 0);
>>>> ...
>>>> visit_end_list(v, NULL);
>>>>
>>>> We know here that on the other side there is an output visitor.
>>>> However, it doesn't work because visit_next_list asserts that tail ==
>>>> NULL. Would it be easy to support this idiom, and would it make sense
>>>> to extend it to other kinds of visitor?
>> visit_next_list() asserts tail != NULL because to protect the
>> next_list() method. qmp_output_next_list() dereferences tail.
>>
>> Note that you don't have to call visit_next_list() in a virtual visit.
>> For an example, see prop_get_fdt(). Good enough already?
>
> Yes, definitely! I'm queueing Guangrong's patch because it fixes a
> crash and the leak existed before, but without next_list we can indeed
> visit a "virtual" list and fix the leak. It can be done during the -rc
> period.
So you want to build uint16List list and save it as a "virtual" list in
host_memory_backend_get_host_nodes(), then its caller can directly fetch
this 'virtual' list from the visit?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
2016-07-15 6:56 ` Xiao Guangrong
@ 2016-07-15 17:16 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2016-07-15 17:16 UTC (permalink / raw)
To: Xiao Guangrong, Paolo Bonzini, Markus Armbruster
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha, imammedo, rth
[-- Attachment #1.1: Type: text/plain, Size: 994 bytes --]
On 07/15/2016 12:56 AM, Xiao Guangrong wrote:
>>> Note that you don't have to call visit_next_list() in a virtual visit.
>>> For an example, see prop_get_fdt(). Good enough already?
>>
>> Yes, definitely! I'm queueing Guangrong's patch because it fixes a
>> crash and the leak existed before, but without next_list we can indeed
>> visit a "virtual" list and fix the leak. It can be done during the -rc
>> period.
>
> So you want to build uint16List list and save it as a "virtual" list in
> host_memory_backend_get_host_nodes(), then its caller can directly fetch
> this 'virtual' list from the visit?
With a virtual list visit, you don't even need a uint16List object.
Merely call visit_start_list(NULL) to start the list with no matching
uint16List, then visit_type_int16() for each list element (note no
visit_next_list() calls), then visit_end_list().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev'
@ 2016-07-15 17:16 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2016-07-15 17:16 UTC (permalink / raw)
To: Xiao Guangrong, Paolo Bonzini, Markus Armbruster
Cc: ehabkost, kvm, mst, gleb, mtosatti, qemu-devel, stefanha, imammedo, rth
[-- Attachment #1: Type: text/plain, Size: 994 bytes --]
On 07/15/2016 12:56 AM, Xiao Guangrong wrote:
>>> Note that you don't have to call visit_next_list() in a virtual visit.
>>> For an example, see prop_get_fdt(). Good enough already?
>>
>> Yes, definitely! I'm queueing Guangrong's patch because it fixes a
>> crash and the leak existed before, but without next_list we can indeed
>> visit a "virtual" list and fix the leak. It can be done during the -rc
>> period.
>
> So you want to build uint16List list and save it as a "virtual" list in
> host_memory_backend_get_host_nodes(), then its caller can directly fetch
> this 'virtual' list from the visit?
With a virtual list visit, you don't even need a uint16List object.
Merely call visit_start_list(NULL) to start the list with no matching
uint16List, then visit_type_int16() for each list element (note no
visit_next_list() calls), then visit_end_list().
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-07-15 17:16 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-13 4:18 [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' Xiao Guangrong
2016-07-13 4:18 ` [Qemu-devel] " Xiao Guangrong
2016-07-13 4:18 ` [PATCH 2/2] hostmem: detect host backend memory is being used properly Xiao Guangrong
2016-07-13 4:18 ` [Qemu-devel] " Xiao Guangrong
2016-07-13 7:30 ` Igor Mammedov
2016-07-13 7:30 ` [Qemu-devel] " Igor Mammedov
2016-07-13 10:49 ` Paolo Bonzini
2016-07-13 10:49 ` [Qemu-devel] " Paolo Bonzini
2016-07-13 10:45 ` [PATCH 1/2] hostmem: fix QEMU crash by 'info memdev' Paolo Bonzini
2016-07-13 10:45 ` [Qemu-devel] " Paolo Bonzini
2016-07-13 11:29 ` Markus Armbruster
2016-07-13 11:29 ` [Qemu-devel] " Markus Armbruster
2016-07-13 11:37 ` Paolo Bonzini
2016-07-13 11:37 ` [Qemu-devel] " Paolo Bonzini
2016-07-15 6:56 ` Xiao Guangrong
2016-07-15 17:16 ` Eric Blake
2016-07-15 17:16 ` Eric Blake
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.