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

[Andreas Färber]

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