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

[Максим Москалец]

On 18.05.2024 12:40, Heinrich Schuchardt wrote:
> 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.
>

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

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