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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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