Re: [PATCH 4/6] virtio/vsock: add VIRTIO_VSOCK_F_DGRAM feature bit

[Bobby Eshleman <bobbyeshleman@gmail.com>]

On Mon, Sep 26, 2022 at 03:17:51PM +0200, Stefano Garzarella wrote:
> On Mon, Aug 15, 2022 at 10:56:07AM -0700, Bobby Eshleman wrote:
> > This commit adds a feature bit for virtio vsock to support datagrams.
> > 
> > Signed-off-by: Jiang Wang <jiang.wang@bytedance.com>
> > Signed-off-by: Bobby Eshleman <bobby.eshleman@bytedance.com>
> > ---
> > drivers/vhost/vsock.c             | 3 ++-
> > include/uapi/linux/virtio_vsock.h | 1 +
> > net/vmw_vsock/virtio_transport.c  | 8 ++++++--
> > 3 files changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index b20ddec2664b..a5d1bdb786fe 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -32,7 +32,8 @@
> > enum {
> > 	VHOST_VSOCK_FEATURES = VHOST_FEATURES |
> > 			       (1ULL << VIRTIO_F_ACCESS_PLATFORM) |
> > -			       (1ULL << VIRTIO_VSOCK_F_SEQPACKET)
> > +			       (1ULL << VIRTIO_VSOCK_F_SEQPACKET) |
> > +			       (1ULL << VIRTIO_VSOCK_F_DGRAM)
> > };
> > 
> > enum {
> > diff --git a/include/uapi/linux/virtio_vsock.h b/include/uapi/linux/virtio_vsock.h
> > index 64738838bee5..857df3a3a70d 100644
> > --- a/include/uapi/linux/virtio_vsock.h
> > +++ b/include/uapi/linux/virtio_vsock.h
> > @@ -40,6 +40,7 @@
> > 
> > /* The feature bitmap for virtio vsock */
> > #define VIRTIO_VSOCK_F_SEQPACKET	1	/* SOCK_SEQPACKET supported */
> > +#define VIRTIO_VSOCK_F_DGRAM		2	/* Host support dgram vsock */
> 
> We already allocated bit 2 for F_NO_IMPLIED_STREAM , so we should use 3:
> https://github.com/oasis-tcs/virtio-spec/blob/26ed30ccb049fd51d6e20aad3de2807d678edb3a/virtio-vsock.tex#L22
> (I'll send patches to implement F_STREAM and F_NO_IMPLIED_STREAM negotiation
> soon).
> 
> As long as it's RFC it's fine to introduce F_DGRAM, but we should first
> change virtio-spec before merging this series.
> 
> About the patch, we should only negotiate the new feature when we really
> have DGRAM support. So, it's better to move this patch after adding support
> for datagram.

Roger that, I'll reorder that for v2 and also clarify the series by
prefixing it with RFC.

Before removing "RFC" from the series, I'll be sure to send out
virtio-spec patches first.

Thanks,
Bobby

> 
> Thanks,
> Stefano
> 
> > 
> > struct virtio_vsock_config {
> > 	__le64 guest_cid;
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index c6212eb38d3c..073314312683 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -35,6 +35,7 @@ static struct virtio_transport virtio_transport; /*
> > forward declaration */
> > struct virtio_vsock {
> > 	struct virtio_device *vdev;
> > 	struct virtqueue *vqs[VSOCK_VQ_MAX];
> > +	bool has_dgram;
> > 
> > 	/* Virtqueue processing is deferred to a workqueue */
> > 	struct work_struct tx_work;
> > @@ -709,7 +710,6 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > 	}
> > 
> > 	vsock->vdev = vdev;
> > -
> > 	vsock->rx_buf_nr = 0;
> > 	vsock->rx_buf_max_nr = 0;
> > 	atomic_set(&vsock->queued_replies, 0);
> > @@ -726,6 +726,9 @@ static int virtio_vsock_probe(struct virtio_device *vdev)
> > 	if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_SEQPACKET))
> > 		vsock->seqpacket_allow = true;
> > 
> > +	if (virtio_has_feature(vdev, VIRTIO_VSOCK_F_DGRAM))
> > +		vsock->has_dgram = true;
> > +
> > 	vdev->priv = vsock;
> > 
> > 	ret = virtio_vsock_vqs_init(vsock);
> > @@ -820,7 +823,8 @@ static struct virtio_device_id id_table[] = {
> > };
> > 
> > static unsigned int features[] = {
> > -	VIRTIO_VSOCK_F_SEQPACKET
> > +	VIRTIO_VSOCK_F_SEQPACKET,
> > +	VIRTIO_VSOCK_F_DGRAM
> > };
> > 
> > static struct virtio_driver virtio_vsock_driver = {
> > -- 
> > 2.35.1
> > 
> 
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization