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