Re: [PATCH v2 4/4] virtio-net: Add support for USO features

[Peter Xu <peterx@redhat.com>]

On Tue, Aug 01, 2023 at 01:31:48AM +0300, Yuri Benditovich wrote:
> USO features of virtio-net device depend on kernel ability
> to support them, for backward compatibility by default the
> features are disabled on 8.0 and earlier.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> Signed-off-by: Andrew Melnychecnko <andrew@daynix.com>

Looks like this patch broke migration when the VM starts on a host that has
USO supported, to another host that doesn't..

Yuri, would it be possible we always keep all the USO* features off by
default (so this feature bit never affects migration ABI), but then:

  - only enable them when the user specified ON

  - meanwhile, if detecting host feature doesn't support USO*, it could
    fail qemu from boot, rather than silently turning it from ON->OFF

?

Silently flipping the bit may cause migration issues like this.

Or any suggestion on how to fix migration?

Thanks,

> ---
>  hw/core/machine.c   |  4 ++++
>  hw/net/virtio-net.c | 31 +++++++++++++++++++++++++++++--
>  2 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index f0d35c6401..a725e76738 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -38,10 +38,14 @@
>  #include "exec/confidential-guest-support.h"
>  #include "hw/virtio/virtio.h"
>  #include "hw/virtio/virtio-pci.h"
> +#include "hw/virtio/virtio-net.h"
>  
>  GlobalProperty hw_compat_8_0[] = {
>      { "migration", "multifd-flush-after-each-section", "on"},
>      { TYPE_PCI_DEVICE, "x-pcie-ari-nextfn-1", "on" },
> +    { TYPE_VIRTIO_NET, "host_uso", "off"},
> +    { TYPE_VIRTIO_NET, "guest_uso4", "off"},
> +    { TYPE_VIRTIO_NET, "guest_uso6", "off"},
>  };
>  const size_t hw_compat_8_0_len = G_N_ELEMENTS(hw_compat_8_0);
>  
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index d2311e7d6e..bd0ead94fe 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -659,6 +659,15 @@ static int peer_has_ufo(VirtIONet *n)
>      return n->has_ufo;
>  }
>  
> +static int peer_has_uso(VirtIONet *n)
> +{
> +    if (!peer_has_vnet_hdr(n)) {
> +        return 0;
> +    }
> +
> +    return qemu_has_uso(qemu_get_queue(n->nic)->peer);
> +}
> +
>  static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int mergeable_rx_bufs,
>                                         int version_1, int hash_report)
>  {
> @@ -796,6 +805,10 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_TSO6);
>          virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_ECN);
>  
> +        virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
> +
>          virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT);
>      }
>  
> @@ -804,6 +817,12 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>          virtio_clear_feature(&features, VIRTIO_NET_F_HOST_UFO);
>      }
>  
> +    if (!peer_has_uso(n)) {
> +        virtio_clear_feature(&features, VIRTIO_NET_F_HOST_USO);
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO4);
> +        virtio_clear_feature(&features, VIRTIO_NET_F_GUEST_USO6);
> +    }
> +
>      if (!get_vhost_net(nc->peer)) {
>          return features;
>      }
> @@ -864,14 +883,16 @@ static void virtio_net_apply_guest_offloads(VirtIONet *n)
>              !!(n->curr_guest_offloads & (1ULL << VIRTIO_NET_F_GUEST_USO6)));
>  }
>  
> -static uint64_t virtio_net_guest_offloads_by_features(uint32_t features)
> +static uint64_t virtio_net_guest_offloads_by_features(uint64_t features)
>  {
>      static const uint64_t guest_offloads_mask =
>          (1ULL << VIRTIO_NET_F_GUEST_CSUM) |
>          (1ULL << VIRTIO_NET_F_GUEST_TSO4) |
>          (1ULL << VIRTIO_NET_F_GUEST_TSO6) |
>          (1ULL << VIRTIO_NET_F_GUEST_ECN)  |
> -        (1ULL << VIRTIO_NET_F_GUEST_UFO);
> +        (1ULL << VIRTIO_NET_F_GUEST_UFO)  |
> +        (1ULL << VIRTIO_NET_F_GUEST_USO4) |
> +        (1ULL << VIRTIO_NET_F_GUEST_USO6);
>  
>      return guest_offloads_mask & features;
>  }
> @@ -3924,6 +3945,12 @@ static Property virtio_net_properties[] = {
>      DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN),
>      DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str),
>      DEFINE_PROP_BOOL("failover", VirtIONet, failover, false),
> +    DEFINE_PROP_BIT64("guest_uso4", VirtIONet, host_features,
> +                      VIRTIO_NET_F_GUEST_USO4, true),
> +    DEFINE_PROP_BIT64("guest_uso6", VirtIONet, host_features,
> +                      VIRTIO_NET_F_GUEST_USO6, true),
> +    DEFINE_PROP_BIT64("host_uso", VirtIONet, host_features,
> +                      VIRTIO_NET_F_HOST_USO, true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> -- 
> 2.34.3
> 
> 

-- 
Peter Xu