From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Wodkowski, PawelX" Subject: Re: [PATCH v4 1/8] lib/librte_vhost: add external backend support Date: Thu, 29 Mar 2018 13:47:38 +0000 Message-ID: References: <20180326095114.11605-1-roy.fan.zhang@intel.com> <1522327975-28769-1-git-send-email-roy.fan.zhang@intel.com> <1522327975-28769-2-git-send-email-roy.fan.zhang@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: "maxime.coquelin@redhat.com" , "jianjay.zhou@huawei.com" , "Tan, Jianfeng" To: "Zhang, Roy Fan" , "dev@dpdk.org" Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 7866E2B82 for ; Thu, 29 Mar 2018 15:47:42 +0200 (CEST) In-Reply-To: <1522327975-28769-2-git-send-email-roy.fan.zhang@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----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 > > Subject: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend > support >=20 > This patch adds external backend support to vhost library. The patch prov= ides > new APIs for the external backend to register pre and post vhost-user > message > handlers. >=20 > Signed-off-by: Fan Zhang > --- > 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(-) >=20 > 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 > */ >=20 > #ifndef _RTE_VHOST_H_ > @@ -88,6 +88,55 @@ struct vhost_device_ops { > }; >=20 > /** > + * function prototype for the vhost backend to handler specific vhost us= er > + * 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 writ= ten 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 us= er > + * 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 writ= ten 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 m= sg '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); >=20 > +/** > + * 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; >=20 > } 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 > */ >=20 > #include > @@ -627,3 +627,18 @@ rte_vhost_rx_queue_count(int vid, uint16_t qid) >=20 > 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 =3D get_device(vid); > + if (dev =3D=3D 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=20 (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=20 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 > */ >=20 > #ifndef _VHOST_NET_CDEV_H_ > @@ -241,8 +241,12 @@ struct virtio_net { > struct guest_page *guest_pages; >=20 > int slave_req_fd; > -} __rte_cache_aligned; >=20 > + /* 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; >=20 > #define VHOST_LOG_PAGE 4096 >=20 > 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 > */ >=20 > #include > @@ -50,6 +50,8 @@ static const char > *vhost_message_str[VHOST_USER_MAX] =3D { > [VHOST_USER_NET_SET_MTU] =3D "VHOST_USER_NET_SET_MTU", > [VHOST_USER_SET_SLAVE_REQ_FD] =3D > "VHOST_USER_SET_SLAVE_REQ_FD", > [VHOST_USER_IOTLB_MSG] =3D "VHOST_USER_IOTLB_MSG", > + [VHOST_USER_CRYPTO_CREATE_SESS] =3D > "VHOST_USER_CRYPTO_CREATE_SESS", > + [VHOST_USER_CRYPTO_CLOSE_SESS] =3D > "VHOST_USER_CRYPTO_CLOSE_SESS", > }; >=20 > static uint64_t > @@ -1302,6 +1304,7 @@ vhost_user_msg_handler(int vid, int fd) > struct VhostUserMsg msg; > int ret; > int unlock_required =3D 0; > + uint32_t skip_master =3D 0; >=20 > dev =3D get_device(vid); > if (dev =3D=3D NULL) > @@ -1379,6 +1382,21 @@ vhost_user_msg_handler(int vid, int fd) >=20 > } >=20 > + if (dev->extern_ops.pre_msg_handle) { > + uint32_t need_reply; > + > + ret =3D (*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 () { }=20 > + > switch (msg.request.master) { > case VHOST_USER_GET_FEATURES: > msg.payload.u64 =3D vhost_user_get_features(dev); > @@ -1479,9 +1497,22 @@ vhost_user_msg_handler(int vid, int fd) > default: > ret =3D -1; > break; > + } > + > +skip_to_post_handle: > + if (dev->extern_ops.post_msg_handle) { > + uint32_t need_reply; > + > + ret =3D (*dev->extern_ops.post_msg_handle)( > + dev->vid, (void *)&msg, &need_reply); > + if (ret < 0) > + goto skip_to_reply; >=20 > + if (need_reply) > + send_vhost_reply(fd, &msg); > } >=20 > +skip_to_reply: > if (unlock_required) > vhost_user_unlock_all_queue_pairs(dev); >=20 > -- > 2.7.4 Overall, I think, this direction where we need to go. Pawel