qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 4/4] virtio-rng: hardware random number generato


From: Anthony Liguori
Subject: Re: [Qemu-devel] [PATCH 4/4] virtio-rng: hardware random number generator device
Date: Mon, 25 Jun 2012 17:59:28 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1

On 06/25/2012 05:46 PM, Anthony Liguori wrote:
From: Amit Shah<address@hidden>

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.

Signed-off-by: Amit Shah<address@hidden>
Signed-off-by: Anthony Liguori<address@hidden>
---
aliguori: converted to new RngBackend interface
---
  hw/Makefile.objs     |    1 +
  hw/pci.h             |    1 +
  hw/s390-virtio-bus.c |   37 ++++++++++
  hw/s390-virtio-bus.h |    2 +
  hw/virtio-pci.c      |   60 ++++++++++++++++
  hw/virtio-pci.h      |    2 +
  hw/virtio-rng.c      |  186 ++++++++++++++++++++++++++++++++++++++++++++++++++
  hw/virtio-rng.h      |   24 +++++++
  hw/virtio.h          |    3 +
  monitor.c            |    4 +-
  monitor.h            |    1 +
  11 files changed, 320 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..af7be91 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,29 @@ static TypeInfo s390_virtio_serial = {
      .class_init    = s390_virtio_serial_class_init,
  };

+static void s390_virtio_rng_initfn(Object *obj)
+{
+    VirtIOS390Device *dev = VIRTIO_S390_DEVICE(obj);
+
+    object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
+                             (Object **)&dev->rng.rng, NULL);
+}
+
+static void s390_virtio_rng_class_init(ObjectClass *klass, void *data)
+{
+    VirtIOS390DeviceClass *k = VIRTIO_S390_DEVICE_CLASS(klass);
+
+    k->init = s390_virtio_rng_init;
+}
+
+static TypeInfo s390_virtio_rng = {
+    .name          = "virtio-rng-s390",
+    .parent        = TYPE_VIRTIO_S390_DEVICE,
+    .instance_size = sizeof(VirtIOS390Device),
+    .instance_init = s390_virtio_rng_initfn,
+    .class_init    = s390_virtio_rng_class_init,
+};
+
  static int s390_virtio_busdev_init(DeviceState *dev)
  {
      VirtIOS390Device *_dev = (VirtIOS390Device *)dev;
@@ -527,6 +563,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..fe19830 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,43 @@ static TypeInfo virtio_balloon_info = {
      .class_init    = virtio_balloon_class_init,
  };

+static void virtio_rng_initfn(Object *obj)
+{
+    PCIDevice *pci_dev = PCI_DEVICE(obj);
+    VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev);
+
+    object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
+                             (Object **)&proxy->rng.rng, NULL);
+}
+
+static Property virtio_rng_properties[] = {
+    DEFINE_VIRTIO_COMMON_FEATURES(VirtIOPCIProxy, host_features),
+    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;

WHQL tends to get very particular about PCI classes. Do we understand the implications of making this CLASS_OTHERS and WHQL?

+    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),
+    .instance_init = virtio_rng_initfn,
+    .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 +1181,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..a4c2ac1
--- /dev/null
+++ b/hw/virtio-rng.c
@@ -0,0 +1,186 @@
+/*
+ * A virtio device implementing a hardware random number generator.
+ *
+ * Copyright 2012 Red Hat, Inc.
+ * Copyright 2012 Amit Shah<address@hidden>
+ *
+ * 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 "qdev.h"
+#include "virtio.h"
+#include "virtio-rng.h"
+#include "qemu/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;
+
+    RngBackend *rng;
+} 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 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;
+}
+
+/* Send data from a char device over to the guest */
+static void chr_read(void *opaque, const void *buf, size_t 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.
+     */
+    len = pop_an_elem(vrng);
+    if (len) {
+        rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
+    }

Because of this above while() loop, you won't see entropy requests for every request that comes from the guest depending on how data gets buffered in the socket.

Things are simplified now because boundaries will be respected (they always would be with qemu_chr_can_read too FWIW). We should have chr_read() process exactly one entropy request. We should only have one pending entropy request at a time too.

That eliminates the need for looping here and should simplify the whole code.

+}
+
+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) {
+        rng_backend_request_entropy(vrng->rng, size, chr_read, vrng);
+    }
+}
+
+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));
+    }

Okay, new rule: if you copy a struct verbatim to savevm, your next 5 patches will be automatically nacked.

Seriously, this is an awful thing to do. I don't care if we do it in other places in the code. It's never the right thing to do.

This is an unpacked unaligned structure with non-fixed sized types in it. that are greater than a byte. This breaks across endianness, compiler versions, etc.

Regards,

Anthony Liguori

+}
+
+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;
+    Error *local_err = NULL;
+
+    vdev = virtio_common_init("virtio-rng", VIRTIO_ID_RNG, 0,
+                              sizeof(VirtIORNG));
+
+    vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
+
+    vrng->rng = conf->rng;
+    if (vrng->rng == NULL) {
+        qerror_report(QERR_INVALID_PARAMETER_VALUE, "rng", "a valid object");
+        return NULL;
+    }
+
+    rng_backend_open(vrng->rng,&local_err);
+    if (local_err) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+        return NULL;
+    }
+
+    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);
+
+    return vdev;
+}
+
+void virtio_rng_exit(VirtIODevice *vdev)
+{
+    VirtIORNG *vrng = DO_UPCAST(VirtIORNG, vdev, vdev);
+
+    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..fbb0104
--- /dev/null
+++ b/hw/virtio-rng.h
@@ -0,0 +1,24 @@
+/*
+ * Virtio RNG Support
+ *
+ * Copyright Red Hat, Inc. 2012
+ * Copyright Amit Shah<address@hidden>
+ *
+ * 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/rng.h"
+
+/* The Virtio ID for the virtio rng device */
+#define VIRTIO_ID_RNG    4
+
+struct VirtIORNGConf {
+    RngBackend *rng;
+};
+
+#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 */




reply via email to

[Prev in Thread] Current Thread [Next in Thread]