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 <mjros...@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
