Re: [PATCH] abootimg: Add init_boot image support

[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.

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