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.aka...@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
> >>>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
