On Fri, Jul 26, 2024 at 6:19 AM Peter Xu <pet...@redhat.com> wrote:
>
> 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.benditov...@daynix.com>
> > Signed-off-by: Andrew Melnychecnko <and...@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
>
> ?
I agree, I have raised the same issue several times in the past.
>
> Silently flipping the bit may cause migration issues like this.
Looking at virtio_net_get_features(), it silently clears a lot of features...
Thanks
>
> 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
>
