On 13.01.21 12:11, 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 load options.

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

include/efi_helper.h | 23 ++++++ lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++ 2 files changed, 169 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..4e6bd2f036df --- /dev/null +++ b/include/efi_helper.h @@ -0,0 +1,23 @@ +/* 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>

+enum load_option_dp_type {

• BOOT_IMAGE_DP,
• INITRD_DP,
• DTB_DP,

+};

+void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); +struct efi_device_path *efi_get_fp_from_boot(int idx); +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,

• 			       efi_uintn_t *size, int idx);
  

+#endif diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c new file mode 100644 index 000000000000..e2437a4f698b --- /dev/null +++ b/lib/efi_loader/efi_helper.c @@ -0,0 +1,146 @@ +// 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>

+/**

• • efi_get_var() - read value of an EFI variable
• • @name: variable name
• • @start: vendor GUID
• • @size: size of allocated buffer
• • Return: buffer with variable data or NULL
• */

+void *efi_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);
  

Please, always check the output of malloc(), e.g.

if (!buf) ret = EFI_OUT_OF_RESOURCES; else

• ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
  

• }
• if (ret != EFI_SUCCESS) {

• free(buf);
  

• *size = 0;
  

• return NULL;
  

• }
• return buf;

+}

+/**

• • efi_dp_instance_by_idx() - Get a file path with a specific index
• • @name: device path array
• • @size: size of the discovered device path
• • @idx: index of the device path
• • Return: device path or NULL. Caller must free the returned value
• */

+struct +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,

• 			efi_uintn_t *size, int idx)
  

+{

idx should be of an unsigned type as we cannot have negative indices.

• struct efi_device_path *instance = NULL;
• efi_uintn_t instance_size = 0;
• if (!efi_dp_is_multi_instance(dp))

Given a device path with one instance, why don't you allow me to read index 0? I assume this check can be removed.

• return NULL;
  

• while (idx >= 0 && dp) {

• instance = efi_dp_get_next_instance(&dp, &instance_size);
  

• if (idx && instance) {
  

• 	efi_free_pool(instance);
  

• 	instance_size = 0;
  

• 	instance = NULL;
  

• }
  

• idx--;
  

• }
• *size = instance_size;
• return instance;

This can be simplified with unsigned idx:

for (; dp; --idx) { instance = efi_dp_get_next_instance(&dp, size); if (!idx) { return instance; efi_free_pool(instance); } return NULL;

+}

+/**

• • create_boot_var_indexed() - Return Boot#### name were #### is replaced by

• • 	       the value of BootCurrent
    

• • @var_name: variable name
• • @var_name_size: size of var_name
• • Return: Status code
• */

+static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) +{

• efi_uintn_t boot_order_size;

You are reading BootCurrent, not BootOrder.

• efi_status_t ret;
• u16 boot_order;

Same here.

• u16 *pos;
• boot_order_size = sizeof(boot_order);
• ret = efi_get_variable_int(L"BootCurrent",

• 		   &efi_global_variable_guid, NULL,
  

• 		   &boot_order_size, &boot_order, NULL);
  

• if (ret != EFI_SUCCESS)

• goto out;
  

• pos = efi_create_indexed_name(var_name, var_name_size, "Boot",

• 		      boot_order);
  

• if (!pos) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto out;
  

• }

+out:

• return ret;

+}

+/**

• • efi_get_fp_from_var() - Retrieve and return a device path from an EFI

• • 	   Boot### variable
    

• • @idx: index of the instance to retrieve
• • Return: device path of NULL. Caller must free the returned value

%s/of/or/

• */

+struct efi_device_path *efi_get_fp_from_boot(int idx) +{

• struct efi_device_path *file_path;
• struct efi_load_option lo;
• void *var_value = NULL;
• efi_uintn_t size;
• efi_status_t ret;
• u16 var_name[16];
• ret = create_boot_var_indexed(var_name, sizeof(var_name));
• if (ret != EFI_SUCCESS)

• return NULL;
  

• var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
• if (!var_value)

• return NULL;
  

• efi_deserialize_load_option(&lo, var_value, &size);
• file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
• if (!file_path) {

• printf("Instance not found\n");
  

This message is not enough for a user to take action. I suggest

("Missing file path with index %d in %ls", idx, var_name)

We want to use logging. I assume this is not an error. Can we use log_debug() here?

• return NULL;
  

• }
• return file_path;

+}

Some other functions would fit into the same C module. Candidates are:

* efi_create_indexed_name() * efi_deserialize_load_option() * efi_serialize_load_option()

But that can be done in a separate patch.

Best regards

Heinrich

On Wed, Jan 13, 2021 at 01:11:47PM +0200, 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),

In this respect,

let's add some helper functions which will retrieve and parse file paths stored in EFI load options. Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

include/efi_helper.h | 23 ++++++ lib/efi_loader/efi_helper.c | 146 ++++++++++++++++++++++++++++++++++++ 2 files changed, 169 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..4e6bd2f036df --- /dev/null +++ b/include/efi_helper.h @@ -0,0 +1,23 @@ +/* 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>

+enum load_option_dp_type {

• BOOT_IMAGE_DP,
• INITRD_DP,
• DTB_DP,

+};

+void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size); +struct efi_device_path *efi_get_fp_from_boot(int idx); +struct efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,

• 			       efi_uintn_t *size, int idx);
  

+#endif diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c new file mode 100644 index 000000000000..e2437a4f698b --- /dev/null +++ b/lib/efi_loader/efi_helper.c @@ -0,0 +1,146 @@ +// 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>

+/**

• • efi_get_var() - read value of an EFI variable
• • @name: variable name
• • @start: vendor GUID
• • @size: size of allocated buffer
• • Return: buffer with variable data or NULL
• */

+void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size) +{

• efi_status_t ret;
• void *buf = NULL;
• *size = 0;
• ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
• if (ret == EFI_BUFFER_TOO_SMALL) {

• buf = malloc(*size);
  

• ret = efi_get_variable_int(name, vendor, NULL, size, buf, NULL);
  

• }
• if (ret != EFI_SUCCESS) {

• free(buf);
  

• *size = 0;
  

• return NULL;
  

• }
• return buf;

+}

+/**

• • efi_dp_instance_by_idx() - Get a file path with a specific index
• • @name: device path array
• • @size: size of the discovered device path
• • @idx: index of the device path
• • Return: device path or NULL. Caller must free the returned value
• */

+struct +efi_device_path *efi_dp_instance_by_idx(struct efi_device_path *dp,

• 			efi_uintn_t *size, int idx)
  

The type of "idx" should be 'enum load_option_dp_type'.

Currently, "idx" is used as an index into the array of device paths, but given each device path is set to have its own guid, "idx" should be unlinked from the 'order' within the array.

Even if you don't want so, this function should at least take care of some special cases like <execution image> + (no initrd) + <dtb> Alternatively, "END device path" can be put at the second place.

I expect that handling those corner cases should be described explicitly.

+{

• struct efi_device_path *instance = NULL;
• efi_uintn_t instance_size = 0;
• if (!efi_dp_is_multi_instance(dp))

• return NULL;
  

• while (idx >= 0 && dp) {

• instance = efi_dp_get_next_instance(&dp, &instance_size);
  

• if (idx && instance) {
  

• 	efi_free_pool(instance);
  

• 	instance_size = 0;
  

• 	instance = NULL;
  

• }
  

• idx--;
  

• }
• *size = instance_size;
• return instance;

+}

+/**

• • create_boot_var_indexed() - Return Boot#### name were #### is replaced by

• • 	       the value of BootCurrent
    

• • @var_name: variable name
• • @var_name_size: size of var_name
• • Return: Status code
• */

+static efi_status_t create_boot_var_indexed(u16 var_name[], size_t var_name_size) +{

• efi_uintn_t boot_order_size;
• efi_status_t ret;
• u16 boot_order;
• u16 *pos;
• boot_order_size = sizeof(boot_order);
• ret = efi_get_variable_int(L"BootCurrent",

• 		   &efi_global_variable_guid, NULL,
  

• 		   &boot_order_size, &boot_order, NULL);
  

• if (ret != EFI_SUCCESS)

• goto out;
  

• pos = efi_create_indexed_name(var_name, var_name_size, "Boot",

• 		      boot_order);
  

• if (!pos) {

• ret = EFI_OUT_OF_RESOURCES;
  

• goto out;
  

• }

+out:

• return ret;

+}

+/**

• • efi_get_fp_from_var() - Retrieve and return a device path from an EFI

• • 	   Boot### variable
    

• • @idx: index of the instance to retrieve
• • Return: device path of NULL. Caller must free the returned value
• */

+struct efi_device_path *efi_get_fp_from_boot(int idx)

Are you sure that this function is called only when "BootCurrent" is appropriately set?

+{

• struct efi_device_path *file_path;
• struct efi_load_option lo;
• void *var_value = NULL;
• efi_uintn_t size;
• efi_status_t ret;
• u16 var_name[16];

The size of var_name: 16 -> 8

-Takahiro Akashi

• ret = create_boot_var_indexed(var_name, sizeof(var_name));
• if (ret != EFI_SUCCESS)

• return NULL;
  

• var_value = efi_get_var(var_name, &efi_global_variable_guid, &size);
• if (!var_value)

• return NULL;
  

• efi_deserialize_load_option(&lo, var_value, &size);
• file_path = efi_dp_instance_by_idx(lo.file_path, &size, idx);
• if (!file_path) {

• printf("Instance not found\n");
  

• return NULL;
  

• }
• return file_path;

+}

2.30.0.rc2

Akashi-san,

On Thu, Jan 14, 2021 at 01:23:30PM +0900, AKASHI Takahiro wrote:

Ilias,

On Wed, Jan 13, 2021 at 01:11:48PM +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 defining a UEFI BootXXXX we can use the filepathlist and store a file path pointing to our initrd. Specifically the EFI spec describes:

"The first element of the array is a device path that describes the device and location of the Image for this load option. Other device paths may optionally exist in the FilePathList, but their usage is OSV specific"

I wonder what "OSV specific" does and should mean.

Judging from the context is used, I assumed it meant OS vendor.

Apparently, using a "array of device paths" is U-Boot specific for now and any distro who wants to use U-Boot as an EFI boot loader needs to (at least, preferably) take care of this. It would be sad that the installation process cannot be EFI-implementation agnostic in terms of EFI's purpose.

That's the reason I changed the approach completely. This one adheres to the EFI spec compared to the previous version that used EFI variables.

So do you intend to propose your idea as a common practice to linux community?

Yes, although that depends on the definition of the linux community. The kernel itself is irrelevant here. We need to make sure though that applications like efibootmgr follow the same principle.

...

When the EFI application is launched through the bootmgr, we'll try to interpret the extra device path. If that points to a file that exists on our disk, we'll now install the load_file2 and the efi-stub will be able to use it.

This opens up another path using U-Boot and defines a new boot flow. A user will be able to control the kernel/initrd pairs without explicit cmdline args or GRUB.

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

cmd/bootefi.c | 3 + include/efi_loader.h | 1 + lib/efi_loader/Kconfig | 13 +-- lib/efi_loader/efi_bootmgr.c | 3 + lib/efi_loader/efi_load_initrd.c | 154 ++++++++++++++++--------------- 5 files changed, 91 insertions(+), 83 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c index fdf909f8da2c..053927d5d986 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -357,6 +357,9 @@ static efi_status_t do_bootefi_exec(efi_handle_t handle, void *load_options)

free(load_options);

• if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))

• efi_initrd_deregister();
  

I understand why you want do "deregister" the initrd handle, but the handle for the loaded image is still valid at this point. So it looks inconsistent from the viewpoint of API's.

Hmm why? Won't this code be called after efi_exit()? In that case the loaded image handle is deleted as well.

...

return ret; }

[...]

...

@@ -222,6 +222,9 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, ret = efi_set_variable_int(L"BootCurrent", &efi_global_variable_guid, attributes, sizeof(n), &n, false);

• /* try to register load file2 for initrd's */
  

• if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD))
  

• 	ret |= efi_initrd_register();
  

I think that, even if the config(EFI_LOAD_FILE@_INITRD) is turned on, we should be allowed to boot linux without initrd if we want. In addition, we may want to boot non-linux images by using U-boot with the config enabled.

That was the case with the previous patchset. The efi_initrd_register() was always returning EFI_SUCCESS. Heinrich asked me respect the bootmgr behavior, since a distro without initrd will most likely fail to boot, so he wanted to allow fallbacks on the bootmgr.

The reasoning is that the kernel, in EFI mode, has 3 ways of discovering an iniitrd. 1. Fixups on initrd-start/end in the DTB. In that case the stub is unaware of how the DTB gets loaded 2. initrd=XXXX in the command line. In that case the kernel uses EFI_SIMPLE_FILESYSYSTEM_PROTOCOL to load the parsed file. 3. The load option this patch implements via LOAD_FILE2

If the user specifies an initrd, he needs it to boot and it's unlikely to specify both the initrd=xxxxx and add it in the Boot#### variable.

I think I can fix the use case and return EFI_SUCCESS from efi_initrd_register() if the extra instance is not found in the device path array. In that case a user will be able to boot without it, but if the device path is specified, the file *must* be there. If it's not the bootmgr will fallback to the next available boot option.

...

if (ret != EFI_SUCCESS) {

• status = EFI_NOT_FOUND;
  

[...]

...

goto out;

The logic here (efi_get_fp_from_boot, then efi_file_from_path) is also seen in check_initrd(). Pls refactor the code.

check_initrd() closes the file handle though and frees the device path. It's supposed to serve as a wrapper for 'is the file present', while in the second function we need the device path and the file handle to load the file.

Thanks for taking the time to review this.

Regards /Ilias

On 13.01.21 12:11, Ilias Apalodimas wrote:

The UEFI spec allow a packed array of UEFI device paths in the FilePathList[] of an EFI_LOAD_OPTION. The first file path must describe the laoded image but the rest are OS specific. Previous patches parse the device path and try to use the second member of the array as an initrd. So let's modify efidebug slightly and install the second file described in the command line as the initrd device path.

Please, describe the syntax of the efidebug command in the commit message.

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

cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 81 insertions(+), 8 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 5fb7b1e3c6a9..8d62981aca92 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -8,6 +8,7 @@ #include <charset.h> #include <common.h> #include <command.h> +#include <efi_helper.h> #include <efi_loader.h> #include <efi_rng.h> #include <exports.h> @@ -17,6 +18,7 @@ #include <mapmem.h> #include <search.h> #include <linux/ctype.h> +#include <linux/err.h>

#define BS systab.boottime #define RT systab.runtime @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; }

+/**

• • add_initrd_instance() - Append a device path to load_options pointing to an

• • 	   inirtd
    

• • @argc: Number of arguments
• • @argv: Argument array
• • @file_path Existing device path, the new instance will be appended
• • Return: Pointer to the device path or ERR_PTR
• */

+static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],

• 				   struct efi_device_path *file_path)
  

+{

• struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
• struct efi_device_path *final_fp = NULL;
• efi_status_t ret;
• if (argc < 8)

• return ERR_PTR(-EINVAL);
  

• ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,

• 	       &tmp_fp);
  

• if (ret != EFI_SUCCESS) {

• printf("Cannot create device path for \"%s %s\"\n",
  

•        argv[6], argv[7]);
  

• goto out;
  

• }
• final_fp = efi_dp_append_instance(file_path, tmp_fp);

+out:

• efi_free_pool(tmp_dp);
• efi_free_pool(tmp_fp);
• return final_fp ? final_fp : ERR_PTR(-EINVAL);

+}

/**

• do_efi_boot_add() - set UEFI load option

@@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,

• Implement efidebug "boot add" sub-command. Create or change UEFI load option.

• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
    

• • Without initrd:
• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
• • With initrd:
• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
  */

static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, size_t label_len, label_len16; u16 *label; struct efi_device_path *device_path = NULL, *file_path = NULL;

• struct efi_device_path *final_fp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size; efi_status_t ret; int r = CMD_RET_SUCCESS;

• if (argc < 6 || argc > 7)

• if (argc < 6 || argc > 9) return CMD_RET_USAGE;

  id = (int)simple_strtoul(argv[1], &endp, 16);

@@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, /* attributes */ lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */

• /* optional data */
• if (argc == 6)

• lo.optional_data = NULL;
  

• else

• lo.optional_data = (const u8 *)argv[6];
  

• /* label */ label_len = strlen(argv[2]); label_len16 = utf8_utf16_strnlen(argv[2], label_len);

@@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, r = CMD_RET_FAILURE; goto out; }

• /* add initrd instance in device path */
• if (argc >= 9) {

• final_fp = add_initrd_instance(argc, argv, file_path);
  

• if (IS_ERR(final_fp)) {
  

• 	r = CMD_RET_FAILURE;
  

• 	goto out;
  

• }
  

• /* add_initrd_instance allocates a new device path */
  

• efi_free_pool(file_path);
  

• file_path = final_fp;
  

• /* update optional data */
  

• if (argc == 9)
  

• 	lo.optional_data = NULL;
  

• else
  

• 	lo.optional_data = (const u8 *)argv[9];
  

• }
• lo.file_path = file_path; lo.file_path_length = efi_dp_size(file_path) + sizeof(struct efi_device_path); /* for END */

• /* optional data */

• if (argc == 6)

• lo.optional_data = NULL;
  

• else

• lo.optional_data = (const u8 *)argv[6];
  
  

  size = efi_serialize_load_option(&lo, (u8 **)&data); if (!size) {

@@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) {

• struct efi_device_path *initrd_path = NULL; struct efi_load_option lo; char *label, *p; size_t label_len16, label_len; u16 *dp_str; efi_status_t ret;

• efi_uintn_t initrd_dp_size;

  ret = efi_deserialize_load_option(&lo, data, size); if (ret != EFI_SUCCESS) {

@@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) dp_str = efi_dp_str(lo.file_path); printf(" file_path: %ls\n", dp_str); efi_free_pool(dp_str);

• initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);

• if (initrd_path) {

• dp_str = efi_dp_str(initrd_path);
  

• printf("  initrd_path: %ls\n", dp_str);
  

• efi_free_pool(dp_str);
  

• efi_free_pool(initrd_path);
  

• }

  printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,

@@ -1583,7 +1656,7 @@ static char efidebug_help_text[] =

Where is the change to efidebug_help_text[]?

Best regards

Heinrich

#endif

U_BOOT_CMD(

• efidebug, 10, 0, do_efidebug,

• efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, "Configure UEFI environment", efidebug_help_text

);

Ilias,

On Wed, Jan 13, 2021 at 01:11:49PM +0200, Ilias Apalodimas wrote:

The UEFI spec allow a packed array of UEFI device paths in the FilePathList[] of an EFI_LOAD_OPTION. The first file path must describe the laoded image but the rest are OS specific. Previous patches parse the device path and try to use the second member of the array as an initrd. So let's modify efidebug slightly and install the second file described in the command line as the initrd device path.

I have a concern about your proposed command line syntax. It takes a lot of parameters as a whole which makes it hard to understand it at a glance, easily overflowing the width of terminal window.

It will even get worse if we want to take dtb as a third device path, and what if we want to specify dtb, but not initrd?

Moreover, if we want to add support for non-linux executabes which utilize extra device paths (neither initrd nor dtb) in a boot load option, the syntax will be problematic.

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

cmd/efidebug.c | 89 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 81 insertions(+), 8 deletions(-)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 5fb7b1e3c6a9..8d62981aca92 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -8,6 +8,7 @@ #include <charset.h> #include <common.h> #include <command.h> +#include <efi_helper.h> #include <efi_loader.h> #include <efi_rng.h> #include <exports.h> @@ -17,6 +18,7 @@ #include <mapmem.h> #include <search.h> #include <linux/ctype.h> +#include <linux/err.h>

#define BS systab.boottime #define RT systab.runtime @@ -782,6 +784,42 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag, return CMD_RET_SUCCESS; }

+/**

• • add_initrd_instance() - Append a device path to load_options pointing to an

• • 	   inirtd
    

• • @argc: Number of arguments
• • @argv: Argument array
• • @file_path Existing device path, the new instance will be appended
• • Return: Pointer to the device path or ERR_PTR
• */

+static struct efi_device_path *add_initrd_instance(int argc, char *const argv[],

• 				   struct efi_device_path *file_path)
  

+{

• struct efi_device_path *tmp_dp = NULL, *tmp_fp = NULL;
• struct efi_device_path *final_fp = NULL;
• efi_status_t ret;
• if (argc < 8)

• return ERR_PTR(-EINVAL);
  

• ret = efi_dp_from_name(argv[6], argv[7], argv[8], &tmp_dp,

• 	       &tmp_fp);
  

• if (ret != EFI_SUCCESS) {

• printf("Cannot create device path for \"%s %s\"\n",
  

•        argv[6], argv[7]);
  

• goto out;
  

• }
• final_fp = efi_dp_append_instance(file_path, tmp_fp);

+out:

• efi_free_pool(tmp_dp);
• efi_free_pool(tmp_fp);
• return final_fp ? final_fp : ERR_PTR(-EINVAL);

+}

/**

• do_efi_boot_add() - set UEFI load option

@@ -794,7 +832,11 @@ static int do_efi_show_tables(struct cmd_tbl *cmdtp, int flag,

• Implement efidebug "boot add" sub-command. Create or change UEFI load option.

• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
    

• • Without initrd:
• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <options>
• • With initrd:
• • efidebug boot add <id> <label> <interface> <devnum>[:<part>] <file> <interface2> <devnum2>[:<part>] <initrd> <options>
  */

static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) @@ -807,13 +849,14 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, size_t label_len, label_len16; u16 *label; struct efi_device_path *device_path = NULL, *file_path = NULL;

• struct efi_device_path *final_fp = NULL; struct efi_load_option lo; void *data = NULL; efi_uintn_t size; efi_status_t ret; int r = CMD_RET_SUCCESS;

• if (argc < 6 || argc > 7)

• if (argc < 6 || argc > 9) return CMD_RET_USAGE;

  id = (int)simple_strtoul(argv[1], &endp, 16);

@@ -829,6 +872,12 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, /* attributes */ lo.attributes = LOAD_OPTION_ACTIVE; /* always ACTIVE */

• /* optional data */
• if (argc == 6)

• lo.optional_data = NULL;
  

• else

• lo.optional_data = (const u8 *)argv[6];
  

• /* label */ label_len = strlen(argv[2]); label_len16 = utf8_utf16_strnlen(argv[2], label_len);

@@ -847,15 +896,30 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, r = CMD_RET_FAILURE; goto out; }

• /* add initrd instance in device path */
• if (argc >= 9) {

• final_fp = add_initrd_instance(argc, argv, file_path);
  

We'd better pass argv[6], argv[7] and argv[8] explicitly here, which will make the code readable and easily extended to support dtb in the future. then, argc -= 3; argv += 3;

• if (IS_ERR(final_fp)) {
  

• 	r = CMD_RET_FAILURE;
  

• 	goto out;
  

• }
  

• /* add_initrd_instance allocates a new device path */
  

• efi_free_pool(file_path);
  

• file_path = final_fp;
  

• /* update optional data */
  

Then, the code should be moved out of this 'if' clause.

• if (argc == 9)
  

• 	lo.optional_data = NULL;
  

• else
  

• 	lo.optional_data = (const u8 *)argv[9];
  

• }
• lo.file_path = file_path; lo.file_path_length = efi_dp_size(file_path) + sizeof(struct efi_device_path); /* for END */

• /* optional data */
• if (argc == 6)

• lo.optional_data = NULL;
  

• else

• lo.optional_data = (const u8 *)argv[6];
  

The code should remain here for non-initrd case.

-Takahiro Akashi

size = efi_serialize_load_option(&lo, (u8 **)&data); if (!size) { @@ -939,11 +1003,13 @@ static int do_efi_boot_rm(struct cmd_tbl *cmdtp, int flag, */ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) {

• struct efi_device_path *initrd_path = NULL; struct efi_load_option lo; char *label, *p; size_t label_len16, label_len; u16 *dp_str; efi_status_t ret;

• efi_uintn_t initrd_dp_size;

  ret = efi_deserialize_load_option(&lo, data, size); if (ret != EFI_SUCCESS) {

@@ -973,6 +1039,13 @@ static void show_efi_boot_opt_data(u16 *varname16, void *data, size_t *size) dp_str = efi_dp_str(lo.file_path); printf(" file_path: %ls\n", dp_str); efi_free_pool(dp_str);

• initrd_path = efi_dp_instance_by_idx(lo.file_path, &initrd_dp_size, INITRD_DP);

• if (initrd_path) {

• dp_str = efi_dp_str(initrd_path);
  

• printf("  initrd_path: %ls\n", dp_str);
  

• efi_free_pool(dp_str);
  

• efi_free_pool(initrd_path);
  

• }

  printf(" data:\n"); print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,

@@ -1583,7 +1656,7 @@ static char efidebug_help_text[] = #endif

U_BOOT_CMD(

• efidebug, 10, 0, do_efidebug,

• efidebug, CONFIG_SYS_MAXARGS, 0, do_efidebug, "Configure UEFI environment", efidebug_help_text

);

2.30.0.rc2