1. Patchew
  2. QEMU
  3. View series

[PATCH] vhost: fail device start if iotlb update fails

Prasad Pandit posted 1 patch 2 weeks, 4 days ago
There is a newer version of this series
hw/virtio/vhost.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
[PATCH] vhost: fail device start if iotlb update fails
Posted by Prasad Pandit 2 weeks, 4 days ago 
From: Prasad Pandit <pjp@fedoraproject.org>

While starting a vhost device, updating iotlb entries
via 'vhost_device_iotlb_miss' may return an error.

  qemu-kvm: vhost_device_iotlb_miss:
    700871,700871: Fail to update device iotlb

Fail device start when such an error occurs.

Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
---
 hw/virtio/vhost.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Note:
 - Earlier this patch was submitted as part of a series to fix vhost device
   related issue. It remained unreviewed, because the other patch in that
   series did not fix the issue entirely.

 - The error fixed in this patch is fairly independent of the other issue.
   It can be reviewed as is, without waiting for the other patch in that
   series. Not sure when the other patch will be revised again.
   So sending this one alone, instead of holding it back.

* https://lore.kernel.org/qemu-devel/20240808095147.291626-2-ppandit@redhat.com/
* https://lore.kernel.org/qemu-devel/20240711131424.181615-3-ppandit@redhat.com/

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 06fc71746e..a70b7422b5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2151,7 +2151,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
          * vhost-kernel code requires for this.*/
         for (i = 0; i < hdev->nvqs; ++i) {
             struct vhost_virtqueue *vq = hdev->vqs + i;
-            vhost_device_iotlb_miss(hdev, vq->used_phys, true);
+            r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
+            if (r) {
+                VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");
+                goto fail_start;
+            }
         }
     }
     vhost_start_config_intr(hdev);
--
2.47.0
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Stefano Garzarella 2 weeks, 4 days ago 
On Tue, Nov 05, 2024 at 11:30:53AM +0530, Prasad Pandit wrote:
>From: Prasad Pandit <pjp@fedoraproject.org>
>
>While starting a vhost device, updating iotlb entries
>via 'vhost_device_iotlb_miss' may return an error.
>
>  qemu-kvm: vhost_device_iotlb_miss:
>    700871,700871: Fail to update device iotlb
>
>Fail device start when such an error occurs.
>
>Signed-off-by: Prasad Pandit <pjp@fedoraproject.org>
>---
> hw/virtio/vhost.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
>Note:
> - Earlier this patch was submitted as part of a series to fix vhost device
>   related issue. It remained unreviewed, because the other patch in that
>   series did not fix the issue entirely.
>
> - The error fixed in this patch is fairly independent of the other issue.
>   It can be reviewed as is, without waiting for the other patch in that
>   series. Not sure when the other patch will be revised again.
>   So sending this one alone, instead of holding it back.
>
>* https://lore.kernel.org/qemu-devel/20240808095147.291626-2-ppandit@redhat.com/
>* https://lore.kernel.org/qemu-devel/20240711131424.181615-3-ppandit@redhat.com/
>
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index 06fc71746e..a70b7422b5 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -2151,7 +2151,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>          * vhost-kernel code requires for this.*/
>         for (i = 0; i < hdev->nvqs; ++i) {
>             struct vhost_virtqueue *vq = hdev->vqs + i;
>-            vhost_device_iotlb_miss(hdev, vq->used_phys, true);
>+            r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
>+            if (r) {
>+                VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");

VHOST_OPS_DEBUG() is usually used in the error path when calling a
`dev->vhost_ops` callback. In addition vhost_device_iotlb_miss() is
already reporting error through error_report() in the error path, so I
think we can remove this line.

>+                goto fail_start;

Should we add a new label in the error path and call
`hdev->vhost_ops->vhost_dev_start` with `false`?

Maybe we need to call also `hdev->vhost_ops->vhost_set_iotlb_callback`
with `false`.

Thanks,
Stefano

>+            }
>         }
>     }
>     vhost_start_config_intr(hdev);
>--
>2.47.0
>
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Prasad Pandit 2 weeks, 3 days ago 
On Tue, 5 Nov 2024 at 16:19, Stefano Garzarella <sgarzare@redhat.com> wrote:
> VHOST_OPS_DEBUG() is usually used in the error path when calling a
> `dev->vhost_ops` callback. In addition vhost_device_iotlb_miss() is
> already reporting error through error_report() in the error path, so I
> think we can remove this line.

* Okay.

> Should we add a new label in the error path and call
> `hdev->vhost_ops->vhost_dev_start` with `false`?
>
> Maybe we need to call also `hdev->vhost_ops->vhost_set_iotlb_callback`
> with `false`.
===
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a70b7422b5..089eff438e 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2137,15 +2137,18 @@ int vhost_dev_start(struct vhost_dev *hdev,
VirtIODevice *vdev, bool vrings)
             goto fail_log;
         }
     }
+
+    bool start = true;
+dev_restart:
     if (hdev->vhost_ops->vhost_dev_start) {
-        r = hdev->vhost_ops->vhost_dev_start(hdev, true);
+        r = hdev->vhost_ops->vhost_dev_start(hdev, start);
         if (r) {
             goto fail_start;
         }
     }
     if (vhost_dev_has_iommu(hdev) &&
         hdev->vhost_ops->vhost_set_iotlb_callback) {
-            hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true);
+            hdev->vhost_ops->vhost_set_iotlb_callback(hdev, start);

         /* Update used ring information for IOTLB to work correctly,
          * vhost-kernel code requires for this.*/
@@ -2154,7 +2157,8 @@ int vhost_dev_start(struct vhost_dev *hdev,
VirtIODevice *vdev, bool vrings)
             r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
             if (r) {
                 VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");
-                goto fail_start;
+                start = false;
+                goto dev_restart;
             }
         }
     }
===

* Something like above?

===
static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
{
    ...
    if (started) {
        return vhost_user_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                          VIRTIO_CONFIG_S_DRIVER |
                                          VIRTIO_CONFIG_S_DRIVER_OK);
    } else {
        return 0;
    }
}

static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
{
    /* No-op as the receive channel is not dedicated to IOTLB messages. */
}
===

* Calling vhost_user_dev_start() and vhost_user_set_iotlb_callback()
with 'false' does not seem to do much. Not sure how'll that help. If
we 'goto fail_start;', libvirtd(8) might restart the guest and thus
start the vhost device again.

...wdyt?

Thank you.
---
  - Prasad
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Stefano Garzarella 2 weeks, 3 days ago 
On Wed, Nov 06, 2024 at 01:20:31PM +0530, Prasad Pandit wrote:
>On Tue, 5 Nov 2024 at 16:19, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> VHOST_OPS_DEBUG() is usually used in the error path when calling a
>> `dev->vhost_ops` callback. In addition vhost_device_iotlb_miss() is
>> already reporting error through error_report() in the error path, so I
>> think we can remove this line.
>
>* Okay.
>
>> Should we add a new label in the error path and call
>> `hdev->vhost_ops->vhost_dev_start` with `false`?
>>
>> Maybe we need to call also `hdev->vhost_ops->vhost_set_iotlb_callback`
>> with `false`.
>===
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index a70b7422b5..089eff438e 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -2137,15 +2137,18 @@ int vhost_dev_start(struct vhost_dev *hdev,
>VirtIODevice *vdev, bool vrings)
>             goto fail_log;
>         }
>     }
>+
>+    bool start = true;
>+dev_restart:
>     if (hdev->vhost_ops->vhost_dev_start) {
>-        r = hdev->vhost_ops->vhost_dev_start(hdev, true);
>+        r = hdev->vhost_ops->vhost_dev_start(hdev, start);
>         if (r) {
>             goto fail_start;
>         }
>     }
>     if (vhost_dev_has_iommu(hdev) &&
>         hdev->vhost_ops->vhost_set_iotlb_callback) {
>-            hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true);
>+            hdev->vhost_ops->vhost_set_iotlb_callback(hdev, start);
>
>         /* Update used ring information for IOTLB to work correctly,
>          * vhost-kernel code requires for this.*/
>@@ -2154,7 +2157,8 @@ int vhost_dev_start(struct vhost_dev *hdev,
>VirtIODevice *vdev, bool vrings)
>             r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
>             if (r) {
>                 VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");
>-                goto fail_start;
>+                start = false;
>+                goto dev_restart;
>             }
>         }
>     }
>===
>
>* Something like above?

mmm, I would avoid goto to go backwards, since we could generate 
infinite loops, plus I think we should call that functions in the 
reverse order, so just add them in the error path, as we already do for 
other calls.

Another option is somehow call vhost_dev_stop() and just do the steps we 
need to, but that seems more complicated to me.

Thanks,
Stefano

>
>===
>static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>{
>    ...
>    if (started) {
>        return vhost_user_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                          VIRTIO_CONFIG_S_DRIVER |
>                                          VIRTIO_CONFIG_S_DRIVER_OK);
>    } else {
>        return 0;
>    }
>}
>
>static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
>{
>    /* No-op as the receive channel is not dedicated to IOTLB messages. */
>}
>===
>
>* Calling vhost_user_dev_start() and vhost_user_set_iotlb_callback()
>with 'false' does not seem to do much. Not sure how'll that help. If
>we 'goto fail_start;', libvirtd(8) might restart the guest and thus
>start the vhost device again.
>
>...wdyt?
>
>Thank you.
>---
>  - Prasad
>
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Prasad Pandit 2 weeks, 3 days ago 
On Wed, 6 Nov 2024 at 14:21, Stefano Garzarella <sgarzare@redhat.com> wrote:
> I think we should call that functions in the reverse order, so just add them in the error path, as we already do for other calls.
===
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index a70b7422b5..f168e1f02a 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2153,14 +2153,16 @@ int vhost_dev_start(struct vhost_dev *hdev,
VirtIODevice *vdev, bool vrings)
             struct vhost_virtqueue *vq = hdev->vqs + i;
             r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
             if (r) {
-                VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");
-                goto fail_start;
+                goto fail_iotlb;
             }
         }
     }
     vhost_start_config_intr(hdev);
     return 0;
+fail_iotlb:
+    hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
 fail_start:
+    hdev->vhost_ops->vhost_dev_start(hdev, false);
     if (vrings) {
         vhost_dev_set_vring_enable(hdev, false);
     }
===

* Is this okay?

* Looking at the vhost_vdpa_dev_start(), when it is called with
'started=false' parameter, it calls the vdpa_suspend, vdpa_stop etc.
functions. ie. probably other ->vhost_dev_start() and
->vhost_set_iotlb_callback() functions need to take appropriate action
when called with 'started/enabled=false' parameter.

Thank you.
---
  - Prasad
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Stefano Garzarella 2 weeks, 3 days ago 
On Wed, Nov 06, 2024 at 03:41:24PM +0530, Prasad Pandit wrote:
>On Wed, 6 Nov 2024 at 14:21, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> I think we should call that functions in the reverse order, so just add them in the error path, as we already do for other calls.
>===
>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>index a70b7422b5..f168e1f02a 100644
>--- a/hw/virtio/vhost.c
>+++ b/hw/virtio/vhost.c
>@@ -2153,14 +2153,16 @@ int vhost_dev_start(struct vhost_dev *hdev,
>VirtIODevice *vdev, bool vrings)
>             struct vhost_virtqueue *vq = hdev->vqs + i;
>             r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
>             if (r) {
>-                VHOST_OPS_DEBUG(r, "vhost_device_iotlb_miss failed");
>-                goto fail_start;
>+                goto fail_iotlb;
>             }
>         }
>     }
>     vhost_start_config_intr(hdev);
>     return 0;
>+fail_iotlb:
>+    hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
> fail_start:
>+    hdev->vhost_ops->vhost_dev_start(hdev, false);

This should go before the fail_start label, since it should not be 
called when vhost_dev_start() fails.

Also we need to check if that callback is defined.

I suggest to follow what we do in vhost_dev_stop(), so something like 
this (not tested):

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 76f9b2aaad..c40f48ac4d 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -2095,11 +2095,22 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
           * vhost-kernel code requires for this.*/
          for (i = 0; i < hdev->nvqs; ++i) {
              struct vhost_virtqueue *vq = hdev->vqs + i;
-            vhost_device_iotlb_miss(hdev, vq->used_phys, true);
+            r = vhost_device_iotlb_miss(hdev, vq->used_phys, true);
+            if (r) {
+                goto fail_iotlb;
+            }
          }
      }
      vhost_start_config_intr(hdev);
      return 0;
+fail_iotlb:
+    if (vhost_dev_has_iommu(hdev) &&
+        hdev->vhost_ops->vhost_set_iotlb_callback) {
+        hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
+    }
+    if (hdev->vhost_ops->vhost_dev_start) {
+        hdev->vhost_ops->vhost_dev_start(hdev, false);
+    }
  fail_start:
      if (vrings) {
          vhost_dev_set_vring_enable(hdev, false);

>     if (vrings) {
>         vhost_dev_set_vring_enable(hdev, false);
>     }
>===
>
>* Is this okay?
>
>* Looking at the vhost_vdpa_dev_start(), when it is called with
>'started=false' parameter, it calls the vdpa_suspend, vdpa_stop etc.
>functions. ie. probably other ->vhost_dev_start() and
>->vhost_set_iotlb_callback() functions need to take appropriate action
>when called with 'started/enabled=false' parameter.

We already call them in vhost_dev_stop(), so I guess we are fine.

@Michael @Jason @Eugenio WDYT?

Thanks,
Stefano
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Prasad Pandit 2 weeks, 3 days ago 
On Wed, 6 Nov 2024 at 16:05, Stefano Garzarella <sgarzare@redhat.com> wrote:
> >+fail_iotlb:
> >+    hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
> > fail_start:
> >+    hdev->vhost_ops->vhost_dev_start(hdev, false);
>
> This should go before the fail_start label, since it should not be
> called when vhost_dev_start() fails.

* I see, okay.

> Also we need to check if that callback is defined.

* That check is already done while reaching the 'goto fail_iotlb;'
statement, no? OR maybe we only check for:
hdev->vhost_ops->vhost_dev_start() function?

> >* Looking at the vhost_vdpa_dev_start(), when it is called with
> >'started=false' parameter, it calls the vdpa_suspend, vdpa_stop etc.
> >functions. ie. probably other ->vhost_dev_start() and
> >->vhost_set_iotlb_callback() functions need to take appropriate action
> >when called with 'started/enabled=false' parameter.
>
> We already call them in vhost_dev_stop(), so I guess we are fine.

* I meant vhost device functions like vhost_user_dev_start() and
vhost_user_set_iotlb_callback() need to take action when called with a
'false' parameter.

Thank you.
---
  - Prasad
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Stefano Garzarella 2 weeks, 3 days ago 
On Wed, Nov 06, 2024 at 05:14:24PM +0530, Prasad Pandit wrote:
>On Wed, 6 Nov 2024 at 16:05, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> >+fail_iotlb:
>> >+    hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
>> > fail_start:
>> >+    hdev->vhost_ops->vhost_dev_start(hdev, false);
>>
>> This should go before the fail_start label, since it should not be
>> called when vhost_dev_start() fails.
>
>* I see, okay.
>
>> Also we need to check if that callback is defined.
>
>* That check is already done while reaching the 'goto fail_iotlb;'
>statement, no? OR maybe we only check for:
>hdev->vhost_ops->vhost_dev_start() function?

For vhost_set_iotlb_callback() that is true because for now we go to 
that label only if the callback is defined, but this is not the case for 
hdev->vhost_ops->vhost_dev_start().

Anyway if in the future we add a new step that need to go on that label 
we may forgot to remember that, so since it's not a hot path, I'd add 
both checks as we do in vhost_dev_stop().

>
>> >* Looking at the vhost_vdpa_dev_start(), when it is called with
>> >'started=false' parameter, it calls the vdpa_suspend, vdpa_stop etc.
>> >functions. ie. probably other ->vhost_dev_start() and
>> >->vhost_set_iotlb_callback() functions need to take appropriate action
>> >when called with 'started/enabled=false' parameter.
>>
>> We already call them in vhost_dev_stop(), so I guess we are fine.
>
>* I meant vhost device functions like vhost_user_dev_start() and
>vhost_user_set_iotlb_callback() need to take action when called with a
>'false' parameter.

IIUC in vhost_dev_stop() we already call both of them with a 'false' 
parameter, so that functions should be already prepared or am I missing 
something?

Thanks,
Stefano
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Prasad Pandit 2 weeks, 3 days ago 
On Wed, 6 Nov 2024 at 17:31, Stefano Garzarella <sgarzare@redhat.com> wrote:
> For vhost_set_iotlb_callback() that is true because for now we go to
> that label only if the callback is defined, but this is not the case for
> hdev->vhost_ops->vhost_dev_start().
>
> Anyway if in the future we add a new step that need to go on that label
> we may forgot to remember that, so since it's not a hot path, I'd add
> both checks as we do in vhost_dev_stop().

* Okay.

> IIUC in vhost_dev_stop() we already call both of them with a 'false'
> parameter, so that functions should be already prepared or am I missing
> something?
===
static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
{
    ...
    if (started) {
        return vhost_user_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
                                          VIRTIO_CONFIG_S_DRIVER |
                                          VIRTIO_CONFIG_S_DRIVER_OK);
    } else {
        return 0;
    }
}
static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
{
    /* No-op as the receive channel is not dedicated to IOTLB messages. */
}
===

* vhost_user_dev_start and vhost_user_set_iotlb_callback() don't seem
to do much when called with 'false' parameter.

Thank you.
---
  - Prasad
Re: [PATCH] vhost: fail device start if iotlb update fails
Posted by Stefano Garzarella 2 weeks, 3 days ago 
On Wed, Nov 06, 2024 at 06:04:59PM +0530, Prasad Pandit wrote:
>On Wed, 6 Nov 2024 at 17:31, Stefano Garzarella <sgarzare@redhat.com> wrote:
>> For vhost_set_iotlb_callback() that is true because for now we go to
>> that label only if the callback is defined, but this is not the case for
>> hdev->vhost_ops->vhost_dev_start().
>>
>> Anyway if in the future we add a new step that need to go on that label
>> we may forgot to remember that, so since it's not a hot path, I'd add
>> both checks as we do in vhost_dev_stop().
>
>* Okay.
>
>> IIUC in vhost_dev_stop() we already call both of them with a 'false'
>> parameter, so that functions should be already prepared or am I missing
>> something?
>===
>static int vhost_user_dev_start(struct vhost_dev *dev, bool started)
>{
>    ...
>    if (started) {
>        return vhost_user_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
>                                          VIRTIO_CONFIG_S_DRIVER |
>                                          VIRTIO_CONFIG_S_DRIVER_OK);
>    } else {
>        return 0;
>    }
>}
>static void vhost_user_set_iotlb_callback(struct vhost_dev *dev, int enabled)
>{
>    /* No-op as the receive channel is not dedicated to IOTLB messages. */
>}
>===
>
>* vhost_user_dev_start and vhost_user_set_iotlb_callback() don't seem
>to do much when called with 'false' parameter.

I think we should be agnostic here about what a specific backend does.
For example VHOST_BACKEND_TYPE_KERNEL doesn't even define that callback, 
but IMHO we have to call it anyway here, like we already do in 
vhost_dev_stop().

If we miss something in that callbacks, when they are called with 
`false`, then it's another problem that we need to fix regardless of 
this patch.

Thanks,
Stefano

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.