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

[Stefano Garzarella]

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