Re: [Qemu-devel] [RFC v3 8/8] memory: add support for deleting HVA mapped MemoryRegion

[Michael S. Tsirkin]

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 <address@hidden>

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