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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 27 Mar 2020 17:51:23 +0100
David Hildenbrand <david@redhat.com> wrote:

> On 27.03.20 17:48, Igor Mammedov wrote:
> > On Fri, 27 Mar 2020 16:29:30 +0100
> > David Hildenbrand <david@redhat.com> 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 <ldoktor@redhat.com>
> >> Cc: Igor Mammedov <imammedo@redhat.com>
> >> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  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.
My understanding of it was not to use globals if possible.
(I planned on removing global ram_size an leave only machine->ram_size
but that a bit tricky since things tend to explode once a global touched,
so it needs some more thought/patience)