Re: [PATCH] virtio-net: prevent offloads reset on migration

[Mikhail Sennikovsky]

Hi Jason,

Thank you for your reply.
I've submitted the third version of the patch which does the runstate
check as you propose.

Thanks,
Mikhail

Am Do., 10. Okt. 2019 um 07:33 Uhr schrieb Jason Wang <address@hidden>:
>
>
> On 2019/10/2 下午5:55, Dr. David Alan Gilbert wrote:
> > Copying in Stefan, Jason and Michael who know the virtio details
> >
> > Dave
> >
> > * Mikhail Sennikovsky (address@hidden) wrote:
> >> Currently offloads disabled by guest via the 
> >> VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
> >> command are not preserved on VM migration.
> >> Instead all offloads reported by guest features (via 
> >> VIRTIO_PCI_GUEST_FEATURES)
> >> get enabled.
> >> What happens is: first the VirtIONet::curr_guest_offloads gets restored 
> >> and offloads
> >> are getting set correctly:
> >>
> >>   #0  qemu_set_offload (nc=0x555556a11400, csum=1, tso4=0, tso6=0, ecn=0, 
> >> ufo=0) at net/net.c:474
> >>   #1  virtio_net_apply_guest_offloads (n=0x555557701ca0) at 
> >> hw/net/virtio-net.c:720
> >>   #2  virtio_net_post_load_device (opaque=0x555557701ca0, version_id=11) 
> >> at hw/net/virtio-net.c:2334
> >>   #3  vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577c80 
> >> <vmstate_virtio_net_device>, opaque=0x555557701ca0, version_id=11)
> >>       at migration/vmstate.c:168
> >>   #4  virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) 
> >> at hw/virtio/virtio.c:2197
> >>   #5  virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, 
> >> field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> >>   #6  vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 
> >> <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at 
> >> migration/vmstate.c:143
> >>   #7  vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at 
> >> migration/savevm.c:829
> >>   #8  qemu_loadvm_section_start_full (f=0x5555569dc010, 
> >> mis=0x5555569eee20) at migration/savevm.c:2211
> >>   #9  qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at 
> >> migration/savevm.c:2395
> >>   #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> >>   #11 process_incoming_migration_co (opaque=0x0) at 
> >> migration/migration.c:449
> >>
> >> However later on the features are getting restored, and offloads get reset 
> >> to
> >> everything supported by features:
> >>
> >>   #0  qemu_set_offload (nc=0x555556a11400, csum=1, tso4=1, tso6=1, ecn=0, 
> >> ufo=0) at net/net.c:474
> >>   #1  virtio_net_apply_guest_offloads (n=0x555557701ca0) at 
> >> hw/net/virtio-net.c:720
> >>   #2  virtio_net_set_features (vdev=0x555557701ca0, features=5104441767) 
> >> at hw/net/virtio-net.c:773
> >>   #3  virtio_set_features_nocheck (vdev=0x555557701ca0, val=5104441767) at 
> >> hw/virtio/virtio.c:2052
> >>   #4  virtio_load (vdev=0x555557701ca0, f=0x5555569dc010, version_id=11) 
> >> at hw/virtio/virtio.c:2220
> >>   #5  virtio_device_get (f=0x5555569dc010, opaque=0x555557701ca0, size=0, 
> >> field=0x55555668cd00 <__compound_literal.5>) at hw/virtio/virtio.c:2036
> >>   #6  vmstate_load_state (f=0x5555569dc010, vmsd=0x555556577ce0 
> >> <vmstate_virtio_net>, opaque=0x555557701ca0, version_id=11) at 
> >> migration/vmstate.c:143
> >>   #7  vmstate_load (f=0x5555569dc010, se=0x5555578189e0) at 
> >> migration/savevm.c:829
> >>   #8  qemu_loadvm_section_start_full (f=0x5555569dc010, 
> >> mis=0x5555569eee20) at migration/savevm.c:2211
> >>   #9  qemu_loadvm_state_main (f=0x5555569dc010, mis=0x5555569eee20) at 
> >> migration/savevm.c:2395
> >>   #10 qemu_loadvm_state (f=0x5555569dc010) at migration/savevm.c:2467
> >>   #11 process_incoming_migration_co (opaque=0x0) at 
> >> migration/migration.c:449
> >>
> >> This patch fixes this by adding an extra argument to the set_features 
> >> callback,
> >> specifying whether the offloads are to be reset, and setting it to false
> >> for the migration case.
> >>
> >> Signed-off-by: Mikhail Sennikovsky <address@hidden>
> >> ---
> >>   hw/display/virtio-gpu-base.c |  3 ++-
> >>   hw/net/virtio-net.c          |  5 +++--
> >>   hw/virtio/virtio.c           | 10 +++++-----
> >>   include/hw/virtio/virtio.h   |  2 +-
> >>   4 files changed, 11 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/hw/display/virtio-gpu-base.c b/hw/display/virtio-gpu-base.c
> >> index 55e0799..04d8a23 100644
> >> --- a/hw/display/virtio-gpu-base.c
> >> +++ b/hw/display/virtio-gpu-base.c
> >> @@ -193,7 +193,8 @@ virtio_gpu_base_get_features(VirtIODevice *vdev, 
> >> uint64_t features,
> >>   }
> >>
> >>   static void
> >> -virtio_gpu_base_set_features(VirtIODevice *vdev, uint64_t features)
> >> +virtio_gpu_base_set_features(VirtIODevice *vdev, uint64_t features,
> >> +                               bool reset_offloads)
>
>
> It's not good for e.g gpu to know anything about net.
>
> How about checking runstate and do not call apply_guest_offload() in
> virtio_net_set_features() when in the state of migration?
>
> Thanks
>
>
> >>   {
> >>       static const uint32_t virgl = (1 << VIRTIO_GPU_F_VIRGL);
> >>       VirtIOGPUBase *g = VIRTIO_GPU_BASE(vdev);
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index b9e1cd7..5d108e5 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -743,7 +743,8 @@ static inline uint64_t 
> >> virtio_net_supported_guest_offloads(VirtIONet *n)
> >>       return virtio_net_guest_offloads_by_features(vdev->guest_features);
> >>   }
> >>
> >> -static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features)
> >> +static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features,
> >> +                                        bool reset_offloads)
> >>   {
> >>       VirtIONet *n = VIRTIO_NET(vdev);
> >>       int i;
> >> @@ -767,7 +768,7 @@ static void virtio_net_set_features(VirtIODevice 
> >> *vdev, uint64_t features)
> >>       n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) 
> >> &&
> >>           virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6);
> >>
> >> -    if (n->has_vnet_hdr) {
> >> +    if (reset_offloads && n->has_vnet_hdr) {
> >>           n->curr_guest_offloads =
> >>               virtio_net_guest_offloads_by_features(features);
> >>           virtio_net_apply_guest_offloads(n);
> >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >> index a94ea18..b89f7b0 100644
> >> --- a/hw/virtio/virtio.c
> >> +++ b/hw/virtio/virtio.c
> >> @@ -2042,14 +2042,14 @@ const VMStateInfo  virtio_vmstate_info = {
> >>       .put = virtio_device_put,
> >>   };
> >>
> >> -static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val)
> >> +static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val, 
> >> bool reset_offloads)
> >>   {
> >>       VirtioDeviceClass *k = VIRTIO_DEVICE_GET_CLASS(vdev);
> >>       bool bad = (val & ~(vdev->host_features)) != 0;
> >>
> >>       val &= vdev->host_features;
> >>       if (k->set_features) {
> >> -        k->set_features(vdev, val);
> >> +        k->set_features(vdev, val, reset_offloads);
> >>       }
> >>       vdev->guest_features = val;
> >>       return bad ? -1 : 0;
> >> @@ -2065,7 +2065,7 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t 
> >> val)
> >>       if (vdev->status & VIRTIO_CONFIG_S_FEATURES_OK) {
> >>           return -EINVAL;
> >>       }
> >> -    ret = virtio_set_features_nocheck(vdev, val);
> >> +    ret = virtio_set_features_nocheck(vdev, val, true);
> >>       if (!ret) {
> >>           if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> >>               /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  
> >> */
> >> @@ -2217,14 +2217,14 @@ int virtio_load(VirtIODevice *vdev, QEMUFile *f, 
> >> int version_id)
> >>            * host_features.
> >>            */
> >>           uint64_t features64 = vdev->guest_features;
> >> -        if (virtio_set_features_nocheck(vdev, features64) < 0) {
> >> +        if (virtio_set_features_nocheck(vdev, features64, false) < 0) {
> >>               error_report("Features 0x%" PRIx64 " unsupported. "
> >>                            "Allowed features: 0x%" PRIx64,
> >>                            features64, vdev->host_features);
> >>               return -1;
> >>           }
> >>       } else {
> >> -        if (virtio_set_features_nocheck(vdev, features) < 0) {
> >> +        if (virtio_set_features_nocheck(vdev, features, false) < 0) {
> >>               error_report("Features 0x%x unsupported. "
> >>                            "Allowed features: 0x%" PRIx64,
> >>                            features, vdev->host_features);
> >> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> >> index b189788..fd8cac5 100644
> >> --- a/include/hw/virtio/virtio.h
> >> +++ b/include/hw/virtio/virtio.h
> >> @@ -128,7 +128,7 @@ typedef struct VirtioDeviceClass {
> >>                                uint64_t requested_features,
> >>                                Error **errp);
> >>       uint64_t (*bad_features)(VirtIODevice *vdev);
> >> -    void (*set_features)(VirtIODevice *vdev, uint64_t val);
> >> +    void (*set_features)(VirtIODevice *vdev, uint64_t val, bool 
> >> reset_offloads);
> >>       int (*validate_features)(VirtIODevice *vdev);
> >>       void (*get_config)(VirtIODevice *vdev, uint8_t *config);
> >>       void (*set_config)(VirtIODevice *vdev, const uint8_t *config);
> >> --
> >> 2.7.4
> >>
> >>
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK