From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NbyFT-0002JT-1O for qemu-devel@nongnu.org; Mon, 01 Feb 2010 10:31:35 -0500 Received: from [199.232.76.173] (port=34357 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NbyFS-0002Ii-7r for qemu-devel@nongnu.org; Mon, 01 Feb 2010 10:31:34 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NbyFN-0005Q7-De for qemu-devel@nongnu.org; Mon, 01 Feb 2010 10:31:33 -0500 Received: from mail-iw0-f187.google.com ([209.85.223.187]:39470) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NbyFL-0005PW-DM for qemu-devel@nongnu.org; Mon, 01 Feb 2010 10:31:27 -0500 Received: by iwn17 with SMTP id 17so1714070iwn.18 for ; Mon, 01 Feb 2010 07:31:26 -0800 (PST) Message-ID: <4B66F3C9.6090103@codemonkey.ws> Date: Mon, 01 Feb 2010 09:31:21 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 4/5] virtio: Add virtio-rng driver References: <1265031265-14717-1-git-send-email-ian.molton@collabora.co.uk> <1265031265-14717-2-git-send-email-ian.molton@collabora.co.uk> <1265031265-14717-3-git-send-email-ian.molton@collabora.co.uk> <1265031265-14717-4-git-send-email-ian.molton@collabora.co.uk> <1265031265-14717-5-git-send-email-ian.molton@collabora.co.uk> In-Reply-To: <1265031265-14717-5-git-send-email-ian.molton@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: qemu-devel@nongnu.org, Gerd Hoffmann On 02/01/2010 07:34 AM, Ian Molton wrote: > 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 > --- > Makefile.target | 2 +- > hw/pci.h | 1 + > hw/virtio-pci.c | 27 +++++++ > hw/virtio-rng.c | 202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio-rng.h | 19 +++++ > hw/virtio.h | 2 + > rng.h | 18 +++++ > 7 files changed, 270 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 5c0ef1f..21d28f4 100644 > --- a/Makefile.target > +++ b/Makefile.target > @@ -172,7 +172,7 @@ ifdef CONFIG_SOFTMMU > obj-y = vl.o async.o monitor.o pci.o pci_host.o pcie_host.o machine.o gdbstub.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-pci.o virtio-serial-bus.o > +obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-pci.o virtio-serial-bus.o virtio-rng.o > obj-$(CONFIG_KVM) += kvm.o kvm-all.o > obj-$(CONFIG_ISA_MMIO) += isa_mmio.o > LIBS+=-lz > diff --git a/hw/pci.h b/hw/pci.h > index 8b511d2..77cb543 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 > > typedef uint64_t pcibus_t; > #define FMT_PCIBUS PRIx64 > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c > index 709d13e..f057388 100644 > --- a/hw/virtio-pci.c > +++ b/hw/virtio-pci.c > @@ -22,6 +22,7 @@ > #include "sysemu.h" > #include "msix.h" > #include "net.h" > +#include "rng.h" > #include "loader.h" > > /* from Linux's linux/virtio_pci.h */ > @@ -94,6 +95,7 @@ typedef struct { > uint32_t nvectors; > DriveInfo *dinfo; > 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; > @@ -550,6 +552,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, > Gerd (or the right person at Red Hat) needs to Ack the assignment of this PCI device id. > + PCI_CLASS_OTHERS, > + 0x00); > + > + return 0; > +} > + > static PCIDeviceInfo virtio_info[] = { > { > .qdev.name = "virtio-blk-pci", > @@ -603,6 +620,16 @@ 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_RNG_PROPERTIES(VirtIOPCIProxy, rng), > I don't see any reason to use a define here. > + 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..d8cbb74 > --- /dev/null > +++ b/hw/virtio-rng.c > @@ -0,0 +1,202 @@ > +/* > + * 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) { > Coding style is off here (newline between ) and { ). > + 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 */ > + 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; > + > + req_entropy(s); > + > + if (s->rate) { > + gettimeofday(&now, NULL); > + timersub(&now,&s->last,&d); > Can't call gettimeofday directly. You have to use qemu_gettimeofday(). But it would be better to not rely on gettimeofday and instead make use of the rt_clock. > +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; > +} > > This doesn't look correct to me. There is absolutely no state maintained by the virtio-rng backend? I find that hard to believe. Regards, Anthony Liguori