diff mbox series

[v5,8/8] virtio: rng: Add a random number generator(rng) driver

Message ID 1577381108-6320-9-git-send-email-sughosh.ganu@linaro.org
State Superseded, archived
Headers show
Series Add a random number generator uclass | expand

Commit Message

Sughosh Ganu Dec. 26, 2019, 5:25 p.m. UTC 
Add a driver for the virtio-rng device on the qemu platform. The
device uses pci as a transport medium. The driver can be enabled with
the following configs

CONFIG_VIRTIO
CONFIG_DM_RNG
CONFIG_VIRTIO_PCI
CONFIG_VIRTIO_RNG

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V4:
* Return number of bytes read on a successful read, and a -ve value on
  error.

 drivers/virtio/Kconfig         |  6 +++
 drivers/virtio/Makefile        |  1 +
 drivers/virtio/virtio-uclass.c |  1 +
 drivers/virtio/virtio_rng.c    | 85 ++++++++++++++++++++++++++++++++++++++++++
 include/virtio.h               |  4 +-
 5 files changed, 96 insertions(+), 1 deletion(-)
 create mode 100644 drivers/virtio/virtio_rng.c

Comments

Heinrich Schuchardt Dec. 27, 2019, 7:12 a.m. UTC | #1 
On 12/26/19 6:25 PM, Sughosh Ganu wrote:
> Add a driver for the virtio-rng device on the qemu platform. The
> device uses pci as a transport medium. The driver can be enabled with
> the following configs
>
> CONFIG_VIRTIO
> CONFIG_DM_RNG
> CONFIG_VIRTIO_PCI
> CONFIG_VIRTIO_RNG
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V4:
> * Return number of bytes read on a successful read, and a -ve value on
>    error.
>
>   drivers/virtio/Kconfig         |  6 +++
>   drivers/virtio/Makefile        |  1 +
>   drivers/virtio/virtio-uclass.c |  1 +
>   drivers/virtio/virtio_rng.c    | 85 ++++++++++++++++++++++++++++++++++++++++++
>   include/virtio.h               |  4 +-
>   5 files changed, 96 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/virtio/virtio_rng.c
>
> diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
> index a9d5fd0..2e3dd3b 100644
> --- a/drivers/virtio/Kconfig
> +++ b/drivers/virtio/Kconfig
> @@ -59,4 +59,10 @@ config VIRTIO_BLK
>   	  This is the virtual block driver for virtio. It can be used with
>   	  QEMU based targets.
>
> +config VIRTIO_RNG
> +       bool "virtio rng driver"
> +       depends on VIRTIO
> +       help
> +         This is the virtual random number generator driver. It can be used
> +	 with Qemu based targets.
>   endmenu
> diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
> index 4579044..dc88809 100644
> --- a/drivers/virtio/Makefile
> +++ b/drivers/virtio/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_VIRTIO_PCI) += virtio_pci_legacy.o virtio_pci_modern.o
>   obj-$(CONFIG_VIRTIO_SANDBOX) += virtio_sandbox.o
>   obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
>   obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
> +obj-$(CONFIG_VIRTIO_RNG) += virtio_rng.o
> diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c
> index 34397d7..436faa4 100644
> --- a/drivers/virtio/virtio-uclass.c
> +++ b/drivers/virtio/virtio-uclass.c
> @@ -24,6 +24,7 @@
>   static const char *const virtio_drv_name[VIRTIO_ID_MAX_NUM] = {
>   	[VIRTIO_ID_NET]		= VIRTIO_NET_DRV_NAME,
>   	[VIRTIO_ID_BLOCK]	= VIRTIO_BLK_DRV_NAME,
> +	[VIRTIO_ID_RNG]		= VIRTIO_RNG_DRV_NAME,
>   };
>
>   int virtio_get_config(struct udevice *vdev, unsigned int offset,
> diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> new file mode 100644
> index 0000000..fda8d04
> --- /dev/null
> +++ b/drivers/virtio/virtio_rng.c
> @@ -0,0 +1,85 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <rng.h>
> +#include <virtio_types.h>
> +#include <virtio.h>
> +#include <virtio_ring.h>
> +
> +struct virtio_rng_priv {
> +	struct virtqueue *rng_vq;
> +};
> +
> +static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> +{
> +	int ret;
> +	unsigned int rsize = 0;
> +	struct virtio_sg sg;
> +	struct virtio_sg *sgs[1];
> +	struct virtio_rng_priv *priv = dev_get_priv(dev);
> +
> +	/*
> +	 * Only INT_MAX number of bytes can be returned. If more
> +	 * bytes need to be read, the caller must do it in a loop.
> +	 */
> +	if (len > INT_MAX)
> +		len = INT_MAX;
> +
> +	sg.addr = data;
> +	sg.length = len;
> +	sgs[0] = &sg;
> +
> +	ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
> +	if (ret)
> +		return ret;
> +
> +	virtqueue_kick(priv->rng_vq);
> +
> +	while (!virtqueue_get_buf(priv->rng_vq, &rsize))

This code assumes that data is 4 byte aligned and may lead to a crash
due to missing alignment on ARM in function le32_to_cpu().

Best regards

Heinrich

> +		;
> +
> +	return rsize;
> +}
> +
> +static int virtio_rng_bind(struct udevice *dev)
> +{
> +	struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent);
> +
> +	/* Indicate what driver features we support */
> +	virtio_driver_features_init(uc_priv, NULL, 0, NULL, 0);
> +
> +	return 0;
> +}
> +
> +static int virtio_rng_probe(struct udevice *dev)
> +{
> +	struct virtio_rng_priv *priv = dev_get_priv(dev);
> +	int ret;
> +
> +	ret = virtio_find_vqs(dev, 1, &priv->rng_vq);
> +	if (ret < 0) {
> +		debug("%s: virtio_find_vqs failed\n", __func__);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dm_rng_ops virtio_rng_ops = {
> +	.read	= virtio_rng_read,
> +};
> +
> +U_BOOT_DRIVER(virtio_rng) = {
> +	.name	= VIRTIO_RNG_DRV_NAME,
> +	.id	= UCLASS_RNG,
> +	.bind	= virtio_rng_bind,
> +	.probe	= virtio_rng_probe,
> +	.remove = virtio_reset,
> +	.ops	= &virtio_rng_ops,
> +	.priv_auto_alloc_size = sizeof(struct virtio_rng_priv),
> +	.flags	= DM_FLAG_ACTIVE_DMA,
> +};
> diff --git a/include/virtio.h b/include/virtio.h
> index 654fdf1..561dcc3 100644
> --- a/include/virtio.h
> +++ b/include/virtio.h
> @@ -22,10 +22,12 @@
>
>   #define VIRTIO_ID_NET		1 /* virtio net */
>   #define VIRTIO_ID_BLOCK		2 /* virtio block */
> -#define VIRTIO_ID_MAX_NUM	3
> +#define VIRTIO_ID_RNG		4 /* virtio rng */
> +#define VIRTIO_ID_MAX_NUM	5
>
>   #define VIRTIO_NET_DRV_NAME	"virtio-net"
>   #define VIRTIO_BLK_DRV_NAME	"virtio-blk"
> +#define VIRTIO_RNG_DRV_NAME	"virtio-rng"
>
>   /* Status byte for guest to report progress, and synchronize features */
>
>
Sughosh Ganu Dec. 27, 2019, 11:29 a.m. UTC | #2 
On Fri, 27 Dec 2019 at 12:42, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/26/19 6:25 PM, Sughosh Ganu wrote:
> > Add a driver for the virtio-rng device on the qemu platform. The
> > device uses pci as a transport medium. The driver can be enabled with
> > the following configs
> >
> > CONFIG_VIRTIO
> > CONFIG_DM_RNG
> > CONFIG_VIRTIO_PCI
> > CONFIG_VIRTIO_RNG
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V4:
> > * Return number of bytes read on a successful read, and a -ve value on
> >    error.
> >
> >   drivers/virtio/Kconfig         |  6 +++
> >   drivers/virtio/Makefile        |  1 +
> >   drivers/virtio/virtio-uclass.c |  1 +
> >   drivers/virtio/virtio_rng.c    | 85
> ++++++++++++++++++++++++++++++++++++++++++
> >   include/virtio.h               |  4 +-
> >   5 files changed, 96 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/virtio/virtio_rng.c
> >
>

<snip>


> > diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
> > new file mode 100644
> > index 0000000..fda8d04
> > --- /dev/null
> > +++ b/drivers/virtio/virtio_rng.c
> > @@ -0,0 +1,85 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <rng.h>
> > +#include <virtio_types.h>
> > +#include <virtio.h>
> > +#include <virtio_ring.h>
> > +
> > +struct virtio_rng_priv {
> > +     struct virtqueue *rng_vq;
> > +};
> > +
> > +static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
> > +{
> > +     int ret;
> > +     unsigned int rsize = 0;
> > +     struct virtio_sg sg;
> > +     struct virtio_sg *sgs[1];
> > +     struct virtio_rng_priv *priv = dev_get_priv(dev);
> > +
> > +     /*
> > +      * Only INT_MAX number of bytes can be returned. If more
> > +      * bytes need to be read, the caller must do it in a loop.
> > +      */
> > +     if (len > INT_MAX)
> > +             len = INT_MAX;
> > +
> > +     sg.addr = data;
> > +     sg.length = len;
> > +     sgs[0] = &sg;
> > +
> > +     ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
> > +     if (ret)
> > +             return ret;
> > +
> > +     virtqueue_kick(priv->rng_vq);
> > +
> > +     while (!virtqueue_get_buf(priv->rng_vq, &rsize))
>
> This code assumes that data is 4 byte aligned and may lead to a crash
> due to missing alignment on ARM in function le32_to_cpu().
>

Are you referring to the virtqueue_get_buf function. Not sure I understand.
In any case, I will be sending an updated version which moves the loop into
the driver, as you had commented on my other patch.

-sughosh
Heinrich Schuchardt Dec. 27, 2019, 12:27 p.m. UTC | #3 
On 12/27/19 12:29 PM, Sughosh Ganu wrote:
>
> On Fri, 27 Dec 2019 at 12:42, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 12/26/19 6:25 PM, Sughosh Ganu wrote:
>      > Add a driver for the virtio-rng device on the qemu platform. The
>      > device uses pci as a transport medium. The driver can be enabled with
>      > the following configs
>      >
>      > CONFIG_VIRTIO
>      > CONFIG_DM_RNG
>      > CONFIG_VIRTIO_PCI
>      > CONFIG_VIRTIO_RNG
>      >
>      > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>>
>      > ---
>      > Changes since V4:
>      > * Return number of bytes read on a successful read, and a -ve
>     value on
>      >    error.
>      >
>      >   drivers/virtio/Kconfig         |  6 +++
>      >   drivers/virtio/Makefile        |  1 +
>      >   drivers/virtio/virtio-uclass.c |  1 +
>      >   drivers/virtio/virtio_rng.c    | 85
>     ++++++++++++++++++++++++++++++++++++++++++
>      >   include/virtio.h               |  4 +-
>      >   5 files changed, 96 insertions(+), 1 deletion(-)
>      >   create mode 100644 drivers/virtio/virtio_rng.c
>      >
>
>
> <snip>
>
>      > diff --git a/drivers/virtio/virtio_rng.c
>     b/drivers/virtio/virtio_rng.c
>      > new file mode 100644
>      > index 0000000..fda8d04
>      > --- /dev/null
>      > +++ b/drivers/virtio/virtio_rng.c
>      > @@ -0,0 +1,85 @@
>      > +// SPDX-License-Identifier: GPL-2.0+
>      > +/*
>      > + * Copyright (c) 2019, Linaro Limited
>      > + */
>      > +
>      > +#include <common.h>
>      > +#include <dm.h>
>      > +#include <rng.h>
>      > +#include <virtio_types.h>
>      > +#include <virtio.h>
>      > +#include <virtio_ring.h>
>      > +
>      > +struct virtio_rng_priv {
>      > +     struct virtqueue *rng_vq;
>      > +};
>      > +
>      > +static int virtio_rng_read(struct udevice *dev, void *data,
>     size_t len)
>      > +{
>      > +     int ret;
>      > +     unsigned int rsize = 0;
>      > +     struct virtio_sg sg;
>      > +     struct virtio_sg *sgs[1];
>      > +     struct virtio_rng_priv *priv = dev_get_priv(dev);
>      > +
>      > +     /*
>      > +      * Only INT_MAX number of bytes can be returned. If more
>      > +      * bytes need to be read, the caller must do it in a loop.
>      > +      */
>      > +     if (len > INT_MAX)
>      > +             len = INT_MAX;
>      > +
>      > +     sg.addr = data;
>      > +     sg.length = len;
>      > +     sgs[0] = &sg;
>      > +
>      > +     ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
>      > +     if (ret)
>      > +             return ret;
>      > +
>      > +     virtqueue_kick(priv->rng_vq);
>      > +
>      > +     while (!virtqueue_get_buf(priv->rng_vq, &rsize))
>
>     This code assumes that data is 4 byte aligned and may lead to a crash
>     due to missing alignment on ARM in function le32_to_cpu().
>
>
> Are you referring to the virtqueue_get_buf function. Not sure I
> understand. In any case, I will be sending an updated version which
> moves the loop into the driver, as you had commented on my other patch.

virtqueue_get_buf() calls virtio32_to_cpu() which calls le32_to_cpu()
which assumes that the destination is properly aligned.

Best regards

Heinrich
diff mbox series

Patch

diff --git a/drivers/virtio/Kconfig b/drivers/virtio/Kconfig
index a9d5fd0..2e3dd3b 100644
--- a/drivers/virtio/Kconfig
+++ b/drivers/virtio/Kconfig
@@ -59,4 +59,10 @@  config VIRTIO_BLK
 	  This is the virtual block driver for virtio. It can be used with
 	  QEMU based targets.
 
+config VIRTIO_RNG
+       bool "virtio rng driver"
+       depends on VIRTIO
+       help
+         This is the virtual random number generator driver. It can be used
+	 with Qemu based targets.
 endmenu
diff --git a/drivers/virtio/Makefile b/drivers/virtio/Makefile
index 4579044..dc88809 100644
--- a/drivers/virtio/Makefile
+++ b/drivers/virtio/Makefile
@@ -9,3 +9,4 @@  obj-$(CONFIG_VIRTIO_PCI) += virtio_pci_legacy.o virtio_pci_modern.o
 obj-$(CONFIG_VIRTIO_SANDBOX) += virtio_sandbox.o
 obj-$(CONFIG_VIRTIO_NET) += virtio_net.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio_blk.o
+obj-$(CONFIG_VIRTIO_RNG) += virtio_rng.o
diff --git a/drivers/virtio/virtio-uclass.c b/drivers/virtio/virtio-uclass.c
index 34397d7..436faa4 100644
--- a/drivers/virtio/virtio-uclass.c
+++ b/drivers/virtio/virtio-uclass.c
@@ -24,6 +24,7 @@ 
 static const char *const virtio_drv_name[VIRTIO_ID_MAX_NUM] = {
 	[VIRTIO_ID_NET]		= VIRTIO_NET_DRV_NAME,
 	[VIRTIO_ID_BLOCK]	= VIRTIO_BLK_DRV_NAME,
+	[VIRTIO_ID_RNG]		= VIRTIO_RNG_DRV_NAME,
 };
 
 int virtio_get_config(struct udevice *vdev, unsigned int offset,
diff --git a/drivers/virtio/virtio_rng.c b/drivers/virtio/virtio_rng.c
new file mode 100644
index 0000000..fda8d04
--- /dev/null
+++ b/drivers/virtio/virtio_rng.c
@@ -0,0 +1,85 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <rng.h>
+#include <virtio_types.h>
+#include <virtio.h>
+#include <virtio_ring.h>
+
+struct virtio_rng_priv {
+	struct virtqueue *rng_vq;
+};
+
+static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
+{
+	int ret;
+	unsigned int rsize = 0;
+	struct virtio_sg sg;
+	struct virtio_sg *sgs[1];
+	struct virtio_rng_priv *priv = dev_get_priv(dev);
+
+	/*
+	 * Only INT_MAX number of bytes can be returned. If more
+	 * bytes need to be read, the caller must do it in a loop.
+	 */
+	if (len > INT_MAX)
+		len = INT_MAX;
+
+	sg.addr = data;
+	sg.length = len;
+	sgs[0] = &sg;
+
+	ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
+	if (ret)
+		return ret;
+
+	virtqueue_kick(priv->rng_vq);
+
+	while (!virtqueue_get_buf(priv->rng_vq, &rsize))
+		;
+
+	return rsize;
+}
+
+static int virtio_rng_bind(struct udevice *dev)
+{
+	struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent);
+
+	/* Indicate what driver features we support */
+	virtio_driver_features_init(uc_priv, NULL, 0, NULL, 0);
+
+	return 0;
+}
+
+static int virtio_rng_probe(struct udevice *dev)
+{
+	struct virtio_rng_priv *priv = dev_get_priv(dev);
+	int ret;
+
+	ret = virtio_find_vqs(dev, 1, &priv->rng_vq);
+	if (ret < 0) {
+		debug("%s: virtio_find_vqs failed\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct dm_rng_ops virtio_rng_ops = {
+	.read	= virtio_rng_read,
+};
+
+U_BOOT_DRIVER(virtio_rng) = {
+	.name	= VIRTIO_RNG_DRV_NAME,
+	.id	= UCLASS_RNG,
+	.bind	= virtio_rng_bind,
+	.probe	= virtio_rng_probe,
+	.remove = virtio_reset,
+	.ops	= &virtio_rng_ops,
+	.priv_auto_alloc_size = sizeof(struct virtio_rng_priv),
+	.flags	= DM_FLAG_ACTIVE_DMA,
+};
diff --git a/include/virtio.h b/include/virtio.h
index 654fdf1..561dcc3 100644
--- a/include/virtio.h
+++ b/include/virtio.h
@@ -22,10 +22,12 @@ 
 
 #define VIRTIO_ID_NET		1 /* virtio net */
 #define VIRTIO_ID_BLOCK		2 /* virtio block */
-#define VIRTIO_ID_MAX_NUM	3
+#define VIRTIO_ID_RNG		4 /* virtio rng */
+#define VIRTIO_ID_MAX_NUM	5
 
 #define VIRTIO_NET_DRV_NAME	"virtio-net"
 #define VIRTIO_BLK_DRV_NAME	"virtio-blk"
+#define VIRTIO_RNG_DRV_NAME	"virtio-rng"
 
 /* Status byte for guest to report progress, and synchronize features */