[U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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;"                                                     \
> >>>  		"load ${devtype} ${devnum}:${distro_bootpart} "           \
> >>>  			"${kernel_addr_r} efi/boot/"BOOTEFI_NAME"; "      \
> >>> -		"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"		  \
> >>>  	\
> >>>  	"load_efi_dtb="                                                   \
> >>>  		"load ${devtype} ${devnum}:${distro_bootpart} "           \
> >>> -			"${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};"			  \
> >>>  		"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_fdtfile\0"
> >>> +		"fi; "							  \
> >>> +		"setenv efi_fdt_addr\0"
> >>>  #define SCAN_DEV_FOR_EFI "run scan_dev_for_efi;"
> >>>  #else
> >>>  #define BOOTENV_SHARED_EFI
> >>>