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