[PATCH v6 0/7] tpm: rng: Move TPM RNG functionality to driver model

[PATCH v6 0/7] tpm: rng: Move TPM RNG functionality to driver model

[Sughosh Ganu]

The TPM device provides the random number generator(RNG)
functionality, whereby sending a command to the TPM device results in
the TPM device responding with random bytes.

There was a discussion on the mailing list earlier[1], where it was
explained that platforms with a TPM device can install the
EFI_RNG_PROTOCOL for getting the random bytes instead of populating
the dtb with the kaslr-seed property. That would make it possible to
measure the dtb.

The TPM uclass driver adds the RNG child device as part of it's
post_probe function.

Some additional changes have also been made to facilitate the
use of the RNG devices, including extending the 'rng' command to take
the RNG device as one of the command-line parameters.

This series depends on a patch[2] from Simon Glass for moving the TPM
device version detection functions to the tpm_api.h header as static
inline functions.

These patches were under discussion earlier, specifically the patch to
add the RNG functionality under the TPM device as a child, either
through manual binding or through the device tree. Ilias had commented
on the discussion last[3]. The discussion can be resumed through this
version.

I have dropped certain patches which were changing some of the TPM API
functions to return an int instead of the current u32. These patches
have been dropped due to review comments from Simon[4]. This work can
be taken up separately, if desired.

[1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linaro.org/
[2] - https://lore.kernel.org/u-boot/20220301001125.1554442-2-sjg@chromium.org/T/#u
[3] - https://lists.denx.de/pipermail/u-boot/2022-April/481708.html
[4] - https://lists.denx.de/pipermail/u-boot/2022-March/477883.html

Simon Glass (1):
  tpm: Export the TPM-version functions

Sughosh Ganu (6):
  tpm: rng: Add driver model interface for TPM RNG device
  tpm: Add the RNG child device
  cmd: rng: Add support for selecting RNG device
  cmd: rng: Use a statically allocated array for random bytes
  doc: rng: Add documentation for the rng command
  test: rng: Add a UT testcase for the rng command

 cmd/Kconfig              |  1 +
 cmd/rng.c                | 42 +++++++++++------
 doc/usage/cmd/rng.rst    | 26 +++++++++++
 doc/usage/index.rst      |  1 +
 drivers/rng/Kconfig      | 11 +++++
 drivers/rng/Makefile     |  1 +
 drivers/rng/tpm_rng.c    | 23 ++++++++++
 drivers/tpm/tpm-uclass.c | 37 +++++++++++++--
 include/tpm_api.h        | 10 ++++
 lib/Kconfig              |  1 +
 lib/tpm-v1.c             | 13 +++---
 lib/tpm-v2.c             |  6 +--
 lib/tpm_api.c            | 98 ++++++++++++++++++----------------------
 test/dm/rng.c            | 29 ++++++++++++
 14 files changed, 217 insertions(+), 82 deletions(-)
 create mode 100644 doc/usage/cmd/rng.rst
 create mode 100644 drivers/rng/tpm_rng.c

-- 
2.25.1

[PATCH v6 1/7] tpm: Export the TPM-version functions

[Sughosh Ganu]

From: Simon Glass <sjg@chromium.org>

These functions should really be available outside the TPM code, so that
other callers can find out which version the TPM is. Rename them to have
a tpm_ prefix() and add them to the header file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes since V5: None

 include/tpm_api.h | 10 ++++++
 lib/tpm_api.c     | 92 +++++++++++++++++++++--------------------------
 2 files changed, 51 insertions(+), 51 deletions(-)

diff --git a/include/tpm_api.h b/include/tpm_api.h
index ef45b43a8f..11aa14eb79 100644
--- a/include/tpm_api.h
+++ b/include/tpm_api.h
@@ -319,4 +319,14 @@ u32 tpm_write_lock(struct udevice *dev, u32 index);
  */
 u32 tpm_resume(struct udevice *dev);
 
+static inline bool tpm_is_v1(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
+}
+
+static inline bool tpm_is_v2(struct udevice *dev)
+{
+	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
+}
+
 #endif /* __TPM_API_H */
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 4c662640a9..4ac4612c81 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -11,21 +11,11 @@
 #include <tpm-v2.h>
 #include <tpm_api.h>
 
-static bool is_tpm1(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
-}
-
-static bool is_tpm2(struct udevice *dev)
-{
-	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
-}
-
 u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 {
-	if (is_tpm1(dev)) {
+	if (tpm_is_v1(dev)) {
 		return tpm1_startup(dev, mode);
-	} else if (is_tpm2(dev)) {
+	} else if (tpm_is_v2(dev)) {
 		enum tpm2_startup_types type;
 
 		switch (mode) {
@@ -47,9 +37,9 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
 
 u32 tpm_resume(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_startup(dev, TPM_ST_STATE);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_startup(dev, TPM2_SU_STATE);
 	else
 		return -ENOSYS;
@@ -57,9 +47,9 @@ u32 tpm_resume(struct udevice *dev)
 
 u32 tpm_self_test_full(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_self_test_full(dev);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_self_test(dev, TPMI_YES);
 	else
 		return -ENOSYS;
@@ -67,9 +57,9 @@ u32 tpm_self_test_full(struct udevice *dev)
 
 u32 tpm_continue_self_test(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_continue_self_test(dev);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_self_test(dev, TPMI_NO);
 	else
 		return -ENOSYS;
@@ -86,7 +76,7 @@ u32 tpm_clear_and_reenable(struct udevice *dev)
 		return ret;
 	}
 
-	if (is_tpm1(dev)) {
+	if (tpm_is_v1(dev)) {
 		ret = tpm1_physical_enable(dev);
 		if (ret != TPM_SUCCESS) {
 			log_err("TPM: Can't set enabled state\n");
@@ -105,9 +95,9 @@ u32 tpm_clear_and_reenable(struct udevice *dev)
 
 u32 tpm_nv_enable_locking(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_nv_define_space(dev, TPM_NV_INDEX_LOCK, 0, 0);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return -ENOSYS;
 	else
 		return -ENOSYS;
@@ -115,9 +105,9 @@ u32 tpm_nv_enable_locking(struct udevice *dev)
 
 u32 tpm_nv_read_value(struct udevice *dev, u32 index, void *data, u32 count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_nv_read_value(dev, index, data, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_nv_read_value(dev, index, data, count);
 	else
 		return -ENOSYS;
@@ -126,9 +116,9 @@ u32 tpm_nv_read_value(struct udevice *dev, u32 index, void *data, u32 count)
 u32 tpm_nv_write_value(struct udevice *dev, u32 index, const void *data,
 		       u32 count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_nv_write_value(dev, index, data, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_nv_write_value(dev, index, data, count);
 	else
 		return -ENOSYS;
@@ -141,9 +131,9 @@ u32 tpm_set_global_lock(struct udevice *dev)
 
 u32 tpm_write_lock(struct udevice *dev, u32 index)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return -ENOSYS;
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_write_lock(dev, index);
 	else
 		return -ENOSYS;
@@ -152,9 +142,9 @@ u32 tpm_write_lock(struct udevice *dev, u32 index)
 u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
 		   void *out_digest)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_extend(dev, index, in_digest, out_digest);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, in_digest,
 				       TPM2_DIGEST_LEN);
 	else
@@ -163,9 +153,9 @@ u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
 
 u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_pcr_read(dev, index, data, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return -ENOSYS;
 	else
 		return -ENOSYS;
@@ -173,14 +163,14 @@ u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
 
 u32 tpm_tsc_physical_presence(struct udevice *dev, u16 presence)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_tsc_physical_presence(dev, presence);
 
 	/*
 	 * Nothing to do on TPM2 for this; use platform hierarchy availability
 	 * instead.
 	 */
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return 0;
 	else
 		return -ENOSYS;
@@ -188,11 +178,11 @@ u32 tpm_tsc_physical_presence(struct udevice *dev, u16 presence)
 
 u32 tpm_finalise_physical_presence(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_finalise_physical_presence(dev);
 
 	/* Nothing needs to be done with tpm2 */
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return 0;
 	else
 		return -ENOSYS;
@@ -200,9 +190,9 @@ u32 tpm_finalise_physical_presence(struct udevice *dev)
 
 u32 tpm_read_pubek(struct udevice *dev, void *data, size_t count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_read_pubek(dev, data, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return -ENOSYS; /* not implemented yet */
 	else
 		return -ENOSYS;
@@ -210,9 +200,9 @@ u32 tpm_read_pubek(struct udevice *dev, void *data, size_t count)
 
 u32 tpm_force_clear(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_force_clear(dev);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_clear(dev, TPM2_RH_PLATFORM, NULL, 0);
 	else
 		return -ENOSYS;
@@ -220,11 +210,11 @@ u32 tpm_force_clear(struct udevice *dev)
 
 u32 tpm_physical_enable(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_physical_enable(dev);
 
 	/* Nothing needs to be done with tpm2 */
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return 0;
 	else
 		return -ENOSYS;
@@ -232,11 +222,11 @@ u32 tpm_physical_enable(struct udevice *dev)
 
 u32 tpm_physical_disable(struct udevice *dev)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_physical_disable(dev);
 
 	/* Nothing needs to be done with tpm2 */
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return 0;
 	else
 		return -ENOSYS;
@@ -244,10 +234,10 @@ u32 tpm_physical_disable(struct udevice *dev)
 
 u32 tpm_physical_set_deactivated(struct udevice *dev, u8 state)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_physical_set_deactivated(dev, state);
 	/* Nothing needs to be done with tpm2 */
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return 0;
 	else
 		return -ENOSYS;
@@ -256,9 +246,9 @@ u32 tpm_physical_set_deactivated(struct udevice *dev, u8 state)
 u32 tpm_get_capability(struct udevice *dev, u32 cap_area, u32 sub_cap,
 		       void *cap, size_t count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_get_capability(dev, cap_area, sub_cap, cap, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return tpm2_get_capability(dev, cap_area, sub_cap, cap, count);
 	else
 		return -ENOSYS;
@@ -266,9 +256,9 @@ u32 tpm_get_capability(struct udevice *dev, u32 cap_area, u32 sub_cap,
 
 u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_get_permissions(dev, index, perm);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return -ENOSYS; /* not implemented yet */
 	else
 		return -ENOSYS;
@@ -276,9 +266,9 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
 
 u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
 {
-	if (is_tpm1(dev))
+	if (tpm_is_v1(dev))
 		return tpm1_get_random(dev, data, count);
-	else if (is_tpm2(dev))
+	else if (tpm_is_v2(dev))
 		return -ENOSYS; /* not implemented yet */
 	else
 		return -ENOSYS;
-- 
2.25.1

[PATCH v6 2/7] tpm: rng: Add driver model interface for TPM RNG device

[Sughosh Ganu]

The TPM device has a builtin random number generator(RNG)
functionality. Expose the RNG functions of the TPM device to the
driver model so that they can be used by the EFI_RNG_PROTOCOL if the
protocol is installed.

Also change the function arguments and return type of the random
number functions to comply with the driver model api.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5:
* Use the dev_get_parent() interface for getting the TPM device when
  calling the tpm_get_random() function

 drivers/rng/Kconfig   | 11 +++++++++++
 drivers/rng/Makefile  |  1 +
 drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
 lib/Kconfig           |  1 +
 lib/tpm-v1.c          | 13 +++++++------
 lib/tpm-v2.c          |  6 +++---
 lib/tpm_api.c         |  6 +++---
 7 files changed, 49 insertions(+), 12 deletions(-)
 create mode 100644 drivers/rng/tpm_rng.c

diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
index c10f7d345b..67c65311c7 100644
--- a/drivers/rng/Kconfig
+++ b/drivers/rng/Kconfig
@@ -58,4 +58,15 @@ config RNG_IPROC200
 	depends on DM_RNG
 	help
 	  Enable random number generator for RPI4.
+
+config TPM_RNG
+	bool "Enable random number generator on TPM device"
+	depends on TPM
+	default y
+	help
+	  The TPM device has an inbuilt random number generator
+	  functionality. Enable random number generator on TPM
+	  devices.
+
+
 endif
diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
index 435b3b965a..e4ca9c4149 100644
--- a/drivers/rng/Makefile
+++ b/drivers/rng/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_RNG_OPTEE) += optee_rng.o
 obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
 obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
 obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
+obj-$(CONFIG_TPM_RNG) += tpm_rng.o
diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
new file mode 100644
index 0000000000..1a5e9e2e4b
--- /dev/null
+++ b/drivers/rng/tpm_rng.c
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2022, Linaro Limited
+ */
+
+#include <dm.h>
+#include <rng.h>
+#include <tpm_api.h>
+
+static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
+{
+	return tpm_get_random(dev_get_parent(dev), data, count);
+}
+
+static const struct dm_rng_ops tpm_rng_ops = {
+	.read = rng_tpm_random_read,
+};
+
+U_BOOT_DRIVER(tpm_rng) = {
+	.name	= "tpm-rng",
+	.id	= UCLASS_RNG,
+	.ops	= &tpm_rng_ops,
+};
diff --git a/lib/Kconfig b/lib/Kconfig
index acc0ac081a..17efaa4c80 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -358,6 +358,7 @@ source lib/crypt/Kconfig
 config TPM
 	bool "Trusted Platform Module (TPM) Support"
 	depends on DM
+	imply DM_RNG
 	help
 	  This enables support for TPMs which can be used to provide security
 	  features for your board. The TPM can be connected via LPC or I2C
diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
index 22a769c587..f7091e5bc7 100644
--- a/lib/tpm-v1.c
+++ b/lib/tpm-v1.c
@@ -9,12 +9,13 @@
 #include <common.h>
 #include <dm.h>
 #include <log.h>
-#include <asm/unaligned.h>
-#include <u-boot/sha1.h>
 #include <tpm-common.h>
 #include <tpm-v1.h>
 #include "tpm-utils.h"
 
+#include <asm/unaligned.h>
+#include <u-boot/sha1.h>
+
 #ifdef CONFIG_TPM_AUTH_SESSIONS
 
 #ifndef CONFIG_SHA1
@@ -892,19 +893,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
 		if (pack_byte_string(buf, sizeof(buf), "sd",
 				     0, command, sizeof(command),
 				     length_offset, this_bytes))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		err = tpm_sendrecv_command(dev, buf, response,
 					   &response_length);
 		if (err)
 			return err;
 		if (unpack_byte_string(response, response_length, "d",
 				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (data_size > count)
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (unpack_byte_string(response, response_length, "s",
 				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 
 		count -= data_size;
 		out += data_size;
diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
index 1bf627853a..abca9a14b0 100644
--- a/lib/tpm-v2.c
+++ b/lib/tpm-v2.c
@@ -585,19 +585,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
 		if (pack_byte_string(buf, sizeof(buf), "sw",
 				     0, command_v2, sizeof(command_v2),
 				     sizeof(command_v2), this_bytes))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		err = tpm_sendrecv_command(dev, buf, response,
 					   &response_length);
 		if (err)
 			return err;
 		if (unpack_byte_string(response, response_length, "w",
 				       data_size_offset, &data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 		if (data_size > this_bytes)
 			return TPM_LIB_ERROR;
 		if (unpack_byte_string(response, response_length, "s",
 				       data_offset, out, data_size))
-			return TPM_LIB_ERROR;
+			return -EIO;
 
 		count -= data_size;
 		out += data_size;
diff --git a/lib/tpm_api.c b/lib/tpm_api.c
index 4ac4612c81..032f383ca0 100644
--- a/lib/tpm_api.c
+++ b/lib/tpm_api.c
@@ -269,7 +269,7 @@ u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
 	if (tpm_is_v1(dev))
 		return tpm1_get_random(dev, data, count);
 	else if (tpm_is_v2(dev))
-		return -ENOSYS; /* not implemented yet */
-	else
-		return -ENOSYS;
+		return tpm2_get_random(dev, data, count);
+
+	return -ENOSYS;
 }
-- 
2.25.1

[PATCH v6 3/7] tpm: Add the RNG child device

[Sughosh Ganu]

The TPM device comes with the random number generator(RNG)
functionality which is built into the TPM device. Add logic to add the
RNG child device in the TPM uclass post probe callback.

The RNG device can then be used to pass a set of random bytes to the
linux kernel, need for address space randomisation through the
EFI_RNG_PROTOCOL interface.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5:
* Check if the TPM RNG device has already been added, through a call
  to device_find_first_child_by_uclass()

 drivers/tpm/tpm-uclass.c | 37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
index f67fe1019b..e1f1ef01e1 100644
--- a/drivers/tpm/tpm-uclass.c
+++ b/drivers/tpm/tpm-uclass.c
@@ -11,10 +11,15 @@
 #include <log.h>
 #include <linux/delay.h>
 #include <linux/unaligned/be_byteshift.h>
+#include <tpm_api.h>
 #include <tpm-v1.h>
 #include <tpm-v2.h>
 #include "tpm_internal.h"
 
+#include <dm/lists.h>
+
+#define TPM_RNG_DRV_NAME	"tpm-rng"
+
 int tpm_open(struct udevice *dev)
 {
 	struct tpm_ops *ops = tpm_get_ops(dev);
@@ -136,12 +141,36 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
 	return 0;
 }
 
+static int tpm_uclass_post_probe(struct udevice *dev)
+{
+	int ret;
+	const char *drv = TPM_RNG_DRV_NAME;
+	struct udevice *child;
+
+	if (CONFIG_IS_ENABLED(TPM_RNG)) {
+		ret = device_find_first_child_by_uclass(dev, UCLASS_RNG,
+							&child);
+
+		if (ret != -ENODEV) {
+			log_debug("RNG child already added to the TPM device\n");
+			return ret;
+		}
+
+		ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
+		if (ret)
+			return log_msg_ret("bind", ret);
+	}
+
+	return 0;
+}
+
 UCLASS_DRIVER(tpm) = {
-	.id		= UCLASS_TPM,
-	.name		= "tpm",
-	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+	.id			= UCLASS_TPM,
+	.name			= "tpm",
+	.flags			= DM_UC_FLAG_SEQ_ALIAS,
 #if CONFIG_IS_ENABLED(OF_REAL)
-	.post_bind	= dm_scan_fdt_dev,
+	.post_bind		= dm_scan_fdt_dev,
 #endif
+	.post_probe		= tpm_uclass_post_probe,
 	.per_device_auto	= sizeof(struct tpm_chip_priv),
 };
-- 
2.25.1

[PATCH v6 4/7] cmd: rng: Add support for selecting RNG device

[Sughosh Ganu]

The 'rng' u-boot command is used for printing a select number of
random bytes on the console. Currently, the RNG device from which the
random bytes are read is fixed. However, a platform can have multiple
RNG devices, one example being qemu, which has a virtio RNG device and
the RNG pseudo device through the TPM chip.

Extend the 'rng' command so that the user can provide the RNG device
number from which the random bytes are to be read. This will be the
device index under the RNG uclass.

Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5: None

 cmd/rng.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/cmd/rng.c b/cmd/rng.c
index 1ad5a096c0..2ddf27545f 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -13,19 +13,34 @@
 
 static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	size_t n = 0x40;
+	size_t n;
 	struct udevice *dev;
 	void *buf;
+	int devnum;
 	int ret = CMD_RET_SUCCESS;
 
-	if (uclass_get_device(UCLASS_RNG, 0, &dev) || !dev) {
+	switch (argc) {
+	case 1:
+		devnum = 0;
+		n = 0x40;
+		break;
+	case 2:
+		devnum = hextoul(argv[1], NULL);
+		n = 0x40;
+		break;
+	case 3:
+		devnum = hextoul(argv[1], NULL);
+		n = hextoul(argv[2], NULL);
+		break;
+	default:
+		return CMD_RET_USAGE;
+	}
+
+	if (uclass_get_device_by_seq(UCLASS_RNG, devnum, &dev) || !dev) {
 		printf("No RNG device\n");
 		return CMD_RET_FAILURE;
 	}
 
-	if (argc >= 2)
-		n = hextoul(argv[1], NULL);
-
 	buf = malloc(n);
 	if (!buf) {
 		printf("Out of memory\n");
@@ -46,12 +61,12 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 
 #ifdef CONFIG_SYS_LONGHELP
 static char rng_help_text[] =
-	"[n]\n"
-	"  - print n random bytes\n";
+	"[dev [n]]\n"
+	"  - print n random bytes read from dev\n";
 #endif
 
 U_BOOT_CMD(
-	rng, 2, 0, do_rng,
+	rng, 3, 0, do_rng,
 	"print bytes from the hardware random number generator",
 	rng_help_text
 );
-- 
2.25.1

[PATCH v6 5/7] cmd: rng: Use a statically allocated array for random bytes

[Sughosh Ganu]

Use a statically allocated buffer on stack instead of using malloc for
reading the random bytes. Using a local array is faster than
allocating heap memory on every initiation of the command.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5: None

 cmd/rng.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/cmd/rng.c b/cmd/rng.c
index 2ddf27545f..81a23964b8 100644
--- a/cmd/rng.c
+++ b/cmd/rng.c
@@ -14,9 +14,9 @@
 static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	size_t n;
-	struct udevice *dev;
-	void *buf;
+	u8 buf[64];
 	int devnum;
+	struct udevice *dev;
 	int ret = CMD_RET_SUCCESS;
 
 	switch (argc) {
@@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		return CMD_RET_FAILURE;
 	}
 
-	buf = malloc(n);
-	if (!buf) {
-		printf("Out of memory\n");
-		return CMD_RET_FAILURE;
-	}
+	if (!n)
+		return 0;
+
+	n = min(n, sizeof(buf));
 
 	if (dm_rng_read(dev, buf, n)) {
 		printf("Reading RNG failed\n");
@@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
 	}
 
-	free(buf);
-
 	return ret;
 }
 
 #ifdef CONFIG_SYS_LONGHELP
 static char rng_help_text[] =
 	"[dev [n]]\n"
-	"  - print n random bytes read from dev\n";
+	"  - print n random bytes(max 64) read from dev\n";
 #endif
 
 U_BOOT_CMD(
-- 
2.25.1

[PATCH v6 6/7] doc: rng: Add documentation for the rng command

[Sughosh Ganu]

Add a usage document for the 'rng' u-boot command.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
Changes since V5: None

 doc/usage/cmd/rng.rst | 26 ++++++++++++++++++++++++++
 doc/usage/index.rst   |  1 +
 2 files changed, 27 insertions(+)
 create mode 100644 doc/usage/cmd/rng.rst

diff --git a/doc/usage/cmd/rng.rst b/doc/usage/cmd/rng.rst
new file mode 100644
index 0000000000..1a352da41a
--- /dev/null
+++ b/doc/usage/cmd/rng.rst
@@ -0,0 +1,26 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+rng command
+===========
+
+Synopsis
+--------
+
+::
+
+    rng [devnum [n]]
+
+Description
+-----------
+
+The *rng* command reads the random number generator(RNG) device and
+prints the random bytes read on the console. A maximum of 64 bytes can
+be read in one invocation of the command.
+
+devnum
+    The RNG device from which the random bytes are to be
+    read. Defaults to 0.
+
+n
+    Number of random bytes to be read and displayed on the
+    console. Default value is 0x40. Max value is 0x40.
diff --git a/doc/usage/index.rst b/doc/usage/index.rst
index 770418434a..ed3a78aa14 100644
--- a/doc/usage/index.rst
+++ b/doc/usage/index.rst
@@ -53,6 +53,7 @@ Shell commands
    cmd/pstore
    cmd/qfw
    cmd/reset
+   cmd/rng
    cmd/sbi
    cmd/sf
    cmd/scp03
-- 
2.25.1

[PATCH v6 7/7] test: rng: Add a UT testcase for the rng command

[Sughosh Ganu]

The 'rng' command dumps a number of random bytes on the console. Add a
set of tests for the 'rng' command. The test function performs basic
sanity testing of the command.

Since a unit test is being added for the command, enable it by default
in the sandbox platforms.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 cmd/Kconfig   |  1 +
 test/dm/rng.c | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 09193b61b9..eee5d44348 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1916,6 +1916,7 @@ config CMD_GETTIME
 config CMD_RNG
 	bool "rng command"
 	depends on DM_RNG
+	default y if SANDBOX
 	select HEXDUMP
 	help
 	  Print bytes from the hardware random number generator.
diff --git a/test/dm/rng.c b/test/dm/rng.c
index 5b34c93ed6..6d1f68848d 100644
--- a/test/dm/rng.c
+++ b/test/dm/rng.c
@@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
 	return 0;
 }
 DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
+
+/* Test the rng command */
+static int dm_test_rng_cmd(struct unit_test_state *uts)
+{
+	struct udevice *dev;
+
+	ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
+	ut_assertnonnull(dev);
+
+	ut_assertok(console_record_reset_enable());
+
+	run_command("rng", 0);
+	ut_assert_nextlinen("00000000:");
+	ut_assert_nextlinen("00000010:");
+	ut_assert_nextlinen("00000020:");
+	ut_assert_nextlinen("00000030:");
+	ut_assert_console_end();
+
+	run_command("rng 0 10", 0);
+	ut_assert_nextlinen("00000000:");
+	ut_assert_console_end();
+
+	run_command("rng 20", 0);
+	ut_assert_nextlinen("No RNG device");
+	ut_assert_console_end();
+
+	return 0;
+}
+DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
-- 
2.25.1

Re: [PATCH v6 2/7] tpm: rng: Add driver model interface for TPM RNG device

[Simon Glass]

Hi Sughosh,

On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device has a builtin random number generator(RNG)
> functionality. Expose the RNG functions of the TPM device to the
> driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> protocol is installed.
>
> Also change the function arguments and return type of the random
> number functions to comply with the driver model api.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5:
> * Use the dev_get_parent() interface for getting the TPM device when
>   calling the tpm_get_random() function
>
>  drivers/rng/Kconfig   | 11 +++++++++++
>  drivers/rng/Makefile  |  1 +
>  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
>  lib/Kconfig           |  1 +
>  lib/tpm-v1.c          | 13 +++++++------
>  lib/tpm-v2.c          |  6 +++---
>  lib/tpm_api.c         |  6 +++---
>  7 files changed, 49 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/rng/tpm_rng.c

The API change should not be needed. The code that calls the TPM
routines should adapt for that.

Regards,
Simon

Re: [PATCH v6 5/7] cmd: rng: Use a statically allocated array for random bytes

[Simon Glass]

On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> Use a statically allocated buffer on stack instead of using malloc for
> reading the random bytes. Using a local array is faster than
> allocating heap memory on every initiation of the command.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5: None
>
>  cmd/rng.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v6 0/7] tpm: rng: Move TPM RNG functionality to driver model

[Simon Glass]

Hi Sughosh,

On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
>
> The TPM device provides the random number generator(RNG)
> functionality, whereby sending a command to the TPM device results in
> the TPM device responding with random bytes.
>
> There was a discussion on the mailing list earlier[1], where it was
> explained that platforms with a TPM device can install the
> EFI_RNG_PROTOCOL for getting the random bytes instead of populating
> the dtb with the kaslr-seed property. That would make it possible to
> measure the dtb.
>
> The TPM uclass driver adds the RNG child device as part of it's
> post_probe function.
>
> Some additional changes have also been made to facilitate the
> use of the RNG devices, including extending the 'rng' command to take
> the RNG device as one of the command-line parameters.
>
> This series depends on a patch[2] from Simon Glass for moving the TPM
> device version detection functions to the tpm_api.h header as static
> inline functions.
>
> These patches were under discussion earlier, specifically the patch to
> add the RNG functionality under the TPM device as a child, either
> through manual binding or through the device tree. Ilias had commented
> on the discussion last[3]. The discussion can be resumed through this
> version.
>
> I have dropped certain patches which were changing some of the TPM API
> functions to return an int instead of the current u32. These patches
> have been dropped due to review comments from Simon[4]. This work can
> be taken up separately, if desired.
>
> [1] - https://lore.kernel.org/u-boot/20220103120738.47835-1-ilias.apalodimas@linaro.org/
> [2] - https://lore.kernel.org/u-boot/20220301001125.1554442-2-sjg@chromium.org/T/#u
> [3] - https://lists.denx.de/pipermail/u-boot/2022-April/481708.html
> [4] - https://lists.denx.de/pipermail/u-boot/2022-March/477883.html
>
> Simon Glass (1):
>   tpm: Export the TPM-version functions
>
> Sughosh Ganu (6):
>   tpm: rng: Add driver model interface for TPM RNG device
>   tpm: Add the RNG child device
>   cmd: rng: Add support for selecting RNG device
>   cmd: rng: Use a statically allocated array for random bytes
>   doc: rng: Add documentation for the rng command
>   test: rng: Add a UT testcase for the rng command
>
>  cmd/Kconfig              |  1 +
>  cmd/rng.c                | 42 +++++++++++------
>  doc/usage/cmd/rng.rst    | 26 +++++++++++
>  doc/usage/index.rst      |  1 +
>  drivers/rng/Kconfig      | 11 +++++
>  drivers/rng/Makefile     |  1 +
>  drivers/rng/tpm_rng.c    | 23 ++++++++++
>  drivers/tpm/tpm-uclass.c | 37 +++++++++++++--
>  include/tpm_api.h        | 10 ++++
>  lib/Kconfig              |  1 +
>  lib/tpm-v1.c             | 13 +++---
>  lib/tpm-v2.c             |  6 +--
>  lib/tpm_api.c            | 98 ++++++++++++++++++----------------------
>  test/dm/rng.c            | 29 ++++++++++++
>  14 files changed, 217 insertions(+), 82 deletions(-)
>  create mode 100644 doc/usage/cmd/rng.rst
>  create mode 100644 drivers/rng/tpm_rng.c
>
> --
> 2.25.1
>
>

This looks OK to me.

Regards,
Simon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi Sughosh,

On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The TPM device comes with the random number generator(RNG)
> functionality which is built into the TPM device. Add logic to add the
> RNG child device in the TPM uclass post probe callback.
>
> The RNG device can then be used to pass a set of random bytes to the
> linux kernel, need for address space randomisation through the
> EFI_RNG_PROTOCOL interface.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5:
> * Check if the TPM RNG device has already been added, through a call
>   to device_find_first_child_by_uclass()
>
>  drivers/tpm/tpm-uclass.c | 37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> index f67fe1019b..e1f1ef01e1 100644
> --- a/drivers/tpm/tpm-uclass.c
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -11,10 +11,15 @@
>  #include <log.h>
>  #include <linux/delay.h>
>  #include <linux/unaligned/be_byteshift.h>
> +#include <tpm_api.h>
>  #include <tpm-v1.h>
>  #include <tpm-v2.h>
>  #include "tpm_internal.h"
>
> +#include <dm/lists.h>
> +
> +#define TPM_RNG_DRV_NAME       "tpm-rng"
> +
>  int tpm_open(struct udevice *dev)
>  {
>         struct tpm_ops *ops = tpm_get_ops(dev);
> @@ -136,12 +141,36 @@ int tpm_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
>         return 0;
>  }
>
> +static int tpm_uclass_post_probe(struct udevice *dev)
> +{
> +       int ret;
> +       const char *drv = TPM_RNG_DRV_NAME;
> +       struct udevice *child;
> +
> +       if (CONFIG_IS_ENABLED(TPM_RNG)) {
> +               ret = device_find_first_child_by_uclass(dev, UCLASS_RNG,
> +                                                       &child);
> +
> +               if (ret != -ENODEV) {

This may return -EIO (for example) in which case your log message is incorrect.


> +                       log_debug("RNG child already added to the TPM device\n");
> +                       return ret;
> +               }
> +
> +               ret = device_bind_driver(dev, drv, "tpm-rng0", &child);
> +               if (ret)
> +                       return log_msg_ret("bind", ret);
> +       }
> +
> +       return 0;
> +}
> +
>  UCLASS_DRIVER(tpm) = {
> -       .id             = UCLASS_TPM,
> -       .name           = "tpm",
> -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> +       .id                     = UCLASS_TPM,
> +       .name                   = "tpm",
> +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
>  #if CONFIG_IS_ENABLED(OF_REAL)
> -       .post_bind      = dm_scan_fdt_dev,
> +       .post_bind              = dm_scan_fdt_dev,
>  #endif
> +       .post_probe             = tpm_uclass_post_probe,
>         .per_device_auto        = sizeof(struct tpm_chip_priv),
>  };
> --
> 2.25.1
>

The driver needs a compatible string so it can be in the device tree.

Regards,
Simon

Re: [PATCH v6 2/7] tpm: rng: Add driver model interface for TPM RNG device

[Sughosh Ganu]

hi Simon,

On Tue, 5 Jul 2022 at 15:17, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Sughosh,
>
> On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> >
> > The TPM device has a builtin random number generator(RNG)
> > functionality. Expose the RNG functions of the TPM device to the
> > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > protocol is installed.
> >
> > Also change the function arguments and return type of the random
> > number functions to comply with the driver model api.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> > Changes since V5:
> > * Use the dev_get_parent() interface for getting the TPM device when
> >   calling the tpm_get_random() function
> >
> >  drivers/rng/Kconfig   | 11 +++++++++++
> >  drivers/rng/Makefile  |  1 +
> >  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> >  lib/Kconfig           |  1 +
> >  lib/tpm-v1.c          | 13 +++++++------
> >  lib/tpm-v2.c          |  6 +++---
> >  lib/tpm_api.c         |  6 +++---
> >  7 files changed, 49 insertions(+), 12 deletions(-)
> >  create mode 100644 drivers/rng/tpm_rng.c
>
> The API change should not be needed. The code that calls the TPM
> routines should adapt for that.

Are you referring to the changes made in tpm[1,2]_get_random
functions? If so, those changes in the return values have been made
based on review comments from Heinrich[1]. If you are referring to
some other changes, please let me know. Thanks.

-sughosh

[1] - https://lists.denx.de/pipermail/u-boot/2022-February/476085.html

>
> Regards,
> Simon

Re: [PATCH v6 1/7] tpm: Export the TPM-version functions

[Ilias Apalodimas]

On Mon, Jul 04, 2022 at 07:04:38PM +0530, Sughosh Ganu wrote:
> From: Simon Glass <sjg@chromium.org>
> 
> These functions should really be available outside the TPM code, so that
> other callers can find out which version the TPM is. Rename them to have
> a tpm_ prefix() and add them to the header file.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes since V5: None
> 
>  include/tpm_api.h | 10 ++++++
>  lib/tpm_api.c     | 92 +++++++++++++++++++++--------------------------
>  2 files changed, 51 insertions(+), 51 deletions(-)
> 
> diff --git a/include/tpm_api.h b/include/tpm_api.h
> index ef45b43a8f..11aa14eb79 100644
> --- a/include/tpm_api.h
> +++ b/include/tpm_api.h
> @@ -319,4 +319,14 @@ u32 tpm_write_lock(struct udevice *dev, u32 index);
>   */
>  u32 tpm_resume(struct udevice *dev);
>  
> +static inline bool tpm_is_v1(struct udevice *dev)
> +{
> +	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> +}
> +
> +static inline bool tpm_is_v2(struct udevice *dev)
> +{
> +	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> +}
> +
>  #endif /* __TPM_API_H */
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 4c662640a9..4ac4612c81 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -11,21 +11,11 @@
>  #include <tpm-v2.h>
>  #include <tpm_api.h>
>  
> -static bool is_tpm1(struct udevice *dev)
> -{
> -	return IS_ENABLED(CONFIG_TPM_V1) && tpm_get_version(dev) == TPM_V1;
> -}
> -
> -static bool is_tpm2(struct udevice *dev)
> -{
> -	return IS_ENABLED(CONFIG_TPM_V2) && tpm_get_version(dev) == TPM_V2;
> -}
> -
>  u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>  {
> -	if (is_tpm1(dev)) {
> +	if (tpm_is_v1(dev)) {
>  		return tpm1_startup(dev, mode);
> -	} else if (is_tpm2(dev)) {
> +	} else if (tpm_is_v2(dev)) {
>  		enum tpm2_startup_types type;
>  
>  		switch (mode) {
> @@ -47,9 +37,9 @@ u32 tpm_startup(struct udevice *dev, enum tpm_startup_type mode)
>  
>  u32 tpm_resume(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_startup(dev, TPM_ST_STATE);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_startup(dev, TPM2_SU_STATE);
>  	else
>  		return -ENOSYS;
> @@ -57,9 +47,9 @@ u32 tpm_resume(struct udevice *dev)
>  
>  u32 tpm_self_test_full(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_self_test_full(dev);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_self_test(dev, TPMI_YES);
>  	else
>  		return -ENOSYS;
> @@ -67,9 +57,9 @@ u32 tpm_self_test_full(struct udevice *dev)
>  
>  u32 tpm_continue_self_test(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_continue_self_test(dev);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_self_test(dev, TPMI_NO);
>  	else
>  		return -ENOSYS;
> @@ -86,7 +76,7 @@ u32 tpm_clear_and_reenable(struct udevice *dev)
>  		return ret;
>  	}
>  
> -	if (is_tpm1(dev)) {
> +	if (tpm_is_v1(dev)) {
>  		ret = tpm1_physical_enable(dev);
>  		if (ret != TPM_SUCCESS) {
>  			log_err("TPM: Can't set enabled state\n");
> @@ -105,9 +95,9 @@ u32 tpm_clear_and_reenable(struct udevice *dev)
>  
>  u32 tpm_nv_enable_locking(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_nv_define_space(dev, TPM_NV_INDEX_LOCK, 0, 0);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return -ENOSYS;
>  	else
>  		return -ENOSYS;
> @@ -115,9 +105,9 @@ u32 tpm_nv_enable_locking(struct udevice *dev)
>  
>  u32 tpm_nv_read_value(struct udevice *dev, u32 index, void *data, u32 count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_nv_read_value(dev, index, data, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_nv_read_value(dev, index, data, count);
>  	else
>  		return -ENOSYS;
> @@ -126,9 +116,9 @@ u32 tpm_nv_read_value(struct udevice *dev, u32 index, void *data, u32 count)
>  u32 tpm_nv_write_value(struct udevice *dev, u32 index, const void *data,
>  		       u32 count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_nv_write_value(dev, index, data, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_nv_write_value(dev, index, data, count);
>  	else
>  		return -ENOSYS;
> @@ -141,9 +131,9 @@ u32 tpm_set_global_lock(struct udevice *dev)
>  
>  u32 tpm_write_lock(struct udevice *dev, u32 index)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return -ENOSYS;
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_write_lock(dev, index);
>  	else
>  		return -ENOSYS;
> @@ -152,9 +142,9 @@ u32 tpm_write_lock(struct udevice *dev, u32 index)
>  u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
>  		   void *out_digest)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_extend(dev, index, in_digest, out_digest);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_pcr_extend(dev, index, TPM2_ALG_SHA256, in_digest,
>  				       TPM2_DIGEST_LEN);
>  	else
> @@ -163,9 +153,9 @@ u32 tpm_pcr_extend(struct udevice *dev, u32 index, const void *in_digest,
>  
>  u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_pcr_read(dev, index, data, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return -ENOSYS;
>  	else
>  		return -ENOSYS;
> @@ -173,14 +163,14 @@ u32 tpm_pcr_read(struct udevice *dev, u32 index, void *data, size_t count)
>  
>  u32 tpm_tsc_physical_presence(struct udevice *dev, u16 presence)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_tsc_physical_presence(dev, presence);
>  
>  	/*
>  	 * Nothing to do on TPM2 for this; use platform hierarchy availability
>  	 * instead.
>  	 */
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return 0;
>  	else
>  		return -ENOSYS;
> @@ -188,11 +178,11 @@ u32 tpm_tsc_physical_presence(struct udevice *dev, u16 presence)
>  
>  u32 tpm_finalise_physical_presence(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_finalise_physical_presence(dev);
>  
>  	/* Nothing needs to be done with tpm2 */
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return 0;
>  	else
>  		return -ENOSYS;
> @@ -200,9 +190,9 @@ u32 tpm_finalise_physical_presence(struct udevice *dev)
>  
>  u32 tpm_read_pubek(struct udevice *dev, void *data, size_t count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_read_pubek(dev, data, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return -ENOSYS; /* not implemented yet */
>  	else
>  		return -ENOSYS;
> @@ -210,9 +200,9 @@ u32 tpm_read_pubek(struct udevice *dev, void *data, size_t count)
>  
>  u32 tpm_force_clear(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_force_clear(dev);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_clear(dev, TPM2_RH_PLATFORM, NULL, 0);
>  	else
>  		return -ENOSYS;
> @@ -220,11 +210,11 @@ u32 tpm_force_clear(struct udevice *dev)
>  
>  u32 tpm_physical_enable(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_physical_enable(dev);
>  
>  	/* Nothing needs to be done with tpm2 */
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return 0;
>  	else
>  		return -ENOSYS;
> @@ -232,11 +222,11 @@ u32 tpm_physical_enable(struct udevice *dev)
>  
>  u32 tpm_physical_disable(struct udevice *dev)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_physical_disable(dev);
>  
>  	/* Nothing needs to be done with tpm2 */
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return 0;
>  	else
>  		return -ENOSYS;
> @@ -244,10 +234,10 @@ u32 tpm_physical_disable(struct udevice *dev)
>  
>  u32 tpm_physical_set_deactivated(struct udevice *dev, u8 state)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_physical_set_deactivated(dev, state);
>  	/* Nothing needs to be done with tpm2 */
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return 0;
>  	else
>  		return -ENOSYS;
> @@ -256,9 +246,9 @@ u32 tpm_physical_set_deactivated(struct udevice *dev, u8 state)
>  u32 tpm_get_capability(struct udevice *dev, u32 cap_area, u32 sub_cap,
>  		       void *cap, size_t count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_get_capability(dev, cap_area, sub_cap, cap, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return tpm2_get_capability(dev, cap_area, sub_cap, cap, count);
>  	else
>  		return -ENOSYS;
> @@ -266,9 +256,9 @@ u32 tpm_get_capability(struct udevice *dev, u32 cap_area, u32 sub_cap,
>  
>  u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_get_permissions(dev, index, perm);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return -ENOSYS; /* not implemented yet */
>  	else
>  		return -ENOSYS;
> @@ -276,9 +266,9 @@ u32 tpm_get_permissions(struct udevice *dev, u32 index, u32 *perm)
>  
>  u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
>  {
> -	if (is_tpm1(dev))
> +	if (tpm_is_v1(dev))
>  		return tpm1_get_random(dev, data, count);
> -	else if (is_tpm2(dev))
> +	else if (tpm_is_v2(dev))
>  		return -ENOSYS; /* not implemented yet */
>  	else
>  		return -ENOSYS;
> -- 
> 2.25.1
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v6 2/7] tpm: rng: Add driver model interface for TPM RNG device

[Ilias Apalodimas]

On Mon, Jul 04, 2022 at 07:04:39PM +0530, Sughosh Ganu wrote:
> The TPM device has a builtin random number generator(RNG)
> functionality. Expose the RNG functions of the TPM device to the
> driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> protocol is installed.
> 
> Also change the function arguments and return type of the random
> number functions to comply with the driver model api.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5:
> * Use the dev_get_parent() interface for getting the TPM device when
>   calling the tpm_get_random() function
> 
>  drivers/rng/Kconfig   | 11 +++++++++++
>  drivers/rng/Makefile  |  1 +
>  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
>  lib/Kconfig           |  1 +
>  lib/tpm-v1.c          | 13 +++++++------
>  lib/tpm-v2.c          |  6 +++---
>  lib/tpm_api.c         |  6 +++---
>  7 files changed, 49 insertions(+), 12 deletions(-)
>  create mode 100644 drivers/rng/tpm_rng.c
> 
> diff --git a/drivers/rng/Kconfig b/drivers/rng/Kconfig
> index c10f7d345b..67c65311c7 100644
> --- a/drivers/rng/Kconfig
> +++ b/drivers/rng/Kconfig
> @@ -58,4 +58,15 @@ config RNG_IPROC200
>  	depends on DM_RNG
>  	help
>  	  Enable random number generator for RPI4.
> +
> +config TPM_RNG
> +	bool "Enable random number generator on TPM device"
> +	depends on TPM
> +	default y
> +	help
> +	  The TPM device has an inbuilt random number generator
> +	  functionality. Enable random number generator on TPM
> +	  devices.

Maybe we discussed this on a previous version, but why do we want to have
this as a config option?  Code size?  A TPM will always be able to generate
a random number.  Couldn't we compile this based on an existing TPM Kconfig
option?

> +
> +
>  endif
> diff --git a/drivers/rng/Makefile b/drivers/rng/Makefile
> index 435b3b965a..e4ca9c4149 100644
> --- a/drivers/rng/Makefile
> +++ b/drivers/rng/Makefile
> @@ -11,3 +11,4 @@ obj-$(CONFIG_RNG_OPTEE) += optee_rng.o
>  obj-$(CONFIG_RNG_STM32MP1) += stm32mp1_rng.o
>  obj-$(CONFIG_RNG_ROCKCHIP) += rockchip_rng.o
>  obj-$(CONFIG_RNG_IPROC200) += iproc_rng200.o
> +obj-$(CONFIG_TPM_RNG) += tpm_rng.o
> diff --git a/drivers/rng/tpm_rng.c b/drivers/rng/tpm_rng.c
> new file mode 100644
> index 0000000000..1a5e9e2e4b
> --- /dev/null
> +++ b/drivers/rng/tpm_rng.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2022, Linaro Limited
> + */
> +
> +#include <dm.h>
> +#include <rng.h>
> +#include <tpm_api.h>
> +
> +static int rng_tpm_random_read(struct udevice *dev, void *data, size_t count)
> +{
> +	return tpm_get_random(dev_get_parent(dev), data, count);
> +}
> +
> +static const struct dm_rng_ops tpm_rng_ops = {
> +	.read = rng_tpm_random_read,
> +};
> +
> +U_BOOT_DRIVER(tpm_rng) = {
> +	.name	= "tpm-rng",
> +	.id	= UCLASS_RNG,
> +	.ops	= &tpm_rng_ops,
> +};
> diff --git a/lib/Kconfig b/lib/Kconfig
> index acc0ac081a..17efaa4c80 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -358,6 +358,7 @@ source lib/crypt/Kconfig
>  config TPM
>  	bool "Trusted Platform Module (TPM) Support"
>  	depends on DM
> +	imply DM_RNG
>  	help
>  	  This enables support for TPMs which can be used to provide security
>  	  features for your board. The TPM can be connected via LPC or I2C
> diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c
> index 22a769c587..f7091e5bc7 100644
> --- a/lib/tpm-v1.c
> +++ b/lib/tpm-v1.c
> @@ -9,12 +9,13 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <log.h>
> -#include <asm/unaligned.h>
> -#include <u-boot/sha1.h>
>  #include <tpm-common.h>
>  #include <tpm-v1.h>
>  #include "tpm-utils.h"
>  
> +#include <asm/unaligned.h>
> +#include <u-boot/sha1.h>
> +
>  #ifdef CONFIG_TPM_AUTH_SESSIONS
>  
>  #ifndef CONFIG_SHA1
> @@ -892,19 +893,19 @@ u32 tpm1_get_random(struct udevice *dev, void *data, u32 count)
>  		if (pack_byte_string(buf, sizeof(buf), "sd",
>  				     0, command, sizeof(command),
>  				     length_offset, this_bytes))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  		err = tpm_sendrecv_command(dev, buf, response,
>  					   &response_length);
>  		if (err)
>  			return err;
>  		if (unpack_byte_string(response, response_length, "d",
>  				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  		if (data_size > count)
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  		if (unpack_byte_string(response, response_length, "s",
>  				       data_offset, out, data_size))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  
>  		count -= data_size;
>  		out += data_size;
> diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c
> index 1bf627853a..abca9a14b0 100644
> --- a/lib/tpm-v2.c
> +++ b/lib/tpm-v2.c
> @@ -585,19 +585,19 @@ u32 tpm2_get_random(struct udevice *dev, void *data, u32 count)
>  		if (pack_byte_string(buf, sizeof(buf), "sw",
>  				     0, command_v2, sizeof(command_v2),
>  				     sizeof(command_v2), this_bytes))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  		err = tpm_sendrecv_command(dev, buf, response,
>  					   &response_length);
>  		if (err)
>  			return err;
>  		if (unpack_byte_string(response, response_length, "w",
>  				       data_size_offset, &data_size))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  		if (data_size > this_bytes)
>  			return TPM_LIB_ERROR;
>  		if (unpack_byte_string(response, response_length, "s",
>  				       data_offset, out, data_size))
> -			return TPM_LIB_ERROR;
> +			return -EIO;
>  
>  		count -= data_size;
>  		out += data_size;
> diff --git a/lib/tpm_api.c b/lib/tpm_api.c
> index 4ac4612c81..032f383ca0 100644
> --- a/lib/tpm_api.c
> +++ b/lib/tpm_api.c
> @@ -269,7 +269,7 @@ u32 tpm_get_random(struct udevice *dev, void *data, u32 count)
>  	if (tpm_is_v1(dev))
>  		return tpm1_get_random(dev, data, count);
>  	else if (tpm_is_v2(dev))
> -		return -ENOSYS; /* not implemented yet */
> -	else
> -		return -ENOSYS;
> +		return tpm2_get_random(dev, data, count);
> +
> +	return -ENOSYS;
>  }
> -- 
> 2.25.1
> 

Thanks
/Ilias

Re: [PATCH v6 5/7] cmd: rng: Use a statically allocated array for random bytes

[Ilias Apalodimas]

On Mon, Jul 04, 2022 at 07:04:42PM +0530, Sughosh Ganu wrote:
> Use a statically allocated buffer on stack instead of using malloc for
> reading the random bytes. Using a local array is faster than
> allocating heap memory on every initiation of the command.
> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> Changes since V5: None
> 
>  cmd/rng.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/cmd/rng.c b/cmd/rng.c
> index 2ddf27545f..81a23964b8 100644
> --- a/cmd/rng.c
> +++ b/cmd/rng.c
> @@ -14,9 +14,9 @@
>  static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>  	size_t n;
> -	struct udevice *dev;
> -	void *buf;
> +	u8 buf[64];
>  	int devnum;
> +	struct udevice *dev;
>  	int ret = CMD_RET_SUCCESS;
>  
>  	switch (argc) {
> @@ -41,11 +41,10 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  		return CMD_RET_FAILURE;
>  	}
>  
> -	buf = malloc(n);
> -	if (!buf) {
> -		printf("Out of memory\n");
> -		return CMD_RET_FAILURE;
> -	}
> +	if (!n)
> +		return 0;
> +
> +	n = min(n, sizeof(buf));
>  
>  	if (dm_rng_read(dev, buf, n)) {
>  		printf("Reading RNG failed\n");
> @@ -54,15 +53,13 @@ static int do_rng(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  		print_hex_dump_bytes("", DUMP_PREFIX_OFFSET, buf, n);
>  	}
>  
> -	free(buf);
> -
>  	return ret;
>  }
>  
>  #ifdef CONFIG_SYS_LONGHELP
>  static char rng_help_text[] =
>  	"[dev [n]]\n"
> -	"  - print n random bytes read from dev\n";
> +	"  - print n random bytes(max 64) read from dev\n";
>  #endif
>  
>  U_BOOT_CMD(
> -- 
> 2.25.1
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v6 7/7] test: rng: Add a UT testcase for the rng command

[Ilias Apalodimas]

On Mon, Jul 04, 2022 at 07:04:44PM +0530, Sughosh Ganu wrote:
> The 'rng' command dumps a number of random bytes on the console. Add a
> set of tests for the 'rng' command. The test function performs basic
> sanity testing of the command.
> 
> Since a unit test is being added for the command, enable it by default
> in the sandbox platforms.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  cmd/Kconfig   |  1 +
>  test/dm/rng.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 30 insertions(+)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 09193b61b9..eee5d44348 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1916,6 +1916,7 @@ config CMD_GETTIME
>  config CMD_RNG
>  	bool "rng command"
>  	depends on DM_RNG
> +	default y if SANDBOX
>  	select HEXDUMP
>  	help
>  	  Print bytes from the hardware random number generator.
> diff --git a/test/dm/rng.c b/test/dm/rng.c
> index 5b34c93ed6..6d1f68848d 100644
> --- a/test/dm/rng.c
> +++ b/test/dm/rng.c
> @@ -25,3 +25,32 @@ static int dm_test_rng_read(struct unit_test_state *uts)
>  	return 0;
>  }
>  DM_TEST(dm_test_rng_read, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT);
> +
> +/* Test the rng command */
> +static int dm_test_rng_cmd(struct unit_test_state *uts)
> +{
> +	struct udevice *dev;
> +
> +	ut_assertok(uclass_get_device(UCLASS_RNG, 0, &dev));
> +	ut_assertnonnull(dev);
> +
> +	ut_assertok(console_record_reset_enable());
> +
> +	run_command("rng", 0);
> +	ut_assert_nextlinen("00000000:");
> +	ut_assert_nextlinen("00000010:");
> +	ut_assert_nextlinen("00000020:");
> +	ut_assert_nextlinen("00000030:");
> +	ut_assert_console_end();
> +
> +	run_command("rng 0 10", 0);
> +	ut_assert_nextlinen("00000000:");
> +	ut_assert_console_end();
> +
> +	run_command("rng 20", 0);
> +	ut_assert_nextlinen("No RNG device");
> +	ut_assert_console_end();
> +
> +	return 0;
> +}
> +DM_TEST(dm_test_rng_cmd, UT_TESTF_SCAN_PDATA | UT_TESTF_SCAN_FDT | UT_TESTF_CONSOLE_REC);
> -- 
> 2.25.1
> 

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Ilias Apalodimas]

Hi Simon,

[...]

> > +
> >  UCLASS_DRIVER(tpm) = {
> > -       .id             = UCLASS_TPM,
> > -       .name           = "tpm",
> > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > +       .id                     = UCLASS_TPM,
> > +       .name                   = "tpm",
> > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> >  #if CONFIG_IS_ENABLED(OF_REAL)
> > -       .post_bind      = dm_scan_fdt_dev,
> > +       .post_bind              = dm_scan_fdt_dev,
> >  #endif
> > +       .post_probe             = tpm_uclass_post_probe,
> >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> >  };
> > --
> > 2.25.1
> >
>
> The driver needs a compatible string so it can be in the device tree.

Why?  I've tried to hint this on the previous iteration of the patch.
The RNG here is not a *device*.  The TPM is the device and you are
guaranteed to have an RNG.  The way to get a random number is send a
special command to the TPM. So all that we should do here is leverage
the fact that the TPM is already in the device tree.

And fwiw we should stick to try to stick to what the DT spec defines
as much as possible.  I personally don't see this as a special usecase
were deviating from the spec is justified.

Regards
/Ilias
>
> Regards,
> Simon

Re: [PATCH v6 2/7] tpm: rng: Add driver model interface for TPM RNG device

[Simon Glass]

Hi Sughosh,

On Tue, 5 Jul 2022 at 11:23, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> hi Simon,
>
> On Tue, 5 Jul 2022 at 15:17, Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Mon, 4 Jul 2022 at 07:35, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
> > >
> > > The TPM device has a builtin random number generator(RNG)
> > > functionality. Expose the RNG functions of the TPM device to the
> > > driver model so that they can be used by the EFI_RNG_PROTOCOL if the
> > > protocol is installed.
> > >
> > > Also change the function arguments and return type of the random
> > > number functions to comply with the driver model api.
> > >
> > > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > > ---
> > > Changes since V5:
> > > * Use the dev_get_parent() interface for getting the TPM device when
> > >   calling the tpm_get_random() function
> > >
> > >  drivers/rng/Kconfig   | 11 +++++++++++
> > >  drivers/rng/Makefile  |  1 +
> > >  drivers/rng/tpm_rng.c | 23 +++++++++++++++++++++++
> > >  lib/Kconfig           |  1 +
> > >  lib/tpm-v1.c          | 13 +++++++------
> > >  lib/tpm-v2.c          |  6 +++---
> > >  lib/tpm_api.c         |  6 +++---
> > >  7 files changed, 49 insertions(+), 12 deletions(-)
> > >  create mode 100644 drivers/rng/tpm_rng.c
> >
> > The API change should not be needed. The code that calls the TPM
> > routines should adapt for that.
>
> Are you referring to the changes made in tpm[1,2]_get_random
> functions? If so, those changes in the return values have been made
> based on review comments from Heinrich[1]. If you are referring to
> some other changes, please let me know. Thanks.

I think Heinrich might have the wrong end of the stick.

The TPM API returns a positive error code. We should not change it.
The tpm_rng module can handle any conversion.

Regards,
Simon



>
> -sughosh
>
> [1] - https://lists.denx.de/pipermail/u-boot/2022-February/476085.html

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi Ilias,

On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Simon,
>
> [...]
>
> > > +
> > >  UCLASS_DRIVER(tpm) = {
> > > -       .id             = UCLASS_TPM,
> > > -       .name           = "tpm",
> > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > +       .id                     = UCLASS_TPM,
> > > +       .name                   = "tpm",
> > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > -       .post_bind      = dm_scan_fdt_dev,
> > > +       .post_bind              = dm_scan_fdt_dev,
> > >  #endif
> > > +       .post_probe             = tpm_uclass_post_probe,
> > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > >  };
> > > --
> > > 2.25.1
> > >
> >
> > The driver needs a compatible string so it can be in the device tree.
>
> Why?  I've tried to hint this on the previous iteration of the patch.
> The RNG here is not a *device*.  The TPM is the device and you are
> guaranteed to have an RNG.  The way to get a random number is send a
> special command to the TPM. So all that we should do here is leverage
> the fact that the TPM is already in the device tree.
>
> And fwiw we should stick to try to stick to what the DT spec defines
> as much as possible.  I personally don't see this as a special usecase
> were deviating from the spec is justified.

This is not a deviation from a spec. What spec? Also, I don't want to
get into another discussion about what a device is. We can disagree on
that if you like.

One reason is that U-Boot generally requires compatible strings, e.g.
with of-platdata. But also we can refer to the rand device from
elsewhere in the tree. I know that Linux does lots of ad-hoc device
creation and doesn't really have the concept of a uclass consistently
applied, but this is U-Boot.

So please add a compatible string sow e can use it in the DT.

\Regards,
Simon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Rob Herring]

On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ilias,
>
> On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > [...]
> >
> > > > +
> > > >  UCLASS_DRIVER(tpm) = {
> > > > -       .id             = UCLASS_TPM,
> > > > -       .name           = "tpm",
> > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > +       .id                     = UCLASS_TPM,
> > > > +       .name                   = "tpm",
> > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > +       .post_bind              = dm_scan_fdt_dev,
> > > >  #endif
> > > > +       .post_probe             = tpm_uclass_post_probe,
> > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > >  };
> > > > --
> > > > 2.25.1
> > > >
> > >
> > > The driver needs a compatible string so it can be in the device tree.
> >
> > Why?  I've tried to hint this on the previous iteration of the patch.
> > The RNG here is not a *device*.  The TPM is the device and you are
> > guaranteed to have an RNG.  The way to get a random number is send a
> > special command to the TPM. So all that we should do here is leverage
> > the fact that the TPM is already in the device tree.
> >
> > And fwiw we should stick to try to stick to what the DT spec defines
> > as much as possible.  I personally don't see this as a special usecase
> > were deviating from the spec is justified.
>
> This is not a deviation from a spec. What spec? Also, I don't want to
> get into another discussion about what a device is. We can disagree on
> that if you like.
>
> One reason is that U-Boot generally requires compatible strings, e.g.
> with of-platdata. But also we can refer to the rand device from
> elsewhere in the tree. I know that Linux does lots of ad-hoc device
> creation and doesn't really have the concept of a uclass consistently
> applied, but this is U-Boot.

You are letting client/OS details define your binding. Doing so
doesn't result in OS agnostic bindings. Sure, it would be nice if DT
nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
sometimes and DT is not an abstraction layer. The general guidance on
whether there are child nodes for sub-blocks is do they have their own
resources in DT or are the sub-blocks reusable on multiple devices.
Neither of those are the case here.

Besides, we already have TPM device bindings defined. Requiring
binding changes when adding a new client/OS feature is not good
practice either.

Rob

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi Rob,

On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
>
> On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > [...]
> > >
> > > > > +
> > > > >  UCLASS_DRIVER(tpm) = {
> > > > > -       .id             = UCLASS_TPM,
> > > > > -       .name           = "tpm",
> > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > +       .id                     = UCLASS_TPM,
> > > > > +       .name                   = "tpm",
> > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > >  #endif
> > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > >  };
> > > > > --
> > > > > 2.25.1
> > > > >
> > > >
> > > > The driver needs a compatible string so it can be in the device tree.
> > >
> > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > The RNG here is not a *device*.  The TPM is the device and you are
> > > guaranteed to have an RNG.  The way to get a random number is send a
> > > special command to the TPM. So all that we should do here is leverage
> > > the fact that the TPM is already in the device tree.
> > >
> > > And fwiw we should stick to try to stick to what the DT spec defines
> > > as much as possible.  I personally don't see this as a special usecase
> > > were deviating from the spec is justified.
> >
> > This is not a deviation from a spec. What spec? Also, I don't want to
> > get into another discussion about what a device is. We can disagree on
> > that if you like.
> >
> > One reason is that U-Boot generally requires compatible strings, e.g.
> > with of-platdata. But also we can refer to the rand device from
> > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > creation and doesn't really have the concept of a uclass consistently
> > applied, but this is U-Boot.
>
> You are letting client/OS details define your binding. Doing so
> doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> sometimes and DT is not an abstraction layer. The general guidance on
> whether there are child nodes for sub-blocks is do they have their own
> resources in DT or are the sub-blocks reusable on multiple devices.
> Neither of those are the case here.
>
> Besides, we already have TPM device bindings defined. Requiring
> binding changes when adding a new client/OS feature is not good
> practice either.

I'm not sure what to do with this, but in general the practice of
implied subnodes is not friendly to U-Boot. Yet it seems like a common
feature of the bindings at present, for example GPIOs.

The device tree is how U-Boot determines which random-number generator
to use for a particular function. For example, for VBE, if we need to
generate some random numbers we'd like to specify which device creates
them. It can't just be 'use the TPM if you find one'. I'm not sure how
that works in Linux?

Regards,
SImon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3712 bytes --]

On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> Hi Rob,
> 
> On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > +
> > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > -       .id             = UCLASS_TPM,
> > > > > > -       .name           = "tpm",
> > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > +       .id                     = UCLASS_TPM,
> > > > > > +       .name                   = "tpm",
> > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > >  #endif
> > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > >  };
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > >
> > > > > The driver needs a compatible string so it can be in the device tree.
> > > >
> > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > special command to the TPM. So all that we should do here is leverage
> > > > the fact that the TPM is already in the device tree.
> > > >
> > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > as much as possible.  I personally don't see this as a special usecase
> > > > were deviating from the spec is justified.
> > >
> > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > get into another discussion about what a device is. We can disagree on
> > > that if you like.
> > >
> > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > with of-platdata. But also we can refer to the rand device from
> > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > creation and doesn't really have the concept of a uclass consistently
> > > applied, but this is U-Boot.
> >
> > You are letting client/OS details define your binding. Doing so
> > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > sometimes and DT is not an abstraction layer. The general guidance on
> > whether there are child nodes for sub-blocks is do they have their own
> > resources in DT or are the sub-blocks reusable on multiple devices.
> > Neither of those are the case here.
> >
> > Besides, we already have TPM device bindings defined. Requiring
> > binding changes when adding a new client/OS feature is not good
> > practice either.
> 
> I'm not sure what to do with this, but in general the practice of
> implied subnodes is not friendly to U-Boot. Yet it seems like a common
> feature of the bindings at present, for example GPIOs.
> 
> The device tree is how U-Boot determines which random-number generator
> to use for a particular function. For example, for VBE, if we need to
> generate some random numbers we'd like to specify which device creates
> them. It can't just be 'use the TPM if you find one'. I'm not sure how
> that works in Linux?

Why can't it be "use the TPM if you find one" since a TPM is a superset
of an RNG?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi Tom,

On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
>
> On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > Hi Rob,
> >
> > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > [...]
> > > > >
> > > > > > > +
> > > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > > -       .id             = UCLASS_TPM,
> > > > > > > -       .name           = "tpm",
> > > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > +       .id                     = UCLASS_TPM,
> > > > > > > +       .name                   = "tpm",
> > > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > > >  #endif
> > > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > >  };
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > >
> > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > >
> > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > special command to the TPM. So all that we should do here is leverage
> > > > > the fact that the TPM is already in the device tree.
> > > > >
> > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > were deviating from the spec is justified.
> > > >
> > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > get into another discussion about what a device is. We can disagree on
> > > > that if you like.
> > > >
> > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > with of-platdata. But also we can refer to the rand device from
> > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > creation and doesn't really have the concept of a uclass consistently
> > > > applied, but this is U-Boot.
> > >
> > > You are letting client/OS details define your binding. Doing so
> > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > sometimes and DT is not an abstraction layer. The general guidance on
> > > whether there are child nodes for sub-blocks is do they have their own
> > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > Neither of those are the case here.
> > >
> > > Besides, we already have TPM device bindings defined. Requiring
> > > binding changes when adding a new client/OS feature is not good
> > > practice either.
> >
> > I'm not sure what to do with this, but in general the practice of
> > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > feature of the bindings at present, for example GPIOs.
> >
> > The device tree is how U-Boot determines which random-number generator
> > to use for a particular function. For example, for VBE, if we need to
> > generate some random numbers we'd like to specify which device creates
> > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > that works in Linux?
>
> Why can't it be "use the TPM if you find one" since a TPM is a superset
> of an RNG?

You mean look through the available rand devices and choose the one
whose parent is a TPM?

Sure. There are lots of things you can do. But device tree is supposed
to provide the tree of devices and allow such things to be configured
deterministically.

Regards,
Simon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4528 bytes --]

On Thu, Jul 14, 2022 at 04:21:58AM -0600, Simon Glass wrote:
> Hi Tom,
> 
> On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > > Hi Rob,
> > >
> > > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ilias,
> > > > >
> > > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > > +
> > > > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > > > -       .id             = UCLASS_TPM,
> > > > > > > > -       .name           = "tpm",
> > > > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > > +       .id                     = UCLASS_TPM,
> > > > > > > > +       .name                   = "tpm",
> > > > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > > > >  #endif
> > > > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > > >  };
> > > > > > > > --
> > > > > > > > 2.25.1
> > > > > > > >
> > > > > > >
> > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > >
> > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > the fact that the TPM is already in the device tree.
> > > > > >
> > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > were deviating from the spec is justified.
> > > > >
> > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > get into another discussion about what a device is. We can disagree on
> > > > > that if you like.
> > > > >
> > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > with of-platdata. But also we can refer to the rand device from
> > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > applied, but this is U-Boot.
> > > >
> > > > You are letting client/OS details define your binding. Doing so
> > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > whether there are child nodes for sub-blocks is do they have their own
> > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > Neither of those are the case here.
> > > >
> > > > Besides, we already have TPM device bindings defined. Requiring
> > > > binding changes when adding a new client/OS feature is not good
> > > > practice either.
> > >
> > > I'm not sure what to do with this, but in general the practice of
> > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > feature of the bindings at present, for example GPIOs.
> > >
> > > The device tree is how U-Boot determines which random-number generator
> > > to use for a particular function. For example, for VBE, if we need to
> > > generate some random numbers we'd like to specify which device creates
> > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > that works in Linux?
> >
> > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > of an RNG?
> 
> You mean look through the available rand devices and choose the one
> whose parent is a TPM?
> 
> Sure. There are lots of things you can do. But device tree is supposed
> to provide the tree of devices and allow such things to be configured
> deterministically.

No, I mean pick the TPM device because by definition it is an RNG.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi Tom,

On Thu, 14 Jul 2022 at 05:19, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Jul 14, 2022 at 04:21:58AM -0600, Simon Glass wrote:
> > Hi Tom,
> >
> > On Wed, 13 Jul 2022 at 12:09, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Wed, Jul 13, 2022 at 09:28:16AM -0600, Simon Glass wrote:
> > > > Hi Rob,
> > > >
> > > > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > > > >
> > > > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > > > >
> > > > > > Hi Ilias,
> > > > > >
> > > > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > > > <ilias.apalodimas@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > > +
> > > > > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > > > > -       .id             = UCLASS_TPM,
> > > > > > > > > -       .name           = "tpm",
> > > > > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > > > +       .id                     = UCLASS_TPM,
> > > > > > > > > +       .name                   = "tpm",
> > > > > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > > > > >  #endif
> > > > > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > > > >  };
> > > > > > > > > --
> > > > > > > > > 2.25.1
> > > > > > > > >
> > > > > > > >
> > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > >
> > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > the fact that the TPM is already in the device tree.
> > > > > > >
> > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > were deviating from the spec is justified.
> > > > > >
> > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > that if you like.
> > > > > >
> > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > applied, but this is U-Boot.
> > > > >
> > > > > You are letting client/OS details define your binding. Doing so
> > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > Neither of those are the case here.
> > > > >
> > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > binding changes when adding a new client/OS feature is not good
> > > > > practice either.
> > > >
> > > > I'm not sure what to do with this, but in general the practice of
> > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > feature of the bindings at present, for example GPIOs.
> > > >
> > > > The device tree is how U-Boot determines which random-number generator
> > > > to use for a particular function. For example, for VBE, if we need to
> > > > generate some random numbers we'd like to specify which device creates
> > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > that works in Linux?
> > >
> > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > of an RNG?
> >
> > You mean look through the available rand devices and choose the one
> > whose parent is a TPM?
> >
> > Sure. There are lots of things you can do. But device tree is supposed
> > to provide the tree of devices and allow such things to be configured
> > deterministically.
>
> No, I mean pick the TPM device because by definition it is an RNG.

I think that is what I said, or meant. The TPM is not a rand device,
it is the child of the TPM which might be. But for example, the Google
cr50 may not be.

Regards,
Simon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Ilias Apalodimas]

Hi Simon,

[...]
> > > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > > >
> > > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > > the fact that the TPM is already in the device tree.
> > > > > > > >
> > > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > > were deviating from the spec is justified.
> > > > > > >
> > > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > > that if you like.
> > > > > > >
> > > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > > applied, but this is U-Boot.
> > > > > >
> > > > > > You are letting client/OS details define your binding. Doing so
> > > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > > Neither of those are the case here.
> > > > > >
> > > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > > binding changes when adding a new client/OS feature is not good
> > > > > > practice either.
> > > > >
> > > > > I'm not sure what to do with this, but in general the practice of
> > > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > > feature of the bindings at present, for example GPIOs.
> > > > >
> > > > > The device tree is how U-Boot determines which random-number generator
> > > > > to use for a particular function. For example, for VBE, if we need to
> > > > > generate some random numbers we'd like to specify which device creates
> > > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > > that works in Linux?
> > > >
> > > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > > of an RNG?
> > >
> > > You mean look through the available rand devices and choose the one
> > > whose parent is a TPM?
> > >
> > > Sure. There are lots of things you can do. But device tree is supposed
> > > to provide the tree of devices and allow such things to be configured
> > > deterministically.
> >
> > No, I mean pick the TPM device because by definition it is an RNG.
>
> I think that is what I said, or meant. The TPM is not a rand device,
> it is the child of the TPM which might be.

The design principle of the tpm1.2 says that it must have an rng [1].
I think something similar is implied on 2.0 architecture as well [2].
Looking at the cr50 description is says "TPM-like" [3]  not TPM.  I
assume an RNG exists internally since the TPM requires and RNG for
nonces, key generation etc.

Any idea what happens if you run tpm_getrandom() against a cr50? If
you get back garbage then we got a problem since you can't really tell
if it's garbage or a random seed.  But if it returns something sane
along the lines of 'not supported' then I guess we can use that and
try the next RNG?

[1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-1-Design-Principles_v1.2_rev116_01032011.pdf
4.2.5 random number generator
[2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
11.4.10 RNG module
[3] https://www.kernel.org/doc/Documentation/devicetree/bindings/security/tpm/google%2Ccr50.txt

Regards
/Ilias

> But for example, the Google
> cr50 may not be.
>
> Regards,
> Simon

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4922 bytes --]

On Thu, Jul 14, 2022 at 06:47:28PM +0300, Ilias Apalodimas wrote:
> Hi Simon,
> 
> [...]
> > > > > > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > > > > > >
> > > > > > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > > > > > special command to the TPM. So all that we should do here is leverage
> > > > > > > > > the fact that the TPM is already in the device tree.
> > > > > > > > >
> > > > > > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > > > > > were deviating from the spec is justified.
> > > > > > > >
> > > > > > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > > > > > get into another discussion about what a device is. We can disagree on
> > > > > > > > that if you like.
> > > > > > > >
> > > > > > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > > > > > with of-platdata. But also we can refer to the rand device from
> > > > > > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > > > > > creation and doesn't really have the concept of a uclass consistently
> > > > > > > > applied, but this is U-Boot.
> > > > > > >
> > > > > > > You are letting client/OS details define your binding. Doing so
> > > > > > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > > > > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > > > > > sometimes and DT is not an abstraction layer. The general guidance on
> > > > > > > whether there are child nodes for sub-blocks is do they have their own
> > > > > > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > > > > > Neither of those are the case here.
> > > > > > >
> > > > > > > Besides, we already have TPM device bindings defined. Requiring
> > > > > > > binding changes when adding a new client/OS feature is not good
> > > > > > > practice either.
> > > > > >
> > > > > > I'm not sure what to do with this, but in general the practice of
> > > > > > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > > > > > feature of the bindings at present, for example GPIOs.
> > > > > >
> > > > > > The device tree is how U-Boot determines which random-number generator
> > > > > > to use for a particular function. For example, for VBE, if we need to
> > > > > > generate some random numbers we'd like to specify which device creates
> > > > > > them. It can't just be 'use the TPM if you find one'. I'm not sure how
> > > > > > that works in Linux?
> > > > >
> > > > > Why can't it be "use the TPM if you find one" since a TPM is a superset
> > > > > of an RNG?
> > > >
> > > > You mean look through the available rand devices and choose the one
> > > > whose parent is a TPM?
> > > >
> > > > Sure. There are lots of things you can do. But device tree is supposed
> > > > to provide the tree of devices and allow such things to be configured
> > > > deterministically.
> > >
> > > No, I mean pick the TPM device because by definition it is an RNG.
> >
> > I think that is what I said, or meant. The TPM is not a rand device,
> > it is the child of the TPM which might be.
> 
> The design principle of the tpm1.2 says that it must have an rng [1].
> I think something similar is implied on 2.0 architecture as well [2].
> Looking at the cr50 description is says "TPM-like" [3]  not TPM.  I
> assume an RNG exists internally since the TPM requires and RNG for
> nonces, key generation etc.
> 
> Any idea what happens if you run tpm_getrandom() against a cr50? If
> you get back garbage then we got a problem since you can't really tell
> if it's garbage or a random seed.  But if it returns something sane
> along the lines of 'not supported' then I guess we can use that and
> try the next RNG?
> 
> [1] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-1-Design-Principles_v1.2_rev116_01032011.pdf
> 4.2.5 random number generator
> [2] https://trustedcomputinggroup.org/wp-content/uploads/TPM-Rev-2.0-Part-1-Architecture-01.07-2014-03-13.pdf
> 11.4.10 RNG module
> [3] https://www.kernel.org/doc/Documentation/devicetree/bindings/security/tpm/google%2Ccr50.txt

What I'm saying is from what I understand a spec compliant TPM is an
RNG, so we should be able to match on that existing compatible.  What
the cr50 does or doesn't do (or is or is not compliant about) would
indeed make things more complex but I think also maybe helps the rest of
us understand where you've been coming from.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Rob Herring]

On Wed, Jul 13, 2022 at 9:28 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Rob,
>
> On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> >
> > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ilias,
> > >
> > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > <ilias.apalodimas@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > [...]
> > > >
> > > > > > +
> > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > -       .id             = UCLASS_TPM,
> > > > > > -       .name           = "tpm",
> > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > +       .id                     = UCLASS_TPM,
> > > > > > +       .name                   = "tpm",
> > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > >  #endif
> > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > >  };
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > > >
> > > > > The driver needs a compatible string so it can be in the device tree.
> > > >
> > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > special command to the TPM. So all that we should do here is leverage
> > > > the fact that the TPM is already in the device tree.
> > > >
> > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > as much as possible.  I personally don't see this as a special usecase
> > > > were deviating from the spec is justified.
> > >
> > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > get into another discussion about what a device is. We can disagree on
> > > that if you like.
> > >
> > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > with of-platdata. But also we can refer to the rand device from
> > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > creation and doesn't really have the concept of a uclass consistently
> > > applied, but this is U-Boot.
> >
> > You are letting client/OS details define your binding. Doing so
> > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > sometimes and DT is not an abstraction layer. The general guidance on
> > whether there are child nodes for sub-blocks is do they have their own
> > resources in DT or are the sub-blocks reusable on multiple devices.
> > Neither of those are the case here.
> >
> > Besides, we already have TPM device bindings defined. Requiring
> > binding changes when adding a new client/OS feature is not good
> > practice either.
>
> I'm not sure what to do with this, but in general the practice of
> implied subnodes is not friendly to U-Boot. Yet it seems like a common
> feature of the bindings at present, for example GPIOs.

It's perfectly normal for a single device to be multiple providers. A
common example is clock AND reset blocks. They are both a clock and
reset provider. So you could have either:

crm {
  compatible = "foo,soc-crm";
  clock-controller {
   compatible = "foo,soc-crm-clocks";
   #clock-cells = <1>;
  };
  reset-controller {
    compatible = "foo,soc-crm-resets";
    #reset-cells = <1>;
  };
};

or:

crm {
  compatible = "foo,soc-crm";
  #clock-cells = <1>;
  #reset-cells = <1>;
};

Which one's right? Well, depends on the h/w really. If clock and reset
controls are interleaved bits or registers, then the first case really
doesn't make sense.

In any case, this ship has sailed. There are tons of the latter case
already, so either you have to figure out support 1 DT node to N
drivers or you're going to be carrying a bunch of your own u-boot
bindings.

> The device tree is how U-Boot determines which random-number generator
> to use for a particular function. For example, for VBE, if we need to
> generate some random numbers we'd like to specify which device creates
> them. It can't just be 'use the TPM if you find one'.

Why not?

> I'm not sure how that works in Linux?

Not sure. I think Linux's answer is use all of the RNG sources and
none of them directly.

Rob

Re: [PATCH v6 3/7] tpm: Add the RNG child device

[Simon Glass]

Hi,

On Thu, 14 Jul 2022 at 11:55, Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Jul 13, 2022 at 9:28 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Rob,
> >
> > On Tue, 12 Jul 2022 at 08:11, Rob Herring <robh@kernel.org> wrote:
> > >
> > > On Tue, Jul 12, 2022 at 5:04 AM Simon Glass <sjg@chromium.org> wrote:
> > > >
> > > > Hi Ilias,
> > > >
> > > > On Fri, 8 Jul 2022 at 02:24, Ilias Apalodimas
> > > > <ilias.apalodimas@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > [...]
> > > > >
> > > > > > > +
> > > > > > >  UCLASS_DRIVER(tpm) = {
> > > > > > > -       .id             = UCLASS_TPM,
> > > > > > > -       .name           = "tpm",
> > > > > > > -       .flags          = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > > +       .id                     = UCLASS_TPM,
> > > > > > > +       .name                   = "tpm",
> > > > > > > +       .flags                  = DM_UC_FLAG_SEQ_ALIAS,
> > > > > > >  #if CONFIG_IS_ENABLED(OF_REAL)
> > > > > > > -       .post_bind      = dm_scan_fdt_dev,
> > > > > > > +       .post_bind              = dm_scan_fdt_dev,
> > > > > > >  #endif
> > > > > > > +       .post_probe             = tpm_uclass_post_probe,
> > > > > > >         .per_device_auto        = sizeof(struct tpm_chip_priv),
> > > > > > >  };
> > > > > > > --
> > > > > > > 2.25.1
> > > > > > >
> > > > > >
> > > > > > The driver needs a compatible string so it can be in the device tree.
> > > > >
> > > > > Why?  I've tried to hint this on the previous iteration of the patch.
> > > > > The RNG here is not a *device*.  The TPM is the device and you are
> > > > > guaranteed to have an RNG.  The way to get a random number is send a
> > > > > special command to the TPM. So all that we should do here is leverage
> > > > > the fact that the TPM is already in the device tree.
> > > > >
> > > > > And fwiw we should stick to try to stick to what the DT spec defines
> > > > > as much as possible.  I personally don't see this as a special usecase
> > > > > were deviating from the spec is justified.
> > > >
> > > > This is not a deviation from a spec. What spec? Also, I don't want to
> > > > get into another discussion about what a device is. We can disagree on
> > > > that if you like.
> > > >
> > > > One reason is that U-Boot generally requires compatible strings, e.g.
> > > > with of-platdata. But also we can refer to the rand device from
> > > > elsewhere in the tree. I know that Linux does lots of ad-hoc device
> > > > creation and doesn't really have the concept of a uclass consistently
> > > > applied, but this is U-Boot.
> > >
> > > You are letting client/OS details define your binding. Doing so
> > > doesn't result in OS agnostic bindings. Sure, it would be nice if DT
> > > nodes and drivers were always a nice clean 1:1 ratio, but h/w is messy
> > > sometimes and DT is not an abstraction layer. The general guidance on
> > > whether there are child nodes for sub-blocks is do they have their own
> > > resources in DT or are the sub-blocks reusable on multiple devices.
> > > Neither of those are the case here.
> > >
> > > Besides, we already have TPM device bindings defined. Requiring
> > > binding changes when adding a new client/OS feature is not good
> > > practice either.
> >
> > I'm not sure what to do with this, but in general the practice of
> > implied subnodes is not friendly to U-Boot. Yet it seems like a common
> > feature of the bindings at present, for example GPIOs.
>
> It's perfectly normal for a single device to be multiple providers. A
> common example is clock AND reset blocks. They are both a clock and
> reset provider. So you could have either:
>
> crm {
>   compatible = "foo,soc-crm";
>   clock-controller {
>    compatible = "foo,soc-crm-clocks";
>    #clock-cells = <1>;
>   };
>   reset-controller {
>     compatible = "foo,soc-crm-resets";
>     #reset-cells = <1>;
>   };
> };
>
> or:
>
> crm {
>   compatible = "foo,soc-crm";
>   #clock-cells = <1>;
>   #reset-cells = <1>;
> };
>
> Which one's right? Well, depends on the h/w really. If clock and reset
> controls are interleaved bits or registers, then the first case really
> doesn't make sense.
>
> In any case, this ship has sailed. There are tons of the latter case
> already, so either you have to figure out support 1 DT node to N
> drivers or you're going to be carrying a bunch of your own u-boot
> bindings.

Thanks for the info. I'm really only talking about this TPM issue here.

>
> > The device tree is how U-Boot determines which random-number generator
> > to use for a particular function. For example, for VBE, if we need to
> > generate some random numbers we'd like to specify which device creates
> > them. It can't just be 'use the TPM if you find one'.
>
> Why not?

It isn't deterministic. This is exactly the sort of selection that
device tree is supposed to permit. It would be better to have a
phandle from the board-config node, VBE method node, or something else
that provides the devices to be used.

>
> > I'm not sure how that works in Linux?
>
> Not sure. I think Linux's answer is use all of the RNG sources and
> none of them directly.

OK.

Re Ilias' question about cr50, I don't know.

Re Tom's point about matching on compatible strings, IMO it is cleaner
for the driver to create a child device for the random-number device
if it can provide one, as is done with PMIC when providing GPIOS and
regulators, for example.

Since it seems that the TPM binding does not have this, my preference
would be to add this to the TPM binding.

If that is not acceptable, then we have the TPM_RNG option which we
can disable when we do have such a binding, perhaps only in U-Boot.

I think I have made my point here and people understand it, so I'll
drop my objection to this patch and we can worry about it later if it
causes problems.

Regards,
Simon