diff mbox series

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

Message ID 20190416042428.5007-9-takahiro.akashi@linaro.org
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: rework bootefi/bootmgr | expand

Commit Message

AKASHI Takahiro April 16, 2019, 4:24 a.m. UTC 
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(-)

Comments

Heinrich Schuchardt April 16, 2019, 5:31 a.m. UTC | #1 
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
>
AKASHI Takahiro April 17, 2019, 7:43 a.m. UTC | #2 
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
> >
>
diff mbox series

Patch

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