From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id E176427D for ; Thu, 29 Mar 2018 04:11:24 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 28 Mar 2018 19:11:22 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,374,1517904000"; d="scan'208";a="28456923" Received: from tanjianf-mobl.ccr.corp.intel.com (HELO [10.67.64.69]) ([10.67.64.69]) by fmsmga008.fm.intel.com with ESMTP; 28 Mar 2018 19:11:21 -0700 To: Fan Zhang , dev@dpdk.org References: <20180326095114.11605-1-roy.fan.zhang@intel.com> <20180326095114.11605-2-roy.fan.zhang@intel.com> Cc: maxime.coquelin@redhat.com, jianjay.zhou@huawei.com, "Liu, Changpeng" , "Wodkowski, PawelX" , "Stojaczyk, DariuszX" , "Kulasek, TomaszX" From: "Tan, Jianfeng" Message-ID: Date: Thu, 29 Mar 2018 10:11:20 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20180326095114.11605-2-roy.fan.zhang@intel.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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 02:11:26 -0000 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. 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. BTW, external backend sounds a little exclusive :-), does extended backend sound better? On 3/26/2018 5:51 PM, Fan Zhang wrote: > This patch adds external backend support to vhost library. The patch provides > new APIs for the external backend to register private data, plus pre and 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 specific handler -> handle > + * vhost user messages > + * > + * @param extern_data > + * private data for external backend There is not such parameter in below function type. > + * @param msg > + * Message pointer > + * @param payload > + * Message payload Ditto. > + * @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_handler)(int vid, void *msg, > + uint32_t *require_reply); > + > +/** > + * pre and post vhost user message handlers > + */ > +struct rte_vhost_user_dev_extern_ops { Considering the original vhost_device_ops, does vhost_user_extern_ops sound better? > + 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_idx); > */ > 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); Considering the original rte_vhost_driver_callback_register, does rte_vhost_message_handler_register sound better? For extern_data, as mentioned in the head, let's discuss if it's necessary to be registered through API. > + > #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; Do we want to rename this internal structure to something like vhost_dev, if it contains not only information for net? > + > + dev = get_device(vid); > + if (dev == NULL) > + return -1; > + > + dev->extern_data = extern_data; > + if (ops) { > + dev->extern_ops.pre_vhost_user_msg_handler = > + ops->pre_vhost_user_msg_handler; > + dev->extern_ops.post_vhost_user_msg_handler = > + 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_user.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] = { > [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", Please leave this patch device agnostic. Put these into crypto related patches. > }; > > 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 = (*dev->extern_ops.pre_vhost_user_msg_handler)(dev->vid, We have a variable vid, why use dev->vid? > + (void *)&msg, &need_reply); > + if (ret < 0) > + goto skip_to_reply; > + > + if (need_reply) > + send_vhost_reply(fd, &msg); 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? > + } > + > switch (msg.request.master) { > case VHOST_USER_GET_FEATURES: > msg.payload.u64 = vhost_user_get_features(dev); > @@ -1477,11 +1491,20 @@ vhost_user_msg_handler(int vid, int fd) > break; > > default: > - ret = -1; > - break; > + if (dev->extern_ops.post_vhost_user_msg_handler) { Do we allow overlapping of common and post handle? > + uint32_t need_reply; > > + ret = (*dev->extern_ops.post_vhost_user_msg_handler)( > + dev->vid, (void *)&msg, &need_reply); > + > + if (need_reply) > + send_vhost_reply(fd, &msg); > + } else > + ret = -1; > + break; > } > > +skip_to_reply: > if (unlock_required) > vhost_user_unlock_all_queue_pairs(dev); >