[Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[Amit Shah]

Hello,

Here's the 3rd iteration of the virtio-rng device.  This update just
rebases the patch on top of current master.

Details on the patch in the commit message.

Please apply,

	Amit

v3:
 * rebase to master
   * Add file to hw/Makefile.objs instead of Makefile.objs
   * Rate-limit event to at most 1 per second
   * Update to slightly different way to define new QEVENTS

v2:
 * Remove hard-wiring to /dev/urandom
 * Use chardev for input
 * Add a QMP event for notifying listeners about entropy needed and
   the bytes asked for by the guest.
 * Add s390 code


Amit Shah (1):
  virtio-rng: hardware random number generator device

 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

-- 
1.7.7.6

[Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Amit Shah]

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

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Amit Shah]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Daniel P. Berrange]

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
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Amit Shah]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH v3 1/1] virtio-rng: hardware random number generator device

[Stefan Berger]

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

Re: [Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[H. Peter Anvin]

On 06/19/2012 11:59 PM, Amit Shah wrote:
> Hello,
>
> Here's the 3rd iteration of the virtio-rng device.  This update just
> rebases the patch on top of current master.
>
> Details on the patch in the commit message.
>

Hi everyone...

I just stumbled on this patchset after realizing that the virtio-rng 
support still is not even in the Qemu git tree even though the kernel 
side has been there for four years(!)

It seems this support has been stuck in "overengineering hell" for 
years.  I have to admit to having a bit of a confusion about what is so 
hard about reading from a file descriptor, which may return partial 
reads.  I understand the fairness argument, but I would argue that if it 
is an actual problem should be solved in the kernel and therefore 
benefit all types of applications rather than at the application level 
(which Qemu, effectively, is.)

However, one key error I spotted was using /dev/urandom.  Using 
/dev/urandom is a very serious cryptographic error, as well as 
completely pointless.

The whole point of this is to provided distilled entropy to the guest, 
so that it can seed true entropy into *its* entropy pool.  As such, 
using /dev/urandom is:

a) needlessly slow.  It will churn the host kernel PRNG needlessly,
    and not provide any backpressure when the host pool is already
    drained.  Using /dev/random instead will indicate that the host
    pool is drained, and not pass a bunch of PRNG noise across an
    expensive channel when the PRNG in the *guest* could do the PRNG
    expansion just as well.

b) cryptographically incorrect.  This will tell the guest that it
    is being provided with entropy that it doesn't actually have, which
    will mean the guest severely overestimates the entropy that it has
    available to it.  To put it differently: /dev/random in the guest
    will behave like (a very expensive version of) /dev/urandom from
    a cryptographic point of view.

The right interface to the host kernel, therefore, is /dev/random.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[Anthony Liguori]

"H. Peter Anvin" <hpa@zytor.com> writes:

> On 06/19/2012 11:59 PM, Amit Shah wrote:
>> Hello,
>>
>> Here's the 3rd iteration of the virtio-rng device.  This update just
>> rebases the patch on top of current master.
>>
>> Details on the patch in the commit message.
>>
>
> Hi everyone...
>
> I just stumbled on this patchset after realizing that the virtio-rng 
> support still is not even in the Qemu git tree even though the kernel 
> side has been there for four years(!)

The kernel implementation was done for lguest.  No implementation was
done for QEMU.

A couple have been attempted since then.  This most recent one will go
in.  However...

> It seems this support has been stuck in "overengineering hell" for 
> years.  I have to admit to having a bit of a confusion about what is so 
> hard about reading from a file descriptor, which may return partial 
> reads.  I understand the fairness argument, but I would argue that if it 
> is an actual problem should be solved in the kernel and therefore 
> benefit all types of applications rather than at the application level 
> (which Qemu, effectively, is.)
>
> However, one key error I spotted was using /dev/urandom.  Using 
> /dev/urandom is a very serious cryptographic error, as well as 
> completely pointless.
>
> The whole point of this is to provided distilled entropy to the guest, 
> so that it can seed true entropy into *its* entropy pool.  As such, 
> using /dev/urandom is:
>
> a) needlessly slow.  It will churn the host kernel PRNG needlessly,
>     and not provide any backpressure when the host pool is already
>     drained.  Using /dev/random instead will indicate that the host
>     pool is drained, and not pass a bunch of PRNG noise across an
>     expensive channel when the PRNG in the *guest* could do the PRNG
>     expansion just as well.
>
> b) cryptographically incorrect.  This will tell the guest that it
>     is being provided with entropy that it doesn't actually have, which
>     will mean the guest severely overestimates the entropy that it has
>     available to it.  To put it differently: /dev/random in the guest
>     will behave like (a very expensive version of) /dev/urandom from
>     a cryptographic point of view.
>
> The right interface to the host kernel, therefore, is /dev/random.

This is *exactly* what the problem is.

If using /dev/urandom is pointless--and so far, many people have made
compelling arguments that it is--then using /dev/random is seemingly
impossible to do fairly.

The virtio-rng interface doesn't have any notion of guarantee of entropy
availability.  The guest just keeps requesting entropy with no
indication to the hypervisor of what it should and shouldn't give.

Since /dev/random is a finite pool, it's quite possible that one guest
could quickly exhaust /dev/random for all other guests.  How is this not
a clear denial of service?

This is why supporting EGD is so important IMHO.  Something else needs
to deal with handing out entropy in a responsible way.

Anyway, this is on my list for 1.3.  The series just needs some light
dusting before resubmission.

Regards,

Anthony Liguori

>
> 	-hpa
>
> -- 
> H. Peter Anvin, Intel Open Source Technology Center
> I work for Intel.  I don't speak on their behalf.

Re: [Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[H. Peter Anvin]

On 09/16/2012 04:23 PM, Anthony Liguori wrote:
> This is *exactly* what the problem is.
>
> If using /dev/urandom is pointless--and so far, many people have made
> compelling arguments that it is--then using /dev/random is seemingly
> impossible to do fairly.

It is not merely pointless, it is a security hole.

> The virtio-rng interface doesn't have any notion of guarantee of entropy
> availability.  The guest just keeps requesting entropy with no
> indication to the hypervisor of what it should and shouldn't give.

With the latest fixes to rngd this is no longer true.  This *was* a bug 
in rngd < 4; it would always claim to be entropy-starved (and so a guest 
running rngd < 4 will have this pathology, but no distro is known to run 
rngd < 4 by default due to a number of other problems with these 
versions of rngd).  This has been fixed.  If that backpressure isn't 
propagated through the virtio-rng driver then that does need to be 
fixed, but from at least a cursory look I think it does.

> Since /dev/random is a finite pool, it's quite possible that one guest
> could quickly exhaust /dev/random for all other guests.  How is this not
> a clear denial of service?

Well, by that definition the fact that the service hasn't been provided 
at all until now is a bigger denial of service...

> This is why supporting EGD is so important IMHO.  Something else needs
> to deal with handing out entropy in a responsible way.

No, you're missing the key point.  Fixing this in applications (and from 
the host kernel's perspective, this is an application) is broken, 
because it is not just Qemu that has that property.  If this is an 
actual problem (and it's not clear to me that it is in anything but 
theory, because although the kernel doesn't do round robin it will at 
least provide amount of stochastic fairness) then it is in the kernel 
that the fix belongs.

Throwing an extra daemon -- one which doesn't even claim to be designed 
for this purpose -- into the middle is just silly.  Either which way, it 
can easily be handled via a sane fairness daemon which doesn't need a 
complicated protocol.

> Anyway, this is on my list for 1.3.  The series just needs some light
> dusting before resubmission.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[Amit Shah]

On (Sun) 16 Sep 2012 [13:42:46], H. Peter Anvin wrote:
> On 06/19/2012 11:59 PM, Amit Shah wrote:
> >Hello,
> >
> >Here's the 3rd iteration of the virtio-rng device.  This update just
> >rebases the patch on top of current master.
> >
> >Details on the patch in the commit message.
> >
> 
> Hi everyone...
> 
> I just stumbled on this patchset after realizing that the virtio-rng
> support still is not even in the Qemu git tree even though the
> kernel side has been there for four years(!)
> 
> It seems this support has been stuck in "overengineering hell" for
> years.  I have to admit to having a bit of a confusion about what is
> so hard about reading from a file descriptor, which may return
> partial reads.  I understand the fairness argument, but I would
> argue that if it is an actual problem should be solved in the kernel
> and therefore benefit all types of applications rather than at the
> application level (which Qemu, effectively, is.)
> 
> However, one key error I spotted was using /dev/urandom.  Using
> /dev/urandom is a very serious cryptographic error, as well as
> completely pointless.
> 
> The whole point of this is to provided distilled entropy to the
> guest, so that it can seed true entropy into *its* entropy pool.  As
> such, using /dev/urandom is:
> 
> a) needlessly slow.  It will churn the host kernel PRNG needlessly,
>    and not provide any backpressure when the host pool is already
>    drained.  Using /dev/random instead will indicate that the host
>    pool is drained, and not pass a bunch of PRNG noise across an
>    expensive channel when the PRNG in the *guest* could do the PRNG
>    expansion just as well.
> 
> b) cryptographically incorrect.  This will tell the guest that it
>    is being provided with entropy that it doesn't actually have, which
>    will mean the guest severely overestimates the entropy that it has
>    available to it.  To put it differently: /dev/random in the guest
>    will behave like (a very expensive version of) /dev/urandom from
>    a cryptographic point of view.
> 
> The right interface to the host kernel, therefore, is /dev/random.

Agreed with your points -- /dev/random on the host itself could be fed
in via a real hwrng.  Also, for the fairness arguments, several guests
doing IO also contribute to the random pool.

The ideal interface, though, in qemu should be sourcing entropy from
a file descriptor, and the admin chooses what he wants to source
entropy from - /dev/random, /dev/urandom, or even /dev/hwrng.

		Amit

Re: [Qemu-devel] [PATCH v3 0/1] virtio-rng: hardware random number generator

[H. Peter Anvin]

Right, obviously.  However, under no circumstances should /dev/urandom be used!

Amit Shah <amit.shah@redhat.com> wrote:

>On (Sun) 16 Sep 2012 [13:42:46], H. Peter Anvin wrote:
>> On 06/19/2012 11:59 PM, Amit Shah wrote:
>> >Hello,
>> >
>> >Here's the 3rd iteration of the virtio-rng device.  This update just
>> >rebases the patch on top of current master.
>> >
>> >Details on the patch in the commit message.
>> >
>> 
>> Hi everyone...
>> 
>> I just stumbled on this patchset after realizing that the virtio-rng
>> support still is not even in the Qemu git tree even though the
>> kernel side has been there for four years(!)
>> 
>> It seems this support has been stuck in "overengineering hell" for
>> years.  I have to admit to having a bit of a confusion about what is
>> so hard about reading from a file descriptor, which may return
>> partial reads.  I understand the fairness argument, but I would
>> argue that if it is an actual problem should be solved in the kernel
>> and therefore benefit all types of applications rather than at the
>> application level (which Qemu, effectively, is.)
>> 
>> However, one key error I spotted was using /dev/urandom.  Using
>> /dev/urandom is a very serious cryptographic error, as well as
>> completely pointless.
>> 
>> The whole point of this is to provided distilled entropy to the
>> guest, so that it can seed true entropy into *its* entropy pool.  As
>> such, using /dev/urandom is:
>> 
>> a) needlessly slow.  It will churn the host kernel PRNG needlessly,
>>    and not provide any backpressure when the host pool is already
>>    drained.  Using /dev/random instead will indicate that the host
>>    pool is drained, and not pass a bunch of PRNG noise across an
>>    expensive channel when the PRNG in the *guest* could do the PRNG
>>    expansion just as well.
>> 
>> b) cryptographically incorrect.  This will tell the guest that it
>>    is being provided with entropy that it doesn't actually have,
>which
>>    will mean the guest severely overestimates the entropy that it has
>>    available to it.  To put it differently: /dev/random in the guest
>>    will behave like (a very expensive version of) /dev/urandom from
>>    a cryptographic point of view.
>> 
>> The right interface to the host kernel, therefore, is /dev/random.
>
>Agreed with your points -- /dev/random on the host itself could be fed
>in via a real hwrng.  Also, for the fairness arguments, several guests
>doing IO also contribute to the random pool.
>
>The ideal interface, though, in qemu should be sourcing entropy from
>a file descriptor, and the admin chooses what he wants to source
>entropy from - /dev/random, /dev/urandom, or even /dev/hwrng.
>
>		Amit

-- 
Sent from my mobile phone. Please excuse brevity and lack of formatting.