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

[David Hildenbrand <david@redhat.com>]

On 29.06.2018 17:56, Igor Mammedov wrote:
> 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())

That sounds like the best alternative when having to guess what is
actually going on behind the curtains.

-- 

Thanks,

David / dhildenb