From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id 30F0537AF for ; Wed, 27 Feb 2019 12:48:09 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20190227114808euoutp027f4234764b1c21fd611da0dd59246049~HNht9DuMr0344303443euoutp02y for ; Wed, 27 Feb 2019 11:48:08 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20190227114808euoutp027f4234764b1c21fd611da0dd59246049~HNht9DuMr0344303443euoutp02y DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1551268088; bh=ifEv1zE2cHSGngsTVu1lB/4XtHfNFYh05EminLR5Oxc=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=S8wE5C65rx0vjCeYig7mZrV9TEBmM1RCxqLQ1eGQKVl1lVkkTCE9alWZvK21L1R1C vdtfyxJvReMAFR8GXU6KSilTPNKfjEGVvR8KjqHnvXcRZOcEBK2mQVSkUKW6CHkem0 XEcqhE1nPUft0I4j+sxxxOhyI4LS5FgvRvq2Tjk0= Received: from eusmges2new.samsung.com (unknown [203.254.199.244]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20190227114807eucas1p153a3074eeca034fd4e204b5150d6606b~HNhtiO0qQ2769927699eucas1p16; Wed, 27 Feb 2019 11:48:07 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges2new.samsung.com (EUCPMTA) with SMTP id D7.92.04294.7F8767C5; Wed, 27 Feb 2019 11:48:07 +0000 (GMT) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190227114807eucas1p15e4089da8901d4ad2e7e32b6113279b2~HNhs04F7B2547325473eucas1p1N; Wed, 27 Feb 2019 11:48:07 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20190227114807eusmtrp183d6a8fbc0939c3588c65d77ebec3227~HNhsxvcn31950819508eusmtrp1H; Wed, 27 Feb 2019 11:48:07 +0000 (GMT) X-AuditID: cbfec7f4-84fff700000010c6-15-5c7678f7d09c Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id D4.C7.04128.7F8767C5; Wed, 27 Feb 2019 11:48:07 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20190227114806eusmtip1f256d022ee24f0e4e9daae425654f6c1~HNhsSXiql2671326713eusmtip1_; Wed, 27 Feb 2019 11:48:06 +0000 (GMT) To: Maxime Coquelin , "Liu, Changpeng" , "dev@dpdk.org" Cc: "Stojaczyk, Dariusz" , "Bie, Tiwei" , "Wang, Zhihong" , Jason Wang From: Ilya Maximets Message-ID: <21c38a1c-732e-e4e4-1129-a175262efabb@samsung.com> Date: Wed, 27 Feb 2019 14:48:01 +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: Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrBKsWRmVeSWpSXmKPExsWy7djP87rfK8piDC50qFtc/PmV3eL97zWM Fu8+bWeyuNL+k91i2aXPTBbHOvewWGxt+M9ksfniJCYHDo9fC5ayeize85LJ4/2+q2wefVtW MQawRHHZpKTmZJalFunbJXBlrNu4maXgt1XF/JaUBsZphl2MHBwSAiYS3y+EdDFycQgJrGCU eHbzOXsXIyeQ84VR4stVEwj7M6PElXn8IDZI/f3729khGpYzSvQteMQG4XxklGidtYEFpEpY wFPiyaVvYLaIQK3EtisfwTqYBVYxSvxZ9gRsBZuAjsSp1UcYQWxeATuJpZ+vM4PYLAKqEnMu XQRrFhWIkHj/dDcLRI2gxMmZT8BsTqD66w3/weqZBcQlmr6sZIWw5SWat85mBlkmIbCJXeLT rg5WiLtdJKb//8gOYQtLvDq+BcqWkTg9uYcFwq6XuN/ykhGiuYNRYvqhf0wQCXuJLa/PsYMC jFlAU2L9Ln1I2DlKNJ4XgzD5JG68FYQ4gU9i0rbpzBBhXomONiGIGSoSvw8uZ4awpSRuvvvM PoFRaRaSx2YheWYWkmdmIaxdwMiyilE8tbQ4Nz212CgvtVyvODG3uDQvXS85P3cTIzD9nP53 /MsOxl1/kg4xCnAwKvHw/jAtjRFiTSwrrsw9xCjBwawkwttbVBYjxJuSWFmVWpQfX1Sak1p8 iFGag0VJnLea4UG0kEB6YklqdmpqQWoRTJaJg1OqgTG37Yrz5cehavn2HLZzZx/f1r7IXmJt TFLQsyizqm/z3jmZsv5zXnCM4ZbgkQ+8h7b/NprOs0u8c/IWL8cFKTHX5i5WZDq31bh0+9vj tTfeh8czn/oWWPd9SqfOTJOYK1p+facV6jPXCf/Wt5Vz/LFSwVeX73Kl+YxZa45rzF8vIWqy Icm3NF+JpTgj0VCLuag4EQAOH75vOwMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrHIsWRmVeSWpSXmKPExsVy+t/xu7rfK8piDF7NYba4+PMru8X732sY Ld592s5kcaX9J7vFskufmSyOde5hsdja8J/JYvPFSUwOHB6/Fixl9Vi85yWTx/t9V9k8+ras YgxgidKzKcovLUlVyMgvLrFVija0MNIztLTQMzKx1DM0No+1MjJV0rezSUnNySxLLdK3S9DL WLdxM0vBb6uK+S0pDYzTDLsYOTkkBEwk7t/fzt7FyMUhJLCUUWLGzNvMEAkpiR+/LrBC2MIS f651sYHYQgLvGSXuXQBrFhbwlHhy6RsLiC0iUCux4O5yNpBBzAKrGCWaHqxkhZh6hVVi79Hd 7CBVbAI6EqdWH2EEsXkF7CSWfr4Oto1FQFVizqWLYJNEBSIk7l58wQJRIyhxcuYTMJsTqP56 w3+wemYBdYk/8y5B2eISTV9WskLY8hLNW2czT2AUmoWkfRaSlllIWmYhaVnAyLKKUSS1tDg3 PbfYSK84Mbe4NC9dLzk/dxMjMO62Hfu5ZQdj17vgQ4wCHIxKPLw/TEtjhFgTy4orcw8xSnAw K4nw9haVxQjxpiRWVqUW5ccXleakFh9iNAV6biKzlGhyPjAl5JXEG5oamltYGpobmxubWSiJ 8543qIwSEkhPLEnNTk0tSC2C6WPi4JRqYMzIOpVTq/Tai0nvh8K+S5pS01cIpN/vuh//wo9h QshCXe6tPMt7bj08Oln5WK6N72a3uj7LHWseSRU+33hAsWMubzXf11uX0487yC3Qmn6M6SRv hN099vOyXpzzU7Qlmw0eiv6c9uRrjLn06UXe3H5ssh3xVpEX6vXetkv+C1LJ6Jrmlyd4S4ml OCPRUIu5qDgRANPOmgjRAgAA X-CMS-MailID: 20190227114807eucas1p15e4089da8901d4ad2e7e32b6113279b2 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20190225132001eucas1p25c1e925b895b3ab36da0aca27110e15c X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20190225132001eucas1p25c1e925b895b3ab36da0aca27110e15c References: <1551081095-14286-1-git-send-email-changpeng.liu@intel.com> <2c1b8abc-47a9-0d0b-5056-0092fc58e310@samsung.com> <4ba6108c-78b0-c7e6-f810-3faa13c7765e@samsung.com> <60245e11-ff68-133d-2577-8526c89e6264@samsung.com> <05908449-ed34-3ac8-b068-7c9e80f758a8@redhat.com> Subject: Re: [dpdk-dev] vhost: add virtio configuration space access socket messages 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: Wed, 27 Feb 2019 11:48:10 -0000 On 27.02.2019 12:04, Maxime Coquelin wrote: > > > On 2/26/19 3:07 PM, Ilya Maximets wrote: >> On 26.02.2019 16:43, Maxime Coquelin wrote: >>> >>> >>> On 2/26/19 2:36 PM, Ilya Maximets wrote: >>>> On 26.02.2019 15:32, Maxime Coquelin wrote: >>>>> >>>>> >>>>> On 2/26/19 9:42 AM, Ilya Maximets wrote: >>>>>> On 26.02.2019 11:13, Liu, Changpeng wrote: >>>>>>> >>>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com] >>>>>>>> Sent: Tuesday, February 26, 2019 3:39 PM >>>>>>>> To: Liu, Changpeng ; dev@dpdk.org >>>>>>>> Cc: Stojaczyk, Dariusz ; >>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei ; Wang, >>>>>>>> Zhihong ; Jason Wang >>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages >>>>>>>> >>>>>>>> On 26.02.2019 10:01, Liu, Changpeng wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>>> -----Original Message----- >>>>>>>>>> From: Ilya Maximets [mailto:i.maximets@samsung.com] >>>>>>>>>> Sent: Monday, February 25, 2019 9:20 PM >>>>>>>>>> To: Liu, Changpeng ; dev@dpdk.org >>>>>>>>>> Cc: Stojaczyk, Dariusz ; >>>>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei ; Wang, >>>>>>>>>> Zhihong ; Jason Wang >>>>>>>>>> Subject: Re: vhost: add virtio configuration space access socket messages >>>>>>>>>> >>>>>>>>>> On 25.02.2019 10:51, Changpeng Liu wrote: >>>>>>>>>>> This patch adds new vhost user messages GET_CONFIG and SET_CONFIG >>>>>>>>>>> used to get/set virtio device's PCI configuration space. >>>>>>>>>> >>>>>>>>>> Beside the fact that some additional description and reasoning required, >>>>>>>>>> I do not see the usage of this feature. You're defining the flag >>>>>>>>>> VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of dpdk >>>>>>>> vhost >>>>>>>>>> backends (vdpa, vhost-user) will use this feature. >>>>>>>>>> You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or >>>>>>>>>> VDPA_SUPPORTED_PROTOCOL_FEATURES. >>>>>>>>>> >>>>>>>>>>    From the other side, current implementation forces application to properly >>>>>>>>>> implement the get/set_config callbacks. Otherwise, receiving of the messages >>>>>>>>>> will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost >>>>>>>>>> disconnection. >>>>>>>>>> This looks strange, because supported protocol features normally enabled by >>>>>>>>>> default. Am I misunderstood something ? >>>>>>>>> QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG >>>>>>>> wasn't enabled. >>>>>>>> >>>>>>>> So, you're going to enable it only by explicit call to >>>>>>>> 'rte_vhost_driver_set_features' ? >>>>>>>> >>>>>>>> In this case I'm assuming that you're implementing your own vhost backend. >>>>>>>> But why you're not using 'dev->extern_ops' and corresponding 'pre_msg_handle' >>>>>>>> or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does >>>>>>>> 'vhost_crypto' backend ? >>>>>>> The patch was developed one year ago, while DPDK didn't have external ops. >>>>>> >>>>>> So, maybe it's time to reconsider the implementation. >>>>> >>>>> +1 >>>>> >>>>>>> The get_config/set_config was defined for all the virtio devices, so I think it makes >>>>>>> more sense adding here. >>>>>> >>>>>> VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio devices >>>>>> too, however they makes sense for vhost_crypto backend only. These messages >>>>>> (GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are >>>>>> implemented, so IMHO it's better to implement their handlers along with the >>>>>> callbacks, i.e. inside the implementation of your vhost backend. >>>>>> >>>>>> Maxime, Tiwei, what do you think ? >>>>> >>>>> I would prefer it to be implemented in SPDK directly as a pre_handler >>>>> callback, as I don't foresee a need for it for other backends, and it >>>>> would avoid breaking the API. >>>>> >>>>> It would imply fixing the beginning of vhost_user_msg_handler() to accept requests > VHOST_USER_MAX and add necessary check before doing >>>>> the debug logs. >>>> >>>> VHOST_USER_MAX is 31 and both new requests are >>>> defined in the same enum VhostUserRequest: >>>> >>>>      VHOST_USER_GET_CONFIG = 24, >>>>      VHOST_USER_SET_CONFIG = 25 >>>> >>>> I don't think that any change is needed here. >>> >>> I didn't meant GET_SET_CONFIG specifically. I meant that if we want >>> something really generic, we would need to do that. >> >> OK. I understand now. >> >>> >>> BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] would not be defined. >>> >>>> >>>>> >>>>> With above change we would also be able to remove VHOST_CRYPTO requests >>>>> from vhost_user.c, >>>> >>>> Maybe you're looking at the different git HEAD ? I don't see any crypto >>>> related code in vhost_user.c. Only name definition in vhost_message_str. >>> >>> Yes, I meant removing their definition in vhost_message_str[]. >>> >>> My point is that if we want to have external backends to handle their >>> specific requests, we should not have to modify vhost_user.c as it >>> creates a useless dependency. >> >> That's a good point. I agree. >> >> Maybe we'll need some new API to make vhost library more dynamic? >> Something like >>      rte_vhost_message_register(enum VhostUserRequest request, >>                                 const char *resuest_str, >>                                 vhost_message_handler_t handler); >> This could be flexible. > > Agree it could be done like that. > > Now, I think we have pretty much every thing we need in the API to > implement it, but maybe I'm missing something? Yes, looks like we have. > > I.e., by implementing the .pre_msg_handle callback and setting its > skip_master to 1, we have the same result. Except that we don't > have the debug message. Sure. This should work. > > Also, it means we would need to either rework all current handlers, > or make "struct virtio_net" part of the API. Actually, 'vhost_crypto' already uses the 'struct virtio_net'. So, it's kind of a part of the API anyway. Do we need a special API to get 'extern_data' ? Right now 'vhost_crypto' gets it directly from the 'vhost_net' structure. 'vhost_crypto' seems very strange though. I don't think that it works. (example calls 'set_features' via 'crypto_create' inside the 'new_device' callback, this should not work). Also, it uses a lot of internal stuff for which we have public APIs. > So maybe we'll have to come to this, but we would need first to do > a significant rework of the library to move all the net specific > stuff out of the generic vhost part. Sure. It seems to me that we need to split out network related stuff from the 'struct virtio_net' to a separate 'struct vhost_net' and rename 'struct virtio_net' to, for example, 'struct virtio_dev'. Making some kind of inheritance between them. Maybe having a 'backend' pointer to the corresponding 'struct vhost_net/crypto/scsi' and replace the 'extern_data' with it. > > Thanks, > Maxime >> >>> >>>>> and we could then work on moving vhost-net bits >>>>> out of this file too. >>>>> >>>>> Regards, >>>>> Maxime >>>>> >>>>> >>>>> >>> >>> > >