From: Jason Wang <jasowang@redhat.com>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>,
Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>,
stefanha@redhat.com, mst@redhat.com
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH] virtio-net: prevent offloads reset on migration
Date: Thu, 10 Oct 2019 13:33:44 +0800 [thread overview]
Message-ID: <778087fb-6d91-3f63-18cb-78cab6a68f77@redhat.com> (raw)
In-Reply-To: <20191002095538.GA2709@work-vm>
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
next prev parent reply other threads:[~2019-10-10 5:34 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2019-10-10 12:15 ` Mikhail Sennikovsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=778087fb-6d91-3f63-18cb-78cab6a68f77@redhat.com \
--to=jasowang@redhat.com \
--cc=dgilbert@redhat.com \
--cc=mikhail.sennikovskii@cloud.ionos.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).