[PATCH] elf: add 64 bits elf support

[PATCH] elf: add 64 bits elf support

[Clément Leger]

This patch add elf64 loading support to the elf loader. Since
elf32 and elf64 uses completely different types, to avoid copying all
the code and simply replace elf32 with elf64, use a macro which will
return the appropriate field for each type of header. This macro
generates getter for elf structures according to the class of the loaded
elf.
All direct elf struct dereference are then replaced by call to generated
functions. This allows to keep a common loader code even if types are
different.

Signed-off-by: Clément Léger <clement.leger@kalray.eu>
---
 common/elf.c  | 46 +++++++++++++++++++++++-----------------------
 include/elf.h | 27 +++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 23 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 8edf38856..bfb878954 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
 	}
 }
 
-
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
-				 Elf32_Phdr *phdr)
+				 void *phdr)
 {
-	void *dst = (void *)phdr->p_paddr;
+	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
 	int ret;
+	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
+	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
 
 	/* we care only about PT_LOAD segments */
-	if (phdr->p_type != PT_LOAD)
+	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
 		return 0;
 
-	if (!phdr->p_filesz)
+	if (!p_filesz)
 		return 0;
 
-	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
+	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
 
-	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
+	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
 	if (ret)
 		return ret;
 
-	memcpy(dst, src, phdr->p_filesz);
+	memcpy(dst, src, p_filesz);
 
-	if (phdr->p_filesz < phdr->p_memsz)
-		memset(dst + phdr->p_filesz, 0x00,
-		       phdr->p_memsz - phdr->p_filesz);
+	if (p_filesz < p_memsz)
+		memset(dst + p_filesz, 0x00,
+		       p_memsz - p_filesz);
 
 	return 0;
 }
@@ -75,14 +76,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 static int load_elf_image_phdr(struct elf_image *elf)
 {
 	void *buf = elf->buf;
-	Elf32_Ehdr *ehdr = buf;
-	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
+	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
 	int i, ret;
 
-	elf->entry = ehdr->e_entry;
+	elf->entry = elf_hdr_e_entry(elf, buf);
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *src = buf + phdr->p_offset;
+	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
+		void *src = buf + elf_phdr_p_offset(elf, phdr);
 
 		ret = load_elf_phdr_segment(elf, src, phdr);
 		/* in case of error elf_load_image() caller should clean up and
@@ -90,22 +90,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
 		if (ret)
 			return ret;
 
-		++phdr;
+		phdr += elf_size_of_phdr(elf);
 	}
 
 	return 0;
 }
 
-static int elf_check_image(void *buf)
+static int elf_check_image(struct elf_image *elf)
 {
-	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
-
-	if (strncmp(buf, ELFMAG, SELFMAG)) {
+	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
 		pr_err("ELF magic not found.\n");
 		return -EINVAL;
 	}
 
-	if (ehdr->e_type != ET_EXEC) {
+	elf->class = ((char *) elf->buf)[EI_CLASS];
+
+	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
 		pr_err("Non EXEC ELF image.\n");
 		return -ENOEXEC;
 	}
@@ -124,7 +124,7 @@ struct elf_image *elf_load_image(void *buf)
 
 	elf->buf = buf;
 
-	ret = elf_check_image(buf);
+	ret = elf_check_image(elf);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/include/elf.h b/include/elf.h
index 92c8d9c12..189a34566 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
 
 struct elf_image {
 	struct list_head list;
+	unsigned long class;
 	unsigned long entry;
 	void *buf;
 };
@@ -407,4 +408,30 @@ struct elf_image {
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(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) \
+		return (__type) ((struct elf32_##__s *) arg)->__field; \
+	else \
+		return (__type) ((struct elf64_##__s *) arg)->__field; \
+}
+
+ELF_GET_FIELD(hdr, e_entry, unsigned long)
+ELF_GET_FIELD(hdr, e_phnum, unsigned long)
+ELF_GET_FIELD(hdr, e_phoff, unsigned long)
+ELF_GET_FIELD(hdr, e_type, unsigned long)
+ELF_GET_FIELD(phdr, p_paddr, unsigned long)
+ELF_GET_FIELD(phdr, p_filesz, unsigned long)
+ELF_GET_FIELD(phdr, p_memsz, unsigned long)
+ELF_GET_FIELD(phdr, p_type, unsigned long)
+ELF_GET_FIELD(phdr, p_offset, unsigned long)
+
+static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
+{
+	if (elf->class == ELFCLASS32)
+		return sizeof(Elf32_Phdr);
+	else
+		return sizeof(Elf64_Phdr);
+}
+
 #endif /* _LINUX_ELF_H */
-- 
2.15.0.276.g89ea799

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

Re: [PATCH] elf: add 64 bits elf support

[Sascha Hauer]

Hi Clément,

On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. This allows to keep a common loader code even if types are
> different.
> 
> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
> ---
>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
>  include/elf.h | 27 +++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 8edf38856..bfb878954 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
>  	}
>  }
>  
> -
>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> -				 Elf32_Phdr *phdr)
> +				 void *phdr)
>  {
> -	void *dst = (void *)phdr->p_paddr;
> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
>  	int ret;
> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
>  
>  	/* we care only about PT_LOAD segments */
> -	if (phdr->p_type != PT_LOAD)
> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>  		return 0;
>  
> -	if (!phdr->p_filesz)
> +	if (!p_filesz)
>  		return 0;
>  
> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);

%lu for p_filesz?

> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
>  
>  struct elf_image {
>  	struct list_head list;
> +	unsigned long class;
>  	unsigned long entry;
>  	void *buf;
>  };
> @@ -407,4 +408,30 @@ struct elf_image {
>  struct elf_image *elf_load_image(void *buf);
>  void elf_release_image(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) \
> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> +	else \
> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> +}
> +
> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
> +ELF_GET_FIELD(hdr, e_type, unsigned long)
> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
> +ELF_GET_FIELD(phdr, p_type, unsigned long)
> +ELF_GET_FIELD(phdr, p_offset, unsigned long)

When it's always unsigned long why do we have to pass in the type as an
argument?

I am undecided if this is the right approach. "unsigned long" is wrong
when a ELF file for a foreign architecture is loaded. This can happen
for example when code for the Cortex M4 cores is loaded from the 64bit
Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
then is not a problem, but maybe it could happen the other way round,
loading a 64bit ELF on a 32bit architecture?

I can't see a real problem here, I just wanted to note. Are there other
opinions?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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] elf: add 64 bits elf support

[Clément Leger]

Hi Sascha,

> Hi Clément,
> 
> On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
>> This patch add elf64 loading support to the elf loader. Since
>> elf32 and elf64 uses completely different types, to avoid copying all
>> the code and simply replace elf32 with elf64, use a macro which will
>> return the appropriate field for each type of header. This macro
>> generates getter for elf structures according to the class of the loaded
>> elf.
>> All direct elf struct dereference are then replaced by call to generated
>> functions. This allows to keep a common loader code even if types are
>> different.
>> 
>> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
>> ---
>>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
>>  include/elf.h | 27 +++++++++++++++++++++++++++
>>  2 files changed, 50 insertions(+), 23 deletions(-)
>> 
>> diff --git a/common/elf.c b/common/elf.c
>> index 8edf38856..bfb878954 100644
>> --- a/common/elf.c
>> +++ b/common/elf.c
>> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
>>  	}
>>  }
>>  
>> -
>>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>> -				 Elf32_Phdr *phdr)
>> +				 void *phdr)
>>  {
>> -	void *dst = (void *)phdr->p_paddr;
>> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
>>  	int ret;
>> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
>> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
>>  
>>  	/* we care only about PT_LOAD segments */
>> -	if (phdr->p_type != PT_LOAD)
>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>>  		return 0;
>>  
>> -	if (!phdr->p_filesz)
>> +	if (!p_filesz)
>>  		return 0;
>>  
>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
>> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
> 
> %lu for p_filesz?

Indeed, I missed this one.

> 
>> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
>>  
>>  struct elf_image {
>>  	struct list_head list;
>> +	unsigned long class;
>>  	unsigned long entry;
>>  	void *buf;
>>  };
>> @@ -407,4 +408,30 @@ struct elf_image {
>>  struct elf_image *elf_load_image(void *buf);
>>  void elf_release_image(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) \
>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
>> +	else \
>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
>> +}
>> +
>> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
>> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
>> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
>> +ELF_GET_FIELD(hdr, e_type, unsigned long)
>> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
>> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
>> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
>> +ELF_GET_FIELD(phdr, p_type, unsigned long)
>> +ELF_GET_FIELD(phdr, p_offset, unsigned long)
> 
> When it's always unsigned long why do we have to pass in the type as an
> argument?

Actually, some of them should not be defined as I did.
For instance, the e_type is an half in both elf32 and elf64 so it should
be defined as u16.

Some other approaches to handle both 64bits/32bits elf were to copy
the whole loading code and s/elf32/elf64. Since the code in barebox is
not so big, maybe I could do that.

> 
> I am undecided if this is the right approach. "unsigned long" is wrong
> when a ELF file for a foreign architecture is loaded. This can happen
> for example when code for the Cortex M4 cores is loaded from the 64bit
> Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
> then is not a problem, but maybe it could happen the other way round,
> loading a 64bit ELF on a 32bit architecture?

I was thinking about this one. I tried loading 32bit and 64bit elf from
a 64bit core but indeed, not the other way. If so, then addresses will
be truncated but since the processor will not be able to access a
64 bits memory space, I guess it's not possible (unless you have some
DMA which can access the upper memory but this will probably not be
handled by barebox elf loader).

> 
> I can't see a real problem here, I just wanted to note. Are there other
> opinions?
> 
> Sascha
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 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] elf: add 64 bits elf support

[Clément Leger]

Here is a V2 which uses correct type for elf header access macros
(instead of simply unsigned long). Moreover types used are now
of fixed size type (u64 instead of unsigned long). This could
potentially allow a 32bit barebox to load a 64bit elf using some custom
hardware which support 64bit addressing (DMA or such thing).


This patch add elf64 loading support to the elf loader. Since
elf32 and elf64 uses completely different types, to avoid copying all
the code and simply replace elf32 with elf64, use a macro which will
return the appropriate field for each type of header. This macro
generates getter for elf structures according to the class of the loaded
elf.
All direct elf struct dereference are then replaced by call to generated
functions. This allows to keep a common loader code even if types are
different.
---
 common/elf.c  | 45 +++++++++++++++++++++++----------------------
 include/elf.h | 29 ++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 8edf38856..4733accb0 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
 
 
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
-				 Elf32_Phdr *phdr)
+				 void *phdr)
 {
-	void *dst = (void *)phdr->p_paddr;
+	void *dst = (void *) 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);
 
 	/* we care only about PT_LOAD segments */
-	if (phdr->p_type != PT_LOAD)
+	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
 		return 0;
 
-	if (!phdr->p_filesz)
+	if (!p_filesz)
 		return 0;
 
-	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
+	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
 
-	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
+	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
 	if (ret)
 		return ret;
 
-	memcpy(dst, src, phdr->p_filesz);
+	memcpy(dst, src, p_filesz);
 
-	if (phdr->p_filesz < phdr->p_memsz)
-		memset(dst + phdr->p_filesz, 0x00,
-		       phdr->p_memsz - phdr->p_filesz);
+	if (p_filesz < p_memsz)
+		memset(dst + p_filesz, 0x00,
+		       p_memsz - p_filesz);
 
 	return 0;
 }
@@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 static int load_elf_image_phdr(struct elf_image *elf)
 {
 	void *buf = elf->buf;
-	Elf32_Ehdr *ehdr = buf;
-	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
+	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
 	int i, ret;
 
-	elf->entry = ehdr->e_entry;
+	elf->entry = elf_hdr_e_entry(elf, buf);
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *src = buf + phdr->p_offset;
+	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
+		void *src = buf + elf_phdr_p_offset(elf, phdr);
 
 		ret = load_elf_phdr_segment(elf, src, phdr);
 		/* in case of error elf_load_image() caller should clean up and
@@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
 		if (ret)
 			return ret;
 
-		++phdr;
+		phdr += elf_size_of_phdr(elf);
 	}
 
 	return 0;
 }
 
-static int elf_check_image(void *buf)
+static int elf_check_image(struct elf_image *elf)
 {
-	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
-
-	if (strncmp(buf, ELFMAG, SELFMAG)) {
+	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
 		pr_err("ELF magic not found.\n");
 		return -EINVAL;
 	}
 
-	if (ehdr->e_type != ET_EXEC) {
+	elf->class = ((char *) elf->buf)[EI_CLASS];
+
+	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
 		pr_err("Non EXEC ELF image.\n");
 		return -ENOEXEC;
 	}
@@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
 
 	elf->buf = buf;
 
-	ret = elf_check_image(buf);
+	ret = elf_check_image(elf);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/include/elf.h b/include/elf.h
index 92c8d9c12..633f4992d 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
 
 struct elf_image {
 	struct list_head list;
-	unsigned long entry;
+	u8 class;
+	u64 entry;
 	void *buf;
 };
 
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(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) \
+		return (__type) ((struct elf32_##__s *) arg)->__field; \
+	else \
+		return (__type) ((struct elf64_##__s *) arg)->__field; \
+}
+
+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_type, u16)
+ELF_GET_FIELD(phdr, p_paddr, u64)
+ELF_GET_FIELD(phdr, p_filesz, u64)
+ELF_GET_FIELD(phdr, p_memsz, u64)
+ELF_GET_FIELD(phdr, p_type, u32)
+ELF_GET_FIELD(phdr, p_offset, u64)
+
+static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
+{
+	if (elf->class == ELFCLASS32)
+		return sizeof(Elf32_Phdr);
+	else
+		return sizeof(Elf64_Phdr);
+}
+
 #endif /* _LINUX_ELF_H */
-- 
2.15.0.276.g89ea799




> Hi Sascha,
> 
>> Hi Clément,
>> 
>> On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
>>> This patch add elf64 loading support to the elf loader. Since
>>> elf32 and elf64 uses completely different types, to avoid copying all
>>> the code and simply replace elf32 with elf64, use a macro which will
>>> return the appropriate field for each type of header. This macro
>>> generates getter for elf structures according to the class of the loaded
>>> elf.
>>> All direct elf struct dereference are then replaced by call to generated
>>> functions. This allows to keep a common loader code even if types are
>>> different.
>>> 
>>> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
>>> ---
>>>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
>>>  include/elf.h | 27 +++++++++++++++++++++++++++
>>>  2 files changed, 50 insertions(+), 23 deletions(-)
>>> 
>>> diff --git a/common/elf.c b/common/elf.c
>>> index 8edf38856..bfb878954 100644
>>> --- a/common/elf.c
>>> +++ b/common/elf.c
>>> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
>>>  	}
>>>  }
>>>  
>>> -
>>>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>>> -				 Elf32_Phdr *phdr)
>>> +				 void *phdr)
>>>  {
>>> -	void *dst = (void *)phdr->p_paddr;
>>> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
>>>  	int ret;
>>> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
>>> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
>>>  
>>>  	/* we care only about PT_LOAD segments */
>>> -	if (phdr->p_type != PT_LOAD)
>>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>>>  		return 0;
>>>  
>>> -	if (!phdr->p_filesz)
>>> +	if (!p_filesz)
>>>  		return 0;
>>>  
>>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
>>> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
>> 
>> %lu for p_filesz?
> 
> Indeed, I missed this one.
> 
>> 
>>> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
>>>  
>>>  struct elf_image {
>>>  	struct list_head list;
>>> +	unsigned long class;
>>>  	unsigned long entry;
>>>  	void *buf;
>>>  };
>>> @@ -407,4 +408,30 @@ struct elf_image {
>>>  struct elf_image *elf_load_image(void *buf);
>>>  void elf_release_image(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) \
>>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
>>> +	else \
>>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
>>> +}
>>> +
>>> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
>>> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
>>> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
>>> +ELF_GET_FIELD(hdr, e_type, unsigned long)
>>> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
>>> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
>>> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
>>> +ELF_GET_FIELD(phdr, p_type, unsigned long)
>>> +ELF_GET_FIELD(phdr, p_offset, unsigned long)
>> 
>> When it's always unsigned long why do we have to pass in the type as an
>> argument?
> 
> Actually, some of them should not be defined as I did.
> For instance, the e_type is an half in both elf32 and elf64 so it should
> be defined as u16.
> 
> Some other approaches to handle both 64bits/32bits elf were to copy
> the whole loading code and s/elf32/elf64. Since the code in barebox is
> not so big, maybe I could do that.
> 
>> 
>> I am undecided if this is the right approach. "unsigned long" is wrong
>> when a ELF file for a foreign architecture is loaded. This can happen
>> for example when code for the Cortex M4 cores is loaded from the 64bit
>> Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
>> then is not a problem, but maybe it could happen the other way round,
>> loading a 64bit ELF on a 32bit architecture?
> 
> I was thinking about this one. I tried loading 32bit and 64bit elf from
> a 64bit core but indeed, not the other way. If so, then addresses will
> be truncated but since the processor will not be able to access a
> 64 bits memory space, I guess it's not possible (unless you have some
> DMA which can access the upper memory but this will probably not be
> handled by barebox elf loader).
> 
>> 
>> I can't see a real problem here, I just wanted to note. Are there other
>> opinions?
>> 
>> Sascha
>> 
>> --
>> Pengutronix e.K.                           |                             |
>> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>> Peiner Str. 6-8, 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

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

Re: [PATCH] elf: add 64 bits elf support

[Sascha Hauer]

Hi Clément,

I guess this version looks good. Could you resend with a Signed-off-by
tag?

Sascha

On Wed, Mar 20, 2019 at 05:28:44PM +0100, Clément Leger wrote:
> Here is a V2 which uses correct type for elf header access macros
> (instead of simply unsigned long). Moreover types used are now
> of fixed size type (u64 instead of unsigned long). This could
> potentially allow a 32bit barebox to load a 64bit elf using some custom
> hardware which support 64bit addressing (DMA or such thing).
> 
> 
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. This allows to keep a common loader code even if types are
> different.
> ---
>  common/elf.c  | 45 +++++++++++++++++++++++----------------------
>  include/elf.h | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 8edf38856..4733accb0 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
>  
>  
>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> -				 Elf32_Phdr *phdr)
> +				 void *phdr)
>  {
> -	void *dst = (void *)phdr->p_paddr;
> +	void *dst = (void *) 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);
>  
>  	/* we care only about PT_LOAD segments */
> -	if (phdr->p_type != PT_LOAD)
> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>  		return 0;
>  
> -	if (!phdr->p_filesz)
> +	if (!p_filesz)
>  		return 0;
>  
> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> +	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
>  
> -	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
> +	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
>  	if (ret)
>  		return ret;
>  
> -	memcpy(dst, src, phdr->p_filesz);
> +	memcpy(dst, src, p_filesz);
>  
> -	if (phdr->p_filesz < phdr->p_memsz)
> -		memset(dst + phdr->p_filesz, 0x00,
> -		       phdr->p_memsz - phdr->p_filesz);
> +	if (p_filesz < p_memsz)
> +		memset(dst + p_filesz, 0x00,
> +		       p_memsz - p_filesz);
>  
>  	return 0;
>  }
> @@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>  static int load_elf_image_phdr(struct elf_image *elf)
>  {
>  	void *buf = elf->buf;
> -	Elf32_Ehdr *ehdr = buf;
> -	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
> +	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
>  	int i, ret;
>  
> -	elf->entry = ehdr->e_entry;
> +	elf->entry = elf_hdr_e_entry(elf, buf);
>  
> -	for (i = 0; i < ehdr->e_phnum; ++i) {
> -		void *src = buf + phdr->p_offset;
> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
> +		void *src = buf + elf_phdr_p_offset(elf, phdr);
>  
>  		ret = load_elf_phdr_segment(elf, src, phdr);
>  		/* in case of error elf_load_image() caller should clean up and
> @@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
>  		if (ret)
>  			return ret;
>  
> -		++phdr;
> +		phdr += elf_size_of_phdr(elf);
>  	}
>  
>  	return 0;
>  }
>  
> -static int elf_check_image(void *buf)
> +static int elf_check_image(struct elf_image *elf)
>  {
> -	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
> -
> -	if (strncmp(buf, ELFMAG, SELFMAG)) {
> +	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
>  		pr_err("ELF magic not found.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (ehdr->e_type != ET_EXEC) {
> +	elf->class = ((char *) elf->buf)[EI_CLASS];
> +
> +	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
>  		pr_err("Non EXEC ELF image.\n");
>  		return -ENOEXEC;
>  	}
> @@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
>  
>  	elf->buf = buf;
>  
> -	ret = elf_check_image(buf);
> +	ret = elf_check_image(elf);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/include/elf.h b/include/elf.h
> index 92c8d9c12..633f4992d 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
>  
>  struct elf_image {
>  	struct list_head list;
> -	unsigned long entry;
> +	u8 class;
> +	u64 entry;
>  	void *buf;
>  };
>  
>  struct elf_image *elf_load_image(void *buf);
>  void elf_release_image(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) \
> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> +	else \
> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> +}
> +
> +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_type, u16)
> +ELF_GET_FIELD(phdr, p_paddr, u64)
> +ELF_GET_FIELD(phdr, p_filesz, u64)
> +ELF_GET_FIELD(phdr, p_memsz, u64)
> +ELF_GET_FIELD(phdr, p_type, u32)
> +ELF_GET_FIELD(phdr, p_offset, u64)
> +
> +static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
> +{
> +	if (elf->class == ELFCLASS32)
> +		return sizeof(Elf32_Phdr);
> +	else
> +		return sizeof(Elf64_Phdr);
> +}
> +
>  #endif /* _LINUX_ELF_H */
> -- 
> 2.15.0.276.g89ea799
> 
> 
> 
> 
> > Hi Sascha,
> > 
> >> Hi Clément,
> >> 
> >> On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
> >>> This patch add elf64 loading support to the elf loader. Since
> >>> elf32 and elf64 uses completely different types, to avoid copying all
> >>> the code and simply replace elf32 with elf64, use a macro which will
> >>> return the appropriate field for each type of header. This macro
> >>> generates getter for elf structures according to the class of the loaded
> >>> elf.
> >>> All direct elf struct dereference are then replaced by call to generated
> >>> functions. This allows to keep a common loader code even if types are
> >>> different.
> >>> 
> >>> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
> >>> ---
> >>>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
> >>>  include/elf.h | 27 +++++++++++++++++++++++++++
> >>>  2 files changed, 50 insertions(+), 23 deletions(-)
> >>> 
> >>> diff --git a/common/elf.c b/common/elf.c
> >>> index 8edf38856..bfb878954 100644
> >>> --- a/common/elf.c
> >>> +++ b/common/elf.c
> >>> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
> >>>  	}
> >>>  }
> >>>  
> >>> -
> >>>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> >>> -				 Elf32_Phdr *phdr)
> >>> +				 void *phdr)
> >>>  {
> >>> -	void *dst = (void *)phdr->p_paddr;
> >>> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
> >>>  	int ret;
> >>> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
> >>> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
> >>>  
> >>>  	/* we care only about PT_LOAD segments */
> >>> -	if (phdr->p_type != PT_LOAD)
> >>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
> >>>  		return 0;
> >>>  
> >>> -	if (!phdr->p_filesz)
> >>> +	if (!p_filesz)
> >>>  		return 0;
> >>>  
> >>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> >>> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
> >> 
> >> %lu for p_filesz?
> > 
> > Indeed, I missed this one.
> > 
> >> 
> >>> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
> >>>  
> >>>  struct elf_image {
> >>>  	struct list_head list;
> >>> +	unsigned long class;
> >>>  	unsigned long entry;
> >>>  	void *buf;
> >>>  };
> >>> @@ -407,4 +408,30 @@ struct elf_image {
> >>>  struct elf_image *elf_load_image(void *buf);
> >>>  void elf_release_image(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) \
> >>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> >>> +	else \
> >>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> >>> +}
> >>> +
> >>> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_offset, unsigned long)
> >> 
> >> When it's always unsigned long why do we have to pass in the type as an
> >> argument?
> > 
> > Actually, some of them should not be defined as I did.
> > For instance, the e_type is an half in both elf32 and elf64 so it should
> > be defined as u16.
> > 
> > Some other approaches to handle both 64bits/32bits elf were to copy
> > the whole loading code and s/elf32/elf64. Since the code in barebox is
> > not so big, maybe I could do that.
> > 
> >> 
> >> I am undecided if this is the right approach. "unsigned long" is wrong
> >> when a ELF file for a foreign architecture is loaded. This can happen
> >> for example when code for the Cortex M4 cores is loaded from the 64bit
> >> Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
> >> then is not a problem, but maybe it could happen the other way round,
> >> loading a 64bit ELF on a 32bit architecture?
> > 
> > I was thinking about this one. I tried loading 32bit and 64bit elf from
> > a 64bit core but indeed, not the other way. If so, then addresses will
> > be truncated but since the processor will not be able to access a
> > 64 bits memory space, I guess it's not possible (unless you have some
> > DMA which can access the upper memory but this will probably not be
> > handled by barebox elf loader).
> > 
> >> 
> >> I can't see a real problem here, I just wanted to note. Are there other
> >> opinions?
> >> 
> >> Sascha
> >> 
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> >> Peiner Str. 6-8, 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
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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] elf: add 64 bits elf support

[Clément Leger]

Hi Sascha,

I will try to compile it in 32bits also (only tried 64bits).
I'm guessing it will spit out some warning about type sizes when casting
so I will resend a V3.

Thanks,

Clément

----- Mail original -----
De: "Sascha Hauer" <s.hauer@pengutronix.de>
À: "Clément Leger" <cleger@kalray.eu>
Cc: "Barebox List" <barebox@lists.infradead.org>
Envoyé: Mercredi 27 Mars 2019 08:48:51
Objet: Re: [PATCH] elf: add 64 bits elf support

Hi Clément,

I guess this version looks good. Could you resend with a Signed-off-by
tag?

Sascha

On Wed, Mar 20, 2019 at 05:28:44PM +0100, Clément Leger wrote:
> Here is a V2 which uses correct type for elf header access macros
> (instead of simply unsigned long). Moreover types used are now
> of fixed size type (u64 instead of unsigned long). This could
> potentially allow a 32bit barebox to load a 64bit elf using some custom
> hardware which support 64bit addressing (DMA or such thing).
> 
> 
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. This allows to keep a common loader code even if types are
> different.
> ---
>  common/elf.c  | 45 +++++++++++++++++++++++----------------------
>  include/elf.h | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 8edf38856..4733accb0 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
>  
>  
>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> -				 Elf32_Phdr *phdr)
> +				 void *phdr)
>  {
> -	void *dst = (void *)phdr->p_paddr;
> +	void *dst = (void *) 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);
>  
>  	/* we care only about PT_LOAD segments */
> -	if (phdr->p_type != PT_LOAD)
> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>  		return 0;
>  
> -	if (!phdr->p_filesz)
> +	if (!p_filesz)
>  		return 0;
>  
> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> +	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
>  
> -	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
> +	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
>  	if (ret)
>  		return ret;
>  
> -	memcpy(dst, src, phdr->p_filesz);
> +	memcpy(dst, src, p_filesz);
>  
> -	if (phdr->p_filesz < phdr->p_memsz)
> -		memset(dst + phdr->p_filesz, 0x00,
> -		       phdr->p_memsz - phdr->p_filesz);
> +	if (p_filesz < p_memsz)
> +		memset(dst + p_filesz, 0x00,
> +		       p_memsz - p_filesz);
>  
>  	return 0;
>  }
> @@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>  static int load_elf_image_phdr(struct elf_image *elf)
>  {
>  	void *buf = elf->buf;
> -	Elf32_Ehdr *ehdr = buf;
> -	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
> +	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
>  	int i, ret;
>  
> -	elf->entry = ehdr->e_entry;
> +	elf->entry = elf_hdr_e_entry(elf, buf);
>  
> -	for (i = 0; i < ehdr->e_phnum; ++i) {
> -		void *src = buf + phdr->p_offset;
> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
> +		void *src = buf + elf_phdr_p_offset(elf, phdr);
>  
>  		ret = load_elf_phdr_segment(elf, src, phdr);
>  		/* in case of error elf_load_image() caller should clean up and
> @@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
>  		if (ret)
>  			return ret;
>  
> -		++phdr;
> +		phdr += elf_size_of_phdr(elf);
>  	}
>  
>  	return 0;
>  }
>  
> -static int elf_check_image(void *buf)
> +static int elf_check_image(struct elf_image *elf)
>  {
> -	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
> -
> -	if (strncmp(buf, ELFMAG, SELFMAG)) {
> +	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
>  		pr_err("ELF magic not found.\n");
>  		return -EINVAL;
>  	}
>  
> -	if (ehdr->e_type != ET_EXEC) {
> +	elf->class = ((char *) elf->buf)[EI_CLASS];
> +
> +	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
>  		pr_err("Non EXEC ELF image.\n");
>  		return -ENOEXEC;
>  	}
> @@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
>  
>  	elf->buf = buf;
>  
> -	ret = elf_check_image(buf);
> +	ret = elf_check_image(elf);
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> diff --git a/include/elf.h b/include/elf.h
> index 92c8d9c12..633f4992d 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
>  
>  struct elf_image {
>  	struct list_head list;
> -	unsigned long entry;
> +	u8 class;
> +	u64 entry;
>  	void *buf;
>  };
>  
>  struct elf_image *elf_load_image(void *buf);
>  void elf_release_image(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) \
> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> +	else \
> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> +}
> +
> +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_type, u16)
> +ELF_GET_FIELD(phdr, p_paddr, u64)
> +ELF_GET_FIELD(phdr, p_filesz, u64)
> +ELF_GET_FIELD(phdr, p_memsz, u64)
> +ELF_GET_FIELD(phdr, p_type, u32)
> +ELF_GET_FIELD(phdr, p_offset, u64)
> +
> +static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
> +{
> +	if (elf->class == ELFCLASS32)
> +		return sizeof(Elf32_Phdr);
> +	else
> +		return sizeof(Elf64_Phdr);
> +}
> +
>  #endif /* _LINUX_ELF_H */
> -- 
> 2.15.0.276.g89ea799
> 
> 
> 
> 
> > Hi Sascha,
> > 
> >> Hi Clément,
> >> 
> >> On Mon, Mar 18, 2019 at 08:16:47PM +0100, Clément Leger wrote:
> >>> This patch add elf64 loading support to the elf loader. Since
> >>> elf32 and elf64 uses completely different types, to avoid copying all
> >>> the code and simply replace elf32 with elf64, use a macro which will
> >>> return the appropriate field for each type of header. This macro
> >>> generates getter for elf structures according to the class of the loaded
> >>> elf.
> >>> All direct elf struct dereference are then replaced by call to generated
> >>> functions. This allows to keep a common loader code even if types are
> >>> different.
> >>> 
> >>> Signed-off-by: Clément Léger <clement.leger@kalray.eu>
> >>> ---
> >>>  common/elf.c  | 46 +++++++++++++++++++++++-----------------------
> >>>  include/elf.h | 27 +++++++++++++++++++++++++++
> >>>  2 files changed, 50 insertions(+), 23 deletions(-)
> >>> 
> >>> diff --git a/common/elf.c b/common/elf.c
> >>> index 8edf38856..bfb878954 100644
> >>> --- a/common/elf.c
> >>> +++ b/common/elf.c
> >>> @@ -43,31 +43,32 @@ static void elf_release_regions(struct elf_image *elf)
> >>>  	}
> >>>  }
> >>>  
> >>> -
> >>>  static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> >>> -				 Elf32_Phdr *phdr)
> >>> +				 void *phdr)
> >>>  {
> >>> -	void *dst = (void *)phdr->p_paddr;
> >>> +	void *dst = (void *) elf_phdr_p_paddr(elf, phdr);
> >>>  	int ret;
> >>> +	unsigned long p_filesz = elf_phdr_p_filesz(elf, phdr);
> >>> +	unsigned long p_memsz = elf_phdr_p_memsz(elf, phdr);
> >>>  
> >>>  	/* we care only about PT_LOAD segments */
> >>> -	if (phdr->p_type != PT_LOAD)
> >>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
> >>>  		return 0;
> >>>  
> >>> -	if (!phdr->p_filesz)
> >>> +	if (!p_filesz)
> >>>  		return 0;
> >>>  
> >>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> >>> +	pr_debug("Loading phdr to 0x%p (%ld bytes)\n", dst, p_filesz);
> >> 
> >> %lu for p_filesz?
> > 
> > Indeed, I missed this one.
> > 
> >> 
> >>> @@ -400,6 +400,7 @@ static inline void arch_write_notes(struct file *file) { }
> >>>  
> >>>  struct elf_image {
> >>>  	struct list_head list;
> >>> +	unsigned long class;
> >>>  	unsigned long entry;
> >>>  	void *buf;
> >>>  };
> >>> @@ -407,4 +408,30 @@ struct elf_image {
> >>>  struct elf_image *elf_load_image(void *buf);
> >>>  void elf_release_image(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) \
> >>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> >>> +	else \
> >>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> >>> +}
> >>> +
> >>> +ELF_GET_FIELD(hdr, e_entry, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phnum, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_phoff, unsigned long)
> >>> +ELF_GET_FIELD(hdr, e_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_paddr, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_filesz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_memsz, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_type, unsigned long)
> >>> +ELF_GET_FIELD(phdr, p_offset, unsigned long)
> >> 
> >> When it's always unsigned long why do we have to pass in the type as an
> >> argument?
> > 
> > Actually, some of them should not be defined as I did.
> > For instance, the e_type is an half in both elf32 and elf64 so it should
> > be defined as u16.
> > 
> > Some other approaches to handle both 64bits/32bits elf were to copy
> > the whole loading code and s/elf32/elf64. Since the code in barebox is
> > not so big, maybe I could do that.
> > 
> >> 
> >> I am undecided if this is the right approach. "unsigned long" is wrong
> >> when a ELF file for a foreign architecture is loaded. This can happen
> >> for example when code for the Cortex M4 cores is loaded from the 64bit
> >> Cortex A cores is loaded on an i.MX8 for example. Using the bigger types
> >> then is not a problem, but maybe it could happen the other way round,
> >> loading a 64bit ELF on a 32bit architecture?
> > 
> > I was thinking about this one. I tried loading 32bit and 64bit elf from
> > a 64bit core but indeed, not the other way. If so, then addresses will
> > be truncated but since the processor will not be able to access a
> > 64 bits memory space, I guess it's not possible (unless you have some
> > DMA which can access the upper memory but this will probably not be
> > handled by barebox elf loader).
> > 
> >> 
> >> I can't see a real problem here, I just wanted to note. Are there other
> >> opinions?
> >> 
> >> Sascha
> >> 
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> >> Peiner Str. 6-8, 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
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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

[PATCH v3] elf: add 64 bits elf loading support

[Clement Leger]

From: Clement Leger <clement.leger@kalray.eu>

This patch add elf64 loading support to the elf loader. Since
elf32 and elf64 uses completely different types, to avoid copying all
the code and simply replace elf32 with elf64, use a macro which will
return the appropriate field for each type of header. This macro
generates getter for elf structures according to the class of the loaded
elf.
All direct elf struct dereference are then replaced by call to generated
functions. THis allows to keep a common loader code even if types are
different.

Signed-off-by: Clement Leger <cleger@kalray.eu>
---
V3: 
- Compiled on ARM with 32bit toolchain (no warning)
- Add Signed-off-by.

 common/elf.c  | 45 +++++++++++++++++++++++----------------------
 include/elf.h | 29 ++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 8edf38856..4733accb0 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
 
 
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
-				 Elf32_Phdr *phdr)
+				 void *phdr)
 {
-	void *dst = (void *)phdr->p_paddr;
+	void *dst = (void *) 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);
 
 	/* we care only about PT_LOAD segments */
-	if (phdr->p_type != PT_LOAD)
+	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
 		return 0;
 
-	if (!phdr->p_filesz)
+	if (!p_filesz)
 		return 0;
 
-	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
+	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
 
-	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
+	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
 	if (ret)
 		return ret;
 
-	memcpy(dst, src, phdr->p_filesz);
+	memcpy(dst, src, p_filesz);
 
-	if (phdr->p_filesz < phdr->p_memsz)
-		memset(dst + phdr->p_filesz, 0x00,
-		       phdr->p_memsz - phdr->p_filesz);
+	if (p_filesz < p_memsz)
+		memset(dst + p_filesz, 0x00,
+		       p_memsz - p_filesz);
 
 	return 0;
 }
@@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 static int load_elf_image_phdr(struct elf_image *elf)
 {
 	void *buf = elf->buf;
-	Elf32_Ehdr *ehdr = buf;
-	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
+	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
 	int i, ret;
 
-	elf->entry = ehdr->e_entry;
+	elf->entry = elf_hdr_e_entry(elf, buf);
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *src = buf + phdr->p_offset;
+	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
+		void *src = buf + elf_phdr_p_offset(elf, phdr);
 
 		ret = load_elf_phdr_segment(elf, src, phdr);
 		/* in case of error elf_load_image() caller should clean up and
@@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
 		if (ret)
 			return ret;
 
-		++phdr;
+		phdr += elf_size_of_phdr(elf);
 	}
 
 	return 0;
 }
 
-static int elf_check_image(void *buf)
+static int elf_check_image(struct elf_image *elf)
 {
-	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
-
-	if (strncmp(buf, ELFMAG, SELFMAG)) {
+	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
 		pr_err("ELF magic not found.\n");
 		return -EINVAL;
 	}
 
-	if (ehdr->e_type != ET_EXEC) {
+	elf->class = ((char *) elf->buf)[EI_CLASS];
+
+	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
 		pr_err("Non EXEC ELF image.\n");
 		return -ENOEXEC;
 	}
@@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
 
 	elf->buf = buf;
 
-	ret = elf_check_image(buf);
+	ret = elf_check_image(elf);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/include/elf.h b/include/elf.h
index 92c8d9c12..633f4992d 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
 
 struct elf_image {
 	struct list_head list;
-	unsigned long entry;
+	u8 class;
+	u64 entry;
 	void *buf;
 };
 
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(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) \
+		return (__type) ((struct elf32_##__s *) arg)->__field; \
+	else \
+		return (__type) ((struct elf64_##__s *) arg)->__field; \
+}
+
+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_type, u16)
+ELF_GET_FIELD(phdr, p_paddr, u64)
+ELF_GET_FIELD(phdr, p_filesz, u64)
+ELF_GET_FIELD(phdr, p_memsz, u64)
+ELF_GET_FIELD(phdr, p_type, u32)
+ELF_GET_FIELD(phdr, p_offset, u64)
+
+static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
+{
+	if (elf->class == ELFCLASS32)
+		return sizeof(Elf32_Phdr);
+	else
+		return sizeof(Elf64_Phdr);
+}
+
 #endif /* _LINUX_ELF_H */
-- 
2.15.0.276.g89ea799


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

Re: [PATCH v3] elf: add 64 bits elf loading support

[Oleksij Rempel]

Hi Clement,

thank you for your patch. I tested it on MIPS32 system.

I get this compile warning:
arch/mips/lib/bootm.c: In function 'do_bootm_elf':
arch/mips/lib/bootm.c:75:10: warning: cast to pointer from integer of different size 
[-Wint-to-pointer-cast]
   entry = (void *)elf->entry;
           ^

Kernel boot is working.
Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>

On 01.09.19 16:09, Clement Leger wrote:
> From: Clement Leger <clement.leger@kalray.eu>
> 
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. THis allows to keep a common loader code even if types are
> different.
> 
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
> V3:
> - Compiled on ARM with 32bit toolchain (no warning)
> - Add Signed-off-by.
> 
>   common/elf.c  | 45 +++++++++++++++++++++++----------------------
>   include/elf.h | 29 ++++++++++++++++++++++++++++-
>   2 files changed, 51 insertions(+), 23 deletions(-)
> 
> diff --git a/common/elf.c b/common/elf.c
> index 8edf38856..4733accb0 100644
> --- a/common/elf.c
> +++ b/common/elf.c
> @@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
>   
>   
>   static int load_elf_phdr_segment(struct elf_image *elf, void *src,
> -				 Elf32_Phdr *phdr)
> +				 void *phdr)
>   {
> -	void *dst = (void *)phdr->p_paddr;
> +	void *dst = (void *) 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);
>   
>   	/* we care only about PT_LOAD segments */
> -	if (phdr->p_type != PT_LOAD)
> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>   		return 0;
>   
> -	if (!phdr->p_filesz)
> +	if (!p_filesz)
>   		return 0;
>   
> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
> +	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
>   
> -	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
> +	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
>   	if (ret)
>   		return ret;
>   
> -	memcpy(dst, src, phdr->p_filesz);
> +	memcpy(dst, src, p_filesz);
>   
> -	if (phdr->p_filesz < phdr->p_memsz)
> -		memset(dst + phdr->p_filesz, 0x00,
> -		       phdr->p_memsz - phdr->p_filesz);
> +	if (p_filesz < p_memsz)
> +		memset(dst + p_filesz, 0x00,
> +		       p_memsz - p_filesz);
>   
>   	return 0;
>   }
> @@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>   static int load_elf_image_phdr(struct elf_image *elf)
>   {
>   	void *buf = elf->buf;
> -	Elf32_Ehdr *ehdr = buf;
> -	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
> +	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
>   	int i, ret;
>   
> -	elf->entry = ehdr->e_entry;
> +	elf->entry = elf_hdr_e_entry(elf, buf);
>   
> -	for (i = 0; i < ehdr->e_phnum; ++i) {
> -		void *src = buf + phdr->p_offset;
> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
> +		void *src = buf + elf_phdr_p_offset(elf, phdr);
>   
>   		ret = load_elf_phdr_segment(elf, src, phdr);
>   		/* in case of error elf_load_image() caller should clean up and
> @@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
>   		if (ret)
>   			return ret;
>   
> -		++phdr;
> +		phdr += elf_size_of_phdr(elf);
>   	}
>   
>   	return 0;
>   }
>   
> -static int elf_check_image(void *buf)
> +static int elf_check_image(struct elf_image *elf)
>   {
> -	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
> -
> -	if (strncmp(buf, ELFMAG, SELFMAG)) {
> +	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
>   		pr_err("ELF magic not found.\n");
>   		return -EINVAL;
>   	}
>   
> -	if (ehdr->e_type != ET_EXEC) {
> +	elf->class = ((char *) elf->buf)[EI_CLASS];
> +
> +	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
>   		pr_err("Non EXEC ELF image.\n");
>   		return -ENOEXEC;
>   	}
> @@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
>   
>   	elf->buf = buf;
>   
> -	ret = elf_check_image(buf);
> +	ret = elf_check_image(elf);
>   	if (ret)
>   		return ERR_PTR(ret);
>   
> diff --git a/include/elf.h b/include/elf.h
> index 92c8d9c12..633f4992d 100644
> --- a/include/elf.h
> +++ b/include/elf.h
> @@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
>   
>   struct elf_image {
>   	struct list_head list;
> -	unsigned long entry;
> +	u8 class;
> +	u64 entry;
>   	void *buf;
>   };
>   
>   struct elf_image *elf_load_image(void *buf);
>   void elf_release_image(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) \
> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
> +	else \
> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
> +}
> +
> +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_type, u16)
> +ELF_GET_FIELD(phdr, p_paddr, u64)
> +ELF_GET_FIELD(phdr, p_filesz, u64)
> +ELF_GET_FIELD(phdr, p_memsz, u64)
> +ELF_GET_FIELD(phdr, p_type, u32)
> +ELF_GET_FIELD(phdr, p_offset, u64)
> +
> +static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
> +{
> +	if (elf->class == ELFCLASS32)
> +		return sizeof(Elf32_Phdr);
> +	else
> +		return sizeof(Elf64_Phdr);
> +}
> +
>   #endif /* _LINUX_ELF_H */
> 

Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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 v3] elf: add 64 bits elf loading support

[Clément Leger]

Hi Oleksij,

Thanks for testing, since I only tested on arm, it explain this problem :)
I will try to search for all elf->entry usage and fix that according to architectures.
Thanks,

Clément

----- On 2 Sep, 2019, at 08:07, Oleksij Rempel o.rempel@pengutronix.de wrote:

> Hi Clement,
> 
> thank you for your patch. I tested it on MIPS32 system.
> 
> I get this compile warning:
> arch/mips/lib/bootm.c: In function 'do_bootm_elf':
> arch/mips/lib/bootm.c:75:10: warning: cast to pointer from integer of different
> size
> [-Wint-to-pointer-cast]
>   entry = (void *)elf->entry;
>           ^
> 
> Kernel boot is working.
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> 
> On 01.09.19 16:09, Clement Leger wrote:
>> From: Clement Leger <clement.leger@kalray.eu>
>> 
>> This patch add elf64 loading support to the elf loader. Since
>> elf32 and elf64 uses completely different types, to avoid copying all
>> the code and simply replace elf32 with elf64, use a macro which will
>> return the appropriate field for each type of header. This macro
>> generates getter for elf structures according to the class of the loaded
>> elf.
>> All direct elf struct dereference are then replaced by call to generated
>> functions. THis allows to keep a common loader code even if types are
>> different.
>> 
>> Signed-off-by: Clement Leger <cleger@kalray.eu>
>> ---
>> V3:
>> - Compiled on ARM with 32bit toolchain (no warning)
>> - Add Signed-off-by.
>> 
>>   common/elf.c  | 45 +++++++++++++++++++++++----------------------
>>   include/elf.h | 29 ++++++++++++++++++++++++++++-
>>   2 files changed, 51 insertions(+), 23 deletions(-)
>> 
>> diff --git a/common/elf.c b/common/elf.c
>> index 8edf38856..4733accb0 100644
>> --- a/common/elf.c
>> +++ b/common/elf.c
>> @@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
>>   
>>   
>>   static int load_elf_phdr_segment(struct elf_image *elf, void *src,
>> -				 Elf32_Phdr *phdr)
>> +				 void *phdr)
>>   {
>> -	void *dst = (void *)phdr->p_paddr;
>> +	void *dst = (void *) 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);
>>   
>>   	/* we care only about PT_LOAD segments */
>> -	if (phdr->p_type != PT_LOAD)
>> +	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
>>   		return 0;
>>   
>> -	if (!phdr->p_filesz)
>> +	if (!p_filesz)
>>   		return 0;
>>   
>> -	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
>> +	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
>>   
>> -	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
>> +	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
>>   	if (ret)
>>   		return ret;
>>   
>> -	memcpy(dst, src, phdr->p_filesz);
>> +	memcpy(dst, src, p_filesz);
>>   
>> -	if (phdr->p_filesz < phdr->p_memsz)
>> -		memset(dst + phdr->p_filesz, 0x00,
>> -		       phdr->p_memsz - phdr->p_filesz);
>> +	if (p_filesz < p_memsz)
>> +		memset(dst + p_filesz, 0x00,
>> +		       p_memsz - p_filesz);
>>   
>>   	return 0;
>>   }
>> @@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void
>> *src,
>>   static int load_elf_image_phdr(struct elf_image *elf)
>>   {
>>   	void *buf = elf->buf;
>> -	Elf32_Ehdr *ehdr = buf;
>> -	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
>> +	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
>>   	int i, ret;
>>   
>> -	elf->entry = ehdr->e_entry;
>> +	elf->entry = elf_hdr_e_entry(elf, buf);
>>   
>> -	for (i = 0; i < ehdr->e_phnum; ++i) {
>> -		void *src = buf + phdr->p_offset;
>> +	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
>> +		void *src = buf + elf_phdr_p_offset(elf, phdr);
>>   
>>   		ret = load_elf_phdr_segment(elf, src, phdr);
>>   		/* in case of error elf_load_image() caller should clean up and
>> @@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
>>   		if (ret)
>>   			return ret;
>>   
>> -		++phdr;
>> +		phdr += elf_size_of_phdr(elf);
>>   	}
>>   
>>   	return 0;
>>   }
>>   
>> -static int elf_check_image(void *buf)
>> +static int elf_check_image(struct elf_image *elf)
>>   {
>> -	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
>> -
>> -	if (strncmp(buf, ELFMAG, SELFMAG)) {
>> +	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
>>   		pr_err("ELF magic not found.\n");
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (ehdr->e_type != ET_EXEC) {
>> +	elf->class = ((char *) elf->buf)[EI_CLASS];
>> +
>> +	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
>>   		pr_err("Non EXEC ELF image.\n");
>>   		return -ENOEXEC;
>>   	}
>> @@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
>>   
>>   	elf->buf = buf;
>>   
>> -	ret = elf_check_image(buf);
>> +	ret = elf_check_image(elf);
>>   	if (ret)
>>   		return ERR_PTR(ret);
>>   
>> diff --git a/include/elf.h b/include/elf.h
>> index 92c8d9c12..633f4992d 100644
>> --- a/include/elf.h
>> +++ b/include/elf.h
>> @@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
>>   
>>   struct elf_image {
>>   	struct list_head list;
>> -	unsigned long entry;
>> +	u8 class;
>> +	u64 entry;
>>   	void *buf;
>>   };
>>   
>>   struct elf_image *elf_load_image(void *buf);
>>   void elf_release_image(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) \
>> +		return (__type) ((struct elf32_##__s *) arg)->__field; \
>> +	else \
>> +		return (__type) ((struct elf64_##__s *) arg)->__field; \
>> +}
>> +
>> +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_type, u16)
>> +ELF_GET_FIELD(phdr, p_paddr, u64)
>> +ELF_GET_FIELD(phdr, p_filesz, u64)
>> +ELF_GET_FIELD(phdr, p_memsz, u64)
>> +ELF_GET_FIELD(phdr, p_type, u32)
>> +ELF_GET_FIELD(phdr, p_offset, u64)
>> +
>> +static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
>> +{
>> +	if (elf->class == ELFCLASS32)
>> +		return sizeof(Elf32_Phdr);
>> +	else
>> +		return sizeof(Elf64_Phdr);
>> +}
>> +
>>   #endif /* _LINUX_ELF_H */
>> 
> 
> Kind regards,
> Oleksij Rempel
> 
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 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

[PATCH v4 1/2] elf: add 64 bits elf loading support

[Clement Leger]

From: Clement Leger <clement.leger@kalray.eu>

This patch add elf64 loading support to the elf loader. Since
elf32 and elf64 uses completely different types, to avoid copying all
the code and simply replace elf32 with elf64, use a macro which will
return the appropriate field for each type of header. This macro
generates getter for elf structures according to the class of the loaded
elf.
All direct elf struct dereference are then replaced by call to generated
functions. This allows to keep a common loader code even if types are
different.

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Clement Leger <cleger@kalray.eu>
---
Changes in V4:
 - Fix mips elf entry cast to pointer
 - Add Tested-by Oleksij Rempel

 common/elf.c  | 45 +++++++++++++++++++++++----------------------
 include/elf.h | 29 ++++++++++++++++++++++++++++-
 2 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/common/elf.c b/common/elf.c
index 8edf38856..4733accb0 100644
--- a/common/elf.c
+++ b/common/elf.c
@@ -45,29 +45,31 @@ static void elf_release_regions(struct elf_image *elf)
 
 
 static int load_elf_phdr_segment(struct elf_image *elf, void *src,
-				 Elf32_Phdr *phdr)
+				 void *phdr)
 {
-	void *dst = (void *)phdr->p_paddr;
+	void *dst = (void *) 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);
 
 	/* we care only about PT_LOAD segments */
-	if (phdr->p_type != PT_LOAD)
+	if (elf_phdr_p_type(elf, phdr) != PT_LOAD)
 		return 0;
 
-	if (!phdr->p_filesz)
+	if (!p_filesz)
 		return 0;
 
-	pr_debug("Loading phdr to 0x%p (%i bytes)\n", dst, phdr->p_filesz);
+	pr_debug("Loading phdr to 0x%p (%llu bytes)\n", dst, p_filesz);
 
-	ret = elf_request_region(elf, (resource_size_t)dst, phdr->p_filesz);
+	ret = elf_request_region(elf, (resource_size_t)dst, p_filesz);
 	if (ret)
 		return ret;
 
-	memcpy(dst, src, phdr->p_filesz);
+	memcpy(dst, src, p_filesz);
 
-	if (phdr->p_filesz < phdr->p_memsz)
-		memset(dst + phdr->p_filesz, 0x00,
-		       phdr->p_memsz - phdr->p_filesz);
+	if (p_filesz < p_memsz)
+		memset(dst + p_filesz, 0x00,
+		       p_memsz - p_filesz);
 
 	return 0;
 }
@@ -75,14 +77,13 @@ static int load_elf_phdr_segment(struct elf_image *elf, void *src,
 static int load_elf_image_phdr(struct elf_image *elf)
 {
 	void *buf = elf->buf;
-	Elf32_Ehdr *ehdr = buf;
-	Elf32_Phdr *phdr = (Elf32_Phdr *)(buf + ehdr->e_phoff);
+	void *phdr = (void *) (buf + elf_hdr_e_phoff(elf, buf));
 	int i, ret;
 
-	elf->entry = ehdr->e_entry;
+	elf->entry = elf_hdr_e_entry(elf, buf);
 
-	for (i = 0; i < ehdr->e_phnum; ++i) {
-		void *src = buf + phdr->p_offset;
+	for (i = 0; i < elf_hdr_e_phnum(elf, buf) ; ++i) {
+		void *src = buf + elf_phdr_p_offset(elf, phdr);
 
 		ret = load_elf_phdr_segment(elf, src, phdr);
 		/* in case of error elf_load_image() caller should clean up and
@@ -90,22 +91,22 @@ static int load_elf_image_phdr(struct elf_image *elf)
 		if (ret)
 			return ret;
 
-		++phdr;
+		phdr += elf_size_of_phdr(elf);
 	}
 
 	return 0;
 }
 
-static int elf_check_image(void *buf)
+static int elf_check_image(struct elf_image *elf)
 {
-	Elf32_Ehdr *ehdr = (Elf32_Ehdr *)buf;
-
-	if (strncmp(buf, ELFMAG, SELFMAG)) {
+	if (strncmp(elf->buf, ELFMAG, SELFMAG)) {
 		pr_err("ELF magic not found.\n");
 		return -EINVAL;
 	}
 
-	if (ehdr->e_type != ET_EXEC) {
+	elf->class = ((char *) elf->buf)[EI_CLASS];
+
+	if (elf_hdr_e_type(elf, elf->buf) != ET_EXEC) {
 		pr_err("Non EXEC ELF image.\n");
 		return -ENOEXEC;
 	}
@@ -124,7 +125,7 @@ struct elf_image *elf_load_image(void *buf)
 
 	elf->buf = buf;
 
-	ret = elf_check_image(buf);
+	ret = elf_check_image(elf);
 	if (ret)
 		return ERR_PTR(ret);
 
diff --git a/include/elf.h b/include/elf.h
index 92c8d9c12..633f4992d 100644
--- a/include/elf.h
+++ b/include/elf.h
@@ -400,11 +400,38 @@ static inline void arch_write_notes(struct file *file) { }
 
 struct elf_image {
 	struct list_head list;
-	unsigned long entry;
+	u8 class;
+	u64 entry;
 	void *buf;
 };
 
 struct elf_image *elf_load_image(void *buf);
 void elf_release_image(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) \
+		return (__type) ((struct elf32_##__s *) arg)->__field; \
+	else \
+		return (__type) ((struct elf64_##__s *) arg)->__field; \
+}
+
+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_type, u16)
+ELF_GET_FIELD(phdr, p_paddr, u64)
+ELF_GET_FIELD(phdr, p_filesz, u64)
+ELF_GET_FIELD(phdr, p_memsz, u64)
+ELF_GET_FIELD(phdr, p_type, u32)
+ELF_GET_FIELD(phdr, p_offset, u64)
+
+static inline unsigned long elf_size_of_phdr(struct elf_image *elf)
+{
+	if (elf->class == ELFCLASS32)
+		return sizeof(Elf32_Phdr);
+	else
+		return sizeof(Elf64_Phdr);
+}
+
 #endif /* _LINUX_ELF_H */
-- 
2.15.0.276.g89ea799


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

[PATCH v4 2/2] mips: bootm: Cast elf entry into unsigned long

[Clement Leger]

Since elf entry type is now a u64 to accomodate both type of elf files
(64 and 32 bits), we need to cast it to the pointer length before
casting it to the pointer type.

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

diff --git a/arch/mips/lib/bootm.c b/arch/mips/lib/bootm.c
index f14540a4c..5bb09cc2d 100644
--- a/arch/mips/lib/bootm.c
+++ b/arch/mips/lib/bootm.c
@@ -72,7 +72,7 @@ static int do_bootm_elf(struct image_data *data)
 
 	shutdown_barebox();
 
-	entry = (void *)elf->entry;
+	entry = (void *) (unsigned long) elf->entry;
 
 	entry(-2, phys_to_virt((unsigned long)fdt));
 
-- 
2.15.0.276.g89ea799


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

Re: [PATCH v4 1/2] elf: add 64 bits elf loading support

[Sascha Hauer]

On Wed, Sep 04, 2019 at 01:24:49PM +0200, Clement Leger wrote:
> From: Clement Leger <clement.leger@kalray.eu>
> 
> This patch add elf64 loading support to the elf loader. Since
> elf32 and elf64 uses completely different types, to avoid copying all
> the code and simply replace elf32 with elf64, use a macro which will
> return the appropriate field for each type of header. This macro
> generates getter for elf structures according to the class of the loaded
> elf.
> All direct elf struct dereference are then replaced by call to generated
> functions. This allows to keep a common loader code even if types are
> different.
> 
> Tested-by: Oleksij Rempel <o.rempel@pengutronix.de>
> Signed-off-by: Clement Leger <cleger@kalray.eu>
> ---
> Changes in V4:
>  - Fix mips elf entry cast to pointer
>  - Add Tested-by Oleksij Rempel

Applied, thanks

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 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