[dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend support

[Wodkowski, PawelX]

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Fan Zhang
> Sent: Thursday, March 29, 2018 2:53 PM
> To: dev at dpdk.org
> Cc: maxime.coquelin at redhat.com; jianjay.zhou at huawei.com; Tan, Jianfeng
> <jianfeng.tan at intel.com>
> Subject: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend
> support
> 
> This patch adds external backend support to vhost library. The patch provides
> new APIs for the external backend to register pre and post vhost-user
> message
> handlers.
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang at intel.com>
> ---
>  lib/librte_vhost/rte_vhost.h           | 64
> +++++++++++++++++++++++++++++++++-
>  lib/librte_vhost/rte_vhost_version.map |  6 ++++
>  lib/librte_vhost/vhost.c               | 17 ++++++++-
>  lib/librte_vhost/vhost.h               |  8 +++--
>  lib/librte_vhost/vhost_user.c          | 33 +++++++++++++++++-
>  5 files changed, 123 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index d332069..b902c44 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2010-2017 Intel Corporation
> + * Copyright(c) 2010-2018 Intel Corporation
>   */
> 
>  #ifndef _RTE_VHOST_H_
> @@ -88,6 +88,55 @@ struct vhost_device_ops {
>  };
> 
>  /**
> + * function prototype for the vhost backend to handler specific vhost user
> + * messages prior to the master message handling
> + *
> + * @param vid
> + *  vhost device id
> + * @param msg
> + *  Message pointer.
> + * @param payload
> + *  Message payload.

No payload parameter.

> + * @param require_reply
> + *  If the handler requires sending a reply, this varaible shall be written 1,
> + *  otherwise 0.
> + * @param skip_master
> + *  If the handler requires skipping the master message handling, this
> variable
> + *  shall be written 1, otherwise 0.
> + * @return
> + *  0 on success, -1 on failure
> + */
> +typedef int (*rte_vhost_msg_pre_handle)(int vid, void *msg,
> +		uint32_t *require_reply, uint32_t *skip_master);
> +
> +/**
> + * function prototype for the vhost backend to handler specific vhost user
> + * messages after the master message handling is done
> + *
> + * @param vid
> + *  vhost device id
> + * @param msg
> + *  Message pointer.
> + * @param payload
> + *  Message payload.

No payload parameter :)

> + * @param require_reply
> + *  If the handler requires sending a reply, this varaible shall be written 1,
> + *  otherwise 0.
> + * @return
> + *  0 on success, -1 on failure
> + */
> +typedef int (*rte_vhost_msg_post_handle)(int vid, void *msg,
> +		uint32_t *require_reply);
> +

What mean 'Message pointer' Is this const for us? Is this payload? Making msg 'void *' is not a
way to go here. Those pre and post handlers need to see exactly the same
structures like vhost_user.c file. Otherwise we can get into troubles when ABI
changes.

Also you can easily merge pre and post handlers into one handler with one
Parameter describing what phase of message processing we are now.

> +/**
> + * pre and post vhost user message handlers
> + */
> +struct vhost_user_extern_ops {
> +	rte_vhost_msg_pre_handle pre_msg_handle;
> +	rte_vhost_msg_post_handle post_msg_handle;
> +};
> +
> +/**
>   * Convert guest physical address to host virtual address
>   *
>   * @param mem
> @@ -434,6 +483,19 @@ int rte_vhost_vring_call(int vid, uint16_t vring_idx);
>   */
>  uint32_t rte_vhost_rx_queue_count(int vid, uint16_t qid);
> 
> +/**
> + * register external vhost backend
> + *
> + * @param vid
> + *  vhost device ID
> + * @param ops
> + *  ops that process external vhost user messages
> + * @return
> + *  0 on success, -1 on failure
> + */
> +int
> +rte_vhost_user_register_extern_ops(int vid, struct
> vhost_user_extern_ops *ops);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/librte_vhost/rte_vhost_version.map
> b/lib/librte_vhost/rte_vhost_version.map
> index df01031..91bf9f0 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -59,3 +59,9 @@ DPDK_18.02 {
>  	rte_vhost_vring_call;
> 
>  } DPDK_17.08;
> +
> +DPDK_18.05 {
> +	global:
> +
> +	rte_vhost_user_register_extern_ops;
> +} DPDK_18.02;
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index a407067..80af341 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2010-2016 Intel Corporation
> + * Copyright(c) 2010-2018 Intel Corporation
>   */
> 
>  #include <linux/vhost.h>
> @@ -627,3 +627,18 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid)
> 
>  	return *((volatile uint16_t *)&vq->avail->idx) - vq->last_avail_idx;
>  }
> +
> +int
> +rte_vhost_user_register_extern_ops(int vid, struct
> vhost_user_extern_ops *ops)
> +{
> +	struct virtio_net *dev;
> +
> +	dev = get_device(vid);
> +	if (dev == NULL)
> +		return -1;
> +
> +	if (ops)
> +		rte_memcpy(&dev->extern_ops, ops, sizeof(*ops));
> +
> +	return 0;
> +}

Why we need this new "register" API? Why can't you use one of the 
(struct vhost_device_ops).reserved[0] field to put this callback there?
I think this is right time to utilize this field.

Can you do something similar to 
http://dpdk.org/ml/archives/dev/2018-March/094213.html ?

> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index d947bc9..2072b88 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2010-2014 Intel Corporation
> + * Copyright(c) 2010-2018 Intel Corporation
>   */
> 
>  #ifndef _VHOST_NET_CDEV_H_
> @@ -241,8 +241,12 @@ struct virtio_net {
>  	struct guest_page       *guest_pages;
> 
>  	int			slave_req_fd;
> -} __rte_cache_aligned;
> 
> +	/* private data for external virtio device */
> +	void			*extern_data;
> +	/* pre and post vhost user message handlers for externel backend */
> +	struct vhost_user_extern_ops extern_ops;
> +} __rte_cache_aligned;
> 
>  #define VHOST_LOG_PAGE	4096
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 90ed211..ede8a5e 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1,5 +1,5 @@
>  /* SPDX-License-Identifier: BSD-3-Clause
> - * Copyright(c) 2010-2016 Intel Corporation
> + * Copyright(c) 2010-2018 Intel Corporation
>   */
> 
>  #include <stdint.h>
> @@ -50,6 +50,8 @@ static const char
> *vhost_message_str[VHOST_USER_MAX] = {
>  	[VHOST_USER_NET_SET_MTU]  = "VHOST_USER_NET_SET_MTU",
>  	[VHOST_USER_SET_SLAVE_REQ_FD]  =
> "VHOST_USER_SET_SLAVE_REQ_FD",
>  	[VHOST_USER_IOTLB_MSG]  = "VHOST_USER_IOTLB_MSG",
> +	[VHOST_USER_CRYPTO_CREATE_SESS] =
> "VHOST_USER_CRYPTO_CREATE_SESS",
> +	[VHOST_USER_CRYPTO_CLOSE_SESS] =
> "VHOST_USER_CRYPTO_CLOSE_SESS",
>  };
> 
>  static uint64_t
> @@ -1302,6 +1304,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	struct VhostUserMsg msg;
>  	int ret;
>  	int unlock_required = 0;
> +	uint32_t skip_master = 0;
> 
>  	dev = get_device(vid);
>  	if (dev == NULL)
> @@ -1379,6 +1382,21 @@ vhost_user_msg_handler(int vid, int fd)
> 
>  	}
> 
> +	if (dev->extern_ops.pre_msg_handle) {
> +		uint32_t need_reply;
> +
> +		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
> +				(void *)&msg, &need_reply, &skip_master);
> +		if (ret < 0)
> +			goto skip_to_reply;
> +
> +		if (need_reply)
> +			send_vhost_reply(fd, &msg);
> +	}
> +
> +	if (skip_master)
> +		goto skip_to_post_handle;

This can be moved inside above  if () { } 

> +
>  	switch (msg.request.master) {
>  	case VHOST_USER_GET_FEATURES:
>  		msg.payload.u64 = vhost_user_get_features(dev);
> @@ -1479,9 +1497,22 @@ vhost_user_msg_handler(int vid, int fd)
>  	default:
>  		ret = -1;
>  		break;
> +	}
> +
> +skip_to_post_handle:
> +	if (dev->extern_ops.post_msg_handle) {
> +		uint32_t need_reply;
> +
> +		ret = (*dev->extern_ops.post_msg_handle)(
> +				dev->vid, (void *)&msg, &need_reply);
> +		if (ret < 0)
> +			goto skip_to_reply;
> 
> +		if (need_reply)
> +			send_vhost_reply(fd, &msg);
>  	}
> 
> +skip_to_reply:
>  	if (unlock_required)
>  		vhost_user_unlock_all_queue_pairs(dev);
> 
> --
> 2.7.4

Overall, I think, this direction where we need to go.

Pawel