[U-Boot] [BUG] U-Boot hangs on fatload, commit ee88eacbdd840199a3dec707234579fb15ddd46a
Heinrich Schuchardt
xypron.glpk at gmx.de
Tue Nov 12 19:50:24 UTC 2019
On 11/11/19 6:14 PM, Gray Remlin wrote:
>
> This content is getting very convoluted, if appropriate feel free to
> crop it.
> New point raised at very bottom.
>
> On Sun, 10 Nov 2019 at 08:41, Heinrich Schuchardt <xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de>> wrote:
>
> On 11/9/19 10:31 PM, Gray Remlin wrote:
> > On Sat, 9 Nov 2019 at 20:50, Heinrich Schuchardt
> <xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> > <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>> wrote:
> >
> > On 11/9/19 8:42 PM, Gray Remlin wrote:
> > > On Sat, 9 Nov 2019 at 18:40, Heinrich Schuchardt
> > <xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>
> > > <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>>> wrote:
> > >
> > > On 11/9/19 6:08 PM, Heinrich Schuchardt wrote:
> > > > On 11/9/19 4:11 PM, Gray Remlin wrote:
> > > >> On Fri, 8 Nov 2019 at 20:08, Heinrich Schuchardt
> > > <xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>
> > <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>>
> > > >> <mailto:xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de> <mailto:xypron.glpk at gmx.de
> <mailto:xypron.glpk at gmx.de>>
> > <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>
> <mailto:xypron.glpk at gmx.de <mailto:xypron.glpk at gmx.de>>>>> wrote:
> > > >>
> > > >> On 11/8/19 7:32 PM, Gray Remlin wrote:
> > > >> > Please excuse the noise. I would like to file a
> > bug report
> > > >> against the
> > > >> > above commit, a quick search of www.denx.de
> <http://www.denx.de>
> > <http://www.denx.de>
> > > <http://www.denx.de> <http://www.denx.de>
> > > >> <http://www.denx.de> did not
> > > >> > reveal how I should proceed. Please point me in
> > the right
> > > >> direction.
> > > >> >
> > > >> >
> > > >> > Issue:
> > > >> > U-Boot hangs (i.e. during boot) whenever
> the command
> > > 'fatload' is
> > > >> used.
> > > >> >
> > > >> > Details:
> > > >> > U-Boot 2019.10 compiled with either
> > dreamplug_defconfig or
> > > >> > guruplug_defconfig.
> > > >> >
> > > >> > After the commit do_load() now additionally
> calls
> > > >> efi_set_bootdev()
> > > >> > which was moved out of do_load_wrapper()
> which is
> > only called
> > > >> by the
> > > >> > 'load' command.
> > > >> >
> > > >> > Reverting the commit fixes this issue for me.
> > > >> >
> > > >>
> > > >> Dear Gray,
> > > >>
> > > >> thanks for reporting the issue with commit
> > > >> fs: do_load: pass device path for efi payload
> > > >> ee88eacbdd840199a3dec707234579fb15ddd46a
> > > >>
> > > >> Is it only the fatload command that fails on your
> > device or
> > > also the
> > > >> load command?
> > > >>
> > > >> There is no bug tracker for U-Boot. So sending
> a mail
> > to the
> > > U-Boot
> > > >> mailing list, the patch author, and the
> maintainer is the
> > > best way to
> > > >> inform the developers about bugs.
> > > >>
> > > >> Best regards
> > > >>
> > > >> Heinrich
> > > >>
> > > >>
>
>
> Distribution and version of GCC:
>
> > > >> Additional information:
> > > >> cross-compiler
> > gcc-linaro-7.4.1-2019.02-x86_64_arm-linux-gnueabi
> > > >>
> > > >> The U-Boot environment being used is the default
> obtained by
> > > >> compiling U-Boot 2020.01-rc1-00100-gee93ef0c4b as
> > > dreamplug_defconfig
> > > >>
> > > >> => printenv
> > > >> baudrate=115200
> > > >> bootcmd=setenv ethact egiga0; ${x_bootcmd_ethernet};
> > setenv ethact
> > > >> egiga1; ${x_bootcmd_ethernet}; ${x_bootcmd_usb};
> > > ${x_bootcmd_kernel};
> > > >> setenv bootargs ${x_bootargs} ${x_bootargs_root};
> bootm
> > 0x6400000;
> > > >> bootdelay=3
> > > >> ethact=egiga0
> > > >> fdtcontroladdr=1fb8e7c8
> > > >> stderr=serial
> > > >> stdin=serial
> > > >> stdout=serial
> > > >> x_bootargs=console=ttyS0,115200
> > > >> x_bootargs_root=root=/dev/sda2 rootdelay=10
> > > >> x_bootcmd_ethernet=ping 192.168.2.1
> > > >> x_bootcmd_kernel=fatload usb 0 0x6400000 uImage
> > > >> x_bootcmd_usb=usb start
> > > >>
> > > >> U-Boot hangs for other syntactically correct
> invocations
> > of either
> > > >> 'fatload' or 'load'
> > > >> Other commands such as 'fatls' function as expected.
> > > >>
> > > >> Program flow is as follows:
> > > >>
> > > >> command 'fatload' (or 'load')
> > > >> efi_set_bootdev()
> > > >> ...
> > > >> efi_dp_split_file_path()
> > > >> ...
> > > >> efi_dp_dup()
> > > >> ....
> > > >> efi_dp_size()
> > > >> *while exit condition
> > never met*
> > > >> *infinite
> loop*
> > > >>
> > > >>
> > > >> This is not an attempted EFI boot, why is EFI code
> being
> > invoked ?
> > > >
> > > > Thanks for debugging.
> > > >
> > > > When booting from EFI we need to know from which device
> > the EFI
> > > binary
> > > > was loaded. We use this information to install the
> loaded
> > image
> > > > protocol. At the time of the load command we do no
> know if
> > you will
> > > > invoke bootz or bootefi.
> > >
> > >
> > > Isn't that the purpose of the 'load' command ?
> > >
> > > >
> > > > It might be that we have a problem with creating device
> > paths for
> > > USB. I
> > > > will try to reproduce this.
> > > >
> > > > You could add
> > > >
> > > > printf("efi_dp_split_file_path(%pD)\n", full_path);
> > > >
> > > > at the beginning of efi_dp_split_file_path() to
> identify
> > what device
> > > > path is passed to the function. This should produce an
> > output like
> > > >
> > > > => load scsi 0:2 $kernel_addr_r description.txt
> > > >
> > >
> >
> efi_dp_split_file_path(/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/HD(2,MBR,0x6fe3a999,0x400,0x400)/description.txt)
> > > >
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > >
> > > I just tested on an OrangePi PC with v2019.10 and got
> this:
> > >
> > > => fatload usb 0:1 $kernel_addr_r test.txt
> > >
> >
> efi_dp_split_file_path(/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0xfae8c6af,0x800,0x3b9f800)/test.txt)
> > > device path =
> > >
> >
> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0xfae8c6af,0x800,0x3b9f800)
> > > file path = /test.txt
> > > 12 bytes read in 26 ms (0 Bytes/s)
> > > => md.b $kernel_addr_r 0c
> > > 42000000: 4a 75 73 74 20 61 20 74 65 73 74 0a Just a
> test.
> > >
> > > So debugging on your specific device is needed.
> > >
> > >
> > > Why do you want to debug EFI code on a device that does not
> > support EFI ?
> > > I am not reporting a bug with EFI, the issue is 'fatload'
> is now
> > broken
> > > by this commit.
> > > Once 'fatload' is fixed I am willing to test U-Boot as
> required for
> > > other bugs whilst the
> > > dreamplug platform is available to me for such.
> >
> > Your system is compiled with EFI_LOADER. So you could be
> using fatload
> > to load an EFI file. do_fatload() is the only place where we
> can get the
> > device from which you load the file.
> >
> >
> > No, that is (was before this commit) the purpose of 'load'.
> > I ask again, why do you want two commands that perform exactly
> the same
> > action ?
> > Is it the intention to first unify them and then discard one ?
>
> You are right that the commands ext2ls, ext2load, ext4ls, ext4load,
> ext4save, ext4size, fatls, fatload, fatsave, and fatsize are rather
> superfluous in the light of ls, load, save, and size. They are just kept
> for backward compatibility.
>
> >
> >
> >
> > Could you, please, change the end of efi_dp_from_file() to
> >
> > printf("fpsize = %u\n", fpsize);
> > printf("dpsize = %u\n", dpsize);
> > size_t i;
> > for (i = 0; i < dpsize + sizeof(END); ++i)
> > printf("0x%02x ", ((char *)start)[i]):;
> > printf("\n");
> >
> > return start;
> >
> > and provide the output.
> >
> > On my system the output is
> >
> > => fatload usb 0 $kernel_addr_r uImage
> > fpsize = 18
> > dpsize = 102
> > 0x01 0x04 0x14 0x00 0xb9 0x73 0x1d 0xe6 0x84 0xa3 0xcc 0x4a
> 0xae 0xab
> > 0x82 0xe8 0x28 0xf3 0x62 0x8b 0x03 0x0f 0x0b 0x00 0x00 0x00
> 0x00 0x00
> > 0x09 0x00 0x01 0x03 0x0f 0x0b 0x00 0x81 0x07 0x71 0x55 0x00
> 0x00 0x00
> > 0x04 0x01 0x2a 0x00 0x01 0x00 0x00 0x00 0x00 0x08 0x00 0x00
> 0x00 0x00
> > 0x00 0x00 0x00 0x18 0x00 0x00 0x00 0x00 0x00 0x00 0xc3 0x43
> 0x04 0xa5
> > 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> 0x01 0x01
> > 0x04 0x04 0x12 0x00 0x75 0x00 0x49 0x00 0x6d 0x00 0x61 0x00
> 0x67 0x00
> > 0x65 0x00 0x00 0x00 0x7f 0xff 0x04 0x00
> >
> efi_dp_split_file_path('/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0xa50443c3,0x800,0x1800)/uImage')
> > 20 bytes read in 2 ms (9.8 KiB/s)
> > =>
> >
> > 0x7f 0xff 0x04 0x00
> > is the end of device path that seems to be missing for you.
> >
> >
> > This is the default (from the environment 'fatload' command)
> > fpsize = 18
> > dpsize = 113
> > 0x01 0x04 0x14 0x00 0xb9 0x73 0x1d 0xe6 0x84 0xa3 0xcc 0x4a 0xae 0xab
> > 0x82 0xe8 0x28 0xf3 0x62 0x8b 0x03 0x0f 0x0b 0x00 0x00 0x00 0x00 0x00
> > 0x09 0x00 0x01 0x03 0x0f 0x0b 0x00 0x40 0x1a 0x01 0x01 0x09 0x00 0x01
> > 0x03 0x0f 0x0b 0x00 0xe3 0x05 0x26 0x07 0x00 0x00 0x00 0x04 0x01 0x2a
> > 0x00 0x01 0x00 0x00 0x00 0x00 0x08 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > 0xf8 0x01 0x00 0x00 0x00 0x00 0x00 0x99 0x28 0x0f 0x00 0x00 0x00 0x00
> > 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x01 0x01 0x04 0x04 0x12
> > 0x75 0x00 0x49 0x00 0x6d 0x00 0x61 0x00 0x67 0x00 0x65 0x00 0x00 0x00
> > 0x00 0x7f 0xff 0x04 0x00
>
> So here "uImage" is copied in one byte left of where we would expect it
> according to the structure definition and overlaps the dp->length field.
>
> struct efi_device_path_file_path {
> struct efi_device_path dp;
> u16 str[];
> } __packed;
>
> Could you, please, send me files lib/charset.o and
> lib/efi_loader/efi_device_path.o.
>
> Which distribution and which version of GCC are you using?
>
> Adding the following printf() statements might give some more insight:
>
> fp->dp.length = fpsize;
> printf("buf = %p\n", buf);
> printf("fp->str = %p\n", fp->str);
> path_to_uefi(fp->str, path);
> buf += fpsize;
>
> Should the above printf() statements have buf + 4 != fp->str:
> What happens when you change the structure to have »u16 str[0];«? (This
> is what was required before the C99 standard. Cf.
> https://gcc.gnu.org/onlinedocs/gcc/Zero-Length.html)
>
> Best regards
>
> Heinrich
>
> >
> efi_dp_split_file_path(/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x1a40,0x101,0x9,0x0,0x1)/UsbClass(0x5e3,0x726,0x0,0x0,0x0)/HD(1,MBR,0x000f2899,0x800,0x1f800)/uImage/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004)/UNKNOWN(0000,0004))
> >
> > This is from the U-Boot command prompt with partition specified
> > => fatload usb 0:1 0x6400000 uImage
> > fpsize = 18
> > dpsize = 113
> > 0x01 0x04 0x14 0x00 0xb9 0x73 0x1d 0xe6 0x84 0xa3 0xcc 0x4a 0xae 0xab
> > 0x82 0xe8 0x28 0xf3 0x62 0x8b 0x03 0x0f 0x0b 0x00 0x00 0x00 0x00 0x00
> > 0x09 0x00 0x01 0x03 0x0f 0x0b 0x00 0x40 0x1a 0x01 0x01 0x09 0x00 0x01
> > 0x03 0x0f 0x0b 0x00 0xe3 0x05 0x26 0x07 0x00 0x00 0x00 0x04 0x01 0x2a
> > 0x00 0x01 0x00 0x00 0x00 0x00 0x08 0x00 0x00 0x00 0x00 0x00 0x00 0x00
> > 0xf8 0x01 0x00 0x00 0x00 0x00 0x00 0x99 0x28 0x0f 0x00 0x00 0x00 0x00
> > 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x00 0x01 0x01 0x04 0x04 0x12
> > 0x75 0x00 0x49 0x00 0x6d 0x00 0x61 0x00 0x67 0x00 0x65 0x00 0x00 0x00
> > 0x00 0x7f 0xff 0x04 0x00
> >
> efi_dp_split_file_path(/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x1a40,0x101,0x9,0x0,0x1)/UsbClass(0x5e3,0x726,0x0,0x0,0x0)/HD(1,MBR,0x000f2899,0x800,0x1f800)/uImage/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000)/UNKNOWN(0000,0000))
> >
> >
> > Best regards
> >
> > Heinrich
> >
> > >
> > >
> > > > x_bootcmd_kernel=fatload usb 0 0x6400000 uImage
> > > You do not specify a partition number. Do you have a
> > partition table?
> > > Than the partition defaults to 1. Or does the file
> system sit
> > directly
> > > on the device?
> > >
> > >
> > > I also tested other syntactically correct invocations of
> > 'fatload' which
> > > included the partition number.
> > > Execution does not return from efi_set_bootdev().
> > >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > >
> > > The issue is this commit forces 'fatload' and 'load' to behave
> > > identically, it does nothing else.
> > > 'git show ee88eacbdd840199a3dec707234579fb15ddd46a'
> > > Why would that duplication even be desired ?
> > >
> > > Further, the current approach of identical behaviour is
> flawed, the
> > > following scenario will fail for all platforms.
> > >
> > > load scsi 0:2 $kernel_addr_r kernimg
> > > fatload scsi 0:1 $script_addr ubscript
> > > source $script_addr
> > >
> > > With this commit reverted the above scenario would work as
> 'fatload'
> > > would not reset the EFI path.
> > >
> > >
> > > Teaching granny to suck eggs....this is where my head is at to
> > clarify
> > > what I am raising.
> > >
> > > The 'fatload' command is used to load discrete files from
> a FAT
> > > filesystem into memory.
> > > It is not exclusively to do with booting, it is often used
> to load a
> > > script for later sourcing
> > > to set variables such as IP's, or kernel command line
> arguments,
> > or even
> > > load the kernel
> > > itself.
> > >
> > > 'fatload' has *nothing* to do with EFI, the fact that EFI is
> > dependant
> > > on a FAT filesystem is a different issue.
> > > Invoking EFI code on non-EFI platforms is bad form and is
> going
> > to bite
> > > back later again and again.
> > >
> > > I feel all this confusion has come about over the
> misnaming of 'load'
> > > (which for consistency should have been named 'loadefi').
> > >
> > >
> > > >
> > > >> Whilst the proposition 'EFI boot = FAT filesystem'
> is True
> > > >> the converse 'FAT filesystem = EFI boot' is Not True
> > > >>
> > > >> I am willing to help, but that may require some EFI
> > hand-holding.
> > > >> Gray
> > > >>
> > > >> PS. If anyone knows how to set '>' on reply content in
> > GMail, please
> > > >> email me off list.
> > > >>
> > > >
> > > >
> > >
> >
>
>
> Looking at the source:
>
> struct efi_device_path *efi_dp_from_file(struct blk_desc *desc, int
> part, const char *path)
> ...
> if (desc)
> buf = dp_part_fill(buf, desc, part);
> // From this point on 'buf' can now be unaligned
> fp = buf;
> fp->dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE;
> fp->dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH;
> ....
>
> Isn,t an unaligned structure an issue on the Kirkwood SoC ?
U-Boot starts without support of unaligned data access. In
path_to_uefi() we call allow_unaligned() to switch the unaligned access
support on.
At least my GCC 9.2.1 creates code for __packed structures that avoids
unaligned access but that may be different with your compiler.
This is why I asked you to tell me which compiler version you are using
and to supply the efi_device_tree.o and charset.o files. I really need
your support to be able to understand what is happening as I can not
reproduce it on my systems. Cf.
https://lists.denx.de/pipermail/u-boot/2019-November/389847.html
I have prepared a patch
https://github.com/xypron/u-boot-patches/blob/efi-next/0001-efi_loader-call-allow_unaligned-in-efi_set_bootdev.patch
that moves the call to allow_unaligned() to efi_set_bootdev() but I
doubt that this will fix your problem.
Best regards
Heinrich
More information about the U-Boot
mailing list