diff mbox series

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

Message ID 20230710153522.3469097-21-alex.bennee@linaro.org
State New
Headers show
Series virtio: add vhost-user-generic, reduce c&p and support standalone | expand

Commit Message

Alex Bennée July 10, 2023, 3:35 p.m. UTC 
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(-)

Comments

Albert Esteve Sept. 1, 2023, 10 a.m. UTC | #1 
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
>
>
>
diff mbox series

Patch

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;
-
-    /* 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);
 }