[v3,1/1] virtio-rng: hardware random number generator device

The Linux kernel already has a virtio-rng driver, this is the device
implementation.

When the guest asks for entropy from the virtio hwrng, it puts a buffer
in the vq.  We then put entropy into that buffer, and push it back to
the guest.

The chardev connected to this device is fed the data to be sent to the
guest.

Invocation is simple:

  $ qemu ... -device virtio-rng-pci,chardev=foo

In the guest, we see

  $ cat /sys/devices/virtual/misc/hw_random/rng_available
  virtio

  $ cat /sys/devices/virtual/misc/hw_random/rng_current
  virtio

  # cat /dev/hwrng

Simply feeding /dev/urandom from the host to the chardev is sufficient:

  $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
             -device virtio-rng,chardev=foo

  $ nc -U /tmp/foo < /dev/urandom

A QMP event is sent for interested apps to monitor activity and send the
appropriate number of bytes that get asked by the guest:

  {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
   "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/Makefile.objs     |    1 +
 hw/pci.h             |    1 +
 hw/s390-virtio-bus.c |   35 +++++++++
 hw/s390-virtio-bus.h |    2 +
 hw/virtio-pci.c      |   51 +++++++++++++
 hw/virtio-pci.h      |    2 +
 hw/virtio-rng.c      |  200 ++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/virtio-rng.h      |   24 ++++++
 hw/virtio.h          |    3 +
 monitor.c            |    4 +-
 monitor.h            |    1 +
 11 files changed, 323 insertions(+), 1 deletions(-)
 create mode 100644 hw/virtio-rng.c
 create mode 100644 hw/virtio-rng.h

On Wed, Jun 20, 2012 at 12:29:32PM +0530, Amit Shah wrote:
> The Linux kernel already has a virtio-rng driver, this is the device
> implementation.
> 
> When the guest asks for entropy from the virtio hwrng, it puts a buffer
> in the vq.  We then put entropy into that buffer, and push it back to
> the guest.
> 
> The chardev connected to this device is fed the data to be sent to the
> guest.
> 
> Invocation is simple:
> 
>   $ qemu ... -device virtio-rng-pci,chardev=foo
> 
> In the guest, we see
> 
>   $ cat /sys/devices/virtual/misc/hw_random/rng_available
>   virtio
> 
>   $ cat /sys/devices/virtual/misc/hw_random/rng_current
>   virtio
> 
>   # cat /dev/hwrng
> 
> Simply feeding /dev/urandom from the host to the chardev is sufficient:
> 
>   $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
>              -device virtio-rng,chardev=foo
> 
>   $ nc -U /tmp/foo < /dev/urandom
> 
> A QMP event is sent for interested apps to monitor activity and send the
> appropriate number of bytes that get asked by the guest:
> 
>   {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
>    "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}
> 
> Signed-off-by: Amit Shah <amit.shah@redhat.com>

ACK to this from a libvirt design requirements POV.

Regards,
Daniel

On 06/20/2012 01:59 AM, Amit Shah wrote:
> The Linux kernel already has a virtio-rng driver, this is the device
> implementation.
>
> When the guest asks for entropy from the virtio hwrng, it puts a buffer
> in the vq.  We then put entropy into that buffer, and push it back to
> the guest.
>
> The chardev connected to this device is fed the data to be sent to the
> guest.
>
> Invocation is simple:
>
>    $ qemu ... -device virtio-rng-pci,chardev=foo
>
> In the guest, we see
>
>    $ cat /sys/devices/virtual/misc/hw_random/rng_available
>    virtio
>
>    $ cat /sys/devices/virtual/misc/hw_random/rng_current
>    virtio
>
>    # cat /dev/hwrng
>
> Simply feeding /dev/urandom from the host to the chardev is sufficient:
>
>    $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
>               -device virtio-rng,chardev=foo
>
>    $ nc -U /tmp/foo<  /dev/urandom
>
> A QMP event is sent for interested apps to monitor activity and send the
> appropriate number of bytes that get asked by the guest:
>
>    {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
>     "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}

Nack.

Use a protocol.  This is not what QMP events are designed for!

No human is going to launch nc to a unix domain socket to launch QEMU.  That's a 
silly use-case to design for.

Regards,

Anthony Liguori

>
> Signed-off-by: Amit Shah<amit.shah@redhat.com>
> ---
>   hw/Makefile.objs     |    1 +
>   hw/pci.h             |    1 +
>   hw/s390-virtio-bus.c |   35 +++++++++
>   hw/s390-virtio-bus.h |    2 +
>   hw/virtio-pci.c      |   51 +++++++++++++
>   hw/virtio-pci.h      |    2 +
>   hw/virtio-rng.c      |  200 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   hw/virtio-rng.h      |   24 ++++++
>   hw/virtio.h          |    3 +
>   monitor.c            |    4 +-
>   monitor.h            |    1 +
>   11 files changed, 323 insertions(+), 1 deletions(-)
>   create mode 100644 hw/virtio-rng.c
>   create mode 100644 hw/virtio-rng.h
>
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index 3d77259..4634637 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -1,6 +1,7 @@
>   hw-obj-y = usb/ ide/
>   hw-obj-y += loader.o
>   hw-obj-$(CONFIG_VIRTIO) += virtio-console.o
> +hw-obj-$(CONFIG_VIRTIO) += virtio-rng.o
>   hw-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>   hw-obj-y += fw_cfg.o
>   hw-obj-$(CONFIG_PCI) += pci.o pci_bridge.o pci_bridge_dev.o
> diff --git a/hw/pci.h b/hw/pci.h
> index 7f223c0..cdcbe1d 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -76,6 +76,7 @@
>   #define PCI_DEVICE_ID_VIRTIO_BALLOON     0x1002
>   #define PCI_DEVICE_ID_VIRTIO_CONSOLE     0x1003
>   #define PCI_DEVICE_ID_VIRTIO_SCSI        0x1004
> +#define PCI_DEVICE_ID_VIRTIO_RNG         0x1005
>
>   #define FMT_PCIBUS                      PRIx64
>
> diff --git a/hw/s390-virtio-bus.c b/hw/s390-virtio-bus.c
> index 4d49b96..0f6638f 100644
> --- a/hw/s390-virtio-bus.c
> +++ b/hw/s390-virtio-bus.c
> @@ -26,6 +26,7 @@
>   #include "loader.h"
>   #include "elf.h"
>   #include "hw/virtio.h"
> +#include "hw/virtio-rng.h"
>   #include "hw/virtio-serial.h"
>   #include "hw/virtio-net.h"
>   #include "hw/sysbus.h"
> @@ -206,6 +207,18 @@ static int s390_virtio_scsi_init(VirtIOS390Device *dev)
>       return s390_virtio_device_init(dev, vdev);
>   }
>
> +static int s390_virtio_rng_init(VirtIOS390Device *dev)
> +{
> +    VirtIODevice *vdev;
> +
> +    vdev = virtio_rng_init((DeviceState *)dev,&dev->rng);
> +    if (!vdev) {
> +        return -1;
> +    }
> +
> +    return s390_virtio_device_init(dev, vdev);
> +}
> +
>   static uint64_t s390_virtio_device_vq_token(VirtIOS390Device *dev, int vq)
>   {
>       ram_addr_t token_off;
> @@ -447,6 +460,27 @@ static TypeInfo s390_virtio_serial = {
>       .class_init    = s390_virtio_serial_class_init,
>   };
>
> +static Property s390_virtio_rng_properties[] = {
> +    DEFINE_PROP_CHR("chardev", VirtIOS390Device, rng.chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void s390_virtio_rng_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
> +
> +    k->init = s390_virtio_rng_init;
> +    dc->props = s390_virtio_rng_properties;
> +}
> +
> +static TypeInfo s390_virtio_rng = {
> +    .name          = "virtio-rng-s390",
> +    .parent        = TYPE_VIRTIO_S390_DEVICE,
> +    .instance_size = sizeof(VirtIOS390Device),
> +    .class_init    = s390_virtio_rng_class_init,
> +};
> +
>   static int s390_virtio_busdev_init(DeviceState *dev)
>   {
>       VirtIOS390Device *_dev = (VirtIOS390Device *)dev;
> @@ -527,6 +561,7 @@ static void s390_virtio_register_types(void)
>       type_register_static(&s390_virtio_blk);
>       type_register_static(&s390_virtio_net);
>       type_register_static(&s390_virtio_scsi);
> +    type_register_static(&s390_virtio_rng);
>       type_register_static(&s390_virtio_bridge_info);
>   }
>
> diff --git a/hw/s390-virtio-bus.h b/hw/s390-virtio-bus.h
> index 4873134..a83afe7 100644
> --- a/hw/s390-virtio-bus.h
> +++ b/hw/s390-virtio-bus.h
> @@ -19,6 +19,7 @@
>
>   #include "virtio-blk.h"
>   #include "virtio-net.h"
> +#include "virtio-rng.h"
>   #include "virtio-serial.h"
>   #include "virtio-scsi.h"
>
> @@ -75,6 +76,7 @@ struct VirtIOS390Device {
>       virtio_serial_conf serial;
>       virtio_net_conf net;
>       VirtIOSCSIConf scsi;
> +    VirtIORNGConf rng;
>   };
>
>   typedef struct VirtIOS390Bus {
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 9342eed..6643139 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -933,6 +933,28 @@ static int virtio_balloon_exit_pci(PCIDevice *pci_dev)
>       return virtio_exit_pci(pci_dev);
>   }
>
> +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);
> +    if (!vdev) {
> +        return -1;
> +    }
> +    virtio_init_pci(proxy, vdev);
> +    return 0;
> +}
> +
> +static int virtio_rng_exit_pci(PCIDevice *pci_dev)
> +{
> +    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> +
> +    virtio_pci_stop_ioeventfd(proxy);
> +    virtio_rng_exit(proxy->vdev);
> +    return virtio_exit_pci(pci_dev);
> +}
> +
>   static Property virtio_blk_properties[] = {
>       DEFINE_PROP_HEX32("class", VirtIOPCIProxy, class_code, 0),
>       DEFINE_BLOCK_PROPERTIES(VirtIOPCIProxy, blk.conf),
> @@ -1061,6 +1083,34 @@ static TypeInfo virtio_balloon_info = {
>       .class_init    = virtio_balloon_class_init,
>   };
>
> +static Property virtio_rng_properties[] = {
> +    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
> +    DEFINE_PROP_CHR("chardev", VirtIOPCIProxy, rng.chr),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void virtio_rng_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> +    k->init = virtio_rng_init_pci;
> +    k->exit = virtio_rng_exit_pci;
> +    k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
> +    k->device_id = PCI_DEVICE_ID_VIRTIO_RNG;
> +    k->revision = VIRTIO_PCI_ABI_VERSION;
> +    k->class_id = PCI_CLASS_OTHERS;
> +    dc->reset = virtio_pci_reset;
> +    dc->props = virtio_rng_properties;
> +}
> +
> +static TypeInfo virtio_rng_info = {
> +    .name          = "virtio-rng-pci",
> +    .parent        = TYPE_PCI_DEVICE,
> +    .instance_size = sizeof(VirtIOPCIProxy),
> +    .class_init    = virtio_rng_class_init,
> +};
> +
>   static int virtio_scsi_init_pci(PCIDevice *pci_dev)
>   {
>       VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
> @@ -1122,6 +1172,7 @@ static void virtio_pci_register_types(void)
>       type_register_static(&virtio_serial_info);
>       type_register_static(&virtio_balloon_info);
>       type_register_static(&virtio_scsi_info);
> +    type_register_static(&virtio_rng_info);
>   }
>
>   type_init(virtio_pci_register_types)
> diff --git a/hw/virtio-pci.h b/hw/virtio-pci.h
> index 91b791b..88df0ea 100644
> --- a/hw/virtio-pci.h
> +++ b/hw/virtio-pci.h
> @@ -17,6 +17,7 @@
>
>   #include "virtio-blk.h"
>   #include "virtio-net.h"
> +#include "virtio-rng.h"
>   #include "virtio-serial.h"
>   #include "virtio-scsi.h"
>
> @@ -47,6 +48,7 @@ typedef struct {
>       virtio_serial_conf serial;
>       virtio_net_conf net;
>       VirtIOSCSIConf scsi;
> +    VirtIORNGConf rng;
>       bool ioeventfd_disabled;
>       bool ioeventfd_started;
>       VirtIOIRQFD *vector_irqfd;
> diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c
> new file mode 100644
> index 0000000..bb87514
> --- /dev/null
> +++ b/hw/virtio-rng.c
> @@ -0,0 +1,200 @@
> +/*
> + * A virtio device implementing a hardware random number generator.
> + *
> + * Copyright 2012 Red Hat, Inc.
> + * Copyright 2012 Amit Shah<amit.shah@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 "iov.h"
> +#include "monitor.h"
> +#include "qdev.h"
> +#include "qjson.h"
> +#include "virtio.h"
> +#include "virtio-rng.h"
> +
> +typedef struct VirtIORNG {
> +    VirtIODevice vdev;
> +
> +    DeviceState *qdev;
> +
> +    /* Only one vq - guest puts buffer(s) on it when it needs entropy */
> +    VirtQueue *vq;
> +    VirtQueueElement elem;
> +
> +    /* Config data for the device -- currently only chardev */
> +    VirtIORNGConf *conf;
> +
> +    /* Whether we've popped a vq element into 'elem' above */
> +    bool popped;
> +} VirtIORNG;
> +
> +static bool is_guest_ready(VirtIORNG *vrng)
> +{
> +    if (virtio_queue_ready(vrng->vq)
> +&&  (vrng->vdev.status&  VIRTIO_CONFIG_S_DRIVER_OK)) {
> +        return true;
> +    }
> +    return false;
> +}
> +
> +static void send_qevent_for_entropy(size_t size)
> +{
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> +                              size);
> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> +    qobject_decref(data);
> +}
> +
> +static size_t pop_an_elem(VirtIORNG *vrng)
> +{
> +    size_t size;
> +
> +    if (!vrng->popped&&  !virtqueue_pop(vrng->vq,&vrng->elem)) {
> +        return 0;
> +    }
> +    vrng->popped = true;
> +
> +    size = iov_size(vrng->elem.in_sg, vrng->elem.in_num);
> +    return size;
> +}
> +
> +static void handle_input(VirtIODevice *vdev, VirtQueue *vq)
> +{
> +    VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
> +    size_t size;
> +
> +    size = pop_an_elem(vrng);
> +    if (size) {
> +        send_qevent_for_entropy(size);
> +    }
> +}
> +
> +static int chr_can_read(void *opaque)
> +{
> +    VirtIORNG *vrng = opaque;
> +
> +    if (!is_guest_ready(vrng)) {
> +        return 0;
> +    }
> +    return pop_an_elem(vrng);
> +}
> +
> +/* Send data from a char device over to the guest */
> +static void chr_read(void *opaque, const uint8_t *buf, int size)
> +{
> +    VirtIORNG *vrng = opaque;
> +    size_t len;
> +    int offset;
> +
> +    if (!is_guest_ready(vrng)) {
> +        return;
> +    }
> +
> +    offset = 0;
> +    while (offset<  size) {
> +        if (!pop_an_elem(vrng)) {
> +            break;
> +        }
> +        len = iov_from_buf(vrng->elem.in_sg, vrng->elem.in_num,
> +                           buf + offset, 0, size - offset);
> +        offset += len;
> +
> +        virtqueue_push(vrng->vq,&vrng->elem, len);
> +        vrng->popped = false;
> +    }
> +    virtio_notify(&vrng->vdev, vrng->vq);
> +
> +    /*
> +     * Lastly, if we had multiple elems queued by the guest, and we
> +     * didn't have enough data to fill them all, indicate we want more
> +     * data.  We can't stick this into chr_can_read(), as it'll just
> +     * end up spamming the management app.
> +     */
> +    len = pop_an_elem(vrng);
> +    if (len) {
> +        send_qevent_for_entropy(len);
> +    }
> +}
> +
> +static uint32_t get_features(VirtIODevice *vdev, uint32_t f)
> +{
> +    return f;
> +}
> +
> +static void virtio_rng_save(QEMUFile *f, void *opaque)
> +{
> +    VirtIORNG *vrng = opaque;
> +
> +    virtio_save(&vrng->vdev, f);
> +
> +    qemu_put_byte(f, vrng->popped);
> +    if (vrng->popped) {
> +        qemu_put_buffer(f, (unsigned char *)&vrng->elem,
> +                        sizeof(vrng->elem));
> +    }
> +}
> +
> +static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id)
> +{
> +    VirtIORNG *vrng = opaque;
> +
> +    if (version_id != 1) {
> +        return -EINVAL;
> +    }
> +    virtio_load(&vrng->vdev, f);
> +
> +    vrng->popped = qemu_get_byte(f);
> +    if (vrng->popped) {
> +        qemu_get_buffer(f, (unsigned char *)&vrng->elem,
> +                        sizeof(vrng->elem));
> +        virtqueue_map_sg(vrng->elem.in_sg, vrng->elem.in_addr,
> +                         vrng->elem.in_num, 1);
> +        virtqueue_map_sg(vrng->elem.out_sg, vrng->elem.out_addr,
> +                         vrng->elem.out_num, 0);
> +    }
> +    return 0;
> +}
> +
> +VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf)
> +{
> +    VirtIORNG *vrng;
> +    VirtIODevice *vdev;
> +
> +    if (!conf->chr) {
> +        error_report("chardev property expected");
> +        return NULL;
> +    }
> +
> +    vdev = virtio_common_init("virtio-rng", VIRTIO_ID_RNG, 0,
> +                              sizeof(VirtIORNG));
> +
> +    vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
> +
> +    vrng->vq = virtio_add_queue(vdev, 8, handle_input);
> +    vrng->vdev.get_features = get_features;
> +
> +    vrng->qdev = dev;
> +    vrng->conf = conf;
> +    vrng->popped = false;
> +    register_savevm(dev, "virtio-rng", -1, 1, virtio_rng_save,
> +                    virtio_rng_load, vrng);
> +
> +    qemu_chr_add_handlers(conf->chr, chr_can_read, chr_read, NULL, vrng);
> +
> +    return vdev;
> +}
> +
> +void virtio_rng_exit(VirtIODevice *vdev)
> +{
> +    VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
> +
> +    qemu_chr_add_handlers(vrng->conf->chr, NULL, NULL, NULL, NULL);
> +    unregister_savevm(vrng->qdev, "virtio-rng", vrng);
> +    virtio_cleanup(vdev);
> +}
> diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h
> new file mode 100644
> index 0000000..b132acd
> --- /dev/null
> +++ b/hw/virtio-rng.h
> @@ -0,0 +1,24 @@
> +/*
> + * Virtio RNG Support
> + *
> + * Copyright Red Hat, Inc. 2012
> + * Copyright Amit Shah<amit.shah@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_RNG_H
> +#define _QEMU_VIRTIO_RNG_H
> +
> +#include "qemu-char.h"
> +
> +/* The Virtio ID for the virtio rng device */
> +#define VIRTIO_ID_RNG    4
> +
> +struct VirtIORNGConf {
> +    CharDriverState *chr;
> +};
> +
> +#endif
> diff --git a/hw/virtio.h b/hw/virtio.h
> index 85aabe5..b4b5bf6 100644
> --- a/hw/virtio.h
> +++ b/hw/virtio.h
> @@ -201,6 +201,8 @@ VirtIODevice *virtio_serial_init(DeviceState *dev, virtio_serial_conf *serial);
>   VirtIODevice *virtio_balloon_init(DeviceState *dev);
>   typedef struct VirtIOSCSIConf VirtIOSCSIConf;
>   VirtIODevice *virtio_scsi_init(DeviceState *dev, VirtIOSCSIConf *conf);
> +typedef struct VirtIORNGConf VirtIORNGConf;
> +VirtIODevice *virtio_rng_init(DeviceState *dev, VirtIORNGConf *conf);
>   #ifdef CONFIG_LINUX
>   VirtIODevice *virtio_9p_init(DeviceState *dev, V9fsConf *conf);
>   #endif
> @@ -211,6 +213,7 @@ void virtio_blk_exit(VirtIODevice *vdev);
>   void virtio_serial_exit(VirtIODevice *vdev);
>   void virtio_balloon_exit(VirtIODevice *vdev);
>   void virtio_scsi_exit(VirtIODevice *vdev);
> +void virtio_rng_exit(VirtIODevice *vdev);
>
>   #define DEFINE_VIRTIO_COMMON_FEATURES(_state, _field) \
>   	DEFINE_PROP_BIT("indirect_desc", _state, _field, \
> diff --git a/monitor.c b/monitor.c
> index f6107ba..8220267 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -458,6 +458,7 @@ static const char *monitor_event_names[] = {
>       [QEVENT_SUSPEND] = "SUSPEND",
>       [QEVENT_WAKEUP] = "WAKEUP",
>       [QEVENT_BALLOON_CHANGE] = "BALLOON_CHANGE",
> +    [QEVENT_ENTROPY_NEEDED] = "ENTROPY_NEEDED",
>   };
>   QEMU_BUILD_BUG_ON(ARRAY_SIZE(monitor_event_names) != QEVENT_MAX)
>
> @@ -590,10 +591,11 @@ monitor_protocol_event_throttle(MonitorEvent event,
>   static void monitor_protocol_event_init(void)
>   {
>       qemu_mutex_init(&monitor_event_state_lock);
> -    /* Limit RTC&  BALLOON events to 1 per second */
> +    /* Limit the following events to 1 per second */
>       monitor_protocol_event_throttle(QEVENT_RTC_CHANGE, 1000);
>       monitor_protocol_event_throttle(QEVENT_BALLOON_CHANGE, 1000);
>       monitor_protocol_event_throttle(QEVENT_WATCHDOG, 1000);
> +    monitor_protocol_event_throttle(QEVENT_ENTROPY_NEEDED, 1000);
>   }
>
>   /**
> diff --git a/monitor.h b/monitor.h
> index 5f4de1b..4bd9197 100644
> --- a/monitor.h
> +++ b/monitor.h
> @@ -42,6 +42,7 @@ typedef enum MonitorEvent {
>       QEVENT_SUSPEND,
>       QEVENT_WAKEUP,
>       QEVENT_BALLOON_CHANGE,
> +    QEVENT_ENTROPY_NEEDED,
>
>       /* Add to 'monitor_event_names' array in monitor.c when
>        * defining new events here */

On (Wed) 20 Jun 2012 [16:29:22], Anthony Liguori wrote:
> On 06/20/2012 01:59 AM, Amit Shah wrote:
> >The Linux kernel already has a virtio-rng driver, this is the device
> >implementation.
> >
> >When the guest asks for entropy from the virtio hwrng, it puts a buffer
> >in the vq.  We then put entropy into that buffer, and push it back to
> >the guest.
> >
> >The chardev connected to this device is fed the data to be sent to the
> >guest.
> >
> >Invocation is simple:
> >
> >   $ qemu ... -device virtio-rng-pci,chardev=foo
> >
> >In the guest, we see
> >
> >   $ cat /sys/devices/virtual/misc/hw_random/rng_available
> >   virtio
> >
> >   $ cat /sys/devices/virtual/misc/hw_random/rng_current
> >   virtio
> >
> >   # cat /dev/hwrng
> >
> >Simply feeding /dev/urandom from the host to the chardev is sufficient:
> >
> >   $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
> >              -device virtio-rng,chardev=foo
> >
> >   $ nc -U /tmp/foo<  /dev/urandom
> >
> >A QMP event is sent for interested apps to monitor activity and send the
> >appropriate number of bytes that get asked by the guest:
> >
> >   {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
> >    "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}
> 
> Nack.
> 
> Use a protocol.

How does one write a program on Linux to get random numbers?

He uses /dev/random, of course.

What's the protocol?  It's just a stream of bytes.

What is egd?  From their website:

  A userspace substitute for /dev/random, written in perl. 

It was written for systems that do not have /dev/random.  How does a
userspace program give out information to those who ask for it?  It
depends on how it gets designed.  These people decided to create some
protocol.  Is there a specification on any protocol for consuming
random numbers?  No, there isn't.

If anyone is so inclined to use a "protocol" for something as simple
as a stream of bytes, he/she is most welcome to write a simple little
script that reads the "protocol" and gives the output to a qemu
chardev.

QEMU has no business whatsoever to be bound to protocols which have no
significance nor specification nor wide-spread usage.  It's just
surprising that we're debating this!

So what are you really thinking about here?  There's no magic that
needs to be done to consume random numbers.

>  This is not what QMP events are designed for!

Anthony, please read my responses to the last thread.  The QMP event
is *not* mandatory to be used.

> No human is going to launch nc to a unix domain socket to launch
> QEMU.  That's a silly use-case to design for.

You're right in two ways: 1) libvirt is going to be the main tool
launching qemu, and libvirt is happy with the design.  2) humans
launching qemu by hand are not going to have much use for a hwrng in
the guest.  If they do, however, the easiest (and, indeed, the best)
way is the way I show above.

		Amit

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 06/20/2012 01:59 AM, Amit Shah wrote:
>> The Linux kernel already has a virtio-rng driver, this is the device
>> implementation.
>>
>> When the guest asks for entropy from the virtio hwrng, it puts a buffer
>> in the vq.  We then put entropy into that buffer, and push it back to
>> the guest.
>>
>> The chardev connected to this device is fed the data to be sent to the
>> guest.
>>
>> Invocation is simple:
>>
>>    $ qemu ... -device virtio-rng-pci,chardev=foo
>>
>> In the guest, we see
>>
>>    $ cat /sys/devices/virtual/misc/hw_random/rng_available
>>    virtio
>>
>>    $ cat /sys/devices/virtual/misc/hw_random/rng_current
>>    virtio
>>
>>    # cat /dev/hwrng
>>
>> Simply feeding /dev/urandom from the host to the chardev is sufficient:
>>
>>    $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
>>               -device virtio-rng,chardev=foo
>>
>>    $ nc -U /tmp/foo<  /dev/urandom
>>
>> A QMP event is sent for interested apps to monitor activity and send the
>> appropriate number of bytes that get asked by the guest:
>>
>>    {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
>>     "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}
>
> Nack.
>
> Use a protocol.  This is not what QMP events are designed for!
>
> No human is going to launch nc to a unix domain socket to launch QEMU.
> That's a silly use-case to design for.

To be honest, I'm a bit surprised to see working code that got an ACK
from the guys with the problem it solves rejected out of hand over
something that feels like artistic license to me.

On 06/22/2012 07:12 AM, Markus Armbruster wrote:
> Anthony Liguori<anthony@codemonkey.ws>  writes:
>> Nack.
>>
>> Use a protocol.  This is not what QMP events are designed for!
>>
>> No human is going to launch nc to a unix domain socket to launch QEMU.
>> That's a silly use-case to design for.
>
> To be honest, I'm a bit surprised to see working code that got an ACK
> from the guys with the problem it solves rejected out of hand over
> something that feels like artistic license to me.

This is an ABI!  We have to support it for the rest of time.  Everything else is 
a detail that is fixable but ABIs need to not suck from the beginning.

And using a QMP event here is sucks.  It disappoints me that this is even 
something I need to explain.

QMP events occur over a single socket.  If you trigger them from guest initiated 
activities (that have no intrinsic rate limit), you run into a situation where 
the guest could flood the management tool and/or queue infinite amounts of 
memory (because events have to be queued before they're sent).  So we have rate 
limiting for QMP events.

That means QMP events (like this one) are unreliable.  But since QMP events 
aren't acked, there's no way for the management tool to know whether a QMP event 
was dropped or not.  So you can run into the following scenario:

- Guest sends randomness request for 10 bytes
- QMP event gets sent for 10 bytes
- Guest sends randomness request for 4 bytes
- QMP is dropped

Now what happens?  With the current virtio-rng, nothing.  It gets stuck in 
read() for ever.  Now what do we do?

The solution is simple--don't use a shared resource for virtio-rng events such 
that you don't need to worry about rate limiting or event queueing.  You process 
one request, then one piece of data, all over the same socket.

Regards,

Anthony Liguori

On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
> On 06/22/2012 07:12 AM, Markus Armbruster wrote:
> >Anthony Liguori<anthony@codemonkey.ws>  writes:
> >>Nack.
> >>
> >>Use a protocol.  This is not what QMP events are designed for!
> >>
> >>No human is going to launch nc to a unix domain socket to launch QEMU.
> >>That's a silly use-case to design for.
> >
> >To be honest, I'm a bit surprised to see working code that got an ACK
> >from the guys with the problem it solves rejected out of hand over
> >something that feels like artistic license to me.
> 
> This is an ABI!  We have to support it for the rest of time.
> Everything else is a detail that is fixable but ABIs need to not
> suck from the beginning.
> 
> And using a QMP event here is sucks.  It disappoints me that this is
> even something I need to explain.
> 
> QMP events occur over a single socket.  If you trigger them from
> guest initiated activities (that have no intrinsic rate limit), you
> run into a situation where the guest could flood the management tool
> and/or queue infinite amounts of memory (because events have to be
> queued before they're sent).  So we have rate limiting for QMP
> events.
> 
> That means QMP events (like this one) are unreliable. 

No it doesn't. As it stands currently, the only events that are
rate limited, are those where there is no state information to
loose. ie, the new event completely superceeds the old event
without loosing any information.

>                                                       But since QMP
> events aren't acked, there's no way for the management tool to know
> whether a QMP event was dropped or not.  So you can run into the
> following scenario:
> 
> - Guest sends randomness request for 10 bytes
> - QMP event gets sent for 10 bytes
> - Guest sends randomness request for 4 bytes
> - QMP is dropped
> 
> Now what happens?  With the current virtio-rng, nothing.  It gets
> stuck in read() for ever.  Now what do we do?

The RNG event will not be able to use the generic rate limiting
since it has state associated with it. The rate limiting of the
RNG QMP event will need to take account of this state, ie it
will have to accumulate the byte count of any events dropped for
rate limiting:

  - Guest sends randomness request for 10 bytes
  - QMP event gets sent for 10 bytes
  - Guest sends randomness request for 4 bytes
  - QMP is dropped
  - Guest sends randomness request for 8 bytes
  - QMP event gets sent for 12 bytes


Daniel

On 06/22/2012 07:31 AM, Daniel P. Berrange wrote:
> On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
>> On 06/22/2012 07:12 AM, Markus Armbruster wrote:
>>> Anthony Liguori<anthony@codemonkey.ws>   writes:
>>>> Nack.
>>>>
>>>> Use a protocol.  This is not what QMP events are designed for!
>>>>
>>>> No human is going to launch nc to a unix domain socket to launch QEMU.
>>>> That's a silly use-case to design for.
>>>
>>> To be honest, I'm a bit surprised to see working code that got an ACK
>> >from the guys with the problem it solves rejected out of hand over
>>> something that feels like artistic license to me.
>>
>> This is an ABI!  We have to support it for the rest of time.
>> Everything else is a detail that is fixable but ABIs need to not
>> suck from the beginning.
>>
>> And using a QMP event here is sucks.  It disappoints me that this is
>> even something I need to explain.
>>
>> QMP events occur over a single socket.  If you trigger them from
>> guest initiated activities (that have no intrinsic rate limit), you
>> run into a situation where the guest could flood the management tool
>> and/or queue infinite amounts of memory (because events have to be
>> queued before they're sent).  So we have rate limiting for QMP
>> events.
>>
>> That means QMP events (like this one) are unreliable.
>
> No it doesn't. As it stands currently, the only events that are
> rate limited, are those where there is no state information to
> loose. ie, the new event completely superceeds the old event
> without loosing any information.
>
>>                                                        But since QMP
>> events aren't acked, there's no way for the management tool to know
>> whether a QMP event was dropped or not.  So you can run into the
>> following scenario:
>>
>> - Guest sends randomness request for 10 bytes
>> - QMP event gets sent for 10 bytes
>> - Guest sends randomness request for 4 bytes
>> - QMP is dropped
>>
>> Now what happens?  With the current virtio-rng, nothing.  It gets
>> stuck in read() for ever.  Now what do we do?
>
> The RNG event will not be able to use the generic rate limiting
> since it has state associated with it. The rate limiting of the
> RNG QMP event will need to take account of this state, ie it
> will have to accumulate the byte count of any events dropped for
> rate limiting:
>
>    - Guest sends randomness request for 10 bytes
>    - QMP event gets sent for 10 bytes
>    - Guest sends randomness request for 4 bytes
>    - QMP is dropped
>    - Guest sends randomness request for 8 bytes
>    - QMP event gets sent for 12 bytes

BTW, in the current design, there's no way to tell *which* virtio-rng device 
needs entropy if you have multiple virtio-rng devices.

All of these problems are naturally solved using a protocol over a CharDriverState.

Regards,

Anthony Liguori

>
>
> Daniel

On Fri, Jun 22, 2012 at 07:58:53AM -0500, Anthony Liguori wrote:
> On 06/22/2012 07:31 AM, Daniel P. Berrange wrote:
> >On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
> >>On 06/22/2012 07:12 AM, Markus Armbruster wrote:
> >>>Anthony Liguori<anthony@codemonkey.ws>   writes:
> >>>>Nack.
> >>>>
> >>>>Use a protocol.  This is not what QMP events are designed for!
> >>>>
> >>>>No human is going to launch nc to a unix domain socket to launch QEMU.
> >>>>That's a silly use-case to design for.
> >>>
> >>>To be honest, I'm a bit surprised to see working code that got an ACK
> >>>from the guys with the problem it solves rejected out of hand over
> >>>something that feels like artistic license to me.
> >>
> >>This is an ABI!  We have to support it for the rest of time.
> >>Everything else is a detail that is fixable but ABIs need to not
> >>suck from the beginning.
> >>
> >>And using a QMP event here is sucks.  It disappoints me that this is
> >>even something I need to explain.
> >>
> >>QMP events occur over a single socket.  If you trigger them from
> >>guest initiated activities (that have no intrinsic rate limit), you
> >>run into a situation where the guest could flood the management tool
> >>and/or queue infinite amounts of memory (because events have to be
> >>queued before they're sent).  So we have rate limiting for QMP
> >>events.
> >>
> >>That means QMP events (like this one) are unreliable.
> >
> >No it doesn't. As it stands currently, the only events that are
> >rate limited, are those where there is no state information to
> >loose. ie, the new event completely superceeds the old event
> >without loosing any information.
> >
> >>                                                       But since QMP
> >>events aren't acked, there's no way for the management tool to know
> >>whether a QMP event was dropped or not.  So you can run into the
> >>following scenario:
> >>
> >>- Guest sends randomness request for 10 bytes
> >>- QMP event gets sent for 10 bytes
> >>- Guest sends randomness request for 4 bytes
> >>- QMP is dropped
> >>
> >>Now what happens?  With the current virtio-rng, nothing.  It gets
> >>stuck in read() for ever.  Now what do we do?
> >
> >The RNG event will not be able to use the generic rate limiting
> >since it has state associated with it. The rate limiting of the
> >RNG QMP event will need to take account of this state, ie it
> >will have to accumulate the byte count of any events dropped for
> >rate limiting:
> >
> >   - Guest sends randomness request for 10 bytes
> >   - QMP event gets sent for 10 bytes
> >   - Guest sends randomness request for 4 bytes
> >   - QMP is dropped
> >   - Guest sends randomness request for 8 bytes
> >   - QMP event gets sent for 12 bytes
> 
> BTW, in the current design, there's no way to tell *which*
> virtio-rng device needs entropy if you have multiple virtio-rng
> devices.

Oh, that's a good point.

> All of these problems are naturally solved using a protocol over a CharDriverState.

Can we at least agree on merging a patch which just includes the
raw chardev backend support for virtio-rng ? ie drop the QMP
event for now, so we can make some step forward.

As mentioned in the previous thread, I see no issue with
later implementing an alternate protocol on the chardev
backend eg as we have raw vs telnet mode for serial ports,
we ought to be able to have a choice of raw vs egd mode for
virtio-rng backends

Daniel

On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> On Fri, Jun 22, 2012 at 07:58:53AM -0500, Anthony Liguori wrote:
>> On 06/22/2012 07:31 AM, Daniel P. Berrange wrote:
>>> On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
>>>> On 06/22/2012 07:12 AM, Markus Armbruster wrote:
>>>>> Anthony Liguori<anthony@codemonkey.ws>    writes:
>>>>>> Nack.
>>>>>>
>>>>>> Use a protocol.  This is not what QMP events are designed for!
>>>>>>
>>>>>> No human is going to launch nc to a unix domain socket to launch QEMU.
>>>>>> That's a silly use-case to design for.
>>>>>
>>>>> To be honest, I'm a bit surprised to see working code that got an ACK
>>>> >from the guys with the problem it solves rejected out of hand over
>>>>> something that feels like artistic license to me.
>>>>
>>>> This is an ABI!  We have to support it for the rest of time.
>>>> Everything else is a detail that is fixable but ABIs need to not
>>>> suck from the beginning.
>>>>
>>>> And using a QMP event here is sucks.  It disappoints me that this is
>>>> even something I need to explain.
>>>>
>>>> QMP events occur over a single socket.  If you trigger them from
>>>> guest initiated activities (that have no intrinsic rate limit), you
>>>> run into a situation where the guest could flood the management tool
>>>> and/or queue infinite amounts of memory (because events have to be
>>>> queued before they're sent).  So we have rate limiting for QMP
>>>> events.
>>>>
>>>> That means QMP events (like this one) are unreliable.
>>>
>>> No it doesn't. As it stands currently, the only events that are
>>> rate limited, are those where there is no state information to
>>> loose. ie, the new event completely superceeds the old event
>>> without loosing any information.
>>>
>>>>                                                        But since QMP
>>>> events aren't acked, there's no way for the management tool to know
>>>> whether a QMP event was dropped or not.  So you can run into the
>>>> following scenario:
>>>>
>>>> - Guest sends randomness request for 10 bytes
>>>> - QMP event gets sent for 10 bytes
>>>> - Guest sends randomness request for 4 bytes
>>>> - QMP is dropped
>>>>
>>>> Now what happens?  With the current virtio-rng, nothing.  It gets
>>>> stuck in read() for ever.  Now what do we do?
>>>
>>> The RNG event will not be able to use the generic rate limiting
>>> since it has state associated with it. The rate limiting of the
>>> RNG QMP event will need to take account of this state, ie it
>>> will have to accumulate the byte count of any events dropped for
>>> rate limiting:
>>>
>>>    - Guest sends randomness request for 10 bytes
>>>    - QMP event gets sent for 10 bytes
>>>    - Guest sends randomness request for 4 bytes
>>>    - QMP is dropped
>>>    - Guest sends randomness request for 8 bytes
>>>    - QMP event gets sent for 12 bytes
>>
>> BTW, in the current design, there's no way to tell *which*
>> virtio-rng device needs entropy if you have multiple virtio-rng
>> devices.
>
> Oh, that's a good point.
>
>> All of these problems are naturally solved using a protocol over a CharDriverState.
>
> Can we at least agree on merging a patch which just includes the
> raw chardev backend support for virtio-rng ? ie drop the QMP
> event for now, so we can make some step forward.

All we need to do to support EGD is instead of doing:

+    QObject *data;
+
+    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
+                              size);
+    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
+    qobject_decref(data);

Do:

     while (size > 0) {
         uint8_t partial_size = MIN(255, size);
         uint8_t msg[2] = { 0x02, partial_size };

         qemu_chr_write(s->chr, msg, sizeof(msg));

         size -= partial_size;
     }

And that's it.  It's an extremely simple protocol to support.  It would actually 
reduce the total size of the patch.

> As mentioned in the previous thread, I see no issue with
> later implementing an alternate protocol on the chardev
> backend eg as we have raw vs telnet mode for serial ports,
> we ought to be able to have a choice of raw vs egd mode for
> virtio-rng backends

I don't really understand how raw mode works other than reading as much from 
/dev/urandom as possible and filling the socket buffer buffer with it.

I think the only two modes that make sense are EGD over a socket and direct open 
of /dev/urandom.

But I think the EGD mode is the more important of the two.

Regards,

Anthony Liguori

> Daniel

On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >On Fri, Jun 22, 2012 at 07:58:53AM -0500, Anthony Liguori wrote:
> >>On 06/22/2012 07:31 AM, Daniel P. Berrange wrote:
> >>>On Fri, Jun 22, 2012 at 07:22:51AM -0500, Anthony Liguori wrote:
> >>>>On 06/22/2012 07:12 AM, Markus Armbruster wrote:
> >>>>>Anthony Liguori<anthony@codemonkey.ws>    writes:
> >>>>>>Nack.
> >>>>>>
> >>>>>>Use a protocol.  This is not what QMP events are designed for!
> >>>>>>
> >>>>>>No human is going to launch nc to a unix domain socket to launch QEMU.
> >>>>>>That's a silly use-case to design for.
> >>>>>
> >>>>>To be honest, I'm a bit surprised to see working code that got an ACK
> >>>>>from the guys with the problem it solves rejected out of hand over
> >>>>>something that feels like artistic license to me.
> >>>>
> >>>>This is an ABI!  We have to support it for the rest of time.
> >>>>Everything else is a detail that is fixable but ABIs need to not
> >>>>suck from the beginning.
> >>>>
> >>>>And using a QMP event here is sucks.  It disappoints me that this is
> >>>>even something I need to explain.
> >>>>
> >>>>QMP events occur over a single socket.  If you trigger them from
> >>>>guest initiated activities (that have no intrinsic rate limit), you
> >>>>run into a situation where the guest could flood the management tool
> >>>>and/or queue infinite amounts of memory (because events have to be
> >>>>queued before they're sent).  So we have rate limiting for QMP
> >>>>events.
> >>>>
> >>>>That means QMP events (like this one) are unreliable.
> >>>
> >>>No it doesn't. As it stands currently, the only events that are
> >>>rate limited, are those where there is no state information to
> >>>loose. ie, the new event completely superceeds the old event
> >>>without loosing any information.

So I'm assuming you don't have a problem with this anymore.

> >>>>                                                       But since QMP
> >>>>events aren't acked, there's no way for the management tool to know
> >>>>whether a QMP event was dropped or not.  So you can run into the
> >>>>following scenario:
> >>>>
> >>>>- Guest sends randomness request for 10 bytes
> >>>>- QMP event gets sent for 10 bytes
> >>>>- Guest sends randomness request for 4 bytes
> >>>>- QMP is dropped
> >>>>
> >>>>Now what happens?  With the current virtio-rng, nothing.  It gets
> >>>>stuck in read() for ever.  Now what do we do?
> >>>
> >>>The RNG event will not be able to use the generic rate limiting
> >>>since it has state associated with it. The rate limiting of the
> >>>RNG QMP event will need to take account of this state, ie it
> >>>will have to accumulate the byte count of any events dropped for
> >>>rate limiting:
> >>>
> >>>   - Guest sends randomness request for 10 bytes
> >>>   - QMP event gets sent for 10 bytes
> >>>   - Guest sends randomness request for 4 bytes
> >>>   - QMP is dropped
> >>>   - Guest sends randomness request for 8 bytes
> >>>   - QMP event gets sent for 12 bytes
> >>
> >>BTW, in the current design, there's no way to tell *which*
> >>virtio-rng device needs entropy if you have multiple virtio-rng
> >>devices.
> >
> >Oh, that's a good point.

But easily fixed.

> >>All of these problems are naturally solved using a protocol over a CharDriverState.
> >
> >Can we at least agree on merging a patch which just includes the
> >raw chardev backend support for virtio-rng ? ie drop the QMP
> >event for now, so we can make some step forward.
> 
> All we need to do to support EGD is instead of doing:
> 
> +    QObject *data;
> +
> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> +                              size);
> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> +    qobject_decref(data);
> 
> Do:
> 
>     while (size > 0) {
>         uint8_t partial_size = MIN(255, size);
>         uint8_t msg[2] = { 0x02, partial_size };
> 
>         qemu_chr_write(s->chr, msg, sizeof(msg));
> 
>         size -= partial_size;
>     }
> 
> And that's it.  It's an extremely simple protocol to support.  It
> would actually reduce the total size of the patch.

So I now get what your objection is, and that is because of an
assumption you're making which is incorrect.

An OS asking for 5038 bytes of entropy is doing just that: asking for
those bytes.  That doesn't mean a hardware device can provide it with
that much entropy.  So, the hardware device here will just provide
whatever is available, and the OS has to be happy with what it got.
If it got 200 bytes from the device, the OS is then free to demand
even 7638 bytes more, but it may not get anything for quite a while.

(This is exactly how things work with /dev/random and /dev/urandom
too, btw.  And /dev/urandom was invented for apps which can't wait for
all the data they're asking to come to them.)

All this is expected.  Keep in mind that the hardware-generated
entropy is read from /dev/hwrng, which again is a /dev/random-like
interface, and /dev/hwrng entropy can be fed into /dev/random by using
rngd.  All that is standard stuff.

So: the QMP event here is just a note to libvirt that the guest is
asking for data, and as an additional hint, it also mentions how much
data the guest wants right now.  No party is in any kind of contract
to provide exactly what's asked for.

> >As mentioned in the previous thread, I see no issue with
> >later implementing an alternate protocol on the chardev
> >backend eg as we have raw vs telnet mode for serial ports,
> >we ought to be able to have a choice of raw vs egd mode for
> >virtio-rng backends
> 
> I don't really understand how raw mode works other than reading as
> much from /dev/urandom as possible and filling the socket buffer
> buffer with it.

That's all that's needed for the simplest (while being as effective as
any other) implementation to work.

> I think the only two modes that make sense are EGD over a socket and
> direct open of /dev/urandom.

Directly wiring /dev/urandom via chardev is more flexible, and doesn't
involve anything else.  No chardev gimmicks as well.

> But I think the EGD mode is the more important of the two.

Why?  There's nothing standard about it.

		Amit

On 06/22/2012 01:50 PM, Amit Shah wrote:
> On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
>> On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
>>>
>>> Oh, that's a good point.
>
> But easily fixed.

Of course, except that now we have to maintain compatibility so some hideous 
hack goes in.

This is what fundamentally makes using events a bad approach.  There are more 
problems lurking.  This is the fundamental complexity of having two 
non-synchronized communication channels when you're trying to synchronize some 
sort of activity.

BTW, despite what danpb mentioned, you are rate limiting entropy requests in 
this patch series....

>>>> All of these problems are naturally solved using a protocol over a CharDriverState.
>>>
>>> Can we at least agree on merging a patch which just includes the
>>> raw chardev backend support for virtio-rng ? ie drop the QMP
>>> event for now, so we can make some step forward.
>>
>> All we need to do to support EGD is instead of doing:
>>
>> +    QObject *data;
>> +
>> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
>> +                              size);
>> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
>> +    qobject_decref(data);
>>
>> Do:
>>
>>      while (size>  0) {
>>          uint8_t partial_size = MIN(255, size);
>>          uint8_t msg[2] = { 0x02, partial_size };
>>
>>          qemu_chr_write(s->chr, msg, sizeof(msg));
>>
>>          size -= partial_size;
>>      }
>>
>> And that's it.  It's an extremely simple protocol to support.  It
>> would actually reduce the total size of the patch.
>
> So I now get what your objection is, and that is because of an
> assumption you're making which is incorrect.
>
> An OS asking for 5038 bytes of entropy is doing just that: asking for
> those bytes.  That doesn't mean a hardware device can provide it with
> that much entropy.  So, the hardware device here will just provide
> whatever is available, and the OS has to be happy with what it got.
> If it got 200 bytes from the device, the OS is then free to demand
> even 7638 bytes more, but it may not get anything for quite a while.
>
> (This is exactly how things work with /dev/random and /dev/urandom
> too, btw.  And /dev/urandom was invented for apps which can't wait for
> all the data they're asking to come to them.)

As it turns out, the EGD protocol also has a mechanism to ask for ask much 
entropy as is available at the current moment.  It also has a mechanism to query 
the amount of available entropy.

And no, you're assertion that we don't care about how much entropy the guest is 
requesting is wrong.  If we lose entropy requests, then we never know if the 
guest is in a state where it's expecting entropy and we're not sending it.

Consider:

- Guest requests entropy (X bytes)
- libvirt sees request
- libvirt sends X bytes to guest
- Guest requests entropy (Y bytes), QEMU filters due to rate limiting

The guest will never request entropy again and libvirt will never send entropy 
again.  The device is effectively dead-locked.

And none of this is a problem using the EGD protocol like I illustrated above.

> All this is expected.  Keep in mind that the hardware-generated
> entropy is read from /dev/hwrng, which again is a /dev/random-like
> interface, and /dev/hwrng entropy can be fed into /dev/random by using
> rngd.  All that is standard stuff.
>
> So: the QMP event here is just a note to libvirt that the guest is
> asking for data, and as an additional hint, it also mentions how much
> data the guest wants right now.  No party is in any kind of contract
> to provide exactly what's asked for.

But you are not using it as a hint!

There is no way for in the virtio-rng protocol for libvirt to just send data to 
the guest out of the kindness of it's heart.  virtio is fundamentally a 
request-response protocol.  Guest requests MUST be processed by something that 
generates a response.

This should be plumbed as a request-response protocol (i.e. EGD).

>>> As mentioned in the previous thread, I see no issue with
>>> later implementing an alternate protocol on the chardev
>>> backend eg as we have raw vs telnet mode for serial ports,
>>> we ought to be able to have a choice of raw vs egd mode for
>>> virtio-rng backends
>>
>> I don't really understand how raw mode works other than reading as
>> much from /dev/urandom as possible and filling the socket buffer
>> buffer with it.
>
> That's all that's needed for the simplest (while being as effective as
> any other) implementation to work.
>
>> I think the only two modes that make sense are EGD over a socket and
>> direct open of /dev/urandom.
>
> Directly wiring /dev/urandom via chardev is more flexible, and doesn't
> involve anything else.  No chardev gimmicks as well.
>
>> But I think the EGD mode is the more important of the two.
>
> Why?  There's nothing standard about it.

Except it's been around for over a decade and is widely supported.

Regards,

Anthony Liguori

>
> 		Amit

On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
> On 06/22/2012 01:50 PM, Amit Shah wrote:
> >On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> >>On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >>>
> >>>Oh, that's a good point.
> >
> >But easily fixed.
> 
> Of course, except that now we have to maintain compatibility so some
> hideous hack goes in.
> 
> This is what fundamentally makes using events a bad approach.  There
> are more problems lurking.  This is the fundamental complexity of
> having two non-synchronized communication channels when you're
> trying to synchronize some sort of activity.
> 
> BTW, despite what danpb mentioned, you are rate limiting entropy
> requests in this patch series....
> 
> >>>>All of these problems are naturally solved using a protocol over a CharDriverState.
> >>>
> >>>Can we at least agree on merging a patch which just includes the
> >>>raw chardev backend support for virtio-rng ? ie drop the QMP
> >>>event for now, so we can make some step forward.
> >>
> >>All we need to do to support EGD is instead of doing:
> >>
> >>+    QObject *data;
> >>+
> >>+    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> >>+                              size);
> >>+    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> >>+    qobject_decref(data);
> >>
> >>Do:
> >>
> >>     while (size>  0) {
> >>         uint8_t partial_size = MIN(255, size);
> >>         uint8_t msg[2] = { 0x02, partial_size };
> >>
> >>         qemu_chr_write(s->chr, msg, sizeof(msg));
> >>
> >>         size -= partial_size;
> >>     }
> >>
> >>And that's it.  It's an extremely simple protocol to support.  It
> >>would actually reduce the total size of the patch.
> >
> >So I now get what your objection is, and that is because of an
> >assumption you're making which is incorrect.
> >
> >An OS asking for 5038 bytes of entropy is doing just that: asking for
> >those bytes.  That doesn't mean a hardware device can provide it with
> >that much entropy.  So, the hardware device here will just provide
> >whatever is available, and the OS has to be happy with what it got.
> >If it got 200 bytes from the device, the OS is then free to demand
> >even 7638 bytes more, but it may not get anything for quite a while.
> >
> >(This is exactly how things work with /dev/random and /dev/urandom
> >too, btw.  And /dev/urandom was invented for apps which can't wait for
> >all the data they're asking to come to them.)
> 
> As it turns out, the EGD protocol also has a mechanism to ask for
> ask much entropy as is available at the current moment.  It also has
> a mechanism to query the amount of available entropy.
> 
> And no, you're assertion that we don't care about how much entropy
> the guest is requesting is wrong.  If we lose entropy requests, then
> we never know if the guest is in a state where it's expecting
> entropy and we're not sending it.
> 
> Consider:
> 
> - Guest requests entropy (X bytes)
> - libvirt sees request
> - libvirt sends X bytes to guest
> - Guest requests entropy (Y bytes), QEMU filters due to rate limiting
> 
> The guest will never request entropy again and libvirt will never
> send entropy again.  The device is effectively dead-locked.

WRT the impl of rate limiting Amit has used in this patch, you
are correct, however, this is not how QEMU should be rate
limiting this event. As mentioned in an earlier reply in this
thread, any rate limited /must/ work as follows:

   - Guest sends randomness request for 10 bytes
   - QMP event gets sent for 10 bytes
   - Guest sends randomness request for 4 bytes
   - QMP is dropped
   - Guest sends randomness request for 8 bytes
   - QMP event gets sent for 12 bytes

ie, the byte count for any events which are dropped, must be added to
the byte count in the next emitted event. Also as Amit mentioned in his
reply to me, it should take account of multiple devices, or we should
limit QEMU to 1 single RNG device per guest, as we do for the balloon
driver.

Daniel

On 06/25/2012 07:10 AM, Daniel P. Berrange wrote:
> On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
>> On 06/22/2012 01:50 PM, Amit Shah wrote:
>>> On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
>>>> On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
>>>>>
>>>>> Oh, that's a good point.
>>>
>>> But easily fixed.
>>
>> Of course, except that now we have to maintain compatibility so some
>> hideous hack goes in.
>>
>> This is what fundamentally makes using events a bad approach.  There
>> are more problems lurking.  This is the fundamental complexity of
>> having two non-synchronized communication channels when you're
>> trying to synchronize some sort of activity.
>>
>> BTW, despite what danpb mentioned, you are rate limiting entropy
>> requests in this patch series....
>>
>>>>>> All of these problems are naturally solved using a protocol over a CharDriverState.
>>>>>
>>>>> Can we at least agree on merging a patch which just includes the
>>>>> raw chardev backend support for virtio-rng ? ie drop the QMP
>>>>> event for now, so we can make some step forward.
>>>>
>>>> All we need to do to support EGD is instead of doing:
>>>>
>>>> +    QObject *data;
>>>> +
>>>> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
>>>> +                              size);
>>>> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
>>>> +    qobject_decref(data);
>>>>
>>>> Do:
>>>>
>>>>      while (size>   0) {
>>>>          uint8_t partial_size = MIN(255, size);
>>>>          uint8_t msg[2] = { 0x02, partial_size };
>>>>
>>>>          qemu_chr_write(s->chr, msg, sizeof(msg));
>>>>
>>>>          size -= partial_size;
>>>>      }
>>>>
>>>> And that's it.  It's an extremely simple protocol to support.  It
>>>> would actually reduce the total size of the patch.
>>>
>>> So I now get what your objection is, and that is because of an
>>> assumption you're making which is incorrect.
>>>
>>> An OS asking for 5038 bytes of entropy is doing just that: asking for
>>> those bytes.  That doesn't mean a hardware device can provide it with
>>> that much entropy.  So, the hardware device here will just provide
>>> whatever is available, and the OS has to be happy with what it got.
>>> If it got 200 bytes from the device, the OS is then free to demand
>>> even 7638 bytes more, but it may not get anything for quite a while.
>>>
>>> (This is exactly how things work with /dev/random and /dev/urandom
>>> too, btw.  And /dev/urandom was invented for apps which can't wait for
>>> all the data they're asking to come to them.)
>>
>> As it turns out, the EGD protocol also has a mechanism to ask for
>> ask much entropy as is available at the current moment.  It also has
>> a mechanism to query the amount of available entropy.
>>
>> And no, you're assertion that we don't care about how much entropy
>> the guest is requesting is wrong.  If we lose entropy requests, then
>> we never know if the guest is in a state where it's expecting
>> entropy and we're not sending it.
>>
>> Consider:
>>
>> - Guest requests entropy (X bytes)
>> - libvirt sees request
>> - libvirt sends X bytes to guest
>> - Guest requests entropy (Y bytes), QEMU filters due to rate limiting
>>
>> The guest will never request entropy again and libvirt will never
>> send entropy again.  The device is effectively dead-locked.
>
> WRT the impl of rate limiting Amit has used in this patch, you
> are correct, however, this is not how QEMU should be rate
> limiting this event. As mentioned in an earlier reply in this
> thread, any rate limited /must/ work as follows:
>
>     - Guest sends randomness request for 10 bytes
>     - QMP event gets sent for 10 bytes
>     - Guest sends randomness request for 4 bytes
>     - QMP is dropped
>     - Guest sends randomness request for 8 bytes
>     - QMP event gets sent for 12 bytes
>
> ie, the byte count for any events which are dropped, must be added to
> the byte count in the next emitted event. Also as Amit mentioned in his
> reply to me, it should take account of multiple devices, or we should
> limit QEMU to 1 single RNG device per guest, as we do for the balloon
> driver.

QMP events are not meant to be used like this.  They are posted events and since 
we cannot know if there is something connected to the monitor, it's always 
possible for the backend to lose the information associated with an event.

Events really should just be used to indicate, "hey, you should go query 
information from QEMU now".

Even what you suggest above doesn't handle the case where a management 
application restarts.

If you were going to do this via QMP, you'd want to introduce a command to query 
the total outstanding requested entropy for a given device and then send a event 
whenever that value changes.

But again, it's silly to use QMP for this.  Using an inline protocol simplifies 
things quite a bit.

Regards,

Anthony Liguori

>
> Daniel

On Mon, Jun 25, 2012 at 07:22:13AM -0500, Anthony Liguori wrote:
> On 06/25/2012 07:10 AM, Daniel P. Berrange wrote:
> >On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
> >>On 06/22/2012 01:50 PM, Amit Shah wrote:
> >>>On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> >>>>On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >>>>>
> >>>>>Oh, that's a good point.
> >>>
> >>>But easily fixed.
> >>
> >>Of course, except that now we have to maintain compatibility so some
> >>hideous hack goes in.
> >>
> >>This is what fundamentally makes using events a bad approach.  There
> >>are more problems lurking.  This is the fundamental complexity of
> >>having two non-synchronized communication channels when you're
> >>trying to synchronize some sort of activity.
> >>
> >>BTW, despite what danpb mentioned, you are rate limiting entropy
> >>requests in this patch series....
> >>
> >>>>>>All of these problems are naturally solved using a protocol over a CharDriverState.
> >>>>>
> >>>>>Can we at least agree on merging a patch which just includes the
> >>>>>raw chardev backend support for virtio-rng ? ie drop the QMP
> >>>>>event for now, so we can make some step forward.
> >>>>
> >>>>All we need to do to support EGD is instead of doing:
> >>>>
> >>>>+    QObject *data;
> >>>>+
> >>>>+    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> >>>>+                              size);
> >>>>+    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> >>>>+    qobject_decref(data);
> >>>>
> >>>>Do:
> >>>>
> >>>>     while (size>   0) {
> >>>>         uint8_t partial_size = MIN(255, size);
> >>>>         uint8_t msg[2] = { 0x02, partial_size };
> >>>>
> >>>>         qemu_chr_write(s->chr, msg, sizeof(msg));
> >>>>
> >>>>         size -= partial_size;
> >>>>     }
> >>>>
> >>>>And that's it.  It's an extremely simple protocol to support.  It
> >>>>would actually reduce the total size of the patch.
> >>>
> >>>So I now get what your objection is, and that is because of an
> >>>assumption you're making which is incorrect.
> >>>
> >>>An OS asking for 5038 bytes of entropy is doing just that: asking for
> >>>those bytes.  That doesn't mean a hardware device can provide it with
> >>>that much entropy.  So, the hardware device here will just provide
> >>>whatever is available, and the OS has to be happy with what it got.
> >>>If it got 200 bytes from the device, the OS is then free to demand
> >>>even 7638 bytes more, but it may not get anything for quite a while.
> >>>
> >>>(This is exactly how things work with /dev/random and /dev/urandom
> >>>too, btw.  And /dev/urandom was invented for apps which can't wait for
> >>>all the data they're asking to come to them.)
> >>
> >>As it turns out, the EGD protocol also has a mechanism to ask for
> >>ask much entropy as is available at the current moment.  It also has
> >>a mechanism to query the amount of available entropy.
> >>
> >>And no, you're assertion that we don't care about how much entropy
> >>the guest is requesting is wrong.  If we lose entropy requests, then
> >>we never know if the guest is in a state where it's expecting
> >>entropy and we're not sending it.
> >>
> >>Consider:
> >>
> >>- Guest requests entropy (X bytes)
> >>- libvirt sees request
> >>- libvirt sends X bytes to guest
> >>- Guest requests entropy (Y bytes), QEMU filters due to rate limiting
> >>
> >>The guest will never request entropy again and libvirt will never
> >>send entropy again.  The device is effectively dead-locked.
> >
> >WRT the impl of rate limiting Amit has used in this patch, you
> >are correct, however, this is not how QEMU should be rate
> >limiting this event. As mentioned in an earlier reply in this
> >thread, any rate limited /must/ work as follows:
> >
> >    - Guest sends randomness request for 10 bytes
> >    - QMP event gets sent for 10 bytes
> >    - Guest sends randomness request for 4 bytes
> >    - QMP is dropped
> >    - Guest sends randomness request for 8 bytes
> >    - QMP event gets sent for 12 bytes
> >
> >ie, the byte count for any events which are dropped, must be added to
> >the byte count in the next emitted event. Also as Amit mentioned in his
> >reply to me, it should take account of multiple devices, or we should
> >limit QEMU to 1 single RNG device per guest, as we do for the balloon
> >driver.
> 
> QMP events are not meant to be used like this.  They are posted
> events and since we cannot know if there is something connected to
> the monitor, it's always possible for the backend to lose the
> information associated with an event.
> 
> Events really should just be used to indicate, "hey, you should go
> query information from QEMU now".
> 
> Even what you suggest above doesn't handle the case where a
> management application restarts.
> 
> If you were going to do this via QMP, you'd want to introduce a
> command to query the total outstanding requested entropy for a given
> device and then send a event whenever that value changes.
> 
> But again, it's silly to use QMP for this.  Using an inline protocol
> simplifies things quite a bit.

Ok, I agree that being rebust against mgmt layer restarts is a good
reason for not relying on QMP events for this, and that using the
EGD protocol would nicely solve this problem. So lets drop the QMP
event. I still think it is key to allow use of raw chardevs in
addition to EDGE though, so you can easily attached QEMU directly
to a /dev/urandom or an equivalent entropy source & just let it pull
as much as it likes.

Thus my suggestion for the syntax

  -chardev [regular chardev opts],mode=raw|egd

similar to how we allow 'telnet' protocol to be optionally turned on
for chardevs with serial/parallel ports


Daniel

On 06/25/2012 07:30 AM, Daniel P. Berrange wrote:
> On Mon, Jun 25, 2012 at 07:22:13AM -0500, Anthony Liguori wrote:
>> On 06/25/2012 07:10 AM, Daniel P. Berrange wrote:
>>> On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
>>>> On 06/22/2012 01:50 PM, Amit Shah wrote:
>>>>> On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
>>>>>> On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
>>>>>>>
>>>>>>> Oh, that's a good point.
>>>>>
>>>>> But easily fixed.
>>>>
>>>> Of course, except that now we have to maintain compatibility so some
>>>> hideous hack goes in.
>>>>
>>>> This is what fundamentally makes using events a bad approach.  There
>>>> are more problems lurking.  This is the fundamental complexity of
>>>> having two non-synchronized communication channels when you're
>>>> trying to synchronize some sort of activity.
>>>>
>>>> BTW, despite what danpb mentioned, you are rate limiting entropy
>>>> requests in this patch series....
>>>>
>>>>>>>> All of these problems are naturally solved using a protocol over a CharDriverState.
>>>>>>>
>>>>>>> Can we at least agree on merging a patch which just includes the
>>>>>>> raw chardev backend support for virtio-rng ? ie drop the QMP
>>>>>>> event for now, so we can make some step forward.
>>>>>>
>>>>>> All we need to do to support EGD is instead of doing:
>>>>>>
>>>>>> +    QObject *data;
>>>>>> +
>>>>>> +    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
>>>>>> +                              size);
>>>>>> +    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
>>>>>> +    qobject_decref(data);
>>>>>>
>>>>>> Do:
>>>>>>
>>>>>>      while (size>    0) {
>>>>>>          uint8_t partial_size = MIN(255, size);
>>>>>>          uint8_t msg[2] = { 0x02, partial_size };
>>>>>>
>>>>>>          qemu_chr_write(s->chr, msg, sizeof(msg));
>>>>>>
>>>>>>          size -= partial_size;
>>>>>>      }
>>>>>>
>>>>>> And that's it.  It's an extremely simple protocol to support.  It
>>>>>> would actually reduce the total size of the patch.
>>>>>
>>>>> So I now get what your objection is, and that is because of an
>>>>> assumption you're making which is incorrect.
>>>>>
>>>>> An OS asking for 5038 bytes of entropy is doing just that: asking for
>>>>> those bytes.  That doesn't mean a hardware device can provide it with
>>>>> that much entropy.  So, the hardware device here will just provide
>>>>> whatever is available, and the OS has to be happy with what it got.
>>>>> If it got 200 bytes from the device, the OS is then free to demand
>>>>> even 7638 bytes more, but it may not get anything for quite a while.
>>>>>
>>>>> (This is exactly how things work with /dev/random and /dev/urandom
>>>>> too, btw.  And /dev/urandom was invented for apps which can't wait for
>>>>> all the data they're asking to come to them.)
>>>>
>>>> As it turns out, the EGD protocol also has a mechanism to ask for
>>>> ask much entropy as is available at the current moment.  It also has
>>>> a mechanism to query the amount of available entropy.
>>>>
>>>> And no, you're assertion that we don't care about how much entropy
>>>> the guest is requesting is wrong.  If we lose entropy requests, then
>>>> we never know if the guest is in a state where it's expecting
>>>> entropy and we're not sending it.
>>>>
>>>> Consider:
>>>>
>>>> - Guest requests entropy (X bytes)
>>>> - libvirt sees request
>>>> - libvirt sends X bytes to guest
>>>> - Guest requests entropy (Y bytes), QEMU filters due to rate limiting
>>>>
>>>> The guest will never request entropy again and libvirt will never
>>>> send entropy again.  The device is effectively dead-locked.
>>>
>>> WRT the impl of rate limiting Amit has used in this patch, you
>>> are correct, however, this is not how QEMU should be rate
>>> limiting this event. As mentioned in an earlier reply in this
>>> thread, any rate limited /must/ work as follows:
>>>
>>>     - Guest sends randomness request for 10 bytes
>>>     - QMP event gets sent for 10 bytes
>>>     - Guest sends randomness request for 4 bytes
>>>     - QMP is dropped
>>>     - Guest sends randomness request for 8 bytes
>>>     - QMP event gets sent for 12 bytes
>>>
>>> ie, the byte count for any events which are dropped, must be added to
>>> the byte count in the next emitted event. Also as Amit mentioned in his
>>> reply to me, it should take account of multiple devices, or we should
>>> limit QEMU to 1 single RNG device per guest, as we do for the balloon
>>> driver.
>>
>> QMP events are not meant to be used like this.  They are posted
>> events and since we cannot know if there is something connected to
>> the monitor, it's always possible for the backend to lose the
>> information associated with an event.
>>
>> Events really should just be used to indicate, "hey, you should go
>> query information from QEMU now".
>>
>> Even what you suggest above doesn't handle the case where a
>> management application restarts.
>>
>> If you were going to do this via QMP, you'd want to introduce a
>> command to query the total outstanding requested entropy for a given
>> device and then send a event whenever that value changes.
>>
>> But again, it's silly to use QMP for this.  Using an inline protocol
>> simplifies things quite a bit.
>
> Ok, I agree that being rebust against mgmt layer restarts is a good
> reason for not relying on QMP events for this, and that using the
> EGD protocol would nicely solve this problem. So lets drop the QMP
> event. I still think it is key to allow use of raw chardevs in
> addition to EDGE though, so you can easily attached QEMU directly
> to a /dev/urandom or an equivalent entropy source&  just let it pull
> as much as it likes.
>
> Thus my suggestion for the syntax
>
>    -chardev [regular chardev opts],mode=raw|egd
>
> similar to how we allow 'telnet' protocol to be optionally turned on
> for chardevs with serial/parallel ports

Why not just do:

-device virtio-rng-pci[,file=/dev/urandom|chardev=foo]

?

I think that's better than doing chardev + protocol argument.

Since the 'file=' option is actually usable by humans.

Regards,

Anthony Liguori

>
>
> Daniel

On Mon, Jun 25, 2012 at 07:54:18AM -0500, Anthony Liguori wrote:
> On 06/25/2012 07:30 AM, Daniel P. Berrange wrote:
> >On Mon, Jun 25, 2012 at 07:22:13AM -0500, Anthony Liguori wrote:
> >>On 06/25/2012 07:10 AM, Daniel P. Berrange wrote:
> >>>On Fri, Jun 22, 2012 at 02:59:13PM -0500, Anthony Liguori wrote:
> >>>>On 06/22/2012 01:50 PM, Amit Shah wrote:
> >>>>>On (Fri) 22 Jun 2012 [08:44:52], Anthony Liguori wrote:
> >>>>>>On 06/22/2012 08:34 AM, Daniel P. Berrange wrote:
> >>>>>>>
> >>>>>>>Oh, that's a good point.
> >>>>>
> >>>>>But easily fixed.
> >>>>
> >>>>Of course, except that now we have to maintain compatibility so some
> >>>>hideous hack goes in.
> >>>>
> >>>>This is what fundamentally makes using events a bad approach.  There
> >>>>are more problems lurking.  This is the fundamental complexity of
> >>>>having two non-synchronized communication channels when you're
> >>>>trying to synchronize some sort of activity.
> >>>>
> >>>>BTW, despite what danpb mentioned, you are rate limiting entropy
> >>>>requests in this patch series....
> >>>>
> >>>>>>>>All of these problems are naturally solved using a protocol over a CharDriverState.
> >>>>>>>
> >>>>>>>Can we at least agree on merging a patch which just includes the
> >>>>>>>raw chardev backend support for virtio-rng ? ie drop the QMP
> >>>>>>>event for now, so we can make some step forward.
> >>>>>>
> >>>>>>All we need to do to support EGD is instead of doing:
> >>>>>>
> >>>>>>+    QObject *data;
> >>>>>>+
> >>>>>>+    data = qobject_from_jsonf("{ 'bytes': %" PRId64 " }",
> >>>>>>+                              size);
> >>>>>>+    monitor_protocol_event(QEVENT_ENTROPY_NEEDED, data);
> >>>>>>+    qobject_decref(data);
> >>>>>>
> >>>>>>Do:
> >>>>>>
> >>>>>>     while (size>    0) {
> >>>>>>         uint8_t partial_size = MIN(255, size);
> >>>>>>         uint8_t msg[2] = { 0x02, partial_size };
> >>>>>>
> >>>>>>         qemu_chr_write(s->chr, msg, sizeof(msg));
> >>>>>>
> >>>>>>         size -= partial_size;
> >>>>>>     }
> >>>>>>
> >>>>>>And that's it.  It's an extremely simple protocol to support.  It
> >>>>>>would actually reduce the total size of the patch.
> >>>>>
> >>>>>So I now get what your objection is, and that is because of an
> >>>>>assumption you're making which is incorrect.
> >>>>>
> >>>>>An OS asking for 5038 bytes of entropy is doing just that: asking for
> >>>>>those bytes.  That doesn't mean a hardware device can provide it with
> >>>>>that much entropy.  So, the hardware device here will just provide
> >>>>>whatever is available, and the OS has to be happy with what it got.
> >>>>>If it got 200 bytes from the device, the OS is then free to demand
> >>>>>even 7638 bytes more, but it may not get anything for quite a while.
> >>>>>
> >>>>>(This is exactly how things work with /dev/random and /dev/urandom
> >>>>>too, btw.  And /dev/urandom was invented for apps which can't wait for
> >>>>>all the data they're asking to come to them.)
> >>>>
> >>>>As it turns out, the EGD protocol also has a mechanism to ask for
> >>>>ask much entropy as is available at the current moment.  It also has
> >>>>a mechanism to query the amount of available entropy.
> >>>>
> >>>>And no, you're assertion that we don't care about how much entropy
> >>>>the guest is requesting is wrong.  If we lose entropy requests, then
> >>>>we never know if the guest is in a state where it's expecting
> >>>>entropy and we're not sending it.
> >>>>
> >>>>Consider:
> >>>>
> >>>>- Guest requests entropy (X bytes)
> >>>>- libvirt sees request
> >>>>- libvirt sends X bytes to guest
> >>>>- Guest requests entropy (Y bytes), QEMU filters due to rate limiting
> >>>>
> >>>>The guest will never request entropy again and libvirt will never
> >>>>send entropy again.  The device is effectively dead-locked.
> >>>
> >>>WRT the impl of rate limiting Amit has used in this patch, you
> >>>are correct, however, this is not how QEMU should be rate
> >>>limiting this event. As mentioned in an earlier reply in this
> >>>thread, any rate limited /must/ work as follows:
> >>>
> >>>    - Guest sends randomness request for 10 bytes
> >>>    - QMP event gets sent for 10 bytes
> >>>    - Guest sends randomness request for 4 bytes
> >>>    - QMP is dropped
> >>>    - Guest sends randomness request for 8 bytes
> >>>    - QMP event gets sent for 12 bytes
> >>>
> >>>ie, the byte count for any events which are dropped, must be added to
> >>>the byte count in the next emitted event. Also as Amit mentioned in his
> >>>reply to me, it should take account of multiple devices, or we should
> >>>limit QEMU to 1 single RNG device per guest, as we do for the balloon
> >>>driver.
> >>
> >>QMP events are not meant to be used like this.  They are posted
> >>events and since we cannot know if there is something connected to
> >>the monitor, it's always possible for the backend to lose the
> >>information associated with an event.
> >>
> >>Events really should just be used to indicate, "hey, you should go
> >>query information from QEMU now".
> >>
> >>Even what you suggest above doesn't handle the case where a
> >>management application restarts.
> >>
> >>If you were going to do this via QMP, you'd want to introduce a
> >>command to query the total outstanding requested entropy for a given
> >>device and then send a event whenever that value changes.
> >>
> >>But again, it's silly to use QMP for this.  Using an inline protocol
> >>simplifies things quite a bit.
> >
> >Ok, I agree that being rebust against mgmt layer restarts is a good
> >reason for not relying on QMP events for this, and that using the
> >EGD protocol would nicely solve this problem. So lets drop the QMP
> >event. I still think it is key to allow use of raw chardevs in
> >addition to EDGE though, so you can easily attached QEMU directly
> >to a /dev/urandom or an equivalent entropy source&  just let it pull
> >as much as it likes.
> >
> >Thus my suggestion for the syntax
> >
> >   -chardev [regular chardev opts],mode=raw|egd
> >
> >similar to how we allow 'telnet' protocol to be optionally turned on
> >for chardevs with serial/parallel ports
> 
> Why not just do:
> 
> -device virtio-rng-pci[,file=/dev/urandom|chardev=foo]
> 
> ?
> 
> I think that's better than doing chardev + protocol argument.
> 
> Since the 'file=' option is actually usable by humans.

This is mixing up frontend + backend config again, which is what
I thought we were trying to get away from, and is uneccessarily
restricting the flexibility in config to just a plain file in
the raw mode.

If we want a 'simple' option, then rather than polluting -device
config, then shouldn't we setup an alias

  -rng virtio,file=/dev/urandom

which QEMU auto-expands to

  -device virtio-rng-pci,chardev=foo
  -chardev id=foo,file=/dev/urandom

as we do for other types of option in QEMU

Regards,
Daniel

On 06/22/2012 07:06 AM, Amit Shah wrote:
> On (Wed) 20 Jun 2012 [16:29:22], Anthony Liguori wrote:
>> On 06/20/2012 01:59 AM, Amit Shah wrote:
>>> The Linux kernel already has a virtio-rng driver, this is the device
>>> implementation.
>>>
>>> When the guest asks for entropy from the virtio hwrng, it puts a buffer
>>> in the vq.  We then put entropy into that buffer, and push it back to
>>> the guest.
>>>
>>> The chardev connected to this device is fed the data to be sent to the
>>> guest.
>>>
>>> Invocation is simple:
>>>
>>>    $ qemu ... -device virtio-rng-pci,chardev=foo
>>>
>>> In the guest, we see
>>>
>>>    $ cat /sys/devices/virtual/misc/hw_random/rng_available
>>>    virtio
>>>
>>>    $ cat /sys/devices/virtual/misc/hw_random/rng_current
>>>    virtio
>>>
>>>    # cat /dev/hwrng
>>>
>>> Simply feeding /dev/urandom from the host to the chardev is sufficient:
>>>
>>>    $ qemu ... -chardev socket,path=/tmp/foo,server,nowait,id=foo \
>>>               -device virtio-rng,chardev=foo
>>>
>>>    $ nc -U /tmp/foo<   /dev/urandom
>>>
>>> A QMP event is sent for interested apps to monitor activity and send the
>>> appropriate number of bytes that get asked by the guest:
>>>
>>>    {"timestamp": {"seconds": 1337966878, "microseconds": 517009}, \
>>>     "event": "ENTROPY_NEEDED", "data": {"bytes": 64}}
>> Nack.
>>
>> Use a protocol.
> How does one write a program on Linux to get random numbers?
>
> He uses /dev/random, of course.
You could also use the nss freebl crypto library that provides a random 
number generator that for example seeds itself from /dev/urandom and 
then uses hash operations on the seed before it goes back to getting 
random numbers from /dev/urandom again. So, another idea:  call 
RNG_GenerateGlobalRandomBytes() to get the entropy.

    Stefan