[U-Boot] [RFC 0/8] efi_loader: rework bootefi/bootmgr

[U-Boot] [RFC 0/8] 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.

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

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

* do_bootmgr_load() should also return a size of image loaded.
  This information will be needed at load_image(0 and also be used to
  verify an image with its signature in "secure boot" in the future.

* 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 #5 are preparatory patches for patch#6.
Patch#7 is for standalone boot manager.

The concern that I'm aware of is:
* 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#6.)

-Takahiro Akashi

AKASHI Takahiro (8):
  efi_loader: boottime: don't add device path protocol to image handle
  efi_loader: boottime: export efi_[un]load_image()
  efi_loader: bootmgr: return pointer and size of buffer in loading
  cmd: bootefi: move do_bootefi_bootmgr_exec() forward
  cmd: bootefi: carve out fdt handling
  cmd: bootefi: carve out efi_selftest code from do_bootefi()
  cmd: bootefi: rework do_bootefi(), using load_image API
  cmd: add efibootmgr command

 cmd/Kconfig                   |   8 +
 cmd/bootefi.c                 | 434 +++++++++++++++++++++++-----------
 include/efi_loader.h          |  14 +-
 lib/efi_loader/efi_bootmgr.c  |  41 ++--
 lib/efi_loader/efi_boottime.c |  39 ++-
 5 files changed, 360 insertions(+), 176 deletions(-)

-- 
2.20.1

[U-Boot] [RFC 1/8] efi_loader: boottime: don't add device path protocol to image handle

[AKASHI Takahiro]

It is just wrong to add devcie path protocol to image handle.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_boottime.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index bd8b8a17ae71..7bd9c0a952d4 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
 	info->file_path = file_path;
 	info->system_table = &systab;
 
-	if (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)
-			goto failure;
-	}
 
 	/*
 	 * When asking for the loaded_image interface, just
-- 
2.20.1

[U-Boot] [RFC 2/8] efi_loader: boottime: export efi_[un]load_image()

[AKASHI Takahiro]

Those two functions will be used later to re-implement do_bootefi_exec().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 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 512880ab8fbf..47a51ddc9406 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -320,10 +320,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 7bd9c0a952d4..c6991d353497 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1672,12 +1672,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 3/8] efi_loader: bootmgr: return pointer and size of buffer in loading

[AKASHI Takahiro]

We need to know the size of image loaded so as to be able to use
load_image() API at do_bootefi_exec() in a later patch.

So change the interface of efi_bootmgr_load().

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 include/efi_loader.h         |  5 +++--
 lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++---------------
 2 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 47a51ddc9406..3f51116155ad 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -564,8 +564,9 @@ 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(struct efi_device_path **device_path,
+			      struct efi_device_path **file_path,
+			      void **image, efi_uintn_t *size);
 
 #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
 
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 1575c5c09e46..38f3fa15f6ef 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -120,14 +120,17 @@ 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,
+				   struct efi_device_path **device_path,
+				   struct efi_device_path **file_path,
+				   void **image, efi_uintn_t *image_size)
 {
 	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,18 +139,17 @@ 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_load_image_from_path(lo.file_path, image, image_size);
 
 		if (ret != EFI_SUCCESS)
 			goto error;
@@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
 
 		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 +181,12 @@ 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(struct efi_device_path **device_path,
+			      struct efi_device_path **file_path,
+			      void **image, efi_uintn_t *image_size)
 {
 	u16 bootnext, *bootorder;
 	efi_uintn_t size;
-	void *image = NULL;
 	int i, num;
 	efi_status_t ret;
 
@@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
 					(efi_guid_t *)&efi_global_variable_guid,
 					0, 0, &bootnext));
 		if (ret == EFI_SUCCESS) {
-			image = try_load_entry(bootnext,
-					       device_path, file_path);
-			if (image)
+			ret = try_load_entry(bootnext, device_path, file_path,
+					     image, image_size);
+			if (ret == EFI_SUCCESS)
 				goto error;
 		}
 	}
@@ -212,19 +216,22 @@ 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)
+		debug("%s: trying to load Boot%04X\n", __func__,
+		      bootorder[i]);
+		ret = try_load_entry(bootorder[i], device_path, file_path,
+				     image, image_size);
+		if (ret == EFI_SUCCESS)
 			break;
 	}
 
 	free(bootorder);
 
 error:
-	return image;
+	return ret;
 }
-- 
2.20.1

[U-Boot] [RFC 4/8] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[AKASHI Takahiro]

This is a preparatory 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 3619a20e6433..1d90e7b4b575 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -314,6 +314,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
 /**
  * bootefi_test_prepare() - prepare to run an EFI test
@@ -362,27 +383,6 @@ failure:
 
 #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 5/8] cmd: bootefi: carve out fdt handling

[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 1d90e7b4b575..8915cdbfd5f5 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 int efi_process_fdt(const char *fdt_opt)
+{
+	unsigned long fdt_addr;
+	efi_status_t r;
+
+	if (fdt_opt) {
+		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
+		if (!fdt_addr && *fdt_opt != '0')
+			return CMD_RET_USAGE;
+
+		/* Install device tree */
+		r = efi_install_fdt(fdt_addr);
+		if (r != EFI_SUCCESS) {
+			printf("ERROR: failed to install device tree\n");
+			return CMD_RET_FAILURE;
+		}
+	} else {
+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
+		efi_install_configuration_table(&efi_guid_fdt, NULL);
+		printf("WARNING: booting without device tree\n");
+	}
+
+	return CMD_RET_SUCCESS;
+}
+
 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");
-	}
+	ret = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
 #ifdef CONFIG_CMD_BOOTEFI_HELLO
 	if (!strcmp(argv[1], "hello")) {
 		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-- 
2.20.1

[U-Boot] [RFC 6/8] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi() in a 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 | 146 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 91 insertions(+), 55 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 8915cdbfd5f5..1470122af266 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -232,39 +232,6 @@ static int efi_process_fdt(const char *fdt_opt)
 	return CMD_RET_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
  *
@@ -311,11 +278,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)
@@ -339,7 +309,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)
@@ -370,6 +342,25 @@ static int do_bootefi_bootmgr_exec(void)
 }
 
 #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
  *
@@ -415,6 +406,63 @@ 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 r;
+	int ret;
+
+	/* 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;
+	}
+
+	ret = efi_process_fdt(fdt_opt);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	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;
+}
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
 /* Interpreter command to boot an arbitrary EFI image from memory */
@@ -425,6 +473,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();
 
@@ -438,9 +493,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;
-
 	ret = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
 	if (ret != EFI_SUCCESS)
 		return ret;
@@ -456,22 +508,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 7/8] cmd: bootefi: rework do_bootefi(), using load_image API

[AKASHI Takahiro]

In this patch, do_bootefi() and do_bootefi_exec() will be reworked,
without any functional change so that load_image() API as well as
start_image() are used in the implementation.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 cmd/bootefi.c                 | 252 ++++++++++++++++++++++------------
 lib/efi_loader/efi_boottime.c |  14 +-
 2 files changed, 170 insertions(+), 96 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 1470122af266..00cacbdd4231 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -236,6 +236,7 @@ static int efi_process_fdt(const char *fdt_opt)
  * do_bootefi_exec() - execute EFI binary
  *
  * @efi:		address of the binary
+ * @efi_size:		size of the binary
  * @device_path:	path of the device from which the binary was loaded
  * @image_path:		device path of the binary
  * Return:		status code
@@ -243,15 +244,16 @@ static int efi_process_fdt(const char *fdt_opt)
  * 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,
+static efi_status_t do_bootefi_exec(void *efi, efi_uintn_t efi_size,
 				    struct efi_device_path *device_path,
 				    struct efi_device_path *image_path)
 {
-	efi_handle_t mem_handle = NULL;
+	efi_handle_t mem_handle = NULL, handle;
 	struct efi_device_path *memdp = NULL;
-	efi_status_t ret;
+	struct efi_device_path *file_path = NULL;
 	struct efi_loaded_image_obj *image_obj = NULL;
 	struct efi_loaded_image *loaded_image_info = NULL;
+	efi_status_t ret;
 
 	/*
 	 * Special case for efi payload not loaded from disk, such as
@@ -260,36 +262,42 @@ static efi_status_t do_bootefi_exec(void *efi,
 	 */
 	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() */
+		/* actual addresses filled in after efi_load_image() */
 		memdp = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
-		device_path = image_path = memdp;
+		file_path = memdp; /* append(memdp, NULL) */
 		/*
-		 * Grub expects that the device path of the loaded image is
-		 * installed on a handle.
+		 * 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)
+		if (ret != EFI_SUCCESS) {
+			efi_free_pool(memdp);
 			return ret; /* TODO: leaks device_path */
+		}
 		ret = efi_add_protocol(mem_handle, &efi_guid_device_path,
-				       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_setup_loaded_image(device_path, image_path, &image_obj,
-				     &loaded_image_info);
-	if (ret)
+	ret = EFI_CALL(efi_load_image(false, (void *)0x1 /* FIXME */, file_path,
+				      efi, efi_size, &handle));
+	if (ret != EFI_SUCCESS)
 		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);
+	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;
+	set_load_options(loaded_image_info, "bootargs");
 
 	if (memdp) {
 		struct efi_device_path_memory *mdp = (void *)memdp;
@@ -304,41 +312,154 @@ static efi_status_t do_bootefi_exec(void *efi,
 		"{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);
+	ret = EFI_CALL(efi_unload_image(handle));
 
 err_add_protocol:
 	if (mem_handle)
 		efi_delete_handle(mem_handle);
+	if (device_path)
+		efi_free_pool(file_path);
 
 	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)
+{
+	void *image_buf;
+	efi_uintn_t buf_size;
+	struct efi_device_path *device_path, *image_path;
+	unsigned long addr;
+	efi_status_t r;
+	int ret;
+
+	/* 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;
+	}
+
+	ret = efi_process_fdt(fdt_opt);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	r = efi_bootmgr_load(&device_path, &image_path, &image_buf, &buf_size);
+	if (r != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	addr = map_to_sysmem(image_buf);
+
+	printf("## Starting EFI application at %08lx ...\n", addr);
+	r = do_bootefi_exec(image_buf, buf_size, device_path, image_path);
+	printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
+
+	if (r != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
+}
+
+/*
+ * 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)
 {
-	struct efi_device_path *device_path, *file_path;
-	void *addr;
+	void *image_buf;
+	struct efi_device_path *device_path, *image_path;
+	const char *saddr;
+	unsigned long addr, size;
+	const char *size_str;
 	efi_status_t r;
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
-	if (!addr)
-		return 1;
+	/* Allow unaligned memory access */
+	allow_unaligned();
 
-	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);
+	switch_to_non_secure_mode();
 
+	/* Initialize EFI drivers */
+	r = efi_init_obj_list();
+	if (r != EFI_SUCCESS) {
+		printf("Error: Cannot set up EFI drivers, r = %lu\n",
+		       r & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	r = efi_process_fdt(fdt_opt);
 	if (r != EFI_SUCCESS)
-		return 1;
+		return r;
 
-	return 0;
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	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;
+
+		image_buf = map_sysmem(addr, size);
+		memcpy(image_buf, __efi_helloworld_begin, size);
+
+		device_path = NULL;
+		image_path = NULL;
+	} else
+#endif
+	{
+		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;
+	}
+
+	printf("## Starting EFI application at %08lx ...\n", addr);
+	r = do_bootefi_exec(image_buf, size, device_path, image_path);
+	printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
+
+	if (r != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	return CMD_RET_SUCCESS;
 }
 
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
@@ -468,69 +589,17 @@ 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;
+
+	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);
 #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;
-	}
-
-	ret = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
-	if (ret != EFI_SUCCESS)
-		return ret;
-
-#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
-	if (!strcmp(argv[1], "bootmgr")) {
-		return do_bootefi_bootmgr_exec();
-	} else {
-		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
@@ -549,7 +618,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"
@@ -574,6 +643,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/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
index c6991d353497..0011a226a3be 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1703,11 +1703,6 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
 					       &source_size);
 		if (ret != EFI_SUCCESS)
 			goto error;
-		/*
-		 * split file_path which contains both the device and
-		 * file parts:
-		 */
-		efi_dp_split_file_path(file_path, &dp, &fp);
 	} else {
 		/* In this case, file_path is the "device" path, i.e.
 		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED
@@ -1723,10 +1718,13 @@ efi_status_t EFIAPI efi_load_image(bool boot_policy,
 		dest_buffer = (void *)(uintptr_t)addr;
 		memcpy(dest_buffer, source_buffer, source_size);
 		source_buffer = dest_buffer;
-
-		dp = file_path;
-		fp = NULL;
 	}
+	/*
+	 * split file_path which contains both the device and
+	 * file parts:
+	 */
+	efi_dp_split_file_path(file_path, &dp, &fp);
+
 	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
 	if (ret != EFI_SUCCESS)
 		goto error_invalid_image;
-- 
2.20.1

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

[AKASHI Takahiro]

Add CONFIG_CMD_STANDALONE_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 00cacbdd4231..6becd37e17a4 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -656,3 +656,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 1/8] efi_loader: boottime: don't add device path protocol to image handle

[Heinrich Schuchardt]

On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> It is just wrong to add devcie path protocol to image handle.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_boottime.c | 11 +----------
>  1 file changed, 1 insertion(+), 10 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index bd8b8a17ae71..7bd9c0a952d4 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>  	info->file_path = file_path;
>  	info->system_table = &systab;
>  
> -	if (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);

Installing the device path is not the problem. It is the GUID that is
wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.

UEFI Spec 2.7:

"The Loaded Image Device Path Protocol must be installed onto the image
handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()."

Best regards

Heinrich

> -		if (ret != EFI_SUCCESS)
> -			goto failure;
> -	}
>  
>  	/*
>  	 * When asking for the loaded_image interface, just
> 

[U-Boot] [RFC 2/8] efi_loader: boottime: export efi_[un]load_image()

[Heinrich Schuchardt]

On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> Those two functions will be used later to re-implement do_bootefi_exec().
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

[U-Boot] [RFC 1/8] efi_loader: boottime: don't add device path protocol to image handle

[AKASHI Takahiro]

On Tue, Mar 05, 2019 at 08:48:37PM +0100, Heinrich Schuchardt wrote:
> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> > It is just wrong to add devcie path protocol to image handle.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  lib/efi_loader/efi_boottime.c | 11 +----------
> >  1 file changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> > index bd8b8a17ae71..7bd9c0a952d4 100644
> > --- a/lib/efi_loader/efi_boottime.c
> > +++ b/lib/efi_loader/efi_boottime.c
> > @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >  	info->file_path = file_path;
> >  	info->system_table = &systab;
> >  
> > -	if (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);
> 
> Installing the device path is not the problem. It is the GUID that is
> wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.

Okay, but I believe that we need duplicate device_path here
before installing it as EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID.

See this line:

> >  		info->device_handle = efi_dp_find_obj(device_path, NULL);

Normally, device_path is expected to be already associated with
another handle. We should not allow two handles to share one protocol(data).
That is also why I initially believed that add_protocol() should be removed.

Thanks,
-Takahiro Akashi


> UEFI Spec 2.7:
> 
> "The Loaded Image Device Path Protocol must be installed onto the image
> handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()."
> 
> Best regards
> 
> Heinrich
> 
> > -		if (ret != EFI_SUCCESS)
> > -			goto failure;
> > -	}
> >  
> >  	/*
> >  	 * When asking for the loaded_image interface, just
> > 
> 

[U-Boot] [RFC 1/8] efi_loader: boottime: don't add device path protocol to image handle

[Heinrich Schuchardt]

On 3/6/19 1:27 AM, AKASHI Takahiro wrote:
> On Tue, Mar 05, 2019 at 08:48:37PM +0100, Heinrich Schuchardt wrote:
>> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
>>> It is just wrong to add devcie path protocol to image handle.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>> ---
>>>  lib/efi_loader/efi_boottime.c | 11 +----------
>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>> index bd8b8a17ae71..7bd9c0a952d4 100644
>>> --- a/lib/efi_loader/efi_boottime.c
>>> +++ b/lib/efi_loader/efi_boottime.c
>>> @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>  	info->file_path = file_path;
>>>  	info->system_table = &systab;
>>>  
>>> -	if (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);
>>
>> Installing the device path is not the problem. It is the GUID that is
>> wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.
> 
> Okay, but I believe that we need duplicate device_path here
> before installing it as EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID.
> 
> See this line:
> 
>>>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> 
> Normally, device_path is expected to be already associated with
> another handle. We should not allow two handles to share one protocol(data).
> That is also why I initially believed that add_protocol() should be removed.

The spec says we should use a copy of the unchanged DevicePath parameter
of LoadImage() which may be NULL.

So we have to rework all callers to get the device_path parameter of
efi_setup_loaded_image() right.

Best regards

Heinrich

> 
> Thanks,
> -Takahiro Akashi
> 
> 
>> UEFI Spec 2.7:
>>
>> "The Loaded Image Device Path Protocol must be installed onto the image
>> handle of a PE/COFF image loaded through the EFI Boot Service LoadImage()."
>>
>> Best regards
>>
>> Heinrich
>>
>>> -		if (ret != EFI_SUCCESS)
>>> -			goto failure;
>>> -	}
>>>  
>>>  	/*
>>>  	 * When asking for the loaded_image interface, just
>>>
>>
> 

[U-Boot] [RFC 1/8] efi_loader: boottime: don't add device path protocol to image handle

[Heinrich Schuchardt]

On 3/6/19 6:04 AM, Heinrich Schuchardt wrote:
> On 3/6/19 1:27 AM, AKASHI Takahiro wrote:
>> On Tue, Mar 05, 2019 at 08:48:37PM +0100, Heinrich Schuchardt wrote:
>>> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
>>>> It is just wrong to add devcie path protocol to image handle.
>>>>
>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> ---
>>>>  lib/efi_loader/efi_boottime.c | 11 +----------
>>>>  1 file changed, 1 insertion(+), 10 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
>>>> index bd8b8a17ae71..7bd9c0a952d4 100644
>>>> --- a/lib/efi_loader/efi_boottime.c
>>>> +++ b/lib/efi_loader/efi_boottime.c
>>>> @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
>>>>  	info->file_path = file_path;
>>>>  	info->system_table = &systab;
>>>>  
>>>> -	if (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);
>>>
>>> Installing the device path is not the problem. It is the GUID that is
>>> wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.
>>
>> Okay, but I believe that we need duplicate device_path here
>> before installing it as EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID.
>>
>> See this line:
>>
>>>>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
>>
>> Normally, device_path is expected to be already associated with
>> another handle. We should not allow two handles to share one protocol(data).
>> That is also why I initially believed that add_protocol() should be removed.
> 
> The spec says we should use a copy of the unchanged DevicePath parameter
> of LoadImage() which may be NULL.
> 
> So we have to rework all callers to get the device_path parameter of
> efi_setup_loaded_image() right.
> 

Why are we constructing a dummy memory device path at all in cmd/bootefi?

The commit message of patch bf19273e81eb "efi_loader: Add mem-mapped for
fallback" that introduced this does not give a valid answer as it is
explicitly allowable to call LoadImage with DevicePath = NULL if
SourceBuffer is provided.

So I suggest we rid ourselves of the dummy device path with this patch
series.

Best regards

Heinrich

[U-Boot] [RFC 0/8] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

Heinrich,

Do you have any comments, in particular, on patch#7 which is
core part of my RFC?

Thanks,
-Takahiro Akashi

On Tue, Mar 05, 2019 at 02:53:29PM +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.
> 
> * Contrary to the other part, efi_selftest part of the code is unusal
>   in terms of loading/execution path in do_bootefi().
> 
> * do_bootefi_exec() would better be implemented using load_image() along
>   with start_image() to be aligned with UEFI interfaces.
> 
> * do_bootmgr_load() should also return a size of image loaded.
>   This information will be needed at load_image(0 and also be used to
>   verify an image with its signature in "secure boot" in the future.
> 
> * 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 #5 are preparatory patches for patch#6.
> Patch#7 is for standalone boot manager.
> 
> The concern that I'm aware of is:
> * 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#6.)
> 
> -Takahiro Akashi
> 
> AKASHI Takahiro (8):
>   efi_loader: boottime: don't add device path protocol to image handle
>   efi_loader: boottime: export efi_[un]load_image()
>   efi_loader: bootmgr: return pointer and size of buffer in loading
>   cmd: bootefi: move do_bootefi_bootmgr_exec() forward
>   cmd: bootefi: carve out fdt handling
>   cmd: bootefi: carve out efi_selftest code from do_bootefi()
>   cmd: bootefi: rework do_bootefi(), using load_image API
>   cmd: add efibootmgr command
> 
>  cmd/Kconfig                   |   8 +
>  cmd/bootefi.c                 | 434 +++++++++++++++++++++++-----------
>  include/efi_loader.h          |  14 +-
>  lib/efi_loader/efi_bootmgr.c  |  41 ++--
>  lib/efi_loader/efi_boottime.c |  39 ++-
>  5 files changed, 360 insertions(+), 176 deletions(-)
> 
> -- 
> 2.20.1
> 

[U-Boot] [RFC 0/8] efi_loader: rework bootefi/bootmgr

[Heinrich Schuchardt]

On 3/19/19 8:23 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> Do you have any comments, in particular, on patch#7 which is
> core part of my RFC?
>
> Thanks,
> -Takahiro Akashi

Hello Takahiro,

the patches are not applicable to current git master. Do you have a repo
where you have applied these patches?

Best regards

Heinrich

>
> On Tue, Mar 05, 2019 at 02:53:29PM +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.
>>
>> * Contrary to the other part, efi_selftest part of the code is unusal
>>   in terms of loading/execution path in do_bootefi().
>>
>> * do_bootefi_exec() would better be implemented using load_image() along
>>   with start_image() to be aligned with UEFI interfaces.
>>
>> * do_bootmgr_load() should also return a size of image loaded.
>>   This information will be needed at load_image(0 and also be used to
>>   verify an image with its signature in "secure boot" in the future.
>>
>> * 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 #5 are preparatory patches for patch#6.
>> Patch#7 is for standalone boot manager.
>>
>> The concern that I'm aware of is:
>> * 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#6.)
>>
>> -Takahiro Akashi
>>
>> AKASHI Takahiro (8):
>>   efi_loader: boottime: don't add device path protocol to image handle
>>   efi_loader: boottime: export efi_[un]load_image()
>>   efi_loader: bootmgr: return pointer and size of buffer in loading
>>   cmd: bootefi: move do_bootefi_bootmgr_exec() forward
>>   cmd: bootefi: carve out fdt handling
>>   cmd: bootefi: carve out efi_selftest code from do_bootefi()
>>   cmd: bootefi: rework do_bootefi(), using load_image API
>>   cmd: add efibootmgr command
>>
>>  cmd/Kconfig                   |   8 +
>>  cmd/bootefi.c                 | 434 +++++++++++++++++++++++-----------
>>  include/efi_loader.h          |  14 +-
>>  lib/efi_loader/efi_bootmgr.c  |  41 ++--
>>  lib/efi_loader/efi_boottime.c |  39 ++-
>>  5 files changed, 360 insertions(+), 176 deletions(-)
>>
>> --
>> 2.20.1
>>
>

[U-Boot] [RFC 3/8] efi_loader: bootmgr: return pointer and size of buffer in loading

[Heinrich Schuchardt]

On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> We need to know the size of image loaded so as to be able to use
> load_image() API at do_bootefi_exec() in a later patch.
>
> So change the interface of efi_bootmgr_load().
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  include/efi_loader.h         |  5 +++--
>  lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++---------------
>  2 files changed, 27 insertions(+), 19 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 47a51ddc9406..3f51116155ad 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -564,8 +564,9 @@ 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(struct efi_device_path **device_path,
> +			      struct efi_device_path **file_path,
> +			      void **image, efi_uintn_t *size);
>
>  #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 1575c5c09e46..38f3fa15f6ef 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -120,14 +120,17 @@ 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,
> +				   struct efi_device_path **device_path,
> +				   struct efi_device_path **file_path,
> +				   void **image, efi_uintn_t *image_size)
>  {
>  	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,18 +139,17 @@ 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_load_image_from_path(lo.file_path, image, image_size);
>

This call to efi_load_image_from_path() leads to duplication of logic.

Why don't we simply call EFI_CALL(efi_load_image())) here and if it
succeeds return from efi_bootmgr_load()?

Best regards

Heinrich

>  		if (ret != EFI_SUCCESS)
>  			goto error;
> @@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
>
>  		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 +181,12 @@ 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(struct efi_device_path **device_path,
> +			      struct efi_device_path **file_path,
> +			      void **image, efi_uintn_t *image_size)
>  {
>  	u16 bootnext, *bootorder;
>  	efi_uintn_t size;
> -	void *image = NULL;
>  	int i, num;
>  	efi_status_t ret;
>
> @@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
>  					(efi_guid_t *)&efi_global_variable_guid,
>  					0, 0, &bootnext));
>  		if (ret == EFI_SUCCESS) {
> -			image = try_load_entry(bootnext,
> -					       device_path, file_path);
> -			if (image)
> +			ret = try_load_entry(bootnext, device_path, file_path,
> +					     image, image_size);
> +			if (ret == EFI_SUCCESS)
>  				goto error;
>  		}
>  	}
> @@ -212,19 +216,22 @@ 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)
> +		debug("%s: trying to load Boot%04X\n", __func__,
> +		      bootorder[i]);
> +		ret = try_load_entry(bootorder[i], device_path, file_path,
> +				     image, image_size);
> +		if (ret == EFI_SUCCESS)
>  			break;
>  	}
>
>  	free(bootorder);
>
>  error:
> -	return image;
> +	return ret;
>  }
>

[U-Boot] [RFC 4/8] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[Heinrich Schuchardt]

On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> This is a preparatory 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 3619a20e6433..1d90e7b4b575 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -314,6 +314,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 CMD_RET_FAILURE ?

> +
> +	return 0;

return CMD_RET_SUCCESS ?

The lines following efi_bootmgr_load() are duplicating code from
do_bootefi().

The patch itself is ok. But in the patch series we should get rid of the
duplication.

Best regards

Heinrich

> +}
> +
>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>  /**
>   * bootefi_test_prepare() - prepare to run an EFI test
> @@ -362,27 +383,6 @@ failure:
>
>  #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 3/8] efi_loader: bootmgr: return pointer and size of buffer in loading

[AKASHI Takahiro]

On Thu, Mar 21, 2019 at 12:41:06PM +0100, Heinrich Schuchardt wrote:
> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> > We need to know the size of image loaded so as to be able to use
> > load_image() API at do_bootefi_exec() in a later patch.
> >
> > So change the interface of efi_bootmgr_load().
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  include/efi_loader.h         |  5 +++--
> >  lib/efi_loader/efi_bootmgr.c | 41 +++++++++++++++++++++---------------
> >  2 files changed, 27 insertions(+), 19 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 47a51ddc9406..3f51116155ad 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -564,8 +564,9 @@ 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(struct efi_device_path **device_path,
> > +			      struct efi_device_path **file_path,
> > +			      void **image, efi_uintn_t *size);
> >
> >  #else /* CONFIG_IS_ENABLED(EFI_LOADER) */
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 1575c5c09e46..38f3fa15f6ef 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -120,14 +120,17 @@ 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,
> > +				   struct efi_device_path **device_path,
> > +				   struct efi_device_path **file_path,
> > +				   void **image, efi_uintn_t *image_size)
> >  {
> >  	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,18 +139,17 @@ 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_load_image_from_path(lo.file_path, image, image_size);
> >
> 
> This call to efi_load_image_from_path() leads to duplication of logic.
> 
> Why don't we simply call EFI_CALL(efi_load_image())) here and if it
> succeeds return from efi_bootmgr_load()?

Make sense. It will make do_bootefi_exec() simpler.
This will also give us a reason why we have do_efibootmgr() apart
from do_boot_efi().

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  		if (ret != EFI_SUCCESS)
> >  			goto error;
> > @@ -164,12 +166,14 @@ static void *try_load_entry(uint16_t n, struct efi_device_path **device_path,
> >
> >  		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 +181,12 @@ 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(struct efi_device_path **device_path,
> > +			      struct efi_device_path **file_path,
> > +			      void **image, efi_uintn_t *image_size)
> >  {
> >  	u16 bootnext, *bootorder;
> >  	efi_uintn_t size;
> > -	void *image = NULL;
> >  	int i, num;
> >  	efi_status_t ret;
> >
> > @@ -201,9 +205,9 @@ void *efi_bootmgr_load(struct efi_device_path **device_path,
> >  					(efi_guid_t *)&efi_global_variable_guid,
> >  					0, 0, &bootnext));
> >  		if (ret == EFI_SUCCESS) {
> > -			image = try_load_entry(bootnext,
> > -					       device_path, file_path);
> > -			if (image)
> > +			ret = try_load_entry(bootnext, device_path, file_path,
> > +					     image, image_size);
> > +			if (ret == EFI_SUCCESS)
> >  				goto error;
> >  		}
> >  	}
> > @@ -212,19 +216,22 @@ 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)
> > +		debug("%s: trying to load Boot%04X\n", __func__,
> > +		      bootorder[i]);
> > +		ret = try_load_entry(bootorder[i], device_path, file_path,
> > +				     image, image_size);
> > +		if (ret == EFI_SUCCESS)
> >  			break;
> >  	}
> >
> >  	free(bootorder);
> >
> >  error:
> > -	return image;
> > +	return ret;
> >  }
> >
> 

[U-Boot] [RFC 4/8] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[AKASHI Takahiro]

On Thu, Mar 21, 2019 at 12:48:53PM +0100, Heinrich Schuchardt wrote:
> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> > This is a preparatory 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 3619a20e6433..1d90e7b4b575 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -314,6 +314,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 CMD_RET_FAILURE ?
> 
> > +
> > +	return 0;
> 
> return CMD_RET_SUCCESS ?
> 
> The lines following efi_bootmgr_load() are duplicating code from
> do_bootefi().

do_bootefi() -> do_boot_efi()?

> The patch itself is ok. But in the patch series we should get rid of the
> duplication.

We only share:
> > +	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;

Can we call it a duplication?
# I don't like the print messages here anyway.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +}
> > +
> >  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >  /**
> >   * bootefi_test_prepare() - prepare to run an EFI test
> > @@ -362,27 +383,6 @@ failure:
> >
> >  #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 1/8] efi_loader: boottime: don't add device path protocol to image handle

[AKASHI Takahiro]

On Wed, Mar 06, 2019 at 06:29:14AM +0100, Heinrich Schuchardt wrote:
> On 3/6/19 6:04 AM, Heinrich Schuchardt wrote:
> > On 3/6/19 1:27 AM, AKASHI Takahiro wrote:
> >> On Tue, Mar 05, 2019 at 08:48:37PM +0100, Heinrich Schuchardt wrote:
> >>> On 3/5/19 6:53 AM, AKASHI Takahiro wrote:
> >>>> It is just wrong to add devcie path protocol to image handle.
> >>>>
> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>> ---
> >>>>  lib/efi_loader/efi_boottime.c | 11 +----------
> >>>>  1 file changed, 1 insertion(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> >>>> index bd8b8a17ae71..7bd9c0a952d4 100644
> >>>> --- a/lib/efi_loader/efi_boottime.c
> >>>> +++ b/lib/efi_loader/efi_boottime.c
> >>>> @@ -1540,17 +1540,8 @@ efi_status_t efi_setup_loaded_image(struct efi_device_path *device_path,
> >>>>  	info->file_path = file_path;
> >>>>  	info->system_table = &systab;
> >>>>  
> >>>> -	if (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);
> >>>
> >>> Installing the device path is not the problem. It is the GUID that is
> >>> wrong. Use EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID here.
> >>
> >> Okay, but I believe that we need duplicate device_path here
> >> before installing it as EFI_LOADED_IMAGE_DEVICE_PATH_PROTOCOL_GUID.
> >>
> >> See this line:
> >>
> >>>>  		info->device_handle = efi_dp_find_obj(device_path, NULL);
> >>
> >> Normally, device_path is expected to be already associated with
> >> another handle. We should not allow two handles to share one protocol(data).
> >> That is also why I initially believed that add_protocol() should be removed.
> > 
> > The spec says we should use a copy of the unchanged DevicePath parameter
> > of LoadImage() which may be NULL.
> > 
> > So we have to rework all callers to get the device_path parameter of
> > efi_setup_loaded_image() right.
> > 
> 
> Why are we constructing a dummy memory device path at all in cmd/bootefi?
> 
> The commit message of patch bf19273e81eb "efi_loader: Add mem-mapped for
> fallback" that introduced this does not give a valid answer as it is
> explicitly allowable to call LoadImage with DevicePath = NULL if
> SourceBuffer is provided.

As far as I know, if we load EDK2's Shell.efi by calling LoadImage
*without* DevicePath, it will fail to boot at some assertion check.

-Takahiro Akashi

> So I suggest we rid ourselves of the dummy device path with this patch
> series.
> 
> Best regards
> 
> Heinrich