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

[Matthew Rosato <mjrosato@linux.vnet.ibm.com>]

On 05/12/2014 03:43 AM, Christian Borntraeger wrote:
> 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).

Hmm, this is a good point.  I didn't think about it when I made the
change per Alex.  If nobody has a strong opinion here, I think I'd
rather go back to special casing this in the next version, to keep the
'normal case' (without standby memory) more robust.

> 
> 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?
> 

Sure, I'll add the doc hit & some extra commentary in the next version.

>> -    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);
>>
> 
> 
> 
>