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

[Amit Shah]

On (Mon) 25 Jun 2012 [17:59:28], Anthony Liguori wrote:
> On 06/25/2012 05:46 PM, Anthony Liguori wrote:
> >From: Amit Shah<address@hidden>

> >diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c

> >+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?

I've not asked around; will update with info when I get it.

> >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.

So the issue is we currently can't get the iov_size without popping
the elem from the vq.  If we had that, we would just send out the
request from handle_input(), and the save/load functions wouldn't have
to bother with the elem, too.

Given that we can't do that, restricting chr_read() to one iov seems
fine.

Note that there's a bug introduced in the
rng_backend_request_entropy() call above - it should advertise 'len'
instead of 'size' here.

> 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.

Any ideas how to get it done?  (CC'ed Juan).

                Amit