[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Christian Borntraeger
Subject: Re: [PATCH v1] s390x: Reject unaligned RAM sizes
Date: Tue, 31 Mar 2020 17:39:31 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.5.0

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.

QEMU did check for alignment and fixed it up. So it was an intentional decision 
(maybe a
bad one). Your change broke working setups and I really do not like that.
See my new patch for proper handling for machine types 5.0 and later + compat
handling for older machines. 

reply via email to

[Prev in Thread] Current Thread [Next in Thread]