[PATCH v2] cmd: move ELF load and boot to lib/elf.c

[Heinrich Schuchardt]

On 5/13/24 20:17, Maxim Moskalets wrote:
> From: Maxim Moskalets <maximmosk4 at gmail.com>
>
> Loading and running the ELF image is the responsibility of the
> library and should not be associated with the command line interface.
>
> It is also required to run ELF images from FIT with the bootm command
> so as not to depend on the command line interface.
>
> Signed-off-by: Maxim Moskalets <maximmosk4 at gmail.com>
> ---
>   cmd/elf.c     | 49 +++++++++++++++++--------------------------------
>   include/elf.h | 10 ++++++++++
>   lib/elf.c     | 38 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 65 insertions(+), 32 deletions(-)
>
> diff --git a/cmd/elf.c b/cmd/elf.c
> index a02361f9f5..1fb955ae41 100644
> --- a/cmd/elf.c
> +++ b/cmd/elf.c
> @@ -19,21 +19,6 @@
>   #include <linux/linkage.h>
>   #endif
>
> -/* Allow ports to override the default behavior */
> -static unsigned long do_bootelf_exec(ulong (*entry)(int, char * const[]),
> -				     int argc, char *const argv[])
> -{
> -	unsigned long ret;
> -
> -	/*
> -	 * pass address parameter as argv[0] (aka command name),
> -	 * and all remaining args

Carving out the library function is a good idea.

Nits

%s/args/arguments/

> -	 */
> -	ret = entry(argc, argv);
> -
> -	return ret;
> -}
> -
>   /* Interpreter command to boot an arbitrary ELF image from memory */

Please, document functions fully:

https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

The preferred place for documenting exported functions is in the header
file.

>   int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
> @@ -43,8 +28,8 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   #endif
>   	unsigned long addr; /* Address of the ELF image */
>   	unsigned long rc; /* Return value from user code */
> -	char *sload = NULL;
>   	int rcode = 0;

Maybe

%s/0/CMD_RET_SUCCESS/

> +	Bootelf_flags flags = {0};
>
>   	/* Consume 'bootelf' */
>   	argc--; argv++;
> @@ -52,7 +37,10 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	/* Check for [-p|-s] flag. */
>   	if (argc >= 1 && (argv[0][0] == '-' && \
>   				(argv[0][1] == 'p' || argv[0][1] == 's'))) {
> -		sload = argv[0];
> +		if (argv[0][1] == 'p')
> +			flags.phdr = 1;
> +		printf("## Try to elf hdr format %s\n",
> +				flags.phdr ? "phdr" : "shdr");

Please, use log_debug() for debug messages and avoid the '## ' noise.

Do you mean:

log_debug("Using ELF header format %s\n",

>   		/* Consume flag. */
>   		argc--; argv++;
>   	}
> @@ -75,14 +63,6 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	} else
>   		addr = image_load_addr;
>
> -	if (!valid_elf_image(addr))
> -		return 1;
> -
> -	if (sload && sload[1] == 'p')
> -		addr = load_elf_image_phdr(addr);
> -	else
> -		addr = load_elf_image_shdr(addr);
> -
>   #if CONFIG_IS_ENABLED(CMD_ELF_FDT_SETUP)
>   	if (fdt_addr) {
>   		printf("## Setting up FDT at 0x%08lx ...\n", fdt_addr);

This looks like a debug message too.

> @@ -93,21 +73,26 @@ int do_bootelf(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	}
>   #endif
>
> -	if (!env_get_autostart())
> -		return rcode;
> -
> -	printf("## Starting application at 0x%08lx ...\n", addr);
> -	flush();
> +	if (env_get_autostart()) {
> +		flags.autostart = 1;
> +		printf("## Starting application at 0x%08lx ...\n", addr);

log_debug("Starting application at 0x%08lx ...\n", addr);

> +		flush();
> +	}
>
>   	/*
>   	 * pass address parameter as argv[0] (aka command name),
>   	 * and all remaining args
>   	 */
> -	rc = do_bootelf_exec((void *)addr, argc, argv);
> +	rc = bootelf(addr, flags, argc, argv);
>   	if (rc != 0)
>   		rcode = 1;

%s/1/CMD_RET_FAILURE/

>
> -	printf("## Application terminated, rc = 0x%lx\n", rc);
> +	if (flags.autostart) {
> +		if (ENOEXEC == errno)

if (rc && ENOEXEC == errno)

> +			printf("## Invalid ELF image\n");

log_err("Invalid ELF image\n");

> +		else
> +			printf("## Application terminated, rc = 0x%lx\n", rc);

This message is only relevant if the application failed:

else if (rc)
     log_err("Application failed, rc = 0x%lx\n", rc);

> +	}
>
>   	return rcode;
>   }
> diff --git a/include/elf.h b/include/elf.h
> index a4ba74d8ab..b88e6cf403 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -12,6 +12,12 @@
>   #ifndef __ASSEMBLY__
>   #include "compiler.h"
>
> +/* Flag param bits for bootelf() function */
> +typedef struct {
> +	unsigned phdr      : 1; /* load via program (not section) headers */
> +	unsigned autostart : 1; /* Start ELF after loading */
> +} Bootelf_flags;
> +
>   /* This version doesn't work for 64-bit ABIs - Erik */

If the elf command does not work in certain ABIs we need to ensure via
Kconfig that it is not build for these.

>
>   /* These typedefs need to be handled better */
> @@ -700,6 +706,10 @@ unsigned long elf_hash(const unsigned char *name);
>   #define R_RISCV_RELATIVE	3
>
>   #ifndef __ASSEMBLY__
> +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]),
> +			   int argc, char *const argv[]);
> +unsigned long bootelf(unsigned long addr, Bootelf_flags flags,
> +		      int argc, char *const argv[]);
>   int valid_elf_image(unsigned long addr);
>   unsigned long load_elf64_image_phdr(unsigned long addr);
>   unsigned long load_elf64_image_shdr(unsigned long addr);
> diff --git a/lib/elf.c b/lib/elf.c
> index 9a794f9cba..f8eeceef3d 100644
> --- a/lib/elf.c
> +++ b/lib/elf.c
> @@ -7,6 +7,7 @@
>   #include <cpu_func.h>
>   #include <elf.h>
>   #include <env.h>
> +#include <errno.h>
>   #include <net.h>
>   #include <vxworks.h>
>   #ifdef CONFIG_X86
> @@ -15,6 +16,43 @@
>   #include <linux/linkage.h>
>   #endif
>
> +/* Allow ports to override the default behavior */

Please, fully document the function.

> +unsigned long bootelf_exec(ulong (*entry)(int, char * const[]),
> +			   int argc, char *const argv[])
> +{
> +	return entry(argc, argv);
> +}
> +
> +/*
> + * @brief Boot ELF from memory
> + *
> + * @param addr       Loading address of ELF in memory
> + * @param flags      Bits like ELF_PHDR to control boot details
> + * @param argc, argv May be used to pass command line arguments (maybe unused)
> + *		     Necessary for backward compatibility with the CLI command
> + * @retuen Number returned by ELF application

This does not match our documentation style, see

https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation

> + *
> + * Sets errno = ENOEXEC if ELF image is not valid

%s/if ELF/if the ELF/

> + */
> +unsigned long bootelf(unsigned long addr, Bootelf_flags flags,
> +		      int argc, char *const argv[])
> +{
> +	unsigned long entry_addr;
> +
> +	if (!valid_elf_image(addr)) {
> +		errno = ENOEXEC;
> +		return 1;
> +	}
> +
> +	entry_addr = flags.phdr ? load_elf_image_phdr(addr)
> +					    : load_elf_image_shdr(addr);
> +
> +	if (!flags.autostart)
> +		return 0;

Please, return early. We don't need entry_addr here.

> +
> +	return bootelf_exec((void *)entry_addr, argc, argv);;

Duplicate ';'.

An ELF binary may expect that argc >= 1 and argv[0] being the binary
name. How do we ensure this?

We should describe in a man-page doc/usage/cmd/elf.rst that the first
extraneous argument passed to the elf command is argv[0]?

On some paths we have not set errno which means it has a random value
set in unrelated functions. How would we able to evaluate errno in the
caller if the return value is non-zero?

Please, initialize errno to 0 at the start of the function.

We need a test case. E.g. we could build an ELF binary returning the
CRC32 of the concatenated arguments.

Best regards

Heinrich

> +}
> +
>   /*
>    * A very simple ELF64 loader, assumes the image is valid, returns the
>    * entry point address.