Re: [dpdk-dev] [PATCH 9/9] vhost: only use vDPA config workaround if needed

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

Hi Matan,

On 6/8/20 11:19 AM, Matan Azrad wrote:
> Hi Maxime
> 
> From: Maxime Coquelin:
>> Hi Matan,
>>
>> On 6/7/20 12:38 PM, Matan Azrad wrote:
>>> Hi Maxime
>>>
>>> Thanks for the huge work.
>>> Please see a suggestion inline.
>>>
>>> From: Maxime Coquelin:
>>>> Sent: Thursday, May 14, 2020 11:02 AM
>>>> To: xiaolong.ye@intel.com; Shahaf Shuler <shahafs@mellanox.com>;
>>>> Matan Azrad <matan@mellanox.com>; amorenoz@redhat.com;
>>>> xiao.w.wang@intel.com; Slava Ovsiienko <viacheslavo@mellanox.com>;
>>>> dev@dpdk.org
>>>> Cc: jasowang@redhat.com; lulu@redhat.com; Maxime Coquelin
>>>> <maxime.coquelin@redhat.com>
>>>> Subject: [PATCH 9/9] vhost: only use vDPA config workaround if needed
>>>>
>>>> Now that we have Virtio device status support, let's only use the
>>>> vDPA workaround if it is not supported.
>>>>
>>>> This patch also document why Virtio device status protocol feature
>>>> support is strongly advised.
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> ---
>>>>  lib/librte_vhost/vhost_user.c | 16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/librte_vhost/vhost_user.c
>>>> b/lib/librte_vhost/vhost_user.c index e5a44be58d..67e96a872a 100644
>>>> --- a/lib/librte_vhost/vhost_user.c
>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>> @@ -2847,8 +2847,20 @@ vhost_user_msg_handler(int vid, int fd)
>>>>  	if (!vdpa_dev)
>>>>  		goto out;
>>>>
>>>> -	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED) &&
>>>> -			request == VHOST_USER_SET_VRING_CALL) {
>>>> +	if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
>>>> +		/*
>>>> +		 * Workaround when Virtio device status protocol
>>>> +		 * feature is not supported, wait for SET_VRING_CALL
>>>> +		 * request. This is not ideal as some frontends like
>>>> +		 * Virtio-user may not send this request, so vDPA device
>>>> +		 * may never be configured. Virtio device status support
>>>> +		 * on frontend side is strongly advised.
>>>> +		 */
>>>> +		if (!(dev->protocol_features &
>>>> +				(1ULL <<
>>>> VHOST_USER_PROTOCOL_F_STATUS)) &&
>>>> +				(request !=
>>>> VHOST_USER_SET_VRING_CALL))
>>>> +			goto out;
>>>> +
>>>
>>>
>>> When status protocol feature is not supported, in the current code, the
>> vDPA configuration triggering depends in:
>>> 1. Device is ready - all the queues are configured (datapath addresses,
>> callfd and kickfd) .
>>> 2. last command is callfd.
>>>
>>>
>>> The code doesn't take into account that some queues may stay disabled.
>>> Maybe the correct timing is:
>>> 1. Device is ready - all the enabled queues are configured and MEM table is
>> configured.
>>
>> I think current virtio_is_ready() already assumes the mem table is
>> configured, otherwise we would not have vq->desc, vq->used and vq->avail
>> being set as it needs to be translated using the mem table.
>>
> 
> Yes, but if you don't expect to check them for disabled queues you need to check mem table to be sure it was set.

Even disabled queues should be allocated/configured by the guest driver.

> 
>>> 2. no need callfd to be last.
>>>
>>> Queues that will be configured later will be configured to the HW when the
>> virtq becoming enabled.
>>>
>>>
>>> What do think?
>>
>> Maybe I did not understood what you mean, so please correct me if needed.
>>
>> If I understood correctly, then your suggestion is just to remove the
>> workaround, but it has been introduced by Intel because the callfd gets set a
>> second time in some cases.
> 
> Not to remove the WA, just to improve it😊
> 
> I don't sure I understand the issue here, can you add details?

My understanding is that callfd is sent early by Qemu but is then
updated after by Qemu and we have no way to distinguish whether the
first one is valid or not... I did a bit of archeology and found this
explanation from Xiao:

https://inbox.dpdk.org/stable/B7F2E978279D1D49A3034B7786DACF407AFAA0C6@SHSMSX106.ccr.corp.intel.com/

I haven't managed to reproduce the issue myself, so that's why I'm a bit
reluctant in trying to improve it. Ideally Xiao could try to reproduce
the issue, so that if we can find something more elegant (and that does
make Virtio-user to work without the SET_STATUS support) we can be
confident in merging it (and maybe even backport it).

Regards,
Maxime

> 
>>
>> Thanks,
>> Maxime
>>>
>>>>  		if (vdpa_dev->ops->dev_conf(dev->vid))
>>>>  			VHOST_LOG_CONFIG(ERR,
>>>>  					"Failed to configure vDPA device\n");
>>>> --
>>>> 2.25.4
>>>
>