[PATCH 0/5] iommu/arm-smmu: Support maintaining bootloader mappings

[PATCH 0/5] iommu/arm-smmu: Support maintaining bootloader mappings

[Bjorn Andersson]

Based on previous attempts and discussions this is the latest attempt at
inheriting stream mappings set up by the bootloader, for e.g. boot splash or
efifb.

The first patch is an implementation of Robin's suggestion that we should just
mark the relevant stream mappings as BYPASS. Relying on something else to set
up the stream mappings wanted - e.g. by reading it back in platform specific
implementation code.

The series then tackles the problem seen in most versions of Qualcomm firmware,
that the hypervisor intercepts BYPASS writes and turn them into FAULTs. It does
this by allocating context banks for identity domains as well, with translation
disabled.

Lastly it amends the stream mapping initialization code to allocate a specific
identity domain that is used for any mappings inherited from the bootloader, if
above Qualcomm quirk is required.


The series has been tested and shown to allow booting SDM845, SDM850, SM8150,
SM8250 with boot splash screen setup by the bootloader. Specifically it also
allows the Lenovo Yoga C630 to boot with SMMU and efifb enabled.

Bjorn Andersson (5):
  iommu/arm-smmu: Make all valid stream mappings BYPASS
  iommu/arm-smmu: Emulate bypass by using context banks
  iommu/arm-smmu: Move SMR and S2CR definitions to header file
  iommu/arm-smmu-qcom: Consstently initialize stream mappings
  iommu/arm-smmu: Setup identity domain for boot mappings

 drivers/iommu/arm-smmu-qcom.c |  48 +++++++++++++
 drivers/iommu/arm-smmu.c      | 124 +++++++++++++++++++++++++++++-----
 drivers/iommu/arm-smmu.h      |  21 ++++++
 3 files changed, 175 insertions(+), 18 deletions(-)

-- 
2.26.2

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

[PATCH 1/5] iommu/arm-smmu: Make all valid stream mappings BYPASS

[Bjorn Andersson]

Turn all stream mappings marked as valid into BYPASS. This allows the
platform specific implementation to configure stream mappings to match
the boot loader's configuration for e.g. display to continue to function
through the reset of the SMMU.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 243bc4cb2705..2e27cf9815ab 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1924,6 +1924,22 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
+{
+	int i;
+
+	for (i = 0; i < smmu->num_mapping_groups; i++) {
+		if (smmu->smrs[i].valid) {
+			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;
+}
+
 struct arm_smmu_match_data {
 	enum arm_smmu_arch_version version;
 	enum arm_smmu_implementation model;
@@ -2181,6 +2197,10 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	err = arm_smmu_setup_identity(smmu);
+	if (err)
+		return err;
+
 	if (smmu->version == ARM_SMMU_V2) {
 		if (smmu->num_context_banks > smmu->num_context_irqs) {
 			dev_err(dev,
-- 
2.26.2

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

[PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Bjorn Andersson]

Some firmware found on various Qualcomm platforms traps writes to S2CR
of type BYPASS and writes FAULT into the register. This prevents us from
marking the streams for the display controller as BYPASS to allow
continued scanout of the screen through the initialization of the ARM
SMMU.

This adds a Qualcomm specific cfg_probe function, which probes the
behavior of the S2CR registers and if found faulty enables the related
quirk. Based on this quirk context banks are allocated for IDENTITY
domains as well, but with ARM_SMMU_SCTLR_M omitted.

The result is valid stream mappings, without translation.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu-qcom.c | 21 +++++++++++++++++++++
 drivers/iommu/arm-smmu.c      | 14 ++++++++++++--
 drivers/iommu/arm-smmu.h      |  3 +++
 3 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index cf01d0215a39..e8a36054e912 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -23,6 +23,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
 	{ }
 };
 
+static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
+{
+	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
+	u32 reg;
+
+	/*
+	 * With some firmware writes to S2CR of type FAULT are ignored, and
+	 * writing BYPASS will end up as FAULT in the register. Perform a write
+	 * to S2CR to detect if this is the case with the current firmware.
+	 */
+	arm_smmu_gr0_write(smmu, last_s2cr, 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));
+	reg = arm_smmu_gr0_read(smmu, last_s2cr);
+	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+		smmu->qcom_bypass_quirk = true;
+
+	return 0;
+}
+
 static int qcom_smmu_def_domain_type(struct device *dev)
 {
 	const struct of_device_id *match =
@@ -61,6 +81,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,
 };
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2e27cf9815ab..f33eda3117fa 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -654,7 +654,9 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
 
 	/* SCTLR */
 	reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE | ARM_SMMU_SCTLR_AFE |
-	      ARM_SMMU_SCTLR_TRE | ARM_SMMU_SCTLR_M;
+	      ARM_SMMU_SCTLR_TRE;
+	if (cfg->m)
+		reg |= ARM_SMMU_SCTLR_M;
 	if (stage1)
 		reg |= ARM_SMMU_SCTLR_S1_ASIDPNE;
 	if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
@@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
+	/*
+	 * Nothing to do for IDENTITY domains,unless disabled context banks are
+	 * used to emulate bypass mappings on Qualcomm platforms.
+	 */
+	if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
 		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
 		smmu_domain->smmu = smmu;
 		goto out_unlock;
@@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	domain->geometry.aperture_end = (1UL << ias) - 1;
 	domain->geometry.force_aperture = true;
 
+	/* Enable translation for non-identity context banks */
+	if (domain->type != IOMMU_DOMAIN_IDENTITY)
+		cfg->m = true;
+
 	/* Initialise the context bank with our page table cfg */
 	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
 	arm_smmu_write_context_bank(smmu, cfg->cbndx);
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index d172c024be61..a71d193073e4 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -305,6 +305,8 @@ struct arm_smmu_device {
 
 	/* IOMMU core code handle */
 	struct iommu_device		iommu;
+
+	bool				qcom_bypass_quirk;
 };
 
 enum arm_smmu_context_fmt {
@@ -323,6 +325,7 @@ struct arm_smmu_cfg {
 	};
 	enum arm_smmu_cbar_type		cbar;
 	enum arm_smmu_context_fmt	fmt;
+	bool				m;
 };
 #define ARM_SMMU_INVALID_IRPTNDX	0xff
 
-- 
2.26.2

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

[PATCH 3/5] iommu/arm-smmu: Move SMR and S2CR definitions to header file

[Bjorn Andersson]

Expose the SMR and S2CR structs in the header file, to allow platform
specific implementations to populate/initialize the smrs and s2cr
arrays.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu.c | 14 --------------
 drivers/iommu/arm-smmu.h | 15 +++++++++++++++
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index f33eda3117fa..e2d6c0aaf1ea 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -68,24 +68,10 @@ module_param(disable_bypass, bool, S_IRUGO);
 MODULE_PARM_DESC(disable_bypass,
 	"Disable bypass streams such that incoming transactions from devices that are not attached to an iommu domain will report an abort back to the device and will not be allowed to pass through the SMMU.");
 
-struct arm_smmu_s2cr {
-	struct iommu_group		*group;
-	int				count;
-	enum arm_smmu_s2cr_type		type;
-	enum arm_smmu_s2cr_privcfg	privcfg;
-	u8				cbndx;
-};
-
 #define s2cr_init_val (struct arm_smmu_s2cr){				\
 	.type = disable_bypass ? S2CR_TYPE_FAULT : S2CR_TYPE_BYPASS,	\
 }
 
-struct arm_smmu_smr {
-	u16				mask;
-	u16				id;
-	bool				valid;
-};
-
 struct arm_smmu_cb {
 	u64				ttbr[2];
 	u32				tcr[2];
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index a71d193073e4..bcd160d01c53 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -251,6 +251,21 @@ enum arm_smmu_implementation {
 	QCOM_SMMUV2,
 };
 
+struct arm_smmu_s2cr {
+	struct iommu_group		*group;
+	int				count;
+	enum arm_smmu_s2cr_type		type;
+	enum arm_smmu_s2cr_privcfg	privcfg;
+	u8				cbndx;
+};
+
+struct arm_smmu_smr {
+	u16				mask;
+	u16				id;
+	bool				valid;
+	bool				pinned;
+};
+
 struct arm_smmu_device {
 	struct device			*dev;
 
-- 
2.26.2

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

[PATCH 4/5] iommu/arm-smmu-qcom: Consstently initialize stream mappings

[Bjorn Andersson]

Firmware that traps writes to S2CR to translate BYPASS into FAULT also
ignores writes of type FAULT. As such booting with "disable_bypass" set
will result in all S2CR registers left as configured by the bootloader.

This has been seen to result in indeterministic results, as these
mappings might linger and reference context banks that Linux is
reconfiguring.

Use the fact that BYPASS writes result in FAULT type to force all stream
mappings to FAULT.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu-qcom.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index e8a36054e912..86b1917459a4 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -27,6 +27,7 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
 	u32 reg;
+	int i;
 
 	/*
 	 * With some firmware writes to S2CR of type FAULT are ignored, and
@@ -37,9 +38,24 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 					    FIELD_PREP(ARM_SMMU_S2CR_CBNDX, 0xff) |
 					    FIELD_PREP(ARM_SMMU_S2CR_PRIVCFG, S2CR_PRIVCFG_DEFAULT));
 	reg = arm_smmu_gr0_read(smmu, last_s2cr);
-	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
+	if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS) {
 		smmu->qcom_bypass_quirk = true;
 
+		/*
+		 * With firmware ignoring writes of type FAULT, booting the
+		 * Linux kernel with disable_bypass disabled (i.e. "enable
+		 * bypass") the initialization during probe will leave mappings
+		 * in an inconsistent state. Avoid this by configuring all
+		 * S2CRs to BYPASS.
+		 */
+		for (i = 0; i < smmu->num_mapping_groups; i++) {
+			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
+			smmu->s2crs[i].cbndx = 0xff;
+			smmu->s2crs[i].count = 0;
+		}
+	}
+
 	return 0;
 }
 
-- 
2.26.2

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

[PATCH 5/5] iommu/arm-smmu: Setup identity domain for boot mappings

[Bjorn Andersson]

With many Qualcomm platforms not having functional S2CR BYPASS a
temporary IOMMU domain, without translation, needs to be allocated in
order to allow these memory transactions.

Unfortunately the boot loader uses the first few context banks, so
rather than overwriting a active bank the last context bank is used and
streams are diverted here during initialization.

This also performs the readback of SMR registers for the Qualcomm
platform, to trigger the mechanism.

This is based on prior work by Thierry Reding and Laurentiu Tudor.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/iommu/arm-smmu-qcom.c | 11 +++++
 drivers/iommu/arm-smmu.c      | 80 +++++++++++++++++++++++++++++++++--
 drivers/iommu/arm-smmu.h      |  3 ++
 3 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
index 86b1917459a4..397df27c1d69 100644
--- a/drivers/iommu/arm-smmu-qcom.c
+++ b/drivers/iommu/arm-smmu-qcom.c
@@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
 static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 {
 	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
+	u32 smr;
 	u32 reg;
 	int i;
 
@@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
 		}
 	}
 
+	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;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index e2d6c0aaf1ea..a7cb27c1a49e 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
 }
 
 static int arm_smmu_init_domain_context(struct iommu_domain *domain,
-					struct arm_smmu_device *smmu)
+					struct arm_smmu_device *smmu,
+					bool boot_domain)
 {
 	int irq, start, ret = 0;
 	unsigned long ias, oas;
@@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 		ret = -EINVAL;
 		goto out_unlock;
 	}
+
+	/*
+	 * Use the last context bank for identity mappings during boot, to
+	 * avoid overwriting in-use bank configuration while we're setting up
+	 * the new mappings.
+	 */
+	if (boot_domain)
+		start = smmu->num_context_banks - 1;
+
 	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
 				      smmu->num_context_banks);
 	if (ret < 0)
@@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct arm_smmu_master_cfg *cfg;
 	struct arm_smmu_device *smmu;
+	bool free_identity_domain = false;
+	int idx;
 	int ret;
+	int i;
 
 	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
 		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
@@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return ret;
 
 	/* Ensure that the domain is finalised */
-	ret = arm_smmu_init_domain_context(domain, smmu);
+	ret = arm_smmu_init_domain_context(domain, smmu, false);
 	if (ret < 0)
 		goto rpm_put;
 
@@ -1190,9 +1203,34 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		goto rpm_put;
 	}
 
+	/* Decrement use counter for any references to the identity domain */
+	mutex_lock(&smmu->stream_map_mutex);
+	if (smmu->identity) {
+		struct arm_smmu_domain *identity = to_smmu_domain(smmu->identity);
+
+		for_each_cfg_sme(cfg, fwspec, i, idx) {
+			dev_err(smmu->dev, "%s() %#x\n", __func__, smmu->smrs[idx].id);
+			if (smmu->s2crs[idx].cbndx == identity->cfg.cbndx) {
+				smmu->num_identity_masters--;
+				if (smmu->num_identity_masters == 0)
+					free_identity_domain = true;
+			}
+		}
+	}
+	mutex_unlock(&smmu->stream_map_mutex);
+
 	/* Looks ok, so add the device to the domain */
 	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
 
+	/*
+	 * The last stream map to reference the identity domain has been
+	 * overwritten, so it's now okay to free it.
+	 */
+	if (free_identity_domain) {
+		arm_smmu_domain_free(smmu->identity);
+		smmu->identity = NULL;
+	}
+
 	/*
 	 * Setup an autosuspend delay to avoid bouncing runpm state.
 	 * Otherwise, if a driver for a suspended consumer device
@@ -1922,17 +1960,51 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 
 int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
 {
+	struct device *dev = smmu->dev;
+	int cbndx = 0xff;
+	int type = S2CR_TYPE_BYPASS;
+	int ret;
 	int i;
 
+	if (smmu->qcom_bypass_quirk) {
+		/* Create a IDENTITY domain to use for all inherited streams */
+		smmu->identity = arm_smmu_domain_alloc(IOMMU_DOMAIN_IDENTITY);
+		if (!smmu->identity) {
+			dev_err(dev, "failed to create identity domain\n");
+			return -ENOMEM;
+		}
+
+		smmu->identity->pgsize_bitmap = smmu->pgsize_bitmap;
+		smmu->identity->type = IOMMU_DOMAIN_IDENTITY;
+		smmu->identity->ops = &arm_smmu_ops;
+
+		ret = arm_smmu_init_domain_context(smmu->identity, smmu, true);
+		if (ret < 0) {
+			dev_err(dev, "failed to initialize identity domain: %d\n", ret);
+			return ret;
+		}
+
+		type = S2CR_TYPE_TRANS;
+		cbndx = to_smmu_domain(smmu->identity)->cfg.cbndx;
+	}
+
 	for (i = 0; i < smmu->num_mapping_groups; i++) {
 		if (smmu->smrs[i].valid) {
-			smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
+			smmu->s2crs[i].type = type;
 			smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
-			smmu->s2crs[i].cbndx = 0xff;
+			smmu->s2crs[i].cbndx = cbndx;
 			smmu->s2crs[i].count++;
+
+			smmu->num_identity_masters++;
 		}
 	}
 
+	/* If no mappings where found, free the identiy domain again */
+	if (smmu->identity && !smmu->num_identity_masters) {
+		arm_smmu_domain_free(smmu->identity);
+		smmu->identity = NULL;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
index bcd160d01c53..37257ede86fa 100644
--- a/drivers/iommu/arm-smmu.h
+++ b/drivers/iommu/arm-smmu.h
@@ -321,6 +321,9 @@ struct arm_smmu_device {
 	/* IOMMU core code handle */
 	struct iommu_device		iommu;
 
+	struct iommu_domain		*identity;
+	unsigned int			num_identity_masters;
+
 	bool				qcom_bypass_quirk;
 };
 
-- 
2.26.2

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

Re: [PATCH 4/5] iommu/arm-smmu-qcom: Consstently initialize stream mappings

[Vinod Koul]

On 08-07-20, 22:01, Bjorn Andersson wrote:
> Firmware that traps writes to S2CR to translate BYPASS into FAULT also
> ignores writes of type FAULT. As such booting with "disable_bypass" set
> will result in all S2CR registers left as configured by the bootloader.
> 
> This has been seen to result in indeterministic results, as these
> mappings might linger and reference context banks that Linux is
> reconfiguring.
> 
> Use the fact that BYPASS writes result in FAULT type to force all stream
> mappings to FAULT.

s/Consstently/Consistently in patch subject

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

Re: [PATCH 0/5] iommu/arm-smmu: Support maintaining bootloader mappings

[Vinod Koul]

On 08-07-20, 22:01, Bjorn Andersson wrote:
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
> 
> The first patch is an implementation of Robin's suggestion that we should just
> mark the relevant stream mappings as BYPASS. Relying on something else to set
> up the stream mappings wanted - e.g. by reading it back in platform specific
> implementation code.
> 
> The series then tackles the problem seen in most versions of Qualcomm firmware,
> that the hypervisor intercepts BYPASS writes and turn them into FAULTs. It does
> this by allocating context banks for identity domains as well, with translation
> disabled.
> 
> Lastly it amends the stream mapping initialization code to allocate a specific
> identity domain that is used for any mappings inherited from the bootloader, if
> above Qualcomm quirk is required.
> 
> 
> The series has been tested and shown to allow booting SDM845, SDM850, SM8150,
> SM8250 with boot splash screen setup by the bootloader. Specifically it also
> allows the Lenovo Yoga C630 to boot with SMMU and efifb enabled.

This resolves issue on RB3 for me so:

Tested-by: Vinod Koul <vkoul@kernel.org>

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

Re: [PATCH 5/5] iommu/arm-smmu: Setup identity domain for boot mappings

[Laurentiu Tudor]

On 7/9/2020 8:01 AM, Bjorn Andersson wrote:
> With many Qualcomm platforms not having functional S2CR BYPASS a
> temporary IOMMU domain, without translation, needs to be allocated in
> order to allow these memory transactions.
> 
> Unfortunately the boot loader uses the first few context banks, so
> rather than overwriting a active bank the last context bank is used and
> streams are diverted here during initialization.
> 
> This also performs the readback of SMR registers for the Qualcomm
> platform, to trigger the mechanism.
> 
> This is based on prior work by Thierry Reding and Laurentiu Tudor.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/iommu/arm-smmu-qcom.c | 11 +++++
>  drivers/iommu/arm-smmu.c      | 80 +++++++++++++++++++++++++++++++++--
>  drivers/iommu/arm-smmu.h      |  3 ++
>  3 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> index 86b1917459a4..397df27c1d69 100644
> --- a/drivers/iommu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm-smmu-qcom.c
> @@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
>  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>  {
>  	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
> +	u32 smr;
>  	u32 reg;
>  	int i;
>  
> @@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>  		}
>  	}
>  
> +	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;
> +		}
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index e2d6c0aaf1ea..a7cb27c1a49e 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>  }
>  
>  static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> -					struct arm_smmu_device *smmu)
> +					struct arm_smmu_device *smmu,
> +					bool boot_domain)
>  {
>  	int irq, start, ret = 0;
>  	unsigned long ias, oas;
> @@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> +
> +	/*
> +	 * Use the last context bank for identity mappings during boot, to
> +	 * avoid overwriting in-use bank configuration while we're setting up
> +	 * the new mappings.
> +	 */
> +	if (boot_domain)
> +		start = smmu->num_context_banks - 1;
> +
>  	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
>  				      smmu->num_context_banks);
>  	if (ret < 0)
> @@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>  	struct arm_smmu_master_cfg *cfg;
>  	struct arm_smmu_device *smmu;
> +	bool free_identity_domain = false;
> +	int idx;
>  	int ret;
> +	int i;
>  
>  	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
>  		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
> @@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		return ret;
>  
>  	/* Ensure that the domain is finalised */
> -	ret = arm_smmu_init_domain_context(domain, smmu);
> +	ret = arm_smmu_init_domain_context(domain, smmu, false);
>  	if (ret < 0)
>  		goto rpm_put;
>  
> @@ -1190,9 +1203,34 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  		goto rpm_put;
>  	}
>  
> +	/* Decrement use counter for any references to the identity domain */
> +	mutex_lock(&smmu->stream_map_mutex);
> +	if (smmu->identity) {
> +		struct arm_smmu_domain *identity = to_smmu_domain(smmu->identity);
> +
> +		for_each_cfg_sme(cfg, fwspec, i, idx) {
> +			dev_err(smmu->dev, "%s() %#x\n", __func__, smmu->smrs[idx].id);

Debug leftovers?

Apart from that I plan to give this a go on some NXP chips. Will keep
you updated.

Thanks a lot Bjorn.

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

Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Rob Clark]

On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Some firmware found on various Qualcomm platforms traps writes to S2CR
> of type BYPASS and writes FAULT into the register. This prevents us from
> marking the streams for the display controller as BYPASS to allow
> continued scanout of the screen through the initialization of the ARM
> SMMU.
>
> This adds a Qualcomm specific cfg_probe function, which probes the
> behavior of the S2CR registers and if found faulty enables the related
> quirk. Based on this quirk context banks are allocated for IDENTITY
> domains as well, but with ARM_SMMU_SCTLR_M omitted.
>
> The result is valid stream mappings, without translation.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/iommu/arm-smmu-qcom.c | 21 +++++++++++++++++++++
>  drivers/iommu/arm-smmu.c      | 14 ++++++++++++--
>  drivers/iommu/arm-smmu.h      |  3 +++
>  3 files changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> index cf01d0215a39..e8a36054e912 100644
> --- a/drivers/iommu/arm-smmu-qcom.c
> +++ b/drivers/iommu/arm-smmu-qcom.c
> @@ -23,6 +23,26 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
>         { }
>  };
>
> +static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> +{
> +       unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
> +       u32 reg;
> +
> +       /*
> +        * With some firmware writes to S2CR of type FAULT are ignored, and
> +        * writing BYPASS will end up as FAULT in the register. Perform a write
> +        * to S2CR to detect if this is the case with the current firmware.
> +        */
> +       arm_smmu_gr0_write(smmu, last_s2cr, 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));
> +       reg = arm_smmu_gr0_read(smmu, last_s2cr);
> +       if (FIELD_GET(ARM_SMMU_S2CR_TYPE, reg) != S2CR_TYPE_BYPASS)
> +               smmu->qcom_bypass_quirk = true;
> +
> +       return 0;
> +}
> +
>  static int qcom_smmu_def_domain_type(struct device *dev)
>  {
>         const struct of_device_id *match =
> @@ -61,6 +81,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,
>  };
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 2e27cf9815ab..f33eda3117fa 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -654,7 +654,9 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>
>         /* SCTLR */
>         reg = ARM_SMMU_SCTLR_CFIE | ARM_SMMU_SCTLR_CFRE | ARM_SMMU_SCTLR_AFE |
> -             ARM_SMMU_SCTLR_TRE | ARM_SMMU_SCTLR_M;
> +             ARM_SMMU_SCTLR_TRE;
> +       if (cfg->m)
> +               reg |= ARM_SMMU_SCTLR_M;
>         if (stage1)
>                 reg |= ARM_SMMU_SCTLR_S1_ASIDPNE;
>         if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> @@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>         if (smmu_domain->smmu)
>                 goto out_unlock;
>
> -       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> +       /*
> +        * Nothing to do for IDENTITY domains,unless disabled context banks are
> +        * used to emulate bypass mappings on Qualcomm platforms.
> +        */
> +       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {

maybe I'm overlooking something, but I think this would put us back to
allocating pgtables (and making iommu->map/unmap() no longer no-ops),
which I don't think we want

BR,
-R

>                 smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
>                 smmu_domain->smmu = smmu;
>                 goto out_unlock;
> @@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>         domain->geometry.aperture_end = (1UL << ias) - 1;
>         domain->geometry.force_aperture = true;
>
> +       /* Enable translation for non-identity context banks */
> +       if (domain->type != IOMMU_DOMAIN_IDENTITY)
> +               cfg->m = true;
> +
>         /* Initialise the context bank with our page table cfg */
>         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
>         arm_smmu_write_context_bank(smmu, cfg->cbndx);
> diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> index d172c024be61..a71d193073e4 100644
> --- a/drivers/iommu/arm-smmu.h
> +++ b/drivers/iommu/arm-smmu.h
> @@ -305,6 +305,8 @@ struct arm_smmu_device {
>
>         /* IOMMU core code handle */
>         struct iommu_device             iommu;
> +
> +       bool                            qcom_bypass_quirk;
>  };
>
>  enum arm_smmu_context_fmt {
> @@ -323,6 +325,7 @@ struct arm_smmu_cfg {
>         };
>         enum arm_smmu_cbar_type         cbar;
>         enum arm_smmu_context_fmt       fmt;
> +       bool                            m;
>  };
>  #define ARM_SMMU_INVALID_IRPTNDX       0xff
>
> --
> 2.26.2
>
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Bjorn Andersson]

On Thu 09 Jul 09:17 PDT 2020, Rob Clark wrote:

> On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
[..]
> > @@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >         if (smmu_domain->smmu)
> >                 goto out_unlock;
> >
> > -       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > +       /*
> > +        * Nothing to do for IDENTITY domains,unless disabled context banks are
> > +        * used to emulate bypass mappings on Qualcomm platforms.
> > +        */
> > +       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
> 
> maybe I'm overlooking something, but I think this would put us back to
> allocating pgtables (and making iommu->map/unmap() no longer no-ops),
> which I don't think we want
> 

You're right, we are allocating page tables for these contexts and
map/unmap would modify the page tables. But afaict traversal is never
performed, given that the banks are never enabled.

But as drivers probe properly, or the direct mapped drivers sets up
their iommu domains explicitly with translation this would not be used.

So afaict we're just wasting some memory - for the gain of not
overcomplicating this function.

Regards,
Bjorn

> BR,
> -R
> 
> >                 smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> >                 smmu_domain->smmu = smmu;
> >                 goto out_unlock;
> > @@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >         domain->geometry.aperture_end = (1UL << ias) - 1;
> >         domain->geometry.force_aperture = true;
> >
> > +       /* Enable translation for non-identity context banks */
> > +       if (domain->type != IOMMU_DOMAIN_IDENTITY)
> > +               cfg->m = true;
> > +
> >         /* Initialise the context bank with our page table cfg */
> >         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> >         arm_smmu_write_context_bank(smmu, cfg->cbndx);
> > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > index d172c024be61..a71d193073e4 100644
> > --- a/drivers/iommu/arm-smmu.h
> > +++ b/drivers/iommu/arm-smmu.h
> > @@ -305,6 +305,8 @@ struct arm_smmu_device {
> >
> >         /* IOMMU core code handle */
> >         struct iommu_device             iommu;
> > +
> > +       bool                            qcom_bypass_quirk;
> >  };
> >
> >  enum arm_smmu_context_fmt {
> > @@ -323,6 +325,7 @@ struct arm_smmu_cfg {
> >         };
> >         enum arm_smmu_cbar_type         cbar;
> >         enum arm_smmu_context_fmt       fmt;
> > +       bool                            m;
> >  };
> >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> >
> > --
> > 2.26.2
> >
> > _______________________________________________
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Rob Clark]

On Thu, Jul 9, 2020 at 9:48 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Thu 09 Jul 09:17 PDT 2020, Rob Clark wrote:
>
> > On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> [..]
> > > @@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > >         if (smmu_domain->smmu)
> > >                 goto out_unlock;
> > >
> > > -       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > > +       /*
> > > +        * Nothing to do for IDENTITY domains,unless disabled context banks are
> > > +        * used to emulate bypass mappings on Qualcomm platforms.
> > > +        */
> > > +       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
> >
> > maybe I'm overlooking something, but I think this would put us back to
> > allocating pgtables (and making iommu->map/unmap() no longer no-ops),
> > which I don't think we want
> >
>
> You're right, we are allocating page tables for these contexts and
> map/unmap would modify the page tables. But afaict traversal is never
> performed, given that the banks are never enabled.
>
> But as drivers probe properly, or the direct mapped drivers sets up
> their iommu domains explicitly with translation this would not be used.
>
> So afaict we're just wasting some memory - for the gain of not
> overcomplicating this function.

the problem is that it makes dma_map/unmap less of a no-op than it
should be (for the case where the driver is explicitly managing it's
own domain)..  I was hoping to get rid of the hacks to use dma_sync go
back to dma_map/unmap for cache cleaning

BR,
-R


>
> Regards,
> Bjorn
>
> > BR,
> > -R
> >
> > >                 smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> > >                 smmu_domain->smmu = smmu;
> > >                 goto out_unlock;
> > > @@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > >         domain->geometry.aperture_end = (1UL << ias) - 1;
> > >         domain->geometry.force_aperture = true;
> > >
> > > +       /* Enable translation for non-identity context banks */
> > > +       if (domain->type != IOMMU_DOMAIN_IDENTITY)
> > > +               cfg->m = true;
> > > +
> > >         /* Initialise the context bank with our page table cfg */
> > >         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> > >         arm_smmu_write_context_bank(smmu, cfg->cbndx);
> > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > index d172c024be61..a71d193073e4 100644
> > > --- a/drivers/iommu/arm-smmu.h
> > > +++ b/drivers/iommu/arm-smmu.h
> > > @@ -305,6 +305,8 @@ struct arm_smmu_device {
> > >
> > >         /* IOMMU core code handle */
> > >         struct iommu_device             iommu;
> > > +
> > > +       bool                            qcom_bypass_quirk;
> > >  };
> > >
> > >  enum arm_smmu_context_fmt {
> > > @@ -323,6 +325,7 @@ struct arm_smmu_cfg {
> > >         };
> > >         enum arm_smmu_cbar_type         cbar;
> > >         enum arm_smmu_context_fmt       fmt;
> > > +       bool                            m;
> > >  };
> > >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > iommu mailing list
> > > iommu@lists.linux-foundation.org
> > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Rob Clark]

On Thu, Jul 9, 2020 at 9:56 AM Rob Clark <robdclark@gmail.com> wrote:
>
> On Thu, Jul 9, 2020 at 9:48 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Thu 09 Jul 09:17 PDT 2020, Rob Clark wrote:
> >
> > > On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > [..]
> > > > @@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > > >         if (smmu_domain->smmu)
> > > >                 goto out_unlock;
> > > >
> > > > -       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > > > +       /*
> > > > +        * Nothing to do for IDENTITY domains,unless disabled context banks are
> > > > +        * used to emulate bypass mappings on Qualcomm platforms.
> > > > +        */
> > > > +       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
> > >
> > > maybe I'm overlooking something, but I think this would put us back to
> > > allocating pgtables (and making iommu->map/unmap() no longer no-ops),
> > > which I don't think we want
> > >
> >
> > You're right, we are allocating page tables for these contexts and
> > map/unmap would modify the page tables. But afaict traversal is never
> > performed, given that the banks are never enabled.
> >
> > But as drivers probe properly, or the direct mapped drivers sets up
> > their iommu domains explicitly with translation this would not be used.
> >
> > So afaict we're just wasting some memory - for the gain of not
> > overcomplicating this function.
>
> the problem is that it makes dma_map/unmap less of a no-op than it
> should be (for the case where the driver is explicitly managing it's
> own domain)..  I was hoping to get rid of the hacks to use dma_sync go
> back to dma_map/unmap for cache cleaning

That said, it seems to cause less explosions than before.. either that
or I'm not trying hard enough.  Still, I think it would probably
result in unnecessary tlb inv's.

Previously, *somehow* we seemed to end up with dma_map/unmap trying to
modify the domain that we had attached.

BR,
-R

> BR,
> -R
>
>
> >
> > Regards,
> > Bjorn
> >
> > > BR,
> > > -R
> > >
> > > >                 smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> > > >                 smmu_domain->smmu = smmu;
> > > >                 goto out_unlock;
> > > > @@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > > >         domain->geometry.aperture_end = (1UL << ias) - 1;
> > > >         domain->geometry.force_aperture = true;
> > > >
> > > > +       /* Enable translation for non-identity context banks */
> > > > +       if (domain->type != IOMMU_DOMAIN_IDENTITY)
> > > > +               cfg->m = true;
> > > > +
> > > >         /* Initialise the context bank with our page table cfg */
> > > >         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> > > >         arm_smmu_write_context_bank(smmu, cfg->cbndx);
> > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > index d172c024be61..a71d193073e4 100644
> > > > --- a/drivers/iommu/arm-smmu.h
> > > > +++ b/drivers/iommu/arm-smmu.h
> > > > @@ -305,6 +305,8 @@ struct arm_smmu_device {
> > > >
> > > >         /* IOMMU core code handle */
> > > >         struct iommu_device             iommu;
> > > > +
> > > > +       bool                            qcom_bypass_quirk;
> > > >  };
> > > >
> > > >  enum arm_smmu_context_fmt {
> > > > @@ -323,6 +325,7 @@ struct arm_smmu_cfg {
> > > >         };
> > > >         enum arm_smmu_cbar_type         cbar;
> > > >         enum arm_smmu_context_fmt       fmt;
> > > > +       bool                            m;
> > > >  };
> > > >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> > > >
> > > > --
> > > > 2.26.2
> > > >
> > > > _______________________________________________
> > > > iommu mailing list
> > > > iommu@lists.linux-foundation.org
> > > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/5] iommu/arm-smmu: Emulate bypass by using context banks

[Bjorn Andersson]

On Thu 09 Jul 11:55 PDT 2020, Rob Clark wrote:

> On Thu, Jul 9, 2020 at 9:56 AM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Thu, Jul 9, 2020 at 9:48 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Thu 09 Jul 09:17 PDT 2020, Rob Clark wrote:
> > >
> > > > On Wed, Jul 8, 2020 at 10:01 PM Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > [..]
> > > > > @@ -678,7 +680,11 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > > > >         if (smmu_domain->smmu)
> > > > >                 goto out_unlock;
> > > > >
> > > > > -       if (domain->type == IOMMU_DOMAIN_IDENTITY) {
> > > > > +       /*
> > > > > +        * Nothing to do for IDENTITY domains,unless disabled context banks are
> > > > > +        * used to emulate bypass mappings on Qualcomm platforms.
> > > > > +        */
> > > > > +       if (domain->type == IOMMU_DOMAIN_IDENTITY && !smmu->qcom_bypass_quirk) {
> > > >
> > > > maybe I'm overlooking something, but I think this would put us back to
> > > > allocating pgtables (and making iommu->map/unmap() no longer no-ops),
> > > > which I don't think we want
> > > >
> > >
> > > You're right, we are allocating page tables for these contexts and
> > > map/unmap would modify the page tables. But afaict traversal is never
> > > performed, given that the banks are never enabled.
> > >
> > > But as drivers probe properly, or the direct mapped drivers sets up
> > > their iommu domains explicitly with translation this would not be used.
> > >
> > > So afaict we're just wasting some memory - for the gain of not
> > > overcomplicating this function.
> >
> > the problem is that it makes dma_map/unmap less of a no-op than it
> > should be (for the case where the driver is explicitly managing it's
> > own domain)..  I was hoping to get rid of the hacks to use dma_sync go
> > back to dma_map/unmap for cache cleaning
> 
> That said, it seems to cause less explosions than before.. either that
> or I'm not trying hard enough.  Still, I think it would probably
> result in unnecessary tlb inv's.
> 
> Previously, *somehow* we seemed to end up with dma_map/unmap trying to
> modify the domain that we had attached.
> 

I might need some help to really understand the plumbing here, but this
is what I read from the code and can see in my debugging...


The display device will upon creation be allocated a default_domain, of
type IOMMU_DOMAIN_IDENTITY - per the qcom-impl. Then you then allocate a
new iommu domain on the platform bus in the display driver and attach
this to the device. This will result in the group's domain being of type
IOMMU_DOMAIN_UNMANAGED.

But when you then invoke dma_map_single() on the display device, the map
operation will acquire the iommu_group's default_domain (not domain) and
as such attempt to map stuff on the IDENTITY domain.

__iommu_map() will however reject this, given that the type does not
have __IOMMU_DOMAIN_PAGING set. Afaict, this would happen regardless of
this patch actually setting up a page table for the default_domain or
not.


So, afaict, you can't use dma_map_single()/dma_unmap() to operate your
page table on a lately attached iommu domain.

This would also imply that the display device consumes two context
banks, which worries me more than the waste of page tables etc.

Regards,
Bjorn

> BR,
> -R
> 
> > BR,
> > -R
> >
> >
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > BR,
> > > > -R
> > > >
> > > > >                 smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
> > > > >                 smmu_domain->smmu = smmu;
> > > > >                 goto out_unlock;
> > > > > @@ -826,6 +832,10 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > > > >         domain->geometry.aperture_end = (1UL << ias) - 1;
> > > > >         domain->geometry.force_aperture = true;
> > > > >
> > > > > +       /* Enable translation for non-identity context banks */
> > > > > +       if (domain->type != IOMMU_DOMAIN_IDENTITY)
> > > > > +               cfg->m = true;
> > > > > +
> > > > >         /* Initialise the context bank with our page table cfg */
> > > > >         arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> > > > >         arm_smmu_write_context_bank(smmu, cfg->cbndx);
> > > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > > index d172c024be61..a71d193073e4 100644
> > > > > --- a/drivers/iommu/arm-smmu.h
> > > > > +++ b/drivers/iommu/arm-smmu.h
> > > > > @@ -305,6 +305,8 @@ struct arm_smmu_device {
> > > > >
> > > > >         /* IOMMU core code handle */
> > > > >         struct iommu_device             iommu;
> > > > > +
> > > > > +       bool                            qcom_bypass_quirk;
> > > > >  };
> > > > >
> > > > >  enum arm_smmu_context_fmt {
> > > > > @@ -323,6 +325,7 @@ struct arm_smmu_cfg {
> > > > >         };
> > > > >         enum arm_smmu_cbar_type         cbar;
> > > > >         enum arm_smmu_context_fmt       fmt;
> > > > > +       bool                            m;
> > > > >  };
> > > > >  #define ARM_SMMU_INVALID_IRPTNDX       0xff
> > > > >
> > > > > --
> > > > > 2.26.2
> > > > >
> > > > > _______________________________________________
> > > > > iommu mailing list
> > > > > iommu@lists.linux-foundation.org
> > > > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 5/5] iommu/arm-smmu: Setup identity domain for boot mappings

[Bjorn Andersson]

On Thu 09 Jul 08:50 PDT 2020, Laurentiu Tudor wrote:

> 
> 
> On 7/9/2020 8:01 AM, Bjorn Andersson wrote:
> > With many Qualcomm platforms not having functional S2CR BYPASS a
> > temporary IOMMU domain, without translation, needs to be allocated in
> > order to allow these memory transactions.
> > 
> > Unfortunately the boot loader uses the first few context banks, so
> > rather than overwriting a active bank the last context bank is used and
> > streams are diverted here during initialization.
> > 
> > This also performs the readback of SMR registers for the Qualcomm
> > platform, to trigger the mechanism.
> > 
> > This is based on prior work by Thierry Reding and Laurentiu Tudor.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  drivers/iommu/arm-smmu-qcom.c | 11 +++++
> >  drivers/iommu/arm-smmu.c      | 80 +++++++++++++++++++++++++++++++++--
> >  drivers/iommu/arm-smmu.h      |  3 ++
> >  3 files changed, 90 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
> > index 86b1917459a4..397df27c1d69 100644
> > --- a/drivers/iommu/arm-smmu-qcom.c
> > +++ b/drivers/iommu/arm-smmu-qcom.c
> > @@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
> >  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >  {
> >  	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
> > +	u32 smr;
> >  	u32 reg;
> >  	int i;
> >  
> > @@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
> >  		}
> >  	}
> >  
> > +	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;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> > index e2d6c0aaf1ea..a7cb27c1a49e 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
> >  }
> >  
> >  static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> > -					struct arm_smmu_device *smmu)
> > +					struct arm_smmu_device *smmu,
> > +					bool boot_domain)
> >  {
> >  	int irq, start, ret = 0;
> >  	unsigned long ias, oas;
> > @@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >  		ret = -EINVAL;
> >  		goto out_unlock;
> >  	}
> > +
> > +	/*
> > +	 * Use the last context bank for identity mappings during boot, to
> > +	 * avoid overwriting in-use bank configuration while we're setting up
> > +	 * the new mappings.
> > +	 */
> > +	if (boot_domain)
> > +		start = smmu->num_context_banks - 1;
> > +
> >  	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
> >  				      smmu->num_context_banks);
> >  	if (ret < 0)
> > @@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >  	struct arm_smmu_master_cfg *cfg;
> >  	struct arm_smmu_device *smmu;
> > +	bool free_identity_domain = false;
> > +	int idx;
> >  	int ret;
> > +	int i;
> >  
> >  	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
> >  		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
> > @@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  		return ret;
> >  
> >  	/* Ensure that the domain is finalised */
> > -	ret = arm_smmu_init_domain_context(domain, smmu);
> > +	ret = arm_smmu_init_domain_context(domain, smmu, false);
> >  	if (ret < 0)
> >  		goto rpm_put;
> >  
> > @@ -1190,9 +1203,34 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> >  		goto rpm_put;
> >  	}
> >  
> > +	/* Decrement use counter for any references to the identity domain */
> > +	mutex_lock(&smmu->stream_map_mutex);
> > +	if (smmu->identity) {
> > +		struct arm_smmu_domain *identity = to_smmu_domain(smmu->identity);
> > +
> > +		for_each_cfg_sme(cfg, fwspec, i, idx) {
> > +			dev_err(smmu->dev, "%s() %#x\n", __func__, smmu->smrs[idx].id);
> 
> Debug leftovers?
> 

Indeed.

> Apart from that I plan to give this a go on some NXP chips. Will keep
> you updated.
> 

Thanks, I'm expecting that all you need is the first patch in the series
and some impl specific cfg_probe that sets up (or read back) the
relevant SMRs and mark them valid.

Regards,
Bjorn

> Thanks a lot Bjorn.
> 

> ---
> Best Regards, Laurentiu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 0/5] iommu/arm-smmu: Support maintaining bootloader mappings

[John Stultz]

On Wed, Jul 8, 2020 at 10:02 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Based on previous attempts and discussions this is the latest attempt at
> inheriting stream mappings set up by the bootloader, for e.g. boot splash or
> efifb.
>
> The first patch is an implementation of Robin's suggestion that we should just
> mark the relevant stream mappings as BYPASS. Relying on something else to set
> up the stream mappings wanted - e.g. by reading it back in platform specific
> implementation code.
>
> The series then tackles the problem seen in most versions of Qualcomm firmware,
> that the hypervisor intercepts BYPASS writes and turn them into FAULTs. It does
> this by allocating context banks for identity domains as well, with translation
> disabled.
>
> Lastly it amends the stream mapping initialization code to allocate a specific
> identity domain that is used for any mappings inherited from the bootloader, if
> above Qualcomm quirk is required.
>
>
> The series has been tested and shown to allow booting SDM845, SDM850, SM8150,
> SM8250 with boot splash screen setup by the bootloader. Specifically it also
> allows the Lenovo Yoga C630 to boot with SMMU and efifb enabled.

This series allows the db845c to boot successfully! (Without it we crash!)
It would be really great to have this upstream!

For the series:
  Tested-by: John Stultz <john.stultz@linaro.org>

Thanks so much!
-john
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5] iommu/arm-smmu: Make all valid stream mappings BYPASS

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

Hi Bjorn,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iommu/next]
[also build test WARNING on arm-perf/for-next/perf v5.8-rc5 next-20200713]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bjorn-Andersson/iommu-arm-smmu-Support-maintaining-bootloader-mappings/20200709-130417
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: x86_64-randconfig-s021-20200713 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-37-gc9676a3b-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/iommu/arm-smmu.c:1927:5: sparse: sparse: symbol 'arm_smmu_setup_identity' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 45554 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

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

[RFC PATCH] iommu/arm-smmu: arm_smmu_setup_identity() can be static

[kernel test robot]

Signed-off-by: kernel test robot <lkp@intel.com>
---
 arm-smmu.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 2e27cf9815ab6..fb85e716ae9ac 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1924,7 +1924,7 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
-int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
+static int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
 {
 	int i;
 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 1/5] iommu/arm-smmu: Make all valid stream mappings BYPASS

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]

Hi Bjorn,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iommu/next]
[also build test WARNING on arm-perf/for-next/perf v5.8-rc5 next-20200715]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bjorn-Andersson/iommu-arm-smmu-Support-maintaining-bootloader-mappings/20200709-130417
base:   https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: arm64-randconfig-r022-20200715 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 02946de3802d3bc65bc9f2eb9b8d4969b5a7add8)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/iommu/arm-smmu.c:1927:5: warning: no previous prototype for function 'arm_smmu_setup_identity' [-Wmissing-prototypes]
   int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
       ^
   drivers/iommu/arm-smmu.c:1927:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
   ^
   static 
   1 warning generated.

vim +/arm_smmu_setup_identity +1927 drivers/iommu/arm-smmu.c

  1926	
> 1927	int arm_smmu_setup_identity(struct arm_smmu_device *smmu)
  1928	{
  1929		int i;
  1930	
  1931		for (i = 0; i < smmu->num_mapping_groups; i++) {
  1932			if (smmu->smrs[i].valid) {
  1933				smmu->s2crs[i].type = S2CR_TYPE_BYPASS;
  1934				smmu->s2crs[i].privcfg = S2CR_PRIVCFG_DEFAULT;
  1935				smmu->s2crs[i].cbndx = 0xff;
  1936				smmu->s2crs[i].count++;
  1937			}
  1938		}
  1939	
  1940		return 0;
  1941	}
  1942	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35323 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

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

Re: [PATCH 5/5] iommu/arm-smmu: Setup identity domain for boot mappings

[Laurentiu Tudor]

On 7/9/2020 10:57 PM, Bjorn Andersson wrote:
> On Thu 09 Jul 08:50 PDT 2020, Laurentiu Tudor wrote:
> 
>>
>>
>> On 7/9/2020 8:01 AM, Bjorn Andersson wrote:
>>> With many Qualcomm platforms not having functional S2CR BYPASS a
>>> temporary IOMMU domain, without translation, needs to be allocated in
>>> order to allow these memory transactions.
>>>
>>> Unfortunately the boot loader uses the first few context banks, so
>>> rather than overwriting a active bank the last context bank is used and
>>> streams are diverted here during initialization.
>>>
>>> This also performs the readback of SMR registers for the Qualcomm
>>> platform, to trigger the mechanism.
>>>
>>> This is based on prior work by Thierry Reding and Laurentiu Tudor.
>>>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>  drivers/iommu/arm-smmu-qcom.c | 11 +++++
>>>  drivers/iommu/arm-smmu.c      | 80 +++++++++++++++++++++++++++++++++--
>>>  drivers/iommu/arm-smmu.h      |  3 ++
>>>  3 files changed, 90 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu-qcom.c b/drivers/iommu/arm-smmu-qcom.c
>>> index 86b1917459a4..397df27c1d69 100644
>>> --- a/drivers/iommu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm-smmu-qcom.c
>>> @@ -26,6 +26,7 @@ static const struct of_device_id qcom_smmu_client_of_match[] = {
>>>  static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>>>  {
>>>  	unsigned int last_s2cr = ARM_SMMU_GR0_S2CR(smmu->num_mapping_groups - 1);
>>> +	u32 smr;
>>>  	u32 reg;
>>>  	int i;
>>>  
>>> @@ -56,6 +57,16 @@ static int qcom_smmu_cfg_probe(struct arm_smmu_device *smmu)
>>>  		}
>>>  	}
>>>  
>>> +	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;
>>> +		}
>>> +	}
>>> +
>>>  	return 0;
>>>  }
>>>  
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index e2d6c0aaf1ea..a7cb27c1a49e 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -652,7 +652,8 @@ static void arm_smmu_write_context_bank(struct arm_smmu_device *smmu, int idx)
>>>  }
>>>  
>>>  static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>> -					struct arm_smmu_device *smmu)
>>> +					struct arm_smmu_device *smmu,
>>> +					bool boot_domain)
>>>  {
>>>  	int irq, start, ret = 0;
>>>  	unsigned long ias, oas;
>>> @@ -770,6 +771,15 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>>  		ret = -EINVAL;
>>>  		goto out_unlock;
>>>  	}
>>> +
>>> +	/*
>>> +	 * Use the last context bank for identity mappings during boot, to
>>> +	 * avoid overwriting in-use bank configuration while we're setting up
>>> +	 * the new mappings.
>>> +	 */
>>> +	if (boot_domain)
>>> +		start = smmu->num_context_banks - 1;
>>> +
>>>  	ret = __arm_smmu_alloc_bitmap(smmu->context_map, start,
>>>  				      smmu->num_context_banks);
>>>  	if (ret < 0)
>>> @@ -1149,7 +1159,10 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>  	struct arm_smmu_master_cfg *cfg;
>>>  	struct arm_smmu_device *smmu;
>>> +	bool free_identity_domain = false;
>>> +	int idx;
>>>  	int ret;
>>> +	int i;
>>>  
>>>  	if (!fwspec || fwspec->ops != &arm_smmu_ops) {
>>>  		dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
>>> @@ -1174,7 +1187,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>  		return ret;
>>>  
>>>  	/* Ensure that the domain is finalised */
>>> -	ret = arm_smmu_init_domain_context(domain, smmu);
>>> +	ret = arm_smmu_init_domain_context(domain, smmu, false);
>>>  	if (ret < 0)
>>>  		goto rpm_put;
>>>  
>>> @@ -1190,9 +1203,34 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>>>  		goto rpm_put;
>>>  	}
>>>  
>>> +	/* Decrement use counter for any references to the identity domain */
>>> +	mutex_lock(&smmu->stream_map_mutex);
>>> +	if (smmu->identity) {
>>> +		struct arm_smmu_domain *identity = to_smmu_domain(smmu->identity);
>>> +
>>> +		for_each_cfg_sme(cfg, fwspec, i, idx) {
>>> +			dev_err(smmu->dev, "%s() %#x\n", __func__, smmu->smrs[idx].id);
>>
>> Debug leftovers?
>>
> 
> Indeed.
> 
>> Apart from that I plan to give this a go on some NXP chips. Will keep
>> you updated.
>>
> 
> Thanks, I'm expecting that all you need is the first patch in the series
> and some impl specific cfg_probe that sets up (or read back) the
> relevant SMRs and mark them valid.
> 

I finally managed to give a go to the v2 of this patchset and tested it
on a NXP LS2088A chip with the diff [1] below, so please feel free to add:

Tested-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>

Now a question for the SMMU folks: would the approach in the diff below
be acceptable?

[1]

--- a/drivers/iommu/arm-smmu-impl.c
+++ b/drivers/iommu/arm-smmu-impl.c
@@ -102,6 +102,32 @@ static struct arm_smmu_device
*cavium_smmu_impl_init(struct arm_smmu_device *smm
        return &cs->smmu;
 }

+static int nxp_cfg_probe(struct arm_smmu_device *smmu)
+{
+       int i, cnt = 0;
+       u32 smr;
+
+       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;
+
+                       cnt++;
+               }
+       }
+
+       dev_notice(smmu->dev, "\tpreserved %d boot mapping%s\n", cnt,
+                  cnt == 1 ? "" : "s");
+
+       return 0;
+}
+
+static const struct arm_smmu_impl nxp_impl = {
+       .cfg_probe = nxp_cfg_probe,
+};

 #define ARM_MMU500_ACTLR_CPRE          (1 << 1)

@@ -171,6 +197,9 @@ struct arm_smmu_device *arm_smmu_impl_init(struct
arm_smmu_device *smmu)
        if (of_property_read_bool(np, "calxeda,smmu-secure-config-access"))
                smmu->impl = &calxeda_impl;

+       if (of_property_read_bool(np, "nxp,keep-bypass-mappings"))
+               smmu->impl = &nxp_impl;
+
        if (of_device_is_compatible(np, "qcom,sdm845-smmu-500") ||
            of_device_is_compatible(np, "qcom,sc7180-smmu-500"))
                return qcom_smmu_impl_init(smmu);

---
Thanks & Best Regards, Laurentiu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu