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.
> > 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?
>
> Thanks,
> Maxime
> >
> >> if (vdpa_dev->ops->dev_conf(dev->vid))
> >> VHOST_LOG_CONFIG(ERR,
> >> "Failed to configure vDPA device\n");
> >> --
> >> 2.25.4
> >
