From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40824) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0Bde-0003Fv-MY for qemu-devel@nongnu.org; Thu, 26 Jun 2014 11:31:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1X0BdU-0000W6-T5 for qemu-devel@nongnu.org; Thu, 26 Jun 2014 11:31:02 -0400 Received: from e7.ny.us.ibm.com ([32.97.182.137]:43833) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1X0BdU-0000Vk-OA for qemu-devel@nongnu.org; Thu, 26 Jun 2014 11:30:52 -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 ; Thu, 26 Jun 2014 11:30:51 -0400 Received: from b01cxnp22035.gho.pok.ibm.com (b01cxnp22035.gho.pok.ibm.com [9.57.198.25]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 4067938C8046 for ; Thu, 26 Jun 2014 11:30:49 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22035.gho.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5QFUnTg1442256 for ; Thu, 26 Jun 2014 15:30:49 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 s5QFUmcG001346 for ; Thu, 26 Jun 2014 11:30:49 -0400 Message-ID: <53AC3CA4.9090405@linux.vnet.ibm.com> Date: Thu, 26 Jun 2014 11:30:44 -0400 From: Matthew Rosato MIME-Version: 1.0 References: <1403706420-20109-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1403706420-20109-4-git-send-email-mjrosato@linux.vnet.ibm.com> <53AC1CC7.3000504@de.ibm.com> In-Reply-To: <53AC1CC7.3000504@de.ibm.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/3] sclp-s390: Add memory hotplug SCLPs 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@de.ibm.com, imammedo@redhat.com, rth@twiddle.net On 06/26/2014 09:14 AM, Christian Borntraeger wrote: > On 25/06/14 16:27, 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 > > In general this looks fine. I gave it some testing and most of it seems to work. > With lots of standy chunks (-m 10240,maxmem=204800M,slots=16) I get an abort, though: > > #0 0x000003fffb9cffd8 in raise () from /lib64/libc.so.6 > #1 0x000003fffb9d1b30 in abort () from /lib64/libc.so.6 > #2 0x000003fffb9c7376 in __assert_fail_base () from /lib64/libc.so.6 > #3 0x000003fffb9c7418 in __assert_fail () from /lib64/libc.so.6 > #4 0x000000008001db28 in find_ram_offset (size=) at /home/cborntra/REPOS/qemu/exec.c:1109 > #5 ram_block_add (new_block=0x3fd70001ab0) at /home/cborntra/REPOS/qemu/exec.c:1236 > #6 0x000000008005ced0 in memory_region_init_ram (mr=mr@entry=0x3fd700019e0, owner=owner@entry=0x0, name=name@entry=0x3fd759fda98 "standby.ram1", size=0) at /home/cborntra/REPOS/qemu/memory.c:1033 > > Why do we pass size==0? > > #7 0x0000000080080cdc in assign_storage (sccb=0x3fd759fca98) at /home/cborntra/REPOS/qemu/hw/s390x/sclp.c:242 > #8 sclp_execute (code=, sccb=0x3fd759fca98) at /home/cborntra/REPOS/qemu/hw/s390x/sclp.c:348 > #9 sclp_service_call (env=env@entry=0x808dcf98, sccb=2144296960, code=) at /home/cborntra/REPOS/qemu/hw/s390x/sclp.c:395 > #10 0x00000000800b127c in kvm_sclp_service_call (run=, ipbh0=35, cpu=0x808d4d00) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:705 > #11 handle_b2 (run=, ipa1=, cpu=0x808d4d00) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:777 > #12 handle_instruction (run=, cpu=0x808d4d00) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1001 > #13 handle_intercept (cpu=0x808d4d00) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1060 > #14 kvm_arch_handle_exit (cs=cs@entry=0x808d4d00, run=run@entry=0x3fffd081000) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1177 > #15 0x000000008005a696 in kvm_cpu_exec (cpu=cpu@entry=0x808d4d00) at /home/cborntra/REPOS/qemu/kvm-all.c:1784 > #16 0x000000008004668a in qemu_kvm_cpu_thread_fn (arg=0x808d4d00) at /home/cborntra/REPOS/qemu/cpus.c:874 > #17 0x000003fffce18412 in start_thread () from /lib64/libpthread.so.0 > #18 0x000003fffba9e0ae in thread_start () from /lib64/libc.so.6' Looks like this_subregion_size is overflowing... [..snip..] >> +static void assign_storage(SCCB *sccb) >> +{ >> + MemoryRegion *mr = NULL; >> + int this_subregion_size; This shouldn't be an int, everything else is 64-bit unsigned terms, so it's causing truncation with large subregion sizes. Should be uint64_t this_subregion_size, which is what memory_region_init_ram expects. This fixes the problem for me. I'll do some more testing with that and include it in the next version -- Thanks! >> + AssignStorage *assign_info = (AssignStorage *) sccb; >> + sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev(); >> + assert(mhd); >> + ram_addr_t assign_addr = (assign_info->rn - 1) * mhd->rzm; >> + MemoryRegion *sysmem = get_system_memory(); >> + >> + if ((assign_addr % MEM_SECTION_SIZE == 0) && >> + (assign_addr >= mhd->padded_ram_size)) { >> + /* Re-use existing memory region if found */ >> + mr = memory_region_find(sysmem, assign_addr, 1).mr; >> + if (!mr) { >> + >> + MemoryRegion *standby_ram = g_new(MemoryRegion, 1); >> + >> + /* offset to align to standby_subregion_size for allocation */ >> + ram_addr_t offset = assign_addr - >> + (assign_addr - mhd->padded_ram_size) >> + % mhd->standby_subregion_size; >> + >> + /* strlen("standby.ram") + 4 (Max of KVM_MEMORY_SLOTS) + NULL */ >> + char id[16]; >> + snprintf(id, 16, "standby.ram%d", >> + (int)((offset - mhd->padded_ram_size) / >> + mhd->standby_subregion_size) + 1); >> + >> + /* Allocate a subregion of the calculated standby_subregion_size */ >> + if (offset + mhd->standby_subregion_size > >> + mhd->padded_ram_size + mhd->standby_mem_size) { >> + this_subregion_size = mhd->padded_ram_size + >> + mhd->standby_mem_size - offset; >> + } else { >> + this_subregion_size = mhd->standby_subregion_size; >> + } >> + >> + memory_region_init_ram(standby_ram, NULL, id, this_subregion_size); >> + vmstate_register_ram_global(standby_ram); >> + memory_region_add_subregion(sysmem, offset, standby_ram); >> + } >> + /* The specified subregion is no longer in standby */ >> + mhd->standby_state_map[(assign_addr - mhd->padded_ram_size) >> + / MEM_SECTION_SIZE] = 1; >> + } >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION); >> +} >> + >> +static void unassign_storage(SCCB *sccb) >> +{ >> + MemoryRegion *mr = NULL; >> + AssignStorage *assign_info = (AssignStorage *) sccb; >> + sclpMemoryHotplugDev *mhd = get_sclp_memory_hotplug_dev(); >> + assert(mhd); >> + ram_addr_t unassign_addr = (assign_info->rn - 1) * mhd->rzm; >> + MemoryRegion *sysmem = get_system_memory(); >> + >> + /* if the addr is a multiple of 256 MB */ >> + if ((unassign_addr % MEM_SECTION_SIZE == 0) && >> + (unassign_addr >= mhd->padded_ram_size)) { >> + mhd->standby_state_map[(unassign_addr - >> + mhd->padded_ram_size) / MEM_SECTION_SIZE] = 0; >> + >> + /* find the specified memory region and destroy it */ >> + mr = memory_region_find(sysmem, unassign_addr, 1).mr; >> + if (mr) { >> + int i; >> + int is_removable = 1; >> + ram_addr_t map_offset = (unassign_addr - mhd->padded_ram_size - >> + (unassign_addr - mhd->padded_ram_size) >> + % mhd->standby_subregion_size); >> + /* Mark all affected subregions as 'standby' once again */ >> + for (i = 0; >> + i < (mhd->standby_subregion_size / MEM_SECTION_SIZE); >> + i++) { >> + >> + if (mhd->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); >> + g_free(mr); >> + } >> + } >> + } >> + sccb->h.response_code = cpu_to_be16(SCLP_RC_NORMAL_COMPLETION); >> +} >> + >> /* Provide information about the CPU */ >> static void sclp_read_cpu_info(SCCB *sccb) >> { >> @@ -103,6 +334,22 @@ static void sclp_execute(SCCB *sccb, uint32_t code) >> case SCLP_CMDW_READ_CPU_INFO: >> sclp_read_cpu_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; >> default: >> efc->command_handler(ef, sccb, code); >> break; >> diff --git a/target-s390x/cpu.h b/target-s390x/cpu.h >> index ba0e4b4..1c1681c 100644 >> --- a/target-s390x/cpu.h >> +++ b/target-s390x/cpu.h >> @@ -1047,6 +1047,7 @@ static inline void cpu_inject_crw_mchk(S390CPU *cpu) >> >> /* from s390-virtio-ccw */ >> #define MEM_SECTION_SIZE 0x10000000UL >> +#define MAX_AVAIL_SLOTS 32 >> >> /* fpu_helper.c */ >> uint32_t set_cc_nz_f32(float32 v); >> @@ -1070,6 +1071,7 @@ void kvm_s390_enable_css_support(S390CPU *cpu); >> int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, >> int vq, bool assign); >> int kvm_s390_cpu_restart(S390CPU *cpu); >> +int kvm_s390_get_memslot_count(KVMState *s); >> void kvm_s390_clear_cmma_callback(void *opaque); >> #else >> static inline void kvm_s390_io_interrupt(uint16_t subchannel_id, >> @@ -1097,6 +1099,10 @@ static inline int kvm_s390_cpu_restart(S390CPU *cpu) >> static inline void kvm_s390_clear_cmma_callback(void *opaque) >> { >> } >> +static inline int kvm_s390_get_memslot_count(KVMState *s) >> +{ >> + return MAX_AVAIL_SLOTS; >> +} >> #endif >> >> static inline void cmma_reset(S390CPU *cpu) >> @@ -1115,6 +1121,15 @@ static inline int s390_cpu_restart(S390CPU *cpu) >> return -ENOSYS; >> } >> >> +static inline int s390_get_memslot_count(KVMState *s) >> +{ >> + if (kvm_enabled()) { >> + return kvm_s390_get_memslot_count(s); >> + } else { >> + return MAX_AVAIL_SLOTS; >> + } >> +} >> + >> void s390_io_interrupt(uint16_t subchannel_id, uint16_t subchannel_nr, >> uint32_t io_int_parm, uint32_t io_int_word); >> void s390_crw_mchk(void); >> diff --git a/target-s390x/kvm.c b/target-s390x/kvm.c >> index a6e587b..4b05d48 100644 >> --- a/target-s390x/kvm.c >> +++ b/target-s390x/kvm.c >> @@ -1283,3 +1283,8 @@ int kvm_s390_assign_subch_ioeventfd(EventNotifier *notifier, uint32_t sch, >> } >> return kvm_vm_ioctl(kvm_state, KVM_IOEVENTFD, &kick); >> } >> + >> +int kvm_s390_get_memslot_count(KVMState *s) >> +{ >> + return kvm_check_extension(s, KVM_CAP_NR_MEMSLOTS); >> +} >> > > > >