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

["Clément Leger" <cleger@kalray.eu>]

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