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...@intel.com; Shahaf Shuler <shah...@mellanox.com>;
>>>>>> Matan Azrad <ma...@mellanox.com>; amore...@redhat.com;
>>>>>> xiao.w.w...@intel.com; Slava Ovsiienko
>> <viachesl...@mellanox.com>;
>>>>>> dev@dpdk.org
>>>>>> Cc: jasow...@redhat.com; l...@redhat.com; Maxime Coquelin
>>>>>> <maxime.coque...@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.coque...@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&data=02%7C01%7Cmatan%40mella
>> nox.com%7Cdafcc4fcf4074202227208d80c542b1e%7Ca652971c7d2e4d9ba6a4
>> d149256f461b%7C0%7C0%7C637272902927735523&sdata=hvUJq5VdXH
>> usbBp1y%2BSr1Yp2AukNQZbRnFS6dR3rgMw%3D&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
>>>>>
>>>
>
