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

[Xuan Zhuo]

On Mon, 8 May 2023 14:44:21 +0800, Jason Wang <jasowang@redhat.com> wrote:
> 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)
>


Do you mean this?

Thanks


commit 69e1c14aa22284f933a6ea134b96d5cb5a88a94d
Author: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Date:   Mon Oct 17 17:25:47 2022 +0800

    virtio: core: vq reset feature negotation support

    A a new command line parameter "queue_reset" is added.

    Meanwhile, the vq reset feature is disabled for pre-7.2 machines.

    Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
    Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
    Acked-by: Jason Wang <jasowang@redhat.com>
    Message-Id: <20221017092558.111082-5-xuanzhuo@linux.alibaba.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

diff --git a/hw/core/machine.c b/hw/core/machine.c
index aa520e74a8..907fa78ff0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -40,7 +40,9 @@
 #include "hw/virtio/virtio-pci.h"
 #include "qom/object_interfaces.h"

-GlobalProperty hw_compat_7_1[] = {};
+GlobalProperty hw_compat_7_1[] = {
+    { "virtio-device", "queue_reset", "false" },
+};
 const size_t hw_compat_7_1_len = G_N_ELEMENTS(hw_compat_7_1);

 GlobalProperty hw_compat_7_0[] = {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index b00b3fcf31..1423dba379 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -313,7 +313,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
     DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
                       VIRTIO_F_IOMMU_PLATFORM, false), \
     DEFINE_PROP_BIT64("packed", _state, _field, \
-                      VIRTIO_F_RING_PACKED, false)
+                      VIRTIO_F_RING_PACKED, false), \
+    DEFINE_PROP_BIT64("queue_reset", _state, _field, \
+                      VIRTIO_F_RING_RESET, true)

 hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);
 bool virtio_queue_enabled_legacy(VirtIODevice *vdev, int n);


>
> 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
> > > >
> >
>