From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49197) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPtib-0007Lk-Ky for qemu-devel@nongnu.org; Thu, 13 Aug 2015 10:42:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZPtiV-00067r-4B for qemu-devel@nongnu.org; Thu, 13 Aug 2015 10:42:57 -0400 Received: from e18.ny.us.ibm.com ([129.33.205.208]:44518) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZPtiV-00067e-0s for qemu-devel@nongnu.org; Thu, 13 Aug 2015 10:42:51 -0400 Received: from /spool/local by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 13 Aug 2015 10:15:32 -0400 Received: from b01cxnp22034.gho.pok.ibm.com (b01cxnp22034.gho.pok.ibm.com [9.57.198.24]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 8295738C814E for ; Thu, 13 Aug 2015 10:11:28 -0400 (EDT) Received: from d01av03.pok.ibm.com (d01av03.pok.ibm.com [9.56.224.217]) by b01cxnp22034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id t7DEBRmb62456034 for ; Thu, 13 Aug 2015 14:11:27 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 t7DEBRS7014670 for ; Thu, 13 Aug 2015 10:11:27 -0400 Message-ID: <55CCA58D.8090708@linux.vnet.ibm.com> Date: Thu, 13 Aug 2015 10:11:25 -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. > >> >>> + } >>> + >>> + /* 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. > What storage key value would you consider invalid? All combinations of the upper four bits are valid. And of the lower four, we have the FP, reference and change bits with the final bit marked as reserved. The only possible answer would be to abuse the reserved bit and set it to 1 when there is an error. The major problem with that: This bit could be used for something someday which would require us to stop using it for an error indicator. Another problem is that we would then have to check every single storage key for this error bit on the destination side. This ioctl should not fail if we've made it this far. If it does we are still covered because the sudden hole in the data will throw off everything else. It could (in VERY rare cases, if at all) cause error messages to surface that are unrelated to the problem but the correct "S390_GET_KEYS error %d" message will still be displayed first. Certainly it is not 100% perfect but since the sending side is not allowed to fail there seems to be simple option here. We could re-engineer the protocol to send packets of [Length][KeyData] and we could decide on an error value for length (0xFFFFFFFF) that would indicate an error, perhaps with 0x0 indicating end of data. I'm happy to do the work if requested but is it really worth it? >> >>> + 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)