[PATCH] abootimg: Add init_boot image support
Roman Stratiienko
r.stratiienko at gmail.com
Wed May 22 23:35:35 CEST 2024
Hi Mattijs,
Thank you for your review.
ср, 22 мая 2024 г. в 17:43, Mattijs Korpershoek <mkorpershoek at baylibre.com>:
>
> Hi Roman,
>
> Thank you for the patch.
>
> On dim., mai 19, 2024 at 12:53, Roman Stratiienko <r.stratiienko at 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 at 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
More information about the U-Boot
mailing list