From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49655) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKpKn-0003MZ-8i for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:01:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKpKf-0005Av-FW for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:01:25 -0400 Received: from e39.co.us.ibm.com ([32.97.110.160]:59870) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKpKf-0005AB-9s for qemu-devel@nongnu.org; Thu, 30 Jul 2015 11:01:17 -0400 Received: from /spool/local by e39.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 30 Jul 2015 09:01:15 -0600 Received: from b01cxnp23034.gho.pok.ibm.com (b01cxnp23034.gho.pok.ibm.com [9.57.198.29]) by d01dlp03.pok.ibm.com (Postfix) with ESMTP id 3AE26C9005A for ; Thu, 30 Jul 2015 10:52:18 -0400 (EDT) Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by b01cxnp23034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t6UF1ALn36372684 for ; Thu, 30 Jul 2015 15:01:10 GMT Received: from d01av02.pok.ibm.com (localhost [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t6UF169i002150 for ; Thu, 30 Jul 2015 11:01:09 -0400 Message-ID: <55BA3C11.2060202@linux.vnet.ibm.com> Date: Thu, 30 Jul 2015 11:00:33 -0400 From: "Jason J. Herne" MIME-Version: 1.0 References: <1437400198-25382-1-git-send-email-cornelia.huck@de.ibm.com> <1437400198-25382-8-git-send-email-cornelia.huck@de.ibm.com> <55ADFE0B.1090407@redhat.com> <20150721123718.6ab0b668@thinkpad-w530> In-Reply-To: <20150721123718.6ab0b668@thinkpad-w530> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only) Reply-To: jjherne@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand , Thomas Huth Cc: Cornelia Huck , borntraeger@de.ibm.com, jfrei@linux.vnet.ibm.com, qemu-devel@nongnu.org, agraf@suse.de On 07/21/2015 06:37 AM, David Hildenbrand wrote: >> >> So if I've got this code right, you send here a "header" that announces >> a packet with all pages ... >> >>> + while (handled_count < total_count) { >>> + cur_count = MIN(total_count - handled_count, S390_SKEYS_BUFFER_SIZE); >>> + >>> + ret = skeyclass->get_skeys(ss, cur_gfn, cur_count, buf); >>> + if (ret < 0) { >>> + error_report("S390_GET_KEYS error %d\n", ret); >>> + break; >> >> ... but when an error occurs here, you suddenly stop in the middle of >> that "packet" with all pages ... > > Indeed, although that should never fail, we never know. > We don't want to overengineer the protocol but still abort migration at least > on the loading side in that (theoretical) case. > I don't have a strong opinion on this either way. I think it is fine just the way it is (for the reasons David described above). However, if people are worried I can see about writing some code that sends fake keys to the destination as described below. Thoughts? >> >>> + } >>> + >>> + /* write keys to stream */ >>> + qemu_put_buffer(f, buf, cur_count); >>> + >>> + cur_gfn += cur_count; >>> + handled_count += cur_count; >>> + } >>> + >>> + g_free(buf); >>> +end_stream: >>> + qemu_put_be64(f, S390_SKEYS_SAVE_FLAG_EOS); >> >> ... and send an EOS marker here instead ... >> >>> +} >>> + >>> +static int s390_storage_keys_load(QEMUFile *f, void *opaque, int version_id) >>> +{ >>> + S390SKeysState *ss = S390_SKEYS(opaque); >>> + S390SKeysClass *skeyclass = S390_SKEYS_GET_CLASS(ss); >>> + int ret = 0; >>> + >>> + while (!ret) { >>> + ram_addr_t addr; >>> + int flags; >>> + >>> + addr = qemu_get_be64(f); >>> + flags = addr & ~TARGET_PAGE_MASK; >>> + addr &= TARGET_PAGE_MASK; >>> + >>> + switch (flags) { >>> + case S390_SKEYS_SAVE_FLAG_SKEYS: { >>> + const uint64_t total_count = qemu_get_be64(f); >>> + uint64_t handled_count = 0, cur_count; >>> + uint64_t cur_gfn = addr / TARGET_PAGE_SIZE; >>> + uint8_t *buf = g_try_malloc(S390_SKEYS_BUFFER_SIZE); >>> + >>> + if (!buf) { >>> + error_report("storage key load could not allocate memory\n"); >>> + ret = -ENOMEM; >>> + break; >>> + } >>> + >>> + while (handled_count < total_count) { >>> + cur_count = MIN(total_count - handled_count, >>> + S390_SKEYS_BUFFER_SIZE); >>> + qemu_get_buffer(f, buf, cur_count); >> >> ... while the receiver can not handle the EOS marker here. >> >> This looks fishy to me (or I might have just missed something), but >> anyway please double check whether your error handling in the sender >> really makes sense. > > My shot would be, to send invalid storage keys if getting the keys from the > kernel fails. So we can detect it on the loading side and abort migration > gracefully. > >> >>> + ret = skeyclass->set_skeys(ss, cur_gfn, cur_count, buf); >>> + if (ret < 0) { >>> + error_report("S390_SET_KEYS error %d\n", ret); >>> + break; >>> + } >>> + handled_count += cur_count; >>> + cur_gfn += cur_count; >>> + } >>> + g_free(buf); >>> + break; >>> + } >>> + case S390_SKEYS_SAVE_FLAG_EOS: >>> + /* normal exit */ >>> + return 0; >>> + default: >>> + error_report("Unexpected storage key flag data: %#x", flags); >>> + ret = -EINVAL; >>> + } >>> + } >>> + >>> + return ret; >>> +} >> >> Thomas > > Thanks Thomas! > > > David > -- -- Jason J. Herne (jjherne@linux.vnet.ibm.com)