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

2019-05-29 Thread Fabien DESSENNE
Hi Lokesh


On 29/05/2019 6:21 AM, Lokesh Vutla wrote:
>
> On 27/05/19 5:53 PM, Fabien Dessenne wrote:
>> The current implementation supports only binary file load.
>> Add helpers to support ELF format (sanity check, and load).
>> Note that since an ELF 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 
>> Signed-off-by: Fabien Dessenne 
> Can you create a new file(rproc-elf-loader.c) or something similar for elf
> loading support.


It sounds good, I'll do that.


>   Ill be sending 64bit elf loading support soon. Instead of
> cluttering rproc-uclass, it is better to separate out elf loading support.


Since you plan support for elf64, I may consider renaming the elf 
functions in elf32. I'll check this too.

BR

Fabien


>
> Thanks and regards,
> Lokesh
>
>> ---
>>   drivers/remoteproc/rproc-uclass.c |  99 +++
>>   drivers/remoteproc/sandbox_testproc.c |  19 ++
>>   include/remoteproc.h  |  30 -
>>   test/dm/remoteproc.c  | 122 
>> ++
>>   4 files changed, 267 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/remoteproc/rproc-uclass.c 
>> b/drivers/remoteproc/rproc-uclass.c
>> index c8a41a6..4d85732 100644
>> --- a/drivers/remoteproc/rproc-uclass.c
>> +++ b/drivers/remoteproc/rproc-uclass.c
>> @@ -5,6 +5,7 @@
>>*/
>>   #define pr_fmt(fmt) "%s: " fmt, __func__
>>   #include 
>> +#include 
>>   #include 
>>   #include 
>>   #include 
>> @@ -291,6 +292,104 @@ int rproc_dev_init(int id)
>>  return ret;
>>   }
>>   
>> +/* Basic function to verify ELF image format */
>> +int rproc_elf_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_elf_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;
>> +
>> +ehdr = (Elf32_Ehdr *)addr;
>> +phdr = (Elf32_Phdr *)(addr + ehdr->e_phoff);
>> +
>> +ops = rproc_get_ops(dev);
>> +
>> +/* Load each program header */
>> +for (i = 0; i < ehdr->e_phnum; ++i) {
>> +void *dst = (void *)(uintptr_t)phdr->p_paddr;
>> +void *src = (void *)addr + phdr->p_offset;
>> +
>> +if (phdr->p_type != PT_LOAD)
>> +continue;
>> +
>> +if (ops->device_to_virt)
>> +dst = ops->device_to_virt(dev, (ulong)dst);
>> +
>> +dev_dbg(dev, "Loading phdr %i to 0x%p (%i bytes)\n",
>> +i, dst, phdr->p_filesz);
>> +if (phdr->p_filesz)
>> +memcpy(dst, src, phdr->p_filesz);
>> +if (phdr->p_filesz != phdr->p_memsz)
>> +memset(dst + phdr->p_filesz, 0x00,
>> +   phdr->p_memsz - phdr->p_filesz);
>> +flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
>> +roundup((unsigned long)dst + phdr->p_filesz,
>> +   

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

2019-05-28 Thread Lokesh Vutla


On 27/05/19 5:53 PM, Fabien Dessenne wrote:
> The current implementation supports only binary file load.
> Add helpers to support ELF format (sanity check, and load).
> Note that since an ELF 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 
> Signed-off-by: Fabien Dessenne 

Can you create a new file(rproc-elf-loader.c) or something similar for elf
loading support. Ill be sending 64bit elf loading support soon. Instead of
cluttering rproc-uclass, it is better to separate out elf loading support.

Thanks and regards,
Lokesh

> ---
>  drivers/remoteproc/rproc-uclass.c |  99 +++
>  drivers/remoteproc/sandbox_testproc.c |  19 ++
>  include/remoteproc.h  |  30 -
>  test/dm/remoteproc.c  | 122 
> ++
>  4 files changed, 267 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/rproc-uclass.c 
> b/drivers/remoteproc/rproc-uclass.c
> index c8a41a6..4d85732 100644
> --- a/drivers/remoteproc/rproc-uclass.c
> +++ b/drivers/remoteproc/rproc-uclass.c
> @@ -5,6 +5,7 @@
>   */
>  #define pr_fmt(fmt) "%s: " fmt, __func__
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -291,6 +292,104 @@ int rproc_dev_init(int id)
>   return ret;
>  }
>  
> +/* Basic function to verify ELF image format */
> +int rproc_elf_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_elf_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;
> +
> + ehdr = (Elf32_Ehdr *)addr;
> + phdr = (Elf32_Phdr *)(addr + ehdr->e_phoff);
> +
> + ops = rproc_get_ops(dev);
> +
> + /* Load each program header */
> + for (i = 0; i < ehdr->e_phnum; ++i) {
> + void *dst = (void *)(uintptr_t)phdr->p_paddr;
> + void *src = (void *)addr + phdr->p_offset;
> +
> + if (phdr->p_type != PT_LOAD)
> + continue;
> +
> + if (ops->device_to_virt)
> + dst = ops->device_to_virt(dev, (ulong)dst);
> +
> + dev_dbg(dev, "Loading phdr %i to 0x%p (%i bytes)\n",
> + i, dst, phdr->p_filesz);
> + if (phdr->p_filesz)
> + memcpy(dst, src, phdr->p_filesz);
> + if (phdr->p_filesz != phdr->p_memsz)
> + memset(dst + phdr->p_filesz, 0x00,
> +phdr->p_memsz - phdr->p_filesz);
> + flush_cache(rounddown((unsigned long)dst, ARCH_DMA_MINALIGN),
> + roundup((unsigned long)dst + phdr->p_filesz,
> + ARCH_DMA_MINALIGN) -
> + rounddown((unsigned long)dst, ARCH_DMA_MINALIGN));
> + ++phdr;
> + }
> +
> + return 0;
> +}
> +
>  int rproc_load(int id, ulong addr, ulong size)
>  {
>   struct udevice *dev = NULL;
> diff --git