[PATCH v4] iommu/arm-smmu-qcom: Limit the SMR groups to 128

[PATCH v4] iommu/arm-smmu-qcom: Limit the SMR groups to 128

[Manivannan Sadhasivam]

On some Qualcomm platforms, the hypervisor emulates more than 128 SMR
(Stream Matching Register) groups. This doesn't conform to the ARM SMMU
architecture specification which defines the range of 0-127. Moreover, the
emulated groups don't exhibit the same behavior as the architecture
supported ones.

For instance, emulated groups will not detect the quirky behavior of some
firmware versions intercepting writes to S2CR register, thus skipping the
quirk implemented in the driver and causing boot crash.

So let's limit the groups to 128 and issue a notice to users in that case.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

Changes in v4:

* Spun off the SMR limiting part into a separate patch
* Dropped the quirk rework part as it is not really needed for now

Changes in v3:

* Limited num_mapping_groups to 128 as per ARM SMMU spec and removed the
  check for 128 groups in qcom_smmu_bypass_quirk()
* Reworded the commit message accordingly

Changes in v2:

* Limited the check to 128 groups as per ARM SMMU spec's NUMSMRG range
* Moved the quirk handling to its own function
* Collected review tag from Bjorn

 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d1b296b95c86..54f62d409619 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -268,12 +268,26 @@ static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
 
 static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
-	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
 	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
+	unsigned int last_s2cr;
 	u32 reg;
 	u32 smr;
 	int i;
 
+	/*
+	 * Limit the number of stream matching groups to 128 as the ARM SMMU
+	 * architecture specification defines NUMSMRG (Number of Stream Mapping
+	 * Register Groups) in the range of 0-127, but some Qcom platforms
+	 * emulate more stream mapping groups. And those groups don't exhibit
+	 * the same behavior as the architecture supported ones.
+	 */
+	if (smmu->num_mapping_groups > 128) {
+		dev_notice(smmu->dev, "\tLimiting the stream matching groups to 128\n");
+		smmu->num_mapping_groups = 128;
+	}
+
+	last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
+
 	/*
 	 * With some firmware versions writes to S2CR of type FAULT are
 	 * ignored, and writing BYPASS will end up written as FAULT in the
-- 
2.25.1

Re: [PATCH v4] iommu/arm-smmu-qcom: Limit the SMR groups to 128

[Johan Hovold]

On Tue, Mar 21, 2023 at 02:43:32PM +0530, Manivannan Sadhasivam wrote:
> On some Qualcomm platforms, the hypervisor emulates more than 128 SMR
> (Stream Matching Register) groups. This doesn't conform to the ARM SMMU
> architecture specification which defines the range of 0-127. Moreover, the
> emulated groups don't exhibit the same behavior as the architecture
> supported ones.
> 
> For instance, emulated groups will not detect the quirky behavior of some
> firmware versions intercepting writes to S2CR register, thus skipping the
> quirk implemented in the driver and causing boot crash.
> 
> So let's limit the groups to 128 and issue a notice to users in that case.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
Tested-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH v4] iommu/arm-smmu-qcom: Limit the SMR groups to 128

[Bjorn Andersson]

On Tue, Mar 21, 2023 at 02:43:32PM +0530, Manivannan Sadhasivam wrote:
> On some Qualcomm platforms, the hypervisor emulates more than 128 SMR
> (Stream Matching Register) groups.

As we last week discussed, this isn't at all the case. The hardware has
more than 128 SMRs, it's _not_ emulating additional SMRs.

As pointed out by Robin that might not be according to spec, so it might
be wrong to claim it's compatible with mmu-500. I think limiting the
num_mapping_groups to 128 is a good way to handle this until further
clarity can be acquired.

> This doesn't conform to the ARM SMMU
> architecture specification which defines the range of 0-127. Moreover, the
> emulated groups don't exhibit the same behavior as the architecture
> supported ones.
> 
> For instance, emulated groups will not detect the quirky behavior of some
> firmware versions intercepting writes to S2CR register, thus skipping the
> quirk implemented in the driver and causing boot crash.
> 

From the history of this driver we know that hypervisor traps the writes
to these registers, could it be that the trap doesn't act correctly for
the higher SMRs - for some reason?

> So let's limit the groups to 128 and issue a notice to users in that case.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> Changes in v4:
> 
> * Spun off the SMR limiting part into a separate patch
> * Dropped the quirk rework part as it is not really needed for now
> 
> Changes in v3:
> 
> * Limited num_mapping_groups to 128 as per ARM SMMU spec and removed the
>   check for 128 groups in qcom_smmu_bypass_quirk()
> * Reworded the commit message accordingly
> 
> Changes in v2:
> 
> * Limited the check to 128 groups as per ARM SMMU spec's NUMSMRG range
> * Moved the quirk handling to its own function
> * Collected review tag from Bjorn
> 
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index d1b296b95c86..54f62d409619 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -268,12 +268,26 @@ static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>  
>  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>  {
> -	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
>  	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> +	unsigned int last_s2cr;
>  	u32 reg;
>  	u32 smr;
>  	int i;
>  
> +	/*
> +	 * Limit the number of stream matching groups to 128 as the ARM SMMU
> +	 * architecture specification defines NUMSMRG (Number of Stream Mapping
> +	 * Register Groups) in the range of 0-127, but some Qcom platforms
> +	 * emulate more stream mapping groups.

As discussed, this isn't true.

>                                              And those groups don't exhibit
> +	 * the same behavior as the architecture supported ones.

I share this observation, and I think the patch is reasonable - but not
the commit message and above part of the comment.

Regards,
Bjorn

> +	 */
> +	if (smmu->num_mapping_groups > 128) {
> +		dev_notice(smmu->dev, "\tLimiting the stream matching groups to 128\n");
> +		smmu->num_mapping_groups = 128;
> +	}
> +
> +	last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
> +
>  	/*
>  	 * With some firmware versions writes to S2CR of type FAULT are
>  	 * ignored, and writing BYPASS will end up written as FAULT in the
> -- 
> 2.25.1
>