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

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |