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

On Wed, 25 Dec 2019 at 07:00, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 12/24/19 10:39 PM, Heinrich Schuchardt wrote:

...

On 12/17/19 12:52 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

drivers/virtio/Kconfig | 6 ++++ drivers/virtio/Makefile | 1 + drivers/virtio/virtio-uclass.c | 1 + drivers/virtio/virtio_rng.c | 72 ++++++++++++++++++++++++++++++++++++++++++ include/virtio.h | 4 ++- 5 files changed, 83 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..19a0cc1 --- /dev/null +++ b/drivers/virtio/virtio_rng.c @@ -0,0 +1,72 @@ +// 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) +{

• struct virtio_sg sg = { data, len };
• struct virtio_sg *sgs[] = { &sg };
• struct virtio_rng_priv *priv = dev_get_priv(dev);
• unsigned int rsize;
• int ret;
• ret = virtqueue_add(priv->rng_vq, sgs, 0, 1);
• if (ret)

Please, add a debug statement for each error path, e.g.

     debug("%s: virtqueue_add() failed\n", __func__)

...

•    return ret;
  

• virtqueue_kick(priv->rng_vq);
• while (!virtqueue_get_buf(priv->rng_vq, &rsize))

•    ;
  

• return rsize;

The return value is inconsistent with sandbox_rng_read() and stm32_rng_read(). Both return 0 on success.

With "return 0;" the driver works fine.

I retested with -device virtio-rng-pci,disable-legacy=on,max-bytes=16,period=1000

In this case virtqueue_get_buf() only returns a maximum of 16 bytes. All other bytes of data remain unchanged. If the next call occurs within 1000 milli-seconds virtqueue_get_buf() blocks until the period is over.

So either you have to change the definition of the return parameter in patch 1/8 (and change the other drivers accordingly) or you have to add a loop to this driver that loops until len random bytes have been retrieved.

I would prefer to add a loop in the driver instead of adding a loop in every consumer.

So I tried putting a loop in the virtio rng driver, but that does not work. I was able to get it working with the loop in the consumer function. So I have changed the return value of the read function in all the drivers to return the number of bytes read, instead of returning 0. I think you will have to change your logic in the rng command patch that you had sent.

-sughosh