Received-SPF: pass (google.com: domain of linux-crypto+bounces-2114-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 16 Feb 2024 13:27:42 +0100
From: Stephan Gerhold <stephan@gerhold.net>
To: Luca Weiss <luca.weiss@fairphone.com>
Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Thara Gopinath <thara.gopinath@gmail.com>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	"David S. Miller" <davem@davemloft.net>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Bhupesh Sharma <bhupesh.sharma@linaro.org>,
	~postmarketos/upstreaming@lists.sr.ht, phone-devel@vger.kernel.org,
	linux-crypto@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] arm64: dts: qcom: sm6350: Add Crypto Engine
Message-ID: <Zc9UvgAsTvazQ_HR@gerhold.net>
References: <20240105-sm6350-qce-v1-0-416e5c7319ac@fairphone.com>
 <20240105-sm6350-qce-v1-2-416e5c7319ac@fairphone.com>
 <ZZguvdJTyVgfxm4D@gerhold.net>
 <CZ6FYZLGWT3K.ZBHYDQ7TDN4B@fairphone.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CZ6FYZLGWT3K.ZBHYDQ7TDN4B@fairphone.com>
Content-Transfer-Encoding: 7bit

On Fri, Feb 16, 2024 at 11:46:49AM +0100, Luca Weiss wrote:
> On Fri Jan 5, 2024 at 5:30 PM CET, Stephan Gerhold wrote:
> > On Fri, Jan 05, 2024 at 05:15:44PM +0100, Luca Weiss wrote:
> > > Add crypto engine (CE) and CE BAM related nodes and definitions for this
> > > SoC.
> > > 
> > > For reference:
> > > 
> > >   [    2.297419] qcrypto 1dfa000.crypto: Crypto device found, version 5.5.1
> > > 
> > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 31 +++++++++++++++++++++++++++++++
> > >  1 file changed, 31 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > index 8fd6f4d03490..516aadbb16bb 100644
> > > --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> > > @@ -1212,6 +1212,37 @@ ufs_mem_phy_lanes: phy@1d87400 {
> > >  			};
> > >  		};
> > >  
> > > +		cryptobam: dma-controller@1dc4000 {
> > > +			compatible = "qcom,bam-v1.7.4", "qcom,bam-v1.7.0";
> > > +			reg = <0 0x01dc4000 0 0x24000>;
> > > +			interrupts = <GIC_SPI 272 IRQ_TYPE_LEVEL_HIGH>;
> > > +			#dma-cells = <1>;
> > > +			qcom,ee = <0>;
> > > +			qcom,controlled-remotely;
> > > +			num-channels = <16>;
> > > +			qcom,num-ees = <4>;
> > > +			iommus = <&apps_smmu 0x432 0x0000>,
> > > +				 <&apps_smmu 0x438 0x0001>,
> > > +				 <&apps_smmu 0x43f 0x0000>,
> > > +				 <&apps_smmu 0x426 0x0011>,
> > > +				 <&apps_smmu 0x436 0x0011>;
> >
> > The last two lines look equivalent to me: 0x436 & ~0x0011 = 0x426.
> 
> I don't understand the IOMMU SID + mask really, but I think I've seen
> somewhere before like here that TZ can be a bit picky with the SIDs?
> 
> https://lore.kernel.org/linux-arm-msm/opqdrmyj3y64nqqqmakjydn5rkspizufyeavm7ec7c7ufqz4wk@ey2a7bq3shfj/
> https://lore.kernel.org/linux-arm-msm/11b5db69-49f5-4d7b-81c9-687d66a5cb0d@linaro.org/
> 
> I don't quite want to risk having some obscure use case breaking because
> we cleaned up the dts ;)
> 
> But if you're more sure than me that it won't break, let me know!
> 

I'm afraid I can't really help with this kind of certainty. My knowledge
about proprietary Qualcomm firmware is probably even more limited than
yours. However, my personal feeling is that the "TZ wants X" arguments
are most often just as badly based on superficial knowledge.

In simplified terms, the SMMU has a number of components connected to it
(the crypto BAM, USB controller, UFS, ...). When the components make
memory requests they are identified by a number of Stream IDs (SIDs).
The purpose of "iommus" in the device tree is to describe all SIDs that
belong to a particular device. These SIDs are then all assigned to a
context bank that allows the device to access selected regions in RAM.

It shouldn't matter *how* the SIDs are matched inside the SMMU, as long
as they end up at the correct context bank. The SMMU will look through
the configured Stream Match Registers (SMRs = SID + Mask) to find the
context bank that is assigned to the SID. The docs say "If MASK[i]==1,
ID[i] is ignored.". This means a SMR with ID=0x426 MASK=0x0011 is
definitely identical to ID=0x436 MASK=0x0011. Having the extra entry
will make absolutely no difference to the SMMU aside from wasting a
pointless SMR.

The links you posted suggest "TZ" looks at the SMRs allocated by Linux.
If that is really the case then I would expect that to be fundamentally
broken. In my opinion there is absolutely no guarantee how or in which
order Linux allocates the SMRs. Such functionality would either be
extremely complex or broken in tons of edge cases.

TL;DR: I cannot provide proof that removing this entry makes a
difference. I can just say that I doubt it does, and if it does, then we
have far more serious problems. The device tree is supposed to describe
the hardware ("This device makes memory requests with the following
SIDs") and not fundamentally broken peculiarities of the proprietary TZ
firmware ("registers must be programmed exactly with these values").

Thanks,
Stephan