mbox series

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

Message ID 20241016-topic-fastboot-fixes-mkbootimg-v1-0-94fd9340722b@linaro.org
Headers show
Series image: android: misc fixes when using on Qualcomm platforms | expand

Message

Neil Armstrong Oct. 16, 2024, 3:46 p.m. UTC 
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,

Comments

Mattijs Korpershoek Oct. 17, 2024, 11:33 a.m. UTC | #1 
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>
Mattijs Korpershoek Oct. 17, 2024, 11:58 a.m. UTC | #2 
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>
Neil Armstrong Oct. 17, 2024, 12:01 p.m. UTC | #3 
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>
Mattijs Korpershoek Oct. 17, 2024, 12:07 p.m. UTC | #4 
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>
Mattijs Korpershoek Oct. 17, 2024, 12:14 p.m. UTC | #5 
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>
Neil Armstrong Oct. 17, 2024, 12:16 p.m. UTC | #6 
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>