On (Mon) 25 Jun 2012 [17:59:28], Anthony Liguori wrote: > On 06/25/2012 05:46 PM, Anthony Liguori wrote: > >From: Amit Shah<amit.s...@redhat.com>
> >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<amit.s...@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 "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