[U-Boot] [PATCH 0/5] efi_loader: run a specific efi application more easily

[U-Boot] [PATCH 0/5] efi_loader: run a specific efi application more easily

[AKASHI Takahiro]

This patch is a result from re-organizing my previous patches;
a combination of [1] and part of [2] so as solely to provide several ways
of executing a specific efi application explicitly.
  * bootmanager via BootNext variable
  * bootefi with boot id
  * run -e

[1] https://lists.denx.de/pipermail/u-boot/2018-November/349281.html
[2] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html

AKASHI Takahiro (5):
  efi_loader: bootmgr: support BootNext and BootCurrent variable
    behavior
  efi_loader: bootmgr: allow for running a given load option
  cmd: bootefi: carve out fdt parameter handling
  cmd: bootefi: run an EFI application of a specific load option
  cmd: run: add "-e" option to run an EFI application

 cmd/bootefi.c                | 71 +++++++++++++++++++++++++-----------
 cmd/bootefi.h                |  3 ++
 cmd/nvedit.c                 |  5 +++
 common/cli.c                 | 31 ++++++++++++++++
 include/command.h            |  3 ++
 include/efi_loader.h         |  3 +-
 lib/efi_loader/efi_bootmgr.c | 46 ++++++++++++++++++++++-
 7 files changed, 138 insertions(+), 24 deletions(-)
 create mode 100644 cmd/bootefi.h

-- 
2.19.1

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[AKASHI Takahiro]

See UEFI v2.7, section 3.1.2 for details of the specification.

With my efishell command[1], you can try as the following:
  => efi boot add 1 SHELL ...
  => efi boot add 2 HELLO ...
  => efi boot order 1 2
  => efi bootmgr
     (starting SHELL ...)
  => efi setvar BootNext =H0200
  => efi bootmgr
     (starting HELLO ...)
  => efi dumpvar
  <snip ...>
  BootCurrent: {boot,run}(blob)
  00000000:  02 00                    ..
  BootOrder: {boot,run}(blob)
  00000000:  01 00 02 00              ....

Using "run -e" would be more human-friendly, though.

[1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a095df3f540b..a54ae28343ce 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 	efi_deserialize_load_option(&lo, load_option);
 
 	if (lo.attributes & LOAD_OPTION_ACTIVE) {
+		u32 attributes;
 		efi_status_t ret;
 
 		debug("%s: trying to load \"%ls\" from %pD\n",
 		      __func__, lo.label, lo.file_path);
 
+		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+			     EFI_VARIABLE_RUNTIME_ACCESS;
+		size = sizeof(n);
+		ret = rs->set_variable(L"BootCurrent",
+				       (efi_guid_t *)&efi_global_variable_guid,
+				       attributes, size, &n);
+		if (ret != EFI_SUCCESS)
+			goto error;
+
 		ret = efi_load_image_from_path(lo.file_path, &image);
 
 		if (ret != EFI_SUCCESS)
@@ -173,16 +183,41 @@ error:
 void *efi_bootmgr_load(struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
-	uint16_t *bootorder;
+	u16 bootnext, *bootorder;
+	u32 attributes;
 	efi_uintn_t size;
 	void *image = NULL;
 	int i, num;
+	efi_status_t ret;
 
 	__efi_entry_check();
 
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	/* BootNext */
+	size = sizeof(bootnext);
+	ret = rs->get_variable(L"BootNext",
+			       (efi_guid_t *)&efi_global_variable_guid,
+			       NULL, &size, &bootnext);
+	if (!bootnext)
+		goto run_list;
+
+	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
+		     EFI_VARIABLE_RUNTIME_ACCESS;
+	size = 0;
+	ret = rs->set_variable(L"BootNext",
+			       (efi_guid_t *)&efi_global_variable_guid,
+			       attributes, size, &bootnext);
+	if (ret != EFI_SUCCESS)
+		goto error;
+
+	image = try_load_entry(bootnext, device_path, file_path);
+	if (image)
+		goto error;
+
+run_list:
+	/* BootOrder */
 	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder)
 		goto error;
-- 
2.19.1

[U-Boot] [PATCH 2/5] efi_loader: bootmgr: allow for running a given load option

[AKASHI Takahiro]

With an extra argument, efi_bootmgr_load() can now load an efi binary
based on a "BootXXXX" variable specified.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                | 2 +-
 include/efi_loader.h         | 3 ++-
 lib/efi_loader/efi_bootmgr.c | 9 ++++++++-
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7012d72ab50d..3ebae1cdad08 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -452,7 +452,7 @@ static int do_bootefi_bootmgr_exec(void)
 	void *addr;
 	efi_status_t r;
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
+	addr = efi_bootmgr_load(-1, &device_path, &file_path);
 	if (!addr)
 		return 1;
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index dd68cfce5c65..5a6321122c9c 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -551,7 +551,8 @@ struct efi_load_option {
 
 void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
 unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a54ae28343ce..db391147fb2d 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -180,7 +180,8 @@ error:
  * available load-options, finding and returning the first one that can
  * be loaded successfully.
  */
-void *efi_bootmgr_load(struct efi_device_path **device_path,
+void *efi_bootmgr_load(int boot_id,
+		       struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
 	u16 bootnext, *bootorder;
@@ -195,6 +196,12 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	/* specified boot option */
+	if (boot_id != -1) {
+		image = try_load_entry(boot_id, device_path, file_path);
+		goto error;
+	}
+
 	/* BootNext */
 	size = sizeof(bootnext);
 	ret = rs->get_variable(L"BootNext",
-- 
2.19.1

[U-Boot] [PATCH 3/5] cmd: bootefi: carve out fdt parameter handling

[AKASHI Takahiro]

The current way how command parameters, particularly "fdt addr," are
handled makes it a bit complicated to add a subcommand-specific parameter.
So just refactor the code and extract efi_handle_fdt().

This commit is a preparatory change for enhancing bootmgr sub-command.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Revert "fixup! fixup! cmd: bootefi: carve out fdt parameter handling"

This reverts commit abc315426e37bdfb96a48d23c1fc96399b68baa5.
---
 cmd/bootefi.c | 49 +++++++++++++++++++++++++++++++++----------------
 cmd/bootefi.h |  3 +++
 2 files changed, 36 insertions(+), 16 deletions(-)
 create mode 100644 cmd/bootefi.h

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3ebae1cdad08..796ca6ee69ec 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -279,6 +279,31 @@ static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
 	efi_delete_handle(&image_obj->header);
 }
 
+int efi_handle_fdt(char *fdt_opt)
+{
+	unsigned long fdt_addr;
+	efi_status_t r;
+
+	if (fdt_opt) {
+		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
+		if (!fdt_addr && *fdt_opt != '0')
+			return CMD_RET_USAGE;
+
+		/* Install device tree */
+		r = efi_install_fdt(fdt_addr);
+		if (r != EFI_SUCCESS) {
+			printf("ERROR: failed to install device tree\n");
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
+		efi_install_configuration_table(&efi_guid_fdt, NULL);
+		printf("WARNING: booting without device tree\n");
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -473,7 +498,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	unsigned long addr;
 	char *saddr;
 	efi_status_t r;
-	unsigned long fdt_addr;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -489,21 +513,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	if (argc > 2) {
-		fdt_addr = simple_strtoul(argv[2], NULL, 16);
-		if (!fdt_addr && *argv[2] != '0')
-			return CMD_RET_USAGE;
-		/* Install device tree */
-		r = efi_install_fdt(fdt_addr);
-		if (r != EFI_SUCCESS) {
-			printf("ERROR: failed to install device tree\n");
-			return CMD_RET_FAILURE;
-		}
-	} else {
-		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
-		efi_install_configuration_table(&efi_guid_fdt, NULL);
-		printf("WARNING: booting without device tree\n");
-	}
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
@@ -521,6 +530,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		struct efi_loaded_image_obj *image_obj;
 		struct efi_loaded_image *loaded_image_info;
 
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
+
 		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
 					 "\\selftest", (uintptr_t)&efi_selftest,
 					 "efi_selftest"))
@@ -533,6 +545,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
+
 		return do_bootefi_bootmgr_exec();
 	} else {
 		saddr = argv[1];
@@ -542,6 +557,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		if (!addr && *saddr != '0')
 			return CMD_RET_USAGE;
 
+		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
+			return CMD_RET_FAILURE;
 	}
 
 	printf("## Starting EFI application at %08lx ...\n", addr);
diff --git a/cmd/bootefi.h b/cmd/bootefi.h
new file mode 100644
index 000000000000..4e11ab1211cb
--- /dev/null
+++ b/cmd/bootefi.h
@@ -0,0 +1,3 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+int efi_handle_fdt(char *fdt_opt);
-- 
2.19.1

[U-Boot] [PATCH 4/5] cmd: bootefi: run an EFI application of a specific load option

[AKASHI Takahiro]

With this patch applied, we will be able to selectively execute
an EFI application by specifying a load option, say "1" for Boot0001,
"2" for Boot0002 and so on.

  => bootefi bootmgr <fdt addr> 1, or
     bootefi bootmgr - 1

Please note that BootXXXX need not be included in "BootOrder".

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 796ca6ee69ec..2fc52e3056d2 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -471,13 +471,13 @@ static efi_status_t bootefi_test_prepare
 
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
-static int do_bootefi_bootmgr_exec(void)
+static int do_bootefi_bootmgr_exec(int boot_id)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
 	efi_status_t r;
 
-	addr = efi_bootmgr_load(-1, &device_path, &file_path);
+	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
 	if (!addr)
 		return 1;
 
@@ -545,10 +545,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
-		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
-			return CMD_RET_FAILURE;
+		char *endp;
+		int boot_id = -1;
+
+		if (argc > 2)
+			if (efi_handle_fdt((argv[2][0] == '-') ?
+					   NULL : argv[2]))
+				return CMD_RET_FAILURE;
+
+		if (argc > 3) {
+			boot_id = (int)simple_strtoul(argv[3], &endp, 0);
+			if ((argv[3] + strlen(argv[3]) != endp) ||
+			    boot_id > 0xffff)
+				return CMD_RET_USAGE;
+		}
 
-		return do_bootefi_bootmgr_exec();
+		return do_bootefi_bootmgr_exec(boot_id);
 	} else {
 		saddr = argv[1];
 
@@ -589,7 +601,7 @@ static char bootefi_help_text[] =
 	"    Use environment variable efi_selftest to select a single test.\n"
 	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
 #endif
-	"bootefi bootmgr [fdt addr]\n"
+	"bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n"
 	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
 	"\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
@@ -597,7 +609,7 @@ static char bootefi_help_text[] =
 #endif
 
 U_BOOT_CMD(
-	bootefi, 3, 0, do_bootefi,
+	bootefi, 5, 0, do_bootefi,
 	"Boots an EFI payload from memory",
 	bootefi_help_text
 );
-- 
2.19.1

[U-Boot] [PATCH 5/5] cmd: run: add "-e" option to run an EFI application

[AKASHI Takahiro]

"run -e" allows for executing EFI application with a specific "BootXXXX"
variable. If no "BootXXXX" is specified or "BootOrder" is specified,
it tries to run an EFI application specified in the order of "bootOrder."

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c     |  2 +-
 cmd/nvedit.c      |  5 +++++
 common/cli.c      | 31 +++++++++++++++++++++++++++++++
 include/command.h |  3 +++
 4 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 2fc52e3056d2..8122793d11c5 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
 
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
-static int do_bootefi_bootmgr_exec(int boot_id)
+int do_bootefi_bootmgr_exec(int boot_id)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index de16c72c23f2..c0facabfc4fe 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1343,8 +1343,13 @@ U_BOOT_CMD(
 U_BOOT_CMD_COMPLETE(
 	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
 	"run commands in an environment variable",
+#if defined(CONFIG_CMD_BOOTEFI)
+	"var -e [BootXXXX]\n"
+	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
 	"var [...]\n"
 	"    - run the commands in the environment variable(s) 'var'",
+#endif
 	var_complete
 );
 #endif
diff --git a/common/cli.c b/common/cli.c
index 51b8d5f85cbb..013dd2e51936 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -12,8 +12,10 @@
 #include <cli.h>
 #include <cli_hush.h>
 #include <console.h>
+#include <efi_loader.h>
 #include <fdtdec.h>
 #include <malloc.h>
+#include "../cmd/bootefi.h"
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
+#ifdef CONFIG_CMD_BOOTEFI
+	if (!strcmp(argv[1], "-e")) {
+		int boot_id = -1;
+		char *endp;
+
+		if (argc == 3) {
+			if (!strcmp(argv[2], "BootOrder")) {
+				boot_id = -1;
+			} else if (!strncmp(argv[2], "Boot", 4)) {
+				boot_id = (int)simple_strtoul(&argv[2][4],
+							      &endp, 0);
+				if ((argv[2] + strlen(argv[2]) != endp) ||
+				    boot_id > 0xffff)
+					return CMD_RET_USAGE;
+			} else {
+				return CMD_RET_USAGE;
+			}
+		}
+
+		if (efi_init_obj_list())
+			return CMD_RET_FAILURE;
+
+		if (efi_handle_fdt(NULL))
+			return CMD_RET_FAILURE;
+
+		return do_bootefi_bootmgr_exec(boot_id);
+	}
+#endif
+
 	for (i = 1; i < argc; ++i) {
 		char *arg;
 
diff --git a/include/command.h b/include/command.h
index 200c7a5e9f4e..9b7b876585d9 100644
--- a/include/command.h
+++ b/include/command.h
@@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
 #if defined(CONFIG_CMD_RUN)
 extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 #endif
+#if defined(CONFIG_CMD_BOOTEFI)
+int do_bootefi_bootmgr_exec(int boot_id);
+#endif
 
 /* common/command.c */
 int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
-- 
2.19.1

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Alexander Graf]

On 18.12.18 06:02, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
> 
> With my efishell command[1], you can try as the following:
>   => efi boot add 1 SHELL ...
>   => efi boot add 2 HELLO ...
>   => efi boot order 1 2
>   => efi bootmgr
>      (starting SHELL ...)
>   => efi setvar BootNext =H0200
>   => efi bootmgr
>      (starting HELLO ...)
>   => efi dumpvar
>   <snip ...>
>   BootCurrent: {boot,run}(blob)
>   00000000:  02 00                    ..
>   BootOrder: {boot,run}(blob)
>   00000000:  01 00 02 00              ....
> 
> Using "run -e" would be more human-friendly, though.
> 
> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a095df3f540b..a54ae28343ce 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>  	efi_deserialize_load_option(&lo, load_option);
>  
>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> +		u32 attributes;
>  		efi_status_t ret;
>  
>  		debug("%s: trying to load \"%ls\" from %pD\n",
>  		      __func__, lo.label, lo.file_path);
>  
> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			     EFI_VARIABLE_RUNTIME_ACCESS;
> +		size = sizeof(n);
> +		ret = rs->set_variable(L"BootCurrent",
> +				       (efi_guid_t *)&efi_global_variable_guid,
> +				       attributes, size, &n);

Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.

> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +
>  		ret = efi_load_image_from_path(lo.file_path, &image);
>  
>  		if (ret != EFI_SUCCESS)
> @@ -173,16 +183,41 @@ error:
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path)
>  {
> -	uint16_t *bootorder;
> +	u16 bootnext, *bootorder;
> +	u32 attributes;
>  	efi_uintn_t size;
>  	void *image = NULL;
>  	int i, num;
> +	efi_status_t ret;
>  
>  	__efi_entry_check();
>  
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	/* BootNext */
> +	size = sizeof(bootnext);
> +	ret = rs->get_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       NULL, &size, &bootnext);

... same here.

> +	if (!bootnext)
> +		goto run_list;
> +
> +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +		     EFI_VARIABLE_RUNTIME_ACCESS;
> +	size = 0;
> +	ret = rs->set_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       attributes, size, &bootnext);

... same here.


Alex

> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	image = try_load_entry(bootnext, device_path, file_path);
> +	if (image)
> +		goto error;
> +
> +run_list:
> +	/* BootOrder */
>  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>  	if (!bootorder)
>  		goto error;
> 

[U-Boot] [PATCH 2/5] efi_loader: bootmgr: allow for running a given load option

[Alexander Graf]

On 18.12.18 06:02, AKASHI Takahiro wrote:
> With an extra argument, efi_bootmgr_load() can now load an efi binary
> based on a "BootXXXX" variable specified.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c                | 2 +-
>  include/efi_loader.h         | 3 ++-
>  lib/efi_loader/efi_bootmgr.c | 9 ++++++++-
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 7012d72ab50d..3ebae1cdad08 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -452,7 +452,7 @@ static int do_bootefi_bootmgr_exec(void)
>  	void *addr;
>  	efi_status_t r;
>  
> -	addr = efi_bootmgr_load(&device_path, &file_path);
> +	addr = efi_bootmgr_load(-1, &device_path, &file_path);

Please make the -1 a special #define that is more verbose to readers.
Something like EFI_BOOTMGR_DEFAULT_ORDER.


Alex

[U-Boot] [PATCH 3/5] cmd: bootefi: carve out fdt parameter handling

[Alexander Graf]

On 18.12.18 06:02, AKASHI Takahiro wrote:
> The current way how command parameters, particularly "fdt addr," are
> handled makes it a bit complicated to add a subcommand-specific parameter.
> So just refactor the code and extract efi_handle_fdt().
> 
> This commit is a preparatory change for enhancing bootmgr sub-command.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Revert "fixup! fixup! cmd: bootefi: carve out fdt parameter handling"
> 
> This reverts commit abc315426e37bdfb96a48d23c1fc96399b68baa5.

You sure you wanted this to be part of the commit message? ;)

> ---
>  cmd/bootefi.c | 49 +++++++++++++++++++++++++++++++++----------------
>  cmd/bootefi.h |  3 +++
>  2 files changed, 36 insertions(+), 16 deletions(-)
>  create mode 100644 cmd/bootefi.h
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3ebae1cdad08..796ca6ee69ec 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -279,6 +279,31 @@ static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
>  	efi_delete_handle(&image_obj->header);
>  }
>  
> +int efi_handle_fdt(char *fdt_opt)
> +{
> +	unsigned long fdt_addr;
> +	efi_status_t r;
> +
> +	if (fdt_opt) {
> +		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
> +		if (!fdt_addr && *fdt_opt != '0')
> +			return CMD_RET_USAGE;
> +
> +		/* Install device tree */
> +		r = efi_install_fdt(fdt_addr);
> +		if (r != EFI_SUCCESS) {
> +			printf("ERROR: failed to install device tree\n");
> +			return CMD_RET_FAILURE;
> +		}
> +	} else {
> +		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> +		efi_install_configuration_table(&efi_guid_fdt, NULL);
> +		printf("WARNING: booting without device tree\n");
> +	}
> +
> +	return CMD_RET_SUCCESS;
> +}
> +
>  /**
>   * do_bootefi_exec() - execute EFI binary
>   *
> @@ -473,7 +498,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	unsigned long addr;
>  	char *saddr;
>  	efi_status_t r;
> -	unsigned long fdt_addr;
>  
>  	/* Allow unaligned memory access */
>  	allow_unaligned();
> @@ -489,21 +513,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> -	if (argc > 2) {
> -		fdt_addr = simple_strtoul(argv[2], NULL, 16);
> -		if (!fdt_addr && *argv[2] != '0')
> -			return CMD_RET_USAGE;
> -		/* Install device tree */
> -		r = efi_install_fdt(fdt_addr);
> -		if (r != EFI_SUCCESS) {
> -			printf("ERROR: failed to install device tree\n");
> -			return CMD_RET_FAILURE;
> -		}
> -	} else {
> -		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> -		efi_install_configuration_table(&efi_guid_fdt, NULL);
> -		printf("WARNING: booting without device tree\n");
> -	}
>  #ifdef CONFIG_CMD_BOOTEFI_HELLO
>  	if (!strcmp(argv[1], "hello")) {
>  		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> @@ -521,6 +530,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		struct efi_loaded_image_obj *image_obj;
>  		struct efi_loaded_image *loaded_image_info;
>  
> +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> +			return CMD_RET_FAILURE;
> +
>  		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
>  					 "\\selftest", (uintptr_t)&efi_selftest,
>  					 "efi_selftest"))
> @@ -533,6 +545,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	} else
>  #endif
>  	if (!strcmp(argv[1], "bootmgr")) {
> +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> +			return CMD_RET_FAILURE;
> +
>  		return do_bootefi_bootmgr_exec();
>  	} else {
>  		saddr = argv[1];
> @@ -542,6 +557,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		if (!addr && *saddr != '0')
>  			return CMD_RET_USAGE;
>  
> +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> +			return CMD_RET_FAILURE;
>  	}
>  
>  	printf("## Starting EFI application at %08lx ...\n", addr);
> diff --git a/cmd/bootefi.h b/cmd/bootefi.h
> new file mode 100644
> index 000000000000..4e11ab1211cb
> --- /dev/null
> +++ b/cmd/bootefi.h
> @@ -0,0 +1,3 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */

This should be a separate patch.

> +
> +int efi_handle_fdt(char *fdt_opt);
> 

[U-Boot] [PATCH 4/5] cmd: bootefi: run an EFI application of a specific load option

[Alexander Graf]

On 18.12.18 06:02, AKASHI Takahiro wrote:
> With this patch applied, we will be able to selectively execute
> an EFI application by specifying a load option, say "1" for Boot0001,
> "2" for Boot0002 and so on.
> 
>   => bootefi bootmgr <fdt addr> 1, or
>      bootefi bootmgr - 1
> 
> Please note that BootXXXX need not be included in "BootOrder".
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 796ca6ee69ec..2fc52e3056d2 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -471,13 +471,13 @@ static efi_status_t bootefi_test_prepare
>  
>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>  
> -static int do_bootefi_bootmgr_exec(void)
> +static int do_bootefi_bootmgr_exec(int boot_id)
>  {
>  	struct efi_device_path *device_path, *file_path;
>  	void *addr;
>  	efi_status_t r;
>  
> -	addr = efi_bootmgr_load(-1, &device_path, &file_path);
> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>  	if (!addr)
>  		return 1;
>  
> @@ -545,10 +545,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	} else
>  #endif
>  	if (!strcmp(argv[1], "bootmgr")) {
> -		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> -			return CMD_RET_FAILURE;
> +		char *endp;
> +		int boot_id = -1;
> +
> +		if (argc > 2)
> +			if (efi_handle_fdt((argv[2][0] == '-') ?
> +					   NULL : argv[2]))
> +				return CMD_RET_FAILURE;

This is slowly getting quite unreadable. How about you make the code
less dense, but easier to grasp?


if (argc > 2) {
    const char *fdtstr = argv[2];

    /* Special address "-" means no device tree */
    if (fdtstr[0] == '-')
        fdtstr = NULL;

    r = efi_handle_fdt(fdtstr);

    if (r)
        return r;
}

Alex

> +
> +		if (argc > 3) {
> +			boot_id = (int)simple_strtoul(argv[3], &endp, 0);
> +			if ((argv[3] + strlen(argv[3]) != endp) ||
> +			    boot_id > 0xffff)
> +				return CMD_RET_USAGE;
> +		}
>  
> -		return do_bootefi_bootmgr_exec();
> +		return do_bootefi_bootmgr_exec(boot_id);
>  	} else {
>  		saddr = argv[1];
>  
> @@ -589,7 +601,7 @@ static char bootefi_help_text[] =
>  	"    Use environment variable efi_selftest to select a single test.\n"
>  	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
>  #endif
> -	"bootefi bootmgr [fdt addr]\n"
> +	"bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n"
>  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>  	"\n"
>  	"    If specified, the device tree located at <fdt address> gets\n"
> @@ -597,7 +609,7 @@ static char bootefi_help_text[] =
>  #endif
>  
>  U_BOOT_CMD(
> -	bootefi, 3, 0, do_bootefi,
> +	bootefi, 5, 0, do_bootefi,
>  	"Boots an EFI payload from memory",
>  	bootefi_help_text
>  );
> 

[U-Boot] [PATCH 5/5] cmd: run: add "-e" option to run an EFI application

[Alexander Graf]

On 18.12.18 06:02, AKASHI Takahiro wrote:
> "run -e" allows for executing EFI application with a specific "BootXXXX"
> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> it tries to run an EFI application specified in the order of "bootOrder."
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c     |  2 +-
>  cmd/nvedit.c      |  5 +++++
>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
>  include/command.h |  3 +++
>  4 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 2fc52e3056d2..8122793d11c5 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
>  
>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>  
> -static int do_bootefi_bootmgr_exec(int boot_id)
> +int do_bootefi_bootmgr_exec(int boot_id)
>  {
>  	struct efi_device_path *device_path, *file_path;
>  	void *addr;
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index de16c72c23f2..c0facabfc4fe 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
>  U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
> +#if defined(CONFIG_CMD_BOOTEFI)
> +	"var -e [BootXXXX]\n"
> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> +#else
>  	"var [...]\n"
>  	"    - run the commands in the environment variable(s) 'var'",
> +#endif
>  	var_complete
>  );
>  #endif
> diff --git a/common/cli.c b/common/cli.c
> index 51b8d5f85cbb..013dd2e51936 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -12,8 +12,10 @@
>  #include <cli.h>
>  #include <cli_hush.h>
>  #include <console.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <malloc.h>
> +#include "../cmd/bootefi.h"
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>  
> +#ifdef CONFIG_CMD_BOOTEFI
> +	if (!strcmp(argv[1], "-e")) {
> +		int boot_id = -1;
> +		char *endp;
> +
> +		if (argc == 3) {
> +			if (!strcmp(argv[2], "BootOrder")) {
> +				boot_id = -1;
> +			} else if (!strncmp(argv[2], "Boot", 4)) {
> +				boot_id = (int)simple_strtoul(&argv[2][4],
> +							      &endp, 0);
> +				if ((argv[2] + strlen(argv[2]) != endp) ||
> +				    boot_id > 0xffff)
> +					return CMD_RET_USAGE;

This duplicates the same logic you added to bootefi.c. Better reuse it.
I guess you can just call a function inside bootefi.c from here if you
detect -e:

  if (!strcmp(argv[1], "-e"))
    return do_bootefi_run(cmdtp, flag, argc, argv);

and just handle it all inside bootefi.c at that point.

> +			} else {
> +				return CMD_RET_USAGE;
> +			}
> +		}
> +
> +		if (efi_init_obj_list())
> +			return CMD_RET_FAILURE;
> +
> +		if (efi_handle_fdt(NULL))
> +			return CMD_RET_FAILURE;
> +
> +		return do_bootefi_bootmgr_exec(boot_id);
> +	}
> +#endif
> +
>  	for (i = 1; i < argc; ++i) {
>  		char *arg;
>  
> diff --git a/include/command.h b/include/command.h
> index 200c7a5e9f4e..9b7b876585d9 100644
> --- a/include/command.h
> +++ b/include/command.h
> @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
>  #if defined(CONFIG_CMD_RUN)
>  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>  #endif
> +#if defined(CONFIG_CMD_BOOTEFI)
> +int do_bootefi_bootmgr_exec(int boot_id);
> +#endif

I would prefer if nvedit.c includes efi_loader.h and we define it there?


Alex

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[AKASHI Takahiro]

On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> > 
> > With my efishell command[1], you can try as the following:
> >   => efi boot add 1 SHELL ...
> >   => efi boot add 2 HELLO ...
> >   => efi boot order 1 2
> >   => efi bootmgr
> >      (starting SHELL ...)
> >   => efi setvar BootNext =H0200
> >   => efi bootmgr
> >      (starting HELLO ...)
> >   => efi dumpvar
> >   <snip ...>
> >   BootCurrent: {boot,run}(blob)
> >   00000000:  02 00                    ..
> >   BootOrder: {boot,run}(blob)
> >   00000000:  01 00 02 00              ....
> > 
> > Using "run -e" would be more human-friendly, though.
> > 
> > [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a095df3f540b..a54ae28343ce 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >  	efi_deserialize_load_option(&lo, load_option);
> >  
> >  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > +		u32 attributes;
> >  		efi_status_t ret;
> >  
> >  		debug("%s: trying to load \"%ls\" from %pD\n",
> >  		      __func__, lo.label, lo.file_path);
> >  
> > +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			     EFI_VARIABLE_RUNTIME_ACCESS;
> > +		size = sizeof(n);
> > +		ret = rs->set_variable(L"BootCurrent",
> > +				       (efi_guid_t *)&efi_global_variable_guid,
> > +				       attributes, size, &n);
> 
> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.

OK, but let me make sure one thing:
My efishell calls efi_* functions directly in some places as
Not all the features can be implemented only with boot/runtime services.
In those cases, we don't have to use EFI_CALL(), right?

Thanks,
-Takahiro Akashi

> > +		if (ret != EFI_SUCCESS)
> > +			goto error;
> > +
> >  		ret = efi_load_image_from_path(lo.file_path, &image);
> >  
> >  		if (ret != EFI_SUCCESS)
> > @@ -173,16 +183,41 @@ error:
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path)
> >  {
> > -	uint16_t *bootorder;
> > +	u16 bootnext, *bootorder;
> > +	u32 attributes;
> >  	efi_uintn_t size;
> >  	void *image = NULL;
> >  	int i, num;
> > +	efi_status_t ret;
> >  
> >  	__efi_entry_check();
> >  
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	/* BootNext */
> > +	size = sizeof(bootnext);
> > +	ret = rs->get_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       NULL, &size, &bootnext);
> 
> ... same here.
> 
> > +	if (!bootnext)
> > +		goto run_list;
> > +
> > +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		     EFI_VARIABLE_RUNTIME_ACCESS;
> > +	size = 0;
> > +	ret = rs->set_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       attributes, size, &bootnext);
> 
> ... same here.
> 
> 
> Alex
> 
> > +	if (ret != EFI_SUCCESS)
> > +		goto error;
> > +
> > +	image = try_load_entry(bootnext, device_path, file_path);
> > +	if (image)
> > +		goto error;
> > +
> > +run_list:
> > +	/* BootOrder */
> >  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >  	if (!bootorder)
> >  		goto error;
> > 

[U-Boot] [PATCH 2/5] efi_loader: bootmgr: allow for running a given load option

[AKASHI Takahiro]

On Sun, Dec 23, 2018 at 03:05:42AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > With an extra argument, efi_bootmgr_load() can now load an efi binary
> > based on a "BootXXXX" variable specified.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c                | 2 +-
> >  include/efi_loader.h         | 3 ++-
> >  lib/efi_loader/efi_bootmgr.c | 9 ++++++++-
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 7012d72ab50d..3ebae1cdad08 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -452,7 +452,7 @@ static int do_bootefi_bootmgr_exec(void)
> >  	void *addr;
> >  	efi_status_t r;
> >  
> > -	addr = efi_bootmgr_load(&device_path, &file_path);
> > +	addr = efi_bootmgr_load(-1, &device_path, &file_path);
> 
> Please make the -1 a special #define that is more verbose to readers.
> Something like EFI_BOOTMGR_DEFAULT_ORDER.

OK
Add it to efi_loader.h

-Takahiro Akashi

> 
> Alex

[U-Boot] [PATCH 3/5] cmd: bootefi: carve out fdt parameter handling

[AKASHI Takahiro]

On Sun, Dec 23, 2018 at 03:08:52AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > The current way how command parameters, particularly "fdt addr," are
> > handled makes it a bit complicated to add a subcommand-specific parameter.
> > So just refactor the code and extract efi_handle_fdt().
> > 
> > This commit is a preparatory change for enhancing bootmgr sub-command.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > 
> > Revert "fixup! fixup! cmd: bootefi: carve out fdt parameter handling"
> > 
> > This reverts commit abc315426e37bdfb96a48d23c1fc96399b68baa5.
> 
> You sure you wanted this to be part of the commit message? ;)

Damn!

-Takahiro Akashi


> > ---
> >  cmd/bootefi.c | 49 +++++++++++++++++++++++++++++++++----------------
> >  cmd/bootefi.h |  3 +++
> >  2 files changed, 36 insertions(+), 16 deletions(-)
> >  create mode 100644 cmd/bootefi.h
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3ebae1cdad08..796ca6ee69ec 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -279,6 +279,31 @@ static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> >  	efi_delete_handle(&image_obj->header);
> >  }
> >  
> > +int efi_handle_fdt(char *fdt_opt)
> > +{
> > +	unsigned long fdt_addr;
> > +	efi_status_t r;
> > +
> > +	if (fdt_opt) {
> > +		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
> > +		if (!fdt_addr && *fdt_opt != '0')
> > +			return CMD_RET_USAGE;
> > +
> > +		/* Install device tree */
> > +		r = efi_install_fdt(fdt_addr);
> > +		if (r != EFI_SUCCESS) {
> > +			printf("ERROR: failed to install device tree\n");
> > +			return CMD_RET_FAILURE;
> > +		}
> > +	} else {
> > +		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> > +		efi_install_configuration_table(&efi_guid_fdt, NULL);
> > +		printf("WARNING: booting without device tree\n");
> > +	}
> > +
> > +	return CMD_RET_SUCCESS;
> > +}
> > +
> >  /**
> >   * do_bootefi_exec() - execute EFI binary
> >   *
> > @@ -473,7 +498,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	unsigned long addr;
> >  	char *saddr;
> >  	efi_status_t r;
> > -	unsigned long fdt_addr;
> >  
> >  	/* Allow unaligned memory access */
> >  	allow_unaligned();
> > @@ -489,21 +513,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >  
> > -	if (argc > 2) {
> > -		fdt_addr = simple_strtoul(argv[2], NULL, 16);
> > -		if (!fdt_addr && *argv[2] != '0')
> > -			return CMD_RET_USAGE;
> > -		/* Install device tree */
> > -		r = efi_install_fdt(fdt_addr);
> > -		if (r != EFI_SUCCESS) {
> > -			printf("ERROR: failed to install device tree\n");
> > -			return CMD_RET_FAILURE;
> > -		}
> > -	} else {
> > -		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> > -		efi_install_configuration_table(&efi_guid_fdt, NULL);
> > -		printf("WARNING: booting without device tree\n");
> > -	}
> >  #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >  	if (!strcmp(argv[1], "hello")) {
> >  		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> > @@ -521,6 +530,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  		struct efi_loaded_image_obj *image_obj;
> >  		struct efi_loaded_image *loaded_image_info;
> >  
> > +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> > +			return CMD_RET_FAILURE;
> > +
> >  		if (bootefi_test_prepare(&image_obj, &loaded_image_info,
> >  					 "\\selftest", (uintptr_t)&efi_selftest,
> >  					 "efi_selftest"))
> > @@ -533,6 +545,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	} else
> >  #endif
> >  	if (!strcmp(argv[1], "bootmgr")) {
> > +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> > +			return CMD_RET_FAILURE;
> > +
> >  		return do_bootefi_bootmgr_exec();
> >  	} else {
> >  		saddr = argv[1];
> > @@ -542,6 +557,8 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  		if (!addr && *saddr != '0')
> >  			return CMD_RET_USAGE;
> >  
> > +		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> > +			return CMD_RET_FAILURE;
> >  	}
> >  
> >  	printf("## Starting EFI application at %08lx ...\n", addr);
> > diff --git a/cmd/bootefi.h b/cmd/bootefi.h
> > new file mode 100644
> > index 000000000000..4e11ab1211cb
> > --- /dev/null
> > +++ b/cmd/bootefi.h
> > @@ -0,0 +1,3 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> 
> This should be a separate patch.
> 
> > +
> > +int efi_handle_fdt(char *fdt_opt);
> > 

[U-Boot] [PATCH 4/5] cmd: bootefi: run an EFI application of a specific load option

[AKASHI Takahiro]

On Sun, Dec 23, 2018 at 03:15:16AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > With this patch applied, we will be able to selectively execute
> > an EFI application by specifying a load option, say "1" for Boot0001,
> > "2" for Boot0002 and so on.
> > 
> >   => bootefi bootmgr <fdt addr> 1, or
> >      bootefi bootmgr - 1
> > 
> > Please note that BootXXXX need not be included in "BootOrder".
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 796ca6ee69ec..2fc52e3056d2 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -471,13 +471,13 @@ static efi_status_t bootefi_test_prepare
> >  
> >  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >  
> > -static int do_bootefi_bootmgr_exec(void)
> > +static int do_bootefi_bootmgr_exec(int boot_id)
> >  {
> >  	struct efi_device_path *device_path, *file_path;
> >  	void *addr;
> >  	efi_status_t r;
> >  
> > -	addr = efi_bootmgr_load(-1, &device_path, &file_path);
> > +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
> >  	if (!addr)
> >  		return 1;
> >  
> > @@ -545,10 +545,22 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	} else
> >  #endif
> >  	if (!strcmp(argv[1], "bootmgr")) {
> > -		if (efi_handle_fdt(argc > 2 ? argv[2] : NULL))
> > -			return CMD_RET_FAILURE;
> > +		char *endp;
> > +		int boot_id = -1;
> > +
> > +		if (argc > 2)
> > +			if (efi_handle_fdt((argv[2][0] == '-') ?
> > +					   NULL : argv[2]))
> > +				return CMD_RET_FAILURE;
> 
> This is slowly getting quite unreadable. How about you make the code
> less dense, but easier to grasp?
> 
> 
> if (argc > 2) {
>     const char *fdtstr = argv[2];
> 
>     /* Special address "-" means no device tree */
>     if (fdtstr[0] == '-')
>         fdtstr = NULL;
> 
>     r = efi_handle_fdt(fdtstr);
> 
>     if (r)
>         return r;
> }

OK

-Takahiro Akashi


> Alex
> 
> > +
> > +		if (argc > 3) {
> > +			boot_id = (int)simple_strtoul(argv[3], &endp, 0);
> > +			if ((argv[3] + strlen(argv[3]) != endp) ||
> > +			    boot_id > 0xffff)
> > +				return CMD_RET_USAGE;
> > +		}
> >  
> > -		return do_bootefi_bootmgr_exec();
> > +		return do_bootefi_bootmgr_exec(boot_id);
> >  	} else {
> >  		saddr = argv[1];
> >  
> > @@ -589,7 +601,7 @@ static char bootefi_help_text[] =
> >  	"    Use environment variable efi_selftest to select a single test.\n"
> >  	"    Use 'setenv efi_selftest list' to enumerate all tests.\n"
> >  #endif
> > -	"bootefi bootmgr [fdt addr]\n"
> > +	"bootefi bootmgr [<fdt addr>|'-' [<boot id>]]\n"
> >  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> >  	"\n"
> >  	"    If specified, the device tree located at <fdt address> gets\n"
> > @@ -597,7 +609,7 @@ static char bootefi_help_text[] =
> >  #endif
> >  
> >  U_BOOT_CMD(
> > -	bootefi, 3, 0, do_bootefi,
> > +	bootefi, 5, 0, do_bootefi,
> >  	"Boots an EFI payload from memory",
> >  	bootefi_help_text
> >  );
> > 

[U-Boot] [PATCH 5/5] cmd: run: add "-e" option to run an EFI application

[AKASHI Takahiro]

On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
> 
> 
> On 18.12.18 06:02, AKASHI Takahiro wrote:
> > "run -e" allows for executing EFI application with a specific "BootXXXX"
> > variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> > it tries to run an EFI application specified in the order of "bootOrder."
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c     |  2 +-
> >  cmd/nvedit.c      |  5 +++++
> >  common/cli.c      | 31 +++++++++++++++++++++++++++++++
> >  include/command.h |  3 +++
> >  4 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 2fc52e3056d2..8122793d11c5 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
> >  
> >  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >  
> > -static int do_bootefi_bootmgr_exec(int boot_id)
> > +int do_bootefi_bootmgr_exec(int boot_id)
> >  {
> >  	struct efi_device_path *device_path, *file_path;
> >  	void *addr;
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index de16c72c23f2..c0facabfc4fe 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
> >  U_BOOT_CMD_COMPLETE(
> >  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >  	"run commands in an environment variable",
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +	"var -e [BootXXXX]\n"
> > +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > +#else
> >  	"var [...]\n"
> >  	"    - run the commands in the environment variable(s) 'var'",
> > +#endif
> >  	var_complete
> >  );
> >  #endif
> > diff --git a/common/cli.c b/common/cli.c
> > index 51b8d5f85cbb..013dd2e51936 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -12,8 +12,10 @@
> >  #include <cli.h>
> >  #include <cli_hush.h>
> >  #include <console.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <malloc.h>
> > +#include "../cmd/bootefi.h"
> >  
> >  DECLARE_GLOBAL_DATA_PTR;
> >  
> > @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >  
> > +#ifdef CONFIG_CMD_BOOTEFI
> > +	if (!strcmp(argv[1], "-e")) {
> > +		int boot_id = -1;
> > +		char *endp;
> > +
> > +		if (argc == 3) {
> > +			if (!strcmp(argv[2], "BootOrder")) {
> > +				boot_id = -1;
> > +			} else if (!strncmp(argv[2], "Boot", 4)) {
> > +				boot_id = (int)simple_strtoul(&argv[2][4],
> > +							      &endp, 0);
> > +				if ((argv[2] + strlen(argv[2]) != endp) ||
> > +				    boot_id > 0xffff)
> > +					return CMD_RET_USAGE;
> 
> This duplicates the same logic you added to bootefi.c. Better reuse it.
> I guess you can just call a function inside bootefi.c from here if you
> detect -e:
> 
>   if (!strcmp(argv[1], "-e"))
>     return do_bootefi_run(cmdtp, flag, argc, argv);
> 
> and just handle it all inside bootefi.c at that point.

Hmm, OK.
In this case, the command syntax of bootmgr will be changed so as to
accept "BootXXXX" instead of just an integer (as boot id).

Thanks,
-Takahiro Akashi

> > +			} else {
> > +				return CMD_RET_USAGE;
> > +			}
> > +		}
> > +
> > +		if (efi_init_obj_list())
> > +			return CMD_RET_FAILURE;
> > +
> > +		if (efi_handle_fdt(NULL))
> > +			return CMD_RET_FAILURE;
> > +
> > +		return do_bootefi_bootmgr_exec(boot_id);
> > +	}
> > +#endif
> > +
> >  	for (i = 1; i < argc; ++i) {
> >  		char *arg;
> >  
> > diff --git a/include/command.h b/include/command.h
> > index 200c7a5e9f4e..9b7b876585d9 100644
> > --- a/include/command.h
> > +++ b/include/command.h
> > @@ -48,6 +48,9 @@ typedef struct cmd_tbl_s	cmd_tbl_t;
> >  #if defined(CONFIG_CMD_RUN)
> >  extern int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >  #endif
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +int do_bootefi_bootmgr_exec(int boot_id);
> > +#endif
> 
> I would prefer if nvedit.c includes efi_loader.h and we define it there?
> 
> 
> Alex

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Alexander Graf]

On 25.12.18 10:36, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>
>>
>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>
>>> With my efishell command[1], you can try as the following:
>>>   => efi boot add 1 SHELL ...
>>>   => efi boot add 2 HELLO ...
>>>   => efi boot order 1 2
>>>   => efi bootmgr
>>>      (starting SHELL ...)
>>>   => efi setvar BootNext =H0200
>>>   => efi bootmgr
>>>      (starting HELLO ...)
>>>   => efi dumpvar
>>>   <snip ...>
>>>   BootCurrent: {boot,run}(blob)
>>>   00000000:  02 00                    ..
>>>   BootOrder: {boot,run}(blob)
>>>   00000000:  01 00 02 00              ....
>>>
>>> Using "run -e" would be more human-friendly, though.
>>>
>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index a095df3f540b..a54ae28343ce 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>  	efi_deserialize_load_option(&lo, load_option);
>>>  
>>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>> +		u32 attributes;
>>>  		efi_status_t ret;
>>>  
>>>  		debug("%s: trying to load \"%ls\" from %pD\n",
>>>  		      __func__, lo.label, lo.file_path);
>>>  
>>> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> +			     EFI_VARIABLE_RUNTIME_ACCESS;
>>> +		size = sizeof(n);
>>> +		ret = rs->set_variable(L"BootCurrent",
>>> +				       (efi_guid_t *)&efi_global_variable_guid,
>>> +				       attributes, size, &n);
>>
>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
> 
> OK, but let me make sure one thing:
> My efishell calls efi_* functions directly in some places as
> Not all the features can be implemented only with boot/runtime services.
> In those cases, we don't have to use EFI_CALL(), right?

If your "efishell" is a UEFI binary, you can directly call boot/runtime
services. If it runs as part of the U-Boot code base, every call to
boot/runtime service callbacks *must* go through EFI_CALL().


Alex

[U-Boot] [PATCH 5/5] cmd: run: add "-e" option to run an EFI application

[Alexander Graf]

On 25.12.18 12:29, AKASHI Takahiro wrote:
> On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
>>
>>
>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>> "run -e" allows for executing EFI application with a specific "BootXXXX"
>>> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
>>> it tries to run an EFI application specified in the order of "bootOrder."
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/bootefi.c     |  2 +-
>>>  cmd/nvedit.c      |  5 +++++
>>>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
>>>  include/command.h |  3 +++
>>>  4 files changed, 40 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 2fc52e3056d2..8122793d11c5 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
>>>  
>>>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>>>  
>>> -static int do_bootefi_bootmgr_exec(int boot_id)
>>> +int do_bootefi_bootmgr_exec(int boot_id)
>>>  {
>>>  	struct efi_device_path *device_path, *file_path;
>>>  	void *addr;
>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>> index de16c72c23f2..c0facabfc4fe 100644
>>> --- a/cmd/nvedit.c
>>> +++ b/cmd/nvedit.c
>>> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
>>>  U_BOOT_CMD_COMPLETE(
>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>  	"run commands in an environment variable",
>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>> +	"var -e [BootXXXX]\n"
>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>> +#else
>>>  	"var [...]\n"
>>>  	"    - run the commands in the environment variable(s) 'var'",
>>> +#endif
>>>  	var_complete
>>>  );
>>>  #endif
>>> diff --git a/common/cli.c b/common/cli.c
>>> index 51b8d5f85cbb..013dd2e51936 100644
>>> --- a/common/cli.c
>>> +++ b/common/cli.c
>>> @@ -12,8 +12,10 @@
>>>  #include <cli.h>
>>>  #include <cli_hush.h>
>>>  #include <console.h>
>>> +#include <efi_loader.h>
>>>  #include <fdtdec.h>
>>>  #include <malloc.h>
>>> +#include "../cmd/bootefi.h"
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  	if (argc < 2)
>>>  		return CMD_RET_USAGE;
>>>  
>>> +#ifdef CONFIG_CMD_BOOTEFI
>>> +	if (!strcmp(argv[1], "-e")) {
>>> +		int boot_id = -1;
>>> +		char *endp;
>>> +
>>> +		if (argc == 3) {
>>> +			if (!strcmp(argv[2], "BootOrder")) {
>>> +				boot_id = -1;
>>> +			} else if (!strncmp(argv[2], "Boot", 4)) {
>>> +				boot_id = (int)simple_strtoul(&argv[2][4],
>>> +							      &endp, 0);
>>> +				if ((argv[2] + strlen(argv[2]) != endp) ||
>>> +				    boot_id > 0xffff)
>>> +					return CMD_RET_USAGE;
>>
>> This duplicates the same logic you added to bootefi.c. Better reuse it.
>> I guess you can just call a function inside bootefi.c from here if you
>> detect -e:
>>
>>   if (!strcmp(argv[1], "-e"))
>>     return do_bootefi_run(cmdtp, flag, argc, argv);
>>
>> and just handle it all inside bootefi.c at that point.
> 
> Hmm, OK.
> In this case, the command syntax of bootmgr will be changed so as to
> accept "BootXXXX" instead of just an integer (as boot id).

Not necessarily. You can just move all the code you have here into a new
function in bootefi.c. The parsing logic really should just be shared.


Alex

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Heinrich Schuchardt]

On 12/26/18 10:23 PM, Alexander Graf wrote:
> 
> 
> On 25.12.18 10:36, AKASHI Takahiro wrote:
>> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>>
>>>
>>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>>
>>>> With my efishell command[1], you can try as the following:
>>>>   => efi boot add 1 SHELL ...
>>>>   => efi boot add 2 HELLO ...
>>>>   => efi boot order 1 2
>>>>   => efi bootmgr
>>>>      (starting SHELL ...)
>>>>   => efi setvar BootNext =H0200
>>>>   => efi bootmgr
>>>>      (starting HELLO ...)
>>>>   => efi dumpvar
>>>>   <snip ...>
>>>>   BootCurrent: {boot,run}(blob)
>>>>   00000000:  02 00                    ..
>>>>   BootOrder: {boot,run}(blob)
>>>>   00000000:  01 00 02 00              ....
>>>>
>>>> Using "run -e" would be more human-friendly, though.
>>>>
>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>> index a095df3f540b..a54ae28343ce 100644
>>>> --- a/lib/efi_loader/efi_bootmgr.c
>>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>>> @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>>  	efi_deserialize_load_option(&lo, load_option);
>>>>  
>>>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>>> +		u32 attributes;
>>>>  		efi_status_t ret;
>>>>  
>>>>  		debug("%s: trying to load \"%ls\" from %pD\n",
>>>>  		      __func__, lo.label, lo.file_path);
>>>>  
>>>> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>> +			     EFI_VARIABLE_RUNTIME_ACCESS;
>>>> +		size = sizeof(n);
>>>> +		ret = rs->set_variable(L"BootCurrent",
>>>> +				       (efi_guid_t *)&efi_global_variable_guid,
>>>> +				       attributes, size, &n);
>>>
>>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
>>
>> OK, but let me make sure one thing:
>> My efishell calls efi_* functions directly in some places as
>> Not all the features can be implemented only with boot/runtime services.
>> In those cases, we don't have to use EFI_CALL(), right?
> 
> If your "efishell" is a UEFI binary, you can directly call boot/runtime
> services. If it runs as part of the U-Boot code base, every call to
> boot/runtime service callbacks *must* go through EFI_CALL().

No, that is not how the call counting has been set up. You will get an
assert error if debugging is enabled. We use EFI_CALL when we want to
call an API function from inside an API function.

Just have a look at all the selftest code. It never uses EFI_CALL.

In this respect the bootmgr code is broken.

Best regards

Heinrich

> 
> 
> Alex
> 

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Alexander Graf]

On 26.12.18 22:33, Heinrich Schuchardt wrote:
> On 12/26/18 10:23 PM, Alexander Graf wrote:
>>
>>
>> On 25.12.18 10:36, AKASHI Takahiro wrote:
>>> On Sun, Dec 23, 2018 at 03:03:51AM +0100, Alexander Graf wrote:
>>>>
>>>>
>>>> On 18.12.18 06:02, AKASHI Takahiro wrote:
>>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>>>
>>>>> With my efishell command[1], you can try as the following:
>>>>>   => efi boot add 1 SHELL ...
>>>>>   => efi boot add 2 HELLO ...
>>>>>   => efi boot order 1 2
>>>>>   => efi bootmgr
>>>>>      (starting SHELL ...)
>>>>>   => efi setvar BootNext =H0200
>>>>>   => efi bootmgr
>>>>>      (starting HELLO ...)
>>>>>   => efi dumpvar
>>>>>   <snip ...>
>>>>>   BootCurrent: {boot,run}(blob)
>>>>>   00000000:  02 00                    ..
>>>>>   BootOrder: {boot,run}(blob)
>>>>>   00000000:  01 00 02 00              ....
>>>>>
>>>>> Using "run -e" would be more human-friendly, though.
>>>>>
>>>>> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 36 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>> index a095df3f540b..a54ae28343ce 100644
>>>>> --- a/lib/efi_loader/efi_bootmgr.c
>>>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>>>> @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>>>>>  	efi_deserialize_load_option(&lo, load_option);
>>>>>  
>>>>>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
>>>>> +		u32 attributes;
>>>>>  		efi_status_t ret;
>>>>>  
>>>>>  		debug("%s: trying to load \"%ls\" from %pD\n",
>>>>>  		      __func__, lo.label, lo.file_path);
>>>>>  
>>>>> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>> +			     EFI_VARIABLE_RUNTIME_ACCESS;
>>>>> +		size = sizeof(n);
>>>>> +		ret = rs->set_variable(L"BootCurrent",
>>>>> +				       (efi_guid_t *)&efi_global_variable_guid,
>>>>> +				       attributes, size, &n);
>>>>
>>>> Every call into UEFI land (rs->foo(), bs->foo(), etc) has to go via the
>>>> EFI_CALL() macro. Otherwise we may destroy the "gd" pointer on ARM.
>>>
>>> OK, but let me make sure one thing:
>>> My efishell calls efi_* functions directly in some places as
>>> Not all the features can be implemented only with boot/runtime services.
>>> In those cases, we don't have to use EFI_CALL(), right?
>>
>> If your "efishell" is a UEFI binary, you can directly call boot/runtime
>> services. If it runs as part of the U-Boot code base, every call to
>> boot/runtime service callbacks *must* go through EFI_CALL().
> 
> No, that is not how the call counting has been set up. You will get an
> assert error if debugging is enabled. We use EFI_CALL when we want to
> call an API function from inside an API function.

Ah, true. I stand corrected.

> Just have a look at all the selftest code. It never uses EFI_CALL.
> 
> In this respect the bootmgr code is broken.

Yes. Maybe this calls for a few travis checks with debugging enabled?


Alex

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Heinrich Schuchardt]

On 12/18/18 6:02 AM, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
> 
> With my efishell command[1], you can try as the following:
>   => efi boot add 1 SHELL ...
>   => efi boot add 2 HELLO ...
>   => efi boot order 1 2
>   => efi bootmgr
>      (starting SHELL ...)
>   => efi setvar BootNext =H0200

If you already have an "efi boot" sub-command wouldn't the following
syntax make more sense?

    efi boot next 2



Best regards

Heinrich

>   => efi bootmgr
>      (starting HELLO ...)
>   => efi dumpvar
>   <snip ...>
>   BootCurrent: {boot,run}(blob)
>   00000000:  02 00                    ..
>   BootOrder: {boot,run}(blob)
>   00000000:  01 00 02 00              ....
> 
> Using "run -e" would be more human-friendly, though.
> 
> [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a095df3f540b..a54ae28343ce 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>  	efi_deserialize_load_option(&lo, load_option);
>  
>  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> +		u32 attributes;
>  		efi_status_t ret;
>  
>  		debug("%s: trying to load \"%ls\" from %pD\n",
>  		      __func__, lo.label, lo.file_path);
>  
> +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +			     EFI_VARIABLE_RUNTIME_ACCESS;
> +		size = sizeof(n);
> +		ret = rs->set_variable(L"BootCurrent",
> +				       (efi_guid_t *)&efi_global_variable_guid,
> +				       attributes, size, &n);
> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +
>  		ret = efi_load_image_from_path(lo.file_path, &image);
>  
>  		if (ret != EFI_SUCCESS)
> @@ -173,16 +183,41 @@ error:
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path)
>  {
> -	uint16_t *bootorder;
> +	u16 bootnext, *bootorder;
> +	u32 attributes;
>  	efi_uintn_t size;
>  	void *image = NULL;
>  	int i, num;
> +	efi_status_t ret;
>  
>  	__efi_entry_check();
>  
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	/* BootNext */
> +	size = sizeof(bootnext);
> +	ret = rs->get_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       NULL, &size, &bootnext);
> +	if (!bootnext)
> +		goto run_list;
> +
> +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +		     EFI_VARIABLE_RUNTIME_ACCESS;

According to the UEFI spec you do not need any attributes when deleting
a variable.

Please, add a comment describing what you are doing here:

/* Delete BootNext variable */

Best regards

Heinrich

> +	size = 0;
> +	ret = rs->set_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       attributes, size, &bootnext);
> +	if (ret != EFI_SUCCESS)
> +		goto error;
> +
> +	image = try_load_entry(bootnext, device_path, file_path);
> +	if (image)
> +		goto error;
> +
> +run_list:
> +	/* BootOrder */
>  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>  	if (!bootorder)
>  		goto error;
> 

[U-Boot] [PATCH 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[AKASHI Takahiro]

On Thu, Dec 27, 2018 at 05:58:49AM +0100, Heinrich Schuchardt wrote:
> On 12/18/18 6:02 AM, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> > 
> > With my efishell command[1], you can try as the following:
> >   => efi boot add 1 SHELL ...
> >   => efi boot add 2 HELLO ...
> >   => efi boot order 1 2
> >   => efi bootmgr
> >      (starting SHELL ...)
> >   => efi setvar BootNext =H0200
> 
> If you already have an "efi boot" sub-command wouldn't the following
> syntax make more sense?
> 
>     efi boot next 2

OK.

> 
> 
> Best regards
> 
> Heinrich
> 
> >   => efi bootmgr
> >      (starting HELLO ...)
> >   => efi dumpvar
> >   <snip ...>
> >   BootCurrent: {boot,run}(blob)
> >   00000000:  02 00                    ..
> >   BootOrder: {boot,run}(blob)
> >   00000000:  01 00 02 00              ....
> > 
> > Using "run -e" would be more human-friendly, though.
> > 
> > [1] https://lists.denx.de/pipermail/u-boot/2018-November/346450.html
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 37 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 36 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a095df3f540b..a54ae28343ce 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -145,11 +145,21 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >  	efi_deserialize_load_option(&lo, load_option);
> >  
> >  	if (lo.attributes & LOAD_OPTION_ACTIVE) {
> > +		u32 attributes;
> >  		efi_status_t ret;
> >  
> >  		debug("%s: trying to load \"%ls\" from %pD\n",
> >  		      __func__, lo.label, lo.file_path);
> >  
> > +		attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +			     EFI_VARIABLE_RUNTIME_ACCESS;
> > +		size = sizeof(n);
> > +		ret = rs->set_variable(L"BootCurrent",
> > +				       (efi_guid_t *)&efi_global_variable_guid,
> > +				       attributes, size, &n);
> > +		if (ret != EFI_SUCCESS)
> > +			goto error;
> > +
> >  		ret = efi_load_image_from_path(lo.file_path, &image);
> >  
> >  		if (ret != EFI_SUCCESS)
> > @@ -173,16 +183,41 @@ error:
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path)
> >  {
> > -	uint16_t *bootorder;
> > +	u16 bootnext, *bootorder;
> > +	u32 attributes;
> >  	efi_uintn_t size;
> >  	void *image = NULL;
> >  	int i, num;
> > +	efi_status_t ret;
> >  
> >  	__efi_entry_check();
> >  
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	/* BootNext */
> > +	size = sizeof(bootnext);
> > +	ret = rs->get_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       NULL, &size, &bootnext);
> > +	if (!bootnext)
> > +		goto run_list;
> > +
> > +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		     EFI_VARIABLE_RUNTIME_ACCESS;
> 
> According to the UEFI spec you do not need any attributes when deleting
> a variable.

OK.

> Please, add a comment describing what you are doing here:
> 
> /* Delete BootNext variable */

OK.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > +	size = 0;
> > +	ret = rs->set_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       attributes, size, &bootnext);
> > +	if (ret != EFI_SUCCESS)
> > +		goto error;
> > +
> > +	image = try_load_entry(bootnext, device_path, file_path);
> > +	if (image)
> > +		goto error;
> > +
> > +run_list:
> > +	/* BootOrder */
> >  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >  	if (!bootorder)
> >  		goto error;
> > 
> 

[U-Boot] [PATCH 5/5] cmd: run: add "-e" option to run an EFI application

[AKASHI Takahiro]

On Wed, Dec 26, 2018 at 10:24:45PM +0100, Alexander Graf wrote:
> 
> 
> On 25.12.18 12:29, AKASHI Takahiro wrote:
> > On Sun, Dec 23, 2018 at 03:19:17AM +0100, Alexander Graf wrote:
> >>
> >>
> >> On 18.12.18 06:02, AKASHI Takahiro wrote:
> >>> "run -e" allows for executing EFI application with a specific "BootXXXX"
> >>> variable. If no "BootXXXX" is specified or "BootOrder" is specified,
> >>> it tries to run an EFI application specified in the order of "bootOrder."
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/bootefi.c     |  2 +-
> >>>  cmd/nvedit.c      |  5 +++++
> >>>  common/cli.c      | 31 +++++++++++++++++++++++++++++++
> >>>  include/command.h |  3 +++
> >>>  4 files changed, 40 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 2fc52e3056d2..8122793d11c5 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -471,7 +471,7 @@ static efi_status_t bootefi_test_prepare
> >>>  
> >>>  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >>>  
> >>> -static int do_bootefi_bootmgr_exec(int boot_id)
> >>> +int do_bootefi_bootmgr_exec(int boot_id)
> >>>  {
> >>>  	struct efi_device_path *device_path, *file_path;
> >>>  	void *addr;
> >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>> index de16c72c23f2..c0facabfc4fe 100644
> >>> --- a/cmd/nvedit.c
> >>> +++ b/cmd/nvedit.c
> >>> @@ -1343,8 +1343,13 @@ U_BOOT_CMD(
> >>>  U_BOOT_CMD_COMPLETE(
> >>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>  	"run commands in an environment variable",
> >>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>> +	"var -e [BootXXXX]\n"
> >>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>> +#else
> >>>  	"var [...]\n"
> >>>  	"    - run the commands in the environment variable(s) 'var'",
> >>> +#endif
> >>>  	var_complete
> >>>  );
> >>>  #endif
> >>> diff --git a/common/cli.c b/common/cli.c
> >>> index 51b8d5f85cbb..013dd2e51936 100644
> >>> --- a/common/cli.c
> >>> +++ b/common/cli.c
> >>> @@ -12,8 +12,10 @@
> >>>  #include <cli.h>
> >>>  #include <cli_hush.h>
> >>>  #include <console.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <malloc.h>
> >>> +#include "../cmd/bootefi.h"
> >>>  
> >>>  DECLARE_GLOBAL_DATA_PTR;
> >>>  
> >>> @@ -125,6 +127,35 @@ int do_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  	if (argc < 2)
> >>>  		return CMD_RET_USAGE;
> >>>  
> >>> +#ifdef CONFIG_CMD_BOOTEFI
> >>> +	if (!strcmp(argv[1], "-e")) {
> >>> +		int boot_id = -1;
> >>> +		char *endp;
> >>> +
> >>> +		if (argc == 3) {
> >>> +			if (!strcmp(argv[2], "BootOrder")) {
> >>> +				boot_id = -1;
> >>> +			} else if (!strncmp(argv[2], "Boot", 4)) {
> >>> +				boot_id = (int)simple_strtoul(&argv[2][4],
> >>> +							      &endp, 0);
> >>> +				if ((argv[2] + strlen(argv[2]) != endp) ||
> >>> +				    boot_id > 0xffff)
> >>> +					return CMD_RET_USAGE;
> >>
> >> This duplicates the same logic you added to bootefi.c. Better reuse it.
> >> I guess you can just call a function inside bootefi.c from here if you
> >> detect -e:
> >>
> >>   if (!strcmp(argv[1], "-e"))
> >>     return do_bootefi_run(cmdtp, flag, argc, argv);
> >>
> >> and just handle it all inside bootefi.c at that point.
> > 
> > Hmm, OK.
> > In this case, the command syntax of bootmgr will be changed so as to
> > accept "BootXXXX" instead of just an integer (as boot id).
> 
> Not necessarily. You can just move all the code you have here into a new
> function in bootefi.c. The parsing logic really should just be shared.

Ah ha, OK.

-Takahiro Akashi

> 
> Alex