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

2019-05-22 Thread Simon Glass
Hi Fabien,

On Wed, 22 May 2019 at 02:07, Fabien Dessenne  wrote:
>
> The current implementation supports only binary file load.
> Add helpers to support ELF format (check validity, sanity check, and
> load).
> Note that since an ELF image is built for the remote processor, the load
> function uses the da_to_pa ops to translate the addresses.
>
> Signed-off-by: Loic Pallardy 
> Signed-off-by: Fabien Dessenne 
> ---
>  drivers/remoteproc/rproc-uclass.c | 128 
> ++
>  include/remoteproc.h  |  29 -
>  2 files changed, 156 insertions(+), 1 deletion(-)

It doesn't look like we can easily make use of the existing ELF loader.

But please add a test for this in test/dm/remoteproc.c and see below.



>
> diff --git a/drivers/remoteproc/rproc-uclass.c 
> b/drivers/remoteproc/rproc-uclass.c
> index c8a41a6..ac5f9e2 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,133 @@ int rproc_dev_init(int id)
> return ret;
>  }
>
> +/*
> + * Determine if a valid ELF image exists at the given memory location.
> + * First look at the ELF header magic field, then make sure that it is
> + * executable.
> + */
> +bool rproc_elf_is_valid(unsigned long addr, int size)
> +{
> +   Elf32_Ehdr *ehdr; /* Elf header structure pointer */
> +
> +   ehdr = (Elf32_Ehdr *)addr;
> +
> +   if (!IS_ELF(*ehdr) || size <= sizeof(Elf32_Ehdr)) {
> +   pr_err("No elf image at address 0x%08lx\n", addr);
> +   return false;
> +   }
> +
> +   if (ehdr->e_type != ET_EXEC) {
> +   pr_err("Not a 32-bit elf image at address 0x%08lx\n", addr);
> +   return false;
> +   }
> +
> +   return true;
> +}
> +
> +/* Basic function to verify ELF image format */
> +int rproc_elf_sanity_check(ulong addr, ulong size)
> +{
> +   Elf32_Ehdr *ehdr;
> +   char class;
> +
> +   if (!addr) {
> +   pr_err("Invalid fw address?\n");
> +   return -EINVAL;

EFAULT ?

> +   }
> +
> +   if (size < sizeof(Elf32_Ehdr)) {
> +   pr_err("Image is too small\n");
> +   return -EINVAL;

ENOSPC?

> +   }
> +
> +   ehdr = (Elf32_Ehdr *)addr;
> +
> +   /* We only support ELF32 at this point */
> +   class = ehdr->e_ident[EI_CLASS];
> +   if (class != ELFCLASS32) {
> +   pr_err("Unsupported class: %d\n", class);
> +   return -EINVAL;

EPROTONOSUPP?

> +   }
> +
> +   /* 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_err("Unsupported firmware endianness\n");
> +   return -EINVAL;
> +   }
> +
> +   if (size < ehdr->e_shoff + sizeof(Elf32_Shdr)) {
> +   pr_err("Image is too small\n");
> +   return -EINVAL;

ESPC

> +   }
> +
> +   if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG)) {
> +   pr_err("Image is corrupted (bad magic)\n");
> +   return -EINVAL;

EBADF

> +   }
> +
> +   if (ehdr->e_phnum == 0) {
> +   pr_err("No loadable segments\n");
> +   return -EINVAL;
> +   }
> +
> +   if (ehdr->e_phoff > size) {
> +   pr_err("Firmware size is too small\n");
> +   return -EINVAL;
> +   }
> +
> +   return 0;
> +}
> +

They are just suggestions, but please try to return useful numbers. In
general it should be possible to see what went wrong without needing
to output text.

> +/* 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);
> +   if (!ops) {
> +   dev_dbg(dev, "Driver has no ops?\n");
> +   return -EINVAL;
> +   }

This is not allowed, so you don't need to check for it.

> +
> +   /* 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->da_to_pa)
> +   dst = (void *)ops->da_to_pa(dev, (ulong)dst);
> +
> +   dev_dbg(dev, "Loading phdr %i to 0x%p (%i bytes)\n",
> +   

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

2019-05-22 Thread Fabien Dessenne
The current implementation supports only binary file load.
Add helpers to support ELF format (check validity, sanity check, and
load).
Note that since an ELF image is built for the remote processor, the load
function uses the da_to_pa ops to translate the addresses.

Signed-off-by: Loic Pallardy 
Signed-off-by: Fabien Dessenne 
---
 drivers/remoteproc/rproc-uclass.c | 128 ++
 include/remoteproc.h  |  29 -
 2 files changed, 156 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/rproc-uclass.c 
b/drivers/remoteproc/rproc-uclass.c
index c8a41a6..ac5f9e2 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,133 @@ int rproc_dev_init(int id)
return ret;
 }
 
+/*
+ * Determine if a valid ELF image exists at the given memory location.
+ * First look at the ELF header magic field, then make sure that it is
+ * executable.
+ */
+bool rproc_elf_is_valid(unsigned long addr, int size)
+{
+   Elf32_Ehdr *ehdr; /* Elf header structure pointer */
+
+   ehdr = (Elf32_Ehdr *)addr;
+
+   if (!IS_ELF(*ehdr) || size <= sizeof(Elf32_Ehdr)) {
+   pr_err("No elf image at address 0x%08lx\n", addr);
+   return false;
+   }
+
+   if (ehdr->e_type != ET_EXEC) {
+   pr_err("Not a 32-bit elf image at address 0x%08lx\n", addr);
+   return false;
+   }
+
+   return true;
+}
+
+/* Basic function to verify ELF image format */
+int rproc_elf_sanity_check(ulong addr, ulong size)
+{
+   Elf32_Ehdr *ehdr;
+   char class;
+
+   if (!addr) {
+   pr_err("Invalid fw address?\n");
+   return -EINVAL;
+   }
+
+   if (size < sizeof(Elf32_Ehdr)) {
+   pr_err("Image is too small\n");
+   return -EINVAL;
+   }
+
+   ehdr = (Elf32_Ehdr *)addr;
+
+   /* We only support ELF32 at this point */
+   class = ehdr->e_ident[EI_CLASS];
+   if (class != ELFCLASS32) {
+   pr_err("Unsupported class: %d\n", class);
+   return -EINVAL;
+   }
+
+   /* 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_err("Unsupported firmware endianness\n");
+   return -EINVAL;
+   }
+
+   if (size < ehdr->e_shoff + sizeof(Elf32_Shdr)) {
+   pr_err("Image is too small\n");
+   return -EINVAL;
+   }
+
+   if (memcmp(ehdr->e_ident, ELFMAG, SELFMAG)) {
+   pr_err("Image is corrupted (bad magic)\n");
+   return -EINVAL;
+   }
+
+   if (ehdr->e_phnum == 0) {
+   pr_err("No loadable segments\n");
+   return -EINVAL;
+   }
+
+   if (ehdr->e_phoff > size) {
+   pr_err("Firmware size is too small\n");
+   return -EINVAL;
+   }
+
+   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);
+   if (!ops) {
+   dev_dbg(dev, "Driver has no ops?\n");
+   return -EINVAL;
+   }
+
+   /* 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->da_to_pa)
+   dst = (void *)ops->da_to_pa(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 a/include/remoteproc.h