From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54428) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkCZT-0004Bw-K2 for qemu-devel@nongnu.org; Tue, 13 May 2014 09:16:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WkCZK-0006ws-Hv for qemu-devel@nongnu.org; Tue, 13 May 2014 09:16:39 -0400 Received: from e9.ny.us.ibm.com ([32.97.182.139]:56955) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WkCZK-0006wZ-CL for qemu-devel@nongnu.org; Tue, 13 May 2014 09:16:30 -0400 Received: from /spool/local by e9.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 13 May 2014 09:16:28 -0400 Received: from b01cxnp22036.gho.pok.ibm.com (b01cxnp22036.gho.pok.ibm.com [9.57.198.26]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 164FEC90040 for ; Tue, 13 May 2014 09:16:20 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22036.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s4DDGOeY12845140 for ; Tue, 13 May 2014 13:16:25 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 s4DDGOmn018003 for ; Tue, 13 May 2014 09:16:24 -0400 Message-ID: <53721B27.1090507@linux.vnet.ibm.com> Date: Tue, 13 May 2014 09:16: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> In-Reply-To: <53707BB3.1060904@de.ibm.com> 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: Christian Borntraeger , qemu-devel@nongnu.org Cc: agraf@suse.de, aliguori@amazon.com, pbonzini@redhat.com, Cornelia Huck , imammedo@redhat.com, rth@twiddle.net 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. > > 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); >> > > > >