* [PATCH 0/4] vhost: Cleanup
@ 2024-04-23 3:24 Gavin Shan
2024-04-23 3:24 ` [PATCH 1/4] vhost: Drop variable last_avail_idx in vhost_get_vq_desc() Gavin Shan
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-23 3:24 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, mst, jasowang, shan.gavin
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/20240327075940-mutt-send-email-mst@kernel.org/
PATCH[1] drops the local variable @last_avail_idx since it's equivalent
to vq->last_avail_idx
PATCH[2] 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[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 (4):
vhost: Drop variable last_avail_idx in vhost_get_vq_desc()
vhost: Improve vhost_get_avail_idx() with smp_rmb()
vhost: Improve vhost_get_avail_head()
vhost: Reformat vhost_{get, put}_user()
drivers/vhost/vhost.c | 199 +++++++++++++++++++-----------------------
1 file changed, 88 insertions(+), 111 deletions(-)
--
2.44.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/4] vhost: Drop variable last_avail_idx in vhost_get_vq_desc()
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
@ 2024-04-23 3:24 ` Gavin Shan
2024-04-23 3:24 ` [PATCH 2/4] vhost: Improve vhost_get_avail_idx() with smp_rmb() Gavin Shan
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-23 3:24 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, mst, jasowang, shan.gavin
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 <gshan@redhat.com>
---
drivers/vhost/vhost.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 8995730ce0bf..ef7942103232 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2498,14 +2498,11 @@ 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;
__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",
@@ -2514,16 +2511,16 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
}
vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
- if (unlikely((u16)(vq->avail_idx - last_avail_idx) > vq->num)) {
+ if (unlikely((u16)(vq->avail_idx - vq->last_avail_idx) > vq->num)) {
vq_err(vq, "Guest moved avail index from %u to %u",
- last_avail_idx, vq->avail_idx);
+ vq->last_avail_idx, vq->avail_idx);
return -EFAULT;
}
/* If there's nothing new since last we looked, return
* invalid.
*/
- if (vq->avail_idx == last_avail_idx)
+ if (vq->avail_idx == vq->last_avail_idx)
return vq->num;
/* Only get avail ring entries after they have been
@@ -2534,10 +2531,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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/4] vhost: Improve vhost_get_avail_idx() with smp_rmb()
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
2024-04-23 3:24 ` [PATCH 1/4] vhost: Drop variable last_avail_idx in vhost_get_vq_desc() Gavin Shan
@ 2024-04-23 3:24 ` Gavin Shan
2024-04-23 3:24 ` [PATCH 3/4] vhost: Improve vhost_get_avail_head() Gavin Shan
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-23 3:24 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, mst, jasowang, shan.gavin
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.
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
drivers/vhost/vhost.c | 91 ++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 57 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ef7942103232..b3adc0bc9e72 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1290,10 +1290,34 @@ 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 avail_idx;
+ int r;
+
+ r = vhost_get_avail(vq, avail_idx, &vq->avail->idx);
+ if (unlikely(r)) {
+ vq_err(vq, "Failed to access avail idx at %p\n",
+ &vq->avail->idx);
+ return r;
+ }
+
+ vq->avail_idx = vhost16_to_cpu(vq, avail_idx);
+ if (vq->avail_idx != vq->last_avail_idx) {
+ /* Ensure the available ring entry read happens
+ * before the avail_idx read when the avail_idx
+ * is advanced.
+ */
+ smp_rmb();
+ }
+
+ if (unlikely((u16)(vq->avail_idx - vq->last_avail_idx) > vq->num)) {
+ vq_err(vq, "Invalid avail index change from %u to %u",
+ vq->last_avail_idx, vq->avail_idx);
+ return -EINVAL;
+ }
+
+ return 0;
}
static inline int vhost_get_avail_head(struct vhost_virtqueue *vq,
@@ -2498,35 +2522,19 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
{
struct vring_desc desc;
unsigned int i, head, found = 0;
- __virtio16 avail_idx;
__virtio16 ring_head;
int ret, access;
- /* Check it isn't doing very strange things with descriptor numbers. */
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 - vq->last_avail_idx) > vq->num)) {
- vq_err(vq, "Guest moved avail index from %u to %u",
- vq->last_avail_idx, vq->avail_idx);
- return -EFAULT;
- }
+ ret = vhost_get_avail_idx(vq);
+ if (unlikely(ret))
+ return ret;
/* If there's nothing new since last we looked, return
* invalid.
*/
if (vq->avail_idx == vq->last_avail_idx)
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
@@ -2787,35 +2795,19 @@ 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();
+ if (unlikely(vhost_get_avail_idx(vq)))
return false;
- }
- return true;
+ return vq->avail_idx == vq->last_avail_idx;
}
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))
@@ -2839,25 +2831,10 @@ 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);
+ if (unlikely(vhost_get_avail_idx(vq)))
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;
- }
- return false;
+ return vq->avail_idx != vq->last_avail_idx;
}
EXPORT_SYMBOL_GPL(vhost_enable_notify);
--
2.44.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/4] vhost: Improve vhost_get_avail_head()
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
2024-04-23 3:24 ` [PATCH 1/4] vhost: Drop variable last_avail_idx in vhost_get_vq_desc() Gavin Shan
2024-04-23 3:24 ` [PATCH 2/4] vhost: Improve vhost_get_avail_idx() with smp_rmb() Gavin Shan
@ 2024-04-23 3:24 ` Gavin Shan
2024-04-25 20:42 ` kernel test robot
2024-04-23 3:24 ` [PATCH 4/4] vhost: Reformat vhost_{get, put}_user() Gavin Shan
2024-04-29 7:02 ` [PATCH 0/4] vhost: Cleanup Michael S. Tsirkin
4 siblings, 1 reply; 9+ messages in thread
From: Gavin Shan @ 2024-04-23 3:24 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, mst, jasowang, shan.gavin
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 <gshan@redhat.com>
---
drivers/vhost/vhost.c | 43 +++++++++++++++++++++++--------------------
1 file changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index b3adc0bc9e72..a3de9325175f 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1320,11 +1320,27 @@ static inline int vhost_get_avail_idx(struct vhost_virtqueue *vq)
return 0;
}
-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: idx %u address %p\n",
+ vq->last_avail_idx,
+ &vq->avail->ring[vq->last_avail_idx % vq->num]);
+ 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,
@@ -2522,7 +2538,6 @@ int vhost_get_vq_desc(struct vhost_virtqueue *vq,
{
struct vring_desc desc;
unsigned int i, head, found = 0;
- __virtio16 ring_head;
int ret, access;
if (vq->avail_idx == vq->last_avail_idx) {
@@ -2539,21 +2554,9 @@ 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, 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;
- }
+ 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 4/4] vhost: Reformat vhost_{get, put}_user()
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
` (2 preceding siblings ...)
2024-04-23 3:24 ` [PATCH 3/4] vhost: Improve vhost_get_avail_head() Gavin Shan
@ 2024-04-23 3:24 ` Gavin Shan
2024-04-29 7:02 ` [PATCH 0/4] vhost: Cleanup Michael S. Tsirkin
4 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-23 3:24 UTC (permalink / raw)
To: virtualization; +Cc: linux-kernel, mst, jasowang, shan.gavin
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 <gshan@redhat.com>
---
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 a3de9325175f..3be19877f9df 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] vhost: Improve vhost_get_avail_head()
2024-04-23 3:24 ` [PATCH 3/4] vhost: Improve vhost_get_avail_head() Gavin Shan
@ 2024-04-25 20:42 ` kernel test robot
2024-04-25 22:45 ` Gavin Shan
0 siblings, 1 reply; 9+ messages in thread
From: kernel test robot @ 2024-04-25 20:42 UTC (permalink / raw)
To: Gavin Shan, virtualization
Cc: oe-kbuild-all, linux-kernel, mst, jasowang, shan.gavin
Hi Gavin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.9-rc5 next-20240424]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Gavin-Shan/vhost-Drop-variable-last_avail_idx-in-vhost_get_vq_desc/20240423-112803
base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
patch link: https://lore.kernel.org/r/20240423032407.262329-4-gshan%40redhat.com
patch subject: [PATCH 3/4] vhost: Improve vhost_get_avail_head()
config: i386-randconfig-141-20240426 (https://download.01.org/0day-ci/archive/20240426/202404260448.g7F06v7M-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404260448.g7F06v7M-lkp@intel.com/
smatch warnings:
drivers/vhost/vhost.c:2614 vhost_get_vq_desc() warn: unsigned 'head' is never less than zero.
drivers/vhost/vhost.c:2614 vhost_get_vq_desc() warn: error code type promoted to positive: 'head'
vim +/head +2614 drivers/vhost/vhost.c
2581
2582 /* This looks in the virtqueue and for the first available buffer, and converts
2583 * it to an iovec for convenient access. Since descriptors consist of some
2584 * number of output then some number of input descriptors, it's actually two
2585 * iovecs, but we pack them into one and note how many of each there were.
2586 *
2587 * This function returns the descriptor number found, or vq->num (which is
2588 * never a valid descriptor number) if none was found. A negative code is
2589 * returned on error. */
2590 int vhost_get_vq_desc(struct vhost_virtqueue *vq,
2591 struct iovec iov[], unsigned int iov_size,
2592 unsigned int *out_num, unsigned int *in_num,
2593 struct vhost_log *log, unsigned int *log_num)
2594 {
2595 struct vring_desc desc;
2596 unsigned int i, head, found = 0;
2597 int ret, access;
2598
2599 if (vq->avail_idx == vq->last_avail_idx) {
2600 ret = vhost_get_avail_idx(vq);
2601 if (unlikely(ret))
2602 return ret;
2603
2604 /* If there's nothing new since last we looked, return
2605 * invalid.
2606 */
2607 if (vq->avail_idx == vq->last_avail_idx)
2608 return vq->num;
2609 }
2610
2611 /* Grab the next descriptor number they're advertising, and increment
2612 * the index we've seen. */
2613 head = vhost_get_avail_head(vq);
> 2614 if (unlikely(head < 0))
2615 return head;
2616
2617 /* When we start there are none of either input nor output. */
2618 *out_num = *in_num = 0;
2619 if (unlikely(log))
2620 *log_num = 0;
2621
2622 i = head;
2623 do {
2624 unsigned iov_count = *in_num + *out_num;
2625 if (unlikely(i >= vq->num)) {
2626 vq_err(vq, "Desc index is %u > %u, head = %u",
2627 i, vq->num, head);
2628 return -EINVAL;
2629 }
2630 if (unlikely(++found > vq->num)) {
2631 vq_err(vq, "Loop detected: last one at %u "
2632 "vq size %u head %u\n",
2633 i, vq->num, head);
2634 return -EINVAL;
2635 }
2636 ret = vhost_get_desc(vq, &desc, i);
2637 if (unlikely(ret)) {
2638 vq_err(vq, "Failed to get descriptor: idx %d addr %p\n",
2639 i, vq->desc + i);
2640 return -EFAULT;
2641 }
2642 if (desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_INDIRECT)) {
2643 ret = get_indirect(vq, iov, iov_size,
2644 out_num, in_num,
2645 log, log_num, &desc);
2646 if (unlikely(ret < 0)) {
2647 if (ret != -EAGAIN)
2648 vq_err(vq, "Failure detected "
2649 "in indirect descriptor at idx %d\n", i);
2650 return ret;
2651 }
2652 continue;
2653 }
2654
2655 if (desc.flags & cpu_to_vhost16(vq, VRING_DESC_F_WRITE))
2656 access = VHOST_ACCESS_WO;
2657 else
2658 access = VHOST_ACCESS_RO;
2659 ret = translate_desc(vq, vhost64_to_cpu(vq, desc.addr),
2660 vhost32_to_cpu(vq, desc.len), iov + iov_count,
2661 iov_size - iov_count, access);
2662 if (unlikely(ret < 0)) {
2663 if (ret != -EAGAIN)
2664 vq_err(vq, "Translation failure %d descriptor idx %d\n",
2665 ret, i);
2666 return ret;
2667 }
2668 if (access == VHOST_ACCESS_WO) {
2669 /* If this is an input descriptor,
2670 * increment that count. */
2671 *in_num += ret;
2672 if (unlikely(log && ret)) {
2673 log[*log_num].addr = vhost64_to_cpu(vq, desc.addr);
2674 log[*log_num].len = vhost32_to_cpu(vq, desc.len);
2675 ++*log_num;
2676 }
2677 } else {
2678 /* If it's an output descriptor, they're all supposed
2679 * to come before any input descriptors. */
2680 if (unlikely(*in_num)) {
2681 vq_err(vq, "Descriptor has out after in: "
2682 "idx %d\n", i);
2683 return -EINVAL;
2684 }
2685 *out_num += ret;
2686 }
2687 } while ((i = next_desc(vq, &desc)) != -1);
2688
2689 /* On success, increment avail index. */
2690 vq->last_avail_idx++;
2691
2692 /* Assume notifications from guest are disabled at this point,
2693 * if they aren't we would need to update avail_event index. */
2694 BUG_ON(!(vq->used_flags & VRING_USED_F_NO_NOTIFY));
2695 return head;
2696 }
2697 EXPORT_SYMBOL_GPL(vhost_get_vq_desc);
2698
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/4] vhost: Improve vhost_get_avail_head()
2024-04-25 20:42 ` kernel test robot
@ 2024-04-25 22:45 ` Gavin Shan
0 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-25 22:45 UTC (permalink / raw)
To: kernel test robot, virtualization
Cc: oe-kbuild-all, linux-kernel, mst, jasowang, shan.gavin
On 4/26/24 06:42, kernel test robot wrote:> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on mst-vhost/linux-next]
> [also build test WARNING on linus/master v6.9-rc5 next-20240424]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Gavin-Shan/vhost-Drop-variable-last_avail_idx-in-vhost_get_vq_desc/20240423-112803
> base: https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
> patch link: https://lore.kernel.org/r/20240423032407.262329-4-gshan%40redhat.com
> patch subject: [PATCH 3/4] vhost: Improve vhost_get_avail_head()
> config: i386-randconfig-141-20240426 (https://download.01.org/0day-ci/archive/20240426/202404260448.g7F06v7M-lkp@intel.com/config)
> compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202404260448.g7F06v7M-lkp@intel.com/
>
> smatch warnings:
> drivers/vhost/vhost.c:2614 vhost_get_vq_desc() warn: unsigned 'head' is never less than zero.
> drivers/vhost/vhost.c:2614 vhost_get_vq_desc() warn: error code type promoted to positive: 'head'
>
> vim +/head +2614 drivers/vhost/vhost.c
>
> 2581
> 2582 /* This looks in the virtqueue and for the first available buffer, and converts
> 2583 * it to an iovec for convenient access. Since descriptors consist of some
> 2584 * number of output then some number of input descriptors, it's actually two
> 2585 * iovecs, but we pack them into one and note how many of each there were.
> 2586 *
> 2587 * This function returns the descriptor number found, or vq->num (which is
> 2588 * never a valid descriptor number) if none was found. A negative code is
> 2589 * returned on error. */
> 2590 int vhost_get_vq_desc(struct vhost_virtqueue *vq,
> 2591 struct iovec iov[], unsigned int iov_size,
> 2592 unsigned int *out_num, unsigned int *in_num,
> 2593 struct vhost_log *log, unsigned int *log_num)
> 2594 {
> 2595 struct vring_desc desc;
> 2596 unsigned int i, head, found = 0;
> 2597 int ret, access;
> 2598
> 2599 if (vq->avail_idx == vq->last_avail_idx) {
> 2600 ret = vhost_get_avail_idx(vq);
> 2601 if (unlikely(ret))
> 2602 return ret;
> 2603
> 2604 /* If there's nothing new since last we looked, return
> 2605 * invalid.
> 2606 */
> 2607 if (vq->avail_idx == vq->last_avail_idx)
> 2608 return vq->num;
> 2609 }
> 2610
> 2611 /* Grab the next descriptor number they're advertising, and increment
> 2612 * the index we've seen. */
> 2613 head = vhost_get_avail_head(vq);
>> 2614 if (unlikely(head < 0))
> 2615 return head;
Thanks for the report. @head needs to be 'int' instead of 'unsigned int'
so that it can hold the error number from vhost_get_avail_head(). I would
give it more time to see if there are other review comments before I revise
it to fix it up.
Thanks,
Gavin
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] vhost: Cleanup
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
` (3 preceding siblings ...)
2024-04-23 3:24 ` [PATCH 4/4] vhost: Reformat vhost_{get, put}_user() Gavin Shan
@ 2024-04-29 7:02 ` Michael S. Tsirkin
2024-04-29 10:17 ` Gavin Shan
4 siblings, 1 reply; 9+ messages in thread
From: Michael S. Tsirkin @ 2024-04-29 7:02 UTC (permalink / raw)
To: Gavin Shan; +Cc: virtualization, linux-kernel, jasowang, shan.gavin
On Tue, Apr 23, 2024 at 01:24:03PM +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.
Gavin I suggested another approach.
1. Start with the patch I sent (vhost: order avail ring reads after
index updates) just do a diff against latest.
simplify error handling a bit.
2. Do any other cleanups on top.
> [1] https://lore.kernel.org/virtualization/20240327075940-mutt-send-email-mst@kernel.org/
>
> PATCH[1] drops the local variable @last_avail_idx since it's equivalent
> to vq->last_avail_idx
> PATCH[2] 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[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 (4):
> vhost: Drop variable last_avail_idx in vhost_get_vq_desc()
> vhost: Improve vhost_get_avail_idx() with smp_rmb()
> vhost: Improve vhost_get_avail_head()
> vhost: Reformat vhost_{get, put}_user()
>
> drivers/vhost/vhost.c | 199 +++++++++++++++++++-----------------------
> 1 file changed, 88 insertions(+), 111 deletions(-)
>
> --
> 2.44.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/4] vhost: Cleanup
2024-04-29 7:02 ` [PATCH 0/4] vhost: Cleanup Michael S. Tsirkin
@ 2024-04-29 10:17 ` Gavin Shan
0 siblings, 0 replies; 9+ messages in thread
From: Gavin Shan @ 2024-04-29 10:17 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: virtualization, linux-kernel, jasowang, shan.gavin
On 4/29/24 17:02, Michael S. Tsirkin wrote:
> On Tue, Apr 23, 2024 at 01:24:03PM +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.
>
>
> Gavin I suggested another approach.
> 1. Start with the patch I sent (vhost: order avail ring reads after
> index updates) just do a diff against latest.
> simplify error handling a bit.
> 2. Do any other cleanups on top.
>
My apologies, Michael. I didn't see your patch until now [1]
[1] https://lore.kernel.org/virtualization/20240327155750-mutt-send-email-mst@kernel.org/
v2 was sent with your changes integrated and other cleanup are applied on
top of it. Please take a look when you getting a chance.
v2: https://lore.kernel.org/virtualization/20240429101400.617007-1-gshan@redhat.com/T/#t
Thanks,
Gavin
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-04-29 10:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-23 3:24 [PATCH 0/4] vhost: Cleanup Gavin Shan
2024-04-23 3:24 ` [PATCH 1/4] vhost: Drop variable last_avail_idx in vhost_get_vq_desc() Gavin Shan
2024-04-23 3:24 ` [PATCH 2/4] vhost: Improve vhost_get_avail_idx() with smp_rmb() Gavin Shan
2024-04-23 3:24 ` [PATCH 3/4] vhost: Improve vhost_get_avail_head() Gavin Shan
2024-04-25 20:42 ` kernel test robot
2024-04-25 22:45 ` Gavin Shan
2024-04-23 3:24 ` [PATCH 4/4] vhost: Reformat vhost_{get, put}_user() Gavin Shan
2024-04-29 7:02 ` [PATCH 0/4] vhost: Cleanup Michael S. Tsirkin
2024-04-29 10:17 ` Gavin Shan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).