Hi Tom,
On Tue, 6 May 2025 at 18:32, Tom Rini <tr...@konsulko.com> wrote:
>
> On Tue, May 06, 2025 at 03:24:21PM +0200, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 5 May 2025 at 20:14, Tom Rini <tr...@konsulko.com> wrote:
> > >
> > > On Thu, May 01, 2025 at 07:18:32AM -0600, Simon Glass wrote:
> > >
> > > > This series restores the original behaviour of extlinux booting linux
> > > > 'Image' files, which is to ignore CONFIG_SYS_BOOTM_LEN and instead uses
> > > > a limit of 10x the compressed size.
> > > >
> > > > It also adds RISC-V support, since it uses a similar format to ARM64.
> > > >
> > > > Future work should integrate the code in 'booti' into main 'bootm'
> > > > logic.
> > >
> > > I don't like "in the future we'll remove duplicated code".
> >
> > Small series, fixes a problem, can be made larger but then it isn't a
> > bug-fix.
>
> Which is why yes, this should have been instead of the however-many-part
> "PXE" series, and then fixups on top, a series to address this problem.
> Then other series to address other problems.
The whole effort is addressing problems, although it depends on your
definition of 'problem'.
I'm quite happy with the approach I have taken here, incremental
improvement towards a long-term goal. It doesn't need to be constantly
rebased into out-of-tree code. People can only test what is in-tree.
>
> > > I also don't
> > > like not seeing that what we really need to do, in all cases (not just
> > > booti) handle decompression like we do for FIT images, and ask LMB to
> > > give us a space to use.
> >
> > See bootm_load_os() which does already do this.
>
> Yes, that's what I was asking you to look at. I assume you're even
> specifically pointing to commit 69544c4fd8b1 ("bootm: Support
> kernel_noload with compression") which you and I did together.
I seldom look at the commits that produced the code, but yes.
>
> > > A problem is that CONFIG_SYS_BOOTM_LEN was never
> > > intended to be the limit on *decompression* as it's the limit on what
> > > we're loading to memory from disk. That's what getting me unhappy with
> > > this part of the series.
> >
> > From what I can tell, that was introduced 11 years ago by this commit:
> >
> > 081cc197472 (HEAD) bootm: Export bootm_decomp_image()
> >
> > I suppose the idea is that BOOTM is supposed to be a limit on the
> > image being loaded, so it is compressed, then the limit needs to apply
> > to the size of the uncompressed image, to maintain parity. Otherwise
> > there would be no limit at all, since it is pretty easy to devise an
> > 100-byte image which expands to fill all available memory.
> >
> > Using 10x the uncompressed size doesn't fill me with the joys of spring,
> > TBH.
>
> Yes, it's a matter of heuristics, and also why we have things like lmb
> to check and make sure we don't overwrite ourselves. I do not recall if
> the 10x used there, or the 4x we used for kernel_noload FIT
> decompression, was based on checking what the compression factor the
> available algorithms get on typical kernel images or not. That would be
> another improvement area, sure.
That commit you pointed to does not actually pass the lmb_alloced()'d
size to image_decomp(), so it isn't perfect.
I would much rather use the BOOTM limit everywhere, including as the
maximum uncompressed size of images. That is what it is for, as I
understand it. Users would be quite surprised if the limit were
ignored in certain circumstances.
Regards,
Simon
