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@xxxxxxxxxxxxxx>
>>>> ---
>>>> 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 |
