From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCqgO-0000FS-GK for qemu-devel@nongnu.org; Wed, 08 Jul 2015 10:50:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCqgL-0007RU-32 for qemu-devel@nongnu.org; Wed, 08 Jul 2015 10:50:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49261) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCqgK-0007RA-EU for qemu-devel@nongnu.org; Wed, 08 Jul 2015 10:50:40 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 108E02B64BF for ; Wed, 8 Jul 2015 14:50:40 +0000 (UTC) Date: Wed, 8 Jul 2015 17:50:37 +0300 From: "Michael S. Tsirkin" Message-ID: <20150708174902-mutt-send-email-mst@redhat.com> References: <1436348808-223033-1-git-send-email-imammedo@redhat.com> <1436348808-223033-9-git-send-email-imammedo@redhat.com> <20150708125615-mutt-send-email-mst@redhat.com> <20150708164355.43b615c9@igors-macbook-pro.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150708164355.43b615c9@igors-macbook-pro.local> Subject: Re: [Qemu-devel] [RFC v3 8/8] memory: add support for deleting HVA mapped MemoryRegion List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, qemu-devel@nongnu.org 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" 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 > > > > 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;