Re: [PATCH RFC 2/2] iommu/arm-smmu-v3: Bypass SID0 for NXP i.MX95

[Pranjal Shrivastava <praan@google.com>]

On Tue, Oct 15, 2024 at 11:14:43AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> i.MX95 eDMA3 connects to DSU ACP, supporting dma coherent memory to
> memory operations. However TBU is in the path between eDMA3 and ACP,
> need to bypass the default SID 0 to make eDMA3 work properly.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 19 ++++++++++++++++---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 +
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 737c5b88235510e3ddb91a28cecbdcdc14854b32..3db7b3e2ac94e16130fc0356f7954ffa1a9dfb33 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -80,6 +80,7 @@ DEFINE_MUTEX(arm_smmu_asid_lock);
>  static struct arm_smmu_option_prop arm_smmu_options[] = {
>  	{ ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>  	{ ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
> +	{ ARM_SMMU_OPT_IMX95_BYPASS_SID0, "nxp,imx95-bypass-sid-zero"},
>  	{ 0, NULL},
>  };

Aghh, let's not put HW-specific bypass under `arm_smmu_options`.
Otherwise, this might become a huge list of SoCs wanting to bypass SIDs.

>  
> @@ -4465,7 +4466,7 @@ static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>  	return devm_ioremap_resource(dev, &res);
>  }
>  
> -static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
> +static void arm_smmu_install_bypass_ste(struct arm_smmu_device *smmu)
>  {
>  	struct list_head rmr_list;
>  	struct iommu_resv_region *e;
> @@ -4496,6 +4497,18 @@ static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device *smmu)
>  	}
>  
>  	iort_put_rmr_sids(dev_fwnode(smmu->dev), &rmr_list);
> +
> +	if (smmu->options & ARM_SMMU_OPT_IMX95_BYPASS_SID0) {
> +		int ret = arm_smmu_init_sid_strtab(smmu, 0);
> +
> +		if (ret) {
> +			dev_err(smmu->dev, "i.MX95 SID0 bypass failed\n");
> +			return;
> +		}
> +
> +		arm_smmu_make_bypass_ste(smmu,
> +					 arm_smmu_get_step_for_sid(smmu, 0));
> +	}
>  }

Umm.. this was specific for rmr not a generic thing. I'd suggest to
avoid meddling with the STEs directly for acheiving bypass. Playing
with the iommu domain type could be neater. Perhaps, modify the
ops->def_domain_type to return an appropriate domain?

In general, adding a property/config for bypassing SIDs/devices could
potentially cause security concerns if *somehow* a device can spoof an
SID. For example, if a PCIe device is bypassed it would be easy for
another PCIe endpoint to spoof it's ID. Similarly, certain HW
implementations may provide the option to programmable SIDs, for
example, a HW register to set SIDs, which if compromised can spoof other
SIDs. Although, I'd like to see what the others think about this.

>  
>  static void arm_smmu_impl_remove(void *data)
> @@ -4614,8 +4627,8 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>  	/* Record our private device structure */
>  	platform_set_drvdata(pdev, smmu);
>  
> -	/* Check for RMRs and install bypass STEs if any */
> -	arm_smmu_rmr_install_bypass_ste(smmu);
> +	/* Install bypass STEs if any */
> +	arm_smmu_install_bypass_ste(smmu);
>  
>  	/* Reset the device */
>  	ret = arm_smmu_device_reset(smmu);
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 1e9952ca989f87957197f4d4b400f9d74bb685ac..06481b923284776e7dc4f3301e5cbe8ab7869a9c 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -733,6 +733,7 @@ struct arm_smmu_device {
>  #define ARM_SMMU_OPT_MSIPOLL		(1 << 2)
>  #define ARM_SMMU_OPT_CMDQ_FORCE_SYNC	(1 << 3)
>  #define ARM_SMMU_OPT_TEGRA241_CMDQV	(1 << 4)
> +#define ARM_SMMU_OPT_IMX95_BYPASS_SID0	(1 << 5)
>  	u32				options;
>  
>  	struct arm_smmu_cmdq		cmdq;
>
> -- 
> 2.37.1
> 
>

Thanks,
Pranjal