On 29.06.2018 16:49, Igor Mammedov wrote:
> On Thu, 28 Jun 2018 14:14:15 +0200
> David Hildenbrand <da...@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?
>
>
>> 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
--
Thanks,
David / dhildenb
