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},
};
@@ -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));
+ }
}
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.1On 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
On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > 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? Yeah, that is the expected way, to force the def_domain_type to IDENTITY and refuse to attach a PAGING/BLOCKED domain. If this is a common thing we could have the core code take on more of the job. Jason
On 2024-10-15 1:47 pm, Jason Gunthorpe wrote: > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > >> 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? > > Yeah, that is the expected way, to force the def_domain_type to > IDENTITY and refuse to attach a PAGING/BLOCKED domain. There is no domain, this is bypassing an arbitrary StreamID not associated with any device. Which incidentally is something an IORT RMR can quite happily achieve already (I think the DT reserved-memory binding does need a proper device node to relate to, though). Thanks, Robin.
On Tue, Oct 15, 2024 at 04:13:13PM +0100, Robin Murphy wrote: > On 2024-10-15 1:47 pm, Jason Gunthorpe wrote: > > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > > > 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? > > > > Yeah, that is the expected way, to force the def_domain_type to > > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > There is no domain, this is bypassing an arbitrary StreamID not associated > with any device. If the stream ID is going to flow traffic shouldn't it have a DT node for it? Something must be driving the DMA on this SID, and the kernel does need to know what that is in some way, even for basic security things like making sure VFIO doesn't get a hold of it :\ Jason
On 2024-10-15 4:31 pm, Jason Gunthorpe wrote: > On Tue, Oct 15, 2024 at 04:13:13PM +0100, Robin Murphy wrote: >> On 2024-10-15 1:47 pm, Jason Gunthorpe wrote: >>> On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: >>> >>>> 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? >>> >>> Yeah, that is the expected way, to force the def_domain_type to >>> IDENTITY and refuse to attach a PAGING/BLOCKED domain. >> >> There is no domain, this is bypassing an arbitrary StreamID not associated >> with any device. > > If the stream ID is going to flow traffic shouldn't it have a DT node > for it? Something must be driving the DMA on this SID, and the kernel > does need to know what that is in some way, even for basic security > things like making sure VFIO doesn't get a hold of it :\ Exactly, hence this RFC is definitely not the right approach. Thanks, Robin.
On Tue, Oct 15, 2024 at 04:37:25PM +0100, Robin Murphy wrote: > On 2024-10-15 4:31 pm, Jason Gunthorpe wrote: > > On Tue, Oct 15, 2024 at 04:13:13PM +0100, Robin Murphy wrote: > > > On 2024-10-15 1:47 pm, Jason Gunthorpe wrote: > > > > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > > > > > > > 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? > > > > > > > > Yeah, that is the expected way, to force the def_domain_type to > > > > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > > > > > There is no domain, this is bypassing an arbitrary StreamID not associated > > > with any device. > > > > If the stream ID is going to flow traffic shouldn't it have a DT node > > for it? Something must be driving the DMA on this SID, and the kernel > > does need to know what that is in some way, even for basic security > > things like making sure VFIO doesn't get a hold of it :\ > > Exactly, hence this RFC is definitely not the right approach. Agreed. I assumed the bypass was needed for a registered SID. > > Thanks, > Robin. Thanks, Pranjal
On Tue, Oct 15, 2024 at 04:13:13PM +0100, Robin Murphy wrote: > On 2024-10-15 1:47 pm, Jason Gunthorpe wrote: > > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > > > 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? > > > > Yeah, that is the expected way, to force the def_domain_type to > > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > There is no domain, this is bypassing an arbitrary StreamID not associated > with any device. Which incidentally is something an IORT RMR can quite > happily achieve already (I think the DT reserved-memory binding does need a > proper device node to relate to, though). +1. I assumed that the use-case was to first attach the streamID to a device and then intall a bypass for that specific streamID. If that's not the case, I'm not sure why are we trying to achieve that. I thought about rmr too, but it looks like the "device" is a DMA and may want to write to more than a fixed region of memory. > > Thanks, > Robin. Thanks, Pranjal
On Tue, Oct 15, 2024 at 09:47:23AM -0300, Jason Gunthorpe wrote: > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > 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? > > Yeah, that is the expected way, to force the def_domain_type to > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > If this is a common thing we could have the core code take on more of > the job. Yes! I've seen the IOMMU being bypassed at multiple places, primarily for performance, people like bypassing the iommu for "trusted" devices. A few examples that are publically accessible: Qcomm SoCs [1], [2]. Seems like Qualcomm have a DT property `qcomm-s1-bypass` to achieve something similar. In fact, *blast from the past*, I tried to do something similar sometime ago with [3]. Although, perhaps that wasn't the best way (and I was a kernel newbie :)) A little off-topic, but I think there has been some interest to bypass the default substream as well while still maintaining PASID isolation.[4] Although, as far as arm-smmu-v3 is concerned, IIRC, I think there was a way to tell that the region is reserved and don't map it. > > Jason Thanks, Pranjal [1] https://github.com/realme-kernel-opensource/realme5-kernel-source/blob/master/arch/arm64/boot/dts/qcom/sa8155-vm-qupv3.dtsi#L22 [2] https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/Documentation/devicetree/bindings/platform/msm/ipa.txt#28 [3] https://lore.kernel.org/all/20230707104857.348353-1-praan@google.com/ [4] https://lore.kernel.org/all/CAGfWUPziSWNMc_px4E-i+_V_Jxdb_WSwOLXHZ+PANz2Tv5pFPA@mail.gmail.com/
On Tue, Oct 15, 2024 at 03:00:10PM +0000, Pranjal Shrivastava wrote: > On Tue, Oct 15, 2024 at 09:47:23AM -0300, Jason Gunthorpe wrote: > > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > > > 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? > > > > Yeah, that is the expected way, to force the def_domain_type to > > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > > > If this is a common thing we could have the core code take on more of > > the job. > > Yes! I've seen the IOMMU being bypassed at multiple places, primarily > for performance, people like bypassing the iommu for "trusted" devices. > A few examples that are publically accessible: Qcomm SoCs [1], [2]. > Seems like Qualcomm have a DT property `qcomm-s1-bypass` to achieve > something similar. It is not good to encode policy in the kernel in this way. If the device works then it should be permitted to be non-identity, even if it is slow. I suppose things are done this way because the policy can't be changed once the drivers are bound, so this has to be decided early boot and so it is easiest path... But it does suggest to me that the DT encoding is more an indication "device is really slow with translation". Once I was looking at the idea of pulling all the identity stuff out of the drivers since alot of it is mostly device specific quirks/etc. It could then be marked as mandatory/performance and that could help understanding alot. Jason
On Tue, Oct 15, 2024 at 03:00:10PM +0000, Pranjal Shrivastava wrote: > On Tue, Oct 15, 2024 at 09:47:23AM -0300, Jason Gunthorpe wrote: > > On Tue, Oct 15, 2024 at 08:13:28AM +0000, Pranjal Shrivastava wrote: > > > > > 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? > > > > Yeah, that is the expected way, to force the def_domain_type to > > IDENTITY and refuse to attach a PAGING/BLOCKED domain. > > > > If this is a common thing we could have the core code take on more of > > the job. > > Yes! I've seen the IOMMU being bypassed at multiple places, primarily > for performance, people like bypassing the iommu for "trusted" devices. > A few examples that are publically accessible: Qcomm SoCs [1], [2]. > Seems like Qualcomm have a DT property `qcomm-s1-bypass` to achieve > something similar. > > In fact, *blast from the past*, I tried to do something similar sometime > ago with [3]. Although, perhaps that wasn't the best way (and I was a > kernel newbie :)) > > A little off-topic, but I think there has been some interest to bypass > the default substream as well while still maintaining PASID isolation.[4] > Agh, apologies, ignore the following part about rmr, it won't solve the problem here. > Although, as far as arm-smmu-v3 is concerned, IIRC, I think there was a > way to tell that the region is reserved and don't map it. > > > > > Jason > > Thanks, > Pranjal > > [1] > https://github.com/realme-kernel-opensource/realme5-kernel-source/blob/master/arch/arm64/boot/dts/qcom/sa8155-vm-qupv3.dtsi#L22 > > [2] > https://android.googlesource.com/kernel/msm/+/android-7.1.0_r0.2/Documentation/devicetree/bindings/platform/msm/ipa.txt#28 > > [3] > https://lore.kernel.org/all/20230707104857.348353-1-praan@google.com/ > > [4] > https://lore.kernel.org/all/CAGfWUPziSWNMc_px4E-i+_V_Jxdb_WSwOLXHZ+PANz2Tv5pFPA@mail.gmail.com/
© 2016 - 2024 Red Hat, Inc.