[PATCH v3 0/3] Add support for efi_rng_protocol

[PATCH v3 0/3] 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/394654.html

Changes since V2:
* Based on review comments from Heinrich Schuchardt, the rng drivers
  read all the bytes requested in the individual
  drivers. Corresponding changes made in getrng routine to remove the
  loop to read the bytes requested, since that would be handled in the
  drivers.

Changes since V1:
* Handle review comments from Heinrich Schuchardt.
* Change the logic in the getrng routine to implement a loop to read
  the number of bytes requested. This change is needed to handle the
  change in the rng uclass's read function, which now returns the
  number of bytes read, which might be less than the number of bytes
  requested.


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 | 41 +++++++++++++++++++
 include/efi_loader.h                |  9 ++++
 include/efi_rng.h                   | 32 +++++++++++++++
 lib/efi_loader/Kconfig              |  8 ++++
 lib/efi_loader/Makefile             |  1 +
 lib/efi_loader/efi_boottime.c       |  4 +-
 lib/efi_loader/efi_rng.c            | 82 +++++++++++++++++++++++++++++++++++++
 lib/efi_loader/efi_root_node.c      |  4 ++
 8 files changed, 179 insertions(+), 2 deletions(-)
 create mode 100644 include/efi_rng.h
 create mode 100644 lib/efi_loader/efi_rng.c

-- 
2.7.4

[PATCH v3 1/3] efi_loader: Add guidcpy function

[Sughosh Ganu]

Add guidcpy function to copy the source guid to the destination
guid. Use this function instead of memcpy for copying to the
destination guid.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          | 5 +++++
 lib/efi_loader/efi_boottime.c | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

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)
 
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 88a7604..3103a50 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1401,7 +1401,7 @@ static efi_status_t EFIAPI efi_register_protocol_notify(
 	}
 
 	item->event = event;
-	memcpy(&item->protocol, protocol, sizeof(efi_guid_t));
+	guidcpy(&item->protocol, protocol);
 	INIT_LIST_HEAD(&item->handles);
 
 	list_add_tail(&item->link, &efi_register_notify_events);
@@ -1632,7 +1632,7 @@ efi_status_t efi_install_configuration_table(const efi_guid_t *guid,
 		return EFI_OUT_OF_RESOURCES;
 
 	/* Add a new entry */
-	memcpy(&systab.tables[i].guid, guid, sizeof(*guid));
+	guidcpy(&systab.tables[i].guid, guid);
 	systab.tables[i].table = table;
 	systab.nr_tables = i + 1;
 
-- 
2.7.4

[PATCH v3 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>
---
Changes since V2:
* Based on review comments from Heinrich Schuchardt, the rng drivers
  read all the bytes requested in the individual
  drivers. Corresponding changes made in getrng routine to remove the
  loop to read the bytes requested, since that would be handled in the
  drivers.

 board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
 include/efi_rng.h                   | 32 +++++++++++++++
 lib/efi_loader/Kconfig              |  8 ++++
 lib/efi_loader/Makefile             |  1 +
 lib/efi_loader/efi_rng.c            | 80 +++++++++++++++++++++++++++++++++++++
 5 files changed, 162 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..1dcf830 100644
--- a/board/emulation/qemu-arm/qemu-arm.c
+++ b/board/emulation/qemu-arm/qemu-arm.c
@@ -91,3 +91,44 @@ 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>
+
+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..a46e66d
--- /dev/null
+++ b/include/efi_rng.h
@@ -0,0 +1,32 @@
+// 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>
+
+/* EFI random number generation protocol related GUID definitions */
+#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)
+
+struct efi_rng_protocol {
+	efi_status_t (EFIAPI *get_info)(struct efi_rng_protocol *protocol,
+					efi_uintn_t *rng_algorithm_list_size,
+					efi_guid_t *rng_algorithm_list);
+	efi_status_t (EFIAPI *get_rng)(struct efi_rng_protocol *protocol,
+				       efi_guid_t *rng_algorithm,
+				       efi_uintn_t rng_value_length, uint8_t *rng_value);
+};
+
+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..24dde6f 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
+	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..eb91aa7
--- /dev/null
+++ b/lib/efi_loader/efi_rng.c
@@ -0,0 +1,80 @@
+// 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_algorithm_list_size,
+				       efi_guid_t *rng_algorithm_list)
+{
+	efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
+
+	EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
+		  rng_algorithm_list);
+
+	if (!this || !rng_algorithm_list_size)
+		return EFI_INVALID_PARAMETER;
+
+	if (!rng_algorithm_list ||
+	    *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
+		*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
+		return EFI_BUFFER_TOO_SMALL;
+	}
+
+	/*
+	 * For now, use EFI_RNG_ALGORITHM_RAW as the default
+	 * algorithm. If a new algorithm gets added in the
+	 * future through a Kconfig, rng_algo_guid will be set
+	 * based on that Kconfig option
+	 */
+	*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
+	guidcpy(rng_algorithm_list, &rng_algo_guid);
+
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
+static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
+				  efi_guid_t *rng_algorithm,
+				  efi_uintn_t rng_value_length,
+				  uint8_t *rng_value)
+{
+	int ret;
+	struct udevice *dev;
+	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
+
+	EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm, rng_value_length,
+		  rng_value);
+
+	if (!this || !rng_value || !rng_value_length)
+		return EFI_INVALID_PARAMETER;
+
+	if (rng_algorithm) {
+		if (guidcmp(rng_algorithm, &rng_raw_guid))
+			return EFI_UNSUPPORTED;
+	}
+
+	ret = platform_get_rng_device(&dev);
+	if (ret != EFI_SUCCESS)
+		return EFI_UNSUPPORTED;
+
+	ret = dm_rng_read(dev, rng_value, rng_value_length);
+	if (ret < 0) {
+		debug("Rng device read failed\n");
+		return EFI_DEVICE_ERROR;
+	}
+
+	return EFI_EXIT(EFI_SUCCESS);
+}
+
+const struct efi_rng_protocol efi_rng_protocol = {
+	.get_info = rng_getinfo,
+	.get_rng = getrng,
+};
-- 
2.7.4

[PATCH v3 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>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 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..bfcfa74 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;
 
 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 eb91aa7..982fc1e 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_algorithm_list_size,
 				       efi_guid_t *rng_algorithm_list)
diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
index f68b0fd..76d18fb 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,
+#endif
 			 NULL));
 	efi_root->type = EFI_OBJECT_TYPE_U_BOOT_FIRMWARE;
 	return ret;
-- 
2.7.4

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

[Heinrich Schuchardt]

On 12/27/19 3:26 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>
> ---
> Changes since V2:
> * Based on review comments from Heinrich Schuchardt, the rng drivers
>    read all the bytes requested in the individual
>    drivers. Corresponding changes made in getrng routine to remove the
>    loop to read the bytes requested, since that would be handled in the
>    drivers.
>
>   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
>   include/efi_rng.h                   | 32 +++++++++++++++
>   lib/efi_loader/Kconfig              |  8 ++++
>   lib/efi_loader/Makefile             |  1 +
>   lib/efi_loader/efi_rng.c            | 80 +++++++++++++++++++++++++++++++++++++
>   5 files changed, 162 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..1dcf830 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -91,3 +91,44 @@ 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>
> +
> +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..a46e66d
> --- /dev/null
> +++ b/include/efi_rng.h
> @@ -0,0 +1,32 @@
> +// 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>
> +
> +/* EFI random number generation protocol related GUID definitions */
> +#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)
> +
> +struct efi_rng_protocol {
> +	efi_status_t (EFIAPI *get_info)(struct efi_rng_protocol *protocol,
> +					efi_uintn_t *rng_algorithm_list_size,
> +					efi_guid_t *rng_algorithm_list);
> +	efi_status_t (EFIAPI *get_rng)(struct efi_rng_protocol *protocol,
> +				       efi_guid_t *rng_algorithm,
> +				       efi_uintn_t rng_value_length, uint8_t *rng_value);
> +};
> +
> +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..24dde6f 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
> +	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..eb91aa7
> --- /dev/null
> +++ b/lib/efi_loader/efi_rng.c
> @@ -0,0 +1,80 @@
> +// 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_algorithm_list_size,
> +				       efi_guid_t *rng_algorithm_list)
> +{
> +	efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
> +		  rng_algorithm_list);
> +
> +	if (!this || !rng_algorithm_list_size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (!rng_algorithm_list ||
> +	    *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
> +		*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> +		return EFI_BUFFER_TOO_SMALL;
> +	}
> +
> +	/*
> +	 * For now, use EFI_RNG_ALGORITHM_RAW as the default
> +	 * algorithm. If a new algorithm gets added in the
> +	 * future through a Kconfig, rng_algo_guid will be set
> +	 * based on that Kconfig option
> +	 */
> +	*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> +	guidcpy(rng_algorithm_list, &rng_algo_guid);
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
> +				  efi_guid_t *rng_algorithm,
> +				  efi_uintn_t rng_value_length,
> +				  uint8_t *rng_value)
> +{
> +	int ret;
> +	struct udevice *dev;
> +	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm, rng_value_length,
> +		  rng_value);
> +
> +	if (!this || !rng_value || !rng_value_length)
> +		return EFI_INVALID_PARAMETER;
> +
> +	if (rng_algorithm) {
> +		if (guidcmp(rng_algorithm, &rng_raw_guid))
> +			return EFI_UNSUPPORTED;
> +	}
> +
> +	ret = platform_get_rng_device(&dev);

This does not compile for sandbox_defconfig.

You could replace this by:

ret = uclass_get_device(UCLASS_RNG, 0, &dev);

Best regards

Heinrich

> +	if (ret != EFI_SUCCESS)
> +		return EFI_UNSUPPORTED;
> +
> +	ret = dm_rng_read(dev, rng_value, rng_value_length);
> +	if (ret < 0) {
> +		debug("Rng device read failed\n");
> +		return EFI_DEVICE_ERROR;
> +	}
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +const struct efi_rng_protocol efi_rng_protocol = {
> +	.get_info = rng_getinfo,
> +	.get_rng = getrng,
> +};
>

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

[Sughosh Ganu]

On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/27/19 3:26 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>
> > ---
> > Changes since V2:
> > * Based on review comments from Heinrich Schuchardt, the rng drivers
> >    read all the bytes requested in the individual
> >    drivers. Corresponding changes made in getrng routine to remove the
> >    loop to read the bytes requested, since that would be handled in the
> >    drivers.
> >
> >   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
> >   include/efi_rng.h                   | 32 +++++++++++++++
> >   lib/efi_loader/Kconfig              |  8 ++++
> >   lib/efi_loader/Makefile             |  1 +
> >   lib/efi_loader/efi_rng.c            | 80
> +++++++++++++++++++++++++++++++++++++
> >   5 files changed, 162 insertions(+)
> >   create mode 100644 include/efi_rng.h
> >   create mode 100644 lib/efi_loader/efi_rng.c
> >
>

<snip>


> > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> > new file mode 100644
> > index 0000000..eb91aa7
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_rng.c
> > @@ -0,0 +1,80 @@
> > +// 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_algorithm_list_size,
> > +                                    efi_guid_t *rng_algorithm_list)
> > +{
> > +     efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
> > +
> > +     EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
> > +               rng_algorithm_list);
> > +
> > +     if (!this || !rng_algorithm_list_size)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     if (!rng_algorithm_list ||
> > +         *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
> > +             *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> > +             return EFI_BUFFER_TOO_SMALL;
> > +     }
> > +
> > +     /*
> > +      * For now, use EFI_RNG_ALGORITHM_RAW as the default
> > +      * algorithm. If a new algorithm gets added in the
> > +      * future through a Kconfig, rng_algo_guid will be set
> > +      * based on that Kconfig option
> > +      */
> > +     *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> > +     guidcpy(rng_algorithm_list, &rng_algo_guid);
> > +
> > +     return EFI_EXIT(EFI_SUCCESS);
> > +}
> > +
> > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
> > +                               efi_guid_t *rng_algorithm,
> > +                               efi_uintn_t rng_value_length,
> > +                               uint8_t *rng_value)
> > +{
> > +     int ret;
> > +     struct udevice *dev;
> > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> > +
> > +     EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm, rng_value_length,
> > +               rng_value);
> > +
> > +     if (!this || !rng_value || !rng_value_length)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     if (rng_algorithm) {
> > +             if (guidcmp(rng_algorithm, &rng_raw_guid))
> > +                     return EFI_UNSUPPORTED;
> > +     }
> > +
> > +     ret = platform_get_rng_device(&dev);
>
> This does not compile for sandbox_defconfig.
>
> You could replace this by:
>
> ret = uclass_get_device(UCLASS_RNG, 0, &dev);
>

Like I had stated in one of my earlier mail, I would prefer having a
platform specific routine for fetching the rng device. For example, on
qemu, where the rng device is the child of a virtio-pci device, the above
logic would not get the rng device without having previously scanned the
virtio devices. Instead, i will add a weak function
platform_get_rng_device, which uses uclass_get_device. Any platform which
has a different topology, can then define it's own platform_get_rng_device
function.

-sughosh

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

[Heinrich Schuchardt]

On 12/28/19 4:03 PM, Sughosh Ganu wrote:
>
> On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk@gmx.de
> <mailto:xypron.glpk@gmx.de>> wrote:
>
>     On 12/27/19 3:26 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>>
>      > ---
>      > Changes since V2:
>      > * Based on review comments from Heinrich Schuchardt, the rng drivers
>      >    read all the bytes requested in the individual
>      >    drivers. Corresponding changes made in getrng routine to
>     remove the
>      >    loop to read the bytes requested, since that would be handled
>     in the
>      >    drivers.
>      >
>      >   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
>      >   include/efi_rng.h                   | 32 +++++++++++++++
>      >   lib/efi_loader/Kconfig              |  8 ++++
>      >   lib/efi_loader/Makefile             |  1 +
>      >   lib/efi_loader/efi_rng.c            | 80
>     +++++++++++++++++++++++++++++++++++++
>      >   5 files changed, 162 insertions(+)
>      >   create mode 100644 include/efi_rng.h
>      >   create mode 100644 lib/efi_loader/efi_rng.c
>      >
>
>
> <snip>
>
>      > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
>      > new file mode 100644
>      > index 0000000..eb91aa7
>      > --- /dev/null
>      > +++ b/lib/efi_loader/efi_rng.c
>      > @@ -0,0 +1,80 @@
>      > +// 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_algorithm_list_size,
>      > +                                    efi_guid_t *rng_algorithm_list)
>      > +{
>      > +     efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
>      > +               rng_algorithm_list);
>      > +
>      > +     if (!this || !rng_algorithm_list_size)
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (!rng_algorithm_list ||
>      > +         *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
>      > +             *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
>      > +             return EFI_BUFFER_TOO_SMALL;
>      > +     }
>      > +
>      > +     /*
>      > +      * For now, use EFI_RNG_ALGORITHM_RAW as the default
>      > +      * algorithm. If a new algorithm gets added in the
>      > +      * future through a Kconfig, rng_algo_guid will be set
>      > +      * based on that Kconfig option
>      > +      */
>      > +     *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
>      > +     guidcpy(rng_algorithm_list, &rng_algo_guid);
>      > +
>      > +     return EFI_EXIT(EFI_SUCCESS);
>      > +}
>      > +
>      > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
>      > +                               efi_guid_t *rng_algorithm,
>      > +                               efi_uintn_t rng_value_length,
>      > +                               uint8_t *rng_value)
>      > +{
>      > +     int ret;
>      > +     struct udevice *dev;
>      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
>      > +
>      > +     EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm,
>     rng_value_length,
>      > +               rng_value);
>      > +
>      > +     if (!this || !rng_value || !rng_value_length)
>      > +             return EFI_INVALID_PARAMETER;
>      > +
>      > +     if (rng_algorithm) {
>      > +             if (guidcmp(rng_algorithm, &rng_raw_guid))
>      > +                     return EFI_UNSUPPORTED;
>      > +     }
>      > +
>      > +     ret = platform_get_rng_device(&dev);
>
>     This does not compile for sandbox_defconfig.
>
>     You could replace this by:
>
>     ret = uclass_get_device(UCLASS_RNG, 0, &dev);
>
>
> Like I had stated in one of my earlier mail, I would prefer having a
> platform specific routine for fetching the rng device. For example, on
> qemu, where the rng device is the child of a virtio-pci device, the
> above logic would not get the rng device without having previously
> scanned the virtio devices. Instead, i will add a weak function
> platform_get_rng_device, which uses uclass_get_device. Any platform
> which has a different topology, can then define it's own
> platform_get_rng_device function.
>
> -sughosh

For patches series the expectation is that the code compiles when
stopping after any patch in the series.

Adding the uclass_get_device() solution in a weak function is ok for me.

When testing I already experienced that I had to issue the `virtio scan`
command.

Best regards

Heinrich

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

[Sughosh Ganu]

On Sat, 28 Dec 2019 at 20:54, Heinrich Schuchardt <xypron.glpk@gmx.de>
wrote:

> On 12/28/19 4:03 PM, Sughosh Ganu wrote:
> >
> > On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     On 12/27/19 3:26 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>>
> >      > ---
> >      > Changes since V2:
> >      > * Based on review comments from Heinrich Schuchardt, the rng
> drivers
> >      >    read all the bytes requested in the individual
> >      >    drivers. Corresponding changes made in getrng routine to
> >     remove the
> >      >    loop to read the bytes requested, since that would be handled
> >     in the
> >      >    drivers.
> >      >
> >      >   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
> >      >   include/efi_rng.h                   | 32 +++++++++++++++
> >      >   lib/efi_loader/Kconfig              |  8 ++++
> >      >   lib/efi_loader/Makefile             |  1 +
> >      >   lib/efi_loader/efi_rng.c            | 80
> >     +++++++++++++++++++++++++++++++++++++
> >      >   5 files changed, 162 insertions(+)
> >      >   create mode 100644 include/efi_rng.h
> >      >   create mode 100644 lib/efi_loader/efi_rng.c
> >      >
> >
> >
> > <snip>
> >
> >      > diff --git a/lib/efi_loader/efi_rng.c b/lib/efi_loader/efi_rng.c
> >      > new file mode 100644
> >      > index 0000000..eb91aa7
> >      > --- /dev/null
> >      > +++ b/lib/efi_loader/efi_rng.c
> >      > @@ -0,0 +1,80 @@
> >      > +// 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_algorithm_list_size,
> >      > +                                    efi_guid_t
> *rng_algorithm_list)
> >      > +{
> >      > +     efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
> >      > +
> >      > +     EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
> >      > +               rng_algorithm_list);
> >      > +
> >      > +     if (!this || !rng_algorithm_list_size)
> >      > +             return EFI_INVALID_PARAMETER;
> >      > +
> >      > +     if (!rng_algorithm_list ||
> >      > +         *rng_algorithm_list_size < sizeof(*rng_algorithm_list))
> {
> >      > +             *rng_algorithm_list_size =
> sizeof(*rng_algorithm_list);
> >      > +             return EFI_BUFFER_TOO_SMALL;
> >      > +     }
> >      > +
> >      > +     /*
> >      > +      * For now, use EFI_RNG_ALGORITHM_RAW as the default
> >      > +      * algorithm. If a new algorithm gets added in the
> >      > +      * future through a Kconfig, rng_algo_guid will be set
> >      > +      * based on that Kconfig option
> >      > +      */
> >      > +     *rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> >      > +     guidcpy(rng_algorithm_list, &rng_algo_guid);
> >      > +
> >      > +     return EFI_EXIT(EFI_SUCCESS);
> >      > +}
> >      > +
> >      > +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
> >      > +                               efi_guid_t *rng_algorithm,
> >      > +                               efi_uintn_t rng_value_length,
> >      > +                               uint8_t *rng_value)
> >      > +{
> >      > +     int ret;
> >      > +     struct udevice *dev;
> >      > +     const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> >      > +
> >      > +     EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm,
> >     rng_value_length,
> >      > +               rng_value);
> >      > +
> >      > +     if (!this || !rng_value || !rng_value_length)
> >      > +             return EFI_INVALID_PARAMETER;
> >      > +
> >      > +     if (rng_algorithm) {
> >      > +             if (guidcmp(rng_algorithm, &rng_raw_guid))
> >      > +                     return EFI_UNSUPPORTED;
> >      > +     }
> >      > +
> >      > +     ret = platform_get_rng_device(&dev);
> >
> >     This does not compile for sandbox_defconfig.
> >
> >     You could replace this by:
> >
> >     ret = uclass_get_device(UCLASS_RNG, 0, &dev);
> >
> >
> > Like I had stated in one of my earlier mail, I would prefer having a
> > platform specific routine for fetching the rng device. For example, on
> > qemu, where the rng device is the child of a virtio-pci device, the
> > above logic would not get the rng device without having previously
> > scanned the virtio devices. Instead, i will add a weak function
> > platform_get_rng_device, which uses uclass_get_device. Any platform
> > which has a different topology, can then define it's own
> > platform_get_rng_device function.
> >
> > -sughosh
>
> For patches series the expectation is that the code compiles when
> stopping after any patch in the series.
>

I do check the build of individual commits before sending out the patches,
although I might have missed that while sending some version. Did you
encounter any build error on a commit. Btw, the error you got for sandbox
was expected, since the patch series only adds support for the qemu arm64
platform. I will send a follow-up series which enables this for sandbox as
well.

-sughosh

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

[Tuomas Tynkkynen]

Hi,

On Sat, 28 Dec 2019 at 17:19, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 12/28/19 4:03 PM, Sughosh Ganu wrote:
> >
> > On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk@gmx.de
> > <mailto:xypron.glpk@gmx.de>> wrote:
> >
> >     On 12/27/19 3:26 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>>
...
>
> Adding the uclass_get_device() solution in a weak function is ok for me.
>
> When testing I already experienced that I had to issue the `virtio scan`
> command.
>

But in board_init() in qemu-arm.c we already call virtio_init(). If that is not
working correctly, there is a bug that should be fixed instead.

Maybe the problem is that PCI bus has not been scanned at the time of
that virtio_init() call (so everything would work correctly with virtio-mmio
but virtio-pci is broken)?

[PATCH v3 1/3] efi_loader: Add guidcpy function

[Heinrich Schuchardt]

On 12/27/19 3:26 PM, Sughosh Ganu wrote:
> Add guidcpy function to copy the source guid to the destination
> guid. Use this function instead of memcpy for copying to the
> destination guid.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>   include/efi_loader.h          | 5 +++++
>   lib/efi_loader/efi_boottime.c | 4 ++--
>   2 files changed, 7 insertions(+), 2 deletions(-)
>
> 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)
>
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 88a7604..3103a50 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1401,7 +1401,7 @@ static efi_status_t EFIAPI efi_register_protocol_notify(
>   	}
>
>   	item->event = event;
> -	memcpy(&item->protocol, protocol, sizeof(efi_guid_t));
> +	guidcpy(&item->protocol, protocol);

This gives me an error with GCC 9:

lib/efi_loader/efi_boottime.c: In function
‘efi_install_configuration_table’:
lib/efi_loader/efi_boottime.c:1635:10: error: taking address of packed
member of ‘struct efi_configuration_table’ may result in an unaligned
pointer value [-Werror=address-of-packed-member]
  1635 |  guidcpy(&systab.tables[i].guid, guid);
       |          ^~~~~~~~~~~~~~~~~~~~~~

efi_configuration_table is naturally packed. There is not reason to have
a __packed attribute. I will provide a patch to remove the __packed
attribute.

So nothing to change in your patch.

Best regards

Heinrich

>   	INIT_LIST_HEAD(&item->handles);
>
>   	list_add_tail(&item->link, &efi_register_notify_events);
> @@ -1632,7 +1632,7 @@ efi_status_t efi_install_configuration_table(const efi_guid_t *guid,
>   		return EFI_OUT_OF_RESOURCES;
>
>   	/* Add a new entry */
> -	memcpy(&systab.tables[i].guid, guid, sizeof(*guid));
> +	guidcpy(&systab.tables[i].guid, guid);
>   	systab.tables[i].table = table;
>   	systab.nr_tables = i + 1;
>
>

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

[Heinrich Schuchardt]

On 12/27/19 3:26 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>
> ---
> Changes since V2:
> * Based on review comments from Heinrich Schuchardt, the rng drivers
>    read all the bytes requested in the individual
>    drivers. Corresponding changes made in getrng routine to remove the
>    loop to read the bytes requested, since that would be handled in the
>    drivers.
>
>   board/emulation/qemu-arm/qemu-arm.c | 41 +++++++++++++++++++
>   include/efi_rng.h                   | 32 +++++++++++++++
>   lib/efi_loader/Kconfig              |  8 ++++
>   lib/efi_loader/Makefile             |  1 +
>   lib/efi_loader/efi_rng.c            | 80 +++++++++++++++++++++++++++++++++++++
>   5 files changed, 162 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..1dcf830 100644
> --- a/board/emulation/qemu-arm/qemu-arm.c
> +++ b/board/emulation/qemu-arm/qemu-arm.c
> @@ -91,3 +91,44 @@ 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>
> +
> +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..a46e66d
> --- /dev/null
> +++ b/include/efi_rng.h
> @@ -0,0 +1,32 @@
> +// 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>
> +
> +/* EFI random number generation protocol related GUID definitions */
> +#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)
> +
> +struct efi_rng_protocol {
> +	efi_status_t (EFIAPI *get_info)(struct efi_rng_protocol *protocol,
> +					efi_uintn_t *rng_algorithm_list_size,
> +					efi_guid_t *rng_algorithm_list);
> +	efi_status_t (EFIAPI *get_rng)(struct efi_rng_protocol *protocol,
> +				       efi_guid_t *rng_algorithm,
> +				       efi_uintn_t rng_value_length, uint8_t *rng_value);
> +};
> +
> +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..24dde6f 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
> +	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..eb91aa7
> --- /dev/null
> +++ b/lib/efi_loader/efi_rng.c
> @@ -0,0 +1,80 @@
> +// 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_algorithm_list_size,
> +				       efi_guid_t *rng_algorithm_list)
> +{
> +	efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
> +		  rng_algorithm_list);
> +
> +	if (!this || !rng_algorithm_list_size)
> +		return EFI_INVALID_PARAMETER;

You have to call EFI_EXIT() when returning to restore the gd register.

> +
> +	if (!rng_algorithm_list ||
> +	    *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
> +		*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> +		return EFI_BUFFER_TOO_SMALL;

EFI_EXIT()

> +	}
> +
> +	/*
> +	 * For now, use EFI_RNG_ALGORITHM_RAW as the default
> +	 * algorithm. If a new algorithm gets added in the
> +	 * future through a Kconfig, rng_algo_guid will be set
> +	 * based on that Kconfig option
> +	 */
> +	*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
> +	guidcpy(rng_algorithm_list, &rng_algo_guid);
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
> +				  efi_guid_t *rng_algorithm,
> +				  efi_uintn_t rng_value_length,
> +				  uint8_t *rng_value)
> +{
> +	int ret;
> +	struct udevice *dev;
> +	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;
> +
> +	EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm, rng_value_length,
> +		  rng_value);
> +
> +	if (!this || !rng_value || !rng_value_length)
> +		return EFI_INVALID_PARAMETER;

You have to call EFI_EXIT() when returning to get the gd register restored.

In most other protocol implementations we have here just

			ret = EFI_INVALID_PARAMETER;
			goto out;

And a label at the end of the code.

> +
> +	if (rng_algorithm) {
> +		if (guidcmp(rng_algorithm, &rng_raw_guid))

Let's have some debug output here:

EFI_PRINT("RNG algorithm %pUl\n", rng_algorthm);

> +			return EFI_UNSUPPORTED;

EFI_EXIT()

> +	}
> +
> +	ret = platform_get_rng_device(&dev);
> +	if (ret != EFI_SUCCESS)

EFI_SUCCESS is what we use for return values of type efi_status_t. It is
misleading here.

> +		return EFI_UNSUPPORTED;

EFI_EXIT()

> +
> +	ret = dm_rng_read(dev, rng_value, rng_value_length);
> +	if (ret < 0) {
> +		debug("Rng device read failed\n");

It is preferable to use EFI_PRINT() here to have correct indentation.

> +		return EFI_DEVICE_ERROR;

EFI_EXIT()

> +	}
> +
> +	return EFI_EXIT(EFI_SUCCESS);
> +}
> +
> +const struct efi_rng_protocol efi_rng_protocol = {
> +	.get_info = rng_getinfo,
> +	.get_rng = getrng,
> +};
>

So I think we will have to get to something like:

static efi_status_t EFIAPI rng_getinfo(struct efi_rng_protocol *this,
				       efi_uintn_t *rng_algorithm_list_size,
				       efi_guid_t *rng_algorithm_list)
{
	efi_guid_t rng_algo_guid = EFI_RNG_ALGORITHM_RAW;
	efi_status_t ret = EFI_SUCCESS;

	EFI_ENTRY("%p, %p, %p", this, rng_algorithm_list_size,
		  rng_algorithm_list);

	if (!this || !rng_algorithm_list_size) {
		ret = EFI_INVALID_PARAMETER;
		goto out;
	}

	if (!rng_algorithm_list ||
	    *rng_algorithm_list_size < sizeof(*rng_algorithm_list)) {
		*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
		ret =  EFI_BUFFER_TOO_SMALL;
		goto out;
	}

	/*
	 * For now, use EFI_RNG_ALGORITHM_RAW as the default
	 * algorithm. If a new algorithm gets added in the
	 * future through a Kconfig, rng_algo_guid will be set
	 * based on that Kconfig option
	 */
	*rng_algorithm_list_size = sizeof(*rng_algorithm_list);
	guidcpy(rng_algorithm_list, &rng_algo_guid);

out:
	return EFI_EXIT(ret);
}

static efi_status_t EFIAPI getrng(struct efi_rng_protocol *this,
				  efi_guid_t *rng_algorithm,
				  efi_uintn_t rng_value_length,
				  uint8_t *rng_value)
{
	int r;
	efi_status_t ret = EFI_SUCCESS;
	struct udevice *dev;
	const efi_guid_t rng_raw_guid = EFI_RNG_ALGORITHM_RAW;

	EFI_ENTRY("%p, %p, %zu, %p", this, rng_algorithm, rng_value_length,
		  rng_value);

	if (!this || !rng_value || !rng_value_length) {
		ret = EFI_INVALID_PARAMETER;
		goto out;
	}

	if (rng_algorithm) {
		EFI_PRINT("RNG algorithm %pUl\n", rng_algorithm);
		if (guidcmp(rng_algorithm, &rng_raw_guid)) {
			ret = EFI_UNSUPPORTED;
			goto out;
		}
	}

	r = platform_get_rng_device(&dev);
	if (r) {
		ret = EFI_UNSUPPORTED;
		goto out;
	}

	ret = dm_rng_read(dev, rng_value, rng_value_length);
	if (ret < 0) {
		EFI_PRINT("Rng device read failed\n");
		ret = EFI_DEVICE_ERROR;
	}

out:
	return EFI_EXIT(ret);
}

Best regards

Heinrich

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

[Sughosh Ganu]

On Sat, 28 Dec 2019 at 21:09, Tuomas Tynkkynen <tuomas.tynkkynen@iki.fi>
wrote:

> Hi,
>
> On Sat, 28 Dec 2019 at 17:19, Heinrich Schuchardt <xypron.glpk@gmx.de>
> wrote:
> >
> > On 12/28/19 4:03 PM, Sughosh Ganu wrote:
> > >
> > > On Sat, 28 Dec 2019 at 20:01, Heinrich Schuchardt <xypron.glpk@gmx.de
> > > <mailto:xypron.glpk@gmx.de>> wrote:
> > >
> > >     On 12/27/19 3:26 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>>
> ...
> >
> > Adding the uclass_get_device() solution in a weak function is ok for me.
> >
> > When testing I already experienced that I had to issue the `virtio scan`
> > command.
> >
>
> But in board_init() in qemu-arm.c we already call virtio_init(). If that
> is not
> working correctly, there is a bug that should be fixed instead.
>

There is no issue with virtio_init.


>
> Maybe the problem is that PCI bus has not been scanned at the time of
> that virtio_init() call (so everything would work correctly with
> virtio-mmio
> but virtio-pci is broken)?
>

Yes, this is the issue. The pci bus has not been scanned when virtio_init
is called. In fact, the pci_init function gets called only as part of the
preboot function, before the main loop is invoked. Thus 'dm tree' shows the
virtio-pci device, but a virtio scan is needed to get the rng device.
Calling pci_init before virtio_init is called shows up all the virtio
devices.

-sughosh