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@xxxxxxxxxxxxxx>
À: "Clément Leger" <cleger@xxxxxxxxx>
Cc: "Barebox List" <barebox@xxxxxxxxxxxxxxxxxxx>
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@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
>
--
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
