Re: [Qemu-devel] [PATCH v4 01/14] memory-device: drop assert related to align and start of address space

[David Hildenbrand <david@redhat.com>]

On 30.05.2018 14:57, Igor Mammedov wrote:
> On Tue, 29 May 2018 18:02:06 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> On 29.05.2018 15:27, Igor Mammedov wrote:
>>> On Thu, 17 May 2018 10:15:14 +0200
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> The start of the address space does not have to be aligned for the
>>>> search. Handle this case explicitly when starting the search for a new
>>>> address.  
>>> That's true,
>>> but commit message doesn't explain why address_space_start
>>> should be allowed to be non aligned.
>>>
>>> At least with this assert we would notice early that
>>> board allocating misaligned address space.
>>> I'd keep the assert unless there is a good reason to drop it.  
>>
>> That reason might be that I can easily crash QEMU
>>
>>  ./x86_64-softmmu/qemu-system-x86_64 -m 256M,maxmem=20G,slots=2 -object
>> memory-backend-file,id=mem0,size=8192M,mem-path=/dev/zero,align=8192M
>> -device pc-dimm,id=dimm1,memdev=mem0
>>
>> ERROR:hw/mem/memory-device.c:146:memory_device_get_free_addr: assertion
>> failed: (QEMU_ALIGN_UP(address_space_start, align) == address_space_start)
> it looks like a different issue.
> As I remember user visible 'align' property was added as duct tape since
> we can't figure out alignment for DAX device no the host,
> so it was left upto upper layers to magically figure that out.
> 
> However we probably shouldn't allow arbitrary or bigger aligment
> than max page size supported by target machine/cpu
> (i.e. currently hardcoded address_space_start alignment),
> as it creates unnecessary fragmentation and not counted in size
> of hotplug region (for x86 we count in additional 1Gb per memory device).
> 
> How about turning that assert into error check that
> inhibits plugging in devices with alignment values
> larger than address_space_start alignment?


Let me explain a little bit why I don't like such restrictions (for
which I don't see a need yet):

virtio-mem devices will later have a certain block size (e.g. 4MB). I
want to give devices during resource assignment the possibility to
specify their alignment requirements.

For said virtio-mem device, this would e.g. be 4MB. (see patch 10 and 14
of how this call "get_align" comes into play), because the addresses of
the memory blocks are all aligned by e.g. 4MB. This is what is
guaranteed by the device specification.

E.g. for DIMMs we might want to allow to specify the section size (e.g.
128MB on x86), otherwise e.g. Linux is not able to add all memory. (but
we should not hardcode this, as this is a Linux specific requirement -
still it would be nice to specify)

So in general, a memory device might have some alignment that is to be
taken care of.

I don't understand right now why an upper limit on the alignment would
make sense at all. We can easily handle it during our search. And we
have to handle it either way during the search, if we plug some device
with strange sizes (e.g. 1MB DIMM).

Of course, we might end up fragmenting guest physical memory, but that
is purely a setup issue (choosing sizes of devices + main memory
properly). I don't see a reason to error out (and of course also not to
assert out :) ).

-- 

Thanks,

David / dhildenb