Re: [PATCH v2 1/4] vhost: Improve vhost_get_avail_idx() with smp_rmb()

[Gavin Shan]

On 4/30/24 04:44, Michael S. Tsirkin wrote:

On Mon, Apr 29, 2024 at 08:13:57PM +1000, Gavin Shan wrote:

From: "Michael S. Tsirkin" <m...@redhat.com>

All the callers of vhost_get_avail_idx() are concerned to the memory

*with* the memory barrier

Thanks, will be corrected in v3.

barrier, imposed by smp_rmb() to ensure the order of the available
ring entry read and avail_idx read.

Improve vhost_get_avail_idx() so that smp_rmb() is executed when
the avail_idx is advanced.

accessed, not advanced. guest advances it.

smp_rmb() is executed only when vp->last_avail_idx != vp->avail_idx.
I used 'advanced' to indicate the condition. 'accessed' is also
correct since the 'advanced' case included to 'accessed' case.

With it, the callers needn't to worry
about the memory barrier.

No functional change intended.

I'd add:

As a side benefit, we also validate the index on all paths now, which
will hopefully help catch future errors earlier.

Note: current code is inconsistent in how it handles errors:
some places treat it as an empty ring, others - non empty.
This patch does not attempt to change the existing behaviour.

Ok, I will integrate this to v3's commit log.

Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
[gshan: repainted vhost_get_avail_idx()]

?repainted?

It's just a indicator to say the changes aren't simply copied from
[1]. Some follow-up changes are also applied. So it needs to be
reviewed. I will drop this in v3.

Reviewed-by: Gavin Shan <gs...@redhat.com>
Acked-by: Will Deacon <w...@kernel.org>
---
  drivers/vhost/vhost.c | 106 +++++++++++++++++-------------------------
  1 file changed, 42 insertions(+), 64 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 8995730ce0bf..7aa623117aab 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1290,10 +1290,36 @@ static void vhost_dev_unlock_vqs(struct vhost_dev *d)
                mutex_unlock(&d->vqs[i]->mutex);
  }

-static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq,

-                                     __virtio16 *idx)
+static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq)
  {
-       return vhost_get_avail(vq, *idx, &vq->avail->idx);
+       __virtio16 idx;
+       int r;
+
+       r = vhost_get_avail(vq, idx, &vq->avail->idx);
+       if (unlikely(r < 0)) {
+               vq_err(vq, "Failed to access available index at %p (%d)\n",
+                      &vq->avail->idx, r);
+               return r;
+       }
+
+       /* Check it isn't doing very strange thing with available indexes */
+       vq->avail_idx = vhost16_to_cpu(vq, idx);
+       if (unlikely((u16)(vq->avail_idx - vq->last_avail_idx) > vq->num)) {
+               vq_err(vq, "Invalid available index change from %u to %u",
+                      vq->last_avail_idx, vq->avail_idx);
+               return -EINVAL;
+       }
+
+       /* We're done if there is nothing new */
+       if (vq->avail_idx == vq->last_avail_idx)
+               return 0;
+
+       /*
+        * We updated vq->avail_idx so we need a memory barrier between
+        * the index read above and the caller reading avail ring entries.
+        */
+       smp_rmb();
+       return 1;
  }

static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,

@@ -2498,38 +2524,17 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
  {
        struct vring_desc desc;
        unsigned int i, head, found = 0;
-       u16 last_avail_idx;
-       __virtio16 avail_idx;
+       u16 last_avail_idx = vq->last_avail_idx;
        __virtio16 ring_head;
        int ret, access;

- /* Check it isn't doing very strange things with descriptor numbers. */

-       last_avail_idx = vq->last_avail_idx;
-
        if (vq->avail_idx == vq->last_avail_idx) {
-               if (unlikely(vhost_get_avail_idx(vq, &avail_idx))) {
-                       vq_err(vq, "Failed to access avail idx at %p\n",
-                               &vq->avail->idx);
-                       return -EFAULT;
-               }
-               vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
-
-               if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) {
-                       vq_err(vq, "Guest moved avail index from %u to %u",
-                               last_avail_idx, vq->avail_idx);
-                       return -EFAULT;
-               }
+               ret = vhost_get_avail_idx(vq);
+               if (unlikely(ret < 0))
+                       return ret;

- /* If there's nothing new since last we looked, return

-                * invalid.
-                */
-               if (vq->avail_idx == last_avail_idx)
+               if (!ret)
                        return vq->num;
-
-               /* Only get avail ring entries after they have been
-                * exposed by guest.
-                */
-               smp_rmb();
        }

/* Grab the next descriptor number they're advertising, and increment

@@ -2790,35 +2795,20 @@ EXPORT_SYMBOL_GPL(vhost_add_used_and_signal_n);
  /* return true if we're sure that avaiable ring is empty */
  bool vhost_vq_avail_empty(struct vhost_dev *dev, struct vhost_virtqueue *vq)
  {
-       __virtio16 avail_idx;
        int r;

if (vq->avail_idx != vq->last_avail_idx)

                return false;

- r = vhost_get_avail_idx(vq, &avail_idx);

-       if (unlikely(r))
-               return false;
-
-       vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
-       if (vq->avail_idx != vq->last_avail_idx) {
-               /* Since we have updated avail_idx, the following
-                * call to vhost_get_vq_desc() will read available
-                * ring entries. Make sure that read happens after
-                * the avail_idx read.
-                */
-               smp_rmb();
-               return false;
-       }
-
-       return true;
+       /* Treat error as non-empty here */

If you write the comment like that then put it before "return":
that is where you treat an error like this.
And I feel Note: is better in that the comment does not
explain all of what is going on, just an aspect of it.

Ok. Will do in v3.

+       r = vhost_get_avail_idx(vq);
+       return r == 0;
  }
  EXPORT_SYMBOL_GPL(vhost_vq_avail_empty);

/* OK, now we need to know about added descriptors. */

  bool vhost_enable_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
  {
-       __virtio16 avail_idx;
        int r;

if (!(vq->used_flags & VRING_USED_F_NO_NOTIFY))

@@ -2842,25 +2832,13 @@ bool vhost_enable_notify(struct vhost_dev *dev, struct 
vhost_virtqueue *vq)
        /* They could have slipped one in as we were doing that: make
         * sure it's written, then check again. */
        smp_mb();
-       r = vhost_get_avail_idx(vq, &avail_idx);
-       if (r) {
-               vq_err(vq, "Failed to check avail idx at %p: %d\n",
-                      &vq->avail->idx, r);
-               return false;
-       }

- vq->avail_idx = vhost16_to_cpu(vq, avail_idx);

-       if (vq->avail_idx != vq->last_avail_idx) {
-               /* Since we have updated avail_idx, the following
-                * call to vhost_get_vq_desc() will read available
-                * ring entries. Make sure that read happens after
-                * the avail_idx read.
-                */
-               smp_rmb();
-               return true;
-       }
+       /* Treat error as empty here */
+       r = vhost_get_avail_idx(vq);

If you write the comment like that then put it before "return":
that is where you treat an error like this.
And I feel Note: is better in that the comment does not
explain all of what is going on, just an aspect of it.

Sure, will do in v3.

+       if (unlikely(r < 0))
+               return false;

- return false;

+       return r;
  }
  EXPORT_SYMBOL_GPL(vhost_enable_notify);

Thanks,
Gavin