From patchwork Thu Jun 30 17:23:08 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 9208649 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 052B460752 for ; Thu, 30 Jun 2016 17:23:57 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E851328364 for ; Thu, 30 Jun 2016 17:23:56 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id DD2372850F; Thu, 30 Jun 2016 17:23:56 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 4D6D328364 for ; Thu, 30 Jun 2016 17:23:56 +0000 (UTC) Received: from localhost ([::1]:51800 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bIfgw-0002GZ-Eo for patchwork-qemu-devel@patchwork.kernel.org; Thu, 30 Jun 2016 13:23:54 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bIfgX-0002GR-S6 for qemu-devel@nongnu.org; Thu, 30 Jun 2016 13:23:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bIfgT-00005a-QF for qemu-devel@nongnu.org; Thu, 30 Jun 2016 13:23:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45095) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bIfgT-00005W-Hn for qemu-devel@nongnu.org; Thu, 30 Jun 2016 13:23:25 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 393D07DCFF; Thu, 30 Jun 2016 17:23:24 +0000 (UTC) Received: from redhat.com (ovpn-112-59.ams2.redhat.com [10.36.112.59]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id u5UHNAAO005100; Thu, 30 Jun 2016 13:23:14 -0400 Date: Thu, 30 Jun 2016 20:23:08 +0300 From: "Michael S. Tsirkin" To: Robin Geuze Message-ID: <20160630200609-mutt-send-email-mst@redhat.com> References: <37e29695-43f6-85d5-f7ef-4ce0cf38c6c1@transip.nl> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <37e29695-43f6-85d5-f7ef-4ce0cf38c6c1@transip.nl> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 30 Jun 2016 17:23:24 +0000 (UTC) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.132.183.28 Subject: Re: [Qemu-devel] Bug in virtio_net_load X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Cornelia Huck , jasowang@redhat.com, qemu-devel@nongnu.org, dgilbert@redhat.com Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Jun 30, 2016 at 10:34:51AM +0200, Robin Geuze wrote: > Hey, > > I work for TransIP and we host a VPS platform based on QEMU/KVM. We are > currently running qemu 2.4.0. A few days ago we noticed that live migrations > for some of our VM's would fail. Further investigation turned out it was > specific to windows server 2012, caused by the fact that the standard virtio > driver from RedHat was replaced in windows updates by a driver called > "Midfin eFabric" (this driver doesn't really seem to be meant for virtio, we > have a case running at MicroSoft about that). Once we knew how to reproduce > we tested this on QEMU 2.6.0 as well and it also seems to be affected > (later we found out that 2.4.0 to 2.6.0 migration does work probably due to > pure luck). > > We started investigating the problem in QEMU 2.4.0 and noticed it was caused > by the fact that virtio_net_device_load requires certain feature flags to be > set, specifically to load curr_guest_offloads which is only written and read > if the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS flag is set, but those flags are set > in virtio_load after the call to virtio_net_device_load. Moving the code > setting the feature flags before the call to virtio_net_device_load fixes > it, however it introduces another problem. Virtio can have 64-bits feature > flags, however the standard save payload for virtio only has space for > 32-bits feature flags. This was solved by putting those in a subsection of > the vmstate_save_state stuff. Unfortunately this is called (and thus binary > offset located) after the virtio_net_device_load code. > > There was an attempt to fix this in QEMU 2.6.0. However, this seems to have > broken it worse. The write code (virtio_net_save, virtio_save and > virtio_net_save_device) still puts the curr_guest_offloads value before the > vmstate_save_state data. However the read code expects and tries to read it > after the vmstate_save_state data. Should we just also change the > virtio_net_save code to have it follow the same order as virtio_net_load? Or > will this potentially break more stuff. > > Regards, > > Robin Geuze > > TransIP BV After going over it several times, I think the change in 2.6 was wrong commit 1f8828ef573c83365b4a87a776daf8bcef1caa21 Author: Jason Wang Date: Fri Sep 11 16:01:56 2015 +0800 virtio-net: unbreak self announcement and guest offloads after migration After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make features 64bit wide"). Device's guest_features was actually set after vdc->load(). This breaks the assumption that device specific load() function can check guest_features. For virtio-net, self announcement and guest offloads won't work after migration. Fixing this by defer them to virtio_net_load() where guest_features were guaranteed to be set. Other virtio devices looks fine. Fixes: 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: make features 64bit wide") Cc: qemu-stable@nongnu.org Cc: Gerd Hoffmann Signed-off-by: Jason Wang Reviewed-by: Michael S. Tsirkin Signed-off-by: Michael S. Tsirkin Reviewed-by: Cornelia Huck I'm not sure what was I thinking when I applied this: it changes load without changing save - how can this work? I am inclined to revert 1f8828ef573c83365b4a87a776daf8bcef1caa21 and apply this instead: Could you please confirm whether this help? Jason, Cornelia - any comments? David, if this goes in I'm afraid your patchset reworking save/load will have to be rebased, but I think we want the bugfix first and new features/changes second. Do you agree? diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 7ed06ea..18153d5 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -1499,6 +1499,16 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id) } qemu_get_be32s(f, &features); + /* + * Temporarily set guest_features low bits - needed by + * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOADS + * VIRTIO_NET_F_GUEST_ANNOUNCE and VIRTIO_NET_F_CTRL_VQ. + * + * Note: devices should always test host features in future - don't create + * new dependencies like this. + */ + vdev->guest_features = features; + config_len = qemu_get_be32(f); /*