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

[AKASHI Takahiro]

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 at 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/"                  \
                "for prefix in ${efi_dtb_prefixes}; do "                  \
                        "load ${devtype} ${devnum}:${distro_bootpart} "   \
                                "${fdt_addr_r} ${prefix}${efi_fdtfile};"  \
                        "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};"                        \
                "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
> >