diff mbox series

[PULL,v4,30/83] virtio: core: vq reset feature negotation support

Message ID 20221107224600.934080-31-mst@redhat.com (mailing list archive)
State New, archived
Headers show
Series [PULL,v4,01/83] hw/i386/e820: remove legacy reserved entries for e820 | expand

Commit Message

Michael S. Tsirkin Nov. 7, 2022, 10:50 p.m. UTC 
From: Kangjie Xu <kangjie.xu@linux.alibaba.com>

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>
---
 include/hw/virtio/virtio.h | 4 +++-
 hw/core/machine.c          | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Stefano Garzarella Nov. 18, 2022, 2:32 p.m. UTC | #1 
Hi,
starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature 
negotation support"), vhost-user-vsock and vhost-vsock fails while 
setting the device features, because VIRTIO_F_RING_RESET is not masked.

I'm not sure vsock is the only one affected.

We could fix in two ways:

1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:

diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 29b9ab4f72..e671cc695f 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -21,6 +21,7 @@
 
 const int feature_bits[] = {
     VIRTIO_VSOCK_F_SEQPACKET,
+    VIRTIO_F_RING_RESET,
     VHOST_INVALID_FEATURE_BIT
 };


2) Or using directly the features of the device. That way we also handle 
other features that we may have already had to mask but never did.

diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index 29b9ab4f72..41a665082c 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -33,7 +33,7 @@ uint64_t vhost_vsock_common_get_features(VirtIODevice *vdev, uint64_t features,
         virtio_add_feature(&features, VIRTIO_VSOCK_F_SEQPACKET);
     }

-    features = vhost_get_features(&vvc->vhost_dev, feature_bits, features);
+    features &= vvc->vhost_dev.features;

     if (vvc->seqpacket == ON_OFF_AUTO_ON &&
         !virtio_has_feature(features, VIRTIO_VSOCK_F_SEQPACKET)) {


I may be missing the real reason for calling vhost_get_features(), 
though.

@Michael what do you recommend we do?

Thanks,
Stefano

On Tue, Nov 8, 2022 at 12:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
>
> 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>
> ---
>  include/hw/virtio/virtio.h | 4 +++-
>  hw/core/machine.c          | 4 +++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> 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);
> 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[] = {
> --
> MST
>
>
Stefano Garzarella Nov. 18, 2022, 2:39 p.m. UTC | #2 
On Fri, Nov 18, 2022 at 3:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> Hi,
> starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature
> negotation support"), vhost-user-vsock and vhost-vsock fails while
> setting the device features, because VIRTIO_F_RING_RESET is not masked.

vhost-vsock issue also reported here:
https://gitlab.com/qemu-project/qemu/-/issues/1318

>
> I'm not sure vsock is the only one affected.
>
> We could fix in two ways:
>
> 1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:
>
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 29b9ab4f72..e671cc695f 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -21,6 +21,7 @@
>
>  const int feature_bits[] = {
>      VIRTIO_VSOCK_F_SEQPACKET,
> +    VIRTIO_F_RING_RESET,
>      VHOST_INVALID_FEATURE_BIT
>  };
>
>
> 2) Or using directly the features of the device. That way we also handle
> other features that we may have already had to mask but never did.
>
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 29b9ab4f72..41a665082c 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -33,7 +33,7 @@ uint64_t vhost_vsock_common_get_features(VirtIODevice *vdev, uint64_t features,
>          virtio_add_feature(&features, VIRTIO_VSOCK_F_SEQPACKET);
>      }
>
> -    features = vhost_get_features(&vvc->vhost_dev, feature_bits, features);
> +    features &= vvc->vhost_dev.features;
>
>      if (vvc->seqpacket == ON_OFF_AUTO_ON &&
>          !virtio_has_feature(features, VIRTIO_VSOCK_F_SEQPACKET)) {
>
>
> I may be missing the real reason for calling vhost_get_features(),
> though.
>
> @Michael what do you recommend we do?
>
> Thanks,
> Stefano
>
> On Tue, Nov 8, 2022 at 12:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >
> > 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>
> > ---
> >  include/hw/virtio/virtio.h | 4 +++-
> >  hw/core/machine.c          | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > 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);
> > 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[] = {
> > --
> > MST
> >
> >
Michael S. Tsirkin Nov. 19, 2022, 5:19 p.m. UTC | #3 
On Fri, Nov 18, 2022 at 03:32:56PM +0100, Stefano Garzarella wrote:
> Hi,
> starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature 
> negotation support"), vhost-user-vsock and vhost-vsock fails while 
> setting the device features, because VIRTIO_F_RING_RESET is not masked.
> 
> I'm not sure vsock is the only one affected.
> 
> We could fix in two ways:
> 
> 1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:
> 
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 29b9ab4f72..e671cc695f 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -21,6 +21,7 @@
>  
>  const int feature_bits[] = {
>      VIRTIO_VSOCK_F_SEQPACKET,
> +    VIRTIO_F_RING_RESET,
>      VHOST_INVALID_FEATURE_BIT
>  };
> 

Let's do this, we need to be conservative.


> 2) Or using directly the features of the device. That way we also handle 
> other features that we may have already had to mask but never did.
> 
> diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> index 29b9ab4f72..41a665082c 100644
> --- a/hw/virtio/vhost-vsock-common.c
> +++ b/hw/virtio/vhost-vsock-common.c
> @@ -33,7 +33,7 @@ uint64_t vhost_vsock_common_get_features(VirtIODevice *vdev, uint64_t features,
>          virtio_add_feature(&features, VIRTIO_VSOCK_F_SEQPACKET);
>      }
> 
> -    features = vhost_get_features(&vvc->vhost_dev, feature_bits, features);
> +    features &= vvc->vhost_dev.features;
> 
>      if (vvc->seqpacket == ON_OFF_AUTO_ON &&
>          !virtio_has_feature(features, VIRTIO_VSOCK_F_SEQPACKET)) {
> 
> 
> I may be missing the real reason for calling vhost_get_features(), 
> though.
> 
> @Michael what do you recommend we do?
> 
> Thanks,
> Stefano
> 
> On Tue, Nov 8, 2022 at 12:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> >
> > 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>
> > ---
> >  include/hw/virtio/virtio.h | 4 +++-
> >  hw/core/machine.c          | 4 +++-
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> >
> > 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);
> > 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[] = {
> > --
> > MST
> >
> >
Jason Wang Nov. 21, 2022, 6:17 a.m. UTC | #4 
On Sun, Nov 20, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Nov 18, 2022 at 03:32:56PM +0100, Stefano Garzarella wrote:
> > Hi,
> > starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature
> > negotation support"), vhost-user-vsock and vhost-vsock fails while
> > setting the device features, because VIRTIO_F_RING_RESET is not masked.
> >
> > I'm not sure vsock is the only one affected.
> >
> > We could fix in two ways:
> >
> > 1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:
> >
> > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > index 29b9ab4f72..e671cc695f 100644
> > --- a/hw/virtio/vhost-vsock-common.c
> > +++ b/hw/virtio/vhost-vsock-common.c
> > @@ -21,6 +21,7 @@
> >
> >  const int feature_bits[] = {
> >      VIRTIO_VSOCK_F_SEQPACKET,
> > +    VIRTIO_F_RING_RESET,
> >      VHOST_INVALID_FEATURE_BIT
> >  };
> >
>
> Let's do this, we need to be conservative.

Ack.

Thanks

>
>
> > 2) Or using directly the features of the device. That way we also handle
> > other features that we may have already had to mask but never did.
> >
> > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > index 29b9ab4f72..41a665082c 100644
> > --- a/hw/virtio/vhost-vsock-common.c
> > +++ b/hw/virtio/vhost-vsock-common.c
> > @@ -33,7 +33,7 @@ uint64_t vhost_vsock_common_get_features(VirtIODevice *vdev, uint64_t features,
> >          virtio_add_feature(&features, VIRTIO_VSOCK_F_SEQPACKET);
> >      }
> >
> > -    features = vhost_get_features(&vvc->vhost_dev, feature_bits, features);
> > +    features &= vvc->vhost_dev.features;
> >
> >      if (vvc->seqpacket == ON_OFF_AUTO_ON &&
> >          !virtio_has_feature(features, VIRTIO_VSOCK_F_SEQPACKET)) {
> >
> >
> > I may be missing the real reason for calling vhost_get_features(),
> > though.
> >
> > @Michael what do you recommend we do?
> >
> > Thanks,
> > Stefano
> >
> > On Tue, Nov 8, 2022 at 12:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > >
> > > 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>
> > > ---
> > >  include/hw/virtio/virtio.h | 4 +++-
> > >  hw/core/machine.c          | 4 +++-
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > >
> > > 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);
> > > 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[] = {
> > > --
> > > MST
> > >
> > >
>
Michael S. Tsirkin Nov. 21, 2022, 7:07 a.m. UTC | #5 
On Mon, Nov 21, 2022 at 02:17:02PM +0800, Jason Wang wrote:
> On Sun, Nov 20, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Nov 18, 2022 at 03:32:56PM +0100, Stefano Garzarella wrote:
> > > Hi,
> > > starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature
> > > negotation support"), vhost-user-vsock and vhost-vsock fails while
> > > setting the device features, because VIRTIO_F_RING_RESET is not masked.
> > >
> > > I'm not sure vsock is the only one affected.
> > >
> > > We could fix in two ways:
> > >
> > > 1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:
> > >
> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > > index 29b9ab4f72..e671cc695f 100644
> > > --- a/hw/virtio/vhost-vsock-common.c
> > > +++ b/hw/virtio/vhost-vsock-common.c
> > > @@ -21,6 +21,7 @@
> > >
> > >  const int feature_bits[] = {
> > >      VIRTIO_VSOCK_F_SEQPACKET,
> > > +    VIRTIO_F_RING_RESET,
> > >      VHOST_INVALID_FEATURE_BIT
> > >  };
> > >
> >
> > Let's do this, we need to be conservative.
> 
> Ack.
> 
> Thanks


Patch pls? Stefano?

> >
> >
> > > 2) Or using directly the features of the device. That way we also handle
> > > other features that we may have already had to mask but never did.
> > >
> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
> > > index 29b9ab4f72..41a665082c 100644
> > > --- a/hw/virtio/vhost-vsock-common.c
> > > +++ b/hw/virtio/vhost-vsock-common.c
> > > @@ -33,7 +33,7 @@ uint64_t vhost_vsock_common_get_features(VirtIODevice *vdev, uint64_t features,
> > >          virtio_add_feature(&features, VIRTIO_VSOCK_F_SEQPACKET);
> > >      }
> > >
> > > -    features = vhost_get_features(&vvc->vhost_dev, feature_bits, features);
> > > +    features &= vvc->vhost_dev.features;
> > >
> > >      if (vvc->seqpacket == ON_OFF_AUTO_ON &&
> > >          !virtio_has_feature(features, VIRTIO_VSOCK_F_SEQPACKET)) {
> > >
> > >
> > > I may be missing the real reason for calling vhost_get_features(),
> > > though.
> > >
> > > @Michael what do you recommend we do?
> > >
> > > Thanks,
> > > Stefano
> > >
> > > On Tue, Nov 8, 2022 at 12:06 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > From: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > >
> > > > 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>
> > > > ---
> > > >  include/hw/virtio/virtio.h | 4 +++-
> > > >  hw/core/machine.c          | 4 +++-
> > > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > 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);
> > > > 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[] = {
> > > > --
> > > > MST
> > > >
> > > >
> >
Stefano Garzarella Nov. 21, 2022, 8:20 a.m. UTC | #6 
On Mon, Nov 21, 2022 at 02:07:41AM -0500, Michael S. Tsirkin wrote:
>On Mon, Nov 21, 2022 at 02:17:02PM +0800, Jason Wang wrote:
>> On Sun, Nov 20, 2022 at 1:19 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Fri, Nov 18, 2022 at 03:32:56PM +0100, Stefano Garzarella wrote:
>> > > Hi,
>> > > starting from this commit 69e1c14aa2 ("virtio: core: vq reset feature
>> > > negotation support"), vhost-user-vsock and vhost-vsock fails while
>> > > setting the device features, because VIRTIO_F_RING_RESET is not masked.
>> > >
>> > > I'm not sure vsock is the only one affected.
>> > >
>> > > We could fix in two ways:
>> > >
>> > > 1) Masking VIRTIO_F_RING_RESET when we call vhost_get_features:
>> > >
>> > > diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
>> > > index 29b9ab4f72..e671cc695f 100644
>> > > --- a/hw/virtio/vhost-vsock-common.c
>> > > +++ b/hw/virtio/vhost-vsock-common.c
>> > > @@ -21,6 +21,7 @@
>> > >
>> > >  const int feature_bits[] = {
>> > >      VIRTIO_VSOCK_F_SEQPACKET,
>> > > +    VIRTIO_F_RING_RESET,
>> > >      VHOST_INVALID_FEATURE_BIT
>> > >  };
>> > >
>> >
>> > Let's do this, we need to be conservative.
>>
>> Ack.

Okay, thanks for the feedbacks!

>>
>> Thanks
>
>
>Patch pls? Stefano?
>

Yep, I'll send it.

Should we do the same in the other vhost and vhost-user devices?
IIUC now we only filter it in vhost-net.

Then I'll try to see if we can rework how we handle features to make 
sure we avoid this in the future. Maybe by creating an array of core 
features always filtered, because for example this one is not device 
specific.

Thanks,
Stefano
diff mbox series

Patch

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);
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[] = {