This is suggested by Michael S. Tsirkin according to [1] and the goal
is to apply smp_rmb() inside vhost_get_avail_idx() if needed. With it,
the caller of the function needn't to worry about memory barriers. Since
we're here, other cleanups are also applied.
[1] https://lore.kernel.org/virtualization/[email protected]/
PATCH[1] improves vhost_get_avail_idx() so that smp_rmb() is applied if
needed. Besides, the sanity checks on the retrieved available
queue index are also squeezed to vhost_get_avail_idx()
PATCH[2] drops the local variable @last_avail_idx since it's equivalent
to vq->last_avail_idx
PATCH[3] improves vhost_get_avail_head(), similar to what we're doing
for vhost_get_avail_idx(), so that the relevant sanity checks
on the head are squeezed to vhost_get_avail_head()
PATCH[4] Reformat vhost_{get, put}_user() by using tab instead of space
as the terminator for each line
Gavin Shan (3):
vhost: Drop variable last_avail_idx in vhost_get_vq_desc()
vhost: Improve vhost_get_avail_head()
vhost: Reformat vhost_{get, put}_user()
Michael S. Tsirkin (1):
vhost: Improve vhost_get_avail_idx() with smp_rmb()
drivers/vhost/vhost.c | 215 +++++++++++++++++++-----------------------
1 file changed, 97 insertions(+), 118 deletions(-)
Changelog
=========
v2:
* Improve vhost_get_avail_idx() as Michael suggested in [1]
as above (Michael)
* Correct @head's type from 'unsigned int' to 'int' ([email protected])
--
2.44.0
From: "Michael S. Tsirkin" <[email protected]>
All the callers of vhost_get_avail_idx() are concerned to the memory
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. With it, the callers needn't to worry
about the memory barrier.
No functional change intended.
Signed-off-by: Michael S. Tsirkin <[email protected]>
[gshan: repainted vhost_get_avail_idx()]
Reviewed-by: Gavin Shan <[email protected]>
Acked-by: Will Deacon <[email protected]>
---
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 */
+ 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 (unlikely(r < 0))
+ return false;
- return false;
+ return r;
}
EXPORT_SYMBOL_GPL(vhost_enable_notify);
--
2.44.0
The local variable @last_avail_idx is equivalent to vq->last_avail_idx.
So the code can be simplified a bit by dropping the local variable
@last_avail_idx.
No functional change intended.
Signed-off-by: Gavin Shan <[email protected]>
---
drivers/vhost/vhost.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 7aa623117aab..b278c0333a66 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2524,7 +2524,6 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
{
struct vring_desc desc;
unsigned int i, head, found = 0;
- u16 last_avail_idx = vq->last_avail_idx;
__virtio16 ring_head;
int ret, access;
@@ -2539,10 +2538,10 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
/* Grab the next descriptor number they're advertising, and increment
* the index we've seen. */
- if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
+ if (unlikely(vhost_get_avail_head(vq, &ring_head, vq->last_avail_idx))) {
vq_err(vq, "Failed to read head: idx %d address %p\n",
- last_avail_idx,
- &vq->avail->ring[last_avail_idx % vq->num]);
+ vq->last_avail_idx,
+ &vq->avail->ring[vq->last_avail_idx % vq->num]);
return -EFAULT;
}
--
2.44.0
Reformat the macros to use tab as the terminator for each line so
that it looks clean.
No functional change intended.
Signed-off-by: Gavin Shan <[email protected]>
---
drivers/vhost/vhost.c | 60 +++++++++++++++++++++----------------------
1 file changed, 30 insertions(+), 30 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 4ddb9ec2fe46..c1ed5e750521 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1207,21 +1207,22 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
return __vhost_get_user_slow(vq, addr, size, type);
}
-#define vhost_put_user(vq, x, ptr) \
-({ \
- int ret; \
- if (!vq->iotlb) { \
- ret = __put_user(x, ptr); \
- } else { \
- __typeof__(ptr) to = \
+#define vhost_put_user(vq, x, ptr) \
+({ \
+ int ret; \
+ if (!vq->iotlb) { \
+ ret = __put_user(x, ptr); \
+ } else { \
+ __typeof__(ptr) to = \
(__typeof__(ptr)) __vhost_get_user(vq, ptr, \
- sizeof(*ptr), VHOST_ADDR_USED); \
- if (to != NULL) \
- ret = __put_user(x, to); \
- else \
- ret = -EFAULT; \
- } \
- ret; \
+ sizeof(*ptr), \
+ VHOST_ADDR_USED); \
+ if (to != NULL) \
+ ret = __put_user(x, to); \
+ else \
+ ret = -EFAULT; \
+ } \
+ ret; \
})
static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
@@ -1252,22 +1253,21 @@ static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
&vq->used->idx);
}
-#define vhost_get_user(vq, x, ptr, type) \
-({ \
- int ret; \
- if (!vq->iotlb) { \
- ret = __get_user(x, ptr); \
- } else { \
- __typeof__(ptr) from = \
- (__typeof__(ptr)) __vhost_get_user(vq, ptr, \
- sizeof(*ptr), \
- type); \
- if (from != NULL) \
- ret = __get_user(x, from); \
- else \
- ret = -EFAULT; \
- } \
- ret; \
+#define vhost_get_user(vq, x, ptr, type) \
+({ \
+ int ret; \
+ if (!vq->iotlb) { \
+ ret = __get_user(x, ptr); \
+ } else { \
+ __typeof__(ptr) from = \
+ (__typeof__(ptr)) __vhost_get_user(vq, ptr, \
+ sizeof(*ptr), type); \
+ if (from != NULL) \
+ ret = __get_user(x, from); \
+ else \
+ ret = -EFAULT; \
+ } \
+ ret; \
})
#define vhost_get_avail(vq, x, ptr) \
--
2.44.0
Improve vhost_get_avail_head() so that the head or errno is returned.
With it, the relevant sanity checks are squeezed to vhost_get_avail_head()
and vhost_get_vq_desc() is further simplified.
No functional change intended.
Signed-off-by: Gavin Shan <[email protected]>
---
drivers/vhost/vhost.c | 50 ++++++++++++++++++++++---------------------
1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b278c0333a66..4ddb9ec2fe46 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1322,11 +1322,27 @@ static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq)
return 1;
}
-static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
- __virtio16 *head, int idx)
+static inline int vhost_get_avail_head(struct vhost_virtqueue *vq)
{
- return vhost_get_avail(vq, *head,
- &vq->avail->ring[idx & (vq->num - 1)]);
+ __virtio16 head;
+ int r;
+
+ r = vhost_get_avail(vq, head,
+ &vq->avail->ring[vq->last_avail_idx & (vq->num - 1)]);
+ if (unlikely(r)) {
+ vq_err(vq, "Failed to read head: index %u address %p\n",
+ vq->last_avail_idx,
+ &vq->avail->ring[vq->last_avail_idx & (vq->num - 1)]);
+ return r;
+ }
+
+ r = vhost16_to_cpu(vq, head);
+ if (unlikely(r >= vq->num)) {
+ vq_err(vq, "Invalid head %d (%u)\n", r, vq->num);
+ return -EINVAL;
+ }
+
+ return r;
}
static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
@@ -2523,9 +2539,8 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
struct vhost_log *log, unsigned int *log_num)
{
struct vring_desc desc;
- unsigned int i, head, found = 0;
- __virtio16 ring_head;
- int ret, access;
+ unsigned int i, found = 0;
+ int head, ret, access;
if (vq->avail_idx == vq->last_avail_idx) {
ret = vhost_get_avail_idx(vq);
@@ -2536,23 +2551,10 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
return vq->num;
}
- /* Grab the next descriptor number they're advertising, and increment
- * the index we've seen. */
- if (unlikely(vhost_get_avail_head(vq, &ring_head, vq->last_avail_idx))) {
- vq_err(vq, "Failed to read head: idx %d address %p\n",
- vq->last_avail_idx,
- &vq->avail->ring[vq->last_avail_idx % vq->num]);
- return -EFAULT;
- }
-
- head = vhost16_to_cpu(vq, ring_head);
-
- /* If their number is silly, that's an error. */
- if (unlikely(head >= vq->num)) {
- vq_err(vq, "Guest says index %u > %u is available",
- head, vq->num);
- return -EINVAL;
- }
+ /* Grab the next descriptor number they're advertising */
+ head = vhost_get_avail_head(vq);
+ if (unlikely(head < 0))
+ return head;
/* When we start there are none of either input nor output. */
*out_num = *in_num = 0;
--
2.44.0
On Mon, Apr 29, 2024 at 08:13:57PM +1000, Gavin Shan wrote:
> From: "Michael S. Tsirkin" <[email protected]>
>
> All the callers of vhost_get_avail_idx() are concerned to the memory
*with* the memory barrier
> 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.
> 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.
> Signed-off-by: Michael S. Tsirkin <[email protected]>
> [gshan: repainted vhost_get_avail_idx()]
?repainted?
> Reviewed-by: Gavin Shan <[email protected]>
> Acked-by: Will Deacon <[email protected]>
> ---
> 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.
> + 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.
> + if (unlikely(r < 0))
> + return false;
>
> - return false;
> + return r;
> }
> EXPORT_SYMBOL_GPL(vhost_enable_notify);
>
> --
> 2.44.0
On Mon, Apr 29, 2024 at 08:13:58PM +1000, Gavin Shan wrote:
> The local variable @last_avail_idx is equivalent to vq->last_avail_idx.
> So the code can be simplified a bit by dropping the local variable
> @last_avail_idx.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <[email protected]>
> ---
> drivers/vhost/vhost.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 7aa623117aab..b278c0333a66 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -2524,7 +2524,6 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> {
> struct vring_desc desc;
> unsigned int i, head, found = 0;
> - u16 last_avail_idx = vq->last_avail_idx;
> __virtio16 ring_head;
> int ret, access;
>
> @@ -2539,10 +2538,10 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
>
> /* Grab the next descriptor number they're advertising, and increment
> * the index we've seen. */
> - if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
> + if (unlikely(vhost_get_avail_head(vq, &ring_head, vq->last_avail_idx))) {
> vq_err(vq, "Failed to read head: idx %d address %p\n",
> - last_avail_idx,
> - &vq->avail->ring[last_avail_idx % vq->num]);
> + vq->last_avail_idx,
> + &vq->avail->ring[vq->last_avail_idx % vq->num]);
> return -EFAULT;
> }
I don't see the big advantage and the line is long now.
>
> --
> 2.44.0
On Mon, Apr 29, 2024 at 08:14:00PM +1000, Gavin Shan wrote:
> Reformat the macros to use tab as the terminator for each line so
> that it looks clean.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <[email protected]>
Just messes up history for no real gain.
> ---
> drivers/vhost/vhost.c | 60 +++++++++++++++++++++----------------------
> 1 file changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 4ddb9ec2fe46..c1ed5e750521 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1207,21 +1207,22 @@ static inline void __user *__vhost_get_user(struct vhost_virtqueue *vq,
> return __vhost_get_user_slow(vq, addr, size, type);
> }
>
> -#define vhost_put_user(vq, x, ptr) \
> -({ \
> - int ret; \
> - if (!vq->iotlb) { \
> - ret = __put_user(x, ptr); \
> - } else { \
> - __typeof__(ptr) to = \
> +#define vhost_put_user(vq, x, ptr) \
> +({ \
> + int ret; \
> + if (!vq->iotlb) { \
> + ret = __put_user(x, ptr); \
> + } else { \
> + __typeof__(ptr) to = \
> (__typeof__(ptr)) __vhost_get_user(vq, ptr, \
> - sizeof(*ptr), VHOST_ADDR_USED); \
> - if (to != NULL) \
> - ret = __put_user(x, to); \
> - else \
> - ret = -EFAULT; \
> - } \
> - ret; \
> + sizeof(*ptr), \
> + VHOST_ADDR_USED); \
> + if (to != NULL) \
> + ret = __put_user(x, to); \
> + else \
> + ret = -EFAULT; \
> + } \
> + ret; \
> })
>
> static inline int vhost_put_avail_event(struct vhost_virtqueue *vq)
> @@ -1252,22 +1253,21 @@ static inline int vhost_put_used_idx(struct vhost_virtqueue *vq)
> &vq->used->idx);
> }
>
> -#define vhost_get_user(vq, x, ptr, type) \
> -({ \
> - int ret; \
> - if (!vq->iotlb) { \
> - ret = __get_user(x, ptr); \
> - } else { \
> - __typeof__(ptr) from = \
> - (__typeof__(ptr)) __vhost_get_user(vq, ptr, \
> - sizeof(*ptr), \
> - type); \
> - if (from != NULL) \
> - ret = __get_user(x, from); \
> - else \
> - ret = -EFAULT; \
> - } \
> - ret; \
> +#define vhost_get_user(vq, x, ptr, type) \
> +({ \
> + int ret; \
> + if (!vq->iotlb) { \
> + ret = __get_user(x, ptr); \
> + } else { \
> + __typeof__(ptr) from = \
> + (__typeof__(ptr)) __vhost_get_user(vq, ptr, \
> + sizeof(*ptr), type); \
> + if (from != NULL) \
> + ret = __get_user(x, from); \
> + else \
> + ret = -EFAULT; \
> + } \
> + ret; \
> })
>
> #define vhost_get_avail(vq, x, ptr) \
> --
> 2.44.0
On Mon, Apr 29, 2024 at 08:13:56PM +1000, Gavin Shan wrote:
> This is suggested by Michael S. Tsirkin according to [1] and the goal
> is to apply smp_rmb() inside vhost_get_avail_idx() if needed. With it,
> the caller of the function needn't to worry about memory barriers. Since
> we're here, other cleanups are also applied.
>
> [1] https://lore.kernel.org/virtualization/[email protected]/
Patch 1 makes some sense, gave some comments. Rest I think we should
just drop.
> PATCH[1] improves vhost_get_avail_idx() so that smp_rmb() is applied if
> needed. Besides, the sanity checks on the retrieved available
> queue index are also squeezed to vhost_get_avail_idx()
> PATCH[2] drops the local variable @last_avail_idx since it's equivalent
> to vq->last_avail_idx
> PATCH[3] improves vhost_get_avail_head(), similar to what we're doing
> for vhost_get_avail_idx(), so that the relevant sanity checks
> on the head are squeezed to vhost_get_avail_head()
> PATCH[4] Reformat vhost_{get, put}_user() by using tab instead of space
> as the terminator for each line
>
> Gavin Shan (3):
> vhost: Drop variable last_avail_idx in vhost_get_vq_desc()
> vhost: Improve vhost_get_avail_head()
> vhost: Reformat vhost_{get, put}_user()
>
> Michael S. Tsirkin (1):
> vhost: Improve vhost_get_avail_idx() with smp_rmb()
>
> drivers/vhost/vhost.c | 215 +++++++++++++++++++-----------------------
> 1 file changed, 97 insertions(+), 118 deletions(-)
>
> Changelog
> =========
> v2:
> * Improve vhost_get_avail_idx() as Michael suggested in [1]
> as above (Michael)
> * Correct @head's type from 'unsigned int' to 'int' ([email protected])
>
> --
> 2.44.0
On Mon, Apr 29, 2024 at 08:13:59PM +1000, Gavin Shan wrote:
> Improve vhost_get_avail_head() so that the head or errno is returned.
> With it, the relevant sanity checks are squeezed to vhost_get_avail_head()
> and vhost_get_vq_desc() is further simplified.
>
> No functional change intended.
>
> Signed-off-by: Gavin Shan <[email protected]>
I don't see what does this moving code around achieve.
> ---
> drivers/vhost/vhost.c | 50 ++++++++++++++++++++++---------------------
> 1 file changed, 26 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index b278c0333a66..4ddb9ec2fe46 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1322,11 +1322,27 @@ static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq)
> return 1;
> }
>
> -static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
> - __virtio16 *head, int idx)
> +static inline int vhost_get_avail_head(struct vhost_virtqueue *vq)
> {
> - return vhost_get_avail(vq, *head,
> - &vq->avail->ring[idx & (vq->num - 1)]);
> + __virtio16 head;
> + int r;
> +
> + r = vhost_get_avail(vq, head,
> + &vq->avail->ring[vq->last_avail_idx & (vq->num - 1)]);
> + if (unlikely(r)) {
> + vq_err(vq, "Failed to read head: index %u address %p\n",
> + vq->last_avail_idx,
> + &vq->avail->ring[vq->last_avail_idx & (vq->num - 1)]);
> + return r;
> + }
> +
> + r = vhost16_to_cpu(vq, head);
> + if (unlikely(r >= vq->num)) {
> + vq_err(vq, "Invalid head %d (%u)\n", r, vq->num);
> + return -EINVAL;
> + }
> +
> + return r;
> }
>
> static inline int vhost_get_avail_flags(struct vhost_virtqueue *vq,
> @@ -2523,9 +2539,8 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> struct vhost_log *log, unsigned int *log_num)
> {
> struct vring_desc desc;
> - unsigned int i, head, found = 0;
> - __virtio16 ring_head;
> - int ret, access;
> + unsigned int i, found = 0;
> + int head, ret, access;
>
> if (vq->avail_idx == vq->last_avail_idx) {
> ret = vhost_get_avail_idx(vq);
> @@ -2536,23 +2551,10 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> return vq->num;
> }
>
> - /* Grab the next descriptor number they're advertising, and increment
> - * the index we've seen. */
> - if (unlikely(vhost_get_avail_head(vq, &ring_head, vq->last_avail_idx))) {
> - vq_err(vq, "Failed to read head: idx %d address %p\n",
> - vq->last_avail_idx,
> - &vq->avail->ring[vq->last_avail_idx % vq->num]);
> - return -EFAULT;
> - }
> -
> - head = vhost16_to_cpu(vq, ring_head);
> -
> - /* If their number is silly, that's an error. */
> - if (unlikely(head >= vq->num)) {
> - vq_err(vq, "Guest says index %u > %u is available",
> - head, vq->num);
> - return -EINVAL;
> - }
> + /* Grab the next descriptor number they're advertising */
> + head = vhost_get_avail_head(vq);
> + if (unlikely(head < 0))
> + return head;
>
> /* When we start there are none of either input nor output. */
> *out_num = *in_num = 0;
> --
> 2.44.0
On 4/30/24 04:45, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2024 at 08:13:58PM +1000, Gavin Shan wrote:
>> The local variable @last_avail_idx is equivalent to vq->last_avail_idx.
>> So the code can be simplified a bit by dropping the local variable
>> @last_avail_idx.
>>
>> No functional change intended.
>>
>> Signed-off-by: Gavin Shan <[email protected]>
>> ---
>> drivers/vhost/vhost.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>> index 7aa623117aab..b278c0333a66 100644
>> --- a/drivers/vhost/vhost.c
>> +++ b/drivers/vhost/vhost.c
>> @@ -2524,7 +2524,6 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
>> {
>> struct vring_desc desc;
>> unsigned int i, head, found = 0;
>> - u16 last_avail_idx = vq->last_avail_idx;
>> __virtio16 ring_head;
>> int ret, access;
>>
>> @@ -2539,10 +2538,10 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
>>
>> /* Grab the next descriptor number they're advertising, and increment
>> * the index we've seen. */
>> - if (unlikely(vhost_get_avail_head(vq, &ring_head, last_avail_idx))) {
>> + if (unlikely(vhost_get_avail_head(vq, &ring_head, vq->last_avail_idx))) {
>> vq_err(vq, "Failed to read head: idx %d address %p\n",
>> - last_avail_idx,
>> - &vq->avail->ring[last_avail_idx % vq->num]);
>> + vq->last_avail_idx,
>> + &vq->avail->ring[vq->last_avail_idx % vq->num]);
>> return -EFAULT;
>> }
>
> I don't see the big advantage and the line is long now.
>
The point is to avoid the local variable @last_avail_idx since it's equivalent
to vq->last_avail_idx, as stated in the commit log. Besides, it paves the way
for PATCH[v2 3/4] where the whole logic fetching the head and sanity check is
moved to vhost_get_avail_head(), so that vhost_get_vq_desc() is simplified
I will drop PATCH[2, 3, 4] as you suggested.
Thanks,
Gavin
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" <[email protected]>
>>
>> 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 <[email protected]>
>> [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 <[email protected]>
>> Acked-by: Will Deacon <[email protected]>
>> ---
>> 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
On 4/30/24 04:50, Michael S. Tsirkin wrote:
> On Mon, Apr 29, 2024 at 08:13:56PM +1000, Gavin Shan wrote:
>> This is suggested by Michael S. Tsirkin according to [1] and the goal
>> is to apply smp_rmb() inside vhost_get_avail_idx() if needed. With it,
>> the caller of the function needn't to worry about memory barriers. Since
>> we're here, other cleanups are also applied.
>>
>> [1] https://lore.kernel.org/virtualization/[email protected]/
>
>
> Patch 1 makes some sense, gave some comments. Rest I think we should
> just drop.
>
Sure, v3 has been sent with PATCH[v2 2/3/4] dropped. Please take a look
when you getting a chance.
v3: https://lore.kernel.org/virtualization/[email protected]/T/#u
Thanks,
Gavin