[U-Boot] [BUG] U-Boot hangs on fatload, commit ee88eacbdd840199a3dec707234579fb15ddd46a
Gray Remlin
gryrmln at gmail.com
Mon Nov 11 17:14:05 UTC 2019
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>
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>> 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>>> 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>>>> 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> 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 ?
More information about the U-Boot
mailing list