From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by dpdk.org (Postfix) with ESMTP id 7C42723C for ; Sun, 1 Apr 2018 21:53:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Apr 2018 12:53:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,392,1517904000"; d="scan'208";a="30206288" Received: from irsmsx101.ger.corp.intel.com ([163.33.3.153]) by orsmga008.jf.intel.com with ESMTP; 01 Apr 2018 12:53:21 -0700 Received: from irsmsx156.ger.corp.intel.com (10.108.20.68) by IRSMSX101.ger.corp.intel.com (163.33.3.153) with Microsoft SMTP Server (TLS) id 14.3.319.2; Sun, 1 Apr 2018 20:53:20 +0100 Received: from irsmsx101.ger.corp.intel.com ([169.254.1.176]) by IRSMSX156.ger.corp.intel.com ([169.254.3.229]) with mapi id 14.03.0319.002; Sun, 1 Apr 2018 20:53:20 +0100 From: "Zhang, Roy Fan" To: "Wodkowski, PawelX" , "dev@dpdk.org" CC: "maxime.coquelin@redhat.com" , "jianjay.zhou@huawei.com" , "Tan, Jianfeng" Thread-Topic: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend support Thread-Index: AQHTx1zzACb9fMOvC0GE3IlgaGuhWaPnKYIAgAUc3AA= Date: Sun, 1 Apr 2018 19:53:19 +0000 Message-ID: <9F7182E3F746AB4EA17801C148F3C60433107799@IRSMSX101.ger.corp.intel.com> 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> In-Reply-To: Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjNlY2M0ZGYtOGE1ZC00YzJmLTg0ZTYtODVhNTU4ZmU2ZDg4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjBBNldyaFV2ZjhLWVZNd24zRzBGY1wvM2VhckR5TTJ4SlwvSXNxOXRZYjNHaz0ifQ== dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external backend support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 01 Apr 2018 19:53:25 -0000 Hi Pawel, > -----Original Message----- > From: Wodkowski, PawelX > Sent: Thursday, March 29, 2018 2:48 PM > To: Zhang, Roy Fan ; dev@dpdk.org > Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Tan, Jianfeng > > Subject: RE: [dpdk-dev] [PATCH v4 1/8] lib/librte_vhost: add external > backend support >=20 > > -----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 > > > > 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 > > --- > > 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. >=20 > No payload parameter. Sorry about that. I will fix the comment. >=20 > > + * @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. >=20 > No payload parameter :) >=20 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); > > + >=20 > 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 back= end may change the payload.=20 >=20 > Also you can easily merge pre and post handlers into one handler with one > Parameter describing what phase of message processing we are now. >=20 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.=20 > > +/** > > + * 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_i= dx); > > */ > > 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 > > @@ -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 =3D get_device(vid); > > + if (dev =3D=3D NULL) > > + return -1; > > + > > + if (ops) > > + rte_memcpy(&dev->extern_ops, ops, sizeof(*ops)); > > + > > + return 0; > > +} >=20 > 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. >=20 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.=20 > 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. Plu= s the patch you mentioned won't help with the pre and post handlers problem -= =20 or it would consume all of two remaining reserved fields in vhost_user_ops structure for pre and post handlers, respectively. >=20 > > 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 > > @@ -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", > > }; > > > > 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; > > > > dev =3D get_device(vid); > > if (dev =3D=3D 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 =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; >=20 > This can be moved inside above if () { } Yes, you are right. >=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; > > > > + if (need_reply) > > + send_vhost_reply(fd, &msg); > > } > > > > +skip_to_reply: > > if (unlock_required) > > vhost_user_unlock_all_queue_pairs(dev); > > > > -- > > 2.7.4 >=20 > Overall, I think, this direction where we need to go. >=20 > Pawel Regards, Fan