[PATCH 0/3] qemu: arm64: Add support for efi_rng_protocol

[PATCH 0/3] qemu: arm64: Add support for efi_rng_protocol

[Sughosh Ganu]

The patch series adds support for the EFI_RNG_PROTOCOL routines for
qemu arm64 platform. The getrng routine, used to get the random bytes,
uses the virtio-rng device found on the platform. The protocol, once
installed, can be used by the efi stub in the kernel for getting
random bytes needed for the kaslr feature.

These patches apply on top of the patch series to add random number
generator driver uclass[1]

[1] - https://lists.denx.de/pipermail/u-boot/2019-December/394010.html

Sughosh Ganu (3):
  efi_loader: Add guidcpy function
  efi: qemu: arm64: Add efi_rng_protocol implementation for the platform
  efi_rng_protocol: Install the efi_rng_protocol on the root node

 board/emulation/qemu-arm/qemu-arm.c | 50 ++++++++++++++++++++++++
 include/efi_loader.h                |  9 +++++
 include/efi_rng.h                   | 34 +++++++++++++++++
 lib/efi_loader/Kconfig              |  8 ++++
 lib/efi_loader/Makefile             |  1 +
 lib/efi_loader/efi_rng.c            | 76 +++++++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_root_node.c      |  4 ++
 7 files changed, 182 insertions(+)
 create mode 100644 include/efi_rng.h
 create mode 100644 lib/efi_loader/efi_rng.c

-- 
2.7.4

[PATCH 1/3] efi_loader: Add guidcpy function

[Sughosh Ganu]

Add guidcpy function to copy the source guid to the destination
guid.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_loader.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 16a1b25..bec7873 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -17,6 +17,11 @@ static inline int guidcmp(const void *g1, const void *g2)
 	return memcmp(g1, g2, sizeof(efi_guid_t));
 }
 
+static inline void *guidcpy(efi_guid_t *dst, const efi_guid_t *src)
+{
+	return memcpy(dst, src, sizeof(*dst));
+}
+
 /* No need for efi loader support in SPL */
 #if CONFIG_IS_ENABLED(EFI_LOADER)
 
-- 
2.7.4

[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Sughosh Ganu]

Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
invoked by the efi stub in the kernel for getting random seed for
kaslr.

The routines are platform specific, and use the virtio-rng device on
the platform to get random data.

The feature can be enabled through the following config
CONFIG_EFI_RNG_PROTOCOL

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 board/emulation/qemu-arm/qemu-arm.c | 50 +++++++++++++++++++++++++
 include/efi_rng.h                   | 34 +++++++++++++++++
 lib/efi_loader/Kconfig              |  8 ++++
 lib/efi_loader/Makefile             |  1 +
 lib/efi_loader/efi_rng.c            | 74 +++++++++++++++++++++++++++++++++++++
 5 files changed, 167 insertions(+)
 create mode 100644 include/efi_rng.h
 create mode 100644 lib/efi_loader/efi_rng.c

diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
index e1f4709..3176421 100644
--- a/board/emulation/qemu-arm/qemu-arm.c
+++ b/board/emulation/qemu-arm/qemu-arm.c
@@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
 	/* QEMU loads a generated DTB for us at the start of RAM. */
 	return (void *)CONFIG_SYS_SDRAM_BASE;
 }
+
+#if defined(CONFIG_EFI_RNG_PROTOCOL)
+#include <efi_loader.h>
+#include <efi_rng.h>
+
+#include <dm/device-internal.h>
+
+#define VIRTIO_RNG_PCI_DEVICE	"virtio-pci.l#0"
+
+void platform_rng_getinfo(efi_rng_algorithm *rng_algo)
+{
+	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
+
+	guidcpy(rng_algo, &rng_raw_guid);
+}
+
+efi_status_t platform_get_rng_device(struct udevice **dev)
+{
+	int ret;
+	efi_status_t status = EFI_DEVICE_ERROR;
+	struct udevice *bus, *devp;
+
+	for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
+	     uclass_next_device(&bus)) {
+		for (device_find_first_child(bus, &devp); devp; device_find_next_child(&devp)) {
+			if (device_get_uclass_id(devp) == UCLASS_RNG) {
+				*dev = devp;
+				status = EFI_SUCCESS;
+				break;
+			}
+		}
+	}
+
+	if (status != EFI_SUCCESS) {
+		debug("No rng device found\n");
+		return EFI_DEVICE_ERROR;
+	}
+
+	if (*dev) {
+		ret = device_probe(*dev);
+		if (ret)
+			return EFI_DEVICE_ERROR;
+	} else {
+		debug("Couldn't get child device\n");
+		return EFI_DEVICE_ERROR;
+	}
+
+	return EFI_SUCCESS;
+}
+#endif /* CONFIG_EFI_RNG_PROTOCOL */
diff --git a/include/efi_rng.h b/include/efi_rng.h
new file mode 100644
index 0000000..df749dd
--- /dev/null
+++ b/include/efi_rng.h
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#if !defined _EFI_RNG_H_
+#define _EFI_RNG_H_
+
+#include <efi.h>
+#include <efi_api.h>
+
+#define EFI_RNG_PROTOCOL_GUID \
+	EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \
+		 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
+
+#define EFI_RNG_ALGORITHM_RAW \
+	EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \
+		 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
+
+typedef efi_guid_t efi_rng_algorithm;
+
+struct efi_rng_protocol {
+	efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,
+				       efi_uintn_t *rng_algo_size,
+				       efi_rng_algorithm *rng_algo);
+	efi_status_t (EFIAPI *getrng)(struct efi_rng_protocol *this,
+				      efi_rng_algorithm *rng_algo,
+				      efi_uintn_t rng_len, uint8_t *rng_data);
+};
+
+void platform_rng_getinfo(efi_rng_algorithm *rng_algo);
+efi_status_t platform_get_rng_device(struct udevice **dev);
+
+#endif /* _EFI_RNG_H_ */
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 21ef440..7437442 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -120,4 +120,12 @@ config EFI_GRUB_ARM32_WORKAROUND
 	  GRUB prior to version 2.04 requires U-Boot to disable caches. This
 	  workaround currently is also needed on systems with caches that
 	  cannot be managed via CP15.
+
+config EFI_RNG_PROTOCOL
+	bool "EFI_RNG_PROTOCOL support"
+	depends on DM_RNG && TARGET_QEMU_ARM_64BIT
+	help
+	  "Support for EFI_RNG_PROTOCOL implementation. Uses the rng
+	   device on the platform"
+
 endif
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 7db4060..04dc864 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_PARTITIONS) += efi_disk.o
 obj-$(CONFIG_NET) += efi_net.o
 obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
+obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
new file mode 100644
index 0000000..8456592
--- /dev/null
+++ b/lib/efi_loader/efi_rng.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2019, Linaro Limited
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <efi_loader.h>
+#include <efi_rng.h>
+#include <rng.h>
+
+DECLARE_GLOBAL_DATA_PTR;
+
+static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
+					    efi_uintn_t *rng_algo_size,
+					    efi_rng_algorithm *rng_algo)
+{
+	if (!this || !rng_algo_size || !rng_algo)
+		return EFI_INVALID_PARAMETER;
+
+	if (*rng_algo_size < sizeof(*rng_algo)) {
+		*rng_algo_size = sizeof(*rng_algo);
+		return EFI_BUFFER_TOO_SMALL;
+	}
+
+	*rng_algo_size = sizeof(*rng_algo);
+	platform_rng_getinfo(rng_algo);
+
+	return EFI_SUCCESS;
+}
+
+static efi_status_t EFIAPI getrng(struct efi_rng_protocol *protocol,
+				       efi_rng_algorithm *rng_algo,
+				       efi_uintn_t rng_len, uint8_t *rng_data)
+{
+	int ret;
+	struct udevice *dev;
+	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
+
+	/*
+	 * Booting into the linux kernel mangles
+	 * x18, which holds the gd.
+	 * When the efi stub in the kernel invokes
+	 * the GetRng routine, gd needs to be
+	 * restored back, without which bad things
+	 * happen
+	 */
+	efi_restore_gd();
+
+	if (!protocol || !rng_data || !rng_len)
+		return EFI_INVALID_PARAMETER;
+
+	if (rng_algo) {
+		if (guidcmp(rng_algo, &rng_raw_guid))
+			return EFI_UNSUPPORTED;
+	}
+
+	ret = platform_get_rng_device(&dev);
+	if (ret != EFI_SUCCESS)
+		return EFI_DEVICE_ERROR;
+
+	ret = dm_rng_read(dev, rng_data, rng_len);
+	if (ret < 0) {
+		debug("Rng device read failed\n");
+		return EFI_DEVICE_ERROR;
+	}
+
+	return EFI_SUCCESS;
+}
+
+const struct efi_rng_protocol efi_rng_protocol_ops = {
+	.getinfo = rng_getinfo,
+	.getrng = getrng,
+};
-- 
2.7.4

[PATCH 3/3] efi_rng_protocol: Install the efi_rng_protocol on the root node

[Sughosh Ganu]

Install the EFI_RNG_PROTOCOL implementation for it's subsequent use by
the kernel for features like kaslr.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 include/efi_loader.h           | 4 ++++
 lib/efi_loader/efi_rng.c       | 2 ++
 lib/efi_loader/efi_root_node.c | 4 ++++
 3 files changed, 10 insertions(+)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index bec7873..71996ec 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -130,6 +130,7 @@ extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
 extern const struct efi_hii_config_access_protocol efi_hii_config_access;
 extern const struct efi_hii_database_protocol efi_hii_database;
 extern const struct efi_hii_string_protocol efi_hii_string;
+extern const struct efi_rng_protocol efi_rng_protocol_ops;
 
 uint16_t *efi_dp_str(struct efi_device_path *dp);
 
@@ -175,6 +176,9 @@ extern const efi_guid_t efi_guid_hii_config_access_protocol;
 extern const efi_guid_t efi_guid_hii_database_protocol;
 extern const efi_guid_t efi_guid_hii_string_protocol;
 
+/* GUID of RNG protocol */
+extern const efi_guid_t efi_guid_rng_protocol;
+
 extern unsigned int __efi_runtime_start, __efi_runtime_stop;
 extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
 
diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
index 8456592..80c8fc9 100644
--- a/lib/efi_loader/efi_rng.c
+++ b/lib/efi_loader/efi_rng.c
@@ -11,6 +11,8 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+const efi_guid_t efi_guid_rng_protocol = EFI_RNG_PROTOCOL_GUID;
+
 static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
 					    efi_uintn_t *rng_algo_size,
 					    efi_rng_algorithm *rng_algo)
diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
index f68b0fd..3d937ce 100644
--- a/lib/efi_loader/efi_root_node.c
+++ b/lib/efi_loader/efi_root_node.c
@@ -81,6 +81,10 @@ efi_status_t efi_root_node_register(void)
 			 &efi_guid_hii_config_routing_protocol,
 			 (void *)&efi_hii_config_routing,
 #endif
+#if CONFIG_IS_ENABLED(EFI_RNG_PROTOCOL)
+			 &efi_guid_rng_protocol,
+			 (void *)&efi_rng_protocol_ops,
+#endif
 			 NULL));
 	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
 	return ret;
-- 
2.7.4

[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Heinrich Schuchardt]

On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
> platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
> invoked by the efi stub in the kernel for getting random seed for
> kaslr.
>
> The routines are platform specific, and use the virtio-rng device on
> the platform to get random data.
>
> The feature can be enabled through the following config
> CONFIG_EFI_RNG_PROTOCOL
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   board/emulation/qemu-arm/qemu-arm.c | 50 +++++++++++++++++++++++++
>   include/efi_rng.h                   | 34 +++++++++++++++++
>   lib/efi_loader/Kconfig              |  8 ++++
>   lib/efi_loader/Makefile             |  1 +
>   lib/efi_loader/efi_rng.c            | 74 +++++++++++++++++++++++++++++++++++++
>   5 files changed, 167 insertions(+)
>   create mode 100644 include/efi_rng.h
>   create mode 100644 lib/efi_loader/efi_rng.c
>
> diff --git a/board/emulation/qemu-arm/qemu-arm.c b/board/emulation/qemu-arm/qemu-arm.c
> index e1f4709..3176421 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
>   	/* QEMU loads a generated DTB for us at the start of RAM. */
>   	return (void *)CONFIG_SYS_SDRAM_BASE;
>   }
> +
> +#if defined(CONFIG_EFI_RNG_PROTOCOL)
> +#include <efi_loader.h>
> +#include <efi_rng.h>
> +
> +#include <dm/device-internal.h>
> +
> +#define VIRTIO_RNG_PCI_DEVICE	"virtio-pci.l#0"
> +
> +void platform_rng_getinfo(efi_rng_algorithm *rng_algo)

Thanks for working on the implementation of the EFI_RNG_PROTOCOL.

Please, put an underscore after each word: platform_rng_get_info

> +{
> +	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	guidcpy(rng_algo, &rng_raw_guid);

This function should be in efi_rng.c if it is needed at all.

> +}
> +
> +efi_status_t platform_get_rng_device(struct udevice **dev)
> +{

Here you are creating platform specific code. The idea of the driver
model in U-Boot is to separate duties.

So the implementation of the EFI_RNG_PROTOCOL should be platform
agnostic and rely simply on looping of the devices of UCLASS_RNG.

Please, move function platform_get_rng_device into the RNG uclass.

> +	int ret;
> +	efi_status_t status = EFI_DEVICE_ERROR;
> +	struct udevice *bus, *devp;
> +
> +	for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
> +	     uclass_next_device(&bus)) {
> +		for (device_find_first_child(bus, &devp); devp; device_find_next_child(&devp)) {
> +			if (device_get_uclass_id(devp) == UCLASS_RNG) {
> +				*dev = devp;
> +				status = EFI_SUCCESS;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (status != EFI_SUCCESS) {
> +		debug("No rng device found\n");
> +		return EFI_DEVICE_ERROR;
> +	}
> +
> +	if (*dev) {
> +		ret = device_probe(*dev);
> +		if (ret)
> +			return EFI_DEVICE_ERROR;
> +	} else {
> +		debug("Couldn't get child device\n");
> +		return EFI_DEVICE_ERROR;
> +	}
> +
> +	return EFI_SUCCESS;
> +}
> +#endif /* CONFIG_EFI_RNG_PROTOCOL */
> diff --git a/include/efi_rng.h b/include/efi_rng.h
> new file mode 100644
> index 0000000..df749dd
> --- /dev/null
> +++ b/include/efi_rng.h
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#if !defined _EFI_RNG_H_
> +#define _EFI_RNG_H_
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +#define EFI_RNG_PROTOCOL_GUID \
> +	EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \
> +		 0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
> +
> +#define EFI_RNG_ALGORITHM_RAW \
> +	EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \
> +		 0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)

Please, add a comment line for each of the GUIDs.

> +
> +typedef efi_guid_t efi_rng_algorithm;

We want to avoid typedefs in U-Boot.

> +
> +struct efi_rng_protocol {
> +	efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,

get_info

> +				       efi_uintn_t *rng_algo_size,

Please, use the parameter names from the UEFI 2.8 specification:

rng_algorithm_list_size

> +				       efi_rng_algorithm *rng_algo);

rng_algorithm_list

> +	efi_status_t (EFIAPI *getrng)(struct efi_rng_protocol *this,

get_rng

> +				      efi_rng_algorithm *rng_algo,

efi_guid_t *rng_algorithm

> +				      efi_uintn_t rng_len, uint8_t *rng_data);

rng_value_length, rng_value

> +};
> +
> +void platform_rng_getinfo(efi_rng_algorithm *rng_algo);
> +efi_status_t platform_get_rng_device(struct udevice **dev);
> +
> +#endif /* _EFI_RNG_H_ */
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 21ef440..7437442 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -120,4 +120,12 @@ config EFI_GRUB_ARM32_WORKAROUND
>   	  GRUB prior to version 2.04 requires U-Boot to disable caches. This
>   	  workaround currently is also needed on systems with caches that
>   	  cannot be managed via CP15.
> +
> +config EFI_RNG_PROTOCOL
> +	bool "EFI_RNG_PROTOCOL support"
> +	depends on DM_RNG && TARGET_QEMU_ARM_64BIT

DM_RNG should be enough.

Just return EFI_UNSUPPORTED from EFI_RNG_PROTOCOL if no device of the
RNG uclass is found.

> +	help
> +	  "Support for EFI_RNG_PROTOCOL implementation. Uses the rng
> +	   device on the platform"
> +
>   endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 7db4060..04dc864 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -42,3 +42,4 @@ obj-$(CONFIG_PARTITIONS) += efi_disk.o
>   obj-$(CONFIG_NET) += efi_net.o
>   obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
>   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> +obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> new file mode 100644
> index 0000000..8456592
> --- /dev/null
> +++ b/lib/efi_loader/efi_rng.c
> @@ -0,0 +1,74 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2019, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <efi_loader.h>
> +#include <efi_rng.h>
> +#include <rng.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;


Please, add platform_rng_get_info() in this file.

> +
> +static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
> +					    efi_uintn_t *rng_algo_size,
> +					    efi_rng_algorithm *rng_algo)
> +{

Please, call EFI_ENTRY here.

> +	if (!this || !rng_algo_size || !rng_algo)

Please, allow RNGAlgorithmList == NULL here. The caller will typically
call this function twice. The first call will inform the caller about
the size of the buffer needed. The caller then allocates the buffer and
calls the function again.

> +		return EFI_INVALID_PARAMETER;
> +
> +	if (*rng_algo_size < sizeof(*rng_algo)) {
> +		*rng_algo_size = sizeof(*rng_algo);
> +		return EFI_BUFFER_TOO_SMALL;
> +	}
> +
> +	*rng_algo_size = sizeof(*rng_algo);
> +	platform_rng_getinfo(rng_algo);

Having a platform_rng_get_info() function does not make much sense if we
hard code the the size of the algorithm list above and the allowed
algorithm below. So you can directly put your memcpy() here.

If no UCLASS_RNG device is found, please, return EFI_UNSUPPORTED.

Please, call EFI_EXIT here.

> +
> +	return EFI_SUCCESS;
> +}
> +
> +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *protocol,
> +				       efi_rng_algorithm *rng_algo,
> +				       efi_uintn_t rng_len, uint8_t *rng_data)
> +{
> +	int ret;
> +	struct udevice *dev;
> +	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	/*
> +	 * Booting into the linux kernel mangles
> +	 * x18, which holds the gd.
> +	 * When the efi stub in the kernel invokes
> +	 * the GetRng routine, gd needs to be
> +	 * restored back, without which bad things
> +	 * happen
> +	 */
> +	efi_restore_gd();

Please, use EFI_ENTRY() instead.

> +
> +	if (!protocol || !rng_data || !rng_len)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (rng_algo) {
> +		if (guidcmp(rng_algo, &rng_raw_guid))
> +			return EFI_UNSUPPORTED;
> +	}
> +
> +	ret = platform_get_rng_device(&dev);
> +	if (ret != EFI_SUCCESS)
> +		return EFI_DEVICE_ERROR;

I think this should be EFI_UNSUPPORTED.

> +
> +	ret = dm_rng_read(dev, rng_data, rng_len);
> +	if (ret < 0) {
> +		debug("Rng device read failed\n");
> +		return EFI_DEVICE_ERROR;
> +	}
> +

You have to call EFI_EXIT(EFI_SUCCESS) here.

Best regards

Heinrich

> +	return EFI_SUCCESS;
> +}
> +
> +const struct efi_rng_protocol efi_rng_protocol_ops = {
> +	.getinfo = rng_getinfo,
> +	.getrng = getrng,
> +};
>

[PATCH 1/3] efi_loader: Add guidcpy function

[Heinrich Schuchardt]

On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> Add guidcpy function to copy the source guid to the destination
> guid.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/efi_loader.h | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 16a1b25..bec7873 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -17,6 +17,11 @@ static inline int guidcmp(const void *g1, const void *g2)
>   	return memcmp(g1, g2, sizeof(efi_guid_t));
>   }
>
> +static inline void *guidcpy(efi_guid_t *dst, const efi_guid_t *src)
> +{
> +	return memcpy(dst, src, sizeof(*dst));

If we introduce this function, here are other places to use it:

lib/efi_loader/efi_boottime.c:1404:
memcpy(&item->protocol, protocol, sizeof(efi_guid_t));
lib/efi_loader/efi_boottime.c:1635:
memcpy(&systab.tables[i].guid, guid, sizeof(*guid));

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

> +}
> +
>   /* No need for efi loader support in SPL */
>   #if CONFIG_IS_ENABLED(EFI_LOADER)
>
>

[PATCH 1/3] efi_loader: Add guidcpy function

[Sughosh Ganu]

On Tue, 24 Dec 2019 at 22:42, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> > Add guidcpy function to copy the source guid to the destination
> > guid.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/efi_loader.h | 5 +++++
> >   1 file changed, 5 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 16a1b25..bec7873 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -17,6 +17,11 @@ static inline int guidcmp(const void *g1, const void
> *g2)
> >       return memcmp(g1, g2, sizeof(efi_guid_t));
> >   }
> >
> > +static inline void *guidcpy(efi_guid_t *dst, const efi_guid_t *src)
> > +{
> > +     return memcpy(dst, src, sizeof(*dst));
>
> If we introduce this function, here are other places to use it:
>
> lib/efi_loader/efi_boottime.c:1404:
> memcpy(&item->protocol, protocol, sizeof(efi_guid_t));
> lib/efi_loader/efi_boottime.c:1635:
> memcpy(&systab.tables[i].guid, guid, sizeof(*guid));
>

Ok. Will make changes to use guidcpy for these two instances as well in V2.

-sughosh

[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Sughosh Ganu]

hi Heinrich,
Thanks for the review.

On Tue, 24 Dec 2019 at 22:35, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> > Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
> > platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
> > invoked by the efi stub in the kernel for getting random seed for
> > kaslr.
> >
> > The routines are platform specific, and use the virtio-rng device on
> > the platform to get random data.
> >
> > The feature can be enabled through the following config
> > CONFIG_EFI_RNG_PROTOCOL
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   board/emulation/qemu-arm/qemu-arm.c | 50 +++++++++++++++++++++++++
> >   include/efi_rng.h                   | 34 +++++++++++++++++
> >   lib/efi_loader/Kconfig              |  8 ++++
> >   lib/efi_loader/Makefile             |  1 +
> >   lib/efi_loader/efi_rng.c            | 74
> +++++++++++++++++++++++++++++++++++++
> >   5 files changed, 167 insertions(+)
> >   create mode 100644 include/efi_rng.h
> >   create mode 100644 lib/efi_loader/efi_rng.c
> >
> > diff --git a/board/emulation/qemu-arm/qemu-arm.c
> b/board/emulation/qemu-arm/qemu-arm.c
> > index e1f4709..3176421 100644
> > --- a/board/emulation/qemu-arm/qemu-arm.c
> > +++ b/board/emulation/qemu-arm/qemu-arm.c
> > @@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
> >       /* QEMU loads a generated DTB for us at the start of RAM. */
> >       return (void *)CONFIG_SYS_SDRAM_BASE;
> >   }
> > +
> > +#if defined(CONFIG_EFI_RNG_PROTOCOL)
> > +#include <efi_loader.h>
> > +#include <efi_rng.h>
> > +
> > +#include <dm/device-internal.h>
> > +
> > +#define VIRTIO_RNG_PCI_DEVICE        "virtio-pci.l#0"
> > +
> > +void platform_rng_getinfo(efi_rng_algorithm *rng_algo)
>
> Thanks for working on the implementation of the EFI_RNG_PROTOCOL.
>
> Please, put an underscore after each word: platform_rng_get_info
>

Ok.


>
> > +{
> > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> > +
> > +     guidcpy(rng_algo, &rng_raw_guid);
>
> This function should be in efi_rng.c if it is needed at all.
>

Is the rng algorithm supported not platform specific. I believe different
platforms might use different algorithms for providing the random seed.


>
> > +}
> > +
> > +efi_status_t platform_get_rng_device(struct udevice **dev)
> > +{
>
> Here you are creating platform specific code. The idea of the driver
> model in U-Boot is to separate duties.
>
> So the implementation of the EFI_RNG_PROTOCOL should be platform
> agnostic and rely simply on looping of the devices of UCLASS_RNG.
>

The core implementation of efi_rng_protocol routines is indeed platform
agnostic. I have just separated the method of getting the rng device(struct
udevice), since different platforms might need different ways of getting
the rng device. For example, on the qemu arm64 platform, the rng device is
a child of a virtio device on the pci bus, while the rng module on the
stm32mp1 platform is a direct memory mapped peripheral. So the mechanism
used to fetch the rng udevice would be platform dependent. Hence the
platform_get_rng_device function. In fact, I see this kind of method
adopted in a lot many other boards, one example being
board/CZ.NIC/turris_omnia/turris_omnia.c(get_atsha204a_dev).


> Please, move function platform_get_rng_device into the RNG uclass.


> > +     int ret;
> > +     efi_status_t status = EFI_DEVICE_ERROR;
> > +     struct udevice *bus, *devp;
> > +
> > +     for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
> > +          uclass_next_device(&bus)) {
> > +             for (device_find_first_child(bus, &devp); devp;
> device_find_next_child(&devp)) {
> > +                     if (device_get_uclass_id(devp) == UCLASS_RNG) {
> > +                             *dev = devp;
> > +                             status = EFI_SUCCESS;
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +
> > +     if (status != EFI_SUCCESS) {
> > +             debug("No rng device found\n");
> > +             return EFI_DEVICE_ERROR;
> > +     }
> > +
> > +     if (*dev) {
> > +             ret = device_probe(*dev);
> > +             if (ret)
> > +                     return EFI_DEVICE_ERROR;
> > +     } else {
> > +             debug("Couldn't get child device\n");
> > +             return EFI_DEVICE_ERROR;
> > +     }
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +#endif /* CONFIG_EFI_RNG_PROTOCOL */
> > diff --git a/include/efi_rng.h b/include/efi_rng.h
> > new file mode 100644
> > index 0000000..df749dd
> > --- /dev/null
> > +++ b/include/efi_rng.h
> > @@ -0,0 +1,34 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#if !defined _EFI_RNG_H_
> > +#define _EFI_RNG_H_
> > +
> > +#include <efi.h>
> > +#include <efi_api.h>
> > +
> > +#define EFI_RNG_PROTOCOL_GUID \
> > +     EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \
> > +              0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
> > +
> > +#define EFI_RNG_ALGORITHM_RAW \
> > +     EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \
> > +              0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
>
> Please, add a comment line for each of the GUIDs.
>

Ok. Will do.


>
> > +
> > +typedef efi_guid_t efi_rng_algorithm;
>
> We want to avoid typedefs in U-Boot.
>

Ok. Will replace efi_rng_algorithm with efi_guid_t


>
> > +
> > +struct efi_rng_protocol {
> > +     efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,
>
> get_info
>

Ok.


>
> > +                                    efi_uintn_t *rng_algo_size,
>
> Please, use the parameter names from the UEFI 2.8 specification:
>
> rng_algorithm_list_size
>
> > +                                    efi_rng_algorithm *rng_algo);
>
> rng_algorithm_list
>
> > +     efi_status_t (EFIAPI *getrng)(struct efi_rng_protocol *this,
>
> get_rng
>
> > +                                   efi_rng_algorithm *rng_algo,
>
> efi_guid_t *rng_algorithm
>
> > +                                   efi_uintn_t rng_len, uint8_t
> *rng_data);
>
> rng_value_length, rng_value
>
>
Ok. Will make the above suggested changes.



> > +};
> > +
> > +void platform_rng_getinfo(efi_rng_algorithm *rng_algo);
> > +efi_status_t platform_get_rng_device(struct udevice **dev);
> > +
> > +#endif /* _EFI_RNG_H_ */
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 21ef440..7437442 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -120,4 +120,12 @@ config EFI_GRUB_ARM32_WORKAROUND
> >         GRUB prior to version 2.04 requires U-Boot to disable caches.
> This
> >         workaround currently is also needed on systems with caches that
> >         cannot be managed via CP15.
> > +
> > +config EFI_RNG_PROTOCOL
> > +     bool "EFI_RNG_PROTOCOL support"
> > +     depends on DM_RNG && TARGET_QEMU_ARM_64BIT
>
> DM_RNG should be enough.
>
> Just return EFI_UNSUPPORTED from EFI_RNG_PROTOCOL if no device of the
> RNG uclass is found.
>

Ok.


>
> > +     help
> > +       "Support for EFI_RNG_PROTOCOL implementation. Uses the rng
> > +        device on the platform"
> > +
> >   endif
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index 7db4060..04dc864 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -42,3 +42,4 @@ obj-$(CONFIG_PARTITIONS) += efi_disk.o
> >   obj-$(CONFIG_NET) += efi_net.o
> >   obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
> >   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
> > +obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
> > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> > new file mode 100644
> > index 0000000..8456592
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_rng.c
> > @@ -0,0 +1,74 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright (c) 2019, Linaro Limited
> > + */
> > +
> > +#include <common.h>
> > +#include <dm.h>
> > +#include <efi_loader.h>
> > +#include <efi_rng.h>
> > +#include <rng.h>
> > +
> > +DECLARE_GLOBAL_DATA_PTR;
>
>
> Please, add platform_rng_get_info() in this file.
>

Like I had mentioned above, could there not be a case where different
platforms might implement different algorithms for providing the random
seed. The uefi spec gives a list of few of the algorithms that might be
used.


>
> > +
> > +static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
> > +                                         efi_uintn_t *rng_algo_size,
> > +                                         efi_rng_algorithm *rng_algo)
> > +{
>
> Please, call EFI_ENTRY here.
>

Ok. Will do.


>
> > +     if (!this || !rng_algo_size || !rng_algo)
>
> Please, allow RNGAlgorithmList == NULL here. The caller will typically
> call this function twice. The first call will inform the caller about
> the size of the buffer needed. The caller then allocates the buffer and
> calls the function again.
>

Ok. Will change. In fact, in case rng_algo is NULL, the rng_algo_size can
be set to reflect the size of the buffer needed, and the call then return
with EFI_BUFFER_TOO_SMALL. I guess that is what the spec requires.


> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     if (*rng_algo_size < sizeof(*rng_algo)) {
> > +             *rng_algo_size = sizeof(*rng_algo);
> > +             return EFI_BUFFER_TOO_SMALL;
> > +     }
> > +
> > +     *rng_algo_size = sizeof(*rng_algo);
> > +     platform_rng_getinfo(rng_algo);
>
> Having a platform_rng_get_info() function does not make much sense if we
> hard code the the size of the algorithm list above and the allowed
> algorithm below. So you can directly put your memcpy() here.
>

Ok. I think I now understand as to why were you asking me to move
platform_rng_get_info function into the efi_rng.c file.  Instead, i will
move the setting of the rng_algo_size to the platform code.


>
> If no UCLASS_RNG device is found, please, return EFI_UNSUPPORTED.
>
> Please, call EFI_EXIT here.
>

Ok.


>
> > +
> > +     return EFI_SUCCESS;
> > +}
> > +
> > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *protocol,
> > +                                    efi_rng_algorithm *rng_algo,
> > +                                    efi_uintn_t rng_len, uint8_t
> *rng_data)
> > +{
> > +     int ret;
> > +     struct udevice *dev;
> > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> > +
> > +     /*
> > +      * Booting into the linux kernel mangles
> > +      * x18, which holds the gd.
> > +      * When the efi stub in the kernel invokes
> > +      * the GetRng routine, gd needs to be
> > +      * restored back, without which bad things
> > +      * happen
> > +      */
> > +     efi_restore_gd();
>
> Please, use EFI_ENTRY() instead.
>

Ok. Will change.


>
> > +
> > +     if (!protocol || !rng_data || !rng_len)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     if (rng_algo) {
> > +             if (guidcmp(rng_algo, &rng_raw_guid))
> > +                     return EFI_UNSUPPORTED;
> > +     }
> > +
> > +     ret = platform_get_rng_device(&dev);
> > +     if (ret != EFI_SUCCESS)
> > +             return EFI_DEVICE_ERROR;
>
> I think this should be EFI_UNSUPPORTED.
>

Ok. Will change.


>
> > +
> > +     ret = dm_rng_read(dev, rng_data, rng_len);
> > +     if (ret < 0) {
> > +             debug("Rng device read failed\n");
> > +             return EFI_DEVICE_ERROR;
> > +     }
> > +
>
> You have to call EFI_EXIT(EFI_SUCCESS) here.
>

Ok. Will change.

-sughosh

[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Heinrich Schuchardt]

On 12/25/19 7:21 AM, Sughosh Ganu wrote:
> hi Heinrich,
> Thanks for the review.
>
> On Tue, 24 Dec 2019 at 22:35, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 12/24/19 4:54 PM, Sughosh Ganu wrote:
>      > Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
>      > platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
>      > invoked by the efi stub in the kernel for getting random seed for
>      > kaslr.
>      >
>      > The routines are platform specific, and use the virtio-rng device on
>      > the platform to get random data.
>      >
>      > The feature can be enabled through the following config
>      > CONFIG_EFI_RNG_PROTOCOL
>      >
>      > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
>     <mailto:sughosh.ganu@linaro.org>>
>      > ---
>      >   board/emulation/qemu-arm/qemu-arm.c | 50 +++++++++++++++++++++++++
>      >   include/efi_rng.h                   | 34 +++++++++++++++++
>      >   lib/efi_loader/Kconfig              |  8 ++++
>      >   lib/efi_loader/Makefile             |  1 +
>      >   lib/efi_loader/efi_rng.c            | 74
>     +++++++++++++++++++++++++++++++++++++
>      >   5 files changed, 167 insertions(+)
>      >   create mode 100644 include/efi_rng.h
>      >   create mode 100644 lib/efi_loader/efi_rng.c
>      >
>      > diff --git a/board/emulation/qemu-arm/qemu-arm.c
>     b/board/emulation/qemu-arm/qemu-arm.c
>      > index e1f4709..3176421 100644
>      > --- a/board/emulation/qemu-arm/qemu-arm.c
>      > +++ b/board/emulation/qemu-arm/qemu-arm.c
>      > @@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
>      >       /* QEMU loads a generated DTB for us at the start of RAM. */
>      >       return (void *)CONFIG_SYS_SDRAM_BASE;
>      >   }
>      > +
>      > +#if defined(CONFIG_EFI_RNG_PROTOCOL)
>      > +#include <efi_loader.h>
>      > +#include <efi_rng.h>
>      > +
>      > +#include <dm/device-internal.h>
>      > +
>      > +#define VIRTIO_RNG_PCI_DEVICE        "virtio-pci.l#0"
>      > +
>      > +void platform_rng_getinfo(efi_rng_algorithm *rng_algo)
>
>     Thanks for working on the implementation of the EFI_RNG_PROTOCOL.
>
>     Please, put an underscore after each word: platform_rng_get_info
>
>
> Ok.
>
>
>      > +{
>      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     guidcpy(rng_algo, &rng_raw_guid);
>
>     This function should be in efi_rng.c if it is needed at all.
>
>
> Is the rng algorithm supported not platform specific. I believe
> different platforms might use different algorithms for providing the
> random seed.

Sure you may later want develop code implementing one of the other RNG
algorithms and than use a Kconfig setting to enable building the code
and calling it from the EFI_RNG_PROTOCOL driver. But this code will not
depend on whether you are using a specific board. A good place to put
the algorithms would be in lib/.

Have a look at EDK II's
SecurityPkg/RandomNumberGenerator/RngDxe/RdRand.c. That code does not
depend on anything from https://github.com/tianocore/edk2-platforms -
EDK II's analogue of our board/ and arch/ directories.

As platform_rng_getinfo() will only depend on Kconfig settings you can
put it into lib/efi_loader/efi_rng.c.

Best regards

Heinrich

>
>
>      > +}
>      > +
>      > +efi_status_t platform_get_rng_device(struct udevice **dev)
>      > +{
>
>     Here you are creating platform specific code. The idea of the driver
>     model in U-Boot is to separate duties.
>
>     So the implementation of the EFI_RNG_PROTOCOL should be platform
>     agnostic and rely simply on looping of the devices of UCLASS_RNG.
>
>
> The core implementation of efi_rng_protocol routines is indeed platform
> agnostic. I have just separated the method of getting the rng
> device(struct udevice), since different platforms might need different
> ways of getting the rng device. For example, on the qemu arm64 platform,
> the rng device is a child of a virtio device on the pci bus, while the
> rng module on the stm32mp1 platform is a direct memory mapped
> peripheral. So the mechanism used to fetch the rng udevice would be
> platform dependent. Hence the platform_get_rng_device function. In fact,
> I see this kind of method adopted in a lot many other boards, one
> example being board/CZ.NIC/turris_omnia/turris_omnia.c(get_atsha204a_dev).
>
>
>     Please, move function platform_get_rng_device into the RNG uclass.
>
>
>      > +     int ret;
>      > +     efi_status_t status = EFI_DEVICE_ERROR;
>      > +     struct udevice *bus, *devp;
>      > +
>      > +     for (uclass_first_device(UCLASS_VIRTIO, &bus); bus;
>      > +          uclass_next_device(&bus)) {
>      > +             for (device_find_first_child(bus, &devp); devp;
>     device_find_next_child(&devp)) {
>      > +                     if (device_get_uclass_id(devp) == UCLASS_RNG) {
>      > +                             *dev = devp;
>      > +                             status = EFI_SUCCESS;
>      > +                             break;
>      > +                     }
>      > +             }
>      > +     }
>      > +
>      > +     if (status != EFI_SUCCESS) {
>      > +             debug("No rng device found\n");
>      > +             return EFI_DEVICE_ERROR;
>      > +     }
>      > +
>      > +     if (*dev) {
>      > +             ret = device_probe(*dev);
>      > +             if (ret)
>      > +                     return EFI_DEVICE_ERROR;
>      > +     } else {
>      > +             debug("Couldn't get child device\n");
>      > +             return EFI_DEVICE_ERROR;
>      > +     }
>      > +
>      > +     return EFI_SUCCESS;
>      > +}
>      > +#endif /* CONFIG_EFI_RNG_PROTOCOL */
>      > diff --git a/include/efi_rng.h b/include/efi_rng.h
>      > new file mode 100644
>      > index 0000000..df749dd
>      > --- /dev/null
>      > +++ b/include/efi_rng.h
>      > @@ -0,0 +1,34 @@
>      > +// SPDX-License-Identifier: GPL-2.0+
>      > +/*
>      > + * Copyright (c) 2019, Linaro Limited
>      > + */
>      > +
>      > +#if !defined _EFI_RNG_H_
>      > +#define _EFI_RNG_H_
>      > +
>      > +#include <efi.h>
>      > +#include <efi_api.h>
>      > +
>      > +#define EFI_RNG_PROTOCOL_GUID \
>      > +     EFI_GUID(0x3152bca5, 0xeade, 0x433d, 0x86, 0x2e, \
>      > +              0xc0, 0x1c, 0xdc, 0x29, 0x1f, 0x44)
>      > +
>      > +#define EFI_RNG_ALGORITHM_RAW \
>      > +     EFI_GUID(0xe43176d7, 0xb6e8, 0x4827, 0xb7, 0x84, \
>      > +              0x7f, 0xfd, 0xc4, 0xb6, 0x85, 0x61)
>
>     Please, add a comment line for each of the GUIDs.
>
>
> Ok. Will do.
>
>
>      > +
>      > +typedef efi_guid_t efi_rng_algorithm;
>
>     We want to avoid typedefs in U-Boot.
>
>
> Ok. Will replace efi_rng_algorithm with efi_guid_t
>
>
>      > +
>      > +struct efi_rng_protocol {
>      > +     efi_status_t (EFIAPI *getinfo)(struct efi_rng_protocol *this,
>
>     get_info
>
>
> Ok.
>
>
>      > +                                    efi_uintn_t *rng_algo_size,
>
>     Please, use the parameter names from the UEFI 2.8 specification:
>
>     rng_algorithm_list_size
>
>      > +                                    efi_rng_algorithm *rng_algo);
>
>     rng_algorithm_list
>
>      > +     efi_status_t (EFIAPI *getrng)(struct efi_rng_protocol *this,
>
>     get_rng
>
>      > +                                   efi_rng_algorithm *rng_algo,
>
>     efi_guid_t *rng_algorithm
>
>      > +                                   efi_uintn_t rng_len, uint8_t
>     *rng_data);
>
>     rng_value_length, rng_value
>
>
> Ok. Will make the above suggested changes.
>
>      > +};
>      > +
>      > +void platform_rng_getinfo(efi_rng_algorithm *rng_algo);
>      > +efi_status_t platform_get_rng_device(struct udevice **dev);
>      > +
>      > +#endif /* _EFI_RNG_H_ */
>      > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
>      > index 21ef440..7437442 100644
>      > --- a/lib/efi_loader/Kconfig
>      > +++ b/lib/efi_loader/Kconfig
>      > @@ -120,4 +120,12 @@ config EFI_GRUB_ARM32_WORKAROUND
>      >         GRUB prior to version 2.04 requires U-Boot to disable
>     caches. This
>      >         workaround currently is also needed on systems with
>     caches that
>      >         cannot be managed via CP15.
>      > +
>      > +config EFI_RNG_PROTOCOL
>      > +     bool "EFI_RNG_PROTOCOL support"
>      > +     depends on DM_RNG && TARGET_QEMU_ARM_64BIT
>
>     DM_RNG should be enough.
>
>     Just return EFI_UNSUPPORTED from EFI_RNG_PROTOCOL if no device of the
>     RNG uclass is found.
>
>
> Ok.
>
>
>      > +     help
>      > +       "Support for EFI_RNG_PROTOCOL implementation. Uses the rng
>      > +        device on the platform"
>      > +
>      >   endif
>      > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
>      > index 7db4060..04dc864 100644
>      > --- a/lib/efi_loader/Makefile
>      > +++ b/lib/efi_loader/Makefile
>      > @@ -42,3 +42,4 @@ obj-$(CONFIG_PARTITIONS) += efi_disk.o
>      >   obj-$(CONFIG_NET) += efi_net.o
>      >   obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
>      >   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>      > +obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>      > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
>      > new file mode 100644
>      > index 0000000..8456592
>      > --- /dev/null
>      > +++ b/lib/efi_loader/efi_rng.c
>      > @@ -0,0 +1,74 @@
>      > +// SPDX-License-Identifier: GPL-2.0+
>      > +/*
>      > + * Copyright (c) 2019, Linaro Limited
>      > + */
>      > +
>      > +#include <common.h>
>      > +#include <dm.h>
>      > +#include <efi_loader.h>
>      > +#include <efi_rng.h>
>      > +#include <rng.h>
>      > +
>      > +DECLARE_GLOBAL_DATA_PTR;
>
>
>     Please, add platform_rng_get_info() in this file.
>
>
> Like I had mentioned above, could there not be a case where different
> platforms might implement different algorithms for providing the random
> seed. The uefi spec gives a list of few of the algorithms that might be
> used.
>
>
>      > +
>      > +static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol
>     *this,
>      > +                                         efi_uintn_t *rng_algo_size,
>      > +                                         efi_rng_algorithm
>     *rng_algo)
>      > +{
>
>     Please, call EFI_ENTRY here.
>
>
> Ok. Will do.
>
>
>      > +     if (!this || !rng_algo_size || !rng_algo)
>
>     Please, allow RNGAlgorithmList == NULL here. The caller will typically
>     call this function twice. The first call will inform the caller about
>     the size of the buffer needed. The caller then allocates the buffer and
>     calls the function again.
>
>
> Ok. Will change. In fact, in case rng_algo is NULL, the rng_algo_size
> can be set to reflect the size of the buffer needed, and the call then
> return with EFI_BUFFER_TOO_SMALL. I guess that is what the spec requires.
>
>
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (*rng_algo_size < sizeof(*rng_algo)) {
>      > +             *rng_algo_size = sizeof(*rng_algo);
>      > +             return EFI_BUFFER_TOO_SMALL;
>      > +     }
>      > +
>      > +     *rng_algo_size = sizeof(*rng_algo);
>      > +     platform_rng_getinfo(rng_algo);
>
>     Having a platform_rng_get_info() function does not make much sense if we
>     hard code the the size of the algorithm list above and the allowed
>     algorithm below. So you can directly put your memcpy() here.
>
>
> Ok. I think I now understand as to why were you asking me to move
> platform_rng_get_info function into the efi_rng.c file.  Instead, i will
> move the setting of the rng_algo_size to the platform code.
>
>
>     If no UCLASS_RNG device is found, please, return EFI_UNSUPPORTED.
>
>     Please, call EFI_EXIT here.
>
>
> Ok.
>
>
>      > +
>      > +     return EFI_SUCCESS;
>      > +}
>      > +
>      > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *protocol,
>      > +                                    efi_rng_algorithm *rng_algo,
>      > +                                    efi_uintn_t rng_len, uint8_t
>     *rng_data)
>      > +{
>      > +     int ret;
>      > +     struct udevice *dev;
>      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     /*
>      > +      * Booting into the linux kernel mangles
>      > +      * x18, which holds the gd.
>      > +      * When the efi stub in the kernel invokes
>      > +      * the GetRng routine, gd needs to be
>      > +      * restored back, without which bad things
>      > +      * happen
>      > +      */
>      > +     efi_restore_gd();
>
>     Please, use EFI_ENTRY() instead.
>
>
> Ok. Will change.
>
>
>      > +
>      > +     if (!protocol || !rng_data || !rng_len)
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (rng_algo) {
>      > +             if (guidcmp(rng_algo, &rng_raw_guid))
>      > +                     return EFI_UNSUPPORTED;
>      > +     }
>      > +
>      > +     ret = platform_get_rng_device(&dev);
>      > +     if (ret != EFI_SUCCESS)
>      > +             return EFI_DEVICE_ERROR;
>
>     I think this should be EFI_UNSUPPORTED.
>
>
> Ok. Will change.
>
>
>      > +
>      > +     ret = dm_rng_read(dev, rng_data, rng_len);
>      > +     if (ret < 0) {
>      > +             debug("Rng device read failed\n");
>      > +             return EFI_DEVICE_ERROR;
>      > +     }
>      > +
>
>     You have to call EFI_EXIT(EFI_SUCCESS) here.
>
>
> Ok. Will change.
>
> -sughosh

[PATCH 3/3] efi_rng_protocol: Install the efi_rng_protocol on the root node

[Heinrich Schuchardt]

On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> Install the EFI_RNG_PROTOCOL implementation for it's subsequent use by
> the kernel for features like kaslr.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   include/efi_loader.h           | 4 ++++
>   lib/efi_loader/efi_rng.c       | 2 ++
>   lib/efi_loader/efi_root_node.c | 4 ++++
>   3 files changed, 10 insertions(+)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index bec7873..71996ec 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -130,6 +130,7 @@ extern const struct efi_hii_config_routing_protocol efi_hii_config_routing;
>   extern const struct efi_hii_config_access_protocol efi_hii_config_access;
>   extern const struct efi_hii_database_protocol efi_hii_database;
>   extern const struct efi_hii_string_protocol efi_hii_string;
> +extern const struct efi_rng_protocol efi_rng_protocol_ops;

Could we, please, call this variable efi_rng_protocol.

Otherwise

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
>   uint16_t *efi_dp_str(struct efi_device_path *dp);
>
> @@ -175,6 +176,9 @@ extern const efi_guid_t efi_guid_hii_config_access_protocol;
>   extern const efi_guid_t efi_guid_hii_database_protocol;
>   extern const efi_guid_t efi_guid_hii_string_protocol;
>
> +/* GUID of RNG protocol */
> +extern const efi_guid_t efi_guid_rng_protocol;
> +
>   extern unsigned int __efi_runtime_start, __efi_runtime_stop;
>   extern unsigned int __efi_runtime_rel_start, __efi_runtime_rel_stop;
>
> diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> index 8456592..80c8fc9 100644
> --- a/lib/efi_loader/efi_rng.c
> +++ b/lib/efi_loader/efi_rng.c
> @@ -11,6 +11,8 @@
>
>   DECLARE_GLOBAL_DATA_PTR;
>
> +const efi_guid_t efi_guid_rng_protocol = EFI_RNG_PROTOCOL_GUID;
> +
>   static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
>   					    efi_uintn_t *rng_algo_size,
>   					    efi_rng_algorithm *rng_algo)
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index f68b0fd..3d937ce 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
> @@ -81,6 +81,10 @@ efi_status_t efi_root_node_register(void)
>   			 &efi_guid_hii_config_routing_protocol,
>   			 (void *)&efi_hii_config_routing,
>   #endif
> +#if CONFIG_IS_ENABLED(EFI_RNG_PROTOCOL)
> +			 &efi_guid_rng_protocol,
> +			 (void *)&efi_rng_protocol_ops,
> +#endif
>   			 NULL));
>   	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
>   	return ret;
>

[PATCH 3/3] efi_rng_protocol: Install the efi_rng_protocol on the root node

[Sughosh Ganu]

On Wed, 25 Dec 2019 at 13:57, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> > Install the EFI_RNG_PROTOCOL implementation for it's subsequent use by
> > the kernel for features like kaslr.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   include/efi_loader.h           | 4 ++++
> >   lib/efi_loader/efi_rng.c       | 2 ++
> >   lib/efi_loader/efi_root_node.c | 4 ++++
> >   3 files changed, 10 insertions(+)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index bec7873..71996ec 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -130,6 +130,7 @@ extern const struct efi_hii_config_routing_protocol
> efi_hii_config_routing;
> >   extern const struct efi_hii_config_access_protocol
> efi_hii_config_access;
> >   extern const struct efi_hii_database_protocol efi_hii_database;
> >   extern const struct efi_hii_string_protocol efi_hii_string;
> > +extern const struct efi_rng_protocol efi_rng_protocol_ops;
>
> Could we, please, call this variable efi_rng_protocol.
>
> Otherwise
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>

Will change the name of the symbol as suggested. Thanks.

-sughosh

[PATCH 2/3] efi: qemu: arm64: Add efi_rng_protocol implementation for the platform

[Sughosh Ganu]

On Wed, 25 Dec 2019 at 13:51, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/25/19 7:21 AM, Sughosh Ganu wrote:
> > hi Heinrich,
> > Thanks for the review.
> >
> > On Tue, 24 Dec 2019 at 22:35, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     On 12/24/19 4:54 PM, Sughosh Ganu wrote:
> >      > Add support for the EFI_RNG_PROTOCOL routines for the qemu arm64
> >      > platform. EFI_RNG_PROTOCOL is an uefi boottime service which is
> >      > invoked by the efi stub in the kernel for getting random seed for
> >      > kaslr.
> >      >
> >      > The routines are platform specific, and use the virtio-rng device
> on
> >      > the platform to get random data.
> >      >
> >      > The feature can be enabled through the following config
> >      > CONFIG_EFI_RNG_PROTOCOL
> >      >
> >      > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org
> >     <mailto:sughosh.ganu@linaro.org>>
> >      > ---
> >      >   board/emulation/qemu-arm/qemu-arm.c | 50
> +++++++++++++++++++++++++
> >      >   include/efi_rng.h                   | 34 +++++++++++++++++
> >      >   lib/efi_loader/Kconfig              |  8 ++++
> >      >   lib/efi_loader/Makefile             |  1 +
> >      >   lib/efi_loader/efi_rng.c            | 74
> >     +++++++++++++++++++++++++++++++++++++
> >      >   5 files changed, 167 insertions(+)
> >      >   create mode 100644 include/efi_rng.h
> >      >   create mode 100644 lib/efi_loader/efi_rng.c
> >      >
> >      > diff --git a/board/emulation/qemu-arm/qemu-arm.c
> >     b/board/emulation/qemu-arm/qemu-arm.c
> >      > index e1f4709..3176421 100644
> >      > --- a/board/emulation/qemu-arm/qemu-arm.c
> >      > +++ b/board/emulation/qemu-arm/qemu-arm.c
> >      > @@ -91,3 +91,53 @@ void *board_fdt_blob_setup(void)
> >      >       /* QEMU loads a generated DTB for us at the start of RAM. */
> >      >       return (void *)CONFIG_SYS_SDRAM_BASE;
> >      >   }
> >      > +
> >      > +#if defined(CONFIG_EFI_RNG_PROTOCOL)
> >      > +#include <efi_loader.h>
> >      > +#include <efi_rng.h>
> >      > +
> >      > +#include <dm/device-internal.h>
> >      > +
> >      > +#define VIRTIO_RNG_PCI_DEVICE        "virtio-pci.l#0"
> >      > +
> >      > +void platform_rng_getinfo(efi_rng_algorithm *rng_algo)
> >
> >     Thanks for working on the implementation of the EFI_RNG_PROTOCOL.
> >
> >     Please, put an underscore after each word: platform_rng_get_info
> >
> >
> > Ok.
> >
> >
> >      > +{
> >      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> >      > +
> >      > +     guidcpy(rng_algo, &rng_raw_guid);
> >
> >     This function should be in efi_rng.c if it is needed at all.
> >
> >
> > Is the rng algorithm supported not platform specific. I believe
> > different platforms might use different algorithms for providing the
> > random seed.
>
> Sure you may later want develop code implementing one of the other RNG
> algorithms and than use a Kconfig setting to enable building the code
> and calling it from the EFI_RNG_PROTOCOL driver. But this code will not
> depend on whether you are using a specific board. A good place to put
> the algorithms would be in lib/.
>

Ok. So I guess what you are suggesting is returning the algorithm in
get_info based on what is enabled through Kconfig.

-sughosh