On Wed, Nov 06, 2024 at 06:04:59PM +0530, Prasad Pandit wrote:
On Wed, 6 Nov 2024 at 17:31, Stefano Garzarella <sgarz...@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
