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: [email protected]; Shahaf Shuler <[email protected]>; Matan >> Azrad <[email protected]>; [email protected]; >> [email protected]; Slava Ovsiienko <[email protected]>; >> [email protected] >> Cc: [email protected]; [email protected]; Maxime Coquelin >> <[email protected]> >> 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 <[email protected]> >> --- >> 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 >
