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 CB979235 for ; Tue, 26 Feb 2019 13:32:31 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 035CB3004416; Tue, 26 Feb 2019 12:32:31 +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 3CF926017E; Tue, 26 Feb 2019 12:32:26 +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: Date: Tue, 26 Feb 2019 13:32:24 +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: <60245e11-ff68-133d-2577-8526c89e6264@samsung.com> 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.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.44]); Tue, 26 Feb 2019 12:32:31 +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 12:32:32 -0000 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. 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