[PATCH v4 0/3] iommu/arm-smmu-qcom: Support maintaining bootloader mappings

[PATCH v4 0/3] iommu/arm-smmu-qcom: Support maintaining bootloader mappings

[Bjorn Andersson]

This is the fourth attempt of inheriting the stream mapping for the framebuffer
on many Qualcomm platforms, in order to not hit catastrophic faults during
arm-smmu initialization.

The new approach does, based on Robin's suggestion, take a much more direct
approach with the allocation of a context bank for bypass emulation and use of
this context bank pretty much isolated to the Qualcomm specific implementation.

As before the patchset has been tested to boot DB845c (with splash screen) and
Lenovo Yoga C630 (with EFI framebuffer).

Bjorn Andersson (3):
  iommu/arm-smmu: Allow implementation specific write_s2cr
  iommu/arm-smmu-qcom: Read back stream mappings
  iommu/arm-smmu-qcom: Implement S2CR quirk

 drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 92 ++++++++++++++++++++++
 drivers/iommu/arm/arm-smmu/arm-smmu.c      | 22 ++++--
 drivers/iommu/arm/arm-smmu/arm-smmu.h      |  1 +
 3 files changed, 107 insertions(+), 8 deletions(-)

-- 
2.28.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 1/3] iommu/arm-smmu: Allow implementation specific write_s2cr

[Bjorn Andersson]

The firmware found in some Qualcomm platforms intercepts writes to the
S2CR register in order to replace the BYPASS type with FAULT. Further
more it treats faults at this level as catastrophic and restarts the
device.

Add support for providing implementation specific versions of the S2CR
write function, to allow the Qualcomm driver to work around this
behavior.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- New patch

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 22 ++++++++++++++--------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 +
 2 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index dad7fa86fbd4..ed3f0428c110 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -929,14 +929,20 @@ static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
 static void arm_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
 {
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
-	u32 reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
-		  FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
-		  FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
-
-	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
-	    smmu->smrs[idx].valid)
-		reg |= ARM_SMMU_S2CR_EXIDVALID;
-	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
+	u32 reg;
+
+	if (smmu->impl && smmu->impl->write_s2cr) {
+		smmu->impl->write_s2cr(smmu, idx);
+	} else {
+		reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
+		      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
+		      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
+
+		if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
+		    smmu->smrs[idx].valid)
+			reg |= ARM_SMMU_S2CR_EXIDVALID;
+		arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
+	}
 }
 
 static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int idx)
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 1a746476927c..b71647eaa319 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -436,6 +436,7 @@ struct arm_smmu_impl {
 	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
 				  struct arm_smmu_device *smmu,
 				  struct device *dev, int start);
+	void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
 };
 
 #define INVALID_SMENDX			-1
-- 
2.28.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 2/3] iommu/arm-smmu-qcom: Read back stream mappings

[Bjorn Andersson]

The Qualcomm boot loader configures stream mapping for the peripherals
that it accesses and in particular it sets up the stream mapping for the
display controller to be allowed to scan out a splash screen or EFI
framebuffer.

Read back the stream mappings during initialization and make the
arm-smmu driver maintain the streams in bypass mode.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Extracted from different patch in v3.
- Now configures the stream as BYPASS, rather than translate, which should work
  for platforms with working S2CR handling as well.

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

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index be4318044f96..0089048342dd 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -23,6 +23,29 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
 	{ }
 };
 
+static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+{
+	u32 smr;
+	int i;
+
+	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)) {
+			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
+			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
+			smmu->smrs[i].valid = true;
+
+			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+			smmu->s2crs[i].cbndx = 0xff;
+			smmu->s2crs[i].count++;
+		}
+	}
+
+	return 0;
+}
+
 static int qcom_smmu_def_domain_type(struct device *dev)
 {
 	const struct of_device_id *match =
@@ -61,6 +84,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
 }
 
 static const struct arm_smmu_impl qcom_smmu_impl = {
+	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
 	.reset = qcom_smmu500_reset,
 };
-- 
2.28.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v4 3/3] iommu/arm-smmu-qcom: Implement S2CR quirk

[Bjorn Andersson]

The firmware found in some Qualcomm platforms intercepts writes to S2CR
in order to replace bypass type streams with fault; and ignore S2CR
updates of type fault.

Detect this behavior and implement a custom write_s2cr function in order
to trick the firmware into supporting bypass streams by the means of
configuring the stream for translation using a reserved and disabled
context bank.

Also circumvent the problem of configuring faulting streams by
configuring the stream as bypass.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v3:
- Move the reservation of the "identity context bank" to the Qualcomm specific
  implementation.
- Implement the S2CR quirk with the newly introduced write_s2cr callback.

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

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
index 0089048342dd..c0f42d6a6e01 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
@@ -10,8 +10,14 @@
 
 struct qcom_smmu {
 	struct arm_smmu_device smmu;
+	bool bypass_cbndx;
 };
 
+static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
+{
+	return container_of(smmu, struct qcom_smmu, smmu);
+}
+
 static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
 	{ .compatible = "qcom,adreno" },
 	{ .compatible = "qcom,mdp4" },
@@ -25,9 +31,32 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
 
 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 reg;
 	u32 smr;
 	int 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.
+	 */
+	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);
+	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
+		qsmmu->bypass_cbndx = smmu->num_context_banks - 1;
+
+		set_bit(qsmmu->bypass_cbndx, smmu->context_map);
+
+		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);
+	}
+
 	for (i = 0; i < smmu->num_mapping_groups; i++) {
 		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
 
@@ -46,6 +75,44 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
+{
+	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
+	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
+	u32 cbndx = s2cr->cbndx;
+	u32 type = s2cr->type;
+	u32 reg;
+
+	if (qsmmu->bypass_cbndx) {
+		if (type == S2CR_TYPE_BYPASS) {
+			/*
+			 * Firmware with quirky S2CR handling will substitute
+			 * BYPASS writes with FAULT, so point the stream to the
+			 * reserved context bank and ask for translation on the
+			 * stream
+			 */
+			type = S2CR_TYPE_TRANS;
+			cbndx = qsmmu->bypass_cbndx;
+		} else if (type == S2CR_TYPE_FAULT) {
+			/*
+			 * Firmware with quirky S2CR handling will ignore FAULT
+			 * writes, so trick it to write FAULT by asking for a
+			 * BYPASS.
+			 */
+			type = S2CR_TYPE_BYPASS;
+			cbndx = 0xff;
+		}
+	}
+
+	reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, type) |
+	      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, cbndx) |
+	      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
+
+	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs && smmu->smrs[idx].valid)
+		reg |= ARM_SMMU_S2CR_EXIDVALID;
+	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
+}
+
 static int qcom_smmu_def_domain_type(struct device *dev)
 {
 	const struct of_device_id *match =
@@ -87,6 +154,7 @@ static const struct arm_smmu_impl qcom_smmu_impl = {
 	.cfg_probe = qcom_smmu_cfg_probe,
 	.def_domain_type = qcom_smmu_def_domain_type,
 	.reset = qcom_smmu500_reset,
+	.write_s2cr = qcom_smmu_write_s2cr,
 };
 
 struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
-- 
2.28.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/3] iommu/arm-smmu: Allow implementation specific write_s2cr

[Robin Murphy]

On 2020-10-17 05:39, Bjorn Andersson wrote:
> The firmware found in some Qualcomm platforms intercepts writes to the
> S2CR register in order to replace the BYPASS type with FAULT. Further
> more it treats faults at this level as catastrophic and restarts the
> device.
> 
> Add support for providing implementation specific versions of the S2CR
> write function, to allow the Qualcomm driver to work around this
> behavior.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v3:
> - New patch
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 22 ++++++++++++++--------
>   drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 +
>   2 files changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index dad7fa86fbd4..ed3f0428c110 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -929,14 +929,20 @@ static void arm_smmu_write_smr(struct arm_smmu_device *smmu, int idx)
>   static void arm_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
>   {
>   	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> -	u32 reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
> -		  FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
> -		  FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> -
> -	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
> -	    smmu->smrs[idx].valid)
> -		reg |= ARM_SMMU_S2CR_EXIDVALID;
> -	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> +	u32 reg;
> +
> +	if (smmu->impl && smmu->impl->write_s2cr) {
> +		smmu->impl->write_s2cr(smmu, idx);

Nit: just add an early return here to avoid reindenting the whole 
function. Otherwise this looks like a reasonable level of abstraction to 
me - we'll still have plenty of flexibility to adjust things in future 
if necessary.

With that change,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> +	} else {
> +		reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, s2cr->type) |
> +		      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, s2cr->cbndx) |
> +		      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> +
> +		if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs &&
> +		    smmu->smrs[idx].valid)
> +			reg |= ARM_SMMU_S2CR_EXIDVALID;
> +		arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> +	}
>   }
>   
>   static void arm_smmu_write_sme(struct arm_smmu_device *smmu, int idx)
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> index 1a746476927c..b71647eaa319 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
> @@ -436,6 +436,7 @@ struct arm_smmu_impl {
>   	int (*alloc_context_bank)(struct arm_smmu_domain *smmu_domain,
>   				  struct arm_smmu_device *smmu,
>   				  struct device *dev, int start);
> +	void (*write_s2cr)(struct arm_smmu_device *smmu, int idx);
>   };
>   
>   #define INVALID_SMENDX			-1
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/arm-smmu-qcom: Read back stream mappings

[Robin Murphy]

On 2020-10-17 05:39, Bjorn Andersson wrote:
> The Qualcomm boot loader configures stream mapping for the peripherals
> that it accesses and in particular it sets up the stream mapping for the
> display controller to be allowed to scan out a splash screen or EFI
> framebuffer.
> 
> Read back the stream mappings during initialization and make the
> arm-smmu driver maintain the streams in bypass mode.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v3:
> - Extracted from different patch in v3.
> - Now configures the stream as BYPASS, rather than translate, which should work
>    for platforms with working S2CR handling as well.
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 ++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index be4318044f96..0089048342dd 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -23,6 +23,29 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>   	{ }
>   };
>   
> +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +	u32 smr;
> +	int i;
> +
> +	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)) {
> +			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> +			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> +			smmu->smrs[i].valid = true;
> +
> +			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> +			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> +			smmu->s2crs[i].cbndx = 0xff;
> +			smmu->s2crs[i].count++;

FWIW I don't think you actually need to adjust the count here - the SMR 
being valid should already prevent the whole SME from being reassigned 
until the display probes, at which point it should "take over" the SMR 
based on matching values and claim the "initial" refcount. After that 
you're back into the standard flow. It might be a little unintuitive to 
have something in a valid but "unused" state, but arguably it's entirely 
accurate in terms of the software abstraction here.

Otherwise, you end up making boot-time SMRs - so potentially all SMRs 
after a kexec - effectively immutable, since even after Linux has taken 
control of the whole system such that they *could* be reassigned safely, 
there's still this undroppable refcount hanging around preventing it.

That said, for a mobile SoC use-case if you have enough SMRs for all 
your stream IDs and don't have any kind of device hotplug, that 
restriction shouldn't make much difference in practice, so I'm not too 
concerned either way. Otherwise this is as nice and tidy as I'd hoped :)

Robin.

> +		}
> +	}
> +
> +	return 0;
> +}
> +
>   static int qcom_smmu_def_domain_type(struct device *dev)
>   {
>   	const struct of_device_id *match =
> @@ -61,6 +84,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
>   }
>   
>   static const struct arm_smmu_impl qcom_smmu_impl = {
> +	.cfg_probe = qcom_smmu_cfg_probe,
>   	.def_domain_type = qcom_smmu_def_domain_type,
>   	.reset = qcom_smmu500_reset,
>   };
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/arm-smmu-qcom: Implement S2CR quirk

[Robin Murphy]

On 2020-10-17 05:39, Bjorn Andersson wrote:
> The firmware found in some Qualcomm platforms intercepts writes to S2CR
> in order to replace bypass type streams with fault; and ignore S2CR
> updates of type fault.
> 
> Detect this behavior and implement a custom write_s2cr function in order
> to trick the firmware into supporting bypass streams by the means of
> configuring the stream for translation using a reserved and disabled
> context bank.
> 
> Also circumvent the problem of configuring faulting streams by
> configuring the stream as bypass.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v3:
> - Move the reservation of the "identity context bank" to the Qualcomm specific
>    implementation.
> - Implement the S2CR quirk with the newly introduced write_s2cr callback.
> 
>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 68 ++++++++++++++++++++++
>   1 file changed, 68 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> index 0089048342dd..c0f42d6a6e01 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> @@ -10,8 +10,14 @@
>   
>   struct qcom_smmu {
>   	struct arm_smmu_device smmu;
> +	bool bypass_cbndx;

Nit: variables named "*ndx" usually hold an actual index value. If it's 
just a flag then maybe name it something like "use_bypass_context"?

>   };
>   
> +static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> +{
> +	return container_of(smmu, struct qcom_smmu, smmu);
> +}
> +
>   static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>   	{ .compatible = "qcom,adreno" },
>   	{ .compatible = "qcom,mdp4" },
> @@ -25,9 +31,32 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
>   
>   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 reg;
>   	u32 smr;
>   	int 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.
> +	 */
> +	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);
> +	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
> +		qsmmu->bypass_cbndx = smmu->num_context_banks - 1;

Oh, so maybe the name is in fact OK but the type is wrong :/

I guess this does happens to work out, but for the wrong reason...

> +
> +		set_bit(qsmmu->bypass_cbndx, smmu->context_map);
> +
> +		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);
> +	}
> +
>   	for (i = 0; i < smmu->num_mapping_groups; i++) {
>   		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
>   
> @@ -46,6 +75,44 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>   	return 0;
>   }
>   
> +static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
> +{
> +	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> +	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> +	u32 cbndx = s2cr->cbndx;
> +	u32 type = s2cr->type;
> +	u32 reg;
> +
> +	if (qsmmu->bypass_cbndx) {

Note that if we are talking indices here then 0 would be perfectly valid 
in general. This works out OK in practice given that we're always 
reserving the last implemented context above, and if we ever *did* only 
have one such that index 0 is the last then we're going to have a bad 
time either way, but it's not necessarily the most obvious.

> +		if (type == S2CR_TYPE_BYPASS) {
> +			/*
> +			 * Firmware with quirky S2CR handling will substitute
> +			 * BYPASS writes with FAULT, so point the stream to the
> +			 * reserved context bank and ask for translation on the
> +			 * stream
> +			 */
> +			type = S2CR_TYPE_TRANS;
> +			cbndx = qsmmu->bypass_cbndx;
> +		} else if (type == S2CR_TYPE_FAULT) {
> +			/*
> +			 * Firmware with quirky S2CR handling will ignore FAULT
> +			 * writes, so trick it to write FAULT by asking for a
> +			 * BYPASS.
> +			 */
> +			type = S2CR_TYPE_BYPASS;

Ha, that's brilliant :)

> +			cbndx = 0xff;
> +		}
> +	}
> +
> +	reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, type) |
> +	      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, cbndx) |
> +	      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> +
> +	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs && smmu->smrs[idx].valid)
> +		reg |= ARM_SMMU_S2CR_EXIDVALID;

Does any of your hardware actually have EXIDS implemented? No big deal 
if you only want this here "just in case", I'm just curious as I was 
under then impression that it was essentially a ThunderX special.

Other than sorting out bypass_cbndx one way or the other, overall this 
is now looking about as nice as it ever could - thanks for persevering!

Robin.

> +	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> +}
> +
>   static int qcom_smmu_def_domain_type(struct device *dev)
>   {
>   	const struct of_device_id *match =
> @@ -87,6 +154,7 @@ static const struct arm_smmu_impl qcom_smmu_impl = {
>   	.cfg_probe = qcom_smmu_cfg_probe,
>   	.def_domain_type = qcom_smmu_def_domain_type,
>   	.reset = qcom_smmu500_reset,
> +	.write_s2cr = qcom_smmu_write_s2cr,
>   };
>   
>   struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 2/3] iommu/arm-smmu-qcom: Read back stream mappings

[Bjorn Andersson]

On Mon 19 Oct 09:03 CDT 2020, Robin Murphy wrote:

> On 2020-10-17 05:39, Bjorn Andersson wrote:
> > The Qualcomm boot loader configures stream mapping for the peripherals
> > that it accesses and in particular it sets up the stream mapping for the
> > display controller to be allowed to scan out a splash screen or EFI
> > framebuffer.
> > 
> > Read back the stream mappings during initialization and make the
> > arm-smmu driver maintain the streams in bypass mode.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v3:
> > - Extracted from different patch in v3.
> > - Now configures the stream as BYPASS, rather than translate, which should work
> >    for platforms with working S2CR handling as well.
> > 
> >   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 24 ++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > index be4318044f96..0089048342dd 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -23,6 +23,29 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
> >   	{ }
> >   };
> > +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> > +{
> > +	u32 smr;
> > +	int i;
> > +
> > +	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)) {
> > +			smmu->smrs[i].id = FIELD_GET(ARM_SMMU_SMR_ID, smr);
> > +			smmu->smrs[i].mask = FIELD_GET(ARM_SMMU_SMR_MASK, smr);
> > +			smmu->smrs[i].valid = true;
> > +
> > +			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
> > +			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
> > +			smmu->s2crs[i].cbndx = 0xff;
> > +			smmu->s2crs[i].count++;
> 
> FWIW I don't think you actually need to adjust the count here - the SMR
> being valid should already prevent the whole SME from being reassigned until
> the display probes, at which point it should "take over" the SMR based on
> matching values and claim the "initial" refcount. After that you're back
> into the standard flow. It might be a little unintuitive to have something
> in a valid but "unused" state, but arguably it's entirely accurate in terms
> of the software abstraction here.
> 
> Otherwise, you end up making boot-time SMRs - so potentially all SMRs after
> a kexec - effectively immutable, since even after Linux has taken control of
> the whole system such that they *could* be reassigned safely, there's still
> this undroppable refcount hanging around preventing it.
> 

I did increment the count here to make sure the stream mapping do
survive a probe deferral of the display controller (which is rather
common when you have some bridge chip hanging off it).

But after digging through the code further I've convinced myself that
the sme won't be freed while the device is pending probe deferral.

So I will drop this.

> That said, for a mobile SoC use-case if you have enough SMRs for all your
> stream IDs and don't have any kind of device hotplug, that restriction
> shouldn't make much difference in practice, so I'm not too concerned either
> way. Otherwise this is as nice and tidy as I'd hoped :)

I agree, I'm quite happy with where we are now!

Thanks,
Bjorn

> 
> Robin.
> 
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   static int qcom_smmu_def_domain_type(struct device *dev)
> >   {
> >   	const struct of_device_id *match =
> > @@ -61,6 +84,7 @@ static int qcom_smmu500_reset(struct arm_smmu_device *smmu)
> >   }
> >   static const struct arm_smmu_impl qcom_smmu_impl = {
> > +	.cfg_probe = qcom_smmu_cfg_probe,
> >   	.def_domain_type = qcom_smmu_def_domain_type,
> >   	.reset = qcom_smmu500_reset,
> >   };
> > 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 3/3] iommu/arm-smmu-qcom: Implement S2CR quirk

[Bjorn Andersson]

On Mon 19 Oct 09:04 CDT 2020, Robin Murphy wrote:

> On 2020-10-17 05:39, Bjorn Andersson wrote:
> > The firmware found in some Qualcomm platforms intercepts writes to S2CR
> > in order to replace bypass type streams with fault; and ignore S2CR
> > updates of type fault.
> > 
> > Detect this behavior and implement a custom write_s2cr function in order
> > to trick the firmware into supporting bypass streams by the means of
> > configuring the stream for translation using a reserved and disabled
> > context bank.
> > 
> > Also circumvent the problem of configuring faulting streams by
> > configuring the stream as bypass.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v3:
> > - Move the reservation of the "identity context bank" to the Qualcomm specific
> >    implementation.
> > - Implement the S2CR quirk with the newly introduced write_s2cr callback.
> > 
> >   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 68 ++++++++++++++++++++++
> >   1 file changed, 68 insertions(+)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > index 0089048342dd..c0f42d6a6e01 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
> > @@ -10,8 +10,14 @@
> >   struct qcom_smmu {
> >   	struct arm_smmu_device smmu;
> > +	bool bypass_cbndx;
> 
> Nit: variables named "*ndx" usually hold an actual index value. If it's just
> a flag then maybe name it something like "use_bypass_context"?
> 
> >   };
> > +static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
> > +{
> > +	return container_of(smmu, struct qcom_smmu, smmu);
> > +}
> > +
> >   static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
> >   	{ .compatible = "qcom,adreno" },
> >   	{ .compatible = "qcom,mdp4" },
> > @@ -25,9 +31,32 @@ static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = {
> >   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 reg;
> >   	u32 smr;
> >   	int 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.
> > +	 */
> > +	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);
> > +	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
> > +		qsmmu->bypass_cbndx = smmu->num_context_banks - 1;
> 
> Oh, so maybe the name is in fact OK but the type is wrong :/
> 
> I guess this does happens to work out, but for the wrong reason...
> 

Odd, but "it works on my machine"... Sorry about that.

> > +
> > +		set_bit(qsmmu->bypass_cbndx, smmu->context_map);
> > +
> > +		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);
> > +	}
> > +
> >   	for (i = 0; i < smmu->num_mapping_groups; i++) {
> >   		smr = arm_smmu_gr0_read(smmu, ARM_SMMU_GR0_SMR(i));
> > @@ -46,6 +75,44 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >   	return 0;
> >   }
> > +static void qcom_smmu_write_s2cr(struct arm_smmu_device *smmu, int idx)
> > +{
> > +	struct arm_smmu_s2cr *s2cr = smmu->s2crs + idx;
> > +	struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
> > +	u32 cbndx = s2cr->cbndx;
> > +	u32 type = s2cr->type;
> > +	u32 reg;
> > +
> > +	if (qsmmu->bypass_cbndx) {
> 
> Note that if we are talking indices here then 0 would be perfectly valid in
> general. This works out OK in practice given that we're always reserving the
> last implemented context above, and if we ever *did* only have one such that
> index 0 is the last then we're going to have a bad time either way, but it's
> not necessarily the most obvious.
> 

Right. In the event that we have a SMMU instance with a single context
bank hitting this quirk would probably be bad regardless, as the
cfg_probe would have just stolen the only available context bank for
bypass purposes :)

But I've updated this to keep track of the need for bypass separate from
the index. We don't have a lot of SMMU controllers, so it's not a big
waste.

> > +		if (type == S2CR_TYPE_BYPASS) {
> > +			/*
> > +			 * Firmware with quirky S2CR handling will substitute
> > +			 * BYPASS writes with FAULT, so point the stream to the
> > +			 * reserved context bank and ask for translation on the
> > +			 * stream
> > +			 */
> > +			type = S2CR_TYPE_TRANS;
> > +			cbndx = qsmmu->bypass_cbndx;
> > +		} else if (type == S2CR_TYPE_FAULT) {
> > +			/*
> > +			 * Firmware with quirky S2CR handling will ignore FAULT
> > +			 * writes, so trick it to write FAULT by asking for a
> > +			 * BYPASS.
> > +			 */
> > +			type = S2CR_TYPE_BYPASS;
> 
> Ha, that's brilliant :)
> 
> > +			cbndx = 0xff;
> > +		}
> > +	}
> > +
> > +	reg = FIELD_PREP(ARM_SMMU_S2CR_TYPE, type) |
> > +	      FIELD_PREP(ARM_SMMU_S2CR_CBNDX, cbndx) |
> > +	      FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, s2cr->privcfg);
> > +
> > +	if (smmu->features & ARM_SMMU_FEAT_EXIDS && smmu->smrs && smmu->smrs[idx].valid)
> > +		reg |= ARM_SMMU_S2CR_EXIDVALID;
> 
> Does any of your hardware actually have EXIDS implemented? No big deal if
> you only want this here "just in case", I'm just curious as I was under then
> impression that it was essentially a ThunderX special.
> 

I tested a couple of platforms and I don't think we do. Dropping it
makes the code slightly better to read, so let's do that.

> Other than sorting out bypass_cbndx one way or the other, overall this is
> now looking about as nice as it ever could - thanks for persevering!
> 

Thank you,
Bjorn

> Robin.
> 
> > +	arm_smmu_gr0_write(smmu, ARM_SMMU_GR0_S2CR(idx), reg);
> > +}
> > +
> >   static int qcom_smmu_def_domain_type(struct device *dev)
> >   {
> >   	const struct of_device_id *match =
> > @@ -87,6 +154,7 @@ static const struct arm_smmu_impl qcom_smmu_impl = {
> >   	.cfg_probe = qcom_smmu_cfg_probe,
> >   	.def_domain_type = qcom_smmu_def_domain_type,
> >   	.reset = qcom_smmu500_reset,
> > +	.write_s2cr = qcom_smmu_write_s2cr,
> >   };
> >   struct arm_smmu_device *qcom_smmu_impl_init(struct arm_smmu_device *smmu)
> > 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu