diff mbox

[2/3] kvm tools: Introduce virtio-rng

Message ID 1304170225-4859-2-git-send-email-levinsasha928@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Sasha Levin April 30, 2011, 1:30 p.m. UTC
Enable virtio-rng, a virtio random number generator.
Guest kernel should be compiled with CONFIG_HW_RANDOM_VIRTIO.
Once enabled, A RNG device will be located at /dev/hwrng.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/Makefile                 |    1 +
 tools/kvm/include/kvm/ioport.h     |    2 +
 tools/kvm/include/kvm/virtio-rng.h |    8 ++
 tools/kvm/virtio-rng.c             |  184 ++++++++++++++++++++++++++++++++++++
 4 files changed, 195 insertions(+), 0 deletions(-)
 create mode 100644 tools/kvm/include/kvm/virtio-rng.h
 create mode 100644 tools/kvm/virtio-rng.c

Comments

Ingo Molnar May 5, 2011, 6:54 a.m. UTC | #1
* Sasha Levin <levinsasha928@gmail.com> wrote:

> +#define PCI_VENDOR_ID_REDHAT_QUMRANET			0x1af4
> +#define PCI_DEVICE_ID_VIRTIO_RNG				0x1004
> +#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET	0x1af4
> +#define PCI_SUBSYSTEM_ID_VIRTIO_RNG				0x0004
> +#define PCI_VIRTIO_RNG_DEVNUM 4
> +
> +#define VIRTIO_RNG_IRQ		11
> +#define VIRTIO_RNG_PIN		1
> +
> +#define NUM_VIRT_QUEUES		1
> +
> +#define VIRTIO_RNG_QUEUE_SIZE	128
> +
> +struct rng_device {
> +	uint8_t				status;
> +	uint16_t			config_vector;
> +	int					fd_rng;
> +
> +	/* virtio queue */
> +	uint16_t			queue_selector;
> +	struct virt_queue	vqs[NUM_VIRT_QUEUES];
> +	void				*jobs[NUM_VIRT_QUEUES];
> +};


Really, have you *looked* at this source code from a distance? Does it look 
neat and orderly to you??

It does not look readable to me at all: it's full of vertical spacing 
misalignments even within the *same* syntactic unit. (Not to mention file-scope 
alignment convention which is all over the place.)

> +static struct ioport_operations virtio_rng_io_ops = {
> +	.io_in		= virtio_rng_pci_io_in,
> +	.io_out		= virtio_rng_pci_io_out,
> +};
> +
> +static struct pci_device_header virtio_rng_pci_device = {
> +	.vendor_id			= PCI_VENDOR_ID_REDHAT_QUMRANET,
> +	.device_id			= PCI_DEVICE_ID_VIRTIO_RNG,
> +	.header_type		= PCI_HEADER_TYPE_NORMAL,
> +	.revision_id		= 0,
> +	.class				= 0x010000,
> +	.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
> +	.subsys_id			= PCI_SUBSYSTEM_ID_VIRTIO_RNG,
> +	.bar[0]				= IOPORT_VIRTIO_RNG | PCI_BASE_ADDRESS_SPACE_IO,
> +	.irq_pin			= VIRTIO_RNG_PIN,
> +	.irq_line			= VIRTIO_RNG_IRQ,
> +};

Same here! It looks like as if random new lines were jumbled within something 
copy & pasted from elsewhere, with no care taken that they look good together 
...

There's also other details like:

> +	int					fd_rng;

The _rng postfix is superfluous, we already know that this is a rng thing, it's 
within struct rng_device! The result is suboptimal usage like this:

  rng_device.fd_rng

which says 'rng' twice and clutters the code needlessly.

Plus remember the blk_device argument i made yesterday? The *same* issue gets 
reintroduced here:

   static struct rng_device rng_device;

We generally try to use different names for the structure and the local (or as 
here, global) variables - and we try to use *short* (while still expressive) 
names for variables.

So the proper and canonical naming, in line with blk_dev and net_dev would be 
rng_dev, not rng_device.

The code looks correct but we really need to try harder to keep the tools/kvm/ 
code maintainable!

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg May 5, 2011, 7:09 a.m. UTC | #2
On Thu, May 5, 2011 at 9:54 AM, Ingo Molnar <mingo@elte.hu> wrote:
> The code looks correct but we really need to try harder to keep the tools/kvm/
> code maintainable!

Aww, I must have been on crack when I applied that. Sasha, please send
a follow up patch to address Ingo's comments.

                        Pekka
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar May 5, 2011, 7:12 a.m. UTC | #3
* Pekka Enberg <penberg@kernel.org> wrote:

> On Thu, May 5, 2011 at 9:54 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > The code looks correct but we really need to try harder to keep the tools/kvm/
> > code maintainable!
> 
> Aww, I must have been on crack when I applied that. Sasha, please send
> a follow up patch to address Ingo's comments.

another thing i noticed: the patch introduces some new uint_* usages.

Once things are fixed can we somehow kill that datatype intelligently, so that 
if new code uses it the build breaks or so?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pekka Enberg May 5, 2011, 7:14 a.m. UTC | #4
On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
> another thing i noticed: the patch introduces some new uint_* usages.
>
> Once things are fixed can we somehow kill that datatype intelligently, so that
> if new code uses it the build breaks or so?

I guess we could do

  include/stdint.h

that does

  #error You lose.

or something.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ingo Molnar May 5, 2011, 7:14 a.m. UTC | #5
btw., another, wider code structure thought: with the recent (very nice!) 
amassing of new virtio drivers, isnt it time to put them into their own 
directory in tools/kvm/virtio/?

That way there would be:

  tools/kvm/virtio/core.c
  tools/kvm/virtio/net.c
  tools/kvm/virtio/blk.c
  tools/kvm/virtio/rng.c

etc.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sasha Levin May 5, 2011, 7:20 a.m. UTC | #6
On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote:
> On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > another thing i noticed: the patch introduces some new uint_* usages.
> >
> > Once things are fixed can we somehow kill that datatype intelligently, so that
> > if new code uses it the build breaks or so?
> 
> I guess we could do
> 
>   include/stdint.h
> 
> that does
> 
>   #error You lose.
> 
> or something.

I'll make a pass to clean up formatting and datatypes throughout the
virtio drivers once I finish with the virtio-blk patch.
Sasha Levin May 5, 2011, 4:04 p.m. UTC | #7
On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote:
> On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > another thing i noticed: the patch introduces some new uint_* usages.
> >
> > Once things are fixed can we somehow kill that datatype intelligently, so that
> > if new code uses it the build breaks or so?
> 
> I guess we could do
> 
>   include/stdint.h
> 
> that does
> 
>   #error You lose.
> 
> or something.

How would we handle <linux/inet.h> which uses uint*_t types?
Ingo Molnar May 5, 2011, 10 p.m. UTC | #8
* Sasha Levin <levinsasha928@gmail.com> wrote:

> On Thu, 2011-05-05 at 10:14 +0300, Pekka Enberg wrote:
> > On Thu, May 5, 2011 at 10:12 AM, Ingo Molnar <mingo@elte.hu> wrote:
> > > another thing i noticed: the patch introduces some new uint_* usages.
> > >
> > > Once things are fixed can we somehow kill that datatype intelligently, so that
> > > if new code uses it the build breaks or so?
> > 
> > I guess we could do
> > 
> >   include/stdint.h
> > 
> > that does
> > 
> >   #error You lose.
> > 
> > or something.
> 
> How would we handle <linux/inet.h> which uses uint*_t types?

Yeah, it's not realistic to get rid of them like that - also, system include 
files are using it as well.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index 659bc35..19e7b9e 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -38,6 +38,7 @@  OBJS    += kvm-run.o
 OBJS    += qcow.o
 OBJS    += mptable.o
 OBJS    += threadpool.o
+OBJS    += virtio-rng.o
 
 DEPS	:= $(patsubst %.o,%.d,$(OBJS))
 
diff --git a/tools/kvm/include/kvm/ioport.h b/tools/kvm/include/kvm/ioport.h
index 2fdcca4..6020124 100644
--- a/tools/kvm/include/kvm/ioport.h
+++ b/tools/kvm/include/kvm/ioport.h
@@ -12,6 +12,8 @@ 
 #define IOPORT_VIRTIO_CONSOLE_SIZE	256
 #define IOPORT_VIRTIO_NET		0xe200	/* Virtio network device */
 #define IOPORT_VIRTIO_NET_SIZE		256
+#define IOPORT_VIRTIO_RNG		0xf200	/* Virtio network device */
+#define IOPORT_VIRTIO_RNG_SIZE		256
 
 struct kvm;
 
diff --git a/tools/kvm/include/kvm/virtio-rng.h b/tools/kvm/include/kvm/virtio-rng.h
new file mode 100644
index 0000000..7015c1f
--- /dev/null
+++ b/tools/kvm/include/kvm/virtio-rng.h
@@ -0,0 +1,8 @@ 
+#ifndef KVM__RNG_VIRTIO_H
+#define KVM__RNG_VIRTIO_H
+
+struct kvm;
+
+void virtio_rng__init(struct kvm *kvm);
+
+#endif /* KVM__RNG_VIRTIO_H */
diff --git a/tools/kvm/virtio-rng.c b/tools/kvm/virtio-rng.c
new file mode 100644
index 0000000..d37fb5e
--- /dev/null
+++ b/tools/kvm/virtio-rng.c
@@ -0,0 +1,184 @@ 
+#include "kvm/virtio-rng.h"
+
+#include "kvm/virtio-pci.h"
+
+#include "kvm/disk-image.h"
+#include "kvm/virtio.h"
+#include "kvm/ioport.h"
+#include "kvm/mutex.h"
+#include "kvm/util.h"
+#include "kvm/kvm.h"
+#include "kvm/pci.h"
+#include "kvm/threadpool.h"
+
+#include <linux/virtio_ring.h>
+#include <linux/virtio_rng.h>
+
+#include <fcntl.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <inttypes.h>
+#include <pthread.h>
+
+#define PCI_VENDOR_ID_REDHAT_QUMRANET			0x1af4
+#define PCI_DEVICE_ID_VIRTIO_RNG				0x1004
+#define PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET	0x1af4
+#define PCI_SUBSYSTEM_ID_VIRTIO_RNG				0x0004
+#define PCI_VIRTIO_RNG_DEVNUM 4
+
+#define VIRTIO_RNG_IRQ		11
+#define VIRTIO_RNG_PIN		1
+
+#define NUM_VIRT_QUEUES		1
+
+#define VIRTIO_RNG_QUEUE_SIZE	128
+
+struct rng_device {
+	uint8_t				status;
+	uint16_t			config_vector;
+	int					fd_rng;
+
+	/* virtio queue */
+	uint16_t			queue_selector;
+	struct virt_queue	vqs[NUM_VIRT_QUEUES];
+	void				*jobs[NUM_VIRT_QUEUES];
+};
+
+static struct rng_device rng_device;
+
+static bool virtio_rng_pci_io_in(struct kvm *kvm, uint16_t port, void *data, int size, uint32_t count)
+{
+	unsigned long offset;
+	bool ret = true;
+
+	offset = port - IOPORT_VIRTIO_RNG;
+
+	switch (offset) {
+	case VIRTIO_PCI_HOST_FEATURES:
+	case VIRTIO_PCI_GUEST_FEATURES:
+	case VIRTIO_PCI_QUEUE_SEL:
+	case VIRTIO_PCI_QUEUE_NOTIFY:
+		ret		= false;
+		break;
+	case VIRTIO_PCI_QUEUE_PFN:
+		ioport__write32(data, rng_device.vqs[rng_device.queue_selector].pfn);
+		break;
+	case VIRTIO_PCI_QUEUE_NUM:
+		ioport__write16(data, VIRTIO_RNG_QUEUE_SIZE);
+		break;
+	case VIRTIO_PCI_STATUS:
+		ioport__write8(data, rng_device.status);
+		break;
+	case VIRTIO_PCI_ISR:
+		ioport__write8(data, 0x1);
+		kvm__irq_line(kvm, VIRTIO_RNG_IRQ, 0);
+		break;
+	case VIRTIO_MSI_CONFIG_VECTOR:
+		ioport__write16(data, rng_device.config_vector);
+		break;
+	default:
+		ret		= false;
+	};
+
+	return ret;
+}
+
+static bool virtio_rng_do_io_request(struct kvm *self, struct virt_queue *queue)
+{
+	struct iovec iov[VIRTIO_RNG_QUEUE_SIZE];
+	uint16_t out, in, head;
+	unsigned int len = 0;
+
+	head = virt_queue__get_iov(queue, iov, &out, &in, self);
+	len = readv(rng_device.fd_rng, iov, in);
+	virt_queue__set_used_elem(queue, head, len);
+
+	return true;
+}
+
+static void virtio_rng_do_io(struct kvm *kvm, void *param)
+{
+	struct virt_queue *vq = param;
+
+	while (virt_queue__available(vq)) {
+		virtio_rng_do_io_request(kvm, vq);
+		kvm__irq_line(kvm, VIRTIO_RNG_IRQ, 1);
+	}
+}
+
+static bool virtio_rng_pci_io_out(struct kvm *kvm, uint16_t port, void *data, int size, uint32_t count)
+{
+	unsigned long offset;
+	bool ret = true;
+
+	offset = port - IOPORT_VIRTIO_RNG;
+
+	switch (offset) {
+	case VIRTIO_MSI_QUEUE_VECTOR:
+	case VIRTIO_PCI_GUEST_FEATURES:
+		break;
+	case VIRTIO_PCI_QUEUE_PFN: {
+		struct virt_queue *queue;
+		void *p;
+
+		queue			= &rng_device.vqs[rng_device.queue_selector];
+		queue->pfn		= ioport__read32(data);
+		p				= guest_flat_to_host(kvm, queue->pfn << 12);
+
+		vring_init(&queue->vring, VIRTIO_RNG_QUEUE_SIZE, p, 4096);
+
+		rng_device.jobs[rng_device.queue_selector] =
+			thread_pool__add_jobtype(kvm, virtio_rng_do_io, queue);
+
+		break;
+	}
+	case VIRTIO_PCI_QUEUE_SEL:
+		rng_device.queue_selector	= ioport__read16(data);
+		break;
+	case VIRTIO_PCI_QUEUE_NOTIFY: {
+		uint16_t queue_index;
+		queue_index		= ioport__read16(data);
+		thread_pool__signal_work(rng_device.jobs[queue_index]);
+		break;
+	}
+	case VIRTIO_PCI_STATUS:
+		rng_device.status		= ioport__read8(data);
+		break;
+	case VIRTIO_MSI_CONFIG_VECTOR:
+		rng_device.config_vector	= VIRTIO_MSI_NO_VECTOR;
+		break;
+	default:
+		ret		= false;
+	};
+
+	return ret;
+}
+
+static struct ioport_operations virtio_rng_io_ops = {
+	.io_in		= virtio_rng_pci_io_in,
+	.io_out		= virtio_rng_pci_io_out,
+};
+
+static struct pci_device_header virtio_rng_pci_device = {
+	.vendor_id			= PCI_VENDOR_ID_REDHAT_QUMRANET,
+	.device_id			= PCI_DEVICE_ID_VIRTIO_RNG,
+	.header_type		= PCI_HEADER_TYPE_NORMAL,
+	.revision_id		= 0,
+	.class				= 0x010000,
+	.subsys_vendor_id	= PCI_SUBSYSTEM_VENDOR_ID_REDHAT_QUMRANET,
+	.subsys_id			= PCI_SUBSYSTEM_ID_VIRTIO_RNG,
+	.bar[0]				= IOPORT_VIRTIO_RNG | PCI_BASE_ADDRESS_SPACE_IO,
+	.irq_pin			= VIRTIO_RNG_PIN,
+	.irq_line			= VIRTIO_RNG_IRQ,
+};
+
+void virtio_rng__init(struct kvm *kvm)
+{
+	rng_device.fd_rng = open("/dev/urandom", O_RDONLY);
+	if (rng_device.fd_rng < 0)
+		die("Failed initializing RNG");
+
+	pci__register(&virtio_rng_pci_device, PCI_VIRTIO_RNG_DEVNUM);
+
+	ioport__register(IOPORT_VIRTIO_RNG, &virtio_rng_io_ops, IOPORT_VIRTIO_RNG_SIZE);
+}