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

[David Hildenbrand]

On 27.03.20 19:16, Halil Pasic wrote:
> On Fri, 27 Mar 2020 17:46:20 +0100
> Igor Mammedov <address@hidden> 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.
>>
>>>> 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.
>> Yep, it's possible but then that tooling/configs should be fixed to work with
>> new QEMU that validates user's input.
>>
> 
> @David: I'm a little confused. Is this fix about adding user input
> validation, or is it about changing what valid inputs are?

We used to silently fixup the user input instead of bailing out. So it's
rather the latter. It was never the right choice to silently fixup
instead of just bailing out. We never should have done that.

> 
> I don't see this alignment requirement documented, so my guess is the
> latter. And then, I'm not sure I'm sold on this.

Well, we can add documentation with the next release. It's something to
end up in the release notes.

If somebody specified "-m 1235", he always received "1234 MB" and never
"1235 MB". You could also call that a BUG, which should have bailed out
right from the start (there wasn't even a warning).

-- 
Thanks,

David / dhildenb