Re: [Qemu-devel] [RFC 1/2] memory: introduce MemoryRegion container with reserved HVA range

[Igor Mammedov]

On Wed, 03 Jun 2015 14:45:32 +0200
Paolo Bonzini <address@hidden> wrote:

> 
> 
> On 03/06/2015 14:22, Igor Mammedov wrote:
> > Patch adds memory_region_init_rsvd_hva() and
> > memory_region_find_rsvd_hva() API to allocate and lookup
> > reserved HVA MemoryRegion.
> > 
> > MemoryRegion with reserved HVA range will be used for
> > providing linear 1:1 HVA->GVA mapping for RAM MemoryRegion-s
> > that is added as subregions inside it.
> > 
> > It will be used for memory hotplug and vhost integration
> > reducing all hotplugged MemoryRegions down to one
> > memory range descriptor, which allows to overcome
> > vhost's limitation on number of allowed memory ranges.
> > 
> > Signed-off-by: Igor Mammedov <address@hidden>
> 
> Very nice patches.  Really.
> 
> About the naming, I would use memory_region_init_hva_range and
> memory_region_find_hva_range.
> 
> Also, it has to be Linux-only (due to MAP_NORESERVE and mremap).
> 
> Finally, instead of overloading ram_addr, just add "void *rsvd_hva" and
> make it NULL for other kinds of regions.  This matches what we do for
> e.g. iommu_ops.
sure , I'll fix it up.

> 
> One question:
> 
> >  void *memory_region_get_ram_ptr(MemoryRegion *mr)
> >  {
> > +    if (mr->rsvd_hva) {
> > +        return (void *)mr->ram_addr;
> > +    }
> > +
> 
> I see how this is "nice" and I don't object to it, but is it also necessary?
I've tried to reuse memory_region_get_ram_ptr(), what is other alternative?
something like 
 memory_region_get_hva_range_ptr()

> 
> Paolo
> 
> > ---
> >  exec.c                    | 13 +++++++++++
> >  include/exec/cpu-common.h |  1 +
> >  include/exec/memory.h     | 42 ++++++++++++++++++++++++++++++++++--
> >  memory.c                  | 55 
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 109 insertions(+), 2 deletions(-)
> > 
> > diff --git a/exec.c b/exec.c
> > index e19ab22..e01cd8f 100644
> > --- a/exec.c
> > +++ b/exec.c
> > @@ -1325,6 +1325,19 @@ static int memory_try_enable_merging(void *addr, 
> > size_t len)
> >      return qemu_madvise(addr, len, QEMU_MADV_MERGEABLE);
> >  }
> >  
> > +void qemu_ram_remap_hva(ram_addr_t addr, void *new_hva)
> > +{
> > +    RAMBlock *block = find_ram_block(addr);
> > +
> > +    assert(block);
> > +    block->host = mremap(block->host, block->used_length,
> > +                      block->used_length,
> > +                      MREMAP_MAYMOVE | MREMAP_FIXED, new_hva);
> > +    memory_try_enable_merging(block->host, block->used_length);
> > +    qemu_ram_setup_dump(block->host, block->used_length);
> > +}
> > +
> > +
> >  /* Only legal before guest might have detected the memory size: e.g. on
> >   * incoming migration, or right after reset.
> >   *
> > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> > index 43428bd..a9f73d6 100644
> > --- a/include/exec/cpu-common.h
> > +++ b/include/exec/cpu-common.h
> > @@ -60,6 +60,7 @@ typedef void CPUWriteMemoryFunc(void *opaque, hwaddr 
> > addr, uint32_t value);
> >  typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr);
> >  
> >  void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
> > +void qemu_ram_remap_hva(ram_addr_t addr, void *new_hva);
> >  /* This should not be used by devices.  */
> >  MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr);
> >  void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState 
> > *dev);
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index b61c84f..9b0858e 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -174,6 +174,7 @@ struct MemoryRegion {
> >      bool terminates;
> >      bool romd_mode;
> >      bool ram;
> > +    bool rsvd_hva;
> >      bool skip_dump;
> >      bool readonly; /* For RAM regions */
> >      bool enabled;
> > @@ -281,6 +282,23 @@ void memory_region_init(MemoryRegion *mr,
> >                          struct Object *owner,
> >                          const char *name,
> >                          uint64_t size);
> > +/**
> > + * memory_region_init_rsvd_hva: Initialize a reserved HVA memory region
> > + *
> > + * The container for RAM memory regions.
> > + * When adding subregion with memory_region_add_subregion(), subregion's
> > + * backing host memory will be remapped to inside the reserved by this
> > + * region HVA.
> > + *
> > + * @mr: the #MemoryRegion to be initialized
> > + * @owner: the object that tracks the region's reference count
> > + * @name: used for debugging; not visible to the user or ABI
> > + * @size: size of the region; any subregions beyond this size will be 
> > clipped
> > + */
> > +void memory_region_init_rsvd_hva(MemoryRegion *mr,
> > +                                 struct Object *owner,
> > +                                 const char *name,
> > +                                 uint64_t size);
> >  
> >  /**
> >   * memory_region_ref: Add 1 to a memory region's reference count
> > @@ -620,8 +638,8 @@ int memory_region_get_fd(MemoryRegion *mr);
> >   * memory_region_get_ram_ptr: Get a pointer into a RAM memory region.
> >   *
> >   * Returns a host pointer to a RAM memory region (created with
> > - * memory_region_init_ram() or memory_region_init_ram_ptr()).  Use with
> > - * care.
> > + * memory_region_init_ram() or memory_region_init_ram_ptr()) or
> > + * memory_region_init_rsvd_hva().  Use with care.
> >   *
> >   * @mr: the memory region being queried.
> >   */
> > @@ -1014,6 +1032,26 @@ MemoryRegionSection memory_region_find(MemoryRegion 
> > *mr,
> >                                         hwaddr addr, uint64_t size);
> >  
> >  /**
> > + * memory_region_find_rsvd_hva: finds a parent MemoryRegion with
> > + * reserved HVA and translates it into a #MemoryRegionSection.
> > + *
> > + * Locates the first parent #MemoryRegion of @mr that is
> > + * of reserved HVA type.
> > + *
> > + * Returns a #MemoryRegionSection that describes a reserved HVA
> > + * memory region.
> > + *    address@hidden is offset of found
> > + *      (in the address@hidden field) memory region relative to the address
> > + *      space that contains it.
> > + *    address@hidden is offset of @mr relative
> > + *      to the returned region (in the address@hidden field).
> > + *    address@hidden is size of found memory region
> > + *
> > + * @mr: a MemoryRegion whose HVA pernt is looked up
> > + */
> > +MemoryRegionSection memory_region_find_rsvd_hva(MemoryRegion *mr);
> > +
> > +/**
> >   * address_space_sync_dirty_bitmap: synchronize the dirty log for all 
> > memory
> >   *
> >   * Synchronizes the dirty page log for an entire address space.
> > diff --git a/memory.c b/memory.c
> > index 03c536b..70564dc 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -25,6 +25,7 @@
> >  #include "exec/memory-internal.h"
> >  #include "exec/ram_addr.h"
> >  #include "sysemu/sysemu.h"
> > +#include <sys/mman.h>
> >  
> >  //#define DEBUG_UNASSIGNED
> >  
> > @@ -939,6 +940,17 @@ void memory_region_init(MemoryRegion *mr,
> >      }
> >  }
> >  
> > +void memory_region_init_rsvd_hva(MemoryRegion *mr,
> > +                                 Object *owner,
> > +                                 const char *name,
> > +                                 uint64_t size)
> > +{
> > +    memory_region_init(mr, owner, name, size);
> > +    mr->rsvd_hva = true;
> > +    mr->ram_addr = (ram_addr_t)mmap(0, memory_region_size(mr),
> > +    PROT_NONE, MAP_NORESERVE | MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> > +}
> > +
> >  static void memory_region_get_addr(Object *obj, Visitor *v, void *opaque,
> >                                     const char *name, Error **errp)
> >  {
> > @@ -1514,6 +1526,10 @@ int memory_region_get_fd(MemoryRegion *mr)
> >  
> >  void *memory_region_get_ram_ptr(MemoryRegion *mr)
> >  {
> > +    if (mr->rsvd_hva) {
> > +        return (void *)mr->ram_addr;
> > +    }
> > +
> >      if (mr->alias) {
> >          return memory_region_get_ram_ptr(mr->alias) + mr->alias_offset;
> >      }
> > @@ -1742,6 +1758,17 @@ static void 
> > memory_region_add_subregion_common(MemoryRegion *mr,
> >      assert(!subregion->container);
> >      subregion->container = mr;
> >      subregion->addr = offset;
> > +
> > +    if (mr->ram) {
> > +   MemoryRegionSection rsvd_hva = memory_region_find_rsvd_hva(mr);
> > +
> > +        if (rsvd_hva.mr) {
> > +            qemu_ram_remap_hva(mr->ram_addr,
> > +                memory_region_get_ram_ptr(rsvd_hva.mr) +
> > +                    rsvd_hva.offset_within_region);
> > +        }
> > +    }
> > +
> >      memory_region_update_container_subregions(subregion);
> >  }
> >  
> > @@ -1884,6 +1911,34 @@ bool memory_region_is_mapped(MemoryRegion *mr)
> >      return mr->container ? true : false;
> >  }
> >  
> > +MemoryRegionSection memory_region_find_rsvd_hva(MemoryRegion *mr)
> > +{
> > +    MemoryRegionSection ret = { .mr = NULL };
> > +    MemoryRegion *hva_container = NULL;
> > +    hwaddr addr = mr->addr;
> > +    MemoryRegion *root;
> > +
> > +    addr += mr->addr;
> > +    for (root = mr; root->container; ) {
> > +        if (!hva_container && root->rsvd_hva) {
> > +            hva_container = root;
> > +            ret.offset_within_region = addr;
> > +        }
> > +        root = root->container;
> > +        addr += root->addr;
> > +    }
> > +
> > +    ret.address_space = memory_region_to_address_space(root);
> > +    if (!ret.address_space || !hva_container) {
> > +        return ret;
> > +    }
> > +
> > +    ret.mr = hva_container;
> > +    ret.offset_within_address_space = addr;
> > +    ret.size = int128_make64(memory_region_size(ret.mr));
> > +    return ret;
> > +}
> > +
> >  MemoryRegionSection memory_region_find(MemoryRegion *mr,
> >                                         hwaddr addr, uint64_t size)
> >  {
> >