| Message ID | 20240519125337.2557883-1-r.stratiienko@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | abootimg: Add init_boot image support | expand |
Hi Roman, Thank you for the patch. On dim., mai 19, 2024 at 12:53, Roman Stratiienko <r.stratiienko@gmail.com> wrote: > Quote from [1]: > > "For devices launching with Android 13, the generic ramdisk is removed > from the boot image and placed in a separate init_boot image. > This change leaves the boot image with only the GKI kernel." > > [1]: https://source.android.com/docs/core/architecture/partitions/generic-boot > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> > --- > boot/image-board.c | 5 ++++- > cmd/abootimg.c | 26 +++++++++++++++++++++----- > include/image.h | 7 +++++++ > 3 files changed, 32 insertions(+), 6 deletions(-) This patch does not apply on: - next: 7d24c3e06fa9 ("Merge patch series "scripts/setlocalversion sync with linux 6.9"") - master: a7f0154c4128 ("Prepare v2024.07-rc3") Please consider rebasing when resending. More review below > > diff --git a/boot/image-board.c b/boot/image-board.c > index 75f6906cd5..715654ff01 100644 > --- a/boot/image-board.c > +++ b/boot/image-board.c > @@ -414,9 +414,12 @@ static int select_ramdisk(struct bootm_headers *images, const char *select, u8 a > int ret; > if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) { > void *boot_img = map_sysmem(get_abootimg_addr(), 0); > + void *init_boot_img = map_sysmem(get_ainit_bootimg_addr(), 0); > void *vendor_boot_img = map_sysmem(get_avendor_bootimg_addr(), 0); > + void *ramdisk_img = (init_boot_img == -1) ? boot_img : > + init_boot_img; This line introduces a build warning: ../boot/image-board.c: In function ‘select_ramdisk’: ../boot/image-board.c:412:68: warning: comparison between pointer and integer 412 | void *ramdisk_img = (init_boot_img == -1) ? boot_img : | ^~ Can we please fix it in v2 ? > > - ret = android_image_get_ramdisk(boot_img, vendor_boot_img, > + ret = android_image_get_ramdisk(ramdisk_img, vendor_boot_img, > rd_datap, rd_lenp); > unmap_sysmem(vendor_boot_img); Can we please add unmap_sysmem(init_boot_img); here as well ? > unmap_sysmem(boot_img); > diff --git a/cmd/abootimg.c b/cmd/abootimg.c > index e68c523705..7c450a3b2d 100644 > --- a/cmd/abootimg.c > +++ b/cmd/abootimg.c > @@ -17,6 +17,7 @@ > > /* Please use abootimg_addr() macro to obtain the boot image address */ > static ulong _abootimg_addr = -1; > +static ulong _ainit_bootimg_addr = -1; > static ulong _avendor_bootimg_addr = -1; > > ulong get_abootimg_addr(void) > @@ -24,6 +25,11 @@ ulong get_abootimg_addr(void) > return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr); > } > > +ulong get_ainit_bootimg_addr(void) > +{ > + return _ainit_bootimg_addr; > +} > + > ulong get_avendor_bootimg_addr(void) > { > return _avendor_bootimg_addr; > @@ -209,7 +215,7 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, > char *endp; > ulong img_addr; > > - if (argc < 2 || argc > 3) > + if (argc < 2 || argc > 4) > return CMD_RET_USAGE; > > img_addr = hextoul(argv[1], &endp); > @@ -220,16 +226,26 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, > > _abootimg_addr = img_addr; > > - if (argc == 3) { > + if (argc > 2) { > img_addr = simple_strtoul(argv[2], &endp, 16); > if (*endp != '\0') { > - printf("Error: Wrong vendor image address\n"); > + printf("Error: Wrong vendor_boot image address\n"); Nitpick: this seems like a harmless change but could be done in a separate patch. To me, it's fine to include this hunk, but can we mention it in the commit message? Something along the lines as "While at it, update wrong error handling message when vendor_boot cannot be loaded". > return CMD_RET_FAILURE; > } > > _avendor_bootimg_addr = img_addr; > } > > + if (argc == 4) { > + img_addr = simple_strtoul(argv[3], &endp, 16); > + if (*endp != '\0') { > + printf("Error: Wrong init_boot image address\n"); > + return CMD_RET_FAILURE; > + } > + > + _ainit_bootimg_addr = img_addr; > + } > + > return CMD_RET_SUCCESS; > } > > @@ -346,7 +362,7 @@ fail: > } > > static struct cmd_tbl cmd_abootimg_sub[] = { > - U_BOOT_CMD_MKENT(addr, 3, 1, do_abootimg_addr, "", ""), > + U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""), > U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""), > U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""), > U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""), > @@ -375,7 +391,7 @@ static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc, > U_BOOT_CMD( > abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg, > "manipulate Android Boot Image", > - "addr <boot_img_addr> [<vendor_boot_img_addr>]>\n" > + "addr <boot_img_addr> [<vendor_boot_img_addr> [<init_boot_img_addr>]]\n" > " - set the address in RAM where boot image is located\n" > " ($loadaddr is used by default)\n" Please include the help text update in the documentation: doc/android/boot-image.rst > "abootimg dump dtb\n" > diff --git a/include/image.h b/include/image.h > index be3c73a72e..b1fd6b3149 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -1982,6 +1982,13 @@ bool is_android_vendor_boot_image_header(const void *vendor_boot_img); > */ > ulong get_abootimg_addr(void); > > +/** > + * get_ainit_bootimg_addr() - Get Android init boot image address > + * > + * Return: Android init boot image address > + */ > +ulong get_ainit_bootimg_addr(void); > + > /** > * get_avendor_bootimg_addr() - Get Android vendor boot image address > * > -- > 2.40.1
Hi Mattijs, Thank you for your review. ср, 22 мая 2024 г. в 17:43, Mattijs Korpershoek <mkorpershoek@baylibre.com>: > > Hi Roman, > > Thank you for the patch. > > On dim., mai 19, 2024 at 12:53, Roman Stratiienko <r.stratiienko@gmail.com> wrote: > > > Quote from [1]: > > > > "For devices launching with Android 13, the generic ramdisk is removed > > from the boot image and placed in a separate init_boot image. > > This change leaves the boot image with only the GKI kernel." > > > > [1]: https://source.android.com/docs/core/architecture/partitions/generic-boot > > Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> > > --- > > boot/image-board.c | 5 ++++- > > cmd/abootimg.c | 26 +++++++++++++++++++++----- > > include/image.h | 7 +++++++ > > 3 files changed, 32 insertions(+), 6 deletions(-) > > This patch does not apply on: > - next: 7d24c3e06fa9 ("Merge patch series "scripts/setlocalversion sync with linux 6.9"") > - master: a7f0154c4128 ("Prepare v2024.07-rc3") > > Please consider rebasing when resending. V2 Rebased > > More review below > > > > > diff --git a/boot/image-board.c b/boot/image-board.c > > index 75f6906cd5..715654ff01 100644 > > --- a/boot/image-board.c > > +++ b/boot/image-board.c > > @@ -414,9 +414,12 @@ static int select_ramdisk(struct bootm_headers *images, const char *select, u8 a > > int ret; > > if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) { > > void *boot_img = map_sysmem(get_abootimg_addr(), 0); > > + void *init_boot_img = map_sysmem(get_ainit_bootimg_addr(), 0); > > void *vendor_boot_img = map_sysmem(get_avendor_bootimg_addr(), 0); > > + void *ramdisk_img = (init_boot_img == -1) ? boot_img : > > + init_boot_img; > > This line introduces a build warning: > > ../boot/image-board.c: In function ‘select_ramdisk’: > ../boot/image-board.c:412:68: warning: comparison between pointer and integer > 412 | void *ramdisk_img = (init_boot_img == -1) ? boot_img : > | ^~ > > Can we please fix it in v2 ? Fixed > > > > > - ret = android_image_get_ramdisk(boot_img, vendor_boot_img, > > + ret = android_image_get_ramdisk(ramdisk_img, vendor_boot_img, > > rd_datap, rd_lenp); > > unmap_sysmem(vendor_boot_img); > > Can we please add unmap_sysmem(init_boot_img); here as well ? Done. > > > unmap_sysmem(boot_img); > > diff --git a/cmd/abootimg.c b/cmd/abootimg.c > > index e68c523705..7c450a3b2d 100644 > > --- a/cmd/abootimg.c > > +++ b/cmd/abootimg.c > > @@ -17,6 +17,7 @@ > > > > /* Please use abootimg_addr() macro to obtain the boot image address */ > > static ulong _abootimg_addr = -1; > > +static ulong _ainit_bootimg_addr = -1; > > static ulong _avendor_bootimg_addr = -1; > > > > ulong get_abootimg_addr(void) > > @@ -24,6 +25,11 @@ ulong get_abootimg_addr(void) > > return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr); > > } > > > > +ulong get_ainit_bootimg_addr(void) > > +{ > > + return _ainit_bootimg_addr; > > +} > > + > > ulong get_avendor_bootimg_addr(void) > > { > > return _avendor_bootimg_addr; > > @@ -209,7 +215,7 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, > > char *endp; > > ulong img_addr; > > > > - if (argc < 2 || argc > 3) > > + if (argc < 2 || argc > 4) > > return CMD_RET_USAGE; > > > > img_addr = hextoul(argv[1], &endp); > > @@ -220,16 +226,26 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, > > > > _abootimg_addr = img_addr; > > > > - if (argc == 3) { > > + if (argc > 2) { > > img_addr = simple_strtoul(argv[2], &endp, 16); > > if (*endp != '\0') { > > - printf("Error: Wrong vendor image address\n"); > > + printf("Error: Wrong vendor_boot image address\n"); > > Nitpick: this seems like a harmless change but could be done in a > separate patch. > > To me, it's fine to include this hunk, but can we mention it in the > commit message? > > Something along the lines as "While at it, update wrong error handling > message when vendor_boot cannot be loaded". Added. > > > return CMD_RET_FAILURE; > > } > > > > _avendor_bootimg_addr = img_addr; > > } > > > > + if (argc == 4) { > > + img_addr = simple_strtoul(argv[3], &endp, 16); > > + if (*endp != '\0') { > > + printf("Error: Wrong init_boot image address\n"); > > + return CMD_RET_FAILURE; > > + } > > + > > + _ainit_bootimg_addr = img_addr; > > + } > > + > > return CMD_RET_SUCCESS; > > } > > > > @@ -346,7 +362,7 @@ fail: > > } > > > > static struct cmd_tbl cmd_abootimg_sub[] = { > > - U_BOOT_CMD_MKENT(addr, 3, 1, do_abootimg_addr, "", ""), > > + U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""), > > U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""), > > U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""), > > U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""), > > @@ -375,7 +391,7 @@ static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc, > > U_BOOT_CMD( > > abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg, > > "manipulate Android Boot Image", > > - "addr <boot_img_addr> [<vendor_boot_img_addr>]>\n" > > + "addr <boot_img_addr> [<vendor_boot_img_addr> [<init_boot_img_addr>]]\n" > > " - set the address in RAM where boot image is located\n" > > " ($loadaddr is used by default)\n" > > Please include the help text update in the documentation: > doc/android/boot-image.rst That documentation does not mention 'abootimg addr' command at the moment. I do not see an easy way to add it in a way that makes sense. > > > "abootimg dump dtb\n" > > diff --git a/include/image.h b/include/image.h > > index be3c73a72e..b1fd6b3149 100644 > > --- a/include/image.h > > +++ b/include/image.h > > @@ -1982,6 +1982,13 @@ bool is_android_vendor_boot_image_header(const void *vendor_boot_img); > > */ > > ulong get_abootimg_addr(void); > > > > +/** > > + * get_ainit_bootimg_addr() - Get Android init boot image address > > + * > > + * Return: Android init boot image address > > + */ > > +ulong get_ainit_bootimg_addr(void); > > + > > /** > > * get_avendor_bootimg_addr() - Get Android vendor boot image address > > * > > -- > > 2.40.1
Hi Roman, On jeu., mai 23, 2024 at 00:35, Roman Stratiienko <r.stratiienko@gmail.com> wrote: [...] >> > >> > static struct cmd_tbl cmd_abootimg_sub[] = { >> > - U_BOOT_CMD_MKENT(addr, 3, 1, do_abootimg_addr, "", ""), >> > + U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""), >> > U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""), >> > U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""), >> > U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""), >> > @@ -375,7 +391,7 @@ static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc, >> > U_BOOT_CMD( >> > abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg, >> > "manipulate Android Boot Image", >> > - "addr <boot_img_addr> [<vendor_boot_img_addr>]>\n" >> > + "addr <boot_img_addr> [<vendor_boot_img_addr> [<init_boot_img_addr>]]\n" >> > " - set the address in RAM where boot image is located\n" >> > " ($loadaddr is used by default)\n" >> >> Please include the help text update in the documentation: >> doc/android/boot-image.rst > > That documentation does not mention 'abootimg addr' command at the moment. > I do not see an easy way to add it in a way that makes sense. > I see. I will send a documentation update later then. >> >> > "abootimg dump dtb\n" >> > diff --git a/include/image.h b/include/image.h >> > index be3c73a72e..b1fd6b3149 100644 [...]
diff --git a/boot/image-board.c b/boot/image-board.c index 75f6906cd5..715654ff01 100644 --- a/boot/image-board.c +++ b/boot/image-board.c @@ -414,9 +414,12 @@ static int select_ramdisk(struct bootm_headers *images, const char *select, u8 a int ret; if (IS_ENABLED(CONFIG_CMD_ABOOTIMG)) { void *boot_img = map_sysmem(get_abootimg_addr(), 0); + void *init_boot_img = map_sysmem(get_ainit_bootimg_addr(), 0); void *vendor_boot_img = map_sysmem(get_avendor_bootimg_addr(), 0); + void *ramdisk_img = (init_boot_img == -1) ? boot_img : + init_boot_img; - ret = android_image_get_ramdisk(boot_img, vendor_boot_img, + ret = android_image_get_ramdisk(ramdisk_img, vendor_boot_img, rd_datap, rd_lenp); unmap_sysmem(vendor_boot_img); unmap_sysmem(boot_img); diff --git a/cmd/abootimg.c b/cmd/abootimg.c index e68c523705..7c450a3b2d 100644 --- a/cmd/abootimg.c +++ b/cmd/abootimg.c @@ -17,6 +17,7 @@ /* Please use abootimg_addr() macro to obtain the boot image address */ static ulong _abootimg_addr = -1; +static ulong _ainit_bootimg_addr = -1; static ulong _avendor_bootimg_addr = -1; ulong get_abootimg_addr(void) @@ -24,6 +25,11 @@ ulong get_abootimg_addr(void) return (_abootimg_addr == -1 ? image_load_addr : _abootimg_addr); } +ulong get_ainit_bootimg_addr(void) +{ + return _ainit_bootimg_addr; +} + ulong get_avendor_bootimg_addr(void) { return _avendor_bootimg_addr; @@ -209,7 +215,7 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, char *endp; ulong img_addr; - if (argc < 2 || argc > 3) + if (argc < 2 || argc > 4) return CMD_RET_USAGE; img_addr = hextoul(argv[1], &endp); @@ -220,16 +226,26 @@ static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc, _abootimg_addr = img_addr; - if (argc == 3) { + if (argc > 2) { img_addr = simple_strtoul(argv[2], &endp, 16); if (*endp != '\0') { - printf("Error: Wrong vendor image address\n"); + printf("Error: Wrong vendor_boot image address\n"); return CMD_RET_FAILURE; } _avendor_bootimg_addr = img_addr; } + if (argc == 4) { + img_addr = simple_strtoul(argv[3], &endp, 16); + if (*endp != '\0') { + printf("Error: Wrong init_boot image address\n"); + return CMD_RET_FAILURE; + } + + _ainit_bootimg_addr = img_addr; + } + return CMD_RET_SUCCESS; } @@ -346,7 +362,7 @@ fail: } static struct cmd_tbl cmd_abootimg_sub[] = { - U_BOOT_CMD_MKENT(addr, 3, 1, do_abootimg_addr, "", ""), + U_BOOT_CMD_MKENT(addr, 4, 1, do_abootimg_addr, "", ""), U_BOOT_CMD_MKENT(dump, 2, 1, do_abootimg_dump, "", ""), U_BOOT_CMD_MKENT(get, 5, 1, do_abootimg_get, "", ""), U_BOOT_CMD_MKENT(load, 5, 1, do_abootimg_load, "", ""), @@ -375,7 +391,7 @@ static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc, U_BOOT_CMD( abootimg, CONFIG_SYS_MAXARGS, 0, do_abootimg, "manipulate Android Boot Image", - "addr <boot_img_addr> [<vendor_boot_img_addr>]>\n" + "addr <boot_img_addr> [<vendor_boot_img_addr> [<init_boot_img_addr>]]\n" " - set the address in RAM where boot image is located\n" " ($loadaddr is used by default)\n" "abootimg dump dtb\n" diff --git a/include/image.h b/include/image.h index be3c73a72e..b1fd6b3149 100644 --- a/include/image.h +++ b/include/image.h @@ -1982,6 +1982,13 @@ bool is_android_vendor_boot_image_header(const void *vendor_boot_img); */ ulong get_abootimg_addr(void); +/** + * get_ainit_bootimg_addr() - Get Android init boot image address + * + * Return: Android init boot image address + */ +ulong get_ainit_bootimg_addr(void); + /** * get_avendor_bootimg_addr() - Get Android vendor boot image address *
Quote from [1]: "For devices launching with Android 13, the generic ramdisk is removed from the boot image and placed in a separate init_boot image. This change leaves the boot image with only the GKI kernel." [1]: https://source.android.com/docs/core/architecture/partitions/generic-boot Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com> --- boot/image-board.c | 5 ++++- cmd/abootimg.c | 26 +++++++++++++++++++++----- include/image.h | 7 +++++++ 3 files changed, 32 insertions(+), 6 deletions(-)