Re: [patch 2/9] Add new generic HW RNG core

Re: [patch 2/9] Add new generic HW RNG core

[Andrew Morton]

Michael Buesch <mb@bu3sch.de> wrote:
>
> ...
>
> +static inline
> +int hwrng_init(struct hwrng *rng)
> +static inline
> +void hwrng_cleanup(struct hwrng *rng)
> +static inline
> +int hwrng_data_present(struct hwrng *rng)
> +static inline
> +int hwrng_data_read(struct hwrng *rng, u32 *data)

Lose the newlines, please.

> +static int rng_dev_open(struct inode *inode, struct file *filp)

Like that.

> +static ssize_t rng_dev_read(struct file *filp, char __user *buf,
> +			    size_t size, loff_t *offp)
> +{
> +	int have_data;
> +	u32 data;
> +	ssize_t ret = 0;
> +	int i, err = 0;
> +
> +	while (size) {
> +		err = -ERESTARTSYS;
> +		if (mutex_lock_interruptible(&rng_mutex))
> +			goto out;
> +		if (!current_rng) {
> +			mutex_unlock(&rng_mutex);
> +			err = -ENODEV;
> +			goto out;
> +		}
> +		have_data = 0;
> +		if (hwrng_data_present(current_rng))
> +			have_data = hwrng_data_read(current_rng, &data);
> +		mutex_unlock(&rng_mutex);
> +
> +		err = -EFAULT;
> +		while (have_data && size) {
> +			if (put_user((u8)data, buf++))
> +				goto out;
> +			size--;
> +			ret++;
> +			have_data--;
> +			data >>= 8;
> +		}
> +
> +		err = -EAGAIN;
> +		if (filp->f_flags & O_NONBLOCK)
> +			goto out;
> +
> +		err = -ERESTARTSYS;
> +		if (need_resched()) {
> +			if (schedule_timeout_interruptible(1))
> +				goto out;
> +		} else {
> +			if (mutex_lock_interruptible(&rng_mutex))
> +				goto out;
> +			if (!current_rng) {
> +				mutex_unlock(&rng_mutex);
> +				err = -ENODEV;
> +				goto out;
> +			}
> +			for (i = 0; i < 20; i++) {
> +				if (hwrng_data_present(current_rng))
> +					break;
> +				udelay(10);
> +			}
> +			mutex_unlock(&rng_mutex);
> +		}
> +		if (signal_pending(current))
> +			goto out;
> +	}
> +out:
> +	return ret ? : err;
> +}

What's going on with the need_resched() tricks in there?  (Unobvious, needs
a comment).  From my reading, it'll cause a caller to this function to hang
for arbitrary periods of time if something if causing heavy scheduling
pressure.

What's the polling of hwrng_data_present() doing in here?  (Unobvious,
needs a comment).

> +static ssize_t hwrng_attr_current_store(struct class_device *class,
> +					const char *buf, size_t len)
> +{
> +	int err;
> +	struct hwrng *rng;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;

Are the sysfs permissions not adequate?

> +MODULE_AUTHOR("The Linux Kernel team");

Mutter.  Might as well remove this.

A MAINTAINERS record would be nice.

Re: [patch 2/9] Add new generic HW RNG core

[Michael Buesch]

I will send patches for the following stuff, on top
of your current -mm, which has my previous patches applied.

On Tuesday 16 May 2006 00:02, you wrote:
> Michael Buesch <mb@bu3sch.de> wrote:
> > +static inline
> > +int hwrng_init(struct hwrng *rng)
> > +static inline
> > +void hwrng_cleanup(struct hwrng *rng)
> > +static inline
> > +int hwrng_data_present(struct hwrng *rng)
> > +static inline
> > +int hwrng_data_read(struct hwrng *rng, u32 *data)
> 
> Lose the newlines, please.

No problem.

> What's going on with the need_resched() tricks in there?  (Unobvious, needs
> a comment).  From my reading, it'll cause a caller to this function to hang
> for arbitrary periods of time if something if causing heavy scheduling
> pressure.

Yeah, it was like this in the old RNG driver. I would also like to
drop it, as I don't see a real advantage from it. But I don't
like to drop the hwrng_data_present() polling bit of it, because...

> What's the polling of hwrng_data_present() doing in here?  (Unobvious,
> needs a comment).

Some HW RNG might require some time between data_reads to gather
new entropy. The short polling here makes sure we don't return too
early from the syscall. So it reduces syscall overhead. Imagine a
device which needs 20usecs between reads to gather new entropy.
The first read will succeed and probably only return one byte of
entropy. The next attempt to data_present in the while loop will
fail and the syscall will return with one byte read. userspace will
need to call the syscall for every byte (in this case).

We could, of course, add a variable to struct hwrng that indicates the
average needed time a device needs to gather new entropy in most cases.
So we could poll for this time instead of the rather random 200usecs. 

> > +static ssize_t hwrng_attr_current_store(struct class_device *class,
> > +					const char *buf, size_t len)
> > +{
> > +	int err;
> > +	struct hwrng *rng;
> > +
> > +	if (!capable(CAP_SYS_ADMIN))
> > +		return -EPERM;
> 
> Are the sysfs permissions not adequate?

Will drop this.

> > +MODULE_AUTHOR("The Linux Kernel team");
> 
> Mutter.  Might as well remove this.

Sure. Leftover from the old driver.

> A MAINTAINERS record would be nice.

Yes, sir.

Re: [patch 6/9] Add VIA HW RNG driver

[Andrew Morton]

Michael Buesch <mb@bu3sch.de> wrote:
>
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> Index: hwrng/drivers/char/hw_random/Kconfig
> ===================================================================
> --- hwrng.orig/drivers/char/hw_random/Kconfig	2006-05-08 00:12:08.000000000 +0200
> +++ hwrng/drivers/char/hw_random/Kconfig	2006-05-08 00:12:20.000000000 +0200
> @@ -48,3 +48,16 @@
>  	  module will be called geode-rng.
>  
>  	  If unsure, say Y.
> +
> +config HW_RANDOM_VIA
> +	tristate "VIA HW Random Number Generator support"
> +	depends on HW_RANDOM && X86

Perhaps you want X86_32 here.

> +	if (!cpu_has_xstore)

Because this doesn't exist on x86_64.

Re: [patch 6/9] Add VIA HW RNG driver

[Jeff Garzik]

Andrew Morton wrote:
> Michael Buesch <mb@bu3sch.de> wrote:
>> Signed-off-by: Michael Buesch <mb@bu3sch.de>
>> Index: hwrng/drivers/char/hw_random/Kconfig
>> ===================================================================
>> --- hwrng.orig/drivers/char/hw_random/Kconfig	2006-05-08 00:12:08.000000000 +0200
>> +++ hwrng/drivers/char/hw_random/Kconfig	2006-05-08 00:12:20.000000000 +0200
>> @@ -48,3 +48,16 @@
>>  	  module will be called geode-rng.
>>  
>>  	  If unsure, say Y.
>> +
>> +config HW_RANDOM_VIA
>> +	tristate "VIA HW Random Number Generator support"
>> +	depends on HW_RANDOM && X86
> 
> Perhaps you want X86_32 here.

correct (for now)


>> +	if (!cpu_has_xstore)
> 
> Because this doesn't exist on x86_64.

correct (for now)

	Jeff

[patch 0/9] New Generic HW RNG (#3)

[mb]

This patch series replaces the old non-generic Hardware Random Number Generator support by a fully generic RNG API.

This makes it possible to register additional RNGs from modules. With this patch series applied, Laptops with
a bcm43xx chip (PowerBook) have a HW RNG available now.

Additionally two new RNG drivers are added for the "ixp4xx" and "omap" devices. (Written by Deepak Saxena).
This patch series includes the old patches by Deepak Saxena.

The old x86-rng driver has beed splitted.

The userspace RNG daemon can later be updated to select the RNG through /sys/class/misc/hw_random/ for convenience. For now it is sufficient to use cat and echo -n on the sysfs attributes.

Please apply to -mm for testing.

[patch 6/9] Add VIA HW RNG driver

[mb]

[-- Attachment #1: add-via-hw-random.patch --]
[-- Type: text/plain, Size: 6537 bytes --]

Signed-off-by: Michael Buesch <mb@bu3sch.de>
Index: hwrng/drivers/char/hw_random/Kconfig
===================================================================
--- hwrng.orig/drivers/char/hw_random/Kconfig	2006-05-08 00:12:08.000000000 +0200
+++ hwrng/drivers/char/hw_random/Kconfig	2006-05-08 00:12:20.000000000 +0200
@@ -48,3 +48,16 @@
 	  module will be called geode-rng.
 
 	  If unsure, say Y.
+
+config HW_RANDOM_VIA
+	tristate "VIA HW Random Number Generator support"
+	depends on HW_RANDOM && X86
+	default y
+	---help---
+	  This driver provides kernel-side support for the Random Number
+	  Generator hardware found on VIA based motherboards.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called via-rng.
+
+	  If unsure, say Y.
Index: hwrng/drivers/char/hw_random/Makefile
===================================================================
--- hwrng.orig/drivers/char/hw_random/Makefile	2006-05-08 00:12:03.000000000 +0200
+++ hwrng/drivers/char/hw_random/Makefile	2006-05-08 00:12:12.000000000 +0200
@@ -6,3 +6,4 @@
 obj-$(CONFIG_HW_RANDOM_INTEL) += intel-rng.o
 obj-$(CONFIG_HW_RANDOM_AMD) += amd-rng.o
 obj-$(CONFIG_HW_RANDOM_GEODE) += geode-rng.o
+obj-$(CONFIG_HW_RANDOM_VIA) += via-rng.o
Index: hwrng/drivers/char/hw_random/via-rng.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ hwrng/drivers/char/hw_random/via-rng.c	2006-05-08 00:12:12.000000000 +0200
@@ -0,0 +1,184 @@
+/*
+ * RNG driver for VIA RNGs
+ *
+ * Copyright 2005 (c) MontaVista Software, Inc.
+ *
+ * with the majority of the code coming from:
+ *
+ * Hardware driver for the Intel/AMD/VIA Random Number Generators (RNG)
+ * (c) Copyright 2003 Red Hat Inc <jgarzik@redhat.com>
+ *
+ * derived from
+ *
+ * Hardware driver for the AMD 768 Random Number Generator (RNG)
+ * (c) Copyright 2001 Red Hat Inc <alan@redhat.com>
+ *
+ * derived from
+ *
+ * Hardware driver for Intel i810 Random Number Generator (RNG)
+ * Copyright 2000,2001 Jeff Garzik <jgarzik@pobox.com>
+ * Copyright 2000,2001 Philipp Rumpf <prumpf@mandrakesoft.com>
+ *
+ * This file is licensed under  the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/pci.h>
+#include <linux/hw_random.h>
+#include <asm/io.h>
+#include <asm/msr.h>
+#include <asm/cpufeature.h>
+
+
+#define PFX	KBUILD_MODNAME ": "
+
+
+enum {
+	VIA_STRFILT_CNT_SHIFT	= 16,
+	VIA_STRFILT_FAIL	= (1 << 15),
+	VIA_STRFILT_ENABLE	= (1 << 14),
+	VIA_RAWBITS_ENABLE	= (1 << 13),
+	VIA_RNG_ENABLE		= (1 << 6),
+	VIA_XSTORE_CNT_MASK	= 0x0F,
+
+	VIA_RNG_CHUNK_8		= 0x00,	/* 64 rand bits, 64 stored bits */
+	VIA_RNG_CHUNK_4		= 0x01,	/* 32 rand bits, 32 stored bits */
+	VIA_RNG_CHUNK_4_MASK	= 0xFFFFFFFF,
+	VIA_RNG_CHUNK_2		= 0x02,	/* 16 rand bits, 32 stored bits */
+	VIA_RNG_CHUNK_2_MASK	= 0xFFFF,
+	VIA_RNG_CHUNK_1		= 0x03,	/* 8 rand bits, 32 stored bits */
+	VIA_RNG_CHUNK_1_MASK	= 0xFF,
+};
+
+/*
+ * Investigate using the 'rep' prefix to obtain 32 bits of random data
+ * in one insn.  The upside is potentially better performance.  The
+ * downside is that the instruction becomes no longer atomic.  Due to
+ * this, just like familiar issues with /dev/random itself, the worst
+ * case of a 'rep xstore' could potentially pause a cpu for an
+ * unreasonably long time.  In practice, this condition would likely
+ * only occur when the hardware is failing.  (or so we hope :))
+ *
+ * Another possible performance boost may come from simply buffering
+ * until we have 4 bytes, thus returning a u32 at a time,
+ * instead of the current u8-at-a-time.
+ */
+
+static inline u32 xstore(u32 *addr, u32 edx_in)
+{
+	u32 eax_out;
+
+	asm(".byte 0x0F,0xA7,0xC0 /* xstore %%edi (addr=%0) */"
+		:"=m"(*addr), "=a"(eax_out)
+		:"D"(addr), "d"(edx_in));
+
+	return eax_out;
+}
+
+static int via_rng_data_present(struct hwrng *rng)
+{
+	u32 bytes_out;
+	u32 *via_rng_datum = (u32 *)(&rng->priv);
+
+	/* We choose the recommended 1-byte-per-instruction RNG rate,
+	 * for greater randomness at the expense of speed.  Larger
+	 * values 2, 4, or 8 bytes-per-instruction yield greater
+	 * speed at lesser randomness.
+	 *
+	 * If you change this to another VIA_CHUNK_n, you must also
+	 * change the ->n_bytes values in rng_vendor_ops[] tables.
+	 * VIA_CHUNK_8 requires further code changes.
+	 *
+	 * A copy of MSR_VIA_RNG is placed in eax_out when xstore
+	 * completes.
+	 */
+
+	*via_rng_datum = 0; /* paranoia, not really necessary */
+	bytes_out = xstore(via_rng_datum, VIA_RNG_CHUNK_1);
+	bytes_out &= VIA_XSTORE_CNT_MASK;
+	if (bytes_out == 0)
+		return 0;
+	return 1;
+}
+
+static int via_rng_data_read(struct hwrng *rng, u32 *data)
+{
+	u32 via_rng_datum = (u32)rng->priv;
+
+	*data = via_rng_datum;
+
+	return 1;
+}
+
+static int via_rng_init(struct hwrng *rng)
+{
+	u32 lo, hi, old_lo;
+
+	/* Control the RNG via MSR.  Tread lightly and pay very close
+	 * close attention to values written, as the reserved fields
+	 * are documented to be "undefined and unpredictable"; but it
+	 * does not say to write them as zero, so I make a guess that
+	 * we restore the values we find in the register.
+	 */
+	rdmsr(MSR_VIA_RNG, lo, hi);
+
+	old_lo = lo;
+	lo &= ~(0x7f << VIA_STRFILT_CNT_SHIFT);
+	lo &= ~VIA_XSTORE_CNT_MASK;
+	lo &= ~(VIA_STRFILT_ENABLE | VIA_STRFILT_FAIL | VIA_RAWBITS_ENABLE);
+	lo |= VIA_RNG_ENABLE;
+
+	if (lo != old_lo)
+		wrmsr(MSR_VIA_RNG, lo, hi);
+
+	/* perhaps-unnecessary sanity check; remove after testing if
+	   unneeded */
+	rdmsr(MSR_VIA_RNG, lo, hi);
+	if ((lo & VIA_RNG_ENABLE) == 0) {
+		printk(KERN_ERR PFX "cannot enable VIA C3 RNG, aborting\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+
+static struct hwrng via_rng = {
+	.name		= "via",
+	.init		= via_rng_init,
+	.data_present	= via_rng_data_present,
+	.data_read	= via_rng_data_read,
+};
+
+
+static int __init mod_init(void)
+{
+	int err;
+
+	if (!cpu_has_xstore)
+		return -ENODEV;
+	printk(KERN_INFO "VIA RNG detected\n");
+	err = hwrng_register(&via_rng);
+	if (err) {
+		printk(KERN_ERR PFX "RNG registering failed (%d)\n",
+		       err);
+		goto out;
+	}
+out:
+	return err;
+}
+
+static void __exit mod_exit(void)
+{
+	hwrng_unregister(&via_rng);
+}
+
+subsys_initcall(mod_init);
+module_exit(mod_exit);
+
+MODULE_AUTHOR("The Linux Kernel team");
+MODULE_DESCRIPTION("H/W RNG driver for VIA chipsets");
+MODULE_LICENSE("GPL");

--