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

[PATCH v2 00/13] 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 (vhost-user-base) which is used by a generic
vhost-user-device implementation. Then the rng, gpio and i2c
vhost-user devices become simple specialisations of the common base
defining the ID, number of queues and potentially the config handling.

In theory we could convert the rest of the vhost-user stubs but there
are complications caused by the config being split between the daemon
and QEMU. For example:

 -device vhost-user-device-pci,chardev=vus,virtio-id=8,num_vqs=3,config_size=36

works with the WIP vhost-user-scsi backend:

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

but the concrete vhost-user-scsi-pci device fails because it expects
to handle config via the command line. You will see the report:

  qemu-system-aarch64: -device vhost-user-scsi-pci,chardev=vus:
  warning: vhost-user backend supports VHOST_USER_PROTOCOL_F_CONFIG
  but QEMU does not.

if you try. We could make the device a bit smarter but then we would
need to untangle the vhost_scsi_common_() logic which is shared with
the pure in kernel vhost implementation. The vhost-user-vsock stub
might be another one worth re-factoring although that has a similar
split architecture.

The overall diffstat shows a net deletion of code as well as
introducing some more documentation and moving the stubs into the
common build, further reducing redundancy.

Next Steps
----------

From Stefan's last email to the v1 posting we need:

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.

and implementing the VHOST_USER_GET_QUEUE_NUM and SET_STATUS messages
to make the generic device "self configuring".

Alex.

Alex Bennée (13):
  include: attempt to document device_class_set_props
  include/hw: document the device_class_set_parent_* fns
  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 vhost-user-base and a generic vhost-user-device
  virtio: add PCI stub for vhost-user-device
  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
  hw/virtio: derive vhost-user-i2c from vhost-user-base
  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 +++
 include/hw/qdev-core.h                 |  36 +++
 include/hw/virtio/vhost-user-device.h  |  46 +++
 include/hw/virtio/vhost-user-gpio.h    |  23 +-
 include/hw/virtio/vhost-user-i2c.h     |  18 +-
 include/hw/virtio/vhost-user-rng.h     |  11 +-
 include/hw/virtio/virtio.h             |  21 ++
 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          | 380 +++++++++++++++++++++++
 hw/virtio/vhost-user-fs.c              |   4 +-
 hw/virtio/vhost-user-gpio.c            | 400 ++-----------------------
 hw/virtio/vhost-user-i2c.c             | 255 +---------------
 hw/virtio/vhost-user-rng.c             | 277 ++---------------
 hw/virtio/vhost-vsock-common.c         |   4 +-
 hw/virtio/virtio-crypto.c              |   4 +-
 hw/virtio/meson.build                  |  20 +-
 19 files changed, 686 insertions(+), 935 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 v2 01/13] 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

Re: [PATCH v2 01/13] include: attempt to document device_class_set_props

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

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

Can we say that the use case description above is now obsolete with the introduction 
of the abstract VHOST_USER_BASE class (i.e. it was a modelling issue)? If so, I think 
the comment is good, except that the last sentence can be removed.


ATB,

Mark.

Re: [PATCH v2 01/13] include: attempt to document device_class_set_props

[Stefan Hajnoczi]

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

On Tue, Apr 18, 2023 at 05:21:28PM +0100, Alex Bennée wrote:
> 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.

Does this mean this patch is an RFC and this patch is not intended to be
merged?

> 
> 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);

I don't know the answer. There doesn't seem to be a way for child
classes to override parent DeviceClass properties. The assumption is the
sets of properties are disjoint (no property name collisions).

Here is a workaround in the vhost-user-rng code:

  /* Set our default if the user didn't specify the id */
  if (vud->id == 0) {
      vud->id = VIRTIO_ID_RNG;
  }

This could be a problem because the value 0 may be valid and there is no
way to distinguish between a user setting 0 and the default 0 value.

Stefan

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

[PATCH v2 02/13] include/hw: document the device_class_set_parent_* fns

[Alex Bennée]

These are useful functions for when you want proper inheritance of
functionality across realize/unrealize calls.

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

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index d4bbc30c92..b1d194b561 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -795,9 +795,36 @@ void device_class_set_props(DeviceClass *dc, Property *props);
 void device_class_set_parent_reset(DeviceClass *dc,
                                    DeviceReset dev_reset,
                                    DeviceReset *parent_reset);
+
+/**
+ * device_class_set_parent_realize(): set up for chaining realize fns
+ * @dc: The device class
+ * @dev_realize: the device realize function
+ * @parent_realize: somewhere to save the parents realize function
+ *
+ * This is intended to be used when the new realize function will
+ * eventually call its parent realization function during creation.
+ * This requires storing the function call somewhere (usually in the
+ * instance structure) so you can eventually call:
+ *   my_dev->parent_realize(dev, errp);
+ */
 void device_class_set_parent_realize(DeviceClass *dc,
                                      DeviceRealize dev_realize,
                                      DeviceRealize *parent_realize);
+
+
+/**
+ * device_class_set_parent_unrealize(): set up for chaining unrealize fns
+ * @dc: The device class
+ * @dev_unrealize: the device realize function
+ * @parent_unrealize: somewhere to save the parents unrealize function
+ *
+ * This is intended to be used when the new unrealize function will
+ * eventually call its parent unrealization function during the
+ * unrealize phase. This requires storing the function call somewhere
+ * (usually in the instance structure) so you can eventually call:
+ *   my_dev->parent_unrealize(dev);
+ */
 void device_class_set_parent_unrealize(DeviceClass *dc,
                                        DeviceUnrealize dev_unrealize,
                                        DeviceUnrealize *parent_unrealize);
-- 
2.39.2

Re: [PATCH v2 02/13] include/hw: document the device_class_set_parent_* fns

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> These are useful functions for when you want proper inheritance of
> functionality across realize/unrealize calls.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   include/hw/qdev-core.h | 27 +++++++++++++++++++++++++++
>   1 file changed, 27 insertions(+)
> 
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d4bbc30c92..b1d194b561 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -795,9 +795,36 @@ void device_class_set_props(DeviceClass *dc, Property *props);
>   void device_class_set_parent_reset(DeviceClass *dc,
>                                      DeviceReset dev_reset,
>                                      DeviceReset *parent_reset);
> +
> +/**
> + * device_class_set_parent_realize(): set up for chaining realize fns
> + * @dc: The device class
> + * @dev_realize: the device realize function
> + * @parent_realize: somewhere to save the parents realize function
> + *
> + * This is intended to be used when the new realize function will
> + * eventually call its parent realization function during creation.
> + * This requires storing the function call somewhere (usually in the
> + * instance structure) so you can eventually call:

I think this should be the class structure, since it is the only possible location 
for the parent realize() function to exist given that these functions are called from 
the .class_init function.

> + *   my_dev->parent_realize(dev, errp);
> + */
>   void device_class_set_parent_realize(DeviceClass *dc,
>                                        DeviceRealize dev_realize,
>                                        DeviceRealize *parent_realize);
> +
> +
> +/**
> + * device_class_set_parent_unrealize(): set up for chaining unrealize fns
> + * @dc: The device class
> + * @dev_unrealize: the device realize function
> + * @parent_unrealize: somewhere to save the parents unrealize function
> + *
> + * This is intended to be used when the new unrealize function will
> + * eventually call its parent unrealization function during the
> + * unrealize phase. This requires storing the function call somewhere
> + * (usually in the instance structure) so you can eventually call:
> + *   my_dev->parent_unrealize(dev);

And same here of course.

> + */
>   void device_class_set_parent_unrealize(DeviceClass *dc,
>                                          DeviceUnrealize dev_unrealize,
>                                          DeviceUnrealize *parent_unrealize);


ATB,

Mark.

[PATCH v2 03/13] 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

Re: [PATCH v2 03/13] hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> 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
>        */

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v2 03/13] hw/virtio: fix typo in VIRTIO_CONFIG_IRQ_IDX comments

[Stefan Hajnoczi]

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

On Tue, Apr 18, 2023 at 05:21:30PM +0100, Alex Bennée wrote:
> 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
>       */

The entire comment could be rewritten to make the punctuation and
grammar correct, if you want.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH v2 04/13] 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

Re: [PATCH v2 04/13] include/hw/virtio: document virtio_notify_config

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> 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);

I can't 100% vouch for the correctness of the description, however it makes sense to 
me so:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v2 04/13] include/hw/virtio: document virtio_notify_config

[Stefan Hajnoczi]

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

On Tue, Apr 18, 2023 at 05:21:31PM +0100, Alex Bennée wrote:
> 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

The spec calls this sending a Configuration Change Notification
(https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html#x1-1110001).
I suggest using VIRTIO specification terminology, otherwise every person
paraphrases the spec and you have to guess how QEMU code/comments map to
the spec.

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

[PATCH v2 05/13] 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

Re: [PATCH v2 05/13] include/hw/virtio: add kerneldoc for virtio_init

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> 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);

Reviewed-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v2 05/13] include/hw/virtio: add kerneldoc for virtio_init

[Stefan Hajnoczi]

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

On Tue, Apr 18, 2023 at 05:21:32PM +0100, Alex Bennée wrote:
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  include/hw/virtio/virtio.h | 6 ++++++
>  1 file changed, 6 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[PATCH v2 06/13] 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

Re: [PATCH v2 06/13] include/hw/virtio: document some more usage of notifiers

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> 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.

Typo: @use_guest_notifier_mask

> +     * 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;e 
>   };

As before, I'm not overly familiar with the virtio code but the description makes 
sense to me so:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [PATCH v2 06/13] include/hw/virtio: document some more usage of notifiers

[Stefan Hajnoczi]

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

On Tue, Apr 18, 2023 at 05:21:33PM +0100, Alex Bennée wrote:
> 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.

s/user_/use_/

> +     * This is used to suppress the masking of guest updates for
> +     * vhost-user devices which are asynchronous by design.

What is the exact reason why masking is not supported by vhost-user?

Only vhost-user-net and vhost-user-crypto set use_guest_notifier_mask to
false. Do the other vhost-user devices need to set it to false too?

> +     */
>      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

Using VIRTIO spec terminology:
"the event notifier that sends Configuration Change Notifications"

> +     */
>      EventNotifier config_notifier;
>  };
>  
> -- 
> 2.39.2
> 

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

[PATCH v2 07/13] virtio: add vhost-user-base and a 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-base QOM
object and a derived 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>

---
v2
  - split into vub and vud
---
 include/hw/virtio/vhost-user-device.h |  45 ++++
 hw/virtio/vhost-user-device.c         | 324 ++++++++++++++++++++++++++
 hw/virtio/meson.build                 |   2 +
 3 files changed, 371 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..9105011e25
--- /dev/null
+++ b/include/hw/virtio/vhost-user-device.h
@@ -0,0 +1,45 @@
+/*
+ * 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_BASE "vhost-user-base"
+
+OBJECT_DECLARE_TYPE(VHostUserBase, VHostUserBaseClass, VHOST_USER_BASE)
+
+struct VHostUserBase {
+    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;
+};
+
+    /* needed so we can use the base realize after specialisation
+       tweaks */
+struct VHostUserBaseClass {
+    /*< private >*/
+    VirtioDeviceClass parent_class;
+    /*< public >*/
+    DeviceRealize parent_realize;
+};
+
+/* shared for the benefit of the derived pci class */
+#define TYPE_VHOST_USER_DEVICE "vhost-user-device"
+
+#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..b0239fa033
--- /dev/null
+++ b/hw/virtio/vhost-user-device.c
@@ -0,0 +1,324 @@
+/*
+ * 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 vub_start(VirtIODevice *vdev)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+    int ret, i;
+
+    if (!k->set_guest_notifiers) {
+        error_report("binding does not support guest notifiers");
+        return;
+    }
+
+    ret = vhost_dev_enable_notifiers(&vub->vhost_dev, vdev);
+    if (ret < 0) {
+        error_report("Error enabling host notifiers: %d", -ret);
+        return;
+    }
+
+    ret = k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, true);
+    if (ret < 0) {
+        error_report("Error binding guest notifier: %d", -ret);
+        goto err_host_notifiers;
+    }
+
+    vub->vhost_dev.acked_features = vdev->guest_features;
+
+    ret = vhost_dev_start(&vub->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 < vub->vhost_dev.nvqs; i++) {
+        vhost_virtqueue_mask(&vub->vhost_dev, vdev, i, false);
+    }
+
+    return;
+
+err_guest_notifiers:
+    k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, false);
+err_host_notifiers:
+    vhost_dev_disable_notifiers(&vub->vhost_dev, vdev);
+}
+
+static void vub_stop(VirtIODevice *vdev)
+{
+    VHostUserBase *vub = VHOST_USER_BASE(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(&vub->vhost_dev, vdev, true);
+
+    ret = k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, false);
+    if (ret < 0) {
+        error_report("vhost guest notifier cleanup failed: %d", ret);
+        return;
+    }
+
+    vhost_dev_disable_notifiers(&vub->vhost_dev, vdev);
+}
+
+static void vub_set_status(VirtIODevice *vdev, uint8_t status)
+{
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+    bool should_start = virtio_device_should_start(vdev, status);
+
+    if (vhost_dev_is_started(&vub->vhost_dev) == should_start) {
+        return;
+    }
+
+    if (should_start) {
+        vub_start(vdev);
+    } else {
+        vub_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 vub_get_features(VirtIODevice *vdev,
+                                 uint64_t requested_features, Error **errp)
+{
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+    /* This should be set when the vhost connection initialises */
+    g_assert(vub->vhost_dev.features);
+    return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+}
+
+static void vub_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, VHostUserBase *vub)
+{
+    vhost_user_cleanup(&vub->vhost_user);
+
+    for (int i = 0; i < vub->num_vqs; i++) {
+        VirtQueue *vq = g_ptr_array_index(vub->vqs, i);
+        virtio_delete_queue(vq);
+    }
+
+    virtio_cleanup(vdev);
+}
+
+static int vub_connect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+
+    if (vub->connected) {
+        return 0;
+    }
+    vub->connected = true;
+
+    /* restore vhost state */
+    if (virtio_device_started(vdev, vdev->status)) {
+        vub_start(vdev);
+    }
+
+    return 0;
+}
+
+static void vub_disconnect(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+
+    if (!vub->connected) {
+        return;
+    }
+    vub->connected = false;
+
+    if (vhost_dev_is_started(&vub->vhost_dev)) {
+        vub_stop(vdev);
+    }
+}
+
+static void vub_event(void *opaque, QEMUChrEvent event)
+{
+    DeviceState *dev = opaque;
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBase *vub = VHOST_USER_BASE(vdev);
+
+    switch (event) {
+    case CHR_EVENT_OPENED:
+        if (vub_connect(dev) < 0) {
+            qemu_chr_fe_disconnect(&vub->chardev);
+            return;
+        }
+        break;
+    case CHR_EVENT_CLOSED:
+        vub_disconnect(dev);
+        break;
+    case CHR_EVENT_BREAK:
+    case CHR_EVENT_MUX_IN:
+    case CHR_EVENT_MUX_OUT:
+        /* Ignore */
+        break;
+    }
+}
+
+static void vub_device_realize(DeviceState *dev, Error **errp)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBase *vub = VHOST_USER_BASE(dev);
+    int ret;
+
+    if (!vub->chardev.chr) {
+        error_setg(errp, "vhost-user-device: missing chardev");
+        return;
+    }
+
+    if (!vub->virtio_id) {
+        error_setg(errp, "vhost-user-device: need to define device id");
+        return;
+    }
+
+    if (!vub->num_vqs) {
+        vub->num_vqs = 1; /* reasonable default? */
+    }
+
+    if (!vhost_user_init(&vub->vhost_user, &vub->chardev, errp)) {
+        return;
+    }
+
+    virtio_init(vdev, vub->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 */
+    vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
+    for (int i = 0; i < vub->num_vqs; i++) {
+        g_ptr_array_add(vub->vqs,
+                        virtio_add_queue(vdev, 4, vub_handle_output));
+    }
+
+    vub->vhost_dev.nvqs = vub->num_vqs;
+    vub->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vub->vhost_dev.nvqs);
+
+    /* connect to backend */
+    ret = vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,
+                         VHOST_BACKEND_TYPE_USER, 0, errp);
+
+    if (ret < 0) {
+        do_vhost_user_cleanup(vdev, vub);
+    }
+
+    qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
+                             dev, NULL, true);
+}
+
+static void vub_device_unrealize(DeviceState *dev)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
+    VHostUserBase *vub = VHOST_USER_BASE(dev);
+    struct vhost_virtqueue *vhost_vqs = vub->vhost_dev.vqs;
+
+    /* This will stop vhost backend if appropriate. */
+    vub_set_status(vdev, 0);
+    vhost_dev_cleanup(&vub->vhost_dev);
+    g_free(vhost_vqs);
+    do_vhost_user_cleanup(vdev, vub);
+}
+
+static void vub_class_init(ObjectClass *klass, void *data)
+{
+    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+
+    vdc->realize = vub_device_realize;
+    vdc->unrealize = vub_device_unrealize;
+    vdc->get_features = vub_get_features;
+    vdc->set_status = vub_set_status;
+}
+
+static const TypeInfo vub_info = {
+    .name = TYPE_VHOST_USER_BASE,
+    .parent = TYPE_VIRTIO_DEVICE,
+    .instance_size = sizeof(VHostUserBase),
+    .class_init = vub_class_init,
+    .class_size = sizeof(VHostUserBaseClass),
+    .abstract = true
+};
+
+
+/*
+ * The following is a concrete implementation of the base class which
+ * allows the user to define the key parameters via the command line.
+ */
+
+static const VMStateDescription vud_vmstate = {
+    .name = "vhost-user-device",
+    .unmigratable = 1,
+};
+
+static Property vud_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
+    DEFINE_PROP_UINT16("virtio-id", VHostUserBase, virtio_id, 0),
+    DEFINE_PROP_UINT32("num_vqs", VHostUserBase, num_vqs, 1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void vud_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, vud_properties);
+    dc->vmsd = &vud_vmstate;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+}
+
+static const TypeInfo vud_info = {
+    .name = TYPE_VHOST_USER_DEVICE,
+    .parent = TYPE_VHOST_USER_BASE,
+    .instance_size = sizeof(VHostUserBase),
+    .class_init = vud_class_init,
+    .class_size = sizeof(VHostUserBaseClass),
+};
+
+static void vu_register_types(void)
+{
+    type_register_static(&vub_info);
+    type_register_static(&vud_info);
+}
+
+type_init(vu_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

Re: [PATCH v2 07/13] virtio: add vhost-user-base and a generic vhost-user-device

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> In theory we shouldn't need to repeat so much boilerplate to support
> vhost-user backends. This provides a generic vhost-user-base QOM
> object and a derived 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>
> 
> ---
> v2
>    - split into vub and vud
> ---
>   include/hw/virtio/vhost-user-device.h |  45 ++++
>   hw/virtio/vhost-user-device.c         | 324 ++++++++++++++++++++++++++
>   hw/virtio/meson.build                 |   2 +
>   3 files changed, 371 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..9105011e25
> --- /dev/null
> +++ b/include/hw/virtio/vhost-user-device.h
> @@ -0,0 +1,45 @@
> +/*
> + * 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_BASE "vhost-user-base"
> +
> +OBJECT_DECLARE_TYPE(VHostUserBase, VHostUserBaseClass, VHOST_USER_BASE)
> +
> +struct VHostUserBase {
> +    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;
> +};

Boring style point: we should ensure that for QOM object definitions there should be 
a newline after the first member, and the first member is always parent_obj e.g.

struct VHostUserBase {
     VirtIODevice parent_obj;

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

This is good for 2 reasons: firstly it screams "this struct is a QOM object" (and 
therefore there should be an OBJECT_DECLARE_*_TYPE() macro nearby, and secondly by 
isolating the parent and making it obvious it is easier for reviewers to understand 
the class hierarchy, and indeed forces the developer to think about what the parent 
should actually be.

Unfortunately it seems that exceptions to this rule have started to creep it, but I 
think it is useful to keep this.

> +    /* needed so we can use the base realize after specialisation
> +       tweaks */
> +struct VHostUserBaseClass {
> +    /*< private >*/
> +    VirtioDeviceClass parent_class;
> +    /*< public >*/
> +    DeviceRealize parent_realize;
> +};

Same here for VHostUserBaseClass except the first member should always be 
parent_class i.e.

struct VHostUserBaseClass {
     VirtioDeviceClass parent_class;

     DeviceRealize parent_realize;
};

Anthony's original documentation made use of the "/*< private >*/" and /*< public >*/ 
comments, but IMO this doesn't really mean anything and they should be removed to 
avoid confusion. If an object member is externally accessible outside of the 
hierarchy then it should simply be declared as a property.

> +/* shared for the benefit of the derived pci class */
> +#define TYPE_VHOST_USER_DEVICE "vhost-user-device"
> +
> +#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..b0239fa033
> --- /dev/null
> +++ b/hw/virtio/vhost-user-device.c
> @@ -0,0 +1,324 @@
> +/*
> + * 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 vub_start(VirtIODevice *vdev)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +    int ret, i;
> +
> +    if (!k->set_guest_notifiers) {
> +        error_report("binding does not support guest notifiers");
> +        return;
> +    }
> +
> +    ret = vhost_dev_enable_notifiers(&vub->vhost_dev, vdev);
> +    if (ret < 0) {
> +        error_report("Error enabling host notifiers: %d", -ret);
> +        return;
> +    }
> +
> +    ret = k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, true);
> +    if (ret < 0) {
> +        error_report("Error binding guest notifier: %d", -ret);
> +        goto err_host_notifiers;
> +    }
> +
> +    vub->vhost_dev.acked_features = vdev->guest_features;
> +
> +    ret = vhost_dev_start(&vub->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 < vub->vhost_dev.nvqs; i++) {
> +        vhost_virtqueue_mask(&vub->vhost_dev, vdev, i, false);
> +    }
> +
> +    return;
> +
> +err_guest_notifiers:
> +    k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, false);
> +err_host_notifiers:
> +    vhost_dev_disable_notifiers(&vub->vhost_dev, vdev);
> +}
> +
> +static void vub_stop(VirtIODevice *vdev)
> +{
> +    VHostUserBase *vub = VHOST_USER_BASE(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(&vub->vhost_dev, vdev, true);
> +
> +    ret = k->set_guest_notifiers(qbus->parent, vub->vhost_dev.nvqs, false);
> +    if (ret < 0) {
> +        error_report("vhost guest notifier cleanup failed: %d", ret);
> +        return;
> +    }
> +
> +    vhost_dev_disable_notifiers(&vub->vhost_dev, vdev);
> +}
> +
> +static void vub_set_status(VirtIODevice *vdev, uint8_t status)
> +{
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +    bool should_start = virtio_device_should_start(vdev, status);
> +
> +    if (vhost_dev_is_started(&vub->vhost_dev) == should_start) {
> +        return;
> +    }
> +
> +    if (should_start) {
> +        vub_start(vdev);
> +    } else {
> +        vub_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 vub_get_features(VirtIODevice *vdev,
> +                                 uint64_t requested_features, Error **errp)
> +{
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +    /* This should be set when the vhost connection initialises */
> +    g_assert(vub->vhost_dev.features);
> +    return vub->vhost_dev.features & ~(1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> +}
> +
> +static void vub_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, VHostUserBase *vub)
> +{
> +    vhost_user_cleanup(&vub->vhost_user);
> +
> +    for (int i = 0; i < vub->num_vqs; i++) {
> +        VirtQueue *vq = g_ptr_array_index(vub->vqs, i);
> +        virtio_delete_queue(vq);
> +    }
> +
> +    virtio_cleanup(vdev);
> +}
> +
> +static int vub_connect(DeviceState *dev)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +
> +    if (vub->connected) {
> +        return 0;
> +    }
> +    vub->connected = true;
> +
> +    /* restore vhost state */
> +    if (virtio_device_started(vdev, vdev->status)) {
> +        vub_start(vdev);
> +    }
> +
> +    return 0;
> +}
> +
> +static void vub_disconnect(DeviceState *dev)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +
> +    if (!vub->connected) {
> +        return;
> +    }
> +    vub->connected = false;
> +
> +    if (vhost_dev_is_started(&vub->vhost_dev)) {
> +        vub_stop(vdev);
> +    }
> +}
> +
> +static void vub_event(void *opaque, QEMUChrEvent event)
> +{
> +    DeviceState *dev = opaque;
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBase *vub = VHOST_USER_BASE(vdev);
> +
> +    switch (event) {
> +    case CHR_EVENT_OPENED:
> +        if (vub_connect(dev) < 0) {
> +            qemu_chr_fe_disconnect(&vub->chardev);
> +            return;
> +        }
> +        break;
> +    case CHR_EVENT_CLOSED:
> +        vub_disconnect(dev);
> +        break;
> +    case CHR_EVENT_BREAK:
> +    case CHR_EVENT_MUX_IN:
> +    case CHR_EVENT_MUX_OUT:
> +        /* Ignore */
> +        break;
> +    }
> +}
> +
> +static void vub_device_realize(DeviceState *dev, Error **errp)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBase *vub = VHOST_USER_BASE(dev);
> +    int ret;
> +
> +    if (!vub->chardev.chr) {
> +        error_setg(errp, "vhost-user-device: missing chardev");
> +        return;
> +    }
> +
> +    if (!vub->virtio_id) {
> +        error_setg(errp, "vhost-user-device: need to define device id");
> +        return;
> +    }
> +
> +    if (!vub->num_vqs) {
> +        vub->num_vqs = 1; /* reasonable default? */
> +    }
> +
> +    if (!vhost_user_init(&vub->vhost_user, &vub->chardev, errp)) {
> +        return;
> +    }
> +
> +    virtio_init(vdev, vub->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 */
> +    vub->vqs = g_ptr_array_sized_new(vub->num_vqs);
> +    for (int i = 0; i < vub->num_vqs; i++) {
> +        g_ptr_array_add(vub->vqs,
> +                        virtio_add_queue(vdev, 4, vub_handle_output));
> +    }
> +
> +    vub->vhost_dev.nvqs = vub->num_vqs;
> +    vub->vhost_dev.vqs = g_new0(struct vhost_virtqueue, vub->vhost_dev.nvqs);
> +
> +    /* connect to backend */
> +    ret = vhost_dev_init(&vub->vhost_dev, &vub->vhost_user,
> +                         VHOST_BACKEND_TYPE_USER, 0, errp);
> +
> +    if (ret < 0) {
> +        do_vhost_user_cleanup(vdev, vub);
> +    }
> +
> +    qemu_chr_fe_set_handlers(&vub->chardev, NULL, NULL, vub_event, NULL,
> +                             dev, NULL, true);
> +}
> +
> +static void vub_device_unrealize(DeviceState *dev)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VHostUserBase *vub = VHOST_USER_BASE(dev);
> +    struct vhost_virtqueue *vhost_vqs = vub->vhost_dev.vqs;
> +
> +    /* This will stop vhost backend if appropriate. */
> +    vub_set_status(vdev, 0);
> +    vhost_dev_cleanup(&vub->vhost_dev);
> +    g_free(vhost_vqs);
> +    do_vhost_user_cleanup(vdev, vub);
> +}
> +
> +static void vub_class_init(ObjectClass *klass, void *data)
> +{
> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> +    vdc->realize = vub_device_realize;
> +    vdc->unrealize = vub_device_unrealize;
> +    vdc->get_features = vub_get_features;
> +    vdc->set_status = vub_set_status;
> +}
> +
> +static const TypeInfo vub_info = {
> +    .name = TYPE_VHOST_USER_BASE,
> +    .parent = TYPE_VIRTIO_DEVICE,
> +    .instance_size = sizeof(VHostUserBase),
> +    .class_init = vub_class_init,
> +    .class_size = sizeof(VHostUserBaseClass),
> +    .abstract = true
> +};
> +
> +
> +/*
> + * The following is a concrete implementation of the base class which
> + * allows the user to define the key parameters via the command line.
> + */
> +
> +static const VMStateDescription vud_vmstate = {
> +    .name = "vhost-user-device",
> +    .unmigratable = 1,
> +};
> +
> +static Property vud_properties[] = {
> +    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
> +    DEFINE_PROP_UINT16("virtio-id", VHostUserBase, virtio_id, 0),
> +    DEFINE_PROP_UINT32("num_vqs", VHostUserBase, num_vqs, 1),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vud_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    device_class_set_props(dc, vud_properties);
> +    dc->vmsd = &vud_vmstate;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> +}
> +
> +static const TypeInfo vud_info = {
> +    .name = TYPE_VHOST_USER_DEVICE,
> +    .parent = TYPE_VHOST_USER_BASE,
> +    .instance_size = sizeof(VHostUserBase),
> +    .class_init = vud_class_init,
> +    .class_size = sizeof(VHostUserBaseClass),
> +};

If you like you can drop the .instance_size and .class_size properties here since 
they are inherited from the parent type by default.

> +static void vu_register_types(void)
> +{
> +    type_register_static(&vub_info);
> +    type_register_static(&vud_info);
> +}
> +
> +type_init(vu_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'))


ATB,

Mark.

[PATCH v2 08/13] 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>
Tested-by: Erik Schilling <erik.schilling@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..41f9b7905b
--- /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;
+    VHostUserBase vub;
+};
+
+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->vub);
+
+    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->vub, sizeof(dev->vub),
+                                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 v2 08/13] virtio: add PCI stub for vhost-user-device

[Mark Cave-Ayland]

On 18/04/2023 17:21, 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>
> ---
>   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..41f9b7905b
> --- /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;
> +    VHostUserBase vub;
> +};

Spacing here, as mentioned in my previous email.

> +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)

You should be able to drop the typedef here and replace DECLARE_INSTANCE_CHECKER with 
OBJECT_DECLARE_SIMPLE_TYPE.

> +static void vhost_user_device_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> +    VHostUserDevicePCI *dev = VHOST_USER_DEVICE_PCI(vpci_dev);
> +    DeviceState *vdev = DEVICE(&dev->vub);
> +
> +    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);

Minor nit: an empty line after the class casts here makes it much easier to read.

> +    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->vub, sizeof(dev->vub),
> +                                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'))


ATB,

Mark.

[PATCH v2 09/13] 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. Also as this
doesn't require any target specific hacks we only need to build the
stubs once.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - new derivation layout
  - move directly to softmmu_virtio_ss
---
 include/hw/virtio/vhost-user-rng.h |  11 +-
 hw/virtio/vhost-user-rng.c         | 277 +++--------------------------
 hw/virtio/meson.build              |   7 +-
 3 files changed, 28 insertions(+), 267 deletions(-)

diff --git a/include/hw/virtio/vhost-user-rng.h b/include/hw/virtio/vhost-user-rng.h
index ddd9f01eea..13139c0d9d 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;
-
+    VHostUserBase parent;
     /*< public >*/
 };
 
diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
index efc54cd3fb..71d3991f93 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,46 @@
 #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),
+static Property vrng_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void vu_rng_base_realize(DeviceState *dev, Error **errp)
+{
+    VHostUserBase *vub = VHOST_USER_BASE(dev);
+    VHostUserBaseClass *vubs = VHOST_USER_BASE_GET_CLASS(dev);
+
+    /* Fixed for RNG */
+    vub->virtio_id = VIRTIO_ID_RNG;
+    vub->num_vqs = 1;
+
+    vubs->parent_realize(dev, errp);
+}
+
 static void vu_rng_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
 
-    device_class_set_props(dc, vu_rng_properties);
     dc->vmsd = &vu_rng_vmstate;
-    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
+    device_class_set_props(dc, vrng_properties);
+    device_class_set_parent_realize(dc, vu_rng_base_realize,
+                                    &vubc->parent_realize);
 
-    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;
+    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
 
 static const TypeInfo vu_rng_info = {
     .name = TYPE_VHOST_USER_RNG,
-    .parent = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_VHOST_USER_BASE,
     .instance_size = sizeof(VHostUserRNG),
     .class_init = vu_rng_class_init,
 };
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index c0a86b94ae..de442dcb96 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -3,6 +3,11 @@ softmmu_virtio_ss.add(files('virtio-bus.c'))
 softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: files('virtio-pci.c'))
 softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c'))
 
+# VirtIO stubs which don't need building per-guest
+softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
+softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
+                      if_true: files('vhost-user-rng-pci.c'))
+
 specific_virtio_ss = ss.source_set()
 specific_virtio_ss.add(files('virtio.c'))
 specific_virtio_ss.add(files('virtio-config-io.c', 'virtio-qmp.c'))
@@ -32,7 +37,6 @@ specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c')
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
-specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
 specific_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
@@ -43,7 +47,6 @@ virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vs
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true: files('vhost-user-input-pci.c'))
-virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_SCSI', if_true: files('vhost-user-scsi-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-pci.c'))
-- 
2.39.2

Re: [PATCH v2 09/13] hw/virtio: derive vhost-user-rng from vhost-user-device

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> Now we can take advantage of our new base class and make
> vhost-user-rng a much simpler boilerplate wrapper. Also as this
> doesn't require any target specific hacks we only need to build the
> stubs once.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - new derivation layout
>    - move directly to softmmu_virtio_ss
> ---
>   include/hw/virtio/vhost-user-rng.h |  11 +-
>   hw/virtio/vhost-user-rng.c         | 277 +++--------------------------
>   hw/virtio/meson.build              |   7 +-
>   3 files changed, 28 insertions(+), 267 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost-user-rng.h b/include/hw/virtio/vhost-user-rng.h
> index ddd9f01eea..13139c0d9d 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;
> -
> +    VHostUserBase parent;
>       /*< public >*/
>   };
>   
> diff --git a/hw/virtio/vhost-user-rng.c b/hw/virtio/vhost-user-rng.c
> index efc54cd3fb..71d3991f93 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,46 @@
>   #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),
> +static Property vrng_properties[] = {
> +    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),

I did a double-take that this wouldn't have to be VHostUserRNG, but I think it should 
still work with the new object hierarchy.

>       DEFINE_PROP_END_OF_LIST(),
>   };
>   
> +static void vu_rng_base_realize(DeviceState *dev, Error **errp)
> +{
> +    VHostUserBase *vub = VHOST_USER_BASE(dev);
> +    VHostUserBaseClass *vubs = VHOST_USER_BASE_GET_CLASS(dev);
> +
> +    /* Fixed for RNG */
> +    vub->virtio_id = VIRTIO_ID_RNG;
> +    vub->num_vqs = 1;
> +
> +    vubs->parent_realize(dev, errp);
> +}
> +
>   static void vu_rng_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> -    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
>   
> -    device_class_set_props(dc, vu_rng_properties);
>       dc->vmsd = &vu_rng_vmstate;
> -    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> +    device_class_set_props(dc, vrng_properties);
> +    device_class_set_parent_realize(dc, vu_rng_base_realize,
> +                                    &vubc->parent_realize);
>   
> -    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;
> +    set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>   }
>   
>   static const TypeInfo vu_rng_info = {
>       .name = TYPE_VHOST_USER_RNG,
> -    .parent = TYPE_VIRTIO_DEVICE,
> +    .parent = TYPE_VHOST_USER_BASE,
>       .instance_size = sizeof(VHostUserRNG),
>       .class_init = vu_rng_class_init,
>   };
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index c0a86b94ae..de442dcb96 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -3,6 +3,11 @@ softmmu_virtio_ss.add(files('virtio-bus.c'))
>   softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true: files('virtio-pci.c'))
>   softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c'))
>   
> +# VirtIO stubs which don't need building per-guest
> +softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
> +softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
> +                      if_true: files('vhost-user-rng-pci.c'))
> +
>   specific_virtio_ss = ss.source_set()
>   specific_virtio_ss.add(files('virtio.c'))
>   specific_virtio_ss.add(files('virtio-config-io.c', 'virtio-qmp.c'))
> @@ -32,7 +37,6 @@ specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c')
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
> -specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
>   specific_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
> @@ -43,7 +47,6 @@ virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vs
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true: files('vhost-user-input-pci.c'))
> -virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_SCSI', if_true: files('vhost-user-scsi-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_FS', if_true: files('vhost-user-fs-pci.c'))

Otherwise minus the struct formatting, the modelling looks good although given I 
can't vouch for the virtio parts:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH v2 10/13] 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 9105011e25..3ddf88a146 100644
--- a/include/hw/virtio/vhost-user-device.h
+++ b/include/hw/virtio/vhost-user-device.h
@@ -22,6 +22,7 @@ struct VHostUserBase {
     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 b0239fa033..2b028cae08 100644
--- a/hw/virtio/vhost-user-device.c
+++ b/hw/virtio/vhost-user-device.c
@@ -117,6 +117,42 @@ static uint64_t vub_get_features(VirtIODevice *vdev,
     return vub->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 vub_get_config(VirtIODevice *vdev, uint8_t *config)
+{
+    VHostUserBase *vub = VHOST_USER_BASE(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(vub->config_size && vub->vhost_user.supports_config == true);
+
+    if (vhost_dev_get_config(&vub->vhost_dev, config,
+                             vub->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 vub_config_notifier(struct vhost_dev *dev)
+{
+    virtio_notify_config(dev->vdev);
+    return 0;
+}
+
+const VhostDevConfigOps vub_config_ops = {
+    .vhost_dev_config_notifier = vub_config_notifier,
+};
+
 static void vub_handle_output(VirtIODevice *vdev, VirtQueue *vq)
 {
     /*
@@ -141,12 +177,21 @@ static int vub_connect(DeviceState *dev)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostUserBase *vub = VHOST_USER_BASE(vdev);
+    struct vhost_dev *vhost_dev = &vub->vhost_dev;
 
     if (vub->connected) {
         return 0;
     }
     vub->connected = true;
 
+    /*
+     * If we support VHOST_USER_GET_CONFIG we must enable the notifier
+     * so we can ping the guest when it updates.
+     */
+    if (vub->vhost_user.supports_config) {
+        vhost_dev_set_config_notifier(vhost_dev, &vub_config_ops);
+    }
+
     /* restore vhost state */
     if (virtio_device_started(vdev, vdev->status)) {
         vub_start(vdev);
@@ -214,11 +259,20 @@ static void vub_device_realize(DeviceState *dev, Error **errp)
         vub->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 (vub->config_size) {
+        vub->vhost_user.supports_config = true;
+    }
+
     if (!vhost_user_init(&vub->vhost_user, &vub->chardev, errp)) {
         return;
     }
 
-    virtio_init(vdev, vub->virtio_id, 0);
+    virtio_init(vdev, vub->virtio_id, vub->config_size);
 
     /*
      * Disable guest notifiers, by default all notifications will be via the
@@ -268,6 +322,7 @@ static void vub_class_init(ObjectClass *klass, void *data)
     vdc->realize = vub_device_realize;
     vdc->unrealize = vub_device_unrealize;
     vdc->get_features = vub_get_features;
+    vdc->get_config = vub_get_config;
     vdc->set_status = vub_set_status;
 }
 
@@ -295,6 +350,7 @@ static Property vud_properties[] = {
     DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
     DEFINE_PROP_UINT16("virtio-id", VHostUserBase, virtio_id, 0),
     DEFINE_PROP_UINT32("num_vqs", VHostUserBase, num_vqs, 1),
+    DEFINE_PROP_UINT32("config_size", VHostUserBase, config_size, 0),
     DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.39.2

[PATCH v2 11/13] hw/virtio: derive vhost-user-gpio from vhost-user-device

[Alex Bennée]

Now the new base class supports config handling we can take advantage
and make vhost-user-gpio a much simpler boilerplate wrapper. Also as
this doesn't require any target specific hacks we only need to build
the stubs once.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - use new vhost-user-base
  - move build to common code
---
 include/hw/virtio/vhost-user-gpio.h |  23 +-
 hw/virtio/vhost-user-gpio.c         | 400 ++--------------------------
 hw/virtio/meson.build               |   5 +-
 3 files changed, 22 insertions(+), 406 deletions(-)

diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
index a9d3f9b049..0948654dec 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;
+    VHostUserBase parent;
     /*< public >*/
 };
 
diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
index 3b013f2d0f..9f37c25415 100644
--- a/hw/virtio/vhost-user-gpio.c
+++ b/hw/virtio/vhost-user-gpio.c
@@ -11,382 +11,25 @@
 #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"
+#include "standard-headers/linux/virtio_gpio.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 Property vgpio_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
+    DEFINE_PROP_END_OF_LIST(),
 };
 
-static int vu_gpio_start(VirtIODevice *vdev)
+static void vgpio_realize(DeviceState *dev, Error **errp)
 {
-    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;
+    VHostUserBase *vub = VHOST_USER_BASE(dev);
+    VHostUserBaseClass *vubc = VHOST_USER_BASE_GET_CLASS(dev);
 
-    if (!k->set_guest_notifiers) {
-        error_report("binding does not support guest notifiers");
-        return -ENOSYS;
-    }
+    /* Fixed for GPIO */
+    vub->virtio_id = VIRTIO_ID_GPIO;
+    vub->num_vqs = 2;
+    vub->config_size = sizeof(struct virtio_gpio_config);
 
-    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);
+    vubc->parent_realize(dev, errp);
 }
 
 static const VMStateDescription vu_gpio_vmstate = {
@@ -394,30 +37,21 @@ static const VMStateDescription vu_gpio_vmstate = {
     .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);
+    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
 
-    device_class_set_props(dc, vu_gpio_properties);
     dc->vmsd = &vu_gpio_vmstate;
+    device_class_set_props(dc, vgpio_properties);
+    device_class_set_parent_realize(dc, vgpio_realize,
+                                    &vubc->parent_realize);
     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;
 }
 
 static const TypeInfo vu_gpio_info = {
     .name = TYPE_VHOST_USER_GPIO,
-    .parent = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_VHOST_USER_BASE,
     .instance_size = sizeof(VHostUserGPIO),
     .class_init = vu_gpio_class_init,
 };
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index de442dcb96..8c6cb2143d 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -7,6 +7,9 @@ softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c'
 softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
 softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
                       if_true: files('vhost-user-rng-pci.c'))
+softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
+softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'],
+                      if_true: files('vhost-user-gpio-pci.c'))
 
 specific_virtio_ss = ss.source_set()
 specific_virtio_ss.add(files('virtio.c'))
@@ -37,8 +40,6 @@ specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c')
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
-specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
-specific_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
 
 virtio_pci_ss = ss.source_set()
-- 
2.39.2

Re: [PATCH v2 11/13] hw/virtio: derive vhost-user-gpio from vhost-user-device

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> Now the new base class supports config handling we can take advantage
> and make vhost-user-gpio a much simpler boilerplate wrapper. Also as
> this doesn't require any target specific hacks we only need to build
> the stubs once.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - use new vhost-user-base
>    - move build to common code
> ---
>   include/hw/virtio/vhost-user-gpio.h |  23 +-
>   hw/virtio/vhost-user-gpio.c         | 400 ++--------------------------
>   hw/virtio/meson.build               |   5 +-
>   3 files changed, 22 insertions(+), 406 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost-user-gpio.h b/include/hw/virtio/vhost-user-gpio.h
> index a9d3f9b049..0948654dec 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;
> +    VHostUserBase parent;
>       /*< public >*/
>   };

As before:

struct VHostUserGPIO {
     VHostUserBase parent_obj;
};

> diff --git a/hw/virtio/vhost-user-gpio.c b/hw/virtio/vhost-user-gpio.c
> index 3b013f2d0f..9f37c25415 100644
> --- a/hw/virtio/vhost-user-gpio.c
> +++ b/hw/virtio/vhost-user-gpio.c
> @@ -11,382 +11,25 @@
>   #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"
> +#include "standard-headers/linux/virtio_gpio.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 Property vgpio_properties[] = {
> +    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
> +    DEFINE_PROP_END_OF_LIST(),
>   };
>   
> -static int vu_gpio_start(VirtIODevice *vdev)
> +static void vgpio_realize(DeviceState *dev, Error **errp)
>   {
> -    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;
> +    VHostUserBase *vub = VHOST_USER_BASE(dev);
> +    VHostUserBaseClass *vubc = VHOST_USER_BASE_GET_CLASS(dev);
>   
> -    if (!k->set_guest_notifiers) {
> -        error_report("binding does not support guest notifiers");
> -        return -ENOSYS;
> -    }
> +    /* Fixed for GPIO */
> +    vub->virtio_id = VIRTIO_ID_GPIO;
> +    vub->num_vqs = 2;
> +    vub->config_size = sizeof(struct virtio_gpio_config);
>   
> -    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);
> +    vubc->parent_realize(dev, errp);
>   }
>   
>   static const VMStateDescription vu_gpio_vmstate = {
> @@ -394,30 +37,21 @@ static const VMStateDescription vu_gpio_vmstate = {
>       .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);
> +    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
>   
> -    device_class_set_props(dc, vu_gpio_properties);
>       dc->vmsd = &vu_gpio_vmstate;
> +    device_class_set_props(dc, vgpio_properties);
> +    device_class_set_parent_realize(dc, vgpio_realize,
> +                                    &vubc->parent_realize);
>       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;
>   }
>   
>   static const TypeInfo vu_gpio_info = {
>       .name = TYPE_VHOST_USER_GPIO,
> -    .parent = TYPE_VIRTIO_DEVICE,
> +    .parent = TYPE_VHOST_USER_BASE,
>       .instance_size = sizeof(VHostUserGPIO),
>       .class_init = vu_gpio_class_init,
>   };
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index de442dcb96..8c6cb2143d 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -7,6 +7,9 @@ softmmu_virtio_ss.add(when: 'CONFIG_VIRTIO_MMIO', if_true: files('virtio-mmio.c'
>   softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true: files('vhost-user-rng.c'))
>   softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
>                         if_true: files('vhost-user-rng-pci.c'))
> +softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
> +softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'],
> +                      if_true: files('vhost-user-gpio-pci.c'))
>   
>   specific_virtio_ss = ss.source_set()
>   specific_virtio_ss.add(files('virtio.c'))
> @@ -37,8 +40,6 @@ specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c')
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
> -specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
> -specific_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'], if_true: files('vhost-user-gpio-pci.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
>   
>   virtio_pci_ss = ss.source_set()

Otherwise LGTM:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

[PATCH v2 12/13] hw/virtio: derive vhost-user-i2c from vhost-user-base

[Alex Bennée]

Now we can take advantage of the new base class and make
vhost-user-i2c a much simpler boilerplate wrapper. Also as this
doesn't require any target specific hacks we only need to build the
stubs once.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - update to new inheritance scheme
  - move build to common code
---
 include/hw/virtio/vhost-user-i2c.h |  18 +-
 hw/virtio/vhost-user-i2c.c         | 255 ++---------------------------
 hw/virtio/meson.build              |   5 +-
 3 files changed, 26 insertions(+), 252 deletions(-)

diff --git a/include/hw/virtio/vhost-user-i2c.h b/include/hw/virtio/vhost-user-i2c.h
index 0f7acd40e3..47153782d1 100644
--- a/include/hw/virtio/vhost-user-i2c.h
+++ b/include/hw/virtio/vhost-user-i2c.h
@@ -12,20 +12,18 @@
 #include "hw/virtio/vhost.h"
 #include "hw/virtio/vhost-user.h"
 
+#include "hw/virtio/virtio.h"
+#include "hw/virtio/vhost.h"
+#include "hw/virtio/vhost-user.h"
+#include "hw/virtio/vhost-user-device.h"
+
 #define TYPE_VHOST_USER_I2C "vhost-user-i2c-device"
 OBJECT_DECLARE_SIMPLE_TYPE(VHostUserI2C, VHOST_USER_I2C)
 
 struct VHostUserI2C {
-    VirtIODevice parent;
-    CharBackend chardev;
-    struct vhost_virtqueue *vhost_vq;
-    struct vhost_dev vhost_dev;
-    VhostUserState vhost_user;
-    VirtQueue *vq;
-    bool connected;
+    /*< private >*/
+    VHostUserBase parent;
+    /*< public >*/
 };
 
-/* Virtio Feature bits */
-#define VIRTIO_I2C_F_ZERO_LENGTH_REQUEST		0
-
 #endif /* QEMU_VHOST_USER_I2C_H */
diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
index 60eaf0d95b..4a1f644a87 100644
--- a/hw/virtio/vhost-user-i2c.c
+++ b/hw/virtio/vhost-user-i2c.c
@@ -14,237 +14,21 @@
 #include "qemu/error-report.h"
 #include "standard-headers/linux/virtio_ids.h"
 
-static const int feature_bits[] = {
-    VIRTIO_I2C_F_ZERO_LENGTH_REQUEST,
-    VIRTIO_F_RING_RESET,
-    VHOST_INVALID_FEATURE_BIT
+static Property vi2c_properties[] = {
+    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
+    DEFINE_PROP_END_OF_LIST(),
 };
 
-static void vu_i2c_start(VirtIODevice *vdev)
-{
-    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-    int ret, i;
-
-    if (!k->set_guest_notifiers) {
-        error_report("binding does not support guest notifiers");
-        return;
-    }
-
-    ret = vhost_dev_enable_notifiers(&i2c->vhost_dev, vdev);
-    if (ret < 0) {
-        error_report("Error enabling host notifiers: %d", -ret);
-        return;
-    }
-
-    ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, true);
-    if (ret < 0) {
-        error_report("Error binding guest notifier: %d", -ret);
-        goto err_host_notifiers;
-    }
-
-    i2c->vhost_dev.acked_features = vdev->guest_features;
-
-    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
-    if (ret < 0) {
-        error_report("Error starting vhost-user-i2c: %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 < i2c->vhost_dev.nvqs; i++) {
-        vhost_virtqueue_mask(&i2c->vhost_dev, vdev, i, false);
-    }
-
-    return;
-
-err_guest_notifiers:
-    k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
-err_host_notifiers:
-    vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
-}
-
-static void vu_i2c_stop(VirtIODevice *vdev)
-{
-    VHostUserI2C *i2c = VHOST_USER_I2C(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(&i2c->vhost_dev, vdev, true);
-
-    ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
-    if (ret < 0) {
-        error_report("vhost guest notifier cleanup failed: %d", ret);
-        return;
-    }
-
-    vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
-}
-
-static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
-{
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-    bool should_start = virtio_device_should_start(vdev, status);
-
-    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
-        return;
-    }
-
-    if (should_start) {
-        vu_i2c_start(vdev);
-    } else {
-        vu_i2c_stop(vdev);
-    }
-}
-
-static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
-                                    uint64_t requested_features, Error **errp)
-{
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    virtio_add_feature(&requested_features, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST);
-    return vhost_get_features(&i2c->vhost_dev, feature_bits, requested_features);
-}
-
-static void vu_i2c_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_i2c_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
-{
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    vhost_virtqueue_mask(&i2c->vhost_dev, vdev, idx, mask);
-}
-
-static bool vu_i2c_guest_notifier_pending(VirtIODevice *vdev, int idx)
-{
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    return vhost_virtqueue_pending(&i2c->vhost_dev, idx);
-}
-
-static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserI2C *i2c)
-{
-    vhost_user_cleanup(&i2c->vhost_user);
-    virtio_delete_queue(i2c->vq);
-    virtio_cleanup(vdev);
-}
-
-static int vu_i2c_connect(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    if (i2c->connected) {
-        return 0;
-    }
-    i2c->connected = true;
-
-    /* restore vhost state */
-    if (virtio_device_started(vdev, vdev->status)) {
-        vu_i2c_start(vdev);
-    }
-
-    return 0;
-}
-
-static void vu_i2c_disconnect(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    if (!i2c->connected) {
-        return;
-    }
-    i2c->connected = false;
-
-    if (vhost_dev_is_started(&i2c->vhost_dev)) {
-        vu_i2c_stop(vdev);
-    }
-}
-
-static void vu_i2c_event(void *opaque, QEMUChrEvent event)
+static void vi2c_realize(DeviceState *dev, Error **errp)
 {
-    DeviceState *dev = opaque;
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
-
-    switch (event) {
-    case CHR_EVENT_OPENED:
-        if (vu_i2c_connect(dev) < 0) {
-            qemu_chr_fe_disconnect(&i2c->chardev);
-            return;
-        }
-        break;
-    case CHR_EVENT_CLOSED:
-        vu_i2c_disconnect(dev);
-        break;
-    case CHR_EVENT_BREAK:
-    case CHR_EVENT_MUX_IN:
-    case CHR_EVENT_MUX_OUT:
-        /* Ignore */
-        break;
-    }
-}
-
-static void vu_i2c_device_realize(DeviceState *dev, Error **errp)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserI2C *i2c = VHOST_USER_I2C(dev);
-    int ret;
-
-    if (!i2c->chardev.chr) {
-        error_setg(errp, "vhost-user-i2c: missing chardev");
-        return;
-    }
-
-    if (!vhost_user_init(&i2c->vhost_user, &i2c->chardev, errp)) {
-        return;
-    }
-
-    virtio_init(vdev, VIRTIO_ID_I2C_ADAPTER, 0);
+    VHostUserBase *vub = VHOST_USER_BASE(dev);
+    VHostUserBaseClass *vubc = VHOST_USER_BASE_GET_CLASS(dev);
 
-    i2c->vhost_dev.nvqs = 1;
-    i2c->vq = virtio_add_queue(vdev, 4, vu_i2c_handle_output);
-    i2c->vhost_dev.vqs = g_new0(struct vhost_virtqueue, i2c->vhost_dev.nvqs);
+    /* Fixed for I2C */
+    vub->virtio_id = VIRTIO_ID_I2C_ADAPTER;
+    vub->num_vqs = 1;
 
-    ret = vhost_dev_init(&i2c->vhost_dev, &i2c->vhost_user,
-                         VHOST_BACKEND_TYPE_USER, 0, errp);
-    if (ret < 0) {
-        g_free(i2c->vhost_dev.vqs);
-        do_vhost_user_cleanup(vdev, i2c);
-    }
-
-    qemu_chr_fe_set_handlers(&i2c->chardev, NULL, NULL, vu_i2c_event, NULL,
-                             dev, NULL, true);
-}
-
-static void vu_i2c_device_unrealize(DeviceState *dev)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
-    VHostUserI2C *i2c = VHOST_USER_I2C(dev);
-    struct vhost_virtqueue *vhost_vqs = i2c->vhost_dev.vqs;
-
-    /* This will stop vhost backend if appropriate. */
-    vu_i2c_set_status(vdev, 0);
-    vhost_dev_cleanup(&i2c->vhost_dev);
-    g_free(vhost_vqs);
-    do_vhost_user_cleanup(vdev, i2c);
+    vubc->parent_realize(dev, errp);
 }
 
 static const VMStateDescription vu_i2c_vmstate = {
@@ -252,30 +36,21 @@ static const VMStateDescription vu_i2c_vmstate = {
     .unmigratable = 1,
 };
 
-static Property vu_i2c_properties[] = {
-    DEFINE_PROP_CHR("chardev", VHostUserI2C, chardev),
-    DEFINE_PROP_END_OF_LIST(),
-};
-
 static void vu_i2c_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
+    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
 
-    device_class_set_props(dc, vu_i2c_properties);
     dc->vmsd = &vu_i2c_vmstate;
+    device_class_set_props(dc, vi2c_properties);
+    device_class_set_parent_realize(dc, vi2c_realize,
+                                    &vubc->parent_realize);
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
-    vdc->realize = vu_i2c_device_realize;
-    vdc->unrealize = vu_i2c_device_unrealize;
-    vdc->get_features = vu_i2c_get_features;
-    vdc->set_status = vu_i2c_set_status;
-    vdc->guest_notifier_mask = vu_i2c_guest_notifier_mask;
-    vdc->guest_notifier_pending = vu_i2c_guest_notifier_pending;
 }
 
 static const TypeInfo vu_i2c_info = {
     .name = TYPE_VHOST_USER_I2C,
-    .parent = TYPE_VIRTIO_DEVICE,
+    .parent = TYPE_VHOST_USER_BASE,
     .instance_size = sizeof(VHostUserI2C),
     .class_init = vu_i2c_class_init,
 };
diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
index 8c6cb2143d..31c0406f05 100644
--- a/hw/virtio/meson.build
+++ b/hw/virtio/meson.build
@@ -10,6 +10,9 @@ softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
 softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
 softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'],
                       if_true: files('vhost-user-gpio-pci.c'))
+softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
+softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_I2C'],
+                       if_true: files('vhost-user-i2c-pci.c'))
 
 specific_virtio_ss = ss.source_set()
 specific_virtio_ss.add(files('virtio.c'))
@@ -39,14 +42,12 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-us
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
 specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
-specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
 specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
 
 virtio_pci_ss = ss.source_set()
 virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vsock-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk-pci.c'))
-virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true: files('vhost-user-input-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_SCSI', if_true: files('vhost-user-scsi-pci.c'))
 virtio_pci_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-pci.c'))
-- 
2.39.2

Re: [PATCH v2 12/13] hw/virtio: derive vhost-user-i2c from vhost-user-base

[Mark Cave-Ayland]

On 18/04/2023 17:21, Alex Bennée wrote:

> Now we can take advantage of the new base class and make
> vhost-user-i2c a much simpler boilerplate wrapper. Also as this
> doesn't require any target specific hacks we only need to build the
> stubs once.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> v2
>    - update to new inheritance scheme
>    - move build to common code
> ---
>   include/hw/virtio/vhost-user-i2c.h |  18 +-
>   hw/virtio/vhost-user-i2c.c         | 255 ++---------------------------
>   hw/virtio/meson.build              |   5 +-
>   3 files changed, 26 insertions(+), 252 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost-user-i2c.h b/include/hw/virtio/vhost-user-i2c.h
> index 0f7acd40e3..47153782d1 100644
> --- a/include/hw/virtio/vhost-user-i2c.h
> +++ b/include/hw/virtio/vhost-user-i2c.h
> @@ -12,20 +12,18 @@
>   #include "hw/virtio/vhost.h"
>   #include "hw/virtio/vhost-user.h"
>   
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/vhost.h"
> +#include "hw/virtio/vhost-user.h"
> +#include "hw/virtio/vhost-user-device.h"
> +
>   #define TYPE_VHOST_USER_I2C "vhost-user-i2c-device"
>   OBJECT_DECLARE_SIMPLE_TYPE(VHostUserI2C, VHOST_USER_I2C)
>   
>   struct VHostUserI2C {
> -    VirtIODevice parent;
> -    CharBackend chardev;
> -    struct vhost_virtqueue *vhost_vq;
> -    struct vhost_dev vhost_dev;
> -    VhostUserState vhost_user;
> -    VirtQueue *vq;
> -    bool connected;
> +    /*< private >*/
> +    VHostUserBase parent;
> +    /*< public >*/
>   };

And same again:

struct VHostUserI2C {
     VHostUserBase parent_obj;
};

> -/* Virtio Feature bits */
> -#define VIRTIO_I2C_F_ZERO_LENGTH_REQUEST		0
> -
>   #endif /* QEMU_VHOST_USER_I2C_H */
> diff --git a/hw/virtio/vhost-user-i2c.c b/hw/virtio/vhost-user-i2c.c
> index 60eaf0d95b..4a1f644a87 100644
> --- a/hw/virtio/vhost-user-i2c.c
> +++ b/hw/virtio/vhost-user-i2c.c
> @@ -14,237 +14,21 @@
>   #include "qemu/error-report.h"
>   #include "standard-headers/linux/virtio_ids.h"
>   
> -static const int feature_bits[] = {
> -    VIRTIO_I2C_F_ZERO_LENGTH_REQUEST,
> -    VIRTIO_F_RING_RESET,
> -    VHOST_INVALID_FEATURE_BIT
> +static Property vi2c_properties[] = {
> +    DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
> +    DEFINE_PROP_END_OF_LIST(),
>   };
>   
> -static void vu_i2c_start(VirtIODevice *vdev)
> -{
> -    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> -    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -    int ret, i;
> -
> -    if (!k->set_guest_notifiers) {
> -        error_report("binding does not support guest notifiers");
> -        return;
> -    }
> -
> -    ret = vhost_dev_enable_notifiers(&i2c->vhost_dev, vdev);
> -    if (ret < 0) {
> -        error_report("Error enabling host notifiers: %d", -ret);
> -        return;
> -    }
> -
> -    ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, true);
> -    if (ret < 0) {
> -        error_report("Error binding guest notifier: %d", -ret);
> -        goto err_host_notifiers;
> -    }
> -
> -    i2c->vhost_dev.acked_features = vdev->guest_features;
> -
> -    ret = vhost_dev_start(&i2c->vhost_dev, vdev, true);
> -    if (ret < 0) {
> -        error_report("Error starting vhost-user-i2c: %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 < i2c->vhost_dev.nvqs; i++) {
> -        vhost_virtqueue_mask(&i2c->vhost_dev, vdev, i, false);
> -    }
> -
> -    return;
> -
> -err_guest_notifiers:
> -    k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
> -err_host_notifiers:
> -    vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
> -}
> -
> -static void vu_i2c_stop(VirtIODevice *vdev)
> -{
> -    VHostUserI2C *i2c = VHOST_USER_I2C(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(&i2c->vhost_dev, vdev, true);
> -
> -    ret = k->set_guest_notifiers(qbus->parent, i2c->vhost_dev.nvqs, false);
> -    if (ret < 0) {
> -        error_report("vhost guest notifier cleanup failed: %d", ret);
> -        return;
> -    }
> -
> -    vhost_dev_disable_notifiers(&i2c->vhost_dev, vdev);
> -}
> -
> -static void vu_i2c_set_status(VirtIODevice *vdev, uint8_t status)
> -{
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -    bool should_start = virtio_device_should_start(vdev, status);
> -
> -    if (vhost_dev_is_started(&i2c->vhost_dev) == should_start) {
> -        return;
> -    }
> -
> -    if (should_start) {
> -        vu_i2c_start(vdev);
> -    } else {
> -        vu_i2c_stop(vdev);
> -    }
> -}
> -
> -static uint64_t vu_i2c_get_features(VirtIODevice *vdev,
> -                                    uint64_t requested_features, Error **errp)
> -{
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    virtio_add_feature(&requested_features, VIRTIO_I2C_F_ZERO_LENGTH_REQUEST);
> -    return vhost_get_features(&i2c->vhost_dev, feature_bits, requested_features);
> -}
> -
> -static void vu_i2c_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_i2c_guest_notifier_mask(VirtIODevice *vdev, int idx, bool mask)
> -{
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    vhost_virtqueue_mask(&i2c->vhost_dev, vdev, idx, mask);
> -}
> -
> -static bool vu_i2c_guest_notifier_pending(VirtIODevice *vdev, int idx)
> -{
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    return vhost_virtqueue_pending(&i2c->vhost_dev, idx);
> -}
> -
> -static void do_vhost_user_cleanup(VirtIODevice *vdev, VHostUserI2C *i2c)
> -{
> -    vhost_user_cleanup(&i2c->vhost_user);
> -    virtio_delete_queue(i2c->vq);
> -    virtio_cleanup(vdev);
> -}
> -
> -static int vu_i2c_connect(DeviceState *dev)
> -{
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    if (i2c->connected) {
> -        return 0;
> -    }
> -    i2c->connected = true;
> -
> -    /* restore vhost state */
> -    if (virtio_device_started(vdev, vdev->status)) {
> -        vu_i2c_start(vdev);
> -    }
> -
> -    return 0;
> -}
> -
> -static void vu_i2c_disconnect(DeviceState *dev)
> -{
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    if (!i2c->connected) {
> -        return;
> -    }
> -    i2c->connected = false;
> -
> -    if (vhost_dev_is_started(&i2c->vhost_dev)) {
> -        vu_i2c_stop(vdev);
> -    }
> -}
> -
> -static void vu_i2c_event(void *opaque, QEMUChrEvent event)
> +static void vi2c_realize(DeviceState *dev, Error **errp)
>   {
> -    DeviceState *dev = opaque;
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(vdev);
> -
> -    switch (event) {
> -    case CHR_EVENT_OPENED:
> -        if (vu_i2c_connect(dev) < 0) {
> -            qemu_chr_fe_disconnect(&i2c->chardev);
> -            return;
> -        }
> -        break;
> -    case CHR_EVENT_CLOSED:
> -        vu_i2c_disconnect(dev);
> -        break;
> -    case CHR_EVENT_BREAK:
> -    case CHR_EVENT_MUX_IN:
> -    case CHR_EVENT_MUX_OUT:
> -        /* Ignore */
> -        break;
> -    }
> -}
> -
> -static void vu_i2c_device_realize(DeviceState *dev, Error **errp)
> -{
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(dev);
> -    int ret;
> -
> -    if (!i2c->chardev.chr) {
> -        error_setg(errp, "vhost-user-i2c: missing chardev");
> -        return;
> -    }
> -
> -    if (!vhost_user_init(&i2c->vhost_user, &i2c->chardev, errp)) {
> -        return;
> -    }
> -
> -    virtio_init(vdev, VIRTIO_ID_I2C_ADAPTER, 0);
> +    VHostUserBase *vub = VHOST_USER_BASE(dev);
> +    VHostUserBaseClass *vubc = VHOST_USER_BASE_GET_CLASS(dev);
>   
> -    i2c->vhost_dev.nvqs = 1;
> -    i2c->vq = virtio_add_queue(vdev, 4, vu_i2c_handle_output);
> -    i2c->vhost_dev.vqs = g_new0(struct vhost_virtqueue, i2c->vhost_dev.nvqs);
> +    /* Fixed for I2C */
> +    vub->virtio_id = VIRTIO_ID_I2C_ADAPTER;
> +    vub->num_vqs = 1;
>   
> -    ret = vhost_dev_init(&i2c->vhost_dev, &i2c->vhost_user,
> -                         VHOST_BACKEND_TYPE_USER, 0, errp);
> -    if (ret < 0) {
> -        g_free(i2c->vhost_dev.vqs);
> -        do_vhost_user_cleanup(vdev, i2c);
> -    }
> -
> -    qemu_chr_fe_set_handlers(&i2c->chardev, NULL, NULL, vu_i2c_event, NULL,
> -                             dev, NULL, true);
> -}
> -
> -static void vu_i2c_device_unrealize(DeviceState *dev)
> -{
> -    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> -    VHostUserI2C *i2c = VHOST_USER_I2C(dev);
> -    struct vhost_virtqueue *vhost_vqs = i2c->vhost_dev.vqs;
> -
> -    /* This will stop vhost backend if appropriate. */
> -    vu_i2c_set_status(vdev, 0);
> -    vhost_dev_cleanup(&i2c->vhost_dev);
> -    g_free(vhost_vqs);
> -    do_vhost_user_cleanup(vdev, i2c);
> +    vubc->parent_realize(dev, errp);
>   }
>   
>   static const VMStateDescription vu_i2c_vmstate = {
> @@ -252,30 +36,21 @@ static const VMStateDescription vu_i2c_vmstate = {
>       .unmigratable = 1,
>   };
>   
> -static Property vu_i2c_properties[] = {
> -    DEFINE_PROP_CHR("chardev", VHostUserI2C, chardev),
> -    DEFINE_PROP_END_OF_LIST(),
> -};
> -
>   static void vu_i2c_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> -    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +    VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
>   
> -    device_class_set_props(dc, vu_i2c_properties);
>       dc->vmsd = &vu_i2c_vmstate;
> +    device_class_set_props(dc, vi2c_properties);
> +    device_class_set_parent_realize(dc, vi2c_realize,
> +                                    &vubc->parent_realize);
>       set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> -    vdc->realize = vu_i2c_device_realize;
> -    vdc->unrealize = vu_i2c_device_unrealize;
> -    vdc->get_features = vu_i2c_get_features;
> -    vdc->set_status = vu_i2c_set_status;
> -    vdc->guest_notifier_mask = vu_i2c_guest_notifier_mask;
> -    vdc->guest_notifier_pending = vu_i2c_guest_notifier_pending;
>   }
>   
>   static const TypeInfo vu_i2c_info = {
>       .name = TYPE_VHOST_USER_I2C,
> -    .parent = TYPE_VIRTIO_DEVICE,
> +    .parent = TYPE_VHOST_USER_BASE,
>       .instance_size = sizeof(VHostUserI2C),
>       .class_init = vu_i2c_class_init,
>   };
> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
> index 8c6cb2143d..31c0406f05 100644
> --- a/hw/virtio/meson.build
> +++ b/hw/virtio/meson.build
> @@ -10,6 +10,9 @@ softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_RNG'],
>   softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_GPIO', if_true: files('vhost-user-gpio.c'))
>   softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_GPIO'],
>                         if_true: files('vhost-user-gpio-pci.c'))
> +softmmu_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
> +softmmu_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI', 'CONFIG_VHOST_USER_I2C'],
> +                       if_true: files('vhost-user-i2c-pci.c'))
>   
>   specific_virtio_ss = ss.source_set()
>   specific_virtio_ss.add(files('virtio.c'))
> @@ -39,14 +42,12 @@ specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-us
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_RNG', if_true: files('virtio-rng.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_IOMMU', if_true: files('virtio-iommu.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VIRTIO_MEM', if_true: files('virtio-mem.c'))
> -specific_virtio_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c.c'))
>   specific_virtio_ss.add(when: 'CONFIG_VHOST_VDPA_DEV', if_true: files('vdpa-dev.c'))
>   
>   virtio_pci_ss = ss.source_set()
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_VSOCK', if_true: files('vhost-vsock-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_VSOCK', if_true: files('vhost-user-vsock-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_BLK', if_true: files('vhost-user-blk-pci.c'))
> -virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_I2C', if_true: files('vhost-user-i2c-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true: files('vhost-user-input-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_USER_SCSI', if_true: files('vhost-user-scsi-pci.c'))
>   virtio_pci_ss.add(when: 'CONFIG_VHOST_SCSI', if_true: files('vhost-scsi-pci.c'))

Otherwise:

Acked-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

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

[Alex Bennée]

Make it clear the vhost-user-device is intended for expert use only.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - make clear vhost-user-device for expert use
---
 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..7038cece3e 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 Development Device
+    - You must manually specify ``virtio-id`` and the correct ``num_vqs``. Intended for expert use.
+  * - 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