[PATCH v3 0/6] Add support for Qualcomm's legacy IOMMU v2

[PATCH v3 0/6] Add support for Qualcomm's legacy IOMMU v2

[AngeloGioacchino Del Regno]

This series adds support for handling "v2" firmware's IOMMU, found
on at least MSM8956 and MSM8976 (some other SoCs also need the same
but I honestly don't remember which ones precisely).

This is strictly required to get functional IOMMUs on these SoCs.

I'm sorry for not performing a much needed schema conversion on
qcom,iommu.txt, but I really didn't have time to do that :-(

This series was tested on Sony Xperia X and X Compact (MSM8956):
ADSP, LPASS, Venus, MSS, MDP and GPU are happy :-)


Changes in v3:
 - Removed useless FSRRESTORE reset and definition as pointed
   out in Robin Murphy's review
 - Fixed qcom,iommu.txt changes: squashed MSM8976 compatible
   string addition with msm-iommu-v2 generics addition

Changes in v2:
 - Added back Marijn's notes (sorry man!)
 - Added ARM_SMMU_CB_FSRRESTORE definition
 - Changed context bank reset to properly set FSR and FSRRESTORE

AngeloGioacchino Del Regno (6):
  dt-bindings: iommu: qcom,iommu: Document qcom,ctx-num property
  iommu/qcom: Use the asid read from device-tree if specified
  iommu/qcom: Properly reset the IOMMU context
  iommu/qcom: Index contexts by asid number to allow asid 0
  dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976
    compatibles
  iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts

 .../devicetree/bindings/iommu/qcom,iommu.txt  |  9 +++
 drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 78 +++++++++++++++----
 2 files changed, 70 insertions(+), 17 deletions(-)

-- 
2.38.1

[PATCH v3 1/6] dt-bindings: iommu: qcom,iommu: Document qcom,ctx-num property

[AngeloGioacchino Del Regno]

Add a new "qcom,ctx-num" property to force an ASID number on IOMMU
contexts where required.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/iommu/qcom,iommu.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 059139abce35..7d4e0a18b08e 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 ASID number associated to the context bank
 
 
 ** Examples:
-- 
2.38.1

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

[AngeloGioacchino Del Regno]

As specified in this driver, the context banks are 0x1000 apart but
on some SoCs the context number does not necessarily match this
logic, hence we end up using the wrong ASID: keeping in mind that
this IOMMU implementation relies heavily on SCM (TZ) calls, it is
mandatory that we communicate the right context number.

Since this is all about how context banks are mapped in firmware,
which may be board dependent (as a different firmware version may
eventually change the expected context bank numbers), introduce a
new property "qcom,ctx-num": when found, the ASID will be forced
as read from the devicetree.

When "qcom,ctx-num" is not found, this driver retains the previous
behavior as to avoid breaking older devicetrees or systems that do
not require forcing ASID numbers.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index bfd7b51eb5db..491a8093f3d6 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -551,7 +551,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)) {
 		put_device(&iommu_pdev->dev);
 		return -EINVAL;
 	}
@@ -638,7 +639,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:
@@ -646,7 +648,17 @@ 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.38.1

[PATCH v3 3/6] iommu/qcom: Properly reset the IOMMU context

[AngeloGioacchino Del Regno]

Avoid context faults by resetting the context(s) entirely at
detach_dev() time and also do the same before programming the
context for domain initialization.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 491a8093f3d6..49f4308f1bd2 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -223,6 +223,20 @@ 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, ARM_SMMU_FSR_FAULT);
+	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);
+}
+
 static int qcom_iommu_init_domain(struct iommu_domain *domain,
 				  struct qcom_iommu_dev *qcom_iommu,
 				  struct device *dev)
@@ -273,6 +287,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
 			ctx->secure_init = true;
 		}
 
+		qcom_iommu_reset_ctx(ctx);
+
 		/* TTBRs */
 		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
 				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
@@ -401,8 +417,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(qcom_domain, 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.38.1

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

[AngeloGioacchino Del Regno]

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 always true: at least for MSM8956 and
MSM8976's GPU IOMMU, the gpu_user context's ASID number is zero.
To allow using a zero asid number, index the contexts by `asid`
instead of by `asid - 1`.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 49f4308f1bd2..94f51cafee17 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -52,7 +52,7 @@ struct qcom_iommu_dev {
 	void __iomem		*local_base;
 	u32			 sec_id;
 	u8			 num_ctxs;
-	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid-1 */
+	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid */
 };
 
 struct qcom_iommu_ctx {
@@ -94,7 +94,7 @@ static struct qcom_iommu_ctx * to_ctx(struct qcom_iommu_domain *d, unsigned asid
 	struct qcom_iommu_dev *qcom_iommu = d->iommu;
 	if (!qcom_iommu)
 		return NULL;
-	return qcom_iommu->ctxs[asid - 1];
+	return qcom_iommu->ctxs[asid];
 }
 
 static inline void
@@ -563,12 +563,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)) {
 		put_device(&iommu_pdev->dev);
 		return -EINVAL;
 	}
@@ -726,7 +724,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;
 }
@@ -738,7 +736,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;
 }
@@ -779,7 +777,7 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct clk *clk;
-	int ret, max_asid = 0;
+	int ret, num_ctxs, max_asid = 0;
 
 	/* find the max asid (which is 1:1 to ctx bank idx), so we know how
 	 * many child ctx devices we have:
@@ -787,11 +785,13 @@ 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),
+	num_ctxs = max_asid + 1;
+
+	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs, num_ctxs),
 				  GFP_KERNEL);
 	if (!qcom_iommu)
 		return -ENOMEM;
-	qcom_iommu->num_ctxs = max_asid;
+	qcom_iommu->num_ctxs = num_ctxs;
 	qcom_iommu->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
2.38.1

[PATCH v3 5/6] dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976 compatibles

[AngeloGioacchino Del Regno]

Add compatible strings for "qcom,msm-iommu-v2" for the inner node,

Add compatible string "qcom,msm-iommu-v2" for the inner node,
along with "qcom,msm8976-iommu" as a first user of it and
"qcom,msm-iommu-v2-ns" and "qcom,msm-iommu-v2-sec" for the context
bank nodes to support Qualcomm's secure fw "SMMU v2" implementation.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 Documentation/devicetree/bindings/iommu/qcom,iommu.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
index 7d4e0a18b08e..a8b42fa45e2d 100644
--- a/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
+++ b/Documentation/devicetree/bindings/iommu/qcom,iommu.txt
@@ -13,6 +13,12 @@ to non-secure vs secure interrupt line.
 
                      Followed by "qcom,msm-iommu-v1".
 
+                     Or it should be one of:
+
+                        "qcom,msm8976-iommu"
+
+                     Followed by "qcom,msm-iommu-v2".
+
 - clock-names      : Should be a pair of "iface" (required for IOMMUs
                      register group access) and "bus" (required for
                      the IOMMUs underlying bus access).
@@ -36,6 +42,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.
 
-- 
2.38.1

[PATCH v3 6/6] iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts

[AngeloGioacchino Del Regno]

On some SoCs like MSM8956, MSM8976 and others, secure contexts are
also secured: these get programmed by the bootloader or TZ (as usual)
but their "interesting" registers are locked out by the hypervisor,
disallowing direct register writes from Linux and, in many cases,
completely disallowing the reprogramming of TTBR, TCR, MAIR and other
registers including, but not limited to, resetting contexts.
This is referred downstream as a "v2" IOMMU but this is effectively
a "v2 firmware configuration" instead.

Luckily, the described behavior of version 2 is effective only on
secure contexts and not on non-secure ones: add support for that,
finally getting a completely working IOMMU on at least MSM8956/76.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
[Marijn: Rebased over next-20221111]
Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/iommu/arm/arm-smmu/qcom_iommu.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
index 94f51cafee17..db7d7cf5cc7d 100644
--- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
+++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
@@ -59,6 +59,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;
 };
@@ -287,6 +288,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;
+			continue;
+		}
+
 		qcom_iommu_reset_ctx(ctx);
 
 		/* TTBRs */
@@ -418,7 +425,8 @@ static void qcom_iommu_detach_dev(struct iommu_domain *domain, struct device *de
 		struct qcom_iommu_ctx *ctx = to_ctx(qcom_domain, fwspec->ids[i]);
 
 		/* Disable and reset the context bank */
-		qcom_iommu_reset_ctx(ctx);
+		if (!ctx->secured_ctx)
+			qcom_iommu_reset_ctx(ctx);
 
 		ctx->domain = NULL;
 	}
@@ -699,10 +707,14 @@ 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,
@@ -744,6 +756,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 */ }
 };
 
@@ -761,7 +775,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")) {
 			of_node_put(child);
 			return true;
 		}
@@ -909,6 +924,7 @@ static const struct dev_pm_ops qcom_iommu_pm_ops = {
 
 static const struct of_device_id qcom_iommu_of_match[] = {
 	{ .compatible = "qcom,msm-iommu-v1" },
+	{ .compatible = "qcom,msm-iommu-v2" },
 	{ /* sentinel */ }
 };
 
-- 
2.38.1

Re: [PATCH v3 5/6] dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976 compatibles

[Krzysztof Kozlowski]

On 15/11/2022 11:11, AngeloGioacchino Del Regno wrote:
> Add compatible strings for "qcom,msm-iommu-v2" for the inner node,
> 
> Add compatible string "qcom,msm-iommu-v2" for the inner node,
> along with "qcom,msm8976-iommu" as a first user of it and
> "qcom,msm-iommu-v2-ns" and "qcom,msm-iommu-v2-sec" for the context
> bank nodes to support Qualcomm's secure fw "SMMU v2" implementation.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  Documentation/devicetree/bindings/iommu/qcom,iommu.txt | 8 ++++++++


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 0/6] Add support for Qualcomm's legacy IOMMU v2

[AngeloGioacchino Del Regno]

Il 15/11/22 11:11, AngeloGioacchino Del Regno ha scritto:
> This series adds support for handling "v2" firmware's IOMMU, found
> on at least MSM8956 and MSM8976 (some other SoCs also need the same
> but I honestly don't remember which ones precisely).
> 
> This is strictly required to get functional IOMMUs on these SoCs.
> 
> I'm sorry for not performing a much needed schema conversion on
> qcom,iommu.txt, but I really didn't have time to do that :-(
> 
> This series was tested on Sony Xperia X and X Compact (MSM8956):
> ADSP, LPASS, Venus, MSS, MDP and GPU are happy :-)
> 
> 

Hello,
this series is really old and got sent and resent many times.
The first time I've sent this one was .. I think in 2019, then, at the
end of 2022, I had some time to actually respin it and send another
three versions. It's been 3 long years :-)
The third version got the last comments addressed.

Since this didn't get any more feedback for 3 months, I'm worried that it
will be forgotten again, hence:

Is there any more feedback? Anything else to fix?
If not, can this be picked, please?

Thank you.

Best regards,
Angelo


> Changes in v3:
>   - Removed useless FSRRESTORE reset and definition as pointed
>     out in Robin Murphy's review
>   - Fixed qcom,iommu.txt changes: squashed MSM8976 compatible
>     string addition with msm-iommu-v2 generics addition
> 
> Changes in v2:
>   - Added back Marijn's notes (sorry man!)
>   - Added ARM_SMMU_CB_FSRRESTORE definition
>   - Changed context bank reset to properly set FSR and FSRRESTORE
> 
> AngeloGioacchino Del Regno (6):
>    dt-bindings: iommu: qcom,iommu: Document qcom,ctx-num property
>    iommu/qcom: Use the asid read from device-tree if specified
>    iommu/qcom: Properly reset the IOMMU context
>    iommu/qcom: Index contexts by asid number to allow asid 0
>    dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976
>      compatibles
>    iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts
> 
>   .../devicetree/bindings/iommu/qcom,iommu.txt  |  9 +++
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 78 +++++++++++++++----
>   2 files changed, 70 insertions(+), 17 deletions(-)
> 

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

[Dmitry Baryshkov]

On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
> As specified in this driver, the context banks are 0x1000 apart but
> on some SoCs the context number does not necessarily match this
> logic, hence we end up using the wrong ASID: keeping in mind that
> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
> mandatory that we communicate the right context number.
> 
> Since this is all about how context banks are mapped in firmware,
> which may be board dependent (as a different firmware version may
> eventually change the expected context bank numbers), introduce a
> new property "qcom,ctx-num": when found, the ASID will be forced
> as read from the devicetree.
> 
> When "qcom,ctx-num" is not found, this driver retains the previous
> behavior as to avoid breaking older devicetrees or systems that do
> not require forcing ASID numbers.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> [Marijn: Rebased over next-20221111]
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index bfd7b51eb5db..491a8093f3d6 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -551,7 +551,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)) {

Separate change in my opinion. Please split it to a separate patch with 
proper Fixes: tag.

>   		put_device(&iommu_pdev->dev);
>   		return -EINVAL;
>   	}
> @@ -638,7 +639,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:
> @@ -646,7 +648,17 @@ 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;

As a matter of preference (and logic) I'd have written that as:

if (!of_property_read(np, "qcom,ctx-num", &val))
     asid = val;
else
     asid = reg / 0x1000;

LGTM otherwise

> +
> +	return asid;
>   }
>   
>   static int qcom_iommu_ctx_probe(struct platform_device *pdev)

-- 
With best wishes
Dmitry

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

[Konrad Dybcio]

On 7.03.2023 17:44, Dmitry Baryshkov wrote:
> On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
>> As specified in this driver, the context banks are 0x1000 apart but
>> on some SoCs the context number does not necessarily match this
>> logic, hence we end up using the wrong ASID: keeping in mind that
>> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
>> mandatory that we communicate the right context number.
>>
>> Since this is all about how context banks are mapped in firmware,
>> which may be board dependent (as a different firmware version may
>> eventually change the expected context bank numbers), introduce a
>> new property "qcom,ctx-num": when found, the ASID will be forced
>> as read from the devicetree.
>>
>> When "qcom,ctx-num" is not found, this driver retains the previous
>> behavior as to avoid breaking older devicetrees or systems that do
>> not require forcing ASID numbers.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> [Marijn: Rebased over next-20221111]
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index bfd7b51eb5db..491a8093f3d6 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -551,7 +551,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)) {
> 
> Separate change in my opinion. Please split it to a separate patch with proper Fixes: tag.
> 
>>           put_device(&iommu_pdev->dev);
>>           return -EINVAL;
>>       }
>> @@ -638,7 +639,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:
>> @@ -646,7 +648,17 @@ static int get_asid(const struct device_node *np)
>>       if (of_property_read_u32_index(np, "reg", 0, &reg))
Use platform_get_resource(pdev, IORESOURCE_MEM, 0); reg = res->start
or something like this, the current approach relies on #address-cells = <1>

Konrad
>>           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;
> 
> As a matter of preference (and logic) I'd have written that as:
> 
> if (!of_property_read(np, "qcom,ctx-num", &val))
>     asid = val;
> else
>     asid = reg / 0x1000;
> 
> LGTM otherwise
> 
>> +
>> +    return asid;
>>   }
>>     static int qcom_iommu_ctx_probe(struct platform_device *pdev)
> 

Re: [PATCH v3 3/6] iommu/qcom: Properly reset the IOMMU context

[Dmitry Baryshkov]

On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
> Avoid context faults by resetting the context(s) entirely at
> detach_dev() time and also do the same before programming the
> context for domain initialization.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> [Marijn: Rebased over next-20221111]
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Probably:
Fixes: 0ae349a0f33f ("iommu/qcom: Add qcom_iommu")

What causes context faults? Shouldn't the context be disabled once we 
write 0 to CB_SCTLR?

> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 491a8093f3d6..49f4308f1bd2 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -223,6 +223,20 @@ 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, ARM_SMMU_FSR_FAULT);
> +	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);
> +}
> +
>   static int qcom_iommu_init_domain(struct iommu_domain *domain,
>   				  struct qcom_iommu_dev *qcom_iommu,
>   				  struct device *dev)
> @@ -273,6 +287,8 @@ static int qcom_iommu_init_domain(struct iommu_domain *domain,
>   			ctx->secure_init = true;
>   		}
>   
> +		qcom_iommu_reset_ctx(ctx);

Is it enough to write 0 to ARM_SMMU_CB_SCTLR here instead of doing the 
full reset?

> +
>   		/* TTBRs */
>   		iommu_writeq(ctx, ARM_SMMU_CB_TTBR0,
>   				pgtbl_cfg.arm_lpae_s1_cfg.ttbr |
> @@ -401,8 +417,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(qcom_domain, 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;
>   	}

-- 
With best wishes
Dmitry

Re: [PATCH v3 4/6] iommu/qcom: Index contexts by asid number to allow asid 0

[Dmitry Baryshkov]

On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
> 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 always true: at least for MSM8956 and
> MSM8976's GPU IOMMU, the gpu_user context's ASID number is zero.
> To allow using a zero asid number, index the contexts by `asid`
> instead of by `asid - 1`.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> [Marijn: Rebased over next-20221111]
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 49f4308f1bd2..94f51cafee17 100644
> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> @@ -52,7 +52,7 @@ struct qcom_iommu_dev {
>   	void __iomem		*local_base;
>   	u32			 sec_id;
>   	u8			 num_ctxs;
> -	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid-1 */
> +	struct qcom_iommu_ctx	*ctxs[];   /* indexed by asid */
>   };
>   
>   struct qcom_iommu_ctx {
> @@ -94,7 +94,7 @@ static struct qcom_iommu_ctx * to_ctx(struct qcom_iommu_domain *d, unsigned asid
>   	struct qcom_iommu_dev *qcom_iommu = d->iommu;
>   	if (!qcom_iommu)
>   		return NULL;
> -	return qcom_iommu->ctxs[asid - 1];
> +	return qcom_iommu->ctxs[asid];
>   }
>   
>   static inline void
> @@ -563,12 +563,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) ||

Could you please change qcom_iommu to store max_asid rather than 
num_ctxs. This piece becomes logical then.

Looks good to me otherwise.

> +	    WARN_ON(qcom_iommu->ctxs[asid] == NULL)) {
>   		put_device(&iommu_pdev->dev);
>   		return -EINVAL;
>   	}
> @@ -726,7 +724,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;
>   }
> @@ -738,7 +736,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;
>   }
> @@ -779,7 +777,7 @@ static int qcom_iommu_device_probe(struct platform_device *pdev)
>   	struct device *dev = &pdev->dev;
>   	struct resource *res;
>   	struct clk *clk;
> -	int ret, max_asid = 0;
> +	int ret, num_ctxs, max_asid = 0;
>   
>   	/* find the max asid (which is 1:1 to ctx bank idx), so we know how
>   	 * many child ctx devices we have:
> @@ -787,11 +785,13 @@ 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),
> +	num_ctxs = max_asid + 1;
> +
> +	qcom_iommu = devm_kzalloc(dev, struct_size(qcom_iommu, ctxs, num_ctxs),
>   				  GFP_KERNEL);
>   	if (!qcom_iommu)
>   		return -ENOMEM;
> -	qcom_iommu->num_ctxs = max_asid;
> +	qcom_iommu->num_ctxs = num_ctxs;
>   	qcom_iommu->dev = dev;
>   
>   	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

-- 
With best wishes
Dmitry

Re: [PATCH v3 6/6] iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts

[Dmitry Baryshkov]

On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
> On some SoCs like MSM8956, MSM8976 and others, secure contexts are
> also secured: these get programmed by the bootloader or TZ (as usual)
> but their "interesting" registers are locked out by the hypervisor,
> disallowing direct register writes from Linux and, in many cases,
> completely disallowing the reprogramming of TTBR, TCR, MAIR and other
> registers including, but not limited to, resetting contexts.
> This is referred downstream as a "v2" IOMMU but this is effectively
> a "v2 firmware configuration" instead.
> 
> Luckily, the described behavior of version 2 is effective only on
> secure contexts and not on non-secure ones: add support for that,
> finally getting a completely working IOMMU on at least MSM8956/76.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> [Marijn: Rebased over next-20221111]
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v3 0/6] Add support for Qualcomm's legacy IOMMU v2

[Luca Weiss]

On Mittwoch, 22. Februar 2023 10:57:47 CEST AngeloGioacchino Del Regno wrote:
> Il 15/11/22 11:11, AngeloGioacchino Del Regno ha scritto:
> > This series adds support for handling "v2" firmware's IOMMU, found
> > on at least MSM8956 and MSM8976 (some other SoCs also need the same
> > but I honestly don't remember which ones precisely).
> > 
> > This is strictly required to get functional IOMMUs on these SoCs.
> > 
> > I'm sorry for not performing a much needed schema conversion on
> > qcom,iommu.txt, but I really didn't have time to do that :-(
> > 
> > This series was tested on Sony Xperia X and X Compact (MSM8956):
> > ADSP, LPASS, Venus, MSS, MDP and GPU are happy :-)
> 
> Hello,
> this series is really old and got sent and resent many times.
> The first time I've sent this one was .. I think in 2019, then, at the
> end of 2022, I had some time to actually respin it and send another
> three versions. It's been 3 long years :-)
> The third version got the last comments addressed.
> 
> Since this didn't get any more feedback for 3 months, I'm worried that it
> will be forgotten again, hence:
> 
> Is there any more feedback? Anything else to fix?
> If not, can this be picked, please?

Hi Angelo,

there's some open review comments since March now on this series. Since some 
of these patches are also needed for msm8953 and msm8974 IOMMU it would be 
nice if you could respin :)

Regards
Luca

> 
> Thank you.
> 
> Best regards,
> Angelo
> 
> > Changes in v3:
> >   - Removed useless FSRRESTORE reset and definition as pointed
> >   
> >     out in Robin Murphy's review
> >   
> >   - Fixed qcom,iommu.txt changes: squashed MSM8976 compatible
> >   
> >     string addition with msm-iommu-v2 generics addition
> > 
> > Changes in v2:
> >   - Added back Marijn's notes (sorry man!)
> >   - Added ARM_SMMU_CB_FSRRESTORE definition
> >   - Changed context bank reset to properly set FSR and FSRRESTORE
> > 
> > AngeloGioacchino Del Regno (6):
> >    dt-bindings: iommu: qcom,iommu: Document qcom,ctx-num property
> >    iommu/qcom: Use the asid read from device-tree if specified
> >    iommu/qcom: Properly reset the IOMMU context
> >    iommu/qcom: Index contexts by asid number to allow asid 0
> >    dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976
> >    
> >      compatibles
> >    
> >    iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts
> >   
> >   .../devicetree/bindings/iommu/qcom,iommu.txt  |  9 +++
> >   drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 78 +++++++++++++++----
> >   2 files changed, 70 insertions(+), 17 deletions(-)

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

[AngeloGioacchino Del Regno]

Il 07/03/23 17:44, Dmitry Baryshkov ha scritto:
> On 15/11/2022 12:11, AngeloGioacchino Del Regno wrote:
>> As specified in this driver, the context banks are 0x1000 apart but
>> on some SoCs the context number does not necessarily match this
>> logic, hence we end up using the wrong ASID: keeping in mind that
>> this IOMMU implementation relies heavily on SCM (TZ) calls, it is
>> mandatory that we communicate the right context number.
>>
>> Since this is all about how context banks are mapped in firmware,
>> which may be board dependent (as a different firmware version may
>> eventually change the expected context bank numbers), introduce a
>> new property "qcom,ctx-num": when found, the ASID will be forced
>> as read from the devicetree.
>>
>> When "qcom,ctx-num" is not found, this driver retains the previous
>> behavior as to avoid breaking older devicetrees or systems that do
>> not require forcing ASID numbers.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> [Marijn: Rebased over next-20221111]
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/iommu/arm/arm-smmu/qcom_iommu.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c 
>> b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> index bfd7b51eb5db..491a8093f3d6 100644
>> --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
>> @@ -551,7 +551,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)) {
> 
> Separate change in my opinion. Please split it to a separate patch with proper 
> Fixes: tag.
> 

This is of_xlate: the array entry at [asid - 1] is always initialized before
the introduction of `qcom,ctx-num`, so this is not a separate change and it
does not require a Fixes tag, as it is not fixing a previous behavior, but
accounting for a new one.

>>           put_device(&iommu_pdev->dev);
>>           return -EINVAL;
>>       }
>> @@ -638,7 +639,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:
>> @@ -646,7 +648,17 @@ 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;
> 
> As a matter of preference (and logic) I'd have written that as:
> 
> if (!of_property_read(np, "qcom,ctx-num", &val))
>      asid = val;
> else
>      asid = reg / 0x1000;
> 
> LGTM otherwise
> 

Will do!

Thanks,
Angelo

P.S.: Sorry for the very late reply.

>> +
>> +    return asid;
>>   }
>>   static int qcom_iommu_ctx_probe(struct platform_device *pdev)
> 

Re: [PATCH v3 0/6] Add support for Qualcomm's legacy IOMMU v2

[AngeloGioacchino Del Regno]

Il 19/06/23 23:42, Luca Weiss ha scritto:
> On Mittwoch, 22. Februar 2023 10:57:47 CEST AngeloGioacchino Del Regno wrote:
>> Il 15/11/22 11:11, AngeloGioacchino Del Regno ha scritto:
>>> This series adds support for handling "v2" firmware's IOMMU, found
>>> on at least MSM8956 and MSM8976 (some other SoCs also need the same
>>> but I honestly don't remember which ones precisely).
>>>
>>> This is strictly required to get functional IOMMUs on these SoCs.
>>>
>>> I'm sorry for not performing a much needed schema conversion on
>>> qcom,iommu.txt, but I really didn't have time to do that :-(
>>>
>>> This series was tested on Sony Xperia X and X Compact (MSM8956):
>>> ADSP, LPASS, Venus, MSS, MDP and GPU are happy :-)
>>
>> Hello,
>> this series is really old and got sent and resent many times.
>> The first time I've sent this one was .. I think in 2019, then, at the
>> end of 2022, I had some time to actually respin it and send another
>> three versions. It's been 3 long years :-)
>> The third version got the last comments addressed.
>>
>> Since this didn't get any more feedback for 3 months, I'm worried that it
>> will be forgotten again, hence:
>>
>> Is there any more feedback? Anything else to fix?
>> If not, can this be picked, please?
> 
> Hi Angelo,
> 
> there's some open review comments since March now on this series. Since some
> of these patches are also needed for msm8953 and msm8974 IOMMU it would be
> nice if you could respin :)
> 

Hello Luca,

I've just sent a v4, but I'm sorry I forgot to Cc you. Please find it at [1].

[1]: 
https://lore.kernel.org/all/20230620095127.96600-1-angelogioacchino.delregno@collabora.com/

Cheers,
Angelo

> Regards
> Luca
> 
>>
>> Thank you.
>>
>> Best regards,
>> Angelo
>>
>>> Changes in v3:
>>>    - Removed useless FSRRESTORE reset and definition as pointed
>>>    
>>>      out in Robin Murphy's review
>>>    
>>>    - Fixed qcom,iommu.txt changes: squashed MSM8976 compatible
>>>    
>>>      string addition with msm-iommu-v2 generics addition
>>>
>>> Changes in v2:
>>>    - Added back Marijn's notes (sorry man!)
>>>    - Added ARM_SMMU_CB_FSRRESTORE definition
>>>    - Changed context bank reset to properly set FSR and FSRRESTORE
>>>
>>> AngeloGioacchino Del Regno (6):
>>>     dt-bindings: iommu: qcom,iommu: Document qcom,ctx-num property
>>>     iommu/qcom: Use the asid read from device-tree if specified
>>>     iommu/qcom: Properly reset the IOMMU context
>>>     iommu/qcom: Index contexts by asid number to allow asid 0
>>>     dt-bindings: iommu: qcom,iommu: Document QSMMUv2 and MSM8976
>>>     
>>>       compatibles
>>>     
>>>     iommu/qcom: Add support for QSMMUv2 and QSMMU-500 secured contexts
>>>    
>>>    .../devicetree/bindings/iommu/qcom,iommu.txt  |  9 +++
>>>    drivers/iommu/arm/arm-smmu/qcom_iommu.c       | 78 +++++++++++++++----
>>>    2 files changed, 70 insertions(+), 17 deletions(-)
> 
> 
> 
> 
>