[VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[Jordan Crouse]

I've been struggling with a problem for a while and I haven't been able to come
up with a clean solution. Rob convinced me to stop complaining and do
_something_ and hopefully this can spur a good discussion.

The scenario is basically this: The MSM GPU wants to manage its own IOMMU
virtual address space in lieu of using the DMA API to do so. The most important
reason is that when per-instance pagetables [1] are enabled each file descriptor
uses its own pagetable which are dynamically switched between contexts. In
conjunction with this we use a split pagetable scheme to allow global buffers 
be persistently mapped so even a single pagetable has multiple ranges that need
to be utilized based on buffer type.

Obviously the DMA API isn't suitable for this kind of specialized use. We want
to push it off to the side, allocate our own domain, and use it directly with
the IOMMU APIs. In a perfect world we would just not use the DMA API and attach
our own domain and that would be the end of the story. Unfortunately this is not
a perfect world.

In order to switch the pagetable from the GPU the SoC has some special wiring so
that the GPU can reprogram the TTBR0 register in the IOMMU to the appropriate
value. For a variety of reasons the hardware is hardcoded to only be able to
access context bank 0 in the SMMU device. Long story short; if the DMA
domain is allocated during bus enumeration and attached to context bank 0 then
by the time we get to the driver domain we have long since lost our chance to
get the context bank we need.

After going back and forth and trying a few possible options it seems like the
"easiest" solution" is to skip attaching the DMA domain in the first place. But
we still need to create a default domain and set up the IOMMU groups correctly
so that when the GPU driver comes along it can attach the device and everything
will Just Work (TM). Rob suggested that we should use a blacklist of devices
that choose to not participate so thats what I'm offering as a starting point
for discussion.

The first patch in this series allows the specific IOMMU driver to gracefully
skip attaching a device by returning -ENOSUPP. In that case, the core will
skip printing error messages and it won't attach the domain to the group but
it still allows the group to get created so that the IOMMU device is properly
set up. In arch_setup_dma_ops() the call to iommu_get_domain_for_dev() will
return NULL and the IOMMU ops won't be set up.

The second patch implements the blacklist in arm-smmu.c based on the compatible
string of the GPU device and returns -ENOTSUPP.

I tested this with my current per-instance pagetable stack and it does the job
but I am very much in the market for better ideas.

[1] https://patchwork.freedesktop.org/series/38729/

Jordan Crouse (2):
  iommu: Gracefully allow drivers to not attach to a default domain
  iommu/arm-smmu: Add list of devices to opt out of DMA domains

 drivers/iommu/arm-smmu.c | 23 +++++++++++++++++++++++
 drivers/iommu/iommu.c    | 18 ++++++++++++++----
 2 files changed, 37 insertions(+), 4 deletions(-)

-- 
2.16.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH 1/2] iommu: Gracefully allow drivers to not attach to a default domain

[Jordan Crouse]

Provide individual device drivers the chance to gracefully refuse
to attach a device to the default domain. If the attach_device
op returns -ENOTSUPP don't print a error message and don't set
group->domain but still return success from iommu_group_add_dev().

This allows all the usual APIs to work and the next domain to try
to attach will take group->domain for itself and everything will
proceed as normal.

Signed-off-by: Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
---
 drivers/iommu/iommu.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 69fef991c651..2403cce138d2 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -592,7 +592,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	if (group->domain)
 		ret = __iommu_attach_device(group->domain, dev);
 	mutex_unlock(&group->mutex);
-	if (ret)
+	if (ret && ret != -ENOTSUPP)
 		goto err_put_group;
 
 	/* Notify any listeners about change to group. */
@@ -617,7 +617,9 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	sysfs_remove_link(&dev->kobj, "iommu_group");
 err_free_device:
 	kfree(device);
-	pr_err("Failed to add device %s to group %d: %d\n", dev_name(dev), group->id, ret);
+	if (ret != -ENOTSUPP)
+		pr_err("Failed to add device %s to group %d: %d\n",
+			dev_name(dev), group->id, ret);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_group_add_device);
@@ -1039,8 +1041,16 @@ struct iommu_group *iommu_group_get_for_dev(struct device *dev)
 
 	ret = iommu_group_add_device(group, dev);
 	if (ret) {
-		iommu_group_put(group);
-		return ERR_PTR(ret);
+		/*
+		 * If the driver chooses not to bind the device, reset
+		 * group->domain so a new domain can be added later
+		 */
+		if (ret == -ENOTSUPP)
+			group->domain = NULL;
+		else {
+			iommu_group_put(group);
+			return ERR_PTR(ret);
+		}
 	}
 
 	return group;
-- 
2.16.1

[PATCH 2/2] iommu/arm-smmu: Add list of devices to opt out of DMA domains

[Jordan Crouse]

Add a list of compatible strings for devices that wish to opt out
of attaching to a DMA domain.  This is for devices that prefer to
manage their own IOMMU space for any number of reasons. Returning
ENOTSUPP for attach device will filter down and force
arch_setup_dma_ops() to not set up the iommu DMA ops. Later
the client device in question can set up and attach their own
domain.

Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
---
 drivers/iommu/arm-smmu.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 69e7c60792a8..09fa03a15f4f 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1187,6 +1187,15 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 	return 0;
 }
 
+/*
+ * This is a list of compatible strings for devices that wish to manage their
+ * own IOMMU space instead of the DMA IOMMU ops. Devices on this list will not
+ * allow themselves to be attached to a IOMMU_DOMAIN_DMA domain
+ */
+static const char * const arm_smmu_dma_blacklist[] = {
+	"qcom,adreno",
+};
+
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
 	int ret;
@@ -1209,6 +1218,20 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	if (!fwspec->iommu_priv)
 		return -ENODEV;
 
+	/*
+	 * If this is the dfeault DMA domain, check to see if the device is on
+	 * the blacklist and reject if so
+	 */
+	if (domain->type == IOMMU_DOMAIN_DMA && dev->of_node) {
+		int i;
+
+		for (i = 0; i < ARRAY_SIZE(arm_smmu_dma_blacklist); i++) {
+			if (of_device_is_compatible(dev->of_node,
+				arm_smmu_dma_blacklist[i]))
+				return -ENOTSUPP;
+		}
+	}
+
 	smmu = fwspec_smmu(fwspec);
 	/* Ensure that the domain is finalised */
 	ret = arm_smmu_init_domain_context(domain, smmu);
-- 
2.16.1

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

Re: [VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[Rob Clark]

On Wed, Apr 11, 2018 at 2:55 PM, Jordan Crouse <jcrouse-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote:
> I've been struggling with a problem for a while and I haven't been able to come
> up with a clean solution. Rob convinced me to stop complaining and do
> _something_ and hopefully this can spur a good discussion.
>
> The scenario is basically this: The MSM GPU wants to manage its own IOMMU
> virtual address space in lieu of using the DMA API to do so. The most important
> reason is that when per-instance pagetables [1] are enabled each file descriptor
> uses its own pagetable which are dynamically switched between contexts. In
> conjunction with this we use a split pagetable scheme to allow global buffers
> be persistently mapped so even a single pagetable has multiple ranges that need
> to be utilized based on buffer type.
>
> Obviously the DMA API isn't suitable for this kind of specialized use. We want
> to push it off to the side, allocate our own domain, and use it directly with
> the IOMMU APIs. In a perfect world we would just not use the DMA API and attach
> our own domain and that would be the end of the story. Unfortunately this is not
> a perfect world.
>
> In order to switch the pagetable from the GPU the SoC has some special wiring so
> that the GPU can reprogram the TTBR0 register in the IOMMU to the appropriate
> value. For a variety of reasons the hardware is hardcoded to only be able to
> access context bank 0 in the SMMU device. Long story short; if the DMA
> domain is allocated during bus enumeration and attached to context bank 0 then
> by the time we get to the driver domain we have long since lost our chance to
> get the context bank we need.
>
> After going back and forth and trying a few possible options it seems like the
> "easiest" solution" is to skip attaching the DMA domain in the first place. But
> we still need to create a default domain and set up the IOMMU groups correctly
> so that when the GPU driver comes along it can attach the device and everything
> will Just Work (TM). Rob suggested that we should use a blacklist of devices
> that choose to not participate so thats what I'm offering as a starting point
> for discussion.

in an ideal world, the dma-mapping layer magic would not be forced on
a driver, but would be an opt-in thing (ie. driver's ->probe() calls
dma_setup_magic_iommu_stuff(dev)), and becomes more of a helper layer
that drivers could opt-in to.. but that changes how things work
significantly at iommu probe time and probably touches a lot of
drivers.  A property in dt would be the easy down-stream solution, but
this is mostly about the driver (but also about how the firmware
works, so maybe dt is not out of bounds?).  The black-list approach is
fairly simple, although maybe a bit of a point-solution.  (Although
the number of drivers this problem applies to might not be much
greater than 1.)

So short version, very interested if anyone has better suggestions.
Because we really need to do *something* here.

BR,
-R

> The first patch in this series allows the specific IOMMU driver to gracefully
> skip attaching a device by returning -ENOSUPP. In that case, the core will
> skip printing error messages and it won't attach the domain to the group but
> it still allows the group to get created so that the IOMMU device is properly
> set up. In arch_setup_dma_ops() the call to iommu_get_domain_for_dev() will
> return NULL and the IOMMU ops won't be set up.
>
> The second patch implements the blacklist in arm-smmu.c based on the compatible
> string of the GPU device and returns -ENOTSUPP.
>
> I tested this with my current per-instance pagetable stack and it does the job
> but I am very much in the market for better ideas.
>
> [1] https://patchwork.freedesktop.org/series/38729/
>
> Jordan Crouse (2):
>   iommu: Gracefully allow drivers to not attach to a default domain
>   iommu/arm-smmu: Add list of devices to opt out of DMA domains
>
>  drivers/iommu/arm-smmu.c | 23 +++++++++++++++++++++++
>  drivers/iommu/iommu.c    | 18 ++++++++++++++----
>  2 files changed, 37 insertions(+), 4 deletions(-)
>
> --
> 2.16.1
>

Re: [VERY RFC 0/2] iommu: optionally skip attaching to a DMA domain

[Robin Murphy]

On 11/04/18 19:55, Jordan Crouse wrote:
> I've been struggling with a problem for a while and I haven't been able to come
> up with a clean solution. Rob convinced me to stop complaining and do
> _something_ and hopefully this can spur a good discussion.
> 
> The scenario is basically this: The MSM GPU wants to manage its own IOMMU
> virtual address space in lieu of using the DMA API to do so. The most important
> reason is that when per-instance pagetables [1] are enabled each file descriptor
> uses its own pagetable which are dynamically switched between contexts. In
> conjunction with this we use a split pagetable scheme to allow global buffers
> be persistently mapped so even a single pagetable has multiple ranges that need
> to be utilized based on buffer type.
> 
> Obviously the DMA API isn't suitable for this kind of specialized use. We want
> to push it off to the side, allocate our own domain, and use it directly with
> the IOMMU APIs. In a perfect world we would just not use the DMA API and attach
> our own domain and that would be the end of the story. Unfortunately this is not
> a perfect world.
> 
> In order to switch the pagetable from the GPU the SoC has some special wiring so
> that the GPU can reprogram the TTBR0 register in the IOMMU to the appropriate
> value. For a variety of reasons the hardware is hardcoded to only be able to
> access context bank 0 in the SMMU device. Long story short; if the DMA
> domain is allocated during bus enumeration and attached to context bank 0 then
> by the time we get to the driver domain we have long since lost our chance to
> get the context bank we need.

Red herrings abound! That's not actually the DMA domain's fault at all, 
it just happens to fall out of the current SMMU driver behaviour. If the 
context bank allocator went top-down instead of bottom-up (IIRC I nearly 
ended up with that at one point during refactoring all that lot, or 
maybe that was the SMR allocator...) then you'd see a very different 
perspective of the problem.

The driver could certainly do with being cleverer in terms of not 
keeping hardware CBs allocated for inactive domains - right now I don't 
think you can even move a device from one domain to another at all if 
you only have a single context bank, which is pretty bogus. That alone 
might even manage to hide this problem, but I wouldn't like to rely on 
it. The "device X can only use context bank N" condition is a real 
hardware limitation which should be described by firmware, but I don't 
have any immediate good ideas as to how :(

The matter of opting out of DMA ops which expect the default domain is a 
separate and more general issue, so I won't start a fork of that 
discussion here.

Robin.

> After going back and forth and trying a few possible options it seems like the
> "easiest" solution" is to skip attaching the DMA domain in the first place. But
> we still need to create a default domain and set up the IOMMU groups correctly
> so that when the GPU driver comes along it can attach the device and everything
> will Just Work (TM). Rob suggested that we should use a blacklist of devices
> that choose to not participate so thats what I'm offering as a starting point
> for discussion.
> 
> The first patch in this series allows the specific IOMMU driver to gracefully
> skip attaching a device by returning -ENOSUPP. In that case, the core will
> skip printing error messages and it won't attach the domain to the group but
> it still allows the group to get created so that the IOMMU device is properly
> set up. In arch_setup_dma_ops() the call to iommu_get_domain_for_dev() will
> return NULL and the IOMMU ops won't be set up.
> 
> The second patch implements the blacklist in arm-smmu.c based on the compatible
> string of the GPU device and returns -ENOTSUPP.
> 
> I tested this with my current per-instance pagetable stack and it does the job
> but I am very much in the market for better ideas.
> 
> [1] https://patchwork.freedesktop.org/series/38729/
> 
> Jordan Crouse (2):
>    iommu: Gracefully allow drivers to not attach to a default domain
>    iommu/arm-smmu: Add list of devices to opt out of DMA domains
> 
>   drivers/iommu/arm-smmu.c | 23 +++++++++++++++++++++++
>   drivers/iommu/iommu.c    | 18 ++++++++++++++----
>   2 files changed, 37 insertions(+), 4 deletions(-)
> 
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno