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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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.

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


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