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

From: Jeffrey Hugo
Date: Thu Oct 31 2019 - 15:51:45 EST

Next message: Sami Tolvanen: "[RESEND PATCH v2] arm64: lse: fix LSE atomics with LLVM's integrated assembler"
Previous message: Sami Tolvanen: "[RESEND PATCH] arm64: fix alternatives with LLVM's integrated assembler"
In reply to: Bjorn Andersson: "Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading"
Next in thread: Jeffrey Hugo: "Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Oct 31, 2019 at 1:41 PM Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
>
> On Thu 31 Oct 12:13 PDT 2019, Jeffrey Hugo wrote:
>
> > On Thu, Oct 31, 2019 at 12:48 PM Bjorn Andersson
> > <bjorn.andersson@xxxxxxxxxx> 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.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>
> > > ---
> > > drivers/remoteproc/qcom_q6v5_mss.c | 74 ++++++++++++++++++++----------
> > > 1 file changed, 50 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> > > index f4a02105d539..bdf1dd00b89b 100644
> > > --- a/drivers/remoteproc/qcom_q6v5_mss.c
> > > +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> > > @@ -358,23 +358,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;
> >
> > So, does this need to be tristate? Downstream makes the HLOS perms
> > RWX only when HLOS is the sole owner. HLOS has RW perms when it
> > shares ownership with the remote (modem).
> >
>
> I've not seen any issues with retaining the X in my testing so far, and
> it does make the code cleaner.

Fair enough. I was just wondering how true to downstream you wanted to be.

>
> > > + 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)
> > > @@ -681,7 +687,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);
> > > @@ -699,7 +705,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");
> > > @@ -786,7 +792,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,
> > > @@ -820,8 +826,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,
> > > @@ -888,7 +894,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);
> > > @@ -915,6 +921,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;
> > > @@ -965,9 +972,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 */
> > > @@ -1016,10 +1033,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);
> >
> > Should there be an explicit wmb() here since presumably this write
> > needs to be flushed out before we do the following readl?
> >
>
> Doesn't the readl of an adjacent register wait for the write to hit the
> hardware before returning its value?

Depends on the micro architecture. I forget what early chip it was,
(somewhere in A family perhaps?) where things changed so that ordering
like this would be guaranteed within a 4k page, but the CPU was free
to reorder accesses between pages (even if you just happened to split
the page boundary), which ended up causing a lot of issues. The
previous chip guaranteed such ordering within an entire MMU mapping.

As far as I am aware you can only depend the CPU executing accesses in
the same order if its the same address.

>
> > > + ret = readl(qproc->rmb_base + RMB_MBA_STATUS_REG);
> >
> > Is this status reg immediately updated when RMB_PMI_CODE_LENGTH_REG is
> > updated? I would expect that the MBA may need some time to update
> > this reg, thus there is an implicit race here.
> >
>
> I don't see any waiting going on downstream and afaict there wouldn't be
> any harm in not waiting for the status register to go negative, as
> issues will either be caught on the next iteration or when we're
> checking the final status.
>
> Presumably though there could be an issue if this somehow depended on us
> not loading the next chunk until this has "stabilized".
>
> But afaict, this is how downstream does it...

Upon second look, I actually think this will be fine.

>
> Regards,
> Bjorn
>
> > > + 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,
> > > @@ -1028,11 +1059,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");
> > > @@ -1096,8 +1122,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
> > >

Next message: Sami Tolvanen: "[RESEND PATCH v2] arm64: lse: fix LSE atomics with LLVM's integrated assembler"
Previous message: Sami Tolvanen: "[RESEND PATCH] arm64: fix alternatives with LLVM's integrated assembler"
In reply to: Bjorn Andersson: "Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading"
Next in thread: Jeffrey Hugo: "Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]