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

[Igor Mammedov <imammedo@redhat.com>]

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)

>      skeys->keydata = g_malloc0(skeys->key_count);
>  }
>  
> diff --git a/hw/s390x/s390-stattrib-kvm.c b/hw/s390x/s390-stattrib-kvm.c
> index c7e1f35524..ae88fbc32e 100644
> --- a/hw/s390x/s390-stattrib-kvm.c
> +++ b/hw/s390x/s390-stattrib-kvm.c
> @@ -10,7 +10,6 @@
>   */
>  
>  #include "qemu/osdep.h"
> -#include "hw/boards.h"
>  #include "migration/qemu-file.h"
>  #include "hw/s390x/storage-attributes.h"
>  #include "qemu/error-report.h"
> @@ -84,8 +83,7 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
>                                          uint8_t *values)
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = ram_size / TARGET_PAGE_SIZE;
>  
>      if (start_gfn + count > max) {
>          error_report("Out of memory bounds when setting storage attributes");
> @@ -103,8 +101,7 @@ static int kvm_s390_stattrib_set_stattr(S390StAttribState *sa,
>  static void kvm_s390_stattrib_synchronize(S390StAttribState *sa)
>  {
>      KVMS390StAttribState *sas = KVM_S390_STATTRIB(sa);
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    unsigned long max = machine->maxram_size / TARGET_PAGE_SIZE;
> +    unsigned long max = ram_size / TARGET_PAGE_SIZE;
>      /* We do not need to reach the maximum buffer size allowed */
>      unsigned long cx, len = KVM_S390_SKEYS_MAX / 2;
>      int r;
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index af0bfbc2ec..6af471fb3f 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -326,8 +326,7 @@ out:
>  
>  static void sclp_memory_init(SCLPDevice *sclp)
>  {
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    ram_addr_t initial_mem = machine->ram_size;
> +    uint64_t initial_mem = ram_size;
>      int increment_size = 20;
>  
>      /* The storage increment size is a multiple of 1M and is a power of 2.
> @@ -339,15 +338,17 @@ static void sclp_memory_init(SCLPDevice *sclp)
>      }
>      sclp->increment_size = increment_size;
>  
> -    /* The core memory area needs to be aligned with the increment size.
> -     * In effect, this can cause the user-specified memory size to be rounded
> -     * down to align with the nearest increment boundary. */
> +    /*
> +     * The core memory area needs to be aligned to the increment size. In
> +     * case it's not aligned, bail out.
> +     */
>      initial_mem = initial_mem >> increment_size << increment_size;
> -
> -    machine->ram_size = initial_mem;
> -    machine->maxram_size = initial_mem;
> -    /* let's propagate the changed ram size into the global variable. */
> -    ram_size = initial_mem;
> +    if (initial_mem != ram_size) {
> +        error_report("RAM size not aligned to storage increments."
> +                     " Possible aligned RAM size: %" PRIu64 " MB",
> +                     initial_mem / MiB);
> +        exit(1);
> +    }
>  }
>  
>  static void sclp_init(Object *obj)