[PATCH] FIT: add first support for compressed images

[PATCH] FIT: add first support for compressed images

[Ahmad Fatoum]

FIT image contents are often compressed, but we got by so far, because
a compressed initramfs is usually meant to be decompressed by the kernel
(and so has compression = "none") and arm32 kernels had their own
decompresser embedded. On ARM64, bootloader is responsible for
uncompressing kernel, so we should properly process the compression
property we so far ignored.

The decompression isn't as efficient as one would hope for, because
the FIT format only describes length of the compressed data. We thus
have two options:

  - define an output size up-front, e.g. by guessing the uncompressed
     buffer size for decompression or hardcoding it (e.g. U-Boot's
     CONFIG_SYS_BOOTM_LEN).

  -  Uncompress to a file descriptor

We choose the second one to play it safe, but it comes with worse
performance because of extra memory copies. Intention is to go with
first option for the kernel image: We know how much size we can spare
for the kernel image and can have bootm_load_os uncompress there
directly without intermittent memory copies. This would involve slight
change to the barebox decompresser API to align it with the kernel's,
which allows to have it accept and observe an output buffer size.
So far, we had the kernel PREBOOT API, which lacks such a parameter,
but that's an optimization for another day.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
 include/uncompress.h |  6 ++++++
 lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index 507a857cadb4..e692fcdaa737 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -21,6 +21,7 @@
 #include <linux/err.h>
 #include <stringlist.h>
 #include <rsa.h>
+#include <uncompress.h>
 #include <image-fit.h>
 
 #define FDT_MAX_DEPTH 32
@@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
 	return ret;
 }
 
+static void fit_uncompress_error_fn(char *x)
+{
+	pr_err("%s\n", x);
+}
+
 /**
  * fit_open_image - Open an image in a FIT image
  * @handle: The FIT image handle
@@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		   unsigned long *outsize)
 {
 	struct device_node *image;
-	const char *unit = name, *type = NULL, *desc= "(no description)";
-	const void *data;
+	const char *unit = name, *type = NULL, *compression = NULL,
+	      *desc= "(no description)";
+	struct property *dataprop;
+	const void *data = NULL;
 	int data_len;
 	int ret = 0;
 
@@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 		return -EINVAL;
 	}
 
-	data = of_get_property(image, "data", &data_len);
+	dataprop = of_find_property(image, "data", &data_len);
+	if (dataprop)
+		data = of_property_get_value(dataprop);
 	if (!data) {
 		pr_err("data not found\n");
 		return -EINVAL;
@@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 	if (ret < 0)
 		return ret;
 
+	of_property_read_string(image, "compression", &compression);
+	if (compression && strcmp(compression, "none") != 0) {
+		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
+			pr_err("image has compression = \"%s\", but support not compiled in\n");
+			return -ENOSYS;
+		}
+		if (!dataprop->value) {
+			void *uc_data;
+
+			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
+						    fit_uncompress_error_fn);
+			if (data_len < 0) {
+				pr_err("data couldn't be decompressed\n");
+				return data_len;
+			}
+
+			/* associate buffer with FIT, so it's not leaked */
+			data = dataprop->value = uc_data;
+			dataprop->length = data_len;
+		}
+	}
+
 	*outdata = data;
 	*outsize = data_len;
 
diff --git a/include/uncompress.h b/include/uncompress.h
index 4bdb03d4f5b6..72ba1dfda607 100644
--- a/include/uncompress.h
+++ b/include/uncompress.h
@@ -15,6 +15,12 @@ int uncompress_fd_to_fd(int infd, int outfd,
 int uncompress_fd_to_buf(int infd, void *output,
 	   void(*error_fn)(char *x));
 
+int uncompress_buf_to_fd(const void *input, size_t input_len,
+			 int outfd, void(*error_fn)(char *x));
+
+ssize_t uncompress_buf_to_buf(const void *input, size_t input_len,
+			      void **buf, void(*error_fn)(char *x));
+
 void uncompress_err_stdout(char *);
 
 #endif /* __UNCOMPRESS_H */
diff --git a/lib/uncompress.c b/lib/uncompress.c
index 15eb3da098c8..1134d8c52801 100644
--- a/lib/uncompress.c
+++ b/lib/uncompress.c
@@ -178,3 +178,43 @@ int uncompress_fd_to_buf(int infd, void *output,
 
 	return uncompress(NULL, 0, fill_fd, NULL, output, NULL, error_fn);
 }
+
+int uncompress_buf_to_fd(const void *input, size_t input_len,
+			 int outfd, void(*error_fn)(char *x))
+{
+	uncompress_outfd = outfd;
+
+	return uncompress((void *)input, input_len, NULL, flush_fd,
+			  NULL, NULL, error_fn);
+}
+
+ssize_t uncompress_buf_to_buf(const void *input, size_t input_len,
+			      void **buf, void(*error_fn)(char *x))
+{
+	char *dstpath;
+	size_t size;
+	int outfd, ret;
+
+	dstpath = make_temp("data-uncompressed");
+	if (!dstpath)
+		return -ENOMEM;
+
+	outfd = open(dstpath, O_CREAT | O_WRONLY);
+	if (outfd < 0) {
+		ret = -ENODEV;
+		goto free_temp;
+	}
+
+	ret = uncompress_buf_to_fd(input, input_len, outfd, uncompress_err_stdout);
+	if (ret)
+		goto close_outfd;
+
+	*buf = read_file(dstpath, &size);
+close_outfd:
+	close(outfd);
+	unlink(dstpath);
+free_temp:
+	free(dstpath);
+
+	return ret ?: size;
+}
-- 
2.30.2

Re: [PATCH] FIT: add first support for compressed images

[Sascha Hauer]

On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
> FIT image contents are often compressed, but we got by so far, because
> a compressed initramfs is usually meant to be decompressed by the kernel
> (and so has compression = "none") and arm32 kernels had their own
> decompresser embedded. On ARM64, bootloader is responsible for
> uncompressing kernel, so we should properly process the compression
> property we so far ignored.
> 
> The decompression isn't as efficient as one would hope for, because
> the FIT format only describes length of the compressed data. We thus
> have two options:
> 
>   - define an output size up-front, e.g. by guessing the uncompressed
>      buffer size for decompression or hardcoding it (e.g. U-Boot's
>      CONFIG_SYS_BOOTM_LEN).
> 
>   -  Uncompress to a file descriptor
> 
> We choose the second one to play it safe, but it comes with worse
> performance because of extra memory copies. Intention is to go with
> first option for the kernel image: We know how much size we can spare
> for the kernel image and can have bootm_load_os uncompress there
> directly without intermittent memory copies. This would involve slight
> change to the barebox decompresser API to align it with the kernel's,
> which allows to have it accept and observe an output buffer size.
> So far, we had the kernel PREBOOT API, which lacks such a parameter,
> but that's an optimization for another day.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
>  include/uncompress.h |  6 ++++++
>  lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 81 insertions(+), 3 deletions(-)
> 
> diff --git a/common/image-fit.c b/common/image-fit.c
> index 507a857cadb4..e692fcdaa737 100644
> --- a/common/image-fit.c
> +++ b/common/image-fit.c
> @@ -21,6 +21,7 @@
>  #include <linux/err.h>
>  #include <stringlist.h>
>  #include <rsa.h>
> +#include <uncompress.h>
>  #include <image-fit.h>
>  
>  #define FDT_MAX_DEPTH 32
> @@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
>  	return ret;
>  }
>  
> +static void fit_uncompress_error_fn(char *x)
> +{
> +	pr_err("%s\n", x);
> +}
> +
>  /**
>   * fit_open_image - Open an image in a FIT image
>   * @handle: The FIT image handle
> @@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		   unsigned long *outsize)
>  {
>  	struct device_node *image;
> -	const char *unit = name, *type = NULL, *desc= "(no description)";
> -	const void *data;
> +	const char *unit = name, *type = NULL, *compression = NULL,
> +	      *desc= "(no description)";
> +	struct property *dataprop;
> +	const void *data = NULL;
>  	int data_len;
>  	int ret = 0;
>  
> @@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  		return -EINVAL;
>  	}
>  
> -	data = of_get_property(image, "data", &data_len);
> +	dataprop = of_find_property(image, "data", &data_len);
> +	if (dataprop)
> +		data = of_property_get_value(dataprop);
>  	if (!data) {
>  		pr_err("data not found\n");
>  		return -EINVAL;
> @@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>  	if (ret < 0)
>  		return ret;
>  
> +	of_property_read_string(image, "compression", &compression);
> +	if (compression && strcmp(compression, "none") != 0) {
> +		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
> +			pr_err("image has compression = \"%s\", but support not compiled in\n");
> +			return -ENOSYS;
> +		}
> +		if (!dataprop->value) {
> +			void *uc_data;
> +
> +			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
> +						    fit_uncompress_error_fn);
> +			if (data_len < 0) {
> +				pr_err("data couldn't be decompressed\n");
> +				return data_len;
> +			}
> +
> +			/* associate buffer with FIT, so it's not leaked */
> +			data = dataprop->value = uc_data;
> +			dataprop->length = data_len;

Why are you fiddling with struct property fields directly?
of_get_property() and of_set_property() should do what you want.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] FIT: add first support for compressed images

[Ahmad Fatoum]

Hello Sascha,

On 08.08.22 13:11, Sascha Hauer wrote:
> On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
>> FIT image contents are often compressed, but we got by so far, because
>> a compressed initramfs is usually meant to be decompressed by the kernel
>> (and so has compression = "none") and arm32 kernels had their own
>> decompresser embedded. On ARM64, bootloader is responsible for
>> uncompressing kernel, so we should properly process the compression
>> property we so far ignored.
>>
>> The decompression isn't as efficient as one would hope for, because
>> the FIT format only describes length of the compressed data. We thus
>> have two options:
>>
>>   - define an output size up-front, e.g. by guessing the uncompressed
>>      buffer size for decompression or hardcoding it (e.g. U-Boot's
>>      CONFIG_SYS_BOOTM_LEN).
>>
>>   -  Uncompress to a file descriptor
>>
>> We choose the second one to play it safe, but it comes with worse
>> performance because of extra memory copies. Intention is to go with
>> first option for the kernel image: We know how much size we can spare
>> for the kernel image and can have bootm_load_os uncompress there
>> directly without intermittent memory copies. This would involve slight
>> change to the barebox decompresser API to align it with the kernel's,
>> which allows to have it accept and observe an output buffer size.
>> So far, we had the kernel PREBOOT API, which lacks such a parameter,
>> but that's an optimization for another day.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
>>  include/uncompress.h |  6 ++++++
>>  lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 81 insertions(+), 3 deletions(-)
>>
>> diff --git a/common/image-fit.c b/common/image-fit.c
>> index 507a857cadb4..e692fcdaa737 100644
>> --- a/common/image-fit.c
>> +++ b/common/image-fit.c
>> @@ -21,6 +21,7 @@
>>  #include <linux/err.h>
>>  #include <stringlist.h>
>>  #include <rsa.h>
>> +#include <uncompress.h>
>>  #include <image-fit.h>
>>  
>>  #define FDT_MAX_DEPTH 32
>> @@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
>>  	return ret;
>>  }
>>  
>> +static void fit_uncompress_error_fn(char *x)
>> +{
>> +	pr_err("%s\n", x);
>> +}
>> +
>>  /**
>>   * fit_open_image - Open an image in a FIT image
>>   * @handle: The FIT image handle
>> @@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		   unsigned long *outsize)
>>  {
>>  	struct device_node *image;
>> -	const char *unit = name, *type = NULL, *desc= "(no description)";
>> -	const void *data;
>> +	const char *unit = name, *type = NULL, *compression = NULL,
>> +	      *desc= "(no description)";
>> +	struct property *dataprop;
>> +	const void *data = NULL;
>>  	int data_len;
>>  	int ret = 0;
>>  
>> @@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  		return -EINVAL;
>>  	}
>>  
>> -	data = of_get_property(image, "data", &data_len);
>> +	dataprop = of_find_property(image, "data", &data_len);
>> +	if (dataprop)
>> +		data = of_property_get_value(dataprop);
>>  	if (!data) {
>>  		pr_err("data not found\n");
>>  		return -EINVAL;
>> @@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> +	of_property_read_string(image, "compression", &compression);
>> +	if (compression && strcmp(compression, "none") != 0) {
>> +		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
>> +			pr_err("image has compression = \"%s\", but support not compiled in\n");
>> +			return -ENOSYS;
>> +		}
>> +		if (!dataprop->value) {
>> +			void *uc_data;
>> +
>> +			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
>> +						    fit_uncompress_error_fn);
>> +			if (data_len < 0) {
>> +				pr_err("data couldn't be decompressed\n");
>> +				return data_len;
>> +			}
>> +
>> +			/* associate buffer with FIT, so it's not leaked */
>> +			data = dataprop->value = uc_data;
>> +			dataprop->length = data_len;
> 
> Why are you fiddling with struct property fields directly?
> of_get_property() and of_set_property() should do what you want.

of_set_property copies the data into a new buffer, which I want
to avoid doing again.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] FIT: add first support for compressed images

[Sascha Hauer]

On Mon, Aug 08, 2022 at 01:38:28PM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 08.08.22 13:11, Sascha Hauer wrote:
> > On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
> >> FIT image contents are often compressed, but we got by so far, because
> >> a compressed initramfs is usually meant to be decompressed by the kernel
> >> (and so has compression = "none") and arm32 kernels had their own
> >> decompresser embedded. On ARM64, bootloader is responsible for
> >> uncompressing kernel, so we should properly process the compression
> >> property we so far ignored.
> >>
> >> The decompression isn't as efficient as one would hope for, because
> >> the FIT format only describes length of the compressed data. We thus
> >> have two options:
> >>
> >>   - define an output size up-front, e.g. by guessing the uncompressed
> >>      buffer size for decompression or hardcoding it (e.g. U-Boot's
> >>      CONFIG_SYS_BOOTM_LEN).
> >>
> >>   -  Uncompress to a file descriptor
> >>
> >> We choose the second one to play it safe, but it comes with worse
> >> performance because of extra memory copies. Intention is to go with
> >> first option for the kernel image: We know how much size we can spare
> >> for the kernel image and can have bootm_load_os uncompress there
> >> directly without intermittent memory copies. This would involve slight
> >> change to the barebox decompresser API to align it with the kernel's,
> >> which allows to have it accept and observe an output buffer size.
> >> So far, we had the kernel PREBOOT API, which lacks such a parameter,
> >> but that's an optimization for another day.
> >>
> >> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >> ---
> >>  common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
> >>  include/uncompress.h |  6 ++++++
> >>  lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 81 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/common/image-fit.c b/common/image-fit.c
> >> index 507a857cadb4..e692fcdaa737 100644
> >> --- a/common/image-fit.c
> >> +++ b/common/image-fit.c
> >> @@ -21,6 +21,7 @@
> >>  #include <linux/err.h>
> >>  #include <stringlist.h>
> >>  #include <rsa.h>
> >> +#include <uncompress.h>
> >>  #include <image-fit.h>
> >>  
> >>  #define FDT_MAX_DEPTH 32
> >> @@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
> >>  	return ret;
> >>  }
> >>  
> >> +static void fit_uncompress_error_fn(char *x)
> >> +{
> >> +	pr_err("%s\n", x);
> >> +}
> >> +
> >>  /**
> >>   * fit_open_image - Open an image in a FIT image
> >>   * @handle: The FIT image handle
> >> @@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>  		   unsigned long *outsize)
> >>  {
> >>  	struct device_node *image;
> >> -	const char *unit = name, *type = NULL, *desc= "(no description)";
> >> -	const void *data;
> >> +	const char *unit = name, *type = NULL, *compression = NULL,
> >> +	      *desc= "(no description)";
> >> +	struct property *dataprop;
> >> +	const void *data = NULL;
> >>  	int data_len;
> >>  	int ret = 0;
> >>  
> >> @@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>  		return -EINVAL;
> >>  	}
> >>  
> >> -	data = of_get_property(image, "data", &data_len);
> >> +	dataprop = of_find_property(image, "data", &data_len);
> >> +	if (dataprop)
> >> +		data = of_property_get_value(dataprop);
> >>  	if (!data) {
> >>  		pr_err("data not found\n");
> >>  		return -EINVAL;
> >> @@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>  	if (ret < 0)
> >>  		return ret;
> >>  
> >> +	of_property_read_string(image, "compression", &compression);
> >> +	if (compression && strcmp(compression, "none") != 0) {
> >> +		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
> >> +			pr_err("image has compression = \"%s\", but support not compiled in\n");
> >> +			return -ENOSYS;
> >> +		}
> >> +		if (!dataprop->value) {
> >> +			void *uc_data;
> >> +
> >> +			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
> >> +						    fit_uncompress_error_fn);
> >> +			if (data_len < 0) {
> >> +				pr_err("data couldn't be decompressed\n");
> >> +				return data_len;
> >> +			}
> >> +
> >> +			/* associate buffer with FIT, so it's not leaked */
> >> +			data = dataprop->value = uc_data;
> >> +			dataprop->length = data_len;
> > 
> > Why are you fiddling with struct property fields directly?
> > of_get_property() and of_set_property() should do what you want.
> 
> of_set_property copies the data into a new buffer, which I want
> to avoid doing again.

Ah, yes. Ok then.

Still the

	if (!dataprop->value) {
		...
	}

part confuses me. With the current code it won't be set. If it will be
set due to future changes then your code won't uncompress the buffer and
silently continues. I think this should rather be

	free(dataprop->value);
	dataprop->value = uc_data;

As a bonus you'll save an indention level.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] FIT: add first support for compressed images

[Ahmad Fatoum]

Hello Sascha,

On 08.08.22 14:03, Sascha Hauer wrote:
> On Mon, Aug 08, 2022 at 01:38:28PM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 08.08.22 13:11, Sascha Hauer wrote:
>>> On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
>>>> FIT image contents are often compressed, but we got by so far, because
>>>> a compressed initramfs is usually meant to be decompressed by the kernel
>>>> (and so has compression = "none") and arm32 kernels had their own
>>>> decompresser embedded. On ARM64, bootloader is responsible for
>>>> uncompressing kernel, so we should properly process the compression
>>>> property we so far ignored.
>>>>
>>>> The decompression isn't as efficient as one would hope for, because
>>>> the FIT format only describes length of the compressed data. We thus
>>>> have two options:
>>>>
>>>>   - define an output size up-front, e.g. by guessing the uncompressed
>>>>      buffer size for decompression or hardcoding it (e.g. U-Boot's
>>>>      CONFIG_SYS_BOOTM_LEN).
>>>>
>>>>   -  Uncompress to a file descriptor
>>>>
>>>> We choose the second one to play it safe, but it comes with worse
>>>> performance because of extra memory copies. Intention is to go with
>>>> first option for the kernel image: We know how much size we can spare
>>>> for the kernel image and can have bootm_load_os uncompress there
>>>> directly without intermittent memory copies. This would involve slight
>>>> change to the barebox decompresser API to align it with the kernel's,
>>>> which allows to have it accept and observe an output buffer size.
>>>> So far, we had the kernel PREBOOT API, which lacks such a parameter,
>>>> but that's an optimization for another day.
>>>>
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>> ---
>>>>  common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
>>>>  include/uncompress.h |  6 ++++++
>>>>  lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 81 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/common/image-fit.c b/common/image-fit.c
>>>> index 507a857cadb4..e692fcdaa737 100644
>>>> --- a/common/image-fit.c
>>>> +++ b/common/image-fit.c
>>>> @@ -21,6 +21,7 @@
>>>>  #include <linux/err.h>
>>>>  #include <stringlist.h>
>>>>  #include <rsa.h>
>>>> +#include <uncompress.h>
>>>>  #include <image-fit.h>
>>>>  
>>>>  #define FDT_MAX_DEPTH 32
>>>> @@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static void fit_uncompress_error_fn(char *x)
>>>> +{
>>>> +	pr_err("%s\n", x);
>>>> +}
>>>> +
>>>>  /**
>>>>   * fit_open_image - Open an image in a FIT image
>>>>   * @handle: The FIT image handle
>>>> @@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>>  		   unsigned long *outsize)
>>>>  {
>>>>  	struct device_node *image;
>>>> -	const char *unit = name, *type = NULL, *desc= "(no description)";
>>>> -	const void *data;
>>>> +	const char *unit = name, *type = NULL, *compression = NULL,
>>>> +	      *desc= "(no description)";
>>>> +	struct property *dataprop;
>>>> +	const void *data = NULL;
>>>>  	int data_len;
>>>>  	int ret = 0;
>>>>  
>>>> @@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>>  		return -EINVAL;
>>>>  	}
>>>>  
>>>> -	data = of_get_property(image, "data", &data_len);
>>>> +	dataprop = of_find_property(image, "data", &data_len);
>>>> +	if (dataprop)
>>>> +		data = of_property_get_value(dataprop);
>>>>  	if (!data) {
>>>>  		pr_err("data not found\n");
>>>>  		return -EINVAL;
>>>> @@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> +	of_property_read_string(image, "compression", &compression);
>>>> +	if (compression && strcmp(compression, "none") != 0) {
>>>> +		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
>>>> +			pr_err("image has compression = \"%s\", but support not compiled in\n");
>>>> +			return -ENOSYS;
>>>> +		}
>>>> +		if (!dataprop->value) {
>>>> +			void *uc_data;
>>>> +
>>>> +			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
>>>> +						    fit_uncompress_error_fn);
>>>> +			if (data_len < 0) {
>>>> +				pr_err("data couldn't be decompressed\n");
>>>> +				return data_len;
>>>> +			}
>>>> +
>>>> +			/* associate buffer with FIT, so it's not leaked */
>>>> +			data = dataprop->value = uc_data;
>>>> +			dataprop->length = data_len;
>>>
>>> Why are you fiddling with struct property fields directly?
>>> of_get_property() and of_set_property() should do what you want.
>>
>> of_set_property copies the data into a new buffer, which I want
>> to avoid doing again.
> 
> Ah, yes. Ok then.
> 
> Still the
> 
> 	if (!dataprop->value) {
> 		...
> 	}
> 
> part confuses me. With the current code it won't be set. If it will be
> set due to future changes then your code won't uncompress the buffer and
> silently continues. I think this should rather be
> 
> 	free(dataprop->value);
> 	dataprop->value = uc_data;
> 
> As a bonus you'll save an indention level.

I don't want to free dataprop->value. I want to cache the uncompressed
data alongside the FIT, so a renewed call to fit_open_image()
doesn't decompress again. I haven't checked if this can happen currently,
but it feels natural to keep value_const for the uncompressed data
and populate value with the compressed data and do the uncompression
only once on first fit_open_image().

Cheers,
Ahmad


> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] FIT: add first support for compressed images

[Sascha Hauer]

On Mon, Aug 08, 2022 at 02:12:01PM +0200, Ahmad Fatoum wrote:
> Hello Sascha,
> 
> On 08.08.22 14:03, Sascha Hauer wrote:
> > On Mon, Aug 08, 2022 at 01:38:28PM +0200, Ahmad Fatoum wrote:
> >> Hello Sascha,
> >>
> >> On 08.08.22 13:11, Sascha Hauer wrote:
> >>> On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
> >>>> FIT image contents are often compressed, but we got by so far, because
> >>>> a compressed initramfs is usually meant to be decompressed by the kernel
> >>>> (and so has compression = "none") and arm32 kernels had their own
> >>>> decompresser embedded. On ARM64, bootloader is responsible for
> >>>> uncompressing kernel, so we should properly process the compression
> >>>> property we so far ignored.
> >>>>
> >>>> The decompression isn't as efficient as one would hope for, because
> >>>> the FIT format only describes length of the compressed data. We thus
> >>>> have two options:
> >>>>
> >>>>   - define an output size up-front, e.g. by guessing the uncompressed
> >>>>      buffer size for decompression or hardcoding it (e.g. U-Boot's
> >>>>      CONFIG_SYS_BOOTM_LEN).
> >>>>
> >>>>   -  Uncompress to a file descriptor
> >>>>
> >>>> We choose the second one to play it safe, but it comes with worse
> >>>> performance because of extra memory copies. Intention is to go with
> >>>> first option for the kernel image: We know how much size we can spare
> >>>> for the kernel image and can have bootm_load_os uncompress there
> >>>> directly without intermittent memory copies. This would involve slight
> >>>> change to the barebox decompresser API to align it with the kernel's,
> >>>> which allows to have it accept and observe an output buffer size.
> >>>> So far, we had the kernel PREBOOT API, which lacks such a parameter,
> >>>> but that's an optimization for another day.
> >>>>
> >>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> >>>> ---
> >>>>  common/image-fit.c   | 38 +++++++++++++++++++++++++++++++++++---
> >>>>  include/uncompress.h |  6 ++++++
> >>>>  lib/uncompress.c     | 40 ++++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 81 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/common/image-fit.c b/common/image-fit.c
> >>>> index 507a857cadb4..e692fcdaa737 100644
> >>>> --- a/common/image-fit.c
> >>>> +++ b/common/image-fit.c
> >>>> @@ -21,6 +21,7 @@
> >>>>  #include <linux/err.h>
> >>>>  #include <stringlist.h>
> >>>>  #include <rsa.h>
> >>>> +#include <uncompress.h>
> >>>>  #include <image-fit.h>
> >>>>  
> >>>>  #define FDT_MAX_DEPTH 32
> >>>> @@ -559,6 +560,11 @@ int fit_get_image_address(struct fit_handle *handle, void *configuration,
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> +static void fit_uncompress_error_fn(char *x)
> >>>> +{
> >>>> +	pr_err("%s\n", x);
> >>>> +}
> >>>> +
> >>>>  /**
> >>>>   * fit_open_image - Open an image in a FIT image
> >>>>   * @handle: The FIT image handle
> >>>> @@ -581,8 +587,10 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>>>  		   unsigned long *outsize)
> >>>>  {
> >>>>  	struct device_node *image;
> >>>> -	const char *unit = name, *type = NULL, *desc= "(no description)";
> >>>> -	const void *data;
> >>>> +	const char *unit = name, *type = NULL, *compression = NULL,
> >>>> +	      *desc= "(no description)";
> >>>> +	struct property *dataprop;
> >>>> +	const void *data = NULL;
> >>>>  	int data_len;
> >>>>  	int ret = 0;
> >>>>  
> >>>> @@ -599,7 +607,9 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>>>  		return -EINVAL;
> >>>>  	}
> >>>>  
> >>>> -	data = of_get_property(image, "data", &data_len);
> >>>> +	dataprop = of_find_property(image, "data", &data_len);
> >>>> +	if (dataprop)
> >>>> +		data = of_property_get_value(dataprop);
> >>>>  	if (!data) {
> >>>>  		pr_err("data not found\n");
> >>>>  		return -EINVAL;
> >>>> @@ -613,6 +623,28 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
> >>>>  	if (ret < 0)
> >>>>  		return ret;
> >>>>  
> >>>> +	of_property_read_string(image, "compression", &compression);
> >>>> +	if (compression && strcmp(compression, "none") != 0) {
> >>>> +		if (!IS_ENABLED(CONFIG_UNCOMPRESS)) {
> >>>> +			pr_err("image has compression = \"%s\", but support not compiled in\n");
> >>>> +			return -ENOSYS;
> >>>> +		}
> >>>> +		if (!dataprop->value) {
> >>>> +			void *uc_data;
> >>>> +
> >>>> +			data_len = uncompress_buf_to_buf(data, data_len, &uc_data,
> >>>> +						    fit_uncompress_error_fn);
> >>>> +			if (data_len < 0) {
> >>>> +				pr_err("data couldn't be decompressed\n");
> >>>> +				return data_len;
> >>>> +			}
> >>>> +
> >>>> +			/* associate buffer with FIT, so it's not leaked */
> >>>> +			data = dataprop->value = uc_data;
> >>>> +			dataprop->length = data_len;
> >>>
> >>> Why are you fiddling with struct property fields directly?
> >>> of_get_property() and of_set_property() should do what you want.
> >>
> >> of_set_property copies the data into a new buffer, which I want
> >> to avoid doing again.
> > 
> > Ah, yes. Ok then.
> > 
> > Still the
> > 
> > 	if (!dataprop->value) {
> > 		...
> > 	}
> > 
> > part confuses me. With the current code it won't be set. If it will be
> > set due to future changes then your code won't uncompress the buffer and
> > silently continues. I think this should rather be
> > 
> > 	free(dataprop->value);
> > 	dataprop->value = uc_data;
> > 
> > As a bonus you'll save an indention level.
> 
> I don't want to free dataprop->value. I want to cache the uncompressed
> data alongside the FIT, so a renewed call to fit_open_image()
> doesn't decompress again. I haven't checked if this can happen currently,
> but it feels natural to keep value_const for the uncompressed data
> and populate value with the compressed data and do the uncompression
> only once on first fit_open_image().

IMO the fields in struct property should only be used inside the core OF
code. In the core OF code "value" and "value_const" hold the same data,
once allocated dynamically and once allocated externally. "value_const"
purely exists to optimize memory usage for FIT images.

We shouldn't overload this and now start to store an uncompressed
version of "value_const" in "value". It also doesn't look very useful as
fit_open_image() isn't called multiple times for the same image. If that
becomes a usecase, then we should cache the data in some other data
structure, not in the device tree property.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] FIT: add first support for compressed images

[Ahmad Fatoum]

On 08.08.22 16:40, Sascha Hauer wrote:
> On Mon, Aug 08, 2022 at 02:12:01PM +0200, Ahmad Fatoum wrote:
>> Hello Sascha,
>>
>> On 08.08.22 14:03, Sascha Hauer wrote:
>>> On Mon, Aug 08, 2022 at 01:38:28PM +0200, Ahmad Fatoum wrote:
>>>> Hello Sascha,
>>>>
>>>> On 08.08.22 13:11, Sascha Hauer wrote:
>>>>> On Mon, Aug 08, 2022 at 08:56:48AM +0200, Ahmad Fatoum wrote:
>>>>>> +			/* associate buffer with FIT, so it's not leaked */
>>>>>> +			data = dataprop->value = uc_data;
>>>>>> +			dataprop->length = data_len;
>>>>>
>>>>> Why are you fiddling with struct property fields directly?
>>>>> of_get_property() and of_set_property() should do what you want.
>>>>
>>>> of_set_property copies the data into a new buffer, which I want
>>>> to avoid doing again.
>>>
>>> Ah, yes. Ok then.
>>>
>>> Still the
>>>
>>> 	if (!dataprop->value) {
>>> 		...
>>> 	}
>>>
>>> part confuses me. With the current code it won't be set. If it will be
>>> set due to future changes then your code won't uncompress the buffer and
>>> silently continues. I think this should rather be
>>>
>>> 	free(dataprop->value);
>>> 	dataprop->value = uc_data;
>>>
>>> As a bonus you'll save an indention level.
>>
>> I don't want to free dataprop->value. I want to cache the uncompressed
>> data alongside the FIT, so a renewed call to fit_open_image()
>> doesn't decompress again. I haven't checked if this can happen currently,
>> but it feels natural to keep value_const for the uncompressed data
>> and populate value with the compressed data and do the uncompression
>> only once on first fit_open_image().
> 
> IMO the fields in struct property should only be used inside the core OF
> code. In the core OF code "value" and "value_const" hold the same data,
> once allocated dynamically and once allocated externally. "value_const"
> purely exists to optimize memory usage for FIT images.
> 
> We shouldn't overload this and now start to store an uncompressed
> version of "value_const" in "value". It also doesn't look very useful as
> fit_open_image() isn't called multiple times for the same image. If that
> becomes a usecase, then we should cache the data in some other data
> structure, not in the device tree property.

We need to link the uncompressed data buffer with the FIT image,
so it doesn't leak. Any of the multiple images can be compressed, so we need
to hold multiple buffers. Storing this as a property in the device tree node
is quite elegant IMO. I agree now that the value/value_const dichotomy
is not the best way, so I added a new uncompressed-data property in v2.

Cheers,
Ahmad

> 
> Sascha
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |