Message-ID: <310913ee-6275-4366-ad74-6565b7443867@linaro.org>
Date: Thu, 15 Aug 2024 15:01:53 +0200
From: Caleb Connolly <caleb.connolly@...aro.org>
To: Marc Gonzalez <mgonzalez@...ebox.fr>,
 Bjorn Andersson <andersson@...nel.org>
Cc: Rob Clark <robdclark@...il.com>, Will Deacon <will@...nel.org>,
 Robin Murphy <robin.murphy@....com>, Joerg Roedel <joro@...tes.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, iommu@...ts.linux.dev,
 linux-arm-msm@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 Konrad Dybcio <konradybcio@...nel.org>, Arnaud Vrac <avrac@...ebox.fr>,
 Pierre-Hugues Husson <phhusson@...ebox.fr>,
 Marijn Suijten <marijn.suijten@...ainline.org>
Subject: Re: [PATCH 2/2] iommu/arm-smmu-qcom: hide last context bank from
 linux

Hi Marc,

On 14/08/2024 19:33, Marc Gonzalez wrote:
> On 14/08/2024 17:29, Bjorn Andersson wrote:
>> On Wed, Aug 14, 2024 at 03:59:56PM GMT, Marc Gonzalez wrote:
>>> On qcom msm8998, writing to the last context bank of lpass_q6_smmu
>>> (base address 0x05100000) produces a system freeze & reboot.
>>>
>>> Specifically, here:
>>>
>>> 	qsmmu->bypass_cbndx = smmu->num_context_banks - 1;
>>> 	arm_smmu_cb_write(smmu, qsmmu->bypass_cbndx, ARM_SMMU_CB_SCTLR, 0);
>>>
>>> and here:
>>>
>>> 	arm_smmu_write_context_bank(smmu, i);
>>> 	arm_smmu_cb_write(smmu, i, ARM_SMMU_CB_FSR, ARM_SMMU_CB_FSR_FAULT);
>>>
>>> It is likely that FW reserves the last context bank for its own use,
>>> thus a simple work-around would be: DON'T USE IT in Linux.
>>>
>>> If we decrease the number of context banks, last one will be "hidden".
>>
>> I asked you to write something like "the hardware/hypervisor reports 12
>> context banks for the lpass smmu on msm8998, but only 11 are
>> accessible...override the number of context banks"
> 
> I don't understand how the exact number of context banks is relevant?
> It's just that the FW reserves one for itself, which happens to be the last,
> probably because some FW dev thought that was a good idea.

It's relevant for your patch description because it offers useful 
context that might help someone in the future. Especially being specific 
that it's the hypervisor which is causing issues.
> 
> Also, I don't like the phrasing "override the number of context banks"
> because while this is indeed what is done in the code, the *intent*
> is to "lie" to Linux about the existence of the last context bank.

I'm not sure if that framing makes this code easier to understand for 
me. You aren't lying to the kernel, you're enabling a quirk/override.

There is another hypervisor quirk on Qualcomm platforms where BYPASS 
type streams are disallowed. We work around this by using the 
(coincidentally) last context bank to emulate them.
> 
>> It also seems, as the different SMMUs in this platform behave
>> differently it might be worth giving them further specific compatibles,
>> in which case we could just check if it's the qcom,msm8998-lpass-smmu,
>> instead of inventing a property for this quirk.
> 
> Wouldn't that be too specific?

If we aren't aware of any other platforms that have this issue, then no.
> 
> Angelo's patches were even more generic than mine, as he supported
> a list of context banks not-to-be-used-by-linux.
> 
> Do you say the LPASS SMMU behaves differently because it's (currently,
> to the best of my knowledge) the only SMMU where a context bank
> (the last) is not available to Linux?

I think that's a reasonable assumption to make.
> 
> 
> For easy future reference, here are the reports for the 5 SMMUs enabled on my system.
> 
> [    0.137343] arm-smmu 1680000.iommu: probing hardware configuration...
> [    0.137354] arm-smmu 1680000.iommu: SMMUv2 with:
> [    0.137381] arm-smmu 1680000.iommu: 	stage 1 translation
> [    0.137390] arm-smmu 1680000.iommu: 	address translation ops
> [    0.137399] arm-smmu 1680000.iommu: 	non-coherent table walk
> [    0.137406] arm-smmu 1680000.iommu: 	(IDR0.CTTW overridden by FW configuration)
> [    0.137417] arm-smmu 1680000.iommu: 	stream matching with 16 register groups
> [    0.137447] arm-smmu 1680000.iommu: 	6 context banks (0 stage-2 only)
> [    0.137733] arm-smmu 1680000.iommu: 	Supported page sizes: 0x63315000
> [    0.137743] arm-smmu 1680000.iommu: 	Stage-1: 36-bit VA -> 36-bit IPA
> [    0.137833] arm-smmu 1680000.iommu: 	preserved 0 boot mappings
> 
> 
> [    0.138963] arm-smmu 16c0000.iommu: probing hardware configuration...
> [    0.138974] arm-smmu 16c0000.iommu: SMMUv2 with:
> [    0.138994] arm-smmu 16c0000.iommu: 	stage 1 translation
> [    0.139003] arm-smmu 16c0000.iommu: 	address translation ops
> [    0.139011] arm-smmu 16c0000.iommu: 	non-coherent table walk
> [    0.139019] arm-smmu 16c0000.iommu: 	(IDR0.CTTW overridden by FW configuration)
> [    0.139030] arm-smmu 16c0000.iommu: 	stream matching with 14 register groups
> [    0.139058] arm-smmu 16c0000.iommu: 	10 context banks (0 stage-2 only)
> [    0.139255] arm-smmu 16c0000.iommu: 	Supported page sizes: 0x63315000
> [    0.139265] arm-smmu 16c0000.iommu: 	Stage-1: 36-bit VA -> 36-bit IPA
> [    0.139341] arm-smmu 16c0000.iommu: 	preserved 0 boot mappings
> 
> 
> [    2.424369] arm-smmu 5040000.iommu: probing hardware configuration...
> [    2.428581] arm-smmu 5040000.iommu: SMMUv2 with:
> [    2.434914] arm-smmu 5040000.iommu: 	stage 1 translation
> [    2.439584] arm-smmu 5040000.iommu: 	address translation ops
> [    2.444881] arm-smmu 5040000.iommu: 	non-coherent table walk
> [    2.450522] arm-smmu 5040000.iommu: 	(IDR0.CTTW overridden by FW configuration)
> [    2.456175] arm-smmu 5040000.iommu: 	stream matching with 3 register groups
> [    2.463216] arm-smmu 5040000.iommu: 	3 context banks (0 stage-2 only)
> [    2.483555] arm-smmu 5040000.iommu: 	Supported page sizes: 0x63315000
> [    2.490455] arm-smmu 5040000.iommu: 	Stage-1: 48-bit VA -> 36-bit IPA
> [    2.497171] arm-smmu 5040000.iommu: 	preserved 0 boot mappings
> 
> 
> [    2.546101] arm-smmu 5100000.iommu: probing hardware configuration...
> [    2.552439] arm-smmu 5100000.iommu: SMMUv2 with:
> [    2.558945] arm-smmu 5100000.iommu: 	stage 1 translation
> [    2.563627] arm-smmu 5100000.iommu: 	address translation ops
> [    2.568923] arm-smmu 5100000.iommu: 	non-coherent table walk
> [    2.574566] arm-smmu 5100000.iommu: 	(IDR0.CTTW overridden by FW configuration)
> [    2.580220] arm-smmu 5100000.iommu: 	stream matching with 12 register groups
> [    2.587263] arm-smmu 5100000.iommu: 	13 context banks (0 stage-2 only)
> [    2.594544] arm-smmu 5100000.iommu: hiding last ctx bank from linux
> [    2.614447] arm-smmu 5100000.iommu: 	Supported page sizes: 0x63315000
> [    2.621358] arm-smmu 5100000.iommu: 	Stage-1: 36-bit VA -> 36-bit IPA
> [    2.627772] arm-smmu 5100000.iommu: 	preserved 0 boot mappings
> 
> 
> [    2.806781] arm-smmu cd00000.iommu: probing hardware configuration...
> [    2.813029] arm-smmu cd00000.iommu: SMMUv2 with:
> [    2.819627] arm-smmu cd00000.iommu: 	stage 1 translation
> [    2.824304] arm-smmu cd00000.iommu: 	address translation ops
> [    2.829601] arm-smmu cd00000.iommu: 	non-coherent table walk
> [    2.835243] arm-smmu cd00000.iommu: 	(IDR0.CTTW overridden by FW configuration)
> [    2.840897] arm-smmu cd00000.iommu: 	stream matching with 54 register groups
> [    2.847954] arm-smmu cd00000.iommu: 	17 context banks (0 stage-2 only)
> [    2.869307] arm-smmu cd00000.iommu: 	Supported page sizes: 0x63315000
> [    2.875785] arm-smmu cd00000.iommu: 	Stage-1: 32-bit VA -> 36-bit IPA
> [    2.882205] arm-smmu cd00000.iommu: 	preserved 0 boot mappings
> 
> 
> [   24.525457] arm-smmu 16c0000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1900
> [   24.525604] arm-smmu 16c0000.iommu: FSYNR0 = 00000001 [S1CBNDX=0 PLVL=1]
> [   24.721874] arm-smmu 16c0000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1900
> [   24.722033] arm-smmu 16c0000.iommu: FSYNR0 = 00000001 [S1CBNDX=0 PLVL=1]
> 
> 

-- 
// Caleb (they/them)

Powered by blists - more mailing lists