[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[Julius Werner]

The Linux ramdisk should always be decompressed by the kernel itself,
not by U-Boot. Therefore, the 'compression' node in the FIT image should
always be set to "none" for ramdisk images, since the only point of
using that node is if you want U-Boot to do the decompression itself.

Yet some systems populate the node to the compression algorithm used by
the kernel instead. This used to be ignored, but now that we support
decompression of all image types it becomes a problem. Since ramdisks
should never be decompressed by U-Boot anyway, this patch adds a special
exception for them to avoid these issues. Still, setting the
'compression' node like that is wrong in the first place, so we still
want to print out a warning so that third-party distributions doing this
can notice and fix it.

Signed-off-by: Julius Werner <jwerner@chromium.org>
---
 common/image-fit.c        | 13 +++++++++----
 test/py/tests/test_fit.py | 10 +++++++++-
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index e346fed550..5c63c769de 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1998,10 +1998,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 	comp = IH_COMP_NONE;
 	loadbuf = buf;
 	/* Kernel images get decompressed later in bootm_load_os(). */
-	if (!(image_type == IH_TYPE_KERNEL ||
-	      image_type == IH_TYPE_KERNEL_NOLOAD) &&
-	    !fit_image_get_comp(fit, noffset, &comp) &&
-	    comp != IH_COMP_NONE) {
+	if (!fit_image_get_comp(fit, noffset, &comp) &&
+	    comp != IH_COMP_NONE &&
+	    !(image_type == IH_TYPE_KERNEL ||
+	      image_type == IH_TYPE_KERNEL_NOLOAD ||
+	      image_type == IH_TYPE_RAMDISK)) {
 		ulong max_decomp_len = len * 20;
 		if (load == data) {
 			loadbuf = malloc(max_decomp_len);
@@ -2021,6 +2022,10 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
 		memcpy(loadbuf, buf, len);
 	}
 
+	if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE)
+		puts("WARNING: 'compression' nodes for ramdisks are deprecated,"
+		     " please fix your .its file!\n");
+
 	/* verify that image data is a proper FDT blob */
 	if (image_type == IH_TYPE_FLATDT && fdt_check_header(loadbuf)) {
 		puts("Subimage data is not a FDT");
diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py
index 8009d2907b..e3210ed43f 100755
--- a/test/py/tests/test_fit.py
+++ b/test/py/tests/test_fit.py
@@ -269,6 +269,11 @@ def test_fit(u_boot_console):
     def check_equal(expected_fname, actual_fname, failure_msg):
         """Check that a file matches its expected contents
 
+        This is always used on out-buffers whose size is decided by the test
+        script anyway, which in some cases may be larger than what we're
+        actually looking for. So it's safe to truncate it to the size of the
+        expected data.
+
         Args:
             expected_fname: Filename containing expected contents
             actual_fname: Filename containing actual contents
@@ -276,6 +281,8 @@ def test_fit(u_boot_console):
         """
         expected_data = read_file(expected_fname)
         actual_data = read_file(actual_fname)
+        if len(expected_data) < len(actual_data):
+            actual_data = actual_data[:len(expected_data)]
         assert expected_data == actual_data, failure_msg
 
     def check_not_equal(expected_fname, actual_fname, failure_msg):
@@ -435,7 +442,8 @@ def test_fit(u_boot_console):
             output = cons.run_command_list(cmd.splitlines())
             check_equal(kernel, kernel_out, 'Kernel not loaded')
             check_equal(control_dtb, fdt_out, 'FDT not loaded')
-            check_equal(ramdisk, ramdisk_out, 'Ramdisk not loaded')
+            check_not_equal(ramdisk, ramdisk_out, 'Ramdisk got decompressed?')
+            check_equal(ramdisk + '.gz', ramdisk_out, 'Ramdist not loaded')
 
 
     cons = u_boot_console
-- 
2.20.1

[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[Heiko Schocher]

Hello Julius,

Am 03.08.2019 um 00:52 schrieb Julius Werner:
> The Linux ramdisk should always be decompressed by the kernel itself,
> not by U-Boot. Therefore, the 'compression' node in the FIT image should
> always be set to "none" for ramdisk images, since the only point of
> using that node is if you want U-Boot to do the decompression itself.
> 
> Yet some systems populate the node to the compression algorithm used by
> the kernel instead. This used to be ignored, but now that we support
> decompression of all image types it becomes a problem. Since ramdisks
> should never be decompressed by U-Boot anyway, this patch adds a special
> exception for them to avoid these issues. Still, setting the
> 'compression' node like that is wrong in the first place, so we still
> want to print out a warning so that third-party distributions doing this
> can notice and fix it.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> ---
>   common/image-fit.c        | 13 +++++++++----
>   test/py/tests/test_fit.py | 10 +++++++++-
>   2 files changed, 18 insertions(+), 5 deletions(-)

Many thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>
Tested-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[Simon Goldschmidt]

On Sat, Aug 3, 2019 at 12:52 AM Julius Werner <jwerner@chromium.org> wrote:
>
> The Linux ramdisk should always be decompressed by the kernel itself,
> not by U-Boot. Therefore, the 'compression' node in the FIT image should
> always be set to "none" for ramdisk images, since the only point of
> using that node is if you want U-Boot to do the decompression itself.
>
> Yet some systems populate the node to the compression algorithm used by
> the kernel instead. This used to be ignored, but now that we support
> decompression of all image types it becomes a problem. Since ramdisks
> should never be decompressed by U-Boot anyway, this patch adds a special
> exception for them to avoid these issues. Still, setting the
> 'compression' node like that is wrong in the first place, so we still
> want to print out a warning so that third-party distributions doing this
> can notice and fix it.
>
> Signed-off-by: Julius Werner <jwerner@chromium.org>

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

> ---
>  common/image-fit.c        | 13 +++++++++----
>  test/py/tests/test_fit.py | 10 +++++++++-
>  2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/common/image-fit.c b/common/image-fit.c
> index e346fed550..5c63c769de 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -1998,10 +1998,11 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>         comp = IH_COMP_NONE;
>         loadbuf = buf;
>         /* Kernel images get decompressed later in bootm_load_os(). */
> -       if (!(image_type == IH_TYPE_KERNEL ||
> -             image_type == IH_TYPE_KERNEL_NOLOAD) &&
> -           !fit_image_get_comp(fit, noffset, &comp) &&
> -           comp != IH_COMP_NONE) {
> +       if (!fit_image_get_comp(fit, noffset, &comp) &&
> +           comp != IH_COMP_NONE &&
> +           !(image_type == IH_TYPE_KERNEL ||
> +             image_type == IH_TYPE_KERNEL_NOLOAD ||
> +             image_type == IH_TYPE_RAMDISK)) {
>                 ulong max_decomp_len = len * 20;
>                 if (load == data) {
>                         loadbuf = malloc(max_decomp_len);
> @@ -2021,6 +2022,10 @@ int fit_image_load(bootm_headers_t *images, ulong addr,
>                 memcpy(loadbuf, buf, len);
>         }
>
> +       if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE)
> +               puts("WARNING: 'compression' nodes for ramdisks are deprecated,"
> +                    " please fix your .its file!\n");
> +
>         /* verify that image data is a proper FDT blob */
>         if (image_type == IH_TYPE_FLATDT && fdt_check_header(loadbuf)) {
>                 puts("Subimage data is not a FDT");
> diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py
> index 8009d2907b..e3210ed43f 100755
> --- a/test/py/tests/test_fit.py
> +++ b/test/py/tests/test_fit.py
> @@ -269,6 +269,11 @@ def test_fit(u_boot_console):
>      def check_equal(expected_fname, actual_fname, failure_msg):
>          """Check that a file matches its expected contents
>
> +        This is always used on out-buffers whose size is decided by the test
> +        script anyway, which in some cases may be larger than what we're
> +        actually looking for. So it's safe to truncate it to the size of the
> +        expected data.
> +
>          Args:
>              expected_fname: Filename containing expected contents
>              actual_fname: Filename containing actual contents
> @@ -276,6 +281,8 @@ def test_fit(u_boot_console):
>          """
>          expected_data = read_file(expected_fname)
>          actual_data = read_file(actual_fname)
> +        if len(expected_data) < len(actual_data):
> +            actual_data = actual_data[:len(expected_data)]
>          assert expected_data == actual_data, failure_msg
>
>      def check_not_equal(expected_fname, actual_fname, failure_msg):
> @@ -435,7 +442,8 @@ def test_fit(u_boot_console):
>              output = cons.run_command_list(cmd.splitlines())
>              check_equal(kernel, kernel_out, 'Kernel not loaded')
>              check_equal(control_dtb, fdt_out, 'FDT not loaded')
> -            check_equal(ramdisk, ramdisk_out, 'Ramdisk not loaded')
> +            check_not_equal(ramdisk, ramdisk_out, 'Ramdisk got decompressed?')
> +            check_equal(ramdisk + '.gz', ramdisk_out, 'Ramdist not loaded')
>
>
>      cons = u_boot_console
> --
> 2.20.1
>

[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[Tom Rini]

On Fri, Aug 02, 2019 at 03:52:28PM -0700, Julius Werner wrote:

> The Linux ramdisk should always be decompressed by the kernel itself,
> not by U-Boot. Therefore, the 'compression' node in the FIT image should
> always be set to "none" for ramdisk images, since the only point of
> using that node is if you want U-Boot to do the decompression itself.
> 
> Yet some systems populate the node to the compression algorithm used by
> the kernel instead. This used to be ignored, but now that we support
> decompression of all image types it becomes a problem. Since ramdisks
> should never be decompressed by U-Boot anyway, this patch adds a special
> exception for them to avoid these issues. Still, setting the
> 'compression' node like that is wrong in the first place, so we still
> want to print out a warning so that third-party distributions doing this
> can notice and fix it.
> 
> Signed-off-by: Julius Werner <jwerner@chromium.org>
> Reviewed-by: Heiko Schocher <hs@denx.de>
> Tested-by: Heiko Schocher <hs@denx.de>
> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190807/08fdd2c7/attachment.sig>

[U-Boot] [PATCH] fit: Do not automatically decompress ramdisk images

[Rasmus Villemoes]

On 08/08/2019 05.16, Tom Rini wrote:
> On Fri, Aug 02, 2019 at 03:52:28PM -0700, Julius Werner wrote:
> 
>> The Linux ramdisk should always be decompressed by the kernel itself,
>> not by U-Boot. Therefore, the 'compression' node in the FIT image should
>> always be set to "none" for ramdisk images, since the only point of
>> using that node is if you want U-Boot to do the decompression itself.
>>
>> Yet some systems populate the node to the compression algorithm used by
>> the kernel instead. This used to be ignored, but now that we support
>> decompression of all image types it becomes a problem. Since ramdisks
>> should never be decompressed by U-Boot anyway, this patch adds a special
>> exception for them to avoid these issues. Still, setting the
>> 'compression' node like that is wrong in the first place, so we still
>> want to print out a warning so that third-party distributions doing this
>> can notice and fix it.
>>
>> Signed-off-by: Julius Werner <jwerner@chromium.org>
>> Reviewed-by: Heiko Schocher <hs@denx.de>
>> Tested-by: Heiko Schocher <hs@denx.de>
>> Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt@gmail.com>

This part

+	if (image_type == IH_TYPE_RAMDISK && comp != IH_COMP_NONE)
+		puts("WARNING: 'compression' nodes for ramdisks are deprecated,"
+		     " please fix your .its file!\n");
+

ends up being a little confusing, because when one dutifully removes the
compression = "foo" property, the warning is still there (because comp
ends up being (u8)-1) - the only way to silence it is by actually
_having_ a 'compression = "none"' property. (It also says node instead
of property).

So, what is the intention? Should ramdisk images not have a compression
property at all, or must it be present but set to "none", or are either
acceptable?

Rasmus