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

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

[Mikhail Sennikovsky]

Hi all,

This is the third version of the patch, implementing the approach
based on checking the VM runstate proposed by Jason Wang.

Regards,
Mikhail

Mikhail Sennikovsky (1):
  virtio-net: prevent offloads reset on migration

 hw/net/virtio-net.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.7.4

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

[Mikhail Sennikovsky]

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

Fix this by making the virtio_net_set_features not reset
the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE

Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
---
 hw/net/virtio-net.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index b9e1cd7..49be172 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -767,7 +767,14 @@ 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) {
+    /*
+     * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
+     * is called as part of VM state restore process.
+     * At this stage we do not want the curr_guest_offloads to be reset,
+     * i.e. want to preserve them in the same state as was set
+     * by the guest on the source machine.
+     */
+    if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
         n->curr_guest_offloads =
             virtio_net_guest_offloads_by_features(features);
         virtio_net_apply_guest_offloads(n);
-- 
2.7.4

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/hw/timer/mc146818rtc.o
  CC      aarch64-softmmu/hw/char/virtio-serial-bus.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: implicit declaration of function 'runstate_check' [-Werror=implicit-function-declaration]
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
     ^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function)
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                                            ^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/hw/core/machine-qmp-cmds.o
  CC      aarch64-softmmu/hw/core/numa.o
---
  CC      aarch64-softmmu/hw/dma/pxa2xx_dma.o
  CC      aarch64-softmmu/hw/gpio/omap_gpio.o
  CC      aarch64-softmmu/hw/gpio/imx_gpio.o
make: *** [x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/hw/gpio/bcm2835_gpio.o
  CC      aarch64-softmmu/hw/gpio/nrf51_gpio.o
---
  CC      aarch64-softmmu/hw/arm/highbank.o
  CC      aarch64-softmmu/hw/arm/integratorcp.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: implicit declaration of function 'runstate_check' [-Werror=implicit-function-declaration]
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
     ^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:5: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function)
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                                            ^
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/hw/arm/mainstone.o
make: *** [aarch64-softmmu/all] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=119d37d80ed3480586be3499dfe1868e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-4idbdkch/src/docker-src.2019-10-10-09.12.40.12656:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=119d37d80ed3480586be3499dfe1868e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-4idbdkch/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m22.220s
user    0m8.256s


The full log is available at
http://patchew.org/logs/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[no-reply]

Patchew URL: https://patchew.org/QEMU/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/hw/i386/acpi-build.o
  CC      x86_64-softmmu/monitor/misc.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: implicit declaration of function 'runstate_check'; did you mean 'type_check'? [-Werror=implicit-function-declaration]
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                             ^~~~~~~~~~~~~~
                             type_check
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function); did you mean 'RX_STATE__MAX'?
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                                            ^~~~~~~~~~~~~~~~~~~
                                            RX_STATE__MAX
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
  CC      aarch64-softmmu/hw/input/pxa2xx_keypad.o
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      x86_64-softmmu/qapi/qapi-introspect.o
  CC      aarch64-softmmu/hw/input/tsc210x.o
---
  CC      aarch64-softmmu/hw/arm/fsl-imx6ul.o
  CC      aarch64-softmmu/hw/arm/mcimx6ul-evk.o
/tmp/qemu-test/src/hw/net/virtio-net.c: In function 'virtio_net_set_features':
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: implicit declaration of function 'runstate_check'; did you mean 'type_check'? [-Werror=implicit-function-declaration]
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                             ^~~~~~~~~~~~~~
                             type_check
/tmp/qemu-test/src/hw/net/virtio-net.c:780:29: error: nested extern declaration of 'runstate_check' [-Werror=nested-externs]
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: error: 'RUN_STATE_INMIGRATE' undeclared (first use in this function); did you mean 'RX_STATE__MAX'?
     if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
                                            ^~~~~~~~~~~~~~~~~~~
                                            RX_STATE__MAX
/tmp/qemu-test/src/hw/net/virtio-net.c:780:44: note: each undeclared identifier is reported only once for each function it appears in
cc1: all warnings being treated as errors
make[1]: *** [/tmp/qemu-test/src/rules.mak:69: hw/net/virtio-net.o] Error 1
make[1]: *** Waiting for unfinished jobs....
  CC      aarch64-softmmu/hw/arm/nrf51_soc.o
make: *** [Makefile:482: x86_64-softmmu/all] Error 2
make: *** Waiting for unfinished jobs....
make: *** [Makefile:482: aarch64-softmmu/all] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=13f31a23cdaa43fab126428102954053', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-8i4i6k8j/src/docker-src.2019-10-10-09.15.36.22582:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=13f31a23cdaa43fab126428102954053
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-8i4i6k8j/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m13.813s
user    0m8.487s


The full log is available at
http://patchew.org/logs/1570709475-32073-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky 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
> 
> Fix this by making the virtio_net_set_features not reset
> the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
> 
> Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>

I thought more about it and I really think the problem here is
different: we really should first set features and then apply offloads.

It is simply dumb that we are calling device specific
code before initializing generic virtio.

The logical order of initialization should be:

init base class
init derived class


instead we have it all spread out :(



What to do about it?
Could we maybe teach vmstate to record post load hooks
it needs to run *somewhere*?
Then we could load device specific state and record post load
hooks, then load generic state, run generic post load,
then run device specific post load hooks.

Dave, what do you think?


> ---
>  hw/net/virtio-net.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index b9e1cd7..49be172 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -767,7 +767,14 @@ 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) {
> +    /*
> +     * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
> +     * is called as part of VM state restore process.
> +     * At this stage we do not want the curr_guest_offloads to be reset,
> +     * i.e. want to preserve them in the same state as was set
> +     * by the guest on the source machine.
> +     */
> +    if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
>          n->curr_guest_offloads =
>              virtio_net_guest_offloads_by_features(features);
>          virtio_net_apply_guest_offloads(n);
> -- 
> 2.7.4

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

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky 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
> > 
> > Fix this by making the virtio_net_set_features not reset
> > the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
> > 
> > Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
> 
> I thought more about it and I really think the problem here is
> different: we really should first set features and then apply offloads.
> 
> It is simply dumb that we are calling device specific
> code before initializing generic virtio.
> 
> The logical order of initialization should be:
> 
> init base class
> init derived class
> 
> 
> instead we have it all spread out :(

Yeh; we've hit problems there before.

> 
> What to do about it?
> Could we maybe teach vmstate to record post load hooks
> it needs to run *somewhere*?
> Then we could load device specific state and record post load
> hooks, then load generic state, run generic post load,
> then run device specific post load hooks.
> 
> Dave, what do you think?

I worry about changing all existing virtio devices; who knows
what existing ordering constraints those devices might have on their
post_load's - for example one field in the vmstate might rely on
another post_load having already run and set a flag; that's not
that unusual.

I can imagine having a Notifier* chain; you initialise it at the start
of virtio_load, provide a virtio_load_post_notifier_add(...) and any
post_load that wants delaying can add itself and then you call it
at the right point in virtio_load; but I'd be too scared to change
the ordering of all post_loads of all devices.  You might be
OK to do that for the top level post_load of a virtio device,
because there are only a few to check.

Dave

> 
> 
> > ---
> >  hw/net/virtio-net.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index b9e1cd7..49be172 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -767,7 +767,14 @@ 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) {
> > +    /*
> > +     * In case of RUN_STATE_INMIGRATE the virtio_net_set_features
> > +     * is called as part of VM state restore process.
> > +     * At this stage we do not want the curr_guest_offloads to be reset,
> > +     * i.e. want to preserve them in the same state as was set
> > +     * by the guest on the source machine.
> > +     */
> > +    if (n->has_vnet_hdr && !runstate_check(RUN_STATE_INMIGRATE)) {
> >          n->curr_guest_offloads =
> >              virtio_net_guest_offloads_by_features(features);
> >          virtio_net_apply_guest_offloads(n);
> > -- 
> > 2.7.4
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Michael S. Tsirkin]

On Thu, Oct 10, 2019 at 04:58:09PM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Thu, Oct 10, 2019 at 02:11:15PM +0200, Mikhail Sennikovsky 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
> > > 
> > > Fix this by making the virtio_net_set_features not reset
> > > the curr_guest_offloads in case the VM runstate is RUN_STATE_INMIGRATE
> > > 
> > > Signed-off-by: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>
> > 
> > I thought more about it and I really think the problem here is
> > different: we really should first set features and then apply offloads.
> > 
> > It is simply dumb that we are calling device specific
> > code before initializing generic virtio.
> > 
> > The logical order of initialization should be:
> > 
> > init base class
> > init derived class
> > 
> > 
> > instead we have it all spread out :(
> 
> Yeh; we've hit problems there before.
> 
> > 
> > What to do about it?
> > Could we maybe teach vmstate to record post load hooks
> > it needs to run *somewhere*?
> > Then we could load device specific state and record post load
> > hooks, then load generic state, run generic post load,
> > then run device specific post load hooks.
> > 
> > Dave, what do you think?
> 
> I worry about changing all existing virtio devices; who knows
> what existing ordering constraints those devices might have on their
> post_load's - for example one field in the vmstate might rely on
> another post_load having already run and set a flag; that's not
> that unusual.
> 
> I can imagine having a Notifier* chain; you initialise it at the start
> of virtio_load, provide a virtio_load_post_notifier_add(...) and any
> post_load that wants delaying can add itself and then you call it
> at the right point in virtio_load; but I'd be too scared to change
> the ordering of all post_loads of all devices.  You might be
> OK to do that for the top level post_load of a virtio device,
> because there are only a few to check.
> 
> Dave


Okay so let's just add a new hook for that.
I'll send a patch that does this shortly.
Mikhail, I'll send a couple of untested patches,
please try them - either ack or rework in any way you like.