Hey Mani, On 1/9/23 14:02, Manivannan Sadhasivam wrote:
On Mon, Jan 09, 2023 at 09:18:38AM +0530, Sibi Sankar wrote:Any access to the dynamically allocated metadata region by the application processor after assigning it to the remote Q6 will result in a XPU violation. Fix this by replacing the dynamically allocated memory region with a no-map carveout and unmap the modem metadata memory region before passing control to the remote Q6. Reported-and-tested-by: Amit Pundir <amit.pundir@xxxxxxxxxx> Fixes: 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch") Signed-off-by: Sibi Sankar <quic_sibis@xxxxxxxxxxx> --- v2: * Revert no_kernel_mapping [Mani/Robin] drivers/remoteproc/qcom_q6v5_mss.c | 48 ++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index e2f765f87ec9..b7a158751cef 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -215,6 +215,7 @@ struct q6v5 { size_t mba_size; size_t dp_size;+ phys_addr_t mdata_phys;phys_addr_t mpss_phys; phys_addr_t mpss_reloc; size_t mpss_size; @@ -973,15 +974,29 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, if (IS_ERR(metadata)) return PTR_ERR(metadata);- ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs);- if (!ptr) { - kfree(metadata); - dev_err(qproc->dev, "failed to allocate mdt buffer\n"); - return -ENOMEM; + if (qproc->mdata_phys) { + phys = qproc->mdata_phys; + ptr = memremap(qproc->mdata_phys, size, MEMREMAP_WC); + if (!ptr) { + dev_err(qproc->dev, "unable to map memory region: %pa+%zx\n", + &qproc->mdata_phys, size); + ret = -EBUSY; + goto free_dma_attrs;There is no memory to free at this point.
we would just free the metadata in the no-map carveout scenario since mdata_phys wouldn't be NULL. I can do a kfree(metadata) directly from this branch and return as well if you think it makes things more readable.
Thanks, Mani+ } + } else { + ptr = dma_alloc_attrs(qproc->dev, size, &phys, GFP_KERNEL, dma_attrs); + if (!ptr) { + kfree(metadata); + dev_err(qproc->dev, "failed to allocate mdt buffer\n"); + return -ENOMEM; + } }memcpy(ptr, metadata, size); + if (qproc->mdata_phys)+ memunmap(ptr); + /* Hypervisor mapping to access metadata by modem */ mdata_perm = BIT(QCOM_SCM_VMID_HLOS); ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, false, true, @@ -1010,7 +1025,8 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw, "mdt buffer not reclaimed system may become unstable\n");free_dma_attrs:- dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); + if (!qproc->mdata_phys) + dma_free_attrs(qproc->dev, size, ptr, phys, dma_attrs); kfree(metadata);return ret < 0 ? ret : 0;@@ -1893,6 +1909,26 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc) qproc->mpss_phys = qproc->mpss_reloc = r.start; qproc->mpss_size = resource_size(&r);+ if (!child) {+ node = of_parse_phandle(qproc->dev->of_node, "memory-region", 2); + } else { + child = of_get_child_by_name(qproc->dev->of_node, "metadata"); + node = of_parse_phandle(child, "memory-region", 0); + of_node_put(child); + } + + if (!node) + return 0; + + ret = of_address_to_resource(node, 0, &r); + of_node_put(node); + if (ret) { + dev_err(qproc->dev, "unable to resolve metadata region\n"); + return ret; + } + + qproc->mdata_phys = r.start; + return 0; }--2.17.1
