From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 3A63A2C39 for ; Tue, 26 Feb 2019 14:43:54 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5FA3A30FD84B; Tue, 26 Feb 2019 13:43:53 +0000 (UTC) Received: from [10.36.112.64] (ovpn-112-64.ams2.redhat.com [10.36.112.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 9F52F60BFB; Tue, 26 Feb 2019 13:43:50 +0000 (UTC) To: Ilya Maximets , "Liu, Changpeng" , "dev@dpdk.org" Cc: "Stojaczyk, Dariusz" , "Bie, Tiwei" , "Wang, Zhihong" , Jason Wang 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> From: Maxime Coquelin Message-ID: <05908449-ed34-3ac8-b068-7c9e80f758a8@redhat.com> Date: Tue, 26 Feb 2019 14:43:48 +0100 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: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Tue, 26 Feb 2019 13:43:53 +0000 (UTC) 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 13:43:55 -0000 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. 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. >> and we could then work on moving vhost-net bits >> out of this file too. >> >> Regards, >> Maxime >> >> >>