* Re: [Qemu-devel] Bug in virtio_net_load
2016-06-30 17:23 ` Michael S. Tsirkin
@ 2016-06-30 17:29 ` Dr. David Alan Gilbert
2016-07-01 2:35 ` Jason Wang
2016-07-01 8:48 ` Cornelia Huck
2 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2016-06-30 17:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Robin Geuze, jasowang, qemu-devel, Cornelia Huck
* Michael S. Tsirkin (mst@redhat.com) 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 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 <jasowang@redhat.com>
> 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 <kraxel@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
>
> 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);
>
> + /*
> + * 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);
>
> /*
>
> 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?
Yes, bug fixes first. But actually the merge is trivial, the only
place I think they collide is in virtio_net_load and it's just the difference
of the return virtio_load vs the ret = virtio_load in the patch you revert.
Dave
> --
> MST
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] Bug in virtio_net_load
2016-06-30 17:23 ` Michael S. Tsirkin
2016-06-30 17:29 ` Dr. David Alan Gilbert
@ 2016-07-01 2:35 ` Jason Wang
2016-07-01 8:48 ` Cornelia Huck
2 siblings, 0 replies; 7+ messages in thread
From: Jason Wang @ 2016-07-01 2:35 UTC (permalink / raw)
To: Michael S. Tsirkin, Robin Geuze; +Cc: qemu-devel, dgilbert, Cornelia Huck
On 2016年07月01日 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 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 <jasowang@redhat.com>
> 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 <kraxel@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
>
>
> 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);
>
> + /*
> + * 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);
>
> /*
>
> 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
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?
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [Qemu-devel] Bug in virtio_net_load
2016-06-30 17:23 ` Michael S. Tsirkin
2016-06-30 17:29 ` Dr. David Alan Gilbert
2016-07-01 2:35 ` Jason Wang
@ 2016-07-01 8:48 ` Cornelia Huck
2016-07-01 8:54 ` Robin Geuze
2016-07-04 7:11 ` Robin Geuze
2 siblings, 2 replies; 7+ messages in thread
From: Cornelia Huck @ 2016-07-01 8:48 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Robin Geuze, jasowang, qemu-devel, dgilbert
On Thu, 30 Jun 2016 20:23:08 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> I'm not sure what was I thinking when I applied this:
> it changes load without changing save - how can this work?
The ordering implications are easy to miss :(
> 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);
>
> + /*
> + * 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.
docs/virtio-migration.txt should probably talk about that as well. And
any conditional stuff needs to go into a subsection in the future.
> + */
> + vdev->guest_features = features;
> +
> config_len = qemu_get_be32(f);
>
> /*
>
> Could you please confirm whether this help?
> Jason, Cornelia - any comments?
After staring at the code, I'm inclined to think that this will work.
virtio migration: Frying unsuspecting brains since 2008.
<To be fair, the original code wasn't that convoluted.>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] Bug in virtio_net_load
2016-07-01 8:48 ` Cornelia Huck
@ 2016-07-01 8:54 ` Robin Geuze
2016-07-04 7:11 ` Robin Geuze
1 sibling, 0 replies; 7+ messages in thread
From: Robin Geuze @ 2016-07-01 8:54 UTC (permalink / raw)
To: Cornelia Huck, Michael S. Tsirkin; +Cc: jasowang, qemu-devel, dgilbert
Hey Guys,
We just tested the patch on QEMU 2.6.0 and confirmed that both 2.6.0 ->
2.6.0 and 2.4.0 -> 2.6.0 migrations work properly.
We will be leaving a migration loop running over the weekend to verify
that everything works as expected, but I don't expect any surprises from
that. Thanks for the quick fix :D
Regards,
Robin Geuze
TransIP BV
On 7/1/2016 10:48, Cornelia Huck wrote:
> On Thu, 30 Jun 2016 20:23:08 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> I'm not sure what was I thinking when I applied this:
>> it changes load without changing save - how can this work?
> The ordering implications are easy to miss :(
>
>> 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);
>>
>> + /*
>> + * 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.
> docs/virtio-migration.txt should probably talk about that as well. And
> any conditional stuff needs to go into a subsection in the future.
>
>> + */
>> + vdev->guest_features = features;
>> +
>> config_len = qemu_get_be32(f);
>>
>> /*
>>
>> Could you please confirm whether this help?
>> Jason, Cornelia - any comments?
> After staring at the code, I'm inclined to think that this will work.
>
> virtio migration: Frying unsuspecting brains since 2008.
> <To be fair, the original code wasn't that convoluted.>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] Bug in virtio_net_load
2016-07-01 8:48 ` Cornelia Huck
2016-07-01 8:54 ` Robin Geuze
@ 2016-07-04 7:11 ` Robin Geuze
1 sibling, 0 replies; 7+ messages in thread
From: Robin Geuze @ 2016-07-04 7:11 UTC (permalink / raw)
To: Cornelia Huck, Michael S. Tsirkin; +Cc: jasowang, qemu-devel, dgilbert
Hey,
So over the weekend we did a bunch of migrations with a bunch of
different guest OSes and such and it all worked fine, so I would say the
patch is working properly :)
Regards,
Robin Geuze
On 7/1/2016 10:48, Cornelia Huck wrote:
> On Thu, 30 Jun 2016 20:23:08 +0300
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
>> I'm not sure what was I thinking when I applied this:
>> it changes load without changing save - how can this work?
> The ordering implications are easy to miss :(
>
>> 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);
>>
>> + /*
>> + * 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.
> docs/virtio-migration.txt should probably talk about that as well. And
> any conditional stuff needs to go into a subsection in the future.
>
>> + */
>> + vdev->guest_features = features;
>> +
>> config_len = qemu_get_be32(f);
>>
>> /*
>>
>> Could you please confirm whether this help?
>> Jason, Cornelia - any comments?
> After staring at the code, I'm inclined to think that this will work.
>
> virtio migration: Frying unsuspecting brains since 2008.
> <To be fair, the original code wasn't that convoluted.>
>
^ permalink raw reply [flat|nested] 7+ messages in thread