Re: [PATCH 06/10] remoteproc: imx_rproc: add load hook

From: Mathieu Poirier
Date: Tue Aug 11 2020 - 17:41:01 EST


On Fri, Jul 24, 2020 at 04:08:09PM +0800, Peng Fan wrote:
> To i.MX8, we not able to see the correct data written into TCM when
> using ioremap_wc, so use ioremap.
>
> However common elf loader using memset.
>
> To arm64, "dc zva, dst" is used in memset.
> Per ARM DDI 0487A.j, chapter C5.3.8 DC ZVA, Data Cache Zero by VA,
>
> "If the memory region being zeroed is any type of Device memory,
> this instruction can give an alignment fault which is prioritized
> in the same way as other alignment faults that are determined
> by the memory type."
>
> On i.MX platforms, when elf is loaded to onchip TCM area, the region
> is ioremapped, so "dc zva, dst" will trigger abort.
>
> So add i.MX specific loader to address the TCM write issue.
>
> The change not impact i.MX6/7 function.
>
> Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> ---
> drivers/remoteproc/imx_rproc.c | 76 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index aee790efbf7b..c23726091228 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -4,6 +4,7 @@
> */
>
> #include <linux/clk.h>
> +#include <linux/elf.h>
> #include <linux/err.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> @@ -15,6 +16,9 @@
> #include <linux/regmap.h>
> #include <linux/remoteproc.h>
>
> +#include "remoteproc_internal.h"
> +#include "remoteproc_elf_helpers.h"
> +
> #define IMX7D_SRC_SCR 0x0C
> #define IMX7D_ENABLE_M4 BIT(3)
> #define IMX7D_SW_M4P_RST BIT(2)
> @@ -247,10 +251,82 @@ static void *imx_rproc_da_to_va(struct rproc *rproc, u64 da, size_t len)
> return va;
> }
>
> +static int imx_rproc_elf_load_segments(struct rproc *rproc, const struct firmware *fw)
> +{
> + struct device *dev = &rproc->dev;
> + const void *ehdr, *phdr;
> + int i, ret = 0;
> + u16 phnum;
> + const u8 *elf_data = fw->data;
> + u8 class = fw_elf_get_class(fw);
> + u32 elf_phdr_get_size = elf_size_of_phdr(class);
> +
> + ehdr = elf_data;
> + phnum = elf_hdr_get_e_phnum(class, ehdr);
> + phdr = elf_data + elf_hdr_get_e_phoff(class, ehdr);
> +
> + /* go through the available ELF segments */
> + for (i = 0; i < phnum; i++, phdr += elf_phdr_get_size) {
> + u64 da = elf_phdr_get_p_paddr(class, phdr);
> + u64 memsz = elf_phdr_get_p_memsz(class, phdr);
> + u64 filesz = elf_phdr_get_p_filesz(class, phdr);
> + u64 offset = elf_phdr_get_p_offset(class, phdr);
> + u32 type = elf_phdr_get_p_type(class, phdr);
> + void *ptr;
> +
> + if (type != PT_LOAD)
> + continue;
> +
> + dev_dbg(dev, "phdr: type %d da 0x%llx memsz 0x%llx filesz 0x%llx\n",
> + type, da, memsz, filesz);
> +
> + if (filesz > memsz) {
> + dev_err(dev, "bad phdr filesz 0x%llx memsz 0x%llx\n",
> + filesz, memsz);
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (offset + filesz > fw->size) {
> + dev_err(dev, "truncated fw: need 0x%llx avail 0x%zx\n",
> + offset + filesz, fw->size);
> + ret = -EINVAL;
> + break;
> + }
> +
> + if (!rproc_u64_fit_in_size_t(memsz)) {
> + dev_err(dev, "size (%llx) does not fit in size_t type\n",
> + memsz);
> + ret = -EOVERFLOW;
> + break;
> + }
> +
> + /* grab the kernel address for this device address */
> + ptr = rproc_da_to_va(rproc, da, memsz);
> + if (!ptr) {
> + dev_err(dev, "bad phdr da 0x%llx mem 0x%llx\n", da,
> + memsz);
> + ret = -EINVAL;
> + break;
> + }
> +
> + /* put the segment where the remote processor expects it */
> + if (filesz)
> + memcpy_toio(ptr, elf_data + offset, filesz);
> + }
> +
> + return ret;
> +}

This is clearly the wrong approach. What you came up with in [1] is far better,
though I would call the the operations elf_memcpy() and elf_memset().

That being said I don't know how [1] fits with this patchset. From where I
stand a new revision is needed.

[1]. https://patchwork.kernel.org/patch/11688751/

> +
> static const struct rproc_ops imx_rproc_ops = {
> .start = imx_rproc_start,
> .stop = imx_rproc_stop,
> .da_to_va = imx_rproc_da_to_va,
> + .load = imx_rproc_elf_load_segments,
> + .parse_fw = rproc_elf_load_rsc_table,
> + .find_loaded_rsc_table = rproc_elf_find_loaded_rsc_table,
> + .sanity_check = rproc_elf_sanity_check,
> + .get_boot_addr = rproc_elf_get_boot_addr,
> };
>
> static int imx_rproc_addr_init(struct imx_rproc *priv,
> --
> 2.16.4
>