[PATCH v2 0/6] elf: add better bootm support

[PATCH v2 0/6] elf: add better bootm support

[Clement Leger]

Currently, when booting an elf file using "bootm /dev/mtdx", bootm will
simply pass the file to the bootm and the read done on it will read the
entire flash partition. This series starts by some cleanup and then add an
elf_open function to load the elf file size only based on the elf header.
A special handling for the elf file is also added in bootm data to allow
using directly the elf file structure. Finally the mips bootm is modified
to use bootm elf loading capability.

Changes v1 -> v2
 - Add BOOTM_ELF config to select elf support and add checks in code
 - Add an elf_get_mem_size function to avoid computing elf size in bootm.c
 - Use xmalloc and read_full in elf_open instead of xzalloc/read
 - Fix data->elf NULL reset
 - Remove elf struct entirely from mips bootm code

Clement Leger (6):
  common: elf: add computation of elf boundaries
  common: elf: fix warning on 32 bits architectures
  common: elf: split init to be reused from other function
  common: elf: add elf_open and elf_close
  common: bootm: add support for elf file loading
  mips: lib: bootm: use bootm elf loading capabilities

 arch/mips/lib/bootm.c |  27 +++--------
 common/Kconfig        |   8 ++++
 common/bootm.c        |  30 ++++++++++++
 common/elf.c          | 107 ++++++++++++++++++++++++++++++++++++++++--
 include/bootm.h       |   3 ++
 include/elf.h         |  14 ++++++
 6 files changed, 164 insertions(+), 25 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 1/6] common: elf: add computation of elf boundaries

[Clement Leger]

In order to correctly load an initrd or a device tree after an elf file,
we need to know its boundaries. This commit adds support for that and
allow the bootm implementations to use it for memory loading.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c  | 7 +++++++
 include/elf.h | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/common/elf.c b/common/elf.c
index 4733accb0..d64de401c 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -59,6 +59,11 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 	if (!p_filesz)
 		return 0;
 
+	if (dst < elf->low_addr)
+		elf->low_addr = dst;
+	if (dst + p_memsz > elf->high_addr)
+		elf->high_addr = dst + p_memsz;
+
 	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
 
 	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
@@ -124,6 +129,8 @@ struct elf_image *elf_load_image(void *buf)
 	INIT_LIST_HEAD(&elf->list);
 
 	elf->buf = buf;
+	elf->low_addr = (void *) (unsigned long) -1;
+	elf->high_addr = 0;
 
 	ret = elf_check_image(elf);
 	if (ret)
diff --git a/include/elf.h b/include/elf.h
index 113728f08..403412f3f 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -403,9 +403,16 @@ struct elf_image {
 	struct list_head list;
 	u8 class;
 	u64 entry;
+	void *low_addr;
+	void *high_addr;
 	void *buf;
 };
 
+static inline size_t elf_get_mem_size(struct elf_image *elf)
+{
+	return elf->high_addr - elf->low_addr;
+}
+
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(struct elf_image *elf);
 
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 2/6] common: elf: fix warning on 32 bits architectures

[Clement Leger]

When pointers are 32 bits wide and we cast a potentially 64 bits
value in it, the compiler will yield an error. Cast that value first
into a phys_addr_t to match the architecture pointer size and then
in a void *.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/elf.c b/common/elf.c
index d64de401c..55f5bc645 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -47,7 +47,7 @@ static void elf_release_regions(struct elf_image *elf)
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 				 void *phdr)
 {
-	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
+	void *dst = (void *) (phys_addr_t) elf_phdr_p_paddr(elf, phdr);
 	int ret;
 	u64 p_filesz = elf_phdr_p_filesz(elf, phdr);
 	u64 p_memsz = elf_phdr_p_memsz(elf, phdr);
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 3/6] common: elf: split init to be reused from other function

[Clement Leger]

New elf_open function will also need to initialize an elf file. Split
this to avoid missing members initialization.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 55f5bc645..5534632b2 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -119,6 +119,15 @@ static int elf_check_image(struct elf_image *elf)
 	return 0;
 }
 
+static int elf_check_init(struct elf_image *elf, void *buf)
+{
+	elf->buf = buf;
+	elf->low_addr = (void *) (unsigned long) -1;
+	elf->high_addr = 0;
+
+	return elf_check_image(elf);
+}
+
 struct elf_image *elf_load_image(void *buf)
 {
 	struct elf_image *elf;
@@ -128,13 +137,11 @@ struct elf_image *elf_load_image(void *buf)
 
 	INIT_LIST_HEAD(&elf->list);
 
-	elf->buf = buf;
-	elf->low_addr = (void *) (unsigned long) -1;
-	elf->high_addr = 0;
-
-	ret = elf_check_image(elf);
-	if (ret)
+	ret = elf_check_init(elf, buf);
+	if (ret) {
+		free(elf);
 		return ERR_PTR(ret);
+	}
 
 	ret = load_elf_image_phdr(elf);
 	if (ret) {
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 4/6] common: elf: add elf_open and elf_close

[Clement Leger]

When loading an elf file from a mtd device, this allows to parse the
header and load only the needed data according to the elf size. Without
that support, loading a elf file from a /dev/mtd would try to read the
entire partition.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/elf.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/elf.h |  7 +++++
 2 files changed, 90 insertions(+)

diff --git a/common/elf.c b/common/elf.c
index 5534632b2..291f883cb 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -5,7 +5,12 @@
 
 #include <common.h>
 #include <elf.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <libfile.h>
 #include <memory.h>
+#include <unistd.h>
+#include <linux/fs.h>
 
 struct elf_section {
 	struct list_head list;
@@ -158,3 +163,81 @@ void elf_release_image(struct elf_image *elf)
 
 	free(elf);
 }
+
+static u64 elf_get_size(struct elf_image *elf)
+{
+	u64 sh_size = elf_hdr_e_shentsize(elf, elf->buf) *
+		      elf_hdr_e_shnum(elf, elf->buf);
+
+	/*
+	 * The section header table is located at the end of the elf file thus
+	 * we can take the offset and add the size of this table to obtain the
+	 * file size.
+	 */
+	return elf_hdr_e_shoff(elf, elf->buf) + sh_size;
+}
+
+struct elf_image *elf_open(const char *filename)
+{
+	int fd, ret;
+	u64 size;
+	struct elf64_hdr hdr;
+	struct elf_image *elf;
+	ssize_t read_ret;
+
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		printf("could not open: %s\n", errno_str());
+		return ERR_PTR(-errno);
+	}
+
+	if (read(fd, &hdr, sizeof(hdr)) < 0) {
+		printf("could not read elf header: %s\n", errno_str());
+		ret = -errno;
+		goto err_close_fd;
+	}
+
+	elf = xzalloc(sizeof(*elf));
+
+	ret = elf_check_init(elf, &hdr);
+	if (ret) {
+		ret = -errno;
+		goto err_free_elf;
+	}
+
+	size = elf_get_size(elf);
+
+	elf->buf = xmalloc(size);
+
+	lseek(fd, 0, SEEK_SET);
+
+	read_ret = read_full(fd, elf->buf, size);
+	if (read_ret < 0) {
+		printf("could not read elf file: %s\n", errno_str());
+		ret = -errno;
+		goto err_free_buf;
+	}
+
+	ret = load_elf_image_phdr(elf);
+	if (ret)
+		goto err_release_elf;
+
+	return elf;
+
+err_release_elf:
+	elf_release_regions(elf);
+err_free_buf:
+	free(elf->buf);
+err_free_elf:
+	free(elf);
+err_close_fd:
+	close(fd);
+
+	return ERR_PTR(ret);
+}
+
+void elf_close(struct elf_image *elf)
+{
+	free(elf->buf);
+	elf_release_image(elf);
+}
diff --git a/include/elf.h b/include/elf.h
index 403412f3f..e9fe977a2 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -416,6 +416,9 @@ static inline size_t elf_get_mem_size(struct elf_image *elf)
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(struct elf_image *elf);
 
+struct elf_image *elf_open(const char *filename);
+void elf_close(struct elf_image *elf);
+
 #define ELF_GET_FIELD(__s, __field, __type) \
 static inline __type elf_##__s##_##__field(struct elf_image *elf, void *arg) { \
 	if (elf->class == ELFCLASS32) \
@@ -427,6 +430,10 @@ static inline __type elf_##__s##_##__field(struct elf_image *elf, void *arg) { \
 ELF_GET_FIELD(hdr, e_entry, u64)
 ELF_GET_FIELD(hdr, e_phnum, u16)
 ELF_GET_FIELD(hdr, e_phoff, u64)
+ELF_GET_FIELD(hdr, e_shoff, u64)
+ELF_GET_FIELD(hdr, e_shentsize, u16)
+ELF_GET_FIELD(hdr, e_machine, u16)
+ELF_GET_FIELD(hdr, e_shnum, u16)
 ELF_GET_FIELD(hdr, e_type, u16)
 ELF_GET_FIELD(phdr, p_paddr, u64)
 ELF_GET_FIELD(phdr, p_filesz, u64)
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 5/6] common: bootm: add support for elf file loading

[Clement Leger]

This will allows elf loader to directly have an elf file available. Thus
filetype_elf bootm handlers will be able to use this elf file directly.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 common/Kconfig  |  8 ++++++++
 common/bootm.c  | 30 ++++++++++++++++++++++++++++++
 include/bootm.h |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/common/Kconfig b/common/Kconfig
index 02ef3631e..24d85fd02 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -597,6 +597,14 @@ config BOOTM_AIMAGE
 	help
 	  Support using Android Images.
 
+config BOOTM_ELF
+	bool
+	depends on BOOTM
+	select ELF
+	prompt "elf loading support"
+	help
+	  Add support to load elf file with bootm.
+
 config BOOTM_FITIMAGE
 	bool
 	prompt "FIT image support"
diff --git a/common/bootm.c b/common/bootm.c
index 366f31455..b5ade12de 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -455,6 +455,8 @@ int bootm_get_os_size(struct image_data *data)
 {
 	int ret;
 
+	if (data->elf)
+		return elf_get_mem_size(data->elf);
 	if (data->os)
 		return uimage_get_size(data->os, uimage_part_num(data->os_part));
 	if (data->os_fit)
@@ -502,6 +504,22 @@ static int bootm_open_os_uimage(struct image_data *data)
 	return 0;
 }
 
+static int bootm_open_elf(struct image_data *data)
+{
+	if (!IS_ENABLED(CONFIG_ELF))
+		return -ENOSYS;
+
+	data->elf = elf_open(data->os_file);
+	if (IS_ERR(data->elf))
+		return PTR_ERR(data->elf);
+
+	printf("Entry Point:  %08llx\n", data->elf->entry);
+
+	data->os_address = data->elf->entry;
+
+	return 0;
+}
+
 static void bootm_print_info(struct image_data *data)
 {
 	if (data->os_res)
@@ -636,6 +654,16 @@ int bootm_boot(struct bootm_data *bootm_data)
 		}
 	}
 
+	if (os_type == filetype_elf) {
+		ret = bootm_open_elf(data);
+		if (ret) {
+			printf("Loading ELF image failed with: %s\n",
+					strerror(-ret));
+			data->elf = NULL;
+			goto err_out;
+		}
+	}
+
 	if (bootm_data->appendroot) {
 		char *rootarg;
 
@@ -705,6 +733,8 @@ err_out:
 		uimage_close(data->initrd);
 	if (data->os)
 		uimage_close(data->os);
+	if (IS_ENABLED(CONFIG_ELF) && data->elf)
+		elf_close(data->elf);
 	if (IS_ENABLED(CONFIG_FITIMAGE) && data->os_fit)
 		fit_close(data->os_fit);
 	if (data->of_root_node && data->of_root_node != of_get_root_node())
diff --git a/include/bootm.h b/include/bootm.h
index 7782de7a4..ef5148f31 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -46,6 +46,9 @@ struct image_data {
 	/* if os is an uImage this will be provided */
 	struct uimage_handle *os;
 
+	/* if os is an elf file this will be provided */
+	struct elf_image *elf;
+
 	/* if os is a FIT image this will be provided */
 	struct fit_handle *os_fit;
 
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 6/6] mips: lib: bootm: use bootm elf loading capabilities

[Clement Leger]

Now that the elf file is loaded by the bootm core, there is no need for
elf pointer anymore. Thus all elf related fields can be removed.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 arch/mips/lib/bootm.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 5bb09cc2d..c53a679c5 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -46,43 +46,30 @@ static struct binfmt_hook binfmt_barebox_hook = {
 static int do_bootm_elf(struct image_data *data)
 {
 	void (*entry)(int, void *);
-	struct elf_image *elf;
-	void *fdt, *buf;
+	void *fdt;
 	int ret = 0;
 
-	buf = read_file(data->os_file, NULL);
-	if (!buf)
-		return -EINVAL;
-
-	elf = elf_load_image(buf);
-	if (IS_ERR(elf))
-		return PTR_ERR(elf);
-
 	fdt = bootm_get_devicetree(data);
-	if (IS_ERR(fdt)) {
-		ret = PTR_ERR(fdt);
-		goto bootm_elf_done;
-	}
+	if (IS_ERR(fdt))
+		return PTR_ERR(fdt);
 
 	pr_info("Starting application at 0x%08lx, dts 0x%08lx...\n",
-		phys_to_virt(elf->entry), data->of_root_node);
+		phys_to_virt(data->os_address), data->of_root_node);
 
 	if (data->dryrun)
-		goto bootm_elf_done;
+		goto bootm_free_fdt;
 
 	shutdown_barebox();
 
-	entry = (void *) (unsigned long) elf->entry;
+	entry = (void *) (unsigned long) data->os_address;
 
 	entry(-2, phys_to_virt((unsigned long)fdt));
 
 	pr_err("ELF application terminated\n");
 	ret = -EINVAL;
 
-bootm_elf_done:
-	elf_release_image(elf);
+bootm_free_fdt:
 	free(fdt);
-	free(buf);
 
 	return ret;
 }
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH v2 6/6] mips: lib: bootm: use new data->elf member

[Clement Leger]

Now that the elf file is loaded by the bootm core, use this field
directly instead of manually loading the elf file.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
 arch/mips/lib/bootm.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index 5bb09cc2d..1e5fdd4ae 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -46,18 +46,10 @@ static struct binfmt_hook binfmt_barebox_hook = {
 static int do_bootm_elf(struct image_data *data)
 {
 	void (*entry)(int, void *);
-	struct elf_image *elf;
-	void *fdt, *buf;
+	struct elf_image *elf = data->elf;
+	void *fdt;
 	int ret = 0;
 
-	buf = read_file(data->os_file, NULL);
-	if (!buf)
-		return -EINVAL;
-
-	elf = elf_load_image(buf);
-	if (IS_ERR(elf))
-		return PTR_ERR(elf);
-
 	fdt = bootm_get_devicetree(data);
 	if (IS_ERR(fdt)) {
 		ret = PTR_ERR(fdt);
@@ -82,7 +74,6 @@ static int do_bootm_elf(struct image_data *data)
 bootm_elf_done:
 	elf_release_image(elf);
 	free(fdt);
-	free(buf);
 
 	return ret;
 }
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 6/6] mips: lib: bootm: use new data->elf member

[Clément Leger]

Hi,

----- On 23 Apr, 2020, at 10:17, Clément Leger cleger@kalray.eu wrote:

> Now that the elf file is loaded by the bootm core, use this field
> directly instead of manually loading the elf file.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
> arch/mips/lib/bootm.c | 13 ++-----------
> 1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
> index 5bb09cc2d..1e5fdd4ae 100644
> --- a/arch/mips/lib/bootm.c
> +++ b/arch/mips/lib/bootm.c
> @@ -46,18 +46,10 @@ static struct binfmt_hook binfmt_barebox_hook = {
> static int do_bootm_elf(struct image_data *data)
> {
> 	void (*entry)(int, void *);
> -	struct elf_image *elf;
> -	void *fdt, *buf;
> +	struct elf_image *elf = data->elf;
> +	void *fdt;
> 	int ret = 0;
> 
> -	buf = read_file(data->os_file, NULL);
> -	if (!buf)
> -		return -EINVAL;
> -
> -	elf = elf_load_image(buf);
> -	if (IS_ERR(elf))
> -		return PTR_ERR(elf);
> -
> 	fdt = bootm_get_devicetree(data);
> 	if (IS_ERR(fdt)) {
> 		ret = PTR_ERR(fdt);
> @@ -82,7 +74,6 @@ static int do_bootm_elf(struct image_data *data)
> bootm_elf_done:
> 	elf_release_image(elf);
> 	free(fdt);
> -	free(buf);
> 
> 	return ret;
> }

I'm sorry, This patch is a leftover from my previous serie and as such should be dropped.
The other mips patch cleans the MIPS elf loading code more thoroughly.

> --
> 2.17.1

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/6] elf: add better bootm support

[Antony Pavlov]

On Thu, 23 Apr 2020 10:17:05 +0200
Clement Leger <cleger@kalray.eu> wrote:

Hi, Clement

Just FYI. On MIPS barebox I use out-of-tree kexec-based ELF loader.

This efforts started in 2012 (sic!):
http://lists.infradead.org/pipermail/barebox/2012-December/011771.html

Alas! kexec patches are not mainlined still.

public kexec patches are available in my github repo, e.g.
https://github.com/frantony/barebox/commits/20170319.mips-malta-elf-linux

If your are interested in using kexec-style ELF loading I can prepare
patches on top of latest barebox master.

> Currently, when booting an elf file using "bootm /dev/mtdx", bootm will
> simply pass the file to the bootm and the read done on it will read the
> entire flash partition. This series starts by some cleanup and then add an
> elf_open function to load the elf file size only based on the elf header.
> A special handling for the elf file is also added in bootm data to allow
> using directly the elf file structure. Finally the mips bootm is modified
> to use bootm elf loading capability.
> 
> Changes v1 -> v2
>  - Add BOOTM_ELF config to select elf support and add checks in code
>  - Add an elf_get_mem_size function to avoid computing elf size in bootm.c
>  - Use xmalloc and read_full in elf_open instead of xzalloc/read
>  - Fix data->elf NULL reset
>  - Remove elf struct entirely from mips bootm code
> 
> Clement Leger (6):
>   common: elf: add computation of elf boundaries
>   common: elf: fix warning on 32 bits architectures
>   common: elf: split init to be reused from other function
>   common: elf: add elf_open and elf_close
>   common: bootm: add support for elf file loading
>   mips: lib: bootm: use bootm elf loading capabilities
> 
>  arch/mips/lib/bootm.c |  27 +++--------
>  common/Kconfig        |   8 ++++
>  common/bootm.c        |  30 ++++++++++++
>  common/elf.c          | 107 ++++++++++++++++++++++++++++++++++++++++--
>  include/bootm.h       |   3 ++
>  include/elf.h         |  14 ++++++
>  6 files changed, 164 insertions(+), 25 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
> _______________________________________________
> barebox mailing list
> barebox@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/barebox


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/6] elf: add better bootm support

[Clément Leger]

Hi Antony,

----- On 23 Apr, 2020, at 12:20, Antony Pavlov antonynpavlov@gmail.com wrote:

> On Thu, 23 Apr 2020 10:17:05 +0200
> Clement Leger <cleger@kalray.eu> wrote:
> 
> Hi, Clement
> 
> Just FYI. On MIPS barebox I use out-of-tree kexec-based ELF loader.
> 
> This efforts started in 2012 (sic!):
> http://lists.infradead.org/pipermail/barebox/2012-December/011771.html
> 
> Alas! kexec patches are not mainlined still.
> 
> public kexec patches are available in my github repo, e.g.
> https://github.com/frantony/barebox/commits/20170319.mips-malta-elf-linux
> 
> If your are interested in using kexec-style ELF loading I can prepare
> patches on top of latest barebox master.

If I understand correctly, the main advantage is to be able to load some code
over the currently running code by using some sort of "relocation" right ?
Currently, on KVX, we load Linux in the beginning of the DDR and barebox is
at DDR base + 256Mo so we don't have overlapping. But I bet that if we
had that it would be useful.

Anyway, even if we don't need it right now, that's a really nice feature !

From what I see from your commit, I think a part of it could be integrated in
the existing  elf parser (in elf_request_region) and add the segment
information in elf_section struct (the name is not really meaningful
since it stores the elf segments :D). And then a bootm option would allow
setting an "offset" at which will be loaded the elf waiting to be relocated
and then call the arch assembly code to do the relocation and boot ?

BTW, it makes me realize that currently, the elf is loaded when doing the
elf_open and not it would probably be better to do it in bootm_load_os as
for other file format. This would allow to open the elf file only in the
first time and load it later and could probably help you to integrate
kexec (ie load elf file only) and then do some kexec magic with the elf
struct.
Tell me if you think other improvements can be made.

Regards,

Clément

> 
>> Currently, when booting an elf file using "bootm /dev/mtdx", bootm will
>> simply pass the file to the bootm and the read done on it will read the
>> entire flash partition. This series starts by some cleanup and then add an
>> elf_open function to load the elf file size only based on the elf header.
>> A special handling for the elf file is also added in bootm data to allow
>> using directly the elf file structure. Finally the mips bootm is modified
>> to use bootm elf loading capability.
>> 
>> Changes v1 -> v2
>>  - Add BOOTM_ELF config to select elf support and add checks in code
>>  - Add an elf_get_mem_size function to avoid computing elf size in bootm.c
>>  - Use xmalloc and read_full in elf_open instead of xzalloc/read
>>  - Fix data->elf NULL reset
>>  - Remove elf struct entirely from mips bootm code
>> 
>> Clement Leger (6):
>>   common: elf: add computation of elf boundaries
>>   common: elf: fix warning on 32 bits architectures
>>   common: elf: split init to be reused from other function
>>   common: elf: add elf_open and elf_close
>>   common: bootm: add support for elf file loading
>>   mips: lib: bootm: use bootm elf loading capabilities
>> 
>>  arch/mips/lib/bootm.c |  27 +++--------
>>  common/Kconfig        |   8 ++++
>>  common/bootm.c        |  30 ++++++++++++
>>  common/elf.c          | 107 ++++++++++++++++++++++++++++++++++++++++--
>>  include/bootm.h       |   3 ++
>>  include/elf.h         |  14 ++++++
>>  6 files changed, 164 insertions(+), 25 deletions(-)
>> 
>> --
>> 2.17.1
>> 
>> 
>> _______________________________________________
>> barebox mailing list
>> barebox@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/barebox
> 
> 
> --
> Best regards,
>   Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 4/6] common: elf: add elf_open and elf_close

[Sascha Hauer]

On Thu, Apr 23, 2020 at 10:17:09AM +0200, Clement Leger wrote:
> When loading an elf file from a mtd device, this allows to parse the
> header and load only the needed data according to the elf size. Without
> that support, loading a elf file from a /dev/mtd would try to read the
> entire partition.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
>  common/elf.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/elf.h |  7 +++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 5534632b2..291f883cb 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -5,7 +5,12 @@
>  
>  #include <common.h>
>  #include <elf.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <libfile.h>
>  #include <memory.h>
> +#include <unistd.h>
> +#include <linux/fs.h>
>  
>  struct elf_section {
>  	struct list_head list;
> @@ -158,3 +163,81 @@ void elf_release_image(struct elf_image *elf)
>  
>  	free(elf);
>  }
> +
> +static u64 elf_get_size(struct elf_image *elf)
> +{
> +	u64 sh_size = elf_hdr_e_shentsize(elf, elf->buf) *
> +		      elf_hdr_e_shnum(elf, elf->buf);
> +
> +	/*
> +	 * The section header table is located at the end of the elf file thus
> +	 * we can take the offset and add the size of this table to obtain the
> +	 * file size.
> +	 */
> +	return elf_hdr_e_shoff(elf, elf->buf) + sh_size;
> +}
> +
> +struct elf_image *elf_open(const char *filename)
> +{
> +	int fd, ret;
> +	u64 size;
> +	struct elf64_hdr hdr;
> +	struct elf_image *elf;
> +	ssize_t read_ret;
> +
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
> +		printf("could not open: %s\n", errno_str());
> +		return ERR_PTR(-errno);
> +	}
> +
> +	if (read(fd, &hdr, sizeof(hdr)) < 0) {
> +		printf("could not read elf header: %s\n", errno_str());
> +		ret = -errno;
> +		goto err_close_fd;
> +	}
> +
> +	elf = xzalloc(sizeof(*elf));
> +
> +	ret = elf_check_init(elf, &hdr);
> +	if (ret) {
> +		ret = -errno;
> +		goto err_free_elf;
> +	}
> +
> +	size = elf_get_size(elf);
> +
> +	elf->buf = xmalloc(size);

As said before, please use malloc rather than one of the x-functions.
The regular malloc can fail when we are out of memory and then you have
a chance to fail gracefully.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 6/6] mips: lib: bootm: use bootm elf loading capabilities

[Sascha Hauer]

Antony, Oleksij,

Are you able to test this?

Sascha

On Thu, Apr 23, 2020 at 10:17:11AM +0200, Clement Leger wrote:
> Now that the elf file is loaded by the bootm core, there is no need for
> elf pointer anymore. Thus all elf related fields can be removed.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
>  arch/mips/lib/bootm.c | 27 +++++++--------------------
>  1 file changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
> index 5bb09cc2d..c53a679c5 100644
> --- a/arch/mips/lib/bootm.c
> +++ b/arch/mips/lib/bootm.c
> @@ -46,43 +46,30 @@ static struct binfmt_hook binfmt_barebox_hook = {
>  static int do_bootm_elf(struct image_data *data)
>  {
>  	void (*entry)(int, void *);
> -	struct elf_image *elf;
> -	void *fdt, *buf;
> +	void *fdt;
>  	int ret = 0;
>  
> -	buf = read_file(data->os_file, NULL);
> -	if (!buf)
> -		return -EINVAL;
> -
> -	elf = elf_load_image(buf);
> -	if (IS_ERR(elf))
> -		return PTR_ERR(elf);
> -
>  	fdt = bootm_get_devicetree(data);
> -	if (IS_ERR(fdt)) {
> -		ret = PTR_ERR(fdt);
> -		goto bootm_elf_done;
> -	}
> +	if (IS_ERR(fdt))
> +		return PTR_ERR(fdt);
>  
>  	pr_info("Starting application at 0x%08lx, dts 0x%08lx...\n",
> -		phys_to_virt(elf->entry), data->of_root_node);
> +		phys_to_virt(data->os_address), data->of_root_node);
>  
>  	if (data->dryrun)
> -		goto bootm_elf_done;
> +		goto bootm_free_fdt;
>  
>  	shutdown_barebox();
>  
> -	entry = (void *) (unsigned long) elf->entry;
> +	entry = (void *) (unsigned long) data->os_address;
>  
>  	entry(-2, phys_to_virt((unsigned long)fdt));
>  
>  	pr_err("ELF application terminated\n");
>  	ret = -EINVAL;
>  
> -bootm_elf_done:
> -	elf_release_image(elf);
> +bootm_free_fdt:
>  	free(fdt);
> -	free(buf);
>  
>  	return ret;
>  }
> -- 
> 2.17.1
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 4/6] common: elf: add elf_open and elf_close

[Clément Leger]

Hi Sascha,

----- On 28 Apr, 2020, at 08:39, Sascha Hauer s.hauer@pengutronix.de wrote:

> On Thu, Apr 23, 2020 at 10:17:09AM +0200, Clement Leger wrote:
>> When loading an elf file from a mtd device, this allows to parse the
>> header and load only the needed data according to the elf size. Without
>> that support, loading a elf file from a /dev/mtd would try to read the
>> entire partition.
>> 
>> Signed-off-by: Clement Leger <cleger@kalray.eu>
>> ---
>>  common/elf.c  | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  include/elf.h |  7 +++++
>>  2 files changed, 90 insertions(+)
>> 
>> diff --git a/common/elf.c b/common/elf.c
>> index 5534632b2..291f883cb 100644
>> --- a/common/elf.c
>> +++ b/common/elf.c
>> @@ -5,7 +5,12 @@
>>  
>>  #include <common.h>
>>  #include <elf.h>
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <libfile.h>
>>  #include <memory.h>
>> +#include <unistd.h>
>> +#include <linux/fs.h>
>>  
>>  struct elf_section {
>>  	struct list_head list;
>> @@ -158,3 +163,81 @@ void elf_release_image(struct elf_image *elf)
>>  
>>  	free(elf);
>>  }
>> +
>> +static u64 elf_get_size(struct elf_image *elf)
>> +{
>> +	u64 sh_size = elf_hdr_e_shentsize(elf, elf->buf) *
>> +		      elf_hdr_e_shnum(elf, elf->buf);
>> +
>> +	/*
>> +	 * The section header table is located at the end of the elf file thus
>> +	 * we can take the offset and add the size of this table to obtain the
>> +	 * file size.
>> +	 */
>> +	return elf_hdr_e_shoff(elf, elf->buf) + sh_size;
>> +}
>> +
>> +struct elf_image *elf_open(const char *filename)
>> +{
>> +	int fd, ret;
>> +	u64 size;
>> +	struct elf64_hdr hdr;
>> +	struct elf_image *elf;
>> +	ssize_t read_ret;
>> +
>> +	fd = open(filename, O_RDONLY);
>> +	if (fd < 0) {
>> +		printf("could not open: %s\n", errno_str());
>> +		return ERR_PTR(-errno);
>> +	}
>> +
>> +	if (read(fd, &hdr, sizeof(hdr)) < 0) {
>> +		printf("could not read elf header: %s\n", errno_str());
>> +		ret = -errno;
>> +		goto err_close_fd;
>> +	}
>> +
>> +	elf = xzalloc(sizeof(*elf));
>> +
>> +	ret = elf_check_init(elf, &hdr);
>> +	if (ret) {
>> +		ret = -errno;
>> +		goto err_free_elf;
>> +	}
>> +
>> +	size = elf_get_size(elf);
>> +
>> +	elf->buf = xmalloc(size);
> 
> As said before, please use malloc rather than one of the x-functions.
> The regular malloc can fail when we are out of memory and then you have
> a chance to fail gracefully.

Sorry, I understood that after and I have a v3 with this fix amongst other.
I also fixed bootm_load_os to load the existing elf file if any and modified
mips bootm accordingly. I will send it today.

Clément

> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH v2 0/6] elf: add better bootm support

[Antony Pavlov]

On Thu, 23 Apr 2020 13:06:17 +0200 (CEST)
Clément Leger <cleger@kalray.eu> wrote:

> Hi Antony,
> 
> ----- On 23 Apr, 2020, at 12:20, Antony Pavlov antonynpavlov@gmail.com wrote:
> 
> > On Thu, 23 Apr 2020 10:17:05 +0200
> > Clement Leger <cleger@kalray.eu> wrote:
> > 
> > Hi, Clement
> > 
> > Just FYI. On MIPS barebox I use out-of-tree kexec-based ELF loader.
> > 
> > This efforts started in 2012 (sic!):
> > http://lists.infradead.org/pipermail/barebox/2012-December/011771.html
> > 
> > Alas! kexec patches are not mainlined still.
> > 
> > public kexec patches are available in my github repo, e.g.
> > https://github.com/frantony/barebox/commits/20170319.mips-malta-elf-linux
> > 
> > If your are interested in using kexec-style ELF loading I can prepare
> > patches on top of latest barebox master.
> 
> If I understand correctly, the main advantage is to be able to load some code
> over the currently running code by using some sort of "relocation" right ?

Yes, you are right.

> Currently, on KVX, we load Linux in the beginning of the DDR and barebox is
> at DDR base + 256Mo so we don't have overlapping. But I bet that if we
> had that it would be useful.

On MIPS the situation is just the same: on start barebox relocates itself
to the top of memory (thank to 2019 Oleksij's patchseries)
so linux kernel and barebox overlaping is unlikely. So kexec-style ELF
loading was actual on MIPS before 2019.


> Anyway, even if we don't need it right now, that's a really nice feature !
> 
> From what I see from your commit, I think a part of it could be integrated in
> the existing  elf parser (in elf_request_region) and add the segment
> information in elf_section struct (the name is not really meaningful
> since it stores the elf segments :D). And then a bootm option would allow
> setting an "offset" at which will be loaded the elf waiting to be relocated
> and then call the arch assembly code to do the relocation and boot ?
> 
> BTW, it makes me realize that currently, the elf is loaded when doing the
> elf_open and not it would probably be better to do it in bootm_load_os as
> for other file format. This would allow to open the elf file only in the
> first time and load it later and could probably help you to integrate
> kexec (ie load elf file only) and then do some kexec magic with the elf
> struct.
> Tell me if you think other improvements can be made.

First of all I have to check your v3 patchseries on MIPS.
The main problem of my kexec elf load patchseries that it was tested only on MIPS.




> Regards,
> 
> Clément
> 
> > 
> >> Currently, when booting an elf file using "bootm /dev/mtdx", bootm will
> >> simply pass the file to the bootm and the read done on it will read the
> >> entire flash partition. This series starts by some cleanup and then add an
> >> elf_open function to load the elf file size only based on the elf header.
> >> A special handling for the elf file is also added in bootm data to allow
> >> using directly the elf file structure. Finally the mips bootm is modified
> >> to use bootm elf loading capability.
> >> 
> >> Changes v1 -> v2
> >>  - Add BOOTM_ELF config to select elf support and add checks in code
> >>  - Add an elf_get_mem_size function to avoid computing elf size in bootm.c
> >>  - Use xmalloc and read_full in elf_open instead of xzalloc/read
> >>  - Fix data->elf NULL reset
> >>  - Remove elf struct entirely from mips bootm code
> >> 
> >> Clement Leger (6):
> >>   common: elf: add computation of elf boundaries
> >>   common: elf: fix warning on 32 bits architectures
> >>   common: elf: split init to be reused from other function
> >>   common: elf: add elf_open and elf_close
> >>   common: bootm: add support for elf file loading
> >>   mips: lib: bootm: use bootm elf loading capabilities
> >> 
> >>  arch/mips/lib/bootm.c |  27 +++--------
> >>  common/Kconfig        |   8 ++++
> >>  common/bootm.c        |  30 ++++++++++++
> >>  common/elf.c          | 107 ++++++++++++++++++++++++++++++++++++++++--
> >>  include/bootm.h       |   3 ++
> >>  include/elf.h         |  14 ++++++
> >>  6 files changed, 164 insertions(+), 25 deletions(-)
> >> 
> >> --
> >> 2.17.1
> >> 
> >> 
> >> _______________________________________________
> >> barebox mailing list
> >> barebox@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/barebox
> > 
> > 
> > --
> > Best regards,
> >   Antony Pavlov


-- 
Best regards,
  Antony Pavlov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox