Re: [PATCH v1] s390x: Reject unaligned RAM sizes

[David Hildenbrand <david@redhat.com>]

On 31.03.20 17:33, Igor Mammedov wrote:
> On Tue, 31 Mar 2020 13:17:38 +0200
> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
> 
>> On 27.03.20 23:13, Igor Mammedov wrote:
>>> On Fri, 27 Mar 2020 17:53:39 +0100
>>> David Hildenbrand <david@redhat.com> wrote:
>>>   
>>>> On 27.03.20 17:46, Igor Mammedov wrote:  
>>>>> On Fri, 27 Mar 2020 17:05:34 +0100
>>>>> Christian Borntraeger <borntraeger@de.ibm.com> wrote:
>>>>>     
>>>>>> On 27.03.20 17:01, David Hildenbrand wrote:    
>>>>>>> On 27.03.20 16:34, Christian Borntraeger wrote:      
>>>>>>>>
>>>>>>>>
>>>>>>>> On 27.03.20 16:29, David Hildenbrand wrote:      
>>>>>>>>> Historically, we fixed up the RAM size (rounded it down), to fit into
>>>>>>>>> storage increments. Since commit 3a12fc61af5c ("390x/s390-virtio-ccw: use
>>>>>>>>> memdev for RAM"), we no longer consider the fixed-up size when
>>>>>>>>> allcoating the RAM block - which will break migration.
>>>>>>>>>
>>>>>>>>> Let's simply drop that manual fixup code and let the user supply sane
>>>>>>>>> RAM sizes. This will bail out early when trying to migrate (and make
>>>>>>>>> an existing guest with e.g., 12345 MB non-migratable), but maybe we
>>>>>>>>> should have rejected such RAM sizes right from the beginning.
>>>>>>>>>
>>>>>>>>> As we no longer fixup maxram_size as well, make other users use ram_size
>>>>>>>>> instead. Keep using maxram_size when setting the maximum ram size in KVM,
>>>>>>>>> as that will come in handy in the future when supporting memory hotplug
>>>>>>>>> (in contrast, storage keys and storage attributes for hotplugged memory
>>>>>>>>>  will have to be migrated per RAM block in the future).
>>>>>>>>>
>>>>>>>>> This fixes (or rather rejects early):
>>>>>>>>>
>>>>>>>>> 1. Migrating older QEMU to upstream QEMU (e.g., with "-m 1235M"), as the
>>>>>>>>>    RAM block size changed.      
>>>>>>>>
>>>>>>>> Not sure I like this variant. Instead of breaking migration (that was 
>>>>>>>> accidentially done by Igors changes) we now reject migration from older
>>>>>>>> QEMUs to 5.0. This is not going to help those that still have such guests
>>>>>>>> running and want to migrate.       
>>>>>>>
>>>>>>> As Igor mentioned on another channel, you most probably can migrate an
>>>>>>> older guest by starting it on the target with a fixed-up size.
>>>>>>>
>>>>>>> E.g., migrate an old QEMU "-m 1235M" to a new QEMU "-m 1234M"      
>>>>>>
>>>>>> Yes, that should probably work.    
>>>>> I'm in process of testing it.  
>>>
>>> it works
>>>   
>>>>>     
>>>>>>> Not sure how many such weird-size VMs we actually do have in practice.      
>>>>>>
>>>>>> I am worried about some automated deployments where tooling has created
>>>>>> these sizes for dozens or hundreds of containers in VMS and so.    
>>>>
>>>> IIRC, e.g., Kata usually uses 2048MB. Not sure about others, but I'd be
>>>> surprised if it's not multiples of, say, 128MB.
>>>>  
>>>>> Yep, it's possible but then that tooling/configs should be fixed to work with
>>>>> new QEMU that validates user's input.
>>>>>     
>>>>
>>>> Yeah, and mention it in the cover letter, +eventually a "fixup" table
>>>> (e.g., old_size < X, has to be aligned to Y).
>>>>
>>>> One alternative is to have an early fixup hack in QEMU, that fixes up
>>>> the sizes as we did before (and eventually warns the user). Not sure if
>>>> we really want/need that.  
>>> That would require at least a callback at machine level, 
>>> also practice shows warnings are of no use.  
>>
>> I would strongly prefer to not break setups that used to work and do an early fixup
>> (a machine callback). I will have a look.
> 
> If it were breaking migration stream or guest ABI,
> I'd agree with you but it isn't.
> 
> So in this case, it's a bug that qemu wasn't checking
> size for alignment and making workaround to keep the bug
> around looks wrong to me.
> It is fixable on user side (one has to fix VM's config),
> so it should be fixed there and not in QEMU.
> And any automatic tooling that generates invalid size
> should be fixed as well. 
> (I think it's not QEMU's job to mask users errors and
> doing something that user not asked for)

Dave G mentioned, that e.g., via Cockpit it can be fairly easy to
produce weird RAM sizes (via a slider). So I guess this "issue" could be
more widespread than I initially thought.

I agree that it's a BUG that we didn't bail out but instead decided to
fix it up.

-- 
Thanks,

David / dhildenb