Re: [Qemu-devel] [PATCH v2 2/4] util/oslib-win32: indicate alignment for qemu_anon_ram_alloc()

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 29 Jun 2018 17:39:10 +0200
David Hildenbrand <david@redhat.com> wrote:

> On 29.06.2018 16:49, Igor Mammedov wrote:
> > On Thu, 28 Jun 2018 14:14:15 +0200
> > David Hildenbrand <david@redhat.com> 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.
> >>
> >> 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.  
> > why not use QEMU_VMALLOC_ALIGN for consistency (on win => getpagesize())
> > instead of TARGET_PAGE_SIZE like linux allocator does?  
> 
> Sure we can do that, I wanted to match here exactly what has been
> written in the comment.
> 
> > 
> > Also looking at FIXME comment it notes that VirtualAlloc might have 64K
> > alignment (though I haven't found it in VirtualAlloc manual).
> > If that's true then we might need set *align to it to avoid auto-picked
> > address overlap with previous allocation (not really sure about it).  
> 
> "To determine the size of a page and the allocation granularity on the
> host computer, use the GetSystemInfo" [1]
> 
> "The size of the region, in bytes. If the lpAddress parameter is NULL,
> this value is rounded up to the next page boundary. " [1]
> 
> Historically, this seems to be 64k. But it will always be at least 4k
> (page size). So what we could do is query the actual allocation granularity:
> 
> int get_allocation_granularity(void) {
> 	SYSTEM_INFO system_info;
> 
> 	GetSystemInfo(&system_info);
> 	return system_info.dwAllocationGranularity
> }
> 
> 
> "dwAllocationGranularity: The granularity for the starting address at
> which virtual memory can be allocated. For more information, see
> VirtualAlloc." [2]
> 
> What do you think?
maybe do following:

    *align = MAX(get_allocation_granularity(), getpagesize())

> 
> > 
> >   
> >> For spapr in spapr_memory_plug(), an alignment of 0 would have been used  
> > note that align == 0 would lead to crash where QEMU_ALIGN_UP() is used,
> > so we don't care to keep it compatible (the same like in commit 92a37a04d)  
> 
> Good point!
> 
> [1]
> https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx
> [2]
> https://msdn.microsoft.com/en-us/library/windows/desktop/ms724958(v=vs.85).aspx
>