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

On 12/28/19 7:28 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 V6:

• Handle review comments from Heinrich Schuchardt to handle the scenario of data buffer being passed to the virtio rng driver not being aligned on 4 bytes.

  drivers/virtio/Kconfig | 6 +++ drivers/virtio/Makefile | 1 + drivers/virtio/virtio-uclass.c | 1 + drivers/virtio/virtio_rng.c | 92 ++++++++++++++++++++++++++++++++++++++++++ include/virtio.h | 4 +- 5 files changed, 103 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..ffb6835 --- /dev/null +++ b/drivers/virtio/virtio_rng.c @@ -0,0 +1,92 @@ +// 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>

+#define BUFFER_SIZE 16UL

+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[BUFFER_SIZE] __aligned(4);
• unsigned char *ptr = data;
• struct virtio_sg sg;
• struct virtio_sg *sgs[1];
• struct virtio_rng_priv *priv = dev_get_priv(dev);
• if (!len)

• return 0;
  

• do {

If you use

while(nbytes) {

here you can go without the if above. This will save 4 bytes of code on ARMv8. In U-Boot we have tight code size constraints.

• sg.addr = buf;
  

• sg.length = nbytes;
  

We must avoid buffer overruns for len > 16:

sg.length = min(nbytes, BUFFER_SIZE);

The other patches of the series look fine to me.

Best regards

Heinrich

• 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))
  

• 	;
  

• memcpy(ptr, buf, rsize);
  

• nbytes -= rsize;
  

• ptr += 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, &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 */