On 22.10.2018 19:49, Simon Glass wrote:
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt
<simon.k.r.goldschm...@gmail.com> wrote:
On 19.10.2018 05:25, Simon Glass wrote:
Hi Simon,
On 17 October 2018 at 03:41, Simon Goldschmidt
<simon.k.r.goldschm...@gmail.com> wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf <ag...@suse.de> wrote:
On 16.10.18 21:33, Simon Goldschmidt wrote:
Currently, only the kernel can be compressed in a FIT image.
By moving the uncompression logic to 'fit_image_load()', all types
of images can be compressed.
This works perfectly for me when using a gzip'ed FPGA image in
a FIT image on a cyclone5 board (socrates). Also, a gzip'ed initrd
being unzipped by U-Boot (not the kernel) worked.
To clean this up, the uncompression function would have to be moved
from bootm.c ('bootm_decomp_image()') to a more generic location,
but I decided to keep it for now to make the patch easier to read.
Because of this setup, the kernel is currently uncompressed twice.
which doesn't work...
There are, however, some more things to discuss:
- The max. size passed to gunzip() (etc.) is not known before, so
we currently configure this to 8 MByte in U-Boot (via
CONFIG_SYS_BOOTM_LEN), which proved too small for my initrd...
We need the uncompressed size. If the legacy header doesn't have, stop
using it and use FIT?
Some compression formats include that in a header I think. But we
should record it in the U-Boot header.
OK, we could embed this information into the FIT by extracting in 'mkimage'?
That way, we know the uncompressed size...
Yes. In fact I don't like the way it works at present. We have to
compress the data before putting it in the FIT, since the .its file
refers to the compressed data.
I think it would be better for the ,its to refer to the original file,
and then mkimage do the compression as it generates the FIT. That way
it knows the size of the original data, and it is simpler for people
to build images, since they don't need to compress everything
beforehand.
Hmm, OK, I think it should not be a problem to add this to mkimage.
Only I don't know if this workflow change would be accepted by everyone
or if the old style of using pre-compressed files would have to be
somehow kept working?
But what would that mean for CONFIG_SYS_BOOTM_LEN? As I already wrote
before, this constant is currently used to trim copy-only, too. Now if
the FIT would embed "uncompressed size", would we limit that to
CONFIG_SYS_BOOTM_LEN, too? I think it would then make more sense to dump
this constant and rely on lmb allocation to detect overlapping. (That
assumes the load address is within lmb, of course.)
- CONFIG_SYS_BOOTM_LEN is set to 64 MByte default in SPL, so it's
a different default for SPL than for U-Boot !?!
Ick
- CONFIG_SYS_BOOTM_LEN seemed to initially be used for kernel
uncompression but is now used as a copy-only limit, too (no unzip)
Yes.
Why do we need an extra limit for uncompressed copy-only? Both load address
and size are known from the FIT.
I'm not suggesting separate limit. We don't need that.
But bootm_decomp_image() *does* use CONFIG_SYS_BOOTM_LEN already with
IH_COMP_NONE to limit the size of an uncompressed kernel image.
Is that a bug then?
I don't understand why we need this limit. It seems arbitrary to me
given we only limit the size but don't know which address we limit...
- Uncompression only works if a load address is given, what should
happen if the FIT image does not contain a load address?
Fail.
Given correct lmb integration, wouldn't it make more sense to use lmb to
allocate a block? Especially if we know the uncompressed size?
Simon
- The whole memory management around FIT images is a bit messed up
in that memory allocation is a mix of where U-Boot relocates itself
(and which address ranges it used), the load addresses of the FIT
image and the load addresses of the FIT image contents (and sizes).
What's the point here to check CONFIG_SYS_BOOTM_LEN? Maybe it would
be better to keep a memory map of allowed and already used data to
check if we're overwriting things or to get the maximum size passed
to gunzip etc.?
See lmb
So I can at least give input on the memory map part :).
In efi_loader, we actually do maintain a full system memory map already,
including allocation functions that give you "safe" allocation
functionality (allocate somewhere in memory where you know nothing
overlaps).
Maybe we should move this into a more generic system and reuse it for
big memory allocations that really don't need to be in the U-Boot
preallocated regions?
Hmm, inspecting this further, it seems that there is such an allocator
for bootm (using lmb_*() functions and struct lmb). Maybe this should be
better integrated into the fit loading function. I don't know if the
lmb functions
correctly detect overlapping of regions allocated by known addresses
though.
Thanks for your thoughts!
Yes lmb is the right mechanism and I think it checks for overlap.
That didn't work for all overlap checks for me. I'll dig into it to see
what's missing for me.
Sounds good.
Also off this stuff could do with more tests. Our current bootm tests
do not check lmb as far as I know.
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
