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

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

[Eugeniu Rosca]

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.

Tested on H3ULCB-KF using the image detailed in [2].

[1] Excerpt from include/android_image.h
    +-----------------+
    | boot header     | 1 page
    +-----------------+
    | kernel          | n pages
    +-----------------+
    | ramdisk         | m pages
    +-----------------+
    | second stage    | o pages
    +-----------------+

[2] => iminfo 4c000000
    ## Checking Image at 4c000000 ...
    Android image found
    kernel size:      85b9d1
    kernel address:   48080000
    ramdisk size:     54ddbc
    ramdisk addrress: 4a180000
    second size:      0
    second address:   48000800
    tags address:     48000100
    page size:        800
    os_version:       1200012a (ver: 0.9.0, level: 2018.10)
    name:
    cmdline:          buildvariant=userdebug

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/bootm.c         |  2 +-
 common/image-android.c | 14 ++++++++++++++
 include/image.h        |  1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/common/bootm.c b/common/bootm.c
index 3adbceaa38e3..bbae66df1001 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -154,7 +154,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
 #ifdef CONFIG_ANDROID_BOOT_IMAGE
 	case IMAGE_FORMAT_ANDROID:
 		images.os.type = IH_TYPE_KERNEL;
-		images.os.comp = IH_COMP_NONE;
+		images.os.comp = android_image_get_kcomp(os_hdr);
 		images.os.os = IH_OS_LINUX;
 
 		images.os.end = android_image_get_end(os_hdr);
diff --git a/common/image-android.c b/common/image-android.c
index 2f38c191e911..eec8c4decdbd 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -126,6 +126,20 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr)
 	return android_image_get_kernel_addr(hdr);
 }
 
+#define LZ4F_MAGIC 0x184D2204
+
+ulong android_image_get_kcomp(const struct andr_img_hdr *hdr)
+{
+	u32 *magic = (u32 *)((ulong)hdr + hdr->page_size);
+
+	switch (le32_to_cpu(*magic)) {
+	case LZ4F_MAGIC:
+		return IH_COMP_LZ4;
+	default:
+		return IH_COMP_NONE;
+	}
+}
+
 int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
 			      ulong *rd_data, ulong *rd_len)
 {
diff --git a/include/image.h b/include/image.h
index 765ffecee0a7..572034afd8c3 100644
--- a/include/image.h
+++ b/include/image.h
@@ -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);
 void android_print_contents(const struct andr_img_hdr *hdr);
 
 #endif /* CONFIG_ANDROID_BOOT_IMAGE */
-- 
2.21.0

[U-Boot] [PATCH 2/2] image: android: fix 'iminfo' typo

[Eugeniu Rosca]

Fix below CP warning triggered by the 'iminfo' output in another patch:
WARNING: 'addrress' may be misspelled - perhaps 'address'?

Fixes: 4f1318b29c7a20 ("common: image: minimal android image iminfo support")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
 common/image-android.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/image-android.c b/common/image-android.c
index eec8c4decdbd..3e9afd4dcfa8 100644
--- a/common/image-android.c
+++ b/common/image-android.c
@@ -200,7 +200,7 @@ void android_print_contents(const struct andr_img_hdr *hdr)
 	printf("%skernel size:      %x\n", p, hdr->kernel_size);
 	printf("%skernel address:   %x\n", p, hdr->kernel_addr);
 	printf("%sramdisk size:     %x\n", p, hdr->ramdisk_size);
-	printf("%sramdisk addrress: %x\n", p, hdr->ramdisk_addr);
+	printf("%sramdisk address:  %x\n", p, hdr->ramdisk_addr);
 	printf("%ssecond size:      %x\n", p, hdr->second_size);
 	printf("%ssecond address:   %x\n", p, hdr->second_addr);
 	printf("%stags address:     %x\n", p, hdr->tags_addr);
-- 
2.21.0

[U-Boot] [PATCH 2/2] image: android: fix 'iminfo' typo

[Marek Vasut]

On 4/3/19 11:35 PM, Eugeniu Rosca wrote:
> Fix below CP warning triggered by the 'iminfo' output in another patch:
> WARNING: 'addrress' may be misspelled - perhaps 'address'?
> 
> Fixes: 4f1318b29c7a20 ("common: image: minimal android image iminfo support")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

Acked-by: Marek Vasut <marek.vasut@gmail.com>

> ---
>  common/image-android.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/common/image-android.c b/common/image-android.c
> index eec8c4decdbd..3e9afd4dcfa8 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -200,7 +200,7 @@ void android_print_contents(const struct andr_img_hdr *hdr)
>  	printf("%skernel size:      %x\n", p, hdr->kernel_size);
>  	printf("%skernel address:   %x\n", p, hdr->kernel_addr);
>  	printf("%sramdisk size:     %x\n", p, hdr->ramdisk_size);
> -	printf("%sramdisk addrress: %x\n", p, hdr->ramdisk_addr);
> +	printf("%sramdisk address:  %x\n", p, hdr->ramdisk_addr);
>  	printf("%ssecond size:      %x\n", p, hdr->second_size);
>  	printf("%ssecond address:   %x\n", p, hdr->second_addr);
>  	printf("%stags address:     %x\n", p, hdr->tags_addr);
> 


-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

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.
> 
> Tested on H3ULCB-KF using the image detailed in [2].
> 
> [1] Excerpt from include/android_image.h
>     +-----------------+
>     | boot header     | 1 page
>     +-----------------+
>     | kernel          | n pages
>     +-----------------+
>     | ramdisk         | m pages
>     +-----------------+
>     | second stage    | o pages
>     +-----------------+
> 
> [2] => iminfo 4c000000
>     ## Checking Image at 4c000000 ...
>     Android image found
>     kernel size:      85b9d1
>     kernel address:   48080000
>     ramdisk size:     54ddbc
>     ramdisk addrress: 4a180000
>     second size:      0
>     second address:   48000800
>     tags address:     48000100
>     page size:        800
>     os_version:       1200012a (ver: 0.9.0, level: 2018.10)
>     name:
>     cmdline:          buildvariant=userdebug
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
>  common/bootm.c         |  2 +-
>  common/image-android.c | 14 ++++++++++++++
>  include/image.h        |  1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/common/bootm.c b/common/bootm.c
> index 3adbceaa38e3..bbae66df1001 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -154,7 +154,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int argc,
>  #ifdef CONFIG_ANDROID_BOOT_IMAGE
>  	case IMAGE_FORMAT_ANDROID:
>  		images.os.type = IH_TYPE_KERNEL;
> -		images.os.comp = IH_COMP_NONE;
> +		images.os.comp = android_image_get_kcomp(os_hdr);
>  		images.os.os = IH_OS_LINUX;
>  
>  		images.os.end = android_image_get_end(os_hdr);
> diff --git a/common/image-android.c b/common/image-android.c
> index 2f38c191e911..eec8c4decdbd 100644
> --- a/common/image-android.c
> +++ b/common/image-android.c
> @@ -126,6 +126,20 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr)
>  	return android_image_get_kernel_addr(hdr);
>  }
>  
> +#define LZ4F_MAGIC 0x184D2204

Don't we already have this magic in some common header ?

> +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) and uintptr_t to cater for both 32 and 64bit pointers.

> +	switch (le32_to_cpu(*magic)) {
> +	case LZ4F_MAGIC:
> +		return IH_COMP_LZ4;
> +	default:
> +		return IH_COMP_NONE;
> +	}
> +}
> +
>  int android_image_get_ramdisk(const struct andr_img_hdr *hdr,
>  			      ulong *rd_data, ulong *rd_len)
>  {
> diff --git a/include/image.h b/include/image.h
> index 765ffecee0a7..572034afd8c3 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -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);
>  void android_print_contents(const struct andr_img_hdr *hdr);
>  
>  #endif /* CONFIG_ANDROID_BOOT_IMAGE */
> 


-- 
Best regards,
Marek Vasut

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

[Eugeniu Rosca]

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?

> > +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).

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?

> >  void android_print_contents(const struct andr_img_hdr *hdr);
> >  
> >  #endif /* CONFIG_ANDROID_BOOT_IMAGE */
> > 
> 
> 
> -- 
> Best regards,
> Marek Vasut

Best regards,
Eugeniu.

[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

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

[Eugeniu Rosca]

Superseded by https://patchwork.ozlabs.org/patch/1081176/
("[U-Boot,v2,1/2] image: android: allow booting lz4-compressed kernels")