[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

There are several reasons that I want to rework/refactor bootefi command
as well as bootmgr:
* Some previous commits on bootefi.c have made the code complicated
  and a bit hard to understand.

* do_bootefi_exec() would better be implemented using load_image() along
  with start_image() to be aligned with UEFI interfaces.

* Contrary to the other part, efi_selftest part of the code is unusual
  in terms of loading/execution path in do_bootefi().

* When we will support "secure boot" in the future, EFI Boot Manager
  is expected to be invoked as a standalone command without any arguments
  to mitigate security surfaces.

In this patch set,
Patch#1 to #7 are preparatory patches for patch#8.
Patch#8 is a core part of reworking.
Patch#9 is for standalone boot manager.

# Please note that some patches, say patch#2 and #3, can be combined into one
# but I intentionally keep them separated to clarify my intentions of changes.

Issues:
* It would be better off to change the semantics of efi_dp_from_name().
no chance to free loaded_image_info->load_options. (see patch #8)

-Takahiro Akashi

Changes in v1 (Apr 19, 2019)
* Travis CI:
  https://travis-ci.org/t-akashi/u-boot/builds/521984187
* rebased on efi-2019-07
* delete already-merged patches
* rework set_load_options() (patch#1 and #8)
* fix compile/Travis CI errors (patch#2, #7 and #8)
* rename do_boot_efi() to do_bootefi_image() (patch#7)
* free file_path in do_bootefi_image() (patch#8)
* use EFI_PRINT instead of debug() (patch#8)
* remove improper warning messages (patch#8)

Changes in RFC v3 (Apr 16, 2019)
* rebased on v2019.04
* delete already-merged patches
* add patch#2 for exporting root node
* use correct/more appropriate return code (CMD_RET_xxx) (patch#3,5,7)
* remove a message at starting an image in bootefi command, instead
  adding a debug message in efi_start_image()
* use root node as a dummy parent handle when calling efi_start_image()
* remove efi_unload_image() in bootefi command

Changes in RFC v2 (Mar 27, 2019)
* rebased on v2019.04-rc4
* use load_image API in do_bootmgr_load()
* merge efi_install_fdt() and efi_process_fdt()
* add EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image (patch#1)
* lots of minor changes

AKASHI Takahiro (9):
  cmd: bootefi: rework set_load_options()
  cmd: bootefi: carve out fdt handling from do_bootefi()
  cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()
  cmd: bootefi: carve out efi_selftest code from do_bootefi()
  cmd: bootefi: move do_bootefi_bootmgr_exec() forward
  cmd: bootefi: carve out bootmgr code from do_bootefi()
  cmd: bootefi: carve out do_bootefi_image() from do_bootefi()
  efi_loader: rework bootmgr/bootefi using load_image API
  cmd: add efibootmgr command

 cmd/Kconfig                   |   8 +
 cmd/bootefi.c                 | 540 ++++++++++++++++++++++------------
 include/efi_loader.h          |   5 +-
 lib/efi_loader/efi_bootmgr.c  |  43 +--
 lib/efi_loader/efi_boottime.c |   2 +
 5 files changed, 383 insertions(+), 215 deletions(-)

-- 
2.20.1

[U-Boot] [PATCH 1/9] cmd: bootefi: rework set_load_options()

[AKASHI Takahiro]

set_load_options() can fail, so it should return error code to stop
invoking an image.
In addition, set_load_options() now takes a handle, instead of
loaded_image_info, to utilize efi_load_image() in a later patch.

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 15ee4af45667..d8ca4ed703ef 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -39,29 +39,49 @@ void __weak allow_unaligned(void)
 /*
  * Set the load options of an image from an environment variable.
  *
- * @loaded_image_info:	the image
- * @env_var:		name of the environment variable
+ * @handle:	the image handle
+ * @env_var:	name of the environment variable
+ * Return:	status code
  */
-static void set_load_options(struct efi_loaded_image *loaded_image_info,
-			     const char *env_var)
+static efi_status_t set_load_options(efi_handle_t handle, const char *env_var)
 {
+	struct efi_loaded_image *loaded_image_info;
 	size_t size;
 	const char *env = env_get(env_var);
 	u16 *pos;
+	efi_status_t ret;
+
+	ret = EFI_CALL(systab.boottime->open_protocol(
+					handle,
+					&efi_guid_loaded_image,
+					(void **)&loaded_image_info,
+					efi_root, NULL,
+					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
+	if (ret != EFI_SUCCESS)
+		return EFI_INVALID_PARAMETER;
 
 	loaded_image_info->load_options = NULL;
 	loaded_image_info->load_options_size = 0;
 	if (!env)
-		return;
+		goto out;
+
 	size = utf8_utf16_strlen(env) + 1;
 	loaded_image_info->load_options = calloc(size, sizeof(u16));
 	if (!loaded_image_info->load_options) {
 		printf("ERROR: Out of memory\n");
-		return;
+		EFI_CALL(systab.boottime->close_protocol(handle,
+							 &efi_guid_loaded_image,
+							 efi_root, NULL));
+		return EFI_OUT_OF_RESOURCES;
 	}
 	pos = loaded_image_info->load_options;
 	utf8_utf16_strcpy(&pos, env);
 	loaded_image_info->load_options_size = size * 2;
+
+out:
+	return EFI_CALL(systab.boottime->close_protocol(handle,
+							&efi_guid_loaded_image,
+							efi_root, NULL));
 }
 
 /**
@@ -212,9 +232,7 @@ static efi_status_t bootefi_run_prepare(const char *load_options_path,
 		return ret;
 
 	/* Transfer environment variable as load options */
-	set_load_options(*loaded_image_infop, load_options_path);
-
-	return 0;
+	return set_load_options((efi_handle_t)*image_objp, load_options_path);
 }
 
 /**
-- 
2.20.1

[U-Boot] [PATCH 2/9] cmd: bootefi: carve out fdt handling from do_bootefi()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.

Carve out a function to handle the installation of the device tree
as a configuration table in system table.

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index d8ca4ed703ef..fb6703ce84f3 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -218,6 +218,38 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
 	return ret;
 }
 
+/**
+ * efi_process_fdt() - process fdt passed by a command argument
+ * @fdt_opt:	pointer to argument
+ * Return:	status code
+ *
+ * If specified, fdt will be installed as configuration table,
+ * otherwise no fdt will be passed.
+ */
+static efi_status_t efi_process_fdt(const char *fdt_opt)
+{
+	unsigned long fdt_addr;
+	efi_status_t ret;
+
+	if (fdt_opt) {
+		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
+		if (!fdt_addr && *fdt_opt != '0')
+			return EFI_INVALID_PARAMETER;
+
+		/* Install device tree */
+		ret = efi_install_fdt(fdt_addr);
+		if (ret != EFI_SUCCESS) {
+			printf("ERROR: failed to install device tree\n");
+			return ret;
+		}
+	} else {
+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
+		efi_install_configuration_table(&efi_guid_fdt, NULL);
+	}
+
+	return EFI_SUCCESS;
+}
+
 static efi_status_t bootefi_run_prepare(const char *load_options_path,
 		struct efi_device_path *device_path,
 		struct efi_device_path *image_path,
@@ -407,7 +439,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();
@@ -425,21 +456,12 @@ 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");
-	}
+	r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
+	if (r == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (r != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-- 
2.20.1

[U-Boot] [PATCH 3/9] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.
For simplicity, merge two functions.

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index fb6703ce84f3..c6d6eb7312e8 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
 	}
 }
 
-static efi_status_t efi_install_fdt(ulong fdt_addr)
-{
-	bootm_headers_t img = { 0 };
-	efi_status_t ret;
-	void *fdt;
-
-	fdt = map_sysmem(fdt_addr, 0);
-	if (fdt_check_header(fdt)) {
-		printf("ERROR: invalid device tree\n");
-		return EFI_INVALID_PARAMETER;
-	}
-
-	/* Create memory reservation as indicated by the device tree */
-	efi_carve_out_dt_rsv(fdt);
-
-	/* Prepare fdt for payload */
-	ret = copy_fdt(&fdt);
-	if (ret)
-		return ret;
-
-	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
-		printf("ERROR: failed to process device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Link to it in the efi tables */
-	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
-	if (ret != EFI_SUCCESS)
-		return EFI_OUT_OF_RESOURCES;
-
-	return ret;
-}
-
 /**
- * efi_process_fdt() - process fdt passed by a command argument
+ * efi_install_fdt() - install fdt passed by a command argument
  * @fdt_opt:	pointer to argument
  * Return:	status code
  *
  * If specified, fdt will be installed as configuration table,
  * otherwise no fdt will be passed.
  */
-static efi_status_t efi_process_fdt(const char *fdt_opt)
+static efi_status_t efi_install_fdt(const char *fdt_opt)
 {
 	unsigned long fdt_addr;
+	void *fdt;
+	bootm_headers_t img = { 0 };
 	efi_status_t ret;
 
 	if (fdt_opt) {
+		/* Install device tree */
 		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
 		if (!fdt_addr && *fdt_opt != '0')
 			return EFI_INVALID_PARAMETER;
 
-		/* Install device tree */
-		ret = efi_install_fdt(fdt_addr);
+		fdt = map_sysmem(fdt_addr, 0);
+		if (fdt_check_header(fdt)) {
+			printf("ERROR: invalid device tree\n");
+			return EFI_INVALID_PARAMETER;
+		}
+
+		/* Create memory reservation as indicated by the device tree */
+		efi_carve_out_dt_rsv(fdt);
+
+		/* Prepare fdt for payload */
+		ret = copy_fdt(&fdt);
+		if (ret)
+			return ret;
+
+		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
+			printf("ERROR: failed to process device tree\n");
+			return EFI_LOAD_ERROR;
+		}
+
+		/* Link to it in the efi tables */
+		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
 		if (ret != EFI_SUCCESS) {
 			printf("ERROR: failed to install device tree\n");
-			return ret;
+			return EFI_OUT_OF_RESOURCES;
 		}
 	} else {
 		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
@@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
+	r = efi_install_fdt(argc > 2 ? argv[2] : NULL);
 	if (r == EFI_INVALID_PARAMETER)
 		return CMD_RET_USAGE;
 	else if (r != EFI_SUCCESS)
-- 
2.20.1

[U-Boot] [PATCH 4/9] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.

Efi_selftest code is unusual in terms of execution path in do_bootefi(),
which make that function complicated and hard to understand. With this
patch, all efi_selftest related code will be put in a separate function.

The change also includes expanding efi_run_prepare() and efi_run_finish()
in do_bootefi_exec().

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index c6d6eb7312e8..e73b4cb563cd 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -239,37 +239,6 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
 	return EFI_SUCCESS;
 }
 
-static efi_status_t bootefi_run_prepare(const char *load_options_path,
-		struct efi_device_path *device_path,
-		struct efi_device_path *image_path,
-		struct efi_loaded_image_obj **image_objp,
-		struct efi_loaded_image **loaded_image_infop)
-{
-	efi_status_t ret;
-
-	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
-				     loaded_image_infop);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
-	/* Transfer environment variable as load options */
-	return set_load_options((efi_handle_t)*image_objp, load_options_path);
-}
-
-/**
- * bootefi_run_finish() - finish up after running an EFI test
- *
- * @loaded_image_info: Pointer to a struct which holds the loaded image info
- * @image_objj: Pointer to a struct which holds the loaded image object
- */
-static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
-			       struct efi_loaded_image *loaded_image_info)
-{
-	efi_restore_gd();
-	free(loaded_image_info->load_options);
-	efi_delete_handle(&image_obj->header);
-}
-
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -316,11 +285,16 @@ static efi_status_t do_bootefi_exec(void *efi,
 		assert(device_path && image_path);
 	}
 
-	ret = bootefi_run_prepare("bootargs", device_path, image_path,
-				  &image_obj, &loaded_image_info);
+	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
+				     &loaded_image_info);
 	if (ret)
 		goto err_prepare;
 
+	/* Transfer environment variable as load options */
+	ret = set_load_options((efi_handle_t)image_obj, "bootargs");
+	if (ret != EFI_SUCCESS)
+		goto err_prepare;
+
 	/* Load the EFI payload */
 	ret = efi_load_pe(image_obj, efi, loaded_image_info);
 	if (ret != EFI_SUCCESS)
@@ -344,7 +318,9 @@ static efi_status_t do_bootefi_exec(void *efi,
 
 err_prepare:
 	/* image has returned, loaded-image obj goes *poof*: */
-	bootefi_run_finish(image_obj, loaded_image_info);
+	efi_restore_gd();
+	free(loaded_image_info->load_options);
+	efi_delete_handle(&image_obj->header);
 
 err_add_protocol:
 	if (mem_handle)
@@ -354,6 +330,23 @@ err_add_protocol:
 }
 
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+static efi_status_t bootefi_run_prepare(const char *load_options_path,
+		struct efi_device_path *device_path,
+		struct efi_device_path *image_path,
+		struct efi_loaded_image_obj **image_objp,
+		struct efi_loaded_image **loaded_image_infop)
+{
+	efi_status_t ret;
+
+	ret = efi_setup_loaded_image(device_path, image_path, image_objp,
+				     loaded_image_infop);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	/* Transfer environment variable as load options */
+	return set_load_options((efi_handle_t)*image_objp, load_options_path);
+}
+
 /**
  * bootefi_test_prepare() - prepare to run an EFI test
  *
@@ -399,6 +392,64 @@ failure:
 	return ret;
 }
 
+/**
+ * bootefi_run_finish() - finish up after running an EFI test
+ *
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ * @image_obj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
+			       struct efi_loaded_image *loaded_image_info)
+{
+	efi_restore_gd();
+	free(loaded_image_info->load_options);
+	efi_delete_handle(&image_obj->header);
+}
+
+/**
+ * do_efi_selftest() - execute EFI Selftest
+ *
+ * @fdt_opt:	string of fdt start address
+ * Return:	status code
+ *
+ * Execute EFI Selftest
+ */
+static int do_efi_selftest(const char *fdt_opt)
+{
+	struct efi_loaded_image_obj *image_obj;
+	struct efi_loaded_image *loaded_image_info;
+	efi_status_t ret;
+
+	/* Allow unaligned memory access */
+	allow_unaligned();
+
+	switch_to_non_secure_mode();
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       ret & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = efi_install_fdt(fdt_opt);
+	if (ret == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	ret = bootefi_test_prepare(&image_obj, &loaded_image_info,
+				   "\\selftest", "efi_selftest");
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	/* Execute the test */
+	ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
+	bootefi_run_finish(image_obj, loaded_image_info);
+
+	return ret != EFI_SUCCESS;
+}
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
 static int do_bootefi_bootmgr_exec(void)
@@ -429,6 +480,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	char *saddr;
 	efi_status_t r;
 
+	if (argc < 2)
+		return CMD_RET_USAGE;
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+	else if (!strcmp(argv[1], "selftest"))
+		return do_efi_selftest(argc > 2 ? argv[2] : NULL);
+#endif
+
 	/* Allow unaligned memory access */
 	allow_unaligned();
 
@@ -442,9 +500,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return CMD_RET_FAILURE;
 	}
 
-	if (argc < 2)
-		return CMD_RET_USAGE;
-
 	r = efi_install_fdt(argc > 2 ? argv[2] : NULL);
 	if (r == EFI_INVALID_PARAMETER)
 		return CMD_RET_USAGE;
@@ -462,22 +517,6 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 			addr = CONFIG_SYS_LOAD_ADDR;
 		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
 	} else
-#endif
-#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
-	if (!strcmp(argv[1], "selftest")) {
-		struct efi_loaded_image_obj *image_obj;
-		struct efi_loaded_image *loaded_image_info;
-
-		r = bootefi_test_prepare(&image_obj, &loaded_image_info,
-					 "\\selftest", "efi_selftest");
-		if (r != EFI_SUCCESS)
-			return CMD_RET_FAILURE;
-
-		/* Execute the test */
-		r = EFI_CALL(efi_selftest(&image_obj->header, &systab));
-		bootefi_run_finish(image_obj, loaded_image_info);
-		return r != EFI_SUCCESS;
-	} else
 #endif
 	if (!strcmp(argv[1], "bootmgr")) {
 		return do_bootefi_bootmgr_exec();
-- 
2.20.1

[U-Boot] [PATCH 5/9] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index e73b4cb563cd..d8ddd770e031 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -329,6 +329,27 @@ err_add_protocol:
 	return ret;
 }
 
+static int do_bootefi_bootmgr_exec(void)
+{
+	struct efi_device_path *device_path, *file_path;
+	void *addr;
+	efi_status_t r;
+
+	addr = efi_bootmgr_load(&device_path, &file_path);
+	if (!addr)
+		return 1;
+
+	printf("## Starting EFI application at %p ...\n", addr);
+	r = do_bootefi_exec(addr, device_path, file_path);
+	printf("## Application terminated, r = %lu\n",
+	       r & ~EFI_ERROR_MASK);
+
+	if (r != EFI_SUCCESS)
+		return 1;
+
+	return 0;
+}
+
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 static efi_status_t bootefi_run_prepare(const char *load_options_path,
 		struct efi_device_path *device_path,
@@ -452,27 +473,6 @@ static int do_efi_selftest(const char *fdt_opt)
 }
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
-static int do_bootefi_bootmgr_exec(void)
-{
-	struct efi_device_path *device_path, *file_path;
-	void *addr;
-	efi_status_t r;
-
-	addr = efi_bootmgr_load(&device_path, &file_path);
-	if (!addr)
-		return 1;
-
-	printf("## Starting EFI application at %p ...\n", addr);
-	r = do_bootefi_exec(addr, device_path, file_path);
-	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
-
-	if (r != EFI_SUCCESS)
-		return 1;
-
-	return 0;
-}
-
 /* 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[])
 {
-- 
2.20.1

[U-Boot] [PATCH 6/9] cmd: bootefi: carve out bootmgr code from do_bootefi()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.
do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary
code into this function.

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index d8ddd770e031..2822456d8128 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -329,22 +329,49 @@ err_add_protocol:
 	return ret;
 }
 
-static int do_bootefi_bootmgr_exec(void)
+/**
+ * do_efibootmgr() - execute EFI Boot Manager
+ *
+ * @fdt_opt:	string of fdt start address
+ * Return:	status code
+ *
+ * Execute EFI Boot Manager
+ */
+static int do_efibootmgr(const char *fdt_opt)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
-	efi_status_t r;
+	efi_status_t ret;
+
+	/* Allow unaligned memory access */
+	allow_unaligned();
+
+	switch_to_non_secure_mode();
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       ret & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = efi_install_fdt(fdt_opt);
+	if (ret == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
 
 	addr = efi_bootmgr_load(&device_path, &file_path);
 	if (!addr)
 		return 1;
 
 	printf("## Starting EFI application at %p ...\n", addr);
-	r = do_bootefi_exec(addr, device_path, file_path);
+	ret = do_bootefi_exec(addr, device_path, file_path);
 	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
+	       ret & ~EFI_ERROR_MASK);
 
-	if (r != EFI_SUCCESS)
+	if (ret != EFI_SUCCESS)
 		return 1;
 
 	return 0;
@@ -482,6 +509,9 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	if (argc < 2)
 		return CMD_RET_USAGE;
+
+	if (!strcmp(argv[1], "bootmgr"))
+		return do_efibootmgr(argc > 2 ? argv[2] : NULL);
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	else if (!strcmp(argv[1], "selftest"))
 		return do_efi_selftest(argc > 2 ? argv[2] : NULL);
@@ -518,9 +548,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
 	} else
 #endif
-	if (!strcmp(argv[1], "bootmgr")) {
-		return do_bootefi_bootmgr_exec();
-	} else {
+	{
 		saddr = argv[1];
 
 		addr = simple_strtoul(saddr, NULL, 16);
-- 
2.20.1

[U-Boot] [PATCH 7/9] cmd: bootefi: carve out do_bootefi_image() from do_bootefi()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in later patch.
All the non-boot-manager-based (that is, bootefi <addr>) code is put
into one function, do_bootefi_image().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/bootefi.c | 122 +++++++++++++++++++++++++++-----------------------
 1 file changed, 67 insertions(+), 55 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 2822456d8128..e2ca68c4dc12 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -377,6 +377,72 @@ static int do_efibootmgr(const char *fdt_opt)
 	return 0;
 }
 
+/*
+ * do_bootefi_image() - execute EFI binary from command line
+ *
+ * @image_opt:	string of image start address
+ * @fdt_opt:	string of fdt start address
+ * Return:	status code
+ *
+ * Set up memory image for the binary to be loaded, prepare
+ * device path and then call do_bootefi_exec() to execute it.
+ */
+static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
+{
+	unsigned long addr;
+	efi_status_t ret;
+
+	/* Allow unaligned memory access */
+	allow_unaligned();
+
+	switch_to_non_secure_mode();
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       ret & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = efi_install_fdt(fdt_opt);
+	if (ret == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	if (!strcmp(image_opt, "hello")) {
+		char *saddr;
+		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
+
+		saddr = env_get("loadaddr");
+		if (saddr)
+			addr = simple_strtoul(saddr, NULL, 16);
+		else
+			addr = CONFIG_SYS_LOAD_ADDR;
+		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
+	} else
+#endif
+	{
+		addr = simple_strtoul(image_opt, NULL, 16);
+		/* Check that a numeric value was passed */
+		if (!addr && *image_opt != '0')
+			return CMD_RET_USAGE;
+	}
+
+	printf("## Starting EFI application at %08lx ...\n", addr);
+	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
+			      bootefi_image_path);
+	printf("## Application terminated, r = %lu\n",
+	       ret & ~EFI_ERROR_MASK);
+
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+	else
+		return CMD_RET_SUCCESS;
+}
+
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 static efi_status_t bootefi_run_prepare(const char *load_options_path,
 		struct efi_device_path *device_path,
@@ -503,10 +569,6 @@ static int do_efi_selftest(const char *fdt_opt)
 /* 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[])
 {
-	unsigned long addr;
-	char *saddr;
-	efi_status_t r;
-
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
@@ -517,57 +579,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		return do_efi_selftest(argc > 2 ? argv[2] : NULL);
 #endif
 
-	/* Allow unaligned memory access */
-	allow_unaligned();
-
-	switch_to_non_secure_mode();
-
-	/* Initialize EFI drivers */
-	r = efi_init_obj_list();
-	if (r != EFI_SUCCESS) {
-		printf("Error: Cannot set up EFI drivers, r = %lu\n",
-		       r & ~EFI_ERROR_MASK);
-		return CMD_RET_FAILURE;
-	}
-
-	r = efi_install_fdt(argc > 2 ? argv[2] : NULL);
-	if (r == EFI_INVALID_PARAMETER)
-		return CMD_RET_USAGE;
-	else if (r != EFI_SUCCESS)
-		return CMD_RET_FAILURE;
-
-#ifdef CONFIG_CMD_BOOTEFI_HELLO
-	if (!strcmp(argv[1], "hello")) {
-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-
-		saddr = env_get("loadaddr");
-		if (saddr)
-			addr = simple_strtoul(saddr, NULL, 16);
-		else
-			addr = CONFIG_SYS_LOAD_ADDR;
-		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
-	} else
-#endif
-	{
-		saddr = argv[1];
-
-		addr = simple_strtoul(saddr, NULL, 16);
-		/* Check that a numeric value was passed */
-		if (!addr && *saddr != '0')
-			return CMD_RET_USAGE;
-
-	}
-
-	printf("## Starting EFI application at %08lx ...\n", addr);
-	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
-			    bootefi_image_path);
-	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
-
-	if (r != EFI_SUCCESS)
-		return 1;
-	else
-		return 0;
+	return do_bootefi_image(argv[1], argc > 2 ? argv[2] : NULL);
 }
 
 #ifdef CONFIG_SYS_LONGHELP
-- 
2.20.1

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[AKASHI Takahiro]

In the current implementation, bootefi command and EFI boot manager
don't use load_image API, instead, use more primitive and internal
functions. This will introduce duplicated code and potentially
unknown bugs as well as inconsistent behaviours.

With this patch, do_efibootmgr() and do_boot_efi() are completely
overhauled and re-implemented using load_image API.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
 include/efi_loader.h          |   5 +-
 lib/efi_loader/efi_bootmgr.c  |  43 ++++----
 lib/efi_loader/efi_boottime.c |   2 +
 4 files changed, 127 insertions(+), 110 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index e2ca68c4dc12..8c84fff590a7 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
 /**
  * do_bootefi_exec() - execute EFI binary
  *
- * @efi:		address of the binary
- * @device_path:	path of the device from which the binary was loaded
- * @image_path:		device path of the binary
+ * @handle:		handle of loaded image
  * Return:		status code
  *
  * Load the EFI binary into a newly assigned memory unwinding the relocation
  * information, install the loaded image protocol, and call the binary.
  */
-static efi_status_t do_bootefi_exec(void *efi,
-				    struct efi_device_path *device_path,
-				    struct efi_device_path *image_path)
+static efi_status_t do_bootefi_exec(efi_handle_t handle)
 {
-	efi_handle_t mem_handle = NULL;
-	struct efi_device_path *memdp = NULL;
 	efi_status_t ret;
-	struct efi_loaded_image_obj *image_obj = NULL;
-	struct efi_loaded_image *loaded_image_info = NULL;
-
-	/*
-	 * Special case for efi payload not loaded from disk, such as
-	 * 'bootefi hello' or for example payload loaded directly into
-	 * memory via JTAG, etc:
-	 */
-	if (!device_path && !image_path) {
-		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
-		/* actual addresses filled in after efi_load_pe() */
-		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
-		device_path = image_path = memdp;
-		/*
-		 * Grub expects that the device path of the loaded image is
-		 * installed on a handle.
-		 */
-		ret = efi_create_handle(&mem_handle);
-		if (ret != EFI_SUCCESS)
-			return ret; /* TODO: leaks device_path */
-		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
-				       device_path);
-		if (ret != EFI_SUCCESS)
-			goto err_add_protocol;
-	} else {
-		assert(device_path && image_path);
-	}
-
-	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
-				     &loaded_image_info);
-	if (ret)
-		goto err_prepare;
 
 	/* Transfer environment variable as load options */
-	ret = set_load_options((efi_handle_t)image_obj, "bootargs");
-	if (ret != EFI_SUCCESS)
-		goto err_prepare;
-
-	/* Load the EFI payload */
-	ret = efi_load_pe(image_obj, efi, loaded_image_info);
+	ret = set_load_options(handle, "bootargs");
 	if (ret != EFI_SUCCESS)
-		goto err_prepare;
-
-	if (memdp) {
-		struct efi_device_path_memory *mdp = (void *)memdp;
-		mdp->memory_type = loaded_image_info->image_code_type;
-		mdp->start_address = (uintptr_t)loaded_image_info->image_base;
-		mdp->end_address = mdp->start_address +
-				loaded_image_info->image_size;
-	}
+		return ret;
 
 	/* we don't support much: */
 	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
 		"{ro,boot}(blob)0000000000000000");
 
 	/* Call our payload! */
-	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
-	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
+	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
 
-err_prepare:
-	/* image has returned, loaded-image obj goes *poof*: */
 	efi_restore_gd();
-	free(loaded_image_info->load_options);
-	efi_delete_handle(&image_obj->header);
 
-err_add_protocol:
-	if (mem_handle)
-		efi_delete_handle(mem_handle);
+	/*
+	 * FIXME: Who is responsible for
+	 *	free(loaded_image_info->load_options);
+	 * Once efi_exit() is implemented correctly,
+	 * handle itself doesn't exist here.
+	 */
 
 	return ret;
 }
@@ -339,8 +286,7 @@ err_add_protocol:
  */
 static int do_efibootmgr(const char *fdt_opt)
 {
-	struct efi_device_path *device_path, *file_path;
-	void *addr;
+	efi_handle_t handle;
 	efi_status_t ret;
 
 	/* Allow unaligned memory access */
@@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
 	else if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
-	if (!addr)
-		return 1;
+	ret = efi_bootmgr_load(&handle);
+	if (ret != EFI_SUCCESS) {
+		printf("EFI boot manager: Cannot load any image\n");
+		return CMD_RET_FAILURE;
+	}
 
-	printf("## Starting EFI application at %p ...\n", addr);
-	ret = do_bootefi_exec(addr, device_path, file_path);
-	printf("## Application terminated, r = %lu\n",
-	       ret & ~EFI_ERROR_MASK);
+	ret = do_bootefi_exec(handle);
+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
 
 	if (ret != EFI_SUCCESS)
-		return 1;
+		return CMD_RET_FAILURE;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 /*
@@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
  */
 static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
 {
-	unsigned long addr;
+	void *image_buf;
+	struct efi_device_path *device_path, *image_path;
+	struct efi_device_path *memdp = NULL, *file_path = NULL;
+	unsigned long addr, size;
+	const char *size_str;
+	efi_handle_t mem_handle = NULL, handle;
 	efi_status_t ret;
 
 	/* Allow unaligned memory access */
@@ -414,33 +365,90 @@ static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(image_opt, "hello")) {
 		char *saddr;
-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
 
 		saddr = env_get("loadaddr");
+		size = __efi_helloworld_end - __efi_helloworld_begin;
+
 		if (saddr)
 			addr = simple_strtoul(saddr, NULL, 16);
 		else
 			addr = CONFIG_SYS_LOAD_ADDR;
-		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
+
+		image_buf = map_sysmem(addr, size);
+		memcpy(image_buf, __efi_helloworld_begin, size);
+
+		device_path = NULL;
+		image_path = NULL;
 	} else
 #endif
 	{
+		size_str = env_get("filesize");
+		if (size_str)
+			size = simple_strtoul(size_str, NULL, 16);
+		else
+			size = 0;
+
 		addr = simple_strtoul(image_opt, NULL, 16);
 		/* Check that a numeric value was passed */
 		if (!addr && *image_opt != '0')
 			return CMD_RET_USAGE;
+
+		image_buf = map_sysmem(addr, size);
+
+		device_path = bootefi_device_path;
+		image_path = bootefi_image_path;
 	}
 
-	printf("## Starting EFI application at %08lx ...\n", addr);
-	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
-			      bootefi_image_path);
-	printf("## Application terminated, r = %lu\n",
-	       ret & ~EFI_ERROR_MASK);
+	if (!device_path && !image_path) {
+		/*
+		 * Special case for efi payload not loaded from disk,
+		 * such as 'bootefi hello' or for example payload
+		 * loaded directly into memory via JTAG, etc:
+		 */
+		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					(uintptr_t)image_buf, size);
+		file_path = memdp; /* append(memdp, NULL) */
+
+		/*
+		 * Make sure that device for device_path exist
+		 * in load_image(). Otherwise, shell and grub will fail.
+		 */
+		ret = efi_create_handle(&mem_handle);
+		if (ret != EFI_SUCCESS)
+			/* TODO: leaks device_path */
+			goto err_add_protocol;
+
+		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
+				       memdp);
+		if (ret != EFI_SUCCESS)
+			goto err_add_protocol;
+	} else {
+		assert(device_path && image_path);
+		file_path = efi_dp_append(device_path, image_path);
+	}
+
+	ret = EFI_CALL(efi_load_image(false, efi_root,
+				      file_path, image_buf, size, &handle));
+	if (ret != EFI_SUCCESS)
+		goto err_prepare;
+
+	ret = do_bootefi_exec(handle);
+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
+
+err_prepare:
+	if (device_path)
+		efi_free_pool(file_path);
+
+err_add_protocol:
+	if (mem_handle)
+		efi_delete_handle(mem_handle);
+	if (file_path) /* hence memdp */
+		efi_free_pool(file_path);
 
 	if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
-	else
-		return CMD_RET_SUCCESS;
+
+	return CMD_RET_SUCCESS;
 }
 
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
@@ -598,7 +606,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 address]\n"
 	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
 	"\n"
 	"    If specified, the device tree located at <fdt address> gets\n"
@@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
 	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
 	if (ret == EFI_SUCCESS) {
 		bootefi_device_path = device;
+		if (image) {
+			/* FIXME: image should not contain device */
+			struct efi_device_path *image_tmp = image;
+
+			efi_dp_split_file_path(image, &device, &image);
+			efi_free_pool(image_tmp);
+		}
 		bootefi_image_path = image;
 	} else {
 		bootefi_device_path = NULL;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 93f7672aecbc..39ed8a6fa592 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 				    struct efi_device_path *file_path,
 				    struct efi_loaded_image_obj **handle_ptr,
 				    struct efi_loaded_image **info_ptr);
-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
-				      void **buffer, efi_uintn_t *size);
 /* Print information about all loaded images */
 void efi_print_image_infos(void *pc);
 
@@ -567,8 +565,7 @@ struct efi_load_option {
 
 void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
 unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
-void *efi_bootmgr_load(struct efi_device_path **device_path,
-		       struct efi_device_path **file_path);
+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
 
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4fccadc5483d..13ec79b2098b 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
  * if successful.  This checks that the EFI_LOAD_OPTION is active (enabled)
  * and that the specified file to boot exists.
  */
-static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
-			    struct efi_device_path **file_path)
+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
 {
 	struct efi_load_option lo;
 	u16 varname[] = L"Boot0000";
 	u16 hexmap[] = L"0123456789ABCDEF";
-	void *load_option, *image = NULL;
+	void *load_option;
 	efi_uintn_t size;
+	efi_status_t ret;
 
 	varname[4] = hexmap[(n & 0xf000) >> 12];
 	varname[5] = hexmap[(n & 0x0f00) >> 8];
@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 
 	load_option = get_var(varname, &efi_global_variable_guid, &size);
 	if (!load_option)
-		return NULL;
+		return EFI_LOAD_ERROR;
 
 	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);
 
-		ret = efi_load_image_from_path(lo.file_path, &image, &size);
-
+		ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
+					      lo.file_path, NULL, 0,
+					      handle));
 		if (ret != EFI_SUCCESS)
 			goto error;
 
@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 				L"BootCurrent",
 				(efi_guid_t *)&efi_global_variable_guid,
 				attributes, size, &n));
-		if (ret != EFI_SUCCESS)
+		if (ret != EFI_SUCCESS) {
+			if (EFI_CALL(efi_unload_image(*handle))
+			    != EFI_SUCCESS)
+				printf("Unloading image failed\n");
 			goto error;
+		}
 
 		printf("Booting: %ls\n", lo.label);
-		efi_dp_split_file_path(lo.file_path, device_path, file_path);
+	} else {
+		ret = EFI_LOAD_ERROR;
 	}
 
 error:
 	free(load_option);
 
-	return image;
+	return ret;
 }
 
 /*
@@ -177,12 +182,10 @@ error:
  * EFI variable, the available load-options, finding and returning
  * the first one that can be loaded successfully.
  */
-void *efi_bootmgr_load(struct efi_device_path **device_path,
-		       struct efi_device_path **file_path)
+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
 {
 	u16 bootnext, *bootorder;
 	efi_uintn_t size;
-	void *image = NULL;
 	int i, num;
 	efi_status_t ret;
 
@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 		/* load BootNext */
 		if (ret == EFI_SUCCESS) {
 			if (size == sizeof(u16)) {
-				image = try_load_entry(bootnext, device_path,
-						       file_path);
-				if (image)
-					return image;
+				ret = try_load_entry(bootnext, handle);
+				if (ret == EFI_SUCCESS)
+					return ret;
 			}
 		} else {
 			printf("Deleting BootNext failed\n");
@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
 	if (!bootorder) {
 		printf("BootOrder not defined\n");
+		ret = EFI_NOT_FOUND;
 		goto error;
 	}
 
 	num = size / sizeof(uint16_t);
 	for (i = 0; i < num; i++) {
 		debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
-		image = try_load_entry(bootorder[i], device_path, file_path);
-		if (image)
+		ret = try_load_entry(bootorder[i], handle);
+		if (ret == EFI_SUCCESS)
 			break;
 	}
 
 	free(bootorder);
 
 error:
-	return image;
+	return ret;
 }
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index abc295e392e9..19a58144cf4b 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1591,6 +1591,7 @@ failure:
  * @size:	size of the loaded image
  * Return:	status code
  */
+static
 efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
 				      void **buffer, efi_uintn_t *size)
 {
@@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	}
 
 	current_image = image_handle;
+	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
 	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
 
 	/*
-- 
2.20.1

[U-Boot] [PATCH 9/9] cmd: add efibootmgr command

[AKASHI Takahiro]

Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
With this patch, EFI boot manager can be kicked in by a standalone
command, efibootmgr.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/Kconfig   |  8 ++++++++
 cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 2bdbfcb3d091..46222be4a3c6 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -224,6 +224,14 @@ config CMD_BOOTEFI
 	help
 	  Boot an EFI image from memory.
 
+config CMD_STANDALONE_EFIBOOTMGR
+	bool "Enable EFI Boot Manager as a standalone command"
+	depends on CMD_BOOTEFI
+	default n
+	help
+          With this option enabled, EFI Boot Manager can be invoked
+	  as a standalone command.
+
 config CMD_BOOTEFI_HELLO_COMPILE
 	bool "Compile a standard EFI hello world binary for testing"
 	depends on CMD_BOOTEFI && !CPU_V7M && !SANDBOX
diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 8c84fff590a7..348c6fdbe28d 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -644,3 +644,38 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
 		bootefi_image_path = NULL;
 	}
 }
+
+#ifdef CONFIG_CMD_STANDALONE_EFIBOOTMGR
+static int do_efibootmgr_cmd(cmd_tbl_t *cmdtp, int flag, int argc,
+			     char * const argv[])
+{
+	char *fdt_opt;
+	int ret;
+
+	if (argc != 1)
+		return CMD_RET_USAGE;
+
+	fdt_opt = env_get("fdtaddr");
+	if (!fdt_opt)
+		fdt_opt = env_get("fdtcontroladdr");
+
+	ret = do_efibootmgr(fdt_opt);
+
+	free(fdt_opt);
+
+	return ret;
+}
+
+#ifdef CONFIG_SYS_LONGHELP
+static char efibootmgr_help_text[] =
+	"(no arguments)\n"
+	" - Launch EFI boot manager and execute EFI application\n"
+	"   based on BootNext and BootOrder variables\n";
+#endif
+
+U_BOOT_CMD(
+	efibootmgr, 1, 0, do_efibootmgr_cmd,
+	"Launch EFI boot manager",
+	efibootmgr_help_text
+);
+#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */
-- 
2.20.1

[U-Boot] [PATCH 1/9] cmd: bootefi: rework set_load_options()

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> set_load_options() can fail, so it should return error code to stop
> invoking an image.
> In addition, set_load_options() now takes a handle, instead of
> loaded_image_info, to utilize efi_load_image() in a later patch.
>
> Signed-off-by: AKASHI Takahiro<takahiro.akashi@linaro.org>

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

[U-Boot] [PATCH 2/9] cmd: bootefi: carve out fdt handling from do_bootefi()

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
>
> Carve out a function to handle the installation of the device tree
> as a configuration table in system table.
>
> Signed-off-by: AKASHI Takahiro<takahiro.akashi@linaro.org>

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

[U-Boot] [PATCH 3/9] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
> For simplicity, merge two functions.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------
>   1 file changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index fb6703ce84f3..c6d6eb7312e8 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
>   	}
>   }
>
> -static efi_status_t efi_install_fdt(ulong fdt_addr)
> -{
> -	bootm_headers_t img = { 0 };
> -	efi_status_t ret;
> -	void *fdt;
> -
> -	fdt = map_sysmem(fdt_addr, 0);
> -	if (fdt_check_header(fdt)) {
> -		printf("ERROR: invalid device tree\n");
> -		return EFI_INVALID_PARAMETER;
> -	}
> -
> -	/* Create memory reservation as indicated by the device tree */
> -	efi_carve_out_dt_rsv(fdt);
> -
> -	/* Prepare fdt for payload */
> -	ret = copy_fdt(&fdt);
> -	if (ret)
> -		return ret;
> -
> -	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> -		printf("ERROR: failed to process device tree\n");
> -		return EFI_LOAD_ERROR;
> -	}
> -
> -	/* Link to it in the efi tables */
> -	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> -	if (ret != EFI_SUCCESS)
> -		return EFI_OUT_OF_RESOURCES;
> -
> -	return ret;
> -}
> -
>   /**
> - * efi_process_fdt() - process fdt passed by a command argument
> + * efi_install_fdt() - install fdt passed by a command argument
>    * @fdt_opt:	pointer to argument
>    * Return:	status code
>    *
>    * If specified, fdt will be installed as configuration table,
>    * otherwise no fdt will be passed.
>    */
> -static efi_status_t efi_process_fdt(const char *fdt_opt)
> +static efi_status_t efi_install_fdt(const char *fdt_opt)
>   {
>   	unsigned long fdt_addr;
> +	void *fdt;
> +	bootm_headers_t img = { 0 };
>   	efi_status_t ret;
>
>   	if (fdt_opt) {
> +		/* Install device tree */
>   		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
>   		if (!fdt_addr && *fdt_opt != '0')
>   			return EFI_INVALID_PARAMETER;
>
> -		/* Install device tree */
> -		ret = efi_install_fdt(fdt_addr);
> +		fdt = map_sysmem(fdt_addr, 0);
> +		if (fdt_check_header(fdt)) {
> +			printf("ERROR: invalid device tree\n");
> +			return EFI_INVALID_PARAMETER;
> +		}
> +
> +		/* Create memory reservation as indicated by the device tree */
> +		efi_carve_out_dt_rsv(fdt);
> +
> +		/* Prepare fdt for payload */
> +		ret = copy_fdt(&fdt);
> +		if (ret)
> +			return ret;
> +
> +		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> +			printf("ERROR: failed to process device tree\n");
> +			return EFI_LOAD_ERROR;
> +		}
> +
> +		/* Link to it in the efi tables */
> +		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>   		if (ret != EFI_SUCCESS) {
>   			printf("ERROR: failed to install device tree\n");
> -			return ret;
> +			return EFI_OUT_OF_RESOURCES;

Why do you want to change the return value here? If there is not enough
space efi_install_configuration_table() will return EFI_OUT_OF_RESOURCES
by itself. Other possible error codes are:

* EFI_INVALID_PARAMETER if you do not provide a GUID. This cannot occur.
* EFI_NOT_FOUND if fdt is NULL.

I you are ok with it I would apply the patch with the original line
			return ret;

Otherwise

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

>   		}
>   	} else {
>   		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> @@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>   	if (argc < 2)
>   		return CMD_RET_USAGE;
>
> -	r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
> +	r = efi_install_fdt(argc > 2 ? argv[2] : NULL);
>   	if (r == EFI_INVALID_PARAMETER)
>   		return CMD_RET_USAGE;
>   	else if (r != EFI_SUCCESS)
>

[U-Boot] [PATCH 3/9] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[AKASHI Takahiro]

On Fri, Apr 19, 2019 at 06:06:08AM +0200, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >This is a preparatory patch for reworking do_bootefi() in later patch.
> >For simplicity, merge two functions.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------
> >  1 file changed, 28 insertions(+), 39 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index fb6703ce84f3..c6d6eb7312e8 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -185,62 +185,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
> >  	}
> >  }
> >
> >-static efi_status_t efi_install_fdt(ulong fdt_addr)
> >-{
> >-	bootm_headers_t img = { 0 };
> >-	efi_status_t ret;
> >-	void *fdt;
> >-
> >-	fdt = map_sysmem(fdt_addr, 0);
> >-	if (fdt_check_header(fdt)) {
> >-		printf("ERROR: invalid device tree\n");
> >-		return EFI_INVALID_PARAMETER;
> >-	}
> >-
> >-	/* Create memory reservation as indicated by the device tree */
> >-	efi_carve_out_dt_rsv(fdt);
> >-
> >-	/* Prepare fdt for payload */
> >-	ret = copy_fdt(&fdt);
> >-	if (ret)
> >-		return ret;
> >-
> >-	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> >-		printf("ERROR: failed to process device tree\n");
> >-		return EFI_LOAD_ERROR;
> >-	}
> >-
> >-	/* Link to it in the efi tables */
> >-	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> >-	if (ret != EFI_SUCCESS)
> >-		return EFI_OUT_OF_RESOURCES;
> >-
> >-	return ret;
> >-}
> >-
> >  /**
> >- * efi_process_fdt() - process fdt passed by a command argument
> >+ * efi_install_fdt() - install fdt passed by a command argument
> >   * @fdt_opt:	pointer to argument
> >   * Return:	status code
> >   *
> >   * If specified, fdt will be installed as configuration table,
> >   * otherwise no fdt will be passed.
> >   */
> >-static efi_status_t efi_process_fdt(const char *fdt_opt)
> >+static efi_status_t efi_install_fdt(const char *fdt_opt)
> >  {
> >  	unsigned long fdt_addr;
> >+	void *fdt;
> >+	bootm_headers_t img = { 0 };
> >  	efi_status_t ret;
> >
> >  	if (fdt_opt) {
> >+		/* Install device tree */
> >  		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
> >  		if (!fdt_addr && *fdt_opt != '0')
> >  			return EFI_INVALID_PARAMETER;
> >
> >-		/* Install device tree */
> >-		ret = efi_install_fdt(fdt_addr);
> >+		fdt = map_sysmem(fdt_addr, 0);
> >+		if (fdt_check_header(fdt)) {
> >+			printf("ERROR: invalid device tree\n");
> >+			return EFI_INVALID_PARAMETER;
> >+		}
> >+
> >+		/* Create memory reservation as indicated by the device tree */
> >+		efi_carve_out_dt_rsv(fdt);
> >+
> >+		/* Prepare fdt for payload */
> >+		ret = copy_fdt(&fdt);
> >+		if (ret)
> >+			return ret;
> >+
> >+		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> >+			printf("ERROR: failed to process device tree\n");
> >+			return EFI_LOAD_ERROR;
> >+		}
> >+
> >+		/* Link to it in the efi tables */
> >+		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> >  		if (ret != EFI_SUCCESS) {
> >  			printf("ERROR: failed to install device tree\n");
> >-			return ret;
> >+			return EFI_OUT_OF_RESOURCES;
> 
> Why do you want to change the return value here? If there is not enough
> space efi_install_configuration_table() will return EFI_OUT_OF_RESOURCES
> by itself. Other possible error codes are:

The original code returns EFI_LOAD_ERROR, so I changed it.

> * EFI_INVALID_PARAMETER if you do not provide a GUID. This cannot occur.
> * EFI_NOT_FOUND if fdt is NULL.
> 
> I you are ok with it I would apply the patch with the original line
> 			return ret;

Go ahead.
# You don't need additional patch, Just leave a note in commit message.

Thanks,
-Takahiro Akashi

> Otherwise
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> >  		}
> >  	} else {
> >  		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> >@@ -456,7 +445,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >
> >-	r = efi_process_fdt(argc > 2 ? argv[2] : NULL);
> >+	r = efi_install_fdt(argc > 2 ? argv[2] : NULL);
> >  	if (r == EFI_INVALID_PARAMETER)
> >  		return CMD_RET_USAGE;
> >  	else if (r != EFI_SUCCESS)
> >
> 

[U-Boot] [PATCH 4/9] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
>
> Efi_selftest code is unusual in terms of execution path in do_bootefi(),
> which make that function complicated and hard to understand. With this
> patch, all efi_selftest related code will be put in a separate function.
>
> The change also includes expanding efi_run_prepare() and efi_run_finish()
> in do_bootefi_exec().
>
> Signed-off-by: AKASHI Takahiro<takahiro.akashi@linaro.org>

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

[U-Boot] [PATCH 6/9] cmd: bootefi: carve out bootmgr code from do_bootefi()

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
> do_bootmgr_exec() is renamed to do_efibootmgr() as we put all the necessary
> code into this function.
>
> Signed-off-by: AKASHI Takahiro<takahiro.akashi@linaro.org>

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

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> +	/*
> +	 * FIXME: Who is responsible for
> +	 *	free(loaded_image_info->load_options);
> +	 * Once efi_exit() is implemented correctly,
> +	 * handle itself doesn't exist here.
> +	 */

Load option can only be freed when the image is unloaded. For
applications that will happen when the application call Exit(). For
drivers there is not guarantee that they will be ever unloaded.

In Unload() we should not free load options if they were not allocated
by us but by an EFI binary.

In your implementation you have allocated the load options via calloc()
and not from the EFI pool. So this may allow us to determine whether we
allocated the load options in the Unload() service.

Best regards

Heinrich

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> In the current implementation, bootefi command and EFI boot manager
> don't use load_image API, instead, use more primitive and internal
> functions. This will introduce duplicated code and potentially
> unknown bugs as well as inconsistent behaviours.
>
> With this patch, do_efibootmgr() and do_boot_efi() are completely
> overhauled and re-implemented using load_image API.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
>   include/efi_loader.h          |   5 +-
>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>   lib/efi_loader/efi_boottime.c |   2 +
>   4 files changed, 127 insertions(+), 110 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index e2ca68c4dc12..8c84fff590a7 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
>   /**
>    * do_bootefi_exec() - execute EFI binary
>    *
> - * @efi:		address of the binary
> - * @device_path:	path of the device from which the binary was loaded
> - * @image_path:		device path of the binary
> + * @handle:		handle of loaded image
>    * Return:		status code
>    *
>    * Load the EFI binary into a newly assigned memory unwinding the relocation
>    * information, install the loaded image protocol, and call the binary.
>    */
> -static efi_status_t do_bootefi_exec(void *efi,
> -				    struct efi_device_path *device_path,
> -				    struct efi_device_path *image_path)
> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
>   {
> -	efi_handle_t mem_handle = NULL;
> -	struct efi_device_path *memdp = NULL;
>   	efi_status_t ret;
> -	struct efi_loaded_image_obj *image_obj = NULL;
> -	struct efi_loaded_image *loaded_image_info = NULL;
> -
> -	/*
> -	 * Special case for efi payload not loaded from disk, such as
> -	 * 'bootefi hello' or for example payload loaded directly into
> -	 * memory via JTAG, etc:
> -	 */
> -	if (!device_path && !image_path) {
> -		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> -		/* actual addresses filled in after efi_load_pe() */
> -		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> -		device_path = image_path = memdp;
> -		/*
> -		 * Grub expects that the device path of the loaded image is
> -		 * installed on a handle.
> -		 */
> -		ret = efi_create_handle(&mem_handle);
> -		if (ret != EFI_SUCCESS)
> -			return ret; /* TODO: leaks device_path */
> -		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> -				       device_path);
> -		if (ret != EFI_SUCCESS)
> -			goto err_add_protocol;
> -	} else {
> -		assert(device_path && image_path);
> -	}
> -
> -	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> -				     &loaded_image_info);
> -	if (ret)
> -		goto err_prepare;
>
>   	/* Transfer environment variable as load options */
> -	ret = set_load_options((efi_handle_t)image_obj, "bootargs");
> -	if (ret != EFI_SUCCESS)
> -		goto err_prepare;
> -
> -	/* Load the EFI payload */
> -	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> +	ret = set_load_options(handle, "bootargs");
>   	if (ret != EFI_SUCCESS)
> -		goto err_prepare;
> -
> -	if (memdp) {
> -		struct efi_device_path_memory *mdp = (void *)memdp;
> -		mdp->memory_type = loaded_image_info->image_code_type;
> -		mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> -		mdp->end_address = mdp->start_address +
> -				loaded_image_info->image_size;
> -	}
> +		return ret;
>
>   	/* we don't support much: */
>   	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>   		"{ro,boot}(blob)0000000000000000");
>
>   	/* Call our payload! */
> -	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> -	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> +	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>
> -err_prepare:
> -	/* image has returned, loaded-image obj goes *poof*: */
>   	efi_restore_gd();
> -	free(loaded_image_info->load_options);
> -	efi_delete_handle(&image_obj->header);
>
> -err_add_protocol:
> -	if (mem_handle)
> -		efi_delete_handle(mem_handle);
> +	/*
> +	 * FIXME: Who is responsible for
> +	 *	free(loaded_image_info->load_options);
> +	 * Once efi_exit() is implemented correctly,
> +	 * handle itself doesn't exist here.
> +	 */
>
>   	return ret;
>   }
> @@ -339,8 +286,7 @@ err_add_protocol:
>    */
>   static int do_efibootmgr(const char *fdt_opt)
>   {
> -	struct efi_device_path *device_path, *file_path;
> -	void *addr;
> +	efi_handle_t handle;
>   	efi_status_t ret;
>
>   	/* Allow unaligned memory access */
> @@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
>   	else if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
>
> -	addr = efi_bootmgr_load(&device_path, &file_path);
> -	if (!addr)
> -		return 1;
> +	ret = efi_bootmgr_load(&handle);
> +	if (ret != EFI_SUCCESS) {
> +		printf("EFI boot manager: Cannot load any image\n");
> +		return CMD_RET_FAILURE;
> +	}
>
> -	printf("## Starting EFI application at %p ...\n", addr);
> -	ret = do_bootefi_exec(addr, device_path, file_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>
>   	if (ret != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;
>
> -	return 0;
> +	return CMD_RET_SUCCESS;
>   }
>
>   /*
> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
>    */
>   static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
>   {
> -	unsigned long addr;
> +	void *image_buf;
> +	struct efi_device_path *device_path, *image_path;
> +	struct efi_device_path *memdp = NULL, *file_path = NULL;
> +	unsigned long addr, size;
> +	const char *size_str;
> +	efi_handle_t mem_handle = NULL, handle;
>   	efi_status_t ret;
>
>   	/* Allow unaligned memory access */
> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
>   	if (!strcmp(image_opt, "hello")) {
>   		char *saddr;
> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>
>   		saddr = env_get("loadaddr");
> +		size = __efi_helloworld_end - __efi_helloworld_begin;
> +
>   		if (saddr)
>   			addr = simple_strtoul(saddr, NULL, 16);
>   		else
>   			addr = CONFIG_SYS_LOAD_ADDR;
> -		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> +
> +		image_buf = map_sysmem(addr, size);
> +		memcpy(image_buf, __efi_helloworld_begin, size);
> +
> +		device_path = NULL;
> +		image_path = NULL;
>   	} else
>   #endif
>   	{
> +		size_str = env_get("filesize");
> +		if (size_str)
> +			size = simple_strtoul(size_str, NULL, 16);
> +		else
> +			size = 0;
> +
>   		addr = simple_strtoul(image_opt, NULL, 16);
>   		/* Check that a numeric value was passed */
>   		if (!addr && *image_opt != '0')
>   			return CMD_RET_USAGE;
> +
> +		image_buf = map_sysmem(addr, size);
> +
> +		device_path = bootefi_device_path;
> +		image_path = bootefi_image_path;
>   	}
>
> -	printf("## Starting EFI application at %08lx ...\n", addr);
> -	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> -			      bootefi_image_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	if (!device_path && !image_path) {
> +		/*
> +		 * Special case for efi payload not loaded from disk,
> +		 * such as 'bootefi hello' or for example payload
> +		 * loaded directly into memory via JTAG, etc:
> +		 */
> +		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					(uintptr_t)image_buf, size);
> +		file_path = memdp; /* append(memdp, NULL) */
> +
> +		/*
> +		 * Make sure that device for device_path exist
> +		 * in load_image(). Otherwise, shell and grub will fail.
> +		 */
> +		ret = efi_create_handle(&mem_handle);
> +		if (ret != EFI_SUCCESS)
> +			/* TODO: leaks device_path */
> +			goto err_add_protocol;
> +
> +		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> +				       memdp);
> +		if (ret != EFI_SUCCESS)
> +			goto err_add_protocol;
> +	} else {
> +		assert(device_path && image_path);
> +		file_path = efi_dp_append(device_path, image_path);
> +	}
> +
> +	ret = EFI_CALL(efi_load_image(false, efi_root,
> +				      file_path, image_buf, size, &handle));
> +	if (ret != EFI_SUCCESS)
> +		goto err_prepare;
> +
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> +
> +err_prepare:
> +	if (device_path)
> +		efi_free_pool(file_path);
> +
> +err_add_protocol:
> +	if (mem_handle)
> +		efi_delete_handle(mem_handle);
> +	if (file_path) /* hence memdp */
> +		efi_free_pool(file_path);
>
>   	if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
> -	else
> -		return CMD_RET_SUCCESS;
> +
> +	return CMD_RET_SUCCESS;
>   }
>
>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> @@ -598,7 +606,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 address]\n"
>   	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
>   	"\n"
>   	"    If specified, the device tree located at <fdt address> gets\n"
> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>   	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>   	if (ret == EFI_SUCCESS) {
>   		bootefi_device_path = device;
> +		if (image) {
> +			/* FIXME: image should not contain device */
> +			struct efi_device_path *image_tmp = image;
> +
> +			efi_dp_split_file_path(image, &device, &image);
> +			efi_free_pool(image_tmp);
> +		}
>   		bootefi_image_path = image;
>   	} else {
>   		bootefi_device_path = NULL;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 93f7672aecbc..39ed8a6fa592 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>   				    struct efi_device_path *file_path,
>   				    struct efi_loaded_image_obj **handle_ptr,
>   				    struct efi_loaded_image **info_ptr);
> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> -				      void **buffer, efi_uintn_t *size);
>   /* Print information about all loaded images */
>   void efi_print_image_infos(void *pc);
>
> @@ -567,8 +565,7 @@ struct efi_load_option {
>
>   void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>   unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> -		       struct efi_device_path **file_path);
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>
>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 4fccadc5483d..13ec79b2098b 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
>    * if successful.  This checks that the EFI_LOAD_OPTION is active (enabled)
>    * and that the specified file to boot exists.
>    */
> -static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> -			    struct efi_device_path **file_path)
> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>   {
>   	struct efi_load_option lo;
>   	u16 varname[] = L"Boot0000";
>   	u16 hexmap[] = L"0123456789ABCDEF";
> -	void *load_option, *image = NULL;
> +	void *load_option;
>   	efi_uintn_t size;
> +	efi_status_t ret;
>
>   	varname[4] = hexmap[(n & 0xf000) >> 12];
>   	varname[5] = hexmap[(n & 0x0f00) >> 8];
> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>
>   	load_option = get_var(varname, &efi_global_variable_guid, &size);
>   	if (!load_option)
> -		return NULL;
> +		return EFI_LOAD_ERROR;
>
>   	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);
>
> -		ret = efi_load_image_from_path(lo.file_path, &image, &size);
> -
> +		ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,

In cmd/booefi.c you use efi_root. So we shall do the same here.

I will fix that. No need to resend the patch.


> +					      lo.file_path, NULL, 0,
> +					      handle));
>   		if (ret != EFI_SUCCESS)
>   			goto error;
TODO:
Now that you have the loaded image protocol you should update the load
options with the value in load_option. I guess you do not want to
overwrite it with the content of bootargs.

A the problem was not introduced with this patch series:

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

>
> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>   				L"BootCurrent",
>   				(efi_guid_t *)&efi_global_variable_guid,
>   				attributes, size, &n));
> -		if (ret != EFI_SUCCESS)
> +		if (ret != EFI_SUCCESS) {
> +			if (EFI_CALL(efi_unload_image(*handle))
> +			    != EFI_SUCCESS)
> +				printf("Unloading image failed\n");
>   			goto error;
> +		}
>
>   		printf("Booting: %ls\n", lo.label);
> -		efi_dp_split_file_path(lo.file_path, device_path, file_path);
> +	} else {
> +		ret = EFI_LOAD_ERROR;
>   	}
>
>   error:
>   	free(load_option);
>
> -	return image;
> +	return ret;
>   }
>
>   /*
> @@ -177,12 +182,10 @@ error:
>    * EFI variable, the available load-options, finding and returning
>    * the first one that can be loaded successfully.
>    */
> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> -		       struct efi_device_path **file_path)
> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
>   {
>   	u16 bootnext, *bootorder;
>   	efi_uintn_t size;
> -	void *image = NULL;
>   	int i, num;
>   	efi_status_t ret;
>
> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>   		/* load BootNext */
>   		if (ret == EFI_SUCCESS) {
>   			if (size == sizeof(u16)) {
> -				image = try_load_entry(bootnext, device_path,
> -						       file_path);
> -				if (image)
> -					return image;
> +				ret = try_load_entry(bootnext, handle);
> +				if (ret == EFI_SUCCESS)
> +					return ret;
>   			}
>   		} else {
>   			printf("Deleting BootNext failed\n");
> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>   	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
>   	if (!bootorder) {
>   		printf("BootOrder not defined\n");
> +		ret = EFI_NOT_FOUND;
>   		goto error;
>   	}
>
>   	num = size / sizeof(uint16_t);
>   	for (i = 0; i < num; i++) {
>   		debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> -		image = try_load_entry(bootorder[i], device_path, file_path);
> -		if (image)
> +		ret = try_load_entry(bootorder[i], handle);
> +		if (ret == EFI_SUCCESS)
>   			break;
>   	}
>
>   	free(bootorder);
>
>   error:
> -	return image;
> +	return ret;
>   }
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index abc295e392e9..19a58144cf4b 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1591,6 +1591,7 @@ failure:
>    * @size:	size of the loaded image
>    * Return:	status code
>    */
> +static
>   efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>   				      void **buffer, efi_uintn_t *size)
>   {
> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>   	}
>
>   	current_image = image_handle;
> +	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>
>   	/*
>

[U-Boot] [PATCH 9/9] cmd: add efibootmgr command

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
> With this patch, EFI boot manager can be kicked in by a standalone
> command, efibootmgr.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

I still do not see that we really need a command efibootmgr duplicationg
`bootefi bootmgr`.

What I think is worthwhile in your patch is falling back to
$fdt_control_addr.

The Embedded Base Boot Requirements Specification (EBBR) requires that
we always have either an ACPI table or a device tree but not both.

So if no ACPI table is provided we should fallback to $fdtcontroladdr.
If an ACPI table is present (i.e. on x86) we should not allow an FDT to
be passed to bootefi.

Best regards

Heinrich


> ---
>   cmd/Kconfig   |  8 ++++++++
>   cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 43 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 2bdbfcb3d091..46222be4a3c6 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -224,6 +224,14 @@ config CMD_BOOTEFI
>   	help
>   	  Boot an EFI image from memory.
>
> +config CMD_STANDALONE_EFIBOOTMGR
> +	bool "Enable EFI Boot Manager as a standalone command"
> +	depends on CMD_BOOTEFI
> +	default n
> +	help
> +          With this option enabled, EFI Boot Manager can be invoked
> +	  as a standalone command.
> +
>   config CMD_BOOTEFI_HELLO_COMPILE
>   	bool "Compile a standard EFI hello world binary for testing"
>   	depends on CMD_BOOTEFI && !CPU_V7M && !SANDBOX
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 8c84fff590a7..348c6fdbe28d 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -644,3 +644,38 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>   		bootefi_image_path = NULL;
>   	}
>   }
> +
> +#ifdef CONFIG_CMD_STANDALONE_EFIBOOTMGR
> +static int do_efibootmgr_cmd(cmd_tbl_t *cmdtp, int flag, int argc,
> +			     char * const argv[])
> +{
> +	char *fdt_opt;
> +	int ret;
> +
> +	if (argc != 1)
> +		return CMD_RET_USAGE;
> +
> +	fdt_opt = env_get("fdtaddr");
> +	if (!fdt_opt)
> +		fdt_opt = env_get("fdtcontroladdr");
> +
> +	ret = do_efibootmgr(fdt_opt);
> +
> +	free(fdt_opt);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_SYS_LONGHELP
> +static char efibootmgr_help_text[] =
> +	"(no arguments)\n"
> +	" - Launch EFI boot manager and execute EFI application\n"
> +	"   based on BootNext and BootOrder variables\n";
> +#endif
> +
> +U_BOOT_CMD(
> +	efibootmgr, 1, 0, do_efibootmgr_cmd,
> +	"Launch EFI boot manager",
> +	efibootmgr_help_text
> +);
> +#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */
>

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/20/19 10:37 AM, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
>> In the current implementation, bootefi command and EFI boot manager
>> don't use load_image API, instead, use more primitive and internal
>> functions. This will introduce duplicated code and potentially
>> unknown bugs as well as inconsistent behaviours.
>>
>> With this patch, do_efibootmgr() and do_boot_efi() are completely
>> overhauled and re-implemented using load_image API.

Hello Takahiro,

with this patch applied iPXE cannot find a chained file on the EFI
partition. Did you test booting GRUB with this patch?

Best regards

Heinrich

EFI: Entry efi_load_image(0, 00000000b9f92020,
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x800512bd,0x800,0x200000)/dtb,
0000000040080000, 21738, 00000000b9f33fc8)
EFI: Exit: efi_load_image: 0
EFI: Entry efi_start_image(00000000b9f95360, 0000000000000000,
0000000000000000)
  EFI: Call: efi_open_protocol(image_handle, &efi_guid_loaded_image,
&info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
  EFI: 0 returned by efi_open_protocol(image_handle,
&efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
  EFI: Jumping into 0x00000000b8e7861c
  EFI: Call: image_obj->entry(image_handle, &systab)
    EFI: Entry efi_load_image(0, 00000000b9f95360, /, 00000000b8e9fbc0,
177, 00000000b9f33e38)
efi_load_pe: Invalid DOS Signature
    EFI: Exit: efi_load_image: 1
iPXE initialising devices...ok
iPXE 1.0.0+ (ac4fb) -- Open Source Network Boot Firmware -- http://ipxe.org
Features: DNS FTP HTTP HTTPS iSCSI NFS TFTP VLAN AoE EFI Menu
iPXE script started
Trying to chain file:/boot.ipxe
Could not start download: No such file or directory
(http://ipxe.org/2d4de08e)
Could not find file:/boot.ipxe
Opening shell
iPXE>


>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>   cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
>>   include/efi_loader.h          |   5 +-
>>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>>   lib/efi_loader/efi_boottime.c |   2 +
>>   4 files changed, 127 insertions(+), 110 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index e2ca68c4dc12..8c84fff590a7 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char
>> *fdt_opt)
>>   /**
>>    * do_bootefi_exec() - execute EFI binary
>>    *
>> - * @efi:        address of the binary
>> - * @device_path:    path of the device from which the binary was loaded
>> - * @image_path:        device path of the binary
>> + * @handle:        handle of loaded image
>>    * Return:        status code
>>    *
>>    * Load the EFI binary into a newly assigned memory unwinding the
>> relocation
>>    * information, install the loaded image protocol, and call the binary.
>>    */
>> -static efi_status_t do_bootefi_exec(void *efi,
>> -                    struct efi_device_path *device_path,
>> -                    struct efi_device_path *image_path)
>> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
>>   {
>> -    efi_handle_t mem_handle = NULL;
>> -    struct efi_device_path *memdp = NULL;
>>       efi_status_t ret;
>> -    struct efi_loaded_image_obj *image_obj = NULL;
>> -    struct efi_loaded_image *loaded_image_info = NULL;
>> -
>> -    /*
>> -     * Special case for efi payload not loaded from disk, such as
>> -     * 'bootefi hello' or for example payload loaded directly into
>> -     * memory via JTAG, etc:
>> -     */
>> -    if (!device_path && !image_path) {
>> -        printf("WARNING: using memory device/image path, this may
>> confuse some payloads!\n");
>> -        /* actual addresses filled in after efi_load_pe() */
>> -        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
>> -        device_path = image_path = memdp;
>> -        /*
>> -         * Grub expects that the device path of the loaded image is
>> -         * installed on a handle.
>> -         */
>> -        ret = efi_create_handle(&mem_handle);
>> -        if (ret != EFI_SUCCESS)
>> -            return ret; /* TODO: leaks device_path */
>> -        ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> -                       device_path);
>> -        if (ret != EFI_SUCCESS)
>> -            goto err_add_protocol;
>> -    } else {
>> -        assert(device_path && image_path);
>> -    }
>> -
>> -    ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
>> -                     &loaded_image_info);
>> -    if (ret)
>> -        goto err_prepare;
>>
>>       /* Transfer environment variable as load options */
>> -    ret = set_load_options((efi_handle_t)image_obj, "bootargs");
>> -    if (ret != EFI_SUCCESS)
>> -        goto err_prepare;
>> -
>> -    /* Load the EFI payload */
>> -    ret = efi_load_pe(image_obj, efi, loaded_image_info);
>> +    ret = set_load_options(handle, "bootargs");
>>       if (ret != EFI_SUCCESS)
>> -        goto err_prepare;
>> -
>> -    if (memdp) {
>> -        struct efi_device_path_memory *mdp = (void *)memdp;
>> -        mdp->memory_type = loaded_image_info->image_code_type;
>> -        mdp->start_address = (uintptr_t)loaded_image_info->image_base;
>> -        mdp->end_address = mdp->start_address +
>> -                loaded_image_info->image_size;
>> -    }
>> +        return ret;
>>
>>       /* we don't support much: */
>>      
>> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>
>>           "{ro,boot}(blob)0000000000000000");
>>
>>       /* Call our payload! */
>> -    debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>> -    ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>> +    ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>>
>> -err_prepare:
>> -    /* image has returned, loaded-image obj goes *poof*: */
>>       efi_restore_gd();
>> -    free(loaded_image_info->load_options);
>> -    efi_delete_handle(&image_obj->header);
>>
>> -err_add_protocol:
>> -    if (mem_handle)
>> -        efi_delete_handle(mem_handle);
>> +    /*
>> +     * FIXME: Who is responsible for
>> +     *    free(loaded_image_info->load_options);
>> +     * Once efi_exit() is implemented correctly,
>> +     * handle itself doesn't exist here.
>> +     */
>>
>>       return ret;
>>   }
>> @@ -339,8 +286,7 @@ err_add_protocol:
>>    */
>>   static int do_efibootmgr(const char *fdt_opt)
>>   {
>> -    struct efi_device_path *device_path, *file_path;
>> -    void *addr;
>> +    efi_handle_t handle;
>>       efi_status_t ret;
>>
>>       /* Allow unaligned memory access */
>> @@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
>>       else if (ret != EFI_SUCCESS)
>>           return CMD_RET_FAILURE;
>>
>> -    addr = efi_bootmgr_load(&device_path, &file_path);
>> -    if (!addr)
>> -        return 1;
>> +    ret = efi_bootmgr_load(&handle);
>> +    if (ret != EFI_SUCCESS) {
>> +        printf("EFI boot manager: Cannot load any image\n");
>> +        return CMD_RET_FAILURE;
>> +    }
>>
>> -    printf("## Starting EFI application at %p ...\n", addr);
>> -    ret = do_bootefi_exec(addr, device_path, file_path);
>> -    printf("## Application terminated, r = %lu\n",
>> -           ret & ~EFI_ERROR_MASK);
>> +    ret = do_bootefi_exec(handle);
>> +    printf("## Application terminated, r = %lu\n", ret &
>> ~EFI_ERROR_MASK);
>>
>>       if (ret != EFI_SUCCESS)
>> -        return 1;
>> +        return CMD_RET_FAILURE;
>>
>> -    return 0;
>> +    return CMD_RET_SUCCESS;
>>   }
>>
>>   /*
>> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
>>    */
>>   static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
>>   {
>> -    unsigned long addr;
>> +    void *image_buf;
>> +    struct efi_device_path *device_path, *image_path;
>> +    struct efi_device_path *memdp = NULL, *file_path = NULL;
>> +    unsigned long addr, size;
>> +    const char *size_str;
>> +    efi_handle_t mem_handle = NULL, handle;
>>       efi_status_t ret;
>>
>>       /* Allow unaligned memory access */
>> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char
>> *image_opt, const char *fdt_opt)
>>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>       if (!strcmp(image_opt, "hello")) {
>>           char *saddr;
>> -        ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>
>>           saddr = env_get("loadaddr");
>> +        size = __efi_helloworld_end - __efi_helloworld_begin;
>> +
>>           if (saddr)
>>               addr = simple_strtoul(saddr, NULL, 16);
>>           else
>>               addr = CONFIG_SYS_LOAD_ADDR;
>> -        memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
>> +
>> +        image_buf = map_sysmem(addr, size);
>> +        memcpy(image_buf, __efi_helloworld_begin, size);
>> +
>> +        device_path = NULL;
>> +        image_path = NULL;
>>       } else
>>   #endif
>>       {
>> +        size_str = env_get("filesize");
>> +        if (size_str)
>> +            size = simple_strtoul(size_str, NULL, 16);
>> +        else
>> +            size = 0;
>> +
>>           addr = simple_strtoul(image_opt, NULL, 16);
>>           /* Check that a numeric value was passed */
>>           if (!addr && *image_opt != '0')
>>               return CMD_RET_USAGE;
>> +
>> +        image_buf = map_sysmem(addr, size);
>> +
>> +        device_path = bootefi_device_path;
>> +        image_path = bootefi_image_path;
>>       }
>>
>> -    printf("## Starting EFI application at %08lx ...\n", addr);
>> -    ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>> -                  bootefi_image_path);
>> -    printf("## Application terminated, r = %lu\n",
>> -           ret & ~EFI_ERROR_MASK);
>> +    if (!device_path && !image_path) {
>> +        /*
>> +         * Special case for efi payload not loaded from disk,
>> +         * such as 'bootefi hello' or for example payload
>> +         * loaded directly into memory via JTAG, etc:
>> +         */
>> +        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>> +                    (uintptr_t)image_buf, size);
>> +        file_path = memdp; /* append(memdp, NULL) */
>> +
>> +        /*
>> +         * Make sure that device for device_path exist
>> +         * in load_image(). Otherwise, shell and grub will fail.
>> +         */
>> +        ret = efi_create_handle(&mem_handle);
>> +        if (ret != EFI_SUCCESS)
>> +            /* TODO: leaks device_path */
>> +            goto err_add_protocol;
>> +
>> +        ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
>> +                       memdp);
>> +        if (ret != EFI_SUCCESS)
>> +            goto err_add_protocol;
>> +    } else {
>> +        assert(device_path && image_path);
>> +        file_path = efi_dp_append(device_path, image_path);
>> +    }
>> +
>> +    ret = EFI_CALL(efi_load_image(false, efi_root,
>> +                      file_path, image_buf, size, &handle));
>> +    if (ret != EFI_SUCCESS)
>> +        goto err_prepare;
>> +
>> +    ret = do_bootefi_exec(handle);
>> +    printf("## Application terminated, r = %lu\n", ret &
>> ~EFI_ERROR_MASK);
>> +
>> +err_prepare:
>> +    if (device_path)
>> +        efi_free_pool(file_path);
>> +
>> +err_add_protocol:
>> +    if (mem_handle)
>> +        efi_delete_handle(mem_handle);
>> +    if (file_path) /* hence memdp */
>> +        efi_free_pool(file_path);
>>
>>       if (ret != EFI_SUCCESS)
>>           return CMD_RET_FAILURE;
>> -    else
>> -        return CMD_RET_SUCCESS;
>> +
>> +    return CMD_RET_SUCCESS;
>>   }
>>
>>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>> @@ -598,7 +606,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 address]\n"
>>       "  - load and boot EFI payload based on BootOrder/BootXXXX
>> variables.\n"
>>       "\n"
>>       "    If specified, the device tree located at <fdt address> gets\n"
>> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char
>> *devnr, const char *path)
>>       ret = efi_dp_from_name(dev, devnr, path, &device, &image);
>>       if (ret == EFI_SUCCESS) {
>>           bootefi_device_path = device;
>> +        if (image) {
>> +            /* FIXME: image should not contain device */
>> +            struct efi_device_path *image_tmp = image;
>> +
>> +            efi_dp_split_file_path(image, &device, &image);
>> +            efi_free_pool(image_tmp);
>> +        }
>>           bootefi_image_path = image;
>>       } else {
>>           bootefi_device_path = NULL;
>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>> index 93f7672aecbc..39ed8a6fa592 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct
>> efi_device_path *device_path,
>>                       struct efi_device_path *file_path,
>>                       struct efi_loaded_image_obj **handle_ptr,
>>                       struct efi_loaded_image **info_ptr);
>> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
>> -                      void **buffer, efi_uintn_t *size);
>>   /* Print information about all loaded images */
>>   void efi_print_image_infos(void *pc);
>>
>> @@ -567,8 +565,7 @@ struct efi_load_option {
>>
>>   void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
>>   unsigned long efi_serialize_load_option(struct efi_load_option *lo,
>> u8 **data);
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> -               struct efi_device_path **file_path);
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
>>
>>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>>
>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> index 4fccadc5483d..13ec79b2098b 100644
>> --- a/lib/efi_loader/efi_bootmgr.c
>> +++ b/lib/efi_loader/efi_bootmgr.c
>> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
>> *vendor,
>>    * if successful.  This checks that the EFI_LOAD_OPTION is active
>> (enabled)
>>    * and that the specified file to boot exists.
>>    */
>> -static void *try_load_entry(uint16_t n, struct efi_device_path
>> **device_path,
>> -                struct efi_device_path **file_path)
>> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>>   {
>>       struct efi_load_option lo;
>>       u16 varname[] = L"Boot0000";
>>       u16 hexmap[] = L"0123456789ABCDEF";
>> -    void *load_option, *image = NULL;
>> +    void *load_option;
>>       efi_uintn_t size;
>> +    efi_status_t ret;
>>
>>       varname[4] = hexmap[(n & 0xf000) >> 12];
>>       varname[5] = hexmap[(n & 0x0f00) >> 8];
>> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>
>>       load_option = get_var(varname, &efi_global_variable_guid, &size);
>>       if (!load_option)
>> -        return NULL;
>> +        return EFI_LOAD_ERROR;
>>
>>       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);
>>
>> -        ret = efi_load_image_from_path(lo.file_path, &image, &size);
>> -
>> +        ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> 
> In cmd/booefi.c you use efi_root. So we shall do the same here.
> 
> I will fix that. No need to resend the patch.
> 
> 
>> +                          lo.file_path, NULL, 0,
>> +                          handle));
>>           if (ret != EFI_SUCCESS)
>>               goto error;
> TODO:
> Now that you have the loaded image protocol you should update the load
> options with the value in load_option. I guess you do not want to
> overwrite it with the content of bootargs.
> 
> A the problem was not introduced with this patch series:
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
>>
>> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>                   L"BootCurrent",
>>                   (efi_guid_t *)&efi_global_variable_guid,
>>                   attributes, size, &n));
>> -        if (ret != EFI_SUCCESS)
>> +        if (ret != EFI_SUCCESS) {
>> +            if (EFI_CALL(efi_unload_image(*handle))
>> +                != EFI_SUCCESS)
>> +                printf("Unloading image failed\n");
>>               goto error;
>> +        }
>>
>>           printf("Booting: %ls\n", lo.label);
>> -        efi_dp_split_file_path(lo.file_path, device_path, file_path);
>> +    } else {
>> +        ret = EFI_LOAD_ERROR;
>>       }
>>
>>   error:
>>       free(load_option);
>>
>> -    return image;
>> +    return ret;
>>   }
>>
>>   /*
>> @@ -177,12 +182,10 @@ error:
>>    * EFI variable, the available load-options, finding and returning
>>    * the first one that can be loaded successfully.
>>    */
>> -void *efi_bootmgr_load(struct efi_device_path **device_path,
>> -               struct efi_device_path **file_path)
>> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
>>   {
>>       u16 bootnext, *bootorder;
>>       efi_uintn_t size;
>> -    void *image = NULL;
>>       int i, num;
>>       efi_status_t ret;
>>
>> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>>           /* load BootNext */
>>           if (ret == EFI_SUCCESS) {
>>               if (size == sizeof(u16)) {
>> -                image = try_load_entry(bootnext, device_path,
>> -                               file_path);
>> -                if (image)
>> -                    return image;
>> +                ret = try_load_entry(bootnext, handle);
>> +                if (ret == EFI_SUCCESS)
>> +                    return ret;
>>               }
>>           } else {
>>               printf("Deleting BootNext failed\n");
>> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
>> **device_path,
>>       bootorder = get_var(L"BootOrder", &efi_global_variable_guid,
>> &size);
>>       if (!bootorder) {
>>           printf("BootOrder not defined\n");
>> +        ret = EFI_NOT_FOUND;
>>           goto error;
>>       }
>>
>>       num = size / sizeof(uint16_t);
>>       for (i = 0; i < num; i++) {
>>           debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
>> -        image = try_load_entry(bootorder[i], device_path, file_path);
>> -        if (image)
>> +        ret = try_load_entry(bootorder[i], handle);
>> +        if (ret == EFI_SUCCESS)
>>               break;
>>       }
>>
>>       free(bootorder);
>>
>>   error:
>> -    return image;
>> +    return ret;
>>   }
>> diff --git a/lib/efi_loader/efi_boottime.c
>> b/lib/efi_loader/efi_boottime.c
>> index abc295e392e9..19a58144cf4b 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1591,6 +1591,7 @@ failure:
>>    * @size:    size of the loaded image
>>    * Return:    status code
>>    */
>> +static
>>   efi_status_t efi_load_image_from_path(struct efi_device_path
>> *file_path,
>>                         void **buffer, efi_uintn_t *size)
>>   {
>> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
>> image_handle,
>>       }
>>
>>       current_image = image_handle;
>> +    EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
>>       ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>>       /*
>>
> 
> 

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/20/19 10:37 AM, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
>> In the current implementation, bootefi command and EFI boot manager
>> don't use load_image API, instead, use more primitive and internal
>> functions. This will introduce duplicated code and potentially
>> unknown bugs as well as inconsistent behaviours.
>>
>> With this patch, do_efibootmgr() and do_boot_efi() are completely
>> overhauled and re-implemented using load_image API.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

<snip />

>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> index 4fccadc5483d..13ec79b2098b 100644
>> --- a/lib/efi_loader/efi_bootmgr.c
>> +++ b/lib/efi_loader/efi_bootmgr.c
>> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
>> *vendor,
>>    * if successful.  This checks that the EFI_LOAD_OPTION is active
>> (enabled)
>>    * and that the specified file to boot exists.
>>    */
>> -static void *try_load_entry(uint16_t n, struct efi_device_path
>> **device_path,
>> -                struct efi_device_path **file_path)
>> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
>>   {
>>       struct efi_load_option lo;
>>       u16 varname[] = L"Boot0000";
>>       u16 hexmap[] = L"0123456789ABCDEF";
>> -    void *load_option, *image = NULL;
>> +    void *load_option;
>>       efi_uintn_t size;
>> +    efi_status_t ret;
>>
>>       varname[4] = hexmap[(n & 0xf000) >> 12];
>>       varname[5] = hexmap[(n & 0x0f00) >> 8];
>> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
>> efi_device_path **device_path,
>>
>>       load_option = get_var(varname, &efi_global_variable_guid, &size);
>>       if (!load_option)
>> -        return NULL;
>> +        return EFI_LOAD_ERROR;
>>
>>       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);
>>
>> -        ret = efi_load_image_from_path(lo.file_path, &image, &size);
>> -
>> +        ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,

boot_policy should be true for requests originating from the boot manager.

Best regards

Heinrich

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> In the current implementation, bootefi command and EFI boot manager
> don't use load_image API, instead, use more primitive and internal
> functions. This will introduce duplicated code and potentially
> unknown bugs as well as inconsistent behaviours.
>
> With this patch, do_efibootmgr() and do_boot_efi() are completely
> overhauled and re-implemented using load_image API.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

<snip />

> +err_prepare:
> +	if (device_path)
> +		efi_free_pool(file_path);

This free is duplicate to the one below. And why should freeing
file_path depend on device_path?

Regards Heinrich

> +
> +err_add_protocol:
> +	if (mem_handle)
> +		efi_delete_handle(mem_handle);
> +	if (file_path) /* hence memdp */
> +		efi_free_pool(file_path);
>
>  	if (ret != EFI_SUCCESS)
>  		return CMD_RET_FAILURE;
> -	else
> -		return CMD_RET_SUCCESS;
> +
> +	return CMD_RET_SUCCESS;
>  }

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[AKASHI Takahiro]

On Sat, Apr 20, 2019 at 07:37:47PM +0200, Heinrich Schuchardt wrote:
> On 4/20/19 10:37 AM, Heinrich Schuchardt wrote:
> > On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >> In the current implementation, bootefi command and EFI boot manager
> >> don't use load_image API, instead, use more primitive and internal
> >> functions. This will introduce duplicated code and potentially
> >> unknown bugs as well as inconsistent behaviours.
> >>
> >> With this patch, do_efibootmgr() and do_boot_efi() are completely
> >> overhauled and re-implemented using load_image API.
> 
> Hello Takahiro,
> 
> with this patch applied iPXE cannot find a chained file on the EFI
> partition. Did you test booting GRUB with this patch?

No. I test my patch only with Shell.efi right now.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> EFI: Entry efi_load_image(0, 00000000b9f92020,
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/SD(0)/SD(0)/HD(1,MBR,0x800512bd,0x800,0x200000)/dtb,
> 0000000040080000, 21738, 00000000b9f33fc8)
> EFI: Exit: efi_load_image: 0
> EFI: Entry efi_start_image(00000000b9f95360, 0000000000000000,
> 0000000000000000)
>   EFI: Call: efi_open_protocol(image_handle, &efi_guid_loaded_image,
> &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
>   EFI: 0 returned by efi_open_protocol(image_handle,
> &efi_guid_loaded_image, &info, NULL, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL)
>   EFI: Jumping into 0x00000000b8e7861c
>   EFI: Call: image_obj->entry(image_handle, &systab)
>     EFI: Entry efi_load_image(0, 00000000b9f95360, /, 00000000b8e9fbc0,
> 177, 00000000b9f33e38)
> efi_load_pe: Invalid DOS Signature
>     EFI: Exit: efi_load_image: 1
> iPXE initialising devices...ok
> iPXE 1.0.0+ (ac4fb) -- Open Source Network Boot Firmware -- http://ipxe.org
> Features: DNS FTP HTTP HTTPS iSCSI NFS TFTP VLAN AoE EFI Menu
> iPXE script started
> Trying to chain file:/boot.ipxe
> Could not start download: No such file or directory
> (http://ipxe.org/2d4de08e)
> Could not find file:/boot.ipxe
> Opening shell
> iPXE>
> 
> 
> >>
> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >> ---
> >>   cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
> >>   include/efi_loader.h          |   5 +-
> >>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
> >>   lib/efi_loader/efi_boottime.c |   2 +
> >>   4 files changed, 127 insertions(+), 110 deletions(-)
> >>
> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >> index e2ca68c4dc12..8c84fff590a7 100644
> >> --- a/cmd/bootefi.c
> >> +++ b/cmd/bootefi.c
> >> @@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char
> >> *fdt_opt)
> >>   /**
> >>    * do_bootefi_exec() - execute EFI binary
> >>    *
> >> - * @efi:        address of the binary
> >> - * @device_path:    path of the device from which the binary was loaded
> >> - * @image_path:        device path of the binary
> >> + * @handle:        handle of loaded image
> >>    * Return:        status code
> >>    *
> >>    * Load the EFI binary into a newly assigned memory unwinding the
> >> relocation
> >>    * information, install the loaded image protocol, and call the binary.
> >>    */
> >> -static efi_status_t do_bootefi_exec(void *efi,
> >> -                    struct efi_device_path *device_path,
> >> -                    struct efi_device_path *image_path)
> >> +static efi_status_t do_bootefi_exec(efi_handle_t handle)
> >>   {
> >> -    efi_handle_t mem_handle = NULL;
> >> -    struct efi_device_path *memdp = NULL;
> >>       efi_status_t ret;
> >> -    struct efi_loaded_image_obj *image_obj = NULL;
> >> -    struct efi_loaded_image *loaded_image_info = NULL;
> >> -
> >> -    /*
> >> -     * Special case for efi payload not loaded from disk, such as
> >> -     * 'bootefi hello' or for example payload loaded directly into
> >> -     * memory via JTAG, etc:
> >> -     */
> >> -    if (!device_path && !image_path) {
> >> -        printf("WARNING: using memory device/image path, this may
> >> confuse some payloads!\n");
> >> -        /* actual addresses filled in after efi_load_pe() */
> >> -        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> >> -        device_path = image_path = memdp;
> >> -        /*
> >> -         * Grub expects that the device path of the loaded image is
> >> -         * installed on a handle.
> >> -         */
> >> -        ret = efi_create_handle(&mem_handle);
> >> -        if (ret != EFI_SUCCESS)
> >> -            return ret; /* TODO: leaks device_path */
> >> -        ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >> -                       device_path);
> >> -        if (ret != EFI_SUCCESS)
> >> -            goto err_add_protocol;
> >> -    } else {
> >> -        assert(device_path && image_path);
> >> -    }
> >> -
> >> -    ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> >> -                     &loaded_image_info);
> >> -    if (ret)
> >> -        goto err_prepare;
> >>
> >>       /* Transfer environment variable as load options */
> >> -    ret = set_load_options((efi_handle_t)image_obj, "bootargs");
> >> -    if (ret != EFI_SUCCESS)
> >> -        goto err_prepare;
> >> -
> >> -    /* Load the EFI payload */
> >> -    ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >> +    ret = set_load_options(handle, "bootargs");
> >>       if (ret != EFI_SUCCESS)
> >> -        goto err_prepare;
> >> -
> >> -    if (memdp) {
> >> -        struct efi_device_path_memory *mdp = (void *)memdp;
> >> -        mdp->memory_type = loaded_image_info->image_code_type;
> >> -        mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> >> -        mdp->end_address = mdp->start_address +
> >> -                loaded_image_info->image_size;
> >> -    }
> >> +        return ret;
> >>
> >>       /* we don't support much: */
> >>      
> >> env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> >>
> >>           "{ro,boot}(blob)0000000000000000");
> >>
> >>       /* Call our payload! */
> >> -    debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> >> -    ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >> +    ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> >>
> >> -err_prepare:
> >> -    /* image has returned, loaded-image obj goes *poof*: */
> >>       efi_restore_gd();
> >> -    free(loaded_image_info->load_options);
> >> -    efi_delete_handle(&image_obj->header);
> >>
> >> -err_add_protocol:
> >> -    if (mem_handle)
> >> -        efi_delete_handle(mem_handle);
> >> +    /*
> >> +     * FIXME: Who is responsible for
> >> +     *    free(loaded_image_info->load_options);
> >> +     * Once efi_exit() is implemented correctly,
> >> +     * handle itself doesn't exist here.
> >> +     */
> >>
> >>       return ret;
> >>   }
> >> @@ -339,8 +286,7 @@ err_add_protocol:
> >>    */
> >>   static int do_efibootmgr(const char *fdt_opt)
> >>   {
> >> -    struct efi_device_path *device_path, *file_path;
> >> -    void *addr;
> >> +    efi_handle_t handle;
> >>       efi_status_t ret;
> >>
> >>       /* Allow unaligned memory access */
> >> @@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
> >>       else if (ret != EFI_SUCCESS)
> >>           return CMD_RET_FAILURE;
> >>
> >> -    addr = efi_bootmgr_load(&device_path, &file_path);
> >> -    if (!addr)
> >> -        return 1;
> >> +    ret = efi_bootmgr_load(&handle);
> >> +    if (ret != EFI_SUCCESS) {
> >> +        printf("EFI boot manager: Cannot load any image\n");
> >> +        return CMD_RET_FAILURE;
> >> +    }
> >>
> >> -    printf("## Starting EFI application at %p ...\n", addr);
> >> -    ret = do_bootefi_exec(addr, device_path, file_path);
> >> -    printf("## Application terminated, r = %lu\n",
> >> -           ret & ~EFI_ERROR_MASK);
> >> +    ret = do_bootefi_exec(handle);
> >> +    printf("## Application terminated, r = %lu\n", ret &
> >> ~EFI_ERROR_MASK);
> >>
> >>       if (ret != EFI_SUCCESS)
> >> -        return 1;
> >> +        return CMD_RET_FAILURE;
> >>
> >> -    return 0;
> >> +    return CMD_RET_SUCCESS;
> >>   }
> >>
> >>   /*
> >> @@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
> >>    */
> >>   static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
> >>   {
> >> -    unsigned long addr;
> >> +    void *image_buf;
> >> +    struct efi_device_path *device_path, *image_path;
> >> +    struct efi_device_path *memdp = NULL, *file_path = NULL;
> >> +    unsigned long addr, size;
> >> +    const char *size_str;
> >> +    efi_handle_t mem_handle = NULL, handle;
> >>       efi_status_t ret;
> >>
> >>       /* Allow unaligned memory access */
> >> @@ -414,33 +365,90 @@ static int do_bootefi_image(const char
> >> *image_opt, const char *fdt_opt)
> >>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >>       if (!strcmp(image_opt, "hello")) {
> >>           char *saddr;
> >> -        ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >>
> >>           saddr = env_get("loadaddr");
> >> +        size = __efi_helloworld_end - __efi_helloworld_begin;
> >> +
> >>           if (saddr)
> >>               addr = simple_strtoul(saddr, NULL, 16);
> >>           else
> >>               addr = CONFIG_SYS_LOAD_ADDR;
> >> -        memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> >> +
> >> +        image_buf = map_sysmem(addr, size);
> >> +        memcpy(image_buf, __efi_helloworld_begin, size);
> >> +
> >> +        device_path = NULL;
> >> +        image_path = NULL;
> >>       } else
> >>   #endif
> >>       {
> >> +        size_str = env_get("filesize");
> >> +        if (size_str)
> >> +            size = simple_strtoul(size_str, NULL, 16);
> >> +        else
> >> +            size = 0;
> >> +
> >>           addr = simple_strtoul(image_opt, NULL, 16);
> >>           /* Check that a numeric value was passed */
> >>           if (!addr && *image_opt != '0')
> >>               return CMD_RET_USAGE;
> >> +
> >> +        image_buf = map_sysmem(addr, size);
> >> +
> >> +        device_path = bootefi_device_path;
> >> +        image_path = bootefi_image_path;
> >>       }
> >>
> >> -    printf("## Starting EFI application at %08lx ...\n", addr);
> >> -    ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >> -                  bootefi_image_path);
> >> -    printf("## Application terminated, r = %lu\n",
> >> -           ret & ~EFI_ERROR_MASK);
> >> +    if (!device_path && !image_path) {
> >> +        /*
> >> +         * Special case for efi payload not loaded from disk,
> >> +         * such as 'bootefi hello' or for example payload
> >> +         * loaded directly into memory via JTAG, etc:
> >> +         */
> >> +        memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >> +                    (uintptr_t)image_buf, size);
> >> +        file_path = memdp; /* append(memdp, NULL) */
> >> +
> >> +        /*
> >> +         * Make sure that device for device_path exist
> >> +         * in load_image(). Otherwise, shell and grub will fail.
> >> +         */
> >> +        ret = efi_create_handle(&mem_handle);
> >> +        if (ret != EFI_SUCCESS)
> >> +            /* TODO: leaks device_path */
> >> +            goto err_add_protocol;
> >> +
> >> +        ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >> +                       memdp);
> >> +        if (ret != EFI_SUCCESS)
> >> +            goto err_add_protocol;
> >> +    } else {
> >> +        assert(device_path && image_path);
> >> +        file_path = efi_dp_append(device_path, image_path);
> >> +    }
> >> +
> >> +    ret = EFI_CALL(efi_load_image(false, efi_root,
> >> +                      file_path, image_buf, size, &handle));
> >> +    if (ret != EFI_SUCCESS)
> >> +        goto err_prepare;
> >> +
> >> +    ret = do_bootefi_exec(handle);
> >> +    printf("## Application terminated, r = %lu\n", ret &
> >> ~EFI_ERROR_MASK);
> >> +
> >> +err_prepare:
> >> +    if (device_path)
> >> +        efi_free_pool(file_path);
> >> +
> >> +err_add_protocol:
> >> +    if (mem_handle)
> >> +        efi_delete_handle(mem_handle);
> >> +    if (file_path) /* hence memdp */
> >> +        efi_free_pool(file_path);
> >>
> >>       if (ret != EFI_SUCCESS)
> >>           return CMD_RET_FAILURE;
> >> -    else
> >> -        return CMD_RET_SUCCESS;
> >> +
> >> +    return CMD_RET_SUCCESS;
> >>   }
> >>
> >>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >> @@ -598,7 +606,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 address]\n"
> >>       "  - load and boot EFI payload based on BootOrder/BootXXXX
> >> variables.\n"
> >>       "\n"
> >>       "    If specified, the device tree located at <fdt address> gets\n"
> >> @@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char
> >> *devnr, const char *path)
> >>       ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> >>       if (ret == EFI_SUCCESS) {
> >>           bootefi_device_path = device;
> >> +        if (image) {
> >> +            /* FIXME: image should not contain device */
> >> +            struct efi_device_path *image_tmp = image;
> >> +
> >> +            efi_dp_split_file_path(image, &device, &image);
> >> +            efi_free_pool(image_tmp);
> >> +        }
> >>           bootefi_image_path = image;
> >>       } else {
> >>           bootefi_device_path = NULL;
> >> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >> index 93f7672aecbc..39ed8a6fa592 100644
> >> --- a/include/efi_loader.h
> >> +++ b/include/efi_loader.h
> >> @@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct
> >> efi_device_path *device_path,
> >>                       struct efi_device_path *file_path,
> >>                       struct efi_loaded_image_obj **handle_ptr,
> >>                       struct efi_loaded_image **info_ptr);
> >> -efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> >> -                      void **buffer, efi_uintn_t *size);
> >>   /* Print information about all loaded images */
> >>   void efi_print_image_infos(void *pc);
> >>
> >> @@ -567,8 +565,7 @@ struct efi_load_option {
> >>
> >>   void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> >>   unsigned long efi_serialize_load_option(struct efi_load_option *lo,
> >> u8 **data);
> >> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> >> -               struct efi_device_path **file_path);
> >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> >>
> >>   #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >>
> >> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >> index 4fccadc5483d..13ec79b2098b 100644
> >> --- a/lib/efi_loader/efi_bootmgr.c
> >> +++ b/lib/efi_loader/efi_bootmgr.c
> >> @@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t
> >> *vendor,
> >>    * if successful.  This checks that the EFI_LOAD_OPTION is active
> >> (enabled)
> >>    * and that the specified file to boot exists.
> >>    */
> >> -static void *try_load_entry(uint16_t n, struct efi_device_path
> >> **device_path,
> >> -                struct efi_device_path **file_path)
> >> +static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> >>   {
> >>       struct efi_load_option lo;
> >>       u16 varname[] = L"Boot0000";
> >>       u16 hexmap[] = L"0123456789ABCDEF";
> >> -    void *load_option, *image = NULL;
> >> +    void *load_option;
> >>       efi_uintn_t size;
> >> +    efi_status_t ret;
> >>
> >>       varname[4] = hexmap[(n & 0xf000) >> 12];
> >>       varname[5] = hexmap[(n & 0x0f00) >> 8];
> >> @@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct
> >> efi_device_path **device_path,
> >>
> >>       load_option = get_var(varname, &efi_global_variable_guid, &size);
> >>       if (!load_option)
> >> -        return NULL;
> >> +        return EFI_LOAD_ERROR;
> >>
> >>       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);
> >>
> >> -        ret = efi_load_image_from_path(lo.file_path, &image, &size);
> >> -
> >> +        ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> > 
> > In cmd/booefi.c you use efi_root. So we shall do the same here.
> > 
> > I will fix that. No need to resend the patch.
> > 
> > 
> >> +                          lo.file_path, NULL, 0,
> >> +                          handle));
> >>           if (ret != EFI_SUCCESS)
> >>               goto error;
> > TODO:
> > Now that you have the loaded image protocol you should update the load
> > options with the value in load_option. I guess you do not want to
> > overwrite it with the content of bootargs.
> > 
> > A the problem was not introduced with this patch series:
> > 
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> > 
> >>
> >> @@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct
> >> efi_device_path **device_path,
> >>                   L"BootCurrent",
> >>                   (efi_guid_t *)&efi_global_variable_guid,
> >>                   attributes, size, &n));
> >> -        if (ret != EFI_SUCCESS)
> >> +        if (ret != EFI_SUCCESS) {
> >> +            if (EFI_CALL(efi_unload_image(*handle))
> >> +                != EFI_SUCCESS)
> >> +                printf("Unloading image failed\n");
> >>               goto error;
> >> +        }
> >>
> >>           printf("Booting: %ls\n", lo.label);
> >> -        efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >> +    } else {
> >> +        ret = EFI_LOAD_ERROR;
> >>       }
> >>
> >>   error:
> >>       free(load_option);
> >>
> >> -    return image;
> >> +    return ret;
> >>   }
> >>
> >>   /*
> >> @@ -177,12 +182,10 @@ error:
> >>    * EFI variable, the available load-options, finding and returning
> >>    * the first one that can be loaded successfully.
> >>    */
> >> -void *efi_bootmgr_load(struct efi_device_path **device_path,
> >> -               struct efi_device_path **file_path)
> >> +efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> >>   {
> >>       u16 bootnext, *bootorder;
> >>       efi_uintn_t size;
> >> -    void *image = NULL;
> >>       int i, num;
> >>       efi_status_t ret;
> >>
> >> @@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path
> >> **device_path,
> >>           /* load BootNext */
> >>           if (ret == EFI_SUCCESS) {
> >>               if (size == sizeof(u16)) {
> >> -                image = try_load_entry(bootnext, device_path,
> >> -                               file_path);
> >> -                if (image)
> >> -                    return image;
> >> +                ret = try_load_entry(bootnext, handle);
> >> +                if (ret == EFI_SUCCESS)
> >> +                    return ret;
> >>               }
> >>           } else {
> >>               printf("Deleting BootNext failed\n");
> >> @@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path
> >> **device_path,
> >>       bootorder = get_var(L"BootOrder", &efi_global_variable_guid,
> >> &size);
> >>       if (!bootorder) {
> >>           printf("BootOrder not defined\n");
> >> +        ret = EFI_NOT_FOUND;
> >>           goto error;
> >>       }
> >>
> >>       num = size / sizeof(uint16_t);
> >>       for (i = 0; i < num; i++) {
> >>           debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> >> -        image = try_load_entry(bootorder[i], device_path, file_path);
> >> -        if (image)
> >> +        ret = try_load_entry(bootorder[i], handle);
> >> +        if (ret == EFI_SUCCESS)
> >>               break;
> >>       }
> >>
> >>       free(bootorder);
> >>
> >>   error:
> >> -    return image;
> >> +    return ret;
> >>   }
> >> diff --git a/lib/efi_loader/efi_boottime.c
> >> b/lib/efi_loader/efi_boottime.c
> >> index abc295e392e9..19a58144cf4b 100644
> >> --- a/lib/efi_loader/efi_boottime.c
> >> +++ b/lib/efi_loader/efi_boottime.c
> >> @@ -1591,6 +1591,7 @@ failure:
> >>    * @size:    size of the loaded image
> >>    * Return:    status code
> >>    */
> >> +static
> >>   efi_status_t efi_load_image_from_path(struct efi_device_path
> >> *file_path,
> >>                         void **buffer, efi_uintn_t *size)
> >>   {
> >> @@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t
> >> image_handle,
> >>       }
> >>
> >>       current_image = image_handle;
> >> +    EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >>       ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >>
> >>       /*
> >>
> > 
> > 
> 

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[AKASHI Takahiro]

On Sat, Apr 20, 2019 at 10:07:20AM +0200, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >+	/*
> >+	 * FIXME: Who is responsible for
> >+	 *	free(loaded_image_info->load_options);
> >+	 * Once efi_exit() is implemented correctly,
> >+	 * handle itself doesn't exist here.
> >+	 */
> 
> Load option can only be freed when the image is unloaded. For
> applications that will happen when the application call Exit(). For
> drivers there is not guarantee that they will be ever unloaded.

Should we be able to utilize "bootargs" even for drivers?

> In Unload() we should not free load options if they were not allocated
> by us but by an EFI binary.
> 
> In your implementation you have allocated the load options via calloc()
> and not from the EFI pool. So this may allow us to determine whether we
> allocated the load options in the Unload() service.

DO you mean:
    efi_unload_image(...)
        ret = EFI_CALL(efi_pool_free(info->load_options));
        if (ret != EFI_SUCCESS)
        /*
         * Don't care, assuming that load_options have been allocated
         * by application.
         */

It looks fragile.

-Takahiro Akashi

> Best regards
> 
> Heinrich

[U-Boot] [PATCH 8/9] efi_loader: rework bootmgr/bootefi using load_image API

[AKASHI Takahiro]

On Sat, Apr 20, 2019 at 10:37:54AM +0200, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >In the current implementation, bootefi command and EFI boot manager
> >don't use load_image API, instead, use more primitive and internal
> >functions. This will introduce duplicated code and potentially
> >unknown bugs as well as inconsistent behaviours.
> >
> >With this patch, do_efibootmgr() and do_boot_efi() are completely
> >overhauled and re-implemented using load_image API.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/bootefi.c                 | 187 ++++++++++++++++++----------------
> >  include/efi_loader.h          |   5 +-
> >  lib/efi_loader/efi_bootmgr.c  |  43 ++++----
> >  lib/efi_loader/efi_boottime.c |   2 +
> >  4 files changed, 127 insertions(+), 110 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index e2ca68c4dc12..8c84fff590a7 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -242,89 +242,36 @@ static efi_status_t efi_install_fdt(const char *fdt_opt)
> >  /**
> >   * do_bootefi_exec() - execute EFI binary
> >   *
> >- * @efi:		address of the binary
> >- * @device_path:	path of the device from which the binary was loaded
> >- * @image_path:		device path of the binary
> >+ * @handle:		handle of loaded image
> >   * Return:		status code
> >   *
> >   * Load the EFI binary into a newly assigned memory unwinding the relocation
> >   * information, install the loaded image protocol, and call the binary.
> >   */
> >-static efi_status_t do_bootefi_exec(void *efi,
> >-				    struct efi_device_path *device_path,
> >-				    struct efi_device_path *image_path)
> >+static efi_status_t do_bootefi_exec(efi_handle_t handle)
> >  {
> >-	efi_handle_t mem_handle = NULL;
> >-	struct efi_device_path *memdp = NULL;
> >  	efi_status_t ret;
> >-	struct efi_loaded_image_obj *image_obj = NULL;
> >-	struct efi_loaded_image *loaded_image_info = NULL;
> >-
> >-	/*
> >-	 * Special case for efi payload not loaded from disk, such as
> >-	 * 'bootefi hello' or for example payload loaded directly into
> >-	 * memory via JTAG, etc:
> >-	 */
> >-	if (!device_path && !image_path) {
> >-		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> >-		/* actual addresses filled in after efi_load_pe() */
> >-		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
> >-		device_path = image_path = memdp;
> >-		/*
> >-		 * Grub expects that the device path of the loaded image is
> >-		 * installed on a handle.
> >-		 */
> >-		ret = efi_create_handle(&mem_handle);
> >-		if (ret != EFI_SUCCESS)
> >-			return ret; /* TODO: leaks device_path */
> >-		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >-				       device_path);
> >-		if (ret != EFI_SUCCESS)
> >-			goto err_add_protocol;
> >-	} else {
> >-		assert(device_path && image_path);
> >-	}
> >-
> >-	ret = efi_setup_loaded_image(device_path, image_path, &image_obj,
> >-				     &loaded_image_info);
> >-	if (ret)
> >-		goto err_prepare;
> >
> >  	/* Transfer environment variable as load options */
> >-	ret = set_load_options((efi_handle_t)image_obj, "bootargs");
> >-	if (ret != EFI_SUCCESS)
> >-		goto err_prepare;
> >-
> >-	/* Load the EFI payload */
> >-	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >+	ret = set_load_options(handle, "bootargs");
> >  	if (ret != EFI_SUCCESS)
> >-		goto err_prepare;
> >-
> >-	if (memdp) {
> >-		struct efi_device_path_memory *mdp = (void *)memdp;
> >-		mdp->memory_type = loaded_image_info->image_code_type;
> >-		mdp->start_address = (uintptr_t)loaded_image_info->image_base;
> >-		mdp->end_address = mdp->start_address +
> >-				loaded_image_info->image_size;
> >-	}
> >+		return ret;
> >
> >  	/* we don't support much: */
> >  	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> >  		"{ro,boot}(blob)0000000000000000");
> >
> >  	/* Call our payload! */
> >-	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> >-	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >+	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> >
> >-err_prepare:
> >-	/* image has returned, loaded-image obj goes *poof*: */
> >  	efi_restore_gd();
> >-	free(loaded_image_info->load_options);
> >-	efi_delete_handle(&image_obj->header);
> >
> >-err_add_protocol:
> >-	if (mem_handle)
> >-		efi_delete_handle(mem_handle);
> >+	/*
> >+	 * FIXME: Who is responsible for
> >+	 *	free(loaded_image_info->load_options);
> >+	 * Once efi_exit() is implemented correctly,
> >+	 * handle itself doesn't exist here.
> >+	 */
> >
> >  	return ret;
> >  }
> >@@ -339,8 +286,7 @@ err_add_protocol:
> >   */
> >  static int do_efibootmgr(const char *fdt_opt)
> >  {
> >-	struct efi_device_path *device_path, *file_path;
> >-	void *addr;
> >+	efi_handle_t handle;
> >  	efi_status_t ret;
> >
> >  	/* Allow unaligned memory access */
> >@@ -362,19 +308,19 @@ static int do_efibootmgr(const char *fdt_opt)
> >  	else if (ret != EFI_SUCCESS)
> >  		return CMD_RET_FAILURE;
> >
> >-	addr = efi_bootmgr_load(&device_path, &file_path);
> >-	if (!addr)
> >-		return 1;
> >+	ret = efi_bootmgr_load(&handle);
> >+	if (ret != EFI_SUCCESS) {
> >+		printf("EFI boot manager: Cannot load any image\n");
> >+		return CMD_RET_FAILURE;
> >+	}
> >
> >-	printf("## Starting EFI application at %p ...\n", addr);
> >-	ret = do_bootefi_exec(addr, device_path, file_path);
> >-	printf("## Application terminated, r = %lu\n",
> >-	       ret & ~EFI_ERROR_MASK);
> >+	ret = do_bootefi_exec(handle);
> >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >
> >  	if (ret != EFI_SUCCESS)
> >-		return 1;
> >+		return CMD_RET_FAILURE;
> >
> >-	return 0;
> >+	return CMD_RET_SUCCESS;
> >  }
> >
> >  /*
> >@@ -389,7 +335,12 @@ static int do_efibootmgr(const char *fdt_opt)
> >   */
> >  static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
> >  {
> >-	unsigned long addr;
> >+	void *image_buf;
> >+	struct efi_device_path *device_path, *image_path;
> >+	struct efi_device_path *memdp = NULL, *file_path = NULL;
> >+	unsigned long addr, size;
> >+	const char *size_str;
> >+	efi_handle_t mem_handle = NULL, handle;
> >  	efi_status_t ret;
> >
> >  	/* Allow unaligned memory access */
> >@@ -414,33 +365,90 @@ static int do_bootefi_image(const char *image_opt, const char *fdt_opt)
> >  #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >  	if (!strcmp(image_opt, "hello")) {
> >  		char *saddr;
> >-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >
> >  		saddr = env_get("loadaddr");
> >+		size = __efi_helloworld_end - __efi_helloworld_begin;
> >+
> >  		if (saddr)
> >  			addr = simple_strtoul(saddr, NULL, 16);
> >  		else
> >  			addr = CONFIG_SYS_LOAD_ADDR;
> >-		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> >+
> >+		image_buf = map_sysmem(addr, size);
> >+		memcpy(image_buf, __efi_helloworld_begin, size);
> >+
> >+		device_path = NULL;
> >+		image_path = NULL;
> >  	} else
> >  #endif
> >  	{
> >+		size_str = env_get("filesize");
> >+		if (size_str)
> >+			size = simple_strtoul(size_str, NULL, 16);
> >+		else
> >+			size = 0;
> >+
> >  		addr = simple_strtoul(image_opt, NULL, 16);
> >  		/* Check that a numeric value was passed */
> >  		if (!addr && *image_opt != '0')
> >  			return CMD_RET_USAGE;
> >+
> >+		image_buf = map_sysmem(addr, size);
> >+
> >+		device_path = bootefi_device_path;
> >+		image_path = bootefi_image_path;
> >  	}
> >
> >-	printf("## Starting EFI application at %08lx ...\n", addr);
> >-	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >-			      bootefi_image_path);
> >-	printf("## Application terminated, r = %lu\n",
> >-	       ret & ~EFI_ERROR_MASK);
> >+	if (!device_path && !image_path) {
> >+		/*
> >+		 * Special case for efi payload not loaded from disk,
> >+		 * such as 'bootefi hello' or for example payload
> >+		 * loaded directly into memory via JTAG, etc:
> >+		 */
> >+		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >+					(uintptr_t)image_buf, size);
> >+		file_path = memdp; /* append(memdp, NULL) */
> >+
> >+		/*
> >+		 * Make sure that device for device_path exist
> >+		 * in load_image(). Otherwise, shell and grub will fail.
> >+		 */
> >+		ret = efi_create_handle(&mem_handle);
> >+		if (ret != EFI_SUCCESS)
> >+			/* TODO: leaks device_path */
> >+			goto err_add_protocol;
> >+
> >+		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
> >+				       memdp);
> >+		if (ret != EFI_SUCCESS)
> >+			goto err_add_protocol;
> >+	} else {
> >+		assert(device_path && image_path);
> >+		file_path = efi_dp_append(device_path, image_path);
> >+	}
> >+
> >+	ret = EFI_CALL(efi_load_image(false, efi_root,
> >+				      file_path, image_buf, size, &handle));
> >+	if (ret != EFI_SUCCESS)
> >+		goto err_prepare;
> >+
> >+	ret = do_bootefi_exec(handle);
> >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >+
> >+err_prepare:
> >+	if (device_path)
> >+		efi_free_pool(file_path);
> >+
> >+err_add_protocol:
> >+	if (mem_handle)
> >+		efi_delete_handle(mem_handle);
> >+	if (file_path) /* hence memdp */
> >+		efi_free_pool(file_path);
> >
> >  	if (ret != EFI_SUCCESS)
> >  		return CMD_RET_FAILURE;
> >-	else
> >-		return CMD_RET_SUCCESS;
> >+
> >+	return CMD_RET_SUCCESS;
> >  }
> >
> >  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >@@ -598,7 +606,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 address]\n"
> >  	"  - load and boot EFI payload based on BootOrder/BootXXXX variables.\n"
> >  	"\n"
> >  	"    If specified, the device tree located at <fdt address> gets\n"
> >@@ -623,6 +631,13 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
> >  	ret = efi_dp_from_name(dev, devnr, path, &device, &image);
> >  	if (ret == EFI_SUCCESS) {
> >  		bootefi_device_path = device;
> >+		if (image) {
> >+			/* FIXME: image should not contain device */
> >+			struct efi_device_path *image_tmp = image;
> >+
> >+			efi_dp_split_file_path(image, &device, &image);
> >+			efi_free_pool(image_tmp);
> >+		}
> >  		bootefi_image_path = image;
> >  	} else {
> >  		bootefi_device_path = NULL;
> >diff --git a/include/efi_loader.h b/include/efi_loader.h
> >index 93f7672aecbc..39ed8a6fa592 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -412,8 +412,6 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >  				    struct efi_device_path *file_path,
> >  				    struct efi_loaded_image_obj **handle_ptr,
> >  				    struct efi_loaded_image **info_ptr);
> >-efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> >-				      void **buffer, efi_uintn_t *size);
> >  /* Print information about all loaded images */
> >  void efi_print_image_infos(void *pc);
> >
> >@@ -567,8 +565,7 @@ struct efi_load_option {
> >
> >  void efi_deserialize_load_option(struct efi_load_option *lo, u8 *data);
> >  unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
> >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> >-		       struct efi_device_path **file_path);
> >+efi_status_t efi_bootmgr_load(efi_handle_t *handle);
> >
> >  #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >
> >diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >index 4fccadc5483d..13ec79b2098b 100644
> >--- a/lib/efi_loader/efi_bootmgr.c
> >+++ b/lib/efi_loader/efi_bootmgr.c
> >@@ -120,14 +120,14 @@ static void *get_var(u16 *name, const efi_guid_t *vendor,
> >   * if successful.  This checks that the EFI_LOAD_OPTION is active (enabled)
> >   * and that the specified file to boot exists.
> >   */
> >-static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >-			    struct efi_device_path **file_path)
> >+static efi_status_t try_load_entry(u16 n, efi_handle_t *handle)
> >  {
> >  	struct efi_load_option lo;
> >  	u16 varname[] = L"Boot0000";
> >  	u16 hexmap[] = L"0123456789ABCDEF";
> >-	void *load_option, *image = NULL;
> >+	void *load_option;
> >  	efi_uintn_t size;
> >+	efi_status_t ret;
> >
> >  	varname[4] = hexmap[(n & 0xf000) >> 12];
> >  	varname[5] = hexmap[(n & 0x0f00) >> 8];
> >@@ -136,19 +136,19 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >
> >  	load_option = get_var(varname, &efi_global_variable_guid, &size);
> >  	if (!load_option)
> >-		return NULL;
> >+		return EFI_LOAD_ERROR;
> >
> >  	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);
> >
> >-		ret = efi_load_image_from_path(lo.file_path, &image, &size);
> >-
> >+		ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */,
> 
> In cmd/booefi.c you use efi_root. So we shall do the same here.
> 
> I will fix that. No need to resend the patch.

Thanks, but only if we need more fixes in this patch.

> 
> >+					      lo.file_path, NULL, 0,
> >+					      handle));
> >  		if (ret != EFI_SUCCESS)
> >  			goto error;
> TODO:
> Now that you have the loaded image protocol you should update the load
> options with the value in load_option. I guess you do not want to
> overwrite it with the content of bootargs.

Right, I have not noticed this issue.

-Takahiro Akashi

> A the problem was not introduced with this patch series:
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> >
> >@@ -159,17 +159,22 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >  				L"BootCurrent",
> >  				(efi_guid_t *)&efi_global_variable_guid,
> >  				attributes, size, &n));
> >-		if (ret != EFI_SUCCESS)
> >+		if (ret != EFI_SUCCESS) {
> >+			if (EFI_CALL(efi_unload_image(*handle))
> >+			    != EFI_SUCCESS)
> >+				printf("Unloading image failed\n");
> >  			goto error;
> >+		}
> >
> >  		printf("Booting: %ls\n", lo.label);
> >-		efi_dp_split_file_path(lo.file_path, device_path, file_path);
> >+	} else {
> >+		ret = EFI_LOAD_ERROR;
> >  	}
> >
> >  error:
> >  	free(load_option);
> >
> >-	return image;
> >+	return ret;
> >  }
> >
> >  /*
> >@@ -177,12 +182,10 @@ error:
> >   * EFI variable, the available load-options, finding and returning
> >   * the first one that can be loaded successfully.
> >   */
> >-void *efi_bootmgr_load(struct efi_device_path **device_path,
> >-		       struct efi_device_path **file_path)
> >+efi_status_t efi_bootmgr_load(efi_handle_t *handle)
> >  {
> >  	u16 bootnext, *bootorder;
> >  	efi_uintn_t size;
> >-	void *image = NULL;
> >  	int i, num;
> >  	efi_status_t ret;
> >
> >@@ -209,10 +212,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  		/* load BootNext */
> >  		if (ret == EFI_SUCCESS) {
> >  			if (size == sizeof(u16)) {
> >-				image = try_load_entry(bootnext, device_path,
> >-						       file_path);
> >-				if (image)
> >-					return image;
> >+				ret = try_load_entry(bootnext, handle);
> >+				if (ret == EFI_SUCCESS)
> >+					return ret;
> >  			}
> >  		} else {
> >  			printf("Deleting BootNext failed\n");
> >@@ -223,19 +225,20 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  	bootorder = get_var(L"BootOrder", &efi_global_variable_guid, &size);
> >  	if (!bootorder) {
> >  		printf("BootOrder not defined\n");
> >+		ret = EFI_NOT_FOUND;
> >  		goto error;
> >  	}
> >
> >  	num = size / sizeof(uint16_t);
> >  	for (i = 0; i < num; i++) {
> >  		debug("%s: trying to load Boot%04X\n", __func__, bootorder[i]);
> >-		image = try_load_entry(bootorder[i], device_path, file_path);
> >-		if (image)
> >+		ret = try_load_entry(bootorder[i], handle);
> >+		if (ret == EFI_SUCCESS)
> >  			break;
> >  	}
> >
> >  	free(bootorder);
> >
> >  error:
> >-	return image;
> >+	return ret;
> >  }
> >diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >index abc295e392e9..19a58144cf4b 100644
> >--- a/lib/efi_loader/efi_boottime.c
> >+++ b/lib/efi_loader/efi_boottime.c
> >@@ -1591,6 +1591,7 @@ failure:
> >   * @size:	size of the loaded image
> >   * Return:	status code
> >   */
> >+static
> >  efi_status_t efi_load_image_from_path(struct efi_device_path *file_path,
> >  				      void **buffer, efi_uintn_t *size)
> >  {
> >@@ -2664,6 +2665,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >  	}
> >
> >  	current_image = image_handle;
> >+	EFI_PRINT("Jumping into 0x%p\n", image_obj->entry);
> >  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >
> >  	/*
> >
> 

[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> There are several reasons that I want to rework/refactor bootefi command
> as well as bootmgr:
> * Some previous commits on bootefi.c have made the code complicated
>   and a bit hard to understand.
>
> * do_bootefi_exec() would better be implemented using load_image() along
>   with start_image() to be aligned with UEFI interfaces.
>
> * Contrary to the other part, efi_selftest part of the code is unusual
>   in terms of loading/execution path in do_bootefi().
>
> * When we will support "secure boot" in the future, EFI Boot Manager
>   is expected to be invoked as a standalone command without any arguments
>   to mitigate security surfaces.
>
> In this patch set,
> Patch#1 to #7 are preparatory patches for patch#8.
> Patch#8 is a core part of reworking.
> Patch#9 is for standalone boot manager.

Hello Takahiro,

your patches 1-8 are now (with some modifications) in the efi-2019-07
branch. I have added some more patches.

https://github.com/xypron2/u-boot/commits/efi-2019-07

Travis CI testing was ok:
https://travis-ci.org/xypron2/u-boot/builds/522947698

I have tested on real hardware without error:
* TinkerBoard (32 bit) boot via GRUB
* Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
* Odroid C2 (64 bit) boot via GRUB

If you are ok with the adjustments to your patch series I will create a
pull request.

Best regards

Heinrich

[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[AKASHI, Takahiro]

Heinrich,

On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> > There are several reasons that I want to rework/refactor bootefi command
> > as well as bootmgr:
> > * Some previous commits on bootefi.c have made the code complicated
> >   and a bit hard to understand.
> >
> > * do_bootefi_exec() would better be implemented using load_image() along
> >   with start_image() to be aligned with UEFI interfaces.
> >
> > * Contrary to the other part, efi_selftest part of the code is unusual
> >   in terms of loading/execution path in do_bootefi().
> >
> > * When we will support "secure boot" in the future, EFI Boot Manager
> >   is expected to be invoked as a standalone command without any arguments
> >   to mitigate security surfaces.
> >
> > In this patch set,
> > Patch#1 to #7 are preparatory patches for patch#8.
> > Patch#8 is a core part of reworking.
> > Patch#9 is for standalone boot manager.
>
> Hello Takahiro,
>
> your patches 1-8 are now (with some modifications) in the efi-2019-07
> branch. I have added some more patches.
>
> https://github.com/xypron2/u-boot/commits/efi-2019-07
>
> Travis CI testing was ok:
> https://travis-ci.org/xypron2/u-boot/builds/522947698
>
> I have tested on real hardware without error:
> * TinkerBoard (32 bit) boot via GRUB
> * Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
> * Odroid C2 (64 bit) boot via GRUB
>
> If you are ok with the adjustments to your patch series I will create a
> pull request.

Thank you for the merge.
One question: how did you fix a grub error that you reported before?

-Takahiro Akashi


> Best regards
>
> Heinrich

[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[Heinrich Schuchardt]

On 4/23/19 2:24 AM, AKASHI, Takahiro wrote:
> Heinrich,
>
> On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
>>> There are several reasons that I want to rework/refactor bootefi command
>>> as well as bootmgr:
>>> * Some previous commits on bootefi.c have made the code complicated
>>>    and a bit hard to understand.
>>>
>>> * do_bootefi_exec() would better be implemented using load_image() along
>>>    with start_image() to be aligned with UEFI interfaces.
>>>
>>> * Contrary to the other part, efi_selftest part of the code is unusual
>>>    in terms of loading/execution path in do_bootefi().
>>>
>>> * When we will support "secure boot" in the future, EFI Boot Manager
>>>    is expected to be invoked as a standalone command without any arguments
>>>    to mitigate security surfaces.
>>>
>>> In this patch set,
>>> Patch#1 to #7 are preparatory patches for patch#8.
>>> Patch#8 is a core part of reworking.
>>> Patch#9 is for standalone boot manager.
>>
>> Hello Takahiro,
>>
>> your patches 1-8 are now (with some modifications) in the efi-2019-07
>> branch. I have added some more patches.
>>
>> https://github.com/xypron2/u-boot/commits/efi-2019-07
>>
>> Travis CI testing was ok:
>> https://travis-ci.org/xypron2/u-boot/builds/522947698
>>
>> I have tested on real hardware without error:
>> * TinkerBoard (32 bit) boot via GRUB
>> * Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
>> * Odroid C2 (64 bit) boot via GRUB
>>
>> If you are ok with the adjustments to your patch series I will create a
>> pull request.
>
> Thank you for the merge.
> One question: how did you fix a grub error that you reported before?

I fixed the issue that iPXE could not find a chained script via
efi_loader: correctly split device path of loaded image
https://lists.denx.de/pipermail/u-boot/2019-April/365907.html

Furthermore I fixed a bug in Debian's QEMU by applying Alex's
https://github.com/qemu/qemu/commit/2d2a4549cc29850aab891495685a7b31f5254b12

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>

[U-Boot] [PATCH 0/9] efi_loader: rework bootefi/bootmgr

[AKASHI, Takahiro]

Okay.
Once your pull is done, I will submit "efi variable" patch,
which is more experimental than bootefi/bootmgr rework.

-Takahiro Akashi

On Tue, 23 Apr 2019 at 13:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/23/19 2:24 AM, AKASHI, Takahiro wrote:
> > Heinrich,
> >
> > On Tue, 23 Apr 2019 at 03:16, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >>> There are several reasons that I want to rework/refactor bootefi command
> >>> as well as bootmgr:
> >>> * Some previous commits on bootefi.c have made the code complicated
> >>>    and a bit hard to understand.
> >>>
> >>> * do_bootefi_exec() would better be implemented using load_image() along
> >>>    with start_image() to be aligned with UEFI interfaces.
> >>>
> >>> * Contrary to the other part, efi_selftest part of the code is unusual
> >>>    in terms of loading/execution path in do_bootefi().
> >>>
> >>> * When we will support "secure boot" in the future, EFI Boot Manager
> >>>    is expected to be invoked as a standalone command without any arguments
> >>>    to mitigate security surfaces.
> >>>
> >>> In this patch set,
> >>> Patch#1 to #7 are preparatory patches for patch#8.
> >>> Patch#8 is a core part of reworking.
> >>> Patch#9 is for standalone boot manager.
> >>
> >> Hello Takahiro,
> >>
> >> your patches 1-8 are now (with some modifications) in the efi-2019-07
> >> branch. I have added some more patches.
> >>
> >> https://github.com/xypron2/u-boot/commits/efi-2019-07
> >>
> >> Travis CI testing was ok:
> >> https://travis-ci.org/xypron2/u-boot/builds/522947698
> >>
> >> I have tested on real hardware without error:
> >> * TinkerBoard (32 bit) boot via GRUB
> >> * Pine64 A64 LTS (64 bit) boot via iPXE and GRUB
> >> * Odroid C2 (64 bit) boot via GRUB
> >>
> >> If you are ok with the adjustments to your patch series I will create a
> >> pull request.
> >
> > Thank you for the merge.
> > One question: how did you fix a grub error that you reported before?
>
> I fixed the issue that iPXE could not find a chained script via
> efi_loader: correctly split device path of loaded image
> https://lists.denx.de/pipermail/u-boot/2019-April/365907.html
>
> Furthermore I fixed a bug in Debian's QEMU by applying Alex's
> https://github.com/qemu/qemu/commit/2d2a4549cc29850aab891495685a7b31f5254b12
>
> Best regards
>
> Heinrich
>
> >
> > -Takahiro Akashi
> >
> >
> >> Best regards
> >>
> >> Heinrich
> >
>

[PATCH 0/9] efi_loader: rework bootefi/bootmgr

[Heinrich Schuchardt]

On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> There are several reasons that I want to rework/refactor bootefi command
> as well as bootmgr:
> * Some previous commits on bootefi.c have made the code complicated
>    and a bit hard to understand.
>
> * do_bootefi_exec() would better be implemented using load_image() along
>    with start_image() to be aligned with UEFI interfaces.
>
> * Contrary to the other part, efi_selftest part of the code is unusual
>    in terms of loading/execution path in do_bootefi().
>
> * When we will support "secure boot" in the future, EFI Boot Manager
>    is expected to be invoked as a standalone command without any arguments
>    to mitigate security surfaces.
>
> In this patch set,
> Patch#1 to #7 are preparatory patches for patch#8.
> Patch#8 is a core part of reworking.
> Patch#9 is for standalone boot manager.
>
> # Please note that some patches, say patch#2 and #3, can be combined into one
> # but I intentionally keep them separated to clarify my intentions of changes.
>
> Issues:
> * It would be better off to change the semantics of efi_dp_from_name().
> no chance to free loaded_image_info->load_options. (see patch #8)
>
> -Takahiro Akashi

Hello Takahiro,

with the `efidebug boot add` command we can define load options for the
BootXXXX variables.

But in do_efibootmgr() we call do_bootefi_exec() which calls
set_load_options() and passes the value of environment variable bootargs
as load options or if the variable is not set an empty string.

Here is an example console output:

=> setenv bootargs This is a value from bootargs
=> efidebug boot add 0000 hello scsi 0:1 helloworld.efi 'This is a value
from efidebug'
=> efidebug boot order 0000
=> bootefi bootmgr
Booting: hello
Hello, world!
Running on UEFI 2.8
Have SMBIOS table
Have device tree
Load options: This is a value from bootargs
## Application terminated, r = 0

Now the same after deleting variable bootargs:

=> setenv bootargs
=> bootefi bootmgr
Booting: hello
Hello, world!
Running on UEFI 2.8
Have SMBIOS table
Have device tree
Load options: <none>
## Application terminated, r = 0
=>

What behavior would you expect:

a) if the boot option has a load options value,
b) if the boot option has no load options value?

One solution would be to define that bootargs is always ignored if the
boot manager is used.

Best regards

Heinrich

[PATCH 0/9] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

Heinrich,

On Fri, Jan 03, 2020 at 01:17:05AM +0100, Heinrich Schuchardt wrote:
> On 4/19/19 5:22 AM, AKASHI Takahiro wrote:
> >There are several reasons that I want to rework/refactor bootefi command
> >as well as bootmgr:
> >* Some previous commits on bootefi.c have made the code complicated
> >   and a bit hard to understand.
> >
> >* do_bootefi_exec() would better be implemented using load_image() along
> >   with start_image() to be aligned with UEFI interfaces.
> >
> >* Contrary to the other part, efi_selftest part of the code is unusual
> >   in terms of loading/execution path in do_bootefi().
> >
> >* When we will support "secure boot" in the future, EFI Boot Manager
> >   is expected to be invoked as a standalone command without any arguments
> >   to mitigate security surfaces.
> >
> >In this patch set,
> >Patch#1 to #7 are preparatory patches for patch#8.
> >Patch#8 is a core part of reworking.
> >Patch#9 is for standalone boot manager.
> >
> ># Please note that some patches, say patch#2 and #3, can be combined into one
> ># but I intentionally keep them separated to clarify my intentions of changes.
> >
> >Issues:
> >* It would be better off to change the semantics of efi_dp_from_name().
> >no chance to free loaded_image_info->load_options. (see patch #8)
> >
> >-Takahiro Akashi
> 
> Hello Takahiro,
> 
> with the `efidebug boot add` command we can define load options for the
> BootXXXX variables.
> 
> But in do_efibootmgr() we call do_bootefi_exec() which calls
> set_load_options() and passes the value of environment variable bootargs
> as load options or if the variable is not set an empty string.
> 
> Here is an example console output:
> 
> => setenv bootargs This is a value from bootargs
> => efidebug boot add 0000 hello scsi 0:1 helloworld.efi 'This is a value
> from efidebug'
> => efidebug boot order 0000
> => bootefi bootmgr
> Booting: hello
> Hello, world!
> Running on UEFI 2.8
> Have SMBIOS table
> Have device tree
> Load options: This is a value from bootargs
> ## Application terminated, r = 0
> 
> Now the same after deleting variable bootargs:
> 
> => setenv bootargs
> => bootefi bootmgr
> Booting: hello
> Hello, world!
> Running on UEFI 2.8
> Have SMBIOS table
> Have device tree
> Load options: <none>
> ## Application terminated, r = 0
> =>

Yeah, this is not what I intended.

> What behavior would you expect:
> 
> a) if the boot option has a load options value,
> b) if the boot option has no load options value?
> 
> One solution would be to define that bootargs is always ignored if the
> boot manager is used.

I agree.
Basically, "bootargs" is only for "bootefi <addr>" command, while
"BootXXXX" should work in the exact same way as the UEFI specification
defines.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich