[dpdk-dev] [PATCH v3 0/2] net/virtio-user: add packed vq support

[dpdk-dev] [PATCH v3 0/2] net/virtio-user: add packed vq support

[Jens Freimann]

Revert patch to error out when cq is used with packed vq and
in second patch add support for packed virtqueues in control
virtqueue code.

Patch 1 fixes a bug in virtio_pq_send_command(). When we wait for
a descriptor to be marked as used we need to consider that the ring
might have wrapped and use the previous wrap counter.

v2->v3:
 * squash revert patch into patch 2
 * add new patch 1 to fix a bug in driver code
 * fix wrap handling code in virtio_user_handle_cq_packed
 * in v2 I was using wrap counters and indexes from shared virtqueue,
   fixed this and added wrap counters and used index to virtio_user code
 * tested wrap around of ring 

v1->v2:
 * split into two patches
 * handle ring wrap correctly
 * add to unsupported_features when packed_vq is 0

Jens Freimann (2):
  net/virtio: check head desc with correct wrap counter
  net/virtio-user: ctrl vq support for packed

 drivers/net/virtio/virtio_ethdev.c            |  11 +-
 .../net/virtio/virtio_user/virtio_user_dev.c  | 104 ++++++++++++++++--
 .../net/virtio/virtio_user/virtio_user_dev.h  |  15 ++-
 drivers/net/virtio/virtio_user_ethdev.c       |  56 +++++++++-
 drivers/net/virtio/virtqueue.h                |  10 +-
 5 files changed, 173 insertions(+), 23 deletions(-)

-- 
2.17.2

[dpdk-dev] [Patch v3 1/2] net/virtio: check head desc with correct wrap counter

[Jens Freimann]

In virtio_pq_send_command() we check for a used descriptor
and wait in an idle loop until it becomes used. We can't use
vq->used_wrap_counter here to check for the first descriptor
we made available because the ring could have wrapped. Let's use
the used_wrap_counter that matches the state of the head descriptor.

Fixes: ec194c2 ("net/virtio: support packed queue in send command")

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_ethdev.c | 11 ++++++-----
 drivers/net/virtio/virtqueue.h     | 10 ++++++++--
 2 files changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 6d461180c..ee5a98b7c 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -149,7 +149,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 	int head;
 	struct vring_packed_desc *desc = vq->ring_packed.desc_packed;
 	struct virtio_pmd_ctrl *result;
-	int wrap_counter;
+	bool avail_wrap_counter, used_wrap_counter;
 	uint16_t flags;
 	int sum = 0;
 	int k;
@@ -161,7 +161,8 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 	 * One RX packet for ACK.
 	 */
 	head = vq->vq_avail_idx;
-	wrap_counter = vq->avail_wrap_counter;
+	avail_wrap_counter = vq->avail_wrap_counter;
+	used_wrap_counter = vq->used_wrap_counter;
 	desc[head].flags = VRING_DESC_F_NEXT;
 	desc[head].addr = cvq->virtio_net_hdr_mem;
 	desc[head].len = sizeof(struct virtio_net_ctrl_hdr);
@@ -199,8 +200,8 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 		 VRING_DESC_F_USED(!vq->avail_wrap_counter);
 	desc[vq->vq_avail_idx].flags = flags;
 	flags = VRING_DESC_F_NEXT;
-	flags |= VRING_DESC_F_AVAIL(wrap_counter) |
-		 VRING_DESC_F_USED(!wrap_counter);
+	flags |= VRING_DESC_F_AVAIL(avail_wrap_counter) |
+		 VRING_DESC_F_USED(!avail_wrap_counter);
 	desc[head].flags = flags;
 	rte_smp_wmb();
 
@@ -216,7 +217,7 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl,
 	do {
 		rte_rmb();
 		usleep(100);
-	} while (!desc_is_used(&desc[head], vq));
+	} while (!__desc_is_used(&desc[head], used_wrap_counter));
 
 	/* now get used descriptors */
 	while (desc_is_used(&desc[vq->vq_used_cons_idx], vq)) {
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 123bec34f..7fcde5643 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -281,7 +281,7 @@ struct virtio_tx_region {
 };
 
 static inline int
-desc_is_used(struct vring_packed_desc *desc, struct virtqueue *vq)
+__desc_is_used(struct vring_packed_desc *desc, bool wrap_counter)
 {
 	uint16_t used, avail, flags;
 
@@ -289,7 +289,13 @@ desc_is_used(struct vring_packed_desc *desc, struct virtqueue *vq)
 	used = !!(flags & VRING_DESC_F_USED(1));
 	avail = !!(flags & VRING_DESC_F_AVAIL(1));
 
-	return avail == used && used == vq->used_wrap_counter;
+	return avail == used && used == wrap_counter;
+}
+
+static inline int
+desc_is_used(struct vring_packed_desc *desc, struct virtqueue *vq)
+{
+	return __desc_is_used(desc, vq->used_wrap_counter);
 }
 
 
-- 
2.17.2

[dpdk-dev] [Patch v3 2/2] net/virtio-user: ctrl vq support for packed

[Jens Freimann]

Add support to virtio-user for control virtqueues.

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 .../net/virtio/virtio_user/virtio_user_dev.c  | 104 ++++++++++++++++--
 .../net/virtio/virtio_user/virtio_user_dev.h  |  15 ++-
 drivers/net/virtio/virtio_user_ethdev.c       |  56 +++++++++-
 3 files changed, 159 insertions(+), 16 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..e7d1cf225 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 {
+	if (packed_vq)
+		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
+	else
 		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
-	}
 
 	if (dev->mac_specified)
 		dev->frontend_features |= (1ull << VIRTIO_NET_F_MAC);
@@ -620,6 +626,82 @@ 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 idx_data, idx_status;
+	/* initialize to one, header is first */
+	uint32_t n_descs = 1;
+
+	/* locate desc for header, data, and status */
+	idx_data = idx_hdr + 1;
+	if (idx_data >= dev->queue_size)
+		idx_data -= dev->queue_size;
+
+	n_descs++;
+
+	idx_status = idx_data;
+	while (vring->desc_packed[idx_status].flags & VRING_DESC_F_NEXT) {
+		idx_status++;
+		if (idx_status >= dev->queue_size)
+			idx_status -= dev->queue_size;
+		n_descs++;
+	}
+
+	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 virtio_user_queue *vq = &dev->packed_queues[queue_idx];
+	struct vring_packed *vring = &dev->packed_vrings[queue_idx];
+	uint16_t id, n_descs;
+
+	while (desc_is_avail(&vring->desc_packed[vq->used_idx],
+			     vq->used_wrap_counter)) {
+		id = vring->desc_packed[vq->used_idx].id;
+
+		n_descs = virtio_user_handle_ctrl_msg_pq(dev, vring, id);
+
+		do {
+			vring->desc_packed[vq->used_idx].flags =
+				VRING_DESC_F_AVAIL(vq->used_wrap_counter) |
+				VRING_DESC_F_USED(vq->used_wrap_counter);
+			if (++vq->used_idx >= dev->queue_size) {
+				vq->used_idx -= dev->queue_size;
+				vq->used_wrap_counter ^= 1;
+			}
+			n_descs--;
+		} while (n_descs);
+	}
+}
+
 void
 virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx)
 {
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 672a8161a..f6ec09be1 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -11,6 +11,12 @@
 #include "../virtio_ring.h"
 #include "vhost.h"
 
+struct virtio_user_queue {
+	uint16_t used_idx;
+	bool avail_wrap_counter;
+	bool used_wrap_counter;
+};
+
 struct virtio_user_dev {
 	/* for vhost_user backend */
 	int		vhostfd;
@@ -39,7 +45,12 @@ struct virtio_user_dev {
 	uint16_t	port_id;
 	uint8_t		mac_addr[ETHER_ADDR_LEN];
 	char		path[PATH_MAX];
-	struct vring	vrings[VIRTIO_MAX_VIRTQUEUES];
+	union {
+		struct vring		vrings[VIRTIO_MAX_VIRTQUEUES];
+		struct vring_packed	packed_vrings[VIRTIO_MAX_VIRTQUEUES];
+	};
+	struct virtio_user_queue packed_queues[VIRTIO_MAX_VIRTQUEUES];
+
 	struct virtio_user_backend_ops *ops;
 	pthread_mutex_t	mutex;
 	bool		started;
@@ -53,5 +64,7 @@ int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 			 int mrg_rxbuf, int in_order, int packed_vq);
 void virtio_user_dev_uninit(struct virtio_user_dev *dev);
 void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx);
+void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
+				  uint16_t queue_idx);
 uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs);
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 2df6eb695..c01f45cab 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -271,10 +271,44 @@ virtio_user_get_queue_num(struct virtio_hw *hw, uint16_t queue_id __rte_unused)
 	return dev->queue_size;
 }
 
-static int
-virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
+static void
+virtio_user_setup_queue_packed(struct virtqueue *vq,
+			       struct virtio_user_dev *dev)
+
+{
+	uint16_t queue_idx = vq->vq_queue_index;
+	struct vring_packed *vring;
+	uint64_t desc_addr;
+	uint64_t avail_addr;
+	uint64_t used_addr;
+	uint16_t i;
+
+	vring  = &dev->packed_vrings[queue_idx];
+	desc_addr = (uintptr_t)vq->vq_ring_virt_mem;
+	avail_addr = desc_addr + vq->vq_nentries *
+		sizeof(struct vring_packed_desc);
+	used_addr = RTE_ALIGN_CEIL(avail_addr +
+			   sizeof(struct vring_packed_desc_event),
+			   VIRTIO_PCI_VRING_ALIGN);
+	vring->num = vq->vq_nentries;
+	vring->desc_packed =
+		(void *)(uintptr_t)desc_addr;
+	vring->driver_event =
+		(void *)(uintptr_t)avail_addr;
+	vring->device_event =
+		(void *)(uintptr_t)used_addr;
+	dev->packed_queues[queue_idx].avail_wrap_counter = true;
+	dev->packed_queues[queue_idx].used_wrap_counter = true;
+
+	for (i = 0; i < vring->num; i++) {
+		vring->desc_packed[i].flags = VRING_DESC_F_USED(1) |
+					      VRING_DESC_F_AVAIL(1);
+	}
+}
+
+static void
+virtio_user_setup_queue_split(struct virtqueue *vq, struct virtio_user_dev *dev)
 {
-	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 	uint16_t queue_idx = vq->vq_queue_index;
 	uint64_t desc_addr, avail_addr, used_addr;
 
@@ -288,6 +322,17 @@ virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	dev->vrings[queue_idx].desc = (void *)(uintptr_t)desc_addr;
 	dev->vrings[queue_idx].avail = (void *)(uintptr_t)avail_addr;
 	dev->vrings[queue_idx].used = (void *)(uintptr_t)used_addr;
+}
+
+static int
+virtio_user_setup_queue(struct virtio_hw *hw, struct virtqueue *vq)
+{
+	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
+
+	if (vtpci_packed_queue(hw))
+		virtio_user_setup_queue_packed(vq, dev);
+	else
+		virtio_user_setup_queue_split(vq, dev);
 
 	return 0;
 }
@@ -317,7 +362,10 @@ virtio_user_notify_queue(struct virtio_hw *hw, struct virtqueue *vq)
 	struct virtio_user_dev *dev = virtio_user_get_dev(hw);
 
 	if (hw->cvq && (hw->cvq->vq == vq)) {
-		virtio_user_handle_cq(dev, vq->vq_queue_index);
+		if (vtpci_packed_queue(vq->hw))
+			virtio_user_handle_cq_packed(dev, vq->vq_queue_index);
+		else
+			virtio_user_handle_cq(dev, vq->vq_queue_index);
 		return;
 	}
 
-- 
2.17.2

Re: [dpdk-dev] [Patch v3 1/2] net/virtio: check head desc with correct wrap counter

[Maxime Coquelin]

On 1/10/19 10:47 PM, Jens Freimann wrote:
> In virtio_pq_send_command() we check for a used descriptor
> and wait in an idle loop until it becomes used. We can't use
> vq->used_wrap_counter here to check for the first descriptor
> we made available because the ring could have wrapped. Let's use
> the used_wrap_counter that matches the state of the head descriptor.
> 
> Fixes: ec194c2 ("net/virtio: support packed queue in send command")
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_ethdev.c | 11 ++++++-----
>   drivers/net/virtio/virtqueue.h     | 10 ++++++++--
>   2 files changed, 14 insertions(+), 7 deletions(-)
> 


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [Patch v3 2/2] net/virtio-user: ctrl vq support for packed

[Maxime Coquelin]

On 1/10/19 10:47 PM, Jens Freimann wrote:
> Add support to virtio-user for control virtqueues.
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   .../net/virtio/virtio_user/virtio_user_dev.c  | 104 ++++++++++++++++--
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  15 ++-
>   drivers/net/virtio/virtio_user_ethdev.c       |  56 +++++++++-
>   3 files changed, 159 insertions(+), 16 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..e7d1cf225 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 {
> +	if (packed_vq)
> +		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
> +	else
>   		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);
> -	}

I think you missed Tiwei comment on v3, i.e. this should be enough:
	if (!packed_vq)
		dev->unsupported_features |= (1ull << VIRTIO_F_RING_PACKED);



Other than that, the patch looks good to me. With above fixed, feel free
to add my:

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks!
Maxime