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