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

[Jason Wang]

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