From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
To: Mikhail Sennikovsky <mikhail.sennikovskii@cloud.ionos.com>,
stefanha@redhat.com, jasowang@redhat.com, mst@redhat.com
Cc: qemu-devel@nongnu.org
Subject: Re: [PATCH] virtio-net: prevent offloads reset on migration
Date: Wed, 2 Oct 2019 10:55:38 +0100 [thread overview]
Message-ID: <20191002095538.GA2709@work-vm> (raw)
In-Reply-To: <1569932308-30478-2-git-send-email-mikhail.sennikovskii@cloud.ionos.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
next prev parent reply other threads:[~2019-10-02 9:56 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 [this message]
2019-10-07 10:31 ` Mikhail Sennikovsky
2019-10-10 5:33 ` Jason Wang
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=20191002095538.GA2709@work-vm \
--to=dgilbert@redhat.com \
--cc=jasowang@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.