On 12/17/19 12:51 PM, Sughosh Ganu wrote:

Add a uclass for reading a random number seed from a random number generator device.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrice Chotard patrice.chotard@st.com

---

Changes since V3: Handle review comments from Patrick Delaunay.

drivers/Kconfig | 2 ++ drivers/Makefile | 1 + drivers/rng/Kconfig | 7 +++++++ drivers/rng/Makefile | 6 ++++++ drivers/rng/rng-uclass.c | 23 +++++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/rng.h | 32 ++++++++++++++++++++++++++++++++ 7 files changed, 72 insertions(+) create mode 100644 drivers/rng/Kconfig create mode 100644 drivers/rng/Makefile create mode 100644 drivers/rng/rng-uclass.c create mode 100644 include/rng.h

diff --git a/drivers/Kconfig b/drivers/Kconfig index 9d99ce0..e34a227 100644 --- a/drivers/Kconfig +++ b/drivers/Kconfig @@ -90,6 +90,8 @@ source "drivers/remoteproc/Kconfig"

source "drivers/reset/Kconfig"

+source "drivers/rng/Kconfig"

• source "drivers/rtc/Kconfig"

  source "drivers/scsi/Kconfig"

diff --git a/drivers/Makefile b/drivers/Makefile index e977f19..6c619b1 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -115,4 +115,5 @@ obj-$(CONFIG_W1_EEPROM) += w1-eeprom/

obj-$(CONFIG_MACH_PIC32) += ddr/microchip/ obj-$(CONFIG_DM_HWSPINLOCK) += hwspinlock/ +obj-$(CONFIG_DM_RNG) += rng/ endif diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig new file mode 100644 index 0000000..dd44cc0 --- /dev/null +++ b/drivers/rng/Kconfig @@ -0,0 +1,7 @@ +config DM_RNG

• bool "Driver support for Random Number Generator devices"
• depends on DM
• help

•  Enable driver model for random number generator(rng) devices.
  

•  This interface is used to initialise the rng device and to
  

•  read the random seed from the device.
  

diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile new file mode 100644 index 0000000..311705b --- /dev/null +++ b/drivers/rng/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# Copyright (c) 2019, Linaro Limited +#

+obj-$(CONFIG_DM_RNG) += rng-uclass.o diff --git a/drivers/rng/rng-uclass.c b/drivers/rng/rng-uclass.c new file mode 100644 index 0000000..b6af3b8 --- /dev/null +++ b/drivers/rng/rng-uclass.c @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2019, Linaro Limited
• */

+#include <common.h> +#include <dm.h> +#include <rng.h>

+int dm_rng_read(struct udevice *dev, void *buffer, size_t size) +{

• const struct dm_rng_ops *ops = device_get_ops(dev);
• if (!ops->read)

• return -ENOSYS;
  

• return ops->read(dev, buffer, size);

+}

+UCLASS_DRIVER(rng) = {

• .name = "rng",
• .id = UCLASS_RNG,

+}; diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h index 0c563d8..192202d 100644 --- a/include/dm/uclass-id.h +++ b/include/dm/uclass-id.h @@ -86,6 +86,7 @@ enum uclass_id { UCLASS_REGULATOR, /* Regulator device */ UCLASS_REMOTEPROC, /* Remote Processor device */ UCLASS_RESET, /* Reset controller device */

• UCLASS_RNG, /* Random Number Generator */ UCLASS_RTC, /* Real time clock device */ UCLASS_SCSI, /* SCSI device */ UCLASS_SERIAL, /* Serial UART */

diff --git a/include/rng.h b/include/rng.h new file mode 100644 index 0000000..80891fc --- /dev/null +++ b/include/rng.h @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (c) 2019, Linaro Limited
• */

+#if !defined _RNG_H_ +#define _RNG_H_

+#include <dm.h>

+/**

• • dm_rng_read() - read a random number seed from the rng device
• • @buffer: input buffer to put the read random seed into
• • @size: number of bytes of random seed read
• • @return 0 if OK, -ve on error

Please, stick to the Spinx syntax (Return:). See https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-do...

The Linux kernel code uses the return value to return the number of bytes retrieved.

Please, provide as description here of what will happen if the RNG device blocks.

• */

+int dm_rng_read(struct udevice *dev, void *buffer, size_t size);

We have different places where we retrieve the first RNG device and read from it. Shouldn't we provide a single function in the uclass that returns random bytes without a udevice parameter?

Linux uses an entropy pool where the hardware RNG is one of multiple sources of randomness and the entropy runs through hash functions (chacha20, sha1) before being consumed.

Shouldn't we call a hashing function in this uclass?

Shouldn't we further provide a function that can later be used to add additional sources of entropy? E.g. a seed file, the MAC address, timing of network accesses.

Please, take a look at Linux' drivers/char/random.c.

Best regards

Heinrich

+/* struct dm_rng_ops - Operations for the hwrng uclass */ +struct dm_rng_ops {

• /**

• * @read() - read a random number seed
  

• *
  

• * @data:	input buffer to read the random seed
  

• * @max:	total number of bytes to read
  

• * @return 0 if OK, -ve on error
  

• */
  

• int (*read)(struct udevice *dev, void *data, size_t max);

+};

+#endif /* _RNG_H_ */

On 12/17/19 12:52 PM, Sughosh Ganu wrote:

Add a sandbox driver for random number generation. Mostly aimed at providing a unit test for rng uclass.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrice Chotard patrice.chotard@st.com

---

Changes since V3: Handle review comments from Patrick Delaunay.

arch/sandbox/dts/test.dts | 4 ++++ drivers/rng/Kconfig | 7 +++++++ drivers/rng/Makefile | 1 + drivers/rng/sandbox_rng.c | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 drivers/rng/sandbox_rng.c

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index fdb08f2..2c85540 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -599,6 +599,10 @@ reset-names = "other", "test"; };

• rng@0 {

• compatible = "sandbox,sandbox-rng";
  

• };
• rproc_1: rproc@1 { compatible = "sandbox,test-processor"; remoteproc-name = "remoteproc-test-dev1";

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig index 5fc11db..3a1d3f0 100644 --- a/drivers/rng/Kconfig +++ b/drivers/rng/Kconfig @@ -6,6 +6,13 @@ config DM_RNG This interface is used to initialise the rng device and to read the random seed from the device.

+config RNG_SANDBOX

•   bool "Sandbox random number generator"
  

•   depends on SANDBOX && DM_RNG
  

•   help
  

•     Enable random number generator for sandbox. This is an
  

• emulation of a rng device.
  

• config RNG_STM32MP1 bool "Enable random number generator for STM32MP1" depends on ARCH_STM32MP && DM_RNG

diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile index 699beb3..3517005 100644 --- a/drivers/rng/Makefile +++ b/drivers/rng/Makefile @@ -4,4 +4,5 @@ #

obj-$(CONFIG_DM_RNG) += rng-uclass.o +obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o diff --git a/drivers/rng/sandbox_rng.c b/drivers/rng/sandbox_rng.c new file mode 100644 index 0000000..8262e0d --- /dev/null +++ b/drivers/rng/sandbox_rng.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2019, Linaro Limited
• */

+#include <common.h> +#include <dm.h> +#include <rng.h>

+static unsigned long random = 0xdeadbeef;

+static int sandbox_rng_read(struct udevice *dev, void *data, size_t len) +{

• if (len != sizeof(random))

• return -EINVAL;
  

This will drop on our feet once we use the RNG class for anything real.

Just imagine an EFI_RNG_PROTOCOL test failing on the sandbox because we cannot read 128 bit.

Can't we use a PRNG here and reinitialize it when len == ~0UL?

Functions rand() and srand() are good enough for the job.

Best regards

Heinrich

• random ^= ~0UL;
• *(unsigned long *)data = random;
• return 0;

+}

+static const struct dm_rng_ops sandbox_rng_ops = {

• .read = sandbox_rng_read,

+};

+static const struct udevice_id sandbox_rng_match[] = {

• {

• .compatible = "sandbox,sandbox-rng",
  

• },
• {},

+};

+U_BOOT_DRIVER(sandbox_rng) = {

• .name = "sandbox-rng",
• .id = UCLASS_RNG,
• .of_match = sandbox_rng_match,
• .ops = &sandbox_rng_ops,

+};

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

Resending with Cc Tom, Simon, Ilias

On 12/17/19 12:52 PM, Sughosh Ganu wrote:

...

Add a sandbox driver for random number generation. Mostly aimed at providing a unit test for rng uclass.

Signed-off-by: Sughosh Ganu sughosh.ganu@linaro.org Reviewed-by: Patrice Chotard patrice.chotard@st.com

---

Changes since V3: Handle review comments from Patrick Delaunay.

arch/sandbox/dts/test.dts | 4 ++++ drivers/rng/Kconfig | 7 +++++++ drivers/rng/Makefile | 1 + drivers/rng/sandbox_rng.c | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 51 insertions(+) create mode 100644 drivers/rng/sandbox_rng.c

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts index fdb08f2..2c85540 100644 --- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -599,6 +599,10 @@ reset-names = "other", "test"; };

• rng@0 {
  

•         compatible = "sandbox,sandbox-rng";
  

• };
  

• rproc_1: rproc@1 {
          compatible = "sandbox,test-processor";
          remoteproc-name = "remoteproc-test-dev1";
  

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig index 5fc11db..3a1d3f0 100644 --- a/drivers/rng/Kconfig +++ b/drivers/rng/Kconfig @@ -6,6 +6,13 @@ config DM_RNG This interface is used to initialise the rng device and to read the random seed from the device.

+config RNG_SANDBOX

•   bool "Sandbox random number generator"
  

•   depends on SANDBOX && DM_RNG
  

•   help
  

•     Enable random number generator for sandbox. This is an
  

•  emulation of a rng device.
  

• config RNG_STM32MP1 bool "Enable random number generator for STM32MP1" depends on ARCH_STM32MP && DM_RNG

diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile index 699beb3..3517005 100644 --- a/drivers/rng/Makefile +++ b/drivers/rng/Makefile @@ -4,4 +4,5 @@ #

obj-$(CONFIG_DM_RNG) += rng-uclass.o +obj-$(CONFIG_RNG_SANDBOX) += sandbox_rng.o obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o diff --git a/drivers/rng/sandbox_rng.c b/drivers/rng/sandbox_rng.c new file mode 100644 index 0000000..8262e0d --- /dev/null +++ b/drivers/rng/sandbox_rng.c @@ -0,0 +1,39 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/*

• • Copyright (c) 2019, Linaro Limited
• */

+#include <common.h> +#include <dm.h> +#include <rng.h>

+static unsigned long random = 0xdeadbeef;

+static int sandbox_rng_read(struct udevice *dev, void *data, size_t len) +{

• if (len != sizeof(random))
  

•         return -EINVAL;
  

This will drop on our feet once we use the RNG class for anything real.

Just imagine an EFI_RNG_PROTOCOL test failing on the sandbox because we cannot read 128 bit.

Can't we use a PRNG here and reinitialize it when len == ~0UL?

Functions rand() and srand() are good enough for the job.

Have sent a V5 using srand and rand functions for returning random bytes. Please take a look. Thanks.

-sughosh

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.

+}

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

Add a debug statement here, please, e.g.

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

Best regards

Heinrich

• 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 */

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