hw/net/virtio-net.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
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
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
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
* 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
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.
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
© 2016 - 2024 Red Hat, Inc.