Re: [PATCH] vhost: fix vring enable with VDPA

[Maxime Coquelin]

On 3/2/23 10:48, David Marchand wrote:

For VDPA devices, vq are not locked once the device has been configured.
But we need to hold the vq lock to check if vhost async is enabled.

Combining VDPA and vhost async does not seem to make much sense, so
prevent this combination, and only assert the lock is taken when VDPA is
not configured on this vq.

Bugzilla ID: 1169
Fixes: 4b02c2673757 ("vhost: annotate async accesses")

Signed-off-by: David Marchand <david.march...@redhat.com>
---
  lib/vhost/vhost.c      | 4 ++--
  lib/vhost/vhost_user.c | 8 ++++----
  2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index c36dc06a0a..6b73c03224 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1754,7 +1754,7 @@ rte_vhost_async_channel_register(int vid, uint16_t 
queue_id)

vq = dev->virtqueue[queue_id]; - if (unlikely(vq == NULL || !dev->async_copy))

+       if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
                return -1;

rte_spinlock_lock(&vq->access_lock);

@@ -1778,7 +1778,7 @@ rte_vhost_async_channel_register_thread_unsafe(int vid, 
uint16_t queue_id)

vq = dev->virtqueue[queue_id]; - if (unlikely(vq == NULL || !dev->async_copy))

+       if (unlikely(vq == NULL || !dev->async_copy || dev->vdpa_dev))
                return -1;

vq_assert_lock(dev, vq);

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index d702d082dd..daf3458985 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2177,10 +2177,10 @@ vhost_user_set_vring_enable(struct virtio_net **pdev,
                enable, index);

vq = dev->virtqueue[index];

-       /* vhost_user_lock_all_queue_pairs locked all qps */
-       vq_assert_lock(dev, vq);
-       if (enable && vq->async) {
-               if (vq->async->pkts_inflight_n) {
+       if (!(dev->flags & VIRTIO_DEV_VDPA_CONFIGURED)) {
+               /* vhost_user_lock_all_queue_pairs locked all qps */
+               vq_assert_lock(dev, vq);
+               if (enable && vq->async && vq->async->pkts_inflight_n) {
                        VHOST_LOG_CONFIG(dev->ifname, ERR,
                                "failed to enable vring. Inflight packets must be 
completed first\n");
                        return RTE_VHOST_MSG_RESULT_ERR;

Reviewed-by: Maxime Coquelin <maxime.coque...@redhat.com>

Thanks,
Maxime