| Message ID | 20181012050757.6925-1-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] efi_loader: rework fdt handling in distro boot script | expand |
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. > - "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). That way you don't need to check for the failure case in load_efi_dtb again. 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 >
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/" \ "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 > >
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. Also I think the way you wrote it right now fails to execute anyway, as it's missing a semicolon :). > "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. > "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. 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 ;). 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 >>>
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 > >>>
On 19.10.18 09:20, AKASHI Takahiro wrote: > 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; No, more like the way it was before, where it was a defined variable similar to boot_efi_binary ;). > >> 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? If anything, bootefi bootmgr would have to do the DTB search on its own based on the boot order devices it's looking at. The problem is that the DT is considered secure. So if we allow the DT to get overwritten by a random SD card that's plugged in, even though the boot order wouldn't have reached that SD card, we're opening a security hole. So the only thing I would consider ok to do is to load the DT from exactly the location the boot binary was loaded from. And only if it was loaded (and executed). >> 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." Well, see above :). Alex
On Fri, Oct 19, 2018 at 09:31:14AM +0200, Alexander Graf wrote: > > > On 19.10.18 09:20, AKASHI Takahiro wrote: > > 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; > > No, more like the way it was before, where it was a defined variable > similar to boot_efi_binary ;). OK, I will rewind my change. > > > >> 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? > > If anything, bootefi bootmgr would have to do the DTB search on its own > based on the boot order devices it's looking at. You didn't answer to my question :) I mean: "distro_bootcmd=" BOOTENV_SET_SCSI_NEED_INIT" <=(a) "for target in ${boot_targets}; do" "run bootcmd_${target};" "done" <=(b) If we still want to call "bootefi bootmgr" once in distro_bootcmd, should we put it at (a) or (b)? > The problem is that the DT is considered secure. So if we allow the DT > to get overwritten by a random SD card that's plugged in, even though > the boot order wouldn't have reached that SD card, we're opening a > security hole. > > So the only thing I would consider ok to do is to load the DT from > exactly the location the boot binary was loaded from. And only if it was > loaded (and executed). It seems to make sense only if verification is assumed to be done per boot device, not per file (signature). Your proposal would be still vulnerable. (Secure boot is yet another big issue.) But anyway, the current bootmgr, either u-boot or edk2, doesn't have such a mechanism, does it? -Takahiro Akashi > >> 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." > > Well, see above :). > > > Alex
On 19.10.18 10:32, AKASHI Takahiro wrote: > On Fri, Oct 19, 2018 at 09:31:14AM +0200, Alexander Graf wrote: >> >> >> On 19.10.18 09:20, AKASHI Takahiro wrote: >>> 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; >> >> No, more like the way it was before, where it was a defined variable >> similar to boot_efi_binary ;). > > OK, I will rewind my change. > >>> >>>> 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? >> >> If anything, bootefi bootmgr would have to do the DTB search on its own >> based on the boot order devices it's looking at. > > You didn't answer to my question :) > I mean: > "distro_bootcmd=" BOOTENV_SET_SCSI_NEED_INIT" > <=(a) > "for target in ${boot_targets}; do" > "run bootcmd_${target};" > "done" > <=(b) > If we still want to call "bootefi bootmgr" once in distro_bootcmd, > should we put it at (a) or (b)? The UEFI spec is pretty clear there, no? It says local boot order always takes precedence over removable boot order. So it has to be (a). > >> The problem is that the DT is considered secure. So if we allow the DT >> to get overwritten by a random SD card that's plugged in, even though >> the boot order wouldn't have reached that SD card, we're opening a >> security hole. >> >> So the only thing I would consider ok to do is to load the DT from >> exactly the location the boot binary was loaded from. And only if it was >> loaded (and executed). > > It seems to make sense only if verification is assumed to be done per > boot device, not per file (signature). Your proposal would be still vulnerable. > (Secure boot is yet another big issue.) > > But anyway, the current bootmgr, either u-boot or edk2, doesn't have > such a mechanism, does it? What mechanism exactly are you referring to? Edk2 doesn't allow for DT loading from a storage device usually. It considers the DT part of firmware. Alex
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 " \ - "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; " \ + "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
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(-)