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

[Matan Azrad <matan@mellanox.com>]

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.


I will send RFC patch with my suggestion.

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