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

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

[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>

---

v1 -> v2:

  - Make fdt parameter optional
  - Explain fdt parameter more technically
  - Fix whitespace
---
 cmd/bootefi.c                   | 36 ++++++++++++++++--------------------
 include/config_distro_bootcmd.h |  9 ++++++---
 2 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/cmd/bootefi.c b/cmd/bootefi.c
index b213ef1..7f552fc 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,8 +209,8 @@ 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 = 0;
 	int r = 0;
 
 	if (argc < 2)
@@ -226,8 +219,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 
 	addr = simple_strtoul(saddr, NULL, 16);
 
+	if (argc > 2) {
+		sfdt = argv[2];
+		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 +236,14 @@ 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>.\n"
+	"    If specified, the device tree located at <fdt address> gets\n"
+	"    exposed as EFI configuration table.\n";
 #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..dddebc3 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"                                                    \
+			"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="                                               \
-- 
1.8.5.6

[U-Boot] [PATCH v2 2/2] efi_loader: Fall back to fdtfile naming convention

[Alexander Graf]

When there is no $fdtfile variable set, we still have a good chance
that on 32bit arm the fdtfile really is just called $soc-$board.dtb.

Enable the exports for $soc and $board in our distr defaults and make
use of them in the efi boot script.

Reported-by: Andreas Faerber <afaerber@suse.de>
Reported-by: Stephen Warren <swarren@wwwdotorg.org>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - set CONFIG_ENV_VARS_UBOOT_CONFIG instead of runtime config
  - s/efifdtfile/efi_fdtfile/g
---
 include/config_distro_bootcmd.h  | 24 +++++++++++++++++++++---
 include/config_distro_defaults.h |  1 +
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index dddebc3..7f67344 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -99,6 +99,21 @@
 #endif
 
 #ifdef BOOTEFI_NAME
+#if defined(CONFIG_ARM) && !defined(CONFIG_ARM64)
+/*
+ * On 32bit ARM systems there is a reasonable number of systems that follow
+ * the $soc-$board$boardver.dtb name scheme for their device trees. Use that
+ * scheme if we don't have an explicit fdtfile variable.
+ */
+#define BOOTENV_EFI_SET_FDTFILE_FALLBACK                                  \
+	"if test -z \"${fdtfile}\" -a -n \"${soc}\"; then "               \
+	  "setenv efi_fdtfile ${soc}-${board}${boardver}.dtb; "           \
+	"fi; "
+#else
+#define BOOTENV_EFI_SET_FDTFILE_FALLBACK
+#endif
+
+
 #define BOOTENV_SHARED_EFI                                                \
 	"boot_efi_binary="                                                \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \
@@ -111,14 +126,16 @@
 	\
 	"load_efi_dtb="                                                   \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \
-			"${fdt_addr_r} ${prefix}${fdtfile}\0"             \
+			"${fdt_addr_r} ${prefix}${efi_fdtfile}\0"         \
 	\
 	"efi_dtb_prefixes=/ /dtb/ /dtb/current/\0"                        \
 	"scan_dev_for_efi="                                               \
+		"setenv efi_fdtfile ${fdtfile}; "                         \
+		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
 		"for prefix in ${efi_dtb_prefixes}; do "                  \
 			"if test -e ${devtype} "                          \
 					"${devnum}:${distro_bootpart} "   \
-					"${prefix}${fdtfile}; then "      \
+					"${prefix}${efi_fdtfile}; then "  \
 				"run load_efi_dtb; "                      \
 			"fi;"                                             \
 		"done;"                                                   \
@@ -128,7 +145,8 @@
 					"efi/boot/"BOOTEFI_NAME"; "       \
 				"run boot_efi_binary; "                   \
 				"echo EFI LOAD FAILED: continuing...; "   \
-		"fi; \0"
+		"fi; "                                                    \
+		"setenv efi_fdtfile\0"
 #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
 #else
 #define BOOTENV_SHARED_EFI
diff --git a/include/config_distro_defaults.h b/include/config_distro_defaults.h
index 076be4d..5cc2af8 100644
--- a/include/config_distro_defaults.h
+++ b/include/config_distro_defaults.h
@@ -65,5 +65,6 @@
 #define CONFIG_ISO_PARTITION
 #define CONFIG_SUPPORT_RAW_INITRD
 #define CONFIG_SYS_HUSH_PARSER
+#define CONFIG_ENV_VARS_UBOOT_CONFIG
 
 #endif	/* _CONFIG_CMD_DISTRO_DEFAULTS_H */
-- 
1.8.5.6

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

[Stephen Warren]

On 04/14/2016 08:07 AM, Alexander Graf wrote:
> 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.

The series,
Acked-by: Stephen Warren <swarren@wwwdotorg.org>

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

[Andreas Färber]

Am 14.04.2016 um 16:07 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>
> 
> ---
> 
> v1 -> v2:
> 
>   - Make fdt parameter optional
>   - Explain fdt parameter more technically
>   - Fix whitespace
> ---
>  cmd/bootefi.c                   | 36 ++++++++++++++++--------------------
>  include/config_distro_bootcmd.h |  9 ++++++---
>  2 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index b213ef1..7f552fc 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");

Nit: Now it can also be an absent DT again.

>  		systab.nr_tables = 0;
>  	}
>  
> @@ -216,8 +209,8 @@ 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 = 0;
>  	int r = 0;
>  
>  	if (argc < 2)
> @@ -226,8 +219,13 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  
>  	addr = simple_strtoul(saddr, NULL, 16);
>  
> +	if (argc > 2) {
> +		sfdt = argv[2];
> +		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 +236,14 @@ 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>.\n"
> +	"    If specified, the device tree located at <fdt address> gets\n"
> +	"    exposed as EFI configuration table.\n";
>  #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..dddebc3 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"                                                    \
> +			"bootefi ${kernel_addr_r} ${fdtcontroladdr};"     \

Stephen, didn't you say you had problems with this? Might've been nice
to put this behavioral change into a separate patch as before, but well:

Reviewed-by: Andreas F?rber <afaerber@suse.de>

Regards,
Andreas

> +		"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="                                               \
> 


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

[U-Boot] [PATCH v2 2/2] efi_loader: Fall back to fdtfile naming convention

[Andreas Färber]

Am 14.04.2016 um 16:07 schrieb Alexander Graf:
> When there is no $fdtfile variable set, we still have a good chance
> that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
> 
> Enable the exports for $soc and $board in our distr defaults and make

"distro"

> use of them in the efi boot script.
> 
> Reported-by: Andreas Faerber <afaerber@suse.de>
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> 
> ---
> 
> v1 -> v2:
> 
>   - set CONFIG_ENV_VARS_UBOOT_CONFIG instead of runtime config
>   - s/efifdtfile/efi_fdtfile/g

Otherwise

Reviewed-by: Andreas F?rber <afaerber@suse.de>

ARM64 can be handled as follow-up.

Thanks,
Andreas

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

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

[Stephen Warren]

On 04/14/2016 09:35 AM, Andreas F?rber wrote:
> Am 14.04.2016 um 16:07 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.

>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>> index ad9045e..dddebc3 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"                                                    \
>> +			"bootefi ${kernel_addr_r} ${fdtcontroladdr};"     \
>
> Stephen, didn't you say you had problems with this? Might've been nice
> to put this behavioral change into a separate patch as before, but well:
>
> Reviewed-by: Andreas F?rber <afaerber@suse.de>

The issue was with bootefi assuming that the control FDT was what gets 
passed to the EFI binary. With the above, the command takes an explicit 
parameter and hence makes no such assumption. It would be nice if there 
was an "fdt validate its a device tree" rather than "fdt addr" command 
to use here, but there isn't right now.

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

[Tom Rini]

On Thu, Apr 14, 2016 at 04:07:53PM +0200, Alexander Graf wrote:

> 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>
> Acked-by: Stephen Warren <swarren@wwwdotorg.org>
> Reviewed-by: Andreas F?rber <afaerber@suse.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160421/d6321499/attachment.sig>

[U-Boot] [U-Boot, v2, 2/2] efi_loader: Fall back to fdtfile naming convention

[Tom Rini]

On Thu, Apr 14, 2016 at 04:07:54PM +0200, Alexander Graf wrote:

> When there is no $fdtfile variable set, we still have a good chance
> that on 32bit arm the fdtfile really is just called $soc-$board.dtb.
> 
> Enable the exports for $soc and $board in our distr defaults and make
> use of them in the efi boot script.
> 
> Reported-by: Andreas Faerber <afaerber@suse.de>
> Reported-by: Stephen Warren <swarren@wwwdotorg.org>
> Signed-off-by: Alexander Graf <agraf@suse.de>
> Reviewed-by: Andreas F?rber <afaerber@suse.de>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160421/a251be2c/attachment.sig>