[U-Boot] [PATCH v2 2/2] RISCV: image: Parse Image.gz support in booti.

[Atish Patra]

On 4/26/19 11:05 AM, Marek Vasut wrote:
> On 4/26/19 7:08 PM, Atish Patra wrote:
>> On 4/25/19 10:33 PM, Marek Vasut wrote:
>>> On 4/25/19 9:56 PM, Atish Patra wrote:
>>>> Add gz parsing logic so that booti can parse both Image
>>>> and Image.gz.
>>>>
>>>> Signed-off-by: Atish Patra <atish.patra at wdc.com>
>>>> ---
>>>>    arch/riscv/lib/image.c | 28 +++++++++++++++++++++++++++-
>>>>    1 file changed, 27 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/riscv/lib/image.c b/arch/riscv/lib/image.c
>>>> index e8802007c446..73ebd0da3885 100644
>>>> --- a/arch/riscv/lib/image.c
>>>> +++ b/arch/riscv/lib/image.c
>>>> @@ -9,6 +9,8 @@
>>>>    #include <common.h>
>>>>    #include <mapmem.h>
>>>>    #include <errno.h>
>>>> +#include <bootm.h>
>>>> +#include <malloc.h>
>>>>    #include <linux/sizes.h>
>>>>    #include <linux/stddef.h>
>>>>    @@ -16,6 +18,8 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>      /* ASCII version of "RISCV" defined in Linux kernel */
>>>>    #define LINUX_RISCV_IMAGE_MAGIC 0x5643534952
>>>> +#define GZ_HEADER_0 0x1f
>>>> +#define GZ_HEADER_1 0x8b
>>>>      struct linux_image_h {
>>>>        uint32_t    code0;        /* Executable code */
>>>> @@ -32,9 +36,31 @@ int booti_setup(ulong image, ulong
>>>> *relocated_addr, ulong *size,
>>>>            bool force_reloc)
>>>>    {
>>>>        struct linux_image_h *lhdr;
>>>> +    uint8_t *temp;
>>>> +    void *dest;
>>>> +    ulong dest_end;
>>>> +    int ret;
>>>> +    /* TODO: Is there a way to figure out length of compressed
>>>> Image.gz ?
>>>> +     * Otherwise, set it to SYS_BOOTM_LEN which should be sufficient.
>>>> +     */
>>>
>>> Presumably this is a RFC patch then ?
>>>
>> Yeah. I am not very sure if there is a better way to determine the size.
>> Hence the comment. I am hoping somebody here would suggest something ;).
> 
> Mark the patch as RFC next time please.
> 

Sure. Sorry for not marking RFC in the first time.

>>> What happens if you have two (or more) gzip-ed files back-to-back ?
>>> Wouldn't you then decompress both ? That might lead to all kinds of
>>> problems.
>>>
>>
>> That will be catastrophic. But this was intended only for booti and the
>> expectation was that only Image.gz must be loaded before this.
> 
> That also means it's horribly fragile.
> 
>> Having said that, if we can find a reliable way of figuring out the
>> compressed file size here, we can get rid of this hack.
> 
> See below
> 
>>>> +    int len = CONFIG_SYS_BOOTM_LEN;
>>>> +
>>>> +    temp = (uint8_t *)map_sysmem(image, 0);
>>>
>>> Is the type cast really needed ?
>>>
>>
>> Just wanted to be explicit. Will remove it.
>>
>>>> -    lhdr = (struct linux_image_h *)map_sysmem(image, 0);
>>>> +    if (*(temp)  == GZ_HEADER_0 && *(temp+1) == GZ_HEADER_1) {
>>>
>>> Wrap the image in some fitImage or so contained, mark the image as gzip
>>> compressed there and all this goes away.
>>>
>>
>> Yes. FIT image parsing can be done in that way. However, the idea was
>> here to load Image.gz directly. Image.gz is default compressed Linux
>> kernel image format in RISC-V.
> 
> Sigh, and the image header is compressed as well, so there's no way to
> identify the image format, right ? And there's no decompressor, so the
> dcompressing has to be done by bootloader, which would need some sort of
> very smart way of figuring out which exact compression method is used ?
> 
Yes. Image.gz is always gunzip. So checking first two bytes is enough to 
confirm that it is a gz file.

The tricky part is length of the compressed file. I took another look at 
the gunzip implementation in U-Boot. It looks like to me that compressed 
header length just to parse the header correctly. It doesn't actually 
use the "length" to decompress. In fact, it updates the length with 
uncompressed bytes after the decompression.


Regards,
Atish