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

[Stefano Garzarella]

On Wed, Nov 06, 2024 at 03:41:24PM +0530, Prasad Pandit wrote:
On Wed, 6 Nov 2024 at 14:21, Stefano Garzarella <sgarz...@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