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

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

[Ilias Apalodimas]

Hi!

This is v2 of [1].

There's been a couple of changes regarding where we install the protocol. 
The initial patchset was completely disregarding BootNext, so that's taken 
into account now and we can use the new feature. 
This brought a few changes on the selftests as well, since we now use the 
loaded image handle to install the protocol, as a consequence the custom 
handle in the tests is now uninstalled during the test .teardown(), but 
the overall approach remains identical.

Changes since v1:
- Use efi_create_indexed_name() instead of open coding it. An extra
  patch has been applied to efi_create_indexed_name() to ensure the
  destination buffer is big enough for our variable name.
- Install the protocol during do_efibootmgr() on the loaded image
  handle and adjust selftests accordingly. The protocol will be 
  uninstalled if efi_exit() is called from and EFI app now.
- Make sure strings are null terminated in efi_helper.c
- Added documentation in uefi.rst describing the new feature.

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

Ilias Apalodimas (8):
  efi_loader: Remove unused headers from efi_load_initrd.c
  efi_loader: Remove unconditional installation of file2 protocol for
    initrd
  efi_loader: Add size checks to efi_create_indexed_name()
  efi_loader: Introduce helper functions for EFI
  efi_loader: bootmgr: Use get_var from efi_helper file
  efi_loader: Replace config option with EFI variable for initrd loading
  efi_selftest: Modify self-tests for initrd loading via Loadfile2
  doc: uefi: Add instruction for initrd loading

 cmd/bootefi.c                               |  13 ++
 doc/uefi/uefi.rst                           |  15 ++
 include/efi_helper.h                        |  28 +++
 include/efi_loader.h                        |   5 +-
 lib/efi_loader/Kconfig                      |  13 +-
 lib/efi_loader/Makefile                     |   2 +-
 lib/efi_loader/efi_bootmgr.c                |  37 +---
 lib/efi_loader/efi_helper.c                 | 180 ++++++++++++++++++++
 lib/efi_loader/efi_load_initrd.c            | 101 ++++-------
 lib/efi_loader/efi_setup.c                  |   5 -
 lib/efi_loader/efi_string.c                 |  10 +-
 lib/efi_selftest/efi_selftest_load_initrd.c | 100 ++++++++++-
 test/unicode_ut.c                           |   2 +-
 13 files changed, 384 insertions(+), 127 deletions(-)
 create mode 100644 include/efi_helper.h
 create mode 100644 lib/efi_loader/efi_helper.c

-- 
2.30.0

[PATCH 1/8 v2] efi_loader: Remove unused headers from efi_load_initrd.c

[Ilias Apalodimas]

dm.h and env.h serve no purpose here. Remove them and sort the
remaining in alphabetical order.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_loader/efi_load_initrd.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index d517d686c330..4ec55d70979d 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 <fs.h>
+#include <malloc.h>
+#include <mapmem.h>
 
 static const efi_guid_t efi_guid_load_file2_protocol =
 		EFI_LOAD_FILE2_PROTOCOL_GUID;
-- 
2.30.0

[PATCH 2/8 v2] efi_loader: Remove unconditional installation of file2 protocol for initrd

[Ilias Apalodimas]

Up to now we install the EFI_LOAD_FILE2_PROTOCOL to load an initrd
unconditionally. Although we correctly return various EFI exit 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 installed.

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 install
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

[PATCH 3/8 v2] efi_loader: Add size checks to efi_create_indexed_name()

[Ilias Apalodimas]

Although the function description states the caller must provide a
sufficient buffer, it's better to have in function checks and ensure
the destination buffer can hold the intended variable name.

So let's add an extra argument with the buffer size and check that
before copying.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_loader.h        |  3 ++-
 lib/efi_loader/efi_string.c | 10 ++++++++--
 test/unicode_ut.c           |  2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3c68b85b68e9..af30dbafab77 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -810,7 +810,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
 void efi_memcpy_runtime(void *dest, const void *src, size_t n);
 
 /* commonly used helper function */
-u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index);
+u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
+			     unsigned int index);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
 
diff --git a/lib/efi_loader/efi_string.c b/lib/efi_loader/efi_string.c
index 3de721f06c7f..962724228866 100644
--- a/lib/efi_loader/efi_string.c
+++ b/lib/efi_loader/efi_string.c
@@ -23,13 +23,19 @@
  * Return: A pointer to the next position after the created string
  *	   in @buffer, or NULL otherwise
  */
-u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index)
+u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
+			     unsigned int index)
 {
 	u16 *p = buffer;
 	char index_buf[5];
+	size_t size;
 
+	size = (utf8_utf16_strlen(name) * sizeof(u16) +
+		sizeof(index_buf) * sizeof(u16));
+	if (buffer_size < size)
+		return NULL;
 	utf8_utf16_strcpy(&p, name);
-	sprintf(index_buf, "%04X", index);
+	snprintf(index_buf, sizeof(index_buf), "%04X", index);
 	utf8_utf16_strcpy(&p, index_buf);
 
 	return p;
diff --git a/test/unicode_ut.c b/test/unicode_ut.c
index 33fc8b0ee1e2..6130ef0b5497 100644
--- a/test/unicode_ut.c
+++ b/test/unicode_ut.c
@@ -603,7 +603,7 @@ static int unicode_test_efi_create_indexed_name(struct unit_test_state *uts)
 	u16 *pos;
 
 	memset(buf, 0xeb, sizeof(buf));
-	pos = efi_create_indexed_name(buf, "Capsule", 0x0af9);
+	pos = efi_create_indexed_name(buf, sizeof(buf), "Capsule", 0x0af9);
 
 	ut_asserteq_mem(expected, buf, sizeof(expected));
 	ut_asserteq(pos - buf, u16_strnlen(buf, SIZE_MAX));
-- 
2.30.0

[PATCH 4/8 v2] 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 file paths stored in EFI variables.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/efi_helper.h        |  28 ++++++
 lib/efi_loader/efi_helper.c | 180 ++++++++++++++++++++++++++++++++++++
 2 files changed, 208 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..5d3a43364619
--- /dev/null
+++ b/include/efi_helper.h
@@ -0,0 +1,28 @@
+/* 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 char *name, struct load_file_info *loc);
+void *get_var_efi(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..e5919343a5cc
--- /dev/null
+++ b/lib/efi_loader/efi_helper.c
@@ -0,0 +1,180 @@
+// 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));
+	info->dev[sizeof(info->dev) - 1] = 0;
+
+	p = strsep(&args, " ");
+	if (!p) {
+		status = EFI_NO_MEDIA;
+		goto out;
+	}
+	strncpy(info->part, p, sizeof(info->part));
+	info->part[sizeof(info->part) - 1] = 0;
+
+	p = strsep(&args, " ");
+	if (!p) {
+		status = EFI_NOT_FOUND;
+		goto out;
+	}
+	strncpy(info->filename, p, sizeof(info->filename));
+	info->filename[sizeof(info->filename) - 1] = 0;
+
+out:
+	return status;
+}
+
+/**
+ * get_var_efi() - 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_efi(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
+ * @info:	struct to fill in with file specific info
+ */
+efi_status_t efi_get_fp_from_var(const char *name, struct load_file_info *info)
+{
+	efi_uintn_t boot_cur_size;
+	void *var_value = NULL;
+	efi_uintn_t size;
+	efi_status_t ret;
+	u16 boot_cur;
+	u16 var_name[16];
+	u16 *pos;
+
+	memset(info, 0, sizeof(*info));
+
+	boot_cur_size = sizeof(boot_cur);
+	ret = efi_get_variable_int(L"BootCurrent",
+				   &efi_global_variable_guid, NULL,
+				   &boot_cur_size, &boot_cur, NULL);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	pos = efi_create_indexed_name(var_name, sizeof(var_name), name,
+				      boot_cur);
+	if (!pos) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
+	var_value = get_var_efi(var_name, &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);
+	return ret;
+}
-- 
2.30.0

[PATCH 5/8 v2] 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_efi()
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 | 37 +++---------------------------------
 2 files changed, 4 insertions(+), 35 deletions(-)

diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index cd4b252a417c..a47160189c42 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 61dc72a23da8..0221fd1261b3 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_variable.h>
 #include <asm/unaligned.h>
@@ -165,38 +166,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
  *
@@ -224,7 +193,7 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
 	varname[6] = hexmap[(n & 0x00f0) >> 4];
 	varname[7] = hexmap[(n & 0x000f) >> 0];
 
-	load_option = get_var(varname, &efi_global_variable_guid, &size);
+	load_option = get_var_efi(varname, &efi_global_variable_guid, &size);
 	if (!load_option)
 		return EFI_LOAD_ERROR;
 
@@ -336,7 +305,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
 	}
 
 	/* BootOrder */
-	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
+	bootorder = get_var_efi(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder) {
 		log_info("BootOrder not defined\n");
 		ret = EFI_NOT_FOUND;
-- 
2.30.0

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

[Ilias Apalodimas]

Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
unconditionally. Although we correctly return various EFI exit 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 installed.

This creates a problem for EFI installers, since they won't be able to
load their own initrd and continue the installation. It also makes the 
feature hard to use, since we can either have a single initrd or we have
to recompile u-boot if the filename changes.

So let's introduce a different logic that will decouple the initrd
path from the config option we currently have.
When the EFI application is launched through the bootmgr, we'll try to
match the BootCurrent value to an Initrd#### EFI variable.
e.g 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 install the
appropriate protocol so the kernel's efi-stub can use it and 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 installed.

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>
---
 cmd/bootefi.c                    | 13 +++++
 include/efi_loader.h             |  2 +-
 lib/efi_loader/Kconfig           | 13 ++---
 lib/efi_loader/efi_load_initrd.c | 93 ++++++++++----------------------
 4 files changed, 46 insertions(+), 75 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index fdf909f8da2c..0234b0df2258 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -377,6 +377,19 @@ static int do_efibootmgr(void)
 		return CMD_RET_FAILURE;
 	}
 
+	/* efi_exit() will delete all protocols and the handle itself on exit */
+	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
+		ret = efi_initrd_register(&handle);
+		/* Just warn the user. If the installation fails, the kernel
+		 * either load an initrd specificied in the cmdline or fail
+		 * to boot.
+		 * If we decide to fail the command here we need to uninstall
+		 * the protocols efi_bootmgr_load() installed
+		 */
+		if (ret != EFI_SUCCESS)
+			log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n");
+	}
+
 	ret = do_bootefi_exec(handle, load_options);
 
 	if (ret != EFI_SUCCESS)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index af30dbafab77..000cc942e31c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void);
 efi_status_t efi_net_register(void);
 /* Called by bootefi to make the watchdog available */
 efi_status_t efi_watchdog_register(void);
-efi_status_t efi_initrd_register(void);
+efi_status_t efi_initrd_register(efi_handle_t *initrd_handle);
 /* Called by bootefi to make SMBIOS tables available */
 /**
  * efi_acpi_register() - write out ACPI tables
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index dd8b93bd3c5a..96b5934a221d 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -212,14 +212,11 @@ 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
+	  installed 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 EFI variable format should be '<dev> <part> <filename>'
+	  i.e 'mmc 0:1 boot/initrd'
 
 config EFI_SECURE_BOOT
 	bool "Enable EFI secure boot support"
diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
index 4ec55d70979d..6289957c97bc 100644
--- a/lib/efi_loader/efi_load_initrd.c
+++ b/lib/efi_loader/efi_load_initrd.c
@@ -5,7 +5,9 @@
 
 #include <common.h>
 #include <efi_loader.h>
+#include <efi_helper.h>
 #include <efi_load_initrd.h>
+#include <efi_variable.h>
 #include <fs.h>
 #include <malloc.h>
 #include <mapmem.h>
@@ -43,40 +45,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.
@@ -96,21 +65,15 @@ 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;
+	char initrd[] = "Initrd";
 
 	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;
@@ -128,24 +91,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(initrd, &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;
 
@@ -153,23 +103,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);
 }
 
@@ -183,13 +135,22 @@ out:
  *
  * Return:	status code
  */
-efi_status_t efi_initrd_register(void)
+efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle)
 {
-	efi_handle_t efi_initrd_handle = NULL;
 	efi_status_t ret;
+	struct load_file_info info;
+	char initrd[] = "Initrd";
+
+	ret = efi_get_fp_from_var(initrd, &info);
+	/*
+	 * Don't fail here. If we don't install 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,
+		       (efi_initrd_handle,
 			/* initramfs */
 			&efi_guid_device_path, &dp,
 			/* LOAD_FILE2 */
-- 
2.30.0

[PATCH 7/8 v2] efi_selftest: Modify self-tests for initrd loading via Loadfile2

[Ilias Apalodimas]

We changed the logic of the initrd discovery and the installation of the 
protocol in the previous patch.
Instead of a config now option it now resides in an EFI variable. Adjust
the existing tests accordingly and add self-tests which will cover the new 
features.

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

diff --git a/lib/efi_selftest/efi_selftest_load_initrd.c b/lib/efi_selftest/efi_selftest_load_initrd.c
index fe060a664402..1534c2ba24fa 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 = {
@@ -76,10 +79,13 @@ static struct efi_initrd_dp dp_invalid = {
 	}
 };
 
+static efi_handle_t handle;
+
 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,16 +93,97 @@ 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;
-	efi_handle_t handle;
 	char buffer[64];
 	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 install the protocol during efibootmgr */
+	status = efi_initrd_register(&handle);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to install initrd protocol\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/*
+	 * We should only install 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 installed\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-install with invalid file path */
+	status = efi_initrd_register(&handle);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to install 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 installed\n");
+		return EFI_ST_FAILURE;
+	}
+
+	/* re-install 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(&handle);
+	if (status != EFI_SUCCESS) {
+		efi_st_error("Failed to install initrd protocol\n");
+		return EFI_ST_FAILURE;
+	}
 
 	dp2 = (struct efi_device_path *)&dp;
 	status = boottime->locate_device_path(&lf2_proto_guid, &dp2, &handle);
@@ -211,10 +298,17 @@ static int execute(void)
 	return EFI_ST_SUCCESS;
 }
 
+static int teardown(void)
+{
+	efi_delete_handle(handle);
+	return EFI_ST_SUCCESS;
+}
+
 EFI_UNIT_TEST(load_initrd) = {
 	.name = "load initrd",
 	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
 	.setup = setup,
 	.execute = execute,
+	.teardown = teardown,
 	.on_request = true,
 };
-- 
2.30.0

[PATCH 8/8 v2] doc: uefi: Add instruction for initrd loading

[Ilias Apalodimas]

Add a description of the EFI variables needed to match a Boot####
entry with an initrd.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 doc/uefi/uefi.rst | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
index dc930d924022..d8b083ffa708 100644
--- a/doc/uefi/uefi.rst
+++ b/doc/uefi/uefi.rst
@@ -229,6 +229,21 @@ UEFI variables. Booting according to these variables is possible via::
 As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
 command 'efidebug' can be used to set the variables.
 
+Initrd with the boot manager
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Kernel versions >= 5.6 can use EFI_LOAD_FILE2_PROTOCOL to load an initramfs.
+When U-Boot is configured with CONFIG_EFI_LOAD_FILE2_INITRD=y the boot manager
+will install the protocol, if an EFI variable matching the BootCurrent value
+is found and contains a valid file path. The EFI variable name is 'Initrd####'
+and the file path format is '<device> <partition> <filename>'.
+
+This allows users to pair a kernel with a specific initramfs.
+
+Example:
+Boot0010 will search for Initrd0010 and try to install the protocol with
+the file path specified in Initrd0010.
+
 Executing the built in hello world application
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.30.0

[PATCH 2/8 v2] efi_loader: Remove unconditional installation of file2 protocol for initrd

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Up to now we install the EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI exit 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 installed.
>
> 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 install
> the required protocol which the kernel's efi-stub can use and load our
> initrd.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

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

[PATCH 1/8 v2] efi_loader: Remove unused headers from efi_load_initrd.c

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> dm.h and env.h serve no purpose here. Remove them and sort the
> remaining in alphabetical order.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Please, rebase upon origin/next.

See commit b6f11098c9a619f480
efi_loader: move EFI_LOAD_FILE2_PROTOCOL_GUID

> ---
>   lib/efi_loader/efi_load_initrd.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index d517d686c330..4ec55d70979d 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 <fs.h>
> +#include <malloc.h>
> +#include <mapmem.h>
>
>   static const efi_guid_t efi_guid_load_file2_protocol =
>   		EFI_LOAD_FILE2_PROTOCOL_GUID;
>

[PATCH 3/8 v2] efi_loader: Add size checks to efi_create_indexed_name()

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Although the function description states the caller must provide a
> sufficient buffer, it's better to have in function checks and ensure
> the destination buffer can hold the intended variable name.
>
> So let's add an extra argument with the buffer size and check that
> before copying.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   include/efi_loader.h        |  3 ++-
>   lib/efi_loader/efi_string.c | 10 ++++++++--
>   test/unicode_ut.c           |  2 +-
>   3 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 3c68b85b68e9..af30dbafab77 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -810,7 +810,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
>   void efi_memcpy_runtime(void *dest, const void *src, size_t n);
>
>   /* commonly used helper function */
> -u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index);
> +u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
> +			     unsigned int index);
>
>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */

Please, rebase upon origin/next.

With this patch U-Boot does not compile:

lib/efi_loader/efi_capsule.c: In function ?set_capsule_result?:
lib/efi_loader/efi_capsule.c:76:43: error: passing argument 2 of
?efi_create_indexed_name? makes integer from pointer without a cast
[-Werror=int-conversion]
    76 |  efi_create_indexed_name(variable_name16, "Capsule", index);
       |                                           ^~~~~~~~~
       |                                           |
       |                                           char *

You missed to update lib/efi_loader/efi_capsule.c as you series is based
on origin/master.

Best regards

Heinrich

>
> diff --git a/lib/efi_loader/efi_string.c b/lib/efi_loader/efi_string.c
> index 3de721f06c7f..962724228866 100644
> --- a/lib/efi_loader/efi_string.c
> +++ b/lib/efi_loader/efi_string.c
> @@ -23,13 +23,19 @@
>    * Return: A pointer to the next position after the created string
>    *	   in @buffer, or NULL otherwise
>    */
> -u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index)
> +u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
> +			     unsigned int index)
>   {
>   	u16 *p = buffer;
>   	char index_buf[5];
> +	size_t size;
>
> +	size = (utf8_utf16_strlen(name) * sizeof(u16) +
> +		sizeof(index_buf) * sizeof(u16));
> +	if (buffer_size < size)
> +		return NULL;
>   	utf8_utf16_strcpy(&p, name);
> -	sprintf(index_buf, "%04X", index);
> +	snprintf(index_buf, sizeof(index_buf), "%04X", index);
>   	utf8_utf16_strcpy(&p, index_buf);
>
>   	return p;
> diff --git a/test/unicode_ut.c b/test/unicode_ut.c
> index 33fc8b0ee1e2..6130ef0b5497 100644
> --- a/test/unicode_ut.c
> +++ b/test/unicode_ut.c
> @@ -603,7 +603,7 @@ static int unicode_test_efi_create_indexed_name(struct unit_test_state *uts)
>   	u16 *pos;
>
>   	memset(buf, 0xeb, sizeof(buf));
> -	pos = efi_create_indexed_name(buf, "Capsule", 0x0af9);
> +	pos = efi_create_indexed_name(buf, sizeof(buf), "Capsule", 0x0af9);
>
>   	ut_asserteq_mem(expected, buf, sizeof(expected));
>   	ut_asserteq(pos - buf, u16_strnlen(buf, SIZE_MAX));
>

[PATCH 4/8 v2] efi_loader: Introduce helper functions for EFI

[Heinrich Schuchardt]

On 12/30/20 4:07 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 file paths stored in EFI variables.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   include/efi_helper.h        |  28 ++++++
>   lib/efi_loader/efi_helper.c | 180 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 208 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..5d3a43364619
> --- /dev/null
> +++ b/include/efi_helper.h
> @@ -0,0 +1,28 @@
> +/* 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];

if_typename_str[] contains all available strings. 8 bytes are long
enough. The value can be validated with blk_get_device_by_str().

> +	char part[16];

Windows allows 128 partitions per device. I would not expect millions of
devices. 8 bytes should be long enough.

> +	char filename[256];

This is a path not a file name. Please, adjust the parameter name.

In Windows the maximum length of a path is 260 characters. But does such
a limit exist in UEFI?

In U-Boot we have:
include/ext_common.h:33:#define EXT2_PATH_MAX 4096

So you cannot assume that the path is shorter then 4096 bytes.

I think the best thing to do is to use strdup() and then put 0x00 at the
end of each string part. How about:

struct load_file_info {
	/*
	 * duplicated filespec, to be freed after usage
	 * 0x00 separated device, part, path
	 */
	char *filespec;
	char *part;
	char *filepath;
}

So filespec would point to a buffer containing:

device\0   part\0   \path\file\0    \0
|          |        |- filepath points here
|          |- part points here
|- filespec points here

> +};
> +
> +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 char *name, struct load_file_info *loc);
> +void *get_var_efi(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..e5919343a5cc
> --- /dev/null
> +++ b/lib/efi_loader/efi_helper.c
> @@ -0,0 +1,180 @@
> +// 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"
> +	 */

Shouldn't you skip over leading spaces?

> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NO_MEDIA;
> +		goto out;
> +	}
> +	strncpy(info->dev, p, sizeof(info->dev));
> +	info->dev[sizeof(info->dev) - 1] = 0;
> +
> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NO_MEDIA;
> +		goto out;
> +	}

Don't make any assumptions about the number of spaces between the file
specification parts:

"     host      0:1       /dir1/dir12/filename     "

We should use strdup() and make_argv().

> +	strncpy(info->part, p, sizeof(info->part));
> +	info->part[sizeof(info->part) - 1] = 0;
> +
> +	p = strsep(&args, " ");
> +	if (!p) {
> +		status = EFI_NOT_FOUND;
> +		goto out;
> +	}
> +	strncpy(info->filename, p, sizeof(info->filename));
> +	info->filename[sizeof(info->filename) - 1] = 0;
> +
> +out:
> +	return status;
> +}
> +
> +/**
> + * get_var_efi() - 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_efi(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
> + * @info:	struct to fill in with file specific info
> + */
> +efi_status_t efi_get_fp_from_var(const char *name, struct load_file_info *info)
> +{
> +	efi_uintn_t boot_cur_size;
> +	void *var_value = NULL;
> +	efi_uintn_t size;
> +	efi_status_t ret;
> +	u16 boot_cur;
> +	u16 var_name[16];
> +	u16 *pos;
> +
> +	memset(info, 0, sizeof(*info));
> +
> +	boot_cur_size = sizeof(boot_cur);
> +	ret = efi_get_variable_int(L"BootCurrent",

Please, put some more text into the function description indicating the
usage of BootCurrent.

> +				   &efi_global_variable_guid, NULL,
> +				   &boot_cur_size, &boot_cur, NULL);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	pos = efi_create_indexed_name(var_name, sizeof(var_name), name,
> +				      boot_cur);
> +	if (!pos) {
> +		ret = EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
> +	var_value = get_var_efi(var_name, &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);
> +	return ret;
> +}
>

Best regards

Heinrich

[PATCH 5/8 v2] efi_loader: bootmgr: Use get_var from efi_helper file

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> A few patches before we introduced a file which includes the get_var_efi()
> 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 | 37 +++---------------------------------
>   2 files changed, 4 insertions(+), 35 deletions(-)
>
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index cd4b252a417c..a47160189c42 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 61dc72a23da8..0221fd1261b3 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_variable.h>
>   #include <asm/unaligned.h>
> @@ -165,38 +166,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
>    *
> @@ -224,7 +193,7 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>   	varname[6] = hexmap[(n & 0x00f0) >> 4];
>   	varname[7] = hexmap[(n & 0x000f) >> 0];
>
> -	load_option = get_var(varname, &efi_global_variable_guid, &size);
> +	load_option = get_var_efi(varname, &efi_global_variable_guid, &size);
>   	if (!load_option)
>   		return EFI_LOAD_ERROR;
>
> @@ -336,7 +305,7 @@ efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options)
>   	}
>
>   	/* BootOrder */
> -	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> +	bootorder = get_var_efi(L"BootOrder", &efi_global_variable_guid, &size);

efi_var_get() would be more in line with our naming of other UEFI
related functions.

Best regards

Heinrich

>   	if (!bootorder) {
>   		log_info("BootOrder not defined\n");
>   		ret = EFI_NOT_FOUND;
>

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

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI exit 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 installed.
>
> This creates a problem for EFI installers, since they won't be able to
> load their own initrd and continue the installation. It also makes the
> feature hard to use, since we can either have a single initrd or we have
> to recompile u-boot if the filename changes.
>
> So let's introduce a different logic that will decouple the initrd
> path from the config option we currently have.
> When the EFI application is launched through the bootmgr, we'll try to
> match the BootCurrent value to an Initrd#### EFI variable.
> e.g 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 install the
> appropriate protocol so the kernel's efi-stub can use it and 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 installed.
>
> 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>
> ---
>   cmd/bootefi.c                    | 13 +++++
>   include/efi_loader.h             |  2 +-
>   lib/efi_loader/Kconfig           | 13 ++---
>   lib/efi_loader/efi_load_initrd.c | 93 ++++++++++----------------------
>   4 files changed, 46 insertions(+), 75 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index fdf909f8da2c..0234b0df2258 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -377,6 +377,19 @@ static int do_efibootmgr(void)
>   		return CMD_RET_FAILURE;
>   	}
>
> +	/* efi_exit() will delete all protocols and the handle itself on exit */
> +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> +		ret = efi_initrd_register(&handle);
> +		/* Just warn the user. If the installation fails, the kernel
> +		 * either load an initrd specificied in the cmdline or fail
> +		 * to boot.
> +		 * If we decide to fail the command here we need to uninstall
> +		 * the protocols efi_bootmgr_load() installed
> +		 */
> +		if (ret != EFI_SUCCESS)
> +			log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n");
> +	}
> +
>   	ret = do_bootefi_exec(handle, load_options);
>
>   	if (ret != EFI_SUCCESS)
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index af30dbafab77..000cc942e31c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void);
>   efi_status_t efi_net_register(void);
>   /* Called by bootefi to make the watchdog available */
>   efi_status_t efi_watchdog_register(void);
> -efi_status_t efi_initrd_register(void);
> +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle);
>   /* Called by bootefi to make SMBIOS tables available */
>   /**
>    * efi_acpi_register() - write out ACPI tables
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index dd8b93bd3c5a..96b5934a221d 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -212,14 +212,11 @@ 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
> +	  installed 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 EFI variable format should be '<dev> <part> <filename>'
> +	  i.e 'mmc 0:1 boot/initrd'
>
>   config EFI_SECURE_BOOT
>   	bool "Enable EFI secure boot support"
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 4ec55d70979d..6289957c97bc 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -5,7 +5,9 @@
>
>   #include <common.h>
>   #include <efi_loader.h>
> +#include <efi_helper.h>
>   #include <efi_load_initrd.h>
> +#include <efi_variable.h>
>   #include <fs.h>
>   #include <malloc.h>
>   #include <mapmem.h>
> @@ -43,40 +45,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.

Please, describe how UEFI variables are used here.

> @@ -96,21 +65,15 @@ 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;
> +	char initrd[] = "Initrd";
>
>   	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;
> @@ -128,24 +91,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(initrd, &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;
>
> @@ -153,23 +103,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);
>   }
>
> @@ -183,13 +135,22 @@ out:
>    *
>    * Return:	status code
>    */
> -efi_status_t efi_initrd_register(void)
> +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle)

Please, describe the new function parameter.

Best regards

Heinrich

>   {
> -	efi_handle_t efi_initrd_handle = NULL;
>   	efi_status_t ret;
> +	struct load_file_info info;
> +	char initrd[] = "Initrd";
> +
> +	ret = efi_get_fp_from_var(initrd, &info);
> +	/*
> +	 * Don't fail here. If we don't install 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,
> +		       (efi_initrd_handle,
>   			/* initramfs */
>   			&efi_guid_device_path, &dp,
>   			/* LOAD_FILE2 */
>

[PATCH 8/8 v2] doc: uefi: Add instruction for initrd loading

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Add a description of the EFI variables needed to match a Boot####
> entry with an initrd.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   doc/uefi/uefi.rst | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)
>
> diff --git a/doc/uefi/uefi.rst b/doc/uefi/uefi.rst
> index dc930d924022..d8b083ffa708 100644
> --- a/doc/uefi/uefi.rst
> +++ b/doc/uefi/uefi.rst
> @@ -229,6 +229,21 @@ UEFI variables. Booting according to these variables is possible via::
>   As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
>   command 'efidebug' can be used to set the variables.
>
> +Initrd with the boot manager
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Kernel versions >= 5.6 can use EFI_LOAD_FILE2_PROTOCOL to load an initramfs.

%s/can use EFI_LOAD_FILE2_PROTOCOL/can use the Load File 2 protocol/

> +When U-Boot is configured with CONFIG_EFI_LOAD_FILE2_INITRD=y the boot manager
> +will install the protocol, if an EFI variable matching the BootCurrent value
> +is found and contains a valid file path. The EFI variable name is 'Initrd####'

When booting via the UEFI boot manager (command 'bootefi bootmgr')
BootNext and BootOrder are used to identify a boot option to boot. The
variable BootCurrent is set to the hexadecimal number of the selected
boot option, e.g. to 0x001E if the image specified by Boot001E is selected.

When U-Boot is configured with CONFIG_EFI_LOAD_FILE2_INITRD=y the boot
manager will install the Load File 2 protocol if an EFI variable
Initrd#### (e.g. Initrd001E) is found where #### matches the value of
BootCurrent and Initrd#### contains a valid file path.

> +and the file path format is '<device> <partition> <filename>'.

The fist string is not a device but a class of devices.
The third string is not a filename but a file path.

<interface> <device>[:<partition>] <filepath>.

partition defaults to 0 (whole disk).

The user should be taught that the value of Initrd#### is a UTF8 string.

An example string should be provided.

> +
> +This allows users to pair a kernel with a specific initramfs.
> +
> +Example:
> +Boot0010 will search for Initrd0010 and try to install the protocol with
> +the file path specified in Initrd0010.

"When booting Boot001E U-Boot tries to install the Load File 2 protocol
with the file specified by Initrd001E. If the intird is not found it
will continue without init.

What happens if Initrd#### does not point to a valid file? Shouldn't we
then skip the boot option and continue with the next boot option?

Please, show how to set Initrd0010 from the console, e.g.

setenv -e -nv -bs -rt Initrd001E 'scsi 0:1 initrd'

Best regards

Heinrich

> +
>   Executing the built in hello world application
>   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
>

[PATCH 7/8 v2] efi_selftest: Modify self-tests for initrd loading via Loadfile2

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> We changed the logic of the initrd discovery and the installation of the
> protocol in the previous patch.
> Instead of a config now option it now resides in an EFI variable. Adjust
> the existing tests accordingly and add self-tests which will cover the new
> features.

A full test would comprise installing a block device of type EFI
containing a binary and an initial ram disk.

The loaded binary could check the content of the ram disk. E.g. the
CRC32 and the length could be returned via exit data by the binary.

Have a look at lib/efi_selftest/efi_selftest_block_device.c.

Best regards

Heinrich

>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   lib/efi_selftest/efi_selftest_load_initrd.c | 100 +++++++++++++++++++-
>   1 file changed, 97 insertions(+), 3 deletions(-)
>
> diff --git a/lib/efi_selftest/efi_selftest_load_initrd.c b/lib/efi_selftest/efi_selftest_load_initrd.c
> index fe060a664402..1534c2ba24fa 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 = {
> @@ -76,10 +79,13 @@ static struct efi_initrd_dp dp_invalid = {
>   	}
>   };
>
> +static efi_handle_t handle;
> +
>   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,16 +93,97 @@ 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;
> -	efi_handle_t handle;
>   	char buffer[64];
>   	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 install the protocol during efibootmgr */
> +	status = efi_initrd_register(&handle);
> +	if (status != EFI_SUCCESS) {
> +		efi_st_error("Failed to install initrd protocol\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	/*
> +	 * We should only install 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 installed\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-install with invalid file path */
> +	status = efi_initrd_register(&handle);
> +	if (status != EFI_SUCCESS) {
> +		efi_st_error("Failed to install 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 installed\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	/* re-install 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(&handle);
> +	if (status != EFI_SUCCESS) {
> +		efi_st_error("Failed to install initrd protocol\n");
> +		return EFI_ST_FAILURE;
> +	}
>
>   	dp2 = (struct efi_device_path *)&dp;
>   	status = boottime->locate_device_path(&lf2_proto_guid, &dp2, &handle);
> @@ -211,10 +298,17 @@ static int execute(void)
>   	return EFI_ST_SUCCESS;
>   }
>
> +static int teardown(void)
> +{
> +	efi_delete_handle(handle);
> +	return EFI_ST_SUCCESS;
> +}
> +
>   EFI_UNIT_TEST(load_initrd) = {
>   	.name = "load initrd",
>   	.phase = EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
>   	.setup = setup,
>   	.execute = execute,
> +	.teardown = teardown,
>   	.on_request = true,
>   };
>

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

[Heinrich Schuchardt]

On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> Hi!
>
> This is v2 of [1].
>
> There's been a couple of changes regarding where we install the protocol.
> The initial patchset was completely disregarding BootNext, so that's taken
> into account now and we can use the new feature.
> This brought a few changes on the selftests as well, since we now use the
> loaded image handle to install the protocol, as a consequence the custom
> handle in the tests is now uninstalled during the test .teardown(), but
> the overall approach remains identical.

Boot#### contains a device path.

Why should Initrd#### contain something U-Boot specific and not a device
path?

Think of a Linux distribution. When updating the kernel it will have to
write Boot#### and Initrd####. It can determine the current boot device
via BootCurrent and Boot####. Next it can add the new file paths for
Boot#### and Initrd#### relative to the same device.

Best regards

Heinrich

>
> Changes since v1:
> - Use efi_create_indexed_name() instead of open coding it. An extra
>    patch has been applied to efi_create_indexed_name() to ensure the
>    destination buffer is big enough for our variable name.
> - Install the protocol during do_efibootmgr() on the loaded image
>    handle and adjust selftests accordingly. The protocol will be
>    uninstalled if efi_exit() is called from and EFI app now.
> - Make sure strings are null terminated in efi_helper.c
> - Added documentation in uefi.rst describing the new feature.
>
> [1] https://lists.denx.de/pipermail/u-boot/2020-December/436080.html
>
> Ilias Apalodimas (8):
>    efi_loader: Remove unused headers from efi_load_initrd.c
>    efi_loader: Remove unconditional installation of file2 protocol for
>      initrd
>    efi_loader: Add size checks to efi_create_indexed_name()
>    efi_loader: Introduce helper functions for EFI
>    efi_loader: bootmgr: Use get_var from efi_helper file
>    efi_loader: Replace config option with EFI variable for initrd loading
>    efi_selftest: Modify self-tests for initrd loading via Loadfile2
>    doc: uefi: Add instruction for initrd loading
>
>   cmd/bootefi.c                               |  13 ++
>   doc/uefi/uefi.rst                           |  15 ++
>   include/efi_helper.h                        |  28 +++
>   include/efi_loader.h                        |   5 +-
>   lib/efi_loader/Kconfig                      |  13 +-
>   lib/efi_loader/Makefile                     |   2 +-
>   lib/efi_loader/efi_bootmgr.c                |  37 +---
>   lib/efi_loader/efi_helper.c                 | 180 ++++++++++++++++++++
>   lib/efi_loader/efi_load_initrd.c            | 101 ++++-------
>   lib/efi_loader/efi_setup.c                  |   5 -
>   lib/efi_loader/efi_string.c                 |  10 +-
>   lib/efi_selftest/efi_selftest_load_initrd.c | 100 ++++++++++-
>   test/unicode_ut.c                           |   2 +-
>   13 files changed, 384 insertions(+), 127 deletions(-)
>   create mode 100644 include/efi_helper.h
>   create mode 100644 lib/efi_loader/efi_helper.c
>

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

[Ilias Apalodimas]

Hi Heinrich, 

On Wed, Dec 30, 2020 at 09:44:39PM +0100, Heinrich Schuchardt wrote:
> On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> > Hi!
> > 
> > This is v2 of [1].
> > 
> > There's been a couple of changes regarding where we install the protocol.
> > The initial patchset was completely disregarding BootNext, so that's taken
> > into account now and we can use the new feature.
> > This brought a few changes on the selftests as well, since we now use the
> > loaded image handle to install the protocol, as a consequence the custom
> > handle in the tests is now uninstalled during the test .teardown(), but
> > the overall approach remains identical.
> 
> Boot#### contains a device path.
> 
> Why should Initrd#### contain something U-Boot specific and not a device
> path?

Because there was no standard on it, since we used the path as a config option
up to now I just kept it as is.

> 
> Think of a Linux distribution. When updating the kernel it will have to
> write Boot#### and Initrd####. It can determine the current boot device
> via BootCurrent and Boot####. Next it can add the new file paths for
> Boot#### and Initrd#### relative to the same device.

This isn't a bad idea. Let me update the patch and send a v3.

Cheers
/Ilias

[PATCH 4/8 v2] efi_loader: Introduce helper functions for EFI

[Ilias Apalodimas]

On Wed, Dec 30, 2020 at 08:29:54PM +0100, Heinrich Schuchardt wrote:
> On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> > A following patch introduces a different logic for loading initrd's

[...]

> > +struct load_file_info {
> > +	char dev[32];
> 
> if_typename_str[] contains all available strings. 8 bytes are long
> enough. The value can be validated with blk_get_device_by_str().
> 
> > +	char part[16];
> 
> Windows allows 128 partitions per device. I would not expect millions of
> devices. 8 bytes should be long enough.
> 
> > +	char filename[256];
> 
> This is a path not a file name. Please, adjust the parameter name.
> 
> In Windows the maximum length of a path is 260 characters. But does such
> a limit exist in UEFI?
> 
> In U-Boot we have:
> include/ext_common.h:33:#define EXT2_PATH_MAX 4096
> 
> So you cannot assume that the path is shorter then 4096 bytes.

This is defining something entirely differnt though. So I dont think we need
to adhere to any of these. 256 is perfectly sane for an initrd path.

> 
> I think the best thing to do is to use strdup() and then put 0x00 at the
> end of each string part. How about:
> 
> struct load_file_info {
> 	/*
> 	 * duplicated filespec, to be freed after usage
> 	 * 0x00 separated device, part, path
> 	 */
> 	char *filespec;
> 	char *part;
> 	char *filepath;
> }
> 
> So filespec would point to a buffer containing:
> 
> device\0   part\0   \path\file\0    \0
> |          |        |- filepath points here
> |          |- part points here
> |- filespec points here
> 

I generally try to avoid functions that need free() after the usagem since
they tend to be error prone. 
In any case this whole thing we probably go away if we just store a device
path in initrd. So let me have a look and see were we'll end up.

[...]

Cheers
/Ilias

[PATCH 3/8 v2] efi_loader: Add size checks to efi_create_indexed_name()

[Ilias Apalodimas]

On Wed, Dec 30, 2020 at 07:34:38PM +0100, Heinrich Schuchardt wrote:
> On 12/30/20 4:07 PM, Ilias Apalodimas wrote:
> > Although the function description states the caller must provide a
> > sufficient buffer, it's better to have in function checks and ensure
> > the destination buffer can hold the intended variable name.
> > 
> > So let's add an extra argument with the buffer size and check that
> > before copying.
> > 
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   include/efi_loader.h        |  3 ++-
> >   lib/efi_loader/efi_string.c | 10 ++++++++--
> >   test/unicode_ut.c           |  2 +-
> >   3 files changed, 11 insertions(+), 4 deletions(-)
> > 
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 3c68b85b68e9..af30dbafab77 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -810,7 +810,8 @@ bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
> >   void efi_memcpy_runtime(void *dest, const void *src, size_t n);
> > 
> >   /* commonly used helper function */
> > -u16 *efi_create_indexed_name(u16 *buffer, const char *name, unsigned int index);
> > +u16 *efi_create_indexed_name(u16 *buffer, size_t buffer_size, const char *name,
> > +			     unsigned int index);
> > 
> >   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> 
> Please, rebase upon origin/next.
> 
> With this patch U-Boot does not compile:
> 
> lib/efi_loader/efi_capsule.c: In function ?set_capsule_result?:
> lib/efi_loader/efi_capsule.c:76:43: error: passing argument 2 of
> ?efi_create_indexed_name? makes integer from pointer without a cast
> [-Werror=int-conversion]
>    76 |  efi_create_indexed_name(variable_name16, "Capsule", index);
>       |                                           ^~~~~~~~~
>       |                                           |
>       |                                           char *
> 
> You missed to update lib/efi_loader/efi_capsule.c as you series is based
> on origin/master.

Bah sorry!
I'll rebase this on top of master and send it as a seperate patchset.

Cheers
/Ilias

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

[AKASHI Takahiro]

On Wed, Dec 30, 2020 at 05:07:18PM +0200, Ilias Apalodimas wrote:
> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> unconditionally. Although we correctly return various EFI exit 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 installed.
> 
> This creates a problem for EFI installers, since they won't be able to
> load their own initrd and continue the installation. It also makes the 
> feature hard to use, since we can either have a single initrd or we have
> to recompile u-boot if the filename changes.
> 
> So let's introduce a different logic that will decouple the initrd
> path from the config option we currently have.
> When the EFI application is launched through the bootmgr, we'll try to
> match the BootCurrent value to an Initrd#### EFI variable.
> e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.

Do you think this semantics be kept U-boot specific?
It seems that it can work for any other EFI firmware (implementation).

> 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 install the

If so, the content of "Initrd####" should contain a generic EFI
representation of file path.
Please note that "Boot####" internally contains a flattened string of
"EFI device path" to the image while efidebug command accepts a style of
"mmc 0:1 image" as arguments to "efidebug boot add" sub-command.

-Takahiro Akashi

> appropriate protocol so the kernel's efi-stub can use it and 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 installed.
> 
> 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>
> ---
>  cmd/bootefi.c                    | 13 +++++
>  include/efi_loader.h             |  2 +-
>  lib/efi_loader/Kconfig           | 13 ++---
>  lib/efi_loader/efi_load_initrd.c | 93 ++++++++++----------------------
>  4 files changed, 46 insertions(+), 75 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index fdf909f8da2c..0234b0df2258 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -377,6 +377,19 @@ static int do_efibootmgr(void)
>  		return CMD_RET_FAILURE;
>  	}
>  
> +	/* efi_exit() will delete all protocols and the handle itself on exit */
> +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> +		ret = efi_initrd_register(&handle);
> +		/* Just warn the user. If the installation fails, the kernel
> +		 * either load an initrd specificied in the cmdline or fail
> +		 * to boot.
> +		 * If we decide to fail the command here we need to uninstall
> +		 * the protocols efi_bootmgr_load() installed
> +		 */
> +		if (ret != EFI_SUCCESS)
> +			log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n");
> +	}
> +
>  	ret = do_bootefi_exec(handle, load_options);
>  
>  	if (ret != EFI_SUCCESS)
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index af30dbafab77..000cc942e31c 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void);
>  efi_status_t efi_net_register(void);
>  /* Called by bootefi to make the watchdog available */
>  efi_status_t efi_watchdog_register(void);
> -efi_status_t efi_initrd_register(void);
> +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle);
>  /* Called by bootefi to make SMBIOS tables available */
>  /**
>   * efi_acpi_register() - write out ACPI tables
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index dd8b93bd3c5a..96b5934a221d 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -212,14 +212,11 @@ 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
> +	  installed 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 EFI variable format should be '<dev> <part> <filename>'
> +	  i.e 'mmc 0:1 boot/initrd'
>  
>  config EFI_SECURE_BOOT
>  	bool "Enable EFI secure boot support"
> diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> index 4ec55d70979d..6289957c97bc 100644
> --- a/lib/efi_loader/efi_load_initrd.c
> +++ b/lib/efi_loader/efi_load_initrd.c
> @@ -5,7 +5,9 @@
>  
>  #include <common.h>
>  #include <efi_loader.h>
> +#include <efi_helper.h>
>  #include <efi_load_initrd.h>
> +#include <efi_variable.h>
>  #include <fs.h>
>  #include <malloc.h>
>  #include <mapmem.h>
> @@ -43,40 +45,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.
> @@ -96,21 +65,15 @@ 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;
> +	char initrd[] = "Initrd";
>  
>  	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;
> @@ -128,24 +91,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(initrd, &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;
>  
> @@ -153,23 +103,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);
>  }
>  
> @@ -183,13 +135,22 @@ out:
>   *
>   * Return:	status code
>   */
> -efi_status_t efi_initrd_register(void)
> +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle)
>  {
> -	efi_handle_t efi_initrd_handle = NULL;
>  	efi_status_t ret;
> +	struct load_file_info info;
> +	char initrd[] = "Initrd";
> +
> +	ret = efi_get_fp_from_var(initrd, &info);
> +	/*
> +	 * Don't fail here. If we don't install 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,
> +		       (efi_initrd_handle,
>  			/* initramfs */
>  			&efi_guid_device_path, &dp,
>  			/* LOAD_FILE2 */
> -- 
> 2.30.0
> 

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

[Ilias Apalodimas]

Akashi san, 

On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote:
> On Wed, Dec 30, 2020 at 05:07:18PM +0200, Ilias Apalodimas wrote:
> > Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> > unconditionally. Although we correctly return various EFI exit 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 installed.
> > 
> > This creates a problem for EFI installers, since they won't be able to
> > load their own initrd and continue the installation. It also makes the 
> > feature hard to use, since we can either have a single initrd or we have
> > to recompile u-boot if the filename changes.
> > 
> > So let's introduce a different logic that will decouple the initrd
> > path from the config option we currently have.
> > When the EFI application is launched through the bootmgr, we'll try to
> > match the BootCurrent value to an Initrd#### EFI variable.
> > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.
> 
> Do you think this semantics be kept U-boot specific?
> It seems that it can work for any other EFI firmware (implementation).
> 

The GUID is very linux specific and it's meant to load an initrd, but I guess
any OS that uses a 'helper' file to load the final OS can be supported.

> > 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 install the
> 
> If so, the content of "Initrd####" should contain a generic EFI
> representation of file path.
> Please note that "Boot####" internally contains a flattened string of
> "EFI device path" to the image while efidebug command accepts a style of
> "mmc 0:1 image" as arguments to "efidebug boot add" sub-command.

Thanks for the pointers. I've already changed the patchset and using exactly 
what you described. This has another advantage, all the helper files for the 
string parsing previous patches introduce, went away as well.

Cheers
/Ilias
> 
> -Takahiro Akashi
> 
> > appropriate protocol so the kernel's efi-stub can use it and 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 installed.
> > 
> > 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>
> > ---
> >  cmd/bootefi.c                    | 13 +++++
> >  include/efi_loader.h             |  2 +-
> >  lib/efi_loader/Kconfig           | 13 ++---
> >  lib/efi_loader/efi_load_initrd.c | 93 ++++++++++----------------------
> >  4 files changed, 46 insertions(+), 75 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index fdf909f8da2c..0234b0df2258 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -377,6 +377,19 @@ static int do_efibootmgr(void)
> >  		return CMD_RET_FAILURE;
> >  	}
> >  
> > +	/* efi_exit() will delete all protocols and the handle itself on exit */
> > +	if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) {
> > +		ret = efi_initrd_register(&handle);
> > +		/* Just warn the user. If the installation fails, the kernel
> > +		 * either load an initrd specificied in the cmdline or fail
> > +		 * to boot.
> > +		 * If we decide to fail the command here we need to uninstall
> > +		 * the protocols efi_bootmgr_load() installed
> > +		 */
> > +		if (ret != EFI_SUCCESS)
> > +			log_warning("EFI boot manager: Failed to install Loadfile2 for initrd\n");
> > +	}
> > +
> >  	ret = do_bootefi_exec(handle, load_options);
> >  
> >  	if (ret != EFI_SUCCESS)
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index af30dbafab77..000cc942e31c 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -422,7 +422,7 @@ efi_status_t efi_gop_register(void);
> >  efi_status_t efi_net_register(void);
> >  /* Called by bootefi to make the watchdog available */
> >  efi_status_t efi_watchdog_register(void);
> > -efi_status_t efi_initrd_register(void);
> > +efi_status_t efi_initrd_register(efi_handle_t *initrd_handle);
> >  /* Called by bootefi to make SMBIOS tables available */
> >  /**
> >   * efi_acpi_register() - write out ACPI tables
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index dd8b93bd3c5a..96b5934a221d 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -212,14 +212,11 @@ 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
> > +	  installed 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 EFI variable format should be '<dev> <part> <filename>'
> > +	  i.e 'mmc 0:1 boot/initrd'
> >  
> >  config EFI_SECURE_BOOT
> >  	bool "Enable EFI secure boot support"
> > diff --git a/lib/efi_loader/efi_load_initrd.c b/lib/efi_loader/efi_load_initrd.c
> > index 4ec55d70979d..6289957c97bc 100644
> > --- a/lib/efi_loader/efi_load_initrd.c
> > +++ b/lib/efi_loader/efi_load_initrd.c
> > @@ -5,7 +5,9 @@
> >  
> >  #include <common.h>
> >  #include <efi_loader.h>
> > +#include <efi_helper.h>
> >  #include <efi_load_initrd.h>
> > +#include <efi_variable.h>
> >  #include <fs.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> > @@ -43,40 +45,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.
> > @@ -96,21 +65,15 @@ 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;
> > +	char initrd[] = "Initrd";
> >  
> >  	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;
> > @@ -128,24 +91,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(initrd, &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;
> >  
> > @@ -153,23 +103,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);
> >  }
> >  
> > @@ -183,13 +135,22 @@ out:
> >   *
> >   * Return:	status code
> >   */
> > -efi_status_t efi_initrd_register(void)
> > +efi_status_t efi_initrd_register(efi_handle_t *efi_initrd_handle)
> >  {
> > -	efi_handle_t efi_initrd_handle = NULL;
> >  	efi_status_t ret;
> > +	struct load_file_info info;
> > +	char initrd[] = "Initrd";
> > +
> > +	ret = efi_get_fp_from_var(initrd, &info);
> > +	/*
> > +	 * Don't fail here. If we don't install 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,
> > +		       (efi_initrd_handle,
> >  			/* initramfs */
> >  			&efi_guid_device_path, &dp,
> >  			/* LOAD_FILE2 */
> > -- 
> > 2.30.0
> > 

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

[Ilias Apalodimas]

On Tue, 5 Jan 2021 at 10:50, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Akashi san,
>
> On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote:
> > On Wed, Dec 30, 2020 at 05:07:18PM +0200, Ilias Apalodimas wrote:
> > > Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
> > > unconditionally. Although we correctly return various EFI exit 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 installed.
> > >
> > > This creates a problem for EFI installers, since they won't be able to
> > > load their own initrd and continue the installation. It also makes the
> > > feature hard to use, since we can either have a single initrd or we have
> > > to recompile u-boot if the filename changes.
> > >
> > > So let's introduce a different logic that will decouple the initrd
> > > path from the config option we currently have.
> > > When the EFI application is launched through the bootmgr, we'll try to
> > > match the BootCurrent value to an Initrd#### EFI variable.
> > > e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.
> >
> > Do you think this semantics be kept U-boot specific?
> > It seems that it can work for any other EFI firmware (implementation).
> >
>
> The GUID is very linux specific and it's meant to load an initrd, but I guess
> any OS that uses a 'helper' file to load the final OS can be supported.
>
> > > 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 install the
> >
> > If so, the content of "Initrd####" should contain a generic EFI
> > representation of file path.
> > Please note that "Boot####" internally contains a flattened string of
> > "EFI device path" to the image while efidebug command accepts a style of
> > "mmc 0:1 image" as arguments to "efidebug boot add" sub-command.
>
> Thanks for the pointers. I've already changed the patchset and using exactly
> what you described. This has another advantage, all the helper files for the
> string parsing previous patches introduce, went away as well.
>

While I was trying to code this I came across a few things that we
need to resolve before posting my v3.
This feature is supposed to be very specific in linux so each OS
loader can decide on how to expose and
load the corresponding initrd.

Moving the contents to a device path adds more problems that it solves
at the moment.
First of all we'll be forced to use efi_load_image_from_file(), which
uses EFI_SIMPLE_FILESYSTEM_PROTOCOL
and EFI_FILE_PROTOCOL to eventually load the file. This has 2
problems. We'll have to place the initrd on the same
filesystem the image we load resides (as opposed to my patch where the
initrd can be anywhere).
Apart from that calling efi_load_image_from_file() will try to free
the memory on errors, but that memory is allocated
and managed by the efi-stub.

I'd prefer keeping the implementation as is, unbless someone has a better idea.

Thoughts?

Cheers
/Ilias


[...]

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

[Heinrich Schuchardt]

On 06.01.21 11:43, Ilias Apalodimas wrote:
> On Tue, 5 Jan 2021 at 10:50, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
>>
>> Akashi san,
>>
>> On Tue, Jan 05, 2021 at 11:42:22AM +0900, AKASHI Takahiro wrote:
>>> On Wed, Dec 30, 2020 at 05:07:18PM +0200, Ilias Apalodimas wrote:
>>>> Up to now we install EFI_LOAD_FILE2_PROTOCOL to load an initrd
>>>> unconditionally. Although we correctly return various EFI exit 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 installed.
>>>>
>>>> This creates a problem for EFI installers, since they won't be able to
>>>> load their own initrd and continue the installation. It also makes the
>>>> feature hard to use, since we can either have a single initrd or we have
>>>> to recompile u-boot if the filename changes.
>>>>
>>>> So let's introduce a different logic that will decouple the initrd
>>>> path from the config option we currently have.
>>>> When the EFI application is launched through the bootmgr, we'll try to
>>>> match the BootCurrent value to an Initrd#### EFI variable.
>>>> e.g Boot0000 -> Initrd0000, Boot0010 -> Initrd0010 etc.
>>>
>>> Do you think this semantics be kept U-boot specific?
>>> It seems that it can work for any other EFI firmware (implementation).
>>>
>>
>> The GUID is very linux specific and it's meant to load an initrd, but I guess
>> any OS that uses a 'helper' file to load the final OS can be supported.
>>
>>>> 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 install the
>>>
>>> If so, the content of "Initrd####" should contain a generic EFI
>>> representation of file path.
>>> Please note that "Boot####" internally contains a flattened string of
>>> "EFI device path" to the image while efidebug command accepts a style of
>>> "mmc 0:1 image" as arguments to "efidebug boot add" sub-command.
>>
>> Thanks for the pointers. I've already changed the patchset and using exactly
>> what you described. This has another advantage, all the helper files for the
>> string parsing previous patches introduce, went away as well.
>>
>
> While I was trying to code this I came across a few things that we
> need to resolve before posting my v3.
> This feature is supposed to be very specific in linux so each OS
> loader can decide on how to expose and
> load the corresponding initrd.
>
> Moving the contents to a device path adds more problems that it solves
> at the moment.
> First of all we'll be forced to use efi_load_image_from_file(), which

You are not obliged to call efi_load_image_from_file(). You could
implement your own function.

Or you could add a check *buffer!=NULL to detect a pre-allocated buffer
and neither allocate nor free the buffer in this case.

> uses EFI_SIMPLE_FILESYSTEM_PROTOCOL
> and EFI_FILE_PROTOCOL to eventually load the file. This has 2
> problems. We'll have to place the initrd on the same
> filesystem the image we load resides (as opposed to my patch where the
> initrd can be anywhere).

Given Boot#### contains

    dp1<sep>dp2<sep>dp3<end>

when calling efi_load_image_from_file() three times, once for dp1, dp2,
dp3, each of these device paths can point to a different block device.

Best regards

Heinrich

> Apart from that calling efi_load_image_from_file() will try to free
> the memory on errors, but that memory is allocated
> and managed by the efi-stub.
>
> I'd prefer keeping the implementation as is, unless someone has a better idea.
>
> Thoughts?
>
> Cheers
> /Ilias
>
>
> [...]
>

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

[Ilias Apalodimas]

> >>>

[...]

> >>> If so, the content of "Initrd####" should contain a generic EFI
> >>> representation of file path.
> >>> Please note that "Boot####" internally contains a flattened string of
> >>> "EFI device path" to the image while efidebug command accepts a style of
> >>> "mmc 0:1 image" as arguments to "efidebug boot add" sub-command.
> >>
> >> Thanks for the pointers. I've already changed the patchset and using exactly
> >> what you described. This has another advantage, all the helper files for the
> >> string parsing previous patches introduce, went away as well.
> >>
> >
> > While I was trying to code this I came across a few things that we
> > need to resolve before posting my v3.
> > This feature is supposed to be very specific in linux so each OS
> > loader can decide on how to expose and
> > load the corresponding initrd.
> >
> > Moving the contents to a device path adds more problems that it solves
> > at the moment.
> > First of all we'll be forced to use efi_load_image_from_file(), which
> 
> You are not obliged to call efi_load_image_from_file(). You could
> implement your own function.
> 
> Or you could add a check *buffer!=NULL to detect a pre-allocated buffer
> and neither allocate nor free the buffer in this case.

Right obliged was a poor choice of words. I was just trying to point out
issues with using the current U-Boot functions.

> 
> > uses EFI_SIMPLE_FILESYSTEM_PROTOCOL
> > and EFI_FILE_PROTOCOL to eventually load the file. This has 2
> > problems. We'll have to place the initrd on the same
> > filesystem the image we load resides (as opposed to my patch where the
> > initrd can be anywhere).
> 
> Given Boot#### contains
> 
>     dp1<sep>dp2<sep>dp3<end>

Let's clarify this a bit first so other people can join the discussion.

What I proposed yesterday to Heinrich, since we can use a device path
directly, is store it in the EFI_LOAD_OPTION we construct when we add the 
Boot#### variables. 
According to the EFI spec FilePathList is an array of device paths. 
The first element of the array is a device path that describes the 
device and location of the Image for this load option, but the rest are OS
specific, so we might as well use them instead of using a custom EFI variable.

Ok I'll send a v3 implementing this idea + a new function that can consume the 
device path and load a file without using EFI_SIMPLE_FILESYSTEM_PROTOCOL.
That would probablty solve all our problems.


Cheers
/Ilias


> 
> when calling efi_load_image_from_file() three times, once for dp1, dp2,
> dp3, each of these device paths can point to a different block device.
> 
> Best regards
> 
> Heinrich
> 
> > Apart from that calling efi_load_image_from_file() will try to free
> > the memory on errors, but that memory is allocated
> > and managed by the efi-stub.
> >
> > I'd prefer keeping the implementation as is, unless someone has a better idea.
> >
> > Thoughts?
> >
> > Cheers
> > /Ilias
> >
> >
> > [...]
> >
>