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 at 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"
>