Hi Tom, On Mon, 7 Apr 2025 at 10:48, Tom Rini <tr...@konsulko.com> wrote: > > On Mon, Apr 07, 2025 at 09:12:32AM +1200, Simon Glass wrote: > > Hi Tom, > > > > On Sun, 6 Apr 2025 at 11:23, Tom Rini <tr...@konsulko.com> wrote: > > > > > > On Sun, Apr 06, 2025 at 08:46:31AM +1200, Simon Glass wrote: > > > > (question for Heinrich below) > > > > > > > > Hi Tom, > > > > > > > > On Sun, 6 Apr 2025 at 02:48, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > On Sat, Apr 05, 2025 at 08:40:08AM -0600, Tom Rini wrote: > > > > > > On Fri, Apr 04, 2025 at 04:48:01PM -0600, Tom Rini wrote: > > > > > > > On Fri, Apr 04, 2025 at 11:32:12PM +0200, Jonas Karlman wrote: > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > On 2025-04-04 00:30, Simon Glass wrote: > > > > > > > > > Hi Jonas, > > > > > > > > > > > > > > > > > > On Fri, 4 Apr 2025 at 09:57, Jonas Karlman <jo...@kwiboo.se> > > > > > > > > > wrote: > > > > > > > > >> > > > > > > > > >> Hi Tom and Simon, > > > > > > > > >> > > > > > > > > >> On 2025-03-19 00:21, Tom Rini wrote: > > > > > > > > >>> On Wed, 05 Mar 2025 17:24:54 -0700, Simon Glass wrote: > > > > > > > > >>> > > > > > > > > >>>> This series includes some patches related to allowing > > > > > > > > >>>> read_all() to be > > > > > > > > >>>> used with the extlinux / PXE bootmeths. > > > > > > > > >>>> > > > > > > > > >>>> These patches were split out from the stb4 series, since > > > > > > > > >>>> it will need to > > > > > > > > >>>> have additional patches for LWIP, to avoid breaking PXE > > > > > > > > >>>> booting when > > > > > > > > >>>> LWIP is used. > > > > > > > > >>>> > > > > > > > > >>>> [...] > > > > > > > > >>> > > > > > > > > >>> Applied to u-boot/next, thanks! > > > > > > > > >> > > > > > > > > >> This series broke booting a compressed arm64 defconfig Linux > > > > > > > > >> kernel > > > > > > > > >> (without module loading) due to changes in decompression > > > > > > > > >> buffer length. > > > > > > > > >> > > > > > > > > >> My arm64 defconfig kernel (Image.gz) is ~24 MiB compressed > > > > > > > > >> and ~85 MiB > > > > > > > > >> uncompressed. > > > > > > > > >> > > > > > > > > >> Before this series the decompression buffer was 10x the > > > > > > > > >> kernel_comp_size > > > > > > > > >> and now it is instead limited by the SYS_BOOTM_LEN Kconfig > > > > > > > > >> symbol. > > > > > > > > >> > > > > > > > > >> A broken boot using current next branch: > > > > > > > > >> > > > > > > > > >> Retrieving file: /Image.gz > > > > > > > > >> Retrieving file: /initramfs.cpio.gz > > > > > > > > >> append: earlycon > > > > > > > > >> cmd 'booti' states 1f1f addr_img '0x02080000' conf_ramdisk > > > > > > > > >> '0x06000000:394d3c' conf_fdt '3df16350' images > > > > > > > > >> 000000003ffe53a0 > > > > > > > > >> kernel data at 0x02080000, len = 0x00000000 (0) > > > > > > > > >> load 2080000 start 2080000 len 0 ep 0 os 5 comp 1 > > > > > > > > >> find_other type 2 os 5 > > > > > > > > >> ## Flattened Device Tree blob at 3df16350 > > > > > > > > >> Booting using the fdt blob at 0x3df16350 > > > > > > > > >> Working FDT set to 3df16350 > > > > > > > > >> load_os load 8000000 image_start 2080000 image_len 2000000 > > > > > > > > >> Uncompressing Kernel Image to 8000000 > > > > > > > > >> Error: inflate() returned -5 > > > > > > > > >> Image too large: increase CONFIG_SYS_BOOTM_LEN > > > > > > > > >> Must RESET board to recover > > > > > > > > >> Resetting the board... > > > > > > > > >> > > > > > > > > >> Changing SYS_BOOTM_LEN from default 64 MiB to 128 MiB fixed > > > > > > > > >> the boot: > > > > > > > > >> > > > > > > > > >> Retrieving file: /Image.gz > > > > > > > > >> Retrieving file: /initramfs.cpio.gz > > > > > > > > >> append: earlycon > > > > > > > > >> cmd 'booti' states 1f1f addr_img '0x02080000' conf_ramdisk > > > > > > > > >> '0x06000000:394d3c' conf_fdt '3df16430' images > > > > > > > > >> 000000003ffe5480 > > > > > > > > >> kernel data at 0x02080000, len = 0x00000000 (0) > > > > > > > > >> load 2080000 start 2080000 len 0 ep 0 os 5 comp 1 > > > > > > > > >> find_other type 2 os 5 > > > > > > > > >> ## Flattened Device Tree blob at 3df16430 > > > > > > > > >> Booting using the fdt blob at 0x3df16430 > > > > > > > > >> Working FDT set to 3df16430 > > > > > > > > >> load_os load 8000000 image_start 2080000 image_len 2000000 > > > > > > > > >> Uncompressing Kernel Image to 8000000 > > > > > > > > >> kernel loaded at 0x08000000, end = 0x0d3b5a00 > > > > > > > > >> Loading Ramdisk to 3cb51000, end 3cee5d3c ... OK > > > > > > > > >> Loading Device Tree to 000000003cb3f000, end > > > > > > > > >> 000000003cb509df ... OK > > > > > > > > >> Working FDT set to 3cb3f000 > > > > > > > > >> > > > > > > > > >> Starting kernel ... > > > > > > > > >> > > > > > > > > >> Do we need to increase the default SYS_BOOTM_LEN for ARM64 > > > > > > > > >> now? > > > > > > > > > > > > > > > > > > Are you using the 'booti' command? Can you post a bit more > > > > > > > > > console > > > > > > > > > output or a script here as it isn't clear what boot command > > > > > > > > > you are > > > > > > > > > using? For now I'm going to assume booti > > > > > > > > > > > > > > > > Above was using an extlinux.conf with bootstd, not using the > > > > > > > > booti cmd. > > > > > > > > > > > > > > > > Use of booti cmd is not affected as it continue to use a 10x > > > > > > > > multiple > > > > > > > > of the kernel_comp_size env var for buffer size. > > > > > > > > > > > > > > > > The change this series introduced was instead of the 10x > > > > > > > > multiple of > > > > > > > > kernel_comp_size now use SYS_BOOTM_LEN for buffer size (320 vs > > > > > > > > 32 MiB). > > > > > > > > > > > > > > > > And because cmd/booti.c was not converted there is now two > > > > > > > > different > > > > > > > > handling of this decompression buffer size. > > > > > > > > > > > > > > > > If the change to now use SYS_BOOTM_LEN was intended this need > > > > > > > > to be > > > > > > > > changed as it has mostly been irrelevant for booting Linux on > > > > > > > > AArch64 > > > > > > > > boards prior to this series. > > > > > > > > > > > > > > We need to clean things up such that the path which is using 10x > > > > > > > multiple (and we lmb_alloc an area iirc) is used in all cases. > > > > > > > > > > > > Looking at the code this morning, I was confusing this with what we > > > > > > do > > > > > > for compressed FIT images, oops. > > > > > > > > > > Ugh. Simon, can you go and fix this whole problem? Both do_bootz and > > > > > do_booti have non-trivial amounts of code that's missed by changing > > > > > from > > > > > calling do_bootX() to bootX_run(). > > > > > > > > For do_bootz() I believe the current code is correct. It does call > > > > bootz_start() and then doesn't call it again when bootz_run() starts. > > > > It looks like you have taken my Ubuntu test so perhaps I can add a > > > > bootz test for another board in my lab. > > > > > > > > For do_booti(), the code in booti_start() is used for the > > > > non-compressed case, so I believe it is still needed and that bootm > > > > doesn't currently honour 'Image' relocation when using an uncompressed > > > > image. But then there is the issue of SYS_BOOTM_LEN, below. > > > > > > > > bootm_load_os() has code for ARM at the end, which I suspect needs to > > > > run on RISC-V too, in case the image needs to be relocated. Marek > > > > added this 7 years ago[1] so it predates RISC-V and Heinrich perhaps > > > > didn't notice it when adding his support , if it is actually needed. > > > > > > > > SYS_BOOTM_LEN is a 'last bastion' security feature to avoid people > > > > loading images that wrap around memory, etc. I could put bootm_len > > > > into struct bootm_info so that a 10x value can be used for the booti > > > > case. > > I'm not sure any of the above is 100% correct. My very quick look was > that it seems like if we want to have the command part of bootX be able > to be equivalently called via API, more of most of the commands needs to > be moved out of cmd/cmdX.c and to boot/bootX.c and put in to some state.
Yes, my intent has been to move code out of commands into boot/ and have things work as before. Also don't forget that booting a FIT with a compressed boot kernel did not work before my series. The series was largely written to fix bugs (in one place) rather that just to refactor for the fun it. > > > > > Tom, we also have the issue that you haven't taken the rest of the PXE > > > > series (due to the lwip case), so our trees are out of sync in this > > > > area. It would really help if you could take that series, and again, I > > > > am happy to do the lwip case once it is landed. > > Your further PXE rework is irrelevant to this discussion. Except in that > it points out there's still further changes that could break other > things even more. Yes, any change in untested code has the potential to create bugs. I do the best I can, but I could use a little more support in this effort. > > > > > So for now, if people agree, I can do a small series to use a > > > > different SYS_BOOTM_LEN for the booti command and perhaps add RISC-V > > > > as above. > > Well, no, that sounds like a step in the wrong direction. I'd like to > instead restore the expected behavior, and then perhaps we could clean > it up such that the compressed case for booti-Images works like FIT > images where we lmb allocate a larger area if needed. Yes, restoring the 10x decompression size is what I am talking about here. Is there anything else? > > > > > Later, I could continue the boot-unification effort, e.g. delete the > > > > booti_start() function. and incorporate the uncompressed case into > > > > bootm code. > > > > > > I'm not sure your reading of the situation is right. I'm more inclined > > > to revert this merge rather than take on more rework in the area. > > > > Well that would be a pretty poor show. No one else has spent effort > > trying to clean up and rationalise this code in the last 10 years, so > > far as I can see. This is in -next so there is plenty of time to > > resolve any issues. > > Yes, things haven't broken in 10 years. This is why I don't value "tidy > up and refactor" in and of itself. Massive reworks then require a long > time to check all of the corner cases. This has been true of many of the > massive reworks you've done over the years. Well, do you want to be able to boot without CMDLINE, or not? > > > I believe my reading of the situation is right, but if you disagree > > with some of it, please indicate which. > > Your answer is not "Oh, I'll go understand what I broke and restore > expected behavior". It's "you should take more of my big rework and I'll > rework this area even more". That is not convincing that we won't have > v2025.07 broken here. You only took half of the series, remember. We are talking about a tiny corner case here, with no test coverage. I'm offering to fix the case that has been reported. I can also even add a test for it. Regards, Simon
