efi_loader: Improvements to .dtb handling

A UEFI setup will typically have an EFI FAT partition, but device trees are more likely to be located on, e.g., Linux volumes. By convention the EFI partition will usually be placed first, so that we would find the EFI removable media binary and enter it without device tree from disk.

Therefore after finding an EFI removable media binary, probe other partitions on the same device for .dtb files.

In the default case the behavior will be unchanged in that only the first partition is searched.
In the case that there are two partitions marked bootable and the first being EFI, we will first check the first partition, then the second partition for the .dtb.
Duplicate loading will only result from non-first EFI partition order.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

include/config_distro_bootcmd.h +2 -3

index 087f576..eadec2e 100644
@@ -124,8 +124,7 @@
 		"fi\0"                                                    \
 	\
 	"load_efi_dtb="                                                   \
-		"load ${devtype} ${devnum}:${distro_bootpart} "           \
-			"${fdt_addr_r} "                                  \
+		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
 			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
 	\
 	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \

@@ -133,7 +132,7 @@
 		"for prefix in ${boot_prefixes}; do "                     \
 			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
 				"if test -e ${devtype} "                  \
-					"${devnum}:${distro_bootpart} "   \
+					"${devnum}:${efi_bootpart} "      \
 					"${prefix}${dtb_prefix}"          \
 					"${efi_fdtfile}; then "           \
 					"run load_efi_dtb; "              \

@@ -143,13 +142,22 @@
 	"scan_dev_for_efi="                                               \
 		"setenv efi_fdtfile ${fdtfile}; "                         \

include/config_distro_bootcmd.h +6 -0

+					"run scan_dev_for_efi_fdt; "      \
+				"done; "                                  \
 				"run boot_efi_binary; "                   \
 				"echo EFI LOAD FAILED: continuing...; "   \
+		"else "                                                   \
+			"setenv efi_bootpart ${distro_bootpart}; "        \
+			"run scan_dev_for_efi_fdt; "                      \
+			"setenv efi_bootpart; "                           \
 		"fi; "                                                    \
 		"setenv efi_fdtfile\0"
 #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"

Quoted 47 lines

A UEFI setup will typically have an EFI FAT partition, but device trees are more likely to be located on, e.g., Linux volumes. By convention the EFI partition will usually be placed first, so that we would find the EFI removable media binary and enter it without device tree from disk.

Therefore after finding an EFI removable media binary, probe other partitions on the same device for .dtb files.

In the default case the behavior will be unchanged in that only the first partition is searched.
In the case that there are two partitions marked bootable and the first being EFI, we will first check the first partition, then the second partition for the .dtb.
Duplicate loading will only result from non-first EFI partition order.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

include/config_distro_bootcmd.h +2 -3

index 087f576..eadec2e 100644
@@ -124,8 +124,7 @@
 		"fi\0"                                                    \
 	\
 	"load_efi_dtb="                                                   \
-		"load ${devtype} ${devnum}:${distro_bootpart} "           \
-			"${fdt_addr_r} "                                  \
+		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
 			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
 	\
 	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \

@@ -133,7 +132,7 @@
 		"for prefix in ${boot_prefixes}; do "                     \
 			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
 				"if test -e ${devtype} "                  \
-					"${devnum}:${distro_bootpart} "   \
+					"${devnum}:${efi_bootpart} "      \
 					"${prefix}${dtb_prefix}"          \
 					"${efi_fdtfile}; then "           \
 					"run load_efi_dtb; "              \

@@ -143,13 +142,22 @@
 	"scan_dev_for_efi="                                               \
 		"setenv efi_fdtfile ${fdtfile}; "                         \

include/config_distro_bootcmd.h line 152

Quoted 10 lines

include/config_distro_bootcmd.h +4 -1

 		BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
-		"run scan_dev_for_efi_fdt; "                              \
 		"if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
 					"efi/boot/"BOOTEFI_NAME"; then "  \
 				"echo Found EFI removable media binary "  \
 					"efi/boot/"BOOTEFI_NAME"; "       \
+				"for efi_bootpart in ${devplist}; do "    \
+					"echo Scanning for device tree "  \
+						"on ${devtype} ${devnum}" \
+						":${efi_bootpart}...; "   \

Printing can be quite slow on certain devices (think LCDs with uncached vram access), but have a *lot* of partitions (think Android partition scheme). I don't think we should print out every partition we're searching in, only the ones we find something at.

Maybe something like

Found EFI removable media binary in efi/boot/bootaa64.efi. Searching for dtb on mmc 0 ...
Found dtb on mmc 0:1 boot/dtb/foobar.dtb

would be a sensible amount of debug output :).

Also given that we're now running 3 cascaded for loops, it might be about time to think about how to break out of the loops in case we actually found something, so that the "normal" case is faster.

Alex

Quoted 12 lines

include/config_distro_bootcmd.h +6 -0

+					"run scan_dev_for_efi_fdt; "      \
+				"done; "                                  \
 				"run boot_efi_binary; "                   \
 				"echo EFI LOAD FAILED: continuing...; "   \
+		"else "                                                   \
+			"setenv efi_bootpart ${distro_bootpart}; "        \
+			"run scan_dev_for_efi_fdt; "                      \
+			"setenv efi_bootpart; "                           \
 		"fi; "                                                    \
 		"setenv efi_fdtfile\0"
 #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/config_distro_bootcmd.h +17 -1

index eadec2e..8f14457 100644
@@ -113,6 +113,15 @@
 #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
 #endif
 
+#if defined(CONFIG_ARM64)
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
+	"if test -n \"${soc_vendor}\"; then "                             \
+		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
+	"fi; "
+#else
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR
+#endif
+
 #define BOOTENV_SHARED_EFI                                                \
 	"boot_efi_binary="                                                \
 		"load ${devtype} ${devnum}:${distro_bootpart} "           \

@@ -125,18 +134,25 @@
 	\
 	"load_efi_dtb="                                                   \
 		"load ${devtype} ${devnum}:${efi_bootpart} ${fdt_addr_r} "\
-			"${prefix}${dtb_prefix}${efi_fdtfile}\0"          \
+			"${prefix}${dtb_prefix}${dtb_vendor_prefix}"      \
+			"${efi_fdtfile}\0"                                \
 	\
 	"efi_dtb_prefixes=\"\" dtb/ dtb/current/\0"                       \
 	"scan_dev_for_efi_fdt="                                           \
 		"for prefix in ${boot_prefixes}; do "                     \
 			"for dtb_prefix in ${efi_dtb_prefixes}; do "      \
+			    BOOTENV_EFI_SET_FDTFILE_VENDOR                \
+			    "for dtb_vendor_prefix in \"\" "              \
+					"${efi_dtb_vendor_prefix}; do "   \
 				"if test -e ${devtype} "                  \
 					"${devnum}:${efi_bootpart} "      \
 					"${prefix}${dtb_prefix}"          \
+					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
 					"run load_efi_dtb; "              \
 				"fi; "                                    \
+			    "done; "                                      \
+			    "setenv efi_dtb_vendor_prefix; "              \
 			"done; "                                          \
 		"done\0"                                                  \
 	"scan_dev_for_efi="                                               \

Quoted 8 lines

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

Stephen had pretty strong opinions on the naming, mostly because "pxe boot" uses the same naming scheme. So we should either change it there as well to stay consistent or just make the implicit ruling always work :).

I guess the best case would be to fix up the path names of boards so that $vendor is always $soc_vendor. Do you have an example where that wouldn't work?

Am 12.07.2016 um 09:29 schrieb Alexander Graf:

Quoted 10 lines · Andreas F?rber

Quoted 8 lines

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

Quoted 7 lines

Stephen had pretty strong opinions on the naming, mostly because "pxe boot" uses the same naming scheme. So we should either change it there as well to stay consistent or just make the implicit ruling always work :).

I guess the best case would be to fix up the path names of boards so that $vendor is always $soc_vendor. Do you have an example where that wouldn't work?

AFAIU for U-Boot $vendor is often the board vendor and matches the board/ subdirectory, so there may be plenty vendors. I didn't see any code setting this value, so I assume this comes from .config options.

My $soc_vendor is the SoC vendor instead and only for the environment.

And because there were opinions on how to form the 32-bit efi_fdtfile, both Tom and Stephen were CC'ed on the cover letter. :)

On the dragonboard410c:
CONFIG_SYS_SOC="snapdragon"
CONFIG_SYS_VENDOR="qualcomm" CONFIG_SYS_BOARD="dragonboard410c" CONFIG_SYS_CONFIG_NAME="dragonboard410c"
soc=snapdragon
vendor=qualcomm
board=dragonboard410c
board_name=dragonboard410c
=> Therefore my previous patch setting fdtfile to apq8016-sbc.dtb, because it is absolutely different. This patch allows searching qcom/ for our dtb-qcom aarch64 openSUSE package.

On the odroid-c2:
CONFIG_SYS_SOC="meson"
CONFIG_SYS_VENDOR="amlogic"
CONFIG_SYS_BOARD="odroid-c2" CONFIG_SYS_CONFIG_NAME="odroid-c2"
board=odroid-c2
board_name=odroid-c2
soc=meson
vendor=amlogic
=> Here $vendor would match Linux' dts subdirectory, but $soc and $board are wrong, I need fdtfile=meson-gxbb-odroidc2.dtb.

But maybe I'm just not understanding what all those configs are good for and how deeply they're tied to board/ subdirectories and mach-foo? If we can clean them up to match Linux then all the better. If that were intended, I wonder why no one flagged that during review.

Anyway, maybe add an else clause to this patch and reuse $vendor if $soc_vendor is unavailable?

Regards,
Andreas

Quoted 8 lines · Andreas F?rber

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

Stephen had pretty strong opinions on the naming, mostly because "pxe boot" uses the same naming scheme. So we should either change it there as well to stay consistent or just make the implicit ruling always work :).

I guess the best case would be to fix up the path names of boards so that $vendor is always $soc_vendor. Do you have an example where that wouldn't work?

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/config_distro_bootcmd.h +0 -0

index eadec2e..8f14457 100644

Quoted 21 lines

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/config_distro_bootcmd.h +0 -0

index eadec2e..8f14457 100644
@@ -113,6 +113,15 @@
 #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
 #endif
 

include/config_distro_bootcmd.h line 121

Quoted 6 lines

include/config_distro_bootcmd.h +6 -0

+#if defined(CONFIG_ARM64)
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
+	"if test -n \"${soc_vendor}\"; then "                             \
+		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
+	"fi; "
+#else

OK. Looking at the current Linux kernel, it's a given that for arm64 a DTB will be in a subdirectory, always. Perhaps we should fix this in Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this correctly per vendor (and yes, eventually there will be some "fun" as NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe try and futureproof ourselves so that we loop over this variable) ?

Am 12.07.2016 um 16:50 schrieb Tom Rini:

Quoted 22 lines · Andreas F?rber

Quoted 21 lines

On arm64 Linux device trees are organized by SoC vendor. Therefore we need to search the vendor subdirectory as well.

Since the SoC vendor may be different from ${vendor}, introduce a new ${soc_vendor}. If this is not set, the behavior remains unchanged.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

include/config_distro_bootcmd.h +0 -0

index eadec2e..8f14457 100644
@@ -113,6 +113,15 @@
 #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
 #endif
 

Quoted 13 lines

include/config_distro_bootcmd.h line 121

Quoted 6 lines

include/config_distro_bootcmd.h +6 -0

+#if defined(CONFIG_ARM64)
+#define BOOTENV_EFI_SET_FDTFILE_VENDOR                                    \
+	"if test -n \"${soc_vendor}\"; then "                             \
+		"setenv efi_dtb_vendor_prefix ${soc_vendor}/; "           \
+	"fi; "
+#else

OK. Looking at the current Linux kernel, it's a given that for arm64 a DTB will be in a subdirectory, always. Perhaps we should fix this in Kconfig and have... CONFIG_FDTFILE_VENDOR_DIRECTORY and set this correctly per vendor (and yes, eventually there will be some "fun" as NXP boards will sometimes be in freescale/ and sometimes nxp/ so maybe try and futureproof ourselves so that we loop over this variable) ?

Yes, it's a given that in the kernel the .dts is always in a vendor subdirectory - as listed in an earlier thread also for mips etc.

And yes, it's also true that due to our scripted openSUSE packaging we therefore install .dtb to /boot/dtb-foo/vendor/ - but that's not guaranteed for other distros and users, is it? Therefore I include "" in the loop further down.

Anyway, adding a config option sounds good. Will you look into that yourself or should I do that as part of this series?

I had considered the ambiguous vendor case and am iterating over $efi_dtb_vendor_prefix in a list of vendors already below, only the addition of the trailing slash above is limiting us to one currently. Do you have a concrete suggestion of how to make that more flexible?

We do so for the EFI binary already and it aids debugging.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 4 ++++
 1 file changed, 4 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 8f14457..0cf74e2 100644

Quoted 8 lines

We do so for the EFI binary already and it aids debugging.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 4 ++++
 1 file changed, 4 insertions(+)

include/config_distro_bootcmd.h line 152

Quoted 9 lines

include/config_distro_bootcmd.h +1 -0

index 8f14457..0cf74e2 100644
@@ -149,6 +149,10 @@
 					"${prefix}${dtb_prefix}"          \
 					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
+					"echo Found ${prefix}"            \

As mentioned in the other reply, I think this message is very useful, but should contain the target device name as well, so that we don't need to print out a message for every single partition we scan :).

Am 12.07.2016 um 09:30 schrieb Alexander Graf:

Quoted 9 lines · Andreas F?rber

Quoted 8 lines

We do so for the EFI binary already and it aids debugging.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 4 ++++
 1 file changed, 4 insertions(+)

Quoted 13 lines

include/config_distro_bootcmd.h line 152

Quoted 9 lines

include/config_distro_bootcmd.h +1 -0

index 8f14457..0cf74e2 100644
@@ -149,6 +149,10 @@
 					"${prefix}${dtb_prefix}"          \
 					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
+					"echo Found ${prefix}"            \

As mentioned in the other reply, I think this message is very useful, but should contain the target device name as well, so that we don't need to print out a message for every single partition we scan :).

I originally had it that way, but remember that issues like with patches 4 and 5 will not lead to any Found message at all. Therefore I still prefer decoupling the two.

Btw where does the "reading efi/boot/bootaa64.efi" message come from? That one could be dropped as duplicate instead! :)

Quoted 24 lines

Am 12.07.2016 um 09:30 schrieb Alexander Graf:

Quoted 22 lines · Andreas F?rber

Quoted 13 lines

We do so for the EFI binary already and it aids debugging.

Cc Alexander Graf <[email protected]>

Signed-off-by Andreas F?rber <[email protected]>

---
 include/config_distro_bootcmd.h | 4 ++++
 1 file changed, 4 insertions(+)

include/config_distro_bootcmd.h +0 -0

index 8f14457..0cf74e2 100644

include/config_distro_bootcmd.h line 152

Quoted 4 lines

include/config_distro_bootcmd.h +1 -0

@@ -149,6 +149,10 @@
 					"${prefix}${dtb_prefix}"          \
 					"${dtb_vendor_prefix}"            \
 					"${efi_fdtfile}; then "           \
+					"echo Found ${prefix}"            \

As mentioned in the other reply, I think this message is very useful, but should contain the target device name as well, so that we don't need to print out a message for every single partition we scan :).

Quoted 3 lines

I originally had it that way, but remember that issues like with patches 4 and 5 will not lead to any Found message at all. Therefore I still prefer decoupling the two.

We do detect the case where we don't have a working fdt loaded already, so we could probably print out the search paths there if we don't find a working fdt?

Quoted 2 lines

Btw where does the "reading efi/boot/bootaa64.efi" message come from? That one could be dropped as duplicate instead! :)

Phew. The "load" command maybe?