From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gray Remlin Date: Mon, 11 Nov 2019 17:14:05 +0000 Subject: [U-Boot] [BUG] U-Boot hangs on fatload, commit ee88eacbdd840199a3dec707234579fb15ddd46a In-Reply-To: References: <469255ff-e461-fafb-8c7a-5e485906f579@gmx.de> <6ec3214e-313f-75e7-6c73-063c2e45ac7e@gmx.de> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 wrote: > On 11/9/19 10:31 PM, Gray Remlin wrote: > > On Sat, 9 Nov 2019 at 20:50, Heinrich Schuchardt > > wrote: > > > > On 11/9/19 8:42 PM, Gray Remlin wrote: > > > On Sat, 9 Nov 2019 at 18:40, Heinrich Schuchardt > > > > > >> 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 > > > > > > > > > >> > > >>> 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 > > > > > > > > >> 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 ?