[PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Neil Armstrong]

When trying to use the Android boot image with header version 2
on recent Qualcomm platforms, we get into some troubles.

First the kernel in-place address can be > 32bit, then since
we use the Android mkbootimg, it uses the default load address
which isn't big enough to uncompress the kernel.

Finally, the ramdisk also uses a default load address, and
it should be taken in account like for the kernel address.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
Neil Armstrong (3):
      image: android: use ulong for kernel address
      boot: image-android: do not boot XIP when kernel is compressed
      image: android: handle ramdisk default address

 boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
 include/android_image.h |  2 +-
 2 files changed, 47 insertions(+), 15 deletions(-)
---
base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d

Best regards,
-- 
Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 1/3] image: android: use ulong for kernel address

[Neil Armstrong]

When booting with platforms having > 4GiB of memory,
the kernel physical address can be more than 32bits.

Use ulong like all the other addresses, and fix the
print to show the > 32bits address numbers.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 boot/image-android.c    | 4 ++--
 include/android_image.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index e74dd498a30..bb5f4f84487 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -256,7 +256,7 @@ int android_image_get_kernel(const void *hdr,
 			     ulong *os_data, ulong *os_len)
 {
 	struct andr_image_data img_data = {0};
-	u32 kernel_addr;
+	ulong kernel_addr;
 	const struct legacy_img_hdr *ihdr;
 
 	if (!android_image_get_data(hdr, vendor_boot_img, &img_data))
@@ -275,7 +275,7 @@ int android_image_get_kernel(const void *hdr,
 	if (strlen(andr_tmp_str))
 		printf("Android's image name: %s\n", andr_tmp_str);
 
-	printf("Kernel load addr 0x%08x size %u KiB\n",
+	printf("Kernel load addr 0x%08lx size %u KiB\n",
 	       kernel_addr, DIV_ROUND_UP(img_data.kernel_size, 1024));
 
 	int len = 0;
diff --git a/include/android_image.h b/include/android_image.h
index d503c980b23..96820709b42 100644
--- a/include/android_image.h
+++ b/include/android_image.h
@@ -348,7 +348,7 @@ struct andr_image_data {
 	ulong bootconfig_addr;  /* bootconfig image address */
 	ulong bootconfig_size;  /* bootconfig image size */
 
-	u32 kernel_addr;  /* physical load addr */
+	ulong kernel_addr;  /* physical load addr */
 	ulong ramdisk_addr;  /* physical load addr */
 	ulong ramdisk_ptr;  /* ramdisk address */
 	ulong dtb_load_addr;  /* physical load address for DTB image */

-- 
2.34.1

[PATCH 2/3] boot: image-android: do not boot XIP when kernel is compressed

[Neil Armstrong]

When trying to boot an android boot image with a compressed
kernel, if the kernel is used in-place because it was created
with mkbootimg, the space will be too small to properly
uncompress.

Take in account the compressed state, and if compressed
use the kernel_addr_r which should be big enough.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 boot/image-android.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index bb5f4f84487..3adcc69a392 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -208,7 +208,8 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
 	return true;
 }
 
-static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
+static ulong android_image_get_kernel_addr(struct andr_image_data *img_data,
+					   ulong comp)
 {
 	/*
 	 * All the Android tools that generate a boot.img use this
@@ -221,8 +222,11 @@ static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
 	 *
 	 * Otherwise, we will return the actual value set by the user.
 	 */
-	if (img_data->kernel_addr  == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR)
-		return img_data->kernel_ptr;
+	if (img_data->kernel_addr  == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR) {
+		if (comp == IH_COMP_NONE)
+			return img_data->kernel_ptr;
+		return env_get_ulong("kernel_addr_r", 16, 0);
+	}
 
 	/*
 	 * abootimg creates images where all load addresses are 0
@@ -258,11 +262,14 @@ int android_image_get_kernel(const void *hdr,
 	struct andr_image_data img_data = {0};
 	ulong kernel_addr;
 	const struct legacy_img_hdr *ihdr;
+	ulong comp;
 
 	if (!android_image_get_data(hdr, vendor_boot_img, &img_data))
 		return -EINVAL;
 
-	kernel_addr = android_image_get_kernel_addr(&img_data);
+	comp = android_image_get_kcomp(hdr, vendor_boot_img);
+
+	kernel_addr = android_image_get_kernel_addr(&img_data, comp);
 	ihdr = (const struct legacy_img_hdr *)img_data.kernel_ptr;
 
 	/*
@@ -359,11 +366,14 @@ ulong android_image_get_kload(const void *hdr,
 			      const void *vendor_boot_img)
 {
 	struct andr_image_data img_data;
+	ulong comp;
 
 	if (!android_image_get_data(hdr, vendor_boot_img, &img_data))
 		return -EINVAL;
 
-	return android_image_get_kernel_addr(&img_data);
+	comp = android_image_get_kcomp(hdr, vendor_boot_img);
+
+	return android_image_get_kernel_addr(&img_data, comp);
 }
 
 ulong android_image_get_kcomp(const void *hdr,

-- 
2.34.1

[PATCH 3/3] image: android: handle ramdisk default address

[Neil Armstrong]

The two tools that create android boot images, mkbootimg and the fastboot
client, set the kernel address by default to 0x11008000.

U-boot always honors this field, and will try to copy the ramdisk to
whatever value is set in the header, which won't be mapped to the actual RAM on
most platforms, resulting in the kernel obviously not booting.

All the targets in U-Boot right now will download the android boot image to
CONFIG_SYS_LOAD_ADDR, which means that it will already have been downloaded to
some location that is suitable to use the ramdisk in-place for header
version 0 to 2. For header version 3 and later, the ramdisk can't be
used in-place to use ramdisk_addr_r in this case.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 boot/image-android.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index 3adcc69a392..a261bb63999 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -14,6 +14,7 @@
 #include <linux/libfdt.h>
 
 #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR	0x10008000
+#define ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR	0x11000000
 
 static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
 
@@ -405,9 +406,24 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
 
 	if (!img_data.ramdisk_size)
 		return -ENOENT;
-
+	/*
+	 * Android tools can generate a boot.img with default load address
+	 * or 0, even though it doesn't really make a lot of sense, and it
+	 * might be valid on some platforms, we treat that address as
+	 * the default value for this field, and try to pass ramdisk
+	 * in place if possible.
+	 */
 	if (img_data.header_version > 2) {
-		ramdisk_ptr = img_data.ramdisk_addr;
+		/* Ramdisk can't be used in-place, copy it to ramdisk_addr_r */
+		if (img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
+			ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, 0);
+			if (!ramdisk_ptr) {
+				printf("Invalid ramdisk_addr_r to copy ramdisk into\n");
+				return -EINVAL;
+			}
+		} else {
+			ramdisk_ptr = img_data.ramdisk_addr;
+		}
 		memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
 		       img_data.vendor_ramdisk_size);
 		ramdisk_ptr += img_data.vendor_ramdisk_size;
@@ -420,15 +436,21 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
 			       img_data.bootconfig_size);
 		}
 	} else {
-		ramdisk_ptr = img_data.ramdisk_addr;
-		memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
-		       img_data.ramdisk_size);
+		/* Ramdisk can be used in-place, use current ptr */
+		if (img_data.ramdisk_addr == 0 ||
+		    img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
+			ramdisk_ptr = img_data.ramdisk_ptr;
+		} else {
+			ramdisk_ptr = img_data.ramdisk_addr;
+			memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
+			       img_data.ramdisk_size);
+		}
 	}
 
 	printf("RAM disk load addr 0x%08lx size %u KiB\n",
-	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
+	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
 
-	*rd_data = img_data.ramdisk_addr;
+	*rd_data = ramdisk_ptr;
 
 	*rd_len = img_data.ramdisk_size;
 	return 0;

-- 
2.34.1

Re: [PATCH 3/3] image: android: handle ramdisk default address

[Mattijs Korpershoek]

Hi Neil,

Thank you for the patch.

On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> The two tools that create android boot images, mkbootimg and the fastboot
> client, set the kernel address by default to 0x11008000.
>
> U-boot always honors this field, and will try to copy the ramdisk to
> whatever value is set in the header, which won't be mapped to the actual RAM on
> most platforms, resulting in the kernel obviously not booting.
>
> All the targets in U-Boot right now will download the android boot image to
> CONFIG_SYS_LOAD_ADDR, which means that it will already have been downloaded to
> some location that is suitable to use the ramdisk in-place for header
> version 0 to 2. For header version 3 and later, the ramdisk can't be
> used in-place to use ramdisk_addr_r in this case.

Is said in [1] I found some boot issues on Beagle Play with boot image
v4.

If a new series need to be send, please also adapt the commit message a
bit as it fails checkpatch:

$ ~/work/amlogic/u-boot/ neil/android-fixes ./scripts/checkpatch.pl --strict --u-boot --git HEAD^..HEAD
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#10: 
whatever value is set in the header, which won't be mapped to the actual RAM on

total: 0 errors, 1 warnings, 0 checks, 59 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Commit 19a75a41096f ("image: android: handle ramdisk default address") has style problems, please review.

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO ENOSYS MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.


[1] https://lore.kernel.org/r/all/87ed4f2ccc.fsf@baylibre.com/
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
>  boot/image-android.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index 3adcc69a392..a261bb63999 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -14,6 +14,7 @@
>  #include <linux/libfdt.h>
>  
>  #define ANDROID_IMAGE_DEFAULT_KERNEL_ADDR	0x10008000
> +#define ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR	0x11000000
>  
>  static char andr_tmp_str[ANDR_BOOT_ARGS_SIZE + 1];
>  
> @@ -405,9 +406,24 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>  
>  	if (!img_data.ramdisk_size)
>  		return -ENOENT;
> -
> +	/*
> +	 * Android tools can generate a boot.img with default load address
> +	 * or 0, even though it doesn't really make a lot of sense, and it
> +	 * might be valid on some platforms, we treat that address as
> +	 * the default value for this field, and try to pass ramdisk
> +	 * in place if possible.
> +	 */
>  	if (img_data.header_version > 2) {
> -		ramdisk_ptr = img_data.ramdisk_addr;
> +		/* Ramdisk can't be used in-place, copy it to ramdisk_addr_r */
> +		if (img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
> +			ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, 0);
> +			if (!ramdisk_ptr) {
> +				printf("Invalid ramdisk_addr_r to copy ramdisk into\n");
> +				return -EINVAL;
> +			}
> +		} else {
> +			ramdisk_ptr = img_data.ramdisk_addr;
> +		}
>  		memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
>  		       img_data.vendor_ramdisk_size);
>  		ramdisk_ptr += img_data.vendor_ramdisk_size;
> @@ -420,15 +436,21 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>  			       img_data.bootconfig_size);
>  		}
>  	} else {
> -		ramdisk_ptr = img_data.ramdisk_addr;
> -		memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
> -		       img_data.ramdisk_size);
> +		/* Ramdisk can be used in-place, use current ptr */
> +		if (img_data.ramdisk_addr == 0 ||
> +		    img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
> +			ramdisk_ptr = img_data.ramdisk_ptr;
> +		} else {
> +			ramdisk_ptr = img_data.ramdisk_addr;
> +			memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
> +			       img_data.ramdisk_size);
> +		}
>  	}
>  
>  	printf("RAM disk load addr 0x%08lx size %u KiB\n",
> -	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
> +	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>  
> -	*rd_data = img_data.ramdisk_addr;
> +	*rd_data = ramdisk_ptr;
>  
>  	*rd_len = img_data.ramdisk_size;
>  	return 0;
>
> -- 
> 2.34.1

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Mattijs Korpershoek]

Hi Neil,

Thank you for the series.

On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> When trying to use the Android boot image with header version 2
> on recent Qualcomm platforms, we get into some troubles.
>
> First the kernel in-place address can be > 32bit, then since
> we use the Android mkbootimg, it uses the default load address
> which isn't big enough to uncompress the kernel.
>
> Finally, the ramdisk also uses a default load address, and
> it should be taken in account like for the kernel address.
>
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> ---
> Neil Armstrong (3):
>       image: android: use ulong for kernel address
>       boot: image-android: do not boot XIP when kernel is compressed
>       image: android: handle ramdisk default address

I have boot tested aosp/main on Khadas VIM3 using
khadas_vim3_android_defconfig

This ensures that boot image v2 still works.

I also tried to boot test the Beagle Play board (which runs Android 14
with boot image v4).

Unfortunetly, that does not boot. The kernel starts but then I see:

[    0.434360][    T1] /dev/root: Can't open blockdev
[    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)

Full boot logs:
https://paste.debian.net/1332547/

Full boot logs on master:
https://paste.debian.net/1332548/

It seems that somehow, the bootconfig section is no longer present.

I'll try to identify the offending patch and help debug this.

>
>  boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>  include/android_image.h |  2 +-
>  2 files changed, 47 insertions(+), 15 deletions(-)
> ---
> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>
> Best regards,
> -- 
> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Mattijs Korpershoek]

Hi Neil,

On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Neil,
>
> Thank you for the series.
>
> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
>> When trying to use the Android boot image with header version 2
>> on recent Qualcomm platforms, we get into some troubles.
>>
>> First the kernel in-place address can be > 32bit, then since
>> we use the Android mkbootimg, it uses the default load address
>> which isn't big enough to uncompress the kernel.
>>
>> Finally, the ramdisk also uses a default load address, and
>> it should be taken in account like for the kernel address.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>> ---
>> Neil Armstrong (3):
>>       image: android: use ulong for kernel address
>>       boot: image-android: do not boot XIP when kernel is compressed
>>       image: android: handle ramdisk default address
>
> I have boot tested aosp/main on Khadas VIM3 using
> khadas_vim3_android_defconfig
>
> This ensures that boot image v2 still works.
>
> I also tried to boot test the Beagle Play board (which runs Android 14
> with boot image v4).
>
> Unfortunetly, that does not boot. The kernel starts but then I see:
>
> [    0.434360][    T1] /dev/root: Can't open blockdev
> [    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>
> Full boot logs:
> https://paste.debian.net/1332547/
>
> Full boot logs on master:
> https://paste.debian.net/1332548/
>
> It seems that somehow, the bootconfig section is no longer present.
>
> I'll try to identify the offending patch and help debug this.

Offending patch is
  [PATCH 3/3] image: android: handle ramdisk default address

The following (invalid) diff "fixes it"

modified   boot/image-android.c
@@ -448,9 +448,9 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
 	}
 
 	printf("RAM disk load addr 0x%08lx size %u KiB\n",
-	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
+	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
 
-	*rd_data = ramdisk_ptr;
+	*rd_data = img_data.ramdisk_addr;
 
 	*rd_len = img_data.ramdisk_size;
 	return 0;

I'll debug a bit more.

>
>>
>>  boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>>  include/android_image.h |  2 +-
>>  2 files changed, 47 insertions(+), 15 deletions(-)
>> ---
>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>>
>> Best regards,
>> -- 
>> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Neil Armstrong]

On 17/10/2024 13:58, Mattijs Korpershoek wrote:
> Hi Neil,
> 
> On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
> 
>> Hi Neil,
>>
>> Thank you for the series.
>>
>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>>> When trying to use the Android boot image with header version 2
>>> on recent Qualcomm platforms, we get into some troubles.
>>>
>>> First the kernel in-place address can be > 32bit, then since
>>> we use the Android mkbootimg, it uses the default load address
>>> which isn't big enough to uncompress the kernel.
>>>
>>> Finally, the ramdisk also uses a default load address, and
>>> it should be taken in account like for the kernel address.
>>>
>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> ---
>>> Neil Armstrong (3):
>>>        image: android: use ulong for kernel address
>>>        boot: image-android: do not boot XIP when kernel is compressed
>>>        image: android: handle ramdisk default address
>>
>> I have boot tested aosp/main on Khadas VIM3 using
>> khadas_vim3_android_defconfig
>>
>> This ensures that boot image v2 still works.
>>
>> I also tried to boot test the Beagle Play board (which runs Android 14
>> with boot image v4).
>>
>> Unfortunetly, that does not boot. The kernel starts but then I see:
>>
>> [    0.434360][    T1] /dev/root: Can't open blockdev
>> [    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>
>> Full boot logs:
>> https://paste.debian.net/1332547/
>>
>> Full boot logs on master:
>> https://paste.debian.net/1332548/
>>
>> It seems that somehow, the bootconfig section is no longer present.
>>
>> I'll try to identify the offending patch and help debug this.
> 
> Offending patch is
>    [PATCH 3/3] image: android: handle ramdisk default address

Thanks for looking

> 
> The following (invalid) diff "fixes it"
> 
> modified   boot/image-android.c
> @@ -448,9 +448,9 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>   	}
>   
>   	printf("RAM disk load addr 0x%08lx size %u KiB\n",
> -	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
> +	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>   
> -	*rd_data = ramdisk_ptr;
> +	*rd_data = img_data.ramdisk_addr;
>   
>   	*rd_len = img_data.ramdisk_size;
>   	return 0;
> 
> I'll debug a bit more.

OK so this basically reverts the patch, so it means on Beagle Play
the 0x11000000 is valid and can't use the randisk in-place.

img_data.ramdisk_ptr is the "real" address the data has been loaded to,
and img_data.ramdisk_addr is the address passed to mkbootimg, where it
should be loaded.

Neil

> 
>>
>>>
>>>   boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>>>   include/android_image.h |  2 +-
>>>   2 files changed, 47 insertions(+), 15 deletions(-)
>>> ---
>>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>>> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>>>
>>> Best regards,
>>> -- 
>>> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Mattijs Korpershoek]

Hi Neil,

On jeu., oct. 17, 2024 at 14:01, Neil Armstrong <neil.armstrong@linaro.org> wrote:

> On 17/10/2024 13:58, Mattijs Korpershoek wrote:
>> Hi Neil,
>> 
>> On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>> 
>>> Hi Neil,
>>>
>>> Thank you for the series.
>>>
>>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>>> When trying to use the Android boot image with header version 2
>>>> on recent Qualcomm platforms, we get into some troubles.
>>>>
>>>> First the kernel in-place address can be > 32bit, then since
>>>> we use the Android mkbootimg, it uses the default load address
>>>> which isn't big enough to uncompress the kernel.
>>>>
>>>> Finally, the ramdisk also uses a default load address, and
>>>> it should be taken in account like for the kernel address.
>>>>
>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>> ---
>>>> Neil Armstrong (3):
>>>>        image: android: use ulong for kernel address
>>>>        boot: image-android: do not boot XIP when kernel is compressed
>>>>        image: android: handle ramdisk default address
>>>
>>> I have boot tested aosp/main on Khadas VIM3 using
>>> khadas_vim3_android_defconfig
>>>
>>> This ensures that boot image v2 still works.
>>>
>>> I also tried to boot test the Beagle Play board (which runs Android 14
>>> with boot image v4).
>>>
>>> Unfortunetly, that does not boot. The kernel starts but then I see:
>>>
>>> [    0.434360][    T1] /dev/root: Can't open blockdev
>>> [    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>>
>>> Full boot logs:
>>> https://paste.debian.net/1332547/
>>>
>>> Full boot logs on master:
>>> https://paste.debian.net/1332548/
>>>
>>> It seems that somehow, the bootconfig section is no longer present.
>>>
>>> I'll try to identify the offending patch and help debug this.
>> 
>> Offending patch is
>>    [PATCH 3/3] image: android: handle ramdisk default address
>
> Thanks for looking
>
>> 
>> The following (invalid) diff "fixes it"
>> 
>> modified   boot/image-android.c
>> @@ -448,9 +448,9 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>>   	}
>>   
>>   	printf("RAM disk load addr 0x%08lx size %u KiB\n",
>> -	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>> +	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>   
>> -	*rd_data = ramdisk_ptr;
>> +	*rd_data = img_data.ramdisk_addr;
>>   
>>   	*rd_len = img_data.ramdisk_size;
>>   	return 0;
>> 
>> I'll debug a bit more.
>
> OK so this basically reverts the patch, so it means on Beagle Play
> the 0x11000000 is valid and can't use the randisk in-place.
>
> img_data.ramdisk_ptr is the "real" address the data has been loaded to,
> and img_data.ramdisk_addr is the address passed to mkbootimg, where it
> should be loaded.

Beagle Play uses boot image v4, therefore, we go through the following
code path:

	if (img_data.header_version > 2) {
		/* Ramdisk can't be used in-place, copy it to ramdisk_addr_r */
		if (img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
			ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, 0);
			if (!ramdisk_ptr) {
				printf("Invalid ramdisk_addr_r to copy ramdisk into\n");
				return -EINVAL;
			}
		} else {
			ramdisk_ptr = img_data.ramdisk_addr;
		}
		memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
		       img_data.vendor_ramdisk_size);
		ramdisk_ptr += img_data.vendor_ramdisk_size;
		memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
		       img_data.boot_ramdisk_size);
		ramdisk_ptr += img_data.boot_ramdisk_size;
		if (img_data.bootconfig_size) {
			memcpy((void *)
			       (ramdisk_ptr), (void *)img_data.bootconfig_addr,
			       img_data.bootconfig_size);
		}

We can see here, that we **increment** ramdisk_ptr.

Therefore, the following line is invalid:

    *rd_data = ramdisk_ptr;

Because ramdisk_ptr is not at the beginning of the ramdisk, but at the
beginning of bootconfig.

I think saving ramdisk_ptr in the above block should fix the issues I see.

>
> Neil
>
>> 
>>>
>>>>
>>>>   boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>>>>   include/android_image.h |  2 +-
>>>>   2 files changed, 47 insertions(+), 15 deletions(-)
>>>> ---
>>>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>>>> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>>>>
>>>> Best regards,
>>>> -- 
>>>> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[Mattijs Korpershoek]

On jeu., oct. 17, 2024 at 14:07, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:

> Hi Neil,
>
> On jeu., oct. 17, 2024 at 14:01, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
>> On 17/10/2024 13:58, Mattijs Korpershoek wrote:
>>> Hi Neil,
>>> 
>>> On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>>> 
>>>> Hi Neil,
>>>>
>>>> Thank you for the series.
>>>>
>>>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>
>>>>> When trying to use the Android boot image with header version 2
>>>>> on recent Qualcomm platforms, we get into some troubles.
>>>>>
>>>>> First the kernel in-place address can be > 32bit, then since
>>>>> we use the Android mkbootimg, it uses the default load address
>>>>> which isn't big enough to uncompress the kernel.
>>>>>
>>>>> Finally, the ramdisk also uses a default load address, and
>>>>> it should be taken in account like for the kernel address.
>>>>>
>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> ---
>>>>> Neil Armstrong (3):
>>>>>        image: android: use ulong for kernel address
>>>>>        boot: image-android: do not boot XIP when kernel is compressed
>>>>>        image: android: handle ramdisk default address
>>>>
>>>> I have boot tested aosp/main on Khadas VIM3 using
>>>> khadas_vim3_android_defconfig
>>>>
>>>> This ensures that boot image v2 still works.
>>>>
>>>> I also tried to boot test the Beagle Play board (which runs Android 14
>>>> with boot image v4).
>>>>
>>>> Unfortunetly, that does not boot. The kernel starts but then I see:
>>>>
>>>> [    0.434360][    T1] /dev/root: Can't open blockdev
>>>> [    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>>>
>>>> Full boot logs:
>>>> https://paste.debian.net/1332547/
>>>>
>>>> Full boot logs on master:
>>>> https://paste.debian.net/1332548/
>>>>
>>>> It seems that somehow, the bootconfig section is no longer present.
>>>>
>>>> I'll try to identify the offending patch and help debug this.
>>> 
>>> Offending patch is
>>>    [PATCH 3/3] image: android: handle ramdisk default address
>>
>> Thanks for looking
>>
>>> 
>>> The following (invalid) diff "fixes it"
>>> 
>>> modified   boot/image-android.c
>>> @@ -448,9 +448,9 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>>>   	}
>>>   
>>>   	printf("RAM disk load addr 0x%08lx size %u KiB\n",
>>> -	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>> +	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>>   
>>> -	*rd_data = ramdisk_ptr;
>>> +	*rd_data = img_data.ramdisk_addr;
>>>   
>>>   	*rd_len = img_data.ramdisk_size;
>>>   	return 0;
>>> 
>>> I'll debug a bit more.
>>
>> OK so this basically reverts the patch, so it means on Beagle Play
>> the 0x11000000 is valid and can't use the randisk in-place.
>>
>> img_data.ramdisk_ptr is the "real" address the data has been loaded to,
>> and img_data.ramdisk_addr is the address passed to mkbootimg, where it
>> should be loaded.
>
> Beagle Play uses boot image v4, therefore, we go through the following
> code path:
>
> 	if (img_data.header_version > 2) {
> 		/* Ramdisk can't be used in-place, copy it to ramdisk_addr_r */
> 		if (img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
> 			ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, 0);
> 			if (!ramdisk_ptr) {
> 				printf("Invalid ramdisk_addr_r to copy ramdisk into\n");
> 				return -EINVAL;
> 			}
> 		} else {
> 			ramdisk_ptr = img_data.ramdisk_addr;
> 		}
> 		memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
> 		       img_data.vendor_ramdisk_size);
> 		ramdisk_ptr += img_data.vendor_ramdisk_size;
> 		memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
> 		       img_data.boot_ramdisk_size);
> 		ramdisk_ptr += img_data.boot_ramdisk_size;
> 		if (img_data.bootconfig_size) {
> 			memcpy((void *)
> 			       (ramdisk_ptr), (void *)img_data.bootconfig_addr,
> 			       img_data.bootconfig_size);
> 		}
>
> We can see here, that we **increment** ramdisk_ptr.
>
> Therefore, the following line is invalid:
>
>     *rd_data = ramdisk_ptr;
>
> Because ramdisk_ptr is not at the beginning of the ramdisk, but at the
> beginning of bootconfig.
>
> I think saving ramdisk_ptr in the above block should fix the issues I see.

The following diff fixes the issue I see on Beagle Play with boot image
v4:

diff --git a/boot/image-android.c b/boot/image-android.c
index a261bb639990..e9d898e003f6 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -424,6 +424,7 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
                } else {
                        ramdisk_ptr = img_data.ramdisk_addr;
                }
+               ulong ramdisk_begin_ptr = ramdisk_ptr;
                memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
                       img_data.vendor_ramdisk_size);
                ramdisk_ptr += img_data.vendor_ramdisk_size;
@@ -435,6 +436,11 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
                               (ramdisk_ptr), (void *)img_data.bootconfig_addr,
                               img_data.bootconfig_size);
                }
+               /*
+                * Since we moved ramdisk_ptr, restore it back to the beginning
+                * of the ramdisk
+                */
+               ramdisk_ptr = ramdisk_begin_ptr;
        } else {
                /* Ramdisk can be used in-place, use current ptr */
                if (img_data.ramdisk_addr == 0 ||

(it's not super clean, but the general idea should work)
Can you add something similar for v2?

>
>>
>> Neil
>>
>>> 
>>>>
>>>>>
>>>>>   boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>>>>>   include/android_image.h |  2 +-
>>>>>   2 files changed, 47 insertions(+), 15 deletions(-)
>>>>> ---
>>>>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>>>>> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>>>>>
>>>>> Best regards,
>>>>> -- 
>>>>> Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms

[neil.armstrong]

On 17/10/2024 14:14, Mattijs Korpershoek wrote:
> On jeu., oct. 17, 2024 at 14:07, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
> 
>> Hi Neil,
>>
>> On jeu., oct. 17, 2024 at 14:01, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>>> On 17/10/2024 13:58, Mattijs Korpershoek wrote:
>>>> Hi Neil,
>>>>
>>>> On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek <mkorpershoek@baylibre.com> wrote:
>>>>
>>>>> Hi Neil,
>>>>>
>>>>> Thank you for the series.
>>>>>
>>>>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>>> When trying to use the Android boot image with header version 2
>>>>>> on recent Qualcomm platforms, we get into some troubles.
>>>>>>
>>>>>> First the kernel in-place address can be > 32bit, then since
>>>>>> we use the Android mkbootimg, it uses the default load address
>>>>>> which isn't big enough to uncompress the kernel.
>>>>>>
>>>>>> Finally, the ramdisk also uses a default load address, and
>>>>>> it should be taken in account like for the kernel address.
>>>>>>
>>>>>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>> ---
>>>>>> Neil Armstrong (3):
>>>>>>         image: android: use ulong for kernel address
>>>>>>         boot: image-android: do not boot XIP when kernel is compressed
>>>>>>         image: android: handle ramdisk default address
>>>>>
>>>>> I have boot tested aosp/main on Khadas VIM3 using
>>>>> khadas_vim3_android_defconfig
>>>>>
>>>>> This ensures that boot image v2 still works.
>>>>>
>>>>> I also tried to boot test the Beagle Play board (which runs Android 14
>>>>> with boot image v4).
>>>>>
>>>>> Unfortunetly, that does not boot. The kernel starts but then I see:
>>>>>
>>>>> [    0.434360][    T1] /dev/root: Can't open blockdev
>>>>> [    0.439587][    T1] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(0,0)
>>>>>
>>>>> Full boot logs:
>>>>> https://paste.debian.net/1332547/
>>>>>
>>>>> Full boot logs on master:
>>>>> https://paste.debian.net/1332548/
>>>>>
>>>>> It seems that somehow, the bootconfig section is no longer present.
>>>>>
>>>>> I'll try to identify the offending patch and help debug this.
>>>>
>>>> Offending patch is
>>>>     [PATCH 3/3] image: android: handle ramdisk default address
>>>
>>> Thanks for looking
>>>
>>>>
>>>> The following (invalid) diff "fixes it"
>>>>
>>>> modified   boot/image-android.c
>>>> @@ -448,9 +448,9 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>>>>    	}
>>>>    
>>>>    	printf("RAM disk load addr 0x%08lx size %u KiB\n",
>>>> -	       ramdisk_ptr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>>> +	       img_data.ramdisk_addr, DIV_ROUND_UP(img_data.ramdisk_size, 1024));
>>>>    
>>>> -	*rd_data = ramdisk_ptr;
>>>> +	*rd_data = img_data.ramdisk_addr;
>>>>    
>>>>    	*rd_len = img_data.ramdisk_size;
>>>>    	return 0;
>>>>
>>>> I'll debug a bit more.
>>>
>>> OK so this basically reverts the patch, so it means on Beagle Play
>>> the 0x11000000 is valid and can't use the randisk in-place.
>>>
>>> img_data.ramdisk_ptr is the "real" address the data has been loaded to,
>>> and img_data.ramdisk_addr is the address passed to mkbootimg, where it
>>> should be loaded.
>>
>> Beagle Play uses boot image v4, therefore, we go through the following
>> code path:
>>
>> 	if (img_data.header_version > 2) {
>> 		/* Ramdisk can't be used in-place, copy it to ramdisk_addr_r */
>> 		if (img_data.ramdisk_addr == ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) {
>> 			ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, 0);
>> 			if (!ramdisk_ptr) {
>> 				printf("Invalid ramdisk_addr_r to copy ramdisk into\n");
>> 				return -EINVAL;
>> 			}
>> 		} else {
>> 			ramdisk_ptr = img_data.ramdisk_addr;
>> 		}
>> 		memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
>> 		       img_data.vendor_ramdisk_size);
>> 		ramdisk_ptr += img_data.vendor_ramdisk_size;
>> 		memcpy((void *)(ramdisk_ptr), (void *)img_data.ramdisk_ptr,
>> 		       img_data.boot_ramdisk_size);
>> 		ramdisk_ptr += img_data.boot_ramdisk_size;
>> 		if (img_data.bootconfig_size) {
>> 			memcpy((void *)
>> 			       (ramdisk_ptr), (void *)img_data.bootconfig_addr,
>> 			       img_data.bootconfig_size);
>> 		}
>>
>> We can see here, that we **increment** ramdisk_ptr.
>>
>> Therefore, the following line is invalid:
>>
>>      *rd_data = ramdisk_ptr;
>>
>> Because ramdisk_ptr is not at the beginning of the ramdisk, but at the
>> beginning of bootconfig.
>>
>> I think saving ramdisk_ptr in the above block should fix the issues I see.
> 
> The following diff fixes the issue I see on Beagle Play with boot image
> v4:
> 
> diff --git a/boot/image-android.c b/boot/image-android.c
> index a261bb639990..e9d898e003f6 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -424,6 +424,7 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>                  } else {
>                          ramdisk_ptr = img_data.ramdisk_addr;
>                  }
> +               ulong ramdisk_begin_ptr = ramdisk_ptr;
>                  memcpy((void *)(ramdisk_ptr), (void *)img_data.vendor_ramdisk_ptr,
>                         img_data.vendor_ramdisk_size);
>                  ramdisk_ptr += img_data.vendor_ramdisk_size;
> @@ -435,6 +436,11 @@ int android_image_get_ramdisk(const void *hdr, const void *vendor_boot_img,
>                                 (ramdisk_ptr), (void *)img_data.bootconfig_addr,
>                                 img_data.bootconfig_size);
>                  }
> +               /*
> +                * Since we moved ramdisk_ptr, restore it back to the beginning
> +                * of the ramdisk
> +                */
> +               ramdisk_ptr = ramdisk_begin_ptr;
>          } else {
>                  /* Ramdisk can be used in-place, use current ptr */
>                  if (img_data.ramdisk_addr == 0 ||
> 
> (it's not super clean, but the general idea should work)
> Can you add something similar for v2?

Neat, I'll try to make it cleaner but I get the idea :-)

Thanks!

Neil

> 
>>
>>>
>>> Neil
>>>
>>>>
>>>>>
>>>>>>
>>>>>>    boot/image-android.c    | 60 +++++++++++++++++++++++++++++++++++++------------
>>>>>>    include/android_image.h |  2 +-
>>>>>>    2 files changed, 47 insertions(+), 15 deletions(-)
>>>>>> ---
>>>>>> base-commit: d5cab0d6adc26ec1bbd45c2fed101184d04454ae
>>>>>> change-id: 20241016-topic-fastboot-fixes-mkbootimg-8d73ab93db3d
>>>>>>
>>>>>> Best regards,
>>>>>> -- 
>>>>>> Neil Armstrong <neil.armstrong@linaro.org>