From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50565) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bIoJO-00011K-KW for qemu-devel@nongnu.org; Thu, 30 Jun 2016 22:36:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bIoJJ-0002HX-IS for qemu-devel@nongnu.org; Thu, 30 Jun 2016 22:36:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46658) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bIoJJ-0002HS-AR for qemu-devel@nongnu.org; Thu, 30 Jun 2016 22:36:05 -0400 References: <37e29695-43f6-85d5-f7ef-4ce0cf38c6c1@transip.nl> <20160630200609-mutt-send-email-mst@redhat.com> From: Jason Wang Message-ID: <5775D70E.4070906@redhat.com> Date: Fri, 1 Jul 2016 10:35:58 +0800 MIME-Version: 1.0 In-Reply-To: <20160630200609-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] Bug in virtio_net_load List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Robin Geuze Cc: qemu-devel@nongnu.org, dgilbert@redhat.com, Cornelia Huck On 2016=E5=B9=B407=E6=9C=8801=E6=97=A5 01:23, Michael S. Tsirkin wrote: > 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 ar= e >> currently running qemu 2.4.0. A few days ago we noticed that live migr= ations >> for some of our VM's would fail. Further investigation turned out it w= as >> 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 virt= io, we >> have a case running at MicroSoft about that). Once we knew how to rep= roduce >> 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 d= ue 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 an= d read >> if the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS flag is set, but those flags a= re set >> in virtio_load after the call to virtio_net_device_load. Moving the co= de >> setting the feature flags before the call to virtio_net_device_load fi= xes >> it, however it introduces another problem. Virtio can have 64-bits fea= ture >> flags, however the standard save payload for virtio only has space for >> 32-bits feature flags. This was solved by putting those in a subsectio= n of >> the vmstate_save_state stuff. Unfortunately this is called (and thus b= inary >> 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 befor= e the >> vmstate_save_state data. However the read code expects and tries to re= ad 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_lo= ad? 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 mig= ration > =20 > After commit 019a3edbb25f1571e876f8af1ce4c55412939e5d ("virtio: ma= ke > features 64bit wide"). Device's guest_features was actually set af= ter > vdc->load(). This breaks the assumption that device specific load(= ) > function can check guest_features. For virtio-net, self announceme= nt > and guest offloads won't work after migration. > =20 > Fixing this by defer them to virtio_net_load() where guest_feature= s > were guaranteed to be set. Other virtio devices looks fine. > =20 > 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: > > 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); > =20 > + /* > + * Temporarily set guest_features low bits - needed by > + * virtio net load code testing for VIRTIO_NET_F_CTRL_GUEST_OFFLOA= DS > + * 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 =3D features; > + > config_len =3D qemu_get_be32(f); > =20 > /* > > Could you please confirm whether this help? > Jason, Cornelia - any comments? Yes, my patch was wrong and won't work if there's any subsections. I=20 agree to revert and apply yours. Thanks > > 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? >