Re: [dpdk-dev] [PATCH] virtio-user: ctrl vq support for packed

[Jens Freimann <jfreimann@redhat.com>]

On Thu, Jan 10, 2019 at 09:46:37AM +0100, Maxime Coquelin wrote:
>Hi Jens,
>
>On 1/9/19 5:59 PM, Jens Freimann wrote:
>>Add support to virtio-user for control virtqueues
>>and reverts commit "5e4e7a752 net/virtio-user: fail if
>>cq used with packed vq".
>
>I would prefer you send two patches:
>1. add ctrl vq support
>2. revert 5e4e7a752

ok

>
>
>>
>>Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>---
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 111 ++++++++++++++++--
>>  .../net/virtio/virtio_user/virtio_user_dev.h  |   8 +-
>>  drivers/net/virtio/virtio_user_ethdev.c       |  49 +++++++-
>>  3 files changed, 151 insertions(+), 17 deletions(-)
>>
>>diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>index b9044faff..95f66d5b4 100644
>>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>@@ -43,15 +43,26 @@ virtio_user_kick_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
>>  	struct vhost_vring_file file;
>>  	struct vhost_vring_state state;
>>  	struct vring *vring = &dev->vrings[queue_sel];
>>+	struct vring_packed *pq_vring = &dev->packed_vrings[queue_sel];
>>  	struct vhost_vring_addr addr = {
>>  		.index = queue_sel,
>>-		.desc_user_addr = (uint64_t)(uintptr_t)vring->desc,
>>-		.avail_user_addr = (uint64_t)(uintptr_t)vring->avail,
>>-		.used_user_addr = (uint64_t)(uintptr_t)vring->used,
>>  		.log_guest_addr = 0,
>>  		.flags = 0, /* disable log */
>>  	};
>>+	if (dev->features & (1ULL << VIRTIO_F_RING_PACKED)) {
>>+		addr.desc_user_addr =
>>+			(uint64_t)(uintptr_t)pq_vring->desc_packed;
>>+		addr.avail_user_addr =
>>+			(uint64_t)(uintptr_t)pq_vring->driver_event;
>>+		addr.used_user_addr =
>>+			(uint64_t)(uintptr_t)pq_vring->device_event;
>>+	} else {
>>+		addr.desc_user_addr = (uint64_t)(uintptr_t)vring->desc;
>>+		addr.avail_user_addr = (uint64_t)(uintptr_t)vring->avail;
>>+		addr.used_user_addr = (uint64_t)(uintptr_t)vring->used;
>>+	}
>>+
>>  	state.index = queue_sel;
>>  	state.num = vring->num;
>>  	dev->ops->send_request(dev, VHOST_USER_SET_VRING_NUM, &state);
>>@@ -467,15 +478,10 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
>>  	if (!in_order)
>>  		dev->unsupported_features |= (1ull << VIRTIO_F_IN_ORDER);
>>-	if (packed_vq) {
>>-		if (cq) {
>>-			PMD_INIT_LOG(ERR, "control vq not supported yet with "
>>-					  "packed virtqueues\n");
>>-			return -1;
>>-		}
>>-	} else {
>>-		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
>>-	}
>>+	if (packed_vq)
>>+		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>>+	else
>>+		dev->device_features &= ~(1ull << VIRTIO_F_RING_PACKED);
>
>Shouldn't you set VIRTIO_F_RING_PACKED bit in unsupported features?

I'll fix it

>>  	if (dev->mac_specified)
>>  		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
>>@@ -620,6 +626,87 @@ virtio_user_handle_ctrl_msg(struct virtio_user_dev *dev, struct vring *vring,
>>  	return n_descs;
>>  }
>>+static inline int
>>+desc_is_avail(struct vring_packed_desc *desc, bool wrap_counter)
>>+{
>>+	return wrap_counter == !!(desc->flags & VRING_DESC_F_AVAIL(1)) &&
>>+		wrap_counter != !!(desc->flags & VRING_DESC_F_USED(1));
>>+}
>>+
>>+static uint32_t
>>+virtio_user_handle_ctrl_msg_pq(struct virtio_user_dev *dev,
>>+			    struct vring_packed *vring,
>>+			    uint16_t idx_hdr)
>>+{
>>+	struct virtio_net_ctrl_hdr *hdr;
>>+	virtio_net_ctrl_ack status = ~0;
>>+	uint16_t i, idx_data, idx_status;
>>+	uint32_t n_descs = 0;
>>+
>>+	/* locate desc for header, data, and status */
>>+	idx_data = idx_hdr + 1;
>>+	n_descs++;
>>+
>>+	i = idx_data;
>>+	while (vring->desc_packed[i].flags & VRING_DESC_F_NEXT) {
>>+		i++;
>
>Why not incrementing idx_status directly?

I'll do that

>>+		n_descs++;
>>+	}
>>+
>>+	/* locate desc for status */
>>+	idx_status = i;
>>+	n_descs++;
>
>I think it would be clearer to initialize n_decsc to 1, as it could
>correspond to the header (and add a comment about it).

ok

>
>>+
>>+	hdr = (void *)(uintptr_t)vring->desc_packed[idx_hdr].addr;
>>+	if (hdr->class == VIRTIO_NET_CTRL_MQ &&
>>+	    hdr->cmd == VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET) {
>>+		uint16_t queues;
>>+
>>+		queues = *(uint16_t *)(uintptr_t)
>>+				vring->desc_packed[idx_data].addr;
>>+		status = virtio_user_handle_mq(dev, queues);
>>+	}
>>+
>>+	/* Update status */
>>+	*(virtio_net_ctrl_ack *)(uintptr_t)
>>+		vring->desc_packed[idx_status].addr = status;
>>+
>>+	return n_descs;
>>+}
>>+
>>+void
>>+virtio_user_handle_cq_packed(struct virtio_user_dev *dev, uint16_t queue_idx,
>>+			     struct virtqueue *vq)
>>+{
>>+	struct vring_packed *vring = &dev->packed_vrings[queue_idx];
>>+	uint16_t id, n_descs;
>>+	int16_t n;
>>+	bool wrap_counter = vq->used_wrap_counter;
>>+
>>+	while (desc_is_avail(&vring->desc_packed[vq->vq_used_cons_idx],
>>+			     vq->avail_wrap_counter)) {
>>+		id = vring->desc_packed[vq->vq_used_cons_idx].id;
>>+
>>+		n_descs = virtio_user_handle_ctrl_msg_pq(dev, vring, id);
>>+
>>+		vring->desc_packed[vq->vq_used_cons_idx].len = n_descs;
>>+		vring->desc_packed[vq->vq_used_cons_idx].id =
>>+			vq->vq_used_cons_idx;
>>+		n = vq->vq_used_cons_idx + n_descs - 1;
>
>I think it is broken as it needs to handle wrapping.

I think you're right. I'll fix it.

thanks for the review!

regards,
Jens