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

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

[Eugenio Pérez]

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

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

[Xuan Zhuo]

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.

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

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

[Michael S. Tsirkin]

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.

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

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

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

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 02:44:21PM +0800, Jason Wang 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)
> 
> 
> Thanks

Compat I am guessing. Eugenio?

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

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

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

[Eugenio Perez Martin]

On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
>

I'm not sure what you mean with close. If the device dataplane is
emulated in qemu (vhost=off), I'm not able to make the device not
offer it.

> In addition, in this case, could you try to repair the problem instead of
> directly revert.
>

I'm not following this. The revert is not to always disable the feature.

By default, the feature is enabled. If cmdline states queue_reset=on,
the feature is enabled. That is true both before and after applying
this patch.

However, in qemu master, queue_reset=off keeps enabling this feature
on the device. It happens that there is a commit explicitly doing
that, so I'm reverting it.

Let me know if that makes sense to you.

Thanks!

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

[Xuan Zhuo]

On Mon, 8 May 2023 11:09:46 +0200, Eugenio Perez Martin <eperezma@redhat.com> wrote:
> On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> >
>
> I'm not sure what you mean with close. If the device dataplane is
> emulated in qemu (vhost=off), I'm not able to make the device not
> offer it.
>
> > In addition, in this case, could you try to repair the problem instead of
> > directly revert.
> >
>
> I'm not following this. The revert is not to always disable the feature.
>
> By default, the feature is enabled. If cmdline states queue_reset=on,
> the feature is enabled. That is true both before and after applying
> this patch.
>
> However, in qemu master, queue_reset=off keeps enabling this feature
> on the device. It happens that there is a commit explicitly doing
> that, so I'm reverting it.
>
> Let me know if that makes sense to you.

I got it.

Looks like it's indeed a bug.

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

Thanks.


>
> Thanks!
>

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

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> >
> 
> I'm not sure what you mean with close. If the device dataplane is
> emulated in qemu (vhost=off), I'm not able to make the device not
> offer it.
> 
> > In addition, in this case, could you try to repair the problem instead of
> > directly revert.
> >
> 
> I'm not following this. The revert is not to always disable the feature.
> 
> By default, the feature is enabled. If cmdline states queue_reset=on,
> the feature is enabled. That is true both before and after applying
> this patch.
> 
> However, in qemu master, queue_reset=off keeps enabling this feature
> on the device. It happens that there is a commit explicitly doing
> that, so I'm reverting it.
> 
> Let me know if that makes sense to you.
> 
> Thanks!


question is this:

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



don't we need compat for 7.2 and back for this property?

-- 
MST

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

[Eugenio Perez Martin]

On Mon, May 8, 2023 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> > On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> > >
> >
> > I'm not sure what you mean with close. If the device dataplane is
> > emulated in qemu (vhost=off), I'm not able to make the device not
> > offer it.
> >
> > > In addition, in this case, could you try to repair the problem instead of
> > > directly revert.
> > >
> >
> > I'm not following this. The revert is not to always disable the feature.
> >
> > By default, the feature is enabled. If cmdline states queue_reset=on,
> > the feature is enabled. That is true both before and after applying
> > this patch.
> >
> > However, in qemu master, queue_reset=off keeps enabling this feature
> > on the device. It happens that there is a commit explicitly doing
> > that, so I'm reverting it.
> >
> > Let me know if that makes sense to you.
> >
> > Thanks!
>
>
> question is this:
>
>     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
>                       VIRTIO_F_RING_RESET, true)
>
>
>
> don't we need compat for 7.2 and back for this property?
>

I think that part is already covered by commit 69e1c14aa222 ("virtio:
core: vq reset feature negotation support"). In that regard, maybe we
can simplify the patch message simply stating that queue_reset=off
does not work.

Thanks!

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

[Michael S. Tsirkin]

On Mon, May 08, 2023 at 07:31:35PM +0200, Eugenio Perez Martin wrote:
> On Mon, May 8, 2023 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> > > On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> > > >
> > >
> > > I'm not sure what you mean with close. If the device dataplane is
> > > emulated in qemu (vhost=off), I'm not able to make the device not
> > > offer it.
> > >
> > > > In addition, in this case, could you try to repair the problem instead of
> > > > directly revert.
> > > >
> > >
> > > I'm not following this. The revert is not to always disable the feature.
> > >
> > > By default, the feature is enabled. If cmdline states queue_reset=on,
> > > the feature is enabled. That is true both before and after applying
> > > this patch.
> > >
> > > However, in qemu master, queue_reset=off keeps enabling this feature
> > > on the device. It happens that there is a commit explicitly doing
> > > that, so I'm reverting it.
> > >
> > > Let me know if that makes sense to you.
> > >
> > > Thanks!
> >
> >
> > question is this:
> >
> >     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> >                       VIRTIO_F_RING_RESET, true)
> >
> >
> >
> > don't we need compat for 7.2 and back for this property?
> >
> 
> I think that part is already covered by commit 69e1c14aa222 ("virtio:
> core: vq reset feature negotation support"). In that regard, maybe we
> can simplify the patch message simply stating that queue_reset=off
> does not work.
> 
> Thanks!

that compat for 7.1 and not 7.2 though? is that correct?

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

[Jason Wang]

On Tue, May 9, 2023 at 1:32 AM Eugenio Perez Martin <eperezma@redhat.com> wrote:
>
> On Mon, May 8, 2023 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> > > On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> > > >
> > >
> > > I'm not sure what you mean with close. If the device dataplane is
> > > emulated in qemu (vhost=off), I'm not able to make the device not
> > > offer it.
> > >
> > > > In addition, in this case, could you try to repair the problem instead of
> > > > directly revert.
> > > >
> > >
> > > I'm not following this. The revert is not to always disable the feature.
> > >
> > > By default, the feature is enabled. If cmdline states queue_reset=on,
> > > the feature is enabled. That is true both before and after applying
> > > this patch.
> > >
> > > However, in qemu master, queue_reset=off keeps enabling this feature
> > > on the device. It happens that there is a commit explicitly doing
> > > that, so I'm reverting it.
> > >
> > > Let me know if that makes sense to you.
> > >
> > > Thanks!
> >
> >
> > question is this:
> >
> >     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> >                       VIRTIO_F_RING_RESET, true)
> >
> >
> >
> > don't we need compat for 7.2 and back for this property?
> >
>
> I think that part is already covered by commit 69e1c14aa222 ("virtio:
> core: vq reset feature negotation support"). In that regard, maybe we
> can simplify the patch message simply stating that queue_reset=off
> does not work.
>
> Thanks!

Ack

Thanks

>

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

[Eugenio Perez Martin]

On Mon, May 8, 2023 at 8:11 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, May 08, 2023 at 07:31:35PM +0200, Eugenio Perez Martin wrote:
> > On Mon, May 8, 2023 at 12:22 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, May 08, 2023 at 11:09:46AM +0200, Eugenio Perez Martin wrote:
> > > > On Sat, May 6, 2023 at 4:25 AM Xuan Zhuo <xuanzhuo@linux.alibaba.com> 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.
> > > > >
> > > >
> > > > I'm not sure what you mean with close. If the device dataplane is
> > > > emulated in qemu (vhost=off), I'm not able to make the device not
> > > > offer it.
> > > >
> > > > > In addition, in this case, could you try to repair the problem instead of
> > > > > directly revert.
> > > > >
> > > >
> > > > I'm not following this. The revert is not to always disable the feature.
> > > >
> > > > By default, the feature is enabled. If cmdline states queue_reset=on,
> > > > the feature is enabled. That is true both before and after applying
> > > > this patch.
> > > >
> > > > However, in qemu master, queue_reset=off keeps enabling this feature
> > > > on the device. It happens that there is a commit explicitly doing
> > > > that, so I'm reverting it.
> > > >
> > > > Let me know if that makes sense to you.
> > > >
> > > > Thanks!
> > >
> > >
> > > question is this:
> > >
> > >     DEFINE_PROP_BIT64("queue_reset", _state, _field, \
> > >                       VIRTIO_F_RING_RESET, true)
> > >
> > >
> > >
> > > don't we need compat for 7.2 and back for this property?
> > >
> >
> > I think that part is already covered by commit 69e1c14aa222 ("virtio:
> > core: vq reset feature negotation support"). In that regard, maybe we
> > can simplify the patch message simply stating that queue_reset=off
> > does not work.
> >
> > Thanks!
>
> that compat for 7.1 and not 7.2 though? is that correct?
>

Yes. queue_reset support was added for 7.2 and there are cases where
it can be on or off, like using vhost=on.

If a new rhel 7.2 guest starts with cmdline vhost=off queue_reset=off,
both the guest and device model still see queue_reset=on, so they will
migrate with queue_reset=on. In the case of a migration to a current
qemu master with cmdline vhost=off queue_reset=off, dst qemu will
complain and block the migration (untested).

I think this is the least bad of all the bad behaviors, as a sudden
change to honor cmdline will cause a change of the device features
that the guest sees. I'm not sure if there are better ways to
accomplish this.

Maybe I'm missing something?

Thanks!