From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 86111CA9ECB for ; Thu, 31 Oct 2019 19:41:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 41EA42086D for ; Thu, 31 Oct 2019 19:41:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="pvebIAAj" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729616AbfJaTl6 (ORCPT ); Thu, 31 Oct 2019 15:41:58 -0400 Received: from mail-pg1-f193.google.com ([209.85.215.193]:46557 "EHLO mail-pg1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726983AbfJaTl5 (ORCPT ); Thu, 31 Oct 2019 15:41:57 -0400 Received: by mail-pg1-f193.google.com with SMTP id f19so4702752pgn.13 for ; Thu, 31 Oct 2019 12:41:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=AhjfIod7SQCQHZV7lhiJk5rEvnUEd1LmAIrAKGF1ifE=; b=pvebIAAjYa8iAiEqMGvnxsLMzzKlHrpnTijyHM1zSWRukHFcMWhu9kJKOf7iO7IpjO JsP1kiL28p94Xdd+8mAO9XFhZppUptWaO5yzT+CBfUUZfVSBZTBjM9CkPGu1NtSirEVO rTqo9TbFRBm4EAEMLIdIMYi5ztPjt8y4Tpg8hxjZi5TUcoGne0v5dR6ThE4AqEUvn86k V92bCCXeiV/EIcdvvjeoBAto0pNnZfqWcwgTeXAgJrofROuY3ynjYOPLoA7ioxSeyHYW hV54DTHeyLkfyGQJpTeDHLBtWTY+XdwCQr8ujU4oRHFzqyAs0Jc7TxKdzuCEvmsG3gLE LJNA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=AhjfIod7SQCQHZV7lhiJk5rEvnUEd1LmAIrAKGF1ifE=; b=NnZ8Mpq+lE0vdBRBKIOuPyB7CfcywfHRKEFT358i4HT14NxS9Uty6izAgoVCP7ZMQ8 HDu19ld4gQL5C5OwGfR64VEKmU2YM/CLjd9yfL9kX6NA36gwpvVgU+d/tlCNgphC3KGl EEQ2PE2FNfNZDHStijaYKrv9r+cxuZCLqAOyXVNjafXgP+HTudcw7tipAcJhePYWUKj6 ZbZTvwb/4a1gi6qRAc1Mn3EIHRnIp5kjJ7tnsyPMeEh+hqgPoxtb6kLFd7q9VC3zREl/ oo0XWWNiNkv2Msnm68rZPXwbehsxaja1E7fzfCYr09wF/aXNu9d9TSmCpExElFmyoELO i7hQ== X-Gm-Message-State: APjAAAXuOXt1aBxho7siCyz2eks5pnL7EAuZfrPd55Wd6IMezwkRjh/+ n/kCjIOXUxt5bnum99ujGmIW4g== X-Google-Smtp-Source: APXvYqzwSncZfbsCu8/7tq36MfxWW3WNUjm6V4/NRyM4kpZCJyiBSQ0YTBUq8dOGWtkNqtMIporK2w== X-Received: by 2002:a63:4501:: with SMTP id s1mr8655227pga.5.1572550916256; Thu, 31 Oct 2019 12:41:56 -0700 (PDT) Received: from tuxbook-pro (104-188-17-28.lightspeed.sndgca.sbcglobal.net. [104.188.17.28]) by smtp.gmail.com with ESMTPSA id t9sm7460172pjq.21.2019.10.31.12.41.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 31 Oct 2019 12:41:55 -0700 (PDT) Date: Thu, 31 Oct 2019 12:41:53 -0700 From: Bjorn Andersson To: Jeffrey Hugo Cc: Ohad Ben-Cohen , Avaneesh Kumar Dwivedi , MSM , linux-remoteproc@vger.kernel.org, lkml , Jeffrey Hugo , Sibi Sankar Subject: Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Validate each segment during loading Message-ID: <20191031194153.GN1929@tuxbook-pro> References: <20191031184632.2938295-1-bjorn.andersson@linaro.org> <20191031184632.2938295-3-bjorn.andersson@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.12.1 (2019-06-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 31 Oct 12:13 PDT 2019, Jeffrey Hugo wrote: > On Thu, Oct 31, 2019 at 12:48 PM 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. > > > > Signed-off-by: Bjorn Andersson > > --- > > 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. > > + 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? > > + 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... 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 > >