Re: [RFC PATCH v3 20/20] hw/virtio: allow vhost-user-device to be driven by backend

[Albert Esteve <aesteve@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4255 bytes --]

On Mon, Jul 10, 2023 at 6:44 PM Alex Bennée <alex.bennee@linaro.org> wrote:

> Instead of requiring all the information up front allow the
> vhost_dev_init to complete and then see what information we have from
> the backend.
>
> This does change the order around somewhat.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  hw/virtio/vhost-user-device.c | 45 +++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c
> index 0109d4829d..b30b6265fb 100644
> --- a/hw/virtio/vhost-user-device.c
> +++ b/hw/virtio/vhost-user-device.c
> @@ -243,7 +243,6 @@ static void vub_device_realize(DeviceState *dev, Error
> **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VHostUserBase *vub = VHOST_USER_BASE(dev);
> -    int ret;
>
>      if (!vub->chardev.chr) {
>          error_setg(errp, "vhost-user-device: missing chardev");
> @@ -254,13 +253,43 @@ static void vub_device_realize(DeviceState *dev,
> Error **errp)
>          return;
>      }
>
> +    if (vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,
> +                       VHOST_BACKEND_TYPE_USER, 0, errp)!=0) {
> +        error_setg(errp, "vhost-user-device: unable to start connection");
> +        return;
> +    }
> +
> +    if (vub->vhost_dev.specs.device_id) {
> +        if (vub->virtio_id && vub->virtio_id !=
> vub->vhost_dev.specs.device_id) {
> +            error_setg(errp, "vhost-user-device: backend id %d doesn't
> match cli %d",
> +                       vub->vhost_dev.specs.device_id, vub->virtio_id);
> +            return;
> +        }
> +        vub->virtio_id = vub->vhost_dev.specs.device_id;
> +    }
> +
>      if (!vub->virtio_id) {
> -        error_setg(errp, "vhost-user-device: need to define device id");
> +        error_setg(errp, "vhost-user-device: need to define or be told
> device id");
>          return;
>      }
>
> +    if (vub->vhost_dev.specs.min_vqs) {
> +        if (vub->num_vqs) {
> +            if (vub->num_vqs < vub->vhost_dev.specs.min_vqs ||
> +                vub->num_vqs > vub->vhost_dev.specs.max_vqs) {
> +                error_setg(errp,
> +                           "vhost-user-device: selected nvqs (%d) out of
> bounds (%d->%d)",
> +                           vub->num_vqs,
> +                           vub->vhost_dev.specs.min_vqs,
> vub->vhost_dev.specs.max_vqs);
> +                return;
> +            }
> +        } else {
> +            vub->num_vqs = vub->vhost_dev.specs.min_vqs;
> +        }
> +    }
> +
>      if (!vub->num_vqs) {
> -        vub->num_vqs = 1; /* reasonable default? */
> +        error_setg(errp, "vhost-user-device: need to define number of
> vqs");
>      }
>
>      /*
> @@ -287,16 +316,6 @@ static void vub_device_realize(DeviceState *dev,
> Error **errp)
>                          virtio_add_queue(vdev, 4, vub_handle_output));
>      }
>
> -    vub->vhost_dev.nvqs = vub->num_vqs;
>

Who sets `vub->vhost_dev.nvqs` after removing this line?
Why having `vub->num_vqs` in the first place? In vub_start for example we
still
use `vub->vhost_dev.nvqs`, and we pass `vhost_dev` to other functions that
use its `nvqs`, so `num_vqs` is redundant and requires a logic to
copy/initialise `vhost_dev.nvqs`.

Maybe it would be better to initialse `nvqs` through a function, in the
device file, instead of doing:
`vub->num_vqs = 2;`
We could have:
`vub_set_nvqs(vub, 2);`
Or something along those lines. And the function will have all the internal
logic in this commit, i.e.,
checking the boundaries, setting the `vhost_dev.nvqs` value, printing the
error, etc.
So we can save the extra variable, and the logic to copy the value to the
device.


> -
> -    /* connect to backend */
> -    ret = vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,
> -                         VHOST_BACKEND_TYPE_USER, 0, errp);
> -
> -    if (ret < 0) {
> -        do_vhost_user_cleanup(vdev, vub);
> -    }
> -
>      qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
>                               dev, NULL, true);
>  }
> --
> 2.39.2
>
>
>

[-- Attachment #2: Type: text/html, Size: 5643 bytes --]