[PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500

[PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

Some Qualcomm Family-B SoCs have got a different version of the QCOM
IOMMU, specifically v2 and 500, which perfectly adhere to the current
qcom_iommu driver, but need some variations due to slightly different
hypervisor behavior.

The personal aim is to upstream MSM8956 as much as possible.

This code has been tested on two Sony phones featuring the Qualcomm
MSM8956 SoC.

Changes in v2:
- Fixed optional properties placement in documentation

Changes in v3:
- Rebased onto linux-next 01/10/2019
- Added missing SCM commit (required by the AArch64 PT switch support)

Changes in v4:
- Removed rej files from the SCM patch (I'm truly sorry for the noise...)

Angelo G. Del Regno (1):
  firmware: qcom: scm: Add function to set IOMMU pagetable addressing

AngeloGioacchino Del Regno (6):
  iommu/qcom: Use the asid read from device-tree if specified
  iommu/qcom: Write TCR before TTBRs to fix ASID access behavior
  iommu/qcom: Properly reset the IOMMU context
  iommu/qcom: Add support for AArch64 IOMMU pagetables
  iommu/qcom: Index contexts by asid number to allow asid 0
  iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts

 .../devicetree/bindings/iommu/qcom,iommu.txt  |   5 +
 drivers/firmware/qcom_scm-32.c                |   6 +
 drivers/firmware/qcom_scm-64.c                |  15 ++
 drivers/firmware/qcom_scm.c                   |   7 +
 drivers/firmware/qcom_scm.h                   |   4 +
 drivers/iommu/qcom_iommu.c                    | 134 ++++++++++++++----
 include/linux/qcom_scm.h                      |   2 +
 7 files changed, 145 insertions(+), 28 deletions(-)

-- 
2.21.0

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

[PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing

[kholk11]

From: "Angelo G. Del Regno" <kholk11@gmail.com>

Add a function to change the IOMMU pagetable addressing to
AArch32 LPAE or AArch64. If doing that, then this must be
done for each IOMMU context (not necessarily at the same time).
---
 drivers/firmware/qcom_scm-32.c |  6 ++++++
 drivers/firmware/qcom_scm-64.c | 15 +++++++++++++++
 drivers/firmware/qcom_scm.c    |  7 +++++++
 drivers/firmware/qcom_scm.h    |  4 ++++
 include/linux/qcom_scm.h       |  2 ++
 5 files changed, 34 insertions(+)

diff --git a/drivers/firmware/qcom_scm-32.c b/drivers/firmware/qcom_scm-32.c
index 215061c581e1..f5437e5c8c4e 100644
--- a/drivers/firmware/qcom_scm-32.c
+++ b/drivers/firmware/qcom_scm-32.c
@@ -597,6 +597,12 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 	return -ENODEV;
 }
 
+int __qcom_scm_iommu_set_pt_format(struct device *dev, u32 sec_id, u32 ctx_num,
+				   u32 pt_fmt)
+{
+	return -ENODEV;
+}
+
 int __qcom_scm_io_readl(struct device *dev, phys_addr_t addr,
 			unsigned int *val)
 {
diff --git a/drivers/firmware/qcom_scm-64.c b/drivers/firmware/qcom_scm-64.c
index 91d5ad7cf58b..aab5446ea9f2 100644
--- a/drivers/firmware/qcom_scm-64.c
+++ b/drivers/firmware/qcom_scm-64.c
@@ -459,6 +459,21 @@ int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr, u32 size,
 	return ret;
 }
 
+int __qcom_scm_iommu_set_pt_format(struct device *dev, u32 sec_id, u32 ctx_num,
+				   u32 pt_fmt)
+{
+	struct qcom_scm_desc desc = {0};
+	struct arm_smccc_res res;
+
+	desc.args[0] = sec_id;
+	desc.args[1] = ctx_num;
+	desc.args[2] = pt_fmt; /* 0: LPAE AArch32 - 1: AArch64 */
+	desc.arginfo = QCOM_SCM_ARGS(3);
+
+	return qcom_scm_call(dev, QCOM_SCM_SVC_SMMU_PROGRAM,
+			     QCOM_SCM_IOMMU_PT_FORMAT, &desc, &res);
+}
+
 int __qcom_scm_set_dload_mode(struct device *dev, bool enable)
 {
 	struct qcom_scm_desc desc = {0};
diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c
index 4802ab170fe5..41af7dc0b34d 100644
--- a/drivers/firmware/qcom_scm.c
+++ b/drivers/firmware/qcom_scm.c
@@ -345,6 +345,13 @@ int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
 }
 EXPORT_SYMBOL(qcom_scm_iommu_secure_ptbl_init);
 
+int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt)
+{
+	return __qcom_scm_iommu_set_pt_format(__scm->dev, sec_id,
+					      ctx_num, pt_fmt);
+}
+EXPORT_SYMBOL(qcom_scm_iommu_set_pt_format);
+
 int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
 {
 	return __qcom_scm_io_readl(__scm->dev, addr, val);
diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h
index 99506bd873c0..f04a3903dc6c 100644
--- a/drivers/firmware/qcom_scm.h
+++ b/drivers/firmware/qcom_scm.h
@@ -95,6 +95,10 @@ extern int __qcom_scm_iommu_secure_ptbl_size(struct device *dev, u32 spare,
 					     size_t *size);
 extern int __qcom_scm_iommu_secure_ptbl_init(struct device *dev, u64 addr,
 					     u32 size, u32 spare);
+#define QCOM_SCM_SVC_SMMU_PROGRAM	0x15
+#define QCOM_SCM_IOMMU_PT_FORMAT	1
+extern int __qcom_scm_iommu_set_pt_format(struct device *dev, u32 sec_id,
+					  u32 ctx_num, u32 pt_fmt);
 #define QCOM_MEM_PROT_ASSIGN_ID	0x16
 extern int  __qcom_scm_assign_mem(struct device *dev,
 				  phys_addr_t mem_region, size_t mem_sz,
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index 2d5eff506e13..49937d62e462 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -58,6 +58,7 @@ extern int qcom_scm_set_remote_state(u32 state, u32 id);
 extern int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare);
 extern int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size);
 extern int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare);
+extern int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt);
 extern int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val);
 extern int qcom_scm_io_writel(phys_addr_t addr, unsigned int val);
 #else
@@ -97,6 +98,7 @@ qcom_scm_set_remote_state(u32 state,u32 id) { return -ENODEV; }
 static inline int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare) { return -ENODEV; }
 static inline int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size) { return -ENODEV; }
 static inline int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare) { return -ENODEV; }
+static inline int qcom_scm_iommu_set_pt_format(u32 sec_id, u32 ctx_num, u32 pt_fmt) { return -ENODEV; }
 static inline int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val) { return -ENODEV; }
 static inline int qcom_scm_io_writel(phys_addr_t addr, unsigned int val) { return -ENODEV; }
 #endif
-- 
2.21.0

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

[PATCH v4 2/7] iommu/qcom: Use the asid read from device-tree if specified

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

As specified in this driver, the context banks are 0x1000 apart.
Problem is that sometimes the context number (our asid) does not
match this logic and we end up using the wrong one: this starts
being a problem in the case that we need to send TZ commands
to do anything on a specific context.

For this reason, read the ASID from the DT if the property
"qcom,ctx-num" is present on the IOMMU context node.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 .../devicetree/bindings/iommu/qcom,iommu.txt    |  1 +
 drivers/iommu/qcom_iommu.c                      | 17 ++++++++++++++---
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 059139abce35..ba0b77889f02 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -46,6 +46,7 @@ to non-secure vs secure interrupt line.
                      for routing of context bank irq's to secure vs non-
                      secure lines.  (Ie. if the iommu contains secure
                      context banks)
+- qcom,ctx-num     : The number associated to the context bank
 
 
 ** Examples:
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index c31e7bc4ccbe..504ca69adc5a 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -585,7 +585,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 	 * index into qcom_iommu->ctxs:
 	 */
 	if (WARN_ON(asid < 1) ||
-	    WARN_ON(asid > qcom_iommu->num_ctxs))
+	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
+	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL))
 		return -EINVAL;
 
 	if (!fwspec->iommu_priv) {
@@ -693,7 +694,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
 
 static int get_asid(const struct device_node *np)
 {
-	u32 reg;
+	u32 reg, val;
+	int asid;
 
 	/* read the "reg" property directly to get the relative address
 	 * of the context bank, and calculate the asid from that:
@@ -701,7 +703,16 @@ static int get_asid(const struct device_node *np)
 	if (of_property_read_u32_index(np, "reg", 0, &reg))
 		return -ENODEV;
 
-	return reg / 0x1000;      /* context banks are 0x1000 apart */
+	/* Context banks are 0x1000 apart but, in some cases, the ASID
+	 * number doesn't match to this logic and needs to be passed
+	 * from the DT configuration explicitly.
+	 */
+	if (of_property_read_u32(np, "qcom,ctx-num", &val))
+		asid = reg / 0x1000;
+	else
+		asid = val;
+
+	return asid;
 }
 
 static int qcom_iommu_ctx_probe(struct platform_device *pdev)
-- 
2.21.0

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

[PATCH v4 3/7] iommu/qcom: Write TCR before TTBRs to fix ASID access behavior

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

As also stated in the arm-smmu driver, we must write the TCR before
writing the TTBRs, since the TCR determines the access behavior of
some fields.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 drivers/iommu/qcom_iommu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index 504ca69adc5a..c8957ec83b92 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -267,6 +267,13 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		/* TCR */
+		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
+				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
+				FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM));
+		iommu_writel(ctx, ARM_SMMU_CB_TCR,
+				pgtbl_cfg.arm_lpae_s1_cfg.tcr);
+
 		/* TTBRs */
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
 				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[0] |
@@ -275,13 +282,6 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 				pgtbl_cfg.arm_lpae_s1_cfg.ttbr[1] |
 				FIELD_PREP(TTBRn_ASID, ctx->asid));
 
-		/* TCR */
-		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
-				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
-				FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM));
-		iommu_writel(ctx, ARM_SMMU_CB_TCR,
-				pgtbl_cfg.arm_lpae_s1_cfg.tcr);
-
 		/* MAIRs (stage-1 only) */
 		iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0,
 				pgtbl_cfg.arm_lpae_s1_cfg.mair[0]);
-- 
2.21.0

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

[PATCH v4 4/7] iommu/qcom: Properly reset the IOMMU context

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

To avoid context faults reset the context entirely on detach and
to ensure a fresh clean start also do a complete reset before
programming the context for domain initialization.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 drivers/iommu/qcom_iommu.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index c8957ec83b92..b4a38ef129e3 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -220,6 +220,23 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
 	return IRQ_HANDLED;
 }
 
+static void qcom_iommu_reset_ctx(struct qcom_iommu_ctx *ctx)
+{
+	iommu_writel(ctx, ARM_SMMU_CB_FAR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_FSR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR1, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_PAR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_TCR2, 0);
+	iommu_writel(ctx, ARM_SMMU_CB_TCR, 0);
+	iommu_writeq(ctx, ARM_SMMU_CB_TTBR0, 0);
+	iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
+
+	/* Should we issue a TLBSYNC there instead? */
+	mb();
+}
+
 static int qcom_iommu_init_domain(struct iommu_domain *domain,
 				  struct qcom_iommu_dev *qcom_iommu,
 				  struct iommu_fwspec *fwspec)
@@ -267,6 +284,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		qcom_iommu_reset_ctx(ctx);
+
 		/* TCR */
 		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
 				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
@@ -412,8 +431,8 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
 	for (i = 0; i < fwspec->num_ids; i++) {
 		struct qcom_iommu_ctx *ctx = to_ctx(fwspec, fwspec->ids[i]);
 
-		/* Disable the context bank: */
-		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
+		/* Disable and reset the context bank */
+		qcom_iommu_reset_ctx(ctx);
 
 		ctx->domain = NULL;
 	}
-- 
2.21.0

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

[PATCH v4 5/7] iommu/qcom: Add support for AArch64 IOMMU pagetables

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

Some IOMMUs associated with some TZ firmwares may support switching
to the AArch64 pagetable format by sending a "set pagetable format"
scm command indicating the IOMMU secure ID and the context number
to switch.

Add a DT property "qcom,use-aarch64-pagetables" for this driver to
send this command to the secure world and to switch the pagetable
format to benefit of the ARM64 IOMMU pagetables, where possible.

Note that, even though the command should be valid to switch each
context, the property is made global because:
1. It doesn't make too much sense to switch only one or two
   context(s) to AA64 instead of just the entire thing
2. Some IOMMUs will go crazy and produce spectacular results when
   trying to mix up the pagetables on a per-context basis.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 .../devicetree/bindings/iommu/qcom,iommu.txt  |  2 +
 drivers/iommu/qcom_iommu.c                    | 55 +++++++++++++++----
 2 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index ba0b77889f02..72ae0595efff 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -47,6 +47,8 @@ to non-secure vs secure interrupt line.
                      secure lines.  (Ie. if the iommu contains secure
                      context banks)
 - qcom,ctx-num     : The number associated to the context bank
+- qcom,use-aarch64-pagetables : Switch to AArch64 pagetable format on all
+                                contexts declared in this IOMMU
 
 
 ** Examples:
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index b4a38ef129e3..dac77dbec0f0 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -48,6 +48,7 @@ struct qcom_iommu_dev {
 	void __iomem		*local_base;
 	u32			 sec_id;
 	u8			 num_ctxs;
+	bool			 use_aarch64_pt;
 	struct qcom_iommu_ctx	*ctxs[0];   /* indexed by asid-1 */
 };
 
@@ -153,11 +154,17 @@ static void qcom_iommu_tlb_inv_range_nosync(unsigned long iova, size_t size,
 	reg = leaf ? ARM_SMMU_CB_S1_TLBIVAL : ARM_SMMU_CB_S1_TLBIVA;
 
 	for (i = 0; i < fwspec->num_ids; i++) {
+		struct qcom_iommu_dev *qcom_iommu = to_iommu(fwspec);
 		struct qcom_iommu_ctx *ctx = to_ctx(fwspec, fwspec->ids[i]);
 		size_t s = size;
 
-		iova = (iova >> 12) << 12;
-		iova |= ctx->asid;
+		if (qcom_iommu->use_aarch64_pt) {
+			iova >>= 12;
+			iova |= (u64)ctx->asid << 48;
+		} else {
+			iova = (iova >> 12) << 12;
+			iova |= ctx->asid;
+		}
 		do {
 			iommu_writel(ctx, reg, iova);
 			iova += granule;
@@ -244,6 +251,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	struct qcom_iommu_domain *qcom_domain = to_qcom_iommu_domain(domain);
 	struct io_pgtable_ops *pgtbl_ops;
 	struct io_pgtable_cfg pgtbl_cfg;
+	enum io_pgtable_fmt pgtbl_fmt;
+	unsigned long ias, oas;
 	int i, ret = 0;
 	u32 reg;
 
@@ -251,16 +260,25 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 	if (qcom_domain->iommu)
 		goto out_unlock;
 
+	if (qcom_iommu->use_aarch64_pt) {
+		pgtbl_fmt = ARM_64_LPAE_S1;
+		ias = oas = 48;
+	} else {
+		pgtbl_fmt = ARM_32_LPAE_S1;
+		ias = 32;
+		oas = 40;
+	}
+
 	pgtbl_cfg = (struct io_pgtable_cfg) {
 		.pgsize_bitmap	= qcom_iommu_ops.pgsize_bitmap,
-		.ias		= 32,
-		.oas		= 40,
+		.ias		= ias,
+		.oas		= oas,
 		.tlb		= &qcom_flush_ops,
 		.iommu_dev	= qcom_iommu->dev,
 	};
 
 	qcom_domain->iommu = qcom_iommu;
-	pgtbl_ops = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &pgtbl_cfg, fwspec);
+	pgtbl_ops = alloc_io_pgtable_ops(pgtbl_fmt, &pgtbl_cfg, fwspec);
 	if (!pgtbl_ops) {
 		dev_err(qcom_iommu->dev, "failed to allocate pagetable ops\n");
 		ret = -ENOMEM;
@@ -274,6 +292,7 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 	for (i = 0; i < fwspec->num_ids; i++) {
 		struct qcom_iommu_ctx *ctx = to_ctx(fwspec, fwspec->ids[i]);
+		u32 tcr[2];
 
 		if (!ctx->secure_init) {
 			ret = qcom_scm_restore_sec_cfg(qcom_iommu->sec_id, ctx->asid);
@@ -286,12 +305,25 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 
 		qcom_iommu_reset_ctx(ctx);
 
+		tcr[0] = pgtbl_cfg.arm_lpae_s1_cfg.tcr;
+		tcr[1] = pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32;
+		tcr[1] |= FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM);
+
+		if (qcom_iommu->use_aarch64_pt) {
+			/* This shall not fail, or spectacular things happen */
+			if (qcom_scm_iommu_set_pt_format(qcom_iommu->sec_id,
+							 ctx->asid, 1)) {
+				dev_warn(qcom_iommu->dev,
+					 "Cannot set AArch64 pt format\n");
+				goto out_clear_iommu;
+			}
+
+			tcr[1] |= TCR2_AS;
+		}
+
 		/* TCR */
-		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
-				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
-				FIELD_PREP(TCR2_SEP, TCR2_SEP_UPSTREAM));
-		iommu_writel(ctx, ARM_SMMU_CB_TCR,
-				pgtbl_cfg.arm_lpae_s1_cfg.tcr);
+		iommu_writel(ctx, ARM_SMMU_CB_TCR2, tcr[1]);
+		iommu_writel(ctx, ARM_SMMU_CB_TCR, tcr[0]);
 
 		/* TTBRs */
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
@@ -869,6 +901,9 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
+	if (of_property_read_bool(dev->of_node, "qcom,use-aarch64-pagetables"))
+		qcom_iommu->use_aarch64_pt = true;
+
 	if (qcom_iommu_has_secure_context(qcom_iommu)) {
 		ret = qcom_iommu_sec_ptbl_init(dev);
 		if (ret) {
-- 
2.21.0

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

[PATCH v4 6/7] iommu/qcom: Index contexts by asid number to allow asid 0

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

This driver was indexing the contexts by asid-1, which is probably
done under the assumption that the first ASID is always 1.

Unfortunately this is not entirely true: at least in the MSM8956
and MSM8976 GPU IOMMU, the gpu_user context's ASID number is zero.
To allow using an asid number of zero, stop indexing the contexts
by asid-1 and rather index them by asid.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 drivers/iommu/qcom_iommu.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index dac77dbec0f0..555cbc55b073 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -49,7 +49,7 @@ struct qcom_iommu_dev {
 	u32			 sec_id;
 	u8			 num_ctxs;
 	bool			 use_aarch64_pt;
-	struct qcom_iommu_ctx	*ctxs[0];   /* indexed by asid-1 */
+	struct qcom_iommu_ctx	*ctxs[0];   /* indexed by asid */
 };
 
 struct qcom_iommu_ctx {
@@ -87,7 +87,7 @@ static struct qcom_iommu_ctx * to_ctx(struct iommu_fwspec *fwspec, unsigned asid
 	struct qcom_iommu_dev *qcom_iommu = to_iommu(fwspec);
 	if (!qcom_iommu)
 		return NULL;
-	return qcom_iommu->ctxs[asid - 1];
+	return qcom_iommu->ctxs[asid];
 }
 
 static inline void
@@ -632,12 +632,10 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
 	qcom_iommu = platform_get_drvdata(iommu_pdev);
 
 	/* make sure the asid specified in dt is valid, so we don't have
-	 * to sanity check this elsewhere, since 'asid - 1' is used to
-	 * index into qcom_iommu->ctxs:
+	 * to sanity check this elsewhere:
 	 */
-	if (WARN_ON(asid < 1) ||
-	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
-	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL))
+	if (WARN_ON(asid > qcom_iommu->num_ctxs) ||
+	    WARN_ON(qcom_iommu->ctxs[asid] == NULL))
 		return -EINVAL;
 
 	if (!fwspec->iommu_priv) {
@@ -815,7 +813,7 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
 
 	dev_dbg(dev, "found asid %u\n", ctx->asid);
 
-	qcom_iommu->ctxs[ctx->asid - 1] = ctx;
+	qcom_iommu->ctxs[ctx->asid] = ctx;
 
 	return 0;
 }
@@ -827,7 +825,7 @@ static int qcom_iommu_ctx_remove(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, NULL);
 
-	qcom_iommu->ctxs[ctx->asid - 1] = NULL;
+	qcom_iommu->ctxs[ctx->asid] = NULL;
 
 	return 0;
 }
@@ -872,8 +870,8 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	for_each_child_of_node(dev->of_node, child)
 		max_asid = max(max_asid, get_asid(child));
 
-	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs, max_asid),
-				  GFP_KERNEL);
+	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs,
+				  max_asid + 1), GFP_KERNEL);
 	if (!qcom_iommu)
 		return -ENOMEM;
 	qcom_iommu->num_ctxs = max_asid;
-- 
2.21.0

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

[PATCH v4 7/7] iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts

[kholk11]

From: AngeloGioacchino Del Regno <kholk11@gmail.com>

This IOMMU is yet another Qualcomm variant of known IOMMUs, found in
Family-B SoCs, such as MSM8956, MSM8976, MSM8953, MSM8917 and others,
and that firmware perfectly adheres to this driver logic.
This time, though, the catch is that the secure contexts are also
secured, meaning that these are programmed by the bootloader or TZ
and their "interesting" registers are locked out, so the hypervisor
disallows touching them from the non-secure world: in this case
the OS is supposed to blindly trust the secure configuration of
these contexts and just use them "as they are".

For this reason, it is necessary to distinguish between the v1 and
500/v2 secure contexts in this driver in order to adhere to this
specification. To do this, add a new DT compatible, named
"qcom,msm-iommu-v2-sec" that will trigger the new behavior.

For the sake of completeness, also add a "qcom,msm-iommu-v2-ns" so
that the human eye gets pleased with it when reading the contexts
in the final SoC DT. Of course, the latter is just cosmetic.

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
---
 .../devicetree/bindings/iommu/qcom,iommu.txt  |  2 ++
 drivers/iommu/qcom_iommu.c                    | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 72ae0595efff..861c0cd9c512 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -36,6 +36,8 @@ to non-secure vs secure interrupt line.
   - compatible     : Should be one of:
         - "qcom,msm-iommu-v1-ns"  : non-secure context bank
         - "qcom,msm-iommu-v1-sec" : secure context bank
+        - "qcom,msm-iommu-v2-ns"  : non-secure QSMMUv2/QSMMU500 context bank
+        - "qcom,msm-iommu-v2-sec" : secure QSMMUv2/QSMMU500 context bank
   - reg            : Base address and size of context bank within the iommu
   - interrupts     : The context fault irq.
 
diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
index 555cbc55b073..eaecb009849c 100644
--- a/drivers/iommu/qcom_iommu.c
+++ b/drivers/iommu/qcom_iommu.c
@@ -56,6 +56,7 @@ struct qcom_iommu_ctx {
 	struct device		*dev;
 	void __iomem		*base;
 	bool			 secure_init;
+	bool			 secured_ctx;
 	u8			 asid;      /* asid and ctx bank # are 1:1 */
 	struct iommu_domain	*domain;
 };
@@ -303,6 +304,12 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		/* Secured QSMMU-500/QSMMU-v2 contexts cannot be programmed */
+		if (ctx->secured_ctx) {
+			ctx->domain = domain;
+			break;
+		}
+
 		qcom_iommu_reset_ctx(ctx);
 
 		tcr[0] = pgtbl_cfg.arm_lpae_s1_cfg.tcr;
@@ -788,10 +795,15 @@ static int qcom_iommu_ctx_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return -ENODEV;
 
+	if (of_device_is_compatible(dev->of_node, "qcom,msm-iommu-v2-sec"))
+		ctx->secured_ctx = true;
+
 	/* clear IRQs before registering fault handler, just in case the
 	 * boot-loader left us a surprise:
 	 */
-	iommu_writel(ctx, ARM_SMMU_CB_FSR, iommu_readl(ctx, ARM_SMMU_CB_FSR));
+	if (!ctx->secured_ctx)
+		iommu_writel(ctx, ARM_SMMU_CB_FSR,
+			     iommu_readl(ctx, ARM_SMMU_CB_FSR));
 
 	ret = devm_request_irq(dev, irq,
 			       qcom_iommu_fault,
@@ -833,6 +845,8 @@ static int qcom_iommu_ctx_remove(struct platform_device *pdev)
 static const struct of_device_id ctx_of_match[] = {
 	{ .compatible = "qcom,msm-iommu-v1-ns" },
 	{ .compatible = "qcom,msm-iommu-v1-sec" },
+	{ .compatible = "qcom,msm-iommu-v2-ns" },
+	{ .compatible = "qcom,msm-iommu-v2-sec" },
 	{ /* sentinel */ }
 };
 
@@ -850,7 +864,8 @@ static bool qcom_iommu_has_secure_context(struct qcom_iommu_dev *qcom_iommu)
 	struct device_node *child;
 
 	for_each_child_of_node(qcom_iommu->dev->of_node, child)
-		if (of_device_is_compatible(child, "qcom,msm-iommu-v1-sec"))
+		if (of_device_is_compatible(child, "qcom,msm-iommu-v1-sec") ||
+		    of_device_is_compatible(child, "qcom,msm-iommu-v2-sec"))
 			return true;
 
 	return false;
-- 
2.21.0

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

Re: [PATCH v4 4/7] iommu/qcom: Properly reset the IOMMU context

[Robin Murphy]

On 01/10/2019 23:02, kholk11@gmail.com wrote:
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> 
> To avoid context faults reset the context entirely on detach and
> to ensure a fresh clean start also do a complete reset before
> programming the context for domain initialization.
> 
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>   drivers/iommu/qcom_iommu.c | 23 +++++++++++++++++++++--
>   1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> index c8957ec83b92..b4a38ef129e3 100644
> --- a/drivers/iommu/qcom_iommu.c
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -220,6 +220,23 @@ static irqreturn_t qcom_iommu_fault(int irq, void *dev)
>   	return IRQ_HANDLED;
>   }
>   
> +static void qcom_iommu_reset_ctx(struct qcom_iommu_ctx *ctx)
> +{
> +	iommu_writel(ctx, ARM_SMMU_CB_FAR, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_FSR, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR1, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_PAR, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_S1_MAIR0, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_TCR2, 0);
> +	iommu_writel(ctx, ARM_SMMU_CB_TCR, 0);
> +	iommu_writeq(ctx, ARM_SMMU_CB_TTBR0, 0);
> +	iommu_writeq(ctx, ARM_SMMU_CB_TTBR1, 0);
> +
> +	/* Should we issue a TLBSYNC there instead? */

Unless there are outstanding TLB invalidations, it wouldn't achieve 
much. That said, doing a TBLIALL as part of the reset sequence *would* 
generally be a good idea.

Also, since the write to SCTLR affects the overall enable state of the 
context bank, it would be best to do that before touching anything else.

Robin.

> +	mb();
> +}
> +
>   static int qcom_iommu_init_domain(struct iommu_domain *domain,
>   				  struct qcom_iommu_dev *qcom_iommu,
>   				  struct iommu_fwspec *fwspec)
> @@ -267,6 +284,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>   			ctx->secure_init = true;
>   		}
>   
> +		qcom_iommu_reset_ctx(ctx);
> +
>   		/* TCR */
>   		iommu_writel(ctx, ARM_SMMU_CB_TCR2,
>   				(pgtbl_cfg.arm_lpae_s1_cfg.tcr >> 32) |
> @@ -412,8 +431,8 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
>   	for (i = 0; i < fwspec->num_ids; i++) {
>   		struct qcom_iommu_ctx *ctx = to_ctx(fwspec, fwspec->ids[i]);
>   
> -		/* Disable the context bank: */
> -		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
> +		/* Disable and reset the context bank */
> +		qcom_iommu_reset_ctx(ctx);
>   
>   		ctx->domain = NULL;
>   	}
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500

[Bjorn Andersson]

On Tue 01 Oct 15:01 PDT 2019, kholk11@gmail.com wrote:

> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> 
> Some Qualcomm Family-B SoCs have got a different version of the QCOM
> IOMMU, specifically v2 and 500, which perfectly adhere to the current
> qcom_iommu driver, but need some variations due to slightly different
> hypervisor behavior.
> 

Do you think it's out of the question to get the arm-smmu driver to play
nice with this platform?


If not, would it be possible to change the DT binding so that we specify
the SID and then read the SMR and S2CR registers to figure out the
associated context bank?

Regards,
Bjorn

> The personal aim is to upstream MSM8956 as much as possible.
> 
> This code has been tested on two Sony phones featuring the Qualcomm
> MSM8956 SoC.
> 
> Changes in v2:
> - Fixed optional properties placement in documentation
> 
> Changes in v3:
> - Rebased onto linux-next 01/10/2019
> - Added missing SCM commit (required by the AArch64 PT switch support)
> 
> Changes in v4:
> - Removed rej files from the SCM patch (I'm truly sorry for the noise...)
> 
> Angelo G. Del Regno (1):
>   firmware: qcom: scm: Add function to set IOMMU pagetable addressing
> 
> AngeloGioacchino Del Regno (6):
>   iommu/qcom: Use the asid read from device-tree if specified
>   iommu/qcom: Write TCR before TTBRs to fix ASID access behavior
>   iommu/qcom: Properly reset the IOMMU context
>   iommu/qcom: Add support for AArch64 IOMMU pagetables
>   iommu/qcom: Index contexts by asid number to allow asid 0
>   iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts
> 
>  .../devicetree/bindings/iommu/qcom,iommu.txt  |   5 +
>  drivers/firmware/qcom_scm-32.c                |   6 +
>  drivers/firmware/qcom_scm-64.c                |  15 ++
>  drivers/firmware/qcom_scm.c                   |   7 +
>  drivers/firmware/qcom_scm.h                   |   4 +
>  drivers/iommu/qcom_iommu.c                    | 134 ++++++++++++++----
>  include/linux/qcom_scm.h                      |   2 +
>  7 files changed, 145 insertions(+), 28 deletions(-)
> 
> -- 
> 2.21.0
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 0/7] Add support for QCOM IOMMU v2 and 500

[AngeloGioacchino Del Regno]

Il giorno sab 5 ott 2019 alle ore 06:56 Bjorn Andersson
<bjorn.andersson@linaro.org> ha scritto:
>
> On Tue 01 Oct 15:01 PDT 2019, kholk11@gmail.com wrote:
>
> > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> >
> > Some Qualcomm Family-B SoCs have got a different version of the QCOM
> > IOMMU, specifically v2 and 500, which perfectly adhere to the current
> > qcom_iommu driver, but need some variations due to slightly different
> > hypervisor behavior.
> >
>
> Do you think it's out of the question to get the arm-smmu driver to play
> nice with this platform?
>
>
> If not, would it be possible to change the DT binding so that we specify
> the SID and then read the SMR and S2CR registers to figure out the
> associated context bank?
>
> Regards,
> Bjorn
>
> > The personal aim is to upstream MSM8956 as much as possible.
> >
> > This code has been tested on two Sony phones featuring the Qualcomm
> > MSM8956 SoC.
> >
> > Changes in v2:
> > - Fixed optional properties placement in documentation
> >
> > Changes in v3:
> > - Rebased onto linux-next 01/10/2019
> > - Added missing SCM commit (required by the AArch64 PT switch support)
> >
> > Changes in v4:
> > - Removed rej files from the SCM patch (I'm truly sorry for the noise...)
> >
> > Angelo G. Del Regno (1):
> >   firmware: qcom: scm: Add function to set IOMMU pagetable addressing
> >
> > AngeloGioacchino Del Regno (6):
> >   iommu/qcom: Use the asid read from device-tree if specified
> >   iommu/qcom: Write TCR before TTBRs to fix ASID access behavior
> >   iommu/qcom: Properly reset the IOMMU context
> >   iommu/qcom: Add support for AArch64 IOMMU pagetables
> >   iommu/qcom: Index contexts by asid number to allow asid 0
> >   iommu/qcom: Add support for QCIOMMUv2 and QCIOMMU-500 secured contexts
> >
> >  .../devicetree/bindings/iommu/qcom,iommu.txt  |   5 +
> >  drivers/firmware/qcom_scm-32.c                |   6 +
> >  drivers/firmware/qcom_scm-64.c                |  15 ++
> >  drivers/firmware/qcom_scm.c                   |   7 +
> >  drivers/firmware/qcom_scm.h                   |   4 +
> >  drivers/iommu/qcom_iommu.c                    | 134 ++++++++++++++----
> >  include/linux/qcom_scm.h                      |   2 +
> >  7 files changed, 145 insertions(+), 28 deletions(-)
> >
> > --
> > 2.21.0
> >

In reality, when I started the IOMMU integration for this SoC, the
arm-smmu didn't even
have the new arm-smmu-impl stuff....
I tried multiple times to get the arm-smmu driver to play nice with
this IOMMU, but it's
really too much work to do there, (even with the new arm-smmu-impl
stuff) as we would
have to make a lot of changes in that driver just to support
this thing which, yes - it's standard-ish - but no, due to the
firmware configuration that
happens on this kind of platforms (the entire family, 8917, 8953,
8956, 8976 and others)
there is a lil percent of the arm-smmu code that would apply.

Shorter said, since it would be a complete mess to integrate the
support there, IMHO
it's really not a good idea.
In my trials for that I've ended up changing like 50% of the arm-smmu driver.

After all, the qcom_iommu driver is there to get IOMMUs with this kind
of firmware
configuration working and, even if it was originally done for
QCIOMMUv1, as I have
also explained in one of the patches here, 98-99% of the reasons why we have a
separate driver called qcom_iommu are applying to the implementation
that I've done.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing

[Joerg Roedel]

On Wed, Oct 02, 2019 at 12:01:59AM +0200, kholk11@gmail.com wrote:
> From: "Angelo G. Del Regno" <kholk11@gmail.com>
> 
> Add a function to change the IOMMU pagetable addressing to
> AArch32 LPAE or AArch64. If doing that, then this must be
> done for each IOMMU context (not necessarily at the same time).

This patch lacks a Signed-off-by.

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

Re: [PATCH v4 2/7] iommu/qcom: Use the asid read from device-tree if specified

[Robin Murphy]

On 01/10/2019 23:02, kholk11@gmail.com wrote:
> From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> 
> As specified in this driver, the context banks are 0x1000 apart.
> Problem is that sometimes the context number (our asid) does not
> match this logic and we end up using the wrong one: this starts
> being a problem in the case that we need to send TZ commands
> to do anything on a specific context.
> 
> For this reason, read the ASID from the DT if the property
> "qcom,ctx-num" is present on the IOMMU context node.

Can you clarify exactly what the situation is here? Is the hypervisor 
broken in that it presents context banks at made-up offsets and fails to 
correctly associate those offsets with the real underlying hardware 
context in some cases? Or is the hypervisor actually intercepting 
TTBR/CONTEXTIDR/TLBI* writes such that we don't actually have control 
over the ASID used by a given context bank (which I would also consider 
broken)? Or is "context number" a specific thing in the SCM interface 
which represents neither of the above?

At the moment, it seems like a bit too much is being conflated here just 
because of some implicit behaviour in the driver.

Robin.

> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> ---
>   .../devicetree/bindings/iommu/qcom,iommu.txt    |  1 +
>   drivers/iommu/qcom_iommu.c                      | 17 ++++++++++++++---
>   2 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> index 059139abce35..ba0b77889f02 100644
> --- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> +++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> @@ -46,6 +46,7 @@ to non-secure vs secure interrupt line.
>                        for routing of context bank irq's to secure vs non-
>                        secure lines.  (Ie. if the iommu contains secure
>                        context banks)
> +- qcom,ctx-num     : The number associated to the context bank
>   
>   
>   ** Examples:
> diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> index c31e7bc4ccbe..504ca69adc5a 100644
> --- a/drivers/iommu/qcom_iommu.c
> +++ b/drivers/iommu/qcom_iommu.c
> @@ -585,7 +585,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
>   	 * index into qcom_iommu->ctxs:
>   	 */
>   	if (WARN_ON(asid < 1) ||
> -	    WARN_ON(asid > qcom_iommu->num_ctxs))
> +	    WARN_ON(asid > qcom_iommu->num_ctxs) ||
> +	    WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL))
>   		return -EINVAL;
>   
>   	if (!fwspec->iommu_priv) {
> @@ -693,7 +694,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
>   
>   static int get_asid(const struct device_node *np)
>   {
> -	u32 reg;
> +	u32 reg, val;
> +	int asid;
>   
>   	/* read the "reg" property directly to get the relative address
>   	 * of the context bank, and calculate the asid from that:
> @@ -701,7 +703,16 @@ static int get_asid(const struct device_node *np)
>   	if (of_property_read_u32_index(np, "reg", 0, &reg))
>   		return -ENODEV;
>   
> -	return reg / 0x1000;      /* context banks are 0x1000 apart */
> +	/* Context banks are 0x1000 apart but, in some cases, the ASID
> +	 * number doesn't match to this logic and needs to be passed
> +	 * from the DT configuration explicitly.
> +	 */
> +	if (of_property_read_u32(np, "qcom,ctx-num", &val))
> +		asid = reg / 0x1000;
> +	else
> +		asid = val;
> +
> +	return asid;
>   }
>   
>   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing

[AngeloGioacchino Del Regno]

Il giorno mar 15 ott 2019 alle ore 13:14 Joerg Roedel
<joro@8bytes.org> ha scritto:
>
> On Wed, Oct 02, 2019 at 12:01:59AM +0200, kholk11@gmail.com wrote:
> > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> >
> > Add a function to change the IOMMU pagetable addressing to
> > AArch32 LPAE or AArch64. If doing that, then this must be
> > done for each IOMMU context (not necessarily at the same time).
>
> This patch lacks a Signed-off-by.
>

I'm sorry for that. Should I resend or is it enough for me to write it here?

Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing

[Joerg Roedel]

On Tue, Oct 15, 2019 at 02:33:47PM +0200, AngeloGioacchino Del Regno wrote:
> Il giorno mar 15 ott 2019 alle ore 13:14 Joerg Roedel
> <joro@8bytes.org> ha scritto:
> >
> > On Wed, Oct 02, 2019 at 12:01:59AM +0200, kholk11@gmail.com wrote:
> > > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> > >
> > > Add a function to change the IOMMU pagetable addressing to
> > > AArch32 LPAE or AArch64. If doing that, then this must be
> > > done for each IOMMU context (not necessarily at the same time).
> >
> > This patch lacks a Signed-off-by.
> >
> 
> I'm sorry for that. Should I resend or is it enough for me to write it here?
> 
> Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>

Please resend, but you are free to wait for the reviews/acks from Rob
Clark, which I'd like to see on the series.

Regards,

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

Re: [PATCH v4 2/7] iommu/qcom: Use the asid read from device-tree if specified

[AngeloGioacchino Del Regno]

Il giorno mar 15 ott 2019 alle ore 14:10 Robin Murphy
<robin.murphy@arm.com> ha scritto:
>
> On 01/10/2019 23:02, kholk11@gmail.com wrote:
> > From: AngeloGioacchino Del Regno <kholk11@gmail.com>
> >
> > As specified in this driver, the context banks are 0x1000 apart.
> > Problem is that sometimes the context number (our asid) does not
> > match this logic and we end up using the wrong one: this starts
> > being a problem in the case that we need to send TZ commands
> > to do anything on a specific context.
> >
> > For this reason, read the ASID from the DT if the property
> > "qcom,ctx-num" is present on the IOMMU context node.
>
> Can you clarify exactly what the situation is here? Is the hypervisor
> broken in that it presents context banks at made-up offsets and fails to
> correctly associate those offsets with the real underlying hardware
> context in some cases? Or is the hypervisor actually intercepting
> TTBR/CONTEXTIDR/TLBI* writes such that we don't actually have control
> over the ASID used by a given context bank (which I would also consider
> broken)? Or is "context number" a specific thing in the SCM interface
> which represents neither of the above?
>
> At the moment, it seems like a bit too much is being conflated here just
> because of some implicit behaviour in the driver.
>
> Robin.
>

From what I've understood, it's a mix of everything you just said!

This IOMMU implementation is considered "secure IOMMU" from the
Qualcomm's point of view (at least that's what they wrote in their original
downstream driver), having - secured and unsecured - pre-set (by one of the
bootloader stages) contexts.

On some SoCs (or some firmwares, if you prefer) the context number does
not correspond to that "ctx_no = addr / 0x1000" calculation for some reason
which I'm 99.9% sure is that the bootloader just configures them like this...
so, let's make a true and practical example of what happens on MSM8956/76
(and not only on this SoC):

- GFX3D IOMMU
Offset 0 -- CTX number 0
Offset 0x1000 -- CTX number 2
Offset 0x2000 -- CTX number 1

- APPS IOMMU
Offset 0x1000 -- CTX number 1
Offset blahblah -- CTX number sequential until it starts doing...
Offset 0x10000 -- CTX number 14 (!!!)
Offset blahblah -- CTX number sequential again until the end

See how the context numbers are jumping around? That's only part of
the story why we want this kind of handling: of course, with the hypervisor
intercepting writes, we would be doing ops on the wrong context and we'd
see (personal experience, tested that) for example image corruption while
trying to use adreno, or worse "random" reboots because of the hypervisor
faulting on us because it doesn't like us writing on secured context regs like
FSR or trying to reset it.

Another part of the story is on another commit of this patch series, which is
the one that's introducing the switch to the AArch64 pagetable format: when
we want to do this operation, we have to send a request to TZ containing,
like the restore_sec_cfg, the sec_id and the ASID... then the hypervisor will
agree to switch our IOMMU context's pt format.

So, at the end of the day:
1. Hypervisor seems to be broken as you seem to think
2. We don't have control on the ASID. It's preallocated and fixed.
3. The SCM interface also uses the ASID number, for which - again - we
    have no control.

I hope that was a good enough and understandable explaination of
what we have here :))

Sorry for the wall of text!
Angelo


> > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
> > ---
> >   .../devicetree/bindings/iommu/qcom,iommu.txt    |  1 +
> >   drivers/iommu/qcom_iommu.c                      | 17 ++++++++++++++---
> >   2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > index 059139abce35..ba0b77889f02 100644
> > --- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > +++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
> > @@ -46,6 +46,7 @@ to non-secure vs secure interrupt line.
> >                        for routing of context bank irq's to secure vs non-
> >                        secure lines.  (Ie. if the iommu contains secure
> >                        context banks)
> > +- qcom,ctx-num     : The number associated to the context bank
> >
> >
> >   ** Examples:
> > diff --git a/drivers/iommu/qcom_iommu.c b/drivers/iommu/qcom_iommu.c
> > index c31e7bc4ccbe..504ca69adc5a 100644
> > --- a/drivers/iommu/qcom_iommu.c
> > +++ b/drivers/iommu/qcom_iommu.c
> > @@ -585,7 +585,8 @@ static int qcom_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
> >        * index into qcom_iommu->ctxs:
> >        */
> >       if (WARN_ON(asid < 1) ||
> > -         WARN_ON(asid > qcom_iommu->num_ctxs))
> > +         WARN_ON(asid > qcom_iommu->num_ctxs) ||
> > +         WARN_ON(qcom_iommu->ctxs[asid - 1] == NULL))
> >               return -EINVAL;
> >
> >       if (!fwspec->iommu_priv) {
> > @@ -693,7 +694,8 @@ static int qcom_iommu_sec_ptbl_init(struct device *dev)
> >
> >   static int get_asid(const struct device_node *np)
> >   {
> > -     u32 reg;
> > +     u32 reg, val;
> > +     int asid;
> >
> >       /* read the "reg" property directly to get the relative address
> >        * of the context bank, and calculate the asid from that:
> > @@ -701,7 +703,16 @@ static int get_asid(const struct device_node *np)
> >       if (of_property_read_u32_index(np, "reg", 0, &reg))
> >               return -ENODEV;
> >
> > -     return reg / 0x1000;      /* context banks are 0x1000 apart */
> > +     /* Context banks are 0x1000 apart but, in some cases, the ASID
> > +      * number doesn't match to this logic and needs to be passed
> > +      * from the DT configuration explicitly.
> > +      */
> > +     if (of_property_read_u32(np, "qcom,ctx-num", &val))
> > +             asid = reg / 0x1000;
> > +     else
> > +             asid = val;
> > +
> > +     return asid;
> >   }
> >
> >   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 1/7] firmware: qcom: scm: Add function to set IOMMU pagetable addressing

[AngeloGioacchino Del Regno]

Il giorno mar 15 ott 2019 alle ore 14:40 Joerg Roedel
<joro@8bytes.org> ha scritto:
>
> On Tue, Oct 15, 2019 at 02:33:47PM +0200, AngeloGioacchino Del Regno wrote:
> > Il giorno mar 15 ott 2019 alle ore 13:14 Joerg Roedel
> > <joro@8bytes.org> ha scritto:
> > >
> > > On Wed, Oct 02, 2019 at 12:01:59AM +0200, kholk11@gmail.com wrote:
> > > > From: "Angelo G. Del Regno" <kholk11@gmail.com>
> > > >
> > > > Add a function to change the IOMMU pagetable addressing to
> > > > AArch32 LPAE or AArch64. If doing that, then this must be
> > > > done for each IOMMU context (not necessarily at the same time).
> > >
> > > This patch lacks a Signed-off-by.
> > >
> >
> > I'm sorry for that. Should I resend or is it enough for me to write it here?
> >
> > Signed-off-by: AngeloGioacchino Del Regno <kholk11@gmail.com>
>
> Please resend, but you are free to wait for the reviews/acks from Rob
> Clark, which I'd like to see on the series.
>
> Regards,
>
>         Joerg

Okay, no problem. I will wait for the reviews/acks from Rob before
resending in order to limit the amount of emails in everyone's inbox :))

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