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

[U-Boot] [PATCH v2 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 to solely provide several ways
of executing a specific efi application explicitly.
  * bootmanager via BootNext variable
  * bootefi with boot id
  * run -e with BootXXXX variable

Changes in v2 (Jan 15, 2019)
* not specify any attributes when deleting BootNext variable in
  efi_bootmgr_load()
* introduce EFI_BOOTMGR_DEFAULT_ORDER macro on behalf of BootOrder
  for use in efi_bootmgr_load()
* simplify the code around efi_handle_fdt() in do_bootefi()
* add do_bootefi_run() and call it at "run -e" so that we don't have to
  export internal efi helper functions

[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                | 114 +++++++++++++++++++++++++++--------
 cmd/nvedit.c                 |   9 ++-
 common/cli.c                 |  10 +++
 include/command.h            |   3 +
 include/efi_loader.h         |   5 +-
 lib/efi_loader/efi_bootmgr.c |  43 ++++++++++++-
 6 files changed, 156 insertions(+), 28 deletions(-)

-- 
2.19.1

[U-Boot] [PATCH v2 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 efitool command, 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 boot next 2
  => efi bootmgr
     (starting HELLO ...)
  => efi dumpvar
  <snip ...>
  BootCurrent: {boot,run}(blob)
  00000000:  02 00                    ..
  BootOrder: {boot,run}(blob)
  00000000:  01 00 02 00              ....

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

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a095df3f540b..6c5303736dc6 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,38 @@ error:
 void *efi_bootmgr_load(struct efi_device_path **device_path,
 		       struct efi_device_path **file_path)
 {
-	uint16_t *bootorder;
+	u16 bootnext, *bootorder;
 	efi_uintn_t size;
 	void *image = NULL;
 	int i, num;
+	efi_status_t ret;
 
 	__efi_entry_check();
 
 	bs = systab.boottime;
 	rs = systab.runtime;
 
+	/* get 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;
+
+	/* delete BootNext */
+	ret = rs->set_variable(L"BootNext",
+			       (efi_guid_t *)&efi_global_variable_guid,
+			       0, 0, &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 v2 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                | 3 ++-
 include/efi_loader.h         | 5 ++++-
 lib/efi_loader/efi_bootmgr.c | 9 ++++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7012d72ab50d..76171ab679c3 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -452,7 +452,8 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
+				&device_path, &file_path);
 	if (!addr)
 		return 1;
 
diff --git a/include/efi_loader.h b/include/efi_loader.h
index dd68cfce5c65..3077a1e9d58b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -549,9 +549,12 @@ struct efi_load_option {
 	u8 *optional_data;
 };
 
+#define EFI_BOOTMGR_DEFAULT_ORDER (-1)
+
 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 6c5303736dc6..7ce51de4dfb0 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;
@@ -194,6 +195,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;
+	}
+
 	/* get BootNext */
 	size = sizeof(bootnext);
 	ret = rs->get_variable(L"BootNext",
-- 
2.19.1

[U-Boot] [PATCH v2 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>
---
 cmd/bootefi.c | 49 +++++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 76171ab679c3..3be01b49b589 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);
 }
 
+static 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
  *
@@ -474,7 +499,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();
@@ -490,21 +514,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;
@@ -522,6 +531,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"))
@@ -534,6 +546,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];
@@ -543,6 +558,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);
-- 
2.19.1

[U-Boot] [PATCH v2 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 | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3be01b49b589..241fd0f987ab 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
-				&device_path, &file_path);
+	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
 	if (!addr)
-		return 1;
+		return CMD_RET_FAILURE;
 
 	printf("## Starting EFI application at %p ...\n", addr);
 	r = do_bootefi_exec(addr, device_path, file_path);
@@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
 	       r & ~EFI_ERROR_MASK);
 
 	if (r != EFI_SUCCESS)
-		return 1;
+		return CMD_RET_FAILURE;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 /* Interpreter command to boot an arbitrary EFI image from memory */
@@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
+		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
+
+		if (argc > 2) {
+			fdtstr = argv[2];
+			 /* Special address "-" means no device tree */
+			if (fdtstr[0] == '-')
+				fdtstr = NULL;
+
+			r = efi_handle_fdt(fdtstr);
+			if (r)
+				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];
 
@@ -590,7 +607,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"
@@ -598,7 +615,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 v2 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     | 31 +++++++++++++++++++++++++++++++
 cmd/nvedit.c      |  9 ++++++++-
 common/cli.c      | 10 ++++++++++
 include/command.h |  3 +++
 4 files changed, 52 insertions(+), 1 deletion(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 241fd0f987ab..ebe149dffa1f 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
 	return CMD_RET_SUCCESS;
 }
 
+/* Called by "run" command */
+int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int boot_id = -1;
+	char *endp;
+
+	if (argc > 2)
+		return CMD_RET_USAGE;
+
+	if (argc == 2) {
+		if (!strcmp(argv[1], "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);
+}
+
 /* Interpreter command to boot an arbitrary EFI image from memory */
 static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 {
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index de16c72c23f2..ce746bbf1b3e 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
 	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
 	"run commands in an environment variable",
 	"var [...]\n"
-	"    - run the commands in the environment variable(s) 'var'",
+	"    - run the commands in the environment variable(s) 'var'"
+#if defined(CONFIG_CMD_BOOTEFI)
+	"\n"
+	"run -e [BootXXXX]\n"
+	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
+#else
+	,
+#endif
 	var_complete
 );
 #endif
diff --git a/common/cli.c b/common/cli.c
index 51b8d5f85cbb..fbb09d5049a4 100644
--- a/common/cli.c
+++ b/common/cli.c
@@ -12,6 +12,7 @@
 #include <cli.h>
 #include <cli_hush.h>
 #include <console.h>
+#include <efi_loader.h>
 #include <fdtdec.h>
 #include <malloc.h>
 
@@ -125,6 +126,15 @@ 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")) {
+		argc--;
+		argv++;
+
+		return do_bootefi_run(cmdtp, flag, argc, argv);
+	}
+#endif
+
 	for (i = 1; i < argc; ++i) {
 		char *arg;
 
diff --git a/include/command.h b/include/command.h
index 200c7a5e9f4e..feddef300ccc 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)
+extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+#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 v2 1/5] efi_loader: bootmgr: support BootNext and BootCurrent variable behavior

[Heinrich Schuchardt]

On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> See UEFI v2.7, section 3.1.2 for details of the specification.
> 
> With my efitool command, 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 boot next 2
>   => efi bootmgr
>      (starting HELLO ...)
>   => efi dumpvar
>   <snip ...>
>   BootCurrent: {boot,run}(blob)
>   00000000:  02 00                    ..
>   BootOrder: {boot,run}(blob)
>   00000000:  01 00 02 00              ....
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a095df3f540b..6c5303736dc6 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,

Use EFI_CALL(). Instead of dereferencing you could directly call
efi_set_variable().

> +				       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,38 @@ error:
>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>  		       struct efi_device_path **file_path)
>  {
> -	uint16_t *bootorder;
> +	u16 bootnext, *bootorder;
>  	efi_uintn_t size;
>  	void *image = NULL;
>  	int i, num;
> +	efi_status_t ret;
>  
>  	__efi_entry_check();
>  
>  	bs = systab.boottime;
>  	rs = systab.runtime;
>  
> +	/* get BootNext */
> +	size = sizeof(bootnext);
> +	ret = rs->get_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       NULL, &size, &bootnext);

You could call efi_get_variable() directly instead of dereferencing rs.
But anyway you have to use EFI_CALL().

> +	if (!bootnext)
> +		goto run_list;

Goto is acceptable for error handling. But otherwise I would rather
avoid it.

> +
> +	/* delete BootNext */
> +	ret = rs->set_variable(L"BootNext",
> +			       (efi_guid_t *)&efi_global_variable_guid,
> +			       0, 0, &bootnext);

EFI_CALL().

Best regards

Heinrich

> +	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 v2 5/5] cmd: run: add "-e" option to run an EFI application

[Heinrich Schuchardt]

On 1/15/19 3:54 AM, 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."
> 

If we cannot specify the device tree what would be the use of this for
ARM processors?

Why do you add the option to run and not to bootefi?

You already introduced the capability to set BootNext. Why isn't that
enough?

Best regards

Heinrich

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
>  cmd/nvedit.c      |  9 ++++++++-
>  common/cli.c      | 10 ++++++++++
>  include/command.h |  3 +++
>  4 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 241fd0f987ab..ebe149dffa1f 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>  	return CMD_RET_SUCCESS;
>  }
>  
> +/* Called by "run" command */
> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int boot_id = -1;
> +	char *endp;
> +
> +	if (argc > 2)
> +		return CMD_RET_USAGE;
> +
> +	if (argc == 2) {
> +		if (!strcmp(argv[1], "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);
> +}
> +
>  /* Interpreter command to boot an arbitrary EFI image from memory */
>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index de16c72c23f2..ce746bbf1b3e 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
>  	"var [...]\n"
> -	"    - run the commands in the environment variable(s) 'var'",
> +	"    - run the commands in the environment variable(s) 'var'"
> +#if defined(CONFIG_CMD_BOOTEFI)
> +	"\n"
> +	"run -e [BootXXXX]\n"
> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> +#else
> +	,
> +#endif
>  	var_complete
>  );
>  #endif
> diff --git a/common/cli.c b/common/cli.c
> index 51b8d5f85cbb..fbb09d5049a4 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -12,6 +12,7 @@
>  #include <cli.h>
>  #include <cli_hush.h>
>  #include <console.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <malloc.h>
>  
> @@ -125,6 +126,15 @@ 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")) {
> +		argc--;
> +		argv++;
> +
> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> +	}
> +#endif
> +
>  	for (i = 1; i < argc; ++i) {
>  		char *arg;
>  
> diff --git a/include/command.h b/include/command.h
> index 200c7a5e9f4e..feddef300ccc 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)
> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> +#endif
>  
>  /* common/command.c */
>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> 

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

[Heinrich Schuchardt]

On 1/15/19 3:54 AM, 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

You already introduced the support for BootNext. So is there a real benefit?

> 
> Please note that BootXXXX need not be included in "BootOrder".
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3be01b49b589..241fd0f987ab 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
> -				&device_path, &file_path);
> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>  	if (!addr)
> -		return 1;
> +		return CMD_RET_FAILURE;
>  
>  	printf("## Starting EFI application at %p ...\n", addr);
>  	r = do_bootefi_exec(addr, device_path, file_path);
> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
>  	       r & ~EFI_ERROR_MASK);
>  
>  	if (r != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;
>  
> -	return 0;
> +	return CMD_RET_SUCCESS;
>  }
>  
>  /* Interpreter command to boot an arbitrary EFI image from memory */
> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
> +
> +		if (argc > 2) {
> +			fdtstr = argv[2];
> +			 /* Special address "-" means no device tree */
> +			if (fdtstr[0] == '-')
> +				fdtstr = NULL;
> +
> +			r = efi_handle_fdt(fdtstr);
> +			if (r)
> +				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);

Why not communicate via the BootNext variable?

>  	} else {
>  		saddr = argv[1];
>  
> @@ -590,7 +607,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"
> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
>  #endif
>  
>  U_BOOT_CMD(
> -	bootefi, 3, 0, do_bootefi,
> +	bootefi, 5, 0, do_bootefi,

Why 5?

Best regards

Heinrich

>  	"Boots an EFI payload from memory",
>  	bootefi_help_text
>  );
> 

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

[AKASHI Takahiro]

On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> > See UEFI v2.7, section 3.1.2 for details of the specification.
> > 
> > With my efitool command, 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 boot next 2
> >   => efi bootmgr
> >      (starting HELLO ...)
> >   => efi dumpvar
> >   <snip ...>
> >   BootCurrent: {boot,run}(blob)
> >   00000000:  02 00                    ..
> >   BootOrder: {boot,run}(blob)
> >   00000000:  01 00 02 00              ....
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a095df3f540b..6c5303736dc6 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,
> 
> Use EFI_CALL().

Okay
But as I said somewhere else, it's quite annoying to me that
some efi_xxx requires EFI_CALL(), and others not.
There should have been consistent naming rules.

> Instead of dereferencing you could directly call
> efi_set_variable().

Yeah, given that this code is under lib/efi_loader, it may be natural
to use efi_set_variable(). But existing get_var() uses the same style of coding.

Do you want to change all of the call sites including get_var()?

> > +				       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,38 @@ error:
> >  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		       struct efi_device_path **file_path)
> >  {
> > -	uint16_t *bootorder;
> > +	u16 bootnext, *bootorder;
> >  	efi_uintn_t size;
> >  	void *image = NULL;
> >  	int i, num;
> > +	efi_status_t ret;
> >  
> >  	__efi_entry_check();
> >  
> >  	bs = systab.boottime;
> >  	rs = systab.runtime;
> >  
> > +	/* get BootNext */
> > +	size = sizeof(bootnext);
> > +	ret = rs->get_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       NULL, &size, &bootnext);
> 
> You could call efi_get_variable() directly instead of dereferencing rs.
> But anyway you have to use EFI_CALL().

Ditto

> > +	if (!bootnext)
> > +		goto run_list;
> 
> Goto is acceptable for error handling. But otherwise I would rather
> avoid it.

Okay with another indentation.

> > +
> > +	/* delete BootNext */
> > +	ret = rs->set_variable(L"BootNext",
> > +			       (efi_guid_t *)&efi_global_variable_guid,
> > +			       0, 0, &bootnext);
> 
> EFI_CALL().

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	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 v2 4/5] cmd: bootefi: run an EFI application of a specific load option

[AKASHI Takahiro]

On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:54 AM, 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
> 
> You already introduced the support for BootNext. So is there a real benefit?

This is a convenient way of running EFI application directly,
but I already removed this feature from the next version.

> > 
> > Please note that BootXXXX need not be included in "BootOrder".
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 28 insertions(+), 11 deletions(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3be01b49b589..241fd0f987ab 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
> > -				&device_path, &file_path);
> > +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
> >  	if (!addr)
> > -		return 1;
> > +		return CMD_RET_FAILURE;
> >  
> >  	printf("## Starting EFI application at %p ...\n", addr);
> >  	r = do_bootefi_exec(addr, device_path, file_path);
> > @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
> >  	       r & ~EFI_ERROR_MASK);
> >  
> >  	if (r != EFI_SUCCESS)
> > -		return 1;
> > +		return CMD_RET_FAILURE;
> >  
> > -	return 0;
> > +	return CMD_RET_SUCCESS;
> >  }
> >  
> >  /* Interpreter command to boot an arbitrary EFI image from memory */
> > @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
> > +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
> > +
> > +		if (argc > 2) {
> > +			fdtstr = argv[2];
> > +			 /* Special address "-" means no device tree */
> > +			if (fdtstr[0] == '-')
> > +				fdtstr = NULL;
> > +
> > +			r = efi_handle_fdt(fdtstr);
> > +			if (r)
> > +				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);
> 
> Why not communicate via the BootNext variable?

I don't get your point.
BootNext and BootOrder are both defined by UEFI specification.

> >  	} else {
> >  		saddr = argv[1];
> >  
> > @@ -590,7 +607,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"
> > @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
> >  #endif
> >  
> >  U_BOOT_CMD(
> > -	bootefi, 3, 0, do_bootefi,
> > +	bootefi, 5, 0, do_bootefi,
> 
> Why 5?

For additional/optional '-' and <boot id>.
But I removed this feature from bootefi.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >  	"Boots an EFI payload from memory",
> >  	bootefi_help_text
> >  );
> > 
> 

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

[AKASHI Takahiro]

On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:54 AM, 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."
> > 
> 
> If we cannot specify the device tree what would be the use of this for
> ARM processors?

I don't get your point. What's the matter with device tree?


> Why do you add the option to run and not to bootefi?
> 
> You already introduced the capability to set BootNext. Why isn't that
> enough?

Simple.

=> run -e Boot0001

versus

=> efidebug boot next 1
=> bootefi bootmgr

First of all, efidebug is only recognized as a *debugging* tool.
I believe that the former syntax is more intuitive, and it looks
a natural extension to "run" command semantics akin to "env -e".

As a result, we don't have to know about bootefi for normal operations.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >  cmd/nvedit.c      |  9 ++++++++-
> >  common/cli.c      | 10 ++++++++++
> >  include/command.h |  3 +++
> >  4 files changed, 52 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 241fd0f987ab..ebe149dffa1f 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +/* Called by "run" command */
> > +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	int boot_id = -1;
> > +	char *endp;
> > +
> > +	if (argc > 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	if (argc == 2) {
> > +		if (!strcmp(argv[1], "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);
> > +}
> > +
> >  /* Interpreter command to boot an arbitrary EFI image from memory */
> >  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  {
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index de16c72c23f2..ce746bbf1b3e 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >  	"run commands in an environment variable",
> >  	"var [...]\n"
> > -	"    - run the commands in the environment variable(s) 'var'",
> > +	"    - run the commands in the environment variable(s) 'var'"
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +	"\n"
> > +	"run -e [BootXXXX]\n"
> > +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > +#else
> > +	,
> > +#endif
> >  	var_complete
> >  );
> >  #endif
> > diff --git a/common/cli.c b/common/cli.c
> > index 51b8d5f85cbb..fbb09d5049a4 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -12,6 +12,7 @@
> >  #include <cli.h>
> >  #include <cli_hush.h>
> >  #include <console.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <malloc.h>
> >  
> > @@ -125,6 +126,15 @@ 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")) {
> > +		argc--;
> > +		argv++;
> > +
> > +		return do_bootefi_run(cmdtp, flag, argc, argv);
> > +	}
> > +#endif
> > +
> >  	for (i = 1; i < argc; ++i) {
> >  		char *arg;
> >  
> > diff --git a/include/command.h b/include/command.h
> > index 200c7a5e9f4e..feddef300ccc 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)
> > +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > +#endif
> >  
> >  /* common/command.c */
> >  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> > 
> 

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

[Heinrich Schuchardt]

On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
> On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>
>>> With my efitool command, 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 boot next 2
>>>   => efi bootmgr
>>>      (starting HELLO ...)
>>>   => efi dumpvar
>>>   <snip ...>
>>>   BootCurrent: {boot,run}(blob)
>>>   00000000:  02 00                    ..
>>>   BootOrder: {boot,run}(blob)
>>>   00000000:  01 00 02 00              ....
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
>>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index a095df3f540b..6c5303736dc6 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,
>>
>> Use EFI_CALL().
> 
> Okay
> But as I said somewhere else, it's quite annoying to me that
> some efi_xxx requires EFI_CALL(), and others not.
> There should have been consistent naming rules.

We started with having separate functions like efi_allocate_pages_ext()
and efi_allocate_pages(). Then Rob Clark came along and introduced
EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.

When running with DEBUG 1 it sometimes is helpful to see which function
is calling which other and where errors are originally reported.

But I am open to changes in this area.

> 
>> Instead of dereferencing you could directly call
>> efi_set_variable().
> 
> Yeah, given that this code is under lib/efi_loader, it may be natural
> to use efi_set_variable(). But existing get_var() uses the same style of coding.
> 
> Do you want to change all of the call sites including get_var()?

Calling efi_set_variable() directly uses less bytes of code than
rs->get_variable() which makes it preferable.

I have seen that iPXE modifies system->boottime to intercept system
calls. The same could be done by an EFI driver to the runtime vectors.

In the light of your work on secure boot I think we should not allow an
EFI driver to intercept the reading and changing of variables here.

We should also rethink it for efidebug.c

Best regards

Heinrich

> 
>>> +				       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,38 @@ error:
>>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>>>  		       struct efi_device_path **file_path)
>>>  {
>>> -	uint16_t *bootorder;
>>> +	u16 bootnext, *bootorder;
>>>  	efi_uintn_t size;
>>>  	void *image = NULL;
>>>  	int i, num;
>>> +	efi_status_t ret;
>>>  
>>>  	__efi_entry_check();
>>>  
>>>  	bs = systab.boottime;
>>>  	rs = systab.runtime;
>>>  
>>> +	/* get BootNext */
>>> +	size = sizeof(bootnext);
>>> +	ret = rs->get_variable(L"BootNext",
>>> +			       (efi_guid_t *)&efi_global_variable_guid,
>>> +			       NULL, &size, &bootnext);
>>
>> You could call efi_get_variable() directly instead of dereferencing rs.
>> But anyway you have to use EFI_CALL().
> 
> Ditto
> 
>>> +	if (!bootnext)
>>> +		goto run_list;
>>
>> Goto is acceptable for error handling. But otherwise I would rather
>> avoid it.
> 
> Okay with another indentation.
> 
>>> +
>>> +	/* delete BootNext */
>>> +	ret = rs->set_variable(L"BootNext",
>>> +			       (efi_guid_t *)&efi_global_variable_guid,
>>> +			       0, 0, &bootnext);
>>
>> EFI_CALL().
> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>> +	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 v2 5/5] cmd: run: add "-e" option to run an EFI application

[Heinrich Schuchardt]

On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
>> On 1/15/19 3:54 AM, 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."
>>>
>>
>> If we cannot specify the device tree what would be the use of this for
>> ARM processors?
> 
> I don't get your point. What's the matter with device tree?

To boot an ARM board on Linux or BSD you need a device tree.

So run -e Boot0001 will not allow you to boot into Linux because it does
not specify a device tree.

> 
> 
>> Why do you add the option to run and not to bootefi?
>>
>> You already introduced the capability to set BootNext. Why isn't that
>> enough?
> 
> Simple.
> 
> => run -e Boot00>
> versus
> 
> => efidebug boot next 1
> => bootefi bootmgr

In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'

So there is no need to go through efidebug.

I think we should avoid alternative commands.

> 
> First of all, efidebug is only recognized as a *debugging* tool.
> I believe that the former syntax is more intuitive, and it looks
> a natural extension to "run" command semantics akin to "env -e".
> 
> As a result, we don't have to know about bootefi for normal operations.

But you have to know about 'run' which you might not need otherwise.

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
>>>  cmd/nvedit.c      |  9 ++++++++-
>>>  common/cli.c      | 10 ++++++++++
>>>  include/command.h |  3 +++
>>>  4 files changed, 52 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 241fd0f987ab..ebe149dffa1f 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>>>  	return CMD_RET_SUCCESS;
>>>  }
>>>  
>>> +/* Called by "run" command */
>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>> +{
>>> +	int boot_id = -1;
>>> +	char *endp;
>>> +
>>> +	if (argc > 2)
>>> +		return CMD_RET_USAGE;
>>> +
>>> +	if (argc == 2) {
>>> +		if (!strcmp(argv[1], "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);
>>> +}
>>> +
>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>  {
>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>> index de16c72c23f2..ce746bbf1b3e 100644
>>> --- a/cmd/nvedit.c
>>> +++ b/cmd/nvedit.c
>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>  	"run commands in an environment variable",
>>>  	"var [...]\n"
>>> -	"    - run the commands in the environment variable(s) 'var'",
>>> +	"    - run the commands in the environment variable(s) 'var'"
>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>> +	"\n"
>>> +	"run -e [BootXXXX]\n"
>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>> +#else
>>> +	,
>>> +#endif
>>>  	var_complete
>>>  );
>>>  #endif
>>> diff --git a/common/cli.c b/common/cli.c
>>> index 51b8d5f85cbb..fbb09d5049a4 100644
>>> --- a/common/cli.c
>>> +++ b/common/cli.c
>>> @@ -12,6 +12,7 @@
>>>  #include <cli.h>
>>>  #include <cli_hush.h>
>>>  #include <console.h>
>>> +#include <efi_loader.h>
>>>  #include <fdtdec.h>
>>>  #include <malloc.h>
>>>  
>>> @@ -125,6 +126,15 @@ 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")) {
>>> +		argc--;
>>> +		argv++;
>>> +
>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
>>> +	}
>>> +#endif
>>> +
>>>  	for (i = 1; i < argc; ++i) {
>>>  		char *arg;
>>>  
>>> diff --git a/include/command.h b/include/command.h
>>> index 200c7a5e9f4e..feddef300ccc 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)
>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>>> +#endif
>>>  
>>>  /* common/command.c */
>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
>>>
>>
> 

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

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
> >> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> >>> See UEFI v2.7, section 3.1.2 for details of the specification.
> >>>
> >>> With my efitool command, 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 boot next 2
> >>>   => efi bootmgr
> >>>      (starting HELLO ...)
> >>>   => efi dumpvar
> >>>   <snip ...>
> >>>   BootCurrent: {boot,run}(blob)
> >>>   00000000:  02 00                    ..
> >>>   BootOrder: {boot,run}(blob)
> >>>   00000000:  01 00 02 00              ....
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
> >>>  1 file changed, 33 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index a095df3f540b..6c5303736dc6 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,
> >>
> >> Use EFI_CALL().
> > 
> > Okay
> > But as I said somewhere else, it's quite annoying to me that
> > some efi_xxx requires EFI_CALL(), and others not.
> > There should have been consistent naming rules.
> 
> We started with having separate functions like efi_allocate_pages_ext()
> and efi_allocate_pages(). Then Rob Clark came along and introduced
> EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
> 
> When running with DEBUG 1 it sometimes is helpful to see which function
> is calling which other and where errors are originally reported.
> 
> But I am open to changes in this area.
> 
> > 
> >> Instead of dereferencing you could directly call
> >> efi_set_variable().
> > 
> > Yeah, given that this code is under lib/efi_loader, it may be natural
> > to use efi_set_variable(). But existing get_var() uses the same style of coding.
> > 
> > Do you want to change all of the call sites including get_var()?
> 
> Calling efi_set_variable() directly uses less bytes of code than
> rs->get_variable() which makes it preferable.

So is your answer yes, or no?

> I have seen that iPXE modifies system->boottime to intercept system
> calls. The same could be done by an EFI driver to the runtime vectors.
> 
> In the light of your work on secure boot I think we should not allow an
> EFI driver to intercept the reading and changing of variables here.
> 
> We should also rethink it for efidebug.c

I'm not sure about your concern here, but no doubt efidebug should
be disabled on production system with secure boot.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>> +				       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,38 @@ error:
> >>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>>  		       struct efi_device_path **file_path)
> >>>  {
> >>> -	uint16_t *bootorder;
> >>> +	u16 bootnext, *bootorder;
> >>>  	efi_uintn_t size;
> >>>  	void *image = NULL;
> >>>  	int i, num;
> >>> +	efi_status_t ret;
> >>>  
> >>>  	__efi_entry_check();
> >>>  
> >>>  	bs = systab.boottime;
> >>>  	rs = systab.runtime;
> >>>  
> >>> +	/* get BootNext */
> >>> +	size = sizeof(bootnext);
> >>> +	ret = rs->get_variable(L"BootNext",
> >>> +			       (efi_guid_t *)&efi_global_variable_guid,
> >>> +			       NULL, &size, &bootnext);
> >>
> >> You could call efi_get_variable() directly instead of dereferencing rs.
> >> But anyway you have to use EFI_CALL().
> > 
> > Ditto
> > 
> >>> +	if (!bootnext)
> >>> +		goto run_list;
> >>
> >> Goto is acceptable for error handling. But otherwise I would rather
> >> avoid it.
> > 
> > Okay with another indentation.
> > 
> >>> +
> >>> +	/* delete BootNext */
> >>> +	ret = rs->set_variable(L"BootNext",
> >>> +			       (efi_guid_t *)&efi_global_variable_guid,
> >>> +			       0, 0, &bootnext);
> >>
> >> EFI_CALL().
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +	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 v2 4/5] cmd: bootefi: run an EFI application of a specific load option

[Heinrich Schuchardt]

On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
>> On 1/15/19 3:54 AM, 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
>>
>> You already introduced the support for BootNext. So is there a real benefit?
> 
> This is a convenient way of running EFI application directly,
> but I already removed this feature from the next version.

Please, remove 'run -e' instead because it cannot specify the device
tree needed for booting ARM boards.

> 
>>>
>>> Please note that BootXXXX need not be included in "BootOrder".
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 3be01b49b589..241fd0f987ab 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
>>> -				&device_path, &file_path);
>>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>>>  	if (!addr)
>>> -		return 1;
>>> +		return CMD_RET_FAILURE;
>>>  
>>>  	printf("## Starting EFI application at %p ...\n", addr);
>>>  	r = do_bootefi_exec(addr, device_path, file_path);
>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
>>>  	       r & ~EFI_ERROR_MASK);
>>>  
>>>  	if (r != EFI_SUCCESS)
>>> -		return 1;
>>> +		return CMD_RET_FAILURE;
>>>  
>>> -	return 0;
>>> +	return CMD_RET_SUCCESS;
>>>  }
>>>  
>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
>>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
>>> +
>>> +		if (argc > 2) {
>>> +			fdtstr = argv[2];
>>> +			 /* Special address "-" means no device tree */
>>> +			if (fdtstr[0] == '-')
>>> +				fdtstr = NULL;
>>> +
>>> +			r = efi_handle_fdt(fdtstr);
>>> +			if (r)
>>> +				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);
>>
>> Why not communicate via the BootNext variable?
> 
> I don't get your point.
> BootNext and BootOrder are both defined by UEFI specification.

Instead of changing the interface of do_bootefi_bootmgr_exec() you could
simply set BootNext. Then the boot manager would pick up the option from
the variable and finally delete the variable. This would result in less
code.

Best regards

Heinrich

> 
>>>  	} else {
>>>  		saddr = argv[1];
>>>  
>>> @@ -590,7 +607,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"
>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
>>>  #endif
>>>  
>>>  U_BOOT_CMD(
>>> -	bootefi, 3, 0, do_bootefi,
>>> +	bootefi, 5, 0, do_bootefi,
>>
>> Why 5?
> 
> For additional/optional '-' and <boot id>.
> But I removed this feature from bootefi.
> 
> Thanks,
> -Takahiro Akashi
> 
> 
>> Best regards
>>
>> Heinrich
>>
>>>  	"Boots an EFI payload from memory",
>>>  	bootefi_help_text
>>>  );
>>>
>>
> 

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

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> >> On 1/15/19 3:54 AM, 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."
> >>>
> >>
> >> If we cannot specify the device tree what would be the use of this for
> >> ARM processors?
> > 
> > I don't get your point. What's the matter with device tree?
> 
> To boot an ARM board on Linux or BSD you need a device tree.

When I discussed with Alex about Boot Manager (and distro_bootcmd?),
he suggested that we should not allow users to specify fdt at a command line.
I believe that it would apply to my case above.

IMO, we should always provide system's fdt to EFI applications.

> So run -e Boot0001 will not allow you to boot into Linux because it does
> not specify a device tree.
> 
> > 
> > 
> >> Why do you add the option to run and not to bootefi?
> >>
> >> You already introduced the capability to set BootNext. Why isn't that
> >> enough?
> > 
> > Simple.
> > 
> > => run -e Boot00>
> > versus
> > 
> > => efidebug boot next 1
> > => bootefi bootmgr
> 
> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> 
> So there is no need to go through efidebug.
> 
> I think we should avoid alternative commands.

As I said, I already removed this feature from bootefi.

> > 
> > First of all, efidebug is only recognized as a *debugging* tool.
> > I believe that the former syntax is more intuitive, and it looks
> > a natural extension to "run" command semantics akin to "env -e".
> > 
> > As a result, we don't have to know about bootefi for normal operations.
> 
> But you have to know about 'run' which you might not need otherwise.

"run" is much better known to U-Boot users than bootefi.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >>>  cmd/nvedit.c      |  9 ++++++++-
> >>>  common/cli.c      | 10 ++++++++++
> >>>  include/command.h |  3 +++
> >>>  4 files changed, 52 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 241fd0f987ab..ebe149dffa1f 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >>>  	return CMD_RET_SUCCESS;
> >>>  }
> >>>  
> >>> +/* Called by "run" command */
> >>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>> +{
> >>> +	int boot_id = -1;
> >>> +	char *endp;
> >>> +
> >>> +	if (argc > 2)
> >>> +		return CMD_RET_USAGE;
> >>> +
> >>> +	if (argc == 2) {
> >>> +		if (!strcmp(argv[1], "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);
> >>> +}
> >>> +
> >>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  {
> >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>> index de16c72c23f2..ce746bbf1b3e 100644
> >>> --- a/cmd/nvedit.c
> >>> +++ b/cmd/nvedit.c
> >>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>  	"run commands in an environment variable",
> >>>  	"var [...]\n"
> >>> -	"    - run the commands in the environment variable(s) 'var'",
> >>> +	"    - run the commands in the environment variable(s) 'var'"
> >>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>> +	"\n"
> >>> +	"run -e [BootXXXX]\n"
> >>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>> +#else
> >>> +	,
> >>> +#endif
> >>>  	var_complete
> >>>  );
> >>>  #endif
> >>> diff --git a/common/cli.c b/common/cli.c
> >>> index 51b8d5f85cbb..fbb09d5049a4 100644
> >>> --- a/common/cli.c
> >>> +++ b/common/cli.c
> >>> @@ -12,6 +12,7 @@
> >>>  #include <cli.h>
> >>>  #include <cli_hush.h>
> >>>  #include <console.h>
> >>> +#include <efi_loader.h>
> >>>  #include <fdtdec.h>
> >>>  #include <malloc.h>
> >>>  
> >>> @@ -125,6 +126,15 @@ 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")) {
> >>> +		argc--;
> >>> +		argv++;
> >>> +
> >>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> >>> +	}
> >>> +#endif
> >>> +
> >>>  	for (i = 1; i < argc; ++i) {
> >>>  		char *arg;
> >>>  
> >>> diff --git a/include/command.h b/include/command.h
> >>> index 200c7a5e9f4e..feddef300ccc 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)
> >>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>> +#endif
> >>>  
> >>>  /* common/command.c */
> >>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >>>
> >>
> > 
> 

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

[Heinrich Schuchardt]

On 2/27/19 7:27 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
>> On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
>>> On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
>>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
>>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
>>>>>
>>>>> With my efitool command, 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 boot next 2
>>>>>   => efi bootmgr
>>>>>      (starting HELLO ...)
>>>>>   => efi dumpvar
>>>>>   <snip ...>
>>>>>   BootCurrent: {boot,run}(blob)
>>>>>   00000000:  02 00                    ..
>>>>>   BootOrder: {boot,run}(blob)
>>>>>   00000000:  01 00 02 00              ....
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 33 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>> index a095df3f540b..6c5303736dc6 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,
>>>>
>>>> Use EFI_CALL().
>>>
>>> Okay
>>> But as I said somewhere else, it's quite annoying to me that
>>> some efi_xxx requires EFI_CALL(), and others not.
>>> There should have been consistent naming rules.
>>
>> We started with having separate functions like efi_allocate_pages_ext()
>> and efi_allocate_pages(). Then Rob Clark came along and introduced
>> EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
>>
>> When running with DEBUG 1 it sometimes is helpful to see which function
>> is calling which other and where errors are originally reported.
>>
>> But I am open to changes in this area.
>>
>>>
>>>> Instead of dereferencing you could directly call
>>>> efi_set_variable().
>>>
>>> Yeah, given that this code is under lib/efi_loader, it may be natural
>>> to use efi_set_variable(). But existing get_var() uses the same style of coding.
>>>
>>> Do you want to change all of the call sites including get_var()?
>>
>> Calling efi_set_variable() directly uses less bytes of code than
>> rs->get_variable() which makes it preferable.
> 
> So is your answer yes, or no?

I would prefer calling efi_get_variable() directly and not to use
rs->get_variable().

> 
>> I have seen that iPXE modifies system->boottime to intercept system
>> calls. The same could be done by an EFI driver to the runtime vectors.
>>
>> In the light of your work on secure boot I think we should not allow an
>> EFI driver to intercept the reading and changing of variables here.
>>
>> We should also rethink it for efidebug.c
> 
> I'm not sure about your concern here, but no doubt efidebug should
> be disabled on production system with secure boot.

Also in efidebug we are creating more runtime code bytes than needed by
using system->runtime->efi_something() or system->boottime->efi_something().

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>> +				       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,38 @@ error:
>>>>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
>>>>>  		       struct efi_device_path **file_path)
>>>>>  {
>>>>> -	uint16_t *bootorder;
>>>>> +	u16 bootnext, *bootorder;
>>>>>  	efi_uintn_t size;
>>>>>  	void *image = NULL;
>>>>>  	int i, num;
>>>>> +	efi_status_t ret;
>>>>>  
>>>>>  	__efi_entry_check();
>>>>>  
>>>>>  	bs = systab.boottime;
>>>>>  	rs = systab.runtime;
>>>>>  
>>>>> +	/* get BootNext */
>>>>> +	size = sizeof(bootnext);
>>>>> +	ret = rs->get_variable(L"BootNext",
>>>>> +			       (efi_guid_t *)&efi_global_variable_guid,
>>>>> +			       NULL, &size, &bootnext);
>>>>
>>>> You could call efi_get_variable() directly instead of dereferencing rs.
>>>> But anyway you have to use EFI_CALL().
>>>
>>> Ditto
>>>
>>>>> +	if (!bootnext)
>>>>> +		goto run_list;
>>>>
>>>> Goto is acceptable for error handling. But otherwise I would rather
>>>> avoid it.
>>>
>>> Okay with another indentation.
>>>
>>>>> +
>>>>> +	/* delete BootNext */
>>>>> +	ret = rs->set_variable(L"BootNext",
>>>>> +			       (efi_guid_t *)&efi_global_variable_guid,
>>>>> +			       0, 0, &bootnext);
>>>>
>>>> EFI_CALL().
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +	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 v2 4/5] cmd: bootefi: run an EFI application of a specific load option

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
> > On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
> >> On 1/15/19 3:54 AM, 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
> >>
> >> You already introduced the support for BootNext. So is there a real benefit?
> > 
> > This is a convenient way of running EFI application directly,
> > but I already removed this feature from the next version.
> 
> Please, remove 'run -e' instead because it cannot specify the device
> tree needed for booting ARM boards.

See my comment for patch#5 first.

> > 
> >>>
> >>> Please note that BootXXXX need not be included in "BootOrder".
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>> ---
> >>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
> >>>  1 file changed, 28 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index 3be01b49b589..241fd0f987ab 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
> >>> -				&device_path, &file_path);
> >>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
> >>>  	if (!addr)
> >>> -		return 1;
> >>> +		return CMD_RET_FAILURE;
> >>>  
> >>>  	printf("## Starting EFI application at %p ...\n", addr);
> >>>  	r = do_bootefi_exec(addr, device_path, file_path);
> >>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
> >>>  	       r & ~EFI_ERROR_MASK);
> >>>  
> >>>  	if (r != EFI_SUCCESS)
> >>> -		return 1;
> >>> +		return CMD_RET_FAILURE;
> >>>  
> >>> -	return 0;
> >>> +	return CMD_RET_SUCCESS;
> >>>  }
> >>>  
> >>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
> >>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
> >>> +
> >>> +		if (argc > 2) {
> >>> +			fdtstr = argv[2];
> >>> +			 /* Special address "-" means no device tree */
> >>> +			if (fdtstr[0] == '-')
> >>> +				fdtstr = NULL;
> >>> +
> >>> +			r = efi_handle_fdt(fdtstr);
> >>> +			if (r)
> >>> +				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);
> >>
> >> Why not communicate via the BootNext variable?
> > 
> > I don't get your point.
> > BootNext and BootOrder are both defined by UEFI specification.
> 
> Instead of changing the interface of do_bootefi_bootmgr_exec()

Who care changing an *internal* function?

> you could
> simply set BootNext. Then the boot manager would pick up the option from
> the variable and finally delete the variable. This would result in less
> code.

No. Even with "run -e," BootNext will disappear after execution.
This is a requirement by UEFI spec.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >>>  	} else {
> >>>  		saddr = argv[1];
> >>>  
> >>> @@ -590,7 +607,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"
> >>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
> >>>  #endif
> >>>  
> >>>  U_BOOT_CMD(
> >>> -	bootefi, 3, 0, do_bootefi,
> >>> +	bootefi, 5, 0, do_bootefi,
> >>
> >> Why 5?
> > 
> > For additional/optional '-' and <boot id>.
> > But I removed this feature from bootefi.
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>  	"Boots an EFI payload from memory",
> >>>  	bootefi_help_text
> >>>  );
> >>>
> >>
> > 
> 

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

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 07:39:50AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 7:27 AM, AKASHI Takahiro wrote:
> > On Wed, Feb 27, 2019 at 07:14:10AM +0100, Heinrich Schuchardt wrote:
> >> On 2/27/19 6:47 AM, AKASHI Takahiro wrote:
> >>> On Tue, Feb 26, 2019 at 07:57:26PM +0100, Heinrich Schuchardt wrote:
> >>>> On 1/15/19 3:54 AM, AKASHI Takahiro wrote:
> >>>>> See UEFI v2.7, section 3.1.2 for details of the specification.
> >>>>>
> >>>>> With my efitool command, 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 boot next 2
> >>>>>   => efi bootmgr
> >>>>>      (starting HELLO ...)
> >>>>>   => efi dumpvar
> >>>>>   <snip ...>
> >>>>>   BootCurrent: {boot,run}(blob)
> >>>>>   00000000:  02 00                    ..
> >>>>>   BootOrder: {boot,run}(blob)
> >>>>>   00000000:  01 00 02 00              ....
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>> ---
> >>>>>  lib/efi_loader/efi_bootmgr.c | 34 +++++++++++++++++++++++++++++++++-
> >>>>>  1 file changed, 33 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>>>> index a095df3f540b..6c5303736dc6 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,
> >>>>
> >>>> Use EFI_CALL().
> >>>
> >>> Okay
> >>> But as I said somewhere else, it's quite annoying to me that
> >>> some efi_xxx requires EFI_CALL(), and others not.
> >>> There should have been consistent naming rules.
> >>
> >> We started with having separate functions like efi_allocate_pages_ext()
> >> and efi_allocate_pages(). Then Rob Clark came along and introduced
> >> EFI_CALL() in a095aadffa96 and I stopped creating _ext() functions.
> >>
> >> When running with DEBUG 1 it sometimes is helpful to see which function
> >> is calling which other and where errors are originally reported.
> >>
> >> But I am open to changes in this area.
> >>
> >>>
> >>>> Instead of dereferencing you could directly call
> >>>> efi_set_variable().
> >>>
> >>> Yeah, given that this code is under lib/efi_loader, it may be natural
> >>> to use efi_set_variable(). But existing get_var() uses the same style of coding.
> >>>
> >>> Do you want to change all of the call sites including get_var()?
> >>
> >> Calling efi_set_variable() directly uses less bytes of code than
> >> rs->get_variable() which makes it preferable.
> > 
> > So is your answer yes, or no?
> 
> I would prefer calling efi_get_variable() directly and not to use
> rs->get_variable().

My point is "including get_var()" or not.
I have never touched that function in my patch.

> > 
> >> I have seen that iPXE modifies system->boottime to intercept system
> >> calls. The same could be done by an EFI driver to the runtime vectors.
> >>
> >> In the light of your work on secure boot I think we should not allow an
> >> EFI driver to intercept the reading and changing of variables here.
> >>
> >> We should also rethink it for efidebug.c
> > 
> > I'm not sure about your concern here, but no doubt efidebug should
> > be disabled on production system with secure boot.
> 
> Also in efidebug we are creating more runtime code bytes than needed by
> using system->runtime->efi_something() or system->boottime->efi_something().

I think we discussed in the past.
I prefer to calling boot time/run time services via system table
as this command is expected to be implemented as an (embedded) EFI application
sometime in the future.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>>> +				       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,38 @@ error:
> >>>>>  void *efi_bootmgr_load(struct efi_device_path **device_path,
> >>>>>  		       struct efi_device_path **file_path)
> >>>>>  {
> >>>>> -	uint16_t *bootorder;
> >>>>> +	u16 bootnext, *bootorder;
> >>>>>  	efi_uintn_t size;
> >>>>>  	void *image = NULL;
> >>>>>  	int i, num;
> >>>>> +	efi_status_t ret;
> >>>>>  
> >>>>>  	__efi_entry_check();
> >>>>>  
> >>>>>  	bs = systab.boottime;
> >>>>>  	rs = systab.runtime;
> >>>>>  
> >>>>> +	/* get BootNext */
> >>>>> +	size = sizeof(bootnext);
> >>>>> +	ret = rs->get_variable(L"BootNext",
> >>>>> +			       (efi_guid_t *)&efi_global_variable_guid,
> >>>>> +			       NULL, &size, &bootnext);
> >>>>
> >>>> You could call efi_get_variable() directly instead of dereferencing rs.
> >>>> But anyway you have to use EFI_CALL().
> >>>
> >>> Ditto
> >>>
> >>>>> +	if (!bootnext)
> >>>>> +		goto run_list;
> >>>>
> >>>> Goto is acceptable for error handling. But otherwise I would rather
> >>>> avoid it.
> >>>
> >>> Okay with another indentation.
> >>>
> >>>>> +
> >>>>> +	/* delete BootNext */
> >>>>> +	ret = rs->set_variable(L"BootNext",
> >>>>> +			       (efi_guid_t *)&efi_global_variable_guid,
> >>>>> +			       0, 0, &bootnext);
> >>>>
> >>>> EFI_CALL().
> >>>
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>> +	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 v2 5/5] cmd: run: add "-e" option to run an EFI application

[Heinrich Schuchardt]

On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
>>>> On 1/15/19 3:54 AM, 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."
>>>>>
>>>>
>>>> If we cannot specify the device tree what would be the use of this for
>>>> ARM processors?
>>>
>>> I don't get your point. What's the matter with device tree?
>>
>> To boot an ARM board on Linux or BSD you need a device tree.
> 
> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> he suggested that we should not allow users to specify fdt at a command line.
> I believe that it would apply to my case above.
> 
> IMO, we should always provide system's fdt to EFI applications.

With current Linux development practice this unfortunately does not
work. Linux device trees sometimes see incompatible changes between
versions. Booting may fail with a device tree that is either too old or
too new for your Linux version.

E.g. for the Odroid C2 some reserved memory regions were removed from
the device tree and replaced by a logic that determines them on the fly
due to changes in ARM trusted firmware location.

The Wandboard rev B1 device tree was moved to a new file when a new
board revision appeared and the new revision changed the old file (sic).

U-Boot is also not perfect at keeping its device tree in sync with the
newest Linux device tree.

> 
>> So run -e Boot0001 will not allow you to boot into Linux because it does
>> not specify a device tree.
>>
>>>
>>>
>>>> Why do you add the option to run and not to bootefi?
>>>>
>>>> You already introduced the capability to set BootNext. Why isn't that
>>>> enough?
>>>
>>> Simple.
>>>
>>> => run -e Boot00>
>>> versus
>>>
>>> => efidebug boot next 1
>>> => bootefi bootmgr
>>
>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
>>
>> So there is no need to go through efidebug.
>>
>> I think we should avoid alternative commands.
> 
> As I said, I already removed this feature from bootefi.
> 
>>>
>>> First of all, efidebug is only recognized as a *debugging* tool.
>>> I believe that the former syntax is more intuitive, and it looks
>>> a natural extension to "run" command semantics akin to "env -e".
>>>
>>> As a result, we don't have to know about bootefi for normal operations.
>>
>> But you have to know about 'run' which you might not need otherwise.
> 
> "run" is much better known to U-Boot users than bootefi.

Do you have a statistic ;)

Up to now booting always required a command starting on boot...

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
>>>>>  cmd/nvedit.c      |  9 ++++++++-
>>>>>  common/cli.c      | 10 ++++++++++
>>>>>  include/command.h |  3 +++
>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index 241fd0f987ab..ebe149dffa1f 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>>>>>  	return CMD_RET_SUCCESS;
>>>>>  }
>>>>>  
>>>>> +/* Called by "run" command */
>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>> +{
>>>>> +	int boot_id = -1;
>>>>> +	char *endp;
>>>>> +
>>>>> +	if (argc > 2)
>>>>> +		return CMD_RET_USAGE;
>>>>> +
>>>>> +	if (argc == 2) {
>>>>> +		if (!strcmp(argv[1], "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);
>>>>> +}
>>>>> +
>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>  {
>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>>>> index de16c72c23f2..ce746bbf1b3e 100644
>>>>> --- a/cmd/nvedit.c
>>>>> +++ b/cmd/nvedit.c
>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>>>  	"run commands in an environment variable",
>>>>>  	"var [...]\n"
>>>>> -	"    - run the commands in the environment variable(s) 'var'",
>>>>> +	"    - run the commands in the environment variable(s) 'var'"
>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>>>> +	"\n"
>>>>> +	"run -e [BootXXXX]\n"
>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>>>> +#else
>>>>> +	,
>>>>> +#endif
>>>>>  	var_complete
>>>>>  );
>>>>>  #endif
>>>>> diff --git a/common/cli.c b/common/cli.c
>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
>>>>> --- a/common/cli.c
>>>>> +++ b/common/cli.c
>>>>> @@ -12,6 +12,7 @@
>>>>>  #include <cli.h>
>>>>>  #include <cli_hush.h>
>>>>>  #include <console.h>
>>>>> +#include <efi_loader.h>
>>>>>  #include <fdtdec.h>
>>>>>  #include <malloc.h>
>>>>>  
>>>>> @@ -125,6 +126,15 @@ 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")) {
>>>>> +		argc--;
>>>>> +		argv++;
>>>>> +
>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
>>>>> +	}
>>>>> +#endif
>>>>> +
>>>>>  	for (i = 1; i < argc; ++i) {
>>>>>  		char *arg;
>>>>>  
>>>>> diff --git a/include/command.h b/include/command.h
>>>>> index 200c7a5e9f4e..feddef300ccc 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)
>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>>>>> +#endif
>>>>>  
>>>>>  /* common/command.c */
>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
>>>>>
>>>>
>>>
>>
> 

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

[Heinrich Schuchardt]

On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
>> On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
>>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
>>>> On 1/15/19 3:54 AM, 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
>>>>
>>>> You already introduced the support for BootNext. So is there a real benefit?
>>>
>>> This is a convenient way of running EFI application directly,
>>> but I already removed this feature from the next version.
>>
>> Please, remove 'run -e' instead because it cannot specify the device
>> tree needed for booting ARM boards.
> 
> See my comment for patch#5 first.
> 
>>>
>>>>>
>>>>> Please note that BootXXXX need not be included in "BootOrder".
>>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
>>>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index 3be01b49b589..241fd0f987ab 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
>>>>> -				&device_path, &file_path);
>>>>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>>>>>  	if (!addr)
>>>>> -		return 1;
>>>>> +		return CMD_RET_FAILURE;
>>>>>  
>>>>>  	printf("## Starting EFI application at %p ...\n", addr);
>>>>>  	r = do_bootefi_exec(addr, device_path, file_path);
>>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
>>>>>  	       r & ~EFI_ERROR_MASK);
>>>>>  
>>>>>  	if (r != EFI_SUCCESS)
>>>>> -		return 1;
>>>>> +		return CMD_RET_FAILURE;
>>>>>  
>>>>> -	return 0;
>>>>> +	return CMD_RET_SUCCESS;
>>>>>  }
>>>>>  
>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>>> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
>>>>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
>>>>> +
>>>>> +		if (argc > 2) {
>>>>> +			fdtstr = argv[2];
>>>>> +			 /* Special address "-" means no device tree */
>>>>> +			if (fdtstr[0] == '-')
>>>>> +				fdtstr = NULL;
>>>>> +
>>>>> +			r = efi_handle_fdt(fdtstr);
>>>>> +			if (r)
>>>>> +				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);
>>>>
>>>> Why not communicate via the BootNext variable?
>>>
>>> I don't get your point.
>>> BootNext and BootOrder are both defined by UEFI specification.
>>
>> Instead of changing the interface of do_bootefi_bootmgr_exec()
> 
> Who care changing an *internal* function?
> 
>> you could
>> simply set BootNext. Then the boot manager would pick up the option from
>> the variable and finally delete the variable. This would result in less
>> code.
> 
> No. Even with "run -e," BootNext will disappear after execution.
> This is a requirement by UEFI spec.

Shouldn't BootNext always be reset when executing bootefi no matter
whether the boot manager is used or not?

Regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>>  	} else {
>>>>>  		saddr = argv[1];
>>>>>  
>>>>> @@ -590,7 +607,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"
>>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
>>>>>  #endif
>>>>>  
>>>>>  U_BOOT_CMD(
>>>>> -	bootefi, 3, 0, do_bootefi,
>>>>> +	bootefi, 5, 0, do_bootefi,
>>>>
>>>> Why 5?
>>>
>>> For additional/optional '-' and <boot id>.
>>> But I removed this feature from bootefi.
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>  	"Boots an EFI payload from memory",
>>>>>  	bootefi_help_text
>>>>>  );
>>>>>
>>>>
>>>
>>
> 

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

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
> > On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
> >> On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
> >>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
> >>>> On 1/15/19 3:54 AM, 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
> >>>>
> >>>> You already introduced the support for BootNext. So is there a real benefit?
> >>>
> >>> This is a convenient way of running EFI application directly,
> >>> but I already removed this feature from the next version.
> >>
> >> Please, remove 'run -e' instead because it cannot specify the device
> >> tree needed for booting ARM boards.
> > 
> > See my comment for patch#5 first.
> > 
> >>>
> >>>>>
> >>>>> Please note that BootXXXX need not be included in "BootOrder".
> >>>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>> ---
> >>>>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
> >>>>>  1 file changed, 28 insertions(+), 11 deletions(-)
> >>>>>
> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>> index 3be01b49b589..241fd0f987ab 100644
> >>>>> --- a/cmd/bootefi.c
> >>>>> +++ b/cmd/bootefi.c
> >>>>> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
> >>>>> -				&device_path, &file_path);
> >>>>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
> >>>>>  	if (!addr)
> >>>>> -		return 1;
> >>>>> +		return CMD_RET_FAILURE;
> >>>>>  
> >>>>>  	printf("## Starting EFI application at %p ...\n", addr);
> >>>>>  	r = do_bootefi_exec(addr, device_path, file_path);
> >>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
> >>>>>  	       r & ~EFI_ERROR_MASK);
> >>>>>  
> >>>>>  	if (r != EFI_SUCCESS)
> >>>>> -		return 1;
> >>>>> +		return CMD_RET_FAILURE;
> >>>>>  
> >>>>> -	return 0;
> >>>>> +	return CMD_RET_SUCCESS;
> >>>>>  }
> >>>>>  
> >>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>>> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
> >>>>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
> >>>>> +
> >>>>> +		if (argc > 2) {
> >>>>> +			fdtstr = argv[2];
> >>>>> +			 /* Special address "-" means no device tree */
> >>>>> +			if (fdtstr[0] == '-')
> >>>>> +				fdtstr = NULL;
> >>>>> +
> >>>>> +			r = efi_handle_fdt(fdtstr);
> >>>>> +			if (r)
> >>>>> +				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);
> >>>>
> >>>> Why not communicate via the BootNext variable?
> >>>
> >>> I don't get your point.
> >>> BootNext and BootOrder are both defined by UEFI specification.
> >>
> >> Instead of changing the interface of do_bootefi_bootmgr_exec()
> > 
> > Who care changing an *internal* function?

So do you agree?

> > 
> >> you could
> >> simply set BootNext. Then the boot manager would pick up the option from
> >> the variable and finally delete the variable. This would result in less
> >> code.
> > 
> > No. Even with "run -e," BootNext will disappear after execution.
> > This is a requirement by UEFI spec.
> 
> Shouldn't BootNext always be reset when executing bootefi no matter
> whether the boot manager is used or not?

Didn't I say the same thing?
Or do you expect that BootNext remain after "run -e"?

-Takahiro Akashi

> Regards
> 
> Heinrich
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>>>  	} else {
> >>>>>  		saddr = argv[1];
> >>>>>  
> >>>>> @@ -590,7 +607,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"
> >>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
> >>>>>  #endif
> >>>>>  
> >>>>>  U_BOOT_CMD(
> >>>>> -	bootefi, 3, 0, do_bootefi,
> >>>>> +	bootefi, 5, 0, do_bootefi,
> >>>>
> >>>> Why 5?
> >>>
> >>> For additional/optional '-' and <boot id>.
> >>> But I removed this feature from bootefi.
> >>>
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>  	"Boots an EFI payload from memory",
> >>>>>  	bootefi_help_text
> >>>>>  );
> >>>>>
> >>>>
> >>>
> >>
> > 
> 

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

[AKASHI Takahiro]

On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> > On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> >> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> >>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> >>>> On 1/15/19 3:54 AM, 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."
> >>>>>
> >>>>
> >>>> If we cannot specify the device tree what would be the use of this for
> >>>> ARM processors?
> >>>
> >>> I don't get your point. What's the matter with device tree?
> >>
> >> To boot an ARM board on Linux or BSD you need a device tree.
> > 
> > When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> > he suggested that we should not allow users to specify fdt at a command line.
> > I believe that it would apply to my case above.
> > 
> > IMO, we should always provide system's fdt to EFI applications.
> 
> With current Linux development practice this unfortunately does not
> work. Linux device trees sometimes see incompatible changes between
> versions. Booting may fail with a device tree that is either too old or
> too new for your Linux version.
> 
> E.g. for the Odroid C2 some reserved memory regions were removed from
> the device tree and replaced by a logic that determines them on the fly
> due to changes in ARM trusted firmware location.
> 
> The Wandboard rev B1 device tree was moved to a new file when a new
> board revision appeared and the new revision changed the old file (sic).
> 
> U-Boot is also not perfect at keeping its device tree in sync with the
> newest Linux device tree.

Why don't you use "fdt" command in that case?
IMO, we don't need <fdt> argument at bootefi (and run -e).
Obviously, I have one assumption that we need change the code
to utilize "fdtaddr" variable in do_bootefi().

Under the current implementation, a similar behavior is achieved
only via distro_bootcmd. IMO, this should be corrected.
If you agree, I will add another patch to my current patchset
for this purpose.

> > 
> >> So run -e Boot0001 will not allow you to boot into Linux because it does
> >> not specify a device tree.
> >>
> >>>
> >>>
> >>>> Why do you add the option to run and not to bootefi?
> >>>>
> >>>> You already introduced the capability to set BootNext. Why isn't that
> >>>> enough?
> >>>
> >>> Simple.
> >>>
> >>> => run -e Boot00>
> >>> versus
> >>>
> >>> => efidebug boot next 1
> >>> => bootefi bootmgr
> >>
> >> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> >>
> >> So there is no need to go through efidebug.
> >>
> >> I think we should avoid alternative commands.
> > 
> > As I said, I already removed this feature from bootefi.
> > 
> >>>
> >>> First of all, efidebug is only recognized as a *debugging* tool.
> >>> I believe that the former syntax is more intuitive, and it looks
> >>> a natural extension to "run" command semantics akin to "env -e".
> >>>
> >>> As a result, we don't have to know about bootefi for normal operations.
> >>
> >> But you have to know about 'run' which you might not need otherwise.
> > 
> > "run" is much better known to U-Boot users than bootefi.
> 
> Do you have a statistic ;)
> 
> Up to now booting always required a command starting on boot...

What I meant is that people need not learn more commands.

# Relating to secure boot, I'm now thinking of pulling bootmgr out of
# bootefi and making it as a single command. In that case,
# bootefi does exist only for hello and selftest.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Thanks,
> > -Takahiro Akashi
> > 
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>> ---
> >>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >>>>>  cmd/nvedit.c      |  9 ++++++++-
> >>>>>  common/cli.c      | 10 ++++++++++
> >>>>>  include/command.h |  3 +++
> >>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>> index 241fd0f987ab..ebe149dffa1f 100644
> >>>>> --- a/cmd/bootefi.c
> >>>>> +++ b/cmd/bootefi.c
> >>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >>>>>  	return CMD_RET_SUCCESS;
> >>>>>  }
> >>>>>  
> >>>>> +/* Called by "run" command */
> >>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>> +{
> >>>>> +	int boot_id = -1;
> >>>>> +	char *endp;
> >>>>> +
> >>>>> +	if (argc > 2)
> >>>>> +		return CMD_RET_USAGE;
> >>>>> +
> >>>>> +	if (argc == 2) {
> >>>>> +		if (!strcmp(argv[1], "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);
> >>>>> +}
> >>>>> +
> >>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>>  {
> >>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>>>> index de16c72c23f2..ce746bbf1b3e 100644
> >>>>> --- a/cmd/nvedit.c
> >>>>> +++ b/cmd/nvedit.c
> >>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>>>  	"run commands in an environment variable",
> >>>>>  	"var [...]\n"
> >>>>> -	"    - run the commands in the environment variable(s) 'var'",
> >>>>> +	"    - run the commands in the environment variable(s) 'var'"
> >>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>>>> +	"\n"
> >>>>> +	"run -e [BootXXXX]\n"
> >>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>>>> +#else
> >>>>> +	,
> >>>>> +#endif
> >>>>>  	var_complete
> >>>>>  );
> >>>>>  #endif
> >>>>> diff --git a/common/cli.c b/common/cli.c
> >>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
> >>>>> --- a/common/cli.c
> >>>>> +++ b/common/cli.c
> >>>>> @@ -12,6 +12,7 @@
> >>>>>  #include <cli.h>
> >>>>>  #include <cli_hush.h>
> >>>>>  #include <console.h>
> >>>>> +#include <efi_loader.h>
> >>>>>  #include <fdtdec.h>
> >>>>>  #include <malloc.h>
> >>>>>  
> >>>>> @@ -125,6 +126,15 @@ 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")) {
> >>>>> +		argc--;
> >>>>> +		argv++;
> >>>>> +
> >>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> >>>>> +	}
> >>>>> +#endif
> >>>>> +
> >>>>>  	for (i = 1; i < argc; ++i) {
> >>>>>  		char *arg;
> >>>>>  
> >>>>> diff --git a/include/command.h b/include/command.h
> >>>>> index 200c7a5e9f4e..feddef300ccc 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)
> >>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>>>> +#endif
> >>>>>  
> >>>>>  /* common/command.c */
> >>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >>>>>
> >>>>
> >>>
> >>
> > 
> 

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

[Heinrich Schuchardt]

On 2/28/19 5:28 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 08:33:17PM +0100, Heinrich Schuchardt wrote:
>> On 2/27/19 7:47 AM, AKASHI Takahiro wrote:
>>> On Wed, Feb 27, 2019 at 07:31:06AM +0100, Heinrich Schuchardt wrote:
>>>> On 2/27/19 6:58 AM, AKASHI Takahiro wrote:
>>>>> On Tue, Feb 26, 2019 at 08:30:50PM +0100, Heinrich Schuchardt wrote:
>>>>>> On 1/15/19 3:54 AM, 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
>>>>>>
>>>>>> You already introduced the support for BootNext. So is there a real benefit?
>>>>>
>>>>> This is a convenient way of running EFI application directly,
>>>>> but I already removed this feature from the next version.
>>>>
>>>> Please, remove 'run -e' instead because it cannot specify the device
>>>> tree needed for booting ARM boards.
>>>
>>> See my comment for patch#5 first.
>>>
>>>>>
>>>>>>>
>>>>>>> Please note that BootXXXX need not be included in "BootOrder".
>>>>>>>
>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>>>> ---
>>>>>>>  cmd/bootefi.c | 39 ++++++++++++++++++++++++++++-----------
>>>>>>>  1 file changed, 28 insertions(+), 11 deletions(-)
>>>>>>>
>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>>>> index 3be01b49b589..241fd0f987ab 100644
>>>>>>> --- a/cmd/bootefi.c
>>>>>>> +++ b/cmd/bootefi.c
>>>>>>> @@ -471,16 +471,15 @@ 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(EFI_BOOTMGR_DEFAULT_ORDER,
>>>>>>> -				&device_path, &file_path);
>>>>>>> +	addr = efi_bootmgr_load(boot_id, &device_path, &file_path);
>>>>>>>  	if (!addr)
>>>>>>> -		return 1;
>>>>>>> +		return CMD_RET_FAILURE;
>>>>>>>  
>>>>>>>  	printf("## Starting EFI application at %p ...\n", addr);
>>>>>>>  	r = do_bootefi_exec(addr, device_path, file_path);
>>>>>>> @@ -488,9 +487,9 @@ static int do_bootefi_bootmgr_exec(void)
>>>>>>>  	       r & ~EFI_ERROR_MASK);
>>>>>>>  
>>>>>>>  	if (r != EFI_SUCCESS)
>>>>>>> -		return 1;
>>>>>>> +		return CMD_RET_FAILURE;
>>>>>>>  
>>>>>>> -	return 0;
>>>>>>> +	return CMD_RET_SUCCESS;
>>>>>>>  }
>>>>>>>  
>>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>>>>> @@ -546,10 +545,28 @@ 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 *fdtstr, *endp;
>>>>>>> +		int boot_id = EFI_BOOTMGR_DEFAULT_ORDER;
>>>>>>> +
>>>>>>> +		if (argc > 2) {
>>>>>>> +			fdtstr = argv[2];
>>>>>>> +			 /* Special address "-" means no device tree */
>>>>>>> +			if (fdtstr[0] == '-')
>>>>>>> +				fdtstr = NULL;
>>>>>>> +
>>>>>>> +			r = efi_handle_fdt(fdtstr);
>>>>>>> +			if (r)
>>>>>>> +				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);
>>>>>>
>>>>>> Why not communicate via the BootNext variable?
>>>>>
>>>>> I don't get your point.
>>>>> BootNext and BootOrder are both defined by UEFI specification.
>>>>
>>>> Instead of changing the interface of do_bootefi_bootmgr_exec()
>>>
>>> Who care changing an *internal* function?
> 
> So do you agree?

What is wrong about calling efi_set_variable(L"BootNext", ) instead?
Wouldn't that result in less code?

> 
>>>
>>>> you could
>>>> simply set BootNext. Then the boot manager would pick up the option from
>>>> the variable and finally delete the variable. This would result in less
>>>> code.
>>>
>>> No. Even with "run -e," BootNext will disappear after execution.
>>> This is a requirement by UEFI spec.
>>
>> Shouldn't BootNext always be reset when executing bootefi no matter
>> whether the boot manager is used or not?
> 
> Didn't I say the same thing?
> Or do you expect that BootNext remain after "run -e"?

As described in the response to patch 5/5 'run -e' is not usable with
ARM given the current state of device trees in Linux.

Patch 1/5 only deletes variable BootNext if the boot manager is called.
Is this really correct? Shouldn't we delete the BootNext variable when
executing `bootefi $kernel_addr_r $fdt_addr_r`, too?

Best regards

Heinrich

> 
> -Takahiro Akashi
> 
>> Regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>>>>  	} else {
>>>>>>>  		saddr = argv[1];
>>>>>>>  
>>>>>>> @@ -590,7 +607,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"
>>>>>>> @@ -598,7 +615,7 @@ static char bootefi_help_text[] =
>>>>>>>  #endif
>>>>>>>  
>>>>>>>  U_BOOT_CMD(
>>>>>>> -	bootefi, 3, 0, do_bootefi,
>>>>>>> +	bootefi, 5, 0, do_bootefi,
>>>>>>
>>>>>> Why 5?
>>>>>
>>>>> For additional/optional '-' and <boot id>.
>>>>> But I removed this feature from bootefi.
>>>>>
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>>  	"Boots an EFI payload from memory",
>>>>>>>  	bootefi_help_text
>>>>>>>  );
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Heinrich Schuchardt]

On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
>> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
>>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
>>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
>>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
>>>>>> On 1/15/19 3:54 AM, 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."
>>>>>>>
>>>>>>
>>>>>> If we cannot specify the device tree what would be the use of this for
>>>>>> ARM processors?
>>>>>
>>>>> I don't get your point. What's the matter with device tree?
>>>>
>>>> To boot an ARM board on Linux or BSD you need a device tree.
>>>
>>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
>>> he suggested that we should not allow users to specify fdt at a command line.
>>> I believe that it would apply to my case above.
>>>
>>> IMO, we should always provide system's fdt to EFI applications.
>>
>> With current Linux development practice this unfortunately does not
>> work. Linux device trees sometimes see incompatible changes between
>> versions. Booting may fail with a device tree that is either too old or
>> too new for your Linux version.
>>
>> E.g. for the Odroid C2 some reserved memory regions were removed from
>> the device tree and replaced by a logic that determines them on the fly
>> due to changes in ARM trusted firmware location.
>>
>> The Wandboard rev B1 device tree was moved to a new file when a new
>> board revision appeared and the new revision changed the old file (sic).
>>
>> U-Boot is also not perfect at keeping its device tree in sync with the
>> newest Linux device tree.
> 
> Why don't you use "fdt" command in that case?
> IMO, we don't need <fdt> argument at bootefi (and run -e).
> Obviously, I have one assumption that we need change the code
> to utilize "fdtaddr" variable in do_bootefi().

Such a change would mean that after an upgrade of U-Boot all boards
running on Suse and Fedora suddenly will not boot again.

We should not change existing commands in an incompatible way.

> 
> Under the current implementation, a similar behavior is achieved
> only via distro_bootcmd. IMO, this should be corrected.
> If you agree, I will add another patch to my current patchset
> for this purpose.

I suggest to drop patch 5/5 from your series.

Best regards

Heinrich

> 
>>>
>>>> So run -e Boot0001 will not allow you to boot into Linux because it does
>>>> not specify a device tree.
>>>>
>>>>>
>>>>>
>>>>>> Why do you add the option to run and not to bootefi?
>>>>>>
>>>>>> You already introduced the capability to set BootNext. Why isn't that
>>>>>> enough?
>>>>>
>>>>> Simple.
>>>>>
>>>>> => run -e Boot00>
>>>>> versus
>>>>>
>>>>> => efidebug boot next 1
>>>>> => bootefi bootmgr
>>>>
>>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
>>>>
>>>> So there is no need to go through efidebug.
>>>>
>>>> I think we should avoid alternative commands.
>>>
>>> As I said, I already removed this feature from bootefi.
>>>
>>>>>
>>>>> First of all, efidebug is only recognized as a *debugging* tool.
>>>>> I believe that the former syntax is more intuitive, and it looks
>>>>> a natural extension to "run" command semantics akin to "env -e".
>>>>>
>>>>> As a result, we don't have to know about bootefi for normal operations.
>>>>
>>>> But you have to know about 'run' which you might not need otherwise.
>>>
>>> "run" is much better known to U-Boot users than bootefi.
>>
>> Do you have a statistic ;)
>>
>> Up to now booting always required a command starting on boot...
> 
> What I meant is that people need not learn more commands.
> 
> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> # bootefi and making it as a single command. In that case,
> # bootefi does exist only for hello and selftest.
> 
> -Takahiro Akashi
> 
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> -Takahiro Akashi
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>>>> ---
>>>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
>>>>>>>  cmd/nvedit.c      |  9 ++++++++-
>>>>>>>  common/cli.c      | 10 ++++++++++
>>>>>>>  include/command.h |  3 +++
>>>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
>>>>>>> --- a/cmd/bootefi.c
>>>>>>> +++ b/cmd/bootefi.c
>>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>>>>>>>  	return CMD_RET_SUCCESS;
>>>>>>>  }
>>>>>>>  
>>>>>>> +/* Called by "run" command */
>>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>>> +{
>>>>>>> +	int boot_id = -1;
>>>>>>> +	char *endp;
>>>>>>> +
>>>>>>> +	if (argc > 2)
>>>>>>> +		return CMD_RET_USAGE;
>>>>>>> +
>>>>>>> +	if (argc == 2) {
>>>>>>> +		if (!strcmp(argv[1], "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);
>>>>>>> +}
>>>>>>> +
>>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>>>  {
>>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
>>>>>>> --- a/cmd/nvedit.c
>>>>>>> +++ b/cmd/nvedit.c
>>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>>>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>>>>>  	"run commands in an environment variable",
>>>>>>>  	"var [...]\n"
>>>>>>> -	"    - run the commands in the environment variable(s) 'var'",
>>>>>>> +	"    - run the commands in the environment variable(s) 'var'"
>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>>>>>> +	"\n"
>>>>>>> +	"run -e [BootXXXX]\n"
>>>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>>>>>> +#else
>>>>>>> +	,
>>>>>>> +#endif
>>>>>>>  	var_complete
>>>>>>>  );
>>>>>>>  #endif
>>>>>>> diff --git a/common/cli.c b/common/cli.c
>>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
>>>>>>> --- a/common/cli.c
>>>>>>> +++ b/common/cli.c
>>>>>>> @@ -12,6 +12,7 @@
>>>>>>>  #include <cli.h>
>>>>>>>  #include <cli_hush.h>
>>>>>>>  #include <console.h>
>>>>>>> +#include <efi_loader.h>
>>>>>>>  #include <fdtdec.h>
>>>>>>>  #include <malloc.h>
>>>>>>>  
>>>>>>> @@ -125,6 +126,15 @@ 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")) {
>>>>>>> +		argc--;
>>>>>>> +		argv++;
>>>>>>> +
>>>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
>>>>>>> +	}
>>>>>>> +#endif
>>>>>>> +
>>>>>>>  	for (i = 1; i < argc; ++i) {
>>>>>>>  		char *arg;
>>>>>>>  
>>>>>>> diff --git a/include/command.h b/include/command.h
>>>>>>> index 200c7a5e9f4e..feddef300ccc 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)
>>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>>>>>>> +#endif
>>>>>>>  
>>>>>>>  /* common/command.c */
>>>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[AKASHI Takahiro]

On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
> On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> > On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
> >> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> >>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> >>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> >>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> >>>>>> On 1/15/19 3:54 AM, 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."
> >>>>>>>
> >>>>>>
> >>>>>> If we cannot specify the device tree what would be the use of this for
> >>>>>> ARM processors?
> >>>>>
> >>>>> I don't get your point. What's the matter with device tree?
> >>>>
> >>>> To boot an ARM board on Linux or BSD you need a device tree.
> >>>
> >>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> >>> he suggested that we should not allow users to specify fdt at a command line.
> >>> I believe that it would apply to my case above.
> >>>
> >>> IMO, we should always provide system's fdt to EFI applications.
> >>
> >> With current Linux development practice this unfortunately does not
> >> work. Linux device trees sometimes see incompatible changes between
> >> versions. Booting may fail with a device tree that is either too old or
> >> too new for your Linux version.
> >>
> >> E.g. for the Odroid C2 some reserved memory regions were removed from
> >> the device tree and replaced by a logic that determines them on the fly
> >> due to changes in ARM trusted firmware location.
> >>
> >> The Wandboard rev B1 device tree was moved to a new file when a new
> >> board revision appeared and the new revision changed the old file (sic).
> >>
> >> U-Boot is also not perfect at keeping its device tree in sync with the
> >> newest Linux device tree.
> > 
> > Why don't you use "fdt" command in that case?
> > IMO, we don't need <fdt> argument at bootefi (and run -e).
> > Obviously, I have one assumption that we need change the code
> > to utilize "fdtaddr" variable in do_bootefi().
> 
> Such a change would mean that after an upgrade of U-Boot all boards
> running on Suse and Fedora suddenly will not boot again.

Why do you think so?
Unless people intentionally run "fdt" command before bootefi,
the system will behave in the exact same way.

How many people really expect that, in the case below,
  => load ... <addr> <file>
  => fdt addr <addr>
  => bootefi bootmgr
bootefi will start EFI application *without* fdt?

-Takahiro Akashi

> We should not change existing commands in an incompatible way.
> 
> > 
> > Under the current implementation, a similar behavior is achieved
> > only via distro_bootcmd. IMO, this should be corrected.
> > If you agree, I will add another patch to my current patchset
> > for this purpose.
> 
> I suggest to drop patch 5/5 from your series.
> 
> Best regards
> 
> Heinrich
> 
> > 
> >>>
> >>>> So run -e Boot0001 will not allow you to boot into Linux because it does
> >>>> not specify a device tree.
> >>>>
> >>>>>
> >>>>>
> >>>>>> Why do you add the option to run and not to bootefi?
> >>>>>>
> >>>>>> You already introduced the capability to set BootNext. Why isn't that
> >>>>>> enough?
> >>>>>
> >>>>> Simple.
> >>>>>
> >>>>> => run -e Boot00>
> >>>>> versus
> >>>>>
> >>>>> => efidebug boot next 1
> >>>>> => bootefi bootmgr
> >>>>
> >>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> >>>>
> >>>> So there is no need to go through efidebug.
> >>>>
> >>>> I think we should avoid alternative commands.
> >>>
> >>> As I said, I already removed this feature from bootefi.
> >>>
> >>>>>
> >>>>> First of all, efidebug is only recognized as a *debugging* tool.
> >>>>> I believe that the former syntax is more intuitive, and it looks
> >>>>> a natural extension to "run" command semantics akin to "env -e".
> >>>>>
> >>>>> As a result, we don't have to know about bootefi for normal operations.
> >>>>
> >>>> But you have to know about 'run' which you might not need otherwise.
> >>>
> >>> "run" is much better known to U-Boot users than bootefi.
> >>
> >> Do you have a statistic ;)
> >>
> >> Up to now booting always required a command starting on boot...
> > 
> > What I meant is that people need not learn more commands.
> > 
> > # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> > # bootefi and making it as a single command. In that case,
> > # bootefi does exist only for hello and selftest.
> > 
> > -Takahiro Akashi
> > 
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Thanks,
> >>> -Takahiro Akashi
> >>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> -Takahiro Akashi
> >>>>>
> >>>>>
> >>>>>> Best regards
> >>>>>>
> >>>>>> Heinrich
> >>>>>>
> >>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>>> ---
> >>>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >>>>>>>  cmd/nvedit.c      |  9 ++++++++-
> >>>>>>>  common/cli.c      | 10 ++++++++++
> >>>>>>>  include/command.h |  3 +++
> >>>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
> >>>>>>> --- a/cmd/bootefi.c
> >>>>>>> +++ b/cmd/bootefi.c
> >>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >>>>>>>  	return CMD_RET_SUCCESS;
> >>>>>>>  }
> >>>>>>>  
> >>>>>>> +/* Called by "run" command */
> >>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>>>> +{
> >>>>>>> +	int boot_id = -1;
> >>>>>>> +	char *endp;
> >>>>>>> +
> >>>>>>> +	if (argc > 2)
> >>>>>>> +		return CMD_RET_USAGE;
> >>>>>>> +
> >>>>>>> +	if (argc == 2) {
> >>>>>>> +		if (!strcmp(argv[1], "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);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>>>>  {
> >>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
> >>>>>>> --- a/cmd/nvedit.c
> >>>>>>> +++ b/cmd/nvedit.c
> >>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >>>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>>>>>  	"run commands in an environment variable",
> >>>>>>>  	"var [...]\n"
> >>>>>>> -	"    - run the commands in the environment variable(s) 'var'",
> >>>>>>> +	"    - run the commands in the environment variable(s) 'var'"
> >>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>>>>>> +	"\n"
> >>>>>>> +	"run -e [BootXXXX]\n"
> >>>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>>>>>> +#else
> >>>>>>> +	,
> >>>>>>> +#endif
> >>>>>>>  	var_complete
> >>>>>>>  );
> >>>>>>>  #endif
> >>>>>>> diff --git a/common/cli.c b/common/cli.c
> >>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
> >>>>>>> --- a/common/cli.c
> >>>>>>> +++ b/common/cli.c
> >>>>>>> @@ -12,6 +12,7 @@
> >>>>>>>  #include <cli.h>
> >>>>>>>  #include <cli_hush.h>
> >>>>>>>  #include <console.h>
> >>>>>>> +#include <efi_loader.h>
> >>>>>>>  #include <fdtdec.h>
> >>>>>>>  #include <malloc.h>
> >>>>>>>  
> >>>>>>> @@ -125,6 +126,15 @@ 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")) {
> >>>>>>> +		argc--;
> >>>>>>> +		argv++;
> >>>>>>> +
> >>>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> >>>>>>> +	}
> >>>>>>> +#endif
> >>>>>>> +
> >>>>>>>  	for (i = 1; i < argc; ++i) {
> >>>>>>>  		char *arg;
> >>>>>>>  
> >>>>>>> diff --git a/include/command.h b/include/command.h
> >>>>>>> index 200c7a5e9f4e..feddef300ccc 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)
> >>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>>>>>> +#endif
> >>>>>>>  
> >>>>>>>  /* common/command.c */
> >>>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> > 
> 

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

[Heinrich Schuchardt]

On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
> On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
>> On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
>>> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
>>>> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
>>>>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
>>>>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
>>>>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
>>>>>>>> On 1/15/19 3:54 AM, 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."
>>>>>>>>>
>>>>>>>>
>>>>>>>> If we cannot specify the device tree what would be the use of this for
>>>>>>>> ARM processors?
>>>>>>>
>>>>>>> I don't get your point. What's the matter with device tree?
>>>>>>
>>>>>> To boot an ARM board on Linux or BSD you need a device tree.
>>>>>
>>>>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
>>>>> he suggested that we should not allow users to specify fdt at a command line.
>>>>> I believe that it would apply to my case above.
>>>>>
>>>>> IMO, we should always provide system's fdt to EFI applications.
>>>>
>>>> With current Linux development practice this unfortunately does not
>>>> work. Linux device trees sometimes see incompatible changes between
>>>> versions. Booting may fail with a device tree that is either too old or
>>>> too new for your Linux version.
>>>>
>>>> E.g. for the Odroid C2 some reserved memory regions were removed from
>>>> the device tree and replaced by a logic that determines them on the fly
>>>> due to changes in ARM trusted firmware location.
>>>>
>>>> The Wandboard rev B1 device tree was moved to a new file when a new
>>>> board revision appeared and the new revision changed the old file (sic).
>>>>
>>>> U-Boot is also not perfect at keeping its device tree in sync with the
>>>> newest Linux device tree.
>>>
>>> Why don't you use "fdt" command in that case?
>>> IMO, we don't need <fdt> argument at bootefi (and run -e).
>>> Obviously, I have one assumption that we need change the code
>>> to utilize "fdtaddr" variable in do_bootefi().
>>
>> Such a change would mean that after an upgrade of U-Boot all boards
>> running on Suse and Fedora suddenly will not boot again.
> 
> Why do you think so?
> Unless people intentionally run "fdt" command before bootefi,
> the system will behave in the exact same way.
> 
> How many people really expect that, in the case below,
>   => load ... <addr> <file>
>   => fdt addr <addr>
>   => bootefi bootmgr
> bootefi will start EFI application *without* fdt?
> 
> -Takahiro Akashi

Your previous mail sounded to me as if you wanted to drop the
possibility to specify an FDT address in the bootefi command. But maybe
I got you wrong.

If your idea is that we should use the address specified in command fdt
and $fdtcontroladdr as fallbacks if no FDT address is specified, that is
another story.

Best regards

Heinrich

> 
>> We should not change existing commands in an incompatible way.
>>
>>>
>>> Under the current implementation, a similar behavior is achieved
>>> only via distro_bootcmd. IMO, this should be corrected.
>>> If you agree, I will add another patch to my current patchset
>>> for this purpose.
>>
>> I suggest to drop patch 5/5 from your series.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>>
>>>>>> So run -e Boot0001 will not allow you to boot into Linux because it does
>>>>>> not specify a device tree.
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> Why do you add the option to run and not to bootefi?
>>>>>>>>
>>>>>>>> You already introduced the capability to set BootNext. Why isn't that
>>>>>>>> enough?
>>>>>>>
>>>>>>> Simple.
>>>>>>>
>>>>>>> => run -e Boot00>
>>>>>>> versus
>>>>>>>
>>>>>>> => efidebug boot next 1
>>>>>>> => bootefi bootmgr
>>>>>>
>>>>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
>>>>>>
>>>>>> So there is no need to go through efidebug.
>>>>>>
>>>>>> I think we should avoid alternative commands.
>>>>>
>>>>> As I said, I already removed this feature from bootefi.
>>>>>
>>>>>>>
>>>>>>> First of all, efidebug is only recognized as a *debugging* tool.
>>>>>>> I believe that the former syntax is more intuitive, and it looks
>>>>>>> a natural extension to "run" command semantics akin to "env -e".
>>>>>>>
>>>>>>> As a result, we don't have to know about bootefi for normal operations.
>>>>>>
>>>>>> But you have to know about 'run' which you might not need otherwise.
>>>>>
>>>>> "run" is much better known to U-Boot users than bootefi.
>>>>
>>>> Do you have a statistic ;)
>>>>
>>>> Up to now booting always required a command starting on boot...
>>>
>>> What I meant is that people need not learn more commands.
>>>
>>> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
>>> # bootefi and making it as a single command. In that case,
>>> # bootefi does exist only for hello and selftest.
>>>
>>> -Takahiro Akashi
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Thanks,
>>>>> -Takahiro Akashi
>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> -Takahiro Akashi
>>>>>>>
>>>>>>>
>>>>>>>> Best regards
>>>>>>>>
>>>>>>>> Heinrich
>>>>>>>>
>>>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>>>>>> ---
>>>>>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
>>>>>>>>>  cmd/nvedit.c      |  9 ++++++++-
>>>>>>>>>  common/cli.c      | 10 ++++++++++
>>>>>>>>>  include/command.h |  3 +++
>>>>>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
>>>>>>>>> --- a/cmd/bootefi.c
>>>>>>>>> +++ b/cmd/bootefi.c
>>>>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>>>>>>>>>  	return CMD_RET_SUCCESS;
>>>>>>>>>  }
>>>>>>>>>  
>>>>>>>>> +/* Called by "run" command */
>>>>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>>>>> +{
>>>>>>>>> +	int boot_id = -1;
>>>>>>>>> +	char *endp;
>>>>>>>>> +
>>>>>>>>> +	if (argc > 2)
>>>>>>>>> +		return CMD_RET_USAGE;
>>>>>>>>> +
>>>>>>>>> +	if (argc == 2) {
>>>>>>>>> +		if (!strcmp(argv[1], "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);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
>>>>>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>>>>>>>  {
>>>>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
>>>>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
>>>>>>>>> --- a/cmd/nvedit.c
>>>>>>>>> +++ b/cmd/nvedit.c
>>>>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>>>>>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>>>>>>>>>  	"run commands in an environment variable",
>>>>>>>>>  	"var [...]\n"
>>>>>>>>> -	"    - run the commands in the environment variable(s) 'var'",
>>>>>>>>> +	"    - run the commands in the environment variable(s) 'var'"
>>>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
>>>>>>>>> +	"\n"
>>>>>>>>> +	"run -e [BootXXXX]\n"
>>>>>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
>>>>>>>>> +#else
>>>>>>>>> +	,
>>>>>>>>> +#endif
>>>>>>>>>  	var_complete
>>>>>>>>>  );
>>>>>>>>>  #endif
>>>>>>>>> diff --git a/common/cli.c b/common/cli.c
>>>>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
>>>>>>>>> --- a/common/cli.c
>>>>>>>>> +++ b/common/cli.c
>>>>>>>>> @@ -12,6 +12,7 @@
>>>>>>>>>  #include <cli.h>
>>>>>>>>>  #include <cli_hush.h>
>>>>>>>>>  #include <console.h>
>>>>>>>>> +#include <efi_loader.h>
>>>>>>>>>  #include <fdtdec.h>
>>>>>>>>>  #include <malloc.h>
>>>>>>>>>  
>>>>>>>>> @@ -125,6 +126,15 @@ 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")) {
>>>>>>>>> +		argc--;
>>>>>>>>> +		argv++;
>>>>>>>>> +
>>>>>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
>>>>>>>>> +	}
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>>  	for (i = 1; i < argc; ++i) {
>>>>>>>>>  		char *arg;
>>>>>>>>>  
>>>>>>>>> diff --git a/include/command.h b/include/command.h
>>>>>>>>> index 200c7a5e9f4e..feddef300ccc 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)
>>>>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
>>>>>>>>> +#endif
>>>>>>>>>  
>>>>>>>>>  /* common/command.c */
>>>>>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

[Heinrich Schuchardt]

On 1/15/19 3:54 AM, 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     | 31 +++++++++++++++++++++++++++++++
>  cmd/nvedit.c      |  9 ++++++++-
>  common/cli.c      | 10 ++++++++++
>  include/command.h |  3 +++
>  4 files changed, 52 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 241fd0f987ab..ebe149dffa1f 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
>  	return CMD_RET_SUCCESS;
>  }
>  
> +/* Called by "run" command */
> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	int boot_id = -1;
> +	char *endp;
> +
> +	if (argc > 2)
> +		return CMD_RET_USAGE;
> +
> +	if (argc == 2) {
> +		if (!strcmp(argv[1], "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);

This function does not invoke:

        allow_unaligned();
        switch_to_non_secure_mode();

Probably these calls should be moved to efi_init_obj_list() anyway.

Best regards

Heinrich

> +}
> +
>  /* Interpreter command to boot an arbitrary EFI image from memory */
>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index de16c72c23f2..ce746bbf1b3e 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
>  	"run commands in an environment variable",
>  	"var [...]\n"
> -	"    - run the commands in the environment variable(s) 'var'",
> +	"    - run the commands in the environment variable(s) 'var'"
> +#if defined(CONFIG_CMD_BOOTEFI)
> +	"\n"
> +	"run -e [BootXXXX]\n"
> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> +#else
> +	,
> +#endif
>  	var_complete
>  );
>  #endif
> diff --git a/common/cli.c b/common/cli.c
> index 51b8d5f85cbb..fbb09d5049a4 100644
> --- a/common/cli.c
> +++ b/common/cli.c
> @@ -12,6 +12,7 @@
>  #include <cli.h>
>  #include <cli_hush.h>
>  #include <console.h>
> +#include <efi_loader.h>
>  #include <fdtdec.h>
>  #include <malloc.h>
>  
> @@ -125,6 +126,15 @@ 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")) {
> +		argc--;
> +		argv++;
> +
> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> +	}
> +#endif
> +
>  	for (i = 1; i < argc; ++i) {
>  		char *arg;
>  
> diff --git a/include/command.h b/include/command.h
> index 200c7a5e9f4e..feddef300ccc 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)
> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> +#endif
>  
>  /* common/command.c */
>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> 

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

[AKASHI Takahiro]

On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
> On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
> > On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
> >> On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> >>> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
> >>>> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> >>>>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> >>>>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> >>>>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> >>>>>>>> On 1/15/19 3:54 AM, 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."
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> If we cannot specify the device tree what would be the use of this for
> >>>>>>>> ARM processors?
> >>>>>>>
> >>>>>>> I don't get your point. What's the matter with device tree?
> >>>>>>
> >>>>>> To boot an ARM board on Linux or BSD you need a device tree.
> >>>>>
> >>>>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> >>>>> he suggested that we should not allow users to specify fdt at a command line.
> >>>>> I believe that it would apply to my case above.
> >>>>>
> >>>>> IMO, we should always provide system's fdt to EFI applications.
> >>>>
> >>>> With current Linux development practice this unfortunately does not
> >>>> work. Linux device trees sometimes see incompatible changes between
> >>>> versions. Booting may fail with a device tree that is either too old or
> >>>> too new for your Linux version.
> >>>>
> >>>> E.g. for the Odroid C2 some reserved memory regions were removed from
> >>>> the device tree and replaced by a logic that determines them on the fly
> >>>> due to changes in ARM trusted firmware location.
> >>>>
> >>>> The Wandboard rev B1 device tree was moved to a new file when a new
> >>>> board revision appeared and the new revision changed the old file (sic).
> >>>>
> >>>> U-Boot is also not perfect at keeping its device tree in sync with the
> >>>> newest Linux device tree.
> >>>
> >>> Why don't you use "fdt" command in that case?
> >>> IMO, we don't need <fdt> argument at bootefi (and run -e).
> >>> Obviously, I have one assumption that we need change the code
> >>> to utilize "fdtaddr" variable in do_bootefi().
> >>
> >> Such a change would mean that after an upgrade of U-Boot all boards
> >> running on Suse and Fedora suddenly will not boot again.
> > 
> > Why do you think so?
> > Unless people intentionally run "fdt" command before bootefi,
> > the system will behave in the exact same way.
> > 
> > How many people really expect that, in the case below,
> >   => load ... <addr> <file>
> >   => fdt addr <addr>
> >   => bootefi bootmgr
> > bootefi will start EFI application *without* fdt?
> > 
> > -Takahiro Akashi
> 
> Your previous mail sounded to me as if you wanted to drop the
> possibility to specify an FDT address in the bootefi command. But maybe
> I got you wrong.

No, even in v2, I didn't change any semantics in do_bootefi()
and hence bootefi command.

> If your idea is that we should use the address specified in command fdt
> and $fdtcontroladdr as fallbacks if no FDT address is specified, that is
> another story.

Exactly for "run -e" case (and probably a new command, efibootmgr, too).

The only concern is an incompatibility with distro_bootcmd.
It has yet another logic regarding fdt.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > 
> >> We should not change existing commands in an incompatible way.
> >>
> >>>
> >>> Under the current implementation, a similar behavior is achieved
> >>> only via distro_bootcmd. IMO, this should be corrected.
> >>> If you agree, I will add another patch to my current patchset
> >>> for this purpose.
> >>
> >> I suggest to drop patch 5/5 from your series.
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>>>
> >>>>>> So run -e Boot0001 will not allow you to boot into Linux because it does
> >>>>>> not specify a device tree.
> >>>>>>
> >>>>>>>
> >>>>>>>
> >>>>>>>> Why do you add the option to run and not to bootefi?
> >>>>>>>>
> >>>>>>>> You already introduced the capability to set BootNext. Why isn't that
> >>>>>>>> enough?
> >>>>>>>
> >>>>>>> Simple.
> >>>>>>>
> >>>>>>> => run -e Boot00>
> >>>>>>> versus
> >>>>>>>
> >>>>>>> => efidebug boot next 1
> >>>>>>> => bootefi bootmgr
> >>>>>>
> >>>>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> >>>>>>
> >>>>>> So there is no need to go through efidebug.
> >>>>>>
> >>>>>> I think we should avoid alternative commands.
> >>>>>
> >>>>> As I said, I already removed this feature from bootefi.
> >>>>>
> >>>>>>>
> >>>>>>> First of all, efidebug is only recognized as a *debugging* tool.
> >>>>>>> I believe that the former syntax is more intuitive, and it looks
> >>>>>>> a natural extension to "run" command semantics akin to "env -e".
> >>>>>>>
> >>>>>>> As a result, we don't have to know about bootefi for normal operations.
> >>>>>>
> >>>>>> But you have to know about 'run' which you might not need otherwise.
> >>>>>
> >>>>> "run" is much better known to U-Boot users than bootefi.
> >>>>
> >>>> Do you have a statistic ;)
> >>>>
> >>>> Up to now booting always required a command starting on boot...
> >>>
> >>> What I meant is that people need not learn more commands.
> >>>
> >>> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> >>> # bootefi and making it as a single command. In that case,
> >>> # bootefi does exist only for hello and selftest.
> >>>
> >>> -Takahiro Akashi
> >>>
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>> Thanks,
> >>>>> -Takahiro Akashi
> >>>>>
> >>>>>> Best regards
> >>>>>>
> >>>>>> Heinrich
> >>>>>>
> >>>>>>>
> >>>>>>> Thanks,
> >>>>>>> -Takahiro Akashi
> >>>>>>>
> >>>>>>>
> >>>>>>>> Best regards
> >>>>>>>>
> >>>>>>>> Heinrich
> >>>>>>>>
> >>>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>>>>> ---
> >>>>>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> >>>>>>>>>  cmd/nvedit.c      |  9 ++++++++-
> >>>>>>>>>  common/cli.c      | 10 ++++++++++
> >>>>>>>>>  include/command.h |  3 +++
> >>>>>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
> >>>>>>>>> --- a/cmd/bootefi.c
> >>>>>>>>> +++ b/cmd/bootefi.c
> >>>>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >>>>>>>>>  	return CMD_RET_SUCCESS;
> >>>>>>>>>  }
> >>>>>>>>>  
> >>>>>>>>> +/* Called by "run" command */
> >>>>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>>>>>> +{
> >>>>>>>>> +	int boot_id = -1;
> >>>>>>>>> +	char *endp;
> >>>>>>>>> +
> >>>>>>>>> +	if (argc > 2)
> >>>>>>>>> +		return CMD_RET_USAGE;
> >>>>>>>>> +
> >>>>>>>>> +	if (argc == 2) {
> >>>>>>>>> +		if (!strcmp(argv[1], "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);
> >>>>>>>>> +}
> >>>>>>>>> +
> >>>>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> >>>>>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>>>>>>>  {
> >>>>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> >>>>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
> >>>>>>>>> --- a/cmd/nvedit.c
> >>>>>>>>> +++ b/cmd/nvedit.c
> >>>>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >>>>>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >>>>>>>>>  	"run commands in an environment variable",
> >>>>>>>>>  	"var [...]\n"
> >>>>>>>>> -	"    - run the commands in the environment variable(s) 'var'",
> >>>>>>>>> +	"    - run the commands in the environment variable(s) 'var'"
> >>>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> >>>>>>>>> +	"\n"
> >>>>>>>>> +	"run -e [BootXXXX]\n"
> >>>>>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> >>>>>>>>> +#else
> >>>>>>>>> +	,
> >>>>>>>>> +#endif
> >>>>>>>>>  	var_complete
> >>>>>>>>>  );
> >>>>>>>>>  #endif
> >>>>>>>>> diff --git a/common/cli.c b/common/cli.c
> >>>>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
> >>>>>>>>> --- a/common/cli.c
> >>>>>>>>> +++ b/common/cli.c
> >>>>>>>>> @@ -12,6 +12,7 @@
> >>>>>>>>>  #include <cli.h>
> >>>>>>>>>  #include <cli_hush.h>
> >>>>>>>>>  #include <console.h>
> >>>>>>>>> +#include <efi_loader.h>
> >>>>>>>>>  #include <fdtdec.h>
> >>>>>>>>>  #include <malloc.h>
> >>>>>>>>>  
> >>>>>>>>> @@ -125,6 +126,15 @@ 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")) {
> >>>>>>>>> +		argc--;
> >>>>>>>>> +		argv++;
> >>>>>>>>> +
> >>>>>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> >>>>>>>>> +	}
> >>>>>>>>> +#endif
> >>>>>>>>> +
> >>>>>>>>>  	for (i = 1; i < argc; ++i) {
> >>>>>>>>>  		char *arg;
> >>>>>>>>>  
> >>>>>>>>> diff --git a/include/command.h b/include/command.h
> >>>>>>>>> index 200c7a5e9f4e..feddef300ccc 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)
> >>>>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> >>>>>>>>> +#endif
> >>>>>>>>>  
> >>>>>>>>>  /* common/command.c */
> >>>>>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> > 
> 

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

[AKASHI Takahiro]

On Thu, Feb 28, 2019 at 09:59:14PM +0100, Heinrich Schuchardt wrote:
> On 1/15/19 3:54 AM, 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     | 31 +++++++++++++++++++++++++++++++
> >  cmd/nvedit.c      |  9 ++++++++-
> >  common/cli.c      | 10 ++++++++++
> >  include/command.h |  3 +++
> >  4 files changed, 52 insertions(+), 1 deletion(-)
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 241fd0f987ab..ebe149dffa1f 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> >  	return CMD_RET_SUCCESS;
> >  }
> >  
> > +/* Called by "run" command */
> > +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > +{
> > +	int boot_id = -1;
> > +	char *endp;
> > +
> > +	if (argc > 2)
> > +		return CMD_RET_USAGE;
> > +
> > +	if (argc == 2) {
> > +		if (!strcmp(argv[1], "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);
> 
> This function does not invoke:
> 
>         allow_unaligned();
>         switch_to_non_secure_mode();

I've already fixed it in my pre-v3 patch.

> Probably these calls should be moved to efi_init_obj_list() anyway.

Can be, but the name no longer represents its function.

-Takahiro Akashi
>
> Best regards
> 
> Heinrich
> 
> > +}
> > +
> >  /* Interpreter command to boot an arbitrary EFI image from memory */
> >  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  {
> > diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > index de16c72c23f2..ce746bbf1b3e 100644
> > --- a/cmd/nvedit.c
> > +++ b/cmd/nvedit.c
> > @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> >  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> >  	"run commands in an environment variable",
> >  	"var [...]\n"
> > -	"    - run the commands in the environment variable(s) 'var'",
> > +	"    - run the commands in the environment variable(s) 'var'"
> > +#if defined(CONFIG_CMD_BOOTEFI)
> > +	"\n"
> > +	"run -e [BootXXXX]\n"
> > +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > +#else
> > +	,
> > +#endif
> >  	var_complete
> >  );
> >  #endif
> > diff --git a/common/cli.c b/common/cli.c
> > index 51b8d5f85cbb..fbb09d5049a4 100644
> > --- a/common/cli.c
> > +++ b/common/cli.c
> > @@ -12,6 +12,7 @@
> >  #include <cli.h>
> >  #include <cli_hush.h>
> >  #include <console.h>
> > +#include <efi_loader.h>
> >  #include <fdtdec.h>
> >  #include <malloc.h>
> >  
> > @@ -125,6 +126,15 @@ 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")) {
> > +		argc--;
> > +		argv++;
> > +
> > +		return do_bootefi_run(cmdtp, flag, argc, argv);
> > +	}
> > +#endif
> > +
> >  	for (i = 1; i < argc; ++i) {
> >  		char *arg;
> >  
> > diff --git a/include/command.h b/include/command.h
> > index 200c7a5e9f4e..feddef300ccc 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)
> > +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > +#endif
> >  
> >  /* common/command.c */
> >  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> > 
> 

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

[AKASHI Takahiro]

Heinrich,

On Fri, Mar 01, 2019 at 10:22:13AM +0900, AKASHI Takahiro wrote:
> On Thu, Feb 28, 2019 at 06:13:39AM +0100, Heinrich Schuchardt wrote:
> > On 2/28/19 6:06 AM, AKASHI Takahiro wrote:
> > > On Thu, Feb 28, 2019 at 05:53:17AM +0100, Heinrich Schuchardt wrote:
> > >> On 2/28/19 5:45 AM, AKASHI Takahiro wrote:
> > >>> On Wed, Feb 27, 2019 at 08:10:36AM +0100, Heinrich Schuchardt wrote:
> > >>>> On 2/27/19 7:37 AM, AKASHI Takahiro wrote:
> > >>>>> On Wed, Feb 27, 2019 at 07:25:52AM +0100, Heinrich Schuchardt wrote:
> > >>>>>> On 2/27/19 7:12 AM, AKASHI Takahiro wrote:
> > >>>>>>> On Tue, Feb 26, 2019 at 08:20:10PM +0100, Heinrich Schuchardt wrote:
> > >>>>>>>> On 1/15/19 3:54 AM, 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."
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>> If we cannot specify the device tree what would be the use of this for
> > >>>>>>>> ARM processors?
> > >>>>>>>
> > >>>>>>> I don't get your point. What's the matter with device tree?
> > >>>>>>
> > >>>>>> To boot an ARM board on Linux or BSD you need a device tree.
> > >>>>>
> > >>>>> When I discussed with Alex about Boot Manager (and distro_bootcmd?),
> > >>>>> he suggested that we should not allow users to specify fdt at a command line.
> > >>>>> I believe that it would apply to my case above.
> > >>>>>
> > >>>>> IMO, we should always provide system's fdt to EFI applications.
> > >>>>
> > >>>> With current Linux development practice this unfortunately does not
> > >>>> work. Linux device trees sometimes see incompatible changes between
> > >>>> versions. Booting may fail with a device tree that is either too old or
> > >>>> too new for your Linux version.
> > >>>>
> > >>>> E.g. for the Odroid C2 some reserved memory regions were removed from
> > >>>> the device tree and replaced by a logic that determines them on the fly
> > >>>> due to changes in ARM trusted firmware location.
> > >>>>
> > >>>> The Wandboard rev B1 device tree was moved to a new file when a new
> > >>>> board revision appeared and the new revision changed the old file (sic).
> > >>>>
> > >>>> U-Boot is also not perfect at keeping its device tree in sync with the
> > >>>> newest Linux device tree.
> > >>>
> > >>> Why don't you use "fdt" command in that case?
> > >>> IMO, we don't need <fdt> argument at bootefi (and run -e).
> > >>> Obviously, I have one assumption that we need change the code
> > >>> to utilize "fdtaddr" variable in do_bootefi().
> > >>
> > >> Such a change would mean that after an upgrade of U-Boot all boards
> > >> running on Suse and Fedora suddenly will not boot again.
> > > 
> > > Why do you think so?
> > > Unless people intentionally run "fdt" command before bootefi,
> > > the system will behave in the exact same way.
> > > 
> > > How many people really expect that, in the case below,
> > >   => load ... <addr> <file>
> > >   => fdt addr <addr>
> > >   => bootefi bootmgr
> > > bootefi will start EFI application *without* fdt?
> > > 
> > > -Takahiro Akashi
> > 
> > Your previous mail sounded to me as if you wanted to drop the
> > possibility to specify an FDT address in the bootefi command. But maybe
> > I got you wrong.
> 
> No, even in v2, I didn't change any semantics in do_bootefi()
> and hence bootefi command.
> 
> > If your idea is that we should use the address specified in command fdt
> > and $fdtcontroladdr as fallbacks if no FDT address is specified, that is
> > another story.
> 
> Exactly for "run -e" case (and probably a new command, efibootmgr, too).
> 
> The only concern is an incompatibility with distro_bootcmd.
> It has yet another logic regarding fdt.

I changed my mind a bit.
I dropped "run -e" patch, and posted "BootNext" patch on its own.
I'd like to focus more on re-working do_bootefi().
See my next RFC.

Thanks,
-Takahiro Akashi

> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > > 
> > >> We should not change existing commands in an incompatible way.
> > >>
> > >>>
> > >>> Under the current implementation, a similar behavior is achieved
> > >>> only via distro_bootcmd. IMO, this should be corrected.
> > >>> If you agree, I will add another patch to my current patchset
> > >>> for this purpose.
> > >>
> > >> I suggest to drop patch 5/5 from your series.
> > >>
> > >> Best regards
> > >>
> > >> Heinrich
> > >>
> > >>>
> > >>>>>
> > >>>>>> So run -e Boot0001 will not allow you to boot into Linux because it does
> > >>>>>> not specify a device tree.
> > >>>>>>
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> Why do you add the option to run and not to bootefi?
> > >>>>>>>>
> > >>>>>>>> You already introduced the capability to set BootNext. Why isn't that
> > >>>>>>>> enough?
> > >>>>>>>
> > >>>>>>> Simple.
> > >>>>>>>
> > >>>>>>> => run -e Boot00>
> > >>>>>>> versus
> > >>>>>>>
> > >>>>>>> => efidebug boot next 1
> > >>>>>>> => bootefi bootmgr
> > >>>>>>
> > >>>>>> In patch 4/5 you already introduced 'bootefi bootmgr $fdt_addr_r 0001'
> > >>>>>>
> > >>>>>> So there is no need to go through efidebug.
> > >>>>>>
> > >>>>>> I think we should avoid alternative commands.
> > >>>>>
> > >>>>> As I said, I already removed this feature from bootefi.
> > >>>>>
> > >>>>>>>
> > >>>>>>> First of all, efidebug is only recognized as a *debugging* tool.
> > >>>>>>> I believe that the former syntax is more intuitive, and it looks
> > >>>>>>> a natural extension to "run" command semantics akin to "env -e".
> > >>>>>>>
> > >>>>>>> As a result, we don't have to know about bootefi for normal operations.
> > >>>>>>
> > >>>>>> But you have to know about 'run' which you might not need otherwise.
> > >>>>>
> > >>>>> "run" is much better known to U-Boot users than bootefi.
> > >>>>
> > >>>> Do you have a statistic ;)
> > >>>>
> > >>>> Up to now booting always required a command starting on boot...
> > >>>
> > >>> What I meant is that people need not learn more commands.
> > >>>
> > >>> # Relating to secure boot, I'm now thinking of pulling bootmgr out of
> > >>> # bootefi and making it as a single command. In that case,
> > >>> # bootefi does exist only for hello and selftest.
> > >>>
> > >>> -Takahiro Akashi
> > >>>
> > >>>>
> > >>>> Best regards
> > >>>>
> > >>>> Heinrich
> > >>>>
> > >>>>>
> > >>>>> Thanks,
> > >>>>> -Takahiro Akashi
> > >>>>>
> > >>>>>> Best regards
> > >>>>>>
> > >>>>>> Heinrich
> > >>>>>>
> > >>>>>>>
> > >>>>>>> Thanks,
> > >>>>>>> -Takahiro Akashi
> > >>>>>>>
> > >>>>>>>
> > >>>>>>>> Best regards
> > >>>>>>>>
> > >>>>>>>> Heinrich
> > >>>>>>>>
> > >>>>>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >>>>>>>>> ---
> > >>>>>>>>>  cmd/bootefi.c     | 31 +++++++++++++++++++++++++++++++
> > >>>>>>>>>  cmd/nvedit.c      |  9 ++++++++-
> > >>>>>>>>>  common/cli.c      | 10 ++++++++++
> > >>>>>>>>>  include/command.h |  3 +++
> > >>>>>>>>>  4 files changed, 52 insertions(+), 1 deletion(-)
> > >>>>>>>>>
> > >>>>>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > >>>>>>>>> index 241fd0f987ab..ebe149dffa1f 100644
> > >>>>>>>>> --- a/cmd/bootefi.c
> > >>>>>>>>> +++ b/cmd/bootefi.c
> > >>>>>>>>> @@ -492,6 +492,37 @@ static int do_bootefi_bootmgr_exec(int boot_id)
> > >>>>>>>>>  	return CMD_RET_SUCCESS;
> > >>>>>>>>>  }
> > >>>>>>>>>  
> > >>>>>>>>> +/* Called by "run" command */
> > >>>>>>>>> +int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > >>>>>>>>> +{
> > >>>>>>>>> +	int boot_id = -1;
> > >>>>>>>>> +	char *endp;
> > >>>>>>>>> +
> > >>>>>>>>> +	if (argc > 2)
> > >>>>>>>>> +		return CMD_RET_USAGE;
> > >>>>>>>>> +
> > >>>>>>>>> +	if (argc == 2) {
> > >>>>>>>>> +		if (!strcmp(argv[1], "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);
> > >>>>>>>>> +}
> > >>>>>>>>> +
> > >>>>>>>>>  /* Interpreter command to boot an arbitrary EFI image from memory */
> > >>>>>>>>>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> > >>>>>>>>>  {
> > >>>>>>>>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > >>>>>>>>> index de16c72c23f2..ce746bbf1b3e 100644
> > >>>>>>>>> --- a/cmd/nvedit.c
> > >>>>>>>>> +++ b/cmd/nvedit.c
> > >>>>>>>>> @@ -1344,7 +1344,14 @@ U_BOOT_CMD_COMPLETE(
> > >>>>>>>>>  	run,	CONFIG_SYS_MAXARGS,	1,	do_run,
> > >>>>>>>>>  	"run commands in an environment variable",
> > >>>>>>>>>  	"var [...]\n"
> > >>>>>>>>> -	"    - run the commands in the environment variable(s) 'var'",
> > >>>>>>>>> +	"    - run the commands in the environment variable(s) 'var'"
> > >>>>>>>>> +#if defined(CONFIG_CMD_BOOTEFI)
> > >>>>>>>>> +	"\n"
> > >>>>>>>>> +	"run -e [BootXXXX]\n"
> > >>>>>>>>> +	"    - load and run UEFI app based on 'BootXXXX' UEFI variable",
> > >>>>>>>>> +#else
> > >>>>>>>>> +	,
> > >>>>>>>>> +#endif
> > >>>>>>>>>  	var_complete
> > >>>>>>>>>  );
> > >>>>>>>>>  #endif
> > >>>>>>>>> diff --git a/common/cli.c b/common/cli.c
> > >>>>>>>>> index 51b8d5f85cbb..fbb09d5049a4 100644
> > >>>>>>>>> --- a/common/cli.c
> > >>>>>>>>> +++ b/common/cli.c
> > >>>>>>>>> @@ -12,6 +12,7 @@
> > >>>>>>>>>  #include <cli.h>
> > >>>>>>>>>  #include <cli_hush.h>
> > >>>>>>>>>  #include <console.h>
> > >>>>>>>>> +#include <efi_loader.h>
> > >>>>>>>>>  #include <fdtdec.h>
> > >>>>>>>>>  #include <malloc.h>
> > >>>>>>>>>  
> > >>>>>>>>> @@ -125,6 +126,15 @@ 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")) {
> > >>>>>>>>> +		argc--;
> > >>>>>>>>> +		argv++;
> > >>>>>>>>> +
> > >>>>>>>>> +		return do_bootefi_run(cmdtp, flag, argc, argv);
> > >>>>>>>>> +	}
> > >>>>>>>>> +#endif
> > >>>>>>>>> +
> > >>>>>>>>>  	for (i = 1; i < argc; ++i) {
> > >>>>>>>>>  		char *arg;
> > >>>>>>>>>  
> > >>>>>>>>> diff --git a/include/command.h b/include/command.h
> > >>>>>>>>> index 200c7a5e9f4e..feddef300ccc 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)
> > >>>>>>>>> +extern int do_bootefi_run(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> > >>>>>>>>> +#endif
> > >>>>>>>>>  
> > >>>>>>>>>  /* common/command.c */
> > >>>>>>>>>  int _do_help (cmd_tbl_t *cmd_start, int cmd_items, cmd_tbl_t * cmdtp, int
> > >>>>>>>>>
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>
> > > 
> >