Re: hw_random fixes

Re: hw_random fixes

[Matt Mackall]

[cc:ing to linux-kernel, finally]

On Wed, 2009-11-25 at 10:16 +0000, Ian Molton wrote:
> Rusty Russell wrote:
> 
> > Make that:
> > 
> > 	ssize_t (*get_rng_data)(void *buf, size_t max, bool wait);
> > 
> > Then, if driver supplies that hook, use it exclusively.  Otherwise, use old
> > ones.  We can convert them gradually that way.
> 
> This doesn't quite solve things neatly, because it means one of:
> 
> 1) The core has to wait until there is nothing left before requesting
> more data, because it doesnt know the alignment requirements of the driver.

Hmm, this seems to imply you'd be calling get_rng_data multiple times
with different offsets into buf to accumulate data. I think that's more
complex than is needed. Just use buf as a nicely aligned scratch buffer
and empty it completely into the final output buffer before the next
driver request. If you end up with 1 byte of data hanging around until
the next read, that's not a problem - the next read might be 5 bytes.

You'll probably want to use cacheline alignment on buf to make Via
Padlock happy, if anything needs larger alignment (ie page) it should
handle it internally.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: hw_random fixes

[Ian Molton]

Matt Mackall wrote:
> [cc:ing to linux-kernel, finally]
> 
> On Wed, 2009-11-25 at 10:16 +0000, Ian Molton wrote:
>> Rusty Russell wrote:
>>
>>> Make that:
>>>
>>> 	ssize_t (*get_rng_data)(void *buf, size_t max, bool wait);
>>>
>>> Then, if driver supplies that hook, use it exclusively.  Otherwise, use old
>>> ones.  We can convert them gradually that way.
>> This doesn't quite solve things neatly, because it means one of:
>>
>> 1) The core has to wait until there is nothing left before requesting
>> more data, because it doesnt know the alignment requirements of the driver.
> 
> Hmm, this seems to imply you'd be calling get_rng_data multiple times
> with different offsets into buf to accumulate data.

Well, I was hoping to, since it'd mean we accumulate spare bytes from
those drivers that can return more than one at a time but not less than
4 or 8. IMO though its just not worth handling the corner cases this
causes so I agree, just using it as a scratch buffer is best.

One or two drivers are completion based so they'd need to quiesce before
 shutdown / change, too.

> You'll probably want to use cacheline alignment on buf to make Via
> Padlock happy,

Noted.

(no subject)

[Ian Molton]

This patchset updates the hw_random core to use more efficient buffering.
As an example, the virtio-rng driver is converted to the new API, which fixes
a buffering bug provoked during my work writing a virtio qdev for qemu.

The old API is preserved and all existing drivers should continue to function
without change.

Summary:
 drivers/char/hw_random/core.c       |  120 ++++++++++++++++++++++--------------
 drivers/char/hw_random/virtio-rng.c |   79 ++++++++---------------
 include/linux/hw_random.h           |    9 +-
 3 files changed, 110 insertions(+), 98 deletions(-)

 [PATCH 1/2] hw_random: core updates to allow more efficient drivers
 [PATCH 2/2] virtio: Convert virtio-rng to the new API

[PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

	This patch implements a new method by which hw_random hardware drivers
can pass data to the core more efficiently, using a shared buffer.

The old methods have been retained as a compatability layer until all the
drivers have been updated.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/core.c |  120 ++++++++++++++++++++++++++---------------
 include/linux/hw_random.h     |    9 ++-
 2 files changed, 82 insertions(+), 47 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1573aeb..e179afd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -47,12 +47,14 @@
 #define RNG_MODULE_NAME		"hw_random"
 #define PFX			RNG_MODULE_NAME ": "
 #define RNG_MISCDEV_MINOR	183 /* official */
+#define RNG_BUFFSIZE		64
 
 
 static struct hwrng *current_rng;
 static LIST_HEAD(rng_list);
 static DEFINE_MUTEX(rng_mutex);
-
+static u8 *rng_buffer;
+static int data_avail;
 
 static inline int hwrng_init(struct hwrng *rng)
 {
@@ -67,19 +69,6 @@ static inline void hwrng_cleanup(struct hwrng *rng)
 		rng->cleanup(rng);
 }
 
-static inline int hwrng_data_present(struct hwrng *rng, int wait)
-{
-	if (!rng->data_present)
-		return 1;
-	return rng->data_present(rng, wait);
-}
-
-static inline int hwrng_data_read(struct hwrng *rng, u32 *data)
-{
-	return rng->data_read(rng, data);
-}
-
-
 static int rng_dev_open(struct inode *inode, struct file *filp)
 {
 	/* enforce read-only access to this chrdev */
@@ -91,54 +80,86 @@ static int rng_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static inline int rng_get_data(struct hwrng *rng, u8 *rng_buffer, size_t size,
+			int wait) {
+	int present;
+
+	if (rng->read)
+		return rng->read(rng, (void *)rng_buffer, size, wait);
+
+	if (rng->data_present)
+		present = rng->data_present(rng, wait);
+	else
+		present = 1;
+
+	if (present)
+		return rng->data_read(rng, (u32 *)rng_buffer);
+
+	return 0;
+}
+
 static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 			    size_t size, loff_t *offp)
 {
-	u32 data;
 	ssize_t ret = 0;
 	int err = 0;
-	int bytes_read;
+	int bytes_read, len;
 
 	while (size) {
-		err = -ERESTARTSYS;
-		if (mutex_lock_interruptible(&rng_mutex))
+		if (mutex_lock_interruptible(&rng_mutex)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
+
 		if (!current_rng) {
-			mutex_unlock(&rng_mutex);
 			err = -ENODEV;
-			goto out;
+			goto out_unlock;
 		}
 
-		bytes_read = 0;
-		if (hwrng_data_present(current_rng,
-				       !(filp->f_flags & O_NONBLOCK)))
-			bytes_read = hwrng_data_read(current_rng, &data);
-		mutex_unlock(&rng_mutex);
-
-		err = -EAGAIN;
-		if (!bytes_read && (filp->f_flags & O_NONBLOCK))
-			goto out;
-		if (bytes_read < 0) {
-			err = bytes_read;
-			goto out;
+		if (!data_avail) {
+			bytes_read = rng_get_data(current_rng, rng_buffer,
+				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));
+			if (bytes_read < 0) {
+				err = bytes_read;
+				goto out_unlock;
+			}
+			data_avail = bytes_read;
 		}
 
-		err = -EFAULT;
-		while (bytes_read && size) {
-			if (put_user((u8)data, buf++))
-				goto out;
-			size--;
-			ret++;
-			bytes_read--;
-			data >>= 8;
+		if (!data_avail) {
+			if (filp->f_flags & O_NONBLOCK) {
+				err = -EAGAIN;
+				goto out_unlock;
+			}
+		} else {
+			len = data_avail;
+			if (len > size)
+				len = size;
+
+			data_avail -= len;
+
+			if (copy_to_user(buf + ret, rng_buffer + data_avail,
+								len)) {
+				err = -EFAULT;
+				goto out_unlock;
+			}
+
+			size -= len;
+			ret += len;
 		}
 
+		mutex_unlock(&rng_mutex);
+
 		if (need_resched())
 			schedule_timeout_interruptible(1);
-		err = -ERESTARTSYS;
-		if (signal_pending(current))
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
 	}
+out_unlock:
+	mutex_unlock(&rng_mutex);
 out:
 	return ret ? : err;
 }
@@ -280,11 +301,19 @@ int hwrng_register(struct hwrng *rng)
 	struct hwrng *old_rng, *tmp;
 
 	if (rng->name == NULL ||
-	    rng->data_read == NULL)
+	    (rng->data_read == NULL && rng->read == NULL))
 		goto out;
 
 	mutex_lock(&rng_mutex);
 
+	if (!rng_buffer) {
+		rng_buffer = kmalloc(RNG_BUFFSIZE, GFP_KERNEL);
+		if (!rng_buffer) {
+			err = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+
 	/* Must not register two RNGs with the same name. */
 	err = -EEXIST;
 	list_for_each_entry(tmp, &rng_list, list) {
@@ -338,8 +367,11 @@ void hwrng_unregister(struct hwrng *rng)
 				current_rng = NULL;
 		}
 	}
-	if (list_empty(&rng_list))
+	if (list_empty(&rng_list)) {
 		unregister_miscdev();
+		kfree(rng_buffer);
+		rng_buffer = NULL;
+	}
 
 	mutex_unlock(&rng_mutex);
 }
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 7244456..8447989 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -22,18 +22,21 @@
  * @cleanup:		Cleanup callback (can be NULL).
  * @data_present:	Callback to determine if data is available
  *			on the RNG. If NULL, it is assumed that
- *			there is always data available.
+ *			there is always data available.  *OBSOLETE*
  * @data_read:		Read data from the RNG device.
  *			Returns the number of lower random bytes in "data".
- *			Must not be NULL.
+ *			Must not be NULL.    *OSOLETE*
+ * @read:		New API. drivers can fill up to max bytes of data
+ *			into the buffer. The buffer is aligned for any type.
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
 	const char *name;
-	int (*init)(struct hwrng *rng);
+	int (*init)(struct hwrng *rng, void *data, size_t size);
 	void (*cleanup)(struct hwrng *rng);
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
+	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
 	unsigned long priv;
 
 	/* internal. */
-- 
1.6.5

[PATCH 2/2] virtio: Convert virtio-rng to the new API

[Ian Molton]

	This patch converts virtio-rng to the new hw_rng API.

In the process it fixes a previously untriggered buffering bug where the
buffer is not drained correctly if it has a non-multiple-of-4 length.

Performance has improved under qemu-kvm testing also.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/virtio-rng.c |   79 ++++++++++++----------------------
 1 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 915157f..76d00c4 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -16,6 +16,7 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
+
 #include <linux/err.h>
 #include <linux/hw_random.h>
 #include <linux/scatterlist.h>
@@ -23,78 +24,65 @@
 #include <linux/virtio.h>
 #include <linux/virtio_rng.h>
 
-/* The host will fill any buffer we give it with sweet, sweet randomness.  We
- * give it 64 bytes at a time, and the hwrng framework takes it 4 bytes at a
- * time. */
-#define RANDOM_DATA_SIZE 64
-
 static struct virtqueue *vq;
-static u32 *random_data;
-static unsigned int data_left;
+static unsigned int data_avail;
 static DECLARE_COMPLETION(have_data);
+static int busy;
+
+/* The host will fill any buffer we give it with sweet, sweet randomness. */
 
 static void random_recv_done(struct virtqueue *vq)
 {
-	unsigned int len;
-
 	/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
-	if (!vq->vq_ops->get_buf(vq, &len))
+	if (!vq->vq_ops->get_buf(vq, &data_avail))
 		return;
 
-	data_left += len;
 	complete(&have_data);
 }
 
-static void register_buffer(void)
+static void register_buffer(u8 *buf, size_t size)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, random_data+data_left, RANDOM_DATA_SIZE-data_left);
+	sg_init_one(&sg, buf, size);
+
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) < 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, buf) < 0)
 		BUG();
+
 	vq->vq_ops->kick(vq);
 }
 
-/* At least we don't udelay() in a loop like some other drivers. */
-static int virtio_data_present(struct hwrng *rng, int wait)
+static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
-	if (data_left >= sizeof(u32))
-		return 1;
 
-again:
+	if (!busy) {
+		busy = 1;
+		init_completion(&have_data);
+		register_buffer(buf, size);
+	}
+
 	if (!wait)
 		return 0;
 
 	wait_for_completion(&have_data);
 
-	/* Not enough?  Re-register. */
-	if (unlikely(data_left < sizeof(u32))) {
-		register_buffer();
-		goto again;
-	}
+	busy = 0;
 
-	return 1;
+	return data_avail;
 }
 
-/* virtio_data_present() must have succeeded before this is called. */
-static int virtio_data_read(struct hwrng *rng, u32 *data)
+static void virtio_cleanup(struct hwrng *rng)
 {
-	BUG_ON(data_left < sizeof(u32));
-	data_left -= sizeof(u32);
-	*data = random_data[data_left / 4];
-
-	if (data_left < sizeof(u32)) {
-		init_completion(&have_data);
-		register_buffer();
-	}
-	return sizeof(*data);
+	if (busy)
+		wait_for_completion(&have_data);
 }
 
+
 static struct hwrng virtio_hwrng = {
-	.name = "virtio",
-	.data_present = virtio_data_present,
-	.data_read = virtio_data_read,
+	.name		= "virtio",
+	.cleanup	= virtio_cleanup,
+	.read		= virtio_read,
 };
 
 static int virtrng_probe(struct virtio_device *vdev)
@@ -112,7 +100,6 @@ static int virtrng_probe(struct virtio_device *vdev)
 		return err;
 	}
 
-	register_buffer();
 	return 0;
 }
 
@@ -138,21 +125,11 @@ static struct virtio_driver virtio_rng = {
 
 static int __init init(void)
 {
-	int err;
-
-	random_data = kmalloc(RANDOM_DATA_SIZE, GFP_KERNEL);
-	if (!random_data)
-		return -ENOMEM;
-
-	err = register_virtio_driver(&virtio_rng);
-	if (err)
-		kfree(random_data);
-	return err;
+	return register_virtio_driver(&virtio_rng);
 }
 
 static void __exit fini(void)
 {
-	kfree(random_data);
 	unregister_virtio_driver(&virtio_rng);
 }
 module_init(init);
-- 
1.6.5

(no subject)

[Ian Molton]

Oops, slightly early patch sent out. One line differs, but this is the correct
(and compileable) version.

Sorry for the noise,

-Ian

[PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

	This patch implements a new method by which hw_random hardware drivers
can pass data to the core more efficiently, using a shared buffer.

The old methods have been retained as a compatability layer until all the
drivers have been updated.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/core.c |  120 ++++++++++++++++++++++++++---------------
 include/linux/hw_random.h     |    7 ++-
 2 files changed, 81 insertions(+), 46 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1573aeb..e179afd 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -47,12 +47,14 @@
 #define RNG_MODULE_NAME		"hw_random"
 #define PFX			RNG_MODULE_NAME ": "
 #define RNG_MISCDEV_MINOR	183 /* official */
+#define RNG_BUFFSIZE		64
 
 
 static struct hwrng *current_rng;
 static LIST_HEAD(rng_list);
 static DEFINE_MUTEX(rng_mutex);
-
+static u8 *rng_buffer;
+static int data_avail;
 
 static inline int hwrng_init(struct hwrng *rng)
 {
@@ -67,19 +69,6 @@ static inline void hwrng_cleanup(struct hwrng *rng)
 		rng->cleanup(rng);
 }
 
-static inline int hwrng_data_present(struct hwrng *rng, int wait)
-{
-	if (!rng->data_present)
-		return 1;
-	return rng->data_present(rng, wait);
-}
-
-static inline int hwrng_data_read(struct hwrng *rng, u32 *data)
-{
-	return rng->data_read(rng, data);
-}
-
-
 static int rng_dev_open(struct inode *inode, struct file *filp)
 {
 	/* enforce read-only access to this chrdev */
@@ -91,54 +80,86 @@ static int rng_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static inline int rng_get_data(struct hwrng *rng, u8 *rng_buffer, size_t size,
+			int wait) {
+	int present;
+
+	if (rng->read)
+		return rng->read(rng, (void *)rng_buffer, size, wait);
+
+	if (rng->data_present)
+		present = rng->data_present(rng, wait);
+	else
+		present = 1;
+
+	if (present)
+		return rng->data_read(rng, (u32 *)rng_buffer);
+
+	return 0;
+}
+
 static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 			    size_t size, loff_t *offp)
 {
-	u32 data;
 	ssize_t ret = 0;
 	int err = 0;
-	int bytes_read;
+	int bytes_read, len;
 
 	while (size) {
-		err = -ERESTARTSYS;
-		if (mutex_lock_interruptible(&rng_mutex))
+		if (mutex_lock_interruptible(&rng_mutex)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
+
 		if (!current_rng) {
-			mutex_unlock(&rng_mutex);
 			err = -ENODEV;
-			goto out;
+			goto out_unlock;
 		}
 
-		bytes_read = 0;
-		if (hwrng_data_present(current_rng,
-				       !(filp->f_flags & O_NONBLOCK)))
-			bytes_read = hwrng_data_read(current_rng, &data);
-		mutex_unlock(&rng_mutex);
-
-		err = -EAGAIN;
-		if (!bytes_read && (filp->f_flags & O_NONBLOCK))
-			goto out;
-		if (bytes_read < 0) {
-			err = bytes_read;
-			goto out;
+		if (!data_avail) {
+			bytes_read = rng_get_data(current_rng, rng_buffer,
+				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));
+			if (bytes_read < 0) {
+				err = bytes_read;
+				goto out_unlock;
+			}
+			data_avail = bytes_read;
 		}
 
-		err = -EFAULT;
-		while (bytes_read && size) {
-			if (put_user((u8)data, buf++))
-				goto out;
-			size--;
-			ret++;
-			bytes_read--;
-			data >>= 8;
+		if (!data_avail) {
+			if (filp->f_flags & O_NONBLOCK) {
+				err = -EAGAIN;
+				goto out_unlock;
+			}
+		} else {
+			len = data_avail;
+			if (len > size)
+				len = size;
+
+			data_avail -= len;
+
+			if (copy_to_user(buf + ret, rng_buffer + data_avail,
+								len)) {
+				err = -EFAULT;
+				goto out_unlock;
+			}
+
+			size -= len;
+			ret += len;
 		}
 
+		mutex_unlock(&rng_mutex);
+
 		if (need_resched())
 			schedule_timeout_interruptible(1);
-		err = -ERESTARTSYS;
-		if (signal_pending(current))
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
 	}
+out_unlock:
+	mutex_unlock(&rng_mutex);
 out:
 	return ret ? : err;
 }
@@ -280,11 +301,19 @@ int hwrng_register(struct hwrng *rng)
 	struct hwrng *old_rng, *tmp;
 
 	if (rng->name == NULL ||
-	    rng->data_read == NULL)
+	    (rng->data_read == NULL && rng->read == NULL))
 		goto out;
 
 	mutex_lock(&rng_mutex);
 
+	if (!rng_buffer) {
+		rng_buffer = kmalloc(RNG_BUFFSIZE, GFP_KERNEL);
+		if (!rng_buffer) {
+			err = -ENOMEM;
+			goto out_unlock;
+		}
+	}
+
 	/* Must not register two RNGs with the same name. */
 	err = -EEXIST;
 	list_for_each_entry(tmp, &rng_list, list) {
@@ -338,8 +367,11 @@ void hwrng_unregister(struct hwrng *rng)
 				current_rng = NULL;
 		}
 	}
-	if (list_empty(&rng_list))
+	if (list_empty(&rng_list)) {
 		unregister_miscdev();
+		kfree(rng_buffer);
+		rng_buffer = NULL;
+	}
 
 	mutex_unlock(&rng_mutex);
 }
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 7244456..9bede76 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -22,10 +22,12 @@
  * @cleanup:		Cleanup callback (can be NULL).
  * @data_present:	Callback to determine if data is available
  *			on the RNG. If NULL, it is assumed that
- *			there is always data available.
+ *			there is always data available.  *OBSOLETE*
  * @data_read:		Read data from the RNG device.
  *			Returns the number of lower random bytes in "data".
- *			Must not be NULL.
+ *			Must not be NULL.    *OSOLETE*
+ * @read:		New API. drivers can fill up to max bytes of data
+ *			into the buffer. The buffer is aligned for any type.
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -34,6 +36,7 @@ struct hwrng {
 	void (*cleanup)(struct hwrng *rng);
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
+	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
 	unsigned long priv;
 
 	/* internal. */
-- 
1.6.5

[PATCH 2/2] virtio: Convert virtio-rng to the new API

[Ian Molton]

	This patch converts virtio-rng to the new hw_rng API.

In the process it fixes a previously untriggered buffering bug where the
buffer is not drained correctly if it has a non-multiple-of-4 length.

Performance has improved under qemu-kvm testing also.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/virtio-rng.c |   79 ++++++++++++----------------------
 1 files changed, 28 insertions(+), 51 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 915157f..76d00c4 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -16,6 +16,7 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
+
 #include <linux/err.h>
 #include <linux/hw_random.h>
 #include <linux/scatterlist.h>
@@ -23,78 +24,65 @@
 #include <linux/virtio.h>
 #include <linux/virtio_rng.h>
 
-/* The host will fill any buffer we give it with sweet, sweet randomness.  We
- * give it 64 bytes at a time, and the hwrng framework takes it 4 bytes at a
- * time. */
-#define RANDOM_DATA_SIZE 64
-
 static struct virtqueue *vq;
-static u32 *random_data;
-static unsigned int data_left;
+static unsigned int data_avail;
 static DECLARE_COMPLETION(have_data);
+static int busy;
+
+/* The host will fill any buffer we give it with sweet, sweet randomness. */
 
 static void random_recv_done(struct virtqueue *vq)
 {
-	unsigned int len;
-
 	/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
-	if (!vq->vq_ops->get_buf(vq, &len))
+	if (!vq->vq_ops->get_buf(vq, &data_avail))
 		return;
 
-	data_left += len;
 	complete(&have_data);
 }
 
-static void register_buffer(void)
+static void register_buffer(u8 *buf, size_t size)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, random_data+data_left, RANDOM_DATA_SIZE-data_left);
+	sg_init_one(&sg, buf, size);
+
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) < 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, buf) < 0)
 		BUG();
+
 	vq->vq_ops->kick(vq);
 }
 
-/* At least we don't udelay() in a loop like some other drivers. */
-static int virtio_data_present(struct hwrng *rng, int wait)
+static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
-	if (data_left >= sizeof(u32))
-		return 1;
 
-again:
+	if (!busy) {
+		busy = 1;
+		init_completion(&have_data);
+		register_buffer(buf, size);
+	}
+
 	if (!wait)
 		return 0;
 
 	wait_for_completion(&have_data);
 
-	/* Not enough?  Re-register. */
-	if (unlikely(data_left < sizeof(u32))) {
-		register_buffer();
-		goto again;
-	}
+	busy = 0;
 
-	return 1;
+	return data_avail;
 }
 
-/* virtio_data_present() must have succeeded before this is called. */
-static int virtio_data_read(struct hwrng *rng, u32 *data)
+static void virtio_cleanup(struct hwrng *rng)
 {
-	BUG_ON(data_left < sizeof(u32));
-	data_left -= sizeof(u32);
-	*data = random_data[data_left / 4];
-
-	if (data_left < sizeof(u32)) {
-		init_completion(&have_data);
-		register_buffer();
-	}
-	return sizeof(*data);
+	if (busy)
+		wait_for_completion(&have_data);
 }
 
+
 static struct hwrng virtio_hwrng = {
-	.name = "virtio",
-	.data_present = virtio_data_present,
-	.data_read = virtio_data_read,
+	.name		= "virtio",
+	.cleanup	= virtio_cleanup,
+	.read		= virtio_read,
 };
 
 static int virtrng_probe(struct virtio_device *vdev)
@@ -112,7 +100,6 @@ static int virtrng_probe(struct virtio_device *vdev)
 		return err;
 	}
 
-	register_buffer();
 	return 0;
 }
 
@@ -138,21 +125,11 @@ static struct virtio_driver virtio_rng = {
 
 static int __init init(void)
 {
-	int err;
-
-	random_data = kmalloc(RANDOM_DATA_SIZE, GFP_KERNEL);
-	if (!random_data)
-		return -ENOMEM;
-
-	err = register_virtio_driver(&virtio_rng);
-	if (err)
-		kfree(random_data);
-	return err;
+	return register_virtio_driver(&virtio_rng);
 }
 
 static void __exit fini(void)
 {
-	kfree(random_data);
 	unregister_virtio_driver(&virtio_rng);
 }
 module_init(init);
-- 
1.6.5

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Matt Mackall]

On Thu, 2009-11-26 at 00:25 +0000, Ian Molton wrote:
> 	This patch implements a new method by which hw_random hardware drivers
> can pass data to the core more efficiently, using a shared buffer.
> 
> The old methods have been retained as a compatability layer until all the
> drivers have been updated.
> 
> Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
> ---
>  drivers/char/hw_random/core.c |  120 ++++++++++++++++++++++++++---------------
>  include/linux/hw_random.h     |    9 ++-
>  2 files changed, 82 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
> index 1573aeb..e179afd 100644
> --- a/drivers/char/hw_random/core.c
> +++ b/drivers/char/hw_random/core.c
> @@ -47,12 +47,14 @@
>  #define RNG_MODULE_NAME		"hw_random"
>  #define PFX			RNG_MODULE_NAME ": "
>  #define RNG_MISCDEV_MINOR	183 /* official */
> +#define RNG_BUFFSIZE		64
>  
> 
>  static struct hwrng *current_rng;
>  static LIST_HEAD(rng_list);
>  static DEFINE_MUTEX(rng_mutex);
> -
> +static u8 *rng_buffer;

How about just:

static u8 rng_buffer[RNG_BUFFSIZE] __cacheline_aligned;

And lose all the kmalloc and kfree code? The memory use will be smaller,
even when the buffer isn't needed.

> +		if (!data_avail) {
> +			bytes_read = rng_get_data(current_rng, rng_buffer,
> +				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));

No need to pass rng_buffer to the helper as there's only one with global
scope.

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Jeff Garzik]

On 11/25/2009 07:25 PM, Ian Molton wrote:
> 	This patch implements a new method by which hw_random hardware drivers
> can pass data to the core more efficiently, using a shared buffer.
>
> The old methods have been retained as a compatability layer until all the
> drivers have been updated.
>
> Signed-off-by: Ian Molton<ian.molton@collabora.co.uk>
> ---
>   drivers/char/hw_random/core.c |  120 ++++++++++++++++++++++++++---------------
>   include/linux/hw_random.h     |    9 ++-
>   2 files changed, 82 insertions(+), 47 deletions(-)

Speaking as the original hw_random author...  very nice!  We have needed 
a more efficient API for years.

	Jeff

(no subject)

[Ian Molton]

Hi guys,

	This version uses a statically allocated buffer. I dont feel it is a
good idea not to pass the address and length of the buffer to the hardware
drivers, as they shouldnt have intimate knowledge of the core, IMO.

Only resendiong the core patch, the virtio-rng driver hasnt changed.

hw_random: core updates to allow more efficient drivers

-Ian

[PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

	This patch implements a new method by which hw_random hardware drivers
can pass data to the core more efficiently, using a shared buffer.

The old methods have been retained as a compatability layer until all the
drivers have been updated.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/core.c |  107 ++++++++++++++++++++++++----------------
 include/linux/hw_random.h     |    7 ++-
 2 files changed, 69 insertions(+), 45 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1573aeb..da31573 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -47,12 +47,14 @@
 #define RNG_MODULE_NAME		"hw_random"
 #define PFX			RNG_MODULE_NAME ": "
 #define RNG_MISCDEV_MINOR	183 /* official */
+#define RNG_BUFFSIZE		64
 
 
 static struct hwrng *current_rng;
 static LIST_HEAD(rng_list);
 static DEFINE_MUTEX(rng_mutex);
-
+static int data_avail;
+static u8 rng_buffer[RNG_BUFFSIZE] __cacheline_aligned;
 
 static inline int hwrng_init(struct hwrng *rng)
 {
@@ -67,19 +69,6 @@ static inline void hwrng_cleanup(struct hwrng *rng)
 		rng->cleanup(rng);
 }
 
-static inline int hwrng_data_present(struct hwrng *rng, int wait)
-{
-	if (!rng->data_present)
-		return 1;
-	return rng->data_present(rng, wait);
-}
-
-static inline int hwrng_data_read(struct hwrng *rng, u32 *data)
-{
-	return rng->data_read(rng, data);
-}
-
-
 static int rng_dev_open(struct inode *inode, struct file *filp)
 {
 	/* enforce read-only access to this chrdev */
@@ -91,54 +80,86 @@ static int rng_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
+			int wait) {
+	int present;
+
+	if (rng->read)
+		return rng->read(rng, (void *)buffer, size, wait);
+
+	if (rng->data_present)
+		present = rng->data_present(rng, wait);
+	else
+		present = 1;
+
+	if (present)
+		return rng->data_read(rng, (u32 *)buffer);
+
+	return 0;
+}
+
 static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 			    size_t size, loff_t *offp)
 {
-	u32 data;
 	ssize_t ret = 0;
 	int err = 0;
-	int bytes_read;
+	int bytes_read, len;
 
 	while (size) {
-		err = -ERESTARTSYS;
-		if (mutex_lock_interruptible(&rng_mutex))
+		if (mutex_lock_interruptible(&rng_mutex)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
+
 		if (!current_rng) {
-			mutex_unlock(&rng_mutex);
 			err = -ENODEV;
-			goto out;
+			goto out_unlock;
 		}
 
-		bytes_read = 0;
-		if (hwrng_data_present(current_rng,
-				       !(filp->f_flags & O_NONBLOCK)))
-			bytes_read = hwrng_data_read(current_rng, &data);
-		mutex_unlock(&rng_mutex);
-
-		err = -EAGAIN;
-		if (!bytes_read && (filp->f_flags & O_NONBLOCK))
-			goto out;
-		if (bytes_read < 0) {
-			err = bytes_read;
-			goto out;
+		if (!data_avail) {
+			bytes_read = rng_get_data(current_rng, rng_buffer,
+				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));
+			if (bytes_read < 0) {
+				err = bytes_read;
+				goto out_unlock;
+			}
+			data_avail = bytes_read;
 		}
 
-		err = -EFAULT;
-		while (bytes_read && size) {
-			if (put_user((u8)data, buf++))
-				goto out;
-			size--;
-			ret++;
-			bytes_read--;
-			data >>= 8;
+		if (!data_avail) {
+			if (filp->f_flags & O_NONBLOCK) {
+				err = -EAGAIN;
+				goto out_unlock;
+			}
+		} else {
+			len = data_avail;
+			if (len > size)
+				len = size;
+
+			data_avail -= len;
+
+			if (copy_to_user(buf + ret, rng_buffer + data_avail,
+								len)) {
+				err = -EFAULT;
+				goto out_unlock;
+			}
+
+			size -= len;
+			ret += len;
 		}
 
+		mutex_unlock(&rng_mutex);
+
 		if (need_resched())
 			schedule_timeout_interruptible(1);
-		err = -ERESTARTSYS;
-		if (signal_pending(current))
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
 	}
+out_unlock:
+	mutex_unlock(&rng_mutex);
 out:
 	return ret ? : err;
 }
@@ -280,7 +301,7 @@ int hwrng_register(struct hwrng *rng)
 	struct hwrng *old_rng, *tmp;
 
 	if (rng->name == NULL ||
-	    rng->data_read == NULL)
+	    (rng->data_read == NULL && rng->read == NULL))
 		goto out;
 
 	mutex_lock(&rng_mutex);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 7244456..9bede76 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -22,10 +22,12 @@
  * @cleanup:		Cleanup callback (can be NULL).
  * @data_present:	Callback to determine if data is available
  *			on the RNG. If NULL, it is assumed that
- *			there is always data available.
+ *			there is always data available.  *OBSOLETE*
  * @data_read:		Read data from the RNG device.
  *			Returns the number of lower random bytes in "data".
- *			Must not be NULL.
+ *			Must not be NULL.    *OSOLETE*
+ * @read:		New API. drivers can fill up to max bytes of data
+ *			into the buffer. The buffer is aligned for any type.
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -34,6 +36,7 @@ struct hwrng {
 	void (*cleanup)(struct hwrng *rng);
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
+	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
 	unsigned long priv;
 
 	/* internal. */
-- 
1.6.5

Re:

[Matt Mackall]

On Thu, 2009-11-26 at 10:49 +0000, Ian Molton wrote:
> Hi guys,
> 
> 	This version uses a statically allocated buffer. I dont feel it is a
> good idea not to pass the address and length of the buffer to the hardware
> drivers, as they shouldnt have intimate knowledge of the core, IMO.

I agree, but let me quote myself:
---
> +             if (!data_avail) {
> +                     bytes_read = rng_get_data(current_rng, rng_buffer,
> +                             RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));

No need to pass rng_buffer to the helper as there's only one with global
scope.
---

-- 
http://selenic.com : development and support for Mercurial and Linux

Re:

[Ian Molton]

Matt Mackall wrote:
> On Thu, 2009-11-26 at 10:49 +0000, Ian Molton wrote:
>> Hi guys,
>>
> 
> No need to pass rng_buffer to the helper as there's only one with global
> scope.

Ah, sorry, I see what you mean now. The logic behind that is that it
matches the new API, whcih is all that will be left once the old drivers
are patched to use it. I planned to drop the helper altogether at that
point and though it'd make the patch more readable when that happens.

I can drop it if thats preferable, though.

Is this enough to get an acked-by: ? If so, I'll do that and see about
getting the change into linux-next.

Rusty: are you happy with the new version of virtio-rng ?

Cheers guys,

-Ian

Re: Re:

[Matt Mackall]

On Thu, 2009-11-26 at 11:48 +0000, Ian Molton wrote:
> Matt Mackall wrote:
> > On Thu, 2009-11-26 at 10:49 +0000, Ian Molton wrote:
> >> Hi guys,
> >>
> > 
> > No need to pass rng_buffer to the helper as there's only one with global
> > scope.
> 
> Ah, sorry, I see what you mean now. The logic behind that is that it
> matches the new API, whcih is all that will be left once the old drivers
> are patched to use it. I planned to drop the helper altogether at that
> point and though it'd make the patch more readable when that happens.

Ok, that's quite reasonable.

> Is this enough to get an acked-by: ? If so, I'll do that and see about
> getting the change into linux-next.

Acked-by: Matt Mackall <mpm@selenic.com>

You should probably go through Herbert's tree to get into -next,
hopefully he won't be too miffed by your repeated failure to cc:
linux-kernel initially and failure to cc: him here..

-- 
http://selenic.com : development and support for Mercurial and Linux

rng updates

[Ian Molton]

Matt Mackall wrote:
> On Thu, 2009-11-26 at 11:48 +0000, Ian Molton wrote:
>> Matt Mackall wrote:
>>> On Thu, 2009-11-26 at 10:49 +0000, Ian Molton wrote:
>>>> Hi guys,
>>>>
>>> No need to pass rng_buffer to the helper as there's only one with global
>>> scope.
>> Ah, sorry, I see what you mean now. The logic behind that is that it
>> matches the new API, whcih is all that will be left once the old drivers
>> are patched to use it. I planned to drop the helper altogether at that
>> point and though it'd make the patch more readable when that happens.
> 
> Ok, that's quite reasonable.
> 
>> Is this enough to get an acked-by: ? If so, I'll do that and see about
>> getting the change into linux-next.
> 
> Acked-by: Matt Mackall <mpm@selenic.com>

Just to check - is that with or without changing the helpers parameters?

> You should probably go through Herbert's tree to get into -next,
> hopefully he won't be too miffed by your repeated failure to cc:
> linux-kernel initially and failure to cc: him here..

Drat, I thought he'd been re-added when LKML got added to the CC: list.
Sorry about that. Re-added to CC:

-Ian

Re: [PATCH 2/2] virtio: Convert virtio-rng to the new API

[Rusty Russell]

On Thu, 26 Nov 2009 10:55:27 am Ian Molton wrote:
> 	This patch converts virtio-rng to the new hw_rng API.
> 
> In the process it fixes a previously untriggered buffering bug where the
> buffer is not drained correctly if it has a non-multiple-of-4 length.

Hi Ian,

	Looks good.  Minor comments below:

> @@ -16,6 +16,7 @@
>   *  along with this program; if not, write to the Free Software
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>   */
> +
>  #include <linux/err.h>
>  #include <linux/hw_random.h>
>  #include <linux/scatterlist.h>

Gratuitous hunk?

> -static unsigned int data_left;
> +static unsigned int data_avail;
>  static DECLARE_COMPLETION(have_data);
> +static int busy;

I prefer bool and true/false for booleans these days.

> +
> +/* The host will fill any buffer we give it with sweet, sweet randomness. */

This comment belongs above register_buffer() now I think.

(But I'm glad you kept it!)

Thanks,
Rusty.

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Rusty Russell]

On Thu, 26 Nov 2009 11:33:23 am Matt Mackall wrote:
> On Thu, 2009-11-26 at 00:25 +0000, Ian Molton wrote:
> >  #define PFX			RNG_MODULE_NAME ": "
> >  #define RNG_MISCDEV_MINOR	183 /* official */
> > +#define RNG_BUFFSIZE		64
> 
> How about just:
> 
> static u8 rng_buffer[RNG_BUFFSIZE] __cacheline_aligned;
> 
> And lose all the kmalloc and kfree code? The memory use will be smaller,
> even when the buffer isn't needed.

And might as well just #defube RNG_BUFFSIZE SMP_CACHE_BYTES (or use
SMP_CACHE_BYTES here and sizeof() elsewhere).

> > +		if (!data_avail) {
> > +			bytes_read = rng_get_data(current_rng, rng_buffer,
> > +				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));
> 
> No need to pass rng_buffer to the helper as there's only one with global
> scope.

Naah, I like the separation; it matches the rest of the kernel and means we
can get funky with buffer management in 10 years time when we rewrite this
again.

Thanks,
Rusty.

Re: [PATCH 2/2] virtio: Convert virtio-rng to the new API

[Ian Molton]

Rusty Russell wrote:
> On Thu, 26 Nov 2009 10:55:27 am Ian Molton wrote:
>> 	This patch converts virtio-rng to the new hw_rng API.
>>
>> In the process it fixes a previously untriggered buffering bug where the
>> buffer is not drained correctly if it has a non-multiple-of-4 length.
> 
> Hi Ian,

Hi!

> 	Looks good.  Minor comments below:
> 
>> @@ -16,6 +16,7 @@
>>   *  along with this program; if not, write to the Free Software
>>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>>   */
>> +
>>  #include <linux/err.h>
>>  #include <linux/hw_random.h>
>>  #include <linux/scatterlist.h>
> 
> Gratuitous hunk?

Didn't look right without a gap :)

>> -static unsigned int data_left;
>> +static unsigned int data_avail;
>>  static DECLARE_COMPLETION(have_data);
>> +static int busy;
> 
> I prefer bool and true/false for booleans these days.

I was wondering what the current opinion was on that one. I have to say
I prefer it too, so I'll change that.

>> +
>> +/* The host will fill any buffer we give it with sweet, sweet randomness. */
> 
> This comment belongs above register_buffer() now I think.

Yeah, didnt wanna make too much fus over it though. I'll move it too
then :-)

> (But I'm glad you kept it!)

I think a sense of humour is important ;-)

Patch to follow.

-Ian

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

Rusty Russell wrote:

> And might as well just #defube RNG_BUFFSIZE SMP_CACHE_BYTES (or use
> SMP_CACHE_BYTES here and sizeof() elsewhere).

This can lead to a rather small (4 byte) buffer on some systems, however
I don't know if in practice a tiny buffer or a big one would be better
for performance on those machines. I guess if its a problem someone can
patch the code to allocate a minimum of (say) 16 bytes in future...

so changed :)

>>> +		if (!data_avail) {
>>> +			bytes_read = rng_get_data(current_rng, rng_buffer,
>>> +				RNG_BUFFSIZE, !(filp->f_flags & O_NONBLOCK));
>> No need to pass rng_buffer to the helper as there's only one with global
>> scope.
> 
> Naah, I like the separation; it matches the rest of the kernel and means we
> can get funky with buffer management in 10 years time when we rewrite this
> again.

Tweaked to use sizeof().

hw_random update

[Ian Molton]

Final (hopefully) version of my hw_random updates. The first patch updates the
hw_random core to use an appropriately sized and aligned buffer to receive
random data and the second patch adapts virtio-rng to use it, fixing a
previously unprovoked buffer alignment bug as a bonus.

[PATCH 1/2] hw_random: core updates to allow more efficient drivers
[PATCH 2/2] virtio: Convert virtio-rng to the new API

[PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

	This patch implements a new method by which hw_random hardware drivers
can pass data to the core more efficiently, using a shared buffer.

The old methods have been retained as a compatability layer until all the
drivers have been updated.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/core.c |  107 ++++++++++++++++++++++++----------------
 include/linux/hw_random.h     |    7 ++-
 2 files changed, 69 insertions(+), 45 deletions(-)

diff --git a/drivers/char/hw_random/core.c b/drivers/char/hw_random/core.c
index 1573aeb..5c2d13c 100644
--- a/drivers/char/hw_random/core.c
+++ b/drivers/char/hw_random/core.c
@@ -52,7 +52,8 @@
 static struct hwrng *current_rng;
 static LIST_HEAD(rng_list);
 static DEFINE_MUTEX(rng_mutex);
-
+static int data_avail;
+static u8 rng_buffer[SMP_CACHE_BYTES] __cacheline_aligned;
 
 static inline int hwrng_init(struct hwrng *rng)
 {
@@ -67,19 +68,6 @@ static inline void hwrng_cleanup(struct hwrng *rng)
 		rng->cleanup(rng);
 }
 
-static inline int hwrng_data_present(struct hwrng *rng, int wait)
-{
-	if (!rng->data_present)
-		return 1;
-	return rng->data_present(rng, wait);
-}
-
-static inline int hwrng_data_read(struct hwrng *rng, u32 *data)
-{
-	return rng->data_read(rng, data);
-}
-
-
 static int rng_dev_open(struct inode *inode, struct file *filp)
 {
 	/* enforce read-only access to this chrdev */
@@ -91,54 +79,87 @@ static int rng_dev_open(struct inode *inode, struct file *filp)
 	return 0;
 }
 
+static inline int rng_get_data(struct hwrng *rng, u8 *buffer, size_t size,
+			int wait) {
+	int present;
+
+	if (rng->read)
+		return rng->read(rng, (void *)buffer, size, wait);
+
+	if (rng->data_present)
+		present = rng->data_present(rng, wait);
+	else
+		present = 1;
+
+	if (present)
+		return rng->data_read(rng, (u32 *)buffer);
+
+	return 0;
+}
+
 static ssize_t rng_dev_read(struct file *filp, char __user *buf,
 			    size_t size, loff_t *offp)
 {
-	u32 data;
 	ssize_t ret = 0;
 	int err = 0;
-	int bytes_read;
+	int bytes_read, len;
 
 	while (size) {
-		err = -ERESTARTSYS;
-		if (mutex_lock_interruptible(&rng_mutex))
+		if (mutex_lock_interruptible(&rng_mutex)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
+
 		if (!current_rng) {
-			mutex_unlock(&rng_mutex);
 			err = -ENODEV;
-			goto out;
+			goto out_unlock;
 		}
 
-		bytes_read = 0;
-		if (hwrng_data_present(current_rng,
-				       !(filp->f_flags & O_NONBLOCK)))
-			bytes_read = hwrng_data_read(current_rng, &data);
-		mutex_unlock(&rng_mutex);
-
-		err = -EAGAIN;
-		if (!bytes_read && (filp->f_flags & O_NONBLOCK))
-			goto out;
-		if (bytes_read < 0) {
-			err = bytes_read;
-			goto out;
+		if (!data_avail) {
+			bytes_read = rng_get_data(current_rng, rng_buffer,
+				sizeof(rng_buffer),
+				!(filp->f_flags & O_NONBLOCK));
+			if (bytes_read < 0) {
+				err = bytes_read;
+				goto out_unlock;
+			}
+			data_avail = bytes_read;
 		}
 
-		err = -EFAULT;
-		while (bytes_read && size) {
-			if (put_user((u8)data, buf++))
-				goto out;
-			size--;
-			ret++;
-			bytes_read--;
-			data >>= 8;
+		if (!data_avail) {
+			if (filp->f_flags & O_NONBLOCK) {
+				err = -EAGAIN;
+				goto out_unlock;
+			}
+		} else {
+			len = data_avail;
+			if (len > size)
+				len = size;
+
+			data_avail -= len;
+
+			if (copy_to_user(buf + ret, rng_buffer + data_avail,
+								len)) {
+				err = -EFAULT;
+				goto out_unlock;
+			}
+
+			size -= len;
+			ret += len;
 		}
 
+		mutex_unlock(&rng_mutex);
+
 		if (need_resched())
 			schedule_timeout_interruptible(1);
-		err = -ERESTARTSYS;
-		if (signal_pending(current))
+
+		if (signal_pending(current)) {
+			err = -ERESTARTSYS;
 			goto out;
+		}
 	}
+out_unlock:
+	mutex_unlock(&rng_mutex);
 out:
 	return ret ? : err;
 }
@@ -280,7 +301,7 @@ int hwrng_register(struct hwrng *rng)
 	struct hwrng *old_rng, *tmp;
 
 	if (rng->name == NULL ||
-	    rng->data_read == NULL)
+	    (rng->data_read == NULL && rng->read == NULL))
 		goto out;
 
 	mutex_lock(&rng_mutex);
diff --git a/include/linux/hw_random.h b/include/linux/hw_random.h
index 7244456..9bede76 100644
--- a/include/linux/hw_random.h
+++ b/include/linux/hw_random.h
@@ -22,10 +22,12 @@
  * @cleanup:		Cleanup callback (can be NULL).
  * @data_present:	Callback to determine if data is available
  *			on the RNG. If NULL, it is assumed that
- *			there is always data available.
+ *			there is always data available.  *OBSOLETE*
  * @data_read:		Read data from the RNG device.
  *			Returns the number of lower random bytes in "data".
- *			Must not be NULL.
+ *			Must not be NULL.    *OSOLETE*
+ * @read:		New API. drivers can fill up to max bytes of data
+ *			into the buffer. The buffer is aligned for any type.
  * @priv:		Private data, for use by the RNG driver.
  */
 struct hwrng {
@@ -34,6 +36,7 @@ struct hwrng {
 	void (*cleanup)(struct hwrng *rng);
 	int (*data_present)(struct hwrng *rng, int wait);
 	int (*data_read)(struct hwrng *rng, u32 *data);
+	int (*read)(struct hwrng *rng, void *data, size_t max, bool wait);
 	unsigned long priv;
 
 	/* internal. */
-- 
1.6.5

[PATCH 2/2] virtio: Convert virtio-rng to the new API

[Ian Molton]

	This patch converts virtio-rng to the new hw_rng API.

In the process it fixes a previously untriggered buffering bug where the
buffer is not drained correctly if it has a non-multiple-of-4 length.

Performance has improved under qemu-kvm testing also.

Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>
---
 drivers/char/hw_random/virtio-rng.c |   78 ++++++++++++-----------------------
 1 files changed, 27 insertions(+), 51 deletions(-)

diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c
index 915157f..bdaef8e 100644
--- a/drivers/char/hw_random/virtio-rng.c
+++ b/drivers/char/hw_random/virtio-rng.c
@@ -16,6 +16,7 @@
  *  along with this program; if not, write to the Free Software
  *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
  */
+
 #include <linux/err.h>
 #include <linux/hw_random.h>
 #include <linux/scatterlist.h>
@@ -23,78 +24,64 @@
 #include <linux/virtio.h>
 #include <linux/virtio_rng.h>
 
-/* The host will fill any buffer we give it with sweet, sweet randomness.  We
- * give it 64 bytes at a time, and the hwrng framework takes it 4 bytes at a
- * time. */
-#define RANDOM_DATA_SIZE 64
-
 static struct virtqueue *vq;
-static u32 *random_data;
-static unsigned int data_left;
+static unsigned int data_avail;
 static DECLARE_COMPLETION(have_data);
+static bool busy;
 
 static void random_recv_done(struct virtqueue *vq)
 {
-	unsigned int len;
-
 	/* We can get spurious callbacks, e.g. shared IRQs + virtio_pci. */
-	if (!vq->vq_ops->get_buf(vq, &len))
+	if (!vq->vq_ops->get_buf(vq, &data_avail))
 		return;
 
-	data_left += len;
 	complete(&have_data);
 }
 
-static void register_buffer(void)
+/* The host will fill any buffer we give it with sweet, sweet randomness. */
+static void register_buffer(u8 *buf, size_t size)
 {
 	struct scatterlist sg;
 
-	sg_init_one(&sg, random_data+data_left, RANDOM_DATA_SIZE-data_left);
+	sg_init_one(&sg, buf, size);
+
 	/* There should always be room for one buffer. */
-	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, random_data) < 0)
+	if (vq->vq_ops->add_buf(vq, &sg, 0, 1, buf) < 0)
 		BUG();
+
 	vq->vq_ops->kick(vq);
 }
 
-/* At least we don't udelay() in a loop like some other drivers. */
-static int virtio_data_present(struct hwrng *rng, int wait)
+static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait)
 {
-	if (data_left >= sizeof(u32))
-		return 1;
 
-again:
+	if (!busy) {
+		busy = true;
+		init_completion(&have_data);
+		register_buffer(buf, size);
+	}
+
 	if (!wait)
 		return 0;
 
 	wait_for_completion(&have_data);
 
-	/* Not enough?  Re-register. */
-	if (unlikely(data_left < sizeof(u32))) {
-		register_buffer();
-		goto again;
-	}
+	busy = false;
 
-	return 1;
+	return data_avail;
 }
 
-/* virtio_data_present() must have succeeded before this is called. */
-static int virtio_data_read(struct hwrng *rng, u32 *data)
+static void virtio_cleanup(struct hwrng *rng)
 {
-	BUG_ON(data_left < sizeof(u32));
-	data_left -= sizeof(u32);
-	*data = random_data[data_left / 4];
-
-	if (data_left < sizeof(u32)) {
-		init_completion(&have_data);
-		register_buffer();
-	}
-	return sizeof(*data);
+	if (busy)
+		wait_for_completion(&have_data);
 }
 
+
 static struct hwrng virtio_hwrng = {
-	.name = "virtio",
-	.data_present = virtio_data_present,
-	.data_read = virtio_data_read,
+	.name		= "virtio",
+	.cleanup	= virtio_cleanup,
+	.read		= virtio_read,
 };
 
 static int virtrng_probe(struct virtio_device *vdev)
@@ -112,7 +99,6 @@ static int virtrng_probe(struct virtio_device *vdev)
 		return err;
 	}
 
-	register_buffer();
 	return 0;
 }
 
@@ -138,21 +124,11 @@ static struct virtio_driver virtio_rng = {
 
 static int __init init(void)
 {
-	int err;
-
-	random_data = kmalloc(RANDOM_DATA_SIZE, GFP_KERNEL);
-	if (!random_data)
-		return -ENOMEM;
-
-	err = register_virtio_driver(&virtio_rng);
-	if (err)
-		kfree(random_data);
-	return err;
+	return register_virtio_driver(&virtio_rng);
 }
 
 static void __exit fini(void)
 {
-	kfree(random_data);
 	unregister_virtio_driver(&virtio_rng);
 }
 module_init(init);
-- 
1.6.5

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Matt Mackall]

On Mon, 2009-11-30 at 10:28 +0000, Ian Molton wrote:
> Rusty Russell wrote:
> 
> > And might as well just #defube RNG_BUFFSIZE SMP_CACHE_BYTES (or use
> > SMP_CACHE_BYTES here and sizeof() elsewhere).
> 
> This can lead to a rather small (4 byte) buffer on some systems, however
> I don't know if in practice a tiny buffer or a big one would be better
> for performance on those machines. I guess if its a problem someone can
> patch the code to allocate a minimum of (say) 16 bytes in future...

Hmmm, I think this was bad advice from Rusty.

The goal is to size and align the buffer so that we know it will always
work. Thus 64 bytes (always big enough but not so big that anyone will
complain) and cache aligned (makes stupid things like Via Padlock happy
-on Vias-). 

Rusty's suggestion could easily have us in trouble if some driver wants
to hand us a mere 64 bits on an architecture with 4-byte cache alignment
but is otherwise perfectly happy with 64-bit stores. 

-- 
http://selenic.com : development and support for Mercurial and Linux

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Rusty Russell]

On Tue, 1 Dec 2009 05:14:16 am Matt Mackall wrote:
> On Mon, 2009-11-30 at 10:28 +0000, Ian Molton wrote:
> > Rusty Russell wrote:
> > 
> > > And might as well just #defube RNG_BUFFSIZE SMP_CACHE_BYTES (or use
> > > SMP_CACHE_BYTES here and sizeof() elsewhere).
> > 
> > This can lead to a rather small (4 byte) buffer on some systems, however
> > I don't know if in practice a tiny buffer or a big one would be better
> > for performance on those machines. I guess if its a problem someone can
> > patch the code to allocate a minimum of (say) 16 bytes in future...
> 
> Hmmm, I think this was bad advice from Rusty.

Me too.  But really, it's because we're using cacheline alignment
as a proxy for something else, and it's not a good fit.

But we're bikeshedding, so apply or revert.

	/* A buffer which can hold any fundamental type: drivers are fussy. */
	static u8 rng_buffer[SMP_CACHE_BYTES < 8 ? 8 : SMP_CACHE_BYTES]
		__cacheline_aligned;

Either way, both patches: Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks,
Rusty.

Re: [PATCH 2/2] virtio: Convert virtio-rng to the new API

[Herbert Xu]

On Mon, Nov 30, 2009 at 10:38:21AM +0000, Ian Molton wrote:
> 	This patch converts virtio-rng to the new hw_rng API.
> 
> In the process it fixes a previously untriggered buffering bug where the
> buffer is not drained correctly if it has a non-multiple-of-4 length.
> 
> Performance has improved under qemu-kvm testing also.
> 
> Signed-off-by: Ian Molton <ian.molton@collabora.co.uk>

Both patched applied.  Thanks everyone!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

Matt Mackall wrote:
> On Mon, 2009-11-30 at 10:28 +0000, Ian Molton wrote:
>> Rusty Russell wrote:
>>
>>> And might as well just #defube RNG_BUFFSIZE SMP_CACHE_BYTES (or use
>>> SMP_CACHE_BYTES here and sizeof() elsewhere).
>> This can lead to a rather small (4 byte) buffer on some systems, however
>> I don't know if in practice a tiny buffer or a big one would be better
>> for performance on those machines. I guess if its a problem someone can
>> patch the code to allocate a minimum of (say) 16 bytes in future...
> 
> Hmmm, I think this was bad advice from Rusty.

Not entirely...

> The goal is to size and align the buffer so that we know it will always
> work. Thus 64 bytes (always big enough but not so big that anyone will
> complain) and cache aligned (makes stupid things like Via Padlock happy
> -on Vias-). 

yep. Although making it the size of a cacheline makes sense on /most/
modern architectures - 32 bytes is a very common size - I think the
(current) worst case is one of the drivers wants to dump 3 u64s in one
go. virtio-rng will take what it can...

> Rusty's suggestion could easily have us in trouble if some driver wants
> to hand us a mere 64 bits on an architecture with 4-byte cache alignment
> but is otherwise perfectly happy with 64-bit stores. 

How about SNP_CACHE_BYTES or if less, then 32 bytes minimum? Or just
stick with 64 bytes. Either way works for me.

-Ian

Re: [PATCH 1/2] hw_random: core updates to allow more efficient drivers

[Ian Molton]

Rusty Russell wrote:

> But we're bikeshedding, so apply or revert.
> 
> 	/* A buffer which can hold any fundamental type: drivers are fussy. */
> 	static u8 rng_buffer[SMP_CACHE_BYTES < 8 ? 8 : SMP_CACHE_BYTES]
> 		__cacheline_aligned;

One nit - theres one driver where <24 bytes will be quite suboptimal.
I'd say pick 32 bytes, which is a common cachline size, too.

> Either way, both patches: Acked-by: Rusty Russell <rusty@rustcorp.com.au>

Thanks!

-Ian

Re: [PATCH 2/2] virtio: Convert virtio-rng to the new API

[Ian Molton]

Herbert Xu wrote:

> Both patched applied.  Thanks everyone!

Ta! Thanks all for the review.