Re: Re: [RFC v6] virtio/vsock: add two more queues for datagram types

[Stefan Hajnoczi]

On Wed, Sep 15, 2021 at 08:59:17PM -0700, Jiang Wang . wrote:
> On Tue, Sep 14, 2021 at 5:46 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Sep 13, 2021 at 10:18:43PM +0000, Jiang Wang wrote:
> > > diff --git a/hw/virtio/vhost-user-vsock.c b/hw/virtio/vhost-user-vsock.c
> > > index 6095ed7349..e9ec0e1c00 100644
> > > --- a/hw/virtio/vhost-user-vsock.c
> > > +++ b/hw/virtio/vhost-user-vsock.c
> > > @@ -105,7 +105,7 @@ static void vuv_device_realize(DeviceState *dev, 
> > > Error **errp)
> > >          return;
> > >      }
> > >
> > > -    vhost_vsock_common_realize(vdev, "vhost-user-vsock");
> > > +    vhost_vsock_common_realize(vdev, "vhost-user-vsock", false);
> >
> > VIRTIO_VSOCK_F_DGRAM support should work equally well for
> > vhost-user-vsock. I don't think there is a need to disable it here.
> >
> Stefano mentioned in previous email ( V3 ) that I can disable dgram
> for vhost-user-vsock for now. I think we need some extra changes to
> fully support vhost-vsock-user, like feature discovery?

I'm not sure why enabling it on the QEMU side poses a problem? VIRTIO
feature negotiation will disable it (if the feature bit is included in
user_feature_bits[]) if the vhost-user device backend lacks support.

> > >      /* The event queue belongs to QEMU */
> > >      vvc->event_vq = virtio_add_queue(vdev, VHOST_VSOCK_QUEUE_SIZE,
> > >                                         vhost_vsock_common_handle_output);
> > >
> > > -    vvc->vhost_dev.nvqs = ARRAY_SIZE(vvc->vhost_vqs);
> > > -    vvc->vhost_dev.vqs = vvc->vhost_vqs;
> > > +    vvc->vhost_dev.nvqs = nvqs;
> > > +    vvc->vhost_dev.vqs = g_new0(struct vhost_virtqueue, 
> > > vvc->vhost_dev.nvqs);
> >
> > IIUC the number of virtqueues needs to be the maximum supported number.
> > It's okay to have more virtqueues than needed. The feature bit is used
> > by the driver to detect the device's support for dgrams, not the number
> > of virtqueues.
> >
> OK. I can just make these changes. But I am not able to test vhost-user-vsock
> as of now. I tried to follow the steps on here:
> https://patchew.org/QEMU/20200515152110.202917-1-sgarzare@redhat.com/
> But the git repo for the vhost-user-vsock is kind of broken. I tried to
> fix it but I am new to rust so it will take some time. Is there any updated
> or an easier way to test vhost-user-vsock?

Harsha or Stefano can help with this.

> > > @@ -132,13 +138,34 @@ static const VMStateDescription 
> > > vmstate_virtio_vhost_vsock = {
> > >      .post_load = vhost_vsock_common_post_load,
> > >  };
> > >
> > > +static bool vhost_vsock_dgram_supported(int vhostfd, Error **errp)
> > > +{
> > > +    uint64_t features;
> > > +    int ret;
> > > +
> > > +    ret = ioctl(vhostfd, VHOST_GET_FEATURES, &features);
> > > +    if (ret) {
> > > +        error_setg(errp, "vhost-vsock: failed to read device freatures. 
> > > %s",
> > > +                     strerror(errno));
> > > +        return false;
> > > +    }
> >
> > ioctl(2) shouldn't be necessary. Let vhost detect features from the
> > device backend (vhost kernel or vhost-user) and don't worry about
> I did not get this part. What are the difference between vhost and
> device backend? I thought vhost is the device backend? vhost kernel
> is one kind of backend and vhost-user is another kind.  Could you explain
> more? Thanks.
> 
> > conditionally adding the exact number of virtqueues.

By "Let vhost detect features" I meant let the QEMU "hw/virtio/vhost.h"
API detect the features from the backend (either vhost kernel or
vhost-user). vhost_dev_init() and the other functions already check the
device backend's features, so there should be no need to manually make
an ioctl() call from hw/vhost-vsock.c. The purpose of the QEMU vhost API
is to abstract the backend so device code like hw/vhost-vsock.c doesn't
need to know whether it is communicating with a vhost kernel or
vhost-user device backend.

signature.asc
Description: PGP signature