[PATCH 0/3] Android partition load optimization

[PATCH 0/3] Android partition load optimization

[Roman Stratiienko]

This series offers an optimization of the Android image loading procedure.

The first patch introduces a new 'abootimg load' command, which loads
partitions from storage into memory. It uses malloc() to find a free
memory region, whereas previously, the developer had to find a free
memory region by himself.

The remaining patches reduce boot time by avoiding extra disk activity.

The second patch implements get_preloaded_partition(), which picks a
partition from memory previously loaded by the 'abootimg load' command.

The third patch optimizes the 'abootimg load' command by skipping zeros
loading at the partition's end.

Roman Stratiienko (3):
  abootcmd: Add load subcommand
  avb: Implement get_preloaded_partition callback
  abootimg: Implement smart image load feature

 boot/image-android.c |  70 ++++++++++++++++++++++++++
 cmd/abootimg.c       | 116 ++++++++++++++++++++++++++++++++++++++++++-
 common/avb_verify.c  |  53 ++++++++++++++++++++
 include/image.h      |  12 +++++
 4 files changed, 250 insertions(+), 1 deletion(-)

-- 
2.40.1

[PATCH 1/3] abootcmd: Add load subcommand

[Roman Stratiienko]

What it does:
1. Allocates the memory in HEAP to fit the partition
2. Loads partition into memory. In the following patch of this series,
   loading will be optimized to avoid loading an empty space.
3. Sets buffer start and buffer size value into environment variables
   abootimg_<partition>_ptr and abootimg_<partition>_size, respectively.
    and duplicate them as
   abootimg_<partition>_<slot>_ptr and abootimg_<partition>_<slot>_size.
   The latter two are needed to access by the AVB get_preloaded_partition.
   (see the next patch).

Before this command, the boot script developer was responsible for
allocating the memory manually by choosing the start and the end,
which is far from good.

Usage example:

    abootcmd load mmc 0 boot a

Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
---
 cmd/abootimg.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

diff --git a/cmd/abootimg.c b/cmd/abootimg.c
index a5321bab6a..808c9c4941 100644
--- a/cmd/abootimg.c
+++ b/cmd/abootimg.c
@@ -8,7 +8,9 @@
 #include <common.h>
 #include <command.h>
 #include <image.h>
+#include <malloc.h>
 #include <mapmem.h>
+#include <part.h>
 
 #define abootimg_addr() \
 	(_abootimg_addr == -1 ? image_load_addr : _abootimg_addr)
@@ -259,10 +261,81 @@ static int do_abootimg_dump(struct cmd_tbl *cmdtp, int flag, int argc,
 	return CMD_RET_SUCCESS;
 }
 
+static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
+			    char *const argv[])
+{
+	int time_start = get_timer(0);
+	struct blk_desc *desc;
+	struct disk_partition info;
+	char buf[512] = { 0 };
+	void *addr;
+	int ret;
+
+	if (argc < 4)
+		return CMD_RET_USAGE;
+	if (argc > 5)
+		return CMD_RET_USAGE;
+
+	ret = blk_get_device_by_str(argv[1], argv[2], &desc);
+	if (ret < 0) {
+		printf("Error: Failed to get device %s %s\n", argv[1], argv[2]);
+		return CMD_RET_FAILURE;
+	}
+
+	if (argc == 5)
+		sprintf(buf, "%s_%s", argv[3], argv[4]);
+	else
+		sprintf(buf, "%s", argv[3]);
+
+	ret = part_get_info_by_name(desc, buf, &info);
+	if (ret < 0) {
+		printf("Error: Failed to get partition %s\n", buf);
+		return CMD_RET_FAILURE;
+	}
+
+	addr = (void *)memalign(4096, info.size * info.blksz);
+	if (!addr) {
+		printf("Error: Failed to allocate memory\n");
+		return CMD_RET_FAILURE;
+	}
+
+	ret = blk_dread(desc, info.start, info.size, addr);
+	if (ret < 0) {
+		printf("Error: Failed to read partition %s\n", buf);
+		goto fail;
+	}
+
+	sprintf(buf, "abootimg_%s_ptr", argv[3]);
+	env_set_hex(buf, (ulong)addr);
+
+	sprintf(buf, "abootimg_%s_size", argv[3]);
+	env_set_hex(buf, info.size * info.blksz);
+
+	if (argc == 5) {
+		sprintf(buf, "abootimg_%s_%s_ptr", argv[3], argv[4]);
+		env_set_hex(buf, (ulong)addr);
+
+		sprintf(buf, "abootimg_%s_%s_size", argv[3], argv[4]);
+		env_set_hex(buf, info.size * info.blksz);
+	}
+
+	int time_end = get_timer(0);
+
+	printf("Loaded '%s' partition to address 0x%p (size: 0x%x) in %lu ms\n",
+	       argv[3], addr, info.size * info.blksz, time_end - time_start);
+
+	return CMD_RET_SUCCESS;
+
+fail:
+	free(addr);
+	return CMD_RET_FAILURE;
+}
+
 static struct cmd_tbl cmd_abootimg_sub[] = {
 	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, "", ""),
 };
 
 static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc,
@@ -305,5 +378,14 @@ U_BOOT_CMD(
 	"    - get address and size (hex) of DT blob in the image by index\n"
 	"      <num>: index number of desired DT blob in DTB area\n"
 	"      [addr_var]: variable name to contain DT blob address\n"
-	"      [size_var]: variable name to contain DT blob size"
+	"      [size_var]: variable name to contain DT blob size\n"
+	"abootimg load interface dev partition [slot_name]\n"
+	"    - load boot image from device partition\n"
+	"      memory is allocated in heap\n"
+	"      address is stored in $abootimg_<partition>_ptr\n"
+	"      size is stored in $abootimg_<partition>_size\n"
+	"      interface: interface type (e.g. mmc, usb)\n"
+	"      dev: device number (e.g. 0, 1)\n"
+	"      partition: partition number (e.g. boot, dtb)\n"
+	"      slot_suffix: slot name (e.g. a, b)"
 );
-- 
2.40.1

Re: [PATCH 1/3] abootcmd: Add load subcommand

[Mattijs Korpershoek]

Hi Roman,

Thank you for the patch.

On dim., mai 19, 2024 at 19:18, Roman Stratiienko <r.stratiienko@gmail.com> wrote:

> What it does:
> 1. Allocates the memory in HEAP to fit the partition
> 2. Loads partition into memory. In the following patch of this series,
>    loading will be optimized to avoid loading an empty space.
> 3. Sets buffer start and buffer size value into environment variables
>    abootimg_<partition>_ptr and abootimg_<partition>_size, respectively.
>     and duplicate them as
>    abootimg_<partition>_<slot>_ptr and abootimg_<partition>_<slot>_size.
>    The latter two are needed to access by the AVB get_preloaded_partition.
>    (see the next patch).
>
> Before this command, the boot script developer was responsible for
> allocating the memory manually by choosing the start and the end,
> which is far from good.
>
> Usage example:
>
>     abootcmd load mmc 0 boot a

Should this be: abootimg load mmc 0 boot a ?

>
> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> ---
>  cmd/abootimg.c | 84 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
>
> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> index a5321bab6a..808c9c4941 100644
> --- a/cmd/abootimg.c
> +++ b/cmd/abootimg.c
> @@ -8,7 +8,9 @@
>  #include <common.h>
>  #include <command.h>
>  #include <image.h>
> +#include <malloc.h>
>  #include <mapmem.h>
> +#include <part.h>
>  
>  #define abootimg_addr() \
>  	(_abootimg_addr == -1 ? image_load_addr : _abootimg_addr)
> @@ -259,10 +261,81 @@ static int do_abootimg_dump(struct cmd_tbl *cmdtp, int flag, int argc,
>  	return CMD_RET_SUCCESS;
>  }
>  
> +static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
> +			    char *const argv[])
> +{
> +	int time_start = get_timer(0);

On -next branch (where the patch will be applied), I see the following
build error:

cmd/abootimg.c: In function ‘do_abootimg_load’:
cmd/abootimg.c:279:26: error: implicit declaration of function ‘get_timer’ [-Wimplicit-function-declaration]
  279 |         int time_start = get_timer(0);
      |                          ^~~~~~~~~

Please add the missing include: #include <time.h>

Base: 7e52d6ccfb76 ("Merge patch series "FWU: Add support for FWU metadata version 2"")

> +	struct blk_desc *desc;
> +	struct disk_partition info;
> +	char buf[512] = { 0 };
> +	void *addr;
> +	int ret;
> +
> +	if (argc < 4)
> +		return CMD_RET_USAGE;
> +	if (argc > 5)
> +		return CMD_RET_USAGE;

Can we please do this in a single line, like done in other commands such
as do_abootimg_addr()?

> +
> +	ret = blk_get_device_by_str(argv[1], argv[2], &desc);
> +	if (ret < 0) {
> +		printf("Error: Failed to get device %s %s\n", argv[1], argv[2]);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	if (argc == 5)
> +		sprintf(buf, "%s_%s", argv[3], argv[4]);
> +	else
> +		sprintf(buf, "%s", argv[3]);
> +
> +	ret = part_get_info_by_name(desc, buf, &info);
> +	if (ret < 0) {
> +		printf("Error: Failed to get partition %s\n", buf);
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	addr = (void *)memalign(4096, info.size * info.blksz);

Why 4096? If this is a known number, can we use a define for it please?

> +	if (!addr) {
> +		printf("Error: Failed to allocate memory\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	ret = blk_dread(desc, info.start, info.size, addr);
> +	if (ret < 0) {
> +		printf("Error: Failed to read partition %s\n", buf);
> +		goto fail;
> +	}
> +
> +	sprintf(buf, "abootimg_%s_ptr", argv[3]);
> +	env_set_hex(buf, (ulong)addr);
> +
> +	sprintf(buf, "abootimg_%s_size", argv[3]);
> +	env_set_hex(buf, info.size * info.blksz);
> +
> +	if (argc == 5) {
> +		sprintf(buf, "abootimg_%s_%s_ptr", argv[3], argv[4]);
> +		env_set_hex(buf, (ulong)addr);
> +
> +		sprintf(buf, "abootimg_%s_%s_size", argv[3], argv[4]);
> +		env_set_hex(buf, info.size * info.blksz);
> +	}
> +
> +	int time_end = get_timer(0);
> +
> +	printf("Loaded '%s' partition to address 0x%p (size: 0x%x) in %lu ms\n",
> +	       argv[3], addr, info.size * info.blksz, time_end - time_start);

This causes warnings on -next when building sandbox:
$ make sandbox_defconfig
$ make

cmd/abootimg.c:339:65: warning: format ‘%x’ expects argument of type ‘unsigned int’, but argument 4 has type ‘long unsigned int’ [-Wformat=]
  339 |         printf("Loaded '%s' partition to address 0x%p (size: 0x%x) in %lu ms\n",
      |                                                                ~^
      |                                                                 |
      |                                                                 unsigned int
      |                                                                %lx
  340 |                argv[3], addr, info.size * info.blksz, time_end - time_start);
      |                               ~~~~~~~~~~~~~~~~~~~~~~
      |                                         |


> +
> +	return CMD_RET_SUCCESS;
> +
> +fail:
> +	free(addr);
> +	return CMD_RET_FAILURE;
> +}
> +
>  static struct cmd_tbl cmd_abootimg_sub[] = {
>  	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, "", ""),
>  };
>  
>  static int do_abootimg(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -305,5 +378,14 @@ U_BOOT_CMD(
>  	"    - get address and size (hex) of DT blob in the image by index\n"
>  	"      <num>: index number of desired DT blob in DTB area\n"
>  	"      [addr_var]: variable name to contain DT blob address\n"
> -	"      [size_var]: variable name to contain DT blob size"
> +	"      [size_var]: variable name to contain DT blob size\n"
> +	"abootimg load interface dev partition [slot_name]\n"
> +	"    - load boot image from device partition\n"
> +	"      memory is allocated in heap\n"
> +	"      address is stored in $abootimg_<partition>_ptr\n"
> +	"      size is stored in $abootimg_<partition>_size\n"
> +	"      interface: interface type (e.g. mmc, usb)\n"
> +	"      dev: device number (e.g. 0, 1)\n"
> +	"      partition: partition number (e.g. boot, dtb)\n"

"partition name" instead of "partition number"?

> +	"      slot_suffix: slot name (e.g. a, b)"
>  );
> -- 
> 2.40.1

[PATCH 2/3] avb: Implement get_preloaded_partition callback

[Roman Stratiienko]

AVB can reuse already loaded images instead of loading them
from the disk.

The get_preloaded_partition now looks for the env. variables
set by the 'abootimg load' to find the correct partition in RAM.

Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
---
 common/avb_verify.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/common/avb_verify.c b/common/avb_verify.c
index cff9117d92..d2626e8844 100644
--- a/common/avb_verify.c
+++ b/common/avb_verify.c
@@ -6,6 +6,7 @@
 #include <avb_verify.h>
 #include <blk.h>
 #include <cpu_func.h>
+#include <env.h>
 #include <image.h>
 #include <malloc.h>
 #include <part.h>
@@ -595,6 +596,55 @@ static AvbIOResult read_from_partition(AvbOps *ops,
 			   num_bytes, buffer, out_num_read, IO_READ);
 }
 
+#ifdef CONFIG_ANDROID_BOOT_IMAGE
+/**
+ * get_preloaded_partition() - Gets the starting pointer of a partition that
+ * is pre-loaded in memory, and save it to |out_pointer|.
+ *
+ * If the partition is not pre-loaded in memory, the out_pointer shall not be
+ * modified.
+ *
+ * @ops: contains AVB ops handlers
+ * @partition: partition name, NUL-terminated UTF-8 string
+ * @num_bytes: amount of bytes to read
+ * @out_pointer: pointer to the starting address of the partition
+ * @out_num_bytes_preloaded: amount of bytes pre-loaded in memory
+ *
+ * @return:
+ *      AVB_IO_RESULT_OK, if partition was found or was not found
+ *
+ */
+static AvbIOResult get_preloaded_partition(AvbOps *ops, const char *partition, size_t num_bytes,
+					   uint8_t **out_pointer, size_t *out_num_bytes_preloaded)
+{
+	size_t partition_start = 0;
+	size_t partition_size = 0;
+	char env_name[64];
+
+	sprintf(env_name, "abootimg_%s_ptr", partition);
+	partition_start = env_get_hex(env_name, 0);
+
+	sprintf(env_name, "abootimg_%s_size", partition);
+	partition_size = env_get_hex(env_name, 0);
+
+	if (partition_start == 0 || partition_size == 0)
+		return AVB_IO_RESULT_OK;
+
+	if (partition_size < num_bytes) {
+		printf("AVB: Preloaded partition %s size %zu is smaller than requested %zu\n",
+		       partition, partition_size, num_bytes);
+		return AVB_IO_RESULT_ERROR_IO;
+	}
+
+	*out_pointer = (uint8_t *)partition_start;
+	*out_num_bytes_preloaded = partition_size;
+
+	printf("AVB: Using preloaded partition %s at %p\n", partition, *out_pointer);
+
+	return AVB_IO_RESULT_OK;
+}
+#endif
+
 /**
  * write_to_partition() - writes N bytes to a partition identified by a string
  * name
@@ -1043,6 +1093,9 @@ AvbOps *avb_ops_alloc(int boot_device)
 	ops_data->ops.user_data = ops_data;
 
 	ops_data->ops.read_from_partition = read_from_partition;
+#ifdef CONFIG_ANDROID_BOOT_IMAGE
+	ops_data->ops.get_preloaded_partition = get_preloaded_partition;
+#endif
 	ops_data->ops.write_to_partition = write_to_partition;
 	ops_data->ops.validate_vbmeta_public_key = validate_vbmeta_public_key;
 	ops_data->ops.read_rollback_index = read_rollback_index;
-- 
2.40.1

Re: [PATCH 2/3] avb: Implement get_preloaded_partition callback

[Mattijs Korpershoek]

Hi Roman,

Thank you for the patch.

On dim., mai 19, 2024 at 19:18, Roman Stratiienko <r.stratiienko@gmail.com> wrote:

> AVB can reuse already loaded images instead of loading them
> from the disk.
>
> The get_preloaded_partition now looks for the env. variables
> set by the 'abootimg load' to find the correct partition in RAM.
>
> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> ---
>  common/avb_verify.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index cff9117d92..d2626e8844 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -6,6 +6,7 @@
>  #include <avb_verify.h>
>  #include <blk.h>
>  #include <cpu_func.h>
> +#include <env.h>
>  #include <image.h>
>  #include <malloc.h>
>  #include <part.h>
> @@ -595,6 +596,55 @@ static AvbIOResult read_from_partition(AvbOps *ops,
>  			   num_bytes, buffer, out_num_read, IO_READ);
>  }
>  
> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
> +/**
> + * get_preloaded_partition() - Gets the starting pointer of a partition that
> + * is pre-loaded in memory, and save it to |out_pointer|.
> + *
> + * If the partition is not pre-loaded in memory, the out_pointer shall not be
> + * modified.
> + *
> + * @ops: contains AVB ops handlers
> + * @partition: partition name, NUL-terminated UTF-8 string

NUL -> NULL

> + * @num_bytes: amount of bytes to read
> + * @out_pointer: pointer to the starting address of the partition
> + * @out_num_bytes_preloaded: amount of bytes pre-loaded in memory
> + *
> + * @return:
> + *      AVB_IO_RESULT_OK, if partition was found or was not found

Add:

           AVB_IO_RESULT_ERROR_IO, if partition size is smaller than requested

With both small remarks addressed, please add:

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

> + *
> + */
> +static AvbIOResult get_preloaded_partition(AvbOps *ops, const char *partition, size_t num_bytes,
> +					   uint8_t **out_pointer, size_t *out_num_bytes_preloaded)
> +{

[...]

> -- 
> 2.40.1

Re: [PATCH 2/3] avb: Implement get_preloaded_partition callback

[Igor Opaniuk]

Hi Roman,

On Sun, May 19, 2024 at 9:19 PM Roman Stratiienko
<r.stratiienko@gmail.com> wrote:
>
> AVB can reuse already loaded images instead of loading them
> from the disk.
>
> The get_preloaded_partition now looks for the env. variables
> set by the 'abootimg load' to find the correct partition in RAM.
>
> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> ---
>  common/avb_verify.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/common/avb_verify.c b/common/avb_verify.c
> index cff9117d92..d2626e8844 100644
> --- a/common/avb_verify.c
> +++ b/common/avb_verify.c
> @@ -6,6 +6,7 @@
>  #include <avb_verify.h>
>  #include <blk.h>
>  #include <cpu_func.h>
> +#include <env.h>
>  #include <image.h>
>  #include <malloc.h>
>  #include <part.h>
> @@ -595,6 +596,55 @@ static AvbIOResult read_from_partition(AvbOps *ops,
>                            num_bytes, buffer, out_num_read, IO_READ);
>  }
>
> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
Please use CONFIG_IS_ENABLED() macro
> +/**
> + * get_preloaded_partition() - Gets the starting pointer of a partition that
> + * is pre-loaded in memory, and save it to |out_pointer|.
> + *
> + * If the partition is not pre-loaded in memory, the out_pointer shall not be
> + * modified.
> + *
> + * @ops: contains AVB ops handlers
> + * @partition: partition name, NUL-terminated UTF-8 string
> + * @num_bytes: amount of bytes to read
> + * @out_pointer: pointer to the starting address of the partition
> + * @out_num_bytes_preloaded: amount of bytes pre-loaded in memory
> + *
> + * @return:
Please follow [1]. I know that the whole file has this type of issues,
I plan to fix it in the near future.

[1] https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html.

Should be "Return: AVB_IO_RESULT_OK, if partition was found or was not found"
> + *      AVB_IO_RESULT_OK, if partition was found or was not found
> + *
> + */
> +static AvbIOResult get_preloaded_partition(AvbOps *ops, const char *partition, size_t num_bytes,
> +                                          uint8_t **out_pointer, size_t *out_num_bytes_preloaded)
> +{
> +       size_t partition_start = 0;
> +       size_t partition_size = 0;
> +       char env_name[64];
> +
> +       sprintf(env_name, "abootimg_%s_ptr", partition);
Please use more secure version snprintf()
> +       partition_start = env_get_hex(env_name, 0);
> +
> +       sprintf(env_name, "abootimg_%s_size", partition);
> +       partition_size = env_get_hex(env_name, 0);
> +
> +       if (partition_start == 0 || partition_size == 0)
> +               return AVB_IO_RESULT_OK;
> +
> +       if (partition_size < num_bytes) {
> +               printf("AVB: Preloaded partition %s size %zu is smaller than requested %zu\n",
> +                      partition, partition_size, num_bytes);
> +               return AVB_IO_RESULT_ERROR_IO;
> +       }
> +
> +       *out_pointer = (uint8_t *)partition_start;
> +       *out_num_bytes_preloaded = partition_size;
> +
> +       printf("AVB: Using preloaded partition %s at %p\n", partition, *out_pointer);
> +
> +       return AVB_IO_RESULT_OK;
> +}
> +#endif
> +
>  /**
>   * write_to_partition() - writes N bytes to a partition identified by a string
>   * name
> @@ -1043,6 +1093,9 @@ AvbOps *avb_ops_alloc(int boot_device)
>         ops_data->ops.user_data = ops_data;
>
>         ops_data->ops.read_from_partition = read_from_partition;
> +#ifdef CONFIG_ANDROID_BOOT_IMAGE
Please use CONFIG_IS_ENABLED() macro
> +       ops_data->ops.get_preloaded_partition = get_preloaded_partition;
> +#endif
>         ops_data->ops.write_to_partition = write_to_partition;
>         ops_data->ops.validate_vbmeta_public_key = validate_vbmeta_public_key;
>         ops_data->ops.read_rollback_index = read_rollback_index;
> --
> 2.40.1
>


-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk

[PATCH 3/3] abootimg: Implement smart image load feature

[Roman Stratiienko]

Load only part of the boot partition that contains valuable
information, thus improving the boot time.

Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
---
 boot/image-android.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
 cmd/abootimg.c       | 40 ++++++++++++++++++++++---
 include/image.h      | 12 ++++++++
 3 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/boot/image-android.c b/boot/image-android.c
index da8003f370..d00a896a40 100644
--- a/boot/image-android.c
+++ b/boot/image-android.c
@@ -204,6 +204,76 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
 	return true;
 }
 
+/**
+ * android_image_get_valuable_size() - get the size of the android image
+ *
+ * This function checks if the image is Android boot image and returns the
+ * valuable size of the image.
+ *
+ * @hdr_addr: Boot image header address (boot or vendor_boot)
+ *
+ * @return size of the image on success, 0 on failure
+ */
+size_t android_image_get_valuable_size(const void *hdr_addr)
+{
+	int version, size;
+
+	if (is_android_boot_image_header(hdr_addr)) {
+		const struct andr_boot_img_hdr_v0 *hdr = hdr_addr;
+
+		version = ((struct andr_boot_img_hdr_v0 *)hdr_addr)->header_version;
+		if (version > 2) {
+			const struct andr_boot_img_hdr_v3 *hdr = hdr_addr;
+
+			size = ALIGN(hdr->header_size, ANDR_GKI_PAGE_SIZE);
+			size += ALIGN(hdr->kernel_size, ANDR_GKI_PAGE_SIZE);
+			size += ALIGN(hdr->ramdisk_size, ANDR_GKI_PAGE_SIZE);
+
+			if (version > 3)
+				size += ALIGN(hdr->signature_size, ANDR_GKI_PAGE_SIZE);
+
+			return size;
+		}
+
+		size = hdr->page_size;
+		size += ALIGN(hdr->kernel_size, hdr->page_size);
+		size += ALIGN(hdr->ramdisk_size, hdr->page_size);
+		size += ALIGN(hdr->second_size, hdr->page_size);
+
+		if (version > 0)
+			size += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
+
+		if (version > 1)
+			size += ALIGN(hdr->dtb_size, hdr->page_size);
+
+		return size;
+	}
+
+	if (is_android_vendor_boot_image_header(hdr_addr)) {
+		const struct andr_vnd_boot_img_hdr *hdr = hdr_addr;
+
+		version = ((struct andr_vnd_boot_img_hdr *)hdr_addr)->header_version;
+		if (version < 3) {
+			printf("Vendor boot image header version %d is not supported\n", version);
+
+			return 0;
+		}
+
+		size = ALIGN(hdr->header_size, hdr->page_size);
+		size += ALIGN(hdr->vendor_ramdisk_size, hdr->page_size);
+		size += ALIGN(hdr->dtb_size, hdr->page_size);
+
+		if (version > 3) {
+			size += ALIGN(hdr->vendor_ramdisk_table_size, hdr->page_size);
+			size += ALIGN(hdr->bootconfig_size, hdr->page_size);
+		}
+
+		return size;
+	}
+
+	return 0;
+}
+
 static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
 {
 	/*
diff --git a/cmd/abootimg.c b/cmd/abootimg.c
index 808c9c4941..fe7c5c5e2c 100644
--- a/cmd/abootimg.c
+++ b/cmd/abootimg.c
@@ -182,6 +182,35 @@ static int abootimg_get_dtb(int argc, char *const argv[])
 	return CMD_RET_USAGE;
 }
 
+static int abootimg_smart_load(struct blk_desc *desc, struct disk_partition *info, void *addr)
+{
+	int ret, size;
+
+	ret = blk_dread(desc, info->start, 4096 / info->blksz, addr);
+	if (ret < 0) {
+		printf("Error: Failed to read partition\n");
+		return CMD_RET_FAILURE;
+	}
+
+	size = android_image_get_valuable_size(addr);
+	if (size == 0)
+		return 0;
+
+	ret = blk_dread(desc, info->start, DIV_ROUND_UP(size, info->blksz), addr);
+	if (ret < 0) {
+		printf("Error: Failed to read partition\n");
+		return CMD_RET_FAILURE;
+	}
+
+	memset(addr + size, 0, info->size * info->blksz - size);
+
+	printf("Loaded Android boot image using smart load (%d/%d MB)\n",
+	       (int)DIV_ROUND_UP(size, 1024 * 1024),
+	       (int)DIV_ROUND_UP(info->size * info->blksz, 1024 * 1024));
+
+	return size;
+}
+
 static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc,
 			    char *const argv[])
 {
@@ -299,10 +328,13 @@ static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
 		return CMD_RET_FAILURE;
 	}
 
-	ret = blk_dread(desc, info.start, info.size, addr);
-	if (ret < 0) {
-		printf("Error: Failed to read partition %s\n", buf);
-		goto fail;
+	ret = abootimg_smart_load(desc, &info, addr);
+	if (ret <= 0) {
+		ret = blk_dread(desc, info.start, info.size, addr);
+		if (ret < 0) {
+			printf("Error: Failed to read partition %s\n", buf);
+			goto fail;
+		}
 	}
 
 	sprintf(buf, "abootimg_%s_ptr", argv[3]);
diff --git a/include/image.h b/include/image.h
index c5b288f62b..7d8ff40c3f 100644
--- a/include/image.h
+++ b/include/image.h
@@ -1826,6 +1826,18 @@ struct andr_image_data;
 bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
 			    struct andr_image_data *data);
 
+/**
+ * android_image_get_valuable_size() - get the size of the android image
+ *
+ * This function checks if the image is Android boot image and returns the
+ * valuable size of the image.
+ *
+ * @hdr_addr: Boot image header address (boot or vendor_boot)
+ *
+ * @return size of the image on success, 0 on failure
+ */
+size_t android_image_get_valuable_size(const void *hdr_addr);
+
 struct andr_boot_img_hdr_v0;
 
 /**
-- 
2.40.1

Re: [PATCH 3/3] abootimg: Implement smart image load feature

[Mattijs Korpershoek]

Hi Roman,

Thank you for the patch.

On dim., mai 19, 2024 at 19:18, Roman Stratiienko <r.stratiienko@gmail.com> wrote:

> Load only part of the boot partition that contains valuable
> information, thus improving the boot time.
>
> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> ---
>  boot/image-android.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
>  cmd/abootimg.c       | 40 ++++++++++++++++++++++---
>  include/image.h      | 12 ++++++++
>  3 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index da8003f370..d00a896a40 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -204,6 +204,76 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
>  	return true;
>  }
>  
> +/**
> + * android_image_get_valuable_size() - get the size of the android image
> + *
> + * This function checks if the image is Android boot image and returns the
> + * valuable size of the image.
> + *
> + * @hdr_addr: Boot image header address (boot or vendor_boot)
> + *
> + * @return size of the image on success, 0 on failure
> + */
> +size_t android_image_get_valuable_size(const void *hdr_addr)

Can't we use a ssize_t to return a negative error code?
0 as error seems odd and not what is used in most functions in boot/image-android.c

> +{
> +	int version, size;
> +
> +	if (is_android_boot_image_header(hdr_addr)) {
> +		const struct andr_boot_img_hdr_v0 *hdr = hdr_addr;
> +
> +		version = ((struct andr_boot_img_hdr_v0 *)hdr_addr)->header_version;
> +		if (version > 2) {
> +			const struct andr_boot_img_hdr_v3 *hdr = hdr_addr;
> +
> +			size = ALIGN(hdr->header_size, ANDR_GKI_PAGE_SIZE);
> +			size += ALIGN(hdr->kernel_size, ANDR_GKI_PAGE_SIZE);
> +			size += ALIGN(hdr->ramdisk_size, ANDR_GKI_PAGE_SIZE);
> +
> +			if (version > 3)
> +				size += ALIGN(hdr->signature_size, ANDR_GKI_PAGE_SIZE);
> +
> +			return size;
> +		}
> +
> +		size = hdr->page_size;
> +		size += ALIGN(hdr->kernel_size, hdr->page_size);
> +		size += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +		size += ALIGN(hdr->second_size, hdr->page_size);
> +
> +		if (version > 0)
> +			size += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
> +
> +		if (version > 1)
> +			size += ALIGN(hdr->dtb_size, hdr->page_size);
> +
> +		return size;
> +	}
> +
> +	if (is_android_vendor_boot_image_header(hdr_addr)) {
> +		const struct andr_vnd_boot_img_hdr *hdr = hdr_addr;
> +
> +		version = ((struct andr_vnd_boot_img_hdr *)hdr_addr)->header_version;
> +		if (version < 3) {
> +			printf("Vendor boot image header version %d is not supported\n", version);
> +
> +			return 0;
> +		}
> +
> +		size = ALIGN(hdr->header_size, hdr->page_size);
> +		size += ALIGN(hdr->vendor_ramdisk_size, hdr->page_size);
> +		size += ALIGN(hdr->dtb_size, hdr->page_size);
> +
> +		if (version > 3) {
> +			size += ALIGN(hdr->vendor_ramdisk_table_size, hdr->page_size);
> +			size += ALIGN(hdr->bootconfig_size, hdr->page_size);
> +		}
> +
> +		return size;
> +	}
> +
> +	return 0;
> +}
> +
>  static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
>  {
>  	/*
> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> index 808c9c4941..fe7c5c5e2c 100644
> --- a/cmd/abootimg.c
> +++ b/cmd/abootimg.c
> @@ -182,6 +182,35 @@ static int abootimg_get_dtb(int argc, char *const argv[])
>  	return CMD_RET_USAGE;
>  }
>  
> +static int abootimg_smart_load(struct blk_desc *desc, struct disk_partition *info, void *addr)
> +{
> +	int ret, size;
> +
> +	ret = blk_dread(desc, info->start, 4096 / info->blksz, addr);

Why 4096? Can we use #define for this or reuse an exising definition?

> +	if (ret < 0) {
> +		printf("Error: Failed to read partition\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	size = android_image_get_valuable_size(addr);
> +	if (size == 0)
> +		return 0;
> +
> +	ret = blk_dread(desc, info->start, DIV_ROUND_UP(size, info->blksz), addr);
> +	if (ret < 0) {
> +		printf("Error: Failed to read partition\n");
> +		return CMD_RET_FAILURE;
> +	}
> +
> +	memset(addr + size, 0, info->size * info->blksz - size);
> +
> +	printf("Loaded Android boot image using smart load (%d/%d MB)\n",

Why only boot image? Should this be more generic be passing the
partition name instead?

> +	       (int)DIV_ROUND_UP(size, 1024 * 1024),
> +	       (int)DIV_ROUND_UP(info->size * info->blksz, 1024 * 1024));
> +
> +	return size;
> +}
> +
>  static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc,
>  			    char *const argv[])
>  {
> @@ -299,10 +328,13 @@ static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
>  		return CMD_RET_FAILURE;
>  	}
>  
> -	ret = blk_dread(desc, info.start, info.size, addr);
> -	if (ret < 0) {
> -		printf("Error: Failed to read partition %s\n", buf);
> -		goto fail;
> +	ret = abootimg_smart_load(desc, &info, addr);
> +	if (ret <= 0) {
> +		ret = blk_dread(desc, info.start, info.size, addr);
> +		if (ret < 0) {
> +			printf("Error: Failed to read partition %s\n", buf);
> +			goto fail;
> +		}
>  	}
>  
>  	sprintf(buf, "abootimg_%s_ptr", argv[3]);
> diff --git a/include/image.h b/include/image.h
> index c5b288f62b..7d8ff40c3f 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1826,6 +1826,18 @@ struct andr_image_data;
>  bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
>  			    struct andr_image_data *data);
>  
> +/**
> + * android_image_get_valuable_size() - get the size of the android image
> + *
> + * This function checks if the image is Android boot image and returns the
> + * valuable size of the image.
> + *
> + * @hdr_addr: Boot image header address (boot or vendor_boot)
> + *
> + * @return size of the image on success, 0 on failure
> + */
> +size_t android_image_get_valuable_size(const void *hdr_addr);
> +
>  struct andr_boot_img_hdr_v0;
>  
>  /**
> -- 
> 2.40.1

Re: [PATCH 3/3] abootimg: Implement smart image load feature

[Igor Opaniuk]

Hi Roman,

On Sun, May 19, 2024 at 9:19 PM Roman Stratiienko
<r.stratiienko@gmail.com> wrote:
>
> Load only part of the boot partition that contains valuable
> information, thus improving the boot time.
>
> Signed-off-by: Roman Stratiienko <r.stratiienko@gmail.com>
> ---
>  boot/image-android.c | 70 ++++++++++++++++++++++++++++++++++++++++++++
>  cmd/abootimg.c       | 40 ++++++++++++++++++++++---
>  include/image.h      | 12 ++++++++
>  3 files changed, 118 insertions(+), 4 deletions(-)
>
> diff --git a/boot/image-android.c b/boot/image-android.c
> index da8003f370..d00a896a40 100644
> --- a/boot/image-android.c
> +++ b/boot/image-android.c
> @@ -204,6 +204,76 @@ bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
>         return true;
>  }
>
> +/**
> + * android_image_get_valuable_size() - get the size of the android image
> + *
> + * This function checks if the image is Android boot image and returns the
> + * valuable size of the image.
> + *
> + * @hdr_addr: Boot image header address (boot or vendor_boot)
> + *
> + * @return size of the image on success, 0 on failure
> + */
> +size_t android_image_get_valuable_size(const void *hdr_addr)
> +{
> +       int version, size;
> +
> +       if (is_android_boot_image_header(hdr_addr)) {
> +               const struct andr_boot_img_hdr_v0 *hdr = hdr_addr;
> +
> +               version = ((struct andr_boot_img_hdr_v0 *)hdr_addr)->header_version;
> +               if (version > 2) {
> +                       const struct andr_boot_img_hdr_v3 *hdr = hdr_addr;
> +
> +                       size = ALIGN(hdr->header_size, ANDR_GKI_PAGE_SIZE);
> +                       size += ALIGN(hdr->kernel_size, ANDR_GKI_PAGE_SIZE);
> +                       size += ALIGN(hdr->ramdisk_size, ANDR_GKI_PAGE_SIZE);
> +
> +                       if (version > 3)
> +                               size += ALIGN(hdr->signature_size, ANDR_GKI_PAGE_SIZE);
> +
> +                       return size;
> +               }
> +
> +               size = hdr->page_size;
> +               size += ALIGN(hdr->kernel_size, hdr->page_size);
> +               size += ALIGN(hdr->ramdisk_size, hdr->page_size);
> +               size += ALIGN(hdr->second_size, hdr->page_size);
> +
> +               if (version > 0)
> +                       size += ALIGN(hdr->recovery_dtbo_size, hdr->page_size);
> +
> +               if (version > 1)
> +                       size += ALIGN(hdr->dtb_size, hdr->page_size);
> +
> +               return size;
> +       }
> +
> +       if (is_android_vendor_boot_image_header(hdr_addr)) {
> +               const struct andr_vnd_boot_img_hdr *hdr = hdr_addr;
> +
> +               version = ((struct andr_vnd_boot_img_hdr *)hdr_addr)->header_version;
> +               if (version < 3) {
> +                       printf("Vendor boot image header version %d is not supported\n", version);
> +
> +                       return 0;
> +               }
> +
> +               size = ALIGN(hdr->header_size, hdr->page_size);
> +               size += ALIGN(hdr->vendor_ramdisk_size, hdr->page_size);
> +               size += ALIGN(hdr->dtb_size, hdr->page_size);
> +
> +               if (version > 3) {
> +                       size += ALIGN(hdr->vendor_ramdisk_table_size, hdr->page_size);
> +                       size += ALIGN(hdr->bootconfig_size, hdr->page_size);
> +               }
> +
> +               return size;
> +       }
> +
> +       return 0;
Can we split this function on two, for example:
android_boot_image_get_valuable_size()
android_vendor_image_get_valuable_size(),

Then check image type in abootimg_smart_load() and call proper function
accordingly?
> +}
> +
>  static ulong android_image_get_kernel_addr(struct andr_image_data *img_data)
>  {
>         /*
> diff --git a/cmd/abootimg.c b/cmd/abootimg.c
> index 808c9c4941..fe7c5c5e2c 100644
> --- a/cmd/abootimg.c
> +++ b/cmd/abootimg.c
> @@ -182,6 +182,35 @@ static int abootimg_get_dtb(int argc, char *const argv[])
>         return CMD_RET_USAGE;
>  }
>
> +static int abootimg_smart_load(struct blk_desc *desc, struct disk_partition *info, void *addr)
> +{
> +       int ret, size;
> +
> +       ret = blk_dread(desc, info->start, 4096 / info->blksz, addr);
> +       if (ret < 0) {
> +               printf("Error: Failed to read partition\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       size = android_image_get_valuable_size(addr);
> +       if (size == 0)
> +               return 0;
> +
> +       ret = blk_dread(desc, info->start, DIV_ROUND_UP(size, info->blksz), addr);
> +       if (ret < 0) {
> +               printf("Error: Failed to read partition\n");
> +               return CMD_RET_FAILURE;
> +       }
> +
> +       memset(addr + size, 0, info->size * info->blksz - size);
> +
> +       printf("Loaded Android boot image using smart load (%d/%d MB)\n",
> +              (int)DIV_ROUND_UP(size, 1024 * 1024),
> +              (int)DIV_ROUND_UP(info->size * info->blksz, 1024 * 1024));
> +
> +       return size;
> +}
> +
>  static int do_abootimg_addr(struct cmd_tbl *cmdtp, int flag, int argc,
>                             char *const argv[])
>  {
> @@ -299,10 +328,13 @@ static int do_abootimg_load(struct cmd_tbl *cmdtp, int flag, int argc,
>                 return CMD_RET_FAILURE;
>         }
>
> -       ret = blk_dread(desc, info.start, info.size, addr);
> -       if (ret < 0) {
> -               printf("Error: Failed to read partition %s\n", buf);
> -               goto fail;
> +       ret = abootimg_smart_load(desc, &info, addr);
> +       if (ret <= 0) {
> +               ret = blk_dread(desc, info.start, info.size, addr);
> +               if (ret < 0) {
> +                       printf("Error: Failed to read partition %s\n", buf);
> +                       goto fail;
> +               }
>         }
>
>         sprintf(buf, "abootimg_%s_ptr", argv[3]);
> diff --git a/include/image.h b/include/image.h
> index c5b288f62b..7d8ff40c3f 100644
> --- a/include/image.h
> +++ b/include/image.h
> @@ -1826,6 +1826,18 @@ struct andr_image_data;
>  bool android_image_get_data(const void *boot_hdr, const void *vendor_boot_hdr,
>                             struct andr_image_data *data);
>
> +/**
> + * android_image_get_valuable_size() - get the size of the android image
> + *
> + * This function checks if the image is Android boot image and returns the
> + * valuable size of the image.
> + *
> + * @hdr_addr: Boot image header address (boot or vendor_boot)
> + *
> + * @return size of the image on success, 0 on failure
> + */
> +size_t android_image_get_valuable_size(const void *hdr_addr);
> +
>  struct andr_boot_img_hdr_v0;
>
>  /**
> --
> 2.40.1
>


-- 
Best regards - Atentamente - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
https://www.linkedin.com/in/iopaniuk