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

[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 <sughosh.ganu at linaro.org>
> ---
> * 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 0000000..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 <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;
> +	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, &sgs, 0, 1);
		if (ret)
			return ret;

		virtqueue_kick(priv->rng_vq);

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

	return 0;
}

> +		sg.length = nbytes;
> +		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))
> +			;
> +		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, &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 */
>
>