[PATCH 0/6] Change logic of EFI LoadFile2 protocol for initrd loading

[PATCH 0/6] Change logic of EFI LoadFile2 protocol for initrd loading

[Ilias Apalodimas]

Right now in U-Boot, we unconditionally register the EFI LoadFile2 protocol
and the initrd file path is a U-boot .config option.

This imposes a number of limitations. Using multiple initrd's implies you'd
have to concatenate them into one big file. Alternatively you'd have to
recompile U-Boot or overwrite your initrd with a newer one in order to use
it.  Since linux efi-stub only falls back to the cmdline interpreted initrd
if the protocol is not registered, this also creates a problem for EFI
installers, since they won't be able to load their own initrd and start the
installation if the option is enabled.

This patchset is trying to address all of the above, by changing the logic
of the protocol registration and initrd discovery.
It introduces a new EFI variable 'Initrd####', which we try to match on the
BootCurrent value the bootmanager sets up.
For example Boot0010 will search for Initrd0010, which should include the
full initrd path i.e 'mmc 0:0 initrd'. If the file is present, the protocol
will be registered, so the efi-stub can use it.

This opens up another path using U-Boot and defines a new boot flow.
A user will be able to control the kernel/initrd pairs without explicit
cmdline args or GRUB, just by setting the correct 'Initrd####' variable.

Ilias Apalodimas (6):
  efi_loader: remove unconditional initialization of file2 protocol for
    initrd
  efi_loader: Introduce helper functions for EFI
  efi_loader: Replace config option with EFI variable for initrd loading
  efi_loader: Remove unused headers from efi_load_initrd.c
  efi_selftest: Modify self-tests for initrd loading
  efi_loader: bootmgr: use get_var from efi_helper file

 include/efi_helper.h                        |  29 +++
 lib/efi_loader/Kconfig                      |  12 +-
 lib/efi_loader/Makefile                     |   4 +-
 lib/efi_loader/efi_bootmgr.c                |  39 +---
 lib/efi_loader/efi_helper.c                 | 189 ++++++++++++++++++++
 lib/efi_loader/efi_load_initrd.c            |  88 +++------
 lib/efi_loader/efi_setup.c                  |   5 -
 lib/efi_selftest/efi_selftest_load_initrd.c |  90 +++++++++-
 8 files changed, 341 insertions(+), 115 deletions(-)
 create mode 100644 include/efi_helper.h
 create mode 100644 lib/efi_loader/efi_helper.c

-- 
2.30.0.rc2

[PATCH 1/6] efi_loader: remove unconditional initialization of file2 protocol for initrd

[Ilias Apalodimas]

Up to now we register EFI_LOAD_FILE2_PROTOCOL to load an initrd
unconditionally. Although we correctly return various EFI return codes
depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
kernel loader, only falls back to the cmdline interpreted initrd if the
protocol is not registered.

This creates a problem for EFI installers, since they won't be able to
load their own initrd and start the installation.

A following patch introduces a different logic where we search for an 
initrd path defined in an EFI variable named 'Initrd####'.
If the bootmgr is used to launch the EFI payload, we'll will try to match
the BootCurrent value and find the corresponding initrd
(i.e Boot0000 -> Initrd0000 etc). If the file is found, we'll register
the required protocol which the kernel's efi-stub can use and load our
initrd.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_setup.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index e206b60bb82c..d7f880921268 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -209,11 +209,6 @@ efi_status_t efi_init_obj_list(void)
 	if (ret != EFI_SUCCESS)
 		goto out;
 #endif
-#ifdef CONFIG_EFI_LOAD_FILE2_INITRD
-	ret = efi_initrd_register();
-	if (ret != EFI_SUCCESS)
-		goto out;
-#endif
 #ifdef CONFIG_NET
 	ret = efi_net_register();
 	if (ret != EFI_SUCCESS)
-- 
2.30.0.rc2

[PATCH 2/6] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

A following patch introduces a different logic for loading initrd's
based on the EFI_LOAD_FILE2_PROTOCOL.
Since similar logic can be applied in the future for other system files
(i.e DTBs). Let's add some helper functions which will retrieve and
parse device paths via EFI variables.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_helper.h        |  29 ++++++
 lib/efi_loader/efi_helper.c | 189 ++++++++++++++++++++++++++++++++++++
 2 files changed, 218 insertions(+)
 create mode 100644 include/efi_helper.h
 create mode 100644 lib/efi_loader/efi_helper.c

diff --git a/include/efi_helper.h b/include/efi_helper.h
new file mode 100644
index 000000000000..d76e24e0f57d
--- /dev/null
+++ b/include/efi_helper.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#if !defined _EFI_HELPER_H_
+#define _EFI_HELPER_H
+
+#include <efi.h>
+#include <efi_api.h>
+
+/*
+ * @dev:	device string i.e 'mmc'
+ * @part:	partition string i.e '0:2'
+ * @filename:	name of the file
+ */
+struct load_file_info {
+	char dev[32];
+	char part[16];
+	char filename[256];
+};
+
+loff_t get_file_size(const struct load_file_info *file_loc,
+		     efi_status_t *status);
+efi_status_t efi_get_fp_from_var(const u16 *name, u16 start,
+				 struct load_file_info *loc);
+void *get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
+
+#endif
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
new file mode 100644
index 000000000000..4cf1f8abed30
--- /dev/null
+++ b/lib/efi_loader/efi_helper.c
@@ -0,0 +1,189 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (c) 2020, Linaro Limited
+ */
+
+#include <common.h>
+#include <env.h>
+#include <malloc.h>
+#include <dm.h>
+#include <fs.h>
+#include <efi_helper.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
+
+/**
+ * get_file_size() - retrieve the size of initramfs, set efi status on error
+ *
+ * @dev:			device to read from, e.g. "mmc"
+ * @part:			device partition, e.g. "0:1"
+ * @file:			name of file
+ * @status:			EFI exit code in case of failure
+ *
+ * Return:			size of file
+ */
+loff_t get_file_size(const struct load_file_info *info, efi_status_t *status)
+{
+	loff_t sz = 0;
+	int ret;
+
+	ret = fs_set_blk_dev(info->dev, info->part, FS_TYPE_ANY);
+	if (ret) {
+		*status = EFI_NO_MEDIA;
+		goto out;
+	}
+
+	ret = fs_size(info->filename, &sz);
+	if (ret) {
+		sz = 0;
+		*status = EFI_NOT_FOUND;
+		goto out;
+	}
+
+out:
+	return sz;
+}
+
+/*
+ * string_to_load_args() - Fill in a struct load_file_info with the file info
+ *			   parsed from an EFI variable
+ *
+ * @args:	value of the EFI variable i.e "mmc 0 initrd"
+ * @info:	struct to fill in with file specific info
+ *
+ * Return:	Status code
+ */
+static efi_status_t string_to_load_args(char *args, struct load_file_info *info)
+{
+	efi_status_t status = EFI_SUCCESS;
+	char *p;
+
+	/*
+	 * expect a string with three space separated parts:
+	 * - block device type, e.g. "mmc"
+	 * - device and partition identifier, e.g. "0:1"
+	 * - file path on the block device, e.g. "/boot/initrd.cpio.gz"
+	 */
+	p = strsep(&args, " ");
+	if (!p) {
+		status = EFI_NO_MEDIA;
+		goto out;
+	}
+	strncpy(info->dev, p, sizeof(info->dev));
+
+	p = strsep(&args, " ");
+	if (!p) {
+		status = EFI_NO_MEDIA;
+		goto out;
+	}
+	strncpy(info->part, p, sizeof(info->part));
+
+	p = strsep(&args, " ");
+	if (!p) {
+		status = EFI_NOT_FOUND;
+		goto out;
+	}
+	strncpy(info->filename, p, sizeof(info->filename));
+
+out:
+	return status;
+}
+
+/**
+ * get_var() - read value of an EFI variable
+ *
+ * @name:	variable name
+ * @start:	vendor GUID
+ * @size:	size of allocated buffer
+ *
+ * Return:	buffer with variable data or NULL
+ */
+void *get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
+{
+	efi_status_t ret;
+	void *buf = NULL;
+
+	*size = 0;
+	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
+	if (ret == EFI_BUFFER_TOO_SMALL) {
+		buf = malloc(*size);
+		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
+	}
+
+	if (ret != EFI_SUCCESS) {
+		free(buf);
+		*size = 0;
+		return NULL;
+	}
+
+	return buf;
+}
+
+/**
+ * efi_get_fp_from_var() - Retrieve a file path from an EFI variable
+ *
+ * @name:	variable name
+ * @start:	start replacing from
+ * @info:	struct to fill in with file specific info
+ */
+efi_status_t efi_get_fp_from_var(const u16 *name, u16 start,
+				 struct load_file_info *info)
+{
+	u16 hexmap[] = L"0123456789ABCDEF";
+	efi_uintn_t boot_order_size;
+	void *var_value = NULL;
+	u16 *name_dup = NULL;
+	efi_uintn_t size;
+	efi_status_t ret;
+	u16 boot_order;
+
+	memset(info, 0, sizeof(*info));
+
+	/* make sure we have enough space for replacements */
+	if (u16_strsize(name) < sizeof(*name) * start + u16_strsize(L"####")) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	boot_order_size = sizeof(boot_order);
+	ret = efi_get_variable_int(L"BootCurrent",
+				   &efi_global_variable_guid, NULL,
+				   &boot_order_size, &boot_order, NULL);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	name_dup = u16_strdup(name);
+	if (!name_dup) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+	/* Match name variable to BootCurrent */
+	name_dup[start] = hexmap[(boot_order & 0xf000) >> 12];
+	name_dup[start + 1] = hexmap[(boot_order & 0x0f00) >> 8];
+	name_dup[start + 2] = hexmap[(boot_order & 0x00f0) >> 4];
+	name_dup[start + 3] = hexmap[(boot_order & 0x000f) >> 0];
+
+	var_value = get_var(name_dup, &efi_global_variable_guid, &size);
+	if (!var_value) {
+		ret = EFI_NOT_FOUND;
+		goto out;
+	}
+
+	ret = string_to_load_args(var_value, info);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	if (fs_set_blk_dev(info->dev, info->part, FS_TYPE_ANY)) {
+		ret = EFI_NO_MEDIA;
+		goto out;
+	}
+
+	if (!fs_exists(info->filename)) {
+		ret = EFI_NOT_FOUND;
+		goto out;
+	}
+
+out:
+	free(var_value);
+	free(name_dup);
+	return ret;
+}
-- 
2.30.0.rc2

[PATCH 3/6] efi_loader: Replace config option with EFI variable for initrd loading

[Ilias Apalodimas]

Up to now we register EFI_LOAD_FILE2_PROTOCOL to load an initrd
unconditionally. Although we correctly return various EFI return codes
depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
kernel loader, only falls back to the cmdline interpreted initrd if the
protocol is not registered.

This creates a problem for EFI installers, since they won't be able to
load their own initrd and continue the installation.

So let's introduce a different logic that will decopouple the initrd
path from the config option we currently support.
When the EFI application is launched through the bootmgr, we'll try to
match the BootCurrent value to an Initrd#### EFI variable.
i.e Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.

The Initrd#### EFI variable is expected to include the full file path,
i.e 'mmc 0:1 initrd'.  If the file is found, we'll register the
appropriate protocol so the kernel's efi-stub load our initrd.
If the file is not found the kernel will still try to load an initrd
parsing the kernel cmdline, since the protocol won't be registered.

This opens up another path using U-Boot and defines a new boot flow.
A user will be able to control the kernel/initrd pairs without explicit
cmdline args or GRUB. So we can base the whole boot flow on the Boot####
and Initrd#### paired values.

Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/Kconfig           | 12 ++---
 lib/efi_loader/Makefile          |  2 +-
 lib/efi_loader/efi_bootmgr.c     |  6 ++-
 lib/efi_loader/efi_load_initrd.c | 86 +++++++++-----------------------
 4 files changed, 33 insertions(+), 73 deletions(-)

diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index dd8b93bd3c5a..eca24e82b8b1 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -212,14 +212,10 @@ config EFI_LOAD_FILE2_INITRD
 	help
 	  Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can
 	  use to load the initial ramdisk. Once this is enabled using
-	  initrd=<ramdisk> will stop working.
-
-config EFI_INITRD_FILESPEC
-	string "initramfs path"
-	default "host 0:1 initrd"
-	depends on EFI_LOAD_FILE2_INITRD
-	help
-	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
+	  initrd=<ramdisk> will stop working. The protocol will only be
+	  registered if bootmgr is used and the file is found on the defined
+	  path. A boot entry of Boot0001 will try to match Initrd0001 and use
+	  it. Initrd format 'mmc 0:1 <filename>'
 
 config EFI_SECURE_BOOT
 	bool "Enable EFI secure boot support"
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index cd4b252a417c..793e5b7f8730 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -54,7 +54,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
 obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
 obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
 obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
-obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
+obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_helper.o efi_load_initrd.o
 obj-y += efi_signature.o
 
 EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE))
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 61dc72a23da8..ceca5c5b1bf3 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -12,6 +12,7 @@
 #include <log.h>
 #include <malloc.h>
 #include <efi_loader.h>
+#include <efi_load_initrd.h>
 #include <efi_variable.h>
 #include <asm/unaligned.h>
 
@@ -348,8 +349,11 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
 		log_debug("%s trying to load Boot%04X\n", __func__,
 			  bootorder[i]);
 		ret = try_load_entry(bootorder[i], handle, load_options);
-		if (ret == EFI_SUCCESS)
+		if (ret == EFI_SUCCESS) {
+			if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
+				ret = efi_initrd_register();
 			break;
+		}
 	}
 
 	free(bootorder);
diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index d517d686c330..984fea1bd679 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -10,7 +10,9 @@
 #include <dm.h>
 #include <fs.h>
 #include <efi_loader.h>
+#include <efi_variable.h>
 #include <efi_load_initrd.h>
+#include <efi_helper.h>
 
 static const efi_guid_t efi_guid_load_file2_protocol =
 		EFI_LOAD_FILE2_PROTOCOL_GUID;
@@ -45,40 +47,7 @@ static const struct efi_initrd_dp dp = {
 };
 
 /**
- * get_file_size() - retrieve the size of initramfs, set efi status on error
- *
- * @dev:			device to read from, e.g. "mmc"
- * @part:			device partition, e.g. "0:1"
- * @file:			name of file
- * @status:			EFI exit code in case of failure
- *
- * Return:			size of file
- */
-static loff_t get_file_size(const char *dev, const char *part, const char *file,
-			    efi_status_t *status)
-{
-	loff_t sz = 0;
-	int ret;
-
-	ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
-	if (ret) {
-		*status = EFI_NO_MEDIA;
-		goto out;
-	}
-
-	ret = fs_size(file, &sz);
-	if (ret) {
-		sz = 0;
-		*status = EFI_NOT_FOUND;
-		goto out;
-	}
-
-out:
-	return sz;
-}
-
-/**
- * efi_load_file2initrd() - load initial RAM disk
+ * efi_load_file2_initrd() - load initial RAM disk
  *
  * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
  * in order to load an initial RAM disk requested by the Linux kernel stub.
@@ -98,21 +67,14 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		      struct efi_device_path *file_path, bool boot_policy,
 		      efi_uintn_t *buffer_size, void *buffer)
 {
-	char *filespec;
 	efi_status_t status = EFI_NOT_FOUND;
 	loff_t file_sz = 0, read_sz = 0;
-	char *dev, *part, *file;
-	char *pos;
 	int ret;
+	struct load_file_info info;
 
 	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
 		  buffer_size, buffer);
 
-	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
-	if (!filespec)
-		goto out;
-	pos = filespec;
-
 	if (!this || this != &efi_lf2_protocol ||
 	    !buffer_size) {
 		status = EFI_INVALID_PARAMETER;
@@ -130,24 +92,11 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		goto out;
 	}
 
-	/*
-	 * expect a string with three space separated parts:
-	 *
-	 * * a block device type, e.g. "mmc"
-	 * * a device and partition identifier, e.g. "0:1"
-	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
-	 */
-	dev = strsep(&pos, " ");
-	if (!dev)
-		goto out;
-	part = strsep(&pos, " ");
-	if (!part)
-		goto out;
-	file = strsep(&pos, " ");
-	if (!file)
+	status = efi_get_fp_from_var(L"Initrd####", 6, &info);
+	if (status != EFI_SUCCESS)
 		goto out;
 
-	file_sz = get_file_size(dev, part, file, &status);
+	file_sz = get_file_size(&info, &status);
 	if (!file_sz)
 		goto out;
 
@@ -155,23 +104,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
 		status = EFI_BUFFER_TOO_SMALL;
 		*buffer_size = file_sz;
 	} else {
-		ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
+		ret = fs_set_blk_dev(info.dev, info.part,
+				     FS_TYPE_ANY);
 		if (ret) {
 			status = EFI_NO_MEDIA;
 			goto out;
 		}
 
-		ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size,
-			      &read_sz);
-		if (ret || read_sz != file_sz)
+		ret = fs_read(info.filename, map_to_sysmem(buffer), 0,
+			      *buffer_size, &read_sz);
+		if (ret || read_sz != file_sz) {
+			status = EFI_DEVICE_ERROR;
 			goto out;
+		}
 		*buffer_size = read_sz;
 
 		status = EFI_SUCCESS;
 	}
 
 out:
-	free(filespec);
 	return EFI_EXIT(status);
 }
 
@@ -189,6 +140,15 @@ efi_status_t efi_initrd_register(void)
 {
 	efi_handle_t efi_initrd_handle = NULL;
 	efi_status_t ret;
+	struct load_file_info info;
+
+	ret = efi_get_fp_from_var(L"Initrd####", 6, &info);
+	/*
+	 * Don't fail here. If we don't register the protocol the efi-stub will
+	 * try to load and initrd parsing the kernel cmdline
+	 */
+	if (ret != EFI_SUCCESS)
+		return EFI_SUCCESS;
 
 	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
 		       (&efi_initrd_handle,
-- 
2.30.0.rc2

[PATCH 4/6] efi_loader: Remove unused headers from efi_load_initrd.c

[Ilias Apalodimas]

dm.h and env.h serve no purpose here. Remove them

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/efi_load_initrd.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index 984fea1bd679..e0b4a82ce037 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -4,10 +4,8 @@
  */
 
 #include <common.h>
-#include <env.h>
 #include <malloc.h>
 #include <mapmem.h>
-#include <dm.h>
 #include <fs.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
-- 
2.30.0.rc2

[PATCH 5/6] efi_selftest: Modify self-tests for initrd loading

[Ilias Apalodimas]

Previous patches change the logic of the initrd discovery and the
registration of the protocol.
Instead of a config now option it now resides in an EFI variable. Adjust
the instructions of execution accordingly and add new self-tests which
will cover the new functionality.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_selftest/efi_selftest_load_initrd.c | 90 ++++++++++++++++++++-
 1 file changed, 88 insertions(+), 2 deletions(-)

diff --git a/lib/efi_selftest/efi_selftest_load_initrd.c b/lib/efi_selftest/efi_selftest_load_initrd.c
index fe060a664402..85afed55425f 100644
--- a/lib/efi_selftest/efi_selftest_load_initrd.c
+++ b/lib/efi_selftest/efi_selftest_load_initrd.c
@@ -14,10 +14,12 @@
  *
  *   CONFIG_EFI_SELFTEST=y
  *   CONFIG_EFI_LOAD_FILE2_INITRD=y
- *   CONFIG_EFI_INITRD_FILESPEC="host 0:1 initrd"
  *
- * * Run ./u-boot and execute
+ * * Create files
+ *   mkdir init_test && cp initrd init_test/
+ *   virt-make-fs -t ext4 init_test test.img
  *
+ * * Run ./u-boot and execute
  *   host bind 0 image
  *   setenv efi_selftest load initrd
  *   bootefi selftest
@@ -43,6 +45,7 @@
 #include <efi_load_initrd.h>
 
 static struct efi_boot_services *boottime;
+static struct efi_runtime_services *runtime;
 
 static struct efi_initrd_dp dp = {
 	.vendor = {
@@ -80,6 +83,7 @@ static int setup(const efi_handle_t handle,
 		 const struct efi_system_table *systable)
 {
 	boottime = systable->boottime;
+	runtime = systable->runtime;
 
 	return EFI_ST_SUCCESS;
 }
@@ -87,6 +91,7 @@ static int setup(const efi_handle_t handle,
 static int execute(void)
 {
 	efi_guid_t lf2_proto_guid = EFI_LOAD_FILE2_PROTOCOL_GUID;
+	efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
 	struct efi_load_file_protocol *lf2;
 	struct efi_device_path *dp2, *dp2_invalid;
 	efi_status_t status;
@@ -95,8 +100,89 @@ static int execute(void)
 	efi_uintn_t buffer_size;
 	void *buf;
 	u32 crc32;
+	u16 boot_current = 0;
+	efi_uintn_t boot_current_size = sizeof(boot_current);
+	char path[] = "host 0 initrd";
+	char invalid_path[] = "host 1 initrd";
+	efi_uintn_t path_size = sizeof(path);
+	efi_uintn_t invalid_path_size = sizeof(invalid_path);
+	u32 attrs = EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_RUNTIME_ACCESS;
 
 	memset(buffer, 0, sizeof(buffer));
+	/* Set variable BootCurrent and Initrd#### to a wrong value */
+	status = runtime->set_variable(L"BootCurrent", &efi_global_variable_guid,
+				       attrs, boot_current_size, &boot_current);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("SetVariable for BootCurrent failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/*
+	 * We don't need NV for Initrd here.
+	 * Set the file to an invalid file path
+	 */
+	status = runtime->set_variable(L"Initrd0010", &efi_global_variable_guid,
+				       attrs, invalid_path_size, invalid_path);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("SetVariable for Initrd failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* We only register the protocol during efibootmgr */
+	status = efi_initrd_register();
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to register initrd protocol\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/*
+	 * We should only register the protocol if the file's found
+	 * Right now both BootCurrent and file path are invalid
+	 */
+	dp2 = (struct efi_device_path *)&dp;
+	status = boottime->locate_device_path(&lf2_proto_guid, &dp2, &handle);
+	if (status != EFI_NOT_FOUND) {
+		efi_st_error("Initrd protocol should't be registered\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* Update BootCurrent to the correct value */
+	boot_current = 0x0010;
+	status = runtime->set_variable(L"BootCurrent", &efi_global_variable_guid,
+				       attrs, boot_current_size, &boot_current);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("SetVariable for BootCurrent failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* re-register with invalid file path */
+	status = efi_initrd_register();
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to register initrd protocol\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* file path is invalid */
+	dp2 = (struct efi_device_path *)&dp;
+	status = boottime->locate_device_path(&lf2_proto_guid, &dp2, &handle);
+	if (status != EFI_NOT_FOUND) {
+		efi_st_error("Initrd protocol should't be registered\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* re-register with correct values now */
+	status = runtime->set_variable(L"Initrd0010", &efi_global_variable_guid,
+				       attrs, path_size, path);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("SetVariable for Initrd failed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	status = efi_initrd_register();
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to register initrd protocol\n");
+		return EFI_ST_FAILURE;
+	}
 
 	dp2 = (struct efi_device_path *)&dp;
 	status = boottime->locate_device_path(&lf2_proto_guid, &dp2, &handle);
-- 
2.30.0.rc2

[PATCH 6/6] efi_loader: bootmgr: use get_var from efi_helper file

[Ilias Apalodimas]

A few patches before we introduced a file which includes the get_var()
function defined in the efi bootmanager.
So let's replace it here and use the common function as much as we can.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 lib/efi_loader/Makefile      |  2 +-
 lib/efi_loader/efi_bootmgr.c | 33 +--------------------------------
 2 files changed, 2 insertions(+), 33 deletions(-)

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 793e5b7f8730..d21d57d5811e 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -21,7 +21,7 @@ targets += helloworld.o
 endif
 
 obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
-obj-y += efi_bootmgr.o
+obj-y += efi_helper.o efi_bootmgr.o
 obj-y += efi_boottime.o
 obj-y += efi_console.o
 obj-y += efi_device_path.o
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index ceca5c5b1bf3..a175842ae053 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -11,6 +11,7 @@
 #include <charset.h>
 #include <log.h>
 #include <malloc.h>
+#include <efi_helper.h>
 #include <efi_loader.h>
 #include <efi_load_initrd.h>
 #include <efi_variable.h>
@@ -166,38 +167,6 @@ unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data)
 	return size;
 }
 
-/**
- * get_var() - get UEFI variable
- *
- * It is the caller's duty to free the returned buffer.
- *
- * @name:	name of variable
- * @vendor:	vendor GUID of variable
- * @size:	size of allocated buffer
- * Return:	buffer with variable data or NULL
- */
-static void *get_var(u16 *name, const efi_guid_t *vendor,
-		     efi_uintn_t *size)
-{
-	efi_status_t ret;
-	void *buf = NULL;
-
-	*size = 0;
-	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
-	if (ret == EFI_BUFFER_TOO_SMALL) {
-		buf = malloc(*size);
-		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
-	}
-
-	if (ret != EFI_SUCCESS) {
-		free(buf);
-		*size = 0;
-		return NULL;
-	}
-
-	return buf;
-}
-
 /**
  * try_load_entry() - try to load image for boot option
  *
-- 
2.30.0.rc2

[PATCH 3/6] efi_loader: Replace config option with EFI variable for initrd loading

[Ilias Apalodimas]

Hi Heinrich, 

[...]
>  			  bootorder[i]);
>  		ret = try_load_entry(bootorder[i], handle, load_options);
> -		if (ret == EFI_SUCCESS)
> +		if (ret == EFI_SUCCESS) {
> +			if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +				ret = efi_initrd_register();

I just realized registering the protocol here won't work for BootNext since
the protocol won't regsiter.
I'll just move the protocol registration in try_load_entry() when I send a v2

Cheers
/Ilias

[PATCH 2/6] efi_loader: Introduce helper functions for EFI

[Heinrich Schuchardt]

On 12/28/20 1:24 PM, Ilias Apalodimas wrote:
> A following patch introduces a different logic for loading initrd's
> based on the EFI_LOAD_FILE2_PROTOCOL.
> Since similar logic can be applied in the future for other system files
> (i.e DTBs). Let's add some helper functions which will retrieve and
> parse device paths via EFI variables.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   include/efi_helper.h        |  29 ++++++
>   lib/efi_loader/efi_helper.c | 189 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 218 insertions(+)
>   create mode 100644 include/efi_helper.h
>   create mode 100644 lib/efi_loader/efi_helper.c
>
> diff --git a/include/efi_helper.h b/include/efi_helper.h
> new file mode 100644
> index 000000000000..d76e24e0f57d
> --- /dev/null
> +++ b/include/efi_helper.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#if !defined _EFI_HELPER_H_
> +#define _EFI_HELPER_H
> +
> +#include <efi.h>
> +#include <efi_api.h>
> +
> +/*
> + * @dev:	device string i.e 'mmc'
> + * @part:	partition string i.e '0:2'
> + * @filename:	name of the file
> + */
> +struct load_file_info {
> +	char dev[32];
> +	char part[16];
> +	char filename[256];
> +};
> +
> +loff_t get_file_size(const struct load_file_info *file_loc,
> +		     efi_status_t *status);
> +efi_status_t efi_get_fp_from_var(const u16 *name, u16 start,
> +				 struct load_file_info *loc);
> +void *get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
> +
> +#endif
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> new file mode 100644
> index 000000000000..4cf1f8abed30
> --- /dev/null
> +++ b/lib/efi_loader/efi_helper.c
> @@ -0,0 +1,189 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (c) 2020, Linaro Limited
> + */
> +
> +#include <common.h>
> +#include <env.h>
> +#include <malloc.h>
> +#include <dm.h>
> +#include <fs.h>
> +#include <efi_helper.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +
> +/**
> + * get_file_size() - retrieve the size of initramfs, set efi status on error
> + *
> + * @dev:			device to read from, e.g. "mmc"
> + * @part:			device partition, e.g. "0:1"
> + * @file:			name of file
> + * @status:			EFI exit code in case of failure
> + *
> + * Return:			size of file
> + */
> +loff_t get_file_size(const struct load_file_info *info, efi_status_t *status)
> +{
> +	loff_t sz = 0;
> +	int ret;
> +
> +	ret = fs_set_blk_dev(info->dev, info->part, FS_TYPE_ANY);
> +	if (ret) {
> +		*status = EFI_NO_MEDIA;
> +		goto out;
> +	}
> +
> +	ret = fs_size(info->filename, &sz);
> +	if (ret) {
> +		sz = 0;
> +		*status = EFI_NOT_FOUND;
> +		goto out;
> +	}
> +
> +out:
> +	return sz;
> +}
> +
> +/*
> + * string_to_load_args() - Fill in a struct load_file_info with the file info
> + *			   parsed from an EFI variable
> + *
> + * @args:	value of the EFI variable i.e "mmc 0 initrd"
> + * @info:	struct to fill in with file specific info
> + *
> + * Return:	Status code
> + */
> +static efi_status_t string_to_load_args(char *args, struct load_file_info *info)
> +{
> +	efi_status_t status = EFI_SUCCESS;
> +	char *p;
> +
> +	/*
> +	 * expect a string with three space separated parts:
> +	 * - block device type, e.g. "mmc"
> +	 * - device and partition identifier, e.g. "0:1"
> +	 * - file path on the block device, e.g. "/boot/initrd.cpio.gz"
> +	 */
> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NO_MEDIA;
> +		goto out;
> +	}
> +	strncpy(info->dev, p, sizeof(info->dev));
> +
> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NO_MEDIA;
> +		goto out;
> +	}
> +	strncpy(info->part, p, sizeof(info->part));
> +
> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NOT_FOUND;
> +		goto out;
> +	}
> +	strncpy(info->filename, p, sizeof(info->filename));
> +
> +out:
> +	return status;
> +}
> +
> +/**
> + * get_var() - read value of an EFI variable
> + *
> + * @name:	variable name
> + * @start:	vendor GUID
> + * @size:	size of allocated buffer
> + *
> + * Return:	buffer with variable data or NULL
> + */
> +void *get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
> +{
> +	efi_status_t ret;
> +	void *buf = NULL;
> +
> +	*size = 0;
> +	ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	if (ret == EFI_BUFFER_TOO_SMALL) {
> +		buf = malloc(*size);
> +		ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
> +	}
> +
> +	if (ret != EFI_SUCCESS) {
> +		free(buf);
> +		*size = 0;
> +		return NULL;
> +	}
> +
> +	return buf;
> +}
> +
> +/**
> + * efi_get_fp_from_var() - Retrieve a file path from an EFI variable
> + *
> + * @name:	variable name
> + * @start:	start replacing from
> + * @info:	struct to fill in with file specific info
> + */
> +efi_status_t efi_get_fp_from_var(const u16 *name, u16 start,
> +				 struct load_file_info *info)
> +{
> +	u16 hexmap[] = L"0123456789ABCDEF";
> +	efi_uintn_t boot_order_size;
> +	void *var_value = NULL;
> +	u16 *name_dup = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	u16 boot_order;
> +
> +	memset(info, 0, sizeof(*info));
> +
> +	/* make sure we have enough space for replacements */
> +	if (u16_strsize(name) < sizeof(*name) * start + u16_strsize(L"####")) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	boot_order_size = sizeof(boot_order);
> +	ret = efi_get_variable_int(L"BootCurrent",
> +				   &efi_global_variable_guid, NULL,
> +				   &boot_order_size, &boot_order, NULL);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	name_dup = u16_strdup(name);
> +	if (!name_dup) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +	/* Match name variable to BootCurrent */
> +	name_dup[start] = hexmap[(boot_order & 0xf000) >> 12];
> +	name_dup[start + 1] = hexmap[(boot_order & 0x0f00) >> 8];
> +	name_dup[start + 2] = hexmap[(boot_order & 0x00f0) >> 4];
> +	name_dup[start + 3] = hexmap[(boot_order & 0x000f) >> 0];

Please, consider using  efi_create_indexed_name().

Best regards

Heinrich

> +
> +	var_value = get_var(name_dup, &efi_global_variable_guid, &size);
> +	if (!var_value) {
> +		ret = EFI_NOT_FOUND;
> +		goto out;
> +	}
> +
> +	ret = string_to_load_args(var_value, info);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	if (fs_set_blk_dev(info->dev, info->part, FS_TYPE_ANY)) {
> +		ret = EFI_NO_MEDIA;
> +		goto out;
> +	}
> +
> +	if (!fs_exists(info->filename)) {
> +		ret = EFI_NOT_FOUND;
> +		goto out;
> +	}
> +
> +out:
> +	free(var_value);
> +	free(name_dup);
> +	return ret;
> +}
>

[PATCH 3/6] efi_loader: Replace config option with EFI variable for initrd loading

[Heinrich Schuchardt]

On 12/28/20 1:24 PM, Ilias Apalodimas wrote:
> Up to now we register EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI return codes
> depending on the file status (i.e EFI_NO_MEDIA, EFI_NOT_FOUND etc), the
> kernel loader, only falls back to the cmdline interpreted initrd if the
> protocol is not registered.
>
> This creates a problem for EFI installers, since they won't be able to
> load their own initrd and continue the installation.
>
> So let's introduce a different logic that will decopouple the initrd
> path from the config option we currently support.
> When the EFI application is launched through the bootmgr, we'll try to
> match the BootCurrent value to an Initrd#### EFI variable.
> i.e Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.
>
> The Initrd#### EFI variable is expected to include the full file path,
> i.e 'mmc 0:1 initrd'.  If the file is found, we'll register the
> appropriate protocol so the kernel's efi-stub load our initrd.
> If the file is not found the kernel will still try to load an initrd
> parsing the kernel cmdline, since the protocol won't be registered.
>
> This opens up another path using U-Boot and defines a new boot flow.
> A user will be able to control the kernel/initrd pairs without explicit
> cmdline args or GRUB. So we can base the whole boot flow on the Boot####
> and Initrd#### paired values.
>
> Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/Kconfig           | 12 ++---
>   lib/efi_loader/Makefile          |  2 +-
>   lib/efi_loader/efi_bootmgr.c     |  6 ++-
>   lib/efi_loader/efi_load_initrd.c | 86 +++++++++-----------------------
>   4 files changed, 33 insertions(+), 73 deletions(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index dd8b93bd3c5a..eca24e82b8b1 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -212,14 +212,10 @@ config EFI_LOAD_FILE2_INITRD
>   	help
>   	  Expose a EFI_FILE_LOAD2_PROTOCOL that the Linux UEFI stub can
>   	  use to load the initial ramdisk. Once this is enabled using
> -	  initrd=<ramdisk> will stop working.
> -
> -config EFI_INITRD_FILESPEC
> -	string "initramfs path"
> -	default "host 0:1 initrd"
> -	depends on EFI_LOAD_FILE2_INITRD
> -	help
> -	  Full path of the initramfs file, e.g. mmc 0:2 initramfs.cpio.gz.
> +	  initrd=<ramdisk> will stop working. The protocol will only be
> +	  registered if bootmgr is used and the file is found on the defined

In UEFI speak protocols are not registered but installed.

further comment below

> +	  path. A boot entry of Boot0001 will try to match Initrd0001 and use
> +	  it. Initrd format 'mmc 0:1 <filename>'
>
>   config EFI_SECURE_BOOT
>   	bool "Enable EFI secure boot support"
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index cd4b252a417c..793e5b7f8730 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -54,7 +54,7 @@ obj-$(CONFIG_GENERATE_ACPI_TABLE) += efi_acpi.o
>   obj-$(CONFIG_GENERATE_SMBIOS_TABLE) += efi_smbios.o
>   obj-$(CONFIG_EFI_RNG_PROTOCOL) += efi_rng.o
>   obj-$(CONFIG_EFI_TCG2_PROTOCOL) += efi_tcg2.o
> -obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_load_initrd.o
> +obj-$(CONFIG_EFI_LOAD_FILE2_INITRD) += efi_helper.o efi_load_initrd.o
>   obj-y += efi_signature.o
>
>   EFI_VAR_SEED_FILE := $(subst $\",,$(CONFIG_EFI_VAR_SEED_FILE))
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 61dc72a23da8..ceca5c5b1bf3 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -12,6 +12,7 @@
>   #include <log.h>
>   #include <malloc.h>
>   #include <efi_loader.h>
> +#include <efi_load_initrd.h>
>   #include <efi_variable.h>
>   #include <asm/unaligned.h>
>
> @@ -348,8 +349,11 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
>   		log_debug("%s trying to load Boot%04X\n", __func__,
>   			  bootorder[i]);
>   		ret = try_load_entry(bootorder[i], handle, load_options);
> -		if (ret == EFI_SUCCESS)
> +		if (ret == EFI_SUCCESS) {
> +			if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> +				ret = efi_initrd_register();

I think this is not enough.

* We should uninstall the protocol once 'bootefi bootmgr' returns
   to U-Boot.
* The EFI_LOAD_FILE2_PROTOCOL should be installed on the handle of
   the loaded image for this purpose.

Best regards

Heinrich

>   			break;
> +		}
>   	}
>
>   	free(bootorder);
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index d517d686c330..984fea1bd679 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -10,7 +10,9 @@
>   #include <dm.h>
>   #include <fs.h>
>   #include <efi_loader.h>
> +#include <efi_variable.h>
>   #include <efi_load_initrd.h>
> +#include <efi_helper.h>
>
>   static const efi_guid_t efi_guid_load_file2_protocol =
>   		EFI_LOAD_FILE2_PROTOCOL_GUID;
> @@ -45,40 +47,7 @@ static const struct efi_initrd_dp dp = {
>   };
>
>   /**
> - * get_file_size() - retrieve the size of initramfs, set efi status on error
> - *
> - * @dev:			device to read from, e.g. "mmc"
> - * @part:			device partition, e.g. "0:1"
> - * @file:			name of file
> - * @status:			EFI exit code in case of failure
> - *
> - * Return:			size of file
> - */
> -static loff_t get_file_size(const char *dev, const char *part, const char *file,
> -			    efi_status_t *status)
> -{
> -	loff_t sz = 0;
> -	int ret;
> -
> -	ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> -	if (ret) {
> -		*status = EFI_NO_MEDIA;
> -		goto out;
> -	}
> -
> -	ret = fs_size(file, &sz);
> -	if (ret) {
> -		sz = 0;
> -		*status = EFI_NOT_FOUND;
> -		goto out;
> -	}
> -
> -out:
> -	return sz;
> -}
> -
> -/**
> - * efi_load_file2initrd() - load initial RAM disk
> + * efi_load_file2_initrd() - load initial RAM disk
>    *
>    * This function implements the LoadFile service of the EFI_LOAD_FILE2_PROTOCOL
>    * in order to load an initial RAM disk requested by the Linux kernel stub.
> @@ -98,21 +67,14 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>   		      struct efi_device_path *file_path, bool boot_policy,
>   		      efi_uintn_t *buffer_size, void *buffer)
>   {
> -	char *filespec;
>   	efi_status_t status = EFI_NOT_FOUND;
>   	loff_t file_sz = 0, read_sz = 0;
> -	char *dev, *part, *file;
> -	char *pos;
>   	int ret;
> +	struct load_file_info info;
>
>   	EFI_ENTRY("%p, %p, %d, %p, %p", this, file_path, boot_policy,
>   		  buffer_size, buffer);
>
> -	filespec = strdup(CONFIG_EFI_INITRD_FILESPEC);
> -	if (!filespec)
> -		goto out;
> -	pos = filespec;
> -
>   	if (!this || this != &efi_lf2_protocol ||
>   	    !buffer_size) {
>   		status = EFI_INVALID_PARAMETER;
> @@ -130,24 +92,11 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>   		goto out;
>   	}
>
> -	/*
> -	 * expect a string with three space separated parts:
> -	 *
> -	 * * a block device type, e.g. "mmc"
> -	 * * a device and partition identifier, e.g. "0:1"
> -	 * * a file path on the block device, e.g. "/boot/initrd.cpio.gz"
> -	 */
> -	dev = strsep(&pos, " ");
> -	if (!dev)
> -		goto out;
> -	part = strsep(&pos, " ");
> -	if (!part)
> -		goto out;
> -	file = strsep(&pos, " ");
> -	if (!file)
> +	status = efi_get_fp_from_var(L"Initrd####", 6, &info);
> +	if (status != EFI_SUCCESS)
>   		goto out;
>
> -	file_sz = get_file_size(dev, part, file, &status);
> +	file_sz = get_file_size(&info, &status);
>   	if (!file_sz)
>   		goto out;
>
> @@ -155,23 +104,25 @@ efi_load_file2_initrd(struct efi_load_file_protocol *this,
>   		status = EFI_BUFFER_TOO_SMALL;
>   		*buffer_size = file_sz;
>   	} else {
> -		ret = fs_set_blk_dev(dev, part, FS_TYPE_ANY);
> +		ret = fs_set_blk_dev(info.dev, info.part,
> +				     FS_TYPE_ANY);
>   		if (ret) {
>   			status = EFI_NO_MEDIA;
>   			goto out;
>   		}
>
> -		ret = fs_read(file, map_to_sysmem(buffer), 0, *buffer_size,
> -			      &read_sz);
> -		if (ret || read_sz != file_sz)
> +		ret = fs_read(info.filename, map_to_sysmem(buffer), 0,
> +			      *buffer_size, &read_sz);
> +		if (ret || read_sz != file_sz) {
> +			status = EFI_DEVICE_ERROR;
>   			goto out;
> +		}
>   		*buffer_size = read_sz;
>
>   		status = EFI_SUCCESS;
>   	}
>
>   out:
> -	free(filespec);
>   	return EFI_EXIT(status);
>   }
>
> @@ -189,6 +140,15 @@ efi_status_t efi_initrd_register(void)
>   {
>   	efi_handle_t efi_initrd_handle = NULL;
>   	efi_status_t ret;
> +	struct load_file_info info;
> +
> +	ret = efi_get_fp_from_var(L"Initrd####", 6, &info);
> +	/*
> +	 * Don't fail here. If we don't register the protocol the efi-stub will
> +	 * try to load and initrd parsing the kernel cmdline
> +	 */
> +	if (ret != EFI_SUCCESS)
> +		return EFI_SUCCESS;
>
>   	ret = EFI_CALL(efi_install_multiple_protocol_interfaces
>   		       (&efi_initrd_handle,
>

[PATCH 4/6] efi_loader: Remove unused headers from efi_load_initrd.c

[Heinrich Schuchardt]

On 12/28/20 1:24 PM, Ilias Apalodimas wrote:
> dm.h and env.h serve no purpose here. Remove them
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_loader/efi_load_initrd.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 984fea1bd679..e0b4a82ce037 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -4,10 +4,8 @@
>    */
>
>   #include <common.h>
> -#include <env.h>
>   #include <malloc.h>
>   #include <mapmem.h>
> -#include <dm.h>
>   #include <fs.h>
>   #include <efi_loader.h>
>   #include <efi_variable.h>

It would be nice to bring the headers into alphabetic order.
I suggest to apply this change before any other changes to efi_load_initrd.c

Best regards

Heinrich

>

[PATCH] efi_loader: Remove unused headers from efi_load_initrd.c

[Heinrich Schuchardt]

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

dm.h and env.h serve no purpose here. Remove them

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

Bring headers into alphabetic order
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_load_initrd.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index 4bf3b5ef68..e4126b33fd 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -4,13 +4,11 @@
  */

 #include <common.h>
-#include <env.h>
-#include <malloc.h>
-#include <mapmem.h>
-#include <dm.h>
 #include <fs.h>
 #include <efi_loader.h>
 #include <efi_load_initrd.h>
+#include <malloc.h>
+#include <mapmem.h>

 static efi_status_t EFIAPI
 efi_load_file2_initrd(struct efi_load_file_protocol *this,
--
2.29.2

[PATCH 3/6] efi_loader: Replace config option with EFI variable for initrd loading

[Ilias Apalodimas]

Hi Heinrich, 

On Mon, Dec 28, 2020 at 03:55:49PM +0100, Heinrich Schuchardt wrote:
[...]
> >   		ret = try_load_entry(bootorder[i], handle, load_options);
> > -		if (ret == EFI_SUCCESS)
> > +		if (ret == EFI_SUCCESS) {
> > +			if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
> > +				ret = efi_initrd_register();
> 
> I think this is not enough.
> 
> * We should uninstall the protocol once 'bootefi bootmgr' returns
>   to U-Boot.
> * The EFI_LOAD_FILE2_PROTOCOL should be installed on the handle of
>   the loaded image for this purpose.
> 

Yep you are right. I completely missed that case, I'll fix it on v2

Regards
/Ilias
 

[PATCH 2/6] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

Hi Heinrich, 

> > +
> > +	/* make sure we have enough space for replacements */
> > +	if (u16_strsize(name) < sizeof(*name) * start + u16_strsize(L"####")) {
> > +		ret = EFI_INVALID_PARAMETER;
> > +		goto out;
> > +	}
> > +	boot_order_size = sizeof(boot_order);
> > +	ret = efi_get_variable_int(L"BootCurrent",
> > +				   &efi_global_variable_guid, NULL,
> > +				   &boot_order_size, &boot_order, NULL);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	name_dup = u16_strdup(name);
> > +	if (!name_dup) {
> > +		ret = EFI_OUT_OF_RESOURCES;
> > +		goto out;
> > +	}
> > +	/* Match name variable to BootCurrent */
> > +	name_dup[start] = hexmap[(boot_order & 0xf000) >> 12];
> > +	name_dup[start + 1] = hexmap[(boot_order & 0x0f00) >> 8];
> > +	name_dup[start + 2] = hexmap[(boot_order & 0x00f0) >> 4];
> > +	name_dup[start + 3] = hexmap[(boot_order & 0x000f) >> 0];
> 
> Please, consider using  efi_create_indexed_name().

That one doesn't check any input variables and just asks for the user to
provide sufficient buffers for the output. 
I am explicitly checking the sizes here. I guess I can add similar checks to 
efi_create_indexed_name() and use it, instead of open coding again.
Just a note here, there's similar code to the efi bootmgr, so we should
probably start replacing all of the functions.

Regards
/Ilias
> 
> Best regards
> 
> Heinrich
> 
> > +
> > +	var_value = get_var(name_dup, &efi_global_variable_guid, &size);
> > +	if (!var_value) {
> > +		ret = EFI_NOT_FOUND;
> > +		goto out;
> > +	}
> > +
> > +	ret = string_to_load_args(var_value, info);
> > +	if (ret != EFI_SUCCESS)
> > +		goto out;
> > +
> > +	if (fs_set_blk_dev(info->dev, info->part, FS_TYPE_ANY)) {
> > +		ret = EFI_NO_MEDIA;
> > +		goto out;
> > +	}
> > +
> > +	if (!fs_exists(info->filename)) {
> > +		ret = EFI_NOT_FOUND;
> > +		goto out;
> > +	}
> > +
> > +out:
> > +	free(var_value);
> > +	free(name_dup);
> > +	return ret;
> > +}
> > 
>