[dpdk-dev] [PATCH] vhost: change the vhost library to a common framework which can support more VIRTIO devices

[Yuanhan Liu]

On Wed, Sep 14, 2016 at 08:15:00PM +0800, Changpeng Liu wrote:
> For storage virtualization use cases, vhost-scsi becomes a more popular
> solution to support VMs. However a user space vhost-scsi-user solution
> does not exist currently. SPDK(Storage Performance Development Kit,
> https://github.com/spdk/spdk) will provide a user space vhost-scsi target
> to support multiple VMs through Qemu. Originally SPDK is built on top
> of DPDK libraries, so we would like to use DPDK vhost library as the
> communication channel between Qemu and vhost-scsi target application.
> 
> Currently DPDK vhost library can only support VIRTIO_ID_NET device type,
> we would like to extend the library to support VIRTIO_ID_SCSI and
> VIRTIO_ID_BLK. Most of DPDK vhost library can be reused only several
> differences:
> 1. VIRTIO SCSI device has different vring queues compared with VIRTIO NET
> device, at least 3 vring queues needed for SCSI device type;
> 2. VIRTIO SCSI will need several extra message operation code, such as
> SCSI_SET_ENDPIONT/SCSI_CLEAR_ENDPOINT;
> 
> First, we would like to extend DPDK vhost library as a common framework

I don't see how common it becomes with this patch applied.

> which be friendly to add other VIRTIO device types, to implement this feature,
> we add a new data structure virtio_dev, which can deliver socket messages
> to different VIRTIO devices, each specific VIRTIO device will register
> callback to virtio_dev.
> 
> Secondly, we would to upstream a patch to Qemu community to add vhost-scsi
> specific operation command such as SCSI_SET_ENDPOINT and SCSI_CLEAR_ENDOINT,
> and user space feature bits.
> 
> Finally, after the Qemu patch set was merged, we will add VIRTIO_ID_SCSI
> support to DPDK vhost library

You actually should send this part out with this patchset. You are making
changes for adding the vhost-scsi support, however, you don't show us how
the code to support vhost-scsi looks like. That means, it's hard for us to
understand why you are doing those changes.

What I said is DPDK will not consider merging vhost-scsi patches unless
QEMU have merged the vhost-scsi part. This doesn't mean you can't send
out the DPDK vhost-scsi patches before that.

> and an example vhost-scsi target which can
> add a SCSI device to VM through this example application.
> 
> This patch set changed the vhost library as a common framework which
> can add other VIRTIO device type in future.
> 
> Signed-off-by: Changpeng Liu <changpeng.liu at intel.com>
> ---
>  lib/librte_vhost/Makefile         |   4 +-
>  lib/librte_vhost/rte_virtio_dev.h | 140 ++++++++
>  lib/librte_vhost/rte_virtio_net.h |  97 +-----

rte_virtio_net.h is the header file will be exported for applications.
Every change there would mean either an API or ABI breakage. Thus, we
should try to avoid touching it. Don't even to say you added yet another
header file, rte_virtio_dev.h.

I confess that the rte_virtio_net.h filename isn't that right: it sticks
to virtio-net so tightly. We may could rename it to rte_vhost.h, but I
doubt it's worthwhile: as said, it breaks the API.

The fortunate thing about this file is that, the context is actually not
sticking to virtio-net too much. I mean, all the APIs are using the "vid",
which is just a number. Well, except the virtio_net_device_ops() structure,
which also should be renamed to vhost_device_ops(). Besides that, the
three ops, "new_device", "destroy_device" and "vring_state_changed", are
actually not limited to virtio-net device.

That is to say, we could have two options here:

- rename the header file and the structure properly, to not limited to
  virtio-net

- live with it, let it be a legacy issue, and document it at somewhere,
  say, "due to history reason, that virtio-net is the first one supported
  in DPDK, we kept the header filename as rte_virtio_net.h, but not ..."

I personally would prefer the later one, which saves us from breaking
applications again. I don't have strong objection to the first one though.

Thomas, any comments?

>  lib/librte_vhost/socket.c         |   6 +-
>  lib/librte_vhost/vhost.c          | 421 ------------------------
>  lib/librte_vhost/vhost.h          | 288 -----------------
>  lib/librte_vhost/vhost_device.h   | 230 +++++++++++++
>  lib/librte_vhost/vhost_net.c      | 659 ++++++++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost_net.h      | 126 ++++++++
>  lib/librte_vhost/vhost_user.c     | 451 +++++++++++++-------------
>  lib/librte_vhost/vhost_user.h     |  17 +-
>  lib/librte_vhost/virtio_net.c     |  37 ++-

That basically means you are heading the wrong way. For example,

> +struct virtio_dev_table {
> +	int (*vhost_dev_ready)(struct virtio_dev *dev);
> +	struct vhost_virtqueue* (*vhost_dev_get_queues)(struct virtio_dev *dev, uint16_t queue_id);
> +	void (*vhost_dev_cleanup)(struct virtio_dev *dev, int destroy);
> +	void (*vhost_dev_free)(struct virtio_dev *dev);
> +	void (*vhost_dev_reset)(struct virtio_dev *dev);
> +	uint64_t (*vhost_dev_get_features)(struct virtio_dev *dev);
> +	int (*vhost_dev_set_features)(struct virtio_dev *dev, uint64_t features);
> +	uint64_t (*vhost_dev_get_protocol_features)(struct virtio_dev *dev);
> +	int (*vhost_dev_set_protocol_features)(struct virtio_dev *dev, uint64_t features);
> +	uint32_t (*vhost_dev_get_default_queue_num)(struct virtio_dev *dev);
> +	uint32_t (*vhost_dev_get_queue_num)(struct virtio_dev *dev);
> +	uint16_t (*vhost_dev_get_avail_entries)(struct virtio_dev *dev, uint16_t queue_id);
> +	int (*vhost_dev_get_vring_base)(struct virtio_dev *dev, struct vhost_virtqueue *vq);
> +	int (*vhost_dev_set_vring_num)(struct virtio_dev *dev, struct vhost_virtqueue *vq);
> +	int (*vhost_dev_set_vring_call)(struct virtio_dev *dev, struct vhost_vring_file *file);
> +	int (*vhost_dev_set_log_base)(struct virtio_dev *dev, int fd, uint64_t size, uint64_t off);
> +};

This looks wrong. Most of them (if not all) should be same, regardless
it's for virtio-net, or virtio-scsi. I don't understand why you should
even touch this. Those are for handling *vhost-user* messages, but not
virtio-net, nor virtio-scsi. They should be same no matter which virtio
device we are dealing with. Well, virtio-scsi may just have few more
messages than virtio-net.

	--yliu