From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1O90KE-0004ld-HQ for qemu-devel@nongnu.org; Mon, 03 May 2010 14:25:02 -0400 Received: from [140.186.70.92] (port=46885 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1O90K6-00042H-Cp for qemu-devel@nongnu.org; Mon, 03 May 2010 14:25:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1O8zu0-0007oE-86 for qemu-devel@nongnu.org; Mon, 03 May 2010 13:58:00 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:54672) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1O8ztz-0007cI-VJ for qemu-devel@nongnu.org; Mon, 03 May 2010 13:57:56 -0400 Received: by vws3 with SMTP id 3so1849934vws.4 for ; Mon, 03 May 2010 10:56:43 -0700 (PDT) Message-ID: <4BDF0E56.60705@codemonkey.ws> Date: Mon, 03 May 2010 12:56:38 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/2] VirtIO RNG References: <4BB2053C.6000701@collabora.co.uk> <4BB206EF.3030100@collabora.co.uk> In-Reply-To: <4BB206EF.3030100@collabora.co.uk> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Ian Molton Cc: Paul Brook , qemu-devel@nongnu.org, Gerd Hoffmann On 03/30/2010 09:13 AM, Ian Molton wrote: > > > From 5f484301d73fa53009bbcd430f8ae85868b67772 Mon Sep 17 00:00:00 2001 > From: Ian Molton > Date: Tue, 17 Nov 2009 14:34:12 +0000 > Subject: [PATCH 2/4] virtio: Add virtio-rng driver > > This patch adds support for virtio-rng. Data is read from a chardev and > can be either raw entropy or received via the EGD protocol. > > Signed-off-by: Ian Molton > Hi Ian, A couple review comments below. I've read the thread. virtio-rng should stand on it's own as it's a spec'd out virtio device so I'm with you there. The SIZE qdev patch can't be applied until there's a consumer of it. I've already expressed my opinion about the socket reconnect patch. I don't think it's the right functionality for qemu to implement because the semantics concern me. Really, a more thorough refactoring of CharDrivers is needed such that it supported connected and disconnected states along with QMP events for the transitions. I agree with Paul that the EGD logic ought to be separated. It probably makes sense to modify this a bit to have a split front-end/back-end. For instance: -device virtio-rng-pci,rngdev=foo -rngdev egd,addr=unix:/path/to/rng,id=foo You could also have a syntax helper like: -rng egd,addr:/path/to/rng See the recent virtfs patch series for an example of this. With those changes, I'd happily apply this series. > --- > Makefile.target | 2 +- > hw/pci.h | 1 + > hw/virtio-pci.c | 29 ++++++++ > hw/virtio-rng.c | 195 > +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio-rng.h | 19 ++++++ > hw/virtio.h | 2 + > rng.h | 14 ++++ > 7 files changed, 261 insertions(+), 1 deletions(-) > create mode 100644 hw/virtio-rng.c > create mode 100644 hw/virtio-rng.h > create mode 100644 rng.h > > diff --git a/Makefile.target b/Makefile.target > index 16ea443..2f9c929 100644 > --- a/Makefile.target > +++ b/Makefile.target > @@ -166,7 +166,7 @@ obj-y += qemu-timer.o > # virtio has to be here due to weird dependency between PCI and virtio-net. > # need to fix this properly > obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o > -obj-y += rwhandler.o > +obj-y += virtio-rng.o rwhandler.o > obj-$(CONFIG_KVM) += kvm.o kvm-all.o > LIBS+=-lz > > diff --git a/hw/pci.h b/hw/pci.h > index 20c670e..dc61cb5 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -69,6 +69,7 @@ > #define PCI_DEVICE_ID_VIRTIO_BLOCK 0x1001 > #define PCI_DEVICE_ID_VIRTIO_BALLOON 0x1002 > #define PCI_DEVICE_ID_VIRTIO_CONSOLE 0x1003 > +#define PCI_DEVICE_ID_VIRTIO_RNG 0x1004 > > #define FMT_PCIBUS PRIx64 > > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 6eb19cd..bee3105 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -23,6 +23,7 @@ > #include "msix.h" > #include "net.h" > #include "block_int.h" > +#include "rng.h" > #include "loader.h" > > /* from Linux's linux/virtio_pci.h */ > @@ -95,6 +96,7 @@ typedef struct { > uint32_t nvectors; > BlockConf block; > NICConf nic; > + RNGConf rng; > uint32_t host_features; > /* Max. number of ports we can have for a the virtio-serial device */ > uint32_t max_virtserial_ports; > @@ -552,6 +554,21 @@ static int virtio_balloon_init_pci(PCIDevice *pci_dev) > return 0; > } > > +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); > + virtio_init_pci(proxy, vdev, > + PCI_VENDOR_ID_REDHAT_QUMRANET, > + PCI_DEVICE_ID_VIRTIO_RNG, > + PCI_CLASS_OTHERS, > + 0x00); > + > + return 0; > +} > + > static PCIDeviceInfo virtio_info[] = { > { > .qdev.name = "virtio-blk-pci", > @@ -606,6 +623,18 @@ static PCIDeviceInfo virtio_info[] = { > }, > .qdev.reset = virtio_pci_reset, > },{ > + .qdev.name = "virtio-rng-pci", > + .qdev.size = sizeof(VirtIOPCIProxy), > + .init = virtio_rng_init_pci, > + .exit = virtio_exit_pci, > + .qdev.props = (Property[]) { > + DEFINE_PROP_CHR("chardev", VirtIOPCIProxy, rng.chrdev), > + DEFINE_PROP_SIZE("rate", VirtIOPCIProxy, rng.rate, 0), > + DEFINE_PROP_STRING("protocol", VirtIOPCIProxy, rng.proto), > + DEFINE_PROP_END_OF_LIST(), > + }, > + .qdev.reset = virtio_pci_reset, > + },{ > /* end of list */ > } > }; > diff --git a/hw/virtio-rng.c b/hw/virtio-rng.c > new file mode 100644 > index 0000000..154ea76 > --- /dev/null > +++ b/hw/virtio-rng.c > @@ -0,0 +1,195 @@ > +/* > + * Virtio RNG Device > + * > + * Copyright Collabora 2009 > + * > + * Authors: > + * Ian Molton > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + * > + */ > + > +#include "hw.h" > +#include "qemu-char.h" > +#include "virtio.h" > +#include "virtio-rng.h" > +#include "rng.h" > +#include > + > +typedef struct VirtIORng > +{ > + VirtIODevice vdev; > + VirtQueue *vq; > + CharDriverState *chr; > + struct timeval last; > + int rate; > + int egd; > + int entropy_remaining; > + int pool; > +} VirtIORng; > + > +/* Maximum size of the buffer the guest expects */ > +#define BUFF_MAX 64 > + > +/* EGD protocol - we only use this command */ > +#define EGD_READ_BLOCK 0x2 > + > +#define EGD_MAX_BLOCK_SIZE 255 > +#define EGD_MAX_REQUESTS 3 > +#define EGD_MAX_POOL_SIZE (EGD_MAX_BLOCK_SIZE * (EGD_MAX_REQUESTS-1)) > + > +static inline void req_entropy(VirtIORng *s) > +{ > + static const unsigned char entropy_rq[2] = { EGD_READ_BLOCK, > + EGD_MAX_BLOCK_SIZE }; > + if (s->egd) { > + /* Let the socket buffer up the incoming data for us. Max block > size > + for EGD protocol is (stupidly) 255, so make sure we always > have a > + block pending for performance. We can have 3 outstanding > buffers */ > Whitespace damaged (broken mailer?). > + if (s->pool<= EGD_MAX_POOL_SIZE) { > + s->chr->chr_write(s->chr, entropy_rq, sizeof(entropy_rq)); > + s->pool += EGD_MAX_BLOCK_SIZE; > + } > + } else { > + s->pool = BUFF_MAX; > + } > +} > + > +static int vrng_can_read(void *opaque) > +{ > + VirtIORng *s = (VirtIORng *) opaque; > + struct timeval now, d; > + int max_entropy; > + > + if (!virtio_queue_ready(s->vq) || > + !(s->vdev.status& VIRTIO_CONFIG_S_DRIVER_OK) || > + virtio_queue_empty(s->vq)) > + return 0; > Missing braces (CODING_STYLE). > + req_entropy(s); > + > + if (s->rate) { > + qemu_gettimeofday(&now); > + timersub(&now,&s->last,&d); > + if (d.tv_sec * 1000000 + d.tv_usec> 1000000) { > + s->entropy_remaining = s->rate; > + s->last = now; > + } > Should be based on rt_clock > + max_entropy = MIN(s->pool, s->entropy_remaining); > + } else { > + max_entropy = s->pool; > + } > + > + /* current implementations have a 64 byte buffer. > + * We fall back to a one byte per read if there is not enough room. > + */ > + max_entropy = MIN(max_entropy, BUFF_MAX); > + if (max_entropy) { > + if (virtqueue_avail_bytes(s->vq, max_entropy, 0)) > + return max_entropy; > + if (virtqueue_avail_bytes(s->vq, 1, 0)) > + return 1; > Missing braces (CODING_STYLE). > + } > + return 0; > +} > + > +static void vrng_read(void *opaque, const uint8_t *buf, int size) > +{ > + VirtIORng *s = (VirtIORng *) opaque; > + VirtQueueElement elem; > + int offset = 0; > + > + /* The current kernel implementation has only one outstanding input > + * buffer of 64 bytes. > + */ > + while (offset< size) { > + int i = 0; > + if (!virtqueue_pop(s->vq,&elem)) > + break; > Braces. > + while (offset< size&& i< elem.in_num) { > + int len = MIN(elem.in_sg[i].iov_len, size - offset); > + memcpy(elem.in_sg[i].iov_base, buf + offset, len); > + offset += len; > + i++; > + } > + virtqueue_push(s->vq,&elem, size); > + } > + > + if (s->rate) > + s->entropy_remaining -= size; > Braces (lots more too). > + s->pool -= size; > + > + virtio_notify(&s->vdev, s->vq); > +} > + > +static void vrng_event(void *opaque, int event) > +{ > + > +} > + > + > + > +static void virtio_rng_handle(VirtIODevice *vdev, VirtQueue *vq) > +{ > + /* Nothing to do - we push data when its available */ > +} > + > +static uint32_t virtio_rng_get_features(VirtIODevice *vdev, uint32_t > features) > +{ > + return 0; > +} > + > +static void virtio_rng_save(QEMUFile *f, void *opaque) > +{ > + VirtIORng *s = opaque; > + > + virtio_save(&s->vdev, f); > +} > + > +static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) > +{ > + VirtIORng *s = opaque; > + > + if (version_id != 1) > + return -EINVAL; > + > + virtio_load(&s->vdev, f); > + return 0; > +} > + > +VirtIODevice *virtio_rng_init(DeviceState *dev, RNGConf *rngdev) > +{ > + VirtIORng *s; > + s = (VirtIORng *)virtio_common_init("virtio-rng", > + VIRTIO_ID_RNG, > + 0, sizeof(VirtIORng)); > + > + if (!s) > + return NULL; > + > + s->vdev.get_features = virtio_rng_get_features; > + > + s->vq = virtio_add_queue(&s->vdev, 128, virtio_rng_handle); > + s->chr = rngdev->chrdev; > + s->rate = rngdev->rate; > + gettimeofday(&s->last, NULL); > + > + if(rngdev->proto&& !strncmp(rngdev->proto, "egd", 3)) > + s->egd = 1; > + > +#ifdef DEBUG > It's more typical to introduce a debugging macro (like dprintf()) and then have a single define at the top of the file to enable the macro. This will make conversion to a more unified logging mechanism a lot easier down the road. Regards, Anthony Liguori > + printf("entropy being read from %s", rngdev->chrdev->label); > + if(s->rate) > + printf(" at %d bytes/sec max.", s->rate); > + printf(" protocol: %s\n", s->egd?"egd":"raw"); > +#endif > + > + qemu_chr_add_handlers(s->chr, vrng_can_read, vrng_read, vrng_event, s); > + > + register_savevm("virtio-rng", -1, 1, virtio_rng_save, > virtio_rng_load, s); > + > + return&s->vdev; > +} > + > diff --git a/hw/virtio-rng.h b/hw/virtio-rng.h > new file mode 100644 > index 0000000..bc4d2d8 > --- /dev/null > +++ b/hw/virtio-rng.h > @@ -0,0 +1,19 @@ > +/* > + * Virtio RNG Support > + * > + * Copyright Collabora 2009 > + * > + * Authors: > + * Ian Molton > + * > + * This work is licensed under the terms of the GNU GPL, version 2. See > + * the COPYING file in the top-level directory. > + * > + */ > +#ifndef _QEMU_VIRTIO_RNG_H > +#define _QEMU_VIRTIO_RNG_H > + > +/* The ID for virtio console */ > +#define VIRTIO_ID_RNG 4 > + > +#endif > diff --git a/hw/virtio.h b/hw/virtio.h > index 3baa2a3..749b2a6 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -16,6 +16,7 @@ > > #include "hw.h" > #include "net.h" > +#include "rng.h" > #include "qdev.h" > #include "sysemu.h" > #include "block_int.h" > @@ -174,6 +175,7 @@ VirtIODevice *virtio_blk_init(DeviceState *dev, > BlockConf *conf); > VirtIODevice *virtio_net_init(DeviceState *dev, NICConf *conf); > VirtIODevice *virtio_serial_init(DeviceState *dev, uint32_t max_nr_ports); > VirtIODevice *virtio_balloon_init(DeviceState *dev); > +VirtIODevice *virtio_rng_init(DeviceState *dev, RNGConf *rngdev); > > void virtio_net_exit(VirtIODevice *vdev); > > diff --git a/rng.h b/rng.h > new file mode 100644 > index 0000000..14ae7e0 > --- /dev/null > +++ b/rng.h > @@ -0,0 +1,14 @@ > +#ifndef QEMU_RNG_H > +#define QEMU_RNG_H > + > +#include "qemu-option.h" > + > +/* qdev rng properties */ > + > +typedef struct RNGConf { > + CharDriverState *chrdev; > + uint64_t rate; > + char *proto; > +} RNGConf; > + > +#endif >