Re: [PATCH v2 1/2] cmd: load: add load command for memory mapped

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Wed, May 11, 2022 at 10:55:40AM +0100, Rui Miguel Silva wrote:
> cp.b is used a lot as a way to load binaries to memory and execute
> them, however we may need to integrate this with the efi subsystem to
> set it up as a bootdev.

I'm not sure what your use case looks like, but even the current
bootefi supports an image from memory without "bootefi_device_path" set
(as you might know).
One of such cases is "bootefi hello". See efi_run_image().

> So, introduce a loadm command that will be consistent with the other
> loadX commands and will call the efi API's.
> 
> ex: loadm $kernel_addr $kernel_addr_r $kernel_size

So calling efi_clear_bootdev() in cp.b is a easier fix?
But it might lead to unexpected result in non-efi usage.

I don't object introducing "loadm" command, but
I'm afraid that it's not quite trivial (to most users) why we need loadm,
instead of cp.

-Takahiro Akashi

> with this a kernel with CONFIG_EFI_STUB enabled will be loaded and
> then subsequently booted with bootefi command.
> 
> Signed-off-by: Rui Miguel Silva <rui.silva@linaro.org>
> ---
>  README                           |  1 +
>  cmd/Kconfig                      |  5 +++
>  cmd/bootefi.c                    | 12 ++++++
>  cmd/load.c                       | 48 +++++++++++++++++++++
>  configs/sandbox64_defconfig      |  1 +
>  configs/sandbox_defconfig        |  1 +
>  doc/usage/loadm.rst              | 49 ++++++++++++++++++++++
>  include/efi_loader.h             |  2 +
>  include/test/suites.h            |  1 +
>  lib/efi_loader/efi_device_path.c |  9 ++++
>  test/cmd/Makefile                |  1 +
>  test/cmd/loadm.c                 | 72 ++++++++++++++++++++++++++++++++
>  test/cmd_ut.c                    |  6 +++
>  13 files changed, 208 insertions(+)
>  create mode 100644 doc/usage/loadm.rst
>  create mode 100644 test/cmd/loadm.c
> 
> diff --git a/README b/README
> index b7ab6e50708d..cd76f95e74c1 100644
> --- a/README
> +++ b/README
> @@ -2578,6 +2578,7 @@ rarpboot- boot image via network using RARP/TFTP protocol
>  diskboot- boot from IDE devicebootd   - boot default, i.e., run 'bootcmd'
>  loads	- load S-Record file over serial line
>  loadb	- load binary file over serial line (kermit mode)
> +loadm   - load binary blob from source address to destination address
>  md	- memory display
>  mm	- memory modify (auto-incrementing)
>  nm	- memory modify (constant address)
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 69c1814d24af..7f98cb16e2bc 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -1153,6 +1153,11 @@ config CMD_LOADB
>  	help
>  	  Load a binary file over serial line.
>  
> +config CMD_LOADM
> +	bool "loadm"
> +	help
> +	  Load a binary over memory mapped.
> +
>  config CMD_LOADS
>  	bool "loads"
>  	default y
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index d80353fa7189..53b584231f07 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -34,6 +34,18 @@ static struct efi_device_path *bootefi_device_path;
>  static void *image_addr;
>  static size_t image_size;
>  
> +/**
> + * efi_get_image_parameters() - return image parameters
> + *
> + * @img_addr:		address of loaded image in memory
> + * @img_size:		size of loaded image
> + */
> +void efi_get_image_parameters(void **img_addr, size_t *img_size)
> +{
> +	*img_addr = image_addr;
> +	*img_size = image_size;
> +}
> +
>  /**
>   * efi_clear_bootdev() - clear boot device
>   */
> diff --git a/cmd/load.c b/cmd/load.c
> index 7e4a552d90ef..1224a7f85bb3 100644
> --- a/cmd/load.c
> +++ b/cmd/load.c
> @@ -1063,6 +1063,44 @@ static ulong load_serial_ymodem(ulong offset, int mode)
>  
>  #endif
>  
> +#if defined(CONFIG_CMD_LOADM)
> +static int do_load_memory_bin(struct cmd_tbl *cmdtp, int flag, int argc,
> +			      char *const argv[])
> +{
> +	ulong	addr, dest, size;
> +	void	*src, *dst;
> +
> +	if (argc != 4)
> +		return CMD_RET_USAGE;
> +
> +	addr = simple_strtoul(argv[1], NULL, 16);
> +
> +	dest = simple_strtoul(argv[2], NULL, 16);
> +
> +	size = simple_strtoul(argv[3], NULL, 16);
> +
> +	if (!size) {
> +		printf("loadm: can not load zero bytes\n");
> +		return 1;
> +	}
> +
> +	src = map_sysmem(addr, size);
> +	dst = map_sysmem(dest, size);
> +
> +	memcpy(dst, src, size);
> +
> +	unmap_sysmem(src);
> +	unmap_sysmem(dst);
> +
> +	if (IS_ENABLED(CONFIG_CMD_BOOTEFI))
> +		efi_set_bootdev("Mem", "", "", map_sysmem(dest, 0), size);
> +
> +	printf("loaded bin to memory: size: %lu\n", size);
> +
> +	return 0;
> +}
> +#endif
> +
>  /* -------------------------------------------------------------------- */
>  
>  #if defined(CONFIG_CMD_LOADS)
> @@ -1137,3 +1175,13 @@ U_BOOT_CMD(
>  );
>  
>  #endif	/* CONFIG_CMD_LOADB */
> +
> +#if defined(CONFIG_CMD_LOADM)
> +U_BOOT_CMD(
> +	loadm, 4, 0,	do_load_memory_bin,
> +	"load binary blob from source address to destination address",
> +	"[src_addr] [dst_addr] [size]\n"
> +	"     - load a binary blob from one memory location to other"
> +	" from src_addr to dst_addr by size bytes"
> +);
> +#endif /* CONFIG_CMD_LOADM */
> diff --git a/configs/sandbox64_defconfig b/configs/sandbox64_defconfig
> index d7f22b39ae51..7ab5698b20cd 100644
> --- a/configs/sandbox64_defconfig
> +++ b/configs/sandbox64_defconfig
> @@ -47,6 +47,7 @@ CONFIG_CMD_GPT=y
>  CONFIG_CMD_GPT_RENAME=y
>  CONFIG_CMD_IDE=y
>  CONFIG_CMD_I2C=y
> +CONFIG_CMD_LOADM=y
>  CONFIG_CMD_OSD=y
>  CONFIG_CMD_PCI=y
>  CONFIG_CMD_READ=y
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index c509a924e6b3..82df5c2728e0 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -67,6 +67,7 @@ CONFIG_CMD_GPT=y
>  CONFIG_CMD_GPT_RENAME=y
>  CONFIG_CMD_IDE=y
>  CONFIG_CMD_I2C=y
> +CONFIG_CMD_LOADM=y
>  CONFIG_CMD_LSBLK=y
>  CONFIG_CMD_MUX=y
>  CONFIG_CMD_OSD=y
> diff --git a/doc/usage/loadm.rst b/doc/usage/loadm.rst
> new file mode 100644
> index 000000000000..b6571140437d
> --- /dev/null
> +++ b/doc/usage/loadm.rst
> @@ -0,0 +1,49 @@
> +.. SPDX-License-Identifier: GPL-2.0+:
> +
> +loadm command
> +=============
> +
> +Synopsis
> +--------
> +
> +::
> +
> +    loadm <src_addr> <dst_addr> <len>
> +
> +Description
> +-----------
> +
> +The loadm command is used to copy memory content from source address
> +to destination address and, if efi is enabled, will setup a "Mem" efi
> +boot device.
> +
> +The number of transferred bytes must be set by bytes parameter
> +
> +src_addr
> +    start address of the memory location to be loaded
> +
> +dst_addr
> +    destination address of the byte stream to be loaded
> +
> +len
> +    number of bytes to be copied in hexadecimal. Can not be 0 (zero).
> +
> +Example
> +-------
> +
> +::
> +
> +    => loadm ${kernel_addr} ${kernel_addr_r} ${kernel_size}
> +    loaded bin to memory: size: 12582912
> +
> +Configuration
> +-------------
> +
> +The command is only available if CONFIG_CMD_LOADM=y.
> +
> +Return value
> +------------
> +
> +The return value $? is set 0 (true) if the loading is succefull, and
> +is set to 1 (false) in case of error.
> +
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 733ee03cd77a..e051481a9766 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -589,6 +589,8 @@ efi_status_t efi_load_pe(struct efi_loaded_image_obj *handle,
>  void efi_save_gd(void);
>  /* Call this to relocate the runtime section to an address space */
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
> +/* Call this to get image parameters */
> +void efi_get_image_parameters(void **img_addr, size_t *img_size);
>  /* Add a new object to the object list. */
>  void efi_add_handle(efi_handle_t obj);
>  /* Create handle */
> diff --git a/include/test/suites.h b/include/test/suites.h
> index ee6858a802a5..ddb8827fdb15 100644
> --- a/include/test/suites.h
> +++ b/include/test/suites.h
> @@ -39,6 +39,7 @@ int do_ut_compression(struct cmd_tbl *cmdtp, int flag, int argc,
>  int do_ut_dm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_ut_env(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_ut_lib(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> +int do_ut_loadm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_ut_log(struct cmd_tbl *cmdtp, int flag, int argc, char * const argv[]);
>  int do_ut_mem(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
>  int do_ut_optee(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]);
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index 50a988c56136..afced03bff56 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -1143,6 +1143,8 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>  {
>  	struct blk_desc *desc = NULL;
>  	struct disk_partition fs_partition;
> +	size_t image_size;
> +	void *image_addr;
>  	int part = 0;
>  	char *filename;
>  	char *s;
> @@ -1158,6 +1160,13 @@ efi_status_t efi_dp_from_name(const char *dev, const char *devnr,
>  	} else if (!strcmp(dev, "Uart")) {
>  		if (device)
>  			*device = efi_dp_from_uart();
> +	} else if (!strcmp(dev, "Mem")) {
> +		efi_get_image_parameters(&image_addr, &image_size);
> +
> +		if (device)
> +			*device = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +						  (uintptr_t)image_addr,
> +						  image_size);
>  	} else {
>  		part = blk_get_device_part_str(dev, devnr, &desc, &fs_partition,
>  					       1);
> diff --git a/test/cmd/Makefile b/test/cmd/Makefile
> index a59adb1e6d60..4b2d7df0d2ef 100644
> --- a/test/cmd/Makefile
> +++ b/test/cmd/Makefile
> @@ -7,6 +7,7 @@ obj-$(CONFIG_CONSOLE_RECORD) += test_echo.o
>  endif
>  obj-y += mem.o
>  obj-$(CONFIG_CMD_ADDRMAP) += addrmap.o
> +obj-$(CONFIG_CMD_LOADM) += loadm.o
>  obj-$(CONFIG_CMD_MEM_SEARCH) += mem_search.o
>  obj-$(CONFIG_CMD_PINMUX) += pinmux.o
>  obj-$(CONFIG_CMD_PWM) += pwm.o
> diff --git a/test/cmd/loadm.c b/test/cmd/loadm.c
> new file mode 100644
> index 000000000000..41e005ac5923
> --- /dev/null
> +++ b/test/cmd/loadm.c
> @@ -0,0 +1,72 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Test for loadm command
> + *
> + * Copyright 2022 ARM Limited
> + * Copyright 2022 Linaro
> + *
> + * Authors:
> + *   Rui Miguel Silva <rui.silva@linaro.org>
> + */
> +
> +#include <common.h>
> +#include <console.h>
> +#include <mapmem.h>
> +#include <asm/global_data.h>
> +#include <dm/test.h>
> +#include <test/suites.h>
> +#include <test/test.h>
> +#include <test/ut.h>
> +
> +#define BUF_SIZE 0x100
> +
> +#define LOADM_TEST(_name, _flags)	UNIT_TEST(_name, _flags, loadm_test)
> +
> +static int loadm_test_params(struct unit_test_state *uts)
> +{
> +	ut_assertok(console_record_reset_enable());
> +	run_command("loadm", 0);
> +	ut_assert_nextline("loadm - load binary blob from source address to destination address");
> +
> +	ut_assertok(console_record_reset_enable());
> +	run_command("loadm 0x12345678", 0);
> +	ut_assert_nextline("loadm - load binary blob from source address to destination address");
> +
> +	ut_assertok(console_record_reset_enable());
> +	run_command("loadm 0x12345678 0x12345678", 0);
> +	ut_assert_nextline("loadm - load binary blob from source address to destination address");
> +
> +	ut_assertok(console_record_reset_enable());
> +	run_command("loadm 0x12345678 0x12345678 0", 0);
> +	ut_assert_nextline("loadm: can not load zero bytes");
> +
> +	return 0;
> +}
> +LOADM_TEST(loadm_test_params, UT_TESTF_CONSOLE_REC);
> +
> +static int loadm_test_load (struct unit_test_state *uts)
> +{
> +	char *buf;
> +
> +	buf = map_sysmem(0, BUF_SIZE);
> +	memset(buf, '\0', BUF_SIZE);
> +	memset(buf, 0xaa, BUF_SIZE / 2);
> +
> +	ut_assertok(console_record_reset_enable());
> +	run_command("loadm 0x0 0x80 0x80", 0);
> +	ut_assert_nextline("loaded bin to memory: size: 128");
> +
> +	unmap_sysmem(buf);
> +
> +	return 0;
> +}
> +LOADM_TEST(loadm_test_load, UT_TESTF_CONSOLE_REC);
> +
> +int do_ut_loadm(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> +{
> +	struct unit_test *tests = UNIT_TEST_SUITE_START(loadm_test);
> +	const int n_ents = UNIT_TEST_SUITE_COUNT(loadm_test);
> +
> +	return cmd_ut_category("loadm", "loadm_test_", tests, n_ents, argc,
> +			       argv);
> +}
> diff --git a/test/cmd_ut.c b/test/cmd_ut.c
> index 67a13ee32b8b..d70b72678aed 100644
> --- a/test/cmd_ut.c
> +++ b/test/cmd_ut.c
> @@ -74,6 +74,9 @@ static struct cmd_tbl cmd_ut_sub[] = {
>  #ifdef CONFIG_CMD_ADDRMAP
>  	U_BOOT_CMD_MKENT(addrmap, CONFIG_SYS_MAXARGS, 1, do_ut_addrmap, "", ""),
>  #endif
> +#ifdef CONFIG_CMD_LOADM
> +	U_BOOT_CMD_MKENT(loadm, CONFIG_SYS_MAXARGS, 1, do_ut_loadm, "", ""),
> +#endif
>  };
>  
>  static int do_ut_all(struct cmd_tbl *cmdtp, int flag, int argc,
> @@ -155,6 +158,9 @@ static char ut_help_text[] =
>  #endif
>  #ifdef CONFIG_CMD_ADDRMAP
>  	"ut addrmap - Very basic test of addrmap command\n"
> +#endif
> +#ifdef CONFIG_CMD_LOADM
> +	"ut loadm [test-name]- test of parameters and load memory blob\n"
>  #endif
>  	;
>  #endif /* CONFIG_SYS_LONGHELP */
> -- 
> 2.36.1
>