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

[dpdk-dev] [PATCH v2 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.

v1->v2:
 * split into two patches
 * handle ring wrap correctly
 * add to unsupported_features when packed_vq is 0
  
Jens Freimann (2):
  net/virtio-user: ctrl vq support for packed
  Revert "net/virtio-user: fail if cq used with packed vq"

 .../net/virtio/virtio_user/virtio_user_dev.c  | 103 ++++++++++++++++--
 .../net/virtio/virtio_user/virtio_user_dev.h  |   8 +-
 drivers/net/virtio/virtio_user_ethdev.c       |  49 ++++++++-
 3 files changed, 144 insertions(+), 16 deletions(-)

-- 
2.17.2

[dpdk-dev] [PATCH v2 1/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  | 92 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  8 +-
 drivers/net/virtio/virtio_user_ethdev.c       | 49 +++++++++-
 3 files changed, 141 insertions(+), 8 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..49fcf48b9 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);
@@ -620,6 +631,81 @@ 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;
+	n_descs++;
+
+	idx_status = idx_data;
+	while (vring->desc_packed[idx_status].flags & VRING_DESC_F_NEXT) {
+		idx_status++;
+		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 virtqueue *vq)
+{
+	struct vring_packed *vring = &dev->packed_vrings[queue_idx];
+	uint16_t id, n_descs;
+	int16_t n;
+
+	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;
+		do {
+			vring->desc_packed[vq->vq_used_cons_idx].flags |=
+				VRING_DESC_F_AVAIL(vq->used_wrap_counter) |
+				VRING_DESC_F_USED(vq->used_wrap_counter);
+			if (vq->vq_used_cons_idx >= dev->queue_size) {
+				vq->vq_used_cons_idx -= dev->queue_size;
+				vq->used_wrap_counter ^= 1;
+			}
+		} while (++vq->vq_used_cons_idx <= n);
+	}
+}
+
 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..8be8ca622 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -39,7 +39,11 @@ 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_backend_ops *ops;
 	pthread_mutex_t	mutex;
 	bool		started;
@@ -53,5 +57,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, struct virtqueue *vq);
 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..efc375cd7 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -271,10 +271,36 @@ 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;
+	uint64_t desc_addr;
+	uint64_t avail_addr;
+	uint64_t used_addr;
+
+	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);
+	dev->packed_vrings[queue_idx].num = vq->vq_nentries;
+	dev->packed_vrings[queue_idx].desc_packed =
+		(void *)(uintptr_t)desc_addr;
+	dev->packed_vrings[queue_idx].driver_event =
+		(void *)(uintptr_t)avail_addr;
+	dev->packed_vrings[queue_idx].device_event =
+		(void *)(uintptr_t)used_addr;
+	vq->avail_wrap_counter = true;
+	vq->used_wrap_counter = true;
+}
+
+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 +314,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 +354,11 @@ 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,
+						     vq);
+		else
+			virtio_user_handle_cq(dev, vq->vq_queue_index);
 		return;
 	}
 
-- 
2.17.2

[dpdk-dev] [PATCH v2 2/2] Revert "net/virtio-user: fail if cq used with packed vq"

[Jens Freimann]

This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.

Not a clean revert, I had to resolve a conflict due to
  616ea5519 net/virtio-user: fix packed vq option parsing

Signed-off-by: Jens Freimann <jfreimann@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 49fcf48b9..b7059cb1e 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -478,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);
-- 
2.17.2

Re: [dpdk-dev] [PATCH v2 1/2] net/virtio-user: ctrl vq support for packed

[Maxime Coquelin]

On 1/10/19 2:17 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  | 92 ++++++++++++++++++-
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  8 +-
>   drivers/net/virtio/virtio_user_ethdev.c       | 49 +++++++++-
>   3 files changed, 141 insertions(+), 8 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..49fcf48b9 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);
> @@ -620,6 +631,81 @@ 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;
> +	n_descs++;
> +
> +	idx_status = idx_data;
> +	while (vring->desc_packed[idx_status].flags & VRING_DESC_F_NEXT) {
> +		idx_status++;
> +		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 virtqueue *vq)
> +{
> +	struct vring_packed *vring = &dev->packed_vrings[queue_idx];
> +	uint16_t id, n_descs;
> +	int16_t n;
> +
> +	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;
Are the above assignments necessary? I don't think so.

> +		n = vq->vq_used_cons_idx + n_descs - 1;
> +		do {
> +			vring->desc_packed[vq->vq_used_cons_idx].flags |=
> +				VRING_DESC_F_AVAIL(vq->used_wrap_counter) |
> +				VRING_DESC_F_USED(vq->used_wrap_counter);
> +			if (vq->vq_used_cons_idx >= dev->queue_size) {
> +				vq->vq_used_cons_idx -= dev->queue_size;
> +				vq->used_wrap_counter ^= 1;
> +			}
> +		} while (++vq->vq_used_cons_idx <= n);

I think it still does not work.
In case of wrap, n will be greater than queue size.
But as vq->vq_used_cons_idx wraps, I guess there will be an endless
loop.

> +	}
> +}
> +
>   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..8be8ca622 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -39,7 +39,11 @@ 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_backend_ops *ops;
>   	pthread_mutex_t	mutex;
>   	bool		started;
> @@ -53,5 +57,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, struct virtqueue *vq);
>   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..efc375cd7 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -271,10 +271,36 @@ 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;
> +	uint64_t desc_addr;
> +	uint64_t avail_addr;
> +	uint64_t used_addr;
> +
> +	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);
> +	dev->packed_vrings[queue_idx].num = vq->vq_nentries;
> +	dev->packed_vrings[queue_idx].desc_packed =
> +		(void *)(uintptr_t)desc_addr;
> +	dev->packed_vrings[queue_idx].driver_event =
> +		(void *)(uintptr_t)avail_addr;
> +	dev->packed_vrings[queue_idx].device_event =
> +		(void *)(uintptr_t)used_addr;
> +	vq->avail_wrap_counter = true;
> +	vq->used_wrap_counter = true;
> +}
> +
> +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 +314,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 +354,11 @@ 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,
> +						     vq);
> +		else
> +			virtio_user_handle_cq(dev, vq->vq_queue_index);
>   		return;
>   	}
>   
> 

Re: [dpdk-dev] [PATCH v2 2/2] Revert "net/virtio-user: fail if cq used with packed vq"

[Maxime Coquelin]

On 1/10/19 2:17 PM, Jens Freimann wrote:
> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
> 
> Not a clean revert, I had to resolve a conflict due to
>    616ea5519 net/virtio-user: fix packed vq option parsing

Maybe good to explain why it can be reverted now.

> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>   1 file changed, 3 insertions(+), 8 deletions(-)
> 

With the clarification in commit message:

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

Re: [dpdk-dev] [PATCH v2 2/2] Revert "net/virtio-user: fail if cq used with packed vq"

[Tiwei Bie]

On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
> 
> Not a clean revert, I had to resolve a conflict due to
>   616ea5519 net/virtio-user: fix packed vq option parsing
> 
> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 49fcf48b9..b7059cb1e 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -478,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;
> -		}

I think it's natural to drop above code in the same patch that
introduces the control vq support.

> -	} else {
> +	if (packed_vq)
> +		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);

We shouldn't add this bit into device_features like this.
We just need this:

	if (!packed_vq)
		dev->unsupported_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);
> -- 
> 2.17.2
> 

Re: [dpdk-dev] [PATCH v2 2/2] Revert "net/virtio-user: fail if cq used with packed vq"

[Jens Freimann]

On Thu, Jan 10, 2019 at 10:40:25PM +0800, Tiwei Bie wrote:
>On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
>> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
>>
>> Not a clean revert, I had to resolve a conflict due to
>>   616ea5519 net/virtio-user: fix packed vq option parsing
>>
>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>> ---
>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> index 49fcf48b9..b7059cb1e 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -478,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;
>> -		}
>
>I think it's natural to drop above code in the same patch that
>introduces the control vq support.

I just split it out because Maxime requested it. 

>
>> -	} else {
>> +	if (packed_vq)
>> +		dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>
>We shouldn't add this bit into device_features like this.
>We just need this:

ok

regards,
Jens 

>
>	if (!packed_vq)
>		dev->unsupported_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);
>> --
>> 2.17.2
>>

Re: [dpdk-dev] [PATCH v2 2/2] Revert "net/virtio-user: fail if cq used with packed vq"

[Maxime Coquelin]

On 1/10/19 3:58 PM, Jens Freimann wrote:
> On Thu, Jan 10, 2019 at 10:40:25PM +0800, Tiwei Bie wrote:
>> On Thu, Jan 10, 2019 at 02:17:51PM +0100, Jens Freimann wrote:
>>> This reverts commit 5e4e7a7524a30c176bd6b1789ab30963f27f2681.
>>>
>>> Not a clean revert, I had to resolve a conflict due to
>>>   616ea5519 net/virtio-user: fix packed vq option parsing
>>>
>>> Signed-off-by: Jens Freimann <jfreimann@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 11 +++--------
>>>  1 file changed, 3 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c 
>>> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> index 49fcf48b9..b7059cb1e 100644
>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>>> @@ -478,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;
>>> -        }
>>
>> I think it's natural to drop above code in the same patch that
>> introduces the control vq support.
> 
> I just split it out because Maxime requested it.

Yeah, disagreements happen. :)

I understand this is not a clean revert anyway, so feel free to keep it
in a single patch.

Thanks,
Maxime

>>
>>> -    } else {
>>> +    if (packed_vq)
>>> +        dev->device_features |= (1ull << VIRTIO_F_RING_PACKED);
>>
>> We shouldn't add this bit into device_features like this.
>> We just need this:
> 
> ok
> 
> regards,
> Jens
>>
>>     if (!packed_vq)
>>         dev->unsupported_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);
>>> -- 
>>> 2.17.2
>>>

Re: [dpdk-dev] [PATCH v2 1/2] net/virtio-user: ctrl vq support for packed

[Jens Freimann]

On Thu, Jan 10, 2019 at 03:23:29PM +0100, Maxime Coquelin wrote:
>
>
>On 1/10/19 2:17 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  | 92 ++++++++++++++++++-
>>  .../net/virtio/virtio_user/virtio_user_dev.h  |  8 +-
>>  drivers/net/virtio/virtio_user_ethdev.c       | 49 +++++++++-
>>  3 files changed, 141 insertions(+), 8 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..49fcf48b9 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);
>>@@ -620,6 +631,81 @@ 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;
>>+	n_descs++;
>>+
>>+	idx_status = idx_data;
>>+	while (vring->desc_packed[idx_status].flags & VRING_DESC_F_NEXT) {
>>+		idx_status++;
>>+		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 virtqueue *vq)
>>+{
>>+	struct vring_packed *vring = &dev->packed_vrings[queue_idx];
>>+	uint16_t id, n_descs;
>>+	int16_t n;
>>+
>>+	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;
>Are the above assignments necessary? I don't think so.

The device could use this to skip n descriptors, but only if we do
in-order and we don't use the id field. So, yes I agree it's not necessary.


>>+		n = vq->vq_used_cons_idx + n_descs - 1;
>>+		do {
>>+			vring->desc_packed[vq->vq_used_cons_idx].flags |=
>>+				VRING_DESC_F_AVAIL(vq->used_wrap_counter) |
>>+				VRING_DESC_F_USED(vq->used_wrap_counter);
>>+			if (vq->vq_used_cons_idx >= dev->queue_size) {
>>+				vq->vq_used_cons_idx -= dev->queue_size;
>>+				vq->used_wrap_counter ^= 1;
>>+			}
>>+		} while (++vq->vq_used_cons_idx <= n);
>
>I think it still does not work.
>In case of wrap, n will be greater than queue size.
>But as vq->vq_used_cons_idx wraps, I guess there will be an endless
>loop.

Now I get what you meant :) yes, I'll fix it.

regards,
Jens 
>
>>+	}
>>+}
>>+
>>  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..8be8ca622 100644
>>--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>@@ -39,7 +39,11 @@ 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_backend_ops *ops;
>>  	pthread_mutex_t	mutex;
>>  	bool		started;
>>@@ -53,5 +57,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, struct virtqueue *vq);
>>  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..efc375cd7 100644
>>--- a/drivers/net/virtio/virtio_user_ethdev.c
>>+++ b/drivers/net/virtio/virtio_user_ethdev.c
>>@@ -271,10 +271,36 @@ 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;
>>+	uint64_t desc_addr;
>>+	uint64_t avail_addr;
>>+	uint64_t used_addr;
>>+
>>+	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);
>>+	dev->packed_vrings[queue_idx].num = vq->vq_nentries;
>>+	dev->packed_vrings[queue_idx].desc_packed =
>>+		(void *)(uintptr_t)desc_addr;
>>+	dev->packed_vrings[queue_idx].driver_event =
>>+		(void *)(uintptr_t)avail_addr;
>>+	dev->packed_vrings[queue_idx].device_event =
>>+		(void *)(uintptr_t)used_addr;
>>+	vq->avail_wrap_counter = true;
>>+	vq->used_wrap_counter = true;
>>+}
>>+
>>+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 +314,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 +354,11 @@ 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,
>>+						     vq);
>>+		else
>>+			virtio_user_handle_cq(dev, vq->vq_queue_index);
>>  		return;
>>  	}
>>