* [PATCH] virtio-net: prevent offloads reset on migration
@ 2019-10-01 12:18 Mikhail Sennikovsky
2019-10-01 12:18 ` Mikhail Sennikovsky
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-01 12:18 UTC (permalink / raw)
To: qemu-devel; +Cc: Mikhail Sennikovsky
Hi all,
I've recently faced an issue that the offloads settings of my Windows VM's
tap devices reported with "ethtool -k" get changed after the VM migration.
After a quick look it appears that the offloads always get resent
to the complete set of features the guest has reported.
Here I'm sending a small patch which fixes this with all the detail
in the commit description.
Perhaps a cleaner approach would've been to actually restore the
curr_guest_offloads after the virtio guest device features.
This approach is much less invasive though, and more intended
to illustrate the problem and potentially initiate a further
discussion on the best way of fixing it.
Thanks & Regards,
Mikhail
Mikhail Sennikovsky (1):
virtio-net: prevent offloads reset on migration
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(-)
--
2.7.4
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] virtio-net: prevent offloads reset on migration
2019-10-01 12:18 [PATCH] virtio-net: prevent offloads reset on migration Mikhail Sennikovsky
@ 2019-10-01 12:18 ` Mikhail Sennikovsky
2019-10-01 16:14 ` no-reply
2019-10-02 9:55 ` Dr. David Alan Gilbert
0 siblings, 2 replies; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-01 12:18 UTC (permalink / raw)
To: qemu-devel; +Cc: 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
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 <mikhail.sennikovskii@cloud.ionos.com>
---
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)
{
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-net: prevent offloads reset on migration
2019-10-01 12:18 ` Mikhail Sennikovsky
@ 2019-10-01 16:14 ` no-reply
2019-10-02 9:55 ` Dr. David Alan Gilbert
1 sibling, 0 replies; 7+ messages in thread
From: no-reply @ 2019-10-01 16:14 UTC (permalink / raw)
To: mikhail.sennikovskii; +Cc: mikhail.sennikovskii, qemu-devel
Patchew URL: https://patchew.org/QEMU/1569932308-30478-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 1569932308-30478-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com
Subject: [PATCH] virtio-net: prevent offloads reset on migration
=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
9d2774e virtio-net: prevent offloads reset on migration
=== OUTPUT BEGIN ===
ERROR: line over 90 characters
#97: FILE: hw/virtio/virtio.c:2049:
+static int virtio_set_features_nocheck(VirtIODevice *vdev, uint64_t val, bool reset_offloads)
total: 1 errors, 0 warnings, 74 lines checked
Commit 9d2774ea7eb1 (virtio-net: prevent offloads reset on migration) has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/1569932308-30478-2-git-send-email-mikhail.sennikovskii@cloud.ionos.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-net: prevent offloads reset on migration
2019-10-01 12:18 ` Mikhail Sennikovsky
2019-10-01 16:14 ` no-reply
@ 2019-10-02 9:55 ` Dr. David Alan Gilbert
2019-10-07 10:31 ` Mikhail Sennikovsky
2019-10-10 5:33 ` Jason Wang
1 sibling, 2 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2019-10-02 9:55 UTC (permalink / raw)
To: Mikhail Sennikovsky, stefanha, jasowang, mst; +Cc: qemu-devel
Copying in Stefan, Jason and Michael who know the virtio details
Dave
* Mikhail Sennikovsky (mikhail.sennikovskii@cloud.ionos.com) 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 <mikhail.sennikovskii@cloud.ionos.com>
> ---
> 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)
> {
> 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 / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-net: prevent offloads reset on migration
2019-10-02 9:55 ` Dr. David Alan Gilbert
@ 2019-10-07 10:31 ` Mikhail Sennikovsky
2019-10-10 5:33 ` Jason Wang
1 sibling, 0 replies; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-07 10:31 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: jasowang, qemu-devel, stefanha, mst
Guys, any update on this? Note that I have also sent a second version
of the patch here, which has the
coding style issue fixed.
Thanks,
Mikhail
Am Mi., 2. Okt. 2019 um 11:55 Uhr schrieb Dr. David Alan Gilbert
<dgilbert@redhat.com>:
>
> Copying in Stefan, Jason and Michael who know the virtio details
>
> Dave
>
> * Mikhail Sennikovsky (mikhail.sennikovskii@cloud.ionos.com) 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 <mikhail.sennikovskii@cloud.ionos.com>
> > ---
> > 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)
> > {
> > 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 / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-net: prevent offloads reset on migration
2019-10-02 9:55 ` Dr. David Alan Gilbert
2019-10-07 10:31 ` Mikhail Sennikovsky
@ 2019-10-10 5:33 ` Jason Wang
2019-10-10 12:15 ` Mikhail Sennikovsky
1 sibling, 1 reply; 7+ messages in thread
From: Jason Wang @ 2019-10-10 5:33 UTC (permalink / raw)
To: Dr. David Alan Gilbert, Mikhail Sennikovsky, stefanha, mst; +Cc: qemu-devel
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 (mikhail.sennikovskii@cloud.ionos.com) 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 <mikhail.sennikovskii@cloud.ionos.com>
>> ---
>> 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 / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] virtio-net: prevent offloads reset on migration
2019-10-10 5:33 ` Jason Wang
@ 2019-10-10 12:15 ` Mikhail Sennikovsky
0 siblings, 0 replies; 7+ messages in thread
From: Mikhail Sennikovsky @ 2019-10-10 12:15 UTC (permalink / raw)
To: Jason Wang; +Cc: qemu-devel, Dr. David Alan Gilbert, stefanha, mst
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 <jasowang@redhat.com>:
>
>
> 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 (mikhail.sennikovskii@cloud.ionos.com) 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 <mikhail.sennikovskii@cloud.ionos.com>
> >> ---
> >> 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 / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-10-10 12:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 12:18 [PATCH] virtio-net: prevent offloads reset on migration Mikhail Sennikovsky
2019-10-01 12:18 ` Mikhail Sennikovsky
2019-10-01 16:14 ` no-reply
2019-10-02 9:55 ` Dr. David Alan Gilbert
2019-10-07 10:31 ` Mikhail Sennikovsky
2019-10-10 5:33 ` Jason Wang
2019-10-10 12:15 ` Mikhail Sennikovsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).