Hi George, On jeu., mai 08, 2025 at 12:22, george chan <[email protected]> wrote:
> Hi Mattijs, > > Thx for feedback. > > 在 2025年5月7日週三 15:47，Mattijs Korpershoek <[email protected]> 寫道： > >> Hi George, >> >> Thank you for the patch. >> I really prefer this solution to the one proposed in v1. >> >> I have tested this on Khadas VIM3 board (using >> khadas-vim3_android_defconfig) >> >> I have a couple of small remarks below. >> >> On lun., mai 05, 2025 at 17:17, George Chan via B4 Relay < >> [email protected]> wrote: >> >> > From: George Chan <[email protected]> >> > >> > Some androidboot image have invalid kernel/ramdisk load addr, >> > force to ignore those value and use loadaddr instead. >> >> Can we please elaborate a bit more in the commit message? >> We have not explained/justified why repacking a boot image with proper >> kernel ramdisk/load addr is not possible. >> >> Is the following an acceptable justification for you? >> """ >> Since it's not always possible to change the load addr by repacking >> the boot.img (due to AVB signature mismatch/<or_another_justification>), >> we need a way to use kernel_addr_r and ramdisk_addr_r. >> """ >> >> Feel free to reformulate the above. >> > > Thx. let me sum up as below 2 reasons. > > 1. there is a concern on exposing the whole memory to image loading is > dangerous > > 2. Since it's not always possible to change the load addr by repacking > the boot.img (mainly due to AVB signature mismatch), > we need a way to use kernel_addr_r and ramdisk_addr_r. That's good summary. Please include it in the commit message for v3. > > >> > >> > Suggested-by: Casey Connolly <[email protected]> >> > Signed-off-by: George Chan <[email protected]> >> > --- >> > boot/Kconfig | 6 ++++++ >> > boot/image-android.c | 9 ++++++--- >> > 2 files changed, 12 insertions(+), 3 deletions(-) >> > >> > diff --git a/boot/Kconfig b/boot/Kconfig >> > index fb37d912bc9..4bdac384181 100644 >> > --- a/boot/Kconfig >> > +++ b/boot/Kconfig >> > @@ -11,6 +11,12 @@ config ANDROID_BOOT_IMAGE >> > This enables support for booting images which use the Android >> > image format header. >> > >> > +config ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR >> > + bool "Android Boot Image ignore addr" >> > + default n >> > + help >> > + This ignore kernel/ramdisk load addr specified in androidboot >> header. >> >> Please add a bit more context here, checkpatch.pl reports issues: >> $ ./scripts/checkpatch.pl --u-boot --git master..HEAD >> >> WARNING: please write a paragraph that describes the config symbol fully >> #27: FILE: boot/Kconfig:14: >> +config ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR >> > > same as above. > >> >> > + >> > config TIMESTAMP >> > bool "Show image date and time when displaying image information" >> > default y if CMD_DATE >> > diff --git a/boot/image-android.c b/boot/image-android.c >> > index 1746b018900..7b8eb6a4f64 100644 >> > --- a/boot/image-android.c >> > +++ b/boot/image-android.c >> > @@ -268,7 +268,8 @@ 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) { >> > + if (img_data->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR || >> > + IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR)) { >> >> Please fix indentation, should be: >> >> + if (img_data->kernel_addr == ANDROID_IMAGE_DEFAULT_KERNEL_ADDR || >> + IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR)) { >> >> (checkpatch.pl reports this) >> > > ack. > > >> > if (comp == IH_COMP_NONE) >> > return img_data->kernel_ptr; >> > return env_get_ulong("kernel_addr_r", 16, 0); >> > @@ -464,7 +465,8 @@ int android_image_get_ramdisk(const void *hdr, const >> void *vendor_boot_img, >> > */ >> > 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) { >> > + if (img_data.ramdisk_addr == >> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR || >> > + >> (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR))) { >> >> Same here. Also, can we please drop the leading parenthesis (IS_ENABLED( >> -> IS_ENABLED( to be consistent with the previous code change? >> > > ack. > > >> > ramdisk_ptr = env_get_ulong("ramdisk_addr_r", 16, >> 0); >> > if (!ramdisk_ptr) { >> > printf("Invalid ramdisk_addr_r to copy >> ramdisk into\n"); >> > @@ -488,7 +490,8 @@ int android_image_get_ramdisk(const void *hdr, const >> void *vendor_boot_img, >> > } else { >> > /* Ramdisk can be used in-place, use current ptr */ >> > if (img_data.ramdisk_addr == 0 || >> > - img_data.ramdisk_addr == >> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR) { >> > + img_data.ramdisk_addr == >> ANDROID_IMAGE_DEFAULT_RAMDISK_ADDR || >> > + >> (IS_ENABLED(CONFIG_ANDROID_BOOT_IMAGE_IGNORE_BLOB_ADDR))) { >> >> Same here. Also, can we please drop the leading parenthesis (IS_ENABLED( >> -> IS_ENABLED( to be consistent with the previous code change? >> > > ack. > > l planed to release v3 later around end of next week and contain only patch > #1 #2 and #5 to ease maintainer processing. please feel free to comment on > any idea, i will include into next version. > > regards, > George > >>
