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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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>