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 E3BF4374C for ; Wed, 27 Feb 2019 10:12:30 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0E04D30AA79F; Wed, 27 Feb 2019 09:12:30 +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 1B1585DE70; Wed, 27 Feb 2019 09:12:27 +0000 (UTC) To: "Liu, Changpeng" , Ilya Maximets , "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: <116c42b6-2f70-ae4a-964e-622aebeb61c9@redhat.com> Date: Wed, 27 Feb 2019 10:12:26 +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: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Wed, 27 Feb 2019 09:12:30 +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: Wed, 27 Feb 2019 09:12:31 -0000 On 2/27/19 2:31 AM, Liu, Changpeng wrote: > > >> -----Original Message----- >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com] >> Sent: Tuesday, February 26, 2019 8:32 PM >> To: Ilya Maximets ; Liu, Changpeng >> ; dev@dpdk.org >> Cc: Stojaczyk, Dariusz ; Bie, Tiwei >> ; Wang, Zhihong ; Jason Wang >> >> Subject: Re: vhost: add virtio configuration space access socket messages >> >> >> >> 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 > Okay, I will only add related messages definition in this patch and remove the > Callbacks. What we need to do is fix vhost lib so that you don't have anything to do in dpdk to add support for the new requests. So we need to fix the few bits in vhost_user_msg_handler() I mentionned yesterday. And I also notice we are missing rte_vhost_driver_set_protocol_feature(), so that you can advertise VHOST_USER_PROTOCOL_F_CONFIG support by your external backend. I'll try to cook the dpdk patch today. Cheers, Maxime >> >>>> 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. >> >> With above change we would also be able to remove VHOST_CRYPTO requests >> from vhost_user.c, and we could then work on moving vhost-net bits >> out of this file too. >> >> Regards, >> Maxime >