* [PATCH 0/2] remoteproc: mss: Improve memory assignment
@ 2019-10-31 18:46 Bjorn Andersson
2019-10-31 18:46 ` [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson
2019-10-31 18:46 ` [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
0 siblings, 2 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-31 18:46 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: Avaneesh Kumar Dwivedi, linux-arm-msm, linux-remoteproc,
linux-kernel, Jeffrey Hugo, Sibi Sankar
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 | 85 +++++++++++++++++++-----------
1 file changed, 53 insertions(+), 32 deletions(-)
--
2.23.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown
2019-10-31 18:46 [PATCH 0/2] remoteproc: mss: Improve memory assignment Bjorn Andersson
@ 2019-10-31 18:46 ` Bjorn Andersson
2019-10-31 19:36 ` Jeffrey Hugo
2019-10-31 18:46 ` [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
1 sibling, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-31 18:46 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: Avaneesh Kumar Dwivedi, linux-arm-msm, linux-remoteproc,
linux-kernel, Jeffrey Hugo, Sibi Sankar, 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>
---
drivers/remoteproc/qcom_q6v5_mss.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index de919f2e8b94..f4a02105d539 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 */
@@ -1111,10 +1110,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] 10+ messages in thread
* [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
2019-10-31 18:46 [PATCH 0/2] remoteproc: mss: Improve memory assignment Bjorn Andersson
2019-10-31 18:46 ` [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson
@ 2019-10-31 18:46 ` Bjorn Andersson
2019-10-31 19:13 ` Jeffrey Hugo
2019-10-31 22:03 ` Jeffrey Hugo
1 sibling, 2 replies; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-31 18:46 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson
Cc: Avaneesh Kumar Dwivedi, linux-arm-msm, linux-remoteproc,
linux-kernel, Jeffrey Hugo, Sibi Sankar
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.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/remoteproc/qcom_q6v5_mss.c | 74 ++++++++++++++++++++----------
1 file changed, 50 insertions(+), 24 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index f4a02105d539..bdf1dd00b89b 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");
@@ -1096,8 +1122,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] 10+ messages in thread
* Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
2019-10-31 18:46 ` [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
@ 2019-10-31 19:13 ` Jeffrey Hugo
2019-10-31 19:41 ` Bjorn Andersson
2019-10-31 22:03 ` Jeffrey Hugo
1 sibling, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2019-10-31 19:13 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar
On Thu, Oct 31, 2019 at 12:48 PM Bjorn Andersson
<bjorn.andersson@linaro.org> 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/remoteproc/qcom_q6v5_mss.c | 74 ++++++++++++++++++++----------
> 1 file changed, 50 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index f4a02105d539..bdf1dd00b89b 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;
So, does this need to be tristate? Downstream makes the HLOS perms
RWX only when HLOS is the sole owner. HLOS has RW perms when it
shares ownership with the remote (modem).
> + 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);
Should there be an explicit wmb() here since presumably this write
needs to be flushed out before we do the following readl?
> + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG);
Is this status reg immediately updated when RMB_PMI_CODE_LENGTH_REG is
updated? I would expect that the MBA may need some time to update
this reg, thus there is an implicit race here.
> + 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");
> @@ -1096,8 +1122,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 [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown
2019-10-31 18:46 ` [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson
@ 2019-10-31 19:36 ` Jeffrey Hugo
2019-10-31 19:43 ` Bjorn Andersson
0 siblings, 1 reply; 10+ messages in thread
From: Jeffrey Hugo @ 2019-10-31 19:36 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar, stable
On Thu, Oct 31, 2019 at 12:48 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>
> ---
Excellent. This addresses the issue I was seeing with the Lenovo Miix 630
Reviewed-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
Tested-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
2019-10-31 19:13 ` Jeffrey Hugo
@ 2019-10-31 19:41 ` Bjorn Andersson
2019-10-31 19:51 ` Jeffrey Hugo
0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-31 19:41 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar
On Thu 31 Oct 12:13 PDT 2019, Jeffrey Hugo wrote:
> On Thu, Oct 31, 2019 at 12:48 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> 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.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > drivers/remoteproc/qcom_q6v5_mss.c | 74 ++++++++++++++++++++----------
> > 1 file changed, 50 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index f4a02105d539..bdf1dd00b89b 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;
>
> So, does this need to be tristate? Downstream makes the HLOS perms
> RWX only when HLOS is the sole owner. HLOS has RW perms when it
> shares ownership with the remote (modem).
>
I've not seen any issues with retaining the X in my testing so far, and
it does make the code cleaner.
> > + 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);
>
> Should there be an explicit wmb() here since presumably this write
> needs to be flushed out before we do the following readl?
>
Doesn't the readl of an adjacent register wait for the write to hit the
hardware before returning its value?
> > + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG);
>
> Is this status reg immediately updated when RMB_PMI_CODE_LENGTH_REG is
> updated? I would expect that the MBA may need some time to update
> this reg, thus there is an implicit race here.
>
I don't see any waiting going on downstream and afaict there wouldn't be
any harm in not waiting for the status register to go negative, as
issues will either be caught on the next iteration or when we're
checking the final status.
Presumably though there could be an issue if this somehow depended on us
not loading the next chunk until this has "stabilized".
But afaict, this is how downstream does it...
Regards,
Bjorn
> > + 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");
> > @@ -1096,8 +1122,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 [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown
2019-10-31 19:36 ` Jeffrey Hugo
@ 2019-10-31 19:43 ` Bjorn Andersson
2019-10-31 22:02 ` Jeffrey Hugo
0 siblings, 1 reply; 10+ messages in thread
From: Bjorn Andersson @ 2019-10-31 19:43 UTC (permalink / raw)
To: Jeffrey Hugo
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar, stable
On Thu 31 Oct 12:36 PDT 2019, Jeffrey Hugo wrote:
> On Thu, Oct 31, 2019 at 12:48 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>
> > ---
>
> Excellent. This addresses the issue I was seeing with the Lenovo Miix 630
>
Sweet!
> Reviewed-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
> Tested-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
Thanks!
Regards,
Bjorn
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
2019-10-31 19:41 ` Bjorn Andersson
@ 2019-10-31 19:51 ` Jeffrey Hugo
0 siblings, 0 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2019-10-31 19:51 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar
On Thu, Oct 31, 2019 at 1:41 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 31 Oct 12:13 PDT 2019, Jeffrey Hugo wrote:
>
> > On Thu, Oct 31, 2019 at 12:48 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> 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.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > > drivers/remoteproc/qcom_q6v5_mss.c | 74 ++++++++++++++++++++----------
> > > 1 file changed, 50 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index f4a02105d539..bdf1dd00b89b 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;
> >
> > So, does this need to be tristate? Downstream makes the HLOS perms
> > RWX only when HLOS is the sole owner. HLOS has RW perms when it
> > shares ownership with the remote (modem).
> >
>
> I've not seen any issues with retaining the X in my testing so far, and
> it does make the code cleaner.
Fair enough. I was just wondering how true to downstream you wanted to be.
>
> > > + 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);
> >
> > Should there be an explicit wmb() here since presumably this write
> > needs to be flushed out before we do the following readl?
> >
>
> Doesn't the readl of an adjacent register wait for the write to hit the
> hardware before returning its value?
Depends on the micro architecture. I forget what early chip it was,
(somewhere in A family perhaps?) where things changed so that ordering
like this would be guaranteed within a 4k page, but the CPU was free
to reorder accesses between pages (even if you just happened to split
the page boundary), which ended up causing a lot of issues. The
previous chip guaranteed such ordering within an entire MMU mapping.
As far as I am aware you can only depend the CPU executing accesses in
the same order if its the same address.
>
> > > + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG);
> >
> > Is this status reg immediately updated when RMB_PMI_CODE_LENGTH_REG is
> > updated? I would expect that the MBA may need some time to update
> > this reg, thus there is an implicit race here.
> >
>
> I don't see any waiting going on downstream and afaict there wouldn't be
> any harm in not waiting for the status register to go negative, as
> issues will either be caught on the next iteration or when we're
> checking the final status.
>
> Presumably though there could be an issue if this somehow depended on us
> not loading the next chunk until this has "stabilized".
>
> But afaict, this is how downstream does it...
Upon second look, I actually think this will be fine.
>
> Regards,
> Bjorn
>
> > > + 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");
> > > @@ -1096,8 +1122,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 [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown
2019-10-31 19:43 ` Bjorn Andersson
@ 2019-10-31 22:02 ` Jeffrey Hugo
0 siblings, 0 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2019-10-31 22:02 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar, stable
On Thu, Oct 31, 2019 at 1:43 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 31 Oct 12:36 PDT 2019, Jeffrey Hugo wrote:
>
> > On Thu, Oct 31, 2019 at 12:48 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>
> > > ---
> >
> > Excellent. This addresses the issue I was seeing with the Lenovo Miix 630
> >
>
> Sweet!
>
> > Reviewed-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
> > Tested-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
>
> Thanks!
As we talked offline, it appears we both missed the crashdump
scenario, so please spin a v2 that reclaims the memory just before
trying to access it in the crashdump scenario. I'll be happy to
re-review and re-test.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading
2019-10-31 18:46 ` [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
2019-10-31 19:13 ` Jeffrey Hugo
@ 2019-10-31 22:03 ` Jeffrey Hugo
1 sibling, 0 replies; 10+ messages in thread
From: Jeffrey Hugo @ 2019-10-31 22:03 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Avaneesh Kumar Dwivedi, MSM, linux-remoteproc,
lkml, Jeffrey Hugo, Sibi Sankar
On Thu, Oct 31, 2019 at 12:48 PM Bjorn Andersson
<bjorn.andersson@linaro.org> 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.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Seems to be just fine on the Lenovo Miix 630
Reviewed-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
Tested-by: Jeffrey Hugo<jeffrey.l.hugo@gmail.com>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-10-31 22:03 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-31 18:46 [PATCH 0/2] remoteproc: mss: Improve memory assignment Bjorn Andersson
2019-10-31 18:46 ` [PATCH 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson
2019-10-31 19:36 ` Jeffrey Hugo
2019-10-31 19:43 ` Bjorn Andersson
2019-10-31 22:02 ` Jeffrey Hugo
2019-10-31 18:46 ` [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson
2019-10-31 19:13 ` Jeffrey Hugo
2019-10-31 19:41 ` Bjorn Andersson
2019-10-31 19:51 ` Jeffrey Hugo
2019-10-31 22:03 ` Jeffrey Hugo
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).