[U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr

[U-Boot] [RFC v3 00/10] efi_loader: rework bootefi/bootmgr

[AKASHI Takahiro]

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

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

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

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

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

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

Issues:
* The semantics of efi_dp_from_name() should be changed.
  (See FIXME in patch#9.)

-Takahiro Akashi

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

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

AKASHI Takahiro (10):
  efi_loader: device_path: handle special case of loading
  efi_loader: export root node handle
  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                    | 515 ++++++++++++++++++++-----------
 include/efi_loader.h             |   7 +-
 lib/efi_loader/efi_bootmgr.c     |  43 +--
 lib/efi_loader/efi_boottime.c    |   2 +
 lib/efi_loader/efi_device_path.c |   8 +
 lib/efi_loader/efi_root_node.c   |  13 +-
 7 files changed, 381 insertions(+), 215 deletions(-)

-- 
2.20.1

[U-Boot] [RFC v3 01/10] 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 v3 01/10] efi_loader: device_path: handle special case of loading

[Heinrich Schuchardt]

On 4/16/19 6:24 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.
>
> 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);
>

Should we really give device paths *starting* with a memory node a
special treatment?

Or should we simply handle all cases where the device path does not end
on a file path the same, e.g.:

diff --git a/lib/efi_loader/efi_device_path.c
b/lib/efi_loader/efi_device_path.c
index d8c052d6ec..d0fd154f54 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -924,7 +924,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
                                     struct efi_device_path **device_path,
                                     struct efi_device_path **file_path)
  {
-       struct efi_device_path *p, *dp, *fp;
+       struct efi_device_path *p, *dp, *fp = NULL;

         *device_path = NULL;
         *file_path = NULL;
@@ -935,7 +935,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
         while (!EFI_DP_TYPE(p, MEDIA_DEVICE, FILE_PATH)) {
                 p = efi_dp_next(p);
                 if (!p)
-                       return EFI_INVALID_PARAMETER;
+                       goto out;
         }
         fp = efi_dp_dup(p);
         if (!fp)
@@ -944,6 +944,7 @@ efi_status_t efi_dp_split_file_path(struct
efi_device_path *full_path,
         p->sub_type = DEVICE_PATH_SUB_TYPE_END;
         p->length = sizeof(*p);

+out:
         *device_path = dp;
         *file_path = fp;
         return EFI_SUCCESS;

I would prefer to go for the more generic version.

Best regards

Heinrich

[U-Boot] [RFC v3 02/10] efi_loader: export root node handle

[AKASHI Takahiro]

This is a preparatory patch.
The root node handle will be used as a dummy parent handle when invoking
an EFI image from bootefi/bootmgr command.

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

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 00b81c6010ff..d524dc7e24c1 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -25,6 +25,8 @@
 	EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
 		 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
 
+extern efi_handle_t efi_root;
+
 int __efi_entry_check(void);
 int __efi_exit_check(void);
 const char *__efi_nesting(void);
diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
index b056ba3ee880..f2521a64091c 100644
--- a/lib/efi_loader/efi_root_node.c
+++ b/lib/efi_loader/efi_root_node.c
@@ -10,6 +10,7 @@
 #include <efi_loader.h>
 
 const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
+efi_handle_t efi_root;
 
 struct efi_root_dp {
 	struct efi_device_path_vendor vendor;
@@ -26,12 +27,11 @@ struct efi_root_dp {
  */
 efi_status_t efi_root_node_register(void)
 {
-	efi_handle_t root;
 	efi_status_t ret;
 	struct efi_root_dp *dp;
 
 	/* Create handle */
-	ret = efi_create_handle(&root);
+	ret = efi_create_handle(&efi_root);
 	if (ret != EFI_SUCCESS)
 		return ret;
 
@@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
 	dp->end.length = sizeof(struct efi_device_path);
 
 	/* Install device path protocol */
-	ret = efi_add_protocol(root, &efi_guid_device_path, dp);
+	ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
 	if (ret != EFI_SUCCESS)
 		goto failure;
 
 	/* Install device path to text protocol */
-	ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
+	ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
 			       (void *)&efi_device_path_to_text);
 	if (ret != EFI_SUCCESS)
 		goto failure;
 
 	/* Install device path utilities protocol */
-	ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
+	ret = efi_add_protocol(efi_root,
+			       &efi_guid_device_path_utilities_protocol,
 			       (void *)&efi_device_path_utilities);
 	if (ret != EFI_SUCCESS)
 		goto failure;
 
 	/* Install Unicode collation protocol */
-	ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
+	ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
 			       (void *)&efi_unicode_collation_protocol);
 	if (ret != EFI_SUCCESS)
 		goto failure;
-- 
2.20.1

[U-Boot] [RFC v3 02/10] efi_loader: export root node handle

[Heinrich Schuchardt]

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> This is a preparatory patch.
> The root node handle will be used as a dummy parent handle when invoking
> an EFI image from bootefi/bootmgr command.

This patch is not based on the efi-2019-07 branch.

Please, rebase your patch series.

Best regards

Heinrich

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   include/efi_loader.h           |  2 ++
>   lib/efi_loader/efi_root_node.c | 13 +++++++------
>   2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 00b81c6010ff..d524dc7e24c1 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -25,6 +25,8 @@
>   	EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
>   		 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
>
> +extern efi_handle_t efi_root;
> +
>   int __efi_entry_check(void);
>   int __efi_exit_check(void);
>   const char *__efi_nesting(void);
> diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> index b056ba3ee880..f2521a64091c 100644
> --- a/lib/efi_loader/efi_root_node.c
> +++ b/lib/efi_loader/efi_root_node.c
> @@ -10,6 +10,7 @@
>   #include <efi_loader.h>
>
>   const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
> +efi_handle_t efi_root;
>
>   struct efi_root_dp {
>   	struct efi_device_path_vendor vendor;
> @@ -26,12 +27,11 @@ struct efi_root_dp {
>    */
>   efi_status_t efi_root_node_register(void)
>   {
> -	efi_handle_t root;
>   	efi_status_t ret;
>   	struct efi_root_dp *dp;
>
>   	/* Create handle */
> -	ret = efi_create_handle(&root);
> +	ret = efi_create_handle(&efi_root);
>   	if (ret != EFI_SUCCESS)
>   		return ret;
>
> @@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
>   	dp->end.length = sizeof(struct efi_device_path);
>
>   	/* Install device path protocol */
> -	ret = efi_add_protocol(root, &efi_guid_device_path, dp);
> +	ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
>   	if (ret != EFI_SUCCESS)
>   		goto failure;
>
>   	/* Install device path to text protocol */
> -	ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
> +	ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
>   			       (void *)&efi_device_path_to_text);
>   	if (ret != EFI_SUCCESS)
>   		goto failure;
>
>   	/* Install device path utilities protocol */
> -	ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
> +	ret = efi_add_protocol(efi_root,
> +			       &efi_guid_device_path_utilities_protocol,
>   			       (void *)&efi_device_path_utilities);
>   	if (ret != EFI_SUCCESS)
>   		goto failure;
>
>   	/* Install Unicode collation protocol */
> -	ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
> +	ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
>   			       (void *)&efi_unicode_collation_protocol);
>   	if (ret != EFI_SUCCESS)
>   		goto failure;
>

[U-Boot] [RFC v3 02/10] efi_loader: export root node handle

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 06:48:46AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >This is a preparatory patch.
> >The root node handle will be used as a dummy parent handle when invoking
> >an EFI image from bootefi/bootmgr command.
> 
> This patch is not based on the efi-2019-07 branch.

This is intentional.

> Please, rebase your patch series.

As efi-2019-07 can be updated frequently and get old quickly (day by day),
in particular before or around rc1 and rc2, I don't want to rebase my patch,
instead look forward to the consensus to my approach first.
That is why I have sent out this series as a RFC.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  include/efi_loader.h           |  2 ++
> >  lib/efi_loader/efi_root_node.c | 13 +++++++------
> >  2 files changed, 9 insertions(+), 6 deletions(-)
> >
> >diff --git a/include/efi_loader.h b/include/efi_loader.h
> >index 00b81c6010ff..d524dc7e24c1 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -25,6 +25,8 @@
> >  	EFI_GUID(0xe61d73b9, 0xa384, 0x4acc, \
> >  		 0xae, 0xab, 0x82, 0xe8, 0x28, 0xf3, 0x62, 0x8b)
> >
> >+extern efi_handle_t efi_root;
> >+
> >  int __efi_entry_check(void);
> >  int __efi_exit_check(void);
> >  const char *__efi_nesting(void);
> >diff --git a/lib/efi_loader/efi_root_node.c b/lib/efi_loader/efi_root_node.c
> >index b056ba3ee880..f2521a64091c 100644
> >--- a/lib/efi_loader/efi_root_node.c
> >+++ b/lib/efi_loader/efi_root_node.c
> >@@ -10,6 +10,7 @@
> >  #include <efi_loader.h>
> >
> >  const efi_guid_t efi_u_boot_guid = U_BOOT_GUID;
> >+efi_handle_t efi_root;
> >
> >  struct efi_root_dp {
> >  	struct efi_device_path_vendor vendor;
> >@@ -26,12 +27,11 @@ struct efi_root_dp {
> >   */
> >  efi_status_t efi_root_node_register(void)
> >  {
> >-	efi_handle_t root;
> >  	efi_status_t ret;
> >  	struct efi_root_dp *dp;
> >
> >  	/* Create handle */
> >-	ret = efi_create_handle(&root);
> >+	ret = efi_create_handle(&efi_root);
> >  	if (ret != EFI_SUCCESS)
> >  		return ret;
> >
> >@@ -52,24 +52,25 @@ efi_status_t efi_root_node_register(void)
> >  	dp->end.length = sizeof(struct efi_device_path);
> >
> >  	/* Install device path protocol */
> >-	ret = efi_add_protocol(root, &efi_guid_device_path, dp);
> >+	ret = efi_add_protocol(efi_root, &efi_guid_device_path, dp);
> >  	if (ret != EFI_SUCCESS)
> >  		goto failure;
> >
> >  	/* Install device path to text protocol */
> >-	ret = efi_add_protocol(root, &efi_guid_device_path_to_text_protocol,
> >+	ret = efi_add_protocol(efi_root, &efi_guid_device_path_to_text_protocol,
> >  			       (void *)&efi_device_path_to_text);
> >  	if (ret != EFI_SUCCESS)
> >  		goto failure;
> >
> >  	/* Install device path utilities protocol */
> >-	ret = efi_add_protocol(root, &efi_guid_device_path_utilities_protocol,
> >+	ret = efi_add_protocol(efi_root,
> >+			       &efi_guid_device_path_utilities_protocol,
> >  			       (void *)&efi_device_path_utilities);
> >  	if (ret != EFI_SUCCESS)
> >  		goto failure;
> >
> >  	/* Install Unicode collation protocol */
> >-	ret = efi_add_protocol(root, &efi_guid_unicode_collation_protocol,
> >+	ret = efi_add_protocol(efi_root, &efi_guid_unicode_collation_protocol,
> >  			       (void *)&efi_unicode_collation_protocol);
> >  	if (ret != EFI_SUCCESS)
> >  		goto failure;
> >
> 

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

[U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()

[Heinrich Schuchardt]

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi() in later patch.

I would prefer a more informative commit message like:

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

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

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

[U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >This is a preparatory patch for reworking do_bootefi() in later patch.
> 
> I would prefer a more informative commit message like:
> 
> Carve out a function to handle the installation of the device tree as a
> configuration table.

Okay, but it doesn't convey more than the subject.

-Takahiro Akashi

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

[U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()

[Heinrich Schuchardt]

On 4/17/19 9:01 AM, AKASHI Takahiro wrote:
> On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
>> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
>>> This is a preparatory patch for reworking do_bootefi() in later patch.
>>
>> I would prefer a more informative commit message like:
>>
>> Carve out a function to handle the installation of the device tree as a
>> configuration table.
>
> Okay, but it doesn't convey more than the subject.
>
> -Takahiro Akashi
>
>> Otherwise
>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>>
>>>
>>> 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..8cd9644115eb 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -198,6 +198,38 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
>>>   	return ret;
>>>   }
>>>
>>> +/**
>>> + * efi_process_fdt() - process fdt passed by a command argument
>>> + * @fdt_opt:	pointer to argument
>>> + * Return:	status code
>>> + *
>>> + * If specified, fdt will be installed as configuration table,
>>> + * otherwise no fdt will be passed.
>>> + */
>>> +static efi_status_t efi_process_fdt(const char *fdt_opt)
>>> +{
>>> +	unsigned long fdt_addr;
>>> +	efi_status_t ret;
>>> +
>>> +	if (fdt_opt) {
>>> +		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
>>> +		if (!fdt_addr && *fdt_opt != '0')
>>> +			return EFI_INVALID_PARAMETER;
>>> +
>>> +		/* Install device tree */
>>> +		ret = efi_install_fdt(fdt_addr);
>>> +		if (ret != EFI_SUCCESS) {
>>> +			printf("ERROR: failed to install device tree\n");
>>> +			return ret;
>>> +		}
>>> +	} else {
>>> +		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
>>> +		efi_install_configuration_table(&efi_guid_fdt, NULL);
>>> +	}
>>> +
>>> +	return EFI_SUCCESS;
>>> +}
>>> +
>>>   static efi_status_t bootefi_run_prepare(const char *load_options_path,
>>>   		struct efi_device_path *device_path,
>>>   		struct efi_device_path *image_path,
>>> @@ -407,21 +439,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>>>   	if (argc < 2)
>>>   		return CMD_RET_USAGE;
>>>
>>> -	if (argc > 2) {
>>> -		fdt_addr = simple_strtoul(argv[2], NULL, 16);
>>> -		if (!fdt_addr && *argv[2] != '0')
>>> -			return CMD_RET_USAGE;
>>> -		/* Install device tree */
>>> -		r = efi_install_fdt(fdt_addr);
>>> -		if (r != EFI_SUCCESS) {
>>> -			printf("ERROR: failed to install device tree\n");
>>> -			return CMD_RET_FAILURE;
>>> -		}
>>> -	} else {
>>> -		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
>>> -		efi_install_configuration_table(&efi_guid_fdt, NULL);
>>> -		printf("WARNING: booting without device tree\n");
>>> -	}
>>> +	r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);

cmd/bootefi.c: In function ‘do_bootefi’:
cmd/bootefi.c:442:6: warning: implicit declaration of function
‘fdt_process_fdt’; did you mean ‘efi_process_fdt’?
[-Wimplicit-function-declaration]
   r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
       ^~~~~~~~~~~~~~~
       efi_process_fdt
cmd/bootefi.c:424:16: warning: unused variable ‘fdt_addr’
[-Wunused-variable]
   unsigned long fdt_addr;
                 ^~~~~~~~
At top level:
   CC      drivers/virtio/virtio_pci_legacy.o
cmd/bootefi.c:209:21: warning: ‘efi_process_fdt’ defined but not used
[-Wunused-function]
  static efi_status_t efi_process_fdt(const char *fdt_opt)
                      ^~~~~~~~~~~~~~~

Please, check.

Best regards

Heinrich

>>> +	if (r == EFI_INVALID_PARAMETER)
>>> +		return CMD_RET_USAGE;
>>> +	else if (r != EFI_SUCCESS)
>>> +		return CMD_RET_FAILURE;
>>> +
>>>   #ifdef CONFIG_CMD_BOOTEFI_HELLO
>>>   	if (!strcmp(argv[1], "hello")) {
>>>   		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>>>
>>
>

[U-Boot] [RFC v3 03/10] cmd: bootefi: carve out fdt handling from do_bootefi()

[AKASHI Takahiro]

On Wed, Apr 17, 2019 at 06:35:57PM +0200, Heinrich Schuchardt wrote:
> On 4/17/19 9:01 AM, AKASHI Takahiro wrote:
> >On Tue, Apr 16, 2019 at 06:54:58AM +0200, Heinrich Schuchardt wrote:
> >>On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >>>This is a preparatory patch for reworking do_bootefi() in later patch.
> >>
> >>I would prefer a more informative commit message like:
> >>
> >>Carve out a function to handle the installation of the device tree as a
> >>configuration table.
> >
> >Okay, but it doesn't convey more than the subject.
> >
> >-Takahiro Akashi
> >
> >>Otherwise
> >>Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >>
> >>>
> >>>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..8cd9644115eb 100644
> >>>--- a/cmd/bootefi.c
> >>>+++ b/cmd/bootefi.c
> >>>@@ -198,6 +198,38 @@ static efi_status_t efi_install_fdt(ulong fdt_addr)
> >>>  	return ret;
> >>>  }
> >>>
> >>>+/**
> >>>+ * efi_process_fdt() - process fdt passed by a command argument
> >>>+ * @fdt_opt:	pointer to argument
> >>>+ * Return:	status code
> >>>+ *
> >>>+ * If specified, fdt will be installed as configuration table,
> >>>+ * otherwise no fdt will be passed.
> >>>+ */
> >>>+static efi_status_t efi_process_fdt(const char *fdt_opt)
> >>>+{
> >>>+	unsigned long fdt_addr;
> >>>+	efi_status_t ret;
> >>>+
> >>>+	if (fdt_opt) {
> >>>+		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
> >>>+		if (!fdt_addr && *fdt_opt != '0')
> >>>+			return EFI_INVALID_PARAMETER;
> >>>+
> >>>+		/* Install device tree */
> >>>+		ret = efi_install_fdt(fdt_addr);
> >>>+		if (ret != EFI_SUCCESS) {
> >>>+			printf("ERROR: failed to install device tree\n");
> >>>+			return ret;
> >>>+		}
> >>>+	} else {
> >>>+		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> >>>+		efi_install_configuration_table(&efi_guid_fdt, NULL);
> >>>+	}
> >>>+
> >>>+	return EFI_SUCCESS;
> >>>+}
> >>>+
> >>>  static efi_status_t bootefi_run_prepare(const char *load_options_path,
> >>>  		struct efi_device_path *device_path,
> >>>  		struct efi_device_path *image_path,
> >>>@@ -407,21 +439,12 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >>>  	if (argc < 2)
> >>>  		return CMD_RET_USAGE;
> >>>
> >>>-	if (argc > 2) {
> >>>-		fdt_addr = simple_strtoul(argv[2], NULL, 16);
> >>>-		if (!fdt_addr && *argv[2] != '0')
> >>>-			return CMD_RET_USAGE;
> >>>-		/* Install device tree */
> >>>-		r = efi_install_fdt(fdt_addr);
> >>>-		if (r != EFI_SUCCESS) {
> >>>-			printf("ERROR: failed to install device tree\n");
> >>>-			return CMD_RET_FAILURE;
> >>>-		}
> >>>-	} else {
> >>>-		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> >>>-		efi_install_configuration_table(&efi_guid_fdt, NULL);
> >>>-		printf("WARNING: booting without device tree\n");
> >>>-	}
> >>>+	r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
> 
> cmd/bootefi.c: In function ‘do_bootefi’:
> cmd/bootefi.c:442:6: warning: implicit declaration of function
> ‘fdt_process_fdt’; did you mean ‘efi_process_fdt’?
> [-Wimplicit-function-declaration]
>   r = fdt_process_fdt(argc > 2 ? argv[2] : NULL);
>       ^~~~~~~~~~~~~~~
>       efi_process_fdt

Oops. I think that I fixed it before.
Will fix along with patch#4.

> cmd/bootefi.c:424:16: warning: unused variable ‘fdt_addr’
> [-Wunused-variable]
>   unsigned long fdt_addr;
>                 ^~~~~~~~

Okay.

Thanks,
-Takahiro Akashi
> At top level:
>   CC      drivers/virtio/virtio_pci_legacy.o
> cmd/bootefi.c:209:21: warning: ‘efi_process_fdt’ defined but not used
> [-Wunused-function]
>  static efi_status_t efi_process_fdt(const char *fdt_opt)
>                      ^~~~~~~~~~~~~~~
> 
> Please, check.
> 
> Best regards
> 
> Heinrich
> 
> >>>+	if (r == EFI_INVALID_PARAMETER)
> >>>+		return CMD_RET_USAGE;
> >>>+	else if (r != EFI_SUCCESS)
> >>>+		return CMD_RET_FAILURE;
> >>>+
> >>>  #ifdef CONFIG_CMD_BOOTEFI_HELLO
> >>>  	if (!strcmp(argv[1], "hello")) {
> >>>  		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >>>
> >>
> >
> 

[U-Boot] [RFC v3 04/10] 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 8cd9644115eb..0f58f51cbc76 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -165,62 +165,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
 	}
 }
 
-static efi_status_t efi_install_fdt(ulong fdt_addr)
-{
-	bootm_headers_t img = { 0 };
-	efi_status_t ret;
-	void *fdt;
-
-	fdt = map_sysmem(fdt_addr, 0);
-	if (fdt_check_header(fdt)) {
-		printf("ERROR: invalid device tree\n");
-		return EFI_INVALID_PARAMETER;
-	}
-
-	/* Create memory reservation as indicated by the device tree */
-	efi_carve_out_dt_rsv(fdt);
-
-	/* Prepare fdt for payload */
-	ret = copy_fdt(&fdt);
-	if (ret)
-		return ret;
-
-	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
-		printf("ERROR: failed to process device tree\n");
-		return EFI_LOAD_ERROR;
-	}
-
-	/* Link to it in the efi tables */
-	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
-	if (ret != EFI_SUCCESS)
-		return EFI_OUT_OF_RESOURCES;
-
-	return ret;
-}
-
 /**
- * efi_process_fdt() - process fdt passed by a command argument
+ * efi_install_fdt() - install fdt passed by a command argument
  * @fdt_opt:	pointer to argument
  * Return:	status code
  *
  * If specified, fdt will be installed as configuration table,
  * otherwise no fdt will be passed.
  */
-static efi_status_t efi_process_fdt(const char *fdt_opt)
+static efi_status_t efi_install_fdt(const char *fdt_opt)
 {
 	unsigned long fdt_addr;
+	void *fdt;
+	bootm_headers_t img = { 0 };
 	efi_status_t ret;
 
 	if (fdt_opt) {
+		/* Install device tree */
 		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
 		if (!fdt_addr && *fdt_opt != '0')
 			return EFI_INVALID_PARAMETER;
 
-		/* Install device tree */
-		ret = efi_install_fdt(fdt_addr);
+		fdt = map_sysmem(fdt_addr, 0);
+		if (fdt_check_header(fdt)) {
+			printf("ERROR: invalid device tree\n");
+			return EFI_INVALID_PARAMETER;
+		}
+
+		/* Create memory reservation as indicated by the device tree */
+		efi_carve_out_dt_rsv(fdt);
+
+		/* Prepare fdt for payload */
+		ret = copy_fdt(&fdt);
+		if (ret)
+			return ret;
+
+		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
+			printf("ERROR: failed to process device tree\n");
+			return EFI_LOAD_ERROR;
+		}
+
+		/* Link to it in the efi tables */
+		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
 		if (ret != EFI_SUCCESS) {
 			printf("ERROR: failed to install device tree\n");
-			return ret;
+			return EFI_OUT_OF_RESOURCES;
 		}
 	} else {
 		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
@@ -439,7 +428,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_INVALID_PARAMETER)
 		return CMD_RET_USAGE;
 	else if (r != EFI_SUCCESS)
-- 
2.20.1

[U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[Heinrich Schuchardt]

On 4/16/19 6:24 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>

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

> ---
>   cmd/bootefi.c | 67 +++++++++++++++++++++------------------------------
>   1 file changed, 28 insertions(+), 39 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 8cd9644115eb..0f58f51cbc76 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -165,62 +165,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
>   	}
>   }
>
> -static efi_status_t efi_install_fdt(ulong fdt_addr)
> -{
> -	bootm_headers_t img = { 0 };
> -	efi_status_t ret;
> -	void *fdt;
> -
> -	fdt = map_sysmem(fdt_addr, 0);
> -	if (fdt_check_header(fdt)) {
> -		printf("ERROR: invalid device tree\n");
> -		return EFI_INVALID_PARAMETER;
> -	}
> -
> -	/* Create memory reservation as indicated by the device tree */
> -	efi_carve_out_dt_rsv(fdt);
> -
> -	/* Prepare fdt for payload */
> -	ret = copy_fdt(&fdt);
> -	if (ret)
> -		return ret;
> -
> -	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> -		printf("ERROR: failed to process device tree\n");
> -		return EFI_LOAD_ERROR;
> -	}
> -
> -	/* Link to it in the efi tables */
> -	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> -	if (ret != EFI_SUCCESS)
> -		return EFI_OUT_OF_RESOURCES;
> -
> -	return ret;
> -}
> -
>   /**
> - * efi_process_fdt() - process fdt passed by a command argument
> + * efi_install_fdt() - install fdt passed by a command argument
>    * @fdt_opt:	pointer to argument
>    * Return:	status code
>    *
>    * If specified, fdt will be installed as configuration table,
>    * otherwise no fdt will be passed.
>    */
> -static efi_status_t efi_process_fdt(const char *fdt_opt)
> +static efi_status_t efi_install_fdt(const char *fdt_opt)
>   {
>   	unsigned long fdt_addr;
> +	void *fdt;
> +	bootm_headers_t img = { 0 };
>   	efi_status_t ret;
>
>   	if (fdt_opt) {
> +		/* Install device tree */
>   		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
>   		if (!fdt_addr && *fdt_opt != '0')
>   			return EFI_INVALID_PARAMETER;
>
> -		/* Install device tree */
> -		ret = efi_install_fdt(fdt_addr);
> +		fdt = map_sysmem(fdt_addr, 0);
> +		if (fdt_check_header(fdt)) {
> +			printf("ERROR: invalid device tree\n");
> +			return EFI_INVALID_PARAMETER;
> +		}
> +
> +		/* Create memory reservation as indicated by the device tree */
> +		efi_carve_out_dt_rsv(fdt);
> +
> +		/* Prepare fdt for payload */
> +		ret = copy_fdt(&fdt);
> +		if (ret)
> +			return ret;
> +
> +		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> +			printf("ERROR: failed to process device tree\n");
> +			return EFI_LOAD_ERROR;
> +		}
> +
> +		/* Link to it in the efi tables */
> +		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>   		if (ret != EFI_SUCCESS) {
>   			printf("ERROR: failed to install device tree\n");
> -			return ret;
> +			return EFI_OUT_OF_RESOURCES;
>   		}
>   	} else {
>   		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> @@ -439,7 +428,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_INVALID_PARAMETER)
>   		return CMD_RET_USAGE;
>   	else if (r != EFI_SUCCESS)
>

[U-Boot] [RFC v3 04/10] cmd: bootefi: merge efi_install_fdt() and efi_process_fdt()

[Heinrich Schuchardt]

On 4/16/19 6:24 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 8cd9644115eb..0f58f51cbc76 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -165,62 +165,51 @@ static void efi_carve_out_dt_rsv(void *fdt)
>   	}
>   }
>
> -static efi_status_t efi_install_fdt(ulong fdt_addr)
> -{
> -	bootm_headers_t img = { 0 };
> -	efi_status_t ret;
> -	void *fdt;
> -
> -	fdt = map_sysmem(fdt_addr, 0);
> -	if (fdt_check_header(fdt)) {
> -		printf("ERROR: invalid device tree\n");
> -		return EFI_INVALID_PARAMETER;
> -	}
> -
> -	/* Create memory reservation as indicated by the device tree */
> -	efi_carve_out_dt_rsv(fdt);
> -
> -	/* Prepare fdt for payload */
> -	ret = copy_fdt(&fdt);
> -	if (ret)
> -		return ret;
> -
> -	if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> -		printf("ERROR: failed to process device tree\n");
> -		return EFI_LOAD_ERROR;
> -	}
> -
> -	/* Link to it in the efi tables */
> -	ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
> -	if (ret != EFI_SUCCESS)
> -		return EFI_OUT_OF_RESOURCES;
> -
> -	return ret;
> -}
> -
>   /**
> - * efi_process_fdt() - process fdt passed by a command argument
> + * efi_install_fdt() - install fdt passed by a command argument
>    * @fdt_opt:	pointer to argument
>    * Return:	status code
>    *
>    * If specified, fdt will be installed as configuration table,
>    * otherwise no fdt will be passed.
>    */
> -static efi_status_t efi_process_fdt(const char *fdt_opt)
> +static efi_status_t efi_install_fdt(const char *fdt_opt)
>   {
>   	unsigned long fdt_addr;
> +	void *fdt;
> +	bootm_headers_t img = { 0 };
>   	efi_status_t ret;
>
>   	if (fdt_opt) {
> +		/* Install device tree */
>   		fdt_addr = simple_strtoul(fdt_opt, NULL, 16);
>   		if (!fdt_addr && *fdt_opt != '0')
>   			return EFI_INVALID_PARAMETER;
>
> -		/* Install device tree */
> -		ret = efi_install_fdt(fdt_addr);
> +		fdt = map_sysmem(fdt_addr, 0);
> +		if (fdt_check_header(fdt)) {
> +			printf("ERROR: invalid device tree\n");
> +			return EFI_INVALID_PARAMETER;
> +		}
> +
> +		/* Create memory reservation as indicated by the device tree */
> +		efi_carve_out_dt_rsv(fdt);
> +
> +		/* Prepare fdt for payload */
> +		ret = copy_fdt(&fdt);
> +		if (ret)
> +			return ret;
> +
> +		if (image_setup_libfdt(&img, fdt, 0, NULL)) {
> +			printf("ERROR: failed to process device tree\n");
> +			return EFI_LOAD_ERROR;
> +		}
> +
> +		/* Link to it in the efi tables */
> +		ret = efi_install_configuration_table(&efi_guid_fdt, fdt);
>   		if (ret != EFI_SUCCESS) {
>   			printf("ERROR: failed to install device tree\n");
> -			return ret;
> +			return EFI_OUT_OF_RESOURCES;
>   		}
>   	} else {
>   		/* Remove device tree. EFI_NOT_FOUND can be ignored here */
> @@ -439,7 +428,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);


cmd/bootefi.c: In function ‘do_bootefi’:
cmd/bootefi.c:431:6: warning: implicit declaration of function
‘fdt_install_fdt’; did you mean ‘efi_install_fdt’?
[-Wimplicit-function-declaration]
   r = fdt_install_fdt(argc > 2 ? argv[2] : NULL);
       ^~~~~~~~~~~~~~~
       efi_install_fdt
cmd/bootefi.c:413:16: warning: unused variable ‘fdt_addr’
[-Wunused-variable]
   unsigned long fdt_addr;
                 ^~~~~~~~
At top level:
cmd/bootefi.c:176:21: warning: ‘efi_install_fdt’ defined but not used
[-Wunused-function]
  static efi_status_t efi_install_fdt(const char *fdt_opt)

Please, check.

Best regards

Heinrich

>   	if (r == EFI_INVALID_PARAMETER)
>   		return CMD_RET_USAGE;
>   	else if (r != EFI_SUCCESS)
>

[U-Boot] [RFC v3 05/10] 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 | 147 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 92 insertions(+), 55 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 0f58f51cbc76..a5dba6645ca2 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -219,39 +219,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
  *
@@ -298,11 +265,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)
@@ -326,7 +296,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)
@@ -336,6 +308,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
  *
@@ -381,6 +372,64 @@ failure:
 	return ret;
 }
 
+/**
+ * bootefi_run_finish() - finish up after running an EFI test
+ *
+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
+ * @image_obj: Pointer to a struct which holds the loaded image object
+ */
+static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
+			       struct efi_loaded_image *loaded_image_info)
+{
+	efi_restore_gd();
+	free(loaded_image_info->load_options);
+	efi_delete_handle(&image_obj->header);
+}
+
+/**
+ * do_efi_selftest() - execute EFI Selftest
+ *
+ * @fdt_opt:	string of fdt start address
+ * Return:	status code
+ *
+ * Execute EFI Selftest
+ */
+static int do_efi_selftest(const char *fdt_opt)
+{
+	struct efi_loaded_image_obj *image_obj;
+	struct efi_loaded_image *loaded_image_info;
+	efi_status_t ret;
+
+	/* Allow unaligned memory access */
+	allow_unaligned();
+
+	switch_to_non_secure_mode();
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       ret & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = efi_install_fdt(fdt_opt);
+	if (ret == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	ret = bootefi_test_prepare(&image_obj, &loaded_image_info,
+				   "\\selftest", "efi_selftest");
+	if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
+
+	/* Execute the test */
+	ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
+	bootefi_run_finish(image_obj, loaded_image_info);
+
+	return ret != EFI_SUCCESS;
+}
 #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
 
 static int do_bootefi_bootmgr_exec(void)
@@ -412,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();
 
@@ -425,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_INVALID_PARAMETER)
 		return CMD_RET_USAGE;
@@ -445,22 +498,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 v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[Heinrich Schuchardt]

On 4/16/19 6:24 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 | 147 +++++++++++++++++++++++++++++++-------------------
>   1 file changed, 92 insertions(+), 55 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0f58f51cbc76..a5dba6645ca2 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -219,39 +219,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
>    *
> @@ -298,11 +265,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)
> @@ -326,7 +296,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)
> @@ -336,6 +308,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
>    *
> @@ -381,6 +372,64 @@ failure:
>   	return ret;
>   }
>
> +/**
> + * bootefi_run_finish() - finish up after running an EFI test
> + *
> + * @loaded_image_info: Pointer to a struct which holds the loaded image info
> + * @image_obj: Pointer to a struct which holds the loaded image object
> + */
> +static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> +			       struct efi_loaded_image *loaded_image_info)
> +{
> +	efi_restore_gd();
> +	free(loaded_image_info->load_options);
> +	efi_delete_handle(&image_obj->header);
> +}
> +
> +/**
> + * do_efi_selftest() - execute EFI Selftest
> + *
> + * @fdt_opt:	string of fdt start address
> + * Return:	status code
> + *
> + * Execute EFI Selftest
> + */
> +static int do_efi_selftest(const char *fdt_opt)
> +{
> +	struct efi_loaded_image_obj *image_obj;
> +	struct efi_loaded_image *loaded_image_info;
> +	efi_status_t ret;
> +
> +	/* Allow unaligned memory access */
> +	allow_unaligned();
> +
> +	switch_to_non_secure_mode();
> +
> +	/* Initialize EFI drivers */
> +	ret = efi_init_obj_list();
> +	if (ret != EFI_SUCCESS) {
> +		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> +		       ret & ~EFI_ERROR_MASK);
> +		return CMD_RET_FAILURE;
> +	}

allow_unaligned(), switch_to_non_secure_mode(), and efi_init_obj_list()
are needed for any invocation do_bootefi(). Putting them here makes only
sense if you want to create a completely separate command for the selftest.

> +
> +	ret = efi_install_fdt(fdt_opt);
> +	if (ret == EFI_INVALID_PARAMETER)
> +		return CMD_RET_USAGE;
> +	else if (ret != EFI_SUCCESS)
> +		return CMD_RET_FAILURE;
> +
> +	ret = bootefi_test_prepare(&image_obj, &loaded_image_info,
> +				   "\\selftest", "efi_selftest");
> +	if (ret != EFI_SUCCESS)
> +		return CMD_RET_FAILURE;
> +
> +	/* Execute the test */
> +	ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
> +	bootefi_run_finish(image_obj, loaded_image_info);
> +
> +	return ret != EFI_SUCCESS;
> +}
>   #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
>
>   static int do_bootefi_bootmgr_exec(void)
> @@ -412,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);

doc/README.commands describes the default way to handle sub-commands.
I think that is where we should move in the long run. Maybe not in this
patch series.

Best regards

Heinrich

> +#endif
> +
>   	/* Allow unaligned memory access */
>   	allow_unaligned();
>
> @@ -425,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_INVALID_PARAMETER)
>   		return CMD_RET_USAGE;
> @@ -445,22 +498,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 v3 05/10] cmd: bootefi: carve out efi_selftest code from do_bootefi()

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 07:21:26AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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 | 147 +++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 92 insertions(+), 55 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 0f58f51cbc76..a5dba6645ca2 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -219,39 +219,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
> >   *
> >@@ -298,11 +265,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)
> >@@ -326,7 +296,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)
> >@@ -336,6 +308,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
> >   *
> >@@ -381,6 +372,64 @@ failure:
> >  	return ret;
> >  }
> >
> >+/**
> >+ * bootefi_run_finish() - finish up after running an EFI test
> >+ *
> >+ * @loaded_image_info: Pointer to a struct which holds the loaded image info
> >+ * @image_obj: Pointer to a struct which holds the loaded image object
> >+ */
> >+static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
> >+			       struct efi_loaded_image *loaded_image_info)
> >+{
> >+	efi_restore_gd();
> >+	free(loaded_image_info->load_options);
> >+	efi_delete_handle(&image_obj->header);
> >+}
> >+
> >+/**
> >+ * do_efi_selftest() - execute EFI Selftest
> >+ *
> >+ * @fdt_opt:	string of fdt start address
> >+ * Return:	status code
> >+ *
> >+ * Execute EFI Selftest
> >+ */
> >+static int do_efi_selftest(const char *fdt_opt)
> >+{
> >+	struct efi_loaded_image_obj *image_obj;
> >+	struct efi_loaded_image *loaded_image_info;
> >+	efi_status_t ret;
> >+
> >+	/* Allow unaligned memory access */
> >+	allow_unaligned();
> >+
> >+	switch_to_non_secure_mode();
> >+
> >+	/* Initialize EFI drivers */
> >+	ret = efi_init_obj_list();
> >+	if (ret != EFI_SUCCESS) {
> >+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
> >+		       ret & ~EFI_ERROR_MASK);
> >+		return CMD_RET_FAILURE;
> >+	}
> 
> allow_unaligned(), switch_to_non_secure_mode(), and efi_init_obj_list()
> are needed for any invocation do_bootefi(). Putting them here makes only
> sense if you want to create a completely separate command for the selftest.

First of all, this is a preparation for do_efibootmgr().
In addition, as I suggested, we should also convert selftest to
a standalone application.

> >+
> >+	ret = efi_install_fdt(fdt_opt);
> >+	if (ret == EFI_INVALID_PARAMETER)
> >+		return CMD_RET_USAGE;
> >+	else if (ret != EFI_SUCCESS)
> >+		return CMD_RET_FAILURE;
> >+
> >+	ret = bootefi_test_prepare(&image_obj, &loaded_image_info,
> >+				   "\\selftest", "efi_selftest");
> >+	if (ret != EFI_SUCCESS)
> >+		return CMD_RET_FAILURE;
> >+
> >+	/* Execute the test */
> >+	ret = EFI_CALL(efi_selftest(&image_obj->header, &systab));
> >+	bootefi_run_finish(image_obj, loaded_image_info);
> >+
> >+	return ret != EFI_SUCCESS;
> >+}
> >  #endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
> >
> >  static int do_bootefi_bootmgr_exec(void)
> >@@ -412,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);
> 
> doc/README.commands describes the default way to handle sub-commands.
> I think that is where we should move in the long run. Maybe not in this
> patch series.

Quit easy to follow.

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >+#endif
> >+
> >  	/* Allow unaligned memory access */
> >  	allow_unaligned();
> >
> >@@ -425,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_INVALID_PARAMETER)
> >  		return CMD_RET_USAGE;
> >@@ -445,22 +498,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 v3 06/10] 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 a5dba6645ca2..10fe10cb4daf 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -307,6 +307,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 v3 06/10] cmd: bootefi: move do_bootefi_bootmgr_exec() forward

[Heinrich Schuchardt]

On 4/16/19 6:24 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>

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

> ---
>   cmd/bootefi.c | 42 +++++++++++++++++++++---------------------
>   1 file changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index a5dba6645ca2..10fe10cb4daf 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -307,6 +307,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 v3 07/10] cmd: bootefi: carve out bootmgr code from do_bootefi()

[AKASHI Takahiro]

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

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 10fe10cb4daf..7cc6f0ee5ac8 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -307,22 +307,49 @@ err_add_protocol:
 	return ret;
 }
 
-static int do_bootefi_bootmgr_exec(void)
+/**
+ * do_efibootmgr() - execute EFI Boot Manager
+ *
+ * @fdt_opt:	string of fdt start address
+ * Return:	status code
+ *
+ * Execute EFI Boot Manager
+ */
+static int do_efibootmgr(const char *fdt_opt)
 {
 	struct efi_device_path *device_path, *file_path;
 	void *addr;
-	efi_status_t r;
+	efi_status_t ret;
+
+	/* Allow unaligned memory access */
+	allow_unaligned();
+
+	switch_to_non_secure_mode();
+
+	/* Initialize EFI drivers */
+	ret = efi_init_obj_list();
+	if (ret != EFI_SUCCESS) {
+		printf("Error: Cannot initialize UEFI sub-system, r = %lu\n",
+		       ret & ~EFI_ERROR_MASK);
+		return CMD_RET_FAILURE;
+	}
+
+	ret = efi_install_fdt(fdt_opt);
+	if (ret == EFI_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else if (ret != EFI_SUCCESS)
+		return CMD_RET_FAILURE;
 
 	addr = efi_bootmgr_load(&device_path, &file_path);
 	if (!addr)
 		return 1;
 
 	printf("## Starting EFI application at %p ...\n", addr);
-	r = do_bootefi_exec(addr, device_path, file_path);
+	ret = do_bootefi_exec(addr, device_path, file_path);
 	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
+	       ret & ~EFI_ERROR_MASK);
 
-	if (r != EFI_SUCCESS)
+	if (ret != EFI_SUCCESS)
 		return 1;
 
 	return 0;
@@ -463,6 +490,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);
@@ -499,9 +529,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 v3 08/10] 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 | 126 ++++++++++++++++++++++++++++----------------------
 1 file changed, 70 insertions(+), 56 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 7cc6f0ee5ac8..f14966961b23 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -355,6 +355,75 @@ 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_INVALID_PARAMETER)
+		return CMD_RET_USAGE;
+	else 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,
@@ -483,11 +552,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;
 
@@ -498,57 +562,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_INVALID_PARAMETER)
-		return CMD_RET_USAGE;
-	else if (r != EFI_SUCCESS)
-		return CMD_RET_FAILURE;
-
-#ifdef CONFIG_CMD_BOOTEFI_HELLO
-	if (!strcmp(argv[1], "hello")) {
-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
-
-		saddr = env_get("loadaddr");
-		if (saddr)
-			addr = simple_strtoul(saddr, NULL, 16);
-		else
-			addr = CONFIG_SYS_LOAD_ADDR;
-		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
-	} else
-#endif
-	{
-		saddr = argv[1];
-
-		addr = simple_strtoul(saddr, NULL, 16);
-		/* Check that a numeric value was passed */
-		if (!addr && *saddr != '0')
-			return CMD_RET_USAGE;
-
-	}
-
-	printf("## Starting EFI application at %08lx ...\n", addr);
-	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
-			    bootefi_image_path);
-	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
-
-	if (r != EFI_SUCCESS)
-		return 1;
-	else
-		return 0;
+	return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
 }
 
 #ifdef CONFIG_SYS_LONGHELP
-- 
2.20.1

[U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[Heinrich Schuchardt]

On 4/16/19 6:24 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 | 126 ++++++++++++++++++++++++++++----------------------
>   1 file changed, 70 insertions(+), 56 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 7cc6f0ee5ac8..f14966961b23 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -355,6 +355,75 @@ static int do_efibootmgr(const char *fdt_opt)
>   	return 0;
>   }
>
> +/*
> + * do_boot_efi() - execute EFI binary from command line

Having a function do_boot_efi() and a function do_bootefi() is a bit
confusing. Please, use something different, like efi_do_boot().

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

> + *
> + * @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_INVALID_PARAMETER)
> +		return CMD_RET_USAGE;
> +	else 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,
> @@ -483,11 +552,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;
>
> @@ -498,57 +562,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_INVALID_PARAMETER)
> -		return CMD_RET_USAGE;
> -	else if (r != EFI_SUCCESS)
> -		return CMD_RET_FAILURE;
> -
> -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> -	if (!strcmp(argv[1], "hello")) {
> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> -
> -		saddr = env_get("loadaddr");
> -		if (saddr)
> -			addr = simple_strtoul(saddr, NULL, 16);
> -		else
> -			addr = CONFIG_SYS_LOAD_ADDR;
> -		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> -	} else
> -#endif
> -	{
> -		saddr = argv[1];
> -
> -		addr = simple_strtoul(saddr, NULL, 16);
> -		/* Check that a numeric value was passed */
> -		if (!addr && *saddr != '0')
> -			return CMD_RET_USAGE;
> -
> -	}
> -
> -	printf("## Starting EFI application at %08lx ...\n", addr);
> -	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> -			    bootefi_image_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       r & ~EFI_ERROR_MASK);
> -
> -	if (r != EFI_SUCCESS)
> -		return 1;
> -	else
> -		return 0;
> +	return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
>   }
>
>   #ifdef CONFIG_SYS_LONGHELP
>

[U-Boot] [RFC v3 08/10] cmd: bootefi: carve out do_boot_efi() from do_bootefi()

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 07:31:28AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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 | 126 ++++++++++++++++++++++++++++----------------------
> >  1 file changed, 70 insertions(+), 56 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index 7cc6f0ee5ac8..f14966961b23 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -355,6 +355,75 @@ static int do_efibootmgr(const char *fdt_opt)
> >  	return 0;
> >  }
> >
> >+/*
> >+ * do_boot_efi() - execute EFI binary from command line
> 
> Having a function do_boot_efi() and a function do_bootefi() is a bit
> confusing. Please, use something different, like efi_do_boot().

Okay, but I don't like that any function in this file starts with
"efi_".
So do_bootefi_image(). This will reflect more precise functionality.

-Takahiro Akashi

> Otherwise
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> >+ *
> >+ * @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_INVALID_PARAMETER)
> >+		return CMD_RET_USAGE;
> >+	else 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,
> >@@ -483,11 +552,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;
> >
> >@@ -498,57 +562,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_INVALID_PARAMETER)
> >-		return CMD_RET_USAGE;
> >-	else if (r != EFI_SUCCESS)
> >-		return CMD_RET_FAILURE;
> >-
> >-#ifdef CONFIG_CMD_BOOTEFI_HELLO
> >-	if (!strcmp(argv[1], "hello")) {
> >-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >-
> >-		saddr = env_get("loadaddr");
> >-		if (saddr)
> >-			addr = simple_strtoul(saddr, NULL, 16);
> >-		else
> >-			addr = CONFIG_SYS_LOAD_ADDR;
> >-		memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size);
> >-	} else
> >-#endif
> >-	{
> >-		saddr = argv[1];
> >-
> >-		addr = simple_strtoul(saddr, NULL, 16);
> >-		/* Check that a numeric value was passed */
> >-		if (!addr && *saddr != '0')
> >-			return CMD_RET_USAGE;
> >-
> >-	}
> >-
> >-	printf("## Starting EFI application at %08lx ...\n", addr);
> >-	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >-			    bootefi_image_path);
> >-	printf("## Application terminated, r = %lu\n",
> >-	       r & ~EFI_ERROR_MASK);
> >-
> >-	if (r != EFI_SUCCESS)
> >-		return 1;
> >-	else
> >-		return 0;
> >+	return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL);
> >  }
> >
> >  #ifdef CONFIG_SYS_LONGHELP
> >
> 

[U-Boot] [RFC v3 09/10] 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                 | 197 +++++++++++++++++++---------------
 include/efi_loader.h          |   5 +-
 lib/efi_loader/efi_bootmgr.c  |  43 ++++----
 lib/efi_loader/efi_boottime.c |   2 +
 4 files changed, 134 insertions(+), 113 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index f14966961b23..97d49a53a44b 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -222,88 +222,39 @@ 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! */
-	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);
 
+err:
 	return ret;
 }
 
@@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
 	else if (ret != EFI_SUCCESS)
 		return CMD_RET_FAILURE;
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
-	if (!addr)
-		return 1;
+	ret = efi_bootmgr_load(&handle);
+	if (ret != EFI_SUCCESS) {
+		printf("EFI boot manager: Cannot load any image\n");
+		return CMD_RET_FAILURE;
+	}
 
-	printf("## Starting EFI application at %p ...\n", addr);
-	ret = do_bootefi_exec(addr, device_path, file_path);
-	printf("## Application terminated, r = %lu\n",
-	       ret & ~EFI_ERROR_MASK);
+	ret = do_bootefi_exec(handle);
+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
 
 	if (ret != EFI_SUCCESS)
-		return 1;
+		return CMD_RET_FAILURE;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 /*
@@ -367,10 +317,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();
@@ -392,36 +346,94 @@ 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;
 	}
 
-	printf("## Starting EFI application at %08lx ...\n", addr);
-	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
-			      bootefi_image_path);
-	printf("## Application terminated, r = %lu\n",
-	       ret & ~EFI_ERROR_MASK);
+	if (!device_path && !image_path) {
+		/*
+		 * Special case for efi payload not loaded from disk,
+		 * such as 'bootefi hello' or for example payload
+		 * loaded directly into memory via JTAG, etc:
+		 */
+		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, efi_root,
+				      file_path, image_buf, size, &handle));
+	if (ret != EFI_SUCCESS)
+		goto err_prepare;
+
+	ret = do_bootefi_exec(handle);
+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
+
+err_prepare:
+	if (device_path)
+		efi_free_pool(file_path);
+
+err_add_protocol:
+	if (mem_handle)
+		efi_delete_handle(mem_handle);
+	if (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
@@ -581,7 +593,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"
@@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -408,8 +408,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);
 
@@ -563,8 +561,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 b215bd7723da..65425fabc08f 100644
--- a/lib/efi_loader/efi_boottime.c
+++ b/lib/efi_loader/efi_boottime.c
@@ -1611,6 +1611,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)
 {
@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
 	}
 
 	current_image = image_handle;
+	debug("EFI: Jumping into 0x%p\n", image_obj->entry);
 	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
 
 	/*
-- 
2.20.1

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

[Heinrich Schuchardt]

On 4/16/19 6:24 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                 | 197 +++++++++++++++++++---------------
>   include/efi_loader.h          |   5 +-
>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>   lib/efi_loader/efi_boottime.c |   2 +
>   4 files changed, 134 insertions(+), 113 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index f14966961b23..97d49a53a44b 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -222,88 +222,39 @@ 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");

In set_load_options() an error could occur (out of memory). So I think
it should return a status.

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

Shouldn't we move this to set_load_options()?

>   	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");

Shouldn't this move to efi_setup.c? Probably we should also set
OsIndications. I would prefer using efi_set_variable(). I think moving
this could be done in a preparatory patch.

>
>   	/* Call our payload! */
> -	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> -	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> +	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>
> -err_prepare:
> -	/* image has returned, loaded-image obj goes *poof*: */
>   	efi_restore_gd();
>   	free(loaded_image_info->load_options);
> -	efi_delete_handle(&image_obj->header);
> -
> -err_add_protocol:
> -	if (mem_handle)
> -		efi_delete_handle(mem_handle);
>
> +err:
>   	return ret;
>   }
>
> @@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>   	else if (ret != EFI_SUCCESS)
>   		return CMD_RET_FAILURE;
>
> -	addr = efi_bootmgr_load(&device_path, &file_path);
> -	if (!addr)
> -		return 1;
> +	ret = efi_bootmgr_load(&handle);
> +	if (ret != EFI_SUCCESS) {
> +		printf("EFI boot manager: Cannot load any image\n");
> +		return CMD_RET_FAILURE;
> +	}
>
> -	printf("## Starting EFI application at %p ...\n", addr);
> -	ret = do_bootefi_exec(addr, device_path, file_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
>
>   	if (ret != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;
>
> -	return 0;
> +	return CMD_RET_SUCCESS;
>   }
>
>   /*
> @@ -367,10 +317,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();
> @@ -392,36 +346,94 @@ 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;
>   	}
>
> -	printf("## Starting EFI application at %08lx ...\n", addr);
> -	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> -			      bootefi_image_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       ret & ~EFI_ERROR_MASK);
> +	if (!device_path && !image_path) {
> +		/*
> +		 * Special case for efi payload not loaded from disk,
> +		 * such as 'bootefi hello' or for example payload
> +		 * loaded directly into memory via JTAG, etc:
> +		 */
> +		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");

The EFI spec says that either of SourceBuffer or DevicePath may be NULL
when calling LoadImage().

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

GRUB will fail anyway because it cannot determine the disk from which it
was loaded. So why are we doing this?

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

Couldn't we as well use the device path of the root node as "file_path"?

> +		if (ret != EFI_SUCCESS)
> +			goto err_add_protocol;
> +	} else {
> +		assert(device_path && image_path);
> +		file_path = efi_dp_append(device_path, image_path);

Where is file_path freed?

> +	}
> +
> +	ret = EFI_CALL(efi_load_image(false, efi_root,
> +				      file_path, image_buf, size, &handle));
> +	if (ret != EFI_SUCCESS)
> +		goto err_prepare;
> +
> +	ret = do_bootefi_exec(handle);
> +	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> +
> +err_prepare:
> +	if (device_path)
> +		efi_free_pool(file_path);
> +
> +err_add_protocol:
> +	if (mem_handle)
> +		efi_delete_handle(mem_handle);
> +	if (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
> @@ -581,7 +593,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"
> @@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -408,8 +408,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);
>
> @@ -563,8 +561,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 b215bd7723da..65425fabc08f 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1611,6 +1611,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)
>   {
> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
>   	}
>
>   	current_image = image_handle;
> +	debug("EFI: Jumping into 0x%p\n", image_obj->entry);

Please, use EFI_PRINT() here.

Best regards

Heinrich

>   	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>
>   	/*
>

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

[Heinrich Schuchardt]

On 4/16/19 12:56 PM, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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                 | 197 +++++++++++++++++++---------------
>>   include/efi_loader.h          |   5 +-
>>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>>   lib/efi_loader/efi_boottime.c |   2 +
>>   4 files changed, 134 insertions(+), 113 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index f14966961b23..97d49a53a44b 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -222,88 +222,39 @@ 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");
> 
> In set_load_options() an error could occur (out of memory). So I think
> it should return a status.
> 
>> -
>> -    /* 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));
> 
> Shouldn't we move this to set_load_options()?
> 
>>       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");
> 
> Shouldn't this move to efi_setup.c? Probably we should also set
> OsIndications. I would prefer using efi_set_variable(). I think moving
> this could be done in a preparatory patch.
> 
>>
>>       /* Call our payload! */
>> -    debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>> -    ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>> +    ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>>
>> -err_prepare:
>> -    /* image has returned, loaded-image obj goes *poof*: */
>>       efi_restore_gd();
>>       free(loaded_image_info->load_options);
>> -    efi_delete_handle(&image_obj->header);
>> -
>> -err_add_protocol:
>> -    if (mem_handle)
>> -        efi_delete_handle(mem_handle);
>>
>> +err:
>>       return ret;
>>   }
>>
>> @@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>>       else if (ret != EFI_SUCCESS)
>>           return CMD_RET_FAILURE;
>>
>> -    addr = efi_bootmgr_load(&device_path, &file_path);
>> -    if (!addr)
>> -        return 1;
>> +    ret = efi_bootmgr_load(&handle);
>> +    if (ret != EFI_SUCCESS) {
>> +        printf("EFI boot manager: Cannot load any image\n");
>> +        return CMD_RET_FAILURE;
>> +    }
>>
>> -    printf("## Starting EFI application at %p ...\n", addr);
>> -    ret = do_bootefi_exec(addr, device_path, file_path);
>> -    printf("## Application terminated, r = %lu\n",
>> -           ret & ~EFI_ERROR_MASK);
>> +    ret = do_bootefi_exec(handle);
>> +    printf("## Application terminated, r = %lu\n", ret & 
>> ~EFI_ERROR_MASK);
>>
>>       if (ret != EFI_SUCCESS)
>> -        return 1;
>> +        return CMD_RET_FAILURE;
>>
>> -    return 0;
>> +    return CMD_RET_SUCCESS;
>>   }
>>
>>   /*
>> @@ -367,10 +317,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();
>> @@ -392,36 +346,94 @@ 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;
>>       }
>>
>> -    printf("## Starting EFI application at %08lx ...\n", addr);
>> -    ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>> -                  bootefi_image_path);
>> -    printf("## Application terminated, r = %lu\n",
>> -           ret & ~EFI_ERROR_MASK);
>> +    if (!device_path && !image_path) {
>> +        /*
>> +         * Special case for efi payload not loaded from disk,
>> +         * such as 'bootefi hello' or for example payload
>> +         * loaded directly into memory via JTAG, etc:
>> +         */
>> +        printf("WARNING: using memory device/image path, this may 
>> confuse some payloads!\n");
> 
> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> when calling LoadImage().
> 
>> +
>> +        /* 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.
> 
> GRUB will fail anyway because it cannot determine the disk from which it
> was loaded. So why are we doing this?

In Rob's original commit  bf19273e81eb ("efi_loader: Add mem-mapped for 
fallback") the comment was:
"This fixes 'bootefi hello' after devicepath refactoring."

EFI Shell.efi starts fine even when we set device_path and image_path to 
NULL.

When loading GRUB from disk or via tftp the device path and the image 
path are set, e.g. for tftp:

=> dhcp grubarm.efi
=> bootefi 0x40200000 $fdtcontroladdr
Found 0 disks
## Starting EFI application at 40200000 ...
device_path:
/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1)
image_path: /grubarm.efi
error: disk `,msdos2' not found.
Entering rescue mode...
grub rescue>

Maybe GRUB would run if all necessary modules were compiled in to do a 
network boot. But how would it find grub.conf?

So my feeling is that we are creating the dummy memory device path because:

- Once `bootefi hello` which is our own test program had a problem.
- GRUB has a bug: it hangs or crashes if the file device path is not
   set in LoadImage().

I think the problem should not be fixed in U-Boot but in GRUB.

I am CC-ing Leif due to all the attention he has paid both to U-Boot and 
to GRUB.

Best regards

Heinrich

> 
>> +         */
>> +        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);
> 
> Couldn't we as well use the device path of the root node as "file_path"?
> 
>> +        if (ret != EFI_SUCCESS)
>> +            goto err_add_protocol;
>> +    } else {
>> +        assert(device_path && image_path);
>> +        file_path = efi_dp_append(device_path, image_path);
> 
> Where is file_path freed?
> 
>> +    }
>> +
>> +    ret = EFI_CALL(efi_load_image(false, efi_root,
>> +                      file_path, image_buf, size, &handle));
>> +    if (ret != EFI_SUCCESS)
>> +        goto err_prepare;
>> +
>> +    ret = do_bootefi_exec(handle);
>> +    printf("## Application terminated, r = %lu\n", ret & 
>> ~EFI_ERROR_MASK);
>> +
>> +err_prepare:
>> +    if (device_path)
>> +        efi_free_pool(file_path);
>> +
>> +err_add_protocol:
>> +    if (mem_handle)
>> +        efi_delete_handle(mem_handle);
>> +    if (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
>> @@ -581,7 +593,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"
>> @@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
>> --- a/include/efi_loader.h
>> +++ b/include/efi_loader.h
>> @@ -408,8 +408,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);
>>
>> @@ -563,8 +561,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 b215bd7723da..65425fabc08f 100644
>> --- a/lib/efi_loader/efi_boottime.c
>> +++ b/lib/efi_loader/efi_boottime.c
>> @@ -1611,6 +1611,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)
>>   {
>> @@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t 
>> image_handle,
>>       }
>>
>>       current_image = image_handle;
>> +    debug("EFI: Jumping into 0x%p\n", image_obj->entry);
> 
> Please, use EFI_PRINT() here.
> 
> Best regards
> 
> Heinrich
> 
>>       ret = EFI_CALL(image_obj->entry(image_handle, &systab));
>>
>>       /*
>>
> 
> 

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

[Alexander Graf]

On 16.04.19 18:20, Heinrich Schuchardt wrote:
> On 4/16/19 12:56 PM, Heinrich Schuchardt wrote:
>> On 4/16/19 6:24 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                 | 197
>>> +++++++++++++++++++---------------
>>>   include/efi_loader.h          |   5 +-
>>>   lib/efi_loader/efi_bootmgr.c  |  43 ++++----
>>>   lib/efi_loader/efi_boottime.c |   2 +
>>>   4 files changed, 134 insertions(+), 113 deletions(-)
>>>
>>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>>> index f14966961b23..97d49a53a44b 100644
>>> --- a/cmd/bootefi.c
>>> +++ b/cmd/bootefi.c
>>> @@ -222,88 +222,39 @@ 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");
>>
>> In set_load_options() an error could occur (out of memory). So I think
>> it should return a status.
>>
>>> -
>>> -    /* 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));
>>
>> Shouldn't we move this to set_load_options()?
>>
>>>       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");
>>
>> Shouldn't this move to efi_setup.c? Probably we should also set
>> OsIndications. I would prefer using efi_set_variable(). I think moving
>> this could be done in a preparatory patch.
>>
>>>
>>>       /* Call our payload! */
>>> -    debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
>>> -    ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
>>> +    ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
>>>
>>> -err_prepare:
>>> -    /* image has returned, loaded-image obj goes *poof*: */
>>>       efi_restore_gd();
>>>       free(loaded_image_info->load_options);
>>> -    efi_delete_handle(&image_obj->header);
>>> -
>>> -err_add_protocol:
>>> -    if (mem_handle)
>>> -        efi_delete_handle(mem_handle);
>>>
>>> +err:
>>>       return ret;
>>>   }
>>>
>>> @@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
>>>       else if (ret != EFI_SUCCESS)
>>>           return CMD_RET_FAILURE;
>>>
>>> -    addr = efi_bootmgr_load(&device_path, &file_path);
>>> -    if (!addr)
>>> -        return 1;
>>> +    ret = efi_bootmgr_load(&handle);
>>> +    if (ret != EFI_SUCCESS) {
>>> +        printf("EFI boot manager: Cannot load any image\n");
>>> +        return CMD_RET_FAILURE;
>>> +    }
>>>
>>> -    printf("## Starting EFI application at %p ...\n", addr);
>>> -    ret = do_bootefi_exec(addr, device_path, file_path);
>>> -    printf("## Application terminated, r = %lu\n",
>>> -           ret & ~EFI_ERROR_MASK);
>>> +    ret = do_bootefi_exec(handle);
>>> +    printf("## Application terminated, r = %lu\n", ret &
>>> ~EFI_ERROR_MASK);
>>>
>>>       if (ret != EFI_SUCCESS)
>>> -        return 1;
>>> +        return CMD_RET_FAILURE;
>>>
>>> -    return 0;
>>> +    return CMD_RET_SUCCESS;
>>>   }
>>>
>>>   /*
>>> @@ -367,10 +317,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();
>>> @@ -392,36 +346,94 @@ 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;
>>>       }
>>>
>>> -    printf("## Starting EFI application at %08lx ...\n", addr);
>>> -    ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
>>> -                  bootefi_image_path);
>>> -    printf("## Application terminated, r = %lu\n",
>>> -           ret & ~EFI_ERROR_MASK);
>>> +    if (!device_path && !image_path) {
>>> +        /*
>>> +         * Special case for efi payload not loaded from disk,
>>> +         * such as 'bootefi hello' or for example payload
>>> +         * loaded directly into memory via JTAG, etc:
>>> +         */
>>> +        printf("WARNING: using memory device/image path, this may
>>> confuse some payloads!\n");
>>
>> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
>> when calling LoadImage().
>>
>>> +
>>> +        /* 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.
>>
>> GRUB will fail anyway because it cannot determine the disk from which it
>> was loaded. So why are we doing this?
>
> In Rob's original commit  bf19273e81eb ("efi_loader: Add mem-mapped
> for fallback") the comment was:
> "This fixes 'bootefi hello' after devicepath refactoring."
>
> EFI Shell.efi starts fine even when we set device_path and image_path
> to NULL.
>
> When loading GRUB from disk or via tftp the device path and the image
> path are set, e.g. for tftp:
>
> => dhcp grubarm.efi
> => bootefi 0x40200000 $fdtcontroladdr
> Found 0 disks
> ## Starting EFI application at 40200000 ...
> device_path:
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/MAC(525400123456,0x1)
> image_path: /grubarm.efi
> error: disk `,msdos2' not found.
> Entering rescue mode...
> grub rescue>
>
> Maybe GRUB would run if all necessary modules were compiled in to do a
> network boot. But how would it find grub.conf?
>
> So my feeling is that we are creating the dummy memory device path
> because:
>
> - Once `bootefi hello` which is our own test program had a problem.
> - GRUB has a bug: it hangs or crashes if the file device path is not
>   set in LoadImage().
>
> I think the problem should not be fixed in U-Boot but in GRUB.
>
> I am CC-ing Leif due to all the attention he has paid both to U-Boot
> and to GRUB.


Please also keep in mind that you can not fix already released versions
of GRUB.



Alex

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

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 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                 | 197 +++++++++++++++++++---------------
> >  include/efi_loader.h          |   5 +-
> >  lib/efi_loader/efi_bootmgr.c  |  43 ++++----
> >  lib/efi_loader/efi_boottime.c |   2 +
> >  4 files changed, 134 insertions(+), 113 deletions(-)
> >
> >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> >index f14966961b23..97d49a53a44b 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -222,88 +222,39 @@ 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");
> 
> In set_load_options() an error could occur (out of memory). So I think
> it should return a status.

I didn't change anything in set_load_options(), but I will follow
your comment.

> >-
> >-	/* 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));
> 
> Shouldn't we move this to set_load_options()?

No. This line has nothing to do with "load options."

> >  	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");
> 
> Shouldn't this move to efi_setup.c? Probably we should also set
> OsIndications. I would prefer using efi_set_variable(). I think moving
> this could be done in a preparatory patch.

Yeah, I am aware of this issue.
Will fix in a separate patch.

> >
> >  	/* Call our payload! */
> >-	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> >-	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> >+	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> >
> >-err_prepare:
> >-	/* image has returned, loaded-image obj goes *poof*: */
> >  	efi_restore_gd();
> >  	free(loaded_image_info->load_options);
> >-	efi_delete_handle(&image_obj->header);
> >-
> >-err_add_protocol:
> >-	if (mem_handle)
> >-		efi_delete_handle(mem_handle);
> >
> >+err:
> >  	return ret;
> >  }
> >
> >@@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
> >  	else if (ret != EFI_SUCCESS)
> >  		return CMD_RET_FAILURE;
> >
> >-	addr = efi_bootmgr_load(&device_path, &file_path);
> >-	if (!addr)
> >-		return 1;
> >+	ret = efi_bootmgr_load(&handle);
> >+	if (ret != EFI_SUCCESS) {
> >+		printf("EFI boot manager: Cannot load any image\n");
> >+		return CMD_RET_FAILURE;
> >+	}
> >
> >-	printf("## Starting EFI application at %p ...\n", addr);
> >-	ret = do_bootefi_exec(addr, device_path, file_path);
> >-	printf("## Application terminated, r = %lu\n",
> >-	       ret & ~EFI_ERROR_MASK);
> >+	ret = do_bootefi_exec(handle);
> >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >
> >  	if (ret != EFI_SUCCESS)
> >-		return 1;
> >+		return CMD_RET_FAILURE;
> >
> >-	return 0;
> >+	return CMD_RET_SUCCESS;
> >  }
> >
> >  /*
> >@@ -367,10 +317,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();
> >@@ -392,36 +346,94 @@ 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;
> >  	}
> >
> >-	printf("## Starting EFI application at %08lx ...\n", addr);
> >-	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> >-			      bootefi_image_path);
> >-	printf("## Application terminated, r = %lu\n",
> >-	       ret & ~EFI_ERROR_MASK);
> >+	if (!device_path && !image_path) {
> >+		/*
> >+		 * Special case for efi payload not loaded from disk,
> >+		 * such as 'bootefi hello' or for example payload
> >+		 * loaded directly into memory via JTAG, etc:
> >+		 */
> >+		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> 
> The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> when calling LoadImage().

So are you suggesting we should remove this message?

> >+
> >+		/* 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.
> 
> GRUB will fail anyway because it cannot determine the disk from which it
> was loaded. So why are we doing this?

I will comment on another reply.

> >+		 */
> >+		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);
> 
> Couldn't we as well use the device path of the root node as "file_path"?

I don't get you point very well, but
it will depend on how we should fix a grub issue mentioned above.

> >+		if (ret != EFI_SUCCESS)
> >+			goto err_add_protocol;
> >+	} else {
> >+		assert(device_path && image_path);
> >+		file_path = efi_dp_append(device_path, image_path);
> 
> Where is file_path freed?

In this function. Will fix.

> >+	}
> >+
> >+	ret = EFI_CALL(efi_load_image(false, efi_root,
> >+				      file_path, image_buf, size, &handle));
> >+	if (ret != EFI_SUCCESS)
> >+		goto err_prepare;
> >+
> >+	ret = do_bootefi_exec(handle);
> >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> >+
> >+err_prepare:
> >+	if (device_path)
> >+		efi_free_pool(file_path);
> >+
> >+err_add_protocol:
> >+	if (mem_handle)
> >+		efi_delete_handle(mem_handle);
> >+	if (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
> >@@ -581,7 +593,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"
> >@@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
> >--- a/include/efi_loader.h
> >+++ b/include/efi_loader.h
> >@@ -408,8 +408,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);
> >
> >@@ -563,8 +561,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 b215bd7723da..65425fabc08f 100644
> >--- a/lib/efi_loader/efi_boottime.c
> >+++ b/lib/efi_loader/efi_boottime.c
> >@@ -1611,6 +1611,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)
> >  {
> >@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> >  	}
> >
> >  	current_image = image_handle;
> >+	debug("EFI: Jumping into 0x%p\n", image_obj->entry);
> 
> Please, use EFI_PRINT() here.

Basically I agree, but there are lots of debug()'s in this file.
We should replace them all at once later.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> >  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> >
> >  	/*
> >
> 

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

[AKASHI Takahiro]

Correction:

On Thu, Apr 18, 2019 at 09:13:52AM +0900, AKASHI Takahiro wrote:
> On Tue, Apr 16, 2019 at 12:56:32PM +0200, Heinrich Schuchardt wrote:
> > On 4/16/19 6:24 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                 | 197 +++++++++++++++++++---------------
> > >  include/efi_loader.h          |   5 +-
> > >  lib/efi_loader/efi_bootmgr.c  |  43 ++++----
> > >  lib/efi_loader/efi_boottime.c |   2 +
> > >  4 files changed, 134 insertions(+), 113 deletions(-)
> > >
> > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > >index f14966961b23..97d49a53a44b 100644
> > >--- a/cmd/bootefi.c
> > >+++ b/cmd/bootefi.c
> > >@@ -222,88 +222,39 @@ 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");
> > 
> > In set_load_options() an error could occur (out of memory). So I think
> > it should return a status.
> 
> I didn't change anything in set_load_options(), but I will follow
> your comment.
> 
> > >-
> > >-	/* 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));
> > 
> > Shouldn't we move this to set_load_options()?
> 
> No. This line has nothing to do with "load options."

Wrong, I will follow your comment.
This change, however, uncovers one issue: Who is responsible
for freeing loaded_image_info->load_option, particularly,
when efi_exit()/unload_image() is fully implemented?
In such a case, we have no chance to access this handle,
hence loaded_image_info, after returning from efi_start_image().

Thanks,
-Takahiro Akashi

> > >  	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");
> > 
> > Shouldn't this move to efi_setup.c? Probably we should also set
> > OsIndications. I would prefer using efi_set_variable(). I think moving
> > this could be done in a preparatory patch.
> 
> Yeah, I am aware of this issue.
> Will fix in a separate patch.
> 
> > >
> > >  	/* Call our payload! */
> > >-	debug("%s: Jumping to 0x%p\n", __func__, image_obj->entry);
> > >-	ret = EFI_CALL(efi_start_image(&image_obj->header, NULL, NULL));
> > >+	ret = EFI_CALL(efi_start_image(handle, NULL, NULL));
> > >
> > >-err_prepare:
> > >-	/* image has returned, loaded-image obj goes *poof*: */
> > >  	efi_restore_gd();
> > >  	free(loaded_image_info->load_options);
> > >-	efi_delete_handle(&image_obj->header);
> > >-
> > >-err_add_protocol:
> > >-	if (mem_handle)
> > >-		efi_delete_handle(mem_handle);
> > >
> > >+err:
> > >  	return ret;
> > >  }
> > >
> > >@@ -317,8 +268,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 +290,19 @@ static int do_efibootmgr(const char *fdt_opt)
> > >  	else if (ret != EFI_SUCCESS)
> > >  		return CMD_RET_FAILURE;
> > >
> > >-	addr = efi_bootmgr_load(&device_path, &file_path);
> > >-	if (!addr)
> > >-		return 1;
> > >+	ret = efi_bootmgr_load(&handle);
> > >+	if (ret != EFI_SUCCESS) {
> > >+		printf("EFI boot manager: Cannot load any image\n");
> > >+		return CMD_RET_FAILURE;
> > >+	}
> > >
> > >-	printf("## Starting EFI application at %p ...\n", addr);
> > >-	ret = do_bootefi_exec(addr, device_path, file_path);
> > >-	printf("## Application terminated, r = %lu\n",
> > >-	       ret & ~EFI_ERROR_MASK);
> > >+	ret = do_bootefi_exec(handle);
> > >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> > >
> > >  	if (ret != EFI_SUCCESS)
> > >-		return 1;
> > >+		return CMD_RET_FAILURE;
> > >
> > >-	return 0;
> > >+	return CMD_RET_SUCCESS;
> > >  }
> > >
> > >  /*
> > >@@ -367,10 +317,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();
> > >@@ -392,36 +346,94 @@ 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;
> > >  	}
> > >
> > >-	printf("## Starting EFI application at %08lx ...\n", addr);
> > >-	ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> > >-			      bootefi_image_path);
> > >-	printf("## Application terminated, r = %lu\n",
> > >-	       ret & ~EFI_ERROR_MASK);
> > >+	if (!device_path && !image_path) {
> > >+		/*
> > >+		 * Special case for efi payload not loaded from disk,
> > >+		 * such as 'bootefi hello' or for example payload
> > >+		 * loaded directly into memory via JTAG, etc:
> > >+		 */
> > >+		printf("WARNING: using memory device/image path, this may confuse some payloads!\n");
> > 
> > The EFI spec says that either of SourceBuffer or DevicePath may be NULL
> > when calling LoadImage().
> 
> So are you suggesting we should remove this message?
> 
> > >+
> > >+		/* 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.
> > 
> > GRUB will fail anyway because it cannot determine the disk from which it
> > was loaded. So why are we doing this?
> 
> I will comment on another reply.
> 
> > >+		 */
> > >+		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);
> > 
> > Couldn't we as well use the device path of the root node as "file_path"?
> 
> I don't get you point very well, but
> it will depend on how we should fix a grub issue mentioned above.
> 
> > >+		if (ret != EFI_SUCCESS)
> > >+			goto err_add_protocol;
> > >+	} else {
> > >+		assert(device_path && image_path);
> > >+		file_path = efi_dp_append(device_path, image_path);
> > 
> > Where is file_path freed?
> 
> In this function. Will fix.
> 
> > >+	}
> > >+
> > >+	ret = EFI_CALL(efi_load_image(false, efi_root,
> > >+				      file_path, image_buf, size, &handle));
> > >+	if (ret != EFI_SUCCESS)
> > >+		goto err_prepare;
> > >+
> > >+	ret = do_bootefi_exec(handle);
> > >+	printf("## Application terminated, r = %lu\n", ret & ~EFI_ERROR_MASK);
> > >+
> > >+err_prepare:
> > >+	if (device_path)
> > >+		efi_free_pool(file_path);
> > >+
> > >+err_add_protocol:
> > >+	if (mem_handle)
> > >+		efi_delete_handle(mem_handle);
> > >+	if (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
> > >@@ -581,7 +593,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"
> > >@@ -606,6 +618,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 d524dc7e24c1..c3eda2bb05d7 100644
> > >--- a/include/efi_loader.h
> > >+++ b/include/efi_loader.h
> > >@@ -408,8 +408,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);
> > >
> > >@@ -563,8 +561,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 b215bd7723da..65425fabc08f 100644
> > >--- a/lib/efi_loader/efi_boottime.c
> > >+++ b/lib/efi_loader/efi_boottime.c
> > >@@ -1611,6 +1611,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)
> > >  {
> > >@@ -2684,6 +2685,7 @@ efi_status_t EFIAPI efi_start_image(efi_handle_t image_handle,
> > >  	}
> > >
> > >  	current_image = image_handle;
> > >+	debug("EFI: Jumping into 0x%p\n", image_obj->entry);
> > 
> > Please, use EFI_PRINT() here.
> 
> Basically I agree, but there are lots of debug()'s in this file.
> We should replace them all at once later.
> 
> Thanks,
> -Takahiro Akashi
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >  	ret = EFI_CALL(image_obj->entry(image_handle, &systab));
> > >
> > >  	/*
> > >
> > 

[U-Boot] [RFC v3 10/10] 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 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -631,3 +631,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 v3 10/10] cmd: add efibootmgr command

[Heinrich Schuchardt]

On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
> With this patch, EFI boot manager can be kicked in by a standalone
> command, efibootmgr.
>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>   cmd/Kconfig   |  8 ++++++++
>   cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
>   2 files changed, 43 insertions(+)
>
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -631,3 +631,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;

Why would you not allow to pass a device tree address?

I think we do not need to different ways to invoke the boot manager.

We should either have either `bootefi bootmgr` or `efibootmgr`.

Best regards

Heinrich

> +
> +	fdt_opt = env_get("fdtaddr");
> +	if (!fdt_opt)
> +		fdt_opt = env_get("fdtcontroladdr");
> +
> +	ret = do_efibootmgr(fdt_opt);
> +
> +	free(fdt_opt);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_SYS_LONGHELP
> +static char efibootmgr_help_text[] =
> +	"(no arguments)\n"
> +	" - Launch EFI boot manager and execute EFI application\n"
> +	"   based on BootNext and BootOrder variables\n";
> +#endif
> +
> +U_BOOT_CMD(
> +	efibootmgr, 1, 0, do_efibootmgr_cmd,
> +	"Launch EFI boot manager",
> +	efibootmgr_help_text
> +);
> +#endif /* CONFIG_CMD_STANDALONE_EFIBOOTMGR */
>

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

[AKASHI Takahiro]

On Tue, Apr 16, 2019 at 07:27:10AM +0200, Heinrich Schuchardt wrote:
> On 4/16/19 6:24 AM, AKASHI Takahiro wrote:
> >Add CONFIG_CMD_STANDALONE_EFIBOOTMGR.
> >With this patch, EFI boot manager can be kicked in by a standalone
> >command, efibootmgr.
> >
> >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >---
> >  cmd/Kconfig   |  8 ++++++++
> >  cmd/bootefi.c | 35 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> >
> >diff --git a/cmd/Kconfig b/cmd/Kconfig
> >index 0b07b3b9d777..6c9a9f821e54 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 97d49a53a44b..1afa86256670 100644
> >--- a/cmd/bootefi.c
> >+++ b/cmd/bootefi.c
> >@@ -631,3 +631,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;
> 
> Why would you not allow to pass a device tree address?

I think I explained in the past:
The current *secure boot* feature in U-Boot assumes that a single
command without any arguments be invoked. See cli_secure_boot_cmd()
in common/cli.c.

> I think we do not need to different ways to invoke the boot manager.
> 
> We should either have either `bootefi bootmgr` or `efibootmgr`.

This patch is not essential in this patch series, so it's up to you
if it's merged or not.
I will re-visit this issue when I submit *secure boot* patch.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >+
> >+	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 */
> >
>