From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43068) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsxAv-0000mI-60 for qemu-devel@nongnu.org; Tue, 17 Dec 2013 11:07:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VsxAm-0006mi-6x for qemu-devel@nongnu.org; Tue, 17 Dec 2013 11:07:13 -0500 Received: from e34.co.us.ibm.com ([32.97.110.152]:41348) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VsxAl-0006mU-Vq for qemu-devel@nongnu.org; Tue, 17 Dec 2013 11:07:04 -0500 Received: from /spool/local by e34.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 17 Dec 2013 09:07:03 -0700 Received: from b03cxnp08025.gho.boulder.ibm.com (b03cxnp08025.gho.boulder.ibm.com [9.17.130.17]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id A1B291FF0049 for ; Tue, 17 Dec 2013 09:06:35 -0700 (MST) Received: from d03av03.boulder.ibm.com (d03av03.boulder.ibm.com [9.17.195.169]) by b03cxnp08025.gho.boulder.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id rBHG6x9F8716680 for ; Tue, 17 Dec 2013 17:06:59 +0100 Received: from d03av03.boulder.ibm.com (localhost [127.0.0.1]) by d03av03.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id rBHG6xFq002285 for ; Tue, 17 Dec 2013 09:06:59 -0700 Message-ID: <52B076A1.8030008@linux.vnet.ibm.com> Date: Tue, 17 Dec 2013 11:06:57 -0500 From: Matthew Rosato MIME-Version: 1.0 References: <1387227072-21965-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1387227072-21965-6-git-send-email-mjrosato@linux.vnet.ibm.com> <66752973-0E0C-4D70-9470-72EE48BD0642@suse.de> In-Reply-To: <66752973-0E0C-4D70-9470-72EE48BD0642@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 5/5] sclp-s390: Add memory hotplug SCLPs List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Gleb Natapov , QEMU Developers , Christian Borntraeger , aliguori@amazon.com, Cornelia Huck , Paolo Bonzini , Richard Henderson On 12/16/2013 04:42 PM, Alexander Graf wrote: > > On 16.12.2013, at 21:51, Matthew Rosato wrote: > >> Add memory information to read SCP info and add handlers for >> Read Storage Element Information, Attach Storage Element, >> Assign Storage and Unassign Storage. >> >> Signed-off-by: Matthew Rosato >> --- >> hw/s390x/sclp.c | 233 +++++++++++++++++++++++++++++++++++++++++++++-- >> include/hw/s390x/sclp.h | 2 + >> 2 files changed, 229 insertions(+), 6 deletions(-) >> >> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c >> index cb53d7e..5d27fc3 100644 >> --- a/hw/s390x/sclp.c >> +++ b/hw/s390x/sclp.c >> @@ -15,9 +15,15 @@ >> #include "cpu.h" >> #include "sysemu/kvm.h" >> #include "exec/memory.h" >> - >> +#include "exec/address-spaces.h" >> #include "hw/s390x/sclp.h" >> >> +int shift; >> +ram_addr_t standby_subregion_size; >> +ram_addr_t padded_ram_size; >> +ram_addr_t rzm; >> +char *standby_state_map; > > Do you really need all these globals? > I think this ties into your device comment below - I agree that these should be encapsulated. >> + >> static inline S390SCLPDevice *get_event_facility(void) >> { >> ObjectProperty *op = object_property_find(qdev_get_machine(), >> @@ -31,16 +37,215 @@ static inline S390SCLPDevice *get_event_facility(void) >> static void read_SCP_info(SCCB *sccb) >> { >> ReadInfo *read_info = (ReadInfo *) sccb; >> - int shift = 0; >> + int rnsize, rnmax; >> + int max_avail_slots = MAX_AVAIL_SLOTS; >> +#ifdef CONFIG_KVM >> + max_avail_slots = kvm_check_extension(kvm_state, KVM_CAP_NR_MEMSLOTS); > > Please don't call kvm specific code from non-kvm specific code. Check out kvm_s390_io_interrupt() as an example how to plumb it in. Please also check kvm_enabled(), as CONFIG_KVM doesn't mean that you actually end up using KVM. It's probably best to completely abstract this away in a helper. > OK, will do. >> +#endif >> + >> + read_info->facilities = 0; >> + >> + if (standby_mem_size) { > > I don't understand why we have 2 code paths - one for standby and one without. Can't we just handle all cases as standby? > Yes I think so - everything should fall-through naturally and we can just skip the malloc of standby_state_map. >> + /* >> + * The storage increment size is a multiple of 1M and is a power of 2. >> + * The number of storage increments must be 1020 or fewer. >> + */ >> + while ((ram_size >> (20 + shift)) > 1020) { >> + shift++; >> + } >> + while ((standby_mem_size >> (20 + shift)) > 1020) { >> + shift++; >> + } >> + >> + standby_subregion_size = MEM_SECTION_SIZE; >> + /* Deduct the memory slot already used for core */ >> + while ((standby_subregion_size * (max_avail_slots - 1) >> + < standby_mem_size)) { >> + standby_subregion_size = standby_subregion_size << 1; >> + } >> + /* >> + * Initialize mapping of guest standby memory sections indicating which >> + * are and are not online. Assume all standby memory begins offline. >> + */ >> + if (standby_mem_size % standby_subregion_size) { >> + standby_state_map = g_malloc0((standby_mem_size / >> + standby_subregion_size + 1) * >> + (standby_subregion_size / >> + MEM_SECTION_SIZE)); >> + } else { >> + standby_state_map = g_malloc0(standby_mem_size / MEM_SECTION_SIZE); >> + } > > You don't free this thing when the guest calls read_SCP_info twice. > Oops. Acknowledged. >> + >> + padded_ram_size = ram_size + pad_size; >> + rzm = 1 << (20 + shift); >> + >> + } else { >> + while ((ram_size >> (20 + shift)) > 65535) { >> + shift++; >> + } >> + } >> >> - while ((ram_size >> (20 + shift)) > 65535) { >> - shift++; >> + rnsize = 1 << shift; >> + if (rnsize <= 128) { >> + read_info->rnsize = rnsize; >> + } else { >> + read_info->rnsize = 0; >> + read_info->rnsize2 = cpu_to_be32(rnsize); >> + } >> + >> + rnmax = (ram_size + standby_mem_size + pad_size) >> (20 + shift); >> + if (rnmax < 0x10000) { >> + read_info->rnmax = cpu_to_be16(rnmax); >> + } else { >> + read_info->rnmax = cpu_to_be16(0); >> + read_info->rnmax2 = cpu_to_be64(rnmax); >> + } >> + >> + if (standby_mem_size) { >> + read_info->facilities |= cpu_to_be64(SCLP_FC_ASSIGN_ATTACH_READ_STOR); > > Any reason to make this conditional? > I suppose not -- I'll make this unconditional. >> } >> - read_info->rnmax = cpu_to_be16(ram_size >> (20 + shift)); >> - read_info->rnsize = 1 << shift; >> sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_READ_COMPLETION); >> } [...] >> +static void unassign_storage(SCCB *sccb) >> +{ >> + MemoryRegion *mr = NULL; >> + AssignStorage *assign_info = (AssignStorage *) sccb; >> + ram_addr_t unassign_addr = (assign_info->rn - 1) * rzm; >> + MemoryRegion *sysmem = get_system_memory(); >> + >> + /* if the addr is a multiple of 256 MB */ >> + if ((unassign_addr % MEM_SECTION_SIZE == 0) && >> + (unassign_addr >= padded_ram_size)) { >> + standby_state_map[(unassign_addr - >> + padded_ram_size) / MEM_SECTION_SIZE] = 0; >> + mr = memory_region_find(sysmem, unassign_addr, 1).mr; >> + if (mr) { >> + int i; >> + int is_removable = 1; >> + ram_addr_t map_offset = (unassign_addr - padded_ram_size - >> + (unassign_addr - padded_ram_size) >> + % standby_subregion_size); >> + for (i = 0; >> + i < (standby_subregion_size / MEM_SECTION_SIZE); >> + i++) { >> + >> + if (standby_state_map[i + map_offset / MEM_SECTION_SIZE]) { >> + is_removable = 0; >> + break; >> + } >> + } >> + if (is_removable) { >> + memory_region_del_subregion(sysmem, mr); >> + memory_region_destroy(mr); > > no g_free()? Oops. Acknowledged. > >> + } >> + } >> + } >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION); >> +} >> + >> static void sclp_execute(SCCB *sccb, uint64_t code) >> { >> S390SCLPDevice *sdev = get_event_facility(); >> @@ -50,6 +255,22 @@ static void sclp_execute(SCCB *sccb, uint64_t code) >> case SCLP_CMDW_READ_SCP_INFO_FORCED: >> read_SCP_info(sccb); >> break; >> + case SCLP_READ_STORAGE_ELEMENT_INFO: >> + if (code & 0xff00) { >> + read_storage_element1_info(sccb); >> + } else { >> + read_storage_element0_info(sccb); >> + } >> + break; >> + case SCLP_ATTACH_STORAGE_ELEMENT: >> + attach_storage_element(sccb, (code & 0xff00) >> 8); >> + break; >> + case SCLP_ASSIGN_STORAGE: >> + assign_storage(sccb); >> + break; >> + case SCLP_UNASSIGN_STORAGE: >> + unassign_storage(sccb); >> + break; > > Do you think it'd be possible to model this whole thing as a device that has its own state? That's where you could keep the bitmap for example. You'd only need some callback mechanism to hook into the SCLP calls, but the PPC guys already have something similar with their hypercalls. > OK, let me look into this for the next version. > Overall, the code could use _a lot_ more comments. Will do. Thanks for your comments. > > > Alex > > > >