From mboxrd@z Thu Jan 1 00:00:00 1970 From: AKASHI Takahiro Date: Mon, 18 Feb 2019 09:39:39 +0900 Subject: [U-Boot] [PATCH 2/2] cmd: bootefi: rework do_bootefi() In-Reply-To: <6e7989cd-b026-74d7-3202-cab5937ddcea@gmx.de> References: <20190214075630.6672-1-takahiro.akashi@linaro.org> <20190214075630.6672-3-takahiro.akashi@linaro.org> <6e7989cd-b026-74d7-3202-cab5937ddcea@gmx.de> Message-ID: <20190218003938.GV20286@linaro.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > > --- > > 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 gets\n" > > >