Re: [PATCH v6 8/8] virtio: rng: Add a random number generator(rng) driver

2019-12-28 Thread Heinrich Schuchardt

On 12/28/19 7:09 AM, Heinrich Schuchardt wrote:

#define BUFFER_SIZE 16UL

static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
{
 int ret;
 unsigned int rsize = 0;
 unsigned char buf[BUFFER_SIZE];


This should be
unsigned char buf[BUFFER_SIZE] __aligned(4);


 unsigned char *ptr = data;
 struct virtio_sg sg;
 struct virtio_sg *sgs = sg;
 struct virtio_rng_priv *priv = dev_get_priv(dev);

 while (len) {
     sg.addr = buf;
     sg.length = min(BUFFER_SIZE, len);

     ret = virtqueue_add(priv->rng_vq, , 0, 1);
     if (ret)
     return ret;

     virtqueue_kick(priv->rng_vq);

     while (!virtqueue_get_buf(priv->rng_vq, ))
     ;
     memcpy(ptr, buf, rsize);
     len -= rsize;
     ptr += rsize;
 }

 return 0;
}




Re: [PATCH v6 8/8] virtio: rng: Add a random number generator(rng) driver

2019-12-27 Thread Heinrich Schuchardt

On 12/27/19 3:23 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 
---
* Handle review comments from Heinrich Schuchardt to read all the
   bytes requested in the individual drivers.

  drivers/virtio/Kconfig |  6 +++
  drivers/virtio/Makefile|  1 +
  drivers/virtio/virtio-uclass.c |  1 +
  drivers/virtio/virtio_rng.c| 87 ++
  include/virtio.h   |  4 +-
  5 files changed, 98 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 000..f8c10bf
--- /dev/null
+++ b/drivers/virtio/virtio_rng.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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;
+   size_t nbytes = len;
+   unsigned char *buf = data;
+   struct virtio_sg sg;
+   struct virtio_sg *sgs[1];
+   struct virtio_rng_priv *priv = dev_get_priv(dev);
+
+   if (!len)
+   return 0;
+
+   do {
+   sg.addr = buf;


As described in the response to an earlier version of the patch the
address passed as sg.addr has to be 4 byte aligned because
virtqueue_get_buf() internally copies u32.

With

qemu-system-aarch64 -M virt \
-device virtio-rng-pci,disable-legacy=on,max-bytes=15,period=1000

sg.addr will not even be 4 byte aligned if data is 4 byte aligned.

So if QEMU some day will correctly emulate alignment traps this function
may lead to a crash.

So I would prefer to use a local variable as buffer and then copy the
random bytes from the buffer to data. See suggestion below.

Best regards

Heinrich

#define BUFFER_SIZE 16UL

static int virtio_rng_read(struct udevice *dev, void *data, size_t len)
{
int ret;
unsigned int rsize = 0;
unsigned char buf[BUFFER_SIZE];
unsigned char *ptr = data;
struct virtio_sg sg;
struct virtio_sg *sgs = sg;
struct virtio_rng_priv *priv = dev_get_priv(dev);

while (len) {
sg.addr = buf;
sg.length = min(BUFFER_SIZE, len);

ret = virtqueue_add(priv->rng_vq, , 0, 1);
if (ret)
return ret;

virtqueue_kick(priv->rng_vq);

while (!virtqueue_get_buf(priv->rng_vq, ))
;
memcpy(ptr, buf, rsize);
len -= rsize;
ptr += rsize;
}

return 0;
}


+   sg.length = nbytes;
+   sgs[0] = 
+
+   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, ))
+   ;
+   nbytes -= rsize;
+   buf += rsize;
+   } while (nbytes != 0);
+
+   return 0;
+}
+
+static int virtio_rng_bind(struct udevice *dev)
+{
+   struct virtio_dev_priv *uc_priv = dev_get_uclass_priv(dev->parent);
+

[PATCH v6 8/8] virtio: rng: Add a random number generator(rng) driver

2019-12-27 Thread Sughosh Ganu
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 
---
* Handle review comments from Heinrich Schuchardt to read all the
  bytes requested in the individual drivers.

 drivers/virtio/Kconfig |  6 +++
 drivers/virtio/Makefile|  1 +
 drivers/virtio/virtio-uclass.c |  1 +
 drivers/virtio/virtio_rng.c| 87 ++
 include/virtio.h   |  4 +-
 5 files changed, 98 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 000..f8c10bf
--- /dev/null
+++ b/drivers/virtio/virtio_rng.c
@@ -0,0 +1,87 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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;
+   size_t nbytes = len;
+   unsigned char *buf = data;
+   struct virtio_sg sg;
+   struct virtio_sg *sgs[1];
+   struct virtio_rng_priv *priv = dev_get_priv(dev);
+
+   if (!len)
+   return 0;
+
+   do {
+   sg.addr = buf;
+   sg.length = nbytes;
+   sgs[0] = 
+
+   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, ))
+   ;
+   nbytes -= rsize;
+   buf += rsize;
+   } while (nbytes != 0);
+
+   return 0;
+}
+
+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, >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= _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_BLOCK2 /* 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 */
 
--