Re: [Qemu-devel] [PATCH v1 5/5] s390: do not call memory_region_allocate_system_memory() multiple times

[David Hildenbrand]

On 18.04.19 16:56, Igor Mammedov wrote:
> On Thu, 18 Apr 2019 14:06:25 +0200
> David Hildenbrand <address@hidden> wrote:
> 
>> On 18.04.19 14:01, Igor Mammedov wrote:
>>> On Thu, 18 Apr 2019 13:24:43 +0200
>>> David Hildenbrand <address@hidden> wrote:
>>>   
>>>> On 18.04.19 11:38, Igor Mammedov wrote:  
>>>>> On Tue, 16 Apr 2019 13:09:08 +0200
>>>>> Christian Borntraeger <address@hidden> wrote:
>>>>>     
>>>>>> This fails with more than 8TB, e.g.  "-m 9T "
>>>>>>
>>>>>> [pid 231065] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0, 
>>>>>> guest_phys_addr=0, memory_size=0, userspace_addr=0x3ffc8500000}) = 0
>>>>>> [pid 231065] ioctl(10, KVM_SET_USER_MEMORY_REGION, {slot=0, flags=0, 
>>>>>> guest_phys_addr=0, memory_size=9895604649984, 
>>>>>> userspace_addr=0x3ffc8500000}) = -1 EINVAL (Invalid argument)
>>>>>>
>>>>>> seems that the 2nd memslot gets the full size (and not 9TB-size of first 
>>>>>> slot).    
>>>>>
>>>>> it turns out MemoryRegions is rendered correctly in to 2 parts (one per 
>>>>> alias),
>>>>> but follow up flatview_simplify() collapses adjacent ranges back
>>>>> into big one.    
>>>>
>>>> That sounds dangerous. Imagine doing that at runtime (e.g. hotplugging a
>>>> DIMM), the kvm memory slot would temporarily be deleted to insert the
>>>> new, bigger one. Guest would crash. This could happen if backing memory
>>>> of two DIMMs would by pure luck be allocated side by side in user space.
>>>>  
>>>
>>> not sure I fully get your concerns, but if you look at can_merge()
>>> it ensures that ranges belong to the same MemoryRegion.
>>>
>>> It's hard for me to say if flatview_simplify() works as designed,
>>> MemoryRegion code is quite complicated so I'd deffer to Paolo's
>>> opinion.
>>>   
>>
>> What I had in mind:
>>
>> We have the Memory Region for memory devices (m->device_memory).
>>
>> Assume The first DIMM is created, allocating memory in the user space
>> process:
>>
>> [0x100000000 .. 0x20000000]. It is placed at offset 0 in m->device_memory.
>>
>> Guests starts to run, a second DIMM is hotplugged. Memory in user space
>> process is allocated (by pure luck) at:
>>
>> [0x200000000 .. 0x30000000]. It is placed at offset 0x100000000 in
>> m->device_memory.
>>
>> Without looking at the code, I could imagine that both might be merged
>> into a single memory slot. That is my concern. Maybe it is not valid.
> it's not. As far as I see ranges are merged only if they belong to
> the same 'mr'. So to dimms will result in 2 memory sections -> 2 KVMSlots.

Okay, so a shared "parent memory region" is not enough to result in a
merge, only aliases.

-- 

Thanks,

David / dhildenb