Re: [PATCH] remoteproc: qcom_q6v5_mss: map/unmap mpss region before/after use

From: Sibi Sankar
Date: Wed Apr 08 2020 - 08:21:15 EST

Hey Bjorn,
Thanks for review!

On 2020-04-08 07:40, Bjorn Andersson wrote:
On Tue 17 Mar 12:19 PDT 2020, Sibi Sankar wrote:

The application processor accessing the mpss region when the Q6 modem
is running will lead to an XPU violation. Fix this by un-mapping the
mpss region post copy during processor out of reset sequence and
coredumps.


Does this problem not apply to the "mba" region?

For mba region, memcpy is expected
to be completed before bringing
the Q6 out of reset. Downstream they
seem to use a wmb() to accomplish
this. Since we havn't observed any
issues until now, we can defer adding
any fixes related to mba region.


Signed-off-by: Sibi Sankar <sibis@xxxxxxxxxxxxxx>
---
drivers/remoteproc/qcom_q6v5_mss.c | 53 ++++++++++++++++--------------
1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index ce49c3236ff7c..b1ad4de179019 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -196,7 +196,6 @@ struct q6v5 {

phys_addr_t mpss_phys;
phys_addr_t mpss_reloc;
- void *mpss_region;
size_t mpss_size;

struct qcom_rproc_glink glink_subdev;
@@ -1061,6 +1060,18 @@ static int q6v5_reload_mba(struct rproc *rproc)
return ret;
}

+static void *q6v5_da_to_va(struct rproc *rproc, u64 da, size_t len)
+{
+ struct q6v5 *qproc = rproc->priv;
+ int offset;
+
+ offset = da - qproc->mpss_reloc;
+ if (offset < 0 || offset + len > qproc->mpss_size)
+ return NULL;
+
+ return devm_ioremap_wc(qproc->dev, qproc->mpss_phys + offset, len);

This function isn't expected to have side effects.

unfortunately doing ioremap/iounmap
for the entire region isn't sufficient,
while testing I found per region remap
/unmap was required i.e after we moved to
away from the validating the entire blog
in a single pass.

Now with the mpss_region out of the picture
da_to_va really made no sense without having
ioremap function in it. Let me know what you
think.


So I think you should add the ioremap/iounmap to the beginning/end of
mpss_load and the dump_segment directly instead.

+}
+
static int q6v5_mpss_load(struct q6v5 *qproc)
{
const struct elf32_phdr *phdrs;
@@ -1156,7 +1167,11 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
goto release_firmware;
}

- ptr = qproc->mpss_region + offset;
+ ptr = q6v5_da_to_va(qproc->rproc, phdr->p_paddr, phdr->p_memsz);

rproc_da_to_va() here.

sure


+ if (!ptr) {
+ dev_err(qproc->dev, "failed to map memory\n");

Now this will be able to fail, so you should add this error handling
snippet, just with a slightly different message.

sure


+ goto release_firmware;
+ }

if (phdr->p_filesz && phdr->p_offset < fw->size) {
/* Firmware is large enough to be non-split */
@@ -1165,6 +1180,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
"failed to load segment %d from truncated file %s\n",
i, fw_name);
ret = -EINVAL;
+ devm_iounmap(qproc->dev, ptr);
goto release_firmware;
}

@@ -1175,6 +1191,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
ret = request_firmware(&seg_fw, fw_name, qproc->dev);
if (ret) {
dev_err(qproc->dev, "failed to load %s\n", fw_name);
+ devm_iounmap(qproc->dev, ptr);
goto release_firmware;
}

@@ -1187,6 +1204,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc)
memset(ptr + phdr->p_filesz, 0,
phdr->p_memsz - phdr->p_filesz);
}
+ devm_iounmap(qproc->dev, ptr);

Move this to the end an unmap the entire thing.

And generally, please avoid devm for things where you manually unmap.

will take care of ^^ in the next re-spin.


Regards,
Bjorn

size += phdr->p_memsz;

code_length = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG);
@@ -1236,7 +1254,7 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc,
int ret = 0;
struct q6v5 *qproc = rproc->priv;
unsigned long mask = BIT((unsigned long)segment->priv);
- void *ptr = rproc_da_to_va(rproc, segment->da, segment->size);
+ void *ptr = NULL;

/* Unlock mba before copying segments */
if (!qproc->dump_mba_loaded) {
@@ -1250,10 +1268,15 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc,
}
}

- if (!ptr || ret)
- memset(dest, 0xff, segment->size);
- else
+ if (!ret)
+ ptr = rproc_da_to_va(rproc, segment->da, segment->size);
+
+ if (ptr) {
memcpy(dest, ptr, segment->size);
+ devm_iounmap(qproc->dev, ptr);
+ } else {
+ memset(dest, 0xff, segment->size);
+ }

qproc->dump_segment_mask |= mask;

@@ -1327,18 +1350,6 @@ static int q6v5_stop(struct rproc *rproc)
return 0;
}

-static void *q6v5_da_to_va(struct rproc *rproc, u64 da, size_t len)
-{
- struct q6v5 *qproc = rproc->priv;
- int offset;
-
- offset = da - qproc->mpss_reloc;
- if (offset < 0 || offset + len > qproc->mpss_size)
- return NULL;
-
- return qproc->mpss_region + offset;
-}
-
static int qcom_q6v5_register_dump_segments(struct rproc *rproc,
const struct firmware *mba_fw)
{
@@ -1595,12 +1606,6 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc)

qproc->mpss_phys = qproc->mpss_reloc = r.start;
qproc->mpss_size = resource_size(&r);
- qproc->mpss_region = devm_ioremap_wc(qproc->dev, qproc->mpss_phys, qproc->mpss_size);
- if (!qproc->mpss_region) {
- dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n",
- &r.start, qproc->mpss_size);
- return -EBUSY;
- }

return 0;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.