Re: [dpdk-dev] vhost: add virtio configuration space access socket messages

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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 <i.maximets@samsung.com>; Liu, Changpeng
>> <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; Wang, Zhihong <zhihong.wang@intel.com>; Jason Wang
>> <jasowang@redhat.com>
>> 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 <changpeng.liu@intel.com>; dev@dpdk.org
>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>> 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 <changpeng.liu@intel.com>; dev@dpdk.org
>>>>>>> Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>;
>>>>>>> maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
>>>>>>> Zhihong <zhihong.wang@intel.com>; Jason Wang <jasowang@redhat.com>
>>>>>>> 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
>