| Message ID | 20200317191918.4123-1-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | remoteproc: qcom_q6v5_mss: map/unmap mpss region before/after use | expand |
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? > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > 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. 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. > + 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. > + 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. 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
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@codeaurora.org> >> --- >> 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
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); +} + 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); + if (!ptr) { + dev_err(qproc->dev, "failed to map memory\n"); + 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); 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 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. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_mss.c | 53 ++++++++++++++++-------------- 1 file changed, 29 insertions(+), 24 deletions(-)