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

[Christian Borntraeger]

On 31.03.20 17:39, David Hildenbrand wrote:
> On 31.03.20 17:33, Igor Mammedov wrote:
>> On Tue, 31 Mar 2020 13:17:38 +0200
>> Christian Borntraeger <address@hidden> wrote:
>>
>>> On 27.03.20 23:13, Igor Mammedov wrote:
>>>> On Fri, 27 Mar 2020 17:53:39 +0100
>>>> David Hildenbrand <address@hidden> wrote:
>>>>   
>>>>> On 27.03.20 17:46, Igor Mammedov wrote:  
>>>>>> On Fri, 27 Mar 2020 17:05:34 +0100
>>>>>> Christian Borntraeger <address@hidden> 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.

It was a decision back than. Likely a wrong one.
Now insisting on "lets breaking user setups because it violates my feeling
of perfection" is not the right answer though.