Re: [Qemu-devel] [PATCH v3 3/4] virtio-ccw: Include standby memory when calculating storage increment

[Christian Borntraeger <borntraeger@de.ibm.com>]

On 07/05/14 20:05, Matthew Rosato wrote:
> When determining the memory increment size, use the maxmem size if
> it was specified.
> 
> Signed-off-by: Matthew Rosato <mjrosato@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-virtio-ccw.c |   44 ++++++++++++++++++++++++++++++++++++--------
>  target-s390x/cpu.h         |    3 +++
>  2 files changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 0d4f6ae..a8be0f7 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -17,6 +17,7 @@
>  #include "ioinst.h"
>  #include "css.h"
>  #include "virtio-ccw.h"
> +#include "qemu/config-file.h"
> 
>  void io_subsystem_reset(void)
>  {
> @@ -84,17 +85,33 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      ram_addr_t my_ram_size = args->ram_size;
>      MemoryRegion *sysmem = get_system_memory();
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
> -    int shift = 0;
> +    sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev();
>      uint8_t *storage_keys;
>      int ret;
>      VirtualCssBus *css_bus;
> -
> -    /* s390x ram size detection needs a 16bit multiplier + an increment. So
> -       guests > 64GB can be specified in 2MB steps etc. */
> -    while ((my_ram_size >> (20 + shift)) > 65535) {
> -        shift++;
> +    QemuOpts *opts = qemu_opts_find(qemu_find_opts("memory"), NULL);
> +    ram_addr_t pad_size = 0;
> +    ram_addr_t maxmem = qemu_opt_get_size(opts, "maxmem", 0);
> +    ram_addr_t standby_mem_size = maxmem - my_ram_size;
> +
> +    /* The storage increment size is a multiple of 1M and is a power of 2.
> +     * The number of storage increments must be MAX_STORAGE_INCREMENTS or fewer.
> +     * The variable 'mhd->increment_size' is an exponent of 2 that can be
> +     * used to calculate the size (in bytes) of an increment. */
> +    mhd->increment_size = 20;
> +    while ((my_ram_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) {
> +        mhd->increment_size++;
> +    }
> +    while ((standby_mem_size >> mhd->increment_size) > MAX_STORAGE_INCREMENTS) {
> +        mhd->increment_size++;
>      }

Looking back into the mail thread, Alex requested to make the code for standy/non-standby identical.
Now: The limit of 1020 (MAX_STORAGE_INCREMENTS) is only given if standby memory exists. Without standby memory, we could still used 64k as a divider.(zVM also does only impose this limit with standby memory).
What does that mean: With this patch the memory size granularity gets bigger. e.g. a guest can have
1019MB, 1020MB, 1022MB, 1024MB and so on (1021MB is no longer possible, but it was before).

We could now special case this or just leave it as is in this patch to make the code simpler. I think this is not a big problem, but we should add this to the comment and maybe also to the help text of the command line option (e.g.

diff --git a/qemu-options.hx b/qemu-options.hx
index 5edffa6..8c71283 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -217,7 +217,8 @@ DEF("m", HAS_ARG, QEMU_OPTION_m,
     "-m [size=]megs\n"
     "                configure guest RAM\n"
     "                size: initial amount of guest memory (default: "
-    stringify(DEFAULT_RAM_SIZE) "MiB)\n",
+    stringify(DEFAULT_RAM_SIZE) "MiB)\n"
+    "NOTE: Some architectures might enforce a specific granularity\n",
     QEMU_ARCH_ALL)
 STEXI
 @item -m [size=]@var{megs}

@Alex,Conny: Any preference?

> -    my_ram_size = my_ram_size >> (20 + shift) << (20 + shift);
> +
> +    /* The core and standby memory areas need to be aligned with
> +     * the increment size */
> +    standby_mem_size = standby_mem_size >> mhd->increment_size
> +                                        << mhd->increment_size;
> +    my_ram_size = my_ram_size >> mhd->increment_size
> +                              << mhd->increment_size;
> 
>      /* let's propagate the changed ram size into the global variable. */
>      ram_size = my_ram_size;
> @@ -109,11 +126,22 @@ static void ccw_init(QEMUMachineInitArgs *args)
>      /* register hypercalls */
>      virtio_ccw_register_hcalls();
> 
> -    /* allocate RAM */
> +    /* allocate RAM for core */
>      memory_region_init_ram(ram, NULL, "s390.ram", my_ram_size);memory_region_init_ram
>      vmstate_register_ram_global(ram);
>      memory_region_add_subregion(sysmem, 0, ram);
> 
> +    /* If the size of ram is not on a MEM_SECTION_SIZE boundary,
> +       calculate the pad size necessary to force this boundary. */
> +    if (standby_mem_size) {
> +        if (my_ram_size % MEM_SECTION_SIZE) {
> +            pad_size = MEM_SECTION_SIZE - my_ram_size % MEM_SECTION_SIZE;
> +        }
> +        my_ram_size += standby_mem_size + pad_size;
> +        mhd->pad_size = pad_size;
> +        mhd->standby_mem_size = standby_mem_size;
> +    }
> +
>      /* allocate storage keys */
>      storage_keys = g_malloc0(my_ram_size / TARGET_PAGE_SIZE);
> 
> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h
> index aad277a..193eac3 100644
> --- a/target-s390x/cpu.h
> +++ b/target-s390x/cpu.h
> @@ -1047,6 +1047,9 @@ static inline void cpu_inject_crw_mchk(S390CPU *cpu)
>      cpu_interrupt(CPU(cpu), CPU_INTERRUPT_HARD);
>  }
> 
> +/* from s390-virtio-ccw */
> +#define MEM_SECTION_SIZE             0x10000000UL
> +
>  /* fpu_helper.c */
>  uint32_t set_cc_nz_f32(float32 v);
>  uint32_t set_cc_nz_f64(float64 v);
>