[U-Boot] [PATCH 1/2] image: android: allow booting lz4-compressed kernels

[Marek Vasut]

On 4/4/19 1:14 PM, Eugeniu Rosca wrote:
> Hi Marek and thanks for your swift comment,
> 
> On Thu, Apr 04, 2019 at 03:39:30AM +0200, Marek Vasut wrote:
>> On 4/3/19 11:35 PM, Eugeniu Rosca wrote:
>>> According to Android image format [1], kernel image resides at 1 page
>>> offset from the boot image address. Grab the magic number from there
>>> and allow U-Boot to handle LZ4-compressed KNL binaries instead of
>>> hardcoding compression type to IH_COMP_NONE. Other compression types,
>>> if needed, can be added later.
>>>
> 
> [..]
> 
>>>  
>>> +#define LZ4F_MAGIC 0x184D2204
>>
>> Don't we already have this magic in some common header ?
> 
> Unfortunately not. It is present in lib/lz4_wrapper.c only.
> Would it be OK to relocate it to include/image.h?

Yes

>>> +ulong android_image_get_kcomp(const struct andr_img_hdr *hdr)
>>> +{
>>> +	u32 *magic = (u32 *)((ulong)hdr + hdr->page_size);
>>
>> Should this be get_unaligned((uintptr_t)hdr + hdr->page_size) ?
>> get_unaligned() because the image may be at unaligned address (although
>> that's unlikely) [..]
> 
> Just out of curiosity I've copied the Android image to 0x4c000001
> instead of 0x4c000000 in RAM and by calling 'bootm 0x4c000001', the
> compression type is still correctly identified and OS boots properly
> (w/o get_unaligned).

It will work on arm64, other platforms might fail.

> But that's because the data cache is enabled. Booting from 0x4c000001
> after calling `dcache off` no longer works and generates a
> "Synchronous Abort".
> 
> Actually having dcache enabled is a requirement for LZ4, since it
> heavily relies on unaligned memory access and produces the same data
> abort in case dcache is turned off (even if it is passed the image at
> a properly aligned location in RAM).
> 
> So, bottom line, even if we use get_unaligned() here, the LZ4 kernel
> still won't boot with data cache disabled. Anyway, I agree to use an
> alignment-aware primitive here, as you suggested.
> 
>> and uintptr_t to cater for both 32 and 64bit pointers.
> 
> Worked for me.
> 
> [..]
> 
>>> @@ -1312,6 +1312,7 @@ int android_image_get_second(const struct andr_img_hdr *hdr,
>>>  			      ulong *second_data, ulong *second_len);
>>>  ulong android_image_get_end(const struct andr_img_hdr *hdr);
>>>  ulong android_image_get_kload(const struct andr_img_hdr *hdr);
>>> +ulong android_image_get_kcomp(const struct andr_img_hdr *hdr);
> 
> Would you like ulong/int here?

ulong to keep it consistent ?

-- 
Best regards,
Marek Vasut