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

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

On 6/9/20 1:09 PM, Matan Azrad wrote:
> 
> Hi Maxime
> 
> From: Maxime Coquelin
>> 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.
> 
> Is it by spec?
> 
> We saw that windows virtio guest driver doesn't configure disabled queues.
> Is it bug in windows guest?
> You probably can take a look here:
> https://github.com/virtio-win/kvm-guest-drivers-windows
> 
>>>>> 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://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Finbox
>> .dpdk.org%2Fstable%2FB7F2E978279D1D49A3034B7786DACF407AFAA0C6%4
>> 0SHSMSX106.ccr.corp.intel.com%2F&amp;data=02%7C01%7Cmatan%40mella
>> nox.com%7Cdafcc4fcf4074202227208d80c542b1e%7Ca652971c7d2e4d9ba6a4
>> d149256f461b%7C0%7C0%7C637272902927735523&amp;sdata=hvUJq5VdXH
>> usbBp1y%2BSr1Yp2AukNQZbRnFS6dR3rgMw%3D&amp;reserved=0
>>
>> 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).
> 
> 
> It looks like very specific case WA which breaks other cases, for example:
> Guest poll mode driver: callfd is configured twice one by one, the first is X=!-1 and the second -1, here vdpa configuration may be triggered in the first one what make the driver think wrongly that the queue is not in poll mode.

Yes, I agree that it would be better to avoid having this workaround, as
it may create regressions.

> I will send RFC patch with my suggestion.

Thanks.
Xiao, any chance you try to reproduce the initial issue? This way we can
test Matan's RFC.

Maxime

>  
>> 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
>>>>>
>>>
>