On Thu, Oct 18, 2018 at 09:31:57AM +0200, Alexander Graf wrote:
On 18.10.18 04:07, AKASHI Takahiro wrote:
On Tue, Oct 16, 2018 at 03:15:13PM +0200, Alexander Graf wrote:
On 12.10.18 07:07, AKASHI Takahiro wrote:
The current scenario for default UEFI booting, scan_dev_for_efi, has several issues:
- invoke 'bootmgr' only if BOOTEFI_NAME binary does exit even though 'bootmgr' can and should work independently whether or not the binary exist,
- always assume that a 'fdtfile' variable is defined,
- redundantly check for 'fdt_addr_r' in boot_efi_binary
In this patch, all the issues above are sorted out.
Signed-off-by: AKASHI Takahiro takahiro.akashi@linaro.org
include/config_distro_bootcmd.h | 43 ++++++++++++++++----------------- 1 file changed, 21 insertions(+), 22 deletions(-)
diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h index 373fee78a999..76e12b7bf4ee 100644 --- a/include/config_distro_bootcmd.h +++ b/include/config_distro_bootcmd.h @@ -124,42 +124,41 @@
#define BOOTENV_SHARED_EFI \ "boot_efi_binary=" \
"if fdt addr ${fdt_addr_r}; then " \"bootefi bootmgr ${fdt_addr_r};" \"else " \"bootefi bootmgr ${fdtcontroladdr};" \"fi;" \"if fdt addr ${fdt_addr_r}; then " \"bootefi ${kernel_addr_r} ${fdt_addr_r};" \"else " \"bootefi ${kernel_addr_r} ${fdtcontroladdr};" \"fi\0" \
"bootefi ${kernel_addr_r} ${efi_fdt_addr};\0" \
"${fdt_addr_r} ${prefix}${efi_fdtfile}\0" \
"${fdt_addr_r} ${prefix}${efi_fdtfile};" \"if fdt addr ${fdt_addr_r}; then " \"setenv efi_fdt_addr ${fdt_addr_r}; " \"else; " \"setenv efi_fdt_addr ${fdtcontroladdr}; " \"fi;\0" \
- "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0" \
- "scan_dev_for_efi=" \
- "set_efi_fdt_addr=" \ "setenv efi_fdtfile ${fdtfile}; " \ BOOTENV_EFI_SET_FDTFILE_FALLBACK \
"for prefix in ${efi_dtb_prefixes}; do " \I fail to see where the prefix logic went? Without that, our distro's dtb loading will break.
Oops, I missed it.
"if test -e ${devtype} " \"${devnum}:${distro_bootpart} " \"${prefix}${efi_fdtfile}; then " \"run load_efi_dtb; " \"fi;" \"done;" \
"if test x${efi_fdtfile} != x -a x{$fdt_addr_r} != x ; then " \"run load_efi_dtb; " \"else; " \"setenv efi_fdt_addr ${fdtcontroladdr}; " \"fi; " \Just unconditionally set efi_fdt_addr=$fdtcontrolladdr before the check (and invocation of load_efi_dtb).
OK.
That way you don't need to check for the failure case in load_efi_dtb again.
Well, I think that we need a check for validity with "fdt addr" command just in case that a fdt file exist but its content be corrupted.
My modified version looks like: ===8<=== #define BOOTENV_SHARED_EFI \ "boot_efi_binary=" \ "load ${devtype} ${devnum}:${distro_bootpart} " \ "${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; " \ "bootefi ${kernel_addr_r} ${efi_fdt_addr};\0" \ \ "load_efi_dtb=" \ "efi_dtb_prefixes=/ /dtb/ /dtb/current/" \
You want to keep those in a global variable so they can get adapted in the environment easily.
Do you mean something like below? if x${efi_dtb_prefixes} = x ; then setenv efi_dtb_prefixes "/ /dtb/ /dtb/current/"; fi ... setenv efi_dtb_prefixes;
Also I think the way you wrote it right now fails to execute anyway, as it's missing a semicolon :).
Thanks.
"for prefix in ${efi_dtb_prefixes}; do " \ "load ${devtype} ${devnum}:${distro_bootpart} " \ "${fdt_addr_r} ${prefix}${efi_fdtfile};" \Will this fail silently or throw error messaged on the screen? If there are error messages, you need to keep the test -e around :). We don't want to confuse users with bogus error messages.
Yes, we will see "** Unable to read file /boot/XXX.dtb **" Will fix.
"if fdt addr ${fdt_addr_r}; then " \ "setenv efi_fdt_addr ${fdt_addr_r}; " \ "fi; " \ "done;\0" \ \ "set_efi_fdt_addr=" \ "efi_fdtfile=${fdtfile}; " \ BOOTENV_EFI_SET_FDTFILE_FALLBACK \ "setenv efi_fdt_addr ${fdtcontroladdr}; " \ "if test x${efi_fdtfile} != x -a x{$fdt_addr_r} != x ; then " \ "run load_efi_dtb; " \ "fi;\0" \ "setenv efi_fdtfile\0" \ \ "scan_dev_for_efi=" \ "run set_efi_fdt_addr; " \ "bootefi bootmgr ${efi_fdt_addr};" \So we're running the bootmgr over and over again for every target device? I don't think that's very useful. It should only run once.
OK, but do you want "bootefi bootmgr" before OR after scanning boot targets in distro_bootcmd?
I'm personally also perfectly ok with not supporting dynamic DT loading with bootmgr. The whole dynamic DT loading logic is only there for boards where we don't have a stable DT yet. On those we usually don't have bootmgr support either, because we're lacking persistent variable storage ;).
Yeah, but it doesn't hurt anything unless we explicitly define "fdtfile."
Thanks, -Takahiro Akashi
Alex
"if test -e ${devtype} ${devnum}:${distro_bootpart} " \ "efi/boot/"BOOTEFI_NAME"; then " \ "echo Found EFI removable media binary " \ "efi/boot/"BOOTEFI_NAME"; " \ "run boot_efi_binary; " \ "echo EFI LOAD FAILED: continuing...; " \ "fi; " \ "setenv efi_fdt_addr\0"===>8=== (not tested yet)
-Takahiro Akashi
Alex
"setenv efi_fdtfile\0" \- \
- "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0" \
- "scan_dev_for_efi=" \
"run set_efi_fdt_addr; " \"bootefi bootmgr ${efi_fdt_addr};" \
"fi; " \"setenv efi_fdtfile\0"
"fi; " \"setenv efi_fdt_addr\0"#define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;" #else #define BOOTENV_SHARED_EFI