On 18.05.2024 12:40, Heinrich Schuchardt wrote:

On 5/13/24 20:17, Maxim Moskalets wrote:

...

From: Maxim Moskalets maximmosk4@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@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-do...

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

Not my code, but okay.

...

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",

It was in original implementation, but i can fix.

...

      /* 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.

Not my comment, i don't know.

...

/* 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-do...

...

• • 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?

Can i use dummy argc = 1 and argv = {"", NULL}; if (!argc && !argv)?

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]?

It is impossible for bootelf CLI command. In FIT we can describe behavior (in next patch with ELFs in FIT).

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.

Not in this patch, but okay. What about example in sandbox tests or/and example app?

Best regards

Maxim

Best regards

Heinrich

...

+}

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