iommu, dma-mapping: Simplify arch_setup_dma_ops()

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------
  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +8 -11

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void iommu_dma_init_options(struct iommu_dma_options *options,
  /**
   * iommu_dma_init_domain - Initialise a DMA mapping domain
   * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
   * @dev: Device the domain is being initialised for
   *
- * @base and @limit + 1 should be exact multiples of IOMMU page granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail.
   */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-				 dma_addr_t limit, struct device *dev)
+static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
+	const struct bus_dma_region *map = dev->dma_range_map;
  	unsigned long order, base_pfn;
  	struct iova_domain *iovad;
  	int ret;

@@ -684,18 +681,18 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 
  	/* Use the smallest supported page size for IOVA granularity */
  	order = __ffs(domain->pgsize_bitmap);
-	base_pfn = max_t(unsigned long, 1, base >> order);
+	base_pfn = 1;
 
  	/* Check the domain allows at least some access to the device... */
-	if (domain->geometry.force_aperture) {
+	if (map) {
+		dma_addr_t base = dma_range_map_min(map);
  		if (base > domain->geometry.aperture_end ||
-		    limit < domain->geometry.aperture_start) {
+		    dma_range_map_max(map) < domain->geometry.aperture_start) {
  			pr_warn("specified DMA range outside IOMMU capability\n");
  			return -EFAULT;
  		}
  		/* ...then finally give it a kicking to make sure it fits */
-		base_pfn = max_t(unsigned long, base_pfn,
-				domain->geometry.aperture_start >> order);
+		base_pfn = max(base, domain->geometry.aperture_start) >> order;
  	}
 
  	/* start_pfn is always nonzero for an already-initialised domain */

Hi Robin,

Quoted 71 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------

  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -3

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void iommu_dma_init_options(struct 
 iommu_dma_options *options,
   /**
    * iommu_dma_init_domain - Initialise a DMA mapping domain
    * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
    * @dev: Device the domain is being initialised for
    *
- * @base and @limit + 1 should be exact multiples of IOMMU page 

granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail. */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-                 dma_addr_t limit, struct device *dev)
+static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
      struct iommu_dma_cookie *cookie = domain->iova_cookie;
+    const struct bus_dma_region *map = dev->dma_range_map; unsigned long order, base_pfn;
      struct iova_domain *iovad;
      int ret;

drivers/iommu/dma-iommu.c +3 -2

@@ -684,18 +681,18 @@ static int iommu_dma_init_domain(struct 
 iommu_domain *domain, dma_addr_t base,
       /* Use the smallest supported page size for IOVA granularity */
       order = __ffs(domain->pgsize_bitmap);
-    base_pfn = max_t(unsigned long, 1, base >> order);
+    base_pfn = 1;
       /* Check the domain allows at least some access to the device... */
-    if (domain->geometry.force_aperture) {
+    if (map) {
+        dma_addr_t base = dma_range_map_min(map);

          if (base > domain->geometry.aperture_end ||
-            limit < domain->geometry.aperture_start) {
+            dma_range_map_max(map) < domain->geometry.aperture_start) { pr_warn("specified DMA range outside IOMMU capability\n"); return -EFAULT;
          }
          /* ...then finally give it a kicking to make sure it fits */ -        base_pfn = max_t(unsigned long, base_pfn,
-                domain->geometry.aperture_start >> order);
+        base_pfn = max(base, domain->geometry.aperture_start) >> order; }
      /* start_pfn is always nonzero for an already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device *dev, u64 
 dma_base, u64 dma_limit)
        * underlying IOMMU driver needs to support via the dma-iommu layer.
        */
       if (iommu_is_dma_domain(domain)) {

Quoted 5 lines

drivers/iommu/dma-iommu.c +1 -1

-        if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
+        if (iommu_dma_init_domain(domain, dev))
               goto out_err;
           dev->dma_ops = &iommu_dma_ops;

      }

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

If you have any ideas on things we can try let me know.

Hi Jon,

Quoted 74 lines · Jon Hunter

Hi Robin,

Quoted 71 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------

  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -3

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void iommu_dma_init_options(struct 
 iommu_dma_options *options,
   /**
    * iommu_dma_init_domain - Initialise a DMA mapping domain
    * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
    * @dev: Device the domain is being initialised for
    *
- * @base and @limit + 1 should be exact multiples of IOMMU page 

granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail. */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-                 dma_addr_t limit, struct device *dev)
+static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
      struct iommu_dma_cookie *cookie = domain->iova_cookie;
+    const struct bus_dma_region *map = dev->dma_range_map; unsigned long order, base_pfn;
      struct iova_domain *iovad;
      int ret;

drivers/iommu/dma-iommu.c +3 -2

@@ -684,18 +681,18 @@ static int iommu_dma_init_domain(struct 
 iommu_domain *domain, dma_addr_t base,
       /* Use the smallest supported page size for IOVA granularity */
       order = __ffs(domain->pgsize_bitmap);
-    base_pfn = max_t(unsigned long, 1, base >> order);
+    base_pfn = 1;
       /* Check the domain allows at least some access to the device... */
-    if (domain->geometry.force_aperture) {
+    if (map) {
+        dma_addr_t base = dma_range_map_min(map);

          if (base > domain->geometry.aperture_end ||
-            limit < domain->geometry.aperture_start) {
+            dma_range_map_max(map) < domain->geometry.aperture_start) { pr_warn("specified DMA range outside IOMMU capability\n"); return -EFAULT;
          }
          /* ...then finally give it a kicking to make sure it fits */ -        base_pfn = max_t(unsigned long, base_pfn,
-                domain->geometry.aperture_start >> order);
+        base_pfn = max(base, domain->geometry.aperture_start) >> order; }
      /* start_pfn is always nonzero for an already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device *dev, 
 u64 dma_base, u64 dma_limit)
        * underlying IOMMU driver needs to support via the dma-iommu layer.
        */
       if (iommu_is_dma_domain(domain)) {

Quoted 8 lines

Quoted 5 lines

drivers/iommu/dma-iommu.c +1 -1

-        if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
+        if (iommu_dma_init_domain(domain, dev))
               goto out_err;
           dev->dma_ops = &iommu_dma_ops;

      }

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

That is... thoroughly bemusing :/ Not only is there supposed to be no real functional change here - we should merely be recalculating the same information from dev->dma_range_map that the callers were already doing to generate the base/limit arguments - but the act of initially setting up a default domain for a device behind an IOMMU should have no connection whatsoever to suspend and especially not to CPU hotplug.

Quoted 1 line

If you have any ideas on things we can try let me know.

Since the symptom seems inexplicable, I'd throw the usual memory debugging stuff like KASAN at it first. I'd also try "no_console_suspend" to check whether any late output is being missed in the suspend case (and if it's already broken, then any additional issues that may be caused by the console itself hopefully shouldn't matter).

For more base-covering, do you have the "arm64: Properly clean up iommu-dma remnants" fix in there already as well? That bug has bisected to patch #6 each time though, so I do still suspect that what you're seeing is likely something else. It does seem potentially significant that those Tegra platforms are making fairly wide use of dma-ranges, but there's no clear idea forming out of that observation just yet...

Quoted 88 lines · Robin Murphy

Hi Jon,

Quoted 76 lines · Jon Hunter

Hi Robin,

Quoted 73 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------

  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -3

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void iommu_dma_init_options(struct 
 iommu_dma_options *options,
   /**
    * iommu_dma_init_domain - Initialise a DMA mapping domain
    * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
    * @dev: Device the domain is being initialised for
    *
- * @base and @limit + 1 should be exact multiples of IOMMU page 

granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail. */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-                 dma_addr_t limit, struct device *dev)
+static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
      struct iommu_dma_cookie *cookie = domain->iova_cookie;
+    const struct bus_dma_region *map = dev->dma_range_map; unsigned long order, base_pfn;
      struct iova_domain *iovad;
      int ret;

drivers/iommu/dma-iommu.c +3 -2

@@ -684,18 +681,18 @@ static int iommu_dma_init_domain(struct 
 iommu_domain *domain, dma_addr_t base,
       /* Use the smallest supported page size for IOVA granularity */
       order = __ffs(domain->pgsize_bitmap);
-    base_pfn = max_t(unsigned long, 1, base >> order);
+    base_pfn = 1;
       /* Check the domain allows at least some access to the device... */
-    if (domain->geometry.force_aperture) {
+    if (map) {
+        dma_addr_t base = dma_range_map_min(map);

          if (base > domain->geometry.aperture_end ||
-            limit < domain->geometry.aperture_start) {
+            dma_range_map_max(map) < domain->geometry.aperture_start) {
              pr_warn("specified DMA range outside IOMMU capability\n"); return -EFAULT;
          }
          /* ...then finally give it a kicking to make sure it fits */ -        base_pfn = max_t(unsigned long, base_pfn,
-                domain->geometry.aperture_start >> order);
+        base_pfn = max(base, domain->geometry.aperture_start) >> order;
      }
      /* start_pfn is always nonzero for an already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device *dev, 
 u64 dma_base, u64 dma_limit)
        * underlying IOMMU driver needs to support via the dma-iommu layer.
        */
       if (iommu_is_dma_domain(domain)) {

Quoted 8 lines

Quoted 5 lines

drivers/iommu/dma-iommu.c +1 -1

-        if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
+        if (iommu_dma_init_domain(domain, dev))
               goto out_err;
           dev->dma_ops = &iommu_dma_ops;

      }

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

Quoted 1 line

That is... thoroughly bemusing :/ Not only is there supposed to be no real functional change here - we should merely be recalculating the same information from dev->dma_range_map that the callers were already doing to generate the base/limit arguments - but the act of initially setting up a default domain for a device behind an IOMMU should have no connection whatsoever to suspend and especially not to CPU hotplug.

Yes it does look odd, but this is what bisect reported ...

git bisect start
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9
git bisect good a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
# bad: [6ba6c795dc73c22ce2c86006f17c4aa802db2a60] Add linux-next specific files for 20240513
git bisect bad 6ba6c795dc73c22ce2c86006f17c4aa802db2a60
# good: [29e7f949865a023a21ecdfbd82d68ac697569f34] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 29e7f949865a023a21ecdfbd82d68ac697569f34
# skip: [150e6cc14e51f2a07034106a4529cdaafd812c46] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
git bisect skip 150e6cc14e51f2a07034106a4529cdaafd812c46
# good: [f5d75327d30af49acf2e4b55f35ce2e6c45d1287] drm/amd/display: Fix invalid Copyright notice
git bisect good f5d75327d30af49acf2e4b55f35ce2e6c45d1287
# skip: [f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git
git bisect skip f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8
# bad: [f091e93306e0429ebb7589b9874590b6a9705e64] dma-mapping: Simplify arch_setup_dma_ops()
git bisect bad f091e93306e0429ebb7589b9874590b6a9705e64
# good: [91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b] ACPI/IORT: Handle memory address size limits as limits
git bisect good 91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b
# bad: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e] iommu/dma: Make limit checks self-contained
git bisect bad ad4750b07d3462ce29a0c9b1e88b2a1f9795290e
# good: [fece6530bf4b59b01a476a12851e07751e73d69f] dma-mapping: Add helpers for dma_range_map bounds
git bisect good fece6530bf4b59b01a476a12851e07751e73d69f

# first bad commit: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e] iommu/dma: Make limit checks self-contained

There is a couple skips in there and so I will try this again.

Quoted 4 lines

Quoted 1 line

If you have any ideas on things we can try let me know.

Since the symptom seems inexplicable, I'd throw the usual memory debugging stuff like KASAN at it first. I'd also try "no_console_suspend" to check whether any late output is being missed in the suspend case (and if it's already broken, then any additional issues that may be caused by the console itself hopefully shouldn't matter).

Quoted 1 line

For more base-covering, do you have the "arm64: Properly clean up iommu-dma remnants" fix in there already as well? That bug has bisected to patch #6 each time though, so I do still suspect that what you're seeing is likely something else. It does seem potentially significant that those Tegra platforms are making fairly wide use of dma-ranges, but there's no clear idea forming out of that observation just yet...

I was hoping it was the same issue other people had reported, but the fix provided did not help. I have also tried today's -next and I am still seeing the issue.

I should have more time next week to look at this further. Let me confirm which change is causing this and add more debug.

Quoted 166 lines · Jon Hunter

Quoted 100 lines · Robin Murphy

Hi Jon,

Quoted 90 lines · Jon Hunter

Hi Robin,

Quoted 77 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------

  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -3

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void
 iommu_dma_init_options(struct iommu_dma_options *options,
   /**
    * iommu_dma_init_domain - Initialise a DMA mapping domain
    * @domain: IOMMU domain previously prepared by
 iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
    * @dev: Device the domain is being initialised for
    *
- * @base and @limit + 1 should be exact multiples of IOMMU

page granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail. */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-                 dma_addr_t limit, struct device *dev) +static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
      struct iommu_dma_cookie *cookie = domain->iova_cookie; +    const struct bus_dma_region *map = dev->dma_range_map; unsigned long order, base_pfn;
      struct iova_domain *iovad;
      int ret;

drivers/iommu/dma-iommu.c +1 -1

@@ -684,18 +681,18 @@ static int
 iommu_dma_init_domain(struct iommu_domain *domain,
 dma_addr_t base,
       /* Use the smallest supported page size for IOVA granularity */
       order = __ffs(domain->pgsize_bitmap);
-    base_pfn = max_t(unsigned long, 1, base >> order);
+    base_pfn = 1;
       /* Check the domain allows at least some access to the

device... */
-    if (domain->geometry.force_aperture) {
+    if (map) {
+        dma_addr_t base = dma_range_map_min(map);
          if (base > domain->geometry.aperture_end ||
-            limit < domain->geometry.aperture_start) {
+            dma_range_map_max(map) < domain->geometry.aperture_start) {
              pr_warn("specified DMA range outside IOMMU capability\n");
              return -EFAULT;
          }
          /* ...then finally give it a kicking to make sure it fits */ -        base_pfn = max_t(unsigned long, base_pfn,
-                domain->geometry.aperture_start >> order); +        base_pfn = max(base, domain->geometry.aperture_start) >> order;
      }
      /* start_pfn is always nonzero for an already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device
 *dev, u64 dma_base, u64 dma_limit)
        * underlying IOMMU driver needs to support via the

Quoted 8 lines

dma-iommu layer.
       */
      if (iommu_is_dma_domain(domain)) {
-        if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev)) +        if (iommu_dma_init_domain(domain, dev))
              goto out_err;
          dev->dma_ops = &iommu_dma_ops;
      }

Quoted 6 lines

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

Quoted 7 lines

That is... thoroughly bemusing :/ Not only is there supposed to be no real functional change here - we should merely be recalculating the same information from dev->dma_range_map that the callers were already doing to generate the base/limit arguments - but the act of initially setting up a default domain for a device behind an IOMMU should have no connection whatsoever to suspend and especially not to CPU hotplug.

Yes it does look odd, but this is what bisect reported ...

git bisect start
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9 git bisect good a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
# bad: [6ba6c795dc73c22ce2c86006f17c4aa802db2a60] Add linux-next specific files for 20240513
git bisect bad 6ba6c795dc73c22ce2c86006f17c4aa802db2a60
# good: [29e7f949865a023a21ecdfbd82d68ac697569f34] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 29e7f949865a023a21ecdfbd82d68ac697569f34
# skip: [150e6cc14e51f2a07034106a4529cdaafd812c46] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
git bisect skip 150e6cc14e51f2a07034106a4529cdaafd812c46
# good: [f5d75327d30af49acf2e4b55f35ce2e6c45d1287] drm/amd/display: Fix invalid Copyright notice
git bisect good f5d75327d30af49acf2e4b55f35ce2e6c45d1287
# skip: [f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8] Merge branch 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git
git bisect skip f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8
# bad: [f091e93306e0429ebb7589b9874590b6a9705e64] dma-mapping: Simplify arch_setup_dma_ops()
git bisect bad f091e93306e0429ebb7589b9874590b6a9705e64
# good: [91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b] ACPI/IORT: Handle memory address size limits as limits
git bisect good 91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b
# bad: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e] iommu/dma: Make limit checks self-contained
git bisect bad ad4750b07d3462ce29a0c9b1e88b2a1f9795290e
# good: [fece6530bf4b59b01a476a12851e07751e73d69f] dma-mapping: Add helpers for dma_range_map bounds
git bisect good fece6530bf4b59b01a476a12851e07751e73d69f

# first bad commit: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e]
iommu/dma: Make limit checks self-contained

There is a couple skips in there and so I will try this again.

Quoted 9 lines

Quoted 1 line

If you have any ideas on things we can try let me know.

Since the symptom seems inexplicable, I'd throw the usual memory debugging stuff like KASAN at it first. I'd also try "no_console_suspend" to check whether any late output is being missed in the suspend case (and if it's already broken, then any additional issues that may be caused by the console itself hopefully shouldn't matter).

Quoted 7 lines

For more base-covering, do you have the "arm64: Properly clean up iommu-dma remnants" fix in there already as well? That bug has bisected to patch #6 each time though, so I do still suspect that what you're seeing is likely something else. It does seem potentially significant that those Tegra platforms are making fairly wide use of dma-ranges, but there's no clear idea forming out of that observation just yet...

Quoted 6 lines

I was hoping it was the same issue other people had reported, but the fix provided did not help. I have also tried today's -next and I am still seeing the issue.

I should have more time next week to look at this further. Let me confirm which change is causing this and add more debug.

Thanks. From staring at the code I think I've spotted one subtlety which may not be quite as intended - can you see if the diff below helps? It occurs to me that suspend and CPU hotplug may not *cause* the symptom, but they could certainly stall if one or more relevant CPUs is *already* stuck in a loop somewhere...

Thanks,
Robin.

----->8-----

drivers/iommu/dma-iommu.c +1 -0

index 89a53c2f2cf9..85eb1846c637 100644
@@ -686,6 +686,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev
 	/* Check the domain allows at least some access to the device... */
 	if (map) {
 		dma_addr_t base = dma_range_map_min(map);
+		base = max(base, (dma_addr_t)1 << order);
 		if (base > domain->geometry.aperture_end ||
 		    dma_range_map_max(map) < domain->geometry.aperture_start) {
 			pr_warn("specified DMA range outside IOMMU capability\n");

Quoted 166 lines · Jon Hunter

Quoted 100 lines · Robin Murphy

Hi Jon,

Quoted 90 lines · Jon Hunter

Hi Robin,

Quoted 77 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------

  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -3

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void
 iommu_dma_init_options(struct iommu_dma_options *options,
   /**
    * iommu_dma_init_domain - Initialise a DMA mapping domain
    * @domain: IOMMU domain previously prepared by
 iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
    * @dev: Device the domain is being initialised for
    *
- * @base and @limit + 1 should be exact multiples of IOMMU

page granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail. */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-                 dma_addr_t limit, struct device *dev) +static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
      struct iommu_dma_cookie *cookie = domain->iova_cookie; +    const struct bus_dma_region *map = dev->dma_range_map; unsigned long order, base_pfn;
      struct iova_domain *iovad;
      int ret;

drivers/iommu/dma-iommu.c +1 -1

@@ -684,18 +681,18 @@ static int
 iommu_dma_init_domain(struct iommu_domain *domain,
 dma_addr_t base,
       /* Use the smallest supported page size for IOVA granularity */
       order = __ffs(domain->pgsize_bitmap);
-    base_pfn = max_t(unsigned long, 1, base >> order);
+    base_pfn = 1;
       /* Check the domain allows at least some access to the

device... */
-    if (domain->geometry.force_aperture) {
+    if (map) {
+        dma_addr_t base = dma_range_map_min(map);
          if (base > domain->geometry.aperture_end ||
-            limit < domain->geometry.aperture_start) {
+            dma_range_map_max(map) < domain->geometry.aperture_start) {
              pr_warn("specified DMA range outside IOMMU capability\n");
              return -EFAULT;
          }
          /* ...then finally give it a kicking to make sure it fits */ -        base_pfn = max_t(unsigned long, base_pfn,
-                domain->geometry.aperture_start >> order); +        base_pfn = max(base, domain->geometry.aperture_start) >> order;
      }
      /* start_pfn is always nonzero for an already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device
 *dev, u64 dma_base, u64 dma_limit)
        * underlying IOMMU driver needs to support via the

Quoted 8 lines

dma-iommu layer.
       */
      if (iommu_is_dma_domain(domain)) {
-        if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev)) +        if (iommu_dma_init_domain(domain, dev))
              goto out_err;
          dev->dma_ops = &iommu_dma_ops;
      }

Quoted 6 lines

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

Quoted 7 lines

That is... thoroughly bemusing :/ Not only is there supposed to be no real functional change here - we should merely be recalculating the same information from dev->dma_range_map that the callers were already doing to generate the base/limit arguments - but the act of initially setting up a default domain for a device behind an IOMMU should have no connection whatsoever to suspend and especially not to CPU hotplug.

Yes it does look odd, but this is what bisect reported ...

git bisect start
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9 git bisect good a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
# bad: [6ba6c795dc73c22ce2c86006f17c4aa802db2a60] Add linux-next specific files for 20240513
git bisect bad 6ba6c795dc73c22ce2c86006f17c4aa802db2a60
# good: [29e7f949865a023a21ecdfbd82d68ac697569f34] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 29e7f949865a023a21ecdfbd82d68ac697569f34
# skip: [150e6cc14e51f2a07034106a4529cdaafd812c46] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
git bisect skip 150e6cc14e51f2a07034106a4529cdaafd812c46
# good: [f5d75327d30af49acf2e4b55f35ce2e6c45d1287] drm/amd/display: Fix invalid Copyright notice
git bisect good f5d75327d30af49acf2e4b55f35ce2e6c45d1287
# skip: [f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8] Merge branch 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git
git bisect skip f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8
# bad: [f091e93306e0429ebb7589b9874590b6a9705e64] dma-mapping: Simplify arch_setup_dma_ops()
git bisect bad f091e93306e0429ebb7589b9874590b6a9705e64
# good: [91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b] ACPI/IORT: Handle memory address size limits as limits
git bisect good 91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b
# bad: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e] iommu/dma: Make limit checks self-contained
git bisect bad ad4750b07d3462ce29a0c9b1e88b2a1f9795290e
# good: [fece6530bf4b59b01a476a12851e07751e73d69f] dma-mapping: Add helpers for dma_range_map bounds
git bisect good fece6530bf4b59b01a476a12851e07751e73d69f

# first bad commit: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e]
iommu/dma: Make limit checks self-contained

There is a couple skips in there and so I will try this again.

Quoted 9 lines

Quoted 1 line

If you have any ideas on things we can try let me know.

Since the symptom seems inexplicable, I'd throw the usual memory debugging stuff like KASAN at it first. I'd also try "no_console_suspend" to check whether any late output is being missed in the suspend case (and if it's already broken, then any additional issues that may be caused by the console itself hopefully shouldn't matter).

Quoted 7 lines

For more base-covering, do you have the "arm64: Properly clean up iommu-dma remnants" fix in there already as well? That bug has bisected to patch #6 each time though, so I do still suspect that what you're seeing is likely something else. It does seem potentially significant that those Tegra platforms are making fairly wide use of dma-ranges, but there's no clear idea forming out of that observation just yet...

Quoted 6 lines

I was hoping it was the same issue other people had reported, but the fix provided did not help. I have also tried today's -next and I am still seeing the issue.

I should have more time next week to look at this further. Let me confirm which change is causing this and add more debug.

Thanks. From staring at the code I think I've spotted one subtlety which may not be quite as intended - can you see if the diff below helps? It occurs to me that suspend and CPU hotplug may not *cause* the symptom, but they could certainly stall if one or more relevant CPUs is *already* stuck in a loop somewhere...

Thanks,
Robin.

----->8-----

drivers/iommu/dma-iommu.c +0 -0

index 89a53c2f2cf9..85eb1846c637 100644

Quoted 183 lines · Robin Murphy

Quoted 166 lines · Jon Hunter

Quoted 100 lines · Robin Murphy

Hi Jon,

Quoted 90 lines · Jon Hunter

Hi Robin,

Quoted 77 lines · Robin Murphy

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
� drivers/iommu/dma-iommu.c | 21 +++++++++------------ � 1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +0 -0

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void
 iommu_dma_init_options(struct iommu_dma_options *options,

� /**
�� * iommu_dma_init_domain - Initialise a DMA mapping domain
�� * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
�� * @dev: Device the domain is being initialised for
�� *
- * @base and @limit + 1 should be exact multiples of IOMMU
page granularity to
- * avoid rounding surprises. If necessary, we reserve the
page at address 0
+ * If the geometry and dma_range_map include address 0, we
reserve that page
�� * to ensure it is an invalid IOVA. It is safe to
reinitialise a domain, but
�� * any change which could make prior IOVAs invalid will fail. �� */
-static int iommu_dma_init_domain(struct iommu_domain
*domain, dma_addr_t base, -���������������� dma_addr_t limit, struct device *dev) +static int iommu_dma_init_domain(struct iommu_domain
*domain, struct device *dev)
� {
����� struct iommu_dma_cookie *cookie = domain->iova_cookie; +��� const struct bus_dma_region *map = dev->dma_range_map; ����� unsigned long order, base_pfn;
����� struct iova_domain *iovad;
����� int ret;

drivers/iommu/dma-iommu.c +0 -0

@@ -684,18 +681,18 @@ static int
 iommu_dma_init_domain(struct iommu_domain *domain,
 dma_addr_t base,

����� /* Use the smallest supported page size for IOVA granularity */ ����� order = __ffs(domain->pgsize_bitmap);
-��� base_pfn = max_t(unsigned long, 1, base >> order);
+��� base_pfn = 1;
����� /* Check the domain allows at least some access to the device... */
-��� if (domain->geometry.force_aperture) {
+��� if (map) {
+������� dma_addr_t base = dma_range_map_min(map); ��������� if (base > domain->geometry.aperture_end || -����������� limit < domain->geometry.aperture_start) { +����������� dma_range_map_max(map) < domain->geometry.aperture_start) {
������������� pr_warn("specified DMA range outside IOMMU capability\n");
������������� return -EFAULT; ��������� }
��������� /* ...then finally give it a kicking to make sure it fits */ -������� base_pfn = max_t(unsigned long, base_pfn, -��������������� domain->geometry.aperture_start >> order); +������� base_pfn = max(base, domain->geometry.aperture_start) >> order;
����� }
����� /* start_pfn is always nonzero for an
already-initialised domain */

drivers/iommu/dma-iommu.c +0 -0

@@ -1760,7 +1757,7 @@ void iommu_setup_dma_ops(struct device
 *dev, u64 dma_base, u64 dma_limit)

������ * underlying IOMMU driver needs to support via the

Quoted 8 lines

dma-iommu layer.
������ */
����� if (iommu_is_dma_domain(domain)) { -������� if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev)) +������� if (iommu_dma_init_domain(domain, dev)) ������������� goto out_err; ��������� dev->dma_ops = &iommu_dma_ops; ����� }

Quoted 6 lines

I have noticed some random test failures on Tegra186 and Tegra194 and bisect is pointing to this commit. Reverting this along with the various dependencies does fix the problem. On Tegra186 CPU hotplug is failing and on Tegra194 suspend is failing. Unfortunately, on neither platform do I see any particular crash but the boards hang somewhere.

Quoted 7 lines

That is... thoroughly bemusing :/ Not only is there supposed to be no real functional change here - we should merely be recalculating the same information from dev->dma_range_map that the callers were already doing to generate the base/limit arguments - but the act of initially setting up a default domain for a device behind an IOMMU should have no connection whatsoever to suspend and especially not to CPU hotplug.

Yes it does look odd, but this is what bisect reported ...

git bisect start
# good: [a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6] Linux 6.9 git bisect good a38297e3fb012ddfa7ce0321a7e5a8daeb1872b6
# bad: [6ba6c795dc73c22ce2c86006f17c4aa802db2a60] Add linux-next specific files for 20240513
git bisect bad 6ba6c795dc73c22ce2c86006f17c4aa802db2a60
# good: [29e7f949865a023a21ecdfbd82d68ac697569f34] Merge branch 'main' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
git bisect good 29e7f949865a023a21ecdfbd82d68ac697569f34
# skip: [150e6cc14e51f2a07034106a4529cdaafd812c46] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git
git bisect skip 150e6cc14e51f2a07034106a4529cdaafd812c46
# good: [f5d75327d30af49acf2e4b55f35ce2e6c45d1287] drm/amd/display: Fix invalid Copyright notice
git bisect good f5d75327d30af49acf2e4b55f35ce2e6c45d1287
# skip: [f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8] Merge branch 'for-next' of
git://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-dt.git
git bisect skip f1ec9a9ffc526df7c9523006c2abbb8ea554cdd8
# bad: [f091e93306e0429ebb7589b9874590b6a9705e64] dma-mapping: Simplify arch_setup_dma_ops()
git bisect bad f091e93306e0429ebb7589b9874590b6a9705e64
# good: [91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b] ACPI/IORT: Handle memory address size limits as limits
git bisect good 91cfd679f9e8b9a7bf2f26adf66eff99dbe2026b
# bad: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e] iommu/dma: Make limit checks self-contained
git bisect bad ad4750b07d3462ce29a0c9b1e88b2a1f9795290e
# good: [fece6530bf4b59b01a476a12851e07751e73d69f] dma-mapping: Add helpers for dma_range_map bounds
git bisect good fece6530bf4b59b01a476a12851e07751e73d69f

# first bad commit: [ad4750b07d3462ce29a0c9b1e88b2a1f9795290e]
iommu/dma: Make limit checks self-contained

There is a couple skips in there and so I will try this again.

Quoted 9 lines

Quoted 1 line

If you have any ideas on things we can try let me know.

Since the symptom seems inexplicable, I'd throw the usual memory debugging stuff like KASAN at it first. I'd also try "no_console_suspend" to check whether any late output is being missed in the suspend case (and if it's already broken, then any additional issues that may be caused by the console itself hopefully shouldn't matter).

Quoted 7 lines

For more base-covering, do you have the "arm64: Properly clean up iommu-dma remnants" fix in there already as well? That bug has bisected to patch #6 each time though, so I do still suspect that what you're seeing is likely something else. It does seem potentially significant that those Tegra platforms are making fairly wide use of dma-ranges, but there's no clear idea forming out of that observation just yet...

Quoted 6 lines

I was hoping it was the same issue other people had reported, but the fix provided did not help. I have also tried today's -next and I am still seeing the issue.

I should have more time next week to look at this further. Let me confirm which change is causing this and add more debug.

Thanks. From staring at the code I think I've spotted one subtlety which may not be quite as intended - can you see if the diff below helps? It occurs to me that suspend and CPU hotplug may not *cause* the symptom, but they could certainly stall if one or more relevant CPUs is *already* stuck in a loop somewhere...

Thanks,
Robin.

drivers/iommu/dma-iommu.c static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev · line 692

Quoted 13 lines

----->8-----

drivers/iommu/dma-iommu.c +1 -0

index 89a53c2f2cf9..85eb1846c637 100644
@@ -686,6 +686,7 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev
  	/* Check the domain allows at least some access to the device... */
  	if (map) {
  		dma_addr_t base = dma_range_map_min(map);
+		base = max(base, (dma_addr_t)1 << order);
  		if (base > domain->geometry.aperture_end ||
  		    dma_range_map_max(map) < domain->geometry.aperture_start) {
  			pr_warn("specified DMA range outside IOMMU capability\n");

With this in place I no longer see the mapping fail on the nvidia system.

Thanks. From staring at the code I think I've spotted one subtlety which may not be quite as intended - can you see if the diff below helps? It occurs to me that suspend and CPU hotplug may not *cause* the symptom, but they could certainly stall if one or more relevant CPUs is *already* stuck in a loop somewhere...

Thanks,
Robin.

----->8-----

drivers/iommu/dma-iommu.c +1 -0

index 89a53c2f2cf9..85eb1846c637 100644
@@ -686,6 +686,7 @@ static int iommu_dma_init_domain(struct iommu_domain 
 *domain, struct device *dev
      /* Check the domain allows at least some access to the device... */
      if (map) {
          dma_addr_t base = dma_range_map_min(map);
+        base = max(base, (dma_addr_t)1 << order);
          if (base > domain->geometry.aperture_end ||
              dma_range_map_max(map) < domain->geometry.aperture_start) {

             pr_warn("specified DMA range outside IOMMU capability\n");

Hi Robin,

an observation from 6.10-rc1: On sc8280xp (Lenovo X13s, Windows Dev Kit 2023), when booted to EL2 with the arm-smmuv3 under control of Linux, it fails to set up DMA transfers to nvme. My box boots from nvme, so I only got a black screen. @craftyguy booted from USB, and got this:

Hi Jens,

Quoted 6 lines · Jens Glathe

Hi Robin,

an observation from 6.10-rc1: On sc8280xp (Lenovo X13s, Windows Dev Kit 2023), when booted to EL2 with the arm-smmuv3 under control of Linux, it fails to set up DMA transfers to nvme. My box boots from nvme, so I only got a black screen. @craftyguy booted from USB, and got this:

Indeed, I see there's a dma-ranges property with a base of 0 in that DT, so all manner of hilarity may ensue. The fix is here, just waiting to be picked up:

https://lore.kernel.org/linux-iommu/159193e80b6a7701c61b32d6119ac68989d457bd.1716997607.git.robin.murphy@arm.com/

Thanks,
Robin.

It's now easy to retrieve the device's DMA limits if we want to check them against the domain aperture, so do that ourselves instead of relying on them being passed through the callchain.

Reviewed-by Jason Gunthorpe<[email protected]>

Tested-by Hanjun Guo<[email protected]>

Signed-off-by Robin Murphy<[email protected]>

---
  drivers/iommu/dma-iommu.c | 21 +++++++++------------
  1 file changed, 9 insertions(+), 12 deletions(-)

drivers/iommu/dma-iommu.c +8 -11

index a3039005b696..f542eabaefa4 100644
@@ -660,19 +660,16 @@ static void iommu_dma_init_options(struct iommu_dma_options *options,
  /**
   * iommu_dma_init_domain - Initialise a DMA mapping domain
   * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
- * @base: IOVA at which the mappable address space starts
- * @limit: Last address of the IOVA space
   * @dev: Device the domain is being initialised for
   *
- * @base and @limit + 1 should be exact multiples of IOMMU page granularity to
- * avoid rounding surprises. If necessary, we reserve the page at address 0
+ * If the geometry and dma_range_map include address 0, we reserve that page
   * to ensure it is an invalid IOVA. It is safe to reinitialise a domain, but
   * any change which could make prior IOVAs invalid will fail.
   */
-static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
-				 dma_addr_t limit, struct device *dev)
+static int iommu_dma_init_domain(struct iommu_domain *domain, struct device *dev)
  {
  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
+	const struct bus_dma_region *map = dev->dma_range_map;
  	unsigned long order, base_pfn;
  	struct iova_domain *iovad;
  	int ret;

@@ -684,18 +681,18 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base,
 
  	/* Use the smallest supported page size for IOVA granularity */
  	order = __ffs(domain->pgsize_bitmap);
-	base_pfn = max_t(unsigned long, 1, base >> order);
+	base_pfn = 1;
 
  	/* Check the domain allows at least some access to the device... */
-	if (domain->geometry.force_aperture) {
+	if (map) {
+		dma_addr_t base = dma_range_map_min(map);
  		if (base > domain->geometry.aperture_end ||
-		    limit < domain->geometry.aperture_start) {
+		    dma_range_map_max(map) < domain->geometry.aperture_start) {
  			pr_warn("specified DMA range outside IOMMU capability\n");
  			return -EFAULT;
  		}
  		/* ...then finally give it a kicking to make sure it fits */
-		base_pfn = max_t(unsigned long, base_pfn,
-				domain->geometry.aperture_start >> order);
+		base_pfn = max(base, domain->geometry.aperture_start) >> order;
  	}
 
  	/* start_pfn is always nonzero for an already-initialised domain */

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

---
 arch/arm64/mm/dma-mapping.c  |  2 --
 drivers/iommu/amd/iommu.c    |  8 --------
 drivers/iommu/dma-iommu.c    | 18 ++++++------------
 drivers/iommu/dma-iommu.h    | 14 ++++++--------
 drivers/iommu/intel/iommu.c  |  7 -------
 drivers/iommu/iommu.c        | 20 +++++++-------------
 drivers/iommu/s390-iommu.c   |  6 ------
 drivers/iommu/virtio-iommu.c | 10 ----------
 include/linux/iommu.h        |  7 -------
 9 files changed, 19 insertions(+), 73 deletions(-)

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
 arch/arm64/mm/dma-mapping.c  |  2 --
 drivers/iommu/amd/iommu.c    |  8 --------
 drivers/iommu/dma-iommu.c    | 18 ++++++------------
 drivers/iommu/dma-iommu.h    | 14 ++++++--------
 drivers/iommu/intel/iommu.c  |  7 -------
 drivers/iommu/iommu.c        | 20 +++++++-------------
 drivers/iommu/s390-iommu.c   |  6 ------
 drivers/iommu/virtio-iommu.c | 10 ----------
 include/linux/iommu.h        |  7 -------
 9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4
[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Quoted 46 lines · Dmitry Baryshkov

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
 arch/arm64/mm/dma-mapping.c  |  2 --
 drivers/iommu/amd/iommu.c    |  8 --------
 drivers/iommu/dma-iommu.c    | 18 ++++++------------
 drivers/iommu/dma-iommu.h    | 14 ++++++--------
 drivers/iommu/intel/iommu.c  |  7 -------
 drivers/iommu/iommu.c        | 20 +++++++-------------
 drivers/iommu/s390-iommu.c   |  6 ------
 drivers/iommu/virtio-iommu.c | 10 ----------
 include/linux/iommu.h        |  7 -------
 9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

Quoted 15 lines

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4
[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Just to confirm: reverting f091e93306e0 ("dma-mapping: Simplify arch_setup_dma_ops()") and b67483b3c44e ("iommu/dma: Centralise iommu_setup_dma_ops()" fixes the issue for me. Please ping me if you'd like me to test a fix.

Quoted 64 lines · Dmitry Baryshkov

Quoted 46 lines · Dmitry Baryshkov

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
  arch/arm64/mm/dma-mapping.c  |  2 --
  drivers/iommu/amd/iommu.c    |  8 --------
  drivers/iommu/dma-iommu.c    | 18 ++++++------------
  drivers/iommu/dma-iommu.h    | 14 ++++++--------
  drivers/iommu/intel/iommu.c  |  7 -------
  drivers/iommu/iommu.c        | 20 +++++++-------------
  drivers/iommu/s390-iommu.c   |  6 ------
  drivers/iommu/virtio-iommu.c | 10 ----------
  include/linux/iommu.h        |  7 -------
  9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

Quoted 15 lines

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4
[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Quoted 4 lines

Just to confirm: reverting f091e93306e0 ("dma-mapping: Simplify arch_setup_dma_ops()") and b67483b3c44e ("iommu/dma: Centralise iommu_setup_dma_ops()" fixes the issue for me. Please ping me if you'd like me to test a fix.

This also triggers a different issue (that also comes down to "ufs bad") on another QC platform (SM8550):

[    4.282098] scsi host0: ufshcd
[    4.315970] ufshcd-qcom 1d84000.ufs: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    4.330155] host_regs: 00000000: 3587031f 00000000 00000400 00000000
[    4.343955] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    4.356027] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    4.370136] host_regs: 00000030: 0000000f 00000003 00000000 00000000
[    4.376662] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    4.383192] host_regs: 00000050: 85109000 00000008 00000000 00000000
[    4.389719] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    4.396245] host_regs: 00000070: 8510a000 00000008 00000000 00000000
[    4.402773] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    4.409298] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    4.415900] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x8851093e0, fsynr=0x3b0001, cbfrsynra=0x60, cb=2
[    4.416135] ufshcd-qcom 1d84000.ufs: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.951750] ufshcd-qcom 1d84000.ufs: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.960644] ufshcd-qcom 1d84000.ufs: ufshcd_async_scan failed: -11

Reverting the commits Dmitry mentioned also fixes this.

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
  arch/arm64/mm/dma-mapping.c  |  2 --
  drivers/iommu/amd/iommu.c    |  8 --------
  drivers/iommu/dma-iommu.c    | 18 ++++++------------
  drivers/iommu/dma-iommu.h    | 14 ++++++--------
  drivers/iommu/intel/iommu.c  |  7 -------
  drivers/iommu/iommu.c        | 20 +++++++-------------
  drivers/iommu/s390-iommu.c   |  6 ------
  drivers/iommu/virtio-iommu.c | 10 ----------
  include/linux/iommu.h        |  7 -------
  9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4

[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Quoted 45 lines · Dmitry Baryshkov

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
  arch/arm64/mm/dma-mapping.c  |  2 --
  drivers/iommu/amd/iommu.c    |  8 --------
  drivers/iommu/dma-iommu.c    | 18 ++++++------------
  drivers/iommu/dma-iommu.h    | 14 ++++++--------
  drivers/iommu/intel/iommu.c  |  7 -------
  drivers/iommu/iommu.c        | 20 +++++++-------------
  drivers/iommu/s390-iommu.c   |  6 ------
  drivers/iommu/virtio-iommu.c | 10 ----------
  include/linux/iommu.h        |  7 -------
  9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

Quoted 1 line

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4

Hmm, a context fault implies that the device did get attached to a DMA domain, thus has successfully been through __iommu_probe_device(), yet somehow still didn't get the right DMA ops (since that "IOVA" looks more like a PA to me). Do you see the "Adding to IOMMU group..." message for this device, and/or any other relevant messages or errors before this point?

I'm guessing there's a fair chance probe deferral might be involved as well. I'd like to understand what path(s) this ends up taking through __iommu_probe_device() and of_dma_configure(), or at least the number and order of probe attempts between the UFS and SMMU drivers.

I'll stare at the code in the morning and see if I can spot any overlooked ways in which what I think might be happening could happen, but any more info to help narrow it down would be much appreciated.

Thanks,
Robin.

Quoted 15 lines

[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Quoted 47 lines · Robin Murphy

Quoted 45 lines · Dmitry Baryshkov

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
   arch/arm64/mm/dma-mapping.c  |  2 --
   drivers/iommu/amd/iommu.c    |  8 --------
   drivers/iommu/dma-iommu.c    | 18 ++++++------------
   drivers/iommu/dma-iommu.h    | 14 ++++++--------
   drivers/iommu/intel/iommu.c  |  7 -------
   drivers/iommu/iommu.c        | 20 +++++++-------------
   drivers/iommu/s390-iommu.c   |  6 ------
   drivers/iommu/virtio-iommu.c | 10 ----------
   include/linux/iommu.h        |  7 -------
   9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

Quoted 8 lines

Quoted 1 line

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4

Hmm, a context fault implies that the device did get attached to a DMA domain, thus has successfully been through __iommu_probe_device(), yet somehow still didn't get the right DMA ops (since that "IOVA" looks more like a PA to me). Do you see the "Adding to IOMMU group..." message for this device, and/or any other relevant messages or errors before this point?

No, nothing relevant.

[    8.372395] ufshcd-qcom 1d84000.ufshc: Adding to iommu group 6

(please ignore the timestamp, it comes before ufshc being probed).

Quoted 5 lines

I'm guessing there's a fair chance probe deferral might be involved as well. I'd like to understand what path(s) this ends up taking through __iommu_probe_device() and of_dma_configure(), or at least the number and order of probe attempts between the UFS and SMMU drivers.

__iommu_probe_device() gets called twice and returns early because ops is NULL.

Then finally of_dma_configure_id() is called. The following branches are taken:

np == dev->of_node
of_dma_get_range() returned 0
bus_dma_limit and dma_range_map are set __iommu_probe_device() is called, using the !group->default_domain && !group_lis case, then group->default_domain() is not NULL, In the end, iommu_setup_dma_ops() is called.

Then the ufshc probe defers (most likely the PHY is not present or some other device is not there yet).

On the next (succeeding) try, of_dma_configure_id() is called again. The call trace is more or less the same, except that __iommu_probe_device() is not called

Quoted 22 lines

I'll stare at the code in the morning and see if I can spot any overlooked ways in which what I think might be happening could happen, but any more info to help narrow it down would be much appreciated.

Thanks,
Robin.

Quoted 15 lines

[    3.846880] ufshcd-qcom 1d84000.ufshc: ufshcd_check_errors: saved_err 0x20000 saved_uic_err 0x0
[    3.846929] host_regs: 00000000: 1587031f 00000000 00000300 00000000
[    3.846935] host_regs: 00000010: 01000000 00010217 00000000 00000000
[    3.846941] host_regs: 00000020: 00000000 00070ef5 00000000 00000000
[    3.846946] host_regs: 00000030: 0000000f 00000001 00000000 00000000
[    3.846951] host_regs: 00000040: 00000000 00000000 00000000 00000000
[    3.846956] host_regs: 00000050: 032db000 00000001 00000000 00000000
[    3.846962] host_regs: 00000060: 00000000 80000000 00000000 00000000
[    3.846967] host_regs: 00000070: 032dd000 00000001 00000000 00000000
[    3.846972] host_regs: 00000080: 00000000 00000000 00000000 00000000
[    3.846977] host_regs: 00000090: 00000016 00000000 00000000 0000000c
[    3.847074] ufshcd-qcom 1d84000.ufshc: ufshcd_err_handler started; HBA state eh_fatal; powered 1; shutting down 0; saved_err = 131072; saved_uic_err = 0; force_reset = 0
[    4.406550] ufshcd-qcom 1d84000.ufshc: ufshcd_verify_dev_init: NOP OUT failed -11
[    4.417953] ufshcd-qcom 1d84000.ufshc: ufshcd_async_scan failed: -11

Quoted 80 lines · Dmitry Baryshkov

Quoted 47 lines · Robin Murphy

Quoted 45 lines · Dmitry Baryshkov

Quoted 29 lines · Robin Murphy

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---
v2: Shuffle around to make sure the iommu_group_do_probe_finalize() case
is covered as well, with bonus side-effects as above.
v3: *Really* do that, remembering the other two probe_finalize sites too.

Quoted 11 lines

---
   arch/arm64/mm/dma-mapping.c  |  2 --
   drivers/iommu/amd/iommu.c    |  8 --------
   drivers/iommu/dma-iommu.c    | 18 ++++++------------
   drivers/iommu/dma-iommu.h    | 14 ++++++--------
   drivers/iommu/intel/iommu.c  |  7 -------
   drivers/iommu/iommu.c        | 20 +++++++-------------
   drivers/iommu/s390-iommu.c   |  6 ------
   drivers/iommu/virtio-iommu.c | 10 ----------
   include/linux/iommu.h        |  7 -------
   9 files changed, 19 insertions(+), 73 deletions(-)

This patch breaks UFS on Qualcomm SC8180X Primus platform:

Quoted 8 lines

Quoted 1 line

[    3.846856] arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x1032db3e0, fsynr=0x130000, cbfrsynra=0x300, cb=4

Hmm, a context fault implies that the device did get attached to a DMA domain, thus has successfully been through __iommu_probe_device(), yet somehow still didn't get the right DMA ops (since that "IOVA" looks more like a PA to me). Do you see the "Adding to IOMMU group..." message for this device, and/or any other relevant messages or errors before this point?

No, nothing relevant.

[    8.372395] ufshcd-qcom 1d84000.ufshc: Adding to iommu group 6

(please ignore the timestamp, it comes before ufshc being probed).

Quoted 5 lines

I'm guessing there's a fair chance probe deferral might be involved as well. I'd like to understand what path(s) this ends up taking through __iommu_probe_device() and of_dma_configure(), or at least the number and order of probe attempts between the UFS and SMMU drivers.

__iommu_probe_device() gets called twice and returns early because ops is NULL.

Then finally of_dma_configure_id() is called. The following branches are taken:

np == dev->of_node
of_dma_get_range() returned 0
bus_dma_limit and dma_range_map are set __iommu_probe_device() is called, using the !group->default_domain && !group_lis case, then group->default_domain() is not NULL, In the end, iommu_setup_dma_ops() is called.

Quoted 2 lines

Then the ufshc probe defers (most likely the PHY is not present or some other device is not there yet).

Ah good, probe deferral. And indeed the half-formed hunch from last night grew into a pretty definite idea by this morning... patch incoming.

It's somewhat hard to see, but arm64's arch_setup_dma_ops() should only ever call iommu_setup_dma_ops() after a successful iommu_probe_device(), which means there should be no harm in achieving the same order of operations by running it off the back of iommu_probe_device() itself. This then puts it in line with the x86 and s390 .probe_finalize bodges, letting us pull it all into the main flow properly. As a bonus this lets us fold in and de-scope the PCI workaround setup as well.

At this point we can also then pull the call up inside the group mutex, and avoid having to think about whether iommu_group_store_type() could theoretically race and free the domain if iommu_setup_dma_ops() ran just *before* iommu_device_use_default_domain() claims it... Furthermore we replace one .probe_finalize call completely, since the only remaining implementations are now one which only needs to run once for the initial boot-time probe, and two which themselves render that path unreachable.

This leaves us a big step closer to realistically being able to unpick the variety of different things that iommu_setup_dma_ops() has been muddling together, and further streamline iommu-dma into core API flows in future.

Reviewed-by Lu Baolu <[email protected]> # For Intel IOMMU

Reviewed-by Jason Gunthorpe <[email protected]>

Tested-by Hanjun Guo <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

drivers/iommu/s390-iommu.c +0 -5

index 9a5196f523de..d8eaa7ea380b 100644
@@ -695,11 +695,6 @@ static size_t s390_iommu_unmap_pages(struct iommu_domain *domain,
 	return size;
 }
 
-static void s390_iommu_probe_finalize(struct device *dev)
-{
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-}
-
 struct zpci_iommu_ctrs *zpci_get_iommu_ctrs(struct zpci_dev *zdev)
 {
 	if (!zdev || !zdev->s390_domain)

v3: https://lore.kernel.org/linux-iommu/[email protected]/

Hi all,

Since this ended up missing the boat for 6.9, here's a rebase and resend with the additional tags from v3 collected.

Since this ended up missing the boat for 6.9, here's a rebase and resend with the additional tags from v3 collected.