From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44921) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkDJt-0001ag-Ig for qemu-devel@nongnu.org; Tue, 13 May 2014 10:04:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkDJj-0008Sg-7J for qemu-devel@nongnu.org; Tue, 13 May 2014 10:04:37 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:44182) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkDJj-0008SP-3S for qemu-devel@nongnu.org; Tue, 13 May 2014 10:04:27 -0400 Received: from /spool/local by e7.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 13 May 2014 10:04:26 -0400 Received: from b01cxnp22033.gho.pok.ibm.com (b01cxnp22033.gho.pok.ibm.com [9.57.198.23]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id B8B6438C803D for ; Tue, 13 May 2014 10:04:24 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22033.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s4DE4O6P917950 for ; Tue, 13 May 2014 14:04:24 GMT Received: from d01av03.pok.ibm.com (localhost [127.0.0.1]) by d01av03.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s4DE4OWU026934 for ; Tue, 13 May 2014 10:04:24 -0400 Message-ID: <53722667.80101@linux.vnet.ibm.com> Date: Tue, 13 May 2014 10:04:23 -0400 From: Matthew Rosato MIME-Version: 1.0 References: <1399485959-15579-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1399485959-15579-4-git-send-email-mjrosato@linux.vnet.ibm.com> <53707BB3.1060904@de.ibm.com> <53721B27.1090507@linux.vnet.ibm.com> <5372219E.6070503@suse.de> In-Reply-To: <5372219E.6070503@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 3/4] virtio-ccw: Include standby memory when calculating storage increment List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , Christian Borntraeger , qemu-devel@nongnu.org Cc: Cornelia Huck , imammedo@redhat.com, rth@twiddle.net, aliguori@amazon.com, pbonzini@redhat.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 >>>> --- >>>> 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