On 18.05.2024 12:40, Heinrich Schuchardt wrote:
> On 5/13/24 20:17, Maxim Moskalets wrote:
>> From: Maxim Moskalets <maximmo...@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 <maximmo...@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.
>
