Re: [Qemu-devel] [PATCH 2/2] VirtIO RNG

[Anthony Liguori]

On 03/30/2010 09:13 AM, Ian Molton wrote:
> > From 5f484301d73fa53009bbcd430f8ae85868b67772 Mon Sep 17 00:00:00 2001
> From: Ian Molton<address@hidden>
> Date: Tue, 17 Nov 2009 14:34:12 +0000
> Subject: [PATCH 2/4] virtio: Add virtio-rng driver
>
>         This patch adds support for virtio-rng. Data is read from a chardev and
> can be either raw entropy or received via the EGD protocol.
>
> Signed-off-by: Ian Molton<address@hidden>
>    

Hi Ian,

A couple review comments below.  I've read the thread.  virtio-rng 
should stand on it's own as it's a spec'd out virtio device so I'm with 
you there.

The SIZE qdev patch can't be applied until there's a consumer of it.  
I've already expressed my opinion about the socket reconnect patch.  I 
don't think it's the right functionality for qemu to implement because 
the semantics concern me.  Really, a more thorough refactoring of 
CharDrivers is needed such that it supported connected and disconnected 
states along with QMP events for the transitions.

I agree with Paul that the EGD logic ought to be separated.  It probably 
makes sense to modify this a bit to have a split front-end/back-end.  
For instance:

-device virtio-rng-pci,rngdev=foo -rngdev egd,addr=unix:/path/to/rng,id=foo

You could also have a syntax helper like:

-rng egd,addr:/path/to/rng

See the recent virtfs patch series for an example of this.

With those changes, I'd happily apply this series.

> ---
>   Makefile.target |    2 +-
>   hw/pci.h        |    1 +
>   hw/virtio-pci.c |   29 ++++++++
>   hw/virtio-rng.c |  195
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio-rng.h |   19 ++++++
>   hw/virtio.h     |    2 +
>   rng.h           |   14 ++++
>   7 files changed, 261 insertions(+), 1 deletions(-)
>   create mode 100644 hw/virtio-rng.c
>   create mode 100644 hw/virtio-rng.h
>   create mode 100644 rng.h
>
> diff --git a/Makefile.target b/Makefile.target
> index 16ea443..2f9c929 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -166,7 +166,7 @@ obj-y += qemu-timer.o
>   # virtio has to be here due to weird dependency between PCI and virtio-net.
>   # need to fix this properly
>   obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
> -obj-y += rwhandler.o
> +obj-y += virtio-rng.o rwhandler.o
>   obj-$(CONFIG_KVM) += kvm.o kvm-all.o
>   LIBS+=-lz
>
> diff --git a/hw/pci.h b/hw/pci.h
> index 20c670e..dc61cb5 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -69,6 +69,7 @@
>   #define PCI_DEVICE_ID_VIRTIO_BLOCK       0x1001
>   #define PCI_DEVICE_ID_VIRTIO_BALLOON     0x1002
>   #define PCI_DEVICE_ID_VIRTIO_CONSOLE     0x1003
> +#define PCI_DEVICE_ID_VIRTIO_RNG         0x1004
>
>   #define FMT_PCIBUS                      PRIx64
>
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 6eb19cd..bee3105 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -23,6 +23,7 @@
>   #include "msix.h"
>   #include "net.h"
>   #include "block_int.h"
> +#include "rng.h"
>   #include "loader.h"
>
>   /* from Linux's linux/virtio_pci.h */
> @@ -95,6 +96,7 @@ typedef struct {
>       uint32_t nvectors;
>       BlockConf block;
>       NICConf nic;
> +    RNGConf rng;
>       uint32_t host_features;
>       /* Max. number of ports we can have for a the virtio-serial device */
>       uint32_t max_virtserial_ports;
> @@ -552,6 +554,21 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev)
>       return 0;
>   }
>
> +static int virtio_rng_init_pci(PCIDevice *pci_dev)
> +{
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> +    VirtIODevice *vdev;
> +
> +    vdev = virtio_rng_init(&pci_dev->qdev,&proxy->rng);
> +    virtio_init_pci(proxy, vdev,
> +                    PCI_VENDOR_ID_REDHAT_QUMRANET,
> +                    PCI_DEVICE_ID_VIRTIO_RNG,
> +                    PCI_CLASS_OTHERS,
> +                    0x00);
> +
> +    return 0;
> +}
> +
>   static PCIDeviceInfo virtio_info[] = {
>       {
>           .qdev.name = "virtio-blk-pci",
> @@ -606,6 +623,18 @@ static PCIDeviceInfo virtio_info[] = {
>           },
>           .qdev.reset = virtio_pci_reset,
>       },{
> +        .qdev.name = "virtio-rng-pci",
> +        .qdev.size = sizeof(VirtIOPCIProxy),
> +        .init      = virtio_rng_init_pci,
> +        .exit      = virtio_exit_pci,
> +        .qdev.props = (Property[]) {
> +            DEFINE_PROP_CHR("chardev",     VirtIOPCIProxy, rng.chrdev),
> +            DEFINE_PROP_SIZE("rate",       VirtIOPCIProxy, rng.rate, 0),
> +            DEFINE_PROP_STRING("protocol", VirtIOPCIProxy, rng.proto),
> +            DEFINE_PROP_END_OF_LIST(),
> +        },
> +        .qdev.reset = virtio_pci_reset,
> +    },{
>           /* end of list */
>       }
>   };
> diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
> new file mode 100644
> index 0000000..154ea76
> --- /dev/null
> +++ b/hw/virtio-rng.c
> @@ -0,0 +1,195 @@
> +/*
> + * Virtio RNG Device
> + *
> + * Copyright Collabora 2009
> + *
> + * Authors:
> + *  Ian Molton<address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw.h"
> +#include "qemu-char.h"
> +#include "virtio.h"
> +#include "virtio-rng.h"
> +#include "rng.h"
> +#include<sys/time.h>
> +
> +typedef struct VirtIORng
> +{
> +    VirtIODevice vdev;
> +    VirtQueue *vq;
> +    CharDriverState *chr;
> +    struct timeval last;
> +    int rate;
> +    int egd;
> +    int entropy_remaining;
> +    int pool;
> +} VirtIORng;
> +
> +/* Maximum size of the buffer the guest expects */
> +#define BUFF_MAX 64
> +
> +/* EGD protocol - we only use this command */
> +#define EGD_READ_BLOCK 0x2
> +
> +#define EGD_MAX_BLOCK_SIZE 255
> +#define EGD_MAX_REQUESTS 3
> +#define EGD_MAX_POOL_SIZE (EGD_MAX_BLOCK_SIZE * (EGD_MAX_REQUESTS-1))
> +
> +static inline void req_entropy(VirtIORng *s)
> +{
> +    static const unsigned char entropy_rq[2] = { EGD_READ_BLOCK,
> +                                                 EGD_MAX_BLOCK_SIZE };
> +    if (s->egd) {
> +        /* Let the socket buffer up the incoming data for us. Max block
> size
> +           for EGD protocol is (stupidly) 255, so make sure we always
> have a
> +           block pending for performance. We can have 3 outstanding
> buffers */
>    

Whitespace damaged (broken mailer?).

> +        if (s->pool<= EGD_MAX_POOL_SIZE) {
> +            s->chr->chr_write(s->chr, entropy_rq, sizeof(entropy_rq));
> +            s->pool += EGD_MAX_BLOCK_SIZE;
> +        }
> +    } else {
> +        s->pool = BUFF_MAX;
> +    }
> +}
> +
> +static int vrng_can_read(void *opaque)
> +{
> +    VirtIORng *s = (VirtIORng *) opaque;
> +    struct timeval now, d;
> +    int max_entropy;
> +
> +    if (!virtio_queue_ready(s->vq) ||
> +        !(s->vdev.status&  VIRTIO_CONFIG_S_DRIVER_OK) ||
> +        virtio_queue_empty(s->vq))
> +        return 0;
>    

Missing braces (CODING_STYLE).

> +    req_entropy(s);
> +
> +    if (s->rate) {
> +        qemu_gettimeofday(&now);
> +        timersub(&now,&s->last,&d);
> +        if (d.tv_sec * 1000000 + d.tv_usec>  1000000) {
> +            s->entropy_remaining = s->rate;
> +            s->last = now;
> +        }
>    

Should be based on rt_clock

> +        max_entropy = MIN(s->pool, s->entropy_remaining);
> +    } else {
> +        max_entropy = s->pool;
> +    }
> +
> +    /* current implementations have a 64 byte buffer.
> +     * We fall back to a one byte per read if there is not enough room.
> +     */
> +    max_entropy = MIN(max_entropy, BUFF_MAX);
> +    if (max_entropy) {
> +        if (virtqueue_avail_bytes(s->vq, max_entropy, 0))
> +            return max_entropy;
> +        if (virtqueue_avail_bytes(s->vq, 1, 0))
> +            return 1;
>    

Missing braces (CODING_STYLE).

> +    }
> +    return 0;
> +}
> +
> +static void vrng_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    VirtIORng *s = (VirtIORng *) opaque;
> +    VirtQueueElement elem;
> +    int offset = 0;
> +
> +    /* The current kernel implementation has only one outstanding input
> +     * buffer of 64 bytes.
> +     */
> +    while (offset<  size) {
> +        int i = 0;
> +        if (!virtqueue_pop(s->vq,&elem))
> +                break;
>    

Braces.

> +        while (offset<  size&&  i<  elem.in_num) {
> +            int len = MIN(elem.in_sg[i].iov_len, size - offset);
> +            memcpy(elem.in_sg[i].iov_base, buf + offset, len);
> +            offset += len;
> +            i++;
> +        }
> +        virtqueue_push(s->vq,&elem, size);
> +    }
> +
> +    if (s->rate)
> +        s->entropy_remaining -= size;
>    

Braces (lots more too).

> +    s->pool -= size;
> +
> +    virtio_notify(&s->vdev, s->vq);
> +}
> +
> +static void vrng_event(void *opaque, int event)
> +{
> +
> +}
> +
> +
> +
> +static void virtio_rng_handle(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    /* Nothing to do - we push data when its available */
> +}
> +
> +static uint32_t virtio_rng_get_features(VirtIODevice *vdev, uint32_t
> features)
> +{
> +    return 0;
> +}
> +
> +static void virtio_rng_save(QEMUFile *f, void *opaque)
> +{
> +    VirtIORng *s = opaque;
> +
> +    virtio_save(&s->vdev, f);
> +}
> +
> +static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    VirtIORng *s = opaque;
> +
> +    if (version_id != 1)
> +        return -EINVAL;
> +
> +    virtio_load(&s->vdev, f);
> +    return 0;
> +}
> +
> +VirtIODevice *virtio_rng_init(DeviceState *dev, RNGConf *rngdev)
> +{
> +    VirtIORng *s;
> +    s = (VirtIORng *)virtio_common_init("virtio-rng",
> +                                            VIRTIO_ID_RNG,
> +                                            0, sizeof(VirtIORng));
> +
> +    if (!s)
> +        return NULL;
> +
> +    s->vdev.get_features = virtio_rng_get_features;
> +
> +    s->vq = virtio_add_queue(&s->vdev, 128, virtio_rng_handle);
> +    s->chr = rngdev->chrdev;
> +    s->rate = rngdev->rate;
> +    gettimeofday(&s->last, NULL);
> +
> +    if(rngdev->proto&&  !strncmp(rngdev->proto, "egd", 3))
> +        s->egd = 1;
> +
> +#ifdef DEBUG
>    

It's more typical to introduce a debugging macro (like dprintf()) and 
then have a single define at the top of the file to enable the macro.

This will make conversion to a more unified logging mechanism a lot 
easier down the road.

Regards,

Anthony Liguori

> +    printf("entropy being read from %s", rngdev->chrdev->label);
> +    if(s->rate)
> +        printf(" at %d bytes/sec max.", s->rate);
> +    printf(" protocol: %s\n", s->egd?"egd":"raw");
> +#endif
> +
> +    qemu_chr_add_handlers(s->chr, vrng_can_read, vrng_read, vrng_event, s);
> +
> +    register_savevm("virtio-rng", -1, 1, virtio_rng_save,
> virtio_rng_load, s);
> +
> +    return&s->vdev;
> +}
> +
> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
> new file mode 100644
> index 0000000..bc4d2d8
> --- /dev/null
> +++ b/hw/virtio-rng.h
> @@ -0,0 +1,19 @@
> +/*
> + * Virtio RNG Support
> + *
> + * Copyright Collabora 2009
> + *
> + * Authors:
> + *  Ian Molton<address@hidden>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.  See
> + * the COPYING file in the top-level directory.
> + *
> + */
> +#ifndef _QEMU_VIRTIO_RNG_H
> +#define _QEMU_VIRTIO_RNG_H
> +
> +/* The ID for virtio console */
> +#define VIRTIO_ID_RNG 4
> +
> +#endif
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 3baa2a3..749b2a6 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -16,6 +16,7 @@
>
>   #include "hw.h"
>   #include "net.h"
> +#include "rng.h"
>   #include "qdev.h"
>   #include "sysemu.h"
>   #include "block_int.h"
> @@ -174,6 +175,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev,
> BlockConf *conf);
>   VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf);
>   VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports);
>   VirtIODevice *virtio_balloon_init(DeviceState *dev);
> +VirtIODevice *virtio_rng_init(DeviceState *dev, RNGConf *rngdev);
>
>   void virtio_net_exit(VirtIODevice *vdev);
>
> diff --git a/rng.h b/rng.h
> new file mode 100644
> index 0000000..14ae7e0
> --- /dev/null
> +++ b/rng.h
> @@ -0,0 +1,14 @@
> +#ifndef QEMU_RNG_H
> +#define QEMU_RNG_H
> +
> +#include "qemu-option.h"
> +
> +/* qdev rng properties */
> +
> +typedef struct RNGConf {
> +    CharDriverState *chrdev;
> +    uint64_t rate;
> +    char *proto;
> +} RNGConf;
> +
> +#endif
>