diff mbox series

[v3] iommu/arm-smmu-qcom: Rework the logic finding the bypass quirk

Message ID 20230314184659.176473-1-manivannan.sadhasivam@linaro.org
State New
Headers
Series [v3] iommu/arm-smmu-qcom: Rework the logic finding the bypass quirk |

Commit Message

Manivannan Sadhasivam March 14, 2023, 6:46 p.m. UTC 
  The logic used to find the quirky firmware that intercepts the writes to
S2CR register to replace bypass type streams with a fault, and ignore the
fault type, is not working with the firmware on newer SoCs like SC8280XP.

The current logic uses the last stream mapping group (num_mapping_groups
- 1) as an index for finding quirky firmware. But on SC8280XP, NUMSMRG
reports a value of 162 due to emulation and the logic is not working for
stream mapping groups > 128. (Note that the ARM SMMU architecture
specification defines NUMSMRG in the range of 0-127).

So the current logic that checks the (162-1)th S2CR entry fails to detect
the quirky firmware on these devices and SMMU triggers invalid context
fault for bypass streams.

To fix this issue, let's limit "num_mapping_groups" to 128 as per ARM SMMU
spec and rework the logic to find the first non-valid (free) stream mapping
register group (SMR) and use that index to access S2CR for detecting the
bypass quirk. If no free groups are available, then just skip the quirk
detection.

While at it, let's move the quirk detection logic to a separate function
and change the local variable name from last_s2cr to free_s2cr.

Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---

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 | 52 ++++++++++++++++++----
 1 file changed, 44 insertions(+), 8 deletions(-)

Comments

Johan Hovold March 15, 2023, 7:37 a.m. UTC | #1 
On Wed, Mar 15, 2023 at 12:16:59AM +0530, Manivannan Sadhasivam wrote:
> The logic used to find the quirky firmware that intercepts the writes to
> S2CR register to replace bypass type streams with a fault, and ignore the
> fault type, is not working with the firmware on newer SoCs like SC8280XP.
> 
> The current logic uses the last stream mapping group (num_mapping_groups
> - 1) as an index for finding quirky firmware. But on SC8280XP, NUMSMRG
> reports a value of 162 due to emulation and the logic is not working for
> stream mapping groups > 128. (Note that the ARM SMMU architecture
> specification defines NUMSMRG in the range of 0-127).
> 
> So the current logic that checks the (162-1)th S2CR entry fails to detect
> the quirky firmware on these devices and SMMU triggers invalid context
> fault for bypass streams.
> 
> To fix this issue, let's limit "num_mapping_groups" to 128 as per ARM SMMU
> spec and rework the logic to find the first non-valid (free) stream mapping
> register group (SMR) and use that index to access S2CR for detecting the
> bypass quirk. If no free groups are available, then just skip the quirk
> detection.
> 
> While at it, let's move the quirk detection logic to a separate function
> and change the local variable name from last_s2cr to free_s2cr.
> 
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> 
> 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

> +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +	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.
> +	 */

Please fix your editor so that it wraps lines at 80 columns, which is
still the preferred (soft) limit.

> +	if (smmu->num_mapping_groups > 128) {
> +		dev_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");

dev_notice() should do since there's nothing a user can do about this.

> +		smmu->num_mapping_groups = 128;
> +	}

So this hunk is really all that is needed to make the current quirk
detection work on sc8280xp. Why not simply stick with the current logic
and use the last group until there is a need for anything more?

Also, should this not be done in arm_smmu_device_cfg_probe() as I
suggested earlier (e.g. to avoid allocating resources for the groups
that will never be used)?

> +
> +	qcom_smmu_bypass_quirk(smmu);
>  
>  	for (i = 0; i < smmu->num_mapping_groups; i++) {
>  		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));

Johan
Manivannan Sadhasivam March 15, 2023, 7:59 a.m. UTC | #2 
On Wed, Mar 15, 2023 at 08:37:32AM +0100, Johan Hovold wrote:
> On Wed, Mar 15, 2023 at 12:16:59AM +0530, Manivannan Sadhasivam wrote:
> > The logic used to find the quirky firmware that intercepts the writes to
> > S2CR register to replace bypass type streams with a fault, and ignore the
> > fault type, is not working with the firmware on newer SoCs like SC8280XP.
> > 
> > The current logic uses the last stream mapping group (num_mapping_groups
> > - 1) as an index for finding quirky firmware. But on SC8280XP, NUMSMRG
> > reports a value of 162 due to emulation and the logic is not working for
> > stream mapping groups > 128. (Note that the ARM SMMU architecture
> > specification defines NUMSMRG in the range of 0-127).
> > 
> > So the current logic that checks the (162-1)th S2CR entry fails to detect
> > the quirky firmware on these devices and SMMU triggers invalid context
> > fault for bypass streams.
> > 
> > To fix this issue, let's limit "num_mapping_groups" to 128 as per ARM SMMU
> > spec and rework the logic to find the first non-valid (free) stream mapping
> > register group (SMR) and use that index to access S2CR for detecting the
> > bypass quirk. If no free groups are available, then just skip the quirk
> > detection.
> > 
> > While at it, let's move the quirk detection logic to a separate function
> > and change the local variable name from last_s2cr to free_s2cr.
> > 
> > Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> > 
> > 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
> 
> > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > +{
> > +	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.
> > +	 */
> 
> Please fix your editor so that it wraps lines at 80 columns, which is
> still the preferred (soft) limit.
> 

If exceeding 80 columns end up making the comment more readable (fewer lines),
then why should we limit ourselves?

> > +	if (smmu->num_mapping_groups > 128) {
> > +		dev_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");
> 
> dev_notice() should do since there's nothing a user can do about this.
> 

Ok.

> > +		smmu->num_mapping_groups = 128;
> > +	}
> 
> So this hunk is really all that is needed to make the current quirk
> detection work on sc8280xp. Why not simply stick with the current logic
> and use the last group until there is a need for anything more?
> 

No! What if the bootloader had set up mapping for 128 groups? In that case
we'll overwrite the last group. It is still required to find the valid group
and use it for quirk detection. If no group is available, we'll skip it.

> Also, should this not be done in arm_smmu_device_cfg_probe() as I
> suggested earlier (e.g. to avoid allocating resources for the groups
> that will never be used)?
> 

I just went with Robin's suggestion on adding this check to cfg_probe() hook.
But I don't have any strong preference either.

Thanks,
Mani

> > +
> > +	qcom_smmu_bypass_quirk(smmu);
> >  
> >  	for (i = 0; i < smmu->num_mapping_groups; i++) {
> >  		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> 
> Johan
Johan Hovold March 15, 2023, 8:38 a.m. UTC | #3 
On Wed, Mar 15, 2023 at 01:29:58PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 15, 2023 at 08:37:32AM +0100, Johan Hovold wrote:

> > > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > > +{
> > > +	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.
> > > +	 */
> > 
> > Please fix your editor so that it wraps lines at 80 columns, which is
> > still the preferred (soft) limit.
> > 
> 
> If exceeding 80 columns end up making the comment more readable (fewer lines),
> then why should we limit ourselves?

Exceeding 80 column for comments does generally not improve readability.

That part of the coding standard has do to with not adding excessive
line breaks to *code*, where it can sometimes impact readability.

> > > +	if (smmu->num_mapping_groups > 128) {
> > > +		dev_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");
> > 
> > dev_notice() should do since there's nothing a user can do about this.
> > 
> 
> Ok.
> 
> > > +		smmu->num_mapping_groups = 128;
> > > +	}
> > 
> > So this hunk is really all that is needed to make the current quirk
> > detection work on sc8280xp. Why not simply stick with the current logic
> > and use the last group until there is a need for anything more?
> > 
> 
> No! What if the bootloader had set up mapping for 128 groups? In that case
> we'll overwrite the last group. It is still required to find the valid group
> and use it for quirk detection. If no group is available, we'll skip it.

Yes, but that's also entirely hypothetical (and could perhaps also be
handled by adding a warning for now).

If you want to rework the quirk handling for this you should at least do
so in a separate patch as it is arguably a separate change from fixing
the current quirk detection for newer SoCs by capping the number of
groups (a minimal fix that could be backported).

Johan
Manivannan Sadhasivam March 15, 2023, 1:37 p.m. UTC | #4 
On Wed, Mar 15, 2023 at 09:38:42AM +0100, Johan Hovold wrote:
> On Wed, Mar 15, 2023 at 01:29:58PM +0530, Manivannan Sadhasivam wrote:
> > On Wed, Mar 15, 2023 at 08:37:32AM +0100, Johan Hovold wrote:
> 
> > > > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > > > +{
> > > > +	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.
> > > > +	 */
> > > 
> > > Please fix your editor so that it wraps lines at 80 columns, which is
> > > still the preferred (soft) limit.
> > > 
> > 
> > If exceeding 80 columns end up making the comment more readable (fewer lines),
> > then why should we limit ourselves?
> 
> Exceeding 80 column for comments does generally not improve readability.
> 

It all depends on the perspective/preference... But I can limit to 80 columns
here.

> That part of the coding standard has do to with not adding excessive
> line breaks to *code*, where it can sometimes impact readability.
> 
> > > > +	if (smmu->num_mapping_groups > 128) {
> > > > +		dev_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");
> > > 
> > > dev_notice() should do since there's nothing a user can do about this.
> > > 
> > 
> > Ok.
> > 
> > > > +		smmu->num_mapping_groups = 128;
> > > > +	}
> > > 
> > > So this hunk is really all that is needed to make the current quirk
> > > detection work on sc8280xp. Why not simply stick with the current logic
> > > and use the last group until there is a need for anything more?
> > > 
> > 
> > No! What if the bootloader had set up mapping for 128 groups? In that case
> > we'll overwrite the last group. It is still required to find the valid group
> > and use it for quirk detection. If no group is available, we'll skip it.
> 
> Yes, but that's also entirely hypothetical (and could perhaps also be
> handled by adding a warning for now).
> 
> If you want to rework the quirk handling for this you should at least do
> so in a separate patch as it is arguably a separate change from fixing
> the current quirk detection for newer SoCs by capping the number of
> groups (a minimal fix that could be backported).
> 

Using a separate patch makes sense to me. Will do that in v4.

Thanks,
Mani

> Johan
Johan Hovold March 15, 2023, 2:12 p.m. UTC | #5 
On Wed, Mar 15, 2023 at 07:07:30PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 15, 2023 at 09:38:42AM +0100, Johan Hovold wrote:
> > On Wed, Mar 15, 2023 at 01:29:58PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Mar 15, 2023 at 08:37:32AM +0100, Johan Hovold wrote:
> > 
> > > > > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > > > > +{
> > > > > +	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.
> > > > > +	 */
> > > > 
> > > > Please fix your editor so that it wraps lines at 80 columns, which is
> > > > still the preferred (soft) limit.
> > > > 
> > > 
> > > If exceeding 80 columns end up making the comment more readable (fewer lines),
> > > then why should we limit ourselves?
> > 
> > Exceeding 80 column for comments does generally not improve readability.
> > 
> 
> It all depends on the perspective/preference... But I can limit to 80 columns
> here.

No, this is not about preference. The soft limit is 80 columns and a
valid reason for breaking that for comments would be, for example, if
you have a long URL or something which does not fit within that width
(and not just personal preference).

Johan
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index d1b296b95c86..301396b29024 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -266,25 +266,42 @@  static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
-static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+static void qcom_smmu_bypass_quirk(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);
-	u32 reg;
-	u32 smr;
+	u32 free_s2cr;
+	u32 reg, smr;
 	int i;
 
+	/*
+	 * Find the first non-valid (free) stream mapping register group and
+	 * use that index to access S2CR for detecting the bypass quirk.
+	 */
+	for (i = 0; i < smmu->num_mapping_groups; i++) {
+		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
+
+		if (!FIELD_GET(ARM_SMMU_SMR_VALID, smr))
+			break;
+	}
+
+	/* If no free stream mapping register group is available, skip the check */
+	if (i == smmu->num_mapping_groups)
+		return;
+
+	free_s2cr = ARM_SMMU_GR0_S2CR(i);
+
 	/*
 	 * With some firmware versions writes to S2CR of type FAULT are
 	 * ignored, and writing BYPASS will end up written as FAULT in the
-	 * register. Perform a write to S2CR to detect if this is the case and
-	 * if so reserve a context bank to emulate bypass streams.
+	 * register. Perform a write to the first free S2CR to detect if
+	 * this is the case and if so reserve a context bank to emulate
+	 * bypass streams.
 	 */
 	reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, S2CR_TYPE_BYPASS) |
 	      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 0xff) |
 	      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, S2CR_PRIVCFG_DEFAULT);
-	arm_smmu_gr0_write(smmu, last_s2cr, reg);
-	reg = arm_smmu_gr0_read(smmu, last_s2cr);
+	arm_smmu_gr0_write(smmu, free_s2cr, reg);
+	reg = arm_smmu_gr0_read(smmu, free_s2cr);
 	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
 		qsmmu->bypass_quirk = true;
 		qsmmu->bypass_cbndx = smmu->num_context_banks - 1;
@@ -296,6 +313,25 @@  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 		reg = FIELD_PREP(ARM_SMMU_CBAR_TYPE, CBAR_TYPE_S1_TRANS_S2_BYPASS);
 		arm_smmu_gr1_write(smmu, ARM_SMMU_GR1_CBAR(qsmmu->bypass_cbndx), reg);
 	}
+}
+
+static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+{
+	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_warn(smmu->dev, "\tLimiting the stream matching groups to 128\n");
+		smmu->num_mapping_groups = 128;
+	}
+
+	qcom_smmu_bypass_quirk(smmu);
 
 	for (i = 0; i < smmu->num_mapping_groups; i++) {
 		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));