On Wed, Jul 08, 2015 at 11:46:48AM +0200, Igor Mammedov wrote:
> Although memory_region_del_subregion() removes MemoryRegion
> from current address space, it's possible that it's still
> in use/referenced until old address space view is destroyed.
> That doesn't allow to unmap it from HVA region at the time
> of memory_region_del_subregion().
> As a solution track HVA mapped MemoryRegions in a list and
> don't allow to map another MemoryRegion at the same address
> until respective MemoryRegion is destroyed, delaying unmapping
> from HVA range to the time MemoryRegion destructor is called.
>
> In memory hotplug terms it would mean that user should delete
> corresponding backend along with pc-dimm device:
> device_del dimm1
> object_del dimm1_backend_memdev
> after that dimm1_backend_memdev's MemoryRegion will be destroyed
> once all accesses to it are gone and old flatview is destroyed as
> well.
> As result it's possible that a following "device_add pc-dimm" at
> the same address may fail due to old mapping is still being present,
s/is still/still/
> hence add error argument to memory_region_add_subregion() API
> so it could report error and hotplug could be cancelled gracefully.
>
> Signed-off-by: Igor Mammedov <imamm...@redhat.com>
The commit log seems a bit confusing.
API was added in previous patch, and this one actually
uses it.
> ---
> hw/mem/pc-dimm.c | 6 +++++-
> include/exec/memory.h | 2 ++
> memory.c | 55
> +++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/hw/mem/pc-dimm.c b/hw/mem/pc-dimm.c
> index 8cc9118..d17c22f 100644
> --- a/hw/mem/pc-dimm.c
> +++ b/hw/mem/pc-dimm.c
> @@ -95,7 +95,11 @@ void pc_dimm_memory_plug(DeviceState *dev,
> MemoryHotplugState *hpms,
> goto out;
> }
>
> - memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr,
> &error_abort);
> + memory_region_add_subregion(&hpms->mr, addr - hpms->base, mr,
> &local_err);
> + if (local_err) {
> + goto out;
> + }
> +
> vmstate_register_ram(mr, dev);
> numa_set_mem_node_id(addr, memory_region_size(mr), dimm->node);
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index ce0320a..d9c53f9 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -174,6 +174,7 @@ struct MemoryRegion {
> bool romd_mode;
> bool ram;
> void *rsvd_hva;
> + bool hva_mapped;
> bool skip_dump;
> bool readonly; /* For RAM regions */
> bool enabled;
> @@ -188,6 +189,7 @@ struct MemoryRegion {
> QTAILQ_HEAD(subregions, MemoryRegion) subregions;
> QTAILQ_ENTRY(MemoryRegion) subregions_link;
> QTAILQ_HEAD(coalesced_ranges, CoalescedMemoryRange) coalesced;
> + QTAILQ_ENTRY(MemoryRegion) hva_link;
> const char *name;
> uint8_t dirty_log_mask;
> unsigned ioeventfd_nb;
> diff --git a/memory.c b/memory.c
> index 360a5b8..e3c1b36 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -34,6 +34,7 @@ static unsigned memory_region_transaction_depth;
> static bool memory_region_update_pending;
> static bool ioeventfd_update_pending;
> static bool global_dirty_log = false;
> +static QemuMutex hva_lock;
>
> static QTAILQ_HEAD(memory_listeners, MemoryListener) memory_listeners
> = QTAILQ_HEAD_INITIALIZER(memory_listeners);
> @@ -1761,6 +1762,24 @@ done:
> memory_region_transaction_commit();
> }
>
> +static QTAILQ_HEAD(, MemoryRegion) hva_mapped_head =
> + QTAILQ_HEAD_INITIALIZER(hva_mapped_head);
> +
> +static void memory_region_destructor_hva_ram(MemoryRegion *mr)
> +{
> + MemoryRegion *h, *tmp;
> +
> + qemu_mutex_lock(&hva_lock);
> + qemu_ram_unmap_hva(mr->ram_addr);
> + memory_region_destructor_ram(mr);
> + QTAILQ_FOREACH_SAFE(h, &hva_mapped_head, hva_link, tmp) {
> + if (mr == h) {
> + QTAILQ_REMOVE(&hva_mapped_head, h, hva_link);
> + }
> + }
> + qemu_mutex_unlock(&hva_lock);
> +}
> +
> static void memory_region_add_subregion_common(MemoryRegion *mr,
> hwaddr offset,
> MemoryRegion *subregion,
> @@ -1772,8 +1791,43 @@ static void
> memory_region_add_subregion_common(MemoryRegion *mr,
>
> if (subregion->ram) {
> if (mr->rsvd_hva) {
> + MemoryRegion *h, *tmp;
> + Int128 e, oe;
> +
> + assert(!subregion->hva_mapped);
> + qemu_mutex_lock(&hva_lock);
> +
> + QTAILQ_FOREACH_SAFE(h, &hva_mapped_head, hva_link, tmp) {
> + if (subregion == h) {
> + error_setg(errp, "HVA mapped memory region '%s' is not "
> + "reusable, use a new one instead",
> + subregion->name);
> + qemu_mutex_unlock(&hva_lock);
> + return;
> + }
> +
> + e = int128_add(int128_make64(h->addr),
> + int128_make64(memory_region_size(h)));
> + oe = int128_add(int128_make64(offset),
> +
> int128_make64(memory_region_size(subregion)));
> + if (offset >= h->addr && int128_le(oe, e)) {
> + MemoryRegionSection rsvd_hva;
> + rsvd_hva = memory_region_find_hva_range(mr);
> + error_setg(errp, "memory at 0x%" PRIx64 " is still in
> use"
> + "by HVA mapped region: %s",
> + rsvd_hva.offset_within_address_space + offset,
> + h->name);
> + qemu_mutex_unlock(&hva_lock);
> + return;
> + }
> + }
> +
> + QTAILQ_INSERT_TAIL(&hva_mapped_head, subregion, hva_link);
> + subregion->destructor = memory_region_destructor_hva_ram;
> + subregion->hva_mapped = true;
> qemu_ram_remap_hva(subregion->ram_addr,
> memory_region_get_ram_ptr(mr) + subregion->addr);
> + qemu_mutex_unlock(&hva_lock);
> }
> }
>
> @@ -2288,6 +2342,7 @@ static const TypeInfo memory_region_info = {
> static void memory_register_types(void)
> {
> type_register_static(&memory_region_info);
> + qemu_mutex_init(&hva_lock);
> }
>
> type_init(memory_register_types)
> --
> 1.8.3.1
