On Tue, Aug 18, 2026 at 12:01:31AM +0800, Aristo Chen wrote: > Hi Simon, Tom, > > On Sun, Aug 16, 2026 at 2:33 AM Simon Glass <[email protected]> wrote: > > > > Hi, > > > > On Wed, 12 Aug 2026 at 09:57, Tom Rini <[email protected]> wrote: > > > > > > On Wed, Aug 12, 2026 at 09:45:52AM +0200, Nora Schiffer wrote: > > > > On Mon, 2026-08-10 at 10:37 -0600, Tom Rini wrote: > > > > > On Mon, Aug 10, 2026 at 10:32:12AM +0800, Aristo Chen wrote: > > > > > > On Sun, Aug 9, 2026 at 11:27 PM Tom Rini <[email protected]> wrote: > > > > > > > > > > > > > > On Sun, Aug 09, 2026 at 04:23:27AM +0000, Aristo Chen wrote: > > > > > > > > > > > > > > > For a compressed kernel_noload image, bootm_load_os() allocates > > > > > > > > a > > > > > > > > per-image decompression buffer of ALIGN(image_len * 8, SZ_1M). > > > > > > > > The 8x > > > > > > > > multiplier is a heuristic: it comfortably covers what zstd and > > > > > > > > xz > > > > > > > > achieve on real kernels, but any well-compressed payload (say, > > > > > > > > a big > > > > > > > > run of zeros) can exceed it and fail decompression, and no fixed > > > > > > > > multiplier is safe against arbitrarily compressible input. > > > > > > > > > > > > > > > > Read the real uncompressed size from the compressor header > > > > > > > > instead. > > > > > > > > Add a small helper image_decomp_get_uncompressed_size() that > > > > > > > > returns > > > > > > > > the uncompressed size when the format carries one: gzip ISIZE, > > > > > > > > lzma > > > > > > > > header uncompressed size, lz4 frame Content_Size when the FLG > > > > > > > > bit is > > > > > > > > set, and zstd Frame_Content_Size. Other formats return > > > > > > > > -EOPNOTSUPP. > > > > > > > > Bootm uses it to size the buffer to ALIGN(hdr_size, SZ_1M), > > > > > > > > capped at > > > > > > > > CONFIG_SYS_BOOTM_LEN because the value is attacker-controlled, > > > > > > > > and > > > > > > > > falls back to the 8x heuristic for formats without a size field > > > > > > > > (bzip2, lzo, xz) or when the header lacks the size (some lzma > > > > > > > > or lz4 > > > > > > > > streams). > > > > > > > > > > > > > > Have we gotten actual problem reports? This is a good bit of > > > > > > > growth for > > > > > > > a problem I'm not sure we're seeing. Thanks. > > > > > > > > > > > > Thanks for the review! Honest answer: no bug report against the > > > > > > current 8x multiplier has crossed the list. This is preventive > > > > > > rather > > > > > > than reactive, and I should have made that clearer in the cover > > > > > > letter. > > > > > > > > > > > > The reasons for this patch set are: > > > > > > * The multiplier is fundamentally a heuristic. Nora raised the > > > > > > same > > > > > > concern in the v1 round of the earlier > > > > > > series(<https://lists.denx.de/pipermail/u-boot/2026-June/621575.html>): > > > > > > "Deriving a buffer size from the compressed size is not possible, as > > > > > > the compression ratio may be arbitrarily high for data with many > > > > > > repetitions (for example ranges of 0x00 or 0xff)."She dropped her > > > > > > replacement patch when we bumped 4x to 8x, but the underlying point > > > > > > stands: any fixed factor can be defeated by a highly compressible > > > > > > payload, and further bumps are just moving the ceiling. > > > > > > > > > > Yeah, I recall this. But we aren't really handling arbitrary data > > > > > here, > > > > > so it's not as much of a valid concern I think, without real examples. > > > > > > > > It's probably not a problem when the OS image is a proper kernel, but > > > > if the > > > > next image is a tiny loader itself, even a small amount of padding > > > > (either > > > > inside the .data section or at the end of the image) might result in > > > > high > > > > compression ratios. > > > > > > > > While irrelevant for current U-Boot, one example would be OpenWrt's > > > > lzma-loader: > > > > it has a build mode where the <100KiB binary is padded to 1MiB (I may be > > > > remembering the exact numbers wrong) before compression to force a cache > > > > writeback during decompression (to work around ancient U-Boot versions > > > > that did > > > > not implement cache handling correctly.) > > > > > > > > Specifically the case of kernel_noload would usually be used with EFI > > > > applications, for which additional loaders (shim, systemd-boot, ...) > > > > are quite > > > > common. The combination with FIT and compression is probably less > > > > common... > > > > > > > > Nonetheless, I think a principled fix is preferable - I like the > > > > EFI-in-FIT > > > > approach a lot (we may make that the default setup in our TQ-Systems > > > > standard > > > > BSPs in the future), thus I would like the feature to be well-supported > > > > and > > > > without known bugs. > > > > > > Thanks for explaining. My concern, now that I've put it through a wider > > > test, is that of about 1550 platforms, 1297 grow under this as-is. Of > > > those, ~375 grow by around 400 bytes (380 is average, a few go higher). > > > The rest are around 170 bytes. This is all presumably the difference > > > between gzip only and gzip+others (with the few much high growth being > > > all algorithms). > > > > > > Maybe a question here is, haven't we already validated the compression > > > header, and so don't need to do it a second time? If we really can't > > > live with a good enough heuristic, we need to work the size growth as > > > this is very much not an opt-in feature. > > > > Given these comments I'm going to hold off reviewing this series. I > > agree that getting the real uncompressed size is a nice idea, but if > > it is too expensive in terms of code size, then we might be better to > > stick with what we have. Another options is to write the uncompressed > > size as a property in the FIT image. > > Thanks Simon, I think a FIT property is an attractive option, > especially for the EFI-in-FIT case that motivated this: the boot-side > cost becomes a single property read, the ITS author or build system > already knows the uncompressed size so nothing needs to parse the > stream anywhere, it works even for formats whose streams carry no > size field, and images without the property simply keep the current > 8x fallback. The trade-offs are that it needs a binding addition plus > image-generation support, only images that carry the property > benefit, and the legacy uImage form of kernel_noload stays on the > heuristic (which is probably acceptable). The property value would > still need the CONFIG_SYS_BOOTM_LEN cap before allocating, same as a > header value. > > To Tom's earlier question about validating the header twice: the > value is used only as an allocation hint. bootm performs only the > format-specific parsing needed to obtain the size, caps it at > CONFIG_SYS_BOOTM_LEN, and the decompressor remains authoritative for > validating and decoding the stream. But I agree the property answers > that concern even more directly, since bootm then reads nothing from > the stream at all. > > On the size growth, since that was the blocker: I reworked the v1 > implementation into per-format helpers that are only compiled when > the matching decompressor is enabled, and re-ran the world build > (all 1550 defconfigs, v1 and the rework applied to the same base > commit). Median growth on changed boards drops from +160 to +96 > bytes, the 856 gzip-only boards go from +112 to +80, boards without > any of the formats go from +108 to zero, and 1222 of 1496 comparable > boards end up smaller than with v1. Tom, the "few go higher" > outliers in your run should be the binutils Cortex-A53 erratum > 843419 workaround: each triggered veneer is padded to a full 4 KiB > page, and any few-hundred-byte change re-rolls which arm64 boards > gain or lose one, so those jumps are not code from the patch itself. > > So from my side both directions are workable: > > - if the reworked cost is acceptable, I can post it as v2, split > per format so any individual decompressor can be dropped; > - if the property route is preferred, I am happy to prototype that > instead (binding plus the bootm side, with the 8x fallback for > images without the property) and hold the rework. > > The two also compose rather than conflict: with both in place bootm > would prefer the property, then the stream header, then the 8x > fallback, so picking one now does not rule out adding the other > later. I have not listed the combination as a third option only > because its boot-side cost is the sum of the two, so it becomes > interesting once the header cost itself is judged acceptable. > > Which would you prefer?
Lets see a v2 of the rework. I think some of the higher size growth I was talking about was on the platforms which enabled multiple algorithms, fwiw. -- Tom
signature.asc
Description: PGP signature
