[U-Boot] [PATCH 0/2] efi_loader: rework loading (continued)

[U-Boot] [PATCH 0/2] efi_loader: rework loading (continued)

[AKASHI Takahiro]

Heinrich,

I like your patch[1], but the resulting code looks a bit sloppy.
Can you add those patches to yours?

Then, I will rework my own "run -e" patch[2] on top of them.

[1] https://lists.denx.de/pipermail/u-boot/2019-February/358042.html
[2] https://lists.denx.de/pipermail/u-boot/2019-January/354611.html

AKASHI Takahiro (2):
  cmd: bootefi: move bootefi_test_prepare() forward
  cmd: bootefi: rework do_bootefi()

 cmd/bootefi.c | 186 +++++++++++++++++++++++++-------------------------
 1 file changed, 94 insertions(+), 92 deletions(-)

-- 
2.19.1

[U-Boot] [PATCH 1/2] cmd: bootefi: move bootefi_test_prepare() forward

[AKASHI Takahiro]

This is a preparatory patch for reworking do_bootefi().

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index 9d9ccdd31a08..e064edcd0cdb 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -231,6 +231,53 @@ static void bootefi_run_finish(struct efi_loaded_image_obj *image_obj,
 	efi_delete_handle(&image_obj->header);
 }
 
+#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
+/**
+ * bootefi_test_prepare() - prepare to run an EFI test
+ *
+ * Prepare to run a test as if it were provided by a loaded image.
+ *
+ * @image_objp:		pointer to be set to the loaded image handle
+ * @loaded_image_infop:	pointer to be set to the loaded image protocol
+ * @path:		dummy file path used to construct the device path
+ *			set in the loaded image protocol
+ * @load_options_path:	name of a U-Boot environment variable. Its value is
+ *			set as load options in the loaded image protocol.
+ * Return:		status code
+ */
+static efi_status_t bootefi_test_prepare
+		(struct efi_loaded_image_obj **image_objp,
+		 struct efi_loaded_image **loaded_image_infop, const char *path,
+		 const char *load_options_path)
+{
+	efi_status_t ret;
+
+	/* Construct a dummy device path */
+	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
+	if (!bootefi_device_path)
+		return EFI_OUT_OF_RESOURCES;
+
+	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
+	if (!bootefi_image_path) {
+		ret = EFI_OUT_OF_RESOURCES;
+		goto failure;
+	}
+
+	ret = bootefi_run_prepare(load_options_path, bootefi_device_path,
+				  bootefi_image_path, image_objp,
+				  loaded_image_infop);
+	if (ret == EFI_SUCCESS)
+		return ret;
+
+	efi_free_pool(bootefi_image_path);
+	bootefi_image_path = NULL;
+failure:
+	efi_free_pool(bootefi_device_path);
+	bootefi_device_path = NULL;
+	return ret;
+}
+#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
+
 /**
  * do_bootefi_exec() - execute EFI binary
  *
@@ -314,54 +361,6 @@ err_add_protocol:
 	return ret;
 }
 
-#ifdef CONFIG_CMD_BOOTEFI_SELFTEST
-/**
- * bootefi_test_prepare() - prepare to run an EFI test
- *
- * Prepare to run a test as if it were provided by a loaded image.
- *
- * @image_objp:		pointer to be set to the loaded image handle
- * @loaded_image_infop:	pointer to be set to the loaded image protocol
- * @path:		dummy file path used to construct the device path
- *			set in the loaded image protocol
- * @load_options_path:	name of a U-Boot environment variable. Its value is
- *			set as load options in the loaded image protocol.
- * Return:		status code
- */
-static efi_status_t bootefi_test_prepare
-		(struct efi_loaded_image_obj **image_objp,
-		 struct efi_loaded_image **loaded_image_infop, const char *path,
-		 const char *load_options_path)
-{
-	efi_status_t ret;
-
-	/* Construct a dummy device path */
-	bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, 0, 0);
-	if (!bootefi_device_path)
-		return EFI_OUT_OF_RESOURCES;
-
-	bootefi_image_path = efi_dp_from_file(NULL, 0, path);
-	if (!bootefi_image_path) {
-		ret = EFI_OUT_OF_RESOURCES;
-		goto failure;
-	}
-
-	ret = bootefi_run_prepare(load_options_path, bootefi_device_path,
-				  bootefi_image_path, image_objp,
-				  loaded_image_infop);
-	if (ret == EFI_SUCCESS)
-		return ret;
-
-	efi_free_pool(bootefi_image_path);
-	bootefi_image_path = NULL;
-failure:
-	efi_free_pool(bootefi_device_path);
-	bootefi_device_path = NULL;
-	return ret;
-}
-
-#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */
-
 static int do_bootefi_bootmgr_exec(void)
 {
 	struct efi_device_path *device_path, *file_path;
-- 
2.19.1

[U-Boot] [PATCH 2/2] cmd: bootefi: rework do_bootefi()

[AKASHI Takahiro]

In this patch, do_bootefi() will be reworked, without any functional
change, as it is a bit sloppy after Heinrich's "efi_loader: rework
loading and starting of images."

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

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index e064edcd0cdb..159dc1ab8a30 100644
--- a/cmd/bootefi.c
+++ b/cmd/bootefi.c
@@ -361,34 +361,68 @@ err_add_protocol:
 	return ret;
 }
 
-static int do_bootefi_bootmgr_exec(void)
+static int do_bootefi_load_and_exec(const char *arg)
 {
-	struct efi_device_path *device_path, *file_path;
-	void *addr;
+	void *image_buf;
+	struct efi_device_path *device_path, *image_path;
+	const char *saddr;
+	unsigned long addr, size;
 	efi_status_t r;
 
-	addr = efi_bootmgr_load(&device_path, &file_path);
-	if (!addr)
-		return 1;
+	if (!strcmp(arg, "bootmgr")) {
+		image_buf = efi_bootmgr_load(&device_path, &image_path);
+		if (!image_buf)
+			return CMD_RET_FAILURE;
+
+		addr = map_to_sysmem(image_buf);
+	} else
+#ifdef CONFIG_CMD_BOOTEFI_HELLO
+	if (!strcmp(arg, "hello")) {
+		saddr = env_get("loadaddr");
+		size = __efi_helloworld_end - __efi_helloworld_begin;
+
+		if (saddr)
+			addr = simple_strtoul(saddr, NULL, 16);
+		else
+			addr = CONFIG_SYS_LOAD_ADDR;
+
+		image_buf = map_sysmem(addr, size);
+		memcpy(image_buf, __efi_helloworld_begin, size);
+
+		device_path = NULL;
+		image_path = NULL;
+	} else
+#endif
+	{
+		saddr = arg;
+		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 %p ...\n", addr);
-	r = do_bootefi_exec(addr, device_path, file_path);
-	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
+	printf("## Starting EFI application at %08lx ...\n", addr);
+	r = do_bootefi_exec(image_buf, device_path, image_path);
+	printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
 
 	if (r != EFI_SUCCESS)
-		return 1;
+		return CMD_RET_FAILURE;
 
-	return 0;
+	return CMD_RET_SUCCESS;
 }
 
 /* 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;
+	efi_status_t r;
 
 	/* Allow unaligned memory access */
 	allow_unaligned();
@@ -421,18 +455,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		efi_install_configuration_table(&efi_guid_fdt, NULL);
 		printf("WARNING: booting without device tree\n");
 	}
-#ifdef CONFIG_CMD_BOOTEFI_HELLO
-	if (!strcmp(argv[1], "hello")) {
-		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
 
-		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
 #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
 	if (!strcmp(argv[1], "selftest")) {
 		struct efi_loaded_image_obj *image_obj;
@@ -447,30 +470,10 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 		r = 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();
-	} else {
-		saddr = argv[1];
-
-		addr = simple_strtoul(saddr, NULL, 16);
-		/* Check that a numeric value was passed */
-		if (!addr && *saddr != '0')
-			return CMD_RET_USAGE;
-
 	}
+#endif
 
-	printf("## Starting EFI application at %08lx ...\n", addr);
-	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
-			    bootefi_image_path);
-	printf("## Application terminated, r = %lu\n",
-	       r & ~EFI_ERROR_MASK);
-
-	if (r != EFI_SUCCESS)
-		return 1;
-	else
-		return 0;
+	return do_bootefi_load_and_exec(argv[1]);
 }
 
 #ifdef CONFIG_SYS_LONGHELP
@@ -489,7 +492,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"
-- 
2.19.1

[U-Boot] [PATCH 1/2] cmd: bootefi: move bootefi_test_prepare() forward

[Heinrich Schuchardt]

On 2/14/19 8:56 AM, AKASHI Takahiro wrote:
> This is a preparatory patch for reworking do_bootefi().
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

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

[U-Boot] [PATCH 2/2] cmd: bootefi: rework do_bootefi()

[Heinrich Schuchardt]

On 2/14/19 8:56 AM, AKASHI Takahiro wrote:
> In this patch, do_bootefi() will be reworked, without any functional
> change, as it is a bit sloppy after Heinrich's "efi_loader: rework
> loading and starting of images."
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  cmd/bootefi.c | 101 ++++++++++++++++++++++++++------------------------
>  1 file changed, 52 insertions(+), 49 deletions(-)

I agree that cmd/bootefi.c is not very easy to read.

We just went through a refactoring by Simon in v2019.01.

- With your proposal the total code does not get any shorter.
- The function modules do not get shorter.
- No bug is resolved.

I tend to revisit this after switching both the bootmgr and the
non-bootmgr paths to call LoadImage() and relying on Exit() to clean up
the image.

> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index e064edcd0cdb..159dc1ab8a30 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -361,34 +361,68 @@ err_add_protocol:
>  	return ret;
>  }
>  
> -static int do_bootefi_bootmgr_exec(void)
> +static int do_bootefi_load_and_exec(const char *arg)
>  {
> -	struct efi_device_path *device_path, *file_path;
> -	void *addr;
> +	void *image_buf;
> +	struct efi_device_path *device_path, *image_path;
> +	const char *saddr;
> +	unsigned long addr, size;

Please, use a pointer when referring to an address in memory and size_t
when referring to the difference between two pointers.

Outside of GCC there is no rule requiring unsigned long to have the size
of a pointer.

>  	efi_status_t r;
>  
> -	addr = efi_bootmgr_load(&device_path, &file_path);
> -	if (!addr)
> -		return 1;
> +	if (!strcmp(arg, "bootmgr")) {
> +		image_buf = efi_bootmgr_load(&device_path, &image_path);
> +		if (!image_buf)
> +			return CMD_RET_FAILURE;
> +
> +		addr = map_to_sysmem(image_buf);
> +	} else
> +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> +	if (!strcmp(arg, "hello")) {
> +		saddr = env_get("loadaddr");
> +		size = __efi_helloworld_end - __efi_helloworld_begin;
> +
> +		if (saddr)
> +			addr = simple_strtoul(saddr, NULL, 16);
> +		else
> +			addr = CONFIG_SYS_LOAD_ADDR;
> +
> +		image_buf = map_sysmem(addr, size);
> +		memcpy(image_buf, __efi_helloworld_begin, size);
> +
> +		device_path = NULL;
> +		image_path = NULL;
> +	} else/a
> +#endif
> +	{
> +		saddr = arg;
> +		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 %p ...\n", addr);
> -	r = do_bootefi_exec(addr, device_path, file_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       r & ~EFI_ERROR_MASK);
> +	printf("## Starting EFI application at %08lx ...\n", addr);

Can we be sure we will never load above 4 GiB? Using %p is a better choice.

> +	r = do_bootefi_exec(image_buf, device_path, image_path);
> +	printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
>  
>  	if (r != EFI_SUCCESS)
> -		return 1;
> +		return CMD_RET_FAILURE;

That's the style I prefer (and Simon dislikes).

>  
> -	return 0;
> +	return CMD_RET_SUCCESS;

Best regards

Heinrich

>  }
>  
>  /* Interpreter command to boot an arbitrary EFI image from memory */
>  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  {
> -	unsigned long addr;
> -	char *saddr;
> -	efi_status_t r;
>  	unsigned long fdt_addr;
> +	efi_status_t r;
>  
>  	/* Allow unaligned memory access */
>  	allow_unaligned();
> @@ -421,18 +455,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		efi_install_configuration_table(&efi_guid_fdt, NULL);
>  		printf("WARNING: booting without device tree\n");
>  	}
> -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> -	if (!strcmp(argv[1], "hello")) {
> -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
>  
> -		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
>  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
>  	if (!strcmp(argv[1], "selftest")) {
>  		struct efi_loaded_image_obj *image_obj;
> @@ -447,30 +470,10 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		r = 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();
> -	} else {
> -		saddr = argv[1];
> -
> -		addr = simple_strtoul(saddr, NULL, 16);
> -		/* Check that a numeric value was passed */
> -		if (!addr && *saddr != '0')
> -			return CMD_RET_USAGE;
> -
>  	}
> +#endif
>  
> -	printf("## Starting EFI application at %08lx ...\n", addr);
> -	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> -			    bootefi_image_path);
> -	printf("## Application terminated, r = %lu\n",
> -	       r & ~EFI_ERROR_MASK);
> -
> -	if (r != EFI_SUCCESS)
> -		return 1;
> -	else
> -		return 0;
> +	return do_bootefi_load_and_exec(argv[1]);
>  }
>  
>  #ifdef CONFIG_SYS_LONGHELP
> @@ -489,7 +492,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"
> 

[U-Boot] [PATCH 2/2] cmd: bootefi: rework do_bootefi()

[AKASHI Takahiro]

Heinrich,

# Did you intentionally remove ML from CC?

On Sat, Feb 16, 2019 at 01:00:07AM +0100, Heinrich Schuchardt wrote:
> On 2/14/19 8:56 AM, AKASHI Takahiro wrote:
> > In this patch, do_bootefi() will be reworked, without any functional
> > change, as it is a bit sloppy after Heinrich's "efi_loader: rework
> > loading and starting of images."
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > ---
> >  cmd/bootefi.c | 101 ++++++++++++++++++++++++++------------------------
> >  1 file changed, 52 insertions(+), 49 deletions(-)
> 
> I agree that cmd/bootefi.c is not very easy to read.
> 
> We just went through a refactoring by Simon in v2019.01.
> 
> - With your proposal the total code does not get any shorter.
> - The function modules do not get shorter.
> - No bug is resolved.

That is why I asked you to merge my patches into yours :)

> I tend to revisit this after switching both the bootmgr and the
> non-bootmgr paths to call LoadImage() and relying on Exit() to clean up
> the image.
> 
> > 
> > diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> > index e064edcd0cdb..159dc1ab8a30 100644
> > --- a/cmd/bootefi.c
> > +++ b/cmd/bootefi.c
> > @@ -361,34 +361,68 @@ err_add_protocol:
> >  	return ret;
> >  }
> >  
> > -static int do_bootefi_bootmgr_exec(void)
> > +static int do_bootefi_load_and_exec(const char *arg)
> >  {
> > -	struct efi_device_path *device_path, *file_path;
> > -	void *addr;
> > +	void *image_buf;
> > +	struct efi_device_path *device_path, *image_path;
> > +	const char *saddr;
> > +	unsigned long addr, size;
> 
> Please, use a pointer when referring to an address in memory and size_t
> when referring to the difference between two pointers.

Well, I agree, but I didn't change any thing from the original code
in this respect.

> Outside of GCC there is no rule requiring unsigned long to have the size
> of a pointer.

Ditto.

> >  	efi_status_t r;
> >  
> > -	addr = efi_bootmgr_load(&device_path, &file_path);
> > -	if (!addr)
> > -		return 1;
> > +	if (!strcmp(arg, "bootmgr")) {
> > +		image_buf = efi_bootmgr_load(&device_path, &image_path);
> > +		if (!image_buf)
> > +			return CMD_RET_FAILURE;
> > +
> > +		addr = map_to_sysmem(image_buf);
> > +	} else
> > +#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > +	if (!strcmp(arg, "hello")) {
> > +		saddr = env_get("loadaddr");
> > +		size = __efi_helloworld_end - __efi_helloworld_begin;
> > +
> > +		if (saddr)
> > +			addr = simple_strtoul(saddr, NULL, 16);
> > +		else
> > +			addr = CONFIG_SYS_LOAD_ADDR;
> > +
> > +		image_buf = map_sysmem(addr, size);
> > +		memcpy(image_buf, __efi_helloworld_begin, size);
> > +
> > +		device_path = NULL;
> > +		image_path = NULL;
> > +	} else/a
> > +#endif
> > +	{
> > +		saddr = arg;
> > +		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 %p ...\n", addr);
> > -	r = do_bootefi_exec(addr, device_path, file_path);
> > -	printf("## Application terminated, r = %lu\n",
> > -	       r & ~EFI_ERROR_MASK);
> > +	printf("## Starting EFI application at %08lx ...\n", addr);
> 
> Can we be sure we will never load above 4 GiB? Using %p is a better choice.

Ditto. I did preserve the original's specifier.

> > +	r = do_bootefi_exec(image_buf, device_path, image_path);
> > +	printf("## Application terminated, r = %lu\n", r & ~EFI_ERROR_MASK);
> >  
> >  	if (r != EFI_SUCCESS)
> > -		return 1;
> > +		return CMD_RET_FAILURE;
> 
> That's the style I prefer (and Simon dislikes).

Really? I believed that returning CMD_RET_XXX was required.

Thanks,
-Takahiro Akashi

> >  
> > -	return 0;
> > +	return CMD_RET_SUCCESS;
> 
> Best regards
> 
> Heinrich
> 
> >  }
> >  
> >  /* Interpreter command to boot an arbitrary EFI image from memory */
> >  static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  {
> > -	unsigned long addr;
> > -	char *saddr;
> > -	efi_status_t r;
> >  	unsigned long fdt_addr;
> > +	efi_status_t r;
> >  
> >  	/* Allow unaligned memory access */
> >  	allow_unaligned();
> > @@ -421,18 +455,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  		efi_install_configuration_table(&efi_guid_fdt, NULL);
> >  		printf("WARNING: booting without device tree\n");
> >  	}
> > -#ifdef CONFIG_CMD_BOOTEFI_HELLO
> > -	if (!strcmp(argv[1], "hello")) {
> > -		ulong size = __efi_helloworld_end - __efi_helloworld_begin;
> >  
> > -		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
> >  #ifdef CONFIG_CMD_BOOTEFI_SELFTEST
> >  	if (!strcmp(argv[1], "selftest")) {
> >  		struct efi_loaded_image_obj *image_obj;
> > @@ -447,30 +470,10 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> >  		r = 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();
> > -	} else {
> > -		saddr = argv[1];
> > -
> > -		addr = simple_strtoul(saddr, NULL, 16);
> > -		/* Check that a numeric value was passed */
> > -		if (!addr && *saddr != '0')
> > -			return CMD_RET_USAGE;
> > -
> >  	}
> > +#endif
> >  
> > -	printf("## Starting EFI application at %08lx ...\n", addr);
> > -	r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path,
> > -			    bootefi_image_path);
> > -	printf("## Application terminated, r = %lu\n",
> > -	       r & ~EFI_ERROR_MASK);
> > -
> > -	if (r != EFI_SUCCESS)
> > -		return 1;
> > -	else
> > -		return 0;
> > +	return do_bootefi_load_and_exec(argv[1]);
> >  }
> >  
> >  #ifdef CONFIG_SYS_LONGHELP
> > @@ -489,7 +492,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"
> > 
>