On jeu., oct. 17, 2024 at 14:07, Mattijs Korpershoek <mkorpersh...@baylibre.com> wrote:
> Hi Neil, > > On jeu., oct. 17, 2024 at 14:01, Neil Armstrong <neil.armstr...@linaro.org> > wrote: > >> On 17/10/2024 13:58, Mattijs Korpershoek wrote: >>> Hi Neil, >>> >>> On jeu., oct. 17, 2024 at 13:33, Mattijs Korpershoek >>> <mkorpersh...@baylibre.com> wrote: >>> >>>> Hi Neil, >>>> >>>> Thank you for the series. >>>> >>>> On mer., oct. 16, 2024 at 17:46, Neil Armstrong >>>> <neil.armstr...@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.armstr...@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.armstr...@linaro.org>
