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 4DC07235 for ; Tue, 26 Feb 2019 15:07:15 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20190226140715euoutp0232abb235b2cb433c2855ee572c0811c4~G7x5STtBK3257532575euoutp02Y for ; Tue, 26 Feb 2019 14:07:15 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20190226140715euoutp0232abb235b2cb433c2855ee572c0811c4~G7x5STtBK3257532575euoutp02Y DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1551190035; bh=21DVn5KGMuNXaG2Kv01q3uzpZSq2lJzrEi25NylDSs8=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=JDTOcrTCuXTvPDTFHckQ+JTSdXcMueLGWro9oJ/E6Ylh60p4a0Nwbi2fJy1lrmSod ayjtEsvTGu9by8FPHsbOCL9lFSNXDgl0HXVuUmhqXV5/kQVUa0SISeTQhBvL6s8sCN nCCOGPHtsKUyV12DvsN6/wHewzBzNOL7p7dHUqSk= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20190226140714eucas1p20651699283be9ca543415deb633545d9~G7x4wHRd_2426524265eucas1p2a; Tue, 26 Feb 2019 14:07:14 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id E2.2B.04806.218457C5; Tue, 26 Feb 2019 14:07:14 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190226140713eucas1p1ae13856b657ccf46c6ac657b67b598f8~G7x3zSan-0913409134eucas1p1X; Tue, 26 Feb 2019 14:07:13 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190226140713eusmtrp21033060989f58feb6638f94d5516443a~G7x3k2TyV0070500705eusmtrp24; Tue, 26 Feb 2019 14:07:13 +0000 (GMT) X-AuditID: cbfec7f5-34dff700000012c6-07-5c754812e6a2 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id BC.1D.04284.118457C5; Tue, 26 Feb 2019 14:07:13 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20190226140712eusmtip2a0e6fc6cf5d9d4a2bb3c02955143f2fd~G7x3FWcnp2010420104eusmtip2i; Tue, 26 Feb 2019 14:07:12 +0000 (GMT) To: Maxime Coquelin , "Liu, Changpeng" , "dev@dpdk.org" Cc: "Stojaczyk, Dariusz" , "Bie, Tiwei" , "Wang, Zhihong" , Jason Wang From: Ilya Maximets Message-ID: Date: Tue, 26 Feb 2019 17:07:12 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <05908449-ed34-3ac8-b068-7c9e80f758a8@redhat.com> Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrNKsWRmVeSWpSXmKPExsWy7djP87pCHqUxBr/n8Fhc/PmV3eL97zWM Fu8+bWeyuNL+k91i2aXPTBbHOvewWGxt+M9ksfniJCYHDo9fC5ayeize85LJ4/2+q2wefVtW MQawRHHZpKTmZJalFunbJXBlXNrxmq2gU7ti97XXrA2MR5S7GDk5JARMJLpn/WLqYuTiEBJY wSgxbf0VFgjnC6PElLabjBDOZ0aJJVOnMcO0TG57DFW1nFHiYsdBNpCEkMBHRonTh41BbGEB T4knl76xgNgiArUS2658ZAdpYBZYxSjxZ9kTdpAEm4COxKnVRxhBbF4BO4ljx++BbWARUJV4 unUCWI2oQITE4d53UDWCEidnPgEbyglUf+TiNLA4s4C4RNOXlawQtrxE89bZzCDLJAS2sUtc PN/GCnG2i8T5a8uZIGxhiVfHt7BD2DIS/3fOh4rXS9xveckI0dzBKDH90D+ohL3EltfngBo4 gDZoSqzfpQ9iSgg4SjSeF4Mw+SRuvBWEOIFPYtK26cwQYV6JjjYhiBkqEr8PLoeGoZTEzXef 2ScwKs1C8tgsJM/MQvLMLIS1CxhZVjGKp5YW56anFhvnpZbrFSfmFpfmpesl5+duYgQmodP/ jn/dwbjvT9IhRgEORiUe3h+mpTFCrIllxZW5hxglOJiVRHhnuwCFeFMSK6tSi/Lji0pzUosP MUpzsCiJ81YzPIgWEkhPLEnNTk0tSC2CyTJxcEo1MMa+E/P0OuHKe86vJ9laMn+lO0/wzS0X zzYXNV24FNcl9e/U8XlHvpR7B+6Z8y9xR27k1Rv3pBnDXx42WObG+inohFbkzd99flsjpI0S Nu3faMu2iqF8tkDQrcLtD+19K/YsvBbScqs62WPv1o+LeNYknlton6197PCF9D8rTjMffXVV aoZR+DolluKMREMt5qLiRADa9ELVPgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrAIsWRmVeSWpSXmKPExsVy+t/xe7qCHqUxBgcOCltc/PmV3eL97zWM Fu8+bWeyuNL+k91i2aXPTBbHOvewWGxt+M9ksfniJCYHDo9fC5ayeize85LJ4/2+q2wefVtW MQawROnZFOWXlqQqZOQXl9gqRRtaGOkZWlroGZlY6hkam8daGZkq6dvZpKTmZJalFunbJehl XNrxmq2gU7ti97XXrA2MR5S7GDk5JARMJCa3PWbpYuTiEBJYyijx6OlcJoiElMSPXxdYIWxh iT/Xutggit4zSlzrOs8MkhAW8JR4cukbC4gtIlArseDucrAiZoFVjBJND1ayQnS8ZpFo6+4G 62AT0JE4tfoII4jNK2Ancez4PbA4i4CqxNOtE9hBbFGBCImPT/cxQdQISpyc+QRsAydQ/ZGL 08B6mQXUJf7Mu8QMYYtLNH1ZyQphy0s0b53NPIFRaBaS9llIWmYhaZmFpGUBI8sqRpHU0uLc 9NxiQ73ixNzi0rx0veT83E2MwNjbduzn5h2MlzYGH2IU4GBU4uH9YVoaI8SaWFZcmXuIUYKD WUmEd7YLUIg3JbGyKrUoP76oNCe1+BCjKdBzE5mlRJPzgWkhryTe0NTQ3MLS0NzY3NjMQkmc 97xBZZSQQHpiSWp2ampBahFMHxMHp1QDY76fluFOpYDbx1fx/FXd9WCKp0hC+0++g1unXN5l LDVz0Z/C/VkrJiWrf5hntu7z8vi44vbEmXmflnROfrT645Waby27jOr3f6g+9+zfjMz+dckl Rx/dD2GbPz1pZvKMqWGStgoCs2NathZGVs6LTj7w5vXCBisW3aKY+X2ulYJLS+YZWGVc4VJi Kc5INNRiLipOBAB6KMu20wIAAA== X-CMS-MailID: 20190226140713eucas1p1ae13856b657ccf46c6ac657b67b598f8 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: Tue, 26 Feb 2019 14:07:16 -0000 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. > >>> and we could then work on moving vhost-net bits >>> out of this file too. >>> >>> Regards, >>> Maxime >>> >>> >>> > >