| Message ID | 20190327044042.13707-10-takahiro.akashi@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: rework bootefi/bootmgr | expand |
On 3/27/19 5:40 AM, AKASHI Takahiro wrote: > This is a preparatory patch for reworking do_bootefi() in later patch. > All the non-boot-manager-based (that is, bootefi <addr>) code is put > into one function, do_boot_efi(). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/bootefi.c | 122 ++++++++++++++++++++++++++++---------------------- > 1 file changed, 68 insertions(+), 54 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 94b5bdeed3f1..39a0712af6ad 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt) > return 0; > } > > +/* > + * do_boot_efi() - execute EFI binary from command line > + * > + * @image_opt: string of image start address > + * @fdt_opt: string of fdt start address > + * Return: status code > + * > + * Set up memory image for the binary to be loaded, prepare > + * device path and then call do_bootefi_exec() to execute it. > + */ > +static int do_boot_efi(const char *image_opt, const char *fdt_opt) > +{ > + unsigned long addr; > + char *saddr; > + efi_status_t ret; > + unsigned long fdt_addr; > + > + /* Allow unaligned memory access */ > + allow_unaligned(); > + > + switch_to_non_secure_mode(); The two call above should be moved to efi_init_obj_list(). > + > + /* Initialize EFI drivers */ > + ret = efi_init_obj_list(); > + if (ret != EFI_SUCCESS) { > + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", > + ret & ~EFI_ERROR_MASK); > + return CMD_RET_FAILURE; > + } > + > + ret = efi_install_fdt(fdt_opt); > + if (ret != EFI_SUCCESS) > + return CMD_RET_FAILURE; > + > +#ifdef CONFIG_CMD_BOOTEFI_HELLO > + if (!strcmp(argv[1], "hello")) { > + ulong size = __efi_helloworld_end - __efi_helloworld_begin; > + > + saddr = env_get("loadaddr"); > + if (saddr) > + addr = simple_strtoul(saddr, NULL, 16); > + else > + addr = CONFIG_SYS_LOAD_ADDR; > + memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); > + } else > +#endif > + { > + saddr = argv[1]; > + > + addr = simple_strtoul(saddr, NULL, 16); > + /* Check that a numeric value was passed */ > + if (!addr && *saddr != '0') > + return CMD_RET_USAGE; > + } > + > + printf("## Starting EFI application at %08lx ...\n", addr); Shouldn't this be debug() in efi_start_image()? Best regards Heinrich > + ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, > + bootefi_image_path); > + printf("## Application terminated, r = %lu\n", > + ret & ~EFI_ERROR_MASK); > + > + if (ret != EFI_SUCCESS) > + return CMD_RET_FAILURE; > + else > + return CMD_RET_SUCCESS; > +} > + > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST > static efi_status_t bootefi_run_prepare(const char *load_options_path, > struct efi_device_path *device_path, > @@ -481,11 +548,6 @@ static int do_efi_selftest(const char *fdt_opt) > /* Interpreter command to boot an arbitrary EFI image from memory */ > static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > { > - unsigned long addr; > - char *saddr; > - efi_status_t r; > - unsigned long fdt_addr; > - > if (argc < 2) > return CMD_RET_USAGE; > > @@ -496,55 +558,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > return do_efi_selftest(argc > 2 ? argv[2] : NULL); > #endif > > - /* Allow unaligned memory access */ > - allow_unaligned(); > - > - switch_to_non_secure_mode(); > - > - /* Initialize EFI drivers */ > - r = efi_init_obj_list(); > - if (r != EFI_SUCCESS) { > - printf("Error: Cannot set up EFI drivers, r = %lu\n", > - r & ~EFI_ERROR_MASK); > - return CMD_RET_FAILURE; > - } > - > - r = fdt_install_fdt(argc > 2 ? argv[2] : NULL); > - if (r != EFI_SUCCESS) > - return r; > - > -#ifdef CONFIG_CMD_BOOTEFI_HELLO > - if (!strcmp(argv[1], "hello")) { > - ulong size = __efi_helloworld_end - __efi_helloworld_begin; > - > - saddr = env_get("loadaddr"); > - if (saddr) > - addr = simple_strtoul(saddr, NULL, 16); > - else > - addr = CONFIG_SYS_LOAD_ADDR; > - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); > - } else > -#endif > - { > - saddr = argv[1]; > - > - addr = simple_strtoul(saddr, NULL, 16); > - /* Check that a numeric value was passed */ > - if (!addr && *saddr != '0') > - return CMD_RET_USAGE; > - > - } > - > - printf("## Starting EFI application at %08lx ...\n", addr); > - r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, > - bootefi_image_path); > - printf("## Application terminated, r = %lu\n", > - r & ~EFI_ERROR_MASK); > - > - if (r != EFI_SUCCESS) > - return 1; > - else > - return 0; > + return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL); > } > > #ifdef CONFIG_SYS_LONGHELP >
On Fri, Apr 12, 2019 at 07:59:51AM +0200, Heinrich Schuchardt wrote: > On 3/27/19 5:40 AM, AKASHI Takahiro wrote: > >This is a preparatory patch for reworking do_bootefi() in later patch. > >All the non-boot-manager-based (that is, bootefi <addr>) code is put > >into one function, do_boot_efi(). > > > >Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > >--- > > cmd/bootefi.c | 122 ++++++++++++++++++++++++++++---------------------- > > 1 file changed, 68 insertions(+), 54 deletions(-) > > > >diff --git a/cmd/bootefi.c b/cmd/bootefi.c > >index 94b5bdeed3f1..39a0712af6ad 100644 > >--- a/cmd/bootefi.c > >+++ b/cmd/bootefi.c > >@@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt) > > return 0; > > } > > > >+/* > >+ * do_boot_efi() - execute EFI binary from command line > >+ * > >+ * @image_opt: string of image start address > >+ * @fdt_opt: string of fdt start address > >+ * Return: status code > >+ * > >+ * Set up memory image for the binary to be loaded, prepare > >+ * device path and then call do_bootefi_exec() to execute it. > >+ */ > >+static int do_boot_efi(const char *image_opt, const char *fdt_opt) > >+{ > >+ unsigned long addr; > >+ char *saddr; > >+ efi_status_t ret; > >+ unsigned long fdt_addr; > >+ > >+ /* Allow unaligned memory access */ > >+ allow_unaligned(); > >+ > >+ switch_to_non_secure_mode(); > > The two call above should be moved to efi_init_obj_list(). Same comment as in patch#8/11 > >+ > >+ /* Initialize EFI drivers */ > >+ ret = efi_init_obj_list(); > >+ if (ret != EFI_SUCCESS) { > >+ printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", > >+ ret & ~EFI_ERROR_MASK); > >+ return CMD_RET_FAILURE; > >+ } > >+ > >+ ret = efi_install_fdt(fdt_opt); > >+ if (ret != EFI_SUCCESS) > >+ return CMD_RET_FAILURE; > >+ > >+#ifdef CONFIG_CMD_BOOTEFI_HELLO > >+ if (!strcmp(argv[1], "hello")) { > >+ ulong size = __efi_helloworld_end - __efi_helloworld_begin; > >+ > >+ saddr = env_get("loadaddr"); > >+ if (saddr) > >+ addr = simple_strtoul(saddr, NULL, 16); > >+ else > >+ addr = CONFIG_SYS_LOAD_ADDR; > >+ memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); > >+ } else > >+#endif > >+ { > >+ saddr = argv[1]; > >+ > >+ addr = simple_strtoul(saddr, NULL, 16); > >+ /* Check that a numeric value was passed */ > >+ if (!addr && *saddr != '0') > >+ return CMD_RET_USAGE; > >+ } > >+ > >+ printf("## Starting EFI application at %08lx ...\n", addr); > > Shouldn't this be debug() in efi_start_image()? Okay, and application -> image # but I'm not sure that this information is useful. -Takahiro Akashi > Best regards > > Heinrich > > >+ ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, > >+ bootefi_image_path); > >+ printf("## Application terminated, r = %lu\n", > >+ ret & ~EFI_ERROR_MASK); > >+ > >+ if (ret != EFI_SUCCESS) > >+ return CMD_RET_FAILURE; > >+ else > >+ return CMD_RET_SUCCESS; > >+} > >+ > > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST > > static efi_status_t bootefi_run_prepare(const char *load_options_path, > > struct efi_device_path *device_path, > >@@ -481,11 +548,6 @@ static int do_efi_selftest(const char *fdt_opt) > > /* Interpreter command to boot an arbitrary EFI image from memory */ > > static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > { > >- unsigned long addr; > >- char *saddr; > >- efi_status_t r; > >- unsigned long fdt_addr; > >- > > if (argc < 2) > > return CMD_RET_USAGE; > > > >@@ -496,55 +558,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > > return do_efi_selftest(argc > 2 ? argv[2] : NULL); > > #endif > > > >- /* Allow unaligned memory access */ > >- allow_unaligned(); > >- > >- switch_to_non_secure_mode(); > >- > >- /* Initialize EFI drivers */ > >- r = efi_init_obj_list(); > >- if (r != EFI_SUCCESS) { > >- printf("Error: Cannot set up EFI drivers, r = %lu\n", > >- r & ~EFI_ERROR_MASK); > >- return CMD_RET_FAILURE; > >- } > >- > >- r = fdt_install_fdt(argc > 2 ? argv[2] : NULL); > >- if (r != EFI_SUCCESS) > >- return r; > >- > >-#ifdef CONFIG_CMD_BOOTEFI_HELLO > >- if (!strcmp(argv[1], "hello")) { > >- ulong size = __efi_helloworld_end - __efi_helloworld_begin; > >- > >- saddr = env_get("loadaddr"); > >- if (saddr) > >- addr = simple_strtoul(saddr, NULL, 16); > >- else > >- addr = CONFIG_SYS_LOAD_ADDR; > >- memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); > >- } else > >-#endif > >- { > >- saddr = argv[1]; > >- > >- addr = simple_strtoul(saddr, NULL, 16); > >- /* Check that a numeric value was passed */ > >- if (!addr && *saddr != '0') > >- return CMD_RET_USAGE; > >- > >- } > >- > >- printf("## Starting EFI application at %08lx ...\n", addr); > >- r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, > >- bootefi_image_path); > >- printf("## Application terminated, r = %lu\n", > >- r & ~EFI_ERROR_MASK); > >- > >- if (r != EFI_SUCCESS) > >- return 1; > >- else > >- return 0; > >+ return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL); > > } > > > > #ifdef CONFIG_SYS_LONGHELP > > >
On 4/12/19 9:22 AM, AKASHI Takahiro wrote: > On Fri, Apr 12, 2019 at 07:59:51AM +0200, Heinrich Schuchardt wrote: >> On 3/27/19 5:40 AM, AKASHI Takahiro wrote: >>> This is a preparatory patch for reworking do_bootefi() in later patch. >>> All the non-boot-manager-based (that is, bootefi <addr>) code is put >>> into one function, do_boot_efi(). >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> cmd/bootefi.c | 122 ++++++++++++++++++++++++++++---------------------- >>> 1 file changed, 68 insertions(+), 54 deletions(-) >>> >>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >>> index 94b5bdeed3f1..39a0712af6ad 100644 >>> --- a/cmd/bootefi.c >>> +++ b/cmd/bootefi.c >>> @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt) >>> return 0; >>> } >>> >>> +/* >>> + * do_boot_efi() - execute EFI binary from command line >>> + * >>> + * @image_opt: string of image start address >>> + * @fdt_opt: string of fdt start address >>> + * Return: status code >>> + * >>> + * Set up memory image for the binary to be loaded, prepare >>> + * device path and then call do_bootefi_exec() to execute it. >>> + */ >>> +static int do_boot_efi(const char *image_opt, const char *fdt_opt) >>> +{ >>> + unsigned long addr; >>> + char *saddr; >>> + efi_status_t ret; >>> + unsigned long fdt_addr; >>> + >>> + /* Allow unaligned memory access */ >>> + allow_unaligned(); >>> + >>> + switch_to_non_secure_mode(); >> >> The two call above should be moved to efi_init_obj_list(). > > Same comment as in patch#8/11 > >>> + >>> + /* Initialize EFI drivers */ >>> + ret = efi_init_obj_list(); >>> + if (ret != EFI_SUCCESS) { >>> + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", >>> + ret & ~EFI_ERROR_MASK); >>> + return CMD_RET_FAILURE; >>> + } >>> + >>> + ret = efi_install_fdt(fdt_opt); >>> + if (ret != EFI_SUCCESS) >>> + return CMD_RET_FAILURE; >>> + >>> +#ifdef CONFIG_CMD_BOOTEFI_HELLO >>> + if (!strcmp(argv[1], "hello")) { >>> + ulong size = __efi_helloworld_end - __efi_helloworld_begin; >>> + >>> + saddr = env_get("loadaddr"); >>> + if (saddr) >>> + addr = simple_strtoul(saddr, NULL, 16); >>> + else >>> + addr = CONFIG_SYS_LOAD_ADDR; >>> + memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >>> + } else >>> +#endif >>> + { >>> + saddr = argv[1]; >>> + >>> + addr = simple_strtoul(saddr, NULL, 16); >>> + /* Check that a numeric value was passed */ >>> + if (!addr && *saddr != '0') >>> + return CMD_RET_USAGE; >>> + } >>> + >>> + printf("## Starting EFI application at %08lx ...\n", addr); >> >> Shouldn't this be debug() in efi_start_image()? > > Okay, and application -> image > # but I'm not sure that this information is useful. When debugging I would be more interested in the address to which the image is loaded so that I have the offset at hand to load the text symbols into gdb. I am happy if you simply drop this entry address output. Regards Heinrich > > -Takahiro Akashi > > >> Best regards >> >> Heinrich >> >>> + ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >>> + bootefi_image_path); >>> + printf("## Application terminated, r = %lu\n", >>> + ret & ~EFI_ERROR_MASK); >>> + >>> + if (ret != EFI_SUCCESS) >>> + return CMD_RET_FAILURE; >>> + else >>> + return CMD_RET_SUCCESS; >>> +} >>> + >>> #ifdef CONFIG_CMD_BOOTEFI_SELFTEST >>> static efi_status_t bootefi_run_prepare(const char *load_options_path, >>> struct efi_device_path *device_path, >>> @@ -481,11 +548,6 @@ static int do_efi_selftest(const char *fdt_opt) >>> /* Interpreter command to boot an arbitrary EFI image from memory */ >>> static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> { >>> - unsigned long addr; >>> - char *saddr; >>> - efi_status_t r; >>> - unsigned long fdt_addr; >>> - >>> if (argc < 2) >>> return CMD_RET_USAGE; >>> >>> @@ -496,55 +558,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) >>> return do_efi_selftest(argc > 2 ? argv[2] : NULL); >>> #endif >>> >>> - /* Allow unaligned memory access */ >>> - allow_unaligned(); >>> - >>> - switch_to_non_secure_mode(); >>> - >>> - /* Initialize EFI drivers */ >>> - r = efi_init_obj_list(); >>> - if (r != EFI_SUCCESS) { >>> - printf("Error: Cannot set up EFI drivers, r = %lu\n", >>> - r & ~EFI_ERROR_MASK); >>> - return CMD_RET_FAILURE; >>> - } >>> - >>> - r = fdt_install_fdt(argc > 2 ? argv[2] : NULL); >>> - if (r != EFI_SUCCESS) >>> - return r; >>> - >>> -#ifdef CONFIG_CMD_BOOTEFI_HELLO >>> - if (!strcmp(argv[1], "hello")) { >>> - ulong size = __efi_helloworld_end - __efi_helloworld_begin; >>> - >>> - saddr = env_get("loadaddr"); >>> - if (saddr) >>> - addr = simple_strtoul(saddr, NULL, 16); >>> - else >>> - addr = CONFIG_SYS_LOAD_ADDR; >>> - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); >>> - } else >>> -#endif >>> - { >>> - saddr = argv[1]; >>> - >>> - addr = simple_strtoul(saddr, NULL, 16); >>> - /* Check that a numeric value was passed */ >>> - if (!addr && *saddr != '0') >>> - return CMD_RET_USAGE; >>> - >>> - } >>> - >>> - printf("## Starting EFI application at %08lx ...\n", addr); >>> - r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, >>> - bootefi_image_path); >>> - printf("## Application terminated, r = %lu\n", >>> - r & ~EFI_ERROR_MASK); >>> - >>> - if (r != EFI_SUCCESS) >>> - return 1; >>> - else >>> - return 0; >>> + return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL); >>> } >>> >>> #ifdef CONFIG_SYS_LONGHELP >>> >> >
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index 94b5bdeed3f1..39a0712af6ad 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -355,6 +355,73 @@ static int do_efibootmgr(const char *fdt_opt) return 0; } +/* + * do_boot_efi() - execute EFI binary from command line + * + * @image_opt: string of image start address + * @fdt_opt: string of fdt start address + * Return: status code + * + * Set up memory image for the binary to be loaded, prepare + * device path and then call do_bootefi_exec() to execute it. + */ +static int do_boot_efi(const char *image_opt, const char *fdt_opt) +{ + unsigned long addr; + char *saddr; + efi_status_t ret; + unsigned long fdt_addr; + + /* Allow unaligned memory access */ + allow_unaligned(); + + switch_to_non_secure_mode(); + + /* Initialize EFI drivers */ + ret = efi_init_obj_list(); + if (ret != EFI_SUCCESS) { + printf("Error: Cannot initialize UEFI sub-system, r = %lu\n", + ret & ~EFI_ERROR_MASK); + return CMD_RET_FAILURE; + } + + ret = efi_install_fdt(fdt_opt); + if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + +#ifdef CONFIG_CMD_BOOTEFI_HELLO + if (!strcmp(argv[1], "hello")) { + ulong size = __efi_helloworld_end - __efi_helloworld_begin; + + saddr = env_get("loadaddr"); + if (saddr) + addr = simple_strtoul(saddr, NULL, 16); + else + addr = CONFIG_SYS_LOAD_ADDR; + memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); + } else +#endif + { + saddr = argv[1]; + + addr = simple_strtoul(saddr, NULL, 16); + /* Check that a numeric value was passed */ + if (!addr && *saddr != '0') + return CMD_RET_USAGE; + } + + printf("## Starting EFI application at %08lx ...\n", addr); + ret = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, + bootefi_image_path); + printf("## Application terminated, r = %lu\n", + ret & ~EFI_ERROR_MASK); + + if (ret != EFI_SUCCESS) + return CMD_RET_FAILURE; + else + return CMD_RET_SUCCESS; +} + #ifdef CONFIG_CMD_BOOTEFI_SELFTEST static efi_status_t bootefi_run_prepare(const char *load_options_path, struct efi_device_path *device_path, @@ -481,11 +548,6 @@ static int do_efi_selftest(const char *fdt_opt) /* Interpreter command to boot an arbitrary EFI image from memory */ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { - unsigned long addr; - char *saddr; - efi_status_t r; - unsigned long fdt_addr; - if (argc < 2) return CMD_RET_USAGE; @@ -496,55 +558,7 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) return do_efi_selftest(argc > 2 ? argv[2] : NULL); #endif - /* Allow unaligned memory access */ - allow_unaligned(); - - switch_to_non_secure_mode(); - - /* Initialize EFI drivers */ - r = efi_init_obj_list(); - if (r != EFI_SUCCESS) { - printf("Error: Cannot set up EFI drivers, r = %lu\n", - r & ~EFI_ERROR_MASK); - return CMD_RET_FAILURE; - } - - r = fdt_install_fdt(argc > 2 ? argv[2] : NULL); - if (r != EFI_SUCCESS) - return r; - -#ifdef CONFIG_CMD_BOOTEFI_HELLO - if (!strcmp(argv[1], "hello")) { - ulong size = __efi_helloworld_end - __efi_helloworld_begin; - - saddr = env_get("loadaddr"); - if (saddr) - addr = simple_strtoul(saddr, NULL, 16); - else - addr = CONFIG_SYS_LOAD_ADDR; - memcpy(map_sysmem(addr, size), __efi_helloworld_begin, size); - } else -#endif - { - saddr = argv[1]; - - addr = simple_strtoul(saddr, NULL, 16); - /* Check that a numeric value was passed */ - if (!addr && *saddr != '0') - return CMD_RET_USAGE; - - } - - printf("## Starting EFI application at %08lx ...\n", addr); - r = do_bootefi_exec(map_sysmem(addr, 0), bootefi_device_path, - bootefi_image_path); - printf("## Application terminated, r = %lu\n", - r & ~EFI_ERROR_MASK); - - if (r != EFI_SUCCESS) - return 1; - else - return 0; + return do_boot_efi(argv[1], argc > 2 ? argv[2] : NULL); } #ifdef CONFIG_SYS_LONGHELP
This is a preparatory patch for reworking do_bootefi() in later patch. All the non-boot-manager-based (that is, bootefi <addr>) code is put into one function, do_boot_efi(). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/bootefi.c | 122 ++++++++++++++++++++++++++++---------------------- 1 file changed, 68 insertions(+), 54 deletions(-)