On Wed, Nov 06, 2024 at 01:20:31PM +0530, Prasad Pandit wrote:
On Tue, 5 Nov 2024 at 16:19, Stefano Garzarella <sgarz...@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
