Re: [PATCH] remoteproc: imx_dsp_rproc: add custom memory copy implementation for i.MX DSP Cores

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mathieu,

On 1/27/2023 12:49 AM, Mathieu Poirier wrote:
On Wed, Jan 25, 2023 at 01:01:00PM +0200, Iuliana Prodan (OSS) wrote:
From: Iuliana Prodan <iuliana.prodan@xxxxxxx>

The IRAM is part of the HiFi DSP.
According to hardware specification only 32-bits write are allowed
otherwise we get a Kernel panic.

Therefore add a custom memory copy function to deal with the
above restriction.

Signed-off-by: Iuliana Prodan <iuliana.prodan@xxxxxxx>
---
  drivers/remoteproc/imx_dsp_rproc.c | 122 ++++++++++++++++++++++++++++-
  1 file changed, 121 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/imx_dsp_rproc.c b/drivers/remoteproc/imx_dsp_rproc.c
index 95da1cbefacf..a9991d085494 100644
--- a/drivers/remoteproc/imx_dsp_rproc.c
+++ b/drivers/remoteproc/imx_dsp_rproc.c
@@ -715,6 +715,126 @@ static void imx_dsp_rproc_kick(struct rproc *rproc, int vqid)
  		dev_err(dev, "%s: failed (%d, err:%d)\n", __func__, vqid, err);
  }
+/*
+ * Custom memory copy implementation for i.MX DSP Cores
+ *
+ * The IRAM is part of the HiFi DSP.
+ * According to hw specs only 32-bits writes are allowed.
+ */
+static int imx_dsp_rproc_memcpy(void *dest, const void *src, size_t size)
+{
+	const u8 *src_byte = src;
+	u32 affected_mask;
+	u32 tmp;
+	int q, r;
+
+	q = size / 4;
+	r = size % 4;
+
+	/* __iowrite32_copy use 32bit size values so divide by 4 */
+	__iowrite32_copy(dest, src, q);
The current driver for imx_dsp_rproc does not provide a rproc_da_to_va()
operation, meaning that @is_iomem in rproc_elf_load_segments() can't be true,
forcing a memcpy() operation to be used.  And yet above an _iowrite32_copy() is
used...

Yes, with rproc_elf_load_segments() we go through memcpy() because io_mem is always false (I already have a patch to get rid of this flag from imx_dsp_rproc since is not used), but memcpy() vs __iowrite32_copy() is crashing on sizes that are not multiple of 32bit.

That's why I added the __iowrite32_copy() and above this, I'm dividing the size by 4.

Also,  in imx_dsp_rproc, we shouldn't use memcpy, it should be memcpy_toio because all addresses are ioremap - see https://elixir.bootlin.com/linux/v6.2-rc5/source/drivers/remoteproc/imx_dsp_rproc.c#L601

In the conversation that came out of[1], Daniel Baluta mentions that
read/writes should be done in multiples of 32/64 bit but here a blanket 32 bit
enforcement is done.

I've checked deeper the documentation and talked to our hardware team, and for NXP's DSPs we have a write restriction of 32bit.

+
+	if (r) {
+		affected_mask = (1 << (8 * r)) - 1;
+
+		/* first read the 32bit data of dest, then change affected
+		 * bytes, and write back to dest.
+		 * For unaffected bytes, it should not be changed
+		 */
+		tmp = ioread32(dest + q * 4);
+		tmp &= ~affected_mask;
+
+		tmp |= *(u32 *)(src_byte + q * 4) & affected_mask;
+		iowrite32(tmp, dest + q * 4);
+	}
+
+	return 0;
+}
+
+/**
+ * imx_dsp_rproc_elf_load_segments() - load firmware segments to memory
+ * @rproc: remote processor which will be booted using these fw segments
+ * @fw: the ELF firmware image
+ *
+ * This function loads the firmware segments to memory, where the remote
+ * processor expects them.
+ *
+ * Return: 0 on success and an appropriate error code otherwise
+ */
+static int imx_dsp_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);
+		bool is_iomem = false;
+		void *ptr;
+
+		if (type != PT_LOAD || !memsz)
+			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, &is_iomem);
+		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) {
+			ret = imx_dsp_rproc_memcpy(ptr, elf_data + offset, filesz);
+			if (ret) {
+				dev_err(dev, "memory copy failed for da 0x%llx memsz 0x%llx\n",
+					da, memsz);
+				break;
+			}
+		}
This patchset from last year[1] goes to great length to avoid using a driver
specific function and now you are trying to bring that back...  So how was it
working before and why are things broken now?

Until now, it was used in a limited scenario and the firmware was correctly built to respect the write restriction - having the IRAM sections size a multiple of 4bytes.

Now, I was trying a simple hello_world sample from Zephyr, complied with gcc and I crashed the Kernel trying to load it on the hifi4 DSP:

[ 2707.135094] SError Interrupt on CPU0, code 0x00000000bf000002 -- SError
[ 2707.135104] CPU: 0 PID: 665 Comm: sh Tainted: G C         6.1.0-rc6-04789-gc80e5155d190 #135
[ 2707.135112] Hardware name: Freescale i.MX8QM MEK (DT)
[ 2707.135115] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[ 2707.135123] pc : vprintk_store+0x3c0/0x460
[ 2707.135141] lr : vprintk_store+0x48/0x460
[ 2707.135149] sp : ffff80000b31b8c0
[ 2707.135152] x29: ffff80000b31b8c0 x28: 0000000000000018 x27: 0000000000000000 [ 2707.135164] x26: 00000000596f8000 x25: ffff000810c15038 x24: 0000000000000000 [ 2707.135173] x23: ffff8000098dfaf8 x22: 0000000000000000 x21: 0000000000000000 [ 2707.135181] x20: ffff80000b31ba30 x19: ffff800009abca31 x18: ffffffffffffffff [ 2707.135190] x17: 6620313231783020 x16: 7a736d656d203030 x15: ffff80008b31b867 [ 2707.135199] x14: 0000000000000000 x13: ffff800009d22428 x12: 000000000000083a [ 2707.135208] x11: 00000000000002be x10: 0000000000000a40 x9 : ffff80000b31bb70 [ 2707.135216] x8 : ffff80000b31bb70 x7 : 00000000ffffffc8 x6 : ffff80000b31bb30 [ 2707.135224] x5 : ffff00081a098000 x4 : ffff80000b31ba30 x3 : ffff8000098dfaf8 [ 2707.135233] x2 : ffff80000990c000 x1 : 0000000000000000 x0 : ffff8008eface000
[ 2707.135243] Kernel panic - not syncing: Asynchronous SError Interrupt
[ 2707.135248] CPU: 0 PID: 665 Comm: sh Tainted: G C         6.1.0-rc6-04789-gc80e5155d190 #135
[ 2707.135254] Hardware name: Freescale i.MX8QM MEK (DT)
[ 2707.135258] Call trace:
[ 2707.135261]  dump_backtrace.part.0+0xdc/0xf0
[ 2707.135275]  show_stack+0x18/0x40
[ 2707.135284]  dump_stack_lvl+0x68/0x84
[ 2707.135295]  dump_stack+0x18/0x34
[ 2707.135302]  panic+0x184/0x344
[ 2707.135311]  nmi_panic+0xac/0xb0
[ 2707.135319]  arm64_serror_panic+0x6c/0x7c
[ 2707.135324]  do_serror+0x58/0x5c
[ 2707.135329]  el1h_64_error_handler+0x30/0x4c
[ 2707.135339]  el1h_64_error+0x64/0x68
[ 2707.135344]  vprintk_store+0x3c0/0x460
[ 2707.135353]  vprintk_emit+0x104/0x294
[ 2707.135360]  vprintk_default+0x38/0x4c
[ 2707.135369]  vprintk+0xc0/0xe4
[ 2707.135376]  _printk+0x5c/0x84
[ 2707.135383]  rproc_elf_load_segments+0x228/0x308
[ 2707.135391]  rproc_start+0x50/0x1c8
[ 2707.135398]  rproc_boot+0x494/0x574
[ 2707.135404]  state_store+0x44/0x110
[ 2707.135413]  dev_attr_store+0x18/0x30
[ 2707.135422]  sysfs_kf_write+0x44/0x54
[ 2707.135433]  kernfs_fop_write_iter+0x118/0x1b0
[ 2707.135441]  vfs_write+0x220/0x2b0
[ 2707.135449]  ksys_write+0x68/0xf4
[ 2707.135455]  __arm64_sys_write+0x1c/0x2c
[ 2707.135461]  invoke_syscall+0x48/0x114
[ 2707.135470]  el0_svc_common.constprop.0+0xd4/0xfc
[ 2707.135477]  do_el0_svc+0x30/0xd0
[ 2707.135484]  el0_svc+0x2c/0x84
[ 2707.135492]  el0t_64_sync_handler+0xbc/0x140
[ 2707.135501]  el0t_64_sync+0x18c/0x190
[ 2707.135510] SMP: stopping secondary CPUs
[ 2707.135520] Kernel Offset: disabled
[ 2707.135522] CPU features: 0x20000,2082c084,0000421b
[ 2707.135527] Memory Limit: none
[ 2707.397688] ---[ end Kernel panic - not syncing: Asynchronous SError Interrupt ]---

  Moreover, function
rproc_elf_load_segments() deals with situations where the memory slot is bigger
than the file size[2], which is omitted here.

I'll add this part in v2.

Thanks,

Iulia

Thanks,
Mathieu

[1]. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flinux-arm-kernel%2F20220323064944.1351923-1-peng.fan%40oss.nxp.com%2F&data=05%7C01%7Ciuliana.prodan%40nxp.com%7Cd946b7c87aaf4016c69908daffef8a64%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103701532520716%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=GNJhJnoiRWfAzvseUsAdwHRtY2w2mA816CKkTL%2F2czw%3D&reserved=0
[2]. https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv6.2-rc5%2Fsource%2Fdrivers%2Fremoteproc%2Fremoteproc_elf_loader.c%23L221&data=05%7C01%7Ciuliana.prodan%40nxp.com%7Cd946b7c87aaf4016c69908daffef8a64%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638103701532520716%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5P5FcuMb6H%2B5S4L2Il4BBAxOOIrr6Er5thNvhADhKdc%3D&reserved=0




[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Photo Sharing]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux