[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