[PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load

[PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load

[Bjorn Andersson]

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 | 95 +++++++++++++++++++-----------
 1 file changed, 62 insertions(+), 33 deletions(-)

-- 
2.23.0

[PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown

[Bjorn Andersson]

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 v2:
- The assignment of mpss memory back to Linux is rejected in the coredump case
  on production devices, so check the return value of q6v5_xfer_mem_ownership()
  before attempting to memcpy() the data.

 drivers/remoteproc/qcom_q6v5_mss.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 471128a2e723..25c03a26bf88 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -887,11 +887,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,
@@ -981,6 +976,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 */
@@ -1070,8 +1069,16 @@ 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);
+		if (!ret) {
+			/* Try to reset ownership back to Linux */
+			ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
+						      false,
+						      qproc->mpss_phys,
+						      qproc->mpss_size);
+		}
+	}
 
 	if (!ptr || ret)
 		memset(dest, 0xff, segment->size);
@@ -1123,10 +1130,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

[PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading

[Bjorn Andersson]

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>
Tested-by: Luca Weiss <luca@z3ntu.xyz>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- Picked up Luca's 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 25c03a26bf88..5eb154f221a4 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -360,23 +360,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)
@@ -693,7 +699,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);
@@ -711,7 +717,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");
@@ -798,7 +804,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,
@@ -832,8 +838,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,
@@ -900,7 +906,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);
@@ -927,6 +933,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;
@@ -977,9 +984,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 */
@@ -1028,10 +1045,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,
@@ -1040,11 +1071,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");
@@ -1074,7 +1100,7 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc,
 		if (!ret) {
 			/* Try to reset ownership back to Linux */
 			ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
-						      false,
+						      true, false,
 						      qproc->mpss_phys,
 						      qproc->mpss_size);
 		}
@@ -1116,8 +1142,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

Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown

[Mathieu Poirier]

Hi Bjorn,

On Mon, Feb 03, 2020 at 10:26:40PM -0800, Bjorn Andersson 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>
> ---
> 
> Changes since v2:
> - The assignment of mpss memory back to Linux is rejected in the coredump case
>   on production devices, so check the return value of q6v5_xfer_mem_ownership()
>   before attempting to memcpy() the data.
> 
>  drivers/remoteproc/qcom_q6v5_mss.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 471128a2e723..25c03a26bf88 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -887,11 +887,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,
> @@ -981,6 +976,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);

Would you mind adding more chatter here, something like what is mentioned in the
changelog?  That way I anyone trying to review this code doesn't have to suffer
through the same mental gymnastic. 

> +
>  	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
>  	qproc->mpss_reloc = mpss_reloc;
>  	/* Load firmware segments */
> @@ -1070,8 +1069,16 @@ 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);
> +		if (!ret) {
> +			/* Try to reset ownership back to Linux */
> +			ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
> +						      false,
> +						      qproc->mpss_phys,
> +						      qproc->mpss_size);
> +		}

I'm a bit puzzled here as to why a different reclaim strategy is needed.  It is
clear to me that if q6v5_mba_load() returns 0 then the MBA is running and we can
safely reclaim ownership of the memory.  But is it absolutely needed when we
know that 1) the MCU has crashed and 2) said memory will be reclaimed in
q6v5_mpss_load()?

If so I think an explanation in the code is needed.

I also assume there is no way to know if the mba is running, hence not taking
any chance.  If that's the case it would be nice to add that to the comment in
q6v5_mpss_load().

Thanks,
Mathieu

> +	}
>  
>  	if (!ptr || ret)
>  		memset(dest, 0xff, segment->size);
> @@ -1123,10 +1130,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
> 

Re: [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading

[Mathieu Poirier]

On Mon, Feb 03, 2020 at 10:26:41PM -0800, 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>
> Tested-by: Luca Weiss <luca@z3ntu.xyz>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Enough people have looked at this and I won't add my two-bits other then
checkpatch is complaining about lines over 80 characters.  In this case they are
easy to fix and have no side effects.

Mathieu

> ---
> 
> Changes since v2:
> - Picked up Luca's 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 25c03a26bf88..5eb154f221a4 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -360,23 +360,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)
> @@ -693,7 +699,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);
> @@ -711,7 +717,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");
> @@ -798,7 +804,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,
> @@ -832,8 +838,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,
> @@ -900,7 +906,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);
> @@ -927,6 +933,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;
> @@ -977,9 +984,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 */
> @@ -1028,10 +1045,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,
> @@ -1040,11 +1071,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");
> @@ -1074,7 +1100,7 @@ static void qcom_q6v5_dump_segment(struct rproc *rproc,
>  		if (!ret) {
>  			/* Try to reset ownership back to Linux */
>  			ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
> -						      false,
> +						      true, false,
>  						      qproc->mpss_phys,
>  						      qproc->mpss_size);
>  		}
> @@ -1116,8 +1142,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
> 

Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown

[Bjorn Andersson]

On Mon 10 Feb 15:05 PST 2020, Mathieu Poirier wrote:

> Hi Bjorn,
> 
> On Mon, Feb 03, 2020 at 10:26:40PM -0800, Bjorn Andersson 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>
> > ---
> > 
> > Changes since v2:
> > - The assignment of mpss memory back to Linux is rejected in the coredump case
> >   on production devices, so check the return value of q6v5_xfer_mem_ownership()
> >   before attempting to memcpy() the data.
> > 
> >  drivers/remoteproc/qcom_q6v5_mss.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > index 471128a2e723..25c03a26bf88 100644
> > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > @@ -887,11 +887,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,
> > @@ -981,6 +976,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);
> 
> Would you mind adding more chatter here, something like what is mentioned in the
> changelog?  That way I anyone trying to review this code doesn't have to suffer
> through the same mental gymnastic. 
> 

Sure thing, as this patch shows this dynamic wasn't clear - and this
patch is based on my observations. With it we no longer crash the entire
system by hitting a security violation during a crash, but there's still
some details that I'm uncertain about.

> > +
> >  	mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> >  	qproc->mpss_reloc = mpss_reloc;
> >  	/* Load firmware segments */
> > @@ -1070,8 +1069,16 @@ 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);
> > +		if (!ret) {
> > +			/* Try to reset ownership back to Linux */
> > +			ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
> > +						      false,
> > +						      qproc->mpss_phys,
> > +						      qproc->mpss_size);
> > +		}
> 
> I'm a bit puzzled here as to why a different reclaim strategy is needed.  It is
> clear to me that if q6v5_mba_load() returns 0 then the MBA is running and we can
> safely reclaim ownership of the memory.  But is it absolutely needed when we
> know that 1) the MCU has crashed and 2) said memory will be reclaimed in
> q6v5_mpss_load()?
> 

The ownership transfer here is a jump into secure world, which somehow
together with the firmware running on the modem processor will update
the access permissions for the mpss memory region.

As we enter this function the recovery handling in the core has just
stopped the remote processor, so we know it's off. As such we must first
boot the remote processor again, in order to reclaim the access to the
mpss memory region.

New in this revision is the fact that this operation might actually be
rejected (e.g. on end-user hardware).

So we need to guard the memcpy below on either of these cases, as unless
we've successfully booted the modem processor and gotten permission to
read the mpss memory this operation will trigger a security violation
and the device will reboot.

> If so I think an explanation in the code is needed.
> 

Makes sense, I will formulate above explanation into a comment. As well
as review the other callers of q6v5_xfer_mem_ownership().

> I also assume there is no way to know if the mba is running, hence not taking
> any chance.  If that's the case it would be nice to add that to the comment in
> q6v5_mpss_load().
> 

We know that we enter q6v5_mpss_load() with the modem processor just
booted, but the memory assignment is there to handle the case where the
mpss memory region for some reason was left in the hands on the modem.
I will have to do some more digging to figure out if this is a valid
scenario or not.

Thanks for your review Mathieu!

Regards,
Bjorn

> Thanks,
> Mathieu
> 
> > +	}
> >  
> >  	if (!ptr || ret)
> >  		memset(dest, 0xff, segment->size);
> > @@ -1123,10 +1130,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
> > 

Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown

[Mathieu Poirier]

On Mon, 10 Feb 2020 at 18:16, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 10 Feb 15:05 PST 2020, Mathieu Poirier wrote:
>
> > Hi Bjorn,
> >
> > On Mon, Feb 03, 2020 at 10:26:40PM -0800, Bjorn Andersson 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>
> > > ---
> > >
> > > Changes since v2:
> > > - The assignment of mpss memory back to Linux is rejected in the coredump case
> > >   on production devices, so check the return value of q6v5_xfer_mem_ownership()
> > >   before attempting to memcpy() the data.
> > >
> > >  drivers/remoteproc/qcom_q6v5_mss.c | 23 +++++++++++++----------
> > >  1 file changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index 471128a2e723..25c03a26bf88 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -887,11 +887,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,
> > > @@ -981,6 +976,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);
> >
> > Would you mind adding more chatter here, something like what is mentioned in the
> > changelog?  That way I anyone trying to review this code doesn't have to suffer
> > through the same mental gymnastic.
> >
>
> Sure thing, as this patch shows this dynamic wasn't clear - and this
> patch is based on my observations. With it we no longer crash the entire
> system by hitting a security violation during a crash, but there's still
> some details that I'm uncertain about.
>
> > > +
> > >     mpss_reloc = relocate ? min_addr : qproc->mpss_phys;
> > >     qproc->mpss_reloc = mpss_reloc;
> > >     /* Load firmware segments */
> > > @@ -1070,8 +1069,16 @@ 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);
> > > +           if (!ret) {
> > > +                   /* Try to reset ownership back to Linux */
> > > +                   ret = q6v5_xfer_mem_ownership(qproc, &qproc->mpss_perm,
> > > +                                                 false,
> > > +                                                 qproc->mpss_phys,
> > > +                                                 qproc->mpss_size);
> > > +           }
> >
> > I'm a bit puzzled here as to why a different reclaim strategy is needed.  It is
> > clear to me that if q6v5_mba_load() returns 0 then the MBA is running and we can
> > safely reclaim ownership of the memory.  But is it absolutely needed when we
> > know that 1) the MCU has crashed and 2) said memory will be reclaimed in
> > q6v5_mpss_load()?
> >
>
> The ownership transfer here is a jump into secure world, which somehow
> together with the firmware running on the modem processor will update
> the access permissions for the mpss memory region.
>
> As we enter this function the recovery handling in the core has just
> stopped the remote processor, so we know it's off. As such we must first
> boot the remote processor again, in order to reclaim the access to the
> mpss memory region.
>
> New in this revision is the fact that this operation might actually be
> rejected (e.g. on end-user hardware).
>
> So we need to guard the memcpy below on either of these cases, as unless
> we've successfully booted the modem processor and gotten permission to
> read the mpss memory this operation will trigger a security violation
> and the device will reboot.
>
> > If so I think an explanation in the code is needed.
> >
>
> Makes sense, I will formulate above explanation into a comment. As well
> as review the other callers of q6v5_xfer_mem_ownership().
>
> > I also assume there is no way to know if the mba is running, hence not taking
> > any chance.  If that's the case it would be nice to add that to the comment in
> > q6v5_mpss_load().
> >
>
> We know that we enter q6v5_mpss_load() with the modem processor just
> booted, but the memory assignment is there to handle the case where the
> mpss memory region for some reason was left in the hands on the modem.
> I will have to do some more digging to figure out if this is a valid
> scenario or not.

I'm really happy that you're also not sure about this patch... I spent
hours (no joke) trying to figure out the workflow and logic of using
q6v5_xfer_mem_ownership() and even then I'm ambivalent...  Carefully
understanding and documenting the scenarios we trying to handle will
go a long way in terms of future stability of the system.

>
> Thanks for your review Mathieu!
>
> Regards,
> Bjorn
>
> > Thanks,
> > Mathieu
> >
> > > +   }
> > >
> > >     if (!ptr || ret)
> > >             memset(dest, 0xff, segment->size);
> > > @@ -1123,10 +1130,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
> > >