[PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Alex Bennée]

A lot of our vhost-user stubs are large chunks of boilerplate that do
(mostly) the same thing. This series attempts to fix that by defining
a new base class for vhost-user devices and then converting the rng
and gpio devices to be based off them. You can even use
vhost-user-device directly if you supply it with the right magic
numbers (which is helpful for development).

However the final patch runs into the weeds because I don't yet have a
clean way to represent in QOM the fixing of certain properties for the
specialised classes.

The series is a net reduction in code and an increase in
documentation but obviously needs to iron out a few more warts. I'm
open to suggestions on the best way to tweak the QOM stuff.

Alex.

Alex Bennée (12):
  hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments
  include/hw/virtio: document virtio_notify_config
  include/hw/virtio: add kerneldoc for virtio_init
  include/hw/virtio: document some more usage of notifiers
  virtio: add generic vhost-user-device
  virtio: add PCI stub for vhost-user-device
  include: attempt to document device_class_set_props
  qom: allow for properties to become "fixed"
  hw/virtio: derive vhost-user-rng from vhost-user-device
  hw/virtio: add config support to vhost-user-device
  hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN)
  docs/system: add a basic enumeration of vhost-user devices

 docs/system/devices/vhost-user-rng.rst |   2 +
 docs/system/devices/vhost-user.rst     |  41 +++
 qapi/qom.json                          |   2 +
 include/hw/qdev-core.h                 |   9 +
 include/hw/virtio/vhost-user-device.h  |  33 ++
 include/hw/virtio/vhost-user-gpio.h    |  23 +-
 include/hw/virtio/vhost-user-rng.h     |  11 +-
 include/hw/virtio/virtio.h             |  21 ++
 include/qom/object.h                   |  16 +-
 hw/display/vhost-user-gpu.c            |   4 +-
 hw/net/virtio-net.c                    |   4 +-
 hw/virtio/vhost-user-device-pci.c      |  71 +++++
 hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
 hw/virtio/vhost-user-fs.c              |   4 +-
 hw/virtio/vhost-user-gpio.c            | 405 +------------------------
 hw/virtio/vhost-user-rng.c             | 264 +---------------
 hw/virtio/vhost-vsock-common.c         |   4 +-
 hw/virtio/virtio-crypto.c              |   4 +-
 qom/object.c                           |  14 +
 qom/object_interfaces.c                |   9 +-
 qom/qom-qmp-cmds.c                     |   1 +
 softmmu/qdev-monitor.c                 |   1 +
 hw/virtio/meson.build                  |   3 +
 23 files changed, 613 insertions(+), 692 deletions(-)
 create mode 100644 include/hw/virtio/vhost-user-device.h
 create mode 100644 hw/virtio/vhost-user-device-pci.c
 create mode 100644 hw/virtio/vhost-user-device.c

-- 
2.39.2

[PATCH 01/12] hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments

[Alex Bennée]

Fixes: 544f0278af (virtio: introduce macro VIRTIO_CONFIG_IRQ_IDX)
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/display/vhost-user-gpu.c    | 4 ++--
 hw/net/virtio-net.c            | 4 ++--
 hw/virtio/vhost-user-fs.c      | 4 ++--
 hw/virtio/vhost-user-gpio.c    | 2 +-
 hw/virtio/vhost-vsock-common.c | 4 ++--
 hw/virtio/virtio-crypto.c      | 4 ++--
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c
index 71dfd956b8..7c61a7c3ac 100644
--- a/hw/display/vhost-user-gpu.c
+++ b/hw/display/vhost-user-gpu.c
@@ -489,7 +489,7 @@ vhost_user_gpu_guest_notifier_pending(VirtIODevice *vdev, int idx)
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
@@ -506,7 +506,7 @@ vhost_user_gpu_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 53e1c32643..c53616a080 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3359,7 +3359,7 @@ static bool virtio_net_guest_notifier_pending(VirtIODevice *vdev, int idx)
     }
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return false
      */
 
@@ -3391,7 +3391,7 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
     }
     /*
      *Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
diff --git a/hw/virtio/vhost-user-fs.c b/hw/virtio/vhost-user-fs.c
index 83fc20e49e..49d699ffc2 100644
--- a/hw/virtio/vhost-user-fs.c
+++ b/hw/virtio/vhost-user-fs.c
@@ -161,7 +161,7 @@ static void vuf_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
@@ -177,7 +177,7 @@ static bool vuf_guest_notifier_pending(VirtIODevice *vdev, int idx)
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index d6927b610a..3b013f2d0f 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -194,7 +194,7 @@ static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
diff --git a/hw/virtio/vhost-vsock-common.c b/hw/virtio/vhost-vsock-common.c
index d2b5519d5a..623bdf91cc 100644
--- a/hw/virtio/vhost-vsock-common.c
+++ b/hw/virtio/vhost-vsock-common.c
@@ -129,7 +129,7 @@ static void vhost_vsock_common_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
@@ -146,7 +146,7 @@ static bool vhost_vsock_common_guest_notifier_pending(VirtIODevice *vdev,
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
diff --git a/hw/virtio/virtio-crypto.c b/hw/virtio/virtio-crypto.c
index 802e1b9659..6b3e607329 100644
--- a/hw/virtio/virtio-crypto.c
+++ b/hw/virtio/virtio-crypto.c
@@ -1208,7 +1208,7 @@ static void virtio_crypto_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
@@ -1227,7 +1227,7 @@ static bool virtio_crypto_guest_notifier_pending(VirtIODevice *vdev, int idx)
 
     /*
      * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the Marco of configure interrupt's IDX, If this driver does not
+     * as the macro of configure interrupt's IDX, If this driver does not
      * support, the function will return
      */
 
-- 
2.39.2

[PATCH 02/12] include/hw/virtio: document virtio_notify_config

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index f236e94ca6..22ec098462 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -274,6 +274,13 @@ extern const VMStateInfo virtio_vmstate_info;
 
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id);
 
+/**
+ * virtio_notify_config() - signal a change to device config
+ * @vdev: the virtio device
+ *
+ * Assuming the virtio device is up (VIRTIO_CONFIG_S_DRIVER_OK) this
+ * will trigger a guest interrupt and update the config version.
+ */
 void virtio_notify_config(VirtIODevice *vdev);
 
 bool virtio_queue_get_notification(VirtQueue *vq);
-- 
2.39.2

[PATCH 03/12] include/hw/virtio: add kerneldoc for virtio_init

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 22ec098462..1ba7a9dd74 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -217,6 +217,12 @@ struct VirtioDeviceClass {
 void virtio_instance_init_common(Object *proxy_obj, void *data,
                                  size_t vdev_size, const char *vdev_name);
 
+/**
+ * virtio_init() - initialise the common VirtIODevice structure
+ * @vdev: pointer to VirtIODevice
+ * @device_id: the VirtIO device ID (see virtio_ids.h)
+ * @config_size: size of the config space
+ */
 void virtio_init(VirtIODevice *vdev, uint16_t device_id, size_t config_size);
 
 void virtio_cleanup(VirtIODevice *vdev);
-- 
2.39.2

[PATCH 04/12] include/hw/virtio: document some more usage of notifiers

[Alex Bennée]

Lets document some more of the core VirtIODevice structure.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/virtio.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 1ba7a9dd74..ef77e9ef0e 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -150,10 +150,18 @@ struct VirtIODevice
     VMChangeStateEntry *vmstate;
     char *bus_name;
     uint8_t device_endian;
+    /**
+     * @user_guest_notifier_mask: gate usage of ->guest_notifier_mask() callback.
+     * This is used to suppress the masking of guest updates for
+     * vhost-user devices which are asynchronous by design.
+     */
     bool use_guest_notifier_mask;
     AddressSpace *dma_as;
     QLIST_HEAD(, VirtQueue) *vector_queues;
     QTAILQ_ENTRY(VirtIODevice) next;
+    /**
+     * @config_notifier: the event notifier that handles config events
+     */
     EventNotifier config_notifier;
 };
 
-- 
2.39.2

[PATCH 05/12] virtio: add generic vhost-user-device

[Alex Bennée]

In theory we shouldn't need to repeat so much boilerplate to support
vhost-user backends. This provides a generic vhost-user-device for
which the user needs to provide the few bits of information that
aren't currently provided by the vhost-user protocol. This should
provide a baseline implementation from which the other vhost-user stub
can specialise.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user-device.h |  32 +++
 hw/virtio/vhost-user-device.c         | 303 ++++++++++++++++++++++++++
 hw/virtio/meson.build                 |   2 +
 3 files changed, 337 insertions(+)
 create mode 100644 include/hw/virtio/vhost-user-device.h
 create mode 100644 hw/virtio/vhost-user-device.c

diff --git a/include/hw/virtio/vhost-user-device.h b/include/hw/virtio/vhost-user-device.h
new file mode 100644
index 0000000000..8d77f06721
--- /dev/null
+++ b/include/hw/virtio/vhost-user-device.h
@@ -0,0 +1,32 @@
+/*
+ * Vhost-user generic virtio device
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#ifndef QEMU_VHOST_USER_DEVICE_H
+#define QEMU_VHOST_USER_DEVICE_H
+
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+
+#define TYPE_VHOST_USER_DEVICE "vhost-user-device"
+OBJECT_DECLARE_SIMPLE_TYPE(VHostUserDevice, VHOST_USER_DEVICE)
+
+struct VHostUserDevice {
+    VirtIODevice parent;
+    /* Properties */
+    CharBackend chardev;
+    uint16_t virtio_id;
+    uint32_t num_vqs;
+    /* State tracking */
+    VhostUserState vhost_user;
+    struct vhost_virtqueue *vhost_vq;
+    struct vhost_dev vhost_dev;
+    GPtrArray *vqs;
+    bool connected;
+};
+
+#endif /* QEMU_VHOST_USER_DEVICE_H */
diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c
new file mode 100644
index 0000000000..bfbf3b29cb
--- /dev/null
+++ b/hw/virtio/vhost-user-device.c
@@ -0,0 +1,303 @@
+/*
+ * Generic vhost-user stub. This can be used to connect to any
+ * vhost-user backend. All configuration details must be handled by
+ * the vhost-user daemon itself
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ * Author: Alex Bennée <alex.bennee@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/vhost-user-device.h"
+#include "qemu/error-report.h"
+
+static void vud_start(VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    int ret, i;
+
+    if (!k->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return;
+    }
+
+    ret = vhost_dev_enable_notifiers(&vud->vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error enabling host notifiers: %d", -ret);
+        return;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, vud->vhost_dev.nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier: %d", -ret);
+        goto err_host_notifiers;
+    }
+
+    vud->vhost_dev.acked_features = vdev->guest_features;
+
+    ret = vhost_dev_start(&vud->vhost_dev, vdev, true);
+    if (ret < 0) {
+        error_report("Error starting vhost-user-device: %d", -ret);
+        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 < vud->vhost_dev.nvqs; i++) {
+        vhost_virtqueue_mask(&vud->vhost_dev, vdev, i, false);
+    }
+
+    return;
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, vud->vhost_dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&vud->vhost_dev, vdev);
+}
+
+static void vud_stop(VirtIODevice *vdev)
+{
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    int ret;
+
+    if (!k->set_guest_notifiers) {
+        return;
+    }
+
+    vhost_dev_stop(&vud->vhost_dev, vdev, true);
+
+    ret = k->set_guest_notifiers(qbus->parent, vud->vhost_dev.nvqs, false);
+    if (ret < 0) {
+        error_report("vhost guest notifier cleanup failed: %d", ret);
+        return;
+    }
+
+    vhost_dev_disable_notifiers(&vud->vhost_dev, vdev);
+}
+
+static void vud_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    bool should_start = virtio_device_should_start(vdev, status);
+
+    if (vhost_dev_is_started(&vud->vhost_dev) == should_start) {
+        return;
+    }
+
+    if (should_start) {
+        vud_start(vdev);
+    } else {
+        vud_stop(vdev);
+    }
+}
+
+/*
+ * For an implementation where everything is delegated to the backend
+ * we don't do anything other than return the full feature set offered
+ * by the daemon (module the reserved feature bit).
+ */
+static uint64_t vud_get_features(VirtIODevice *vdev,
+                                 uint64_t requested_features, Error **errp)
+{
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    /* This should be set when the vhost connection initialises */
+    g_assert(vud->vhost_dev.features);
+    return vud->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+}
+
+static void vud_handle_output(VirtIODevice *vdev, VirtQueue *vq)
+{
+    /*
+     * Not normally called; it's the daemon that handles the queue;
+     * however virtio's cleanup path can call this.
+     */
+}
+
+static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserDevice *vud)
+{
+    vhost_user_cleanup(&vud->vhost_user);
+
+    for (int i = 0; i < vud->num_vqs; i++) {
+        VirtQueue *vq = g_ptr_array_index(vud->vqs, i);
+        virtio_delete_queue(vq);
+    }
+
+    virtio_cleanup(vdev);
+}
+
+static int vud_connect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+
+    if (vud->connected) {
+        return 0;
+    }
+    vud->connected = true;
+
+    /* restore vhost state */
+    if (virtio_device_started(vdev, vdev->status)) {
+        vud_start(vdev);
+    }
+
+    return 0;
+}
+
+static void vud_disconnect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+
+    if (!vud->connected) {
+        return;
+    }
+    vud->connected = false;
+
+    if (vhost_dev_is_started(&vud->vhost_dev)) {
+        vud_stop(vdev);
+    }
+}
+
+static void vud_event(void *opaque, QEMUChrEvent event)
+{
+    DeviceState *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+        if (vud_connect(dev) < 0) {
+            qemu_chr_fe_disconnect(&vud->chardev);
+            return;
+        }
+        break;
+    case CHR_EVENT_CLOSED:
+        vud_disconnect(dev);
+        break;
+    case CHR_EVENT_BREAK:
+    case CHR_EVENT_MUX_IN:
+    case CHR_EVENT_MUX_OUT:
+        /* Ignore */
+        break;
+    }
+}
+
+static void vud_device_realize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(dev);
+    int ret;
+
+    if (!vud->chardev.chr) {
+        error_setg(errp, "vhost-user-device: missing chardev");
+        return;
+    }
+
+    if (!vud->virtio_id) {
+        error_setg(errp, "vhost-user-device: need to define device id");
+        return;
+    }
+
+    if (!vud->num_vqs) {
+        vud->num_vqs = 1; /* reasonable default? */
+    }
+
+    if (!vhost_user_init(&vud->vhost_user, &vud->chardev, errp)) {
+        return;
+    }
+
+    virtio_init(vdev, vud->virtio_id, 0);
+
+    /*
+     * Disable guest notifiers, by default all notifications will be via the
+     * asynchronous vhost-user socket.
+     */
+    vdev->use_guest_notifier_mask = false;
+
+    /* Allocate queues */
+    vud->vqs = g_ptr_array_sized_new(vud->num_vqs);
+    for (int i = 0; i < vud->num_vqs; i++) {
+        g_ptr_array_add(vud->vqs,
+                        virtio_add_queue(vdev, 4, vud_handle_output));
+    }
+
+    vud->vhost_dev.nvqs = vud->num_vqs;
+    vud->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vud->vhost_dev.nvqs);
+
+    /* connect to backend */
+    fprintf(stderr, "%s: doing vhost_dev_init()\n", __func__);
+    ret = vhost_dev_init(&vud->vhost_dev, &vud->vhost_user,
+                         VHOST_BACKEND_TYPE_USER, 0, errp);
+
+    if (ret < 0) {
+        do_vhost_user_cleanup(vdev, vud);
+    }
+
+    qemu_chr_fe_set_handlers(&vud->chardev, NULL, NULL, vud_event, NULL,
+                             dev, NULL, true);
+}
+
+static void vud_device_unrealize(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserDevice *vud = VHOST_USER_DEVICE(dev);
+    struct vhost_virtqueue *vhost_vqs = vud->vhost_dev.vqs;
+
+    /* This will stop vhost backend if appropriate. */
+    vud_set_status(vdev, 0);
+    vhost_dev_cleanup(&vud->vhost_dev);
+    g_free(vhost_vqs);
+    do_vhost_user_cleanup(vdev, vud);
+}
+
+static const VMStateDescription vud_vmstate = {
+    .name = "vhost-user-device",
+    .unmigratable = 1,
+};
+
+static Property vud_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev),
+    DEFINE_PROP_UINT16("virtio-id", VHostUserDevice, virtio_id, 0),
+    DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vud_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, vud_properties);
+    dc->vmsd = &vud_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    vdc->realize = vud_device_realize;
+    vdc->unrealize = vud_device_unrealize;
+    vdc->get_features = vud_get_features;
+    vdc->set_status = vud_set_status;
+}
+
+static const TypeInfo vud_info = {
+    .name = TYPE_VHOST_USER_DEVICE,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VHostUserDevice),
+    .class_init = vud_class_init,
+};
+
+static void vud_register_types(void)
+{
+    type_register_static(&vud_info);
+}
+
+type_init(vud_register_types)
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index bdec78bfc6..43e5fa3f7d 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -10,7 +10,9 @@ specific_virtio_ss.add(files('virtio-config-io.c', 'virtio-qmp.c'))
 if have_vhost
   specific_virtio_ss.add(files('vhost.c', 'vhost-backend.c', 'vhost-iova-tree.c'))
   if have_vhost_user
+    # fixme - this really should be generic
     specific_virtio_ss.add(files('vhost-user.c'))
+    softmmu_virtio_ss.add(files('vhost-user-device.c'))
   endif
   if have_vhost_vdpa
     specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow-virtqueue.c'))
-- 
2.39.2

[PATCH 06/12] virtio: add PCI stub for vhost-user-device

[Alex Bennée]

This is all pretty much boilerplate.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 hw/virtio/vhost-user-device-pci.c | 71 +++++++++++++++++++++++++++++++
 hw/virtio/meson.build             |  1 +
 2 files changed, 72 insertions(+)
 create mode 100644 hw/virtio/vhost-user-device-pci.c

diff --git a/hw/virtio/vhost-user-device-pci.c b/hw/virtio/vhost-user-device-pci.c
new file mode 100644
index 0000000000..96bf99d5fd
--- /dev/null
+++ b/hw/virtio/vhost-user-device-pci.c
@@ -0,0 +1,71 @@
+/*
+ * Vhost-user generic virtio device PCI glue
+ *
+ * Copyright (c) 2023 Linaro Ltd
+ * Author: Alex Bennée <alex.bennee@linaro.org>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/vhost-user-device.h"
+#include "hw/virtio/virtio-pci.h"
+
+struct VHostUserDevicePCI {
+    VirtIOPCIProxy parent_obj;
+    VHostUserDevice vdev;
+};
+
+typedef struct VHostUserDevicePCI VHostUserDevicePCI;
+
+#define TYPE_VHOST_USER_DEVICE_PCI "vhost-user-device-pci-base"
+
+DECLARE_INSTANCE_CHECKER(VHostUserDevicePCI,
+                         VHOST_USER_DEVICE_PCI,
+                         TYPE_VHOST_USER_DEVICE_PCI)
+
+static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+    VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
+    DeviceState *vdev = DEVICE(&dev->vdev);
+
+    vpci_dev->nvectors = 1;
+    qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
+}
+
+static void vhost_user_device_pci_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+    PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+    k->realize = vhost_user_device_pci_realize;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+    pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
+    pcidev_k->revision = 0x00;
+    pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
+}
+
+static void vhost_user_device_pci_instance_init(Object *obj)
+{
+    VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(obj);
+
+    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
+                                TYPE_VHOST_USER_DEVICE);
+}
+
+static const VirtioPCIDeviceTypeInfo vhost_user_device_pci_info = {
+    .base_name = TYPE_VHOST_USER_DEVICE_PCI,
+    .non_transitional_name = "vhost-user-device-pci",
+    .instance_size = sizeof(VHostUserDevicePCI),
+    .instance_init = vhost_user_device_pci_instance_init,
+    .class_init = vhost_user_device_pci_class_init,
+};
+
+static void vhost_user_device_pci_register(void)
+{
+    virtio_pci_types_register(&vhost_user_device_pci_info);
+}
+
+type_init(vhost_user_device_pci_register);
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 43e5fa3f7d..c0a86b94ae 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -13,6 +13,7 @@ if have_vhost
     # fixme - this really should be generic
     specific_virtio_ss.add(files('vhost-user.c'))
     softmmu_virtio_ss.add(files('vhost-user-device.c'))
+    softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: files('vhost-user-device-pci.c'))
   endif
   if have_vhost_vdpa
     specific_virtio_ss.add(files('vhost-vdpa.c', 'vhost-shadow-virtqueue.c'))
-- 
2.39.2

Re: [PATCH 06/12] virtio: add PCI stub for vhost-user-device

[Erik Schilling]

On 4/14/23 18:04, Alex Bennée wrote:
> This is all pretty much boilerplate.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Tested-by: Erik Schilling <erik.schilling@linaro.org>

Tested with d6f9fb0 of a rust-vmm SCSI device [1] and -device 
vhost-user-device-pci,virtio-id=8,num_vqs=3,config_size=36,chardev=vus

[1] https://github.com/rust-vmm/vhost-device/pull/301

- Erik

[PATCH 07/12] include: attempt to document device_class_set_props

[Alex Bennée]

I'm still not sure how I achieve by use case of the parent class
defining the following properties:

  static Property vud_properties[] = {
      DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev),
      DEFINE_PROP_UINT16("id", VHostUserDevice, id, 0),
      DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1),
      DEFINE_PROP_END_OF_LIST(),
  };

But for the specialisation of the class I want the id to default to
the actual device id, e.g.:

  static Property vu_rng_properties[] = {
      DEFINE_PROP_UINT16("id", VHostUserDevice, id, VIRTIO_ID_RNG),
      DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1),
      DEFINE_PROP_END_OF_LIST(),
  };

And so far the API for doing that isn't super clear.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/qdev-core.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index bd50ad5ee1..d4bbc30c92 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -776,6 +776,15 @@ BusState *sysbus_get_default(void);
 char *qdev_get_fw_dev_path(DeviceState *dev);
 char *qdev_get_own_fw_dev_path_from_handler(BusState *bus, DeviceState *dev);
 
+/**
+ * device_class_set_props(): add a set of properties to an device
+ * @dc: the parent DeviceClass all devices inherit
+ * @props: an array of properties, terminate by DEFINE_PROP_END_OF_LIST()
+ *
+ * This will add a set of properties to the object. It will fault if
+ * you attempt to add an existing property defined by a parent class.
+ * To modify an inherited property you need to use????
+ */
 void device_class_set_props(DeviceClass *dc, Property *props);
 
 /**
-- 
2.39.2

[PATCH 08/12] qom: allow for properties to become "fixed"

[Alex Bennée]

When specialising general purpose objects it is sometimes useful to
"fix" some of the properties that were configurable by the base
classes. We will use this facility when specialising
vhost-user-device.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 qapi/qom.json           |  2 ++
 include/qom/object.h    | 16 +++++++++++++++-
 qom/object.c            | 14 ++++++++++++++
 qom/object_interfaces.c |  9 ++++++---
 qom/qom-qmp-cmds.c      |  1 +
 softmmu/qdev-monitor.c  |  1 +
 6 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/qapi/qom.json b/qapi/qom.json
index a877b879b9..4cda191f00 100644
--- a/qapi/qom.json
+++ b/qapi/qom.json
@@ -33,12 +33,14 @@
 # @description: if specified, the description of the property.
 #
 # @default-value: the default value, if any (since 5.0)
+# @fixed: if specified if value has been fixed (since 8.1)
 #
 # Since: 1.2
 ##
 { 'struct': 'ObjectPropertyInfo',
   'data': { 'name': 'str',
             'type': 'str',
+            'fixed': 'bool',
             '*description': 'str',
             '*default-value': 'any' } }
 
diff --git a/include/qom/object.h b/include/qom/object.h
index ef7258a5e1..f18d1a8254 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -97,6 +97,8 @@ struct ObjectProperty
     ObjectPropertyInit *init;
     void *opaque;
     QObject *defval;
+    /** @fixed: if the property has been fixed at its default */
+    bool fixed;
 };
 
 /**
@@ -1111,6 +1113,17 @@ void object_property_set_default_int(ObjectProperty *prop, int64_t value);
  */
 void object_property_set_default_uint(ObjectProperty *prop, uint64_t value);
 
+/**
+ * object_property_fix_default_uint:
+ * @prop: the property to be fixed
+ * @value: the fixed value to be written to the property
+ *
+ * When specialising an object it may make send to fix some values and
+ * not allow them to be changed. This can only be applied to
+ * properties that previously had a default and now cannot be changed.
+ */
+void object_property_fix_default_uint(ObjectProperty *prop, uint64_t value);
+
 /**
  * object_property_find:
  * @obj: the object
@@ -1961,13 +1974,14 @@ size_t object_type_get_instance_size(const char *typename);
  * object_property_help:
  * @name: the name of the property
  * @type: the type of the property
+ * @fixed: has the value been fixed
  * @defval: the default value
  * @description: description of the property
  *
  * Returns: a user-friendly formatted string describing the property
  * for help purposes.
  */
-char *object_property_help(const char *name, const char *type,
+char *object_property_help(const char *name, const char *type, bool fixed,
                            QObject *defval, const char *description);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(Object, object_unref)
diff --git a/qom/object.c b/qom/object.c
index e25f1e96db..b5aba3ffc8 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1584,6 +1584,20 @@ void object_property_set_default_uint(ObjectProperty *prop, uint64_t value)
     object_property_set_default(prop, QOBJECT(qnum_from_uint(value)));
 }
 
+static void object_property_fix_default(ObjectProperty *prop, QObject *defval)
+{
+    g_assert(prop->init == object_property_init_defval);
+    g_assert(!prop->fixed);
+
+    prop->defval = defval;
+    prop->fixed = true;
+}
+
+void object_property_fix_default_uint(ObjectProperty *prop, uint64_t value)
+{
+    object_property_fix_default(prop, QOBJECT(qnum_from_uint(value)));
+}
+
 bool object_property_set_uint(Object *obj, const char *name,
                               uint64_t value, Error **errp)
 {
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 7d31589b04..e351938f8f 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -161,7 +161,7 @@ void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
     visit_free(v);
 }
 
-char *object_property_help(const char *name, const char *type,
+char *object_property_help(const char *name, const char *type, bool fixed,
                            QObject *defval, const char *description)
 {
     GString *str = g_string_new(NULL);
@@ -179,7 +179,9 @@ char *object_property_help(const char *name, const char *type,
     if (defval) {
         g_autofree char *def_json = g_string_free(qobject_to_json(defval),
                                                   false);
-        g_string_append_printf(str, " (default: %s)", def_json);
+        g_string_append_printf(str, " (%s: %s)",
+                               fixed ? "fixed" : "default",
+                               def_json);
     }
 
     return g_string_free(str, false);
@@ -220,7 +222,8 @@ bool type_print_class_properties(const char *type)
 
         g_ptr_array_add(array,
                         object_property_help(prop->name, prop->type,
-                                             prop->defval, prop->description));
+                                             prop->fixed, prop->defval,
+                                             prop->description));
     }
     g_ptr_array_sort(array, (GCompareFunc)qemu_pstrcmp0);
     if (array->len > 0) {
diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c
index 7c087299de..f4cdf4ddde 100644
--- a/qom/qom-qmp-cmds.c
+++ b/qom/qom-qmp-cmds.c
@@ -55,6 +55,7 @@ ObjectPropertyInfoList *qmp_qom_list(const char *path, Error **errp)
 
         value->name = g_strdup(prop->name);
         value->type = g_strdup(prop->type);
+        value->fixed = prop->fixed;
     }
 
     return props;
diff --git a/softmmu/qdev-monitor.c b/softmmu/qdev-monitor.c
index b8d2c4dadd..b56b2af2f2 100644
--- a/softmmu/qdev-monitor.c
+++ b/softmmu/qdev-monitor.c
@@ -315,6 +315,7 @@ int qdev_device_help(QemuOpts *opts)
         g_ptr_array_add(array,
                         object_property_help(prop->value->name,
                                              prop->value->type,
+                                             prop->value->fixed,
                                              prop->value->default_value,
                                              prop->value->description));
     }
-- 
2.39.2

Re: [PATCH 08/12] qom: allow for properties to become "fixed"

[Markus Armbruster]

Alex Bennée <alex.bennee@linaro.org> writes:

> When specialising general purpose objects it is sometimes useful to
> "fix" some of the properties that were configurable by the base
> classes. We will use this facility when specialising
> vhost-user-device.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  qapi/qom.json           |  2 ++
>  include/qom/object.h    | 16 +++++++++++++++-
>  qom/object.c            | 14 ++++++++++++++
>  qom/object_interfaces.c |  9 ++++++---
>  qom/qom-qmp-cmds.c      |  1 +
>  softmmu/qdev-monitor.c  |  1 +
>  6 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/qom.json b/qapi/qom.json
> index a877b879b9..4cda191f00 100644
> --- a/qapi/qom.json
> +++ b/qapi/qom.json
> @@ -33,12 +33,14 @@
>  # @description: if specified, the description of the property.
>  #
>  # @default-value: the default value, if any (since 5.0)
> +# @fixed: if specified if value has been fixed (since 8.1)

Wat?

>  #
>  # Since: 1.2
>  ##
>  { 'struct': 'ObjectPropertyInfo',
>    'data': { 'name': 'str',
>              'type': 'str',
> +            'fixed': 'bool',
>              '*description': 'str',
>              '*default-value': 'any' } }
>  

qom-list and qom-list-properties return a list of this.  Use cases for
the new member?

[...]

Re: [PATCH 08/12] qom: allow for properties to become "fixed"

[Alex Bennée]

Markus Armbruster <armbru@redhat.com> writes:

> Alex Bennée <alex.bennee@linaro.org> writes:
>
>> When specialising general purpose objects it is sometimes useful to
>> "fix" some of the properties that were configurable by the base
>> classes. We will use this facility when specialising
>> vhost-user-device.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  qapi/qom.json           |  2 ++
>>  include/qom/object.h    | 16 +++++++++++++++-
>>  qom/object.c            | 14 ++++++++++++++
>>  qom/object_interfaces.c |  9 ++++++---
>>  qom/qom-qmp-cmds.c      |  1 +
>>  softmmu/qdev-monitor.c  |  1 +
>>  6 files changed, 39 insertions(+), 4 deletions(-)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index a877b879b9..4cda191f00 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -33,12 +33,14 @@
>>  # @description: if specified, the description of the property.
>>  #
>>  # @default-value: the default value, if any (since 5.0)
>> +# @fixed: if specified if value has been fixed (since 8.1)
>
> Wat?
>
>>  #
>>  # Since: 1.2
>>  ##
>>  { 'struct': 'ObjectPropertyInfo',
>>    'data': { 'name': 'str',
>>              'type': 'str',
>> +            'fixed': 'bool',
>>              '*description': 'str',
>>              '*default-value': 'any' } }
>>  
>
> qom-list and qom-list-properties return a list of this.  Use cases for
> the new member?

The use-case is this whole series. Basically I want to have a generic
device (vhost-user-device) which has a bunch of control knobs the user
can fiddle with (e.g. virtio id, num_vqs and the like). However for the
specialised versions of this device (e.g. vhost-user-gpio) some of these
values (e.g. virtio id) need to be fixed.

Mark suggested maybe just duplicating the properties in a similar way to
DEFINE_AUDIO_PROPERTIES but that doesn't really address the problem
wanting to "fix" some of the values for the subclasses and preventing
the user from changing things.

I appreciate this is possibly a horrible hack so I'm open to the QOM
experts showing me the correct way to model this sort of behaviour.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 08/12] qom: allow for properties to become "fixed"

[Peter Maydell]

On Mon, 17 Apr 2023 at 12:33, Alex Bennée <alex.bennee@linaro.org> wrote:
> The use-case is this whole series. Basically I want to have a generic
> device (vhost-user-device) which has a bunch of control knobs the user
> can fiddle with (e.g. virtio id, num_vqs and the like). However for the
> specialised versions of this device (e.g. vhost-user-gpio) some of these
> values (e.g. virtio id) need to be fixed.

> Mark suggested maybe just duplicating the properties in a similar way to
> DEFINE_AUDIO_PROPERTIES but that doesn't really address the problem
> wanting to "fix" some of the values for the subclasses and preventing
> the user from changing things.

This shouldn't be something visible to the user of the object,
though, surely? An object which doesn't have a configurable
virtio-id property because the specific subclass has a fixed
value should look exactly like an object which doesn't have
a configurable virtio-id property because that property just
doesn't exist.

If we add a facility for "constant properties" (which is pretty
much what this would be) then we should do it because it's
useful for users of QOM objects (and especially for users of
QOM objects via the QMP interface) to be able to introspect
them and say "ah, this is a property of the object but it's
a constant value".

thanks
-- PMM

[PATCH 09/12] hw/virtio: derive vhost-user-rng from vhost-user-device

[Alex Bennée]

Now we can take advantage of our new base class and make
vhost-user-rng a much simpler boilerplate wrapper.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user-rng.h |  11 +-
 hw/virtio/vhost-user-rng.c         | 264 +----------------------------
 2 files changed, 8 insertions(+), 267 deletions(-)

diff --git a/include/hw/virtio/vhost-user-rng.h b/include/hw/virtio/vhost-user-rng.h
index ddd9f01eea..5be2999498 100644
--- a/include/hw/virtio/vhost-user-rng.h
+++ b/include/hw/virtio/vhost-user-rng.h
@@ -12,21 +12,14 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user.h"
-#include "chardev/char-fe.h"
+#include "hw/virtio/vhost-user-device.h"
 
 #define TYPE_VHOST_USER_RNG "vhost-user-rng"
 OBJECT_DECLARE_SIMPLE_TYPE(VHostUserRNG, VHOST_USER_RNG)
 
 struct VHostUserRNG {
     /*< private >*/
-    VirtIODevice parent;
-    CharBackend chardev;
-    struct vhost_virtqueue *vhost_vq;
-    struct vhost_dev vhost_dev;
-    VhostUserState vhost_user;
-    VirtQueue *req_vq;
-    bool connected;
-
+    VHostUserDevice parent;
     /*< public >*/
 };
 
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index efc54cd3fb..1f23442b81 100644
--- a/hw/virtio/vhost-user-rng.c
+++ b/hw/virtio/vhost-user-rng.c
@@ -3,7 +3,7 @@
  *
  * Copyright (c) 2021 Mathieu Poirier <mathieu.poirier@linaro.org>
  *
- * Implementation seriously tailored on vhost-user-i2c.c
+ * Simple wrapper of the generic vhost-user-device.
  *
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
@@ -13,281 +13,29 @@
 #include "hw/qdev-properties.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/vhost-user-rng.h"
-#include "qemu/error-report.h"
 #include "standard-headers/linux/virtio_ids.h"
 
-static const int feature_bits[] = {
-    VIRTIO_F_RING_RESET,
-    VHOST_INVALID_FEATURE_BIT
-};
-
-static void vu_rng_start(VirtIODevice *vdev)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    int ret;
-    int i;
-
-    if (!k->set_guest_notifiers) {
-        error_report("binding does not support guest notifiers");
-        return;
-    }
-
-    ret = vhost_dev_enable_notifiers(&rng->vhost_dev, vdev);
-    if (ret < 0) {
-        error_report("Error enabling host notifiers: %d", -ret);
-        return;
-    }
-
-    ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, true);
-    if (ret < 0) {
-        error_report("Error binding guest notifier: %d", -ret);
-        goto err_host_notifiers;
-    }
-
-    rng->vhost_dev.acked_features = vdev->guest_features;
-    ret = vhost_dev_start(&rng->vhost_dev, vdev, true);
-    if (ret < 0) {
-        error_report("Error starting vhost-user-rng: %d", -ret);
-        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 < rng->vhost_dev.nvqs; i++) {
-        vhost_virtqueue_mask(&rng->vhost_dev, vdev, i, false);
-    }
-
-    return;
-
-err_guest_notifiers:
-    k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
-err_host_notifiers:
-    vhost_dev_disable_notifiers(&rng->vhost_dev, vdev);
-}
-
-static void vu_rng_stop(VirtIODevice *vdev)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    int ret;
-
-    if (!k->set_guest_notifiers) {
-        return;
-    }
-
-    vhost_dev_stop(&rng->vhost_dev, vdev, true);
-
-    ret = k->set_guest_notifiers(qbus->parent, rng->vhost_dev.nvqs, false);
-    if (ret < 0) {
-        error_report("vhost guest notifier cleanup failed: %d", ret);
-        return;
-    }
-
-    vhost_dev_disable_notifiers(&rng->vhost_dev, vdev);
-}
-
-static void vu_rng_set_status(VirtIODevice *vdev, uint8_t status)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    bool should_start = virtio_device_should_start(vdev, status);
-
-    if (vhost_dev_is_started(&rng->vhost_dev) == should_start) {
-        return;
-    }
-
-    if (should_start) {
-        vu_rng_start(vdev);
-    } else {
-        vu_rng_stop(vdev);
-    }
-}
-
-static uint64_t vu_rng_get_features(VirtIODevice *vdev,
-                                    uint64_t requested_features, Error **errp)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-
-    return vhost_get_features(&rng->vhost_dev, feature_bits,
-                              requested_features);
-}
-
-static void vu_rng_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-    /*
-     * Not normally called; it's the daemon that handles the queue;
-     * however virtio's cleanup path can call this.
-     */
-}
-
-static void vu_rng_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-
-    vhost_virtqueue_mask(&rng->vhost_dev, vdev, idx, mask);
-}
-
-static bool vu_rng_guest_notifier_pending(VirtIODevice *vdev, int idx)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-
-    return vhost_virtqueue_pending(&rng->vhost_dev, idx);
-}
-
-static void vu_rng_connect(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-
-    if (rng->connected) {
-        return;
-    }
-
-    rng->connected = true;
-
-    /* restore vhost state */
-    if (virtio_device_started(vdev, vdev->status)) {
-        vu_rng_start(vdev);
-    }
-}
-
-static void vu_rng_disconnect(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-
-    if (!rng->connected) {
-        return;
-    }
-
-    rng->connected = false;
-
-    if (vhost_dev_is_started(&rng->vhost_dev)) {
-        vu_rng_stop(vdev);
-    }
-}
-
-static void vu_rng_event(void *opaque, QEMUChrEvent event)
-{
-    DeviceState *dev = opaque;
-
-    switch (event) {
-    case CHR_EVENT_OPENED:
-        vu_rng_connect(dev);
-        break;
-    case CHR_EVENT_CLOSED:
-        vu_rng_disconnect(dev);
-        break;
-    case CHR_EVENT_BREAK:
-    case CHR_EVENT_MUX_IN:
-    case CHR_EVENT_MUX_OUT:
-        /* Ignore */
-        break;
-    }
-}
-
-static void vu_rng_device_realize(DeviceState *dev, Error **errp)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserRNG *rng = VHOST_USER_RNG(dev);
-    int ret;
-
-    if (!rng->chardev.chr) {
-        error_setg(errp, "missing chardev");
-        return;
-    }
-
-    if (!vhost_user_init(&rng->vhost_user, &rng->chardev, errp)) {
-        return;
-    }
-
-    virtio_init(vdev, VIRTIO_ID_RNG, 0);
-
-    rng->req_vq = virtio_add_queue(vdev, 4, vu_rng_handle_output);
-    if (!rng->req_vq) {
-        error_setg_errno(errp, -1, "virtio_add_queue() failed");
-        goto virtio_add_queue_failed;
-    }
-
-    rng->vhost_dev.nvqs = 1;
-    rng->vhost_dev.vqs = g_new0(struct vhost_virtqueue, rng->vhost_dev.nvqs);
-    ret = vhost_dev_init(&rng->vhost_dev, &rng->vhost_user,
-                         VHOST_BACKEND_TYPE_USER, 0, errp);
-    if (ret < 0) {
-        error_setg_errno(errp, -ret, "vhost_dev_init() failed");
-        goto vhost_dev_init_failed;
-    }
-
-    qemu_chr_fe_set_handlers(&rng->chardev, NULL, NULL, vu_rng_event, NULL,
-                             dev, NULL, true);
-
-    return;
-
-vhost_dev_init_failed:
-    g_free(rng->vhost_dev.vqs);
-    virtio_delete_queue(rng->req_vq);
-virtio_add_queue_failed:
-    virtio_cleanup(vdev);
-    vhost_user_cleanup(&rng->vhost_user);
-}
-
-static void vu_rng_device_unrealize(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserRNG *rng = VHOST_USER_RNG(dev);
-    struct vhost_virtqueue *vhost_vqs = rng->vhost_dev.vqs;
-
-    vu_rng_set_status(vdev, 0);
-
-    vhost_dev_cleanup(&rng->vhost_dev);
-    g_free(vhost_vqs);
-    virtio_delete_queue(rng->req_vq);
-    virtio_cleanup(vdev);
-    vhost_user_cleanup(&rng->vhost_user);
-}
-
-static struct vhost_dev *vu_rng_get_vhost(VirtIODevice *vdev)
-{
-    VHostUserRNG *rng = VHOST_USER_RNG(vdev);
-    return &rng->vhost_dev;
-}
-
 static const VMStateDescription vu_rng_vmstate = {
     .name = "vhost-user-rng",
     .unmigratable = 1,
 };
 
-static Property vu_rng_properties[] = {
-    DEFINE_PROP_CHR("chardev", VHostUserRNG, chardev),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void vu_rng_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    ObjectProperty *op;
 
-    device_class_set_props(dc, vu_rng_properties);
     dc->vmsd = &vu_rng_vmstate;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 
-    vdc->realize = vu_rng_device_realize;
-    vdc->unrealize = vu_rng_device_unrealize;
-    vdc->get_features = vu_rng_get_features;
-    vdc->set_status = vu_rng_set_status;
-    vdc->guest_notifier_mask = vu_rng_guest_notifier_mask;
-    vdc->guest_notifier_pending = vu_rng_guest_notifier_pending;
-    vdc->get_vhost = vu_rng_get_vhost;
+    op = object_class_property_find(klass, "virtio-id");
+    g_assert(op);
+    object_property_fix_default_uint(op, VIRTIO_ID_RNG);
 }
 
 static const TypeInfo vu_rng_info = {
     .name = TYPE_VHOST_USER_RNG,
-    .parent = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_VHOST_USER_DEVICE,
     .instance_size = sizeof(VHostUserRNG),
     .class_init = vu_rng_class_init,
 };
-- 
2.39.2

[PATCH 10/12] hw/virtio: add config support to vhost-user-device

[Alex Bennée]

To use the generic device the user will need to provide the config
region size via the command line. We also add a notifier so the guest
can be pinged if the remote daemon updates the config.

With these changes:

  -device vhost-user-device-pci,virtio-id=41,num_vqs=2,config_size=8

is equivalent to:

  -device vhost-user-gpio-pci

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user-device.h |  1 +
 hw/virtio/vhost-user-device.c         | 58 ++++++++++++++++++++++++++-
 2 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/include/hw/virtio/vhost-user-device.h b/include/hw/virtio/vhost-user-device.h
index 8d77f06721..cb98e0dcaa 100644
--- a/include/hw/virtio/vhost-user-device.h
+++ b/include/hw/virtio/vhost-user-device.h
@@ -21,6 +21,7 @@ struct VHostUserDevice {
     CharBackend chardev;
     uint16_t virtio_id;
     uint32_t num_vqs;
+    uint32_t config_size;
     /* State tracking */
     VhostUserState vhost_user;
     struct vhost_virtqueue *vhost_vq;
diff --git a/hw/virtio/vhost-user-device.c b/hw/virtio/vhost-user-device.c
index bfbf3b29cb..977cfed247 100644
--- a/hw/virtio/vhost-user-device.c
+++ b/hw/virtio/vhost-user-device.c
@@ -117,6 +117,42 @@ static uint64_t vud_get_features(VirtIODevice *vdev,
     return vud->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
 }
 
+/*
+ * To handle VirtIO config we need to know the size of the config
+ * space. We don't cache the config but re-fetch it from the guest
+ * every time in case something has changed.
+ */
+static void vud_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    Error *local_err = NULL;
+
+    /*
+     * There will have been a warning during vhost_dev_init, but lets
+     * assert here as nothing will go right now.
+     */
+    g_assert(vud->config_size && vud->vhost_user.supports_config == true);
+
+    if (vhost_dev_get_config(&vud->vhost_dev, config,
+                             vud->config_size, &local_err)) {
+        error_report_err(local_err);
+    }
+}
+
+/*
+ * When the daemon signals an update to the config we just need to
+ * signal the guest as we re-read the config on demand above.
+ */
+static int vud_config_notifier(struct vhost_dev *dev)
+{
+    virtio_notify_config(dev->vdev);
+    return 0;
+}
+
+const VhostDevConfigOps vud_config_ops = {
+    .vhost_dev_config_notifier = vud_config_notifier,
+};
+
 static void vud_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     /*
@@ -141,12 +177,21 @@ static int vud_connect(DeviceState *dev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostUserDevice *vud = VHOST_USER_DEVICE(vdev);
+    struct vhost_dev *vhost_dev = &vud->vhost_dev;
 
     if (vud->connected) {
         return 0;
     }
     vud->connected = true;
 
+    /*
+     * If we support VHOST_USER_GET_CONFIG we must enable the notifier
+     * so we can ping the guest when it updates.
+     */
+    if (vud->vhost_user.supports_config) {
+        vhost_dev_set_config_notifier(vhost_dev, &vud_config_ops);
+    }
+
     /* restore vhost state */
     if (virtio_device_started(vdev, vdev->status)) {
         vud_start(vdev);
@@ -214,11 +259,20 @@ static void vud_device_realize(DeviceState *dev, Error **errp)
         vud->num_vqs = 1; /* reasonable default? */
     }
 
+    /*
+     * We can't handle config requests unless we know the size of the
+     * config region, specialisations of the vhost-user-device will be
+     * able to set this.
+     */
+    if (vud->config_size) {
+        vud->vhost_user.supports_config = true;
+    }
+
     if (!vhost_user_init(&vud->vhost_user, &vud->chardev, errp)) {
         return;
     }
 
-    virtio_init(vdev, vud->virtio_id, 0);
+    virtio_init(vdev, vud->virtio_id, vud->config_size);
 
     /*
      * Disable guest notifiers, by default all notifications will be via the
@@ -271,6 +325,7 @@ static Property vud_properties[] = {
     DEFINE_PROP_CHR("chardev", VHostUserDevice, chardev),
     DEFINE_PROP_UINT16("virtio-id", VHostUserDevice, virtio_id, 0),
     DEFINE_PROP_UINT32("num_vqs", VHostUserDevice, num_vqs, 1),
+    DEFINE_PROP_UINT32("config_size", VHostUserDevice, config_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -285,6 +340,7 @@ static void vud_class_init(ObjectClass *klass, void *data)
     vdc->realize = vud_device_realize;
     vdc->unrealize = vud_device_unrealize;
     vdc->get_features = vud_get_features;
+    vdc->get_config = vud_get_config;
     vdc->set_status = vud_set_status;
 }
 
-- 
2.39.2

[PATCH 11/12] hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN)

[Alex Bennée]

Now we can take advantage of our new base class and make
vhost-user-gpio a much simpler boilerplate wrapper.

[AJB - and this breaks because of the class init propery hack leading
to the config getting overriden and firing the assert. I need a clean
QOM way to do this derived class]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 include/hw/virtio/vhost-user-gpio.h |  23 +-
 hw/virtio/vhost-user-gpio.c         | 405 ++--------------------------
 2 files changed, 17 insertions(+), 411 deletions(-)

diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
index a9d3f9b049..82a2e36c21 100644
--- a/include/hw/virtio/vhost-user-gpio.h
+++ b/include/hw/virtio/vhost-user-gpio.h
@@ -12,33 +12,14 @@
 #include "hw/virtio/virtio.h"
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user.h"
-#include "standard-headers/linux/virtio_gpio.h"
-#include "chardev/char-fe.h"
+#include "hw/virtio/vhost-user-device.h"
 
 #define TYPE_VHOST_USER_GPIO "vhost-user-gpio-device"
 OBJECT_DECLARE_SIMPLE_TYPE(VHostUserGPIO, VHOST_USER_GPIO);
 
 struct VHostUserGPIO {
     /*< private >*/
-    VirtIODevice parent_obj;
-    CharBackend chardev;
-    struct virtio_gpio_config config;
-    struct vhost_virtqueue *vhost_vqs;
-    struct vhost_dev vhost_dev;
-    VhostUserState vhost_user;
-    VirtQueue *command_vq;
-    VirtQueue *interrupt_vq;
-    /**
-     * There are at least two steps of initialization of the
-     * vhost-user device. The first is a "connect" step and
-     * second is a "start" step. Make a separation between
-     * those initialization phases by using two fields.
-     *
-     * @connected: see vu_gpio_connect()/vu_gpio_disconnect()
-     * @started_vu: see vu_gpio_start()/vu_gpio_stop()
-     */
-    bool connected;
-    bool started_vu;
+    VHostUserDevice parent;
     /*< public >*/
 };
 
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3b013f2d0f..c76af1fc69 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -11,413 +11,38 @@
 #include "hw/qdev-properties.h"
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/vhost-user-gpio.h"
-#include "qemu/error-report.h"
 #include "standard-headers/linux/virtio_ids.h"
-#include "trace.h"
-
-#define REALIZE_CONNECTION_RETRIES 3
-#define VHOST_NVQS 2
-
-/* Features required from VirtIO */
-static const int feature_bits[] = {
-    VIRTIO_F_VERSION_1,
-    VIRTIO_F_NOTIFY_ON_EMPTY,
-    VIRTIO_RING_F_INDIRECT_DESC,
-    VIRTIO_RING_F_EVENT_IDX,
-    VIRTIO_GPIO_F_IRQ,
-    VIRTIO_F_RING_RESET,
-    VHOST_INVALID_FEATURE_BIT
-};
-
-static void vu_gpio_get_config(VirtIODevice *vdev, uint8_t *config)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-
-    memcpy(config, &gpio->config, sizeof(gpio->config));
-}
-
-static int vu_gpio_config_notifier(struct vhost_dev *dev)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev->vdev);
-
-    memcpy(dev->vdev->config, &gpio->config, sizeof(gpio->config));
-    virtio_notify_config(dev->vdev);
-
-    return 0;
-}
-
-const VhostDevConfigOps gpio_ops = {
-    .vhost_dev_config_notifier = vu_gpio_config_notifier,
-};
-
-static int vu_gpio_start(VirtIODevice *vdev)
-{
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
-    int ret, i;
-
-    if (!k->set_guest_notifiers) {
-        error_report("binding does not support guest notifiers");
-        return -ENOSYS;
-    }
-
-    ret = vhost_dev_enable_notifiers(vhost_dev, vdev);
-    if (ret < 0) {
-        error_report("Error enabling host notifiers: %d", ret);
-        return ret;
-    }
-
-    ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, true);
-    if (ret < 0) {
-        error_report("Error binding guest notifier: %d", ret);
-        goto err_host_notifiers;
-    }
-
-    /*
-     * Before we start up we need to ensure we have the final feature
-     * set needed for the vhost configuration. The backend may also
-     * apply backend_features when the feature set is sent.
-     */
-    vhost_ack_features(&gpio->vhost_dev, feature_bits, vdev->guest_features);
-
-    ret = vhost_dev_start(&gpio->vhost_dev, vdev, false);
-    if (ret < 0) {
-        error_report("Error starting vhost-user-gpio: %d", ret);
-        goto err_guest_notifiers;
-    }
-    gpio->started_vu = true;
-
-    /*
-     * 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 < gpio->vhost_dev.nvqs; i++) {
-        vhost_virtqueue_mask(&gpio->vhost_dev, vdev, i, false);
-    }
-
-    /*
-     * As we must have VHOST_USER_F_PROTOCOL_FEATURES (because
-     * VHOST_USER_GET_CONFIG requires it) we need to explicitly enable
-     * the vrings.
-     */
-    g_assert(vhost_dev->vhost_ops &&
-             vhost_dev->vhost_ops->vhost_set_vring_enable);
-    ret = vhost_dev->vhost_ops->vhost_set_vring_enable(vhost_dev, true);
-    if (ret == 0) {
-        return 0;
-    }
-
-    error_report("Failed to start vrings for vhost-user-gpio: %d", ret);
-
-err_guest_notifiers:
-    k->set_guest_notifiers(qbus->parent, gpio->vhost_dev.nvqs, false);
-err_host_notifiers:
-    vhost_dev_disable_notifiers(&gpio->vhost_dev, vdev);
-
-    return ret;
-}
-
-static void vu_gpio_stop(VirtIODevice *vdev)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
-    int ret;
-
-    if (!gpio->started_vu) {
-        return;
-    }
-    gpio->started_vu = false;
-
-    if (!k->set_guest_notifiers) {
-        return;
-    }
-
-    vhost_dev_stop(vhost_dev, vdev, false);
-
-    ret = k->set_guest_notifiers(qbus->parent, vhost_dev->nvqs, false);
-    if (ret < 0) {
-        error_report("vhost guest notifier cleanup failed: %d", ret);
-        return;
-    }
-
-    vhost_dev_disable_notifiers(vhost_dev, vdev);
-}
-
-static void vu_gpio_set_status(VirtIODevice *vdev, uint8_t status)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-    bool should_start = virtio_device_should_start(vdev, status);
-
-    trace_virtio_gpio_set_status(status);
-
-    if (!gpio->connected) {
-        return;
-    }
-
-    if (vhost_dev_is_started(&gpio->vhost_dev) == should_start) {
-        return;
-    }
-
-    if (should_start) {
-        if (vu_gpio_start(vdev)) {
-            qemu_chr_fe_disconnect(&gpio->chardev);
-        }
-    } else {
-        vu_gpio_stop(vdev);
-    }
-}
-
-static uint64_t vu_gpio_get_features(VirtIODevice *vdev, uint64_t features,
-                                     Error **errp)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-
-    return vhost_get_features(&gpio->vhost_dev, feature_bits, features);
-}
-
-static void vu_gpio_handle_output(VirtIODevice *vdev, VirtQueue *vq)
-{
-    /*
-     * Not normally called; it's the daemon that handles the queue;
-     * however virtio's cleanup path can call this.
-     */
-}
-
-static void vu_gpio_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
-{
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-
-    /*
-     * Add the check for configure interrupt, Use VIRTIO_CONFIG_IRQ_IDX -1
-     * as the macro of configure interrupt's IDX, If this driver does not
-     * support, the function will return
-     */
-
-    if (idx == VIRTIO_CONFIG_IRQ_IDX) {
-        return;
-    }
-
-    vhost_virtqueue_mask(&gpio->vhost_dev, vdev, idx, mask);
-}
-
-static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserGPIO *gpio)
-{
-    virtio_delete_queue(gpio->command_vq);
-    virtio_delete_queue(gpio->interrupt_vq);
-    g_free(gpio->vhost_vqs);
-    virtio_cleanup(vdev);
-    vhost_user_cleanup(&gpio->vhost_user);
-}
-
-static int vu_gpio_connect(DeviceState *dev, Error **errp)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
-    int ret;
-
-    if (gpio->connected) {
-        return 0;
-    }
-    gpio->connected = true;
-
-    vhost_dev_set_config_notifier(vhost_dev, &gpio_ops);
-    gpio->vhost_user.supports_config = true;
-
-    gpio->vhost_dev.nvqs = VHOST_NVQS;
-    gpio->vhost_dev.vqs = gpio->vhost_vqs;
-
-    ret = vhost_dev_init(vhost_dev, &gpio->vhost_user,
-                         VHOST_BACKEND_TYPE_USER, 0, errp);
-    if (ret < 0) {
-        return ret;
-    }
-
-    /* restore vhost state */
-    if (virtio_device_started(vdev, vdev->status)) {
-        vu_gpio_start(vdev);
-    }
-
-    return 0;
-}
-
-static void vu_gpio_event(void *opaque, QEMUChrEvent event);
-
-static void vu_gpio_disconnect(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-
-    if (!gpio->connected) {
-        return;
-    }
-    gpio->connected = false;
-
-    vu_gpio_stop(vdev);
-    vhost_dev_cleanup(&gpio->vhost_dev);
-
-    /* Re-instate the event handler for new connections */
-    qemu_chr_fe_set_handlers(&gpio->chardev,
-                             NULL, NULL, vu_gpio_event,
-                             NULL, dev, NULL, true);
-}
-
-static void vu_gpio_event(void *opaque, QEMUChrEvent event)
-{
-    DeviceState *dev = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(vdev);
-    Error *local_err = NULL;
-
-    switch (event) {
-    case CHR_EVENT_OPENED:
-        if (vu_gpio_connect(dev, &local_err) < 0) {
-            qemu_chr_fe_disconnect(&gpio->chardev);
-            return;
-        }
-        break;
-    case CHR_EVENT_CLOSED:
-        /* defer close until later to avoid circular close */
-        vhost_user_async_close(dev, &gpio->chardev, &gpio->vhost_dev,
-                               vu_gpio_disconnect);
-        break;
-    case CHR_EVENT_BREAK:
-    case CHR_EVENT_MUX_IN:
-    case CHR_EVENT_MUX_OUT:
-        /* Ignore */
-        break;
-    }
-}
-
-static int vu_gpio_realize_connect(VHostUserGPIO *gpio, Error **errp)
-{
-    VirtIODevice *vdev = &gpio->parent_obj;
-    DeviceState *dev = &vdev->parent_obj;
-    struct vhost_dev *vhost_dev = &gpio->vhost_dev;
-    int ret;
-
-    ret = qemu_chr_fe_wait_connected(&gpio->chardev, errp);
-    if (ret < 0) {
-        return ret;
-    }
-
-    /*
-     * vu_gpio_connect() may have already connected (via the event
-     * callback) in which case it will just report success.
-     */
-    ret = vu_gpio_connect(dev, errp);
-    if (ret < 0) {
-        qemu_chr_fe_disconnect(&gpio->chardev);
-        return ret;
-    }
-    g_assert(gpio->connected);
-
-    ret = vhost_dev_get_config(vhost_dev, (uint8_t *)&gpio->config,
-                               sizeof(gpio->config), errp);
-
-    if (ret < 0) {
-        error_report("vhost-user-gpio: get config failed");
-
-        qemu_chr_fe_disconnect(&gpio->chardev);
-        vhost_dev_cleanup(vhost_dev);
-        return ret;
-    }
-
-    return 0;
-}
-
-static void vu_gpio_device_realize(DeviceState *dev, Error **errp)
-{
-    ERRP_GUARD();
-
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
-    int retries, ret;
-
-    if (!gpio->chardev.chr) {
-        error_setg(errp, "vhost-user-gpio: chardev is mandatory");
-        return;
-    }
-
-    if (!vhost_user_init(&gpio->vhost_user, &gpio->chardev, errp)) {
-        return;
-    }
-
-    virtio_init(vdev, VIRTIO_ID_GPIO, sizeof(gpio->config));
-
-    gpio->command_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
-    gpio->interrupt_vq = virtio_add_queue(vdev, 256, vu_gpio_handle_output);
-    gpio->vhost_vqs = g_new0(struct vhost_virtqueue, VHOST_NVQS);
-
-    gpio->connected = false;
-
-    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, vu_gpio_event, NULL,
-                             dev, NULL, true);
-
-    retries = REALIZE_CONNECTION_RETRIES;
-    g_assert(!*errp);
-    do {
-        if (*errp) {
-            error_prepend(errp, "Reconnecting after error: ");
-            error_report_err(*errp);
-            *errp = NULL;
-        }
-        ret = vu_gpio_realize_connect(gpio, errp);
-    } while (ret < 0 && retries--);
-
-    if (ret < 0) {
-        do_vhost_user_cleanup(vdev, gpio);
-    }
-
-    return;
-}
-
-static void vu_gpio_device_unrealize(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserGPIO *gpio = VHOST_USER_GPIO(dev);
-
-    vu_gpio_set_status(vdev, 0);
-    qemu_chr_fe_set_handlers(&gpio->chardev, NULL, NULL, NULL, NULL, NULL, NULL,
-                             false);
-    vhost_dev_cleanup(&gpio->vhost_dev);
-    do_vhost_user_cleanup(vdev, gpio);
-}
+#include "standard-headers/linux/virtio_gpio.h"
 
 static const VMStateDescription vu_gpio_vmstate = {
     .name = "vhost-user-gpio",
     .unmigratable = 1,
 };
 
-static Property vu_gpio_properties[] = {
-    DEFINE_PROP_CHR("chardev", VHostUserGPIO, chardev),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void vu_gpio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    ObjectProperty *op;
 
-    device_class_set_props(dc, vu_gpio_properties);
     dc->vmsd = &vu_gpio_vmstate;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
-    vdc->realize = vu_gpio_device_realize;
-    vdc->unrealize = vu_gpio_device_unrealize;
-    vdc->get_features = vu_gpio_get_features;
-    vdc->get_config = vu_gpio_get_config;
-    vdc->set_status = vu_gpio_set_status;
-    vdc->guest_notifier_mask = vu_gpio_guest_notifier_mask;
+
+    op = object_class_property_find(klass, "virtio-id");
+    g_assert(op);
+    object_property_fix_default_uint(op, VIRTIO_ID_GPIO);
+
+    op = object_class_property_find(klass, "num-vqs");
+    g_assert(op);
+    object_property_fix_default_uint(op, 2);
+
+    op = object_class_property_find(klass, "config_size");
+    g_assert(op);
+    object_property_fix_default_uint(op, sizeof(struct virtio_gpio_config));
 }
 
 static const TypeInfo vu_gpio_info = {
     .name = TYPE_VHOST_USER_GPIO,
-    .parent = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_VHOST_USER_DEVICE,
     .instance_size = sizeof(VHostUserGPIO),
     .class_init = vu_gpio_class_init,
 };
-- 
2.39.2

[PATCH 12/12] docs/system: add a basic enumeration of vhost-user devices

[Alex Bennée]

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 docs/system/devices/vhost-user-rng.rst |  2 ++
 docs/system/devices/vhost-user.rst     | 41 ++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/docs/system/devices/vhost-user-rng.rst b/docs/system/devices/vhost-user-rng.rst
index a145d4105c..ead1405326 100644
--- a/docs/system/devices/vhost-user-rng.rst
+++ b/docs/system/devices/vhost-user-rng.rst
@@ -1,3 +1,5 @@
+.. _vhost_user_rng:
+
 QEMU vhost-user-rng - RNG emulation
 ===================================
 
diff --git a/docs/system/devices/vhost-user.rst b/docs/system/devices/vhost-user.rst
index 86128114fa..99b352823e 100644
--- a/docs/system/devices/vhost-user.rst
+++ b/docs/system/devices/vhost-user.rst
@@ -15,6 +15,47 @@ to the guest. The code is mostly boilerplate although each device has
 a ``chardev`` option which specifies the ID of the ``--chardev``
 device that connects via a socket to the vhost-user *daemon*.
 
+Each device will have an virtio-mmio and virtio-pci variant. See your
+platform details for what sort of virtio bus to use.
+
+.. list-table:: vhost-user devices
+  :widths: 20 20 60
+  :header-rows: 1
+
+  * - Device
+    - Type
+    - Notes
+  * - vhost-user-device
+    - Generic
+    - You must manually specify ``virtio-id`` and the correct ``num_vqs``
+  * - vhost-user-blk
+    - Block storage
+    -
+  * - vhost-user-fs
+    - File based storage driver
+    - See https://gitlab.com/virtio-fs/virtiofsd
+  * - vhost-user-scsi
+    - SCSI based storage
+    - See contrib/vhost-user/scsi
+  * - vhost-user-gpio
+    - Proxy gpio pins to host
+    - See https://github.com/rust-vmm/vhost-device
+  * - vhost-user-i2c
+    - Proxy i2c devices to host
+    - See https://github.com/rust-vmm/vhost-device
+  * - vhost-user-input
+    - Generic input driver
+    - See contrib/vhost-user-input
+  * - vhost-user-rng
+    - Entropy driver
+    - :ref:`vhost_user_rng`
+  * - vhost-user-gpu
+    - GPU driver
+    -
+  * - vhost-user-vsock
+    - Socket based communication
+    - See https://github.com/rust-vmm/vhost-device
+
 vhost-user daemon
 =================
 
-- 
2.39.2

Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Viresh Kumar]

On 14-04-23, 17:04, Alex Bennée wrote:
>  hw/virtio/vhost-user-device-pci.c      |  71 +++++
>  hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
>  hw/virtio/vhost-user-fs.c              |   4 +-
>  hw/virtio/vhost-user-gpio.c            | 405 +------------------------
>  hw/virtio/vhost-user-rng.c             | 264 +---------------

I wonder why isn't i2c removed as well ?

-- 
viresh

Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Alex Bennée]

Viresh Kumar <viresh.kumar@linaro.org> writes:

> On 14-04-23, 17:04, Alex Bennée wrote:
>>  hw/virtio/vhost-user-device-pci.c      |  71 +++++
>>  hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
>>  hw/virtio/vhost-user-fs.c              |   4 +-
>>  hw/virtio/vhost-user-gpio.c            | 405 +------------------------
>>  hw/virtio/vhost-user-rng.c             | 264 +---------------
>
> I wonder why isn't i2c removed as well ?

I will do - mainly I want to do a similar dummy device addition to the
i2c daemon for testing.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Stefan Hajnoczi]

On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> A lot of our vhost-user stubs are large chunks of boilerplate that do
> (mostly) the same thing. This series attempts to fix that by defining
> a new base class for vhost-user devices and then converting the rng
> and gpio devices to be based off them. You can even use
> vhost-user-device directly if you supply it with the right magic
> numbers (which is helpful for development).
>
> However the final patch runs into the weeds because I don't yet have a
> clean way to represent in QOM the fixing of certain properties for the
> specialised classes.
>
> The series is a net reduction in code and an increase in
> documentation but obviously needs to iron out a few more warts. I'm
> open to suggestions on the best way to tweak the QOM stuff.

--device vhost-user-device is not really possible because vhost-user
devices are not full VIRTIO devices. vhost-user devices depend on
device-specific code in the VMM by design.

The "subset of a VIRTIO device" design made sense for vhost_net.
Nowadays there are other device types that are close to full VIRTIO
devices, although the vhost-user protocol doesn't support the full
VIRTIO device lifecycle.

I think a user-creatable --device vhost-user-device is not a good idea
today. It creates confusion. Many people aren't aware of the
architectural difference between vhost-user and VIRTIO devices. The
result is that VMMs and vhost-user backends implement increasingly
brittle VIRTIO configuration space and feature bit logic as they
knowingly or unknowingly try to paper over the fact that a traditional
vhost-user device isn't a full VIRTIO device.

It is possible to resolve this difference and make --device
vhost-user-device work properly for devices that want to be full
VIRTIO devices. See "Making VMM device shims optional" here:
https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html

Even after extending the vhost-user protocol to solve the current
limitations, existing backends would still only be partial VIRTIO
devices that wouldn't work with --device vhost-user-device.

Reducing boilerplate is helpful, but I think --device
vhost-user-device should not be user-creatable.

Stefan

>
> Alex.
>
> Alex Bennée (12):
>   hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments
>   include/hw/virtio: document virtio_notify_config
>   include/hw/virtio: add kerneldoc for virtio_init
>   include/hw/virtio: document some more usage of notifiers
>   virtio: add generic vhost-user-device
>   virtio: add PCI stub for vhost-user-device
>   include: attempt to document device_class_set_props
>   qom: allow for properties to become "fixed"
>   hw/virtio: derive vhost-user-rng from vhost-user-device
>   hw/virtio: add config support to vhost-user-device
>   hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN)
>   docs/system: add a basic enumeration of vhost-user devices
>
>  docs/system/devices/vhost-user-rng.rst |   2 +
>  docs/system/devices/vhost-user.rst     |  41 +++
>  qapi/qom.json                          |   2 +
>  include/hw/qdev-core.h                 |   9 +
>  include/hw/virtio/vhost-user-device.h  |  33 ++
>  include/hw/virtio/vhost-user-gpio.h    |  23 +-
>  include/hw/virtio/vhost-user-rng.h     |  11 +-
>  include/hw/virtio/virtio.h             |  21 ++
>  include/qom/object.h                   |  16 +-
>  hw/display/vhost-user-gpu.c            |   4 +-
>  hw/net/virtio-net.c                    |   4 +-
>  hw/virtio/vhost-user-device-pci.c      |  71 +++++
>  hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
>  hw/virtio/vhost-user-fs.c              |   4 +-
>  hw/virtio/vhost-user-gpio.c            | 405 +------------------------
>  hw/virtio/vhost-user-rng.c             | 264 +---------------
>  hw/virtio/vhost-vsock-common.c         |   4 +-
>  hw/virtio/virtio-crypto.c              |   4 +-
>  qom/object.c                           |  14 +
>  qom/object_interfaces.c                |   9 +-
>  qom/qom-qmp-cmds.c                     |   1 +
>  softmmu/qdev-monitor.c                 |   1 +
>  hw/virtio/meson.build                  |   3 +
>  23 files changed, 613 insertions(+), 692 deletions(-)
>  create mode 100644 include/hw/virtio/vhost-user-device.h
>  create mode 100644 hw/virtio/vhost-user-device-pci.c
>  create mode 100644 hw/virtio/vhost-user-device.c
>
> --
> 2.39.2
>
>

Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Alex Bennée]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> A lot of our vhost-user stubs are large chunks of boilerplate that do
>> (mostly) the same thing. This series attempts to fix that by defining
>> a new base class for vhost-user devices and then converting the rng
>> and gpio devices to be based off them. You can even use
>> vhost-user-device directly if you supply it with the right magic
>> numbers (which is helpful for development).
>>
>> However the final patch runs into the weeds because I don't yet have a
>> clean way to represent in QOM the fixing of certain properties for the
>> specialised classes.
>>
>> The series is a net reduction in code and an increase in
>> documentation but obviously needs to iron out a few more warts. I'm
>> open to suggestions on the best way to tweak the QOM stuff.
>
> --device vhost-user-device is not really possible because vhost-user
> devices are not full VIRTIO devices. vhost-user devices depend on
> device-specific code in the VMM by design.

What device specific code? You certainly need to instantiate stuff in
the DTB/ACPI tables for -M virt but everything else can be handed off to
the vhost-user daemon.

Indeed the split brain is a bit silly in some places. For example is
QEMU really the best arbiter of a block device config when the actual
backend is a separate process. We have config passing in the vhost-user
spec.

> The "subset of a VIRTIO device" design made sense for vhost_net.
> Nowadays there are other device types that are close to full VIRTIO
> devices, although the vhost-user protocol doesn't support the full
> VIRTIO device lifecycle.

What are we missing?

> I think a user-creatable --device vhost-user-device is not a good idea
> today. It creates confusion. Many people aren't aware of the
> architectural difference between vhost-user and VIRTIO devices. The
> result is that VMMs and vhost-user backends implement increasingly
> brittle VIRTIO configuration space and feature bit logic as they
> knowingly or unknowingly try to paper over the fact that a traditional
> vhost-user device isn't a full VIRTIO device.

I've always found the device feature gating in QEMU confusing. Surely we
can rely on the daemon to properly enumerate the features it supports?

> It is possible to resolve this difference and make --device
> vhost-user-device work properly for devices that want to be full
> VIRTIO devices. See "Making VMM device shims optional" here:
> https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html
>
> Even after extending the vhost-user protocol to solve the current
> limitations, existing backends would still only be partial VIRTIO
> devices that wouldn't work with --device vhost-user-device.

It works for RNG, GPIO (and soon I'll be testing i2c and SCSI). All we
need is the virtioid and the number of virtio queues.

> Reducing boilerplate is helpful, but I think --device
> vhost-user-device should not be user-creatable.

After this series lands it will certainly make adding new shims easier
but having a vhost-user-device will make testing of new backends easier.
Can we not simply document it as an advanced feature for those who know
what they are doing? I'm not intending to deprecate the existing shims.

>
> Stefan
>
>>
>> Alex.
>>
>> Alex Bennée (12):
>>   hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments
>>   include/hw/virtio: document virtio_notify_config
>>   include/hw/virtio: add kerneldoc for virtio_init
>>   include/hw/virtio: document some more usage of notifiers
>>   virtio: add generic vhost-user-device
>>   virtio: add PCI stub for vhost-user-device
>>   include: attempt to document device_class_set_props
>>   qom: allow for properties to become "fixed"
>>   hw/virtio: derive vhost-user-rng from vhost-user-device
>>   hw/virtio: add config support to vhost-user-device
>>   hw/virtio: derive vhost-user-gpio from vhost-user-device (!BROKEN)
>>   docs/system: add a basic enumeration of vhost-user devices
>>
>>  docs/system/devices/vhost-user-rng.rst |   2 +
>>  docs/system/devices/vhost-user.rst     |  41 +++
>>  qapi/qom.json                          |   2 +
>>  include/hw/qdev-core.h                 |   9 +
>>  include/hw/virtio/vhost-user-device.h  |  33 ++
>>  include/hw/virtio/vhost-user-gpio.h    |  23 +-
>>  include/hw/virtio/vhost-user-rng.h     |  11 +-
>>  include/hw/virtio/virtio.h             |  21 ++
>>  include/qom/object.h                   |  16 +-
>>  hw/display/vhost-user-gpu.c            |   4 +-
>>  hw/net/virtio-net.c                    |   4 +-
>>  hw/virtio/vhost-user-device-pci.c      |  71 +++++
>>  hw/virtio/vhost-user-device.c          | 359 ++++++++++++++++++++++
>>  hw/virtio/vhost-user-fs.c              |   4 +-
>>  hw/virtio/vhost-user-gpio.c            | 405 +------------------------
>>  hw/virtio/vhost-user-rng.c             | 264 +---------------
>>  hw/virtio/vhost-vsock-common.c         |   4 +-
>>  hw/virtio/virtio-crypto.c              |   4 +-
>>  qom/object.c                           |  14 +
>>  qom/object_interfaces.c                |   9 +-
>>  qom/qom-qmp-cmds.c                     |   1 +
>>  softmmu/qdev-monitor.c                 |   1 +
>>  hw/virtio/meson.build                  |   3 +
>>  23 files changed, 613 insertions(+), 692 deletions(-)
>>  create mode 100644 include/hw/virtio/vhost-user-device.h
>>  create mode 100644 hw/virtio/vhost-user-device-pci.c
>>  create mode 100644 hw/virtio/vhost-user-device.c
>>
>> --
>> 2.39.2
>>
>>


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 00/12] virtio: add vhost-user-generic and reduce copy and paste

[Stefan Hajnoczi]

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

On Mon, Apr 17, 2023 at 05:14:59PM +0100, Alex Bennée wrote:
> 
> Stefan Hajnoczi <stefanha@gmail.com> writes:
> 
> > On Fri, 14 Apr 2023 at 12:06, Alex Bennée <alex.bennee@linaro.org> wrote:
> >>
> >> A lot of our vhost-user stubs are large chunks of boilerplate that do
> >> (mostly) the same thing. This series attempts to fix that by defining
> >> a new base class for vhost-user devices and then converting the rng
> >> and gpio devices to be based off them. You can even use
> >> vhost-user-device directly if you supply it with the right magic
> >> numbers (which is helpful for development).
> >>
> >> However the final patch runs into the weeds because I don't yet have a
> >> clean way to represent in QOM the fixing of certain properties for the
> >> specialised classes.
> >>
> >> The series is a net reduction in code and an increase in
> >> documentation but obviously needs to iron out a few more warts. I'm
> >> open to suggestions on the best way to tweak the QOM stuff.
> >
> > --device vhost-user-device is not really possible because vhost-user
> > devices are not full VIRTIO devices. vhost-user devices depend on
> > device-specific code in the VMM by design.
> 
> What device specific code? You certainly need to instantiate stuff in
> the DTB/ACPI tables for -M virt but everything else can be handed off to
> the vhost-user daemon.

There are vhost-user device types that lack functionality entirely, like
vhost-user-net's lack of the controlq virtqueue and configuration space.
It is not even possible to query the MAC address from a vhost-user-net
device. It's not a full virtio-net device and the VMM has to fill in the
gaps to emulate the missing virtqueues and configuration space.

There are device type-specific vhost-user messages like
VHOST_USER_SEND_RARP and VHOST_USER_CREATE_CRYPTO_SESSION that can't
really be supported by a generic --device vhost-user-device.

Live migration is another device-specific aspect that is handled by the
vhost-user frontend.

> Indeed the split brain is a bit silly in some places. For example is
> QEMU really the best arbiter of a block device config when the actual
> backend is a separate process. We have config passing in the vhost-user
> spec.

Optional vhost-user messages like configuration space access may be in
the spec, but existing device types cannot rely on them for backwards
compatibility reasons. Therefore most existing device types don't use
the configuration space. Those that do may only access parts of the
configuration space from the backend and emulate the rest.

The blog post I linked mentions a new protocol feature bit
(VHOST_USER_PROTOCOL_F_VDPA) to distinguish new vhost-user backends that
are full VIRTIO devices. This doesn't exist today, but I think something
like that is necessary to detect devices that will work with --device
vhost-user-device.

> > The "subset of a VIRTIO device" design made sense for vhost_net.
> > Nowadays there are other device types that are close to full VIRTIO
> > devices, although the vhost-user protocol doesn't support the full
> > VIRTIO device lifecycle.
> 
> What are we missing?

vhost-user needs:
- A GET_DEVICE_ID message.
- A GET_CONFIG_SIZE message. Today it is assumed that the vhost-user
  frontend already knows the configuration space size.
- A protocol feature bit indicating that the device is a full VIRTIO
  device. These devices also need to implement the SET_STATUS message,
  which is rarely implemented today.

> > I think a user-creatable --device vhost-user-device is not a good idea
> > today. It creates confusion. Many people aren't aware of the
> > architectural difference between vhost-user and VIRTIO devices. The
> > result is that VMMs and vhost-user backends implement increasingly
> > brittle VIRTIO configuration space and feature bit logic as they
> > knowingly or unknowingly try to paper over the fact that a traditional
> > vhost-user device isn't a full VIRTIO device.
> 
> I've always found the device feature gating in QEMU confusing. Surely we
> can rely on the daemon to properly enumerate the features it supports?

The backend's features cannot be passed through to the guest because
there is also an emulated VIRTIO transport (virtio-pci, virtio-mmio,
virtio-ccw) involved. The transport may support a different feature set
from the vhost-user backend. For example, the transport may support
Packed Virtqueues but the backend may not. So some filtering is
necessary in the VMM.

Since some of the device-specific functionality may be handled by the
VMM, then this extends beyond just the transport feature bits.

But I agree with you that it's ugly and complex. I have to re-read the
code every time because it works in a strange way.

> > It is possible to resolve this difference and make --device
> > vhost-user-device work properly for devices that want to be full
> > VIRTIO devices. See "Making VMM device shims optional" here:
> > https://blog.vmsplice.net/2020/09/on-unifying-vhost-user-and-virtio.html
> >
> > Even after extending the vhost-user protocol to solve the current
> > limitations, existing backends would still only be partial VIRTIO
> > devices that wouldn't work with --device vhost-user-device.
> 
> It works for RNG, GPIO (and soon I'll be testing i2c and SCSI). All we
> need is the virtioid and the number of virtio queues.

There is already a vhost-user message for querying the number of
virtqueues: VHOST_USER_GET_QUEUE_NUM.

See above for other missing stuff.

> > Reducing boilerplate is helpful, but I think --device
> > vhost-user-device should not be user-creatable.
> 
> After this series lands it will certainly make adding new shims easier
> but having a vhost-user-device will make testing of new backends easier.
> Can we not simply document it as an advanced feature for those who know
> what they are doing? I'm not intending to deprecate the existing shims.

If you add the missing pieces to vhost-user then --device
vhost-user-device can be done properly. Two messages and one protocol
feature bit are required, it's not that bad.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]