From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53284) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHUvU-00055c-K0 for qemu-devel@nongnu.org; Tue, 21 Jul 2015 06:37:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZHUvP-0001vB-2O for qemu-devel@nongnu.org; Tue, 21 Jul 2015 06:37:32 -0400 Received: from e06smtp14.uk.ibm.com ([195.75.94.110]:34019) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZHUvO-0001uV-N5 for qemu-devel@nongnu.org; Tue, 21 Jul 2015 06:37:26 -0400 Received: from /spool/local by e06smtp14.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 21 Jul 2015 11:37:23 +0100 Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by d06dlp02.portsmouth.uk.ibm.com (Postfix) with ESMTP id 663C42190023 for ; Tue, 21 Jul 2015 11:36:57 +0100 (BST) Received: from d06av07.portsmouth.uk.ibm.com (d06av07.portsmouth.uk.ibm.com [9.149.37.248]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t6LAbKb038928466 for ; Tue, 21 Jul 2015 10:37:20 GMT Received: from d06av07.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av07.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id t6LAbKo2014452 for ; Tue, 21 Jul 2015 06:37:20 -0400 Date: Tue, 21 Jul 2015 12:37:18 +0200 From: David Hildenbrand Message-ID: <20150721123718.6ab0b668@thinkpad-w530> In-Reply-To: <55ADFE0B.1090407@redhat.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH for-2.5 7/8] s390x: Migrate guest storage keys (initial memory only) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth , jjherne@linux.vnet.ibm.com Cc: Cornelia Huck , borntraeger@de.ibm.com, jfrei@linux.vnet.ibm.com, qemu-devel@nongnu.org, agraf@suse.de > > 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. > > > + } > > + > > + /* 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