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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
