Re: [Qemu-devel] [PATCH v2 05/12] Add vhost-user-backend

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Feb 07, 2019 at 05:54:42PM +0100, Marc-André Lureau wrote:
> Create a vhost-user-backend object that holds a connection to a
> vhost-user backend and can be referenced from virtio devices that
> support it. See later patches for input & gpu usage.
> 
> A chardev must be specified to communicate with the vhost-user
> backend, ex: -chardev socket,id=char0,path=/tmp/foo.sock -object
> vhost-user-backend,id=vuid,chardev=char0.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

So having an internal object that can maintain runtime state
might be a good idea. However I don't yet really see
what kind of property could such an object have that
the char device couldn't.

I could see value if user did not have to specify
a completely separate device.

Consider:

-chardev socket,id=char0,path=/tmp/foo.sock 
-object vhost-user-backend,id=vuid,chardev=char0
-device vhost-user-input-pci,vhost-user=vuid


There's 3 times vhost-user here, and nothing actually says it's
virtio-input, that is implicit :(

So I feel CLI needs to change.  But I do think the idea of
an object in between does have some potential.

Consider virtio net which now has modern, legacy and transitional
variants. How is vhost-user-X type going to scale there?
That's a problem worth solving IMHO.

Also, there's a problem right now in that if backend
connects before device is available (e.g. because we
want to hotplug the device later) then we can not
validate the backend. So it will fail way later.
I am not sure how much do we want to validate,
but if e.g. it's a different device type completely,
that seems like a reasonable thing to validate.

So I do see potential in a vhost user backend object
but then it has to encapsulate all vhost user things,
such that you can connect a virtio device to a
vhost user object.





> ---
>  include/sysemu/vhost-user-backend.h |  60 +++++++
>  backends/vhost-user.c               | 244 ++++++++++++++++++++++++++++
>  vl.c                                |   3 +-
>  MAINTAINERS                         |   2 +
>  backends/Makefile.objs              |   3 +-
>  qemu-options.hx                     |  20 +++
>  6 files changed, 330 insertions(+), 2 deletions(-)
>  create mode 100644 include/sysemu/vhost-user-backend.h
>  create mode 100644 backends/vhost-user.c
> 
> diff --git a/include/sysemu/vhost-user-backend.h b/include/sysemu/vhost-user-backend.h
> new file mode 100644
> index 0000000000..60f811cae7
> --- /dev/null
> +++ b/include/sysemu/vhost-user-backend.h
> @@ -0,0 +1,60 @@
> +/*
> + * QEMU vhost-user backend
> + *
> + * Copyright (C) 2018 Red Hat Inc
> + *
> + * Authors:
> + *  Marc-André Lureau <marcandre.lureau@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#ifndef QEMU_VHOST_USER_BACKEND_H
> +#define QEMU_VHOST_USER_BACKEND_H
> +
> +#include "qom/object.h"
> +#include "exec/memory.h"
> +#include "qemu/option.h"
> +#include "qemu/bitmap.h"
> +#include "hw/virtio/vhost.h"
> +#include "hw/virtio/vhost-user.h"
> +#include "chardev/char-fe.h"
> +#include "io/channel.h"
> +
> +#define TYPE_VHOST_USER_BACKEND "vhost-user-backend"
> +#define VHOST_USER_BACKEND(obj) \
> +    OBJECT_CHECK(VhostUserBackend, (obj), TYPE_VHOST_USER_BACKEND)
> +#define VHOST_USER_BACKEND_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(VhostUserBackendClass, (obj), TYPE_VHOST_USER_BACKEND)
> +#define VHOST_USER_BACKEND_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(VhostUserBackendClass, (klass), TYPE_VHOST_USER_BACKEND)
> +
> +typedef struct VhostUserBackend VhostUserBackend;
> +typedef struct VhostUserBackendClass VhostUserBackendClass;
> +
> +struct VhostUserBackendClass {
> +    ObjectClass parent_class;
> +};
> +
> +struct VhostUserBackend {
> +    /* private */
> +    Object parent;
> +
> +    char *cmd;
> +    char *chr_name;
> +
> +    CharBackend chr;
> +    VhostUserState vhost_user;
> +    struct vhost_dev dev;
> +    QIOChannel *child;
> +    VirtIODevice *vdev;
> +    bool started;
> +    bool completed;
> +};
> +
> +int vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
> +                                unsigned nvqs, Error **errp);
> +void vhost_user_backend_start(VhostUserBackend *b);
> +void vhost_user_backend_stop(VhostUserBackend *b);
> +
> +#endif
> diff --git a/backends/vhost-user.c b/backends/vhost-user.c
> new file mode 100644
> index 0000000000..bf39c0751d
> --- /dev/null
> +++ b/backends/vhost-user.c
> @@ -0,0 +1,244 @@
> +/*
> + * QEMU vhost-user backend
> + *
> + * Copyright (C) 2018 Red Hat Inc
> + *
> + * Authors:
> + *  Marc-André Lureau <marcandre.lureau@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +
> +#include "qemu/osdep.h"
> +#include "hw/qdev.h"
> +#include "qapi/error.h"
> +#include "qapi/qmp/qerror.h"
> +#include "qemu/error-report.h"
> +#include "qom/object_interfaces.h"
> +#include "sysemu/vhost-user-backend.h"
> +#include "sysemu/kvm.h"
> +#include "io/channel-command.h"
> +#include "hw/virtio/virtio-bus.h"
> +
> +static bool
> +ioeventfd_enabled(void)
> +{
> +    return kvm_enabled() && kvm_eventfds_enabled();
> +}
> +
> +int
> +vhost_user_backend_dev_init(VhostUserBackend *b, VirtIODevice *vdev,
> +                            unsigned nvqs, Error **errp)
> +{
> +    int ret;
> +
> +    assert(!b->vdev && vdev);
> +
> +    if (!ioeventfd_enabled()) {
> +        error_setg(errp, "vhost initialization failed: requires kvm");
> +        return -1;
> +    }
> +
> +    if (!vhost_user_init(&b->vhost_user, &b->chr, errp)) {
> +        return -1;
> +    }
> +
> +    b->vdev = vdev;
> +    b->dev.nvqs = nvqs;
> +    b->dev.vqs = g_new(struct vhost_virtqueue, nvqs);
> +
> +    ret = vhost_dev_init(&b->dev, &b->vhost_user, VHOST_BACKEND_TYPE_USER, 0);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "vhost initialization failed");
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +void
> +vhost_user_backend_start(VhostUserBackend *b)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    int ret, i ;
> +
> +    if (b->started) {
> +        return;
> +    }
> +
> +    if (!k->set_guest_notifiers) {
> +        error_report("binding does not support guest notifiers");
> +        return;
> +    }
> +
> +    ret = vhost_dev_enable_notifiers(&b->dev, b->vdev);
> +    if (ret < 0) {
> +        return;
> +    }
> +
> +    ret = k->set_guest_notifiers(qbus->parent, b->dev.nvqs, true);
> +    if (ret < 0) {
> +        error_report("Error binding guest notifier");
> +        goto err_host_notifiers;
> +    }
> +
> +    b->dev.acked_features = b->vdev->guest_features;
> +    ret = vhost_dev_start(&b->dev, b->vdev);
> +    if (ret < 0) {
> +        error_report("Error start vhost dev");
> +        goto err_guest_notifiers;
> +    }
> +
> +    /* guest_notifier_mask/pending not used yet, so just unmask
> +     * everything here.  virtio-pci will do the right thing by
> +     * enabling/disabling irqfd.
> +     */
> +    for (i = 0; i < b->dev.nvqs; i++) {
> +        vhost_virtqueue_mask(&b->dev, b->vdev,
> +                             b->dev.vq_index + i, false);
> +    }
> +
> +    b->started = true;
> +    return;
> +
> +err_guest_notifiers:
> +    k->set_guest_notifiers(qbus->parent, b->dev.nvqs, false);
> +err_host_notifiers:
> +    vhost_dev_disable_notifiers(&b->dev, b->vdev);
> +}
> +
> +void
> +vhost_user_backend_stop(VhostUserBackend *b)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(b->vdev)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    int ret = 0;
> +
> +    if (!b->started) {
> +        return;
> +    }
> +
> +    vhost_dev_stop(&b->dev, b->vdev);
> +
> +    if (k->set_guest_notifiers) {
> +        ret = k->set_guest_notifiers(qbus->parent,
> +                                     b->dev.nvqs, false);
> +        if (ret < 0) {
> +            error_report("vhost guest notifier cleanup failed: %d", ret);
> +        }
> +    }
> +    assert(ret >= 0);
> +
> +    vhost_dev_disable_notifiers(&b->dev, b->vdev);
> +    b->started = false;
> +}
> +
> +static void
> +vhost_user_backend_complete(UserCreatable *uc, Error **errp)
> +{
> +    VhostUserBackend *b = VHOST_USER_BACKEND(uc);
> +    Chardev *chr;
> +
> +    if (!b->chr_name) {
> +        error_setg(errp, "You must specificy 'chardev'.");
> +        return;
> +    }
> +
> +    chr = qemu_chr_find(b->chr_name);
> +    if (chr == NULL) {
> +        error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
> +                  "Chardev '%s' not found", b->chr_name);
> +        return;
> +    }
> +
> +    if (!qemu_chr_fe_init(&b->chr, chr, errp)) {
> +        return;
> +    }
> +
> +    b->completed = true;
> +    /* could vhost_dev_init() happen here, so early vhost-user message
> +     * can be exchanged */
> +}
> +
> +static void set_chardev(Object *obj, const char *value, Error **errp)
> +{
> +    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
> +
> +    if (b->completed) {
> +        error_setg(errp, QERR_PERMISSION_DENIED);
> +    } else {
> +        g_free(b->chr_name);
> +        b->chr_name = g_strdup(value);
> +    }
> +}
> +
> +static char *get_chardev(Object *obj, Error **errp)
> +{
> +    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
> +    Chardev *chr = qemu_chr_fe_get_driver(&b->chr);
> +
> +    if (chr && chr->label) {
> +        return g_strdup(chr->label);
> +    }
> +
> +    return NULL;
> +}
> +
> +static void vhost_user_backend_init(Object *obj)
> +{
> +    object_property_add_str(obj, "chardev", get_chardev, set_chardev, NULL);
> +}
> +
> +static void vhost_user_backend_finalize(Object *obj)
> +{
> +    VhostUserBackend *b = VHOST_USER_BACKEND(obj);
> +
> +    g_free(b->dev.vqs);
> +    g_free(b->chr_name);
> +
> +    vhost_user_cleanup(&b->vhost_user);
> +    qemu_chr_fe_deinit(&b->chr, true);
> +
> +    if (b->child) {
> +        object_unref(OBJECT(b->child));
> +    }
> +}
> +
> +static bool
> +vhost_user_backend_can_be_deleted(UserCreatable *uc)
> +{
> +    return true;
> +}
> +
> +static void
> +vhost_user_backend_class_init(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> +    ucc->complete = vhost_user_backend_complete;
> +    ucc->can_be_deleted = vhost_user_backend_can_be_deleted;
> +}
> +
> +static const TypeInfo vhost_user_backend_info = {
> +    .name = TYPE_VHOST_USER_BACKEND,
> +    .parent = TYPE_OBJECT,
> +    .instance_size = sizeof(VhostUserBackend),
> +    .instance_init = vhost_user_backend_init,
> +    .instance_finalize = vhost_user_backend_finalize,
> +    .class_size = sizeof(VhostUserBackendClass),
> +    .class_init = vhost_user_backend_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void register_types(void)
> +{
> +    type_register_static(&vhost_user_backend_info);
> +}
> +
> +type_init(register_types);
> diff --git a/vl.c b/vl.c
> index 9e4dba7f92..43012ee6a3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2784,7 +2784,8 @@ static bool object_create_initial(const char *type, QemuOpts *opts)
>      }
>  
>  #if defined(CONFIG_VHOST_USER) && defined(CONFIG_LINUX)
> -    if (g_str_equal(type, "cryptodev-vhost-user")) {
> +    if (g_str_equal(type, "cryptodev-vhost-user") ||
> +        g_str_equal(type, "vhost-user-backend")) {
>          return false;
>      }
>  #endif
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 16b6264412..e077fe788d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1424,6 +1424,8 @@ F: hw/*/*vhost*
>  F: docs/interop/vhost-user.json
>  F: docs/interop/vhost-user.txt
>  F: contrib/vhost-user-*/
> +F: backends/vhost-user.c
> +F: include/sysemu/vhost-user-backend.h
>  
>  virtio
>  M: Michael S. Tsirkin <mst@redhat.com>
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 717fcbdae4..a5ec0bf907 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -12,7 +12,8 @@ common-obj-y += cryptodev-builtin.o
>  ifeq ($(CONFIG_VIRTIO),y)
>  common-obj-y += cryptodev-vhost.o
>  common-obj-$(call land,$(CONFIG_VHOST_USER),$(CONFIG_LINUX)) += \
> -    cryptodev-vhost-user.o
> +    cryptodev-vhost-user.o \
> +    vhost-user.o
>  endif
>  
>  common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 06ef1a7c5c..24315a4cda 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4203,6 +4203,26 @@ secondary:
>  If you want to know the detail of above command line, you can read
>  the colo-compare git log.
>  
> +@item -object vhost-user-backend,id=id=@var{id},chardev=@var{chardevid}
> +
> +Create a vhost-user-backend object that holds a connection to a
> +vhost-user backend and can be referenced from virtio/vhost-user
> +devices that support it.
> +
> +The @var{id} parameter is a unique ID that will be used to reference
> +this vhost-user backend from the @option{vhost-user} device. The
> +@var{chardev} parameter is the unique ID of a character device backend
> +that provides the connection to the vhost-user slave process. (Since 3.2)
> +
> +@example
> +
> + # qemu-system-x86_64 \
> +   [...] \
> +   -object vhost-user-backend,id=vuid,chardev=char0 \
> +   -device vhost-user-input-pci,vhost-user=vuid
> +   [...]
> +@end example
> +
>  @item -object cryptodev-backend-builtin,id=@var{id}[,queues=@var{queues}]
>  
>  Creates a cryptodev backend which executes crypto opreation from
> -- 
> 2.20.1.519.g8feddda32c