[dpdk-dev] [PATCH 7/7] net/virtio: introduce Vhost-vDPA backend

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: dev <dev-bounces at dpdk.org> On Behalf Of Maxime Coquelin
> Sent: Friday, September 11, 2020 11:08 PM
> To: dev at dpdk.org; Fu, Patrick <patrick.fu at intel.com>; amorenoz at redhat.com
> Cc: Maxime Coquelin <maxime.coquelin at redhat.com>
> Subject: [dpdk-dev] [PATCH 7/7] net/virtio: introduce Vhost-vDPA backend
> 
> vhost-vDPA is a new virtio backend type introduced by vDPA kernel
> framework, which provides abstruction to the vDPA devices and
> exposes an unified control interface through a char dev.
> 
> This patch adds support to the vhost-vDPA backend. As similar to
> the existing vhost kernel backend, a set of virtio_user ops were
> introduced for vhost-vDPA backend to handle device specific operations
> such as:
>  - device setup
>  - ioctl message handling
>  - queue pair enabling
>  - dma map/unmap
> vDPA relevant ioctl codes and data structures are also defined in
> this patch.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin at redhat.com>
> ---
>  drivers/net/virtio/meson.build                |   1 +
>  drivers/net/virtio/virtio_user/vhost.h        |   1 +
>  drivers/net/virtio/virtio_user/vhost_vdpa.c   | 310 ++++++++++++++++++
>  .../net/virtio/virtio_user/virtio_user_dev.c  |   9 +-
>  4 files changed, 320 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/virtio/virtio_user/vhost_vdpa.c
> 
> diff --git a/drivers/net/virtio/meson.build
> b/drivers/net/virtio/meson.build
> index 3fd6051f4b..eaed46373d 100644
> --- a/drivers/net/virtio/meson.build
> +++ b/drivers/net/virtio/meson.build
> @@ -42,6 +42,7 @@ if is_linux

[snip]

> +static int
> +vhost_vdpa_ioctl(struct virtio_user_dev *dev,
> +		   enum vhost_user_request req,
> +		   void *arg)
> +{
> +	int ret = -1;
> +	uint64_t req_vdpa;
> +	struct vhost_memory_vdpa *vm = NULL;
> +
> +	PMD_DRV_LOG(INFO, "%s", vhost_msg_strings[req]);
> +
> +	req_vdpa = vhost_req_user_to_vdpa[req];
> +
> +	if (req_vdpa == VHOST_SET_MEM_TABLE)
> +		return vhost_vdpa_dma_map_all(dev);
> +
> +	if (req_vdpa == VHOST_SET_FEATURES) {
> +		/* WORKAROUND */
> +		*(uint64_t *)arg |= 1ULL << VIRTIO_F_IOMMU_PLATFORM;
> +
> +		/* Multiqueue not supported for now */
> +		*(uint64_t *)arg &= ~(1ULL << VIRTIO_NET_F_MQ);
> +	}
> +
> +	switch (req_vdpa) {
> +	case VHOST_SET_VRING_NUM:
> +	case VHOST_SET_VRING_ADDR:
> +	case VHOST_SET_VRING_BASE:
> +	case VHOST_GET_VRING_BASE:
> +	case VHOST_SET_VRING_KICK:
> +	case VHOST_SET_VRING_CALL:
> +		*(unsigned int *)arg = *(unsigned int *)arg;

Above line should be deleted?

> +		PMD_DRV_LOG(DEBUG, "vhostfd=%d, index=%u",
> +			    dev->vhostfd, *(unsigned int *)arg);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	ret = ioctl(dev->vhostfd, req_vdpa, arg);
> +
> +	if (vm)
> +		free(vm);

I think 'vm' is never changed after it's init-ed as NULL. Maybe it
should be deleted? If it's not needed, the definition of struct
vhost_memory_vdpa should also be deleted.

> +
> +	if (ret < 0)
> +		PMD_DRV_LOG(ERR, "%s failed: %s",
> +			    vhost_msg_strings[req], strerror(errno));
> +
> +	return ret;
> +}
> +
> +/**
> + * Set up environment to talk with a vhost vdpa backend.
> + *
> + * @return
> + *   - (-1) if fail to set up;
> + *   - (>=0) if successful.
> + */
> +static int
> +vhost_vdpa_setup(struct virtio_user_dev *dev)
> +{
> +	uint32_t did = (uint32_t)-1;

I see in kernel, 'did' should be u8:

static long vhost_vdpa_get_device_id(struct vhost_vdpa *v, u8 __user *argp)

So I think here did should be uint8_t?

Besides, there are two coding style issues:
CHECK:SPACING: spaces preferred around that '*' (ctx:WxV)
#236: FILE: drivers/net/virtio/virtio_user/vhost_vdpa.c:112:
+vhost_vdpa_dma_unmap(struct virtio_user_dev *dev, void __rte_unused *addr,
                                                                     ^

CHECK:OPEN_ENDED_LINE: Lines should not end with a '('
#298: FILE: drivers/net/virtio/virtio_user/vhost_vdpa.c:174:
+		int ret = rte_memseg_contig_walk_thread_unsafe(

Thanks!
Chenbo

> +
> +	dev->vhostfd = open(dev->path, O_RDWR);
> +	if (dev->vhostfd < 0) {
> +		PMD_DRV_LOG(ERR, "Failed to open %s: %s\n",
> +				dev->path, strerror(errno));
> +		return -1;
> +	}
> +
> +	if (ioctl(dev->vhostfd, VHOST_VDPA_GET_DEVICE_ID, &did) < 0 ||
> +			did != VIRTIO_ID_NETWORK) {
> +		PMD_DRV_LOG(ERR, "Invalid vdpa device ID: %u\n", did);
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +vhost_vdpa_enable_queue_pair(struct virtio_user_dev *dev,
> +			       uint16_t pair_idx,
> +			       int enable)
> +{
> +	int i;
> +
> +	if (dev->qp_enabled[pair_idx] == enable)
> +		return 0;
> +
> +	for (i = 0; i < 2; ++i) {
> +		struct vhost_vring_state state = {
> +			.index = pair_idx * 2 + i,
> +			.num   = enable,
> +		};
> +
> +		if (vhost_vdpa_ioctl(dev, VHOST_USER_SET_VRING_ENABLE, &state))
> +			return -1;
> +	}
> +
> +	dev->qp_enabled[pair_idx] = enable;
> +
> +	return 0;
> +}
> +
> +struct virtio_user_backend_ops virtio_ops_vdpa = {
> +	.setup = vhost_vdpa_setup,
> +	.send_request = vhost_vdpa_ioctl,
> +	.enable_qp = vhost_vdpa_enable_queue_pair,
> +	.dma_map = vhost_vdpa_dma_map,
> +	.dma_unmap = vhost_vdpa_dma_unmap,
> +};
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 2e097a95ea..2e8311147b 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -444,6 +444,12 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>  				dev->vhostfds[q] = -1;
>  				dev->tapfds[q] = -1;
>  			}
> +		} else if (dev->backend_type ==
> +				VIRTIO_USER_BACKEND_VHOST_VDPA) {
> +			dev->ops = &virtio_ops_vdpa;
> +		} else {
> +			PMD_DRV_LOG(ERR, "Unknown backend type");
> +			return -1;
>  		}
>  	}
> 
> @@ -878,7 +884,8 @@ virtio_user_update_status(struct virtio_user_dev *dev)
>  	enum virtio_user_backend_type backend_type =
>  				virtio_user_backend_type(dev->path);
> 
> -	if (backend_type != VIRTIO_USER_BACKEND_VHOST_USER)
> +	if (backend_type != VIRTIO_USER_BACKEND_VHOST_USER &&
> +			backend_type != VIRTIO_USER_BACKEND_VHOST_VDPA)
>  		return 0;
> 
>  	err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret);
> --
> 2.26.2