Re: [External] Re: [RFC v1 1/6] virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

[Stefano Garzarella <sgarzare@redhat.com>]

On Mon, Jun 21, 2021 at 10:24:20AM -0700, Jiang Wang . wrote:
>On Fri, Jun 18, 2021 at 2:40 AM Stefano Garzarella <sgarzare@redhat.com> wrote:
>>
>> On Wed, Jun 09, 2021 at 11:24:53PM +0000, Jiang Wang wrote:
>> >When this feature is enabled, allocate 5 queues,
>> >otherwise, allocate 3 queues to be compatible with
>> >old QEMU versions.
>> >
>> >Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
>> >---
>> > drivers/vhost/vsock.c             |  3 +-
>> > include/linux/virtio_vsock.h      |  9 +++++
>> > include/uapi/linux/virtio_vsock.h |  3 ++
>> > net/vmw_vsock/virtio_transport.c  | 73 +++++++++++++++++++++++++++++++++++----
>> > 4 files changed, 80 insertions(+), 8 deletions(-)
>> >
>> >diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
>> >index 5e78fb719602..81d064601093 100644
>> >--- a/drivers/vhost/vsock.c
>> >+++ b/drivers/vhost/vsock.c
>> >@@ -31,7 +31,8 @@
>> >
>> > enum {
>> >       VHOST_VSOCK_FEATURES = VHOST_FEATURES |
>> >-                             (1ULL << VIRTIO_F_ACCESS_PLATFORM)
>> >+                             (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
>> >+                             (1ULL << VIRTIO_VSOCK_F_DGRAM)
>> > };
>> >
>> > enum {
>> >diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h
>> >index dc636b727179..ba3189ed9345 100644
>> >--- a/include/linux/virtio_vsock.h
>> >+++ b/include/linux/virtio_vsock.h
>> >@@ -18,6 +18,15 @@ enum {
>> >       VSOCK_VQ_MAX    = 3,
>> > };
>> >
>> >+enum {
>> >+      VSOCK_VQ_STREAM_RX     = 0, /* for host to guest data */
>> >+      VSOCK_VQ_STREAM_TX     = 1, /* for guest to host data */
>> >+      VSOCK_VQ_DGRAM_RX       = 2,
>> >+      VSOCK_VQ_DGRAM_TX       = 3,
>> >+      VSOCK_VQ_EX_EVENT       = 4,
>> >+      VSOCK_VQ_EX_MAX         = 5,
>> >+};
>> >+
>> > /* Per-socket state (accessed via vsk->trans) */
>> > struct virtio_vsock_sock {
>> >       struct vsock_sock *vsk;
>> >diff --git a/include/uapi/linux/virtio_vsock.h b/include/uapi/linux/virtio_vsock.h
>> >index 1d57ed3d84d2..b56614dff1c9 100644
>> >--- a/include/uapi/linux/virtio_vsock.h
>> >+++ b/include/uapi/linux/virtio_vsock.h
>> >@@ -38,6 +38,9 @@
>> > #include <linux/virtio_ids.h>
>> > #include <linux/virtio_config.h>
>> >
>> >+/* The feature bitmap for virtio net */
>> >+#define VIRTIO_VSOCK_F_DGRAM  0       /* Host support dgram vsock */
>> >+
>> > struct virtio_vsock_config {
>> >       __le64 guest_cid;
>> > } __attribute__((packed));
>> >diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
>> >index 2700a63ab095..7dcb8db23305 100644
>> >--- a/net/vmw_vsock/virtio_transport.c
>> >+++ b/net/vmw_vsock/virtio_transport.c
>> >@@ -27,7 +27,8 @@ static DEFINE_MUTEX(the_virtio_vsock_mutex); /* protects the_virtio_vsock */
>> >
>> > struct virtio_vsock {
>> >       struct virtio_device *vdev;
>> >-      struct virtqueue *vqs[VSOCK_VQ_MAX];
>> >+      struct virtqueue **vqs;
>> >+      bool has_dgram;
>> >
>> >       /* Virtqueue processing is deferred to a workqueue */
>> >       struct work_struct tx_work;
>> >@@ -333,7 +334,10 @@ static int virtio_vsock_event_fill_one(struct virtio_vsock *vsock,
>> >       struct scatterlist sg;
>> >       struct virtqueue *vq;
>> >
>> >-      vq = vsock->vqs[VSOCK_VQ_EVENT];
>> >+      if (vsock->has_dgram)
>> >+              vq = vsock->vqs[VSOCK_VQ_EX_EVENT];
>> >+      else
>> >+              vq = vsock->vqs[VSOCK_VQ_EVENT];
>> >
>> >       sg_init_one(&sg, event, sizeof(*event));
>> >
>> >@@ -351,7 +355,10 @@ static void virtio_vsock_event_fill(struct virtio_vsock *vsock)
>> >               virtio_vsock_event_fill_one(vsock, event);
>> >       }
>> >
>> >-      virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]);
>> >+      if (vsock->has_dgram)
>> >+              virtqueue_kick(vsock->vqs[VSOCK_VQ_EX_EVENT]);
>> >+      else
>> >+              virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]);
>> > }
>> >
>> > static void virtio_vsock_reset_sock(struct sock *sk)
>> >@@ -391,7 +398,10 @@ static void virtio_transport_event_work(struct work_struct *work)
>> >               container_of(work, struct virtio_vsock, event_work);
>> >       struct virtqueue *vq;
>> >
>> >-      vq = vsock->vqs[VSOCK_VQ_EVENT];
>> >+      if (vsock->has_dgram)
>> >+              vq = vsock->vqs[VSOCK_VQ_EX_EVENT];
>> >+      else
>> >+              vq = vsock->vqs[VSOCK_VQ_EVENT];
>> >
>> >       mutex_lock(&vsock->event_lock);
>> >
>> >@@ -411,7 +421,10 @@ static void virtio_transport_event_work(struct work_struct *work)
>> >               }
>> >       } while (!virtqueue_enable_cb(vq));
>> >
>> >-      virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]);
>> >+      if (vsock->has_dgram)
>> >+              virtqueue_kick(vsock->vqs[VSOCK_VQ_EX_EVENT]);
>> >+      else
>> >+              virtqueue_kick(vsock->vqs[VSOCK_VQ_EVENT]);
>> > out:
>> >       mutex_unlock(&vsock->event_lock);
>> > }
>> >@@ -434,6 +447,10 @@ static void virtio_vsock_tx_done(struct virtqueue *vq)
>> >       queue_work(virtio_vsock_workqueue, &vsock->tx_work);
>> > }
>> >
>> >+static void virtio_vsock_dgram_tx_done(struct virtqueue *vq)
>> >+{
>> >+}
>> >+
>> > static void virtio_vsock_rx_done(struct virtqueue *vq)
>> > {
>> >       struct virtio_vsock *vsock = vq->vdev->priv;
>> >@@ -443,6 +460,10 @@ static void virtio_vsock_rx_done(struct virtqueue *vq)
>> >       queue_work(virtio_vsock_workqueue, &vsock->rx_work);
>> > }
>> >
>> >+static void virtio_vsock_dgram_rx_done(struct virtqueue *vq)
>> >+{
>> >+}
>> >+
>> > static struct virtio_transport virtio_transport = {
>> >       .transport = {
>> >               .module                   = THIS_MODULE,
>> >@@ -545,13 +566,29 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>> >               virtio_vsock_tx_done,
>> >               virtio_vsock_event_done,
>> >       };
>> >+      vq_callback_t *ex_callbacks[] = {
>>
>> 'ex' is not clear, maybe better 'dgram'?
>>
>sure.
>
>> What happen if F_DGRAM is negotiated, but not F_STREAM?
>>
>Hmm. In my mind, F_STREAM is always negotiated. Do we want to add
>support when F_STREAM is not negotiated?
>

Yep, I think we should support this case.

The main purpose of the feature bits is to enable/disable the 
functionality after the negotiation.
Initially we didn't want to introduce it, but then we thought it was 
better because there could be a device for example that wants to support 
only datagram.

Since you're touching this part of the code, it would be very helpful to 
fix the problem now.

But if you think it's too complex, we can do it in a second step.

Thanks,
Stefano

>> >+              virtio_vsock_rx_done,
>> >+              virtio_vsock_tx_done,
>> >+              virtio_vsock_dgram_rx_done,
>> >+              virtio_vsock_dgram_tx_done,
>> >+              virtio_vsock_event_done,
>> >+      };
>> >+
>> >       static const char * const names[] = {
>> >               "rx",
>> >               "tx",
>> >               "event",
>> >       };
>> >+      static const char * const ex_names[] = {
>> >+              "rx",
>> >+              "tx",
>> >+              "dgram_rx",
>> >+              "dgram_tx",
>> >+              "event",
>> >+      };
>> >+
>> >       struct virtio_vsock *vsock = NULL;
>> >-      int ret;
>> >+      int ret, max_vq;
>> >
>> >       ret = mutex_lock_interruptible(&the_virtio_vsock_mutex);
>> >       if (ret)
>> >@@ -572,9 +609,30 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
>> >
>> >       vsock->vdev = vdev;
>> >
>> >-      ret = virtio_find_vqs(vsock->vdev, VSOCK_VQ_MAX,
>> >+      if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_DGRAM))
>> >+              vsock->has_dgram = true;
>> >+
>> >+      if (vsock->has_dgram)
>> >+              max_vq = VSOCK_VQ_EX_MAX;
>> >+      else
>> >+              max_vq = VSOCK_VQ_MAX;
>> >+
>> >+      vsock->vqs = kmalloc_array(max_vq, sizeof(struct virtqueue *), GFP_KERNEL);
>> >+      if (!vsock->vqs) {
>> >+              ret = -ENOMEM;
>> >+              goto out;
>> >+      }
>> >+
>> >+      if (vsock->has_dgram) {
>> >+              ret = virtio_find_vqs(vsock->vdev, max_vq,
>> >+                            vsock->vqs, ex_callbacks, ex_names,
>> >+                            NULL);
>> >+      } else {
>> >+              ret = virtio_find_vqs(vsock->vdev, max_vq,
>> >                             vsock->vqs, callbacks, names,
>> >                             NULL);
>> >+      }
>> >+
>> >       if (ret < 0)
>> >               goto out;
>> >
>> >@@ -695,6 +753,7 @@ static struct virtio_device_id id_table[] = {
>> > };
>> >
>> > static unsigned int features[] = {
>> >+      VIRTIO_VSOCK_F_DGRAM,
>> > };
>> >
>> > static struct virtio_driver virtio_vsock_driver = {
>> >--
>> >2.11.0
>> >
>>
>