[U-Boot] [PATCH 1/2] doc: FIT image: clarify usage of "compression" property
Compressed images should have their compression property set to "none" if U-Boot should leave them compressed.
This is especially the case for compressed ramdisks that should be uncompressed by the kernel only.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com ---
doc/uImage.FIT/source_file_format.txt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/doc/uImage.FIT/source_file_format.txt b/doc/uImage.FIT/source_file_format.txt index d2793a195d..d701b9bb76 100644 --- a/doc/uImage.FIT/source_file_format.txt +++ b/doc/uImage.FIT/source_file_format.txt @@ -164,7 +164,9 @@ the '/images' node should have the following layout: - data : Path to the external file which contains this node's binary data. - compression : Compression used by included data. Supported compressions are "gzip" and "bzip2". If no compression is used compression property - should be set to "none". + should be set to "none". If the data is compressed but it should not be + uncompressed by U-Boot (e.g. compressed ramdisk), this should also be set + to "none".
Conditionally mandatory property: - os : OS name, mandatory for types "kernel" and "ramdisk". Valid OS names
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com ---
arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- doc/README.bcm7xxx | 2 +- doc/uImage.FIT/multi.its | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon b/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon index 7dae9f03c3..b3c6693a42 100644 --- a/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon +++ b/arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon @@ -110,7 +110,7 @@ Example: type = "ramdisk"; arch = "arm64"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <0xa0000000>; }; }; diff --git a/doc/README.bcm7xxx b/doc/README.bcm7xxx index 9b5eae4741..1de2f63181 100644 --- a/doc/README.bcm7xxx +++ b/doc/README.bcm7xxx @@ -91,7 +91,7 @@ image.its: type = "ramdisk"; arch = "arm"; os = "linux"; - compression = "gzip"; + compression = "none"; /* * Set the environment variable initrd_high to * 0xffffffff, and set "load" and "entry" here diff --git a/doc/uImage.FIT/multi.its b/doc/uImage.FIT/multi.its index 26c8dad6a2..814d1f73b5 100644 --- a/doc/uImage.FIT/multi.its +++ b/doc/uImage.FIT/multi.its @@ -60,7 +60,7 @@ type = "ramdisk"; arch = "ppc"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <00000000>; entry = <00000000>; hash-1 { @@ -74,7 +74,7 @@ type = "ramdisk"; arch = "ppc"; os = "linux"; - compression = "gzip"; + compression = "none"; load = <00000000>; entry = <00000000>; hash-1 {
Dear Simon,
In message 20180730105319.79424-2-sgoldschmidt@de.pepperl-fuchs.com you wrote:
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples.
What makes you think this is a correct thing to do?
There are different approaches to handle things. For example, traditionally on Power Architecture we would use a raw kernel binray, compress this (for example with gzip) before wrapping it with mkimage into an (uImage or FIT) U-Boot image, and then let U-Boot uncompress the kernel image into rum and start it. On ARM the kernel comes traditionally with it's own wrapper that does unompressions and such.
Same for ramdisk handling. On some systems it may make sense to have U-Boot handle the uncompressing, so compression = "gzip"; may be fully intentional.
Best regards,
Wolfgang Denk
Dear Wolfgang,
On 30.07.2018 13:45, Wolfgang Denk wrote:
Dear Simon,
In message 20180730105319.79424-2-sgoldschmidt@de.pepperl-fuchs.com you wrote:
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples.
What makes you think this is a correct thing to do?
There are different approaches to handle things. For example, traditionally on Power Architecture we would use a raw kernel binray, compress this (for example with gzip) before wrapping it with mkimage into an (uImage or FIT) U-Boot image, and then let U-Boot uncompress the kernel image into rum and start it. On ARM the kernel comes traditionally with it's own wrapper that does unompressions and such.
Same for ramdisk handling. On some systems it may make sense to have U-Boot handle the uncompressing, so compression = "gzip"; may be fully intentional.
That's the whole point and in the thread I mentioned (https://lists.denx.de/pipermail/u-boot/2018-July/336435.html) it has been discussed that this is the future goal.
However, uncompression currently is only implemented for kernels, not for other sub-images. This patch aims at updating the docs and the current .its examples to what they do now.
Best regards, Simon
Dear Simon,
In message 6bce9049-a812-d8e4-cd2a-4b86dccae67e@de.pepperl-fuchs.com you wrote:
However, uncompression currently is only implemented for kernels, not for other sub-images. This patch aims at updating the docs and the current .its examples to what they do now.
Yes, but the current state should IMO not been taken as reference. I don't have time ATM to achtually check with older versions, but I'm pretty sure that that was handled correctly in the original FIT implementation, and the examples made sense at the time they were being written. I suspect that later changes broke the code, so only the code needs to be fixed.
In general for the image handling there should be no distinction at all whether this is a kernel or ramdisk or FPGA or whatever image - compression is a feature independent of the image type, and that's how the code should handle it.
Best regards,
Wolfgang Denk
On 30.07.2018 14:36, Wolfgang Denk wrote:
Dear Simon,
In message 6bce9049-a812-d8e4-cd2a-4b86dccae67e@de.pepperl-fuchs.com you wrote:
However, uncompression currently is only implemented for kernels, not for other sub-images. This patch aims at updating the docs and the current .its examples to what they do now.
Yes, but the current state should IMO not been taken as reference. I don't have time ATM to achtually check with older versions, but I'm pretty sure that that was handled correctly in the original FIT implementation, and the examples made sense at the time they were being written. I suspect that later changes broke the code, so only the code needs to be fixed.
In general for the image handling there should be no distinction at all whether this is a kernel or ramdisk or FPGA or whatever image - compression is a feature independent of the image type, and that's how the code should handle it.
OK, so at least we all seem to have a common sense of how it should be :-)
I'd still argue that the standard example in 'multi.its' should have compression = "none" for the ramdisks.
Best regards, Simon
Dear Simon,
In message 0787afb7-2da1-4eaa-437e-6f7c7582809c@de.pepperl-fuchs.com you wrote:
I'd still argue that the standard example in 'multi.its' should have compression = "none" for the ramdisks.
OK, this is your position then. I can only explain where we are coming from: in the early days of U-Boot (well, PPCBoot by then) resources were much tighter than today - look at the examples in the README: a v2.4.4 Linux kernel image would be around 780 kB uncompressed or 330 kB compressed. At this time it was considered a waste of resources to have the gzip uncompression code in the boot loader and again duplicated in the Linux kernel - so it was omitted there and U-Boot would do all uncompression, also of the ramdisks. I agree this is not standard any more, but it is still a valid use case, I think. I know, I'm an old man ;-)
Best regards,
Wolfgang Denk
On 30.07.2018 15:25, Wolfgang Denk wrote:
Dear Simon,
In message 0787afb7-2da1-4eaa-437e-6f7c7582809c@de.pepperl-fuchs.com you wrote:
I'd still argue that the standard example in 'multi.its' should have compression = "none" for the ramdisks.
OK, this is your position then. I can only explain where we are coming from: in the early days of U-Boot (well, PPCBoot by then) resources were much tighter than today - look at the examples in the README: a v2.4.4 Linux kernel image would be around 780 kB uncompressed or 330 kB compressed. At this time it was considered a waste of resources to have the gzip uncompression code in the boot loader and again duplicated in the Linux kernel - so it was omitted there and U-Boot would do all uncompression, also of the ramdisks.
Well, my point was that I couldn't see U-Boot ever handling this. But this discussion got a bit confused, split into two threads...
I agree this is not standard any more, but it is still a valid use case, I think. I know, I'm an old man ;-)
It *is* a valid use case. But for current kernels (if compression is enabled), it might be faster to let the kernel unzip the content directly to the memory location where it should reside in the end.
However, I don't have a strong opinion on these examples. I'll let the maintainers decide :-)
Best regards, Simon
Dear Simon,
In message 97a1d1e5-4415-4db4-9c77-72023f37bb4f@de.pepperl-fuchs.com you wrote:
It *is* a valid use case. But for current kernels (if compression is enabled), it might be faster to let the kernel unzip the content directly to the memory location where it should reside in the end.
This was true a long, long time ago on ARM (and this is where most of the unclean implmentations originate), when U-Boot did not enable caches. With this fixed, it is almost always wrong, because we now have an unnecessary memory copy: U-Boot loads the compressed kernel image to one location in RAM, which then runs and uncompresses the code to another address. Is it not obvious that it would be more efficient if U-Boot did the uncompressing directly?
Best regards,
Wolfgang Denk
On Mon, Jul 30, 2018 at 12:53:19PM +0200, Simon Goldschmidt wrote:
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- doc/README.bcm7xxx | 2 +- doc/uImage.FIT/multi.its | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
Sorry for the delay. I think this patch as-is, is wrong. Please split it into a patch per file that CCs the appropriate custodian(s) for them to ack/nak and pick up. It's also appropriate to do a patch now that corrects the behavior of uncompressing ramdisks that can be uncompressed automatically (and no need for a CONFIG option). Thanks!
On Sat, Aug 18, 2018 at 1:14 AM Tom Rini trini@konsulko.com wrote:
On Mon, Jul 30, 2018 at 12:53:19PM +0200, Simon Goldschmidt wrote:
To prepare supporting compression for all image types, change compression to "none" for ramdisks in all examples.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
arch/arm/cpu/armv8/fsl-layerscape/doc/README.falcon | 2 +- doc/README.bcm7xxx | 2 +- doc/uImage.FIT/multi.its | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-)
Sorry for the delay. I think this patch as-is, is wrong. Please split it into a patch per file that CCs the appropriate custodian(s) for them to ack/nak and pick up. It's also appropriate to do a patch now that corrects the behavior of uncompressing ramdisks that can be uncompressed automatically (and no need for a CONFIG option). Thanks!
OK, I'll rework it. But I won't have time for that until in two weeks or even later, so I won't be able to provide something for 2018.09.
Simon
Dear Simon,
In message 20180730105319.79424-1-sgoldschmidt@de.pepperl-fuchs.com you wrote:
Compressed images should have their compression property set to "none" if U-Boot should leave them compressed.
This is especially the case for compressed ramdisks that should be uncompressed by the kernel only.
Is this not self-explaining as is? When you use "none", U-boot wil do nothing to the data - it passes it on unchanged as binay blob.
I donrt see the need for this additional explanation of what seems obvious to me.
Best regards,
Wolfgang Denk
On Mon, Jul 30, 2018 at 01:19:16PM +0200, Wolfgang Denk wrote:
Dear Simon,
In message 20180730105319.79424-1-sgoldschmidt@de.pepperl-fuchs.com you wrote:
Compressed images should have their compression property set to "none" if U-Boot should leave them compressed.
This is especially the case for compressed ramdisks that should be uncompressed by the kernel only.
Is this not self-explaining as is? When you use "none", U-boot wil do nothing to the data - it passes it on unchanged as binay blob.
I donrt see the need for this additional explanation of what seems obvious to me.
Ah, but it's not spelled out. And also given that currently we don't decompress say a ramdisk that spells out compression = "gzip" (is that a regression from initial FIT behavior?) it helps to be clear that putting none here for "don't touch the data" is valid and that the compression property isn't just a descriptive one, U-Boot may decompress the data.
On 30.07.2018 13:19, Wolfgang Denk wrote:
Dear Simon,
In message 20180730105319.79424-1-sgoldschmidt@de.pepperl-fuchs.com you wrote:
Compressed images should have their compression property set to "none" if U-Boot should leave them compressed.
This is especially the case for compressed ramdisks that should be uncompressed by the kernel only.
Is this not self-explaining as is? When you use "none", U-boot wil do nothing to the data - it passes it on unchanged as binay blob.
I donrt see the need for this additional explanation of what seems obvious to me.
This has been explicitly requested in this mail: https://lists.denx.de/pipermail/u-boot/2018-July/336435.html
It might seem obvious to you, but given the examples had both "none" and "gzip" for ramdisk, it seems it has not been obvious for everybody.
Simon
Dear Simon,
In message 6009778b-1c55-d67b-26a5-7d9039c85e47@de.pepperl-fuchs.com you wrote:
It might seem obvious to you, but given the examples had both "none" and "gzip" for ramdisk, it seems it has not been obvious for everybody.
These may be different examples, documenting different use cases which do exactly what they say?
Best regards,
Wolfgang Denk
Dear Wolfgang,
On 30.07.2018 13:46, Wolfgang Denk wrote:
Dear Simon,
In message 6009778b-1c55-d67b-26a5-7d9039c85e47@de.pepperl-fuchs.com you wrote:
It might seem obvious to you, but given the examples had both "none" and "gzip" for ramdisk, it seems it has not been obvious for everybody.
These may be different examples, documenting different use cases which do exactly what they say?
That might well be, but given that compression in FIT images only works for kernel sub-images up to now, I strongly doubt that.
Best Regards, Simon
Dear Simon,
In message a218ccb8-a461-b119-f2d7-d1cb7c88b8c6@de.pepperl-fuchs.com you wrote:
These may be different examples, documenting different use cases which do exactly what they say?
That might well be, but given that compression in FIT images only works for kernel sub-images up to now, I strongly doubt that.
This might actually be a regression the image handling rework that went unnoticed for a long time. I'm pretty sure that the original implementation of FIT images handled this correctly.
Best regards,
Wolfgang Denk
On 30.07.2018 14:32, Wolfgang Denk wrote:
Dear Simon,
In message a218ccb8-a461-b119-f2d7-d1cb7c88b8c6@de.pepperl-fuchs.com you wrote:
These may be different examples, documenting different use cases which do exactly what they say?
That might well be, but given that compression in FIT images only works for kernel sub-images up to now, I strongly doubt that.
This might actually be a regression the image handling rework that went unnoticed for a long time. I'm pretty sure that the original implementation of FIT images handled this correctly.
In that case, we should ignore parts of patch 2/2, of course. I'm pretty sure the default examples would still leave the ramdisk compressed for the kernel to uncompress though.
As I'm not that long with U-Boot, can you point me to a rough date of a release that I could check to see if it worked at that time?
Best regards, Simon
Dear Simon,
In message 642b8dae-f941-5255-42c7-3761e12d04cb@de.pepperl-fuchs.com you wrote:
As I'm not that long with U-Boot, can you point me to a rough date of a release that I could check to see if it worked at that time?
I can only speculate... The first bigger rework of the cose appears to be this patch series:
30864 859e92b775 2013-05-14 15:37:25 -0400 image: Move timestamp #ifdefs to header file 30863 61a439a873 2013-05-14 15:37:25 -0400 image: Export fit_check_ramdisk() 30862 53fbb7e885 2013-05-14 15:37:25 -0400 image: Split FIT code into new image-fit.c 30861 604f23dde0 2013-05-14 15:37:25 -0400 image: Move HOSTCC image code to tools/ 30860 94e5fa46a0 2013-05-14 15:37:25 -0400 image: Split hash node processing into its own function 30859 b7260910dc 2013-05-14 15:37:25 -0400 image: Convert fit_image_hash_set_value() to static, and rename 30858 b8da836650 2013-05-14 15:37:25 -0400 image: Rename fit_image_check_hashes() to fit_image_verify() 30857 ab9efc665a 2013-05-14 15:37:25 -0400 image: Move hash checking into its own function 30856 e754da2aee 2013-05-14 15:37:25 -0400 image: Move error! string to common place 30855 003efd7da4 2013-05-14 15:37:25 -0400 image: Export fit_conf_get_prop_node() 30854 bbb467dc3c 2013-05-14 15:37:25 -0400 image: Rename fit_add_hashes() to fit_add_verification_data() 30853 d8b75360ee 2013-05-14 15:37:25 -0400 image: Rename hash printing to fit_image_print_verification_dat a() 30852 35e7b0f179 2013-05-14 15:37:25 -0400 sandbox: image: Add support for booting images in sandbox 30851 aa6d6db4d4 2013-05-14 15:37:25 -0400 mkimage: Put FIT loading in function and tidy error handling 30850 1fe7d93891 2013-05-14 15:37:25 -0400 image: Remove remaining #ifdefs in image-fit.c 30849 87ebee39e9 2013-05-14 15:37:25 -0400 image: Add CONFIG_FIT_SPL_PRINT to control FIT image printing i n SPL 30848 44d3a3066b 2013-05-14 15:37:25 -0400 image: Split libfdt code into image-fdt.c 30847 13d06981a9 2013-05-14 15:37:25 -0400 image: Add device tree setup to image library 30846 c19d13b030 2013-05-14 15:37:25 -0400 arm: Refactor bootm to reduce #ifdefs 30845 6caa195614 2013-05-14 15:37:25 -0400 arm: Use image_setup_linux() instead of local code 30844 3e51266a4e 2013-05-14 15:37:25 -0400 powerpc: Use image_setup_linux() instead of local code 30843 24507cf50a 2013-05-14 15:37:26 -0400 m68k: Use image_setup_linux() instead of local code 30842 2a08b740e3 2013-05-14 15:37:26 -0400 sparc: Use image_setup_linux() instead of local code
But there have been earlier changes, and many later (heavy) reworks, too.
Best regards,
Wolfgang Denk
On 30.07.2018 14:44, Wolfgang Denk wrote:
Dear Simon,
In message 642b8dae-f941-5255-42c7-3761e12d04cb@de.pepperl-fuchs.com you wrote:
As I'm not that long with U-Boot, can you point me to a rough date of a release that I could check to see if it worked at that time?
I can only speculate... The first bigger rework of the cose appears to be this patch series:
30864 859e92b775 2013-05-14 15:37:25 -0400 image: Move timestamp #ifdefs to header file 30863 61a439a873 2013-05-14 15:37:25 -0400 image: Export fit_check_ramdisk() 30862 53fbb7e885 2013-05-14 15:37:25 -0400 image: Split FIT code into new image-fit.c 30861 604f23dde0 2013-05-14 15:37:25 -0400 image: Move HOSTCC image code to tools/ 30860 94e5fa46a0 2013-05-14 15:37:25 -0400 image: Split hash node processing into its own function 30859 b7260910dc 2013-05-14 15:37:25 -0400 image: Convert fit_image_hash_set_value() to static, and rename 30858 b8da836650 2013-05-14 15:37:25 -0400 image: Rename fit_image_check_hashes() to fit_image_verify() 30857 ab9efc665a 2013-05-14 15:37:25 -0400 image: Move hash checking into its own function 30856 e754da2aee 2013-05-14 15:37:25 -0400 image: Move error! string to common place 30855 003efd7da4 2013-05-14 15:37:25 -0400 image: Export fit_conf_get_prop_node() 30854 bbb467dc3c 2013-05-14 15:37:25 -0400 image: Rename fit_add_hashes() to fit_add_verification_data() 30853 d8b75360ee 2013-05-14 15:37:25 -0400 image: Rename hash printing to fit_image_print_verification_dat a() 30852 35e7b0f179 2013-05-14 15:37:25 -0400 sandbox: image: Add support for booting images in sandbox 30851 aa6d6db4d4 2013-05-14 15:37:25 -0400 mkimage: Put FIT loading in function and tidy error handling 30850 1fe7d93891 2013-05-14 15:37:25 -0400 image: Remove remaining #ifdefs in image-fit.c 30849 87ebee39e9 2013-05-14 15:37:25 -0400 image: Add CONFIG_FIT_SPL_PRINT to control FIT image printing i n SPL 30848 44d3a3066b 2013-05-14 15:37:25 -0400 image: Split libfdt code into image-fdt.c 30847 13d06981a9 2013-05-14 15:37:25 -0400 image: Add device tree setup to image library 30846 c19d13b030 2013-05-14 15:37:25 -0400 arm: Refactor bootm to reduce #ifdefs 30845 6caa195614 2013-05-14 15:37:25 -0400 arm: Use image_setup_linux() instead of local code 30844 3e51266a4e 2013-05-14 15:37:25 -0400 powerpc: Use image_setup_linux() instead of local code 30843 24507cf50a 2013-05-14 15:37:26 -0400 m68k: Use image_setup_linux() instead of local code 30842 2a08b740e3 2013-05-14 15:37:26 -0400 sparc: Use image_setup_linux() instead of local code
But there have been earlier changes, and many later (heavy) reworks, too.
I have checked starting with v1.3.3 (i.e. v2008.05) and even then, the compression property has only been handled for kernels. It seems like only the kernel has been loaded from FIT back then...
Next I checked v2009.01, where FDT and ramdisk are loaded from FIT, too. There is even a check that FDT is uncompressed. For ramdisk, the compression property is not checked.
Reading that, I tend to think uncompression has always been like it is now.
Best Regards, Simon
Best regards,
Wolfgang Denk
On Mon, Jul 30, 2018 at 12:53:18PM +0200, Simon Goldschmidt wrote:
Compressed images should have their compression property set to "none" if U-Boot should leave them compressed.
This is especially the case for compressed ramdisks that should be uncompressed by the kernel only.
Signed-off-by: Simon Goldschmidt sgoldschmidt@de.pepperl-fuchs.com
Applied to u-boot/master, thanks!
participants (4)
-
Simon Goldschmidt -
Simon Goldschmidt -
Tom Rini -
Wolfgang Denk