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

[Alexander Graf]

> Am 03.12.2018 um 03:22 schrieb AKASHI Takahiro <takahiro.akashi at linaro.org>:
> 
>> On Sun, Dec 02, 2018 at 11:58:45PM +0100, Alexander Graf wrote:
>> 
>> 
>>> On 27.11.18 01:33, AKASHI Takahiro wrote:
>>> The current scenario for default UEFI booting, scan_dev_for_efi, has
>>> several issues:
>>> * load dtb dynamically even if its loacation (device) is not the same
>>>  as BOOTEFI_NAME binary's, (reported by Alex)
>>> * invoke 'bootmgr' only if BOOTEFI_NAME binary does exit even though
>>>  'bootmgr' can and should work independently whether or not the binary
>>>  exist,
>>> * in addition, invoke 'bootmgr' with dynamically-loaded dtb.
>>>  This behavior is not expected. (reported by Alex)
>>> * always assume that a 'fdtfile' variable is defined,
>>>  ("test -e ${devtype} ${devnum}:${distro_bootpart} "${prefix}${efi_fdtfile}"
>>>  always returns true even if fdtfile is NULL with prefix=="/".)
>>> * redundantly check for 'fdt_addr_r' in boot_efi_binary
>>> 
>>> In this patch, all the issues above are sorted out.
>>> Please note that the default behavior can be customized with:
>>>    fdtfile: a dtb file name
>>>    efi_dtb_prefixes: a list of paths for searching for a dtb file
>>> 
>>> (this feature does work even without this patch.)
>>> 
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
>>> ---
>>> include/config_distro_bootcmd.h | 38 +++++++++++++++++----------------
>>> 1 file changed, 20 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>> index 373fee78a999..256698309eb9 100644
>>> --- a/include/config_distro_bootcmd.h
>>> +++ b/include/config_distro_bootcmd.h
>>> @@ -115,7 +115,7 @@
>>>  */
>>> #define BOOTENV_EFI_SET_FDTFILE_FALLBACK                                  \
>>>    "if test -z \"${fdtfile}\" -a -n \"${soc}\"; then "               \
>>> -      "setenv efi_fdtfile ${soc}-${board}${boardver}.dtb; "           \
>>> +      "efi_fdtfile=${soc}-${board}${boardver}.dtb; "           \
>>>    "fi; "
>>> #else
>>> #define BOOTENV_EFI_SET_FDTFILE_FALLBACK
>>> @@ -124,26 +124,20 @@
>>> 
>>> #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"         \
>>> +        "load ${devtype} ${devnum}:${distro_bootpart} "          \
>>> +            "${fdt_addr_r} ${prefix}${efi_fdtfile}; "      \
>>> +        "if fdt addr ${fdt_addr_r}; then "              \
>>> +            "efi_fdt_addr=${fdt_addr_r}; "              \
>>> +        "fi;\0"                              \
>>>    \
>>>    "efi_dtb_prefixes=/ /dtb/ /dtb/current/\0"                        \
>>> -    "scan_dev_for_efi="                                               \
>>> -        "setenv efi_fdtfile ${fdtfile}; "                         \
>>> +    "set_efi_fdt_addr="                          \
>>> +        "efi_fdtfile=${fdtfile}; "                         \
>>>        BOOTENV_EFI_SET_FDTFILE_FALLBACK                          \
>>>        "for prefix in ${efi_dtb_prefixes}; do "                  \
>>>            "if test -e ${devtype} "                          \
>>> @@ -151,19 +145,26 @@
>>>                    "${prefix}${efi_fdtfile}; then "  \
>>>                "run load_efi_dtb; "                      \
>>>            "fi;"                                             \
>>> -        "done;"                                                   \
>>> +        "done;\0"                                                   \
>>> +    \
>>> +    "scan_dev_for_efi="                                               \
>>>        "if test -e ${devtype} ${devnum}:${distro_bootpart} "     \
>>>                    "efi/boot/"BOOTEFI_NAME"; then "  \
>>>                "echo Found EFI removable media binary "  \
>>>                    "efi/boot/"BOOTEFI_NAME"; "       \
>>> +                "efi_fdt_addr=${fdtcontroladdr}; "      \
>>> +                "if test -n \"${fdtfile}\"; then "      \
>>> +                    "run set_efi_fdt_addr; "      \
>>> +                "fi; "                      \
>>>                "run boot_efi_binary; "                   \
>>>                "echo EFI LOAD FAILED: continuing...; "   \
>>> -        "fi; "                                                    \
>>> -        "setenv efi_fdtfile\0"
>> 
>> This drops the bit that unsets the variable. So after the script, your
>> env will be different from before the script.
> 
> Yeah, but
> 
>> Please run
>> 
>>  U-Boot# printenv
>>  U-Boot# boot (w/o any efi payload to boot)
>>  U-Boot# printenv
>> 
>> Then diff the two environments. They should be identical.
> 
> Are you sure? When I try this on unmodified v2018.11,
> I also see some difference:
> 
> 18a19
>> devnum=0
> 37a39
>> scsi_need_init=false
> 44c46
> < Environment size: 3614/65532 bytes
> ---
>> Environment size: 3644/65532 bytes
> 
> It seems that 'devnum' and 'scsi_need_init' come from 'bootcmd_scsi0.'
> Can you clarify your point a bit more, please?

That is a bug :). Thanks for catching it? Can you please send a patch to fix it?

The problem is that a saveenv after a fsiled boot attempt should not contain intermediate variables from that attempt.


Alex

> 
> Thanks,
> -Takahiro Akashi
> 
>> 
>> 
>> Alex