* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script @ 2018-10-12 5:07 AKASHI Takahiro 2018-10-12 5:07 ` [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r AKASHI Takahiro 2018-10-16 13:15 ` [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script Alexander Graf 0 siblings, 2 replies; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-12 5:07 UTC (permalink / raw) To: u-boot 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 " \ - "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 -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-12 5:07 [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script AKASHI Takahiro @ 2018-10-12 5:07 ` AKASHI Takahiro 2018-10-15 1:01 ` Tuomas Tynkkynen 2018-10-16 13:15 ` [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script Alexander Graf 1 sibling, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-12 5:07 UTC (permalink / raw) To: u-boot This variable, fdt_addr_t, is missing in the current qemu-arm.h while it seems to be mandatory, at least, to run distro_bootcmd as expected. So just add its definition. A size of 1MB would be enough. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/configs/qemu-arm.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h index 91fb8d47edf8..0e66f946dde5 100644 --- a/include/configs/qemu-arm.h +++ b/include/configs/qemu-arm.h @@ -55,6 +55,7 @@ "fdt_high=0xffffffff\0" \ "initrd_high=0xffffffff\0" \ "fdt_addr=0x40000000\0" \ + "fdt_addr_r=0x40100000\0" \ "scriptaddr=0x40200000\0" \ "pxefile_addr_r=0x40300000\0" \ "kernel_addr_r=0x40400000\0" \ -- 2.19.0 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-12 5:07 ` [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r AKASHI Takahiro @ 2018-10-15 1:01 ` Tuomas Tynkkynen 2018-10-15 5:14 ` AKASHI Takahiro 0 siblings, 1 reply; 18+ messages in thread From: Tuomas Tynkkynen @ 2018-10-15 1:01 UTC (permalink / raw) To: u-boot Hi Takahiro, On Fri, 12 Oct 2018 14:07:57 +0900 AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > This variable, fdt_addr_t, is missing in the current qemu-arm.h while > it seems to be mandatory, at least, to run distro_bootcmd as expected. > So just add its definition. A size of 1MB would be enough. > In what way is this required for distro_bootcmd to work? At least in the past I've tested qemu_arm64_defconfig and EFI boot with the Fedora netinst image and it has worked fine in stock U-Boot. Note that these '-machine virt' based targets are slightly different from real hardware in the sense that instead of loading a .dtb file provided by the OS, the device tree is provided by QEMU. In the hunk below you can see "fdt_addr=0x40000000\0" providing the address of the QEMU-provided device tree which the distro scripts should be using. I guess in principle having ${fdt_addr_r} set as well shouldn't hurt and might be used for testing/unusual purposes. Glancing at cmd/pxe.c, there is a problem though, in that if ${fdt_addr_r} were defined, a PXE file using the FDTDIR directive would attempt loading a dtb file named "<NULL>-qemu-arm.dtb" instead of falling back to using ${fdt_addr}. That bug would need to be fixed first before applying this patch. > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > include/configs/qemu-arm.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > index 91fb8d47edf8..0e66f946dde5 100644 > --- a/include/configs/qemu-arm.h > +++ b/include/configs/qemu-arm.h > @@ -55,6 +55,7 @@ > "fdt_high=0xffffffff\0" \ > "initrd_high=0xffffffff\0" \ > "fdt_addr=0x40000000\0" \ > + "fdt_addr_r=0x40100000\0" \ > "scriptaddr=0x40200000\0" \ > "pxefile_addr_r=0x40300000\0" \ > "kernel_addr_r=0x40400000\0" \ ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-15 1:01 ` Tuomas Tynkkynen @ 2018-10-15 5:14 ` AKASHI Takahiro 2018-10-16 13:04 ` Alexander Graf 0 siblings, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-15 5:14 UTC (permalink / raw) To: u-boot On Mon, Oct 15, 2018 at 04:01:06AM +0300, Tuomas Tynkkynen wrote: > Hi Takahiro, > > On Fri, 12 Oct 2018 14:07:57 +0900 > AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > > This variable, fdt_addr_t, is missing in the current qemu-arm.h while > > it seems to be mandatory, at least, to run distro_bootcmd as expected. > > So just add its definition. A size of 1MB would be enough. > > > > In what way is this required for distro_bootcmd to work? At least in the > past I've tested qemu_arm64_defconfig and EFI boot with the Fedora > netinst image and it has worked fine in stock U-Boot. > > Note that these '-machine virt' based targets are slightly different > from real hardware in the sense that instead of loading a .dtb file > provided by the OS, the device tree is provided by QEMU. In the hunk > below you can see "fdt_addr=0x40000000\0" providing the address of > the QEMU-provided device tree which the distro scripts should be > using. Yep, I know that. I was not clear; what distro bootcmd, or more specifically scan_dev_for_efi, tries to do regarding fdt handling is that, if "fdtfile" variable is defined, it supersedes qemu's own dtb (that is what "fdt_addr" points to), but this trick doesn't work expectedly if "fdt_addr_r" variable is not defined. > I guess in principle having ${fdt_addr_r} set as well shouldn't hurt and > might be used for testing/unusual purposes. I don't know whether the case is "unsual" or not. But doc/README.distro cleary says that fdt_addr_r is *mandatory*. If u-boot works without it, it's a bug, otherwise we must correct the doc (or scan_dev_for_efi in the first place?). Thanks, -Takahiro Akashi > Glancing at cmd/pxe.c, > there is a problem though, in that if ${fdt_addr_r} were defined, a PXE > file using the FDTDIR directive would attempt loading a dtb file named > "<NULL>-qemu-arm.dtb" instead of falling back to using ${fdt_addr}. > That bug would need to be fixed first before applying this patch. > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > include/configs/qemu-arm.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/include/configs/qemu-arm.h b/include/configs/qemu-arm.h > > index 91fb8d47edf8..0e66f946dde5 100644 > > --- a/include/configs/qemu-arm.h > > +++ b/include/configs/qemu-arm.h > > @@ -55,6 +55,7 @@ > > "fdt_high=0xffffffff\0" \ > > "initrd_high=0xffffffff\0" \ > > "fdt_addr=0x40000000\0" \ > > + "fdt_addr_r=0x40100000\0" \ > > "scriptaddr=0x40200000\0" \ > > "pxefile_addr_r=0x40300000\0" \ > > "kernel_addr_r=0x40400000\0" \ > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-15 5:14 ` AKASHI Takahiro @ 2018-10-16 13:04 ` Alexander Graf 2018-10-17 22:25 ` Tuomas Tynkkynen 0 siblings, 1 reply; 18+ messages in thread From: Alexander Graf @ 2018-10-16 13:04 UTC (permalink / raw) To: u-boot On 15.10.18 07:14, AKASHI Takahiro wrote: > On Mon, Oct 15, 2018 at 04:01:06AM +0300, Tuomas Tynkkynen wrote: >> Hi Takahiro, >> >> On Fri, 12 Oct 2018 14:07:57 +0900 >> AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: >> >>> This variable, fdt_addr_t, is missing in the current qemu-arm.h while >>> it seems to be mandatory, at least, to run distro_bootcmd as expected. >>> So just add its definition. A size of 1MB would be enough. >>> >> >> In what way is this required for distro_bootcmd to work? At least in the >> past I've tested qemu_arm64_defconfig and EFI boot with the Fedora >> netinst image and it has worked fine in stock U-Boot. >> >> Note that these '-machine virt' based targets are slightly different >> from real hardware in the sense that instead of loading a .dtb file >> provided by the OS, the device tree is provided by QEMU. In the hunk >> below you can see "fdt_addr=0x40000000\0" providing the address of >> the QEMU-provided device tree which the distro scripts should be >> using. > > Yep, I know that. > I was not clear; what distro bootcmd, or more specifically scan_dev_for_efi, > tries to do regarding fdt handling is that, if "fdtfile" variable is defined, > it supersedes qemu's own dtb (that is what "fdt_addr" points to), but > this trick doesn't work expectedly if "fdt_addr_r" variable is not defined. > >> I guess in principle having ${fdt_addr_r} set as well shouldn't hurt and >> might be used for testing/unusual purposes. > > I don't know whether the case is "unsual" or not. > But doc/README.distro cleary says that fdt_addr_r is *mandatory*. > If u-boot works without it, it's a bug, otherwise we must correct > the doc (or scan_dev_for_efi in the first place?). I agree. Boards that support distro boot *must* provide fdt_addr_r. Otherwise distro scripts may fail unexpectedly because they assume the variable is present. > > Thanks, > -Takahiro Akashi > >> Glancing at cmd/pxe.c, >> there is a problem though, in that if ${fdt_addr_r} were defined, a PXE >> file using the FDTDIR directive would attempt loading a dtb file named >> "<NULL>-qemu-arm.dtb" instead of falling back to using ${fdt_addr}. >> That bug would need to be fixed first before applying this patch. Well, and that load will fail and everyone's happy, no? IMHO we should fall back to $fdtcontroladdr always, but the pxe boot path is not something I would endorse onto anyone (what you want as PXE really is called 'dhcp' in the efi_loader distro boot world) Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-16 13:04 ` Alexander Graf @ 2018-10-17 22:25 ` Tuomas Tynkkynen 2018-10-18 7:25 ` Alexander Graf 0 siblings, 1 reply; 18+ messages in thread From: Tuomas Tynkkynen @ 2018-10-17 22:25 UTC (permalink / raw) To: u-boot Hi Alexander, On Tue, 16 Oct 2018 15:04:26 +0200 Alexander Graf <agraf@suse.de> wrote: ... > > > >> Glancing at cmd/pxe.c, > >> there is a problem though, in that if ${fdt_addr_r} were defined, > >> a PXE file using the FDTDIR directive would attempt loading a dtb > >> file named "<NULL>-qemu-arm.dtb" instead of falling back to using > >> ${fdt_addr}. That bug would need to be fixed first before applying > >> this patch. > > Well, and that load will fail and everyone's happy, no? No, because currently if DTB loading from the filesystem/tftp is attempted and it fails, it aborts the boot. It doesn't matter if it's via a 'FDT' or 'FDTDIR' directive. In the case of typical hardware that's probably the desired behaviour. I guess the qemu_arm + FDTDIR case can be fixed by not attempting a .dtb load if the default fdtfile is not known due to $soc or $board being unset. At least I doubt that some other board could be relying on a filename containing literally "<NULL>" :) > IMHO we should > fall back to $fdtcontroladdr always FWIW, to me the idea of passing $fdtcontroladdr to the OS has always filled me with dread. On top of the usual backwards- and forwards-compatibility problems that happen when mixing and matching kernels and DTBs from different releases, you now have to deal with issues like U-Boot specific .dts that are majorly diverged from Linux ones, or where the .dts is otherwise from Linux but the U-Boot specific additions break it for Linux, or where the .dts used is wrong for the specific hardware revision but close enough for U-Boot's purposes, and so on... - Tuomas ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-17 22:25 ` Tuomas Tynkkynen @ 2018-10-18 7:25 ` Alexander Graf 2018-10-19 6:33 ` AKASHI Takahiro 2018-10-24 10:36 ` Tuomas Tynkkynen 0 siblings, 2 replies; 18+ messages in thread From: Alexander Graf @ 2018-10-18 7:25 UTC (permalink / raw) To: u-boot On 18.10.18 00:25, Tuomas Tynkkynen wrote: > Hi Alexander, > > On Tue, 16 Oct 2018 15:04:26 +0200 > Alexander Graf <agraf@suse.de> wrote: > > ... >>> >>>> Glancing at cmd/pxe.c, >>>> there is a problem though, in that if ${fdt_addr_r} were defined, >>>> a PXE file using the FDTDIR directive would attempt loading a dtb >>>> file named "<NULL>-qemu-arm.dtb" instead of falling back to using >>>> ${fdt_addr}. That bug would need to be fixed first before applying >>>> this patch. >> >> Well, and that load will fail and everyone's happy, no? > > No, because currently if DTB loading from the filesystem/tftp is > attempted and it fails, it aborts the boot. It doesn't matter if it's > via a 'FDT' or 'FDTDIR' directive. In the case of typical hardware > that's probably the desired behaviour. > > I guess the qemu_arm + FDTDIR case can be fixed by not attempting > a .dtb load if the default fdtfile is not known due to $soc or $board > being unset. At least I doubt that some other board could be relying > on a filename containing literally "<NULL>" :) Well - IIRC $soc and $board should also always be defined ;). So yet another thing to fix in the QEMU port. > >> IMHO we should >> fall back to $fdtcontroladdr always > > FWIW, to me the idea of passing $fdtcontroladdr to the OS has always > filled me with dread. On top of the usual backwards- and > forwards-compatibility problems that happen when mixing and matching > kernels and DTBs from different releases, you now have to deal with That's something that we seriously as a community need to get sorted out. We're pushing hard for it in the EBBR context. The fact that people are afraid of putting *hardware desciption* into their firmware is just mind boggling. > issues like U-Boot specific .dts that are majorly diverged from Linux > ones, or where the .dts is otherwise from Linux but the U-Boot specific These case should really be the minority. And if you see those, please fix them. > additions break it for Linux, or where the .dts used is wrong for the I've never seen a case where a U-Boot addition broke the DT for Linux. > specific hardware revision but close enough for U-Boot's purposes, > and so on... Again, something that just needs fixing. Device trees belong to the entity that knows hardware, not to the OS. Otherwise you lose the abstraction layer that DT gives you and you lose the ability to run "generic" kernels. And of course you break the ecosystem, because now good luck running BSD, your own little toy OS, etc ;) Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-18 7:25 ` Alexander Graf @ 2018-10-19 6:33 ` AKASHI Takahiro 2018-10-19 7:46 ` Alexander Graf 2018-10-24 10:36 ` Tuomas Tynkkynen 1 sibling, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-19 6:33 UTC (permalink / raw) To: u-boot On Thu, Oct 18, 2018 at 09:25:04AM +0200, Alexander Graf wrote: > > > On 18.10.18 00:25, Tuomas Tynkkynen wrote: > > Hi Alexander, > > > > On Tue, 16 Oct 2018 15:04:26 +0200 > > Alexander Graf <agraf@suse.de> wrote: > > > > ... > >>> > >>>> Glancing at cmd/pxe.c, > >>>> there is a problem though, in that if ${fdt_addr_r} were defined, > >>>> a PXE file using the FDTDIR directive would attempt loading a dtb > >>>> file named "<NULL>-qemu-arm.dtb" instead of falling back to using > >>>> ${fdt_addr}. That bug would need to be fixed first before applying > >>>> this patch. > >> > >> Well, and that load will fail and everyone's happy, no? > > > > No, because currently if DTB loading from the filesystem/tftp is > > attempted and it fails, it aborts the boot. It doesn't matter if it's > > via a 'FDT' or 'FDTDIR' directive. In the case of typical hardware > > that's probably the desired behaviour. > > > > I guess the qemu_arm + FDTDIR case can be fixed by not attempting > > a .dtb load if the default fdtfile is not known due to $soc or $board > > being unset. At least I doubt that some other board could be relying > > on a filename containing literally "<NULL>" :) > > Well - IIRC $soc and $board should also always be defined ;). So yet > another thing to fix in the QEMU port. See my patch below. Are you happy with it? (qemu-qemu-arm.dtb doesn't make sense to me, though :) > > > >> IMHO we should > >> fall back to $fdtcontroladdr always > > > > FWIW, to me the idea of passing $fdtcontroladdr to the OS has always > > filled me with dread. On top of the usual backwards- and > > forwards-compatibility problems that happen when mixing and matching > > kernels and DTBs from different releases, you now have to deal with > > That's something that we seriously as a community need to get sorted > out. We're pushing hard for it in the EBBR context. The fact that people > are afraid of putting *hardware desciption* into their firmware is just > mind boggling. I don't have a strong opinion, but it seems to me that 'fall-back' approach is quite normal. If it doesn't work on a specific board, 'fdt_addr' should be defined. Thanks, -Takahiro Akashi > > issues like U-Boot specific .dts that are majorly diverged from Linux > > ones, or where the .dts is otherwise from Linux but the U-Boot specific > > These case should really be the minority. And if you see those, please > fix them. > > > additions break it for Linux, or where the .dts used is wrong for the > > I've never seen a case where a U-Boot addition broke the DT for Linux. > > > specific hardware revision but close enough for U-Boot's purposes, > > and so on... > > Again, something that just needs fixing. Device trees belong to the > entity that knows hardware, not to the OS. Otherwise you lose the > abstraction layer that DT gives you and you lose the ability to run > "generic" kernels. And of course you break the ecosystem, because now > good luck running BSD, your own little toy OS, etc ;) > > > Alex ===8<=== ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-19 6:33 ` AKASHI Takahiro @ 2018-10-19 7:46 ` Alexander Graf 2018-10-19 8:17 ` AKASHI Takahiro 0 siblings, 1 reply; 18+ messages in thread From: Alexander Graf @ 2018-10-19 7:46 UTC (permalink / raw) To: u-boot On 19.10.18 08:33, AKASHI Takahiro wrote: > On Thu, Oct 18, 2018 at 09:25:04AM +0200, Alexander Graf wrote: >> >> >> On 18.10.18 00:25, Tuomas Tynkkynen wrote: >>> Hi Alexander, >>> >>> On Tue, 16 Oct 2018 15:04:26 +0200 >>> Alexander Graf <agraf@suse.de> wrote: >>> >>> ... >>>>> >>>>>> Glancing at cmd/pxe.c, >>>>>> there is a problem though, in that if ${fdt_addr_r} were defined, >>>>>> a PXE file using the FDTDIR directive would attempt loading a dtb >>>>>> file named "<NULL>-qemu-arm.dtb" instead of falling back to using >>>>>> ${fdt_addr}. That bug would need to be fixed first before applying >>>>>> this patch. >>>> >>>> Well, and that load will fail and everyone's happy, no? >>> >>> No, because currently if DTB loading from the filesystem/tftp is >>> attempted and it fails, it aborts the boot. It doesn't matter if it's >>> via a 'FDT' or 'FDTDIR' directive. In the case of typical hardware >>> that's probably the desired behaviour. >>> >>> I guess the qemu_arm + FDTDIR case can be fixed by not attempting >>> a .dtb load if the default fdtfile is not known due to $soc or $board >>> being unset. At least I doubt that some other board could be relying >>> on a filename containing literally "<NULL>" :) >> >> Well - IIRC $soc and $board should also always be defined ;). So yet >> another thing to fix in the QEMU port. > > See my patch below. Are you happy with it? > (qemu-qemu-arm.dtb doesn't make sense to me, though :) > >>> >>>> IMHO we should >>>> fall back to $fdtcontroladdr always >>> >>> FWIW, to me the idea of passing $fdtcontroladdr to the OS has always >>> filled me with dread. On top of the usual backwards- and >>> forwards-compatibility problems that happen when mixing and matching >>> kernels and DTBs from different releases, you now have to deal with >> >> That's something that we seriously as a community need to get sorted >> out. We're pushing hard for it in the EBBR context. The fact that people >> are afraid of putting *hardware desciption* into their firmware is just >> mind boggling. > > I don't have a strong opinion, but it seems to me that 'fall-back' approach > is quite normal. If it doesn't work on a specific board, 'fdt_addr' > should be defined. > > Thanks, > -Takahiro Akashi > > >>> issues like U-Boot specific .dts that are majorly diverged from Linux >>> ones, or where the .dts is otherwise from Linux but the U-Boot specific >> >> These case should really be the minority. And if you see those, please >> fix them. >> >>> additions break it for Linux, or where the .dts used is wrong for the >> >> I've never seen a case where a U-Boot addition broke the DT for Linux. >> >>> specific hardware revision but close enough for U-Boot's purposes, >>> and so on... >> >> Again, something that just needs fixing. Device trees belong to the >> entity that knows hardware, not to the OS. Otherwise you lose the >> abstraction layer that DT gives you and you lose the ability to run >> "generic" kernels. And of course you break the ecosystem, because now >> good luck running BSD, your own little toy OS, etc ;) >> >> >> Alex > > ===8<=== > From 47b26a86359a3b612e890f2ca2d5d20092f9f4e1 Mon Sep 17 00:00:00 2001 > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > Date: Fri, 19 Oct 2018 15:22:02 +0900 > Subject: [PATCH] arm: qemu: rework Kconfig > > Define SYS_SOC and move SYS_* to a canonical place (under board). > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> Looks good enough to me, but it's up to Tuomas to double check and take as he's the qemu-arm maintainer :). It also usually helps to post the patch as a new message, not buried inside a thread ;). Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-19 7:46 ` Alexander Graf @ 2018-10-19 8:17 ` AKASHI Takahiro 0 siblings, 0 replies; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-19 8:17 UTC (permalink / raw) To: u-boot On Fri, Oct 19, 2018 at 09:46:51AM +0200, Alexander Graf wrote: > > > On 19.10.18 08:33, AKASHI Takahiro wrote: > > On Thu, Oct 18, 2018 at 09:25:04AM +0200, Alexander Graf wrote: > >> > >> > >> On 18.10.18 00:25, Tuomas Tynkkynen wrote: > >>> Hi Alexander, > >>> > >>> On Tue, 16 Oct 2018 15:04:26 +0200 > >>> Alexander Graf <agraf@suse.de> wrote: > >>> > >>> ... > >>>>> > >>>>>> Glancing at cmd/pxe.c, > >>>>>> there is a problem though, in that if ${fdt_addr_r} were defined, > >>>>>> a PXE file using the FDTDIR directive would attempt loading a dtb > >>>>>> file named "<NULL>-qemu-arm.dtb" instead of falling back to using > >>>>>> ${fdt_addr}. That bug would need to be fixed first before applying > >>>>>> this patch. > >>>> > >>>> Well, and that load will fail and everyone's happy, no? > >>> > >>> No, because currently if DTB loading from the filesystem/tftp is > >>> attempted and it fails, it aborts the boot. It doesn't matter if it's > >>> via a 'FDT' or 'FDTDIR' directive. In the case of typical hardware > >>> that's probably the desired behaviour. > >>> > >>> I guess the qemu_arm + FDTDIR case can be fixed by not attempting > >>> a .dtb load if the default fdtfile is not known due to $soc or $board > >>> being unset. At least I doubt that some other board could be relying > >>> on a filename containing literally "<NULL>" :) > >> > >> Well - IIRC $soc and $board should also always be defined ;). So yet > >> another thing to fix in the QEMU port. > > > > See my patch below. Are you happy with it? > > (qemu-qemu-arm.dtb doesn't make sense to me, though :) > > > >>> > >>>> IMHO we should > >>>> fall back to $fdtcontroladdr always > >>> > >>> FWIW, to me the idea of passing $fdtcontroladdr to the OS has always > >>> filled me with dread. On top of the usual backwards- and > >>> forwards-compatibility problems that happen when mixing and matching > >>> kernels and DTBs from different releases, you now have to deal with > >> > >> That's something that we seriously as a community need to get sorted > >> out. We're pushing hard for it in the EBBR context. The fact that people > >> are afraid of putting *hardware desciption* into their firmware is just > >> mind boggling. > > > > I don't have a strong opinion, but it seems to me that 'fall-back' approach > > is quite normal. If it doesn't work on a specific board, 'fdt_addr' > > should be defined. > > > > Thanks, > > -Takahiro Akashi > > > > > >>> issues like U-Boot specific .dts that are majorly diverged from Linux > >>> ones, or where the .dts is otherwise from Linux but the U-Boot specific > >> > >> These case should really be the minority. And if you see those, please > >> fix them. > >> > >>> additions break it for Linux, or where the .dts used is wrong for the > >> > >> I've never seen a case where a U-Boot addition broke the DT for Linux. > >> > >>> specific hardware revision but close enough for U-Boot's purposes, > >>> and so on... > >> > >> Again, something that just needs fixing. Device trees belong to the > >> entity that knows hardware, not to the OS. Otherwise you lose the > >> abstraction layer that DT gives you and you lose the ability to run > >> "generic" kernels. And of course you break the ecosystem, because now > >> good luck running BSD, your own little toy OS, etc ;) > >> > >> > >> Alex > > > > ===8<=== > > From 47b26a86359a3b612e890f2ca2d5d20092f9f4e1 Mon Sep 17 00:00:00 2001 > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > Date: Fri, 19 Oct 2018 15:22:02 +0900 > > Subject: [PATCH] arm: qemu: rework Kconfig > > > > Define SYS_SOC and move SYS_* to a canonical place (under board). > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > Looks good enough to me, but it's up to Tuomas to double check and take > as he's the qemu-arm maintainer :). > > It also usually helps to post the patch as a new message, not buried > inside a thread ;). OK. Since I found a small bug, I will send out a new patch separately. -Takahiro Akashi > > Alex ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r 2018-10-18 7:25 ` Alexander Graf 2018-10-19 6:33 ` AKASHI Takahiro @ 2018-10-24 10:36 ` Tuomas Tynkkynen 1 sibling, 0 replies; 18+ messages in thread From: Tuomas Tynkkynen @ 2018-10-24 10:36 UTC (permalink / raw) To: u-boot On Thu, 18 Oct 2018 09:25:04 +0200 Alexander Graf <agraf@suse.de> wrote: > On 18.10.18 00:25, Tuomas Tynkkynen wrote: > > Hi Alexander, > > > > On Tue, 16 Oct 2018 15:04:26 +0200 > > Alexander Graf <agraf@suse.de> wrote: > > > > ... > >>> > >>>> Glancing at cmd/pxe.c, > >>>> there is a problem though, in that if ${fdt_addr_r} were defined, > >>>> a PXE file using the FDTDIR directive would attempt loading a dtb > >>>> file named "<NULL>-qemu-arm.dtb" instead of falling back to using > >>>> ${fdt_addr}. That bug would need to be fixed first before > >>>> applying this patch. > >> > >> Well, and that load will fail and everyone's happy, no? > > > > No, because currently if DTB loading from the filesystem/tftp is > > attempted and it fails, it aborts the boot. It doesn't matter if > > it's via a 'FDT' or 'FDTDIR' directive. In the case of typical > > hardware that's probably the desired behaviour. > > > > I guess the qemu_arm + FDTDIR case can be fixed by not attempting > > a .dtb load if the default fdtfile is not known due to $soc or > > $board being unset. At least I doubt that some other board could be > > relying on a filename containing literally "<NULL>" :) > > Well - IIRC $soc and $board should also always be defined ;). So yet > another thing to fix in the QEMU port. As far as I know $soc is only used for computing the default $fdtfile, but we explicitly don't want to compute one for qemu_arm. > > > >> IMHO we should > >> fall back to $fdtcontroladdr always > > > > FWIW, to me the idea of passing $fdtcontroladdr to the OS has always > > filled me with dread. On top of the usual backwards- and > > forwards-compatibility problems that happen when mixing and matching > > kernels and DTBs from different releases, you now have to deal > > with > > That's something that we seriously as a community need to get sorted > out. We're pushing hard for it in the EBBR context. The fact that > people are afraid of putting *hardware desciption* into their > firmware is just mind boggling. But until/if that is solved anyone trying to rely on $fdtcontroladdr will get burned, or at least that's what the experience for sunxi sounds like where stuff like adding new regulators to DT breaks old kernels that lack the regulator driver. (And where the proposed solution to that was some hack that just uses fixed regulators instead, throwing the hardware description aspect out the window). And it doesn't look like an isolated situation, you'd have got equally hosed e.g when RPi switched to using the sdhost IP in place of the other MMC controller, or when Tegra landed USB3 driver support thus switched the .dts to using the XUSB controllers over the USB2 ones. > > issues like U-Boot specific .dts that are majorly diverged from > > Linux ones, or where the .dts is otherwise from Linux but the > > U-Boot specific > > These case should really be the minority. And if you see those, please > fix them. Well, for the case of Tegra124, being able to use a recent unmodified .dts from Linux for U-Boot would require writing an USB3 driver in order to not break USB functionality. That's not exactly a simple fix. > > additions break it for Linux, or where the .dts used is wrong for > > the > > I've never seen a case where a U-Boot addition broke the DT for Linux. > See e.g. 96a82d33f8c5bd3e90098b540349b7b9e43e27da fixing such instances. > > specific hardware revision but close enough for U-Boot's purposes, > > and so on... > > Again, something that just needs fixing. Device trees belong to the > entity that knows hardware, not to the OS. Otherwise you lose the > abstraction layer that DT gives you and you lose the ability to run > "generic" kernels. And of course you break the ecosystem, because now > good luck running BSD, your own little toy OS, etc ;) > It's been possible to make generic images using the U-Boot distro stuff (where U-Boot knows just the .dtb filename to load from the ones installed by kernel's `make dtbs_install`) for over three years now. Sure it doesn't fulfill the "DT doesn't belong to the OS" goal... but given that features (incl. DT bindings) get developed in vendor kernels first but upstream won't typically accept then unchanged, having a single .dtb that satisfies both has been a logical impossibility so far. - Tuomas ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-12 5:07 [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script AKASHI Takahiro 2018-10-12 5:07 ` [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r AKASHI Takahiro @ 2018-10-16 13:15 ` Alexander Graf 2018-10-18 2:07 ` AKASHI Takahiro 1 sibling, 1 reply; 18+ messages in thread From: Alexander Graf @ 2018-10-16 13:15 UTC (permalink / raw) To: u-boot 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 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-16 13:15 ` [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script Alexander Graf @ 2018-10-18 2:07 ` AKASHI Takahiro 2018-10-18 7:31 ` Alexander Graf 0 siblings, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-18 2:07 UTC (permalink / raw) To: u-boot 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 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-18 2:07 ` AKASHI Takahiro @ 2018-10-18 7:31 ` Alexander Graf 2018-10-19 7:20 ` AKASHI Takahiro 0 siblings, 1 reply; 18+ messages in thread From: Alexander Graf @ 2018-10-18 7:31 UTC (permalink / raw) To: u-boot 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 >>> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-18 7:31 ` Alexander Graf @ 2018-10-19 7:20 ` AKASHI Takahiro 2018-10-19 7:31 ` Alexander Graf 0 siblings, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-19 7:20 UTC (permalink / raw) To: u-boot 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 > >>> ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-19 7:20 ` AKASHI Takahiro @ 2018-10-19 7:31 ` Alexander Graf 2018-10-19 8:32 ` AKASHI Takahiro 0 siblings, 1 reply; 18+ messages in thread From: Alexander Graf @ 2018-10-19 7:31 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-19 7:31 ` Alexander Graf @ 2018-10-19 8:32 ` AKASHI Takahiro 2018-10-19 8:50 ` Alexander Graf 0 siblings, 1 reply; 18+ messages in thread From: AKASHI Takahiro @ 2018-10-19 8:32 UTC (permalink / raw) To: u-boot 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@(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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script 2018-10-19 8:32 ` AKASHI Takahiro @ 2018-10-19 8:50 ` Alexander Graf 0 siblings, 0 replies; 18+ messages in thread From: Alexander Graf @ 2018-10-19 8:50 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-10-24 10:36 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-12 5:07 [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script AKASHI Takahiro 2018-10-12 5:07 ` [U-Boot] [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r AKASHI Takahiro 2018-10-15 1:01 ` Tuomas Tynkkynen 2018-10-15 5:14 ` AKASHI Takahiro 2018-10-16 13:04 ` Alexander Graf 2018-10-17 22:25 ` Tuomas Tynkkynen 2018-10-18 7:25 ` Alexander Graf 2018-10-19 6:33 ` AKASHI Takahiro 2018-10-19 7:46 ` Alexander Graf 2018-10-19 8:17 ` AKASHI Takahiro 2018-10-24 10:36 ` Tuomas Tynkkynen 2018-10-16 13:15 ` [U-Boot] [PATCH 1/2] efi_loader: rework fdt handling in distro boot script Alexander Graf 2018-10-18 2:07 ` AKASHI Takahiro 2018-10-18 7:31 ` Alexander Graf 2018-10-19 7:20 ` AKASHI Takahiro 2018-10-19 7:31 ` Alexander Graf 2018-10-19 8:32 ` AKASHI Takahiro 2018-10-19 8:50 ` Alexander Graf
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.