linux-kernel - Re: [PATCH 1/2] dt-bindings: arm-smmu: Add qcom,last-ctx-bank-reserved

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <17893776-9666-4bbe-b5fc-c3fe977d0337@arm.com>
Date: Mon, 19 Aug 2024 13:57:12 +0100
From: Robin Murphy <robin.murphy@....com>
To: Marc Gonzalez <mgonzalez@...ebox.fr>, Rob Herring <robh@...nel.org>
Cc: Rob Clark <robdclark@...il.com>, Will Deacon <will@...nel.org>,
 Joerg Roedel <joro@...tes.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,
 Bjorn Andersson <andersson@...nel.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 1/2] dt-bindings: arm-smmu: Add
 qcom,last-ctx-bank-reserved

On 19/08/2024 12:37 pm, Marc Gonzalez wrote:
> On 18/08/2024 17:25, Rob Herring wrote:
> 
>> On Wed, Aug 14, 2024 at 03:59:55PM +0200, 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.
>>>
>>> Signed-off-by: Marc Gonzalez <mgonzalez@...ebox.fr>
>>> ---
>>>   Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> index 280b4e49f2191..f9b23aef351b0 100644
>>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu.yaml
>>> @@ -204,6 +204,12 @@ properties:
>>>         access to SMMU configuration registers. In this case non-secure aliases of
>>>         secure registers have to be used during SMMU configuration.
>>>   
>>> +  qcom,last-ctx-bank-reserved:
>>> +    type: boolean
>>> +    description:
>>> +      FW reserves the last context bank of this SMMU for its own use.
>>> +      If Linux tries to use it, Linux gets nuked.
>>
>> How is this Qualcomm specific? Presumably any implementation could do
>> this if there's no way to properly partition things. Robin?
> 
> Obviously, there is nothing Qualcomm specific about reserving
> an SMMU context bank for the FW / hypervisor, other than it
> appears that qcom is the first to do it; or at least the
> LPASS SMMU on qcom msm8998 is the first known SMMU where such
> a work-around is required.

Yes, the Qualcomm-specific aspect is that it's Qualcomm's hypervisor 
which is broken and reporting a larger number in its emulated 
SMMU_IDR1.NUMCB than the number of context banks it's actually willing 
to emulate.

> What is the correct nomenclature?
> 
> Can we just drop the vendor prefix if a property is generic
> across vendors? But does it require a subsystem prefix like
> "iommu" in order to not clash with generic props in other subsystems?

I guess if we *were* to consider a generic property to endorse violating 
the SMMU architecture, then it would logically be vendored to Arm as the 
owner of the SMMU architecture. However I am strongly against that idea, 
not only because I obviously don't want to normalise hypervisors 
emulating non-architectural behaviour which every DT-consuming OS will 
have to understand how to work around, but it's also less than great for 
the user to have a workaround that's not compatible with existing DTBs.

Luckily, in this case it seems straightforward enough to be able to see 
that if we have a "qcom,msm8996-smmu-v2" with 13 context banks then we 
should just treat it as if it has 12 - it's also notable that it only 
reports NUMSMRG=12, so we couldn't use more than that many S1 context 
banks at once anyway.

Thanks,
Robin.

>> Also, this property isn't very flexible. What happens when it is not the
>> last bank or more than 1 bank reserved? This should probably be a mask
>> instead.
> 
> OK, I'm getting conflicting requests here.
> 
> Bjorn has recommended dropping the property altogether:
> 
>> 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.
> 
> 
> I'll send a patch series in line with Bjorn's request.
> 
> Regards
>