[dpdk-dev,v4,1/8] lib/librte_vhost: add external backend support
Checks
Commit Message
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(-)
Comments
> -----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.
> + * @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
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
Hi Pawel, Fan,
On 04/01/2018 09:53 PM, Zhang, Roy Fan wrote:
> 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 @@
<snip/>
>
>>> + * @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.
Time is running out for v18.05 integration deadline (April 6th), and we
haven't reached a consensus.
Except this API point, I think vhost-crypto is at the right level.
Since vhost-crypto lives in librte_vhost, I propose Fan cooks an
intermediate solution that does not need API change.
Doing this, we postpone the API change to v18.08, so we have time to
discuss what the right API should be. Once agreed, vhost-crypto moves to
the new API.
Pawel, Jianfeng, Fan, is it fine for you?
Thanks,
Maxime
Hi Maxime,
No problem. I will work on that.
Pawel, Jianfeng, if you guys have other concerns or suggestions, please give me a shout.
Thanks a lot guys, for the review and help!
Regards,
Fan
> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Tuesday, April 3, 2018 2:45 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski@intel.com>; dev@dpdk.org
> Cc: 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
>
> Hi Pawel, Fan,
>
> On 04/01/2018 09:53 PM, Zhang, Roy Fan wrote:
> > 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 @@
>
> <snip/>
>
> >
> >>> + * @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.
>
> Time is running out for v18.05 integration deadline (April 6th), and we haven't
> reached a consensus.
>
> Except this API point, I think vhost-crypto is at the right level.
> Since vhost-crypto lives in librte_vhost, I propose Fan cooks an intermediate
> solution that does not need API change.
>
> Doing this, we postpone the API change to v18.08, so we have time to discuss
> what the right API should be. Once agreed, vhost-crypto moves to the new
> API.
>
> Pawel, Jianfeng, Fan, is it fine for you?
>
> Thanks,
> Maxime
On 4/3/2018 9:44 PM, Maxime Coquelin wrote:
> Hi Pawel, Fan,
>
> On 04/01/2018 09:53 PM, Zhang, Roy Fan wrote:
>> 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 @@
>
> <snip/>
>
>>
>>>> + * @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.
>
> Time is running out for v18.05 integration deadline (April 6th), and
> we haven't reached a consensus.
>
> Except this API point, I think vhost-crypto is at the right level.
> Since vhost-crypto lives in librte_vhost, I propose Fan cooks an
> intermediate solution that does not need API change.
>
> Doing this, we postpone the API change to v18.08, so we have time to
> discuss what the right API should be. Once agreed, vhost-crypto moves to
> the new API.
>
> Pawel, Jianfeng, Fan, is it fine for you?
+1. This can avoid blocking this patch set, and give more time for
discussing new APIs and external structs.
Thanks,
Jianfeng
>
> Thanks,
> Maxime
> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Tuesday, April 3, 2018 4:43 PM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Wodkowski, PawelX
> <pawelx.wodkowski@intel.com>; dev@dpdk.org
> Cc: jianjay.zhou@huawei.com
> Subject: Re: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external
> backend support
>
>
>
> On 4/3/2018 9:44 PM, Maxime Coquelin wrote:
> > Hi Pawel, Fan,
> >
> > On 04/01/2018 09:53 PM, Zhang, Roy Fan wrote:
> >> 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 @@
> >
> > <snip/>
> >
> >>
> >>>> + * @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.
> >
> > Time is running out for v18.05 integration deadline (April 6th), and
> > we haven't reached a consensus.
> >
> > Except this API point, I think vhost-crypto is at the right level.
> > Since vhost-crypto lives in librte_vhost, I propose Fan cooks an
> > intermediate solution that does not need API change.
> >
> > Doing this, we postpone the API change to v18.08, so we have time to
> > discuss what the right API should be. Once agreed, vhost-crypto moves to
> > the new API.
> >
> > Pawel, Jianfeng, Fan, is it fine for you?
>
> +1. This can avoid blocking this patch set, and give more time for
> discussing new APIs and external structs.
>
> Thanks,
> Jianfeng
>
> >
> > Thanks,
> > Maxime
Fine for me too.
Pawel
@@ -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.
+ * @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.
+ * @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);
+
+/**
+ * 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
@@ -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;
@@ -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;
+}
@@ -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
@@ -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;
+
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);