* [PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load @ 2020-02-04 6:26 Bjorn Andersson 2020-02-04 6:26 ` [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson 2020-02-04 6:26 ` [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 0 siblings, 2 replies; 7+ messages in thread From: Bjorn Andersson @ 2020-02-04 6:26 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2020-02-04 6:26 [PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson @ 2020-02-04 6:26 ` Bjorn Andersson 2020-02-10 23:05 ` Mathieu Poirier 2020-02-04 6:26 ` [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2020-02-04 6:26 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, 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> --- 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2020-02-04 6:26 ` [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson @ 2020-02-10 23:05 ` Mathieu Poirier 2020-02-11 1:16 ` Bjorn Andersson 0 siblings, 1 reply; 7+ messages in thread From: Mathieu Poirier @ 2020-02-10 23:05 UTC (permalink / raw) To: Bjorn Andersson Cc: Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, stable 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2020-02-10 23:05 ` Mathieu Poirier @ 2020-02-11 1:16 ` Bjorn Andersson 2020-02-11 18:42 ` Mathieu Poirier 0 siblings, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2020-02-11 1:16 UTC (permalink / raw) To: Mathieu Poirier Cc: Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, stable 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 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown 2020-02-11 1:16 ` Bjorn Andersson @ 2020-02-11 18:42 ` Mathieu Poirier 0 siblings, 0 replies; 7+ messages in thread From: Mathieu Poirier @ 2020-02-11 18:42 UTC (permalink / raw) To: Bjorn Andersson Cc: Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, Linux Kernel Mailing List, Sibi Sankar, Jeffrey Hugo, # 4 . 7 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 > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading 2020-02-04 6:26 [PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson 2020-02-04 6:26 ` [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson @ 2020-02-04 6:26 ` Bjorn Andersson 2020-02-10 23:10 ` Mathieu Poirier 1 sibling, 1 reply; 7+ messages in thread From: Bjorn Andersson @ 2020-02-04 6:26 UTC (permalink / raw) To: Bjorn Andersson, Ohad Ben-Cohen Cc: linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, Luca Weiss 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading 2020-02-04 6:26 ` [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson @ 2020-02-10 23:10 ` Mathieu Poirier 0 siblings, 0 replies; 7+ messages in thread From: Mathieu Poirier @ 2020-02-10 23:10 UTC (permalink / raw) To: Bjorn Andersson Cc: Ohad Ben-Cohen, linux-arm-msm, linux-remoteproc, linux-kernel, Sibi Sankar, Jeffrey Hugo, Luca Weiss 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 > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-02-11 18:42 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-04 6:26 [PATCH v3 0/2] remoteproc: mss: Improve mem_assign and firmware load Bjorn Andersson 2020-02-04 6:26 ` [PATCH v3 1/2] remoteproc: qcom_q6v5_mss: Don't reassign mpss region on shutdown Bjorn Andersson 2020-02-10 23:05 ` Mathieu Poirier 2020-02-11 1:16 ` Bjorn Andersson 2020-02-11 18:42 ` Mathieu Poirier 2020-02-04 6:26 ` [PATCH v3 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Bjorn Andersson 2020-02-10 23:10 ` Mathieu Poirier
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).