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

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

On 2/26/19 3:07 PM, Ilya Maximets wrote:
> 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 <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
>>>>
>>>>>> 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.

Agree it could be done like that.

Now, I think we have pretty much every thing we need in the API to
implement it, but maybe I'm missing something?

I.e., by implementing the .pre_msg_handle callback and setting its
skip_master to 1, we have the same result. Except that we don't
have the debug message.

Also, it means we would need to either rework all current handlers,
or make "struct virtio_net" part of the API.
So maybe we'll have to come to this, but we would need first to do
a significant rework of the library to move all the net specific
stuff out of the generic vhost part.

Thanks,
Maxime
> 
>>
>>>> and we could then work on moving vhost-net bits
>>>> out of this file too.
>>>>
>>>> Regards,
>>>> Maxime
>>>>
>>>>
>>>>
>>
>>