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

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jul 08, 2015 at 04:43:55PM +0200, Igor Mammedov wrote:
> On Wed, 8 Jul 2015 12:58:55 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > 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/
> fixed
> 
> > 
> > > 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 <imammedo@redhat.com>
> > 
> > The commit log seems a bit confusing.
> > API was added in previous patch, and this one actually
> > uses it.
> previous patch added qemu_ram_* API utilities at exec.c
> but this patch adds memory_region_* API changes that would allow
> to delete HVA mapped region safely.

I see two changes in memory.h, all private fields.

> Is there any suggestion how to make commit message less confusing.


Just make it match what the patch does.

> > > 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;