| Message ID | 20230504101447.389398-1-eperezma@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | virtio-net: not enable vq reset feature unconditionally | expand |
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 >
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 > >
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 > > > >
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 > > > > > >
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 > > > > > > >
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!
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! >
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?
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!
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?
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 >
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!
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; }
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(-)