diff mbox series

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

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

Commit Message

Manivannan Sadhasivam Feb. 1, 2023, 8:25 a.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, this
logic is not working as the number of stream mapping groups reported by
the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.
So the current logic that checks the (163-1) S2CR entry fails to detect
the quirky firmware on these devices and triggers invalid context fault
for bypass streams.

To fix this issue, 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.

This also warrants a change in variable name from last_s2cr to free_s2cr.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

Comments

Bjorn Andersson Feb. 10, 2023, 5:47 p.m. UTC | #1 
On Wed, Feb 01, 2023 at 01:55:00PM +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, this
> logic is not working as the number of stream mapping groups reported by
> the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.
> So the current logic that checks the (163-1) S2CR entry fails to detect
> the quirky firmware on these devices and triggers invalid context fault
> for bypass streams.
> 
> To fix this issue, 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.
> 
> This also warrants a change in variable name from last_s2cr to free_s2cr.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 78fc0e1bf215..4104f81b8d8f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -267,23 +267,37 @@ 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);
> +	u32 free_s2cr;
>  	u32 reg;
>  	u32 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;
> +	}
> +
> +	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;
> -- 
> 2.25.1
>
Johan Hovold Feb. 13, 2023, 4:43 p.m. UTC | #2 
On Wed, Feb 01, 2023 at 01:55:00PM +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, this
> logic is not working as the number of stream mapping groups reported by
> the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.

NUMSMRG read back as 162 here, both on my CRD and X13s. Was '163' a typo
or a real difference?

> So the current logic that checks the (163-1) S2CR entry fails to detect
> the quirky firmware on these devices and triggers invalid context fault
> for bypass streams.
> 
> To fix this issue, 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.

So while this works for the quirk detection, shouldn't we also do
something about that bogus NUMSMRG value? At least cap it at 128, which
appears to be the maximum according to the specification, for example,
by clearing bit 7 when any of the lower bits are set?

That would give us 35 (or 36) groups and working quirk detection with
just the following smaller patch:

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 2ff7a72cf377..0f564a86c352 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1744,6 +1744,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
                        return -ENODEV;
                }
 
+               if (size > 0x80) {
+                       dev_warn(smmu->dev,
+                                "invalid number of SMR groups, clearing bit 7\n");
+                       size -= 0x80;
+               }
+
                /* Zero-initialised to mark as invalid */
                smmu->smrs = devm_kcalloc(smmu->dev, size, sizeof(*smmu->smrs),
                                          GFP_KERNEL);

I also verified that using index 127 (group 128) for the quirk detection
works on my CRD, while the invalid index 128 fails (as do index 161
which would currently be used).

> This also warrants a change in variable name from last_s2cr to free_s2cr.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 78fc0e1bf215..4104f81b8d8f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -267,23 +267,37 @@ 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);
> +	u32 free_s2cr;
>  	u32 reg;
>  	u32 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;
> +	}
> +
> +	free_s2cr = ARM_SMMU_GR0_S2CR(i);

In the unlikely event that there is no free group this would access an
invalid index.

> +
>  	/*
>  	 * 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;

Johan
Manivannan Sadhasivam Feb. 14, 2023, 7:53 a.m. UTC | #3 
On Mon, Feb 13, 2023 at 05:43:56PM +0100, Johan Hovold wrote:
> On Wed, Feb 01, 2023 at 01:55:00PM +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, this
> > logic is not working as the number of stream mapping groups reported by
> > the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.
> 
> NUMSMRG read back as 162 here, both on my CRD and X13s. Was '163' a typo
> or a real difference?
> 

Ah yes, it is 162 indeed. Sorry, typo!

> > So the current logic that checks the (163-1) S2CR entry fails to detect
> > the quirky firmware on these devices and triggers invalid context fault
> > for bypass streams.
> > 
> > To fix this issue, 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.
> 
> So while this works for the quirk detection, shouldn't we also do
> something about that bogus NUMSMRG value? At least cap it at 128, which
> appears to be the maximum according to the specification, for example,
> by clearing bit 7 when any of the lower bits are set?
> 
> That would give us 35 (or 36) groups and working quirk detection with
> just the following smaller patch:
> 

I'm not certain if the value is bogus or not. It is clear that the spec
specifies 128 as the max but internal qcom document shows that they indeed
set 162 on purpose in the hypervisor.

So until we get a clear view on that, I'd not cap it.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 2ff7a72cf377..0f564a86c352 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1744,6 +1744,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
>                         return -ENODEV;
>                 }
>  
> +               if (size > 0x80) {
> +                       dev_warn(smmu->dev,
> +                                "invalid number of SMR groups, clearing bit 7\n");
> +                       size -= 0x80;
> +               }
> +
>                 /* Zero-initialised to mark as invalid */
>                 smmu->smrs = devm_kcalloc(smmu->dev, size, sizeof(*smmu->smrs),
>                                           GFP_KERNEL);
> 
> I also verified that using index 127 (group 128) for the quirk detection
> works on my CRD, while the invalid index 128 fails (as do index 161
> which would currently be used).
> 
> > This also warrants a change in variable name from last_s2cr to free_s2cr.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > index 78fc0e1bf215..4104f81b8d8f 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -267,23 +267,37 @@ 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);
> > +	u32 free_s2cr;
> >  	u32 reg;
> >  	u32 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;
> > +	}
> > +
> > +	free_s2cr = ARM_SMMU_GR0_S2CR(i);
> 
> In the unlikely event that there is no free group this would access an
> invalid index.
> 

Hmm, theoretically yes. But what would be the plan of action if that happens?
Should we just bail out with error or skip the quirk detection?

Thanks,
Mani

> > +
> >  	/*
> >  	 * 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;
> 
> Johan
Johan Hovold Feb. 14, 2023, 9:07 a.m. UTC | #4 
On Tue, Feb 14, 2023 at 01:23:12PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Feb 13, 2023 at 05:43:56PM +0100, Johan Hovold wrote:
> > On Wed, Feb 01, 2023 at 01:55:00PM +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, this
> > > logic is not working as the number of stream mapping groups reported by
> > > the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.
> > 
> > NUMSMRG read back as 162 here, both on my CRD and X13s. Was '163' a typo
> > or a real difference?
> > 
> 
> Ah yes, it is 162 indeed. Sorry, typo!
> 
> > > So the current logic that checks the (163-1) S2CR entry fails to detect
> > > the quirky firmware on these devices and triggers invalid context fault
> > > for bypass streams.
> > > 
> > > To fix this issue, 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.
> > 
> > So while this works for the quirk detection, shouldn't we also do
> > something about that bogus NUMSMRG value? At least cap it at 128, which
> > appears to be the maximum according to the specification, for example,
> > by clearing bit 7 when any of the lower bits are set?
> > 
> > That would give us 35 (or 36) groups and working quirk detection with
> > just the following smaller patch:
> > 
> 
> I'm not certain if the value is bogus or not. It is clear that the spec
> specifies 128 as the max but internal qcom document shows that they indeed
> set 162 on purpose in the hypervisor.
>
> So until we get a clear view on that, I'd not cap it.

But if we fault as soon as we try to do something with those register
groups above 128 that also violate the spec, it doesn't seem right to
trust the fw value here.

Clarification from Qualcomm would be good either way, but if they are
indication that it's not just a bug that has left bit 7 set then
limiting to 128 also seems reasonable (i.e. not by clearing the high
bit, but by using the minimum of 128 and size below).

> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 2ff7a72cf377..0f564a86c352 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -1744,6 +1744,12 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
> >                         return -ENODEV;
> >                 }
> >  
> > +               if (size > 0x80) {
> > +                       dev_warn(smmu->dev,
> > +                                "invalid number of SMR groups, clearing bit 7\n");
> > +                       size -= 0x80;
> > +               }
> > +
> >                 /* Zero-initialised to mark as invalid */
> >                 smmu->smrs = devm_kcalloc(smmu->dev, size, sizeof(*smmu->smrs),
> >                                           GFP_KERNEL);
> > 
> > I also verified that using index 127 (group 128) for the quirk detection
> > works on my CRD, while the invalid index 128 fails (as do index 161
> > which would currently be used).
> > 
> > > This also warrants a change in variable name from last_s2cr to free_s2cr.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > index 78fc0e1bf215..4104f81b8d8f 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > @@ -267,23 +267,37 @@ 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);
> > > +	u32 free_s2cr;
> > >  	u32 reg;
> > >  	u32 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;
> > > +	}
> > > +
> > > +	free_s2cr = ARM_SMMU_GR0_S2CR(i);
> > 
> > In the unlikely event that there is no free group this would access an
> > invalid index.
> > 
> 
> Hmm, theoretically yes. But what would be the plan of action if that happens?
> Should we just bail out with error or skip the quirk detection?

Yes, skipping quirk detection seems preferable to crashing systems that
don't need the quirk.

Johan
Johan Hovold Feb. 15, 2023, 1:08 p.m. UTC | #5 
On Tue, Feb 14, 2023 at 10:07:36AM +0100, Johan Hovold wrote:
> On Tue, Feb 14, 2023 at 01:23:12PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Feb 13, 2023 at 05:43:56PM +0100, Johan Hovold wrote:
> > > On Wed, Feb 01, 2023 at 01:55:00PM +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, this
> > > > logic is not working as the number of stream mapping groups reported by
> > > > the SMMU (163 as on the SC8280XP-CRD device) is not valid for some reason.
> > > 
> > > NUMSMRG read back as 162 here, both on my CRD and X13s. Was '163' a typo
> > > or a real difference?
> > > 
> > 
> > Ah yes, it is 162 indeed. Sorry, typo!
> > 
> > > > So the current logic that checks the (163-1) S2CR entry fails to detect
> > > > the quirky firmware on these devices and triggers invalid context fault
> > > > for bypass streams.
> > > > 
> > > > To fix this issue, 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.
> > > 
> > > So while this works for the quirk detection, shouldn't we also do
> > > something about that bogus NUMSMRG value? At least cap it at 128, which
> > > appears to be the maximum according to the specification, for example,
> > > by clearing bit 7 when any of the lower bits are set?
> > > 
> > > That would give us 35 (or 36) groups and working quirk detection with
> > > just the following smaller patch:
> > > 
> > 
> > I'm not certain if the value is bogus or not. It is clear that the spec
> > specifies 128 as the max but internal qcom document shows that they indeed
> > set 162 on purpose in the hypervisor.
> >
> > So until we get a clear view on that, I'd not cap it.
> 
> But if we fault as soon as we try to do something with those register
> groups above 128 that also violate the spec, it doesn't seem right to
> trust the fw value here.

I realised that the fault is due to the quirk not being detected
properly as writes to groups above index 127 apparently succeeds
(including out-of-bounds index 162):

	qcom_smmu_cfg_probe - index = 127, reg = 200ff, type = 02
	qcom_smmu_cfg_probe - index = 128, reg = 100ff, type = 01
	qcom_smmu_cfg_probe - index = 161, reg = 100ff, type = 01
	qcom_smmu_cfg_probe - index = 162, reg = 100ff, type = 01

So leaving smmu->num_mapping_groups unchanged for now and using the
first available group for the detection indeed seems like the right
thing to do here (alternatively, never use an index above 127).

But perhaps you can update to commit message to reflect this finding
(i.e. that the num groups value is probably bogus, and that you at least
need to use an index < 128 for quirk detection).

By the way, I noticed that the number of groups is reported as 162 on
the sa8295p-adp as well.

> > > > This also warrants a change in variable name from last_s2cr to free_s2cr.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 +++++++++++++++++-----
> > > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > index 78fc0e1bf215..4104f81b8d8f 100644
> > > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > > > @@ -267,23 +267,37 @@ 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);
> > > > +	u32 free_s2cr;
> > > >  	u32 reg;
> > > >  	u32 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;
> > > > +	}
> > > > +
> > > > +	free_s2cr = ARM_SMMU_GR0_S2CR(i);
> > > 
> > > In the unlikely event that there is no free group this would access an
> > > invalid index.
> > > 
> > 
> > Hmm, theoretically yes. But what would be the plan of action if that happens?
> > Should we just bail out with error or skip the quirk detection?
> 
> Yes, skipping quirk detection seems preferable to crashing systems that
> don't need the quirk.

Perhaps you can move the quirk handling to its own function and simply
return early in case there is no free group.

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 78fc0e1bf215..4104f81b8d8f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -267,23 +267,37 @@  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);
+	u32 free_s2cr;
 	u32 reg;
 	u32 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;
+	}
+
+	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;