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

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.

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.

Later, I could continue the boot-unification effort, e.g. delete the
booti_start() function. and incorporate the uncompressed case into
bootm code.

Regards,
SImon

[1] 487b5fa6deb (HEAD) bootm: Handle kernel_noload on arm64

Reply via email to