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

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

On 9/24/20 7:25 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>> Sent: Friday, September 11, 2020 11:08 PM
>> To: dev@dpdk.org; Fu, Patrick <patrick.fu@intel.com>; amorenoz@redhat.com
>> Cc: Maxime Coquelin <maxime.coquelin@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@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?

Yes!

>> +		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.

Correct, will remove in v2.

>> +
>> +	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,

Not sure about this one.

                                                                     ^
> 
> 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(

Same here.

> Thanks!
> Chenbo