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

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

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

AKASHI Takahiro
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 <[hidden email]>
---
 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

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

AKASHI Takahiro
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 <[hidden email]>
---
 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

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

Tuomas Tynkkynen-2
Hi Takahiro,

On Fri, 12 Oct 2018 14:07:57 +0900
AKASHI Takahiro <[hidden email]> 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 <[hidden email]>
> ---
>  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" \

_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

AKASHI Takahiro
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 <[hidden email]> 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 <[hidden email]>
> > ---
> >  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" \
>
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

Alexander Graf


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 <[hidden email]> 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] efi_loader: rework fdt handling in distro boot script

Alexander Graf
In reply to this post by AKASHI Takahiro


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 <[hidden email]>
> ---
>  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
>
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

Tuomas Tynkkynen-2
In reply to this post by Alexander Graf
Hi Alexander,

On Tue, 16 Oct 2018 15:04:26 +0200
Alexander Graf <[hidden email]> 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] efi_loader: rework fdt handling in distro boot script

AKASHI Takahiro
In reply to this post by Alexander Graf
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 <[hidden email]>
> > ---
> >  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
> >
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/2] ARM: qemu-arm: define fdt_addr_r

Alexander Graf
In reply to this post by Tuomas Tynkkynen-2


On 18.10.18 00:25, Tuomas Tynkkynen wrote:

> Hi Alexander,
>
> On Tue, 16 Oct 2018 15:04:26 +0200
> Alexander Graf <[hidden email]> 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
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/2] efi_loader: rework fdt handling in distro boot script

Alexander Graf
In reply to this post by AKASHI Takahiro


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 <[hidden email]>
>>> ---
>>>  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
>>>
_______________________________________________
U-Boot mailing list
[hidden email]
https://lists.denx.de/listinfo/u-boot