* [PATCH v2 0/2] remoteproc: mss: Improve mem_assign and firmware load @ 2019-11-09 0:40 Bjorn Andersson 2019-11-09 0:40 ` [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson 2019-11-09 0:40 ` [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 0 siblings, 2 replies; 7+ messages in thread From: Bjorn Andersson @ 2019-11-09 0:40 UTC (permalink / raw) To: Ohad Ben-Cohen, Bjorn Andersson, Avaneesh Kumar Dwivedi Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo Two things came up in the effort of figuring out why the modem crashed the entire system when being restarted; the first one solves the actual problem, in that it's not possible to reclaim the main modem firmware region unless the modem subsystem is running - causing the crash. The second patch aligns the firmware loading process to that of the downstream driver, which seems to be a requirement in 8974 as well. Bjorn Andersson (2): remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown remoteproc: qcom_q6v5_mss: Validate each segment during loading drivers/remoteproc/qcom_q6v5_mss.c | 92 +++++++++++++++++++----------- 1 file changed, 59 insertions(+), 33 deletions(-) -- 2.23.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2019-11-09 0:40 [PATCH v2 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson @ 2019-11-09 0:40 ` Bjorn Andersson 2019-11-12 17:01 ` Jeffrey Hugo 2019-11-09 0:40 ` [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2019-11-09 0:40 UTC (permalink / raw) To: Ohad Ben-Cohen, Bjorn Andersson, Avaneesh Kumar Dwivedi Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, stable Trying to reclaim mpss memory while the mba is not running causes the system to crash on devices with security fuses blown, so leave it assigned to the remote on shutdown and recover it on a subsequent boot. Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch") Cc: stable@vger.kernel.org Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v1: - Assign memory back to Linux in coredump case drivers/remoteproc/qcom_q6v5_mss.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index de919f2e8b94..efab574b2e12 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -875,11 +875,6 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); } - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, - false, qproc->mpss_phys, - qproc->mpss_size); - WARN_ON(ret); - q6v5_reset_assert(qproc); q6v5_clk_disable(qproc->dev, qproc->reset_clks, @@ -969,6 +964,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K); } + /* Try to reset ownership back to Linux */ + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, + qproc->mpss_phys, qproc->mpss_size); + mpss_reloc = relocate ? min_addr : qproc->mpss_phys; qproc->mpss_reloc = mpss_reloc; /* Load firmware segments */ @@ -1058,9 +1057,14 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc, void *ptr = rproc_da_to_va(rproc, segment->da, segment->size); /* Unlock mba before copying segments */ - if (!qproc->dump_mba_loaded) + if (!qproc->dump_mba_loaded) { ret = q6v5_mba_load(qproc); + /* Try to reset ownership back to Linux */ + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, + qproc->mpss_phys, qproc->mpss_size); + } + if (!ptr || ret) memset(dest, 0xff, segment->size); else @@ -1111,10 +1115,6 @@ static int q6v5_start(struct rproc *rproc) return 0; reclaim_mpss: - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, - false, qproc->mpss_phys, - qproc->mpss_size); - WARN_ON(xfermemop_ret); q6v5_mba_reclaim(qproc); return ret; -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2019-11-09 0:40 ` [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson @ 2019-11-12 17:01 ` Jeffrey Hugo 2019-11-13 18:02 ` Jeffrey Hugo 0 siblings, 1 reply; 7+ messages in thread From: Jeffrey Hugo @ 2019-11-12 17:01 UTC (permalink / raw) To: Bjorn Andersson Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc, lkml, Sibi Sankar, stable On Fri, Nov 8, 2019 at 5:40 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > Trying to reclaim mpss memory while the mba is not running causes the > system to crash on devices with security fuses blown, so leave it > assigned to the remote on shutdown and recover it on a subsequent boot. > > Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch") > Cc: stable@vger.kernel.org > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> Stuff still works on the laptop, and I don't hit the access violation with the crash dump scenario on the mtp. Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > > Changes since v1: > - Assign memory back to Linux in coredump case > > drivers/remoteproc/qcom_q6v5_mss.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index de919f2e8b94..efab574b2e12 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -875,11 +875,6 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) > writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > } > > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, > - false, qproc->mpss_phys, > - qproc->mpss_size); > - WARN_ON(ret); > - > q6v5_reset_assert(qproc); > > q6v5_clk_disable(qproc->dev, qproc->reset_clks, > @@ -969,6 +964,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K); > } > > + /* Try to reset ownership back to Linux */ > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > + qproc->mpss_phys, qproc->mpss_size); > + > mpss_reloc = relocate ? min_addr : qproc->mpss_phys; > qproc->mpss_reloc = mpss_reloc; > /* Load firmware segments */ > @@ -1058,9 +1057,14 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc, > void *ptr = rproc_da_to_va(rproc, segment->da, segment->size); > > /* Unlock mba before copying segments */ > - if (!qproc->dump_mba_loaded) > + if (!qproc->dump_mba_loaded) { > ret = q6v5_mba_load(qproc); > > + /* Try to reset ownership back to Linux */ > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > + qproc->mpss_phys, qproc->mpss_size); > + } > + > if (!ptr || ret) > memset(dest, 0xff, segment->size); > else > @@ -1111,10 +1115,6 @@ static int q6v5_start(struct rproc *rproc) > return 0; > > reclaim_mpss: > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, > - false, qproc->mpss_phys, > - qproc->mpss_size); > - WARN_ON(xfermemop_ret); > q6v5_mba_reclaim(qproc); > > return ret; > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2019-11-12 17:01 ` Jeffrey Hugo @ 2019-11-13 18:02 ` Jeffrey Hugo 0 siblings, 0 replies; 7+ messages in thread From: Jeffrey Hugo @ 2019-11-13 18:02 UTC (permalink / raw) To: Bjorn Andersson Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc, lkml, Sibi Sankar, stable On Tue, Nov 12, 2019 at 10:01 AM Jeffrey Hugo <jeffrey.l.hugo@gmail.com> wrote: > > On Fri, Nov 8, 2019 at 5:40 PM Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > > > Trying to reclaim mpss memory while the mba is not running causes the > > system to crash on devices with security fuses blown, so leave it > > assigned to the remote on shutdown and recover it on a subsequent boot. > > > > Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch") > > Cc: stable@vger.kernel.org > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > Stuff still works on the laptop, and I don't hit the access violation > with the crash dump scenario on the mtp. > > Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Actually, nack that. See comment below. > > > --- > > > > Changes since v1: > > - Assign memory back to Linux in coredump case > > > > drivers/remoteproc/qcom_q6v5_mss.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > > index de919f2e8b94..efab574b2e12 100644 > > --- a/drivers/remoteproc/qcom_q6v5_mss.c > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > > @@ -875,11 +875,6 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) > > writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG); > > } > > > > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, > > - false, qproc->mpss_phys, > > - qproc->mpss_size); > > - WARN_ON(ret); > > - > > q6v5_reset_assert(qproc); > > > > q6v5_clk_disable(qproc->dev, qproc->reset_clks, > > @@ -969,6 +964,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > max_addr = ALIGN(phdr->p_paddr + phdr->p_memsz, SZ_4K); > > } > > > > + /* Try to reset ownership back to Linux */ > > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > > + qproc->mpss_phys, qproc->mpss_size); > > + > > mpss_reloc = relocate ? min_addr : qproc->mpss_phys; > > qproc->mpss_reloc = mpss_reloc; > > /* Load firmware segments */ > > @@ -1058,9 +1057,14 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc, > > void *ptr = rproc_da_to_va(rproc, segment->da, segment->size); > > > > /* Unlock mba before copying segments */ > > - if (!qproc->dump_mba_loaded) > > + if (!qproc->dump_mba_loaded) { > > ret = q6v5_mba_load(qproc); > > > > + /* Try to reset ownership back to Linux */ > > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > > + qproc->mpss_phys, qproc->mpss_size); If the load fails, we can't pull the memory otherwise we'll hit the access violation (serror). I happened to see this on a production device, where I think the load fails because crashdumps are not enabled. > > + } > > + > > if (!ptr || ret) > > memset(dest, 0xff, segment->size); > > else > > @@ -1111,10 +1115,6 @@ static int q6v5_start(struct rproc *rproc) > > return 0; > > > > reclaim_mpss: > > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, > > - false, qproc->mpss_phys, > > - qproc->mpss_size); > > - WARN_ON(xfermemop_ret); > > q6v5_mba_reclaim(qproc); > > > > return ret; > > -- > > 2.23.0 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading 2019-11-09 0:40 [PATCH v2 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson 2019-11-09 0:40 ` [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson @ 2019-11-09 0:40 ` Bjorn Andersson 2019-11-10 14:05 ` Luca Weiss 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2019-11-09 0:40 UTC (permalink / raw) To: Ohad Ben-Cohen, Bjorn Andersson, Avaneesh Kumar Dwivedi Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo The code used to sync with the MBA after each segment loaded and this is still what's done downstream. So reduce the delta towards downstream by switching to a model where the content is iteratively validated. Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> --- Changes since v1: - Picked up Jeff's r-b and t-b drivers/remoteproc/qcom_q6v5_mss.c | 76 ++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 25 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index efab574b2e12..914d5546e1cf 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -358,23 +358,29 @@ static void q6v5_pds_disable(struct q6v5 *qproc, struct device **pds, } static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm, - bool remote_owner, phys_addr_t addr, + bool local, bool remote, phys_addr_t addr, size_t size) { - struct qcom_scm_vmperm next; + struct qcom_scm_vmperm next[2]; + int perms = 0; if (!qproc->need_mem_protection) return 0; - if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA)) - return 0; - if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS)) - return 0; - next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA : QCOM_SCM_VMID_HLOS; - next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX; + if (local) { + next[perms].vmid = QCOM_SCM_VMID_HLOS; + next[perms].perm = QCOM_SCM_PERM_RWX; + perms++; + } + + if (remote) { + next[perms].vmid = QCOM_SCM_VMID_MSS_MSA; + next[perms].perm = QCOM_SCM_PERM_RW; + perms++; + } return qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), - current_perm, &next, 1); + current_perm, next, perms); } static int q6v5_load(struct rproc *rproc, const struct firmware *fw) @@ -681,7 +687,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) /* Hypervisor mapping to access metadata by modem */ mdata_perm = BIT(QCOM_SCM_VMID_HLOS); - ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, phys, size); + ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true, phys, size); if (ret) { dev_err(qproc->dev, "assigning Q6 access to metadata failed: %d\n", ret); @@ -699,7 +705,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) dev_err(qproc->dev, "MPSS header authentication failed: %d\n", ret); /* Metadata authentication done, remove modem access */ - xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, phys, size); + xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, false, phys, size); if (xferop_ret) dev_warn(qproc->dev, "mdt buffer not reclaimed system may become unstable\n"); @@ -786,7 +792,7 @@ static int q6v5_mba_load(struct q6v5 *qproc) } /* Assign MBA image access in DDR to q6 */ - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, true, qproc->mba_phys, qproc->mba_size); if (ret) { dev_err(qproc->dev, @@ -820,8 +826,8 @@ static int q6v5_mba_load(struct q6v5 *qproc) q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); reclaim_mba: - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, - qproc->mba_phys, + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, + false, qproc->mba_phys, qproc->mba_size); if (xfermemop_ret) { dev_err(qproc->dev, @@ -888,7 +894,7 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) /* In case of failure or coredump scenario where reclaiming MBA memory * could not happen reclaim it here. */ - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, false, qproc->mba_phys, qproc->mba_size); WARN_ON(ret); @@ -915,6 +921,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) phys_addr_t boot_addr; phys_addr_t min_addr = PHYS_ADDR_MAX; phys_addr_t max_addr = 0; + u32 code_length; bool relocate = false; char *fw_name; size_t fw_name_len; @@ -965,9 +972,19 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } /* Try to reset ownership back to Linux */ - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false, qproc->mpss_phys, qproc->mpss_size); + /* Share ownership between Linux and MSS, during segment loading */ + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, true, + qproc->mpss_phys, qproc->mpss_size); + if (ret) { + dev_err(qproc->dev, + "assigning Q6 access to mpss memory failed: %d\n", ret); + ret = -EAGAIN; + goto release_firmware; + } + mpss_reloc = relocate ? min_addr : qproc->mpss_phys; qproc->mpss_reloc = mpss_reloc; /* Load firmware segments */ @@ -1016,10 +1033,24 @@ static int q6v5_mpss_load(struct q6v5 *qproc) phdr->p_memsz - phdr->p_filesz); } size += phdr->p_memsz; + + code_length = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); + if (!code_length) { + boot_addr = relocate ? qproc->mpss_phys : min_addr; + writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); + writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); + } + writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); + + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG); + if (ret < 0) { + dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret); + goto release_firmware; + } } /* Transfer ownership of modem ddr region to q6 */ - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, true, qproc->mpss_phys, qproc->mpss_size); if (ret) { dev_err(qproc->dev, @@ -1028,11 +1059,6 @@ static int q6v5_mpss_load(struct q6v5 *qproc) goto release_firmware; } - boot_addr = relocate ? qproc->mpss_phys : min_addr; - writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); - writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); - ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000); if (ret == -ETIMEDOUT) dev_err(qproc->dev, "MPSS authentication timed out\n"); @@ -1061,7 +1087,7 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc, ret = q6v5_mba_load(qproc); /* Try to reset ownership back to Linux */ - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false, qproc->mpss_phys, qproc->mpss_size); } @@ -1101,8 +1127,8 @@ static int q6v5_start(struct rproc *rproc) goto reclaim_mpss; } - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, - qproc->mba_phys, + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, + false, qproc->mba_phys, qproc->mba_size); if (xfermemop_ret) dev_err(qproc->dev, -- 2.23.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading 2019-11-09 0:40 ` [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson @ 2019-11-10 14:05 ` Luca Weiss 2019-11-11 6:45 ` Bjorn Andersson 0 siblings, 1 reply; 7+ messages in thread From: Luca Weiss @ 2019-11-10 14:05 UTC (permalink / raw) To: linux-arm-msm Cc: Bjorn Andersson, Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, Brian Masney [-- Attachment #1: Type: text/plain, Size: 7811 bytes --] Hi Bjorn, with your patches and modifications in qcom-msm8974.dtsi, I can boot the modem successfully on the Fairphone 2, without the 'hack' commit we had in the tree before! Thanks! Tested-by: Luca Weiss <luca@z3ntu.xyz> On Samstag, 9. November 2019 01:40:33 CET Bjorn Andersson wrote: > The code used to sync with the MBA after each segment loaded and this is > still what's done downstream. So reduce the delta towards downstream by > switching to a model where the content is iteratively validated. > > Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > > Changes since v1: > - Picked up Jeff's r-b and t-b > > drivers/remoteproc/qcom_q6v5_mss.c | 76 ++++++++++++++++++++---------- > 1 file changed, 51 insertions(+), 25 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c > b/drivers/remoteproc/qcom_q6v5_mss.c index efab574b2e12..914d5546e1cf > 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -358,23 +358,29 @@ static void q6v5_pds_disable(struct q6v5 *qproc, > struct device **pds, } > > static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm, > - bool remote_owner, phys_addr_t addr, > + bool local, bool remote, phys_addr_t addr, > size_t size) > { > - struct qcom_scm_vmperm next; > + struct qcom_scm_vmperm next[2]; > + int perms = 0; > > if (!qproc->need_mem_protection) > return 0; > - if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA)) > - return 0; > - if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS)) > - return 0; > > - next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA : QCOM_SCM_VMID_HLOS; > - next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX; > + if (local) { > + next[perms].vmid = QCOM_SCM_VMID_HLOS; > + next[perms].perm = QCOM_SCM_PERM_RWX; > + perms++; > + } > + > + if (remote) { > + next[perms].vmid = QCOM_SCM_VMID_MSS_MSA; > + next[perms].perm = QCOM_SCM_PERM_RW; > + perms++; > + } > > return qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), > - current_perm, &next, 1); > + current_perm, next, perms); > } > > static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > @@ -681,7 +687,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, > const struct firmware *fw) > > /* Hypervisor mapping to access metadata by modem */ > mdata_perm = BIT(QCOM_SCM_VMID_HLOS); > - ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, phys, size); > + ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true, phys, > size); if (ret) { > dev_err(qproc->dev, > "assigning Q6 access to metadata failed: %d\n", ret); > @@ -699,7 +705,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, > const struct firmware *fw) dev_err(qproc->dev, "MPSS header authentication > failed: %d\n", ret); > > /* Metadata authentication done, remove modem access */ > - xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, phys, > size); + xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, > false, phys, size); if (xferop_ret) > dev_warn(qproc->dev, > "mdt buffer not reclaimed system may become unstable\n"); > @@ -786,7 +792,7 @@ static int q6v5_mba_load(struct q6v5 *qproc) > } > > /* Assign MBA image access in DDR to q6 */ > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, true, > qproc->mba_phys, qproc- >mba_size); > if (ret) { > dev_err(qproc->dev, > @@ -820,8 +826,8 @@ static int q6v5_mba_load(struct q6v5 *qproc) > q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); > > reclaim_mba: > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, > - qproc- >mba_phys, > + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, > + false, qproc->mba_phys, > qproc- >mba_size); > if (xfermemop_ret) { > dev_err(qproc->dev, > @@ -888,7 +894,7 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) > /* In case of failure or coredump scenario where reclaiming MBA memory > * could not happen reclaim it here. > */ > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, false, > qproc->mba_phys, > qproc->mba_size); > WARN_ON(ret); > @@ -915,6 +921,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > phys_addr_t boot_addr; > phys_addr_t min_addr = PHYS_ADDR_MAX; > phys_addr_t max_addr = 0; > + u32 code_length; > bool relocate = false; > char *fw_name; > size_t fw_name_len; > @@ -965,9 +972,19 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > } > > /* Try to reset ownership back to Linux */ > - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false, > qproc->mpss_phys, qproc- >mpss_size); > > + /* Share ownership between Linux and MSS, during segment loading */ > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, true, > + qproc->mpss_phys, qproc- >mpss_size); > + if (ret) { > + dev_err(qproc->dev, > + "assigning Q6 access to mpss memory failed: %d\n", ret); > + ret = -EAGAIN; > + goto release_firmware; > + } > + > mpss_reloc = relocate ? min_addr : qproc->mpss_phys; > qproc->mpss_reloc = mpss_reloc; > /* Load firmware segments */ > @@ -1016,10 +1033,24 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > phdr->p_memsz - phdr->p_filesz); > } > size += phdr->p_memsz; > + > + code_length = readl(qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > + if (!code_length) { > + boot_addr = relocate ? qproc->mpss_phys : min_addr; > + writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); > + writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); > + } > + writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > + > + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG); > + if (ret < 0) { > + dev_err(qproc->dev, "MPSS authentication failed: %d\n", ret); > + goto release_firmware; > + } > } > > /* Transfer ownership of modem ddr region to q6 */ > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, true, > qproc->mpss_phys, qproc- >mpss_size); > if (ret) { > dev_err(qproc->dev, > @@ -1028,11 +1059,6 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > goto release_firmware; > } > > - boot_addr = relocate ? qproc->mpss_phys : min_addr; > - writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); > - writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); > - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > - > ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000); > if (ret == -ETIMEDOUT) > dev_err(qproc->dev, "MPSS authentication timed out\n"); > @@ -1061,7 +1087,7 @@ static void qcom_q6v5_dump_segment(struct rproc > *rproc, ret = q6v5_mba_load(qproc); > > /* Try to reset ownership back to Linux */ > - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false, > qproc->mpss_phys, qproc->mpss_size); > } > > @@ -1101,8 +1127,8 @@ static int q6v5_start(struct rproc *rproc) > goto reclaim_mpss; > } > > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, > - qproc- >mba_phys, > + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, > + false, qproc->mba_phys, > qproc- >mba_size); > if (xfermemop_ret) > dev_err(qproc->dev, [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading 2019-11-10 14:05 ` Luca Weiss @ 2019-11-11 6:45 ` Bjorn Andersson 0 siblings, 0 replies; 7+ messages in thread From: Bjorn Andersson @ 2019-11-11 6:45 UTC (permalink / raw) To: Luca Weiss Cc: linux-arm-msm, Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, Brian Masney On Sun 10 Nov 06:05 PST 2019, Luca Weiss wrote: > Hi Bjorn, > > with your patches and modifications in qcom-msm8974.dtsi, I can boot the modem > successfully on the Fairphone 2, without the 'hack' commit we had in the tree > before! Thanks! > > Tested-by: Luca Weiss <luca@z3ntu.xyz> > Thanks for reminding me about this being an issue in 8974, had forgotten about that.I'll slap a Fixes on it once I'm applying it. Regards, Bjorn > On Samstag, 9. November 2019 01:40:33 CET Bjorn Andersson wrote: > > The code used to sync with the MBA after each segment loaded and this is > > still what's done downstream. So reduce the delta towards downstream by > > switching to a model where the content is iteratively validated. > > > > Reviewed-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > Tested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org> > > --- > > > > Changes since v1: > > - Picked up Jeff's r-b and t-b > > > > drivers/remoteproc/qcom_q6v5_mss.c | 76 ++++++++++++++++++++---------- > > 1 file changed, 51 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c > > b/drivers/remoteproc/qcom_q6v5_mss.c index efab574b2e12..914d5546e1cf > > 100644 > > --- a/drivers/remoteproc/qcom_q6v5_mss.c > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > > @@ -358,23 +358,29 @@ static void q6v5_pds_disable(struct q6v5 *qproc, > > struct device **pds, } > > > > static int q6v5_xfer_mem_ownership(struct q6v5 *qproc, int *current_perm, > > - bool remote_owner, phys_addr_t > addr, > > + bool local, bool remote, > phys_addr_t addr, > > size_t size) > > { > > - struct qcom_scm_vmperm next; > > + struct qcom_scm_vmperm next[2]; > > + int perms = 0; > > > > if (!qproc->need_mem_protection) > > return 0; > > - if (remote_owner && *current_perm == BIT(QCOM_SCM_VMID_MSS_MSA)) > > - return 0; > > - if (!remote_owner && *current_perm == BIT(QCOM_SCM_VMID_HLOS)) > > - return 0; > > > > - next.vmid = remote_owner ? QCOM_SCM_VMID_MSS_MSA : > QCOM_SCM_VMID_HLOS; > > - next.perm = remote_owner ? QCOM_SCM_PERM_RW : QCOM_SCM_PERM_RWX; > > + if (local) { > > + next[perms].vmid = QCOM_SCM_VMID_HLOS; > > + next[perms].perm = QCOM_SCM_PERM_RWX; > > + perms++; > > + } > > + > > + if (remote) { > > + next[perms].vmid = QCOM_SCM_VMID_MSS_MSA; > > + next[perms].perm = QCOM_SCM_PERM_RW; > > + perms++; > > + } > > > > return qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), > > - current_perm, &next, 1); > > + current_perm, next, perms); > > } > > > > static int q6v5_load(struct rproc *rproc, const struct firmware *fw) > > @@ -681,7 +687,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, > > const struct firmware *fw) > > > > /* Hypervisor mapping to access metadata by modem */ > > mdata_perm = BIT(QCOM_SCM_VMID_HLOS); > > - ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, phys, > size); > > + ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true, > phys, > > size); if (ret) { > > dev_err(qproc->dev, > > "assigning Q6 access to metadata failed: > %d\n", ret); > > @@ -699,7 +705,7 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, > > const struct firmware *fw) dev_err(qproc->dev, "MPSS header authentication > > failed: %d\n", ret); > > > > /* Metadata authentication done, remove modem access */ > > - xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, > phys, > > size); + xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, > > false, phys, size); if (xferop_ret) > > dev_warn(qproc->dev, > > "mdt buffer not reclaimed system may become > unstable\n"); > > @@ -786,7 +792,7 @@ static int q6v5_mba_load(struct q6v5 *qproc) > > } > > > > /* Assign MBA image access in DDR to q6 */ > > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, > > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, true, > > qproc->mba_phys, qproc- > >mba_size); > > if (ret) { > > dev_err(qproc->dev, > > @@ -820,8 +826,8 @@ static int q6v5_mba_load(struct q6v5 *qproc) > > q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); > > > > reclaim_mba: > > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, > false, > > - qproc- > >mba_phys, > > + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, > true, > > + false, > qproc->mba_phys, > > qproc- > >mba_size); > > if (xfermemop_ret) { > > dev_err(qproc->dev, > > @@ -888,7 +894,7 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc) > > /* In case of failure or coredump scenario where reclaiming MBA > memory > > * could not happen reclaim it here. > > */ > > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, false, > > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, true, false, > > qproc->mba_phys, > > qproc->mba_size); > > WARN_ON(ret); > > @@ -915,6 +921,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > phys_addr_t boot_addr; > > phys_addr_t min_addr = PHYS_ADDR_MAX; > > phys_addr_t max_addr = 0; > > + u32 code_length; > > bool relocate = false; > > char *fw_name; > > size_t fw_name_len; > > @@ -965,9 +972,19 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > } > > > > /* Try to reset ownership back to Linux */ > > - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, false, > > qproc->mpss_phys, qproc- > >mpss_size); > > > > + /* Share ownership between Linux and MSS, during segment loading */ > > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, true, > > + qproc->mpss_phys, qproc- > >mpss_size); > > + if (ret) { > > + dev_err(qproc->dev, > > + "assigning Q6 access to mpss memory failed: > %d\n", ret); > > + ret = -EAGAIN; > > + goto release_firmware; > > + } > > + > > mpss_reloc = relocate ? min_addr : qproc->mpss_phys; > > qproc->mpss_reloc = mpss_reloc; > > /* Load firmware segments */ > > @@ -1016,10 +1033,24 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > phdr->p_memsz - phdr->p_filesz); > > } > > size += phdr->p_memsz; > > + > > + code_length = readl(qproc->rmb_base + > RMB_PMI_CODE_LENGTH_REG); > > + if (!code_length) { > > + boot_addr = relocate ? qproc->mpss_phys : > min_addr; > > + writel(boot_addr, qproc->rmb_base + > RMB_PMI_CODE_START_REG); > > + writel(RMB_CMD_LOAD_READY, qproc->rmb_base + > RMB_MBA_COMMAND_REG); > > + } > > + writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > > + > > + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG); > > + if (ret < 0) { > > + dev_err(qproc->dev, "MPSS authentication > failed: %d\n", ret); > > + goto release_firmware; > > + } > > } > > > > /* Transfer ownership of modem ddr region to q6 */ > > - ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, > > + ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, false, > true, > > qproc->mpss_phys, qproc- > >mpss_size); > > if (ret) { > > dev_err(qproc->dev, > > @@ -1028,11 +1059,6 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > goto release_firmware; > > } > > > > - boot_addr = relocate ? qproc->mpss_phys : min_addr; > > - writel(boot_addr, qproc->rmb_base + RMB_PMI_CODE_START_REG); > > - writel(RMB_CMD_LOAD_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); > > - writel(size, qproc->rmb_base + RMB_PMI_CODE_LENGTH_REG); > > - > > ret = q6v5_rmb_mba_wait(qproc, RMB_MBA_AUTH_COMPLETE, 10000); > > if (ret == -ETIMEDOUT) > > dev_err(qproc->dev, "MPSS authentication timed out\n"); > > @@ -1061,7 +1087,7 @@ static void qcom_q6v5_dump_segment(struct rproc > > *rproc, ret = q6v5_mba_load(qproc); > > > > /* Try to reset ownership back to Linux */ > > - q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, > false, > > + q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm, true, > false, > > qproc->mpss_phys, > qproc->mpss_size); > > } > > > > @@ -1101,8 +1127,8 @@ static int q6v5_start(struct rproc *rproc) > > goto reclaim_mpss; > > } > > > > - xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, > false, > > - qproc- > >mba_phys, > > + xfermemop_ret = q6v5_xfer_mem_ownership(qproc, &qproc->mba_perm, > true, > > + false, > qproc->mba_phys, > > qproc- > >mba_size); > > if (xfermemop_ret) > > dev_err(qproc->dev, > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-11-13 18:03 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-11-09 0:40 [PATCH v2 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson 2019-11-09 0:40 ` [PATCH v2 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson 2019-11-12 17:01 ` Jeffrey Hugo 2019-11-13 18:02 ` Jeffrey Hugo 2019-11-09 0:40 ` [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 2019-11-10 14:05 ` Luca Weiss 2019-11-11 6:45 ` Bjorn Andersson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).