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

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

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