On Mon, Apr 07, 2025 at 01:07:34PM +1200, Simon Glass wrote: > 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.
Then none of this should be in a "pxe" series, and you should have been removing a bunch more code out of the command part of the file. > 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. I'm not sure which FIT use case you're talking about here exactly and when it got broken (because if it was in use already, someone would have reported 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. And the best way to get support for that is to slow down and make smaller more easily testable series. There's yet another bug in this series too that Svyatoslav has or will post about shortly. > > > > > 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? Functional wise, the being reported now issue Svyatoslav found too. Implementation wise, yes, I don't want to refactor and re-wrtie a bunch of this as the first step. The first step should be just what's needed to move the working functionality out of cmd/bootX.c and in to boot/bootX.c and let people verify things haven't regressed. Then some rework can be reviewed on its own. > > > > > 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? It's an interesting long term goal. But since there's always some distribution or another that ships with each release we do, not having functional regressions is more important than speed of new features. Being able to practically disable the command line has been a feature for forever. Being able to literally has implications for both security and end user rights that need to be considered too. > > > 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. It wasn't half of the series. I didn't take more because it was either the stuff that broke other functionality or because it wasn't used until some further series from you. It's not a tiny corner case. A test case is nice, sure, but we'll never have 100% test coverage and need physical testing too. Which most people only have time for around releases. Which is why we need to go slow. Which again is why you need to split up your changes more and expect them to go slower. So there's time to test things. -- Tom
signature.asc
Description: PGP signature
