[U-Boot] [RFC] fit: include uncompression into fit_image_load
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... - CONFIG_SYS_BOOTM_LEN is set to 64 MByte default in SPL, so it's a different default for SPL than for U-Boot !?! - CONFIG_SYS_BOOTM_LEN seemed to initially be used for kernel uncompression but is now used as a copy-only limit, too (no unzip) - Uncompression only works if a load address is given, what should happen if the FIT image does not contain a load address? - 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.? - Some code paths (like the command 'fpga loadmk') directly use 'fit_image_get_data()'. These will have to be converted to use 'fit_image_load()' instead (which will do nothing if the subimage does not contain a load address. - 'fit_image_load()' should probably check for subimages that have a compression set but no load address... Or should we try to malloc() here?
A long list of questions, hopefully someone will join me in discussing them :-)
Signed-off-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com ---
common/bootm.c | 8 ++++---- common/image-fit.c | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/common/bootm.c b/common/bootm.c index 8bf84ebcb7..6b85edb80b 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -361,6 +361,9 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, { int ret = 0;
+ if (!unc_len) + unc_len = CONFIG_SYS_BOOTM_LEN; + *load_end = load; print_decomp_msg(comp, type, load == image_start);
@@ -373,10 +376,7 @@ int bootm_decomp_image(int comp, ulong load, ulong image_start, int type, case IH_COMP_NONE: if (load == image_start) break; - if (image_len <= unc_len) - memmove_wd(load_buf, image_buf, image_len, CHUNKSZ); - else - ret = 1; + memmove_wd(load_buf, image_buf, image_len, CHUNKSZ); break; #ifdef CONFIG_GZIP case IH_COMP_GZIP: { diff --git a/common/image-fit.c b/common/image-fit.c index 8d39a243f8..030f0098c7 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -23,6 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; #endif /* !USE_HOSTCC*/
#include <image.h> +#include <bootm.h> #include <bootstage.h> #include <u-boot/crc.h> #include <u-boot/md5.h> @@ -1969,8 +1970,9 @@ int fit_image_load(bootm_headers_t *images, ulong addr, } } else if (load_op != FIT_LOAD_OPTIONAL_NON_ZERO || load) { ulong image_start, image_end; - ulong load_end; + ulong load_end, uncomp_len; void *dst; + u8 image_comp;
/* * move image data to the load address, @@ -1986,11 +1988,22 @@ int fit_image_load(bootm_headers_t *images, ulong addr, return -EXDEV; }
+ if (fit_image_get_comp(fit, noffset, &image_comp)) { + puts("Can't get image compression!\n"); + return -EINVAL; + } printf(" Loading %s from 0x%08lx to 0x%08lx\n", prop_name, data, load);
dst = map_sysmem(load, len); - memmove(dst, buf, len); + ret = bootm_decomp_image(image_comp, load, image_start, + image_type, dst, (void *)buf, len, 0, + &load_end); + if (ret) { + bootstage_error(BOOTSTAGE_ID_DECOMP_IMAGE); + return ret; + } + len = load_end - load; data = load; } bootstage_mark(bootstage_id + BOOTSTAGE_SUB_LOAD);
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...
- CONFIG_SYS_BOOTM_LEN is set to 64 MByte default in SPL, so it's a different default for SPL than for U-Boot !?!
- CONFIG_SYS_BOOTM_LEN seemed to initially be used for kernel uncompression but is now used as a copy-only limit, too (no unzip)
- Uncompression only works if a load address is given, what should happen if the FIT image does not contain a load address?
- 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.?
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?
Alex
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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...
- CONFIG_SYS_BOOTM_LEN is set to 64 MByte default in SPL, so it's a different default for SPL than for U-Boot !?!
- CONFIG_SYS_BOOTM_LEN seemed to initially be used for kernel uncompression but is now used as a copy-only limit, too (no unzip)
- Uncompression only works if a load address is given, what should happen if the FIT image does not contain a load address?
- 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.?
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!
Simon
Hi Simon,
On 17 October 2018 at 03:41, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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.
- 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.
- 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.
Simon
Regards, Simon
On 19.10.2018 05:25, Simon Glass wrote:
Hi Simon,
On 17 October 2018 at 03:41, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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...
- 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
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt simon.k.r.goldschmidt@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.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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.
- 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.
- 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.
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
On 22.10.2018 19:49, Simon Glass wrote:
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt simon.k.r.goldschmidt@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.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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
Hi,
On 22 October 2018 at 11:55, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 22.10.2018 19:49, Simon Glass wrote:
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt simon.k.r.goldschmidt@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.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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?
I suggest supporting the old way with a flag. Also is it possible to detect an already-compressed file and print an warning?
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.)
Yes that should be the limit I think.
> - 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 suppose it is just that we have no information about the size of the kernel, so use this fixed limit?
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...
Perhaps for security reasons, to avoid memory overflow?
> - 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?
Yes I think so.
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
Regards, Simon
On Fri, Nov 16, 2018 at 3:05 AM Simon Glass sjg@chromium.org wrote:
Hi,
On 22 October 2018 at 11:55, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 22.10.2018 19:49, Simon Glass wrote:
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt simon.k.r.goldschmidt@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.goldschmidt@gmail.com wrote:
On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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?
I suggest supporting the old way with a flag. Also is it possible to detect an already-compressed file and print an warning?
I'm working on this and have it partly running, but I had to add this ugly "#ifndef USE_HOSTCC" thing to many files in lib/ to get the compression algorithms to compile for the tools.
Is this acceptable? Or should we find a more generic approach, i.e. fixing the central include files (common.h, etc.) to handle USE_HOSTCC?
Simon
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.)
Yes that should be the limit I think.
>> - 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 suppose it is just that we have no information about the size of the kernel, so use this fixed limit?
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...
Perhaps for security reasons, to avoid memory overflow?
>> - 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?
Yes I think so.
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
Regards, Simon
Hi Simon,
On Tue, 20 Nov 2018 at 14:03, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On Fri, Nov 16, 2018 at 3:05 AM Simon Glass sjg@chromium.org wrote:
Hi,
On 22 October 2018 at 11:55, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com wrote:
On 22.10.2018 19:49, Simon Glass wrote:
Hi Simon,
On 19 October 2018 at 00:33, Simon Goldschmidt simon.k.r.goldschmidt@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.goldschmidt@gmail.com wrote: > > On Wed, Oct 17, 2018 at 8:54 AM Alexander Graf agraf@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?
I suggest supporting the old way with a flag. Also is it possible to detect an already-compressed file and print an warning?
I'm working on this and have it partly running, but I had to add this ugly "#ifndef USE_HOSTCC" thing to many files in lib/ to get the compression algorithms to compile for the tools.
Is this acceptable? Or should we find a more generic approach, i.e. fixing the central include files (common.h, etc.) to handle USE_HOSTCC?
What sort of things don't build? It's not great, but it may be necessary.
Bonus points if the #ifdefs can be just in a header file, like mkimage.h
Regards, Simon
Hi Simon,
On 16 October 2018 at 13:33, Simon Goldschmidt simon.k.r.goldschmidt@gmail.com 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.
- 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
- 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.?
lmb is the mechanism for this and I think it checks for overlap.
- Some code paths (like the command 'fpga loadmk') directly use 'fit_image_get_data()'. These will have to be converted to use 'fit_image_load()' instead (which will do nothing if the subimage does not contain a load address.
OK
- 'fit_image_load()' should probably check for subimages that have a compression set but no load address... Or should we try to malloc() here?
malloc() is generally small. Best to use lmb.
A long list of questions, hopefully someone will join me in discussing them :-)
I think FIT provides a better format. We should be able to do the right thing in all cases I believe.
Signed-off-by: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com
common/bootm.c | 8 ++++---- common/image-fit.c | 17 +++++++++++++++-- 2 files changed, 19 insertions(+), 6 deletions(-)
Regards, Simon
participants (3)
-
Alexander Graf -
Simon Glass -
Simon Goldschmidt