| Message ID | 20190214075630.6672-3-takahiro.akashi@linaro.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: rework loading (continued) | expand |
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" >
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" > > >
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"
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(-)