From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 6F0E623D for ; Thu, 29 Mar 2018 06:17:28 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga106.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 21:17:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,375,1517904000"; d="scan'208";a="27896654" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga007.fm.intel.com with ESMTP; 28 Mar 2018 21:17:27 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Mar 2018 21:17:26 -0700 Received: from shsmsx102.ccr.corp.intel.com (10.239.4.154) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Wed, 28 Mar 2018 21:17:26 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.235]) by shsmsx102.ccr.corp.intel.com ([169.254.2.80]) with mapi id 14.03.0319.002; Thu, 29 Mar 2018 12:17:23 +0800 From: "Liu, Changpeng" To: "Tan, Jianfeng" , "Zhang, Roy Fan" , "dev@dpdk.org" CC: "maxime.coquelin@redhat.com" , "jianjay.zhou@huawei.com" , "Wodkowski, PawelX" , "Stojaczyk, DariuszX" , "Kulasek, TomaszX" Thread-Topic: [dpdk-dev] [PATCH v3 01/10] lib/librte_vhost: add external backend support Thread-Index: AQHTxwM6rZCiSrIgVE6XHN02fj+d4qPmmgyA Date: Thu, 29 Mar 2018 04:17:23 +0000 Message-ID: References: <20180326095114.11605-1-roy.fan.zhang@intel.com> <20180326095114.11605-2-roy.fan.zhang@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMDc0Yzg2MWItOTFiZS00NzE0LTg4NjUtOGYyNWJmZjRlZTJiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE2LjUuOS4zIiwiVHJ1c3RlZExhYmVsSGFzaCI6IjdPVFJPSDBZUkhHVjZydGJUMlY5YTZHYkdyRktKTVZHTVA2QlA0TUpxb2c9In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v3 01/10] 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: Thu, 29 Mar 2018 04:17:30 -0000 > -----Original Message----- > From: Tan, Jianfeng > Sent: Thursday, March 29, 2018 10:11 AM > To: Zhang, Roy Fan ; dev@dpdk.org > Cc: maxime.coquelin@redhat.com; jianjay.zhou@huawei.com; Liu, Changpeng > ; Wodkowski, PawelX > ; Stojaczyk, DariuszX > ; Kulasek, TomaszX > Subject: Re: [dpdk-dev] [PATCH v3 01/10] lib/librte_vhost: add external b= ackend > support >=20 >=20 > It's interesting that we add some new APIs to be used by the > lib/librte_vhost/ itself. I can understand as we planned to not put > vhost crypto into the lib. >=20 > As vhost crypto is not a real "external backend", we could ask opinion > of a real external backend if these are really necessary. pre and post > message handlers would be OK. But do we really need register private > data from external backend? @Changpeng @Pawel @Dariusz @Tomasz. For now I'm not sure whether we need a private data structure. But why put post_vhost_user_msg_handler into default section ? >=20 > BTW, external backend sounds a little exclusive :-), does extended > backend sound better? >=20 >=20 > On 3/26/2018 5:51 PM, Fan Zhang wrote: > > This patch adds external backend support to vhost library. The patch pr= ovides > > new APIs for the external backend to register private data, plus pre an= d post > > vhost-user message handlers. > > > > Signed-off-by: Fan Zhang > > --- > > lib/librte_vhost/rte_vhost.h | 45 > ++++++++++++++++++++++++++++++++++++++++++- > > lib/librte_vhost/vhost.c | 23 +++++++++++++++++++++- > > lib/librte_vhost/vhost.h | 8 ++++++-- > > lib/librte_vhost/vhost_user.c | 29 +++++++++++++++++++++++++--- > > 4 files changed, 98 insertions(+), 7 deletions(-) > > > > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.= h > > index d332069..591b731 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,33 @@ struct vhost_device_ops { > > }; > > > > /** > > + * function prototype for external virtio device to handler device spe= cific >=20 > handler -> handle >=20 > > + * vhost user messages > > + * > > + * @param extern_data > > + * private data for external backend >=20 > There is not such parameter in below function type. >=20 > > + * @param msg > > + * Message pointer > > + * @param payload > > + * Message payload >=20 > Ditto. >=20 > > + * @param require_reply > > + * If the handler requires sending a reply, this varaible shall be wr= itten 1, > > + * otherwise 0 > > + * @return > > + * 0 on success, -1 on failure > > + */ > > +typedef int (*rte_vhost_msg_handler)(int vid, void *msg, > > + uint32_t *require_reply); > > + > > +/** > > + * pre and post vhost user message handlers > > + */ > > +struct rte_vhost_user_dev_extern_ops { >=20 > Considering the original vhost_device_ops, does vhost_user_extern_ops > sound better? >=20 > > + rte_vhost_msg_handler pre_vhost_user_msg_handler; > > + rte_vhost_msg_handler post_vhost_user_msg_handler; > > +}; > > + > > +/** > > * Convert guest physical address to host virtual address > > * > > * @param mem > > @@ -434,6 +461,22 @@ 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 extern_data > > + * private data for external backend > > + * @param ops > > + * ops that process external vhost user messages > > + * @return > > + * 0 on success, -1 on failure > > + */ > > +int > > +rte_vhost_user_register_extern_backend(int vid, void *extern_data, > > + struct rte_vhost_user_dev_extern_ops *ops); >=20 > Considering the original rte_vhost_driver_callback_register, does > rte_vhost_message_handler_register sound better? >=20 > For extern_data, as mentioned in the head, let's discuss if it's > necessary to be registered through API. >=20 > > + > > #ifdef __cplusplus > > } > > #endif > > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c > > index a407067..0932537 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,24 @@ 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_backend(int vid, void *extern_data, > > + struct rte_vhost_user_dev_extern_ops *ops) > > +{ > > + struct virtio_net *dev; >=20 > Do we want to rename this internal structure to something like > vhost_dev, if it contains not only information for net? >=20 > > + > > + dev =3D get_device(vid); > > + if (dev =3D=3D NULL) > > + return -1; > > + > > + dev->extern_data =3D extern_data; > > + if (ops) { > > + dev->extern_ops.pre_vhost_user_msg_handler =3D > > + ops->pre_vhost_user_msg_handler; > > + dev->extern_ops.post_vhost_user_msg_handler =3D > > + ops->post_vhost_user_msg_handler; > > + } > > + > > + return 0; > > +} > > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > > index d947bc9..6aaa46c 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 rte_vhost_user_dev_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_use= r.c > > index 90ed211..c064cb3 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", >=20 > Please leave this patch device agnostic. Put these into crypto related > patches. >=20 > > }; > > > > static uint64_t > > @@ -1379,6 +1381,18 @@ vhost_user_msg_handler(int vid, int fd) > > > > } > > > > + if (dev->extern_ops.pre_vhost_user_msg_handler) { > > + uint32_t need_reply; > > + > > + ret =3D (*dev->extern_ops.pre_vhost_user_msg_handler)(dev->vid, >=20 > We have a variable vid, why use dev->vid? >=20 > > + (void *)&msg, &need_reply); > > + if (ret < 0) > > + goto skip_to_reply; > > + > > + if (need_reply) > > + send_vhost_reply(fd, &msg); >=20 > Do we have case that, if device handles that, we don't need to common > handle and post handle below? In other words, how to handle overlapping > of message handle? >=20 > > + } > > + > > switch (msg.request.master) { > > case VHOST_USER_GET_FEATURES: > > msg.payload.u64 =3D vhost_user_get_features(dev); > > @@ -1477,11 +1491,20 @@ vhost_user_msg_handler(int vid, int fd) > > break; > > > > default: > > - ret =3D -1; > > - break; > > + if (dev->extern_ops.post_vhost_user_msg_handler) { >=20 > Do we allow overlapping of common and post handle? >=20 > > + uint32_t need_reply; > > > > + ret =3D (*dev->extern_ops.post_vhost_user_msg_handler)( > > + dev->vid, (void *)&msg, &need_reply); > > + > > + if (need_reply) > > + send_vhost_reply(fd, &msg); > > + } else > > + ret =3D -1; > > + break; > > } > > > > +skip_to_reply: > > if (unlock_required) > > vhost_user_unlock_all_queue_pairs(dev); > >