[PATCH 0/2] Work around reserved SMMU context bank on msm8998

[PATCH 0/2] Work around reserved SMMU context bank on msm8998

[Marc Gonzalez]

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.

This patch series:
1) introduces a DT prop "qcom,last-ctx-bank-reserved" to indicate
that FW reserves the last context bank of a given SMMU.
2) tweaks the driver to "hide" the last context bank from Linux.

For reference, the lpass_q6_smmu node looks like this:

	lpass_q6_smmu: iommu@5100000 {
		compatible = "qcom,msm8998-smmu-v2", "qcom,smmu-v2";
		reg = <0x05100000 0x40000>;
		clocks = <&gcc HLOS1_VOTE_LPASS_ADSP_SMMU_CLK>;
		clock-names = "iface";

		#global-interrupts = <0>;
		#iommu-cells = <1>;
		interrupts =
			<GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 393 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 394 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 395 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 396 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 397 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 398 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 399 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 400 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 401 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 402 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 403 IRQ_TYPE_LEVEL_HIGH>,
			<GIC_SPI 137 IRQ_TYPE_LEVEL_HIGH>;

		power-domains = <&gcc LPASS_ADSP_GDSC>;
		status = "disabled";
	};

---
Marc Gonzalez (2):
      dt-bindings: arm-smmu: Add qcom,last-ctx-bank-reserved
      iommu/arm-smmu-qcom: hide last context bank from linux

 Documentation/devicetree/bindings/iommu/arm,smmu.yaml | 6 ++++++
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c            | 5 +++++
 2 files changed, 11 insertions(+)
---
base-commit: c934f6908ad1f210c1d1e289934d1182a6e7cb62
change-id: 20240814-smmu-d572c1a16aac

Best regards,
-- 
Marc Gonzalez <mgonzalez@freebox.fr>

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

[Marc Gonzalez]

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@freebox.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.
+
   stream-match-mask:
     $ref: /schemas/types.yaml#/definitions/uint32
     description: |

-- 
2.34.1

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

[Rob Herring]

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@freebox.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?

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.

Rob

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

[Marc Gonzalez]

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@freebox.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.

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?

> 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

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

[Robin Murphy]

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@freebox.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
> 

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

[Marc Gonzalez]

On 19/08/2024 14:57, Robin Murphy wrote:

> 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.

This is what the hypervisor reports:

[    2.550974] arm-smmu 5100000.iommu: probing hardware configuration...
[    2.557309] arm-smmu 5100000.iommu: SMMUv2 with:
[    2.563815] arm-smmu 5100000.iommu:  stage 1 translation
[    2.568494] arm-smmu 5100000.iommu:  address translation ops
[    2.573791] arm-smmu 5100000.iommu:  non-coherent table walk
[    2.579434] arm-smmu 5100000.iommu:  (IDR0.CTTW overridden by FW configuration)
[    2.585088] arm-smmu 5100000.iommu:  stream matching with 12 register groups
[    2.592132] arm-smmu 5100000.iommu:  13 context banks (0 stage-2 only)
[    2.619316] arm-smmu 5100000.iommu:  Supported page sizes: 0x63315000
[    2.626225] arm-smmu 5100000.iommu:  Stage-1: 36-bit VA -> 36-bit IPA
[    2.632645] arm-smmu 5100000.iommu:  preserved 0 boot mappings


smmu->num_mapping_groups = 12
smmu->num_context_banks  = 13


Are you saying that

	smmu->num_context_banks > smmu->num_mapping_groups

does not make sense?


Would a well-placed

	if (smmu->num_context_banks > smmu->num_mapping_groups)
		smmu->num_context_banks = smmu->num_mapping_groups;

be a proper work-around?

(Probably in qcom_smmu_cfg_probe() so as to not interfere with other platforms.)


Maybe to limit the side effects even more:

	if (of_device_is_compatible(smmu->dev->of_node, "qcom,msm8998-smmu-v2") &&
		smmu->num_context_banks > smmu->num_mapping_groups))
		smmu->num_context_banks = smmu->num_mapping_groups;


Neither work-around would require changing the binding.

Is either work-around acceptable, Robin?

Regards

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

[Bjorn Andersson]

On Mon, Aug 19, 2024 at 05:02:16PM +0200, Marc Gonzalez wrote:
> On 19/08/2024 14:57, Robin Murphy wrote:
> 
> > 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.
> 
> This is what the hypervisor reports:
> 
> [    2.550974] arm-smmu 5100000.iommu: probing hardware configuration...
> [    2.557309] arm-smmu 5100000.iommu: SMMUv2 with:
> [    2.563815] arm-smmu 5100000.iommu:  stage 1 translation
> [    2.568494] arm-smmu 5100000.iommu:  address translation ops
> [    2.573791] arm-smmu 5100000.iommu:  non-coherent table walk
> [    2.579434] arm-smmu 5100000.iommu:  (IDR0.CTTW overridden by FW configuration)
> [    2.585088] arm-smmu 5100000.iommu:  stream matching with 12 register groups
> [    2.592132] arm-smmu 5100000.iommu:  13 context banks (0 stage-2 only)
> [    2.619316] arm-smmu 5100000.iommu:  Supported page sizes: 0x63315000
> [    2.626225] arm-smmu 5100000.iommu:  Stage-1: 36-bit VA -> 36-bit IPA
> [    2.632645] arm-smmu 5100000.iommu:  preserved 0 boot mappings
> 
> 
> smmu->num_mapping_groups = 12

Ignore num_mapping_groups, they are used to define which streams should
be mapped to which context bank. But there's no relationship between
these numbers.

> smmu->num_context_banks  = 13
> 
> 
> Are you saying that
> 
> 	smmu->num_context_banks > smmu->num_mapping_groups
> 
> does not make sense?
> 
> 
> Would a well-placed
> 
> 	if (smmu->num_context_banks > smmu->num_mapping_groups)
> 		smmu->num_context_banks = smmu->num_mapping_groups;
> 
> be a proper work-around?

No, something like this would apply your quirk to other targets (and
specifically it would be wrong, per above).

> 
> (Probably in qcom_smmu_cfg_probe() so as to not interfere with other platforms.)
> 
> 
> Maybe to limit the side effects even more:
> 
> 	if (of_device_is_compatible(smmu->dev->of_node, "qcom,msm8998-smmu-v2") &&
> 		smmu->num_context_banks > smmu->num_mapping_groups))
> 		smmu->num_context_banks = smmu->num_mapping_groups;

If we don't want to introduce a more specific compatible for this SMMU
instance, then let's add this to qcom_smmu_cfg_probe():

	/* MSM8998 LPASS SMMU reports 13 context banks, but only 12 are accessible */
 	if (of_device_is_compatible(smmu->dev->of_node, "qcom,msm8998-smmu-v2") && smmu->num_context_banks == 13)
		smmu->num_context_banks = 12;


Regards,
Bjorn

> 
> 
> Neither work-around would require changing the binding.
> 
> Is either work-around acceptable, Robin?
> 
> Regards
> 
> 

[PATCH 2/2] iommu/arm-smmu-qcom: hide last context bank from linux

[Marc Gonzalez]

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".

Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 7e65189ca7b8c..e2e1fd9e2452b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -282,6 +282,11 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 	u32 smr;
 	int i;
 
+	if (of_property_read_bool(smmu->dev->of_node, "qcom,last-ctx-bank-reserved")) {
+		dev_warn(smmu->dev, "hiding last ctx bank from linux");
+		--smmu->num_context_banks;
+	}
+
 	/*
 	 * Some platforms support more than the Arm SMMU architected maximum of
 	 * 128 stream matching groups. For unknown reasons, the additional

-- 
2.34.1

Re: [PATCH 2/2] iommu/arm-smmu-qcom: hide last context bank from linux

[Bjorn Andersson]

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"

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.

Regards,
Bjorn

> Signed-off-by: Marc Gonzalez <mgonzalez@freebox.fr>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 7e65189ca7b8c..e2e1fd9e2452b 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -282,6 +282,11 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>  	u32 smr;
>  	int i;
>  
> +	if (of_property_read_bool(smmu->dev->of_node, "qcom,last-ctx-bank-reserved")) {
> +		dev_warn(smmu->dev, "hiding last ctx bank from linux");
> +		--smmu->num_context_banks;
> +	}
> +
>  	/*
>  	 * Some platforms support more than the Arm SMMU architected maximum of
>  	 * 128 stream matching groups. For unknown reasons, the additional
> 
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] iommu/arm-smmu-qcom: hide last context bank from linux

[Marc Gonzalez]

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.

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.

> 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?

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?


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]

Re: [PATCH 2/2] iommu/arm-smmu-qcom: hide last context bank from linux

[Caleb Connolly]

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)