From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Goldschmidt Date: Fri, 19 Oct 2018 08:33:49 +0200 Subject: [U-Boot] [RFC] fit: include uncompression into fit_image_load In-Reply-To: References: <20181016193348.13863-1-simon.k.r.goldschmidt@gmail.com> <35ad844e-d682-299e-d037-0637527b5e5e@suse.de> Message-ID: <0ec358ce-9326-fea9-5a89-361677bb0968@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 19.10.2018 05:25, Simon Glass wrote: > Hi Simon, > > On 17 October 2018 at 03:41, Simon Goldschmidt > wrote: >> On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf 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... > >>>> - 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. > >>>> - Uncompression only works if a load address is given, what should >>>> happen if the FIT image does not contain a load address? > Fail. > >>>> - 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. Simon