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


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