[PATCH 1/3] kvm tools: Lock job_mutex before signalling

[PATCH 1/3] kvm tools: Lock job_mutex before signalling

[Sasha Levin]

Locking mutex before signalling to prevent unexpected
scheduling.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/threadpool.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/threadpool.c b/tools/kvm/threadpool.c
index c584ec7..700c08f 100644
--- a/tools/kvm/threadpool.c
+++ b/tools/kvm/threadpool.c
@@ -166,5 +166,7 @@ void thread_pool__signal_work(void *job)
 		thread_pool__job_info_push_locked(job);
 	mutex_unlock(&jobinfo->mutex);
 
+	mutex_lock(&job_mutex);
 	pthread_cond_signal(&job_cond);
+	mutex_unlock(&job_mutex);
 }
-- 
1.7.5.rc3

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

[Sasha Levin]

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

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);
+}
-- 
1.7.5.rc3

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

[Ingo Molnar]

* 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

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

[Pekka Enberg]

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

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

[Ingo Molnar]

* 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

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

[Pekka Enberg]

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.

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

[Sasha Levin]

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.

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

[Sasha Levin]

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?

-- 

Sasha.

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

[Ingo Molnar]

* 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

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

[Ingo Molnar]

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

[PATCH 3/3] kvm tools: Add cmdline switch to enable virtio-rng

[Sasha Levin]

Add --virtio-rnd switch to enable virtio RNG in the guest.
Once enabled, The RNG device will be located at /dev/hwrng.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/kvm-run.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
index 64f3409..6fd46ed 100644
--- a/tools/kvm/kvm-run.c
+++ b/tools/kvm/kvm-run.c
@@ -19,6 +19,7 @@
 #include <kvm/virtio-blk.h>
 #include <kvm/virtio-net.h>
 #include <kvm/virtio-console.h>
+#include <kvm/virtio-rng.h>
 #include <kvm/disk-image.h>
 #include <kvm/util.h>
 #include <kvm/pci.h>
@@ -59,6 +60,7 @@ static const char *guest_mac;
 static const char *script;
 static bool single_step;
 static bool readonly_image;
+static bool virtio_rng;
 extern bool ioport_debug;
 extern int  active_console;
 
@@ -78,6 +80,8 @@ static const struct option options[] = {
 			"Don't write changes back to disk image"),
 	OPT_STRING('c', "console", &console, "serial or virtio",
 			"Console to use"),
+	OPT_BOOLEAN('\0', "virtio-rng", &virtio_rng,
+			"Enable virtio Random Number Generator"),
 
 	OPT_GROUP("Kernel options:"),
 	OPT_STRING('k', "kernel", &kernel_filename, "kernel",
@@ -429,6 +433,9 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 
 	virtio_console__init(kvm);
 
+	if (virtio_rng)
+		virtio_rng__init(kvm);
+
 	if (!network)
 		network = DEFAULT_NETWORK;
 
-- 
1.7.5.rc3

Re: [PATCH 1/3] kvm tools: Lock job_mutex before signalling

[Pekka Enberg]

On Sat, 2011-04-30 at 16:30 +0300, Sasha Levin wrote:
> Locking mutex before signalling to prevent unexpected
> scheduling.
> 
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>

Yeah, I think we need to do this but the changelog is not really
adequate for this kind of change. Care to elaborate more why it's
needed?

Re: [PATCH 1/3] kvm tools: Lock job_mutex before signalling

[Sasha Levin]

On Sun, 2011-05-01 at 10:10 +0300, Pekka Enberg wrote:
> On Sat, 2011-04-30 at 16:30 +0300, Sasha Levin wrote:
> > Locking mutex before signalling to prevent unexpected
> > scheduling.
> > 
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> 
> Yeah, I think we need to do this but the changelog is not really
> adequate for this kind of change. Care to elaborate more why it's
> needed?
> 

By holding the mutex while signaling, we can expect predictable
scheduling between the worker threads. This will make debugging various
thread pool related issues much easier (and maybe solve some obscure
bugs too).

-- 

Sasha.