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

[David Hildenbrand]

On 27.03.20 17:48, Igor Mammedov wrote:
> On Fri, 27 Mar 2020 16:29:30 +0100
> David Hildenbrand <address@hidden> 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.
>>
>> 2. Migrating upstream QEMU to upstream QEMU (e.g., with "-m 1235M"), as
>>    we receive storage attributes for memory we don't expect (as we fixed up
>>    ram_size and maxram_size).
>>
>> Fixes: 3a12fc61af5c ("390x/s390-virtio-ccw: use memdev for RAM")
>> Reported-by: Lukáš Doktor <address@hidden>
>> Cc: Igor Mammedov <address@hidden>
>> Cc: Dr. David Alan Gilbert <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/s390x/s390-skeys.c        |  4 +---
>>  hw/s390x/s390-stattrib-kvm.c |  7 ++-----
>>  hw/s390x/sclp.c              | 21 +++++++++++----------
>>  3 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/s390x/s390-skeys.c b/hw/s390x/s390-skeys.c
>> index 5da6e5292f..2545b1576b 100644
>> --- a/hw/s390x/s390-skeys.c
>> +++ b/hw/s390x/s390-skeys.c
>> @@ -11,7 +11,6 @@
>>  
>>  #include "qemu/osdep.h"
>>  #include "qemu/units.h"
>> -#include "hw/boards.h"
>>  #include "hw/s390x/storage-keys.h"
>>  #include "qapi/error.h"
>>  #include "qapi/qapi-commands-misc-target.h"
>> @@ -174,9 +173,8 @@ out:
>>  static void qemu_s390_skeys_init(Object *obj)
>>  {
>>      QEMUS390SKeysState *skeys = QEMU_S390_SKEYS(obj);
>> -    MachineState *machine = MACHINE(qdev_get_machine());
>>  
>> -    skeys->key_count = machine->maxram_size / TARGET_PAGE_SIZE;
>> +    skeys->key_count = ram_size / TARGET_PAGE_SIZE;
> 
> why are you dropping machine->foo all around and switching to global ram_size?
> (I'd rather do other way around)

Not sure what the latest best practice is. I can also simply convert to
machine->ram_size if that's the right thing to do.


-- 
Thanks,

David / dhildenb