From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37156) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjku4-0004xd-SV for qemu-devel@nongnu.org; Mon, 12 May 2014 03:44:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Wjktt-0000fN-Tr for qemu-devel@nongnu.org; Mon, 12 May 2014 03:44:04 -0400 Received: from e06smtp12.uk.ibm.com ([195.75.94.108]:55577) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Wjktt-0000fD-LJ for qemu-devel@nongnu.org; Mon, 12 May 2014 03:43:53 -0400 Received: from /spool/local by e06smtp12.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 12 May 2014 08:43:52 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id F15EC17D8042 for ; Mon, 12 May 2014 08:44:56 +0100 (BST) Received: from d06av10.portsmouth.uk.ibm.com (d06av10.portsmouth.uk.ibm.com [9.149.37.251]) by b06cxnps4076.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s4C7hnKY51183848 for ; Mon, 12 May 2014 07:43:49 GMT Received: from d06av10.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av10.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s4C7hmTY007117 for ; Mon, 12 May 2014 01:43:48 -0600 Message-ID: <53707BB3.1060904@de.ibm.com> Date: Mon, 12 May 2014 09:43:47 +0200 From: Christian Borntraeger 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> In-Reply-To: <1399485959-15579-4-git-send-email-mjrosato@linux.vnet.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: Matthew Rosato , qemu-devel@nongnu.org Cc: agraf@suse.de, aliguori@amazon.com, imammedo@redhat.com, Cornelia Huck , pbonzini@redhat.com, rth@twiddle.net 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). 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? > - 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); >