| Message ID | 1494957722-13264-4-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote: > +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc, > + phys_addr_t addr, size_t size) > +{ > + struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA, > + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} }; struct qcom_scm_destVmPerm next = { .vmid = QCOM_SCM_VMID_MSS_MSA, .perm = QCOM_SCM_PERM_RW }; > + int ret; > + > + size = ALIGN(size, SZ_4K); I believe it will be cleaner if you just put this alignment directly in the function call below. > + if (!qproc->need_mem_protection) > + return 0; Put a blank line here, to give separation between the sections of the function. > + ret = qcom_scm_assign_mem(addr, size, > + BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next)); qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), BIT(QCOM_SCM_VMID_HLOS), &next, 1); > + if (ret) > + pr_err("%s: Failed to assign memory access, ret = %d\n", > + __func__, ret); There's no point in printing an error here and in the calling function, but as it makes sense to have the error message to include which memory region we operated on (mba vs mpss) I think you should remove the print here and keep it in the callers. So just return qcom-scm_assign_mem(). > + return ret; > +} > + > +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc, > + phys_addr_t addr, size_t size) > +{ > + struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS, > + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)} > + }; struct qcom_scm_destVmPerm next = { .vmid = QCOM_SCM_VMID_HLOS, .perm = QCOM_SCM_PERM_RWX, }; (And add RWX to the list of defines in patch 1) [..] > @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) > dev_err(qproc->dev, > "metadata authentication failed: %d\n", ret); > > + /* Metadata authentication done, remove modem access */ > + ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size); > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim metadata memory, ret - %d\n", ret); If this assignment fails (for any reason) we can't return the memory to the free pool in Linux, because at some point in the future these pages will be allocated to someone else resulting in a memory access violation scenario that will be just terrible to debug. I do however not have a better idea at the moment than just leaking the memory. > dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); > > return ret < 0 ? ret : 0; [..] > @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) > > ret = q6v5_mpss_load(qproc); > if (ret) > - goto halt_axi_ports; > + goto reclaim_mem; This doesn't allow us to distinguish between failures before or after the memory assignment in mpss_load(), so although you're duplicating the reclaim code, mpss_load() must be responsible of restoring the state on failure. The timeout below still has to reclaim the memory. > > ret = wait_for_completion_timeout(&qproc->start_done, > msecs_to_jiffies(5000)); > if (ret == 0) { > dev_err(qproc->dev, "start timed out\n"); > ret = -ETIMEDOUT; > - goto halt_axi_ports; > + goto reclaim_mem; > } > > + ret = q6v5_assign_mem_to_linux(qproc, > + qproc->mba_phys, qproc->mba_size); > + if (ret) > + dev_err(qproc->dev, > + "Failed to reclaim mba memory, ret - %d\n", ret); I think it's okay for symmetrical purposes to keep the memory assigned to the remote until we call stop(). Although this shows that we should be able to release the mba allocation here. > qproc->running = true; > > q6v5_clk_disable(qproc->dev, qproc->proxy_clks, > @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc) > > return 0; > > +reclaim_mem: > + assign_mem_result = > + q6v5_assign_mem_to_linux(qproc, > + qproc->mpss_phys, qproc->mpss_size); If this fails we're screwed. We have no way to fail in a way that will not free the memory at any later point in time. So I do believe you should have a BUG_ON(assign_mem_result); here. With a clear comment in the lines of: /* * Failed to reclaim memory, any future access to these pages will cause * security violations and a seemingly random crash */ > halt_axi_ports: > q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); > q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); > q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); > q6v5_clk_disable(qproc->dev, qproc->active_clks, > qproc->active_clk_count); > + assign_mem_result = > + q6v5_assign_mem_to_linux(qproc, > + qproc->mba_phys, qproc->mba_size); Shouldn't we move them even further down? Same BUG_ON() as above. > assert_reset: > reset_control_assert(qproc->mss_restart); > disable_vdd: Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 5/26/2017 7:46 AM, Bjorn Andersson wrote: > On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote: > >> +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc, >> + phys_addr_t addr, size_t size) >> +{ >> + struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA, >> + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} }; > struct qcom_scm_destVmPerm next = { > .vmid = QCOM_SCM_VMID_MSS_MSA, > .perm = QCOM_SCM_PERM_RW > }; OK. > >> + int ret; >> + >> + size = ALIGN(size, SZ_4K); > I believe it will be cleaner if you just put this alignment directly in > the function call below. OK. > >> + if (!qproc->need_mem_protection) >> + return 0; > Put a blank line here, to give separation between the sections of the > function. OK. > >> + ret = qcom_scm_assign_mem(addr, size, >> + BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next)); > qcom_scm_assign_mem(addr, ALIGN(size, SZ_4K), > BIT(QCOM_SCM_VMID_HLOS), &next, 1); OK > >> + if (ret) >> + pr_err("%s: Failed to assign memory access, ret = %d\n", >> + __func__, ret); > There's no point in printing an error here and in the calling function, > but as it makes sense to have the error message to include which memory > region we operated on (mba vs mpss) I think you should remove the print > here and keep it in the callers. OK > > So just return qcom-scm_assign_mem(). OK > >> + return ret; >> +} >> + >> +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc, >> + phys_addr_t addr, size_t size) >> +{ >> + struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS, >> + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)} >> + }; > struct qcom_scm_destVmPerm next = { > .vmid = QCOM_SCM_VMID_HLOS, > .perm = QCOM_SCM_PERM_RWX, > }; > > (And add RWX to the list of defines in patch 1) OK > > [..] >> @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) >> dev_err(qproc->dev, >> "metadata authentication failed: %d\n", ret); >> >> + /* Metadata authentication done, remove modem access */ >> + ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size); >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim metadata memory, ret - %d\n", ret); > If this assignment fails (for any reason) we can't return the memory to > the free pool in Linux, because at some point in the future these pages > will be allocated to someone else resulting in a memory access violation > scenario that will be just terrible to debug. > > I do however not have a better idea at the moment than just leaking the > memory. Then shall we BUG_ON here itself? > >> dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); >> >> return ret < 0 ? ret : 0; > [..] >> @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) >> >> ret = q6v5_mpss_load(qproc); >> if (ret) >> - goto halt_axi_ports; >> + goto reclaim_mem; > This doesn't allow us to distinguish between failures before or after > the memory assignment in mpss_load(), so although you're duplicating the > reclaim code, mpss_load() must be responsible of restoring the state on > failure. OK, will move one of reclaim path in mpss_load it self. > > The timeout below still has to reclaim the memory. Yes that any way will do. > >> >> ret = wait_for_completion_timeout(&qproc->start_done, >> msecs_to_jiffies(5000)); >> if (ret == 0) { >> dev_err(qproc->dev, "start timed out\n"); >> ret = -ETIMEDOUT; >> - goto halt_axi_ports; >> + goto reclaim_mem; >> } >> >> + ret = q6v5_assign_mem_to_linux(qproc, >> + qproc->mba_phys, qproc->mba_size); >> + if (ret) >> + dev_err(qproc->dev, >> + "Failed to reclaim mba memory, ret - %d\n", ret); > I think it's okay for symmetrical purposes to keep the memory assigned > to the remote until we call stop(). Actually MBA image is transferred into internal memory of q6 and does not run from DDR that is why it is OK to release it here. let me know if you dont want to do that. > > Although this shows that we should be able to release the mba allocation > here. Yes that is true. > >> qproc->running = true; >> >> q6v5_clk_disable(qproc->dev, qproc->proxy_clks, >> @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc) >> >> return 0; >> >> +reclaim_mem: >> + assign_mem_result = >> + q6v5_assign_mem_to_linux(qproc, >> + qproc->mpss_phys, qproc->mpss_size); > If this fails we're screwed. We have no way to fail in a way that will > not free the memory at any later point in time. > > So I do believe you should have a BUG_ON(assign_mem_result); here. With > a clear comment in the lines of: > > /* > * Failed to reclaim memory, any future access to these pages will cause > * security violations and a seemingly random crash > */ Yes this is very good suggestion thanks. >> halt_axi_ports: >> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); >> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); >> q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); >> q6v5_clk_disable(qproc->dev, qproc->active_clks, >> qproc->active_clk_count); >> + assign_mem_result = >> + q6v5_assign_mem_to_linux(qproc, >> + qproc->mba_phys, qproc->mba_size); > Shouldn't we move them even further down? There does not seem any restriction w.r.t. keeping it here. Do you think any side effect ? > > Same BUG_ON() as above. Yes sure. > >> assert_reset: >> reset_control_assert(qproc->mss_restart); >> disable_vdd: > Regards, > Bjorn
On Fri 26 May 06:19 PDT 2017, Dwivedi, Avaneesh Kumar (avani) wrote: > > > On 5/26/2017 7:46 AM, Bjorn Andersson wrote: > > On Tue 16 May 11:02 PDT 2017, Avaneesh Kumar Dwivedi wrote: > > > @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) > > > dev_err(qproc->dev, > > > "metadata authentication failed: %d\n", ret); > > > + /* Metadata authentication done, remove modem access */ > > > + ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size); > > > + if (ret) > > > + dev_err(qproc->dev, > > > + "Failed to reclaim metadata memory, ret - %d\n", ret); > > If this assignment fails (for any reason) we can't return the memory to > > the free pool in Linux, because at some point in the future these pages > > will be allocated to someone else resulting in a memory access violation > > scenario that will be just terrible to debug. > > > > I do however not have a better idea at the moment than just leaking the > > memory. > Then shall we BUG_ON here itself? Here we have the chance to "handle" the problem (by leaking the memory), so it's not a catastrophic error. But perhaps better to replace the dev_err() with a WARN(ret, "failed to reclaim memory\n"); that way this issue will stand out in the log. [..] > > > @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) [..] > > > } > > > + ret = q6v5_assign_mem_to_linux(qproc, > > > + qproc->mba_phys, qproc->mba_size); > > > + if (ret) > > > + dev_err(qproc->dev, > > > + "Failed to reclaim mba memory, ret - %d\n", ret); > > I think it's okay for symmetrical purposes to keep the memory assigned > > to the remote until we call stop(). > Actually MBA image is transferred into internal memory of q6 and does not > run from DDR > that is why it is OK to release it here. let me know if you dont want to do > that. Leave it here, but please add a comment above this snippet saying something like: /* * The MBA is transferred to internal memory of the Q6 and no longer * need access. */ [..] > > > + assign_mem_result = > > > + q6v5_assign_mem_to_linux(qproc, > > > + qproc->mba_phys, qproc->mba_size); > > Shouldn't we move them even further down? > There does not seem any restriction w.r.t. keeping it here. > Do you think any side effect ? No, I'm fine with this if you say that the MPSS is no longer executing anything at this point. Thanks, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index 2626954..57a4cfec 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -110,6 +110,7 @@ struct rproc_hexagon_res { struct qcom_mss_reg_res *active_supply; char **proxy_clk_names; char **active_clk_names; + bool need_mem_protection; }; struct q6v5 { @@ -151,6 +152,7 @@ struct q6v5 { phys_addr_t mpss_reloc; void *mpss_region; size_t mpss_size; + bool need_mem_protection; struct qcom_rproc_subdev smd_subdev; }; @@ -288,6 +290,43 @@ static struct resource_table *q6v5_find_rsc_table(struct rproc *rproc, return &table; } +static int q6v5_assign_mem_to_subsys(struct q6v5 *qproc, + phys_addr_t addr, size_t size) +{ + struct qcom_scm_destVmPerm next[] = {{ QCOM_SCM_VMID_MSS_MSA, + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)} }; + int ret; + + size = ALIGN(size, SZ_4K); + if (!qproc->need_mem_protection) + return 0; + ret = qcom_scm_assign_mem(addr, size, + BIT(QCOM_SCM_VMID_HLOS), next, sizeof(next)); + if (ret) + pr_err("%s: Failed to assign memory access, ret = %d\n", + __func__, ret); + return ret; +} + +static int q6v5_assign_mem_to_linux(struct q6v5 *qproc, + phys_addr_t addr, size_t size) +{ + struct qcom_scm_destVmPerm next[] = { { QCOM_SCM_VMID_HLOS, + (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE | QCOM_SCM_PERM_EXEC)} + }; + int ret; + + size = ALIGN(size, SZ_4K); + if (!qproc->need_mem_protection) + return 0; + ret = qcom_scm_assign_mem(addr, size, + BIT(QCOM_SCM_VMID_MSS_MSA), next, sizeof(next)); + if (ret) + pr_err("%s: Failed to assign memory access, ret = %d\n", + __func__, ret); + return ret; +} + static int q6v5_load(struct rproc *rproc, const struct firmware *fw) { struct q6v5 *qproc = rproc->priv; @@ -461,6 +500,13 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) memcpy(ptr, fw->data, fw->size); + /* Hypervisor mapping to access metadata by modem */ + ret = q6v5_assign_mem_to_subsys(qproc, phys, fw->size); + if (ret) { + dev_err(qproc->dev, + "Failed to assign metadata memory, ret - %d\n", ret); + return -ENOMEM; + } writel(phys, qproc->rmb_base + RMB_PMI_META_DATA_REG); writel(RMB_CMD_META_DATA_READY, qproc->rmb_base + RMB_MBA_COMMAND_REG); @@ -471,6 +517,11 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw) dev_err(qproc->dev, "metadata authentication failed: %d\n", ret); + /* Metadata authentication done, remove modem access */ + ret = q6v5_assign_mem_to_linux(qproc, phys, fw->size); + if (ret) + dev_err(qproc->dev, + "Failed to reclaim metadata memory, ret - %d\n", ret); dma_free_attrs(qproc->dev, fw->size, ptr, phys, dma_attrs); return ret < 0 ? ret : 0; @@ -581,6 +632,10 @@ static int q6v5_mpss_load(struct q6v5 *qproc) } /* Transfer ownership of modem region with modem fw */ boot_addr = relocate ? qproc->mpss_phys : min_addr; + ret = q6v5_assign_mem_to_subsys(qproc, + qproc->mpss_phys, qproc->mpss_size); + if (ret) + return ret; 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); @@ -600,6 +655,7 @@ static int q6v5_mpss_load(struct q6v5 *qproc) static int q6v5_start(struct rproc *rproc) { struct q6v5 *qproc = (struct q6v5 *)rproc->priv; + int assign_mem_result; int ret; ret = q6v5_regulator_enable(qproc, qproc->proxy_regs, @@ -635,6 +691,13 @@ static int q6v5_start(struct rproc *rproc) goto assert_reset; } + ret = q6v5_assign_mem_to_subsys(qproc, + qproc->mba_phys, qproc->mba_size); + if (ret) { + dev_err(qproc->dev, + "Failed to assign mba memory access, ret - %d\n", ret); + goto assert_reset; + } writel(qproc->mba_phys, qproc->rmb_base + RMB_MBA_IMAGE_REG); ret = q6v5proc_reset(qproc); @@ -656,16 +719,21 @@ static int q6v5_start(struct rproc *rproc) ret = q6v5_mpss_load(qproc); if (ret) - goto halt_axi_ports; + goto reclaim_mem; ret = wait_for_completion_timeout(&qproc->start_done, msecs_to_jiffies(5000)); if (ret == 0) { dev_err(qproc->dev, "start timed out\n"); ret = -ETIMEDOUT; - goto halt_axi_ports; + goto reclaim_mem; } + ret = q6v5_assign_mem_to_linux(qproc, + qproc->mba_phys, qproc->mba_size); + if (ret) + dev_err(qproc->dev, + "Failed to reclaim mba memory, ret - %d\n", ret); qproc->running = true; q6v5_clk_disable(qproc->dev, qproc->proxy_clks, @@ -675,12 +743,19 @@ static int q6v5_start(struct rproc *rproc) return 0; +reclaim_mem: + assign_mem_result = + q6v5_assign_mem_to_linux(qproc, + qproc->mpss_phys, qproc->mpss_size); halt_axi_ports: q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6); q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); q6v5_clk_disable(qproc->dev, qproc->active_clks, qproc->active_clk_count); + assign_mem_result = + q6v5_assign_mem_to_linux(qproc, + qproc->mba_phys, qproc->mba_size); assert_reset: reset_control_assert(qproc->mss_restart); disable_vdd: @@ -717,6 +792,8 @@ static int q6v5_stop(struct rproc *rproc) q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem); q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc); + ret = q6v5_assign_mem_to_linux(qproc, + qproc->mpss_phys, qproc->mpss_size); reset_control_assert(qproc->mss_restart); q6v5_clk_disable(qproc->dev, qproc->active_clks, qproc->active_clk_count); @@ -1014,6 +1091,7 @@ static int q6v5_probe(struct platform_device *pdev) if (ret) goto free_rproc; + qproc->need_mem_protection = desc->need_mem_protection; ret = q6v5_request_irq(qproc, pdev, "wdog", q6v5_wdog_interrupt); if (ret < 0) goto free_rproc; @@ -1089,6 +1167,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .need_mem_protection = false, }; static const struct rproc_hexagon_res msm8974_mss = { @@ -1126,6 +1205,7 @@ static int q6v5_remove(struct platform_device *pdev) "mem", NULL }, + .need_mem_protection = false, }; static const struct of_device_id q6v5_of_match[] = {
MSS proc on msm8996 can not access fw loaded region without stage second translation of memory pages where mpss image are loaded. This patch in order to enable mss boot on msm8996 invoke scm call to switch or share ownership between apps and modem. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_pil.c | 84 +++++++++++++++++++++++++++++++++++++- 1 file changed, 82 insertions(+), 2 deletions(-)