[U-Boot] [PATCH v3 4/7] remoteproc: add elf file load support

[Patrick DELAUNAY]

Hi Lokesh,

Thanks for the review.
Fabien is in vacation and I will integrate this serie in my next stm32 pull request.

> From: Lokesh Vutla <lokeshvutla at ti.com>
> Sent: lundi 3 juin 2019 07:31
> 
> 
> 
> On 31/05/19 6:41 PM, Fabien Dessenne wrote:
> > The current implementation supports only binary file load.
> > Add helpers to support ELF32 format (sanity check, and load).
> > Note that since an ELF32 image is built for the remote processor, the
> > load function uses the device_to_virt ops to translate the addresses.
> > Implement a basic translation for sandbox_testproc.
> >
> > Add related tests. Test result:
> > => ut dm remoteproc_elf
> > Test: dm_test_remoteproc_elf: remoteproc.c
> > Test: dm_test_remoteproc_elf: remoteproc.c (flat tree)
> > Failures: 0
> >
> > Signed-off-by: Loic Pallardy <loic.pallardy at st.com>
> > Signed-off-by: Fabien Dessenne <fabien.dessenne at st.com>
> > ---
> 
> [...snip...]
> 
> > +/* Basic function to verify ELF32 image format */ int
> > +rproc_elf32_sanity_check(ulong addr, ulong size) {
> > +	Elf32_Ehdr *ehdr;
> > +	char class;
> > +
> > +	if (!addr) {
> > +		pr_debug("Invalid fw address?\n");
> > +		return -EFAULT;
> > +	}
> > +
> > +	if (size < sizeof(Elf32_Ehdr)) {
> > +		pr_debug("Image is too small\n");
> > +		return -ENOSPC;
> > +	}
> > +
> > +	ehdr = (Elf32_Ehdr *)addr;
> > +	class = ehdr->e_ident[EI_CLASS];
> > +
> > +	if (!IS_ELF(*ehdr) || ehdr->e_type != ET_EXEC || class != ELFCLASS32) {
> > +		pr_debug("Not an executable ELF32 image\n");
> > +		return -EPROTONOSUPPORT;
> > +	}
> > +
> > +	/* We assume the firmware has the same endianness as the host */ #
> > +ifdef __LITTLE_ENDIAN
> > +	if (ehdr->e_ident[EI_DATA] != ELFDATA2LSB) { # else /* BIG ENDIAN */
> > +	if (ehdr->e_ident[EI_DATA] != ELFDATA2MSB) { # endif
> > +		pr_debug("Unsupported firmware endianness\n");
> > +		return -EILSEQ;
> > +	}
> > +
> > +	if (size < ehdr->e_shoff + sizeof(Elf32_Shdr)) {
> > +		pr_debug("Image is too small\n");
> > +		return -ENOSPC;
> > +	}
> > +
> > +	if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG)) {
> > +		pr_debug("Image is corrupted (bad magic)\n");
> > +		return -EBADF;
> > +	}
> > +
> > +	if (ehdr->e_phnum == 0) {
> > +		pr_debug("No loadable segments\n");
> > +		return -ENOEXEC;
> > +	}
> > +
> > +	if (ehdr->e_phoff > size) {
> > +		pr_debug("Firmware size is too small\n");
> > +		return -ENOSPC;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* A very simple elf loader, assumes the image is valid */ int
> > +rproc_elf32_load_image(struct udevice *dev, unsigned long addr) {
> > +	Elf32_Ehdr *ehdr; /* Elf header structure pointer */
> > +	Elf32_Phdr *phdr; /* Program header structure pointer */
> > +	const struct dm_rproc_ops *ops;
> > +	unsigned int i;
> > +
> 
> I would prefer to call  rproc_elf32_sanity_check() here and reduce the burden on
> user. It's my preference and no strong objections.

Yes it is a possibility, but for my side I prefer the Fabien proposition.
(we can perhaps reuse the check of ELF for other use case).

I will merge the patch with this version (to have the patch in v2019.10) .
But I let Fabien conclude and potentially sent a minor update.

> Other than that:
> 
> Reviewed-by: Lokesh Vutla <lokeshvutla at ti.com>
> 
> Thanks and regards,
> Lokesh

Thanks
Patrick