diff mbox series

[v3,5/6] vhost-vdpa: backend feature should set only once

Message ID 1651812874-31967-6-git-send-email-si-wei.liu@oracle.com (mailing list archive)
State New, archived
Headers show
Series vhost-vdpa multiqueue fixes | expand

Commit Message

Si-Wei Liu May 6, 2022, 4:54 a.m. UTC 
The vhost_vdpa_one_time_request() branch in
vhost_vdpa_set_backend_cap() incorrectly sends down
ioctls on vhost_dev with non-zero index. This may
end up with multiple VHOST_SET_BACKEND_FEATURES
ioctl calls sent down on the vhost-vdpa fd that is
shared between all these vhost_dev's.

To fix it, send down ioctl only once via the first
vhost_dev with index 0. For more readibility of
code, vhost_vdpa_one_time_request() is renamed to
vhost_vdpa_first_dev() with polarity flipped.
This call is only applicable to the request that
performs operation before setting up queues, and
usually at the beginning of operation. Document
the requirement for it in place.

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-vdpa.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Stefano Garzarella May 6, 2022, 12:22 p.m. UTC | #1 
On Thu, May 05, 2022 at 09:54:33PM -0700, Si-Wei Liu wrote:
>The vhost_vdpa_one_time_request() branch in
>vhost_vdpa_set_backend_cap() incorrectly sends down
>ioctls on vhost_dev with non-zero index. This may
>end up with multiple VHOST_SET_BACKEND_FEATURES
>ioctl calls sent down on the vhost-vdpa fd that is
>shared between all these vhost_dev's.
>
>To fix it, send down ioctl only once via the first
>vhost_dev with index 0. For more readibility of
>code, vhost_vdpa_one_time_request() is renamed to
>vhost_vdpa_first_dev() with polarity flipped.

Sorry for being late, you may have already discussed this, but IMHO it 
would be better to split this patch into two:
- fix the problem in vhost_vdpa_set_backend_cap()
- change name and polarity of vhost_vdpa_one_time_request()

Thanks,
Stefano

>This call is only applicable to the request that
>performs operation before setting up queues, and
>usually at the beginning of operation. Document
>the requirement for it in place.
>
>Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>Acked-by: Jason Wang <jasowang@redhat.com>
>Acked-by: Eugenio Pérez <eperezma@redhat.com>
>---
> hw/virtio/vhost-vdpa.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>index 8adf7c0..fd1268e 100644
>--- a/hw/virtio/vhost-vdpa.c
>+++ b/hw/virtio/vhost-vdpa.c
>@@ -366,11 +366,18 @@ static void vhost_vdpa_get_iova_range(struct vhost_vdpa *v)
>                                     v->iova_range.last);
> }
>
>-static bool vhost_vdpa_one_time_request(struct vhost_dev *dev)
>+/*
>+ * The use of this function is for requests that only need to be
>+ * applied once. Typically such request occurs at the beginning
>+ * of operation, and before setting up queues. It should not be
>+ * used for request that performs operation until all queues are
>+ * set, which would need to check dev->vq_index_end instead.
>+ */
>+static bool vhost_vdpa_first_dev(struct vhost_dev *dev)
> {
>     struct vhost_vdpa *v = dev->opaque;
>
>-    return v->index != 0;
>+    return v->index == 0;
> }
>
> static int vhost_vdpa_get_dev_features(struct vhost_dev *dev,
>@@ -451,7 +458,7 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
>
>     vhost_vdpa_get_iova_range(v);
>
>-    if (vhost_vdpa_one_time_request(dev)) {
>+    if (!vhost_vdpa_first_dev(dev)) {
>         return 0;
>     }
>
>@@ -594,7 +601,7 @@ static int vhost_vdpa_memslots_limit(struct vhost_dev *dev)
> static int vhost_vdpa_set_mem_table(struct vhost_dev *dev,
>                                     struct vhost_memory *mem)
> {
>-    if (vhost_vdpa_one_time_request(dev)) {
>+    if (!vhost_vdpa_first_dev(dev)) {
>         return 0;
>     }
>
>@@ -623,7 +630,7 @@ static int vhost_vdpa_set_features(struct vhost_dev *dev,
>     struct vhost_vdpa *v = dev->opaque;
>     int ret;
>
>-    if (vhost_vdpa_one_time_request(dev)) {
>+    if (!vhost_vdpa_first_dev(dev)) {
>         return 0;
>     }
>
>@@ -665,7 +672,7 @@ static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
>
>     features &= f;
>
>-    if (vhost_vdpa_one_time_request(dev)) {
>+    if (vhost_vdpa_first_dev(dev)) {
>         r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
>         if (r) {
>             return -EFAULT;
>@@ -1118,7 +1125,7 @@ static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
>                                      struct vhost_log *log)
> {
>     struct vhost_vdpa *v = dev->opaque;
>-    if (v->shadow_vqs_enabled || vhost_vdpa_one_time_request(dev)) {
>+    if (v->shadow_vqs_enabled || !vhost_vdpa_first_dev(dev)) {
>         return 0;
>     }
>
>@@ -1240,7 +1247,7 @@ static int vhost_vdpa_get_features(struct vhost_dev *dev,
>
> static int vhost_vdpa_set_owner(struct vhost_dev *dev)
> {
>-    if (vhost_vdpa_one_time_request(dev)) {
>+    if (!vhost_vdpa_first_dev(dev)) {
>         return 0;
>     }
>
>-- 
>1.8.3.1
>
diff mbox series

Patch

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 8adf7c0..fd1268e 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -366,11 +366,18 @@  static void vhost_vdpa_get_iova_range(struct vhost_vdpa *v)
                                     v->iova_range.last);
 }
 
-static bool vhost_vdpa_one_time_request(struct vhost_dev *dev)
+/*
+ * The use of this function is for requests that only need to be
+ * applied once. Typically such request occurs at the beginning 
+ * of operation, and before setting up queues. It should not be
+ * used for request that performs operation until all queues are
+ * set, which would need to check dev->vq_index_end instead.
+ */
+static bool vhost_vdpa_first_dev(struct vhost_dev *dev)
 {
     struct vhost_vdpa *v = dev->opaque;
 
-    return v->index != 0;
+    return v->index == 0;
 }
 
 static int vhost_vdpa_get_dev_features(struct vhost_dev *dev,
@@ -451,7 +458,7 @@  static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque, Error **errp)
 
     vhost_vdpa_get_iova_range(v);
 
-    if (vhost_vdpa_one_time_request(dev)) {
+    if (!vhost_vdpa_first_dev(dev)) {
         return 0;
     }
 
@@ -594,7 +601,7 @@  static int vhost_vdpa_memslots_limit(struct vhost_dev *dev)
 static int vhost_vdpa_set_mem_table(struct vhost_dev *dev,
                                     struct vhost_memory *mem)
 {
-    if (vhost_vdpa_one_time_request(dev)) {
+    if (!vhost_vdpa_first_dev(dev)) {
         return 0;
     }
 
@@ -623,7 +630,7 @@  static int vhost_vdpa_set_features(struct vhost_dev *dev,
     struct vhost_vdpa *v = dev->opaque;
     int ret;
 
-    if (vhost_vdpa_one_time_request(dev)) {
+    if (!vhost_vdpa_first_dev(dev)) {
         return 0;
     }
 
@@ -665,7 +672,7 @@  static int vhost_vdpa_set_backend_cap(struct vhost_dev *dev)
 
     features &= f;
 
-    if (vhost_vdpa_one_time_request(dev)) {
+    if (vhost_vdpa_first_dev(dev)) {
         r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
         if (r) {
             return -EFAULT;
@@ -1118,7 +1125,7 @@  static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
                                      struct vhost_log *log)
 {
     struct vhost_vdpa *v = dev->opaque;
-    if (v->shadow_vqs_enabled || vhost_vdpa_one_time_request(dev)) {
+    if (v->shadow_vqs_enabled || !vhost_vdpa_first_dev(dev)) {
         return 0;
     }
 
@@ -1240,7 +1247,7 @@  static int vhost_vdpa_get_features(struct vhost_dev *dev,
 
 static int vhost_vdpa_set_owner(struct vhost_dev *dev)
 {
-    if (vhost_vdpa_one_time_request(dev)) {
+    if (!vhost_vdpa_first_dev(dev)) {
         return 0;
     }