Re: [PATCH v4 1/8] lib/librte_vhost: add external backend support

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Pawel,

> -----Original Message-----
> From: Wodkowski, PawelX
> Sent: Thursday, March 29, 2018 2:48 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Tan, Jianfeng
> <jianfeng.tan@intel.com>
> Subject: RE: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external
> backend support
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Fan Zhang
> > Sent: Thursday, March 29, 2018 2:53 PM
> > To: dev@dpdk.org
> > Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Tan,
> Jianfeng
> > <jianfeng.tan@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@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.
Sorry about that. I will fix the comment.

> 
> > + * @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 :)
> 

Same here

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

It is the pointer to the vhost_user message. It cannot be const as the backend
may change the payload. 

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

No I don't think so. To do so it will be quite unclear in the future as we are
using one function to do two totally different things. 

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

The patch here is a more generic and intuitive way for external backend to
register the handlers to process the vhost user message only recognized by it.
Please read Maxime's comments in v2 version of this patch.
http://dpdk.org/ml/archives/dev/2018-March/093408.html.
As we discussed we need 2 different handlers for external vhost user device
to handle device specifc vhost user messages. A public API is needed. 

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

The patch content here causes the least damage to the existing library. Plus
the patch you mentioned won't help with the pre and post handlers problem - 
or it would consume all of two remaining reserved fields in vhost_user_ops
structure for pre and post handlers, respectively.

> 
> > 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 () { }

Yes, you are right.

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

Regards,
Fan