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

[Matan Azrad <matan@mellanox.com>]

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


>  		if (vdpa_dev->ops->dev_conf(dev->vid))
>  			VHOST_LOG_CONFIG(ERR,
>  					"Failed to configure vDPA device\n");
> --
> 2.25.4