[PATCH v5 00/29] pxe: Precursor series for supporting read_all() in extlinux / PXE

Mon Apr 7 00:48:26 CEST 2025

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 <trini at 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 <trini at 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 <jonas at 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.

> > > 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.

> > > 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.

> > > 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.

> 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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250406/28206096/attachment.sig>