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

[Stefano Garzarella]

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