[Qemu-devel] Bug in virtio_net_load

[Qemu-devel] Bug in virtio_net_load

[Robin Geuze]

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

Re: [Qemu-devel] Bug in virtio_net_load

[Michael S. Tsirkin]

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?

-- 
MST

Re: [Qemu-devel] Bug in virtio_net_load

[Dr. David Alan Gilbert]

* 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

Re: [Qemu-devel] Bug in virtio_net_load

[Jason Wang]

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?
>

Re: [Qemu-devel] Bug in virtio_net_load

[Cornelia Huck]

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.>

Re: [Qemu-devel] Bug in virtio_net_load

[Robin Geuze]

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.>
>

Re: [Qemu-devel] Bug in virtio_net_load

[Robin Geuze]

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.>
>