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

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 22.06.2023 11:27, 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>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  drivers/iommu/arm/arm-smmu/qcom_iommu.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c
> index 9786fd094e7d..7b6241f36698 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;
>  };
> @@ -273,6 +274,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;
> +		}
> +
>  		/* Disable context bank before programming */
>  		iommu_writel(ctx, ARM_SMMU_CB_SCTLR, 0);
>  
> @@ -669,10 +676,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,
> @@ -712,6 +723,8 @@ static void 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 */ }
>  };
>  
> @@ -729,7 +742,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;
>  		}
> @@ -873,6 +887,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 */ }
>  };
>