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/13/2014 09:43 AM, Alexander Graf wrote:
> 
> On 13.05.14 15:16, Matthew Rosato wrote:
>> 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.
> 
> Wouldn't it be more confusing if the guest configuration suddenly
> changes when you add standby memory?
> 

In fairness, you are already changing the guest memory layout with the
introduction of standby memory in the first place, so what's a little
more change? :)

But, yes, I hear you -- The value in keeping the environment uniform
across configurations outweighs the benefits of allowing odd boundaries
in some cases (probably only test cases anyway).  I can live with that.
 Thanks for the feedback.

Matt