Re: [Qemu-devel] [RFC-PATCH] Introducing virtio-example device.

[Eduardo Habkost <ehabkost@redhat.com>]

Hi,

Thanks for the patch, comments below:

On Mon, Apr 01, 2019 at 02:18:43PM +0300, Yoni Bettan wrote:
> The main goal is to add an example device to Qemu to be used as template or
> guideline for contributors when they wish to create a new virtio device.
> 
> Another reason for this device is to document "the right way" to write
> a new virtio device in Qemu.
> 
> This device is a simple device and its functionality is to increase its input
> by 1.

I believe we need a clearer description of what the device does,
especially considering that you are using base 10 strings as
input and output.

(See additional comments about the string conversion below)

> 
> The device driver is located at:
> https://github.com/ybettan/QemuDeviceDrivers.git
> 
> In addition I am writing a blog to give a logical overview of the virtio
> protocol and a step-by-step guide to write a new virtio device.
> This blog can be found at https://howtovms.wordpress.com.
> 
> scripts/checkpatch.pl have some errors do to "//" (old style one line comment),
> those lines contains FIXMEs for the next version and will be removed.
> 
> Signed-off-by: Yoni Bettan <ybettan@redhat.com>
> ---
>  hw/virtio/Makefile.objs                       |   1 +
>  hw/virtio/virtio-example.c                    | 121 ++++++++++++++++++
>  hw/virtio/virtio-pci.c                        |  49 +++++++
>  hw/virtio/virtio-pci.h                        |  14 ++
>  include/hw/pci/pci.h                          |   1 +
>  include/hw/virtio/virtio-example.h            |  31 +++++
>  .../standard-headers/linux/virtio_example.h   |   8 ++
>  include/standard-headers/linux/virtio_ids.h   |   1 +
>  8 files changed, 226 insertions(+)
>  create mode 100644 hw/virtio/virtio-example.c
>  create mode 100644 include/hw/virtio/virtio-example.h
>  create mode 100644 include/standard-headers/linux/virtio_example.h
> 
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 1b2799cfd8..7a6fb2505c 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -7,6 +7,7 @@ common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>  common-obj-$(CONFIG_VIRTIO_MMIO) += virtio-mmio.o
>  obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon.o
>  obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-crypto.o
> +obj-$(CONFIG_VIRTIO_CRYPTO) += virtio-example.o
>  obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) += virtio-crypto-pci.o
>  
>  obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> diff --git a/hw/virtio/virtio-example.c b/hw/virtio/virtio-example.c
> new file mode 100644
> index 0000000000..3c170f8022
> --- /dev/null
> +++ b/hw/virtio/virtio-example.c
> @@ -0,0 +1,121 @@
> +/*
> + * A virtio device example.
> + *
> + * Copyright 2019 Red Hat, Inc.
> + * Copyright 2019 Yoni Bettan <ybettan@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/iov.h"
> +#include "hw/virtio/virtio.h"
> +#include "hw/virtio/virtio-example.h"
> +
> +#define MAX_DATA_SIZE 10
> +
> +
> +/*
> + * this function is called when the driver 'kick' the virtqueue.
> + * since we can have more than 1 virtqueue we need the vq argument in order to
> + * know which one was kicked by the driver.
> + */
> +static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtQueueElement *elem;
> +    int data, len;
> +    char buf[MAX_DATA_SIZE];
> +
> +    /* check that virtqueue have at least 2 elements, input and output */
> +    //FIXME: implement
> +
> +    /* get the virtqueue input element sent from the driver */
> +    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
> +
> +    /* read the input element (sg) into a buffer */
> +    len = iov_to_buf(elem->in_sg, elem->out_num, 0, buf, MAX_DATA_SIZE);

I believe you mean elem->in_num above.


> +    //FIXME: should i check buffer overflow, or iov_to_buf() is safe ?

If iov_to_buf() didn't check the buffer size, checking it after
it returned would be too late.

> +    if (len > MAX_DATA_SIZE) {
> +        goto error;
> +    }
> +
> +    /* process the data */
> +    data = atoi(buf);

What if there's no null terminator on the input buffer?

> +    sprintf(buf, "%d", ++data);

MAX_DATA_SIZE is 10, so if input data is "999999999" you will
write beyond the end of the buffer.

I suggest not dealing with the complexity of string conversion,
and just do something simpler (like increasing every byte by 1).


> +
> +    /* get the virtqueue output element sent from the driver */
> +    elem = virtqueue_pop(vq, sizeof(VirtQueueElement));

I don't understand why you are using two separate buffers here.
A single request may contain both input and output buffers.

Also, I believe there's a risk here that the device will see the
input buffer before the output buffer is added to the queue by
the guest.


> +
> +    /* write the result to the output element (sg) */
> +    len = iov_from_buf(elem->in_sg, elem->in_num, 0, buf, MAX_DATA_SIZE);

Why are you writing to in_sg instead of out_sg?

> +    //FIXME: should i check buffer overflow, or iov_from_buf() is safe ?
> +
> +    /* push back the result into the virtqueue */
> +    virtqueue_push(vq, elem, /*len=*/1);

I suggest removing the "len=" comment in the next version.

> +
> +    /* interrupt the driver */
> +    virtio_notify(vdev, vq);

These last two lines look right, but I would like somebody who
works on virtio drivers to confirm.

> +
> +    return;
> +
> +error:
> +    printf("ERROR: buffer overflow\n");
> +    //FIXME: can we make the request fail ?, if iov_*_buf is safe we don't need it

I suggest simply dropping the data if there's no room on the
output buffer.  This way there's no need for any error handling.

> +}
> +
> +/*
> + * There is no currently defined feature bits, we still need this function
> + * because the backend driver checks that VirtioDeviceClass.get_features is
> + * initialized
> + */

Maybe we should explain what get_features() does and why, as this
is an example device implementation.

> +static uint64_t get_features(VirtIODevice *vdev, uint64_t features, Error **errp)
> +{
> +    return features;
> +}
> +
> +static void virtio_example_device_realize(DeviceState *dev, Error **errp)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +    VirtIOEXAMPLE *vexmp = VIRTIO_EXAMPLE(dev);
> +
> +    /* base class initialization */

I'm not sure "base class initialization" is an accurate
description of virtio_init().  Maybe the comment makes it more
confusing.

> +    virtio_init(vdev, "virtio-example", VIRTIO_ID_EXAMPLE, /*config_size=*/0);

I suggest removing the "config_size=" comment in the next
version.

> +
> +    /* this device suppot 1 virtqueue */
> +    vexmp->vq = virtio_add_queue(vdev, 8, handle_input);
> +}
> +
> +static void virtio_example_device_unrealize(DeviceState *dev, Error **errp)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> +
> +    /* base class cleanup */
> +    virtio_cleanup(vdev);
> +}
> +
> +static void virtio_example_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VirtioDeviceClass *vdc = VIRTIO_DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +    vdc->realize = virtio_example_device_realize;
> +    vdc->unrealize = virtio_example_device_unrealize;
> +    vdc->get_features = get_features;
> +}
> +
> +static const TypeInfo virtio_example_info = {
> +    .name = TYPE_VIRTIO_EXAMPLE,
> +    .parent = TYPE_VIRTIO_DEVICE,
> +    .instance_size = sizeof(VirtIOEXAMPLE),
> +    .class_init = virtio_example_class_init,
> +};
> +
> +static void virtio_register_types(void)
> +{
> +    type_register_static(&virtio_example_info);
> +}
> +
> +type_init(virtio_register_types)
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 3a01fe90f0..99fc6ce79b 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -2521,6 +2521,54 @@ static const TypeInfo virtio_rng_pci_info = {
>      .class_init    = virtio_rng_pci_class_init,
>  };
>  
> +/* virtio-example-pci */
> +
> +static void virtio_example_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> +{
> +    VirtIOExamplePCI *vexmp = VIRTIO_EXAMPLE_PCI(vpci_dev);
> +    DeviceState *vdev = DEVICE(&vexmp->vdev);
> +    Error *err = NULL;
> +
> +    qdev_set_parent_bus(vdev, BUS(&vpci_dev->bus));
> +    object_property_set_bool(OBJECT(vdev), true, "realized", &err);
> +    if (err) {
> +        error_propagate(errp, err);
> +        return;
> +    }

The 5 lines above can be replaced by:

    object_property_set_bool(OBJECT(vdev), true, "realized", errp);

> +
> +}
> +
> +static void virtio_example_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 = virtio_example_pci_realize;
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +
> +    pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> +    pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_EXAMPLE;
> +    pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
> +    pcidev_k->class_id = PCI_CLASS_OTHERS;
> +}
> +
> +static void virtio_example_initfn(Object *obj)
> +{
> +    VirtIOExamplePCI *dev = VIRTIO_EXAMPLE_PCI(obj);
> +
> +    virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
> +                                TYPE_VIRTIO_EXAMPLE);
> +}
> +
> +static const TypeInfo virtio_example_pci_info = {
> +    .name          = TYPE_VIRTIO_EXAMPLE_PCI,
> +    .parent        = TYPE_VIRTIO_PCI,
> +    .instance_size = sizeof(VirtIOExamplePCI),
> +    .instance_init = virtio_example_initfn,
> +    .class_init    = virtio_example_pci_class_init,
> +};
> +
>  /* virtio-input-pci */
>  
>  static Property virtio_input_pci_properties[] = {
> @@ -2693,6 +2741,7 @@ static const TypeInfo virtio_pci_bus_info = {
>  static void virtio_pci_register_types(void)
>  {
>      type_register_static(&virtio_rng_pci_info);
> +    type_register_static(&virtio_example_pci_info);
>      type_register_static(&virtio_input_pci_info);
>      type_register_static(&virtio_input_hid_pci_info);
>      type_register_static(&virtio_keyboard_pci_info);
> diff --git a/hw/virtio/virtio-pci.h b/hw/virtio/virtio-pci.h
> index 813082b0d7..db3f5ec17d 100644
> --- a/hw/virtio/virtio-pci.h
> +++ b/hw/virtio/virtio-pci.h
> @@ -19,6 +19,7 @@
>  #include "hw/virtio/virtio-blk.h"
>  #include "hw/virtio/virtio-net.h"
>  #include "hw/virtio/virtio-rng.h"
> +#include "hw/virtio/virtio-example.h"
>  #include "hw/virtio/virtio-serial.h"
>  #include "hw/virtio/virtio-scsi.h"
>  #include "hw/virtio/virtio-balloon.h"
> @@ -51,6 +52,7 @@ typedef struct VHostSCSIPCI VHostSCSIPCI;
>  typedef struct VHostUserSCSIPCI VHostUserSCSIPCI;
>  typedef struct VHostUserBlkPCI VHostUserBlkPCI;
>  typedef struct VirtIORngPCI VirtIORngPCI;
> +typedef struct VirtIOExamplePCI VirtIOExamplePCI;
>  typedef struct VirtIOInputPCI VirtIOInputPCI;
>  typedef struct VirtIOInputHIDPCI VirtIOInputHIDPCI;
>  typedef struct VirtIOInputHostPCI VirtIOInputHostPCI;
> @@ -339,6 +341,18 @@ struct VirtIORngPCI {
>      VirtIORNG vdev;
>  };
>  
> +/*
> + * virtio-example-pci: This extends VirtioPCIProxy.
> + */
> +#define TYPE_VIRTIO_EXAMPLE_PCI "virtio-example-pci"
> +#define VIRTIO_EXAMPLE_PCI(obj) \
> +        OBJECT_CHECK(VirtIOExamplePCI, (obj), TYPE_VIRTIO_EXAMPLE_PCI)
> +
> +struct VirtIOExamplePCI {
> +    VirtIOPCIProxy parent_obj;
> +    VirtIOEXAMPLE vdev;
> +};
> +
>  /*
>   * virtio-input-pci: This extends VirtioPCIProxy.
>   */
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 990d6fcbde..c69d5997b7 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -83,6 +83,7 @@ extern bool pci_available;
>  #define PCI_DEVICE_ID_VIRTIO_CONSOLE     0x1003
>  #define PCI_DEVICE_ID_VIRTIO_SCSI        0x1004
>  #define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
> +#define PCI_DEVICE_ID_VIRTIO_EXAMPLE     0x1006 //FIXME: update to valid ID

I'm not sure what to do about the device ID here.  Should we really allocate a
device ID for this example device?


>  #define PCI_DEVICE_ID_VIRTIO_9P          0x1009
>  #define PCI_DEVICE_ID_VIRTIO_VSOCK       0x1012
>  
> diff --git a/include/hw/virtio/virtio-example.h b/include/hw/virtio/virtio-example.h
> new file mode 100644
> index 0000000000..c08db28e8f
> --- /dev/null
> +++ b/include/hw/virtio/virtio-example.h
> @@ -0,0 +1,31 @@
> +/*
> + * Virtio EXAMPLE Support
> + *
> + * Copyright Red Hat, Inc. 2019
> + * Copyright Yoni Bettan <ybettan@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or
> + * (at your option) any later version.  See the COPYING file in the
> + * top-level directory.
> + */
> +
> +#ifndef QEMU_VIRTIO_EXAMPLE_H
> +#define QEMU_VIRTIO_EXAMPLE_H
> +
> +#include "standard-headers/linux/virtio_example.h"
> +
> +#define TYPE_VIRTIO_EXAMPLE "virtio-example-device"
> +#define VIRTIO_EXAMPLE(obj) \
> +        OBJECT_CHECK(VirtIOEXAMPLE, (obj), TYPE_VIRTIO_EXAMPLE)
> +#define VIRTIO_EXAMPLE_GET_PARENT_CLASS(obj) \
> +        OBJECT_GET_PARENT_CLASS(obj, TYPE_VIRTIO_EXAMPLE)
> +
> +typedef struct VirtIOEXAMPLE {
> +    VirtIODevice parent_obj;
> +
> +    /* Only one vq - guest puts buffer(s) on it when it needs computation */
> +    VirtQueue *vq;
> +
> +} VirtIOEXAMPLE;
> +
> +#endif
> diff --git a/include/standard-headers/linux/virtio_example.h b/include/standard-headers/linux/virtio_example.h
> new file mode 100644
> index 0000000000..6321c60c5c
> --- /dev/null
> +++ b/include/standard-headers/linux/virtio_example.h
> @@ -0,0 +1,8 @@
> +#ifndef _LINUX_VIRTIO_EXAMPLE_H
> +#define _LINUX_VIRTIO_EXAMPLE_H
> +/* This header is BSD licensed so anyone can use the definitions to implement
> + * compatible drivers/servers. */
> +#include "standard-headers/linux/virtio_ids.h"
> +#include "standard-headers/linux/virtio_config.h"
> +
> +#endif /* _LINUX_VIRTIO_EXAMPLE_H */
> diff --git a/include/standard-headers/linux/virtio_ids.h b/include/standard-headers/linux/virtio_ids.h
> index 6d5c3b2d4f..30c189303b 100644
> --- a/include/standard-headers/linux/virtio_ids.h
> +++ b/include/standard-headers/linux/virtio_ids.h
> @@ -43,5 +43,6 @@
>  #define VIRTIO_ID_INPUT        18 /* virtio input */
>  #define VIRTIO_ID_VSOCK        19 /* virtio vsock transport */
>  #define VIRTIO_ID_CRYPTO       20 /* virtio crypto */
> +#define VIRTIO_ID_EXAMPLE      21 /* virtio example */
>  

Same question about device ID here.  Should we really allocate a device ID for
this example device?

Also, standard-headers is supposed to be copied from the Linux source tree, and
not touched by any patch except when running update-linux-headers.sh.

-- 
Eduardo