[PATCH 0/3] image: android: misc fixes when using on Qualcomm platforms
neil.armstrong at linaro.org
neil.armstrong at linaro.org
Thu Oct 17 14:16:49 CEST 2024
On 17/10/2024 14:14, Mattijs Korpershoek wrote:
> On jeu., oct. 17, 2024 at 14:07, Mattijs Korpershoek <mkorpershoek at baylibre.com> wrote:
>
>> Hi Neil,
>>
>> On jeu., oct. 17, 2024 at 14:01, Neil Armstrong <neil.armstrong at 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 at baylibre.com> wrote:
>>>>
>>>>> Hi Neil,
>>>>>
>>>>> Thank you for the series.
>>>>>
>>>>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong <neil.armstrong at 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 at 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 at linaro.org>
More information about the U-Boot
mailing list