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

[Sascha Hauer <sha@pengutronix.de>]

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 |