On Wed, 13 Jul 2016 12:18:06 +0800 Xiao Guangrong <guangrong.xiao@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
