From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58978) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fZvsa-0004r6-79 for qemu-devel@nongnu.org; Mon, 02 Jul 2018 06:16:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fZvsW-0002zp-Vt for qemu-devel@nongnu.org; Mon, 02 Jul 2018 06:16:20 -0400 Date: Mon, 2 Jul 2018 12:16:11 +0200 From: Igor Mammedov Message-ID: <20180702121611.0a3f3ed4@redhat.com> In-Reply-To: <20180702093755.7384-3-david@redhat.com> References: <20180702093755.7384-1-david@redhat.com> <20180702093755.7384-3-david@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Eduardo Habkost , "Michael S . Tsirkin" , Marcel Apfelbaum , Paolo Bonzini , Richard Henderson , Xiao Guangrong , David Gibson , Alexander Graf , Stefan Weil On Mon, 2 Jul 2018 11:37:53 +0200 David Hildenbrand wrote: > Let's set the alignment just like for the posix variant. This will > implicitly set the alignment of the underlying memory region and > therefore make memory_region_get_alignment(mr) return something > 0 for > all memory backends applicable to PCDIMM/NVDIMM. > > The allocation granularity is ususally 64k, while the page size is 4k. > The documentation of VirtualAlloc is not really comprehensible in case > only MEM_COMMIT is specified without an address. We'll detect the actual > values and then go for the bigger one. The expection is, that it will > always be 64k aligned. (The assumption is that MEM_COMMIT does an > implicit MEM_RESERVE, so the address will always be aligned to the > allocation granularity. And the allocation granularity is always bigger > than the page size). > > This will allow us to drop special handling in pc.c for > memory_region_get_alignment(mr) == 0, as we can then assume that it is > always set (and AFAICS >= getpagesize()). > > For pc in pc_memory_plug(), under Windows TARGET_PAGE_SIZE == getpagesize(), > therefore alignment of DIMMs will not change, and therefore also not the > guest physical memory layout. > > For spapr in spapr_memory_plug(), an alignment of 0 would have been used > until now. As QEMU_ALIGN_UP will crash with the alignment being 0, this > never worked, so we don't have to care about compatibility handling. > > Reviewed-by: David Gibson > Signed-off-by: David Hildenbrand > --- > util/oslib-win32.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index bb5ad28bd3..25dd1595ad 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -67,15 +67,24 @@ void *qemu_memalign(size_t alignment, size_t size) > return qemu_oom_check(qemu_try_memalign(alignment, size)); > } > > +static int get_allocation_granularity(void) > +{ > + SYSTEM_INFO system_info; > + > + GetSystemInfo(&system_info); > + return system_info.dwAllocationGranularity; > +} > + > void *qemu_anon_ram_alloc(size_t size, uint64_t *align, bool shared) > { > void *ptr; > > - /* FIXME: this is not exactly optimal solution since VirtualAlloc > - has 64Kb granularity, but at least it guarantees us that the > - memory is page aligned. */ > ptr = VirtualAlloc(NULL, size, MEM_COMMIT, PAGE_READWRITE); > trace_qemu_anon_ram_alloc(size, ptr); > + > + if (ptr && align) { > + *align = MAX(get_allocation_granularity(), getpagesize()); > + } > return ptr; > } > Not really tested, but patch looks correct to me, hence: Reviewed-by: Igor Mammedov