Re: [PATCH] virtio-net: not enable vq reset feature unconditionally

[Jason Wang]

On Sun, May 7, 2023 at 2:01 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, May 06, 2023 at 10:13:36AM +0800, Xuan Zhuo wrote:
> > On Thu,  4 May 2023 12:14:47 +0200, =?utf-8?q?Eugenio_P=C3=A9rez?= 
> > <eperezma@redhat.com> wrote:
> > > The commit 93a97dc5200a ("virtio-net: enable vq reset feature") enables
> > > unconditionally vq reset feature as long as the device is emulated.
> > > This makes impossible to actually disable the feature, and it causes
> > > migration problems from qemu version previous than 7.2.
> > >
> > > The entire final commit is unneeded as device system already enable or
> > > disable the feature properly.
> > >
> > > This reverts commit 93a97dc5200a95e63b99cb625f20b7ae802ba413.
> > > Fixes: 93a97dc5200a ("virtio-net: enable vq reset feature")
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >
> > > ---
> > > Tested by checking feature bit at  /sys/devices/pci.../virtio0/features
> > > enabling and disabling queue_reset virtio-net feature and vhost=on/off
> > > on net device backend.
> >
> > Do you mean that this feature cannot be closed?
> >
> > I tried to close in the guest, it was successful.
> >
> > In addition, in this case, could you try to repair the problem instead of
> > directly revert.
> >
> > Thanks.
>
> What does you patch accomplish though? If it's not needed
> let's not do it.

It looks to me the unconditional set of this feature breaks the
migration of pre 7.2 machines.

Also we probably need to make ring_reset as false by default, or
compat it for pre 7.2 machines.

    DEFINE_PROP_BIT64("queue_reset", _state, _field, \
                      VIRTIO_F_RING_RESET, true)


Thanks

>
> > > ---
> > >  hw/net/virtio-net.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > > index 53e1c32643..4ea33b6e2e 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -805,7 +805,6 @@ static uint64_t virtio_net_get_features(VirtIODevice 
> > > *vdev, uint64_t features,
> > >      }
> > >
> > >      if (!get_vhost_net(nc->peer)) {
> > > -        virtio_add_feature(&features, VIRTIO_F_RING_RESET);
> > >          return features;
> > >      }
> > >
> > > --
> > > 2.31.1
> > >
>