Re: [RFC PATCH] docs/devel: start documenting writing VirtIO devices

[Cornelia Huck]

On Wed, Mar 09 2022, Alex Bennée <alex.bennee@linaro.org> wrote:

> While writing my own VirtIO devices I've gotten confused with how
> things are structured and what sort of shared infrastructure there is.
> If we can document how everything is supposed to work we can then
> maybe start cleaning up inconsistencies in the code.

I agree that we could use some documentation here; OTOH, I'm a bit
confused in turn by your patch :) Let me comment below.

>
> Based-on: 20220309135355.4149689-1-alex.bennee@linaro.org
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  docs/devel/index-internals.rst |   2 +-
>  docs/devel/virtio-backends.rst | 139 +++++++++++++++++++++++++++++++++
>  2 files changed, 140 insertions(+), 1 deletion(-)
>  create mode 100644 docs/devel/virtio-backends.rst

(...)

> diff --git a/docs/devel/virtio-backends.rst b/docs/devel/virtio-backends.rst
> new file mode 100644
> index 0000000000..230538f46b
> --- /dev/null
> +++ b/docs/devel/virtio-backends.rst
> @@ -0,0 +1,139 @@
> +..
> +   Copyright (c) 2022, Linaro Limited
> +   Written by Alex Bennée
> +
> +Writing VirtIO backends for QEMU
> +================================
> +
> +This document attempts to outline the information a developer needs to
> +know to write backends for QEMU. It is specifically focused on
> +implementing VirtIO devices.

I think you first need to define a bit more clearly what you consider a
"backend". For virtio, it is probably "everything a device needs to
function as a specific device type like net, block, etc., which may be
implemented by different methods" (as you describe further below).

> +
> +Front End Transports
> +--------------------
> +
> +VirtIO supports a number of different front end transports. The
> +details of the device remain the same but there are differences in
> +command line for specifying the device (e.g. -device virtio-foo
> +and -device virtio-foo-pci). For example:
> +
> +.. code:: c
> +
> +  static const TypeInfo vhost_user_blk_info = {
> +      .name = TYPE_VHOST_USER_BLK,
> +      .parent = TYPE_VIRTIO_DEVICE,
> +      .instance_size = sizeof(VHostUserBlk),
> +      .instance_init = vhost_user_blk_instance_init,
> +      .class_init = vhost_user_blk_class_init,
> +  };
> +
> +defines ``TYPE_VHOST_USER_BLK`` as a child of the generic
> +``TYPE_VIRTIO_DEVICE``.

That's not what I'd consider a "front end", though?

> And then for the PCI device it wraps around the
> +base device (although explicitly initialising via
> +virtio_instance_init_common):
> +
> +.. code:: c
> +
> +  struct VHostUserBlkPCI {
> +      VirtIOPCIProxy parent_obj;
> +      VHostUserBlk vdev;
> +  };

The VirtIOPCIProxy seems to materialize a bit out of thin air
here... maybe the information simply needs to be structured in a
different way? Perhaps:

- describe that virtio devices consist of a part that implements the
  device functionality, which ultimately derives from VirtIODevice (the
  "backend"), and a part that exposes a way for the operating system to
  discover and use the device (the "frontend", what the virtio spec
  calls a "transport")
- decribe how the "frontend" part works (maybe mention VirtIOPCIProxy,
  VirtIOMMIOProxy, and VirtioCcwDevice as specialized proxy devices for
  PCI, MMIO, and CCW devices)
- list the different types of "backends" (as you did below), and give
  two examples of how VirtIODevice is extended (a plain one, and a
  vhost-user one)
- explain how frontend and backend together create an actual device
  (with the two device examples, and maybe also with the plain one
  plugged as both PCI and CCW?); maybe also mention that MMIO is a bit
  different? (it always confuses me)

> +   
> +  static void vhost_user_blk_pci_instance_init(Object *obj)
> +  {
> +      VHostUserBlkPCI *dev = VHOST_USER_BLK_PCI(obj);
> +
> +      virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                  TYPE_VHOST_USER_BLK);
> +      object_property_add_alias(obj, "bootindex", OBJECT(&dev->vdev),
> +                                "bootindex");
> +  }
> +
> +  static const VirtioPCIDeviceTypeInfo vhost_user_blk_pci_info = {
> +      .base_name               = TYPE_VHOST_USER_BLK_PCI,
> +      .generic_name            = "vhost-user-blk-pci",
> +      .transitional_name       = "vhost-user-blk-pci-transitional",
> +      .non_transitional_name   = "vhost-user-blk-pci-non-transitional",
> +      .instance_size  = sizeof(VHostUserBlkPCI),
> +      .instance_init  = vhost_user_blk_pci_instance_init,
> +      .class_init     = vhost_user_blk_pci_class_init,
> +  };
> +
> +Back End Implementations
> +------------------------
> +
> +There are a number of places where the implementation of the backend
> +can be done:
> +
> +* in QEMU itself
> +* in the host kernel (a.k.a vhost)
> +* in a separate process (a.k.a. vhost-user)
> +
> +where a vhost-user implementation is being done the code in QEMU is
> +mainly boilerplate to handle the command line definition and
> +connection to the separate process with a socket (using the ``chardev``
> +functionality).
> +
> +Implementing a vhost-user wrapper
> +---------------------------------
> +
> +There are some classes defined that can wrap a lot of the common
> +vhost-user code in a ``VhostUserBackend``. For example:

Is VhostUserBackend something that is expected to be commonly used? I
think gpu and input use it, but not virtiofs (unless I misread the code).

> +
> +.. code:: c
> +
> +  struct VhostUserGPU {
> +      VirtIOGPUBase parent_obj;
> +
> +      VhostUserBackend *vhost;
> +      ...
> +  };
> +
> +  static void
> +  vhost_user_gpu_instance_init(Object *obj)
> +  {
> +      VhostUserGPU *g = VHOST_USER_GPU(obj);
> +
> +      g->vhost = VHOST_USER_BACKEND(object_new(TYPE_VHOST_USER_BACKEND));
> +      object_property_add_alias(obj, "chardev",
> +                                OBJECT(g->vhost), "chardev");
> +  }
> +
> +  static void
> +  vhost_user_gpu_device_realize(DeviceState *qdev, Error **errp)
> +  {
> +      VhostUserGPU *g = VHOST_USER_GPU(qdev);
> +      VirtIODevice *vdev = VIRTIO_DEVICE(g);
> +
> +      vhost_dev_set_config_notifier(&g->vhost->dev, &config_ops);
> +      if (vhost_user_backend_dev_init(g->vhost, vdev, 2, errp) < 0) {
> +          return;
> +      }
> +      ...
> +  }
> +
> +  static void
> +  vhost_user_gpu_class_init(ObjectClass *klass, void *data)
> +  {
> +      DeviceClass *dc = DEVICE_CLASS(klass);
> +      VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> +      vdc->realize = vhost_user_gpu_device_realize;
> +      ...
> +  }
> +
> +  static const TypeInfo vhost_user_gpu_info = {
> +      .name = TYPE_VHOST_USER_GPU,
> +      .parent = TYPE_VIRTIO_GPU_BASE,
> +      .instance_size = sizeof(VhostUserGPU),
> +      .instance_init = vhost_user_gpu_instance_init,
> +      .class_init = vhost_user_gpu_class_init,
> +      ...
> +  };
> +
> +Here the ``TYPE_VHOST_USER_GPU`` is based off a shared base class
> +(``TYPE_VIRTIO_GPU_BASE`` which itself is based on
> +``TYPE_VIRTIO_DEVICE``). The chardev property is aliased to the
> +VhostUserBackend chardev so it can be specified on the command line
> +for this device.
> + 

I think using a "base" device is something that is device-specific; for
gpu, it makes sense as it can be implemented in different ways, but
e.g. virtiofs does not have a "plain" implementation, and some device
types have only "plain" implementations.