From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 9ECF3A3 for ; Mon, 4 Mar 2019 16:25:22 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20190304152520euoutp01cac917108b0ab3c4abd405bfb349fad3~Iyty4-Y2t3124031240euoutp01g for ; Mon, 4 Mar 2019 15:25:20 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20190304152520euoutp01cac917108b0ab3c4abd405bfb349fad3~Iyty4-Y2t3124031240euoutp01g DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1551713121; bh=vmHhqPPGv6zs7rwMXnbLb+tEyodXxaY/HygGtPkFGTY=; h=Subject:To:From:Date:In-Reply-To:References:From; b=i/7CwkpWYgKjyEJJojjrju+C1liRzwwEKs8ekznjHZhH46nFhUSB3ab8TieK8nE+g iIuxuAl0OU61+zzygkSPev1DtDnlH0nM2ssOSdJzl/0dq9wTOW+LNKonGW7S3P9chJ t8djJOt2ThXdRCsvvy+LBHXDtzG/unD+cGmTkGXw= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20190304152520eucas1p18365edaf20eb195f4154ccdcc56be34e~IytyfX0oC1252512525eucas1p12; Mon, 4 Mar 2019 15:25:20 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id 98.41.04441.0634D7C5; Mon, 4 Mar 2019 15:25:20 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190304152519eucas1p13eaae9f93c104acb5f0255d101ab883c~IytxyqUfB1229412294eucas1p1I; Mon, 4 Mar 2019 15:25:19 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190304152519eusmtrp217d919814a8eb85fc58348390eb154cb~IytxkfcGr0410104101eusmtrp2e; Mon, 4 Mar 2019 15:25:19 +0000 (GMT) X-AuditID: cbfec7f2-5e3ff70000001159-5e-5c7d43607ea1 Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 06.65.04128.F534D7C5; Mon, 4 Mar 2019 15:25:19 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20190304152519eusmtip12dca5d954f5f925cb0927226b0439c39~IytxNRaFc1046710467eusmtip1a; Mon, 4 Mar 2019 15:25:19 +0000 (GMT) To: Maxime Coquelin , dev@dpdk.org, changpeng.liu@intel.com, tiwei.bie@intel.com From: Ilya Maximets Message-ID: <37a4d589-8094-9c03-244e-cf2710bfee4c@samsung.com> Date: Mon, 4 Mar 2019 18:25:18 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.5.1 MIME-Version: 1.0 In-Reply-To: <20190228153134.31865-3-maxime.coquelin@redhat.com> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprNKsWRmVeSWpSXmKPExsWy7djP87oJzrUxBtM6hSwu/vzKbvHu03Ym iyvtP9ktjnXuYbHY2vCfyYHV49eCpawei/e8ZPJ4v+8qm0ffllWMASxRXDYpqTmZZalF+nYJ XBnL175gLnjpVXHq3RHWBsat1l2MHBwSAiYSN35odzFycQgJrGCUaH+4lR3C+cIoseHxalYI 5zOjxLfDyxm7GDnBOqZvOQFVtZxRYu+TBSwQzkdGicczDzODVAkLhEhsWnwYrENEIE9i7qlT TCA2m4COxKnVR8DivAJ2Eh9fbGYFsVkEVCQauk+DxUUFIiTeP93NAlEjKHFy5hMwm1PAQWL7 sX6w+cwC4hJNX1ayQtjyEtvfzmEGOUJCoJtd4k/vLUaI51wk1h1hhbhaWOLV8S3sELaMxP+d 85kg7HqJ+y0vGSF6Oxglph/6B5Wwl9jy+hw7yBxmAU2J9bv0IcKOEveXPGCBGM8nceOtIMQJ fBKTtk1nhgjzSnS0CUFUq0j8PricGcKWkrj57jPUBR4SR/7PYp7AqDgLyZOzkDw2C8ljsxBu WMDIsopRPLW0ODc9tdgwL7Vcrzgxt7g0L10vOT93EyMwyZz+d/zTDsavl5IOMQpwMCrx8GaY 1cYIsSaWFVfmHmKU4GBWEuE1cwAK8aYkVlalFuXHF5XmpBYfYpTmYFES561meBAtJJCeWJKa nZpakFoEk2Xi4JRqYFy+59KWeKHXYf87xHwq5qcESmn1Mct/FIt+c3bW22MvvvQv49H5KBSY bB4p63zqx9d+sefKD86bNCSXS83e3rlKnj1RzpHzAWN4kotkybuSWIZ/InEfdVu6f7cXP20z 09YX2gMMoBsm5Q8Uu9ICHfbuMHt8Zc6JRkX+us/i2fm1rFGqL7qVWIozEg21mIuKEwEM37Ch LgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrMIsWRmVeSWpSXmKPExsVy+t/xu7rxzrUxBk//yVlc/PmV3eLdp+1M Flfaf7JbHOvcw2KxteE/kwOrx68FS1k9Fu95yeTxft9VNo++LasYA1ii9GyK8ktLUhUy8otL bJWiDS2M9AwtLfSMTCz1DI3NY62MTJX07WxSUnMyy1KL9O0S9DKWr33BXPDSq+LUuyOsDYxb rbsYOTkkBEwkpm85wd7FyMUhJLCUUeLimkZ2iISUxI9fF1ghbGGJP9e62CCK3jNKPO67wAiS EBYIkdi0+DCYLSKQJ3Hs9hNmEFtI4CSjxNdfriA2m4COxKnVR8BqeAXsJD6+2Aw2lEVARaKh +zRYXFQgQuLuxRcsEDWCEidnPgGzOQUcJLYf6webySygLvFn3iUoW1yi6ctKVghbXmL72znM ExgFZyFpn4WkZRaSlllIWhYwsqxiFEktLc5Nzy020itOzC0uzUvXS87P3cQIjKBtx35u2cHY 9S74EKMAB6MSD2+GWW2MEGtiWXFl7iFGCQ5mJRFeMwegEG9KYmVValF+fFFpTmrxIUZToOcm MkuJJucDozuvJN7Q1NDcwtLQ3Njc2MxCSZz3vEFllJBAemJJanZqakFqEUwfEwenVANjmEXT Cs4epxeMK60v9j+d7by2fd565o1/LFPUHR4v1bg58W5a3utWn/g3k6atbds667tCwSS/slfr C2YVXKl4bzfx/93jv+Ly6jXtP+yPrDfUuJhxI+OkQGZdz+XYIomg0IVHL/zTZl4wk5drl75/ TxdDVny9uwlT9Uvvf+f271fRuXZ90c4IJZbijERDLeai4kQA+rm/erYCAAA= X-CMS-MailID: 20190304152519eucas1p13eaae9f93c104acb5f0255d101ab883c X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20190228153153epcas4p166039186da1d9cf15a34da5d3296ba0d X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20190228153153epcas4p166039186da1d9cf15a34da5d3296ba0d References: <20190228153134.31865-1-maxime.coquelin@redhat.com> <20190228153134.31865-3-maxime.coquelin@redhat.com> Subject: Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend 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: Mon, 04 Mar 2019 15:25:22 -0000 On 28.02.2019 18:31, Maxime Coquelin wrote: > External backends may have specific requests to handle, and so > we don't want the vhost-user lib to handle these requests as > errors. > > This patch also changes the experimental API by introducing > RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib > can report an error if a message is handled neither by > the vhost-user library nor by the external backend. > > The logic changes a bit so that if the callback returns > with ERR, OK or REPLY, it is considered the message > is handled by the external backend so it won't be > handled by the vhost-user library. > It is still possible for an external backend to listen > to requests that have to be handled by the vhost-user > library like SET_MEM_TABLE, but the callback have to > return NOT_HANDLED in that case. > > Suggested-by: Ilya Maximets > Signed-off-by: Maxime Coquelin > --- > lib/librte_vhost/rte_vhost.h | 16 +++++--- > lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------ > 2 files changed, 60 insertions(+), 31 deletions(-) > > diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h > index c9c392975..b1c5a0908 100644 > --- a/lib/librte_vhost/rte_vhost.h > +++ b/lib/librte_vhost/rte_vhost.h > @@ -121,6 +121,8 @@ enum rte_vhost_msg_result { > RTE_VHOST_MSG_RESULT_OK = 0, > /* Message handling successful and reply prepared */ > RTE_VHOST_MSG_RESULT_REPLY = 1, > + /* Message not handled */ > + RTE_VHOST_MSG_RESULT_NOT_HANDLED, > }; > > /** > @@ -135,11 +137,13 @@ enum rte_vhost_msg_result { > * If the handler requires skipping the master message handling, this variable > * shall be written 1, otherwise 0. > * @return > - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, > - * VH_RESULT_ERR on failure > + * RTE_VHOST_MSG_RESULT_OK on success, > + * RTE_VHOST_MSG_RESULT_REPLY on success with reply, > + * RTE_VHOST_MSG_RESULT_ERR on failure, > + * RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled. > */ > typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid, > - void *msg, uint32_t *skip_master); > + void *msg); > > /** > * Function prototype for the vhost backend to handler specific vhost user > @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid, > * @param msg > * Message pointer. > * @return > - * VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply, > - * VH_RESULT_ERR on failure > + * RTE_VHOST_MSG_RESULT_OK on success, > + * RTE_VHOST_MSG_RESULT_REPLY on success with reply, > + * RTE_VHOST_MSG_RESULT_ERR on failure, > + * RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled. > */ > typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid, > void *msg); According to above definition, we should make corresponding change in vhost_crypto. Something like this: --- diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c index 0f437c4a1..f0eedd422 100644 --- a/lib/librte_vhost/vhost_crypto.c +++ b/lib/librte_vhost/vhost_crypto.c @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg) return RTE_VHOST_MSG_RESULT_ERR; } - if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) { + switch (vmsg->request.master) { + VHOST_USER_CRYPTO_CREATE_SESS: vhost_crypto_create_sess(vcrypto, &vmsg->payload.crypto_session); vmsg->fd_num = 0; ret = RTE_VHOST_MSG_RESULT_REPLY; - } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) { + break; + VHOST_USER_CRYPTO_CLOSE_SESS: if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64)) ret = RTE_VHOST_MSG_RESULT_ERR; + break; + default: + ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED; + break; } return ret; --- > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index 36c0c676d..ca9167f1d 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd) > int did = -1; > int ret; > int unlock_required = 0; > - uint32_t skip_master = 0; > + bool handled; In below code 'handled' equals to 'false' only if 'ret' equals to 'RTE_VHOST_MSG_RESULT_NOT_HANDLED'. Looks like we don't need this variable. > int request; > > dev = get_device(vid); > @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd) > } > > ret = read_vhost_message(fd, &msg); > - if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) { > + if (ret <= 0) { > if (ret < 0) > RTE_LOG(ERR, VHOST_CONFIG, > "vhost read message failed\n"); > - else if (ret == 0) > + else > RTE_LOG(INFO, VHOST_CONFIG, > "vhost peer closed\n"); > - else > - RTE_LOG(ERR, VHOST_CONFIG, > - "vhost read incorrect message\n"); > > return -1; > } > > ret = 0; > - if (msg.request.master != VHOST_USER_IOTLB_MSG) > - RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", > - vhost_message_str[msg.request.master]); > - else > - RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", > - vhost_message_str[msg.request.master]); > + request = msg.request.master; > + if (request < VHOST_USER_MAX && vhost_message_str[request]) { We probably need to check for 'request > VHOST_USER_NONE' because it has signed type. BTW, do we heed to allow requests out of (VHOST_USER_NONE, VHOST_USER_MAX) range? This 'if' statement reports them as 'External' requests. However, the 'master' 'if' statement will treat them as error, printing "Requested invalid message type". If we don't need to handle messages out of our range, we could check the range once at the top of this function and never check again. > + if (request != VHOST_USER_IOTLB_MSG) > + RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n", > + vhost_message_str[request]); > + else > + RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n", > + vhost_message_str[request]); > + } else { > + RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request); > + } > > ret = vhost_user_check_and_alloc_queue_pair(dev, &msg); > if (ret < 0) { > @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd) > * inactive, so it is safe. Otherwise taking the access_lock > * would cause a dead lock. > */ > - switch (msg.request.master) { > + switch (request) { > case VHOST_USER_SET_FEATURES: > case VHOST_USER_SET_PROTOCOL_FEATURES: > case VHOST_USER_SET_OWNER: > @@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd) > > } > > + handled = false; 'ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED' instead. > if (dev->extern_ops.pre_msg_handle) { > ret = (*dev->extern_ops.pre_msg_handle)(dev->vid, > - (void *)&msg, &skip_master); > - if (ret == RTE_VHOST_MSG_RESULT_ERR) > - goto skip_to_reply; > - else if (ret == RTE_VHOST_MSG_RESULT_REPLY) > + (void *)&msg); > + switch (ret) { > + case RTE_VHOST_MSG_RESULT_REPLY: > send_vhost_reply(fd, &msg); > - > - if (skip_master) > + case RTE_VHOST_MSG_RESULT_ERR: > + case RTE_VHOST_MSG_RESULT_OK: > + handled = true; > goto skip_to_post_handle; > + case RTE_VHOST_MSG_RESULT_NOT_HANDLED: > + default: > + break; > + } > } > > - request = msg.request.master; > if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) { > if (!vhost_message_handlers[request]) > goto skip_to_post_handle; > @@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd) > RTE_LOG(ERR, VHOST_CONFIG, > "Processing %s failed.\n", > vhost_message_str[request]); > + handled = true; > break; > case RTE_VHOST_MSG_RESULT_OK: > RTE_LOG(DEBUG, VHOST_CONFIG, > "Processing %s succeeded.\n", > vhost_message_str[request]); > + handled = true; > break; > case RTE_VHOST_MSG_RESULT_REPLY: > RTE_LOG(DEBUG, VHOST_CONFIG, > "Processing %s succeeded and needs reply.\n", > vhost_message_str[request]); > send_vhost_reply(fd, &msg); > + handled = true; > + break; > + default: > break; > } > } else { > @@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd) > skip_to_post_handle: > if (ret != RTE_VHOST_MSG_RESULT_ERR && > dev->extern_ops.post_msg_handle) { > - ret = (*dev->extern_ops.post_msg_handle)( > - dev->vid, (void *)&msg); > - if (ret == RTE_VHOST_MSG_RESULT_ERR) > - goto skip_to_reply; > - else if (ret == RTE_VHOST_MSG_RESULT_REPLY) > + ret = (*dev->extern_ops.post_msg_handle)(dev->vid, > + (void *)&msg); > + switch (ret) { > + case RTE_VHOST_MSG_RESULT_REPLY: > send_vhost_reply(fd, &msg); > + case RTE_VHOST_MSG_RESULT_ERR: > + case RTE_VHOST_MSG_RESULT_OK: > + handled = true; > + case RTE_VHOST_MSG_RESULT_NOT_HANDLED: > + default: > + break; > + } > } > > -skip_to_reply: > if (unlock_required) > vhost_user_unlock_all_queue_pairs(dev); > > + /* If message was not handled at this stage, treat it as an error */ > + if (!handled) { if (ret == RTE_VHOST_MSG_RESULT_NOT_HANDLED) > + RTE_LOG(ERR, VHOST_CONFIG, > + "vhost message (req: %d) was not handled.\n", request); > + ret = RTE_VHOST_MSG_RESULT_ERR; > + } > + > /* > * If the request required a reply that was already sent, > * this optional reply-ack won't be sent as the >