[U-Boot] [RFC v2 00/11] efi_loader: rework bootefi/bootmgr

[U-Boot] [RFC v2 00/11] 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 unusal
  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 is a bug fix.
Patch#2 to #9 are preparatory patches for patch#10.
Patch#10 is a core part of reworking.
Patch#11 is for standalone boot manager.

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

Prerequsite patch:
* "efi_loader: bootmgr: support BootNext and BootCurrent variable behavior"
* "efi_loader: release file buffer after loading image"

Issues:
* load_image() will take an argument of "parent_handle," but obviously
  we don't have any parent when invoking an application from command line.
  (See FIXME in patch#10.)
* Starting EDK2's Shell.efi from boot manager fails.
  (I need to find out a fix.)
* The semantics of efi_dp_from_name() should be changed.
  (See FIXME in patch#10.)

-Takahiro Akashi

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 (11):
  efi_loader: boottime: add loaded image device path protocol to image
    handle
  efi_loader: boottime: export efi_[un]load_image()
  efi_loader: device_path: handle special case of loading
  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_boot_efi() from do_bootefi()
  efi_loader: rework bootmgr/bootefi using load_image API
  cmd: add efibootmgr command

 cmd/Kconfig                       |   8 +
 cmd/bootefi.c                     | 521 +++++++++++++++++++-----------
 include/efi_api.h                 |   4 +
 include/efi_loader.h              |  15 +-
 lib/efi_loader/efi_bootmgr.c      |  43 +--
 lib/efi_loader/efi_boottime.c     |  34 +-
 lib/efi_loader/efi_device_path.c  |   8 +
 lib/efi_loader/efi_image_loader.c |   2 +
 8 files changed, 411 insertions(+), 224 deletions(-)

-- 
2.20.1

[U-Boot] [RFC v2 01/11] efi_loader: boottime: add loaded image device path protocol to image handle

[AKASHI Takahiro]

To meet UEFI spec v2.7a section 9.2, we should add
EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
instead of EFI_DEVICE_PATH_PROTOCOL.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_api.h                 |  4 ++++
 include/efi_loader.h              |  1 +
 lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
 lib/efi_loader/efi_image_loader.c |  2 ++
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/include/efi_api.h b/include/efi_api.h
index ccf608653d4a..63b703c951a7 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -333,6 +333,10 @@ struct efi_system_table {
 	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
 		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
 
+#define LOADED_IMAGE_DEVICE_PATH_GUID \
+	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
+		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
+
 #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
 
 struct efi_loaded_image {
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 512880ab8fbf..3f54e354bdcd 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system;
 /* GUID of the device tree table */
 extern const efi_guid_t efi_guid_fdt;
 extern const efi_guid_t efi_guid_loaded_image;
+extern const efi_guid_t efi_guid_loaded_image_device_path;
 extern const efi_guid_t efi_guid_device_path_to_text_protocol;
 extern const efi_guid_t efi_simple_file_system_protocol_guid;
 extern const efi_guid_t efi_file_info_guid;
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 391681260c27..578b32a05ffd 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 	efi_status_t ret;
 	struct efi_loaded_image *info = NULL;
 	struct efi_loaded_image_obj *obj = NULL;
+	struct efi_device_path *dp;
 
 	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
 	*handle_ptr = NULL;
@@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 
 	if (device_path) {
 		info->device_handle = efi_dp_find_obj(device_path, NULL);
-		/*
-		 * When asking for the device path interface, return
-		 * bootefi_device_path
-		 */
-		ret = efi_add_protocol(&obj->header,
-				       &efi_guid_device_path, device_path);
-		if (ret != EFI_SUCCESS)
+
+		dp = efi_dp_append(device_path, file_path);
+		if (!dp) {
+			ret = EFI_OUT_OF_RESOURCES;
 			goto failure;
+		}
+	} else {
+		dp = NULL;
 	}
+	ret = efi_add_protocol(&obj->header,
+			       &efi_guid_loaded_image_device_path, dp);
+	if (ret != EFI_SUCCESS)
+		goto failure;
 
 	/*
 	 * When asking for the loaded_image interface, just
diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
index fe66e7b9ffe1..c6177b9ff832 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -14,6 +14,8 @@
 const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
 const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
 const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
+const efi_guid_t efi_guid_loaded_image_device_path
+		= LOADED_IMAGE_DEVICE_PATH_GUID;
 const efi_guid_t efi_simple_file_system_protocol_guid =
 		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
 const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
-- 
2.20.1

[U-Boot] [RFC v2 02/11] efi_loader: boottime: export efi_[un]load_image()

[AKASHI Takahiro]

This is a preparatory patch.
Those two functions will be used later for reworking do_bootefi().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 include/efi_loader.h          |  9 +++++++++
 lib/efi_loader/efi_boottime.c | 14 +++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 3f54e354bdcd..6dfa2fef3cf0 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -321,10 +321,19 @@ efi_status_t efi_create_handle(efi_handle_t *handle);
 void efi_delete_handle(efi_handle_t obj);
 /* Call this to validate a handle and find the EFI object for it */
 struct efi_object *efi_search_obj(const efi_handle_t handle);
+/* Load image */
+efi_status_t EFIAPI efi_load_image(bool boot_policy,
+				   efi_handle_t parent_image,
+				   struct efi_device_path *file_path,
+				   void *source_buffer,
+				   efi_uintn_t source_size,
+				   efi_handle_t *image_handle);
 /* Start image */
 efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 				    efi_uintn_t *exit_data_size,
 				    u16 **exit_data);
+/* Unload image */
+efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle);
 /* Find a protocol on a handle */
 efi_status_t efi_search_protocol(const efi_handle_t handle,
 				 const efi_guid_t *protocol_guid,
diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index 578b32a05ffd..74da6b01054a 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1686,12 +1686,12 @@ error:
  *
  * Return: status code
  */
-static efi_status_t EFIAPI efi_load_image(bool boot_policy,
-					  efi_handle_t parent_image,
-					  struct efi_device_path *file_path,
-					  void *source_buffer,
-					  efi_uintn_t source_size,
-					  efi_handle_t *image_handle)
+efi_status_t EFIAPI efi_load_image(bool boot_policy,
+				   efi_handle_t parent_image,
+				   struct efi_device_path *file_path,
+				   void *source_buffer,
+				   efi_uintn_t source_size,
+				   efi_handle_t *image_handle)
 {
 	struct efi_device_path *dp, *fp;
 	struct efi_loaded_image *info = NULL;
@@ -1871,7 +1871,7 @@ static efi_status_t EFIAPI efi_exit(efi_handle_t image_handle,
  *
  * Return: status code
  */
-static efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
+efi_status_t EFIAPI efi_unload_image(efi_handle_t image_handle)
 {
 	struct efi_object *efiobj;
 
-- 
2.20.1

[U-Boot] [RFC v2 03/11] efi_loader: device_path: handle special case of loading

[AKASHI Takahiro]

This is a preparatory patch.

efi_dp_split_file_path() is used to create device_path and file_path
from file_path for efi_setup_loaded_image().
In a special case, however, of HARDWARE_DEVICE/MEMORY, it doesn't
work expectedly since this path doesn't contain any FILE_PATH sub-type.

This patch makes a workaround.

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

diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index 53b40c8c3c2d..e283fad767ed 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -933,6 +933,14 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
 	dp = efi_dp_dup(full_path);
 	if (!dp)
 		return EFI_OUT_OF_RESOURCES;
+
+	if (EFI_DP_TYPE(dp, HARDWARE_DEVICE, MEMORY)) {
+		/* no FILE_PATH */
+		*device_path = dp;
+
+		return EFI_SUCCESS;
+	}
+
 	p = dp;
 	while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
 		p = efi_dp_next(p);
-- 
2.20.1

[U-Boot] [RFC v2 04/11] cmd: bootefi: carve out fdt handling from do_bootefi()

[AKASHI Takahiro]

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 3619a20e6433..aba8b203d419 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -198,6 +198,40 @@ 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:	CMD_RET_SUCCESS on success,
+		CMD_RET_USAGE or CMD_RET_FAILURE otherwise
+ *
+ * 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);
+		printf("WARNING: booting without device tree\n");
+	}
+
+	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,21 +441,10 @@ 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 = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
+	if (r != EFI_SUCCESS)
+		return r;
+
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-- 
2.20.1

[U-Boot] [RFC v2 05/11] 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 aba8b203d419..ce0db07f8633 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -165,41 +165,8 @@ 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:	CMD_RET_SUCCESS on success,
 		CMD_RET_USAGE or CMD_RET_FAILURE otherwise
@@ -207,21 +174,43 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
  * 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 */
@@ -441,7 +430,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
-	r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
+	r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
 	if (r != EFI_SUCCESS)
 		return r;
 
-- 
2.20.1

[U-Boot] [RFC v2 06/11] 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, 90 insertions(+), 55 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index ce0db07f8633..1267d895472c 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -221,39 +221,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 */
-	set_load_options(*loaded_image_infop, load_options_path);
-
-	return 0;
-}
-
-/**
- * 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
  *
@@ -300,11 +267,14 @@ 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 */
+	set_load_options(loaded_image_info, "bootargs");
+
 	/* Load the EFI payload */
 	ret = efi_load_pe(image_obj, efi, loaded_image_info);
 	if (ret != EFI_SUCCESS)
@@ -328,7 +298,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)
@@ -338,6 +310,25 @@ 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 */
+	set_load_options(*loaded_image_infop, load_options_path);
+
+	return 0;
+}
+
 /**
  * bootefi_test_prepare() - prepare to run an EFI test
  *
@@ -383,6 +374,62 @@ 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_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_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_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)
@@ -414,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	efi_status_t r;
 	unsigned long fdt_addr;
 
+	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();
 
@@ -427,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
 	if (r != EFI_SUCCESS)
 		return r;
@@ -445,22 +496,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] [RFC v2 07/11] 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>
---
 cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 1267d895472c..e9d4881997a1 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -309,6 +309,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,
@@ -432,27 +453,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] [RFC v2 08/11] 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 | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index e9d4881997a1..94b5bdeed3f1 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -309,22 +309,47 @@ 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_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;
@@ -463,6 +488,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);
@@ -497,9 +525,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] [RFC v2 09/11] cmd: bootefi: carve out do_boot_efi() 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_boot_efi().

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 94b5bdeed3f1..39a0712af6ad 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt)
 	return 0;
 }
 
+/*
+ * do_boot_efi() - 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_boot_efi(const char *image_opt, const char *fdt_opt)
+{
+	unsigned long addr;
+	char *saddr;
+	efi_status_t ret;
+	unsigned long fdt_addr;
+
+	/* 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_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);
+	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,
@@ -481,11 +548,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;
-	unsigned long fdt_addr;
-
 	if (argc < 2)
 		return CMD_RET_USAGE;
 
@@ -496,55 +558,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
-	if (r != EFI_SUCCESS)
-		return r;
-
-#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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
 }
 
 #ifdef CONFIG_SYS_LONGHELP
-- 
2.20.1

[U-Boot] [RFC v2 10/11] 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                 | 199 +++++++++++++++++++---------------
 include/efi_loader.h          |   5 +-
 lib/efi_loader/efi_bootmgr.c  |  43 ++++----
 lib/efi_loader/efi_boottime.c |   1 +
 4 files changed, 138 insertions(+), 110 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 39a0712af6ad..17dccd718008 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -224,88 +224,43 @@ 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;
+	efi_status_t ret;
 
 	/* Transfer environment variable as load options */
-	set_load_options(loaded_image_info, "bootargs");
-
-	/* Load the EFI payload */
-	ret = efi_load_pe(image_obj, efi, loaded_image_info);
+	ret = EFI_CALL(systab.boottime->open_protocol(
+					handle,
+					&efi_guid_loaded_image,
+					(void **)&loaded_image_info,
+					NULL, NULL,
+					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
 	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;
-	}
+		goto err;
+	set_load_options(loaded_image_info, "bootargs");
 
 	/* we don't support much: */
 	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
 		"{ro,boot}(blob)0000000000000000");
 
 	/* Call our payload! */
+	image_obj = container_of(handle, struct efi_loaded_image_obj, header);
 	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);
+	efi_free_pool(loaded_image_info);
 
+err:
 	return ret;
 }
 
@@ -319,8 +274,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 */
@@ -340,19 +294,21 @@ static int do_efibootmgr(const char *fdt_opt)
 	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);
+
+	EFI_CALL(efi_unload_image(handle));
 
 	if (ret != EFI_SUCCESS)
-		return 1;
+		return CMD_RET_FAILURE;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 /*
@@ -367,10 +323,14 @@ static int do_efibootmgr(const char *fdt_opt)
  */
 static int do_boot_efi(const char *image_opt, const char *fdt_opt)
 {
-	unsigned long addr;
-	char *saddr;
+	void *image_buf;
+	struct efi_device_path *device_path, *image_path;
+	struct efi_device_path *memdp = NULL, *file_path = NULL;
+	const char *saddr;
+	unsigned long addr, size;
+	const char *size_str;
+	efi_handle_t mem_handle = NULL, handle;
 	efi_status_t ret;
-	unsigned long fdt_addr;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -390,36 +350,96 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
 		return CMD_RET_FAILURE;
 
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
-	if (!strcmp(argv[1], "hello")) {
-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-
+	if (!strcmp(image_opt, "hello")) {
 		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
 	{
-		saddr = argv[1];
+		saddr = image_opt;
+		size_str = env_get("filesize");
+		if (size_str)
+			size = simple_strtoul(size_str, NULL, 16);
+		else
+			size = 0;
 
 		addr = simple_strtoul(saddr, NULL, 16);
 		/* Check that a numeric value was passed */
 		if (!addr && *saddr != '0')
 			return CMD_RET_USAGE;
+
+		image_buf = map_sysmem(addr, size);
+
+		device_path = bootefi_device_path;
+		image_path = bootefi_image_path;
+	}
+
+	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:
+		 */
+		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
+
+		/* actual addresses filled in after efi_load_image() */
+		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					(uint64_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, (void *)0x1 /* FIXME */,
+				      file_path, image_buf, size, &handle));
+	if (ret != EFI_SUCCESS)
+		goto err_prepare;
+
 	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);
+	ret = do_bootefi_exec(handle);
+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
+
+	EFI_CALL(efi_unload_image(handle));
+err_prepare:
+	if (device_path)
+		efi_free_pool(file_path);
+
+err_add_protocol:
+	if (mem_handle)
+		efi_delete_handle(mem_handle);
+	if (memdp)
+		efi_free_pool(memdp);
 
 	if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
-	else
-		return CMD_RET_SUCCESS;
+
+	return CMD_RET_SUCCESS;
 }
 
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
@@ -577,7 +597,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"
@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -410,8 +410,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);
 
@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1610,6 +1610,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)
 {
-- 
2.20.1

[U-Boot] [RFC v2 11/11] 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 4bcc5c45579d..aa27b21956b3 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 17dccd718008..2a2ff4034831 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -635,3 +635,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] [RFC v2 01/11] efi_loader: boottime: add loaded image device path protocol to image handle

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> To meet UEFI spec v2.7a section 9.2, we should add
> EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
> instead of EFI_DEVICE_PATH_PROTOCOL.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_api.h                 |  4 ++++
>  include/efi_loader.h              |  1 +
>  lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
>  lib/efi_loader/efi_image_loader.c |  2 ++
>  4 files changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index ccf608653d4a..63b703c951a7 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -333,6 +333,10 @@ struct efi_system_table {
>  	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
>  		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
>
> +#define LOADED_IMAGE_DEVICE_PATH_GUID \
> +	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
> +		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
> +
>  #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
>
>  struct efi_loaded_image {
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 512880ab8fbf..3f54e354bdcd 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system;
>  /* GUID of the device tree table */
>  extern const efi_guid_t efi_guid_fdt;
>  extern const efi_guid_t efi_guid_loaded_image;
> +extern const efi_guid_t efi_guid_loaded_image_device_path;
>  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
>  extern const efi_guid_t efi_simple_file_system_protocol_guid;
>  extern const efi_guid_t efi_file_info_guid;
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index 391681260c27..578b32a05ffd 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>  	efi_status_t ret;
>  	struct efi_loaded_image *info = NULL;
>  	struct efi_loaded_image_obj *obj = NULL;
> +	struct efi_device_path *dp;
>
>  	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
>  	*handle_ptr = NULL;
> @@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>
>  	if (device_path) {
>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> -		/*
> -		 * When asking for the device path interface, return
> -		 * bootefi_device_path
> -		 */
> -		ret = efi_add_protocol(&obj->header,
> -				       &efi_guid_device_path, device_path);
> -		if (ret != EFI_SUCCESS)
> +
> +		dp = efi_dp_append(device_path, file_path);

I think we should pass the original device_path as a parameter to
efi_setup_loaded_image instead of first splitting and then recombining
devicepath elements.

But let's leave this as a TODO after the rest of the refactoring.

> +		if (!dp) {
> +			ret = EFI_OUT_OF_RESOURCES;
>  			goto failure;
> +		}
> +	} else {
> +		dp = NULL;
>  	}
> +	ret = efi_add_protocol(&obj->header,
> +			       &efi_guid_loaded_image_device_path, dp);

I wondered about the loaded imaged device path protocol possibly being
NULL. But the spec explicitely states:

It is legal to call LoadImage() for a buffer in memory with a NULL
DevicePath parameter. In this case, the Loaded Image Device Path
Protocol is installed with a NULL interface pointer.

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

> +	if (ret != EFI_SUCCESS)
> +		goto failure;
>
>  	/*
>  	 * When asking for the loaded_image interface, just
> diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> index fe66e7b9ffe1..c6177b9ff832 100644
> --- a/lib/efi_loader/efi_image_loader.c
> +++ b/lib/efi_loader/efi_image_loader.c
> @@ -14,6 +14,8 @@
>  const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
>  const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
>  const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
> +const efi_guid_t efi_guid_loaded_image_device_path
> +		= LOADED_IMAGE_DEVICE_PATH_GUID;
>  const efi_guid_t efi_simple_file_system_protocol_guid =
>  		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
>  const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
>

[U-Boot] [RFC v2 03/11] efi_loader: device_path: handle special case of loading

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> This is a preparatory patch.
>
> efi_dp_split_file_path() is used to create device_path and file_path
> from file_path for efi_setup_loaded_image().
> In a special case, however, of HARDWARE_DEVICE/MEMORY, it doesn't
> work expectedly since this path doesn't contain any FILE_PATH sub-type.

As already mentioned in a comment to patch 1/11 I would prefer that we
pass the original device path to efi_setup_loaded_image() instead of
recombining device path and file path there again.

This would avoid special treatment here with possible side effects in
efi_fs_from_path().

Best regards

Heinrich

>
> This patch makes a workaround.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_device_path.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 53b40c8c3c2d..e283fad767ed 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -933,6 +933,14 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
>  	dp = efi_dp_dup(full_path);
>  	if (!dp)
>  		return EFI_OUT_OF_RESOURCES;
> +
> +	if (EFI_DP_TYPE(dp, HARDWARE_DEVICE, MEMORY)) {
> +		/* no FILE_PATH */
> +		*device_path = dp;
> +
> +		return EFI_SUCCESS;
> +	}
> +
>  	p = dp;
>  	while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
>  		p = efi_dp_next(p);
>

[U-Boot] [RFC v2 04/11] cmd: bootefi: carve out fdt handling from do_bootefi()

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 15 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 3619a20e6433..aba8b203d419 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -198,6 +198,40 @@ 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:	CMD_RET_SUCCESS on success,
> +		CMD_RET_USAGE or CMD_RET_FAILURE otherwise

This does not match the actual return values.

> + *
> + * 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);
> +		printf("WARNING: booting without device tree\n");

As x86 does not require a device tree, please, remove this message.

> +	}
> +
> +	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,21 +441,10 @@ 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 = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> +	if (r != EFI_SUCCESS)
> +		return r;

Now you return EFI_INVALID_PARAMETER instead of CMD_RET_USAGE.

Best regards

Heinrich

> +
>  #ifdef CONFIG_CMD_BOOTEFI_HELLO
>  	if (!strcmp(argv[1], "hello")) {
>  		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>

[U-Boot] [RFC v2 05/11] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[Heinrich Schuchardt]

On 3/27/19 5:40 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 aba8b203d419..ce0db07f8633 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -165,41 +165,8 @@ 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:	CMD_RET_SUCCESS on success,
>  		CMD_RET_USAGE or CMD_RET_FAILURE otherwise
> @@ -207,21 +174,43 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
>   * 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 */
> @@ -441,7 +430,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	if (argc < 2)
>  		return CMD_RET_USAGE;
>
> -	r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> +	r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
>  	if (r != EFI_SUCCESS)
>  		return r;

We want to return CMD_RET_* from do_bootefi() and not EFI_*.

Best regards

Heinrich

>
>

[U-Boot] [RFC v2 06/11] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[Heinrich Schuchardt]

On 3/27/19 5:40 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>
> ---
>  cmd/bootefi.c | 145 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 90 insertions(+), 55 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index ce0db07f8633..1267d895472c 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -221,39 +221,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 */
> -	set_load_options(*loaded_image_infop, load_options_path);
> -
> -	return 0;
> -}
> -
> -/**
> - * 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
>   *
> @@ -300,11 +267,14 @@ 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 */
> +	set_load_options(loaded_image_info, "bootargs");
> +
>  	/* Load the EFI payload */
>  	ret = efi_load_pe(image_obj, efi, loaded_image_info);
>  	if (ret != EFI_SUCCESS)
> @@ -328,7 +298,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);

Deletion of the image and the handle should be handled by Exit() or
UnloadImage(). But maybe we should leave that to correction to a later
patch. Please, add a TODO comment here.

>
>  err_add_protocol:
>  	if (mem_handle)
> @@ -338,6 +310,25 @@ 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 */
> +	set_load_options(*loaded_image_infop, load_options_path);
> +
> +	return 0;
> +}
> +
>  /**
>   * bootefi_test_prepare() - prepare to run an EFI test
>   *
> @@ -383,6 +374,62 @@ 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_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_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();

This looks like code duplication to me.

I would prefer do_bootefi() to call efi_init_obj_list() before
evaluating arguments.

Best regards

Heinrich

> +	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_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)
> @@ -414,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  	efi_status_t r;
>  	unsigned long fdt_addr;
>
> +	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();
>
> @@ -427,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
>  	if (r != EFI_SUCCESS)
>  		return r;
> @@ -445,22 +496,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();
>

[U-Boot] [RFC v2 07/11] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.

I would suggest as commit message:

Move do_bootefi_bootmgr_exec() up in code to avoid a forward declaration
in a later patch.

Otherwise

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

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 1267d895472c..e9d4881997a1 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -309,6 +309,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,
> @@ -432,27 +453,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[])
>  {
>

[U-Boot] [RFC v2 11/11] cmd: add efibootmgr command

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
> With this patch, EFI boot manager can be kicked in by a standalone
> command, efibootmgr.

I miss your comment form 0/11 here:

 * 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.

Simply requiring that environment variable $fdtaddr, which is under user
control, is used for loading the device tree does not offer any security
at all.

For secure boot I would expect that the device tree has to be part of a
signed binary and that that signature is checked.

If the user passes in a device tree that should be okay if it has the
required signature.

>
> 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 4bcc5c45579d..aa27b21956b3 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 17dccd718008..2a2ff4034831 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -635,3 +635,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;

It is unclear why you do not allow the user to pass the location of the
device tree as a parameter.

> +
> +	fdt_opt = env_get("fdtaddr");

You are looking at some environment variable $fdtaddr while most boards
use $fdt_addr_r as preferred address to load the device tree.

Best regards

Heinrich

> +	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] [RFC v2 01/11] efi_loader: boottime: add loaded image device path protocol to image handle

[Heinrich Schuchardt]

On 3/27/19 6:58 AM, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
>> To meet UEFI spec v2.7a section 9.2, we should add
>> EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
>> instead of EFI_DEVICE_PATH_PROTOCOL.
>>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---

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

Added to for efi-2019-07 branch.

[U-Boot] [RFC v2 02/11] efi_loader: boottime: export efi_[un]load_image()

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> This is a preparatory patch.
> Those two functions will be used later for reworking do_bootefi().
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Added to for efi-2019-07 branch.

[U-Boot] [RFC v2 01/11] efi_loader: boottime: add loaded image device path protocol to image handle

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 06:58:31AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> > To meet UEFI spec v2.7a section 9.2, we should add
> > EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL to image handle,
> > instead of EFI_DEVICE_PATH_PROTOCOL.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_api.h                 |  4 ++++
> >  include/efi_loader.h              |  1 +
> >  lib/efi_loader/efi_boottime.c     | 19 ++++++++++++-------
> >  lib/efi_loader/efi_image_loader.c |  2 ++
> >  4 files changed, 19 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/efi_api.h b/include/efi_api.h
> > index ccf608653d4a..63b703c951a7 100644
> > --- a/include/efi_api.h
> > +++ b/include/efi_api.h
> > @@ -333,6 +333,10 @@ struct efi_system_table {
> >  	EFI_GUID(0x5b1b31a1, 0x9562, 0x11d2, \
> >  		 0x8e, 0x3f, 0x00, 0xa0, 0xc9, 0x69, 0x72, 0x3b)
> >
> > +#define LOADED_IMAGE_DEVICE_PATH_GUID \
> > +	EFI_GUID(0xbc62157e, 0x3e33, 0x4fec, \
> > +		 0x99, 0x20, 0x2d, 0x3b, 0x36, 0xd7, 0x50, 0xdf)
> > +
> >  #define EFI_LOADED_IMAGE_PROTOCOL_REVISION 0x1000
> >
> >  struct efi_loaded_image {
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 512880ab8fbf..3f54e354bdcd 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -135,6 +135,7 @@ extern const efi_guid_t efi_guid_event_group_reset_system;
> >  /* GUID of the device tree table */
> >  extern const efi_guid_t efi_guid_fdt;
> >  extern const efi_guid_t efi_guid_loaded_image;
> > +extern const efi_guid_t efi_guid_loaded_image_device_path;
> >  extern const efi_guid_t efi_guid_device_path_to_text_protocol;
> >  extern const efi_guid_t efi_simple_file_system_protocol_guid;
> >  extern const efi_guid_t efi_file_info_guid;
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index 391681260c27..578b32a05ffd 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1519,6 +1519,7 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >  	efi_status_t ret;
> >  	struct efi_loaded_image *info = NULL;
> >  	struct efi_loaded_image_obj *obj = NULL;
> > +	struct efi_device_path *dp;
> >
> >  	/* In case of EFI_OUT_OF_RESOURCES avoid illegal free by caller. */
> >  	*handle_ptr = NULL;
> > @@ -1542,15 +1543,19 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >
> >  	if (device_path) {
> >  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> > -		/*
> > -		 * When asking for the device path interface, return
> > -		 * bootefi_device_path
> > -		 */
> > -		ret = efi_add_protocol(&obj->header,
> > -				       &efi_guid_device_path, device_path);
> > -		if (ret != EFI_SUCCESS)
> > +
> > +		dp = efi_dp_append(device_path, file_path);
> 
> I think we should pass the original device_path as a parameter to
> efi_setup_loaded_image instead of first splitting and then recombining
> devicepath elements.

Agree, but I didn't modify the code because efi_setup_loaded_image()
is still used to load "efi_selftest" in bootefi.

I hope that you will work on do_efi_selftest().

> But let's leave this as a TODO after the rest of the refactoring.
> 
> > +		if (!dp) {
> > +			ret = EFI_OUT_OF_RESOURCES;
> >  			goto failure;
> > +		}
> > +	} else {
> > +		dp = NULL;
> >  	}
> > +	ret = efi_add_protocol(&obj->header,
> > +			       &efi_guid_loaded_image_device_path, dp);
> 
> I wondered about the loaded imaged device path protocol possibly being
> NULL. But the spec explicitely states:
> 
> It is legal to call LoadImage() for a buffer in memory with a NULL
> DevicePath parameter. In this case, the Loaded Image Device Path
> Protocol is installed with a NULL interface pointer.

Right, I have also found this statement.

-Takahiro Akashi

> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> > +	if (ret != EFI_SUCCESS)
> > +		goto failure;
> >
> >  	/*
> >  	 * When asking for the loaded_image interface, just
> > diff --git a/lib/efi_loader/efi_image_loader.c b/lib/efi_loader/efi_image_loader.c
> > index fe66e7b9ffe1..c6177b9ff832 100644
> > --- a/lib/efi_loader/efi_image_loader.c
> > +++ b/lib/efi_loader/efi_image_loader.c
> > @@ -14,6 +14,8 @@
> >  const efi_guid_t efi_global_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
> >  const efi_guid_t efi_guid_device_path = DEVICE_PATH_GUID;
> >  const efi_guid_t efi_guid_loaded_image = LOADED_IMAGE_GUID;
> > +const efi_guid_t efi_guid_loaded_image_device_path
> > +		= LOADED_IMAGE_DEVICE_PATH_GUID;
> >  const efi_guid_t efi_simple_file_system_protocol_guid =
> >  		EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_GUID;
> >  const efi_guid_t efi_file_info_guid = EFI_FILE_INFO_GUID;
> >
> 

[U-Boot] [RFC v2 03/11] efi_loader: device_path: handle special case of loading

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 07:17:02AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> > This is a preparatory patch.
> >
> > efi_dp_split_file_path() is used to create device_path and file_path
> > from file_path for efi_setup_loaded_image().
> > In a special case, however, of HARDWARE_DEVICE/MEMORY, it doesn't
> > work expectedly since this path doesn't contain any FILE_PATH sub-type.
> 
> As already mentioned in a comment to patch 1/11 I would prefer that we
> pass the original device path to efi_setup_loaded_image() instead of
> recombining device path and file path there again.

This change would only come, as I said, after do_efi_selftest() is
also refactored.

Thanks,
-Takahiro Akashi

> This would avoid special treatment here with possible side effects in
> efi_fs_from_path().
> 
> Best regards
> 
> Heinrich
> 
> >
> > This patch makes a workaround.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_device_path.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index 53b40c8c3c2d..e283fad767ed 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -933,6 +933,14 @@ efi_status_t efi_dp_split_file_path(struct efi_device_path *full_path,
> >  	dp = efi_dp_dup(full_path);
> >  	if (!dp)
> >  		return EFI_OUT_OF_RESOURCES;
> > +
> > +	if (EFI_DP_TYPE(dp, HARDWARE_DEVICE, MEMORY)) {
> > +		/* no FILE_PATH */
> > +		*device_path = dp;
> > +
> > +		return EFI_SUCCESS;
> > +	}
> > +
> >  	p = dp;
> >  	while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
> >  		p = efi_dp_next(p);
> >
> 
> 
> 
> 

[U-Boot] [RFC v2 04/11] cmd: bootefi: carve out fdt handling from do_bootefi()

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 07:29:50AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> > This is a preparatory patch for reworking do_bootefi() in later patch.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c | 53 ++++++++++++++++++++++++++++++++++++---------------
> >  1 file changed, 38 insertions(+), 15 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 3619a20e6433..aba8b203d419 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -198,6 +198,40 @@ 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:	CMD_RET_SUCCESS on success,
> > +		CMD_RET_USAGE or CMD_RET_FAILURE otherwise
> 
> This does not match the actual return values.

Oops, re-organizing my patch-set introduced this obvious bug.

> > + *
> > + * 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);
> > +		printf("WARNING: booting without device tree\n");
> 
> As x86 does not require a device tree, please, remove this message.

Okay, but I didn't change this message from the original :)

> > +	}
> > +
> > +	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,21 +441,10 @@ 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 = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> > +	if (r != EFI_SUCCESS)
> > +		return r;
> 
> Now you return EFI_INVALID_PARAMETER instead of CMD_RET_USAGE.

Okay, will fix.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +
> >  #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >  	if (!strcmp(argv[1], "hello")) {
> >  		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >
> 

[U-Boot] [RFC v2 05/11] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 07:33:49AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 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 aba8b203d419..ce0db07f8633 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -165,41 +165,8 @@ 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:	CMD_RET_SUCCESS on success,
> >  		CMD_RET_USAGE or CMD_RET_FAILURE otherwise
> > @@ -207,21 +174,43 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
> >   * 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 */
> > @@ -441,7 +430,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >
> > -	r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> > +	r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> >  	if (r != EFI_SUCCESS)
> >  		return r;
> 
> We want to return CMD_RET_* from do_bootefi() and not EFI_*.

Right, will fix.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >
> 

[U-Boot] [RFC v2 06/11] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 07:45:36AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 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>
> > ---
> >  cmd/bootefi.c | 145 +++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 90 insertions(+), 55 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index ce0db07f8633..1267d895472c 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -221,39 +221,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 */
> > -	set_load_options(*loaded_image_infop, load_options_path);
> > -
> > -	return 0;
> > -}
> > -
> > -/**
> > - * 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
> >   *
> > @@ -300,11 +267,14 @@ 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 */
> > +	set_load_options(loaded_image_info, "bootargs");
> > +
> >  	/* Load the EFI payload */
> >  	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >  	if (ret != EFI_SUCCESS)
> > @@ -328,7 +298,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);
> 
> Deletion of the image and the handle should be handled by Exit() or
> UnloadImage(). But maybe we should leave that to correction to a later
> patch. Please, add a TODO comment here.

I don't think we need a comment here.
Those code will be eventually replaced by efi_unload_image() in patch#10.

> >
> >  err_add_protocol:
> >  	if (mem_handle)
> > @@ -338,6 +310,25 @@ 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 */
> > +	set_load_options(*loaded_image_infop, load_options_path);
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * bootefi_test_prepare() - prepare to run an EFI test
> >   *
> > @@ -383,6 +374,62 @@ 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_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_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();
> 
> This looks like code duplication to me.

Another "carve out" patch?

> I would prefer do_bootefi() to call efi_init_obj_list() before
> evaluating arguments.

Well, we won't do that so as to make do_efibootmgr() self-contained
in a later patch.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +	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_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)
> > @@ -414,6 +461,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  	efi_status_t r;
> >  	unsigned long fdt_addr;
> >
> > +	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();
> >
> > @@ -427,9 +481,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> >  	if (r != EFI_SUCCESS)
> >  		return r;
> > @@ -445,22 +496,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();
> >
> 

[U-Boot] [RFC v2 07/11] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[AKASHI Takahiro]

On Wed, Mar 27, 2019 at 07:50:05AM +0100, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> > This is a preparatory patch for reworking do_bootefi() in later patch.
> 
> I would suggest as commit message:
> 
> Move do_bootefi_bootmgr_exec() up in code to avoid a forward declaration
> in a later patch.

No. This "move" is just from a historical reason :)
I just want the main code to be placed first, and exceptional code
(i.e. efi_selftest) to follow it.

-Takahiro Akashi

> Otherwise
> 
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
> >  1 file changed, 21 insertions(+), 21 deletions(-)
> >
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index 1267d895472c..e9d4881997a1 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -309,6 +309,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,
> > @@ -432,27 +453,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[])
> >  {
> >
> 

[U-Boot] [RFC v2 11/11] cmd: add efibootmgr command

[Alexander Graf]

On 27.03.19 14:10, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
>> Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
>> With this patch, EFI boot manager can be kicked in by a standalone
>> command, efibootmgr.
> I miss your comment form 0/11 here:
>
>  * 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.
>
> Simply requiring that environment variable $fdtaddr, which is under user
> control, is used for loading the device tree does not offer any security
> at all.
>
> For secure boot I would expect that the device tree has to be part of a
> signed binary and that that signature is checked.


For secure boot, the device tree must not come from the user, correct.
However, for secure boot, the user must never get access to the U-Boot
shell either, as that would allow access to arbitrary memory
modification commands. So in a way, we can assume that $fdtaddr is
trusted I guess?

At the same time, it means that the boot command (and thus environment)
is trusted too, so there is no need to prohibit passing a DT location, no?

Sorry for mentioning this so late :)


>
> If the user passes in a device tree that should be okay if it has the
> required signature.


How would you do a signature check on the DT?


Alex


>
>> 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 4bcc5c45579d..aa27b21956b3 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 17dccd718008..2a2ff4034831 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -635,3 +635,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;
> It is unclear why you do not allow the user to pass the location of the
> device tree as a parameter.
>
>> +
>> +	fdt_opt = env_get("fdtaddr");
> You are looking at some environment variable $fdtaddr while most boards
> use $fdt_addr_r as preferred address to load the device tree.
>
> Best regards
>
> Heinrich
>
>> +	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] [RFC v2 00/11] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

Heinrich,

Can you kindly review this series of patches, in particular, patch#10?
I would like to push it for v2019.07.

-Takahiro Akashi

On Wed, Mar 27, 2019 at 01:40:31PM +0900, 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 unusal
>   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 is a bug fix.
> Patch#2 to #9 are preparatory patches for patch#10.
> Patch#10 is a core part of reworking.
> Patch#11 is for standalone boot manager.
> 
> # Please note that some patches, say patch#4 and #5, can be combined into one
> # but I intentionally keep them separated to clearify my intentions of changes.
> 
> Prerequsite patch:
> * "efi_loader: bootmgr: support BootNext and BootCurrent variable behavior"
> * "efi_loader: release file buffer after loading image"
> 
> Issues:
> * load_image() will take an argument of "parent_handle," but obviously
>   we don't have any parent when invoking an application from command line.
>   (See FIXME in patch#10.)
> * Starting EDK2's Shell.efi from boot manager fails.
>   (I need to find out a fix.)
> * The semantics of efi_dp_from_name() should be changed.
>   (See FIXME in patch#10.)
> 
> -Takahiro Akashi
> 
> 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 (11):
>   efi_loader: boottime: add loaded image device path protocol to image
>     handle
>   efi_loader: boottime: export efi_[un]load_image()
>   efi_loader: device_path: handle special case of loading
>   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_boot_efi() from do_bootefi()
>   efi_loader: rework bootmgr/bootefi using load_image API
>   cmd: add efibootmgr command
> 
>  cmd/Kconfig                       |   8 +
>  cmd/bootefi.c                     | 521 +++++++++++++++++++-----------
>  include/efi_api.h                 |   4 +
>  include/efi_loader.h              |  15 +-
>  lib/efi_loader/efi_bootmgr.c      |  43 +--
>  lib/efi_loader/efi_boottime.c     |  34 +-
>  lib/efi_loader/efi_device_path.c  |   8 +
>  lib/efi_loader/efi_image_loader.c |   2 +
>  8 files changed, 411 insertions(+), 224 deletions(-)
> 
> -- 
> 2.20.1
> 

[U-Boot] [RFC v2 10/11] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 3/27/19 5:40 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                 | 199 +++++++++++++++++++---------------
>   include/efi_loader.h          |   5 +-
>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>   lib/efi_loader/efi_boottime.c |   1 +
>   4 files changed, 138 insertions(+), 110 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 39a0712af6ad..17dccd718008 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -224,88 +224,43 @@ 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;
> +	efi_status_t ret;
>
>   	/* Transfer environment variable as load options */
> -	set_load_options(loaded_image_info, "bootargs");
> -
> -	/* Load the EFI payload */
> -	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> +	ret = EFI_CALL(systab.boottime->open_protocol(
> +					handle,
> +					&efi_guid_loaded_image,
> +					(void **)&loaded_image_info,
> +					NULL, NULL,
> +					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>   	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;
> -	}
> +		goto err;
> +	set_load_options(loaded_image_info, "bootargs");
>
>   	/* we don't support much: */
>   	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>   		"{ro,boot}(blob)0000000000000000");
>
>   	/* Call our payload! */
> +	image_obj = container_of(handle, struct efi_loaded_image_obj, header);
This is not needed, if we remove the debug statement.

>   	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
That line should go to StartImage() if needed. Please, drop it here.

> -	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);
> +	efi_free_pool(loaded_image_info);

Wouldn't this be done by unload_image()?

>
> +err:
>   	return ret;
>   }
>
> @@ -319,8 +274,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 */
> @@ -340,19 +294,21 @@ static int do_efibootmgr(const char *fdt_opt)
>   	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);
> +
> +	EFI_CALL(efi_unload_image(handle));

Only applications have to be unloaded, not drivers. Unloading is to be
handled by exit(), see UEFI spec.

>
>   	if (ret != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;
>
> -	return 0;
> +	return CMD_RET_SUCCESS;
>   }
>
>   /*
> @@ -367,10 +323,14 @@ static int do_efibootmgr(const char *fdt_opt)
>    */
>   static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>   {
> -	unsigned long addr;
> -	char *saddr;
> +	void *image_buf;
> +	struct efi_device_path *device_path, *image_path;
> +	struct efi_device_path *memdp = NULL, *file_path = NULL;
> +	const char *saddr;
> +	unsigned long addr, size;
> +	const char *size_str;
> +	efi_handle_t mem_handle = NULL, handle;
>   	efi_status_t ret;
> -	unsigned long fdt_addr;
>
>   	/* Allow unaligned memory access */
>   	allow_unaligned();
> @@ -390,36 +350,96 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>   		return CMD_RET_FAILURE;
>
>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
> -	if (!strcmp(argv[1], "hello")) {
> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> -
> +	if (!strcmp(image_opt, "hello")) {
>   		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
>   	{
> -		saddr = argv[1];
> +		saddr = image_opt;
> +		size_str = env_get("filesize");
> +		if (size_str)
> +			size = simple_strtoul(size_str, NULL, 16);
> +		else
> +			size = 0;
>
>   		addr = simple_strtoul(saddr, NULL, 16);
>   		/* Check that a numeric value was passed */
>   		if (!addr && *saddr != '0')
>   			return CMD_RET_USAGE;
> +
> +		image_buf = map_sysmem(addr, size);
> +
> +		device_path = bootefi_device_path;
> +		image_path = bootefi_image_path;
> +	}
> +
> +	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:
> +		 */
> +		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> +
> +		/* actual addresses filled in after efi_load_image() */
> +		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					(uint64_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, (void *)0x1 /* FIXME */,
> +				      file_path, image_buf, size, &handle));
> +	if (ret != EFI_SUCCESS)
> +		goto err_prepare;
> +
>   	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);
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);

Why should we need all this duplicate code. Cant we move that to
do_bootefi_exec()?

> +
> +	EFI_CALL(efi_unload_image(handle));

That is the task of efi_exit().

Best regards

Heinrich

> +err_prepare:
> +	if (device_path)
> +		efi_free_pool(file_path);
> +
> +err_add_protocol:
> +	if (mem_handle)
> +		efi_delete_handle(mem_handle);
> +	if (memdp)
> +		efi_free_pool(memdp);
>
>   	if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
> -	else
> -		return CMD_RET_SUCCESS;
> +
> +	return CMD_RET_SUCCESS;
>   }
>
>   #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> @@ -577,7 +597,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"
> @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -410,8 +410,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);
>
> @@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1610,6 +1610,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)
>   {
>

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[Heinrich Schuchardt]

On 3/27/19 5:40 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>
> ---
>   cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
>   1 file changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index e9d4881997a1..94b5bdeed3f1 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -309,22 +309,47 @@ 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();
> +

Shouldn't we move these two call to efi_init_obj_list()?

Best regards

Heinrich

> +	/* 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_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;
> @@ -463,6 +488,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);
> @@ -497,9 +525,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);
>

[U-Boot] [RFC v2 09/11] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[Heinrich Schuchardt]

On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> 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_boot_efi().
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/bootefi.c | 122 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 68 insertions(+), 54 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 94b5bdeed3f1..39a0712af6ad 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt)
>   	return 0;
>   }
>
> +/*
> + * do_boot_efi() - 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_boot_efi(const char *image_opt, const char *fdt_opt)
> +{
> +	unsigned long addr;
> +	char *saddr;
> +	efi_status_t ret;
> +	unsigned long fdt_addr;
> +
> +	/* Allow unaligned memory access */
> +	allow_unaligned();
> +
> +	switch_to_non_secure_mode();

The two call above should be moved to efi_init_obj_list().

> +
> +	/* 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_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);

Shouldn't this be debug() in efi_start_image()?

Best regards

Heinrich

> +	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,
> @@ -481,11 +548,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;
> -	unsigned long fdt_addr;
> -
>   	if (argc < 2)
>   		return CMD_RET_USAGE;
>
> @@ -496,55 +558,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> -	if (r != EFI_SUCCESS)
> -		return r;
> -
> -#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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
>   }
>
>   #ifdef CONFIG_SYS_LONGHELP
>

[U-Boot] [RFC v2 00/11] efi_loader: rework bootefi/bootmgr

[Heinrich Schuchardt]

On 4/12/19 3:18 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> Can you kindly review this series of patches, in particular, patch#10?
> I would like to push it for v2019.07.
>
> -Takahiro Akashi


The first two patches have been merged by Tom Rini.

You should have comments for all of the remaining patches.

Please, base your series on the efi-2019-07 branch available at
https://git.u-boot.de/u-boot-efi.git
(or https://github.com/xypron2/u-boot.git).

Thanks a lot for taking the arduous task of cleaning this up.

Best regards

Heinrich

[U-Boot] [RFC v2 00/11] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 08:11:04AM +0200, Heinrich Schuchardt wrote:
> On 4/12/19 3:18 AM, AKASHI Takahiro wrote:
> >Heinrich,
> >
> >Can you kindly review this series of patches, in particular, patch#10?
> >I would like to push it for v2019.07.
> >
> >-Takahiro Akashi
> 
> 
> The first two patches have been merged by Tom Rini.

Actually, first three?

> You should have comments for all of the remaining patches.

Sure

> Please, base your series on the efi-2019-07 branch available at
> https://git.u-boot.de/u-boot-efi.git
> (or https://github.com/xypron2/u-boot.git).
> 
> Thanks a lot for taking the arduous task of cleaning this up.

I think that it's time to convert efi_selftest to a real app, isn't it?

-Takahiro Akashi

> Best regards
> 
> Heinrich

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 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>
> >---
> >  cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
> >  1 file changed, 34 insertions(+), 8 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index e9d4881997a1..94b5bdeed3f1 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -309,22 +309,47 @@ 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();
> >+
> 
> Shouldn't we move these two call to efi_init_obj_list()?

Given the fact that efi_init_obj_list() is called without invoking
any UEFI binary at some places, I'm not sure that it is the right
place where switch_to_non_secure_mode() be called.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >+	/* 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_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;
> >@@ -463,6 +488,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);
> >@@ -497,9 +525,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);
> >
> 

[U-Boot] [RFC v2 09/11] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 07:59:51AM +0200, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
> >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_boot_efi().
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/bootefi.c | 122 ++++++++++++++++++++++++++++----------------------
> >  1 file changed, 68 insertions(+), 54 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 94b5bdeed3f1..39a0712af6ad 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt)
> >  	return 0;
> >  }
> >
> >+/*
> >+ * do_boot_efi() - 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_boot_efi(const char *image_opt, const char *fdt_opt)
> >+{
> >+	unsigned long addr;
> >+	char *saddr;
> >+	efi_status_t ret;
> >+	unsigned long fdt_addr;
> >+
> >+	/* Allow unaligned memory access */
> >+	allow_unaligned();
> >+
> >+	switch_to_non_secure_mode();
> 
> The two call above should be moved to efi_init_obj_list().

Same comment as in patch#8/11

> >+
> >+	/* 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_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);
> 
> Shouldn't this be debug() in efi_start_image()?

Okay, and application -> image
# but I'm not sure that this information is useful.

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >+	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,
> >@@ -481,11 +548,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;
> >-	unsigned long fdt_addr;
> >-
> >  	if (argc < 2)
> >  		return CMD_RET_USAGE;
> >
> >@@ -496,55 +558,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
> >-	if (r != EFI_SUCCESS)
> >-		return r;
> >-
> >-#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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
> >  }
> >
> >  #ifdef CONFIG_SYS_LONGHELP
> >
> 

[U-Boot] [RFC v2 10/11] efi_loader: rework bootmgr/bootefi using load_image API

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote:
> On 3/27/19 5:40 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                 | 199 +++++++++++++++++++---------------
> >  include/efi_loader.h          |   5 +-
> >  lib/efi_loader/efi_bootmgr.c  |  43 ++++----
> >  lib/efi_loader/efi_boottime.c |   1 +
> >  4 files changed, 138 insertions(+), 110 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 39a0712af6ad..17dccd718008 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -224,88 +224,43 @@ 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;
> >+	efi_status_t ret;
> >
> >  	/* Transfer environment variable as load options */
> >-	set_load_options(loaded_image_info, "bootargs");
> >-
> >-	/* Load the EFI payload */
> >-	ret = efi_load_pe(image_obj, efi, loaded_image_info);
> >+	ret = EFI_CALL(systab.boottime->open_protocol(
> >+					handle,
> >+					&efi_guid_loaded_image,
> >+					(void **)&loaded_image_info,
> >+					NULL, NULL,
> >+					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
> >  	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;
> >-	}
> >+		goto err;
> >+	set_load_options(loaded_image_info, "bootargs");
> >
> >  	/* we don't support much: */
> >  	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
> >  		"{ro,boot}(blob)0000000000000000");
> >
> >  	/* Call our payload! */
> >+	image_obj = container_of(handle, struct efi_loaded_image_obj, header);
> This is not needed, if we remove the debug statement.
> 
> >  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> That line should go to StartImage() if needed. Please, drop it here.
> 
> >-	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);
> >+	efi_free_pool(loaded_image_info);
> 
> Wouldn't this be done by unload_image()?

but we should not call unload_image(), efi_exit() does. Right?

> >
> >+err:
> >  	return ret;
> >  }
> >
> >@@ -319,8 +274,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 */
> >@@ -340,19 +294,21 @@ static int do_efibootmgr(const char *fdt_opt)
> >  	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);
> >+
> >+	EFI_CALL(efi_unload_image(handle));
> 
> Only applications have to be unloaded, not drivers. Unloading is to be
> handled by exit(), see UEFI spec.

How do we know whether the loaded image is an application, or driver?

> >
> >  	if (ret != EFI_SUCCESS)
> >-		return 1;
> >+		return CMD_RET_FAILURE;
> >
> >-	return 0;
> >+	return CMD_RET_SUCCESS;
> >  }
> >
> >  /*
> >@@ -367,10 +323,14 @@ static int do_efibootmgr(const char *fdt_opt)
> >   */
> >  static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> >  {
> >-	unsigned long addr;
> >-	char *saddr;
> >+	void *image_buf;
> >+	struct efi_device_path *device_path, *image_path;
> >+	struct efi_device_path *memdp = NULL, *file_path = NULL;
> >+	const char *saddr;
> >+	unsigned long addr, size;
> >+	const char *size_str;
> >+	efi_handle_t mem_handle = NULL, handle;
> >  	efi_status_t ret;
> >-	unsigned long fdt_addr;
> >
> >  	/* Allow unaligned memory access */
> >  	allow_unaligned();
> >@@ -390,36 +350,96 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
> >  		return CMD_RET_FAILURE;
> >
> >  #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >-	if (!strcmp(argv[1], "hello")) {
> >-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >-
> >+	if (!strcmp(image_opt, "hello")) {
> >  		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
> >  	{
> >-		saddr = argv[1];
> >+		saddr = image_opt;
> >+		size_str = env_get("filesize");
> >+		if (size_str)
> >+			size = simple_strtoul(size_str, NULL, 16);
> >+		else
> >+			size = 0;
> >
> >  		addr = simple_strtoul(saddr, NULL, 16);
> >  		/* Check that a numeric value was passed */
> >  		if (!addr && *saddr != '0')
> >  			return CMD_RET_USAGE;
> >+
> >+		image_buf = map_sysmem(addr, size);
> >+
> >+		device_path = bootefi_device_path;
> >+		image_path = bootefi_image_path;
> >+	}
> >+
> >+	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:
> >+		 */
> >+		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> >+
> >+		/* actual addresses filled in after efi_load_image() */
> >+		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> >+					(uint64_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, (void *)0x1 /* FIXME */,
> >+				      file_path, image_buf, size, &handle));
> >+	if (ret != EFI_SUCCESS)
> >+		goto err_prepare;
> >+
> >  	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);
> >+	ret = do_bootefi_exec(handle);
> >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> 
> Why should we need all this duplicate code. Cant we move that to
> do_bootefi_exec()?
> 
> >+
> >+	EFI_CALL(efi_unload_image(handle));
> 
> That is the task of efi_exit().

Who should be blamed for reclaiming a handle which is created
with load_image()?
What if an application doesn't call efi_exit() by itself?

-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >+err_prepare:
> >+	if (device_path)
> >+		efi_free_pool(file_path);
> >+
> >+err_add_protocol:
> >+	if (mem_handle)
> >+		efi_delete_handle(mem_handle);
> >+	if (memdp)
> >+		efi_free_pool(memdp);
> >
> >  	if (ret != EFI_SUCCESS)
> >  		return CMD_RET_FAILURE;
> >-	else
> >-		return CMD_RET_SUCCESS;
> >+
> >+	return CMD_RET_SUCCESS;
> >  }
> >
> >  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >@@ -577,7 +597,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"
> >@@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -410,8 +410,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);
> >
> >@@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644
> >--- a/lib/efi_loader/efi_boottime.c
> >+++ b/lib/efi_loader/efi_boottime.c
> >@@ -1610,6 +1610,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)
> >  {
> >
> 

[U-Boot] [RFC v2 10/11] efi_loader: rework bootmgr/bootefi using load_image API

[Heinrich Schuchardt]

On 4/12/19 10:38 AM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 07:53:25AM +0200, Heinrich Schuchardt wrote:
>> On 3/27/19 5:40 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                 | 199 +++++++++++++++++++---------------
>>>  include/efi_loader.h          |   5 +-
>>>  lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>>>  lib/efi_loader/efi_boottime.c |   1 +
>>>  4 files changed, 138 insertions(+), 110 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 39a0712af6ad..17dccd718008 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -224,88 +224,43 @@ 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;
>>> +	efi_status_t ret;
>>>
>>>  	/* Transfer environment variable as load options */
>>> -	set_load_options(loaded_image_info, "bootargs");
>>> -
>>> -	/* Load the EFI payload */
>>> -	ret = efi_load_pe(image_obj, efi, loaded_image_info);
>>> +	ret = EFI_CALL(systab.boottime->open_protocol(
>>> +					handle,
>>> +					&efi_guid_loaded_image,
>>> +					(void **)&loaded_image_info,
>>> +					NULL, NULL,
>>> +					EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL));
>>>  	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;
>>> -	}
>>> +		goto err;
>>> +	set_load_options(loaded_image_info, "bootargs");
>>>
>>>  	/* we don't support much: */
>>>  	env_set("efi_8be4df61-93ca-11d2-aa0d-00e098032b8c_OsIndicationsSupported",
>>>  		"{ro,boot}(blob)0000000000000000");
>>>
>>>  	/* Call our payload! */
>>> +	image_obj = container_of(handle, struct efi_loaded_image_obj, header);
>> This is not needed, if we remove the debug statement.
>>
>>>  	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>> That line should go to StartImage() if needed. Please, drop it here.
>>
>>> -	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);
>>> +	efi_free_pool(loaded_image_info);
>>
>> Wouldn't this be done by unload_image()?
>
> but we should not call unload_image(), efi_exit() does. Right?
> Yes, correct.

>>>
>>> +err:
>>>  	return ret;
>>>  }
>>>
>>> @@ -319,8 +274,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 */
>>> @@ -340,19 +294,21 @@ static int do_efibootmgr(const char *fdt_opt)
>>>  	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);
>>> +
>>> +	EFI_CALL(efi_unload_image(handle));
>>
>> Only applications have to be unloaded, not drivers. Unloading is to be
>> handled by exit(), see UEFI spec.
>
> How do we know whether the loaded image is an application, or driver?

In efi_load_pe() we should evaluate the field opt->Subsystem describing
the "Windows Subsystem". I once started on this but then I saw that
cmd/bootefi.c needed to be cleaned up first:

https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-implement-UnloadImage.patch

>
>>>
>>>  	if (ret != EFI_SUCCESS)
>>> -		return 1;
>>> +		return CMD_RET_FAILURE;
>>>
>>> -	return 0;
>>> +	return CMD_RET_SUCCESS;
>>>  }
>>>
>>>  /*
>>> @@ -367,10 +323,14 @@ static int do_efibootmgr(const char *fdt_opt)
>>>   */
>>>  static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>>>  {
>>> -	unsigned long addr;
>>> -	char *saddr;
>>> +	void *image_buf;
>>> +	struct efi_device_path *device_path, *image_path;
>>> +	struct efi_device_path *memdp = NULL, *file_path = NULL;
>>> +	const char *saddr;
>>> +	unsigned long addr, size;
>>> +	const char *size_str;
>>> +	efi_handle_t mem_handle = NULL, handle;
>>>  	efi_status_t ret;
>>> -	unsigned long fdt_addr;
>>>
>>>  	/* Allow unaligned memory access */
>>>  	allow_unaligned();
>>> @@ -390,36 +350,96 @@ static int do_boot_efi(const char *image_opt, const char *fdt_opt)
>>>  		return CMD_RET_FAILURE;
>>>
>>>  #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>> -	if (!strcmp(argv[1], "hello")) {
>>> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>> -
>>> +	if (!strcmp(image_opt, "hello")) {
>>>  		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
>>>  	{
>>> -		saddr = argv[1];
>>> +		saddr = image_opt;
>>> +		size_str = env_get("filesize");
>>> +		if (size_str)
>>> +			size = simple_strtoul(size_str, NULL, 16);
>>> +		else
>>> +			size = 0;
>>>
>>>  		addr = simple_strtoul(saddr, NULL, 16);
>>>  		/* Check that a numeric value was passed */
>>>  		if (!addr && *saddr != '0')
>>>  			return CMD_RET_USAGE;
>>> +
>>> +		image_buf = map_sysmem(addr, size);
>>> +
>>> +		device_path = bootefi_device_path;
>>> +		image_path = bootefi_image_path;
>>> +	}
>>> +
>>> +	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:
>>> +		 */
>>> +		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
>>> +
>>> +		/* actual addresses filled in after efi_load_image() */
>>> +		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
>>> +					(uint64_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, (void *)0x1 /* FIXME */,
>>> +				      file_path, image_buf, size, &handle));
>>> +	if (ret != EFI_SUCCESS)
>>> +		goto err_prepare;
>>> +
>>>  	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);
>>> +	ret = do_bootefi_exec(handle);
>>> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>>
>> Why should we need all this duplicate code. Cant we move that to
>> do_bootefi_exec()?
>>
>>> +
>>> +	EFI_CALL(efi_unload_image(handle));
>>
>> That is the task of efi_exit().
>
> Who should be blamed for reclaiming a handle which is created
> with load_image()?
> What if an application doesn't call efi_exit() by itself?

When the last protocol interface is deleted from a handle via
UninstallProtocolInterface() the handle is deleted.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +err_prepare:
>>> +	if (device_path)
>>> +		efi_free_pool(file_path);
>>> +
>>> +err_add_protocol:
>>> +	if (mem_handle)
>>> +		efi_delete_handle(mem_handle);
>>> +	if (memdp)
>>> +		efi_free_pool(memdp);
>>>
>>>  	if (ret != EFI_SUCCESS)
>>>  		return CMD_RET_FAILURE;
>>> -	else
>>> -		return CMD_RET_SUCCESS;
>>> +
>>> +	return CMD_RET_SUCCESS;
>>>  }
>>>
>>>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>>> @@ -577,7 +597,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"
>>> @@ -602,6 +622,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 6dfa2fef3cf0..6c09a7f40d02 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -410,8 +410,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);
>>>
>>> @@ -565,8 +563,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 74da6b01054a..8e2fa0111591 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1610,6 +1610,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)
>>>  {
>>>
>>
>

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[Heinrich Schuchardt]

On 4/12/19 9:06 AM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
>> On 3/27/19 5:40 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>
>>> ---
>>>  cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
>>>  1 file changed, 34 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index e9d4881997a1..94b5bdeed3f1 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -309,22 +309,47 @@ 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();
>>> +
>>
>> Shouldn't we move these two call to efi_init_obj_list()?
>
> Given the fact that efi_init_obj_list() is called without invoking
> any UEFI binary at some places, I'm not sure that it is the right
> place where switch_to_non_secure_mode() be called.

I think this could even be done in initr_reloc_global_data().

@Alex
What are your thoughts.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +	/* 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_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;
>>> @@ -463,6 +488,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);
>>> @@ -497,9 +525,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);
>>>
>>
>

[U-Boot] [RFC v2 09/11] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[Heinrich Schuchardt]

On 4/12/19 9:22 AM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 07:59:51AM +0200, Heinrich Schuchardt wrote:
>> On 3/27/19 5:40 AM, AKASHI Takahiro wrote:
>>> 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_boot_efi().
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  cmd/bootefi.c | 122 ++++++++++++++++++++++++++++----------------------
>>>  1 file changed, 68 insertions(+), 54 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index 94b5bdeed3f1..39a0712af6ad 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt)
>>>  	return 0;
>>>  }
>>>
>>> +/*
>>> + * do_boot_efi() - 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_boot_efi(const char *image_opt, const char *fdt_opt)
>>> +{
>>> +	unsigned long addr;
>>> +	char *saddr;
>>> +	efi_status_t ret;
>>> +	unsigned long fdt_addr;
>>> +
>>> +	/* Allow unaligned memory access */
>>> +	allow_unaligned();
>>> +
>>> +	switch_to_non_secure_mode();
>>
>> The two call above should be moved to efi_init_obj_list().
>
> Same comment as in patch#8/11
>
>>> +
>>> +	/* 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_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);
>>
>> Shouldn't this be debug() in efi_start_image()?
>
> Okay, and application -> image
> # but I'm not sure that this information is useful.

When debugging I would be more interested in the address to which the
image is loaded so that I have the offset at hand to load the text
symbols into gdb.

I am happy if you simply drop this entry address output.

Regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Best regards
>>
>> Heinrich
>>
>>> +	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,
>>> @@ -481,11 +548,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;
>>> -	unsigned long fdt_addr;
>>> -
>>>  	if (argc < 2)
>>>  		return CMD_RET_USAGE;
>>>
>>> @@ -496,55 +558,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 = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
>>> -	if (r != EFI_SUCCESS)
>>> -		return r;
>>> -
>>> -#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_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
>>>  }
>>>
>>>  #ifdef CONFIG_SYS_LONGHELP
>>>
>>
>

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:
> 
> 
> On 4/12/19 9:06 AM, AKASHI Takahiro wrote:
> > On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
> >> On 3/27/19 5:40 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>
> >>> ---
> >>>  cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
> >>>  1 file changed, 34 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>> index e9d4881997a1..94b5bdeed3f1 100644
> >>> --- a/cmd/bootefi.c
> >>> +++ b/cmd/bootefi.c
> >>> @@ -309,22 +309,47 @@ 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();
> >>> +
> >>
> >> Shouldn't we move these two call to efi_init_obj_list()?
> >
> > Given the fact that efi_init_obj_list() is called without invoking
> > any UEFI binary at some places, I'm not sure that it is the right
> > place where switch_to_non_secure_mode() be called.

If this is UEFI(and arm)-specific, it should be placed just
before setjmp() in efi_start_image().

But I'm not sure whether we should call switch_to_non_secure_mode()
even for a driver, which is logically part of U-Boot UEFI.

-Takahiro Akashi

> I think this could even be done in initr_reloc_global_data().
> 
> @Alex
> What are your thoughts.
> 
> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +	/* 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_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;
> >>> @@ -463,6 +488,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);
> >>> @@ -497,9 +525,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);
> >>>
> >>
> >

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[Heinrich Schuchardt]

On 4/12/19 4:19 PM, AKASHI Takahiro wrote:
> On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:
>>
>>
>> On 4/12/19 9:06 AM, AKASHI Takahiro wrote:
>>> On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
>>>> On 3/27/19 5:40 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>
>>>>> ---
>>>>>   cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
>>>>>   1 file changed, 34 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>>>> index e9d4881997a1..94b5bdeed3f1 100644
>>>>> --- a/cmd/bootefi.c
>>>>> +++ b/cmd/bootefi.c
>>>>> @@ -309,22 +309,47 @@ 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();
>>>>> +
>>>>
>>>> Shouldn't we move these two call to efi_init_obj_list()?
>>>
>>> Given the fact that efi_init_obj_list() is called without invoking
>>> any UEFI binary at some places, I'm not sure that it is the right
>>> place where switch_to_non_secure_mode() be called.
>
> If this is UEFI(and arm)-specific, it should be placed just
> before setjmp() in efi_start_image().
>
> But I'm not sure whether we should call switch_to_non_secure_mode()
> even for a driver, which is logically part of U-Boot UEFI.

switch_to_not_secure_mode() is a weak function which is implemented only
for armv7 and armv8.

efi_init_obj_list() would be a safe place to call it.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>> I think this could even be done in initr_reloc_global_data().
>>
>> @Alex
>> What are your thoughts.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>> +	/* 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_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;
>>>>> @@ -463,6 +488,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);
>>>>> @@ -497,9 +525,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);
>>>>>
>>>>
>>>
>

[U-Boot] [RFC v2 08/11] cmd: bootefi: carve out bootmgr code from do_bootefi()

[AKASHI Takahiro]

On Fri, Apr 12, 2019 at 10:28:09PM +0200, Heinrich Schuchardt wrote:
> On 4/12/19 4:19 PM, AKASHI Takahiro wrote:
> >On Fri, Apr 12, 2019 at 10:58:25AM +0200, Heinrich Schuchardt wrote:
> >>
> >>
> >>On 4/12/19 9:06 AM, AKASHI Takahiro wrote:
> >>>On Fri, Apr 12, 2019 at 07:55:16AM +0200, Heinrich Schuchardt wrote:
> >>>>On 3/27/19 5:40 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>
> >>>>>---
> >>>>>  cmd/bootefi.c | 42 ++++++++++++++++++++++++++++++++++--------
> >>>>>  1 file changed, 34 insertions(+), 8 deletions(-)
> >>>>>
> >>>>>diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >>>>>index e9d4881997a1..94b5bdeed3f1 100644
> >>>>>--- a/cmd/bootefi.c
> >>>>>+++ b/cmd/bootefi.c
> >>>>>@@ -309,22 +309,47 @@ 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();
> >>>>>+
> >>>>
> >>>>Shouldn't we move these two call to efi_init_obj_list()?
> >>>
> >>>Given the fact that efi_init_obj_list() is called without invoking
> >>>any UEFI binary at some places, I'm not sure that it is the right
> >>>place where switch_to_non_secure_mode() be called.
> >
> >If this is UEFI(and arm)-specific, it should be placed just
> >before setjmp() in efi_start_image().
> >
> >But I'm not sure whether we should call switch_to_non_secure_mode()
> >even for a driver, which is logically part of U-Boot UEFI.
> 
> switch_to_not_secure_mode() is a weak function which is implemented only
> for armv7 and armv8.
> 
> efi_init_obj_list() would be a safe place to call it.

You also suggested initr_reloc_global_data(). No?

Anyhow, I think you ignored my concern:
> >But I'm not sure whether we should call switch_to_non_secure_mode()
> >even for a driver, which is logically part of U-Boot UEFI.

Since I think that we need discuss more, I will leave the code
unchanged in the next version.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> >
> >-Takahiro Akashi
> >
> >>I think this could even be done in initr_reloc_global_data().
> >>
> >>@Alex
> >>What are your thoughts.
> >>
> >>Best regards
> >>
> >>Heinrich
> >>
> >>>
> >>>-Takahiro Akashi
> >>>
> >>>
> >>>>Best regards
> >>>>
> >>>>Heinrich
> >>>>
> >>>>>+	/* 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_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;
> >>>>>@@ -463,6 +488,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);
> >>>>>@@ -497,9 +525,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);
> >>>>>
> >>>>
> >>>
> >
>