Messages in this thread | | | Date | Thu, 8 Sep 2022 18:29:18 +0530 | Subject | Re: [RESEND v5 6/7] remoteproc: qcom: Add support for memory sandbox | From | Srinivasa Rao Mandadapu <> |
| |
On 8/25/2022 3:34 PM, Srinivasa Rao Mandadapu wrote: > > On 8/23/2022 8:55 AM, Stephen Boyd wrote: > Thanks for your time and valuable suggestions!!! >> Quoting Srinivasa Rao Mandadapu (2022-08-22 01:22:02) >>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c >>> b/drivers/remoteproc/qcom_q6v5_adsp.c >>> index b0a63a0..d01c97e 100644 >>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c >>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c >>> @@ -333,6 +336,185 @@ static int adsp_load(struct rproc *rproc, >>> const struct firmware *fw) >>> return 0; >>> } >>> >>> +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, >>> const __be32 *prop, int len) >>> +{ >>> + unsigned long iova; >>> + unsigned int mem_size; >>> + int i; >>> + >>> + len /= sizeof(__be32); >>> + for (i = 0; i < len; i++) { >>> + iova = be32_to_cpu(prop[i++]); >>> + /* Skip Physical address*/ >>> + i++; >>> + mem_size = be32_to_cpu(prop[i++]); >>> + iommu_unmap(iommu_dom, iova, mem_size); >>> + } >>> +} >>> + >>> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len) >>> +{ >>> + struct fw_rsc_devmem *rsc_fw; >>> + struct fw_rsc_hdr *hdr; >>> + int offset; >>> + int i; >>> + >>> + for (i = 0; i < len; i++) { >>> + offset = rproc->table_ptr->offset[i]; >>> + hdr = (void *)rproc->table_ptr + offset; >>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr); >>> + >>> + iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len); >>> + } >>> +} >>> + >>> +static void adsp_unmap_smmu(struct rproc *rproc) >>> +{ >>> + struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; >>> + const __be32 *prop; >>> + unsigned int len; >>> + >>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size); >>> + >>> + prop = of_get_property(adsp->dev->of_node, >>> "qcom,adsp-memory-regions", &len); >>> + if (prop) { >>> + adsp_of_unmap_smmu(adsp->iommu_dom, prop, len); >>> + } else { >>> + if (rproc->table_ptr) >>> + adsp_rproc_unmap_smmu(rproc, >>> rproc->table_ptr->num); >>> + } >>> + >>> + iommu_domain_free(adsp->iommu_dom); >>> +} >>> + >>> +static int adsp_of_map_smmu(struct iommu_domain *iommu_dom, const >>> __be32 *prop, int len) >>> +{ >>> + unsigned long mem_phys; >>> + unsigned long iova; >>> + unsigned int mem_size; >>> + unsigned int flag; >>> + int access_level; >>> + int ret; >>> + int i; >>> + >>> + len /= sizeof(__be32); >>> + for (i = 0; i < len; i++) { >>> + iova = be32_to_cpu(prop[i++]); >>> + mem_phys = be32_to_cpu(prop[i++]); >>> + mem_size = be32_to_cpu(prop[i++]); >>> + access_level = be32_to_cpu(prop[i]); >>> + >>> + if (access_level) >>> + flag = IOMMU_READ | IOMMU_WRITE; >>> + else >>> + flag = IOMMU_READ; >>> + >>> + ret = iommu_map(iommu_dom, iova, mem_phys, mem_size, >>> flag); >>> + if (ret) { >>> + pr_err("failed to map addr = %p mem_size = >>> %x\n", &(mem_phys), mem_size); >> Why can't this be dev_err()? > Actually, dev pointer is not available here, hence used pr_err. >> >>> + goto of_smmu_unmap; >>> + } >>> + } >>> + return 0; >>> +of_smmu_unmap: >>> + adsp_of_unmap_smmu(iommu_dom, prop, i); >>> + return ret; >>> +} >>> + >>> +static int adsp_rproc_map_smmu(struct rproc *rproc, int len) >>> +{ >>> + struct fw_rsc_devmem *rsc_fw; >> const? > Okay. will update. >> >>> + struct fw_rsc_hdr *hdr; >> const? > Okay. Will update. >> >>> + int offset; >>> + int ret; >>> + int i; >>> + >>> + if (!rproc->table_ptr) >>> + return 0; >>> + >>> + for (i = 0; i < rproc->table_ptr->num; i++) { >>> + offset = rproc->table_ptr->offset[i]; >>> + hdr = (void *)rproc->table_ptr + offset; >>> + rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr); >>> + >>> + ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa, >>> + rsc_fw->len, rsc_fw->flags); >>> + if (ret) { >>> + pr_err("failed to map addr = %x mem_size = >>> %x\n", rsc_fw->pa, rsc_fw->len); >> Why can't this be dev_err()? > Okay. Will change it. >> >>> + goto rproc_smmu_unmap; >>> + } >>> + } >>> + >>> + return 0; >>> + >>> +rproc_smmu_unmap: >>> + adsp_rproc_unmap_smmu(rproc, i); >> Does i need to be incremented? And/or unmap should be done in reverse. > > Here i is the upper bound index in mapping failure case, hence it is > used as length. un-mapping is being done from starting till i value. > > Please correct me if I am missing some thing. > >> >>> + return ret; >>> +} >>> + >>> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc) >>> +{ >>> + struct of_phandle_args args; >>> + const __be32 *prop; >>> + long long sid; >>> + unsigned int len; >>> + int ret; >>> + >>> + ret = of_parse_phandle_with_args(adsp->dev->of_node, >>> "iommus", "#iommu-cells", 0, &args); >>> + if (ret < 0) >>> + sid = -1; >> Is it a good idea to set the sid to -1? Does that mean all stream IDs? > It seems, if sid is -1 iomap fails, because of alignment issues. Any I > will update with return in this case. >> >>> + else >>> + sid = args.args[0] & SID_MASK_DEFAULT; >>> + >>> + adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus); >>> + if (!adsp->iommu_dom) { >>> + dev_err(adsp->dev, "failed to allocate iommu >>> domain\n"); >>> + ret = -ENOMEM; >>> + goto domain_free; >>> + } >>> + >>> + ret = iommu_attach_device(adsp->iommu_dom, adsp->dev); >>> + if (ret) { >>> + dev_err(adsp->dev, "could not attach device ret = >>> %d\n", ret); >>> + ret = -EBUSY; >> Why do we overwrite the error value? > It seems not required. Will remove it. >> >>> + goto detach_device; >>> + } >>> + >>> + /* Add SID configuration for ADSP Firmware to SMMU */ >>> + adsp->mem_phys = adsp->mem_phys | (sid << 32); >>> + >>> + ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, >>> adsp->mem_phys, >>> + adsp->mem_size, IOMMU_READ | IOMMU_WRITE); >>> + if (ret) { >>> + dev_err(adsp->dev, "Unable to map ADSP Physical >>> Memory\n"); >>> + goto sid_unmap; >>> + } >>> + >>> + prop = of_get_property(adsp->dev->of_node, >>> "qcom,adsp-memory-regions", &len); >> I find it odd that we're encoding virtual addresses (iovas) into > Actually from HLOS perspective, same IOVA and physical memory is being > used. Hence will remove virtual address field in DT. >> devicetree. Presumably the physical address needs to be in DT as a >> carveout, but after that I would think we're free to allocate the > Will try to carveout the physical addresses and handle it accordingly. > If this method is ignored I don't think we need to mention in DT in > anyway. >> segments from the carveout however we see fit and then program that into >> the SMMU. Maybe DT can be a suggestion, but otherwise can it be ignored? > > Yes, it seems it can be ignored. As it is the approach we did for > bringing up AudioReach solution, and used when ADSP binary is without > metadata section header info. > > Will check internally and update accordingly. > > Your opinion also helps please!!. Is it okay to keep it as backup > option with proper comment, such that this method can be used > internally, with raw ADSP binary in early stage bring-up scenarios?
After internal discussions, decided to remove sandboxing using device tree and explicitly doing in PIL driver.
Instead, decided to use rproc's parse_fw call back "rproc_elf_load_rsc_table" function and achieve the sand boxing.
Will re post the patches with corresponding changes.
> >> >>> + if (prop) { >>> + ret = adsp_of_map_smmu(adsp->iommu_dom, prop, len); >>> + if (ret) { >>> + dev_err(adsp->dev, "Unable to map memory >>> regions accessed by ADSP\n"); >>> + goto sid_unmap; >>> + } >>> + } else { >>> + ret = adsp_rproc_map_smmu(rproc, len); >>> + if (ret) { >>> + dev_err(adsp->dev, "Unable to map memory >>> regions accessed by ADSP\n"); >> Maybe this should be a different string in case it is confused with the >> above print of the same string. > Okay. Will modify the string. As explained above will remove both of the above methods. >> >>> + goto sid_unmap; >>> + } >>> + } >>> + return 0; >>> + >>> +sid_unmap: >>> + iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size); >>> +detach_device: >>> + iommu_domain_free(adsp->iommu_dom); >>> +domain_free: >>> + return ret; >>> +} >>> + >>> + >>> static int adsp_start(struct rproc *rproc) >>> { >>> struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv; >>> @@ -343,9 +525,16 @@ static int adsp_start(struct rproc *rproc) >>> if (ret) >>> return ret; >>> >>> + if (adsp->adsp_sandbox_needed) { >>> + ret = adsp_map_smmu(adsp, rproc); >>> + if (ret) { >>> + dev_err(adsp->dev, "ADSP smmu mapping >>> failed\n"); >>> + goto disable_irqs; >>> + } >>> + } >> Newline here please. > Okay. >> >>> ret = clk_prepare_enable(adsp->xo); >>> if (ret) >>> - goto disable_irqs; >>> + goto adsp_smmu_unmap; >>> >>> ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds, >>> adsp->proxy_pd_count);
| |