[U-Boot] [PATCH 1/2] efi_loader: Pass fdt address directly to bootefi cmd

Andreas Färber
2016-04-13T22:26:38Z

Am 13.04.2016 um 23:22 schrieb Alexander Graf:
> The bootefi cmd today fetches its device tree pointer from either the
> location appointed by "fdt addr" with a fallback to the U-Boot control
> fdt.
>
> This integration is unusual for U-Boot and diverges from the way we
> usually handle parameters to boot commands. So let's pass the fdt
> directly into the bootefi command instead and move the control fdt
> logic into the distro boot script.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
> cmd/bootefi.c | 34 +++++++++++++---------------------
> include/config_distro_bootcmd.h | 9 ++++++---
> 2 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index b213ef1..ab39b95 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -142,12 +142,11 @@ static void *copy_fdt(void *fdt)
> * Load an EFI payload into a newly allocated piece of memory, register all
> * EFI objects it would want to access and jump to it.
> */
> -static unsigned long do_bootefi_exec(void *efi)
> +static unsigned long do_bootefi_exec(void *efi, void *fdt)
> {
> ulong (*entry)(void *image_handle, struct efi_system_table *st);
> ulong fdt_pages, fdt_size, fdt_start, fdt_end;
> bootm_headers_t img = { 0 };
> - void *fdt = working_fdt;
>
> /*
> * gd lives in a fixed register which may get clobbered while we execute
> @@ -155,13 +154,7 @@ static unsigned long do_bootefi_exec(void *efi)
> */
> efi_save_gd();
>
> - /* Update system table to point to our currently loaded FDT */
> -
> - /* Fall back to included fdt if none was manually loaded */
> - if (!fdt && gd->fdt_blob)
> - fdt = (void *)gd->fdt_blob;
> -
> - if (fdt) {
> + if (fdt && !fdt_check_header(fdt)) {
> /* Prepare fdt for payload */
> fdt = copy_fdt(fdt);
>
> @@ -185,7 +178,7 @@ static unsigned long do_bootefi_exec(void *efi)
> efi_add_memory_map(fdt_start, fdt_pages,
> EFI_BOOT_SERVICES_DATA, true);
> } else {
> - printf("WARNING: No device tree loaded, expect boot to fail\n");
> + printf("WARNING: Invalid device tree, expect boot to fail\n");
> systab.nr_tables = 0;
> }
>
> @@ -216,18 +209,20 @@ static unsigned long do_bootefi_exec(void *efi)
> /* 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[])
> {
> - char *saddr;
> - unsigned long addr;
> + char *saddr, *sfdt;
> + unsigned long addr, fdt_addr;
> int r = 0;
>
> - if (argc < 2)
> + if (argc < 3)
> return 1;

Hm, I had specifically requested to make it an optional argument...
do_bootefi_exec() seems to still handle !fdt, but here we seem to choke
on absence of the third argument, not matching bootm/bootz/booti? GRUB
does not need a DT to display its shell/menu, so we don't need to force
it IMO. Think of people calling it from the prompt or from a script.

Or am I misunderstanding U-Boot argument handling?

> saddr = argv[1];
> + sfdt = argv[2];
>
> addr = simple_strtoul(saddr, NULL, 16);
> + fdt_addr = simple_strtoul(sfdt, NULL, 16);
>
> printf("## Starting EFI application at 0x%08lx ...\n", addr);
> - r = do_bootefi_exec((void *)addr);
> + r = do_bootefi_exec((void *)addr, (void*)fdt_addr);
> printf("## Application terminated, r = %d\n", r);
>
> if (r != 0)
> @@ -238,16 +233,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>
> #ifdef CONFIG_SYS_LONGHELP
> static char bootefi_help_text[] =
> - "<image address>\n"
> - " - boot EFI payload stored at address <image address>\n"
> - "\n"
> - "Since most EFI payloads want to have a device tree provided, please\n"
> - "make sure you load a device tree using the fdt addr command before\n"
> - "executing bootefi.\n";
> + "<image address> <fdt address>\n"
> + " - boot EFI payload stored at address <image address> with a device\n"
> + " tree located at <fdt address>\n";

Nit: This could get a better explanation than "with a ...". :) U-Boot
exposes it as some table/hook/callback/etc.?

> #endif
>
> U_BOOT_CMD(
> - bootefi, 2, 0, do_bootefi,
> + bootefi, 3, 0, do_bootefi,
> "Boots an EFI payload from memory\n",
> bootefi_help_text
> );
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index ad9045e..67eb8f2 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -103,12 +103,15 @@
> "boot_efi_binary=" \
> "load ${devtype} ${devnum}:${distro_bootpart} " \
> "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \
> - "bootefi ${kernel_addr_r}\0" \
> + "if fdt addr ${fdt_addr_r}; then " \
> + "bootefi ${kernel_addr_r} ${fdt_addr_r};" \
> + "else" \

Spaces vs. tabs?

> + "bootefi ${kernel_addr_r} ${fdtcontroladdr};" \
> + "fi\0" \
> \
> "load_efi_dtb=" \
> "load ${devtype} ${devnum}:${distro_bootpart} " \
> - "${fdt_addr_r} ${prefix}${fdtfile}; " \
> - "fdt addr ${fdt_addr_r}\0" \
> + "${fdt_addr_r} ${prefix}${fdtfile}\0" \
> \
> "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0" \
> "scan_dev_for_efi=" \

Regards,
Andreas

--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 N?rnberg, Germany
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)