[PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Robin Murphy]

Hi all,

Prompted by Jason's proposal[1], here's a first step towards truly
unpicking the dma_configure vs. IOMMU mess. As I commented before, we
have an awful lot of accumulated cruft and technical debt here making
things more complicated than they need to be, and we already have hacks
on top of hacks trying to work around it, so polishing those hacks even
further is really not a desirable direction of travel. And I do know
they're hacks, because I wrote most of them and still remember enough of
the context of the time ;)

I'm taking a methodical bottom-up approach here, so step 1 is cleaning
*all* the out-of-date stuff from arch_setup_dma_ops() and simplifying
that interface, which gets it right out of the way for the next step of
pulling apart {of,acpi}_dma_configure(). This part is really a
dma-mapping series, but I'm not sure yet if would need to target the
IOMMU tree - nothing here should strictly depend on the pending IOMMU
change, but the follow-up patches might. Still working on those, so
hopefully I'll know soon...

Thanks,
Robin.

[1] https://lore.kernel.org/linux-iommu/0-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com/


Robin Murphy (7):
  OF: Retire dma-ranges mask workaround
  OF: Simplify DMA range calculations
  ACPI/IORT: Handle memory address size limits as limits
  dma-mapping: Add helpers for dma_range_map bounds
  iommu/dma: Make limit checks self-contained
  iommu/dma: Centralise iommu_setup_dma_ops()
  dma-mapping: Simplify arch_setup_dma_ops()

 arch/arc/mm/dma.c               |  3 +--
 arch/arm/mm/dma-mapping-nommu.c |  3 +--
 arch/arm/mm/dma-mapping.c       | 12 ++++++----
 arch/arm64/mm/dma-mapping.c     |  5 +---
 arch/loongarch/kernel/dma.c     |  9 ++-----
 arch/mips/mm/dma-noncoherent.c  |  3 +--
 arch/riscv/mm/dma-noncoherent.c |  3 +--
 drivers/acpi/arm64/dma.c        | 17 ++++---------
 drivers/acpi/arm64/iort.c       | 18 +++++++-------
 drivers/acpi/scan.c             |  3 +--
 drivers/hv/hv_common.c          |  6 +----
 drivers/iommu/amd/iommu.c       |  8 -------
 drivers/iommu/dma-iommu.c       | 35 ++++++++++-----------------
 drivers/iommu/dma-iommu.h       |  6 +++++
 drivers/iommu/intel/iommu.c     |  7 ------
 drivers/iommu/iommu.c           |  2 ++
 drivers/iommu/s390-iommu.c      |  6 -----
 drivers/iommu/virtio-iommu.c    | 10 --------
 drivers/of/device.c             | 42 ++++++---------------------------
 include/linux/acpi_iort.h       |  4 ++--
 include/linux/dma-direct.h      | 18 ++++++++++++++
 include/linux/dma-map-ops.h     |  6 ++---
 include/linux/iommu.h           |  7 ------
 23 files changed, 78 insertions(+), 155 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Robin Murphy]

Hi all,

Prompted by Jason's proposal[1], here's a first step towards truly
unpicking the dma_configure vs. IOMMU mess. As I commented before, we
have an awful lot of accumulated cruft and technical debt here making
things more complicated than they need to be, and we already have hacks
on top of hacks trying to work around it, so polishing those hacks even
further is really not a desirable direction of travel. And I do know
they're hacks, because I wrote most of them and still remember enough of
the context of the time ;)

I'm taking a methodical bottom-up approach here, so step 1 is cleaning
*all* the out-of-date stuff from arch_setup_dma_ops() and simplifying
that interface, which gets it right out of the way for the next step of
pulling apart {of,acpi}_dma_configure(). This part is really a
dma-mapping series, but I'm not sure yet if would need to target the
IOMMU tree - nothing here should strictly depend on the pending IOMMU
change, but the follow-up patches might. Still working on those, so
hopefully I'll know soon...

Thanks,
Robin.

[1] https://lore.kernel.org/linux-iommu/0-v1-720585788a7d+811b-iommu_fwspec_p1_jgg@nvidia.com/


Robin Murphy (7):
  OF: Retire dma-ranges mask workaround
  OF: Simplify DMA range calculations
  ACPI/IORT: Handle memory address size limits as limits
  dma-mapping: Add helpers for dma_range_map bounds
  iommu/dma: Make limit checks self-contained
  iommu/dma: Centralise iommu_setup_dma_ops()
  dma-mapping: Simplify arch_setup_dma_ops()

 arch/arc/mm/dma.c               |  3 +--
 arch/arm/mm/dma-mapping-nommu.c |  3 +--
 arch/arm/mm/dma-mapping.c       | 12 ++++++----
 arch/arm64/mm/dma-mapping.c     |  5 +---
 arch/loongarch/kernel/dma.c     |  9 ++-----
 arch/mips/mm/dma-noncoherent.c  |  3 +--
 arch/riscv/mm/dma-noncoherent.c |  3 +--
 drivers/acpi/arm64/dma.c        | 17 ++++---------
 drivers/acpi/arm64/iort.c       | 18 +++++++-------
 drivers/acpi/scan.c             |  3 +--
 drivers/hv/hv_common.c          |  6 +----
 drivers/iommu/amd/iommu.c       |  8 -------
 drivers/iommu/dma-iommu.c       | 35 ++++++++++-----------------
 drivers/iommu/dma-iommu.h       |  6 +++++
 drivers/iommu/intel/iommu.c     |  7 ------
 drivers/iommu/iommu.c           |  2 ++
 drivers/iommu/s390-iommu.c      |  6 -----
 drivers/iommu/virtio-iommu.c    | 10 --------
 drivers/of/device.c             | 42 ++++++---------------------------
 include/linux/acpi_iort.h       |  4 ++--
 include/linux/dma-direct.h      | 18 ++++++++++++++
 include/linux/dma-map-ops.h     |  6 ++---
 include/linux/iommu.h           |  7 ------
 23 files changed, 78 insertions(+), 155 deletions(-)

-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/7] OF: Retire dma-ranges mask workaround

[Robin Murphy]

From what I remember, the fixup adding 1 to the dma-ranges size was for
the benefit of some early AMD Seattle DTs. Those are likely extinct by
now, and anyone else who might have deserved to get the message has
hopefully seen the warning in the 9 years we've had it there. The modern
dma_range_map mechanism should happily handle odd-sized ranges with no
ill effect, so there's little need to care anyway now. Clean it up.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/of/device.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 1ca42ad9dd15..526a42cdf66e 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -129,22 +129,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 				dma_end = r->dma_start + r->size;
 		}
 		size = dma_end - dma_start;
-
-		/*
-		 * Add a work around to treat the size as mask + 1 in case
-		 * it is defined in DT as a mask.
-		 */
-		if (size & 1) {
-			dev_warn(dev, "Invalid size 0x%llx for dma-range(s)\n",
-				 size);
-			size = size + 1;
-		}
-
-		if (!size) {
-			dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
-			kfree(map);
-			return -EINVAL;
-		}
 	}
 
 	/*
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 1/7] OF: Retire dma-ranges mask workaround

[Robin Murphy]

From what I remember, the fixup adding 1 to the dma-ranges size was for
the benefit of some early AMD Seattle DTs. Those are likely extinct by
now, and anyone else who might have deserved to get the message has
hopefully seen the warning in the 9 years we've had it there. The modern
dma_range_map mechanism should happily handle odd-sized ranges with no
ill effect, so there's little need to care anyway now. Clean it up.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/of/device.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 1ca42ad9dd15..526a42cdf66e 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -129,22 +129,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 				dma_end = r->dma_start + r->size;
 		}
 		size = dma_end - dma_start;
-
-		/*
-		 * Add a work around to treat the size as mask + 1 in case
-		 * it is defined in DT as a mask.
-		 */
-		if (size & 1) {
-			dev_warn(dev, "Invalid size 0x%llx for dma-range(s)\n",
-				 size);
-			size = size + 1;
-		}
-
-		if (!size) {
-			dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
-			kfree(map);
-			return -EINVAL;
-		}
 	}
 
 	/*
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/7] OF: Retire dma-ranges mask workaround

[Rob Herring]

On Wed, Nov 29, 2023 at 11:43 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> From what I remember, the fixup adding 1 to the dma-ranges size was for
> the benefit of some early AMD Seattle DTs. Those are likely extinct by
> now, and anyone else who might have deserved to get the message has
> hopefully seen the warning in the 9 years we've had it there. The modern
> dma_range_map mechanism should happily handle odd-sized ranges with no
> ill effect, so there's little need to care anyway now. Clean it up.

The commit has a tested by for Seattle, but the series adding this was
for an issue on TI Keystone[1]. Looks like the patch adding this fixup
and warning did 2 things. It added 1 to the default mask when
'dma-ranges' was not present (which keystone needed) and added 1 if
the DT value was a mask along with the warning. It's not clear what
Seattle needed, but there was a fix to dma-ranges about a year
later[2].

I thought at some point we allowed 32-bit DTs to specify a ~0 size to
avoid having to use 2 cells to express 4G size which wouldn't have
been a warning, but I can't find any discussion on that. It would have
been earlier than 2015 I think... Anyways, there is no upstream dts
with that either, so I think we're good.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/of/device.c | 16 ----------------
>  1 file changed, 16 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

[1] https://lore.kernel.org/all/1425405134-24707-1-git-send-email-m-karicheri2@ti.com/
[2] https://lore.kernel.org/all/1455162671-16044-4-git-send-email-Suravee.Suthikulpanit@amd.com/

Re: [PATCH 1/7] OF: Retire dma-ranges mask workaround

[Rob Herring]

On Wed, Nov 29, 2023 at 11:43 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> From what I remember, the fixup adding 1 to the dma-ranges size was for
> the benefit of some early AMD Seattle DTs. Those are likely extinct by
> now, and anyone else who might have deserved to get the message has
> hopefully seen the warning in the 9 years we've had it there. The modern
> dma_range_map mechanism should happily handle odd-sized ranges with no
> ill effect, so there's little need to care anyway now. Clean it up.

The commit has a tested by for Seattle, but the series adding this was
for an issue on TI Keystone[1]. Looks like the patch adding this fixup
and warning did 2 things. It added 1 to the default mask when
'dma-ranges' was not present (which keystone needed) and added 1 if
the DT value was a mask along with the warning. It's not clear what
Seattle needed, but there was a fix to dma-ranges about a year
later[2].

I thought at some point we allowed 32-bit DTs to specify a ~0 size to
avoid having to use 2 cells to express 4G size which wouldn't have
been a warning, but I can't find any discussion on that. It would have
been earlier than 2015 I think... Anyways, there is no upstream dts
with that either, so I think we're good.

> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/of/device.c | 16 ----------------
>  1 file changed, 16 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

[1] https://lore.kernel.org/all/1425405134-24707-1-git-send-email-m-karicheri2@ti.com/
[2] https://lore.kernel.org/all/1455162671-16044-4-git-send-email-Suravee.Suthikulpanit@amd.com/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/7] OF: Simplify DMA range calculations

[Robin Murphy]

Juggling start, end, and size values for a range is somewhat redundant
and a little hard to follow. Consolidate down to just using inclusive
start and end, which saves us worrying about size overflows for full
64-bit ranges (note that passing a potentially-overflowed value through
to arch_setup_dma_ops() is benign for all current implementations, and
this is working towards removing that anyway).

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/of/device.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 526a42cdf66e..51062a831970 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -97,7 +97,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	const struct bus_dma_region *map = NULL;
 	struct device_node *bus_np;
 	u64 dma_start = 0;
-	u64 mask, end, size = 0;
+	u64 mask, end = 0;
 	bool coherent;
 	int ret;
 
@@ -118,17 +118,15 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 			return ret == -ENODEV ? 0 : ret;
 	} else {
 		const struct bus_dma_region *r = map;
-		u64 dma_end = 0;
 
 		/* Determine the overall bounds of all DMA regions */
 		for (dma_start = ~0; r->size; r++) {
 			/* Take lower and upper limits */
 			if (r->dma_start < dma_start)
 				dma_start = r->dma_start;
-			if (r->dma_start + r->size > dma_end)
-				dma_end = r->dma_start + r->size;
+			if (r->dma_start + r->size > end)
+				end = r->dma_start + r->size;
 		}
-		size = dma_end - dma_start;
 	}
 
 	/*
@@ -142,16 +140,15 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		dev->dma_mask = &dev->coherent_dma_mask;
 	}
 
-	if (!size && dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
-	else if (!size)
-		size = 1ULL << 32;
+	if (!end && dev->coherent_dma_mask)
+		end = dev->coherent_dma_mask;
+	else if (!end)
+		end = (1ULL << 32) - 1;
 
 	/*
 	 * Limit coherent and dma mask based on size and default mask
 	 * set by the driver.
 	 */
-	end = dma_start + size - 1;
 	mask = DMA_BIT_MASK(ilog2(end) + 1);
 	dev->coherent_dma_mask &= mask;
 	*dev->dma_mask &= mask;
@@ -177,7 +174,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
+	arch_setup_dma_ops(dev, dma_start, end - dma_start + 1, iommu, coherent);
 
 	if (!iommu)
 		of_dma_set_restricted_buffer(dev, np);
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 2/7] OF: Simplify DMA range calculations

[Robin Murphy]

Juggling start, end, and size values for a range is somewhat redundant
and a little hard to follow. Consolidate down to just using inclusive
start and end, which saves us worrying about size overflows for full
64-bit ranges (note that passing a potentially-overflowed value through
to arch_setup_dma_ops() is benign for all current implementations, and
this is working towards removing that anyway).

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/of/device.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 526a42cdf66e..51062a831970 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -97,7 +97,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	const struct bus_dma_region *map = NULL;
 	struct device_node *bus_np;
 	u64 dma_start = 0;
-	u64 mask, end, size = 0;
+	u64 mask, end = 0;
 	bool coherent;
 	int ret;
 
@@ -118,17 +118,15 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 			return ret == -ENODEV ? 0 : ret;
 	} else {
 		const struct bus_dma_region *r = map;
-		u64 dma_end = 0;
 
 		/* Determine the overall bounds of all DMA regions */
 		for (dma_start = ~0; r->size; r++) {
 			/* Take lower and upper limits */
 			if (r->dma_start < dma_start)
 				dma_start = r->dma_start;
-			if (r->dma_start + r->size > dma_end)
-				dma_end = r->dma_start + r->size;
+			if (r->dma_start + r->size > end)
+				end = r->dma_start + r->size;
 		}
-		size = dma_end - dma_start;
 	}
 
 	/*
@@ -142,16 +140,15 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		dev->dma_mask = &dev->coherent_dma_mask;
 	}
 
-	if (!size && dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
-	else if (!size)
-		size = 1ULL << 32;
+	if (!end && dev->coherent_dma_mask)
+		end = dev->coherent_dma_mask;
+	else if (!end)
+		end = (1ULL << 32) - 1;
 
 	/*
 	 * Limit coherent and dma mask based on size and default mask
 	 * set by the driver.
 	 */
-	end = dma_start + size - 1;
 	mask = DMA_BIT_MASK(ilog2(end) + 1);
 	dev->coherent_dma_mask &= mask;
 	*dev->dma_mask &= mask;
@@ -177,7 +174,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_start, size, iommu, coherent);
+	arch_setup_dma_ops(dev, dma_start, end - dma_start + 1, iommu, coherent);
 
 	if (!iommu)
 		of_dma_set_restricted_buffer(dev, np);
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] OF: Simplify DMA range calculations

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:42:59PM +0000, Robin Murphy wrote:
> Juggling start, end, and size values for a range is somewhat redundant
> and a little hard to follow. Consolidate down to just using inclusive
> start and end, which saves us worrying about size overflows for full
> 64-bit ranges (note that passing a potentially-overflowed value through
> to arch_setup_dma_ops() is benign for all current implementations, and
> this is working towards removing that anyway).

In iommu code I've been trying to use consistent language with other
parts of the kernel like interval tree and maple tree:

 * In this file the term 'last' indicates an inclusive and closed interval, eg
 * [0,0] refers to a single PFN. 'end' means an open range, eg [0,0) refers to
 * no PFNs.

Here I think you've swapped end to mean last?

Regardless the change looks correct

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 2/7] OF: Simplify DMA range calculations

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:42:59PM +0000, Robin Murphy wrote:
> Juggling start, end, and size values for a range is somewhat redundant
> and a little hard to follow. Consolidate down to just using inclusive
> start and end, which saves us worrying about size overflows for full
> 64-bit ranges (note that passing a potentially-overflowed value through
> to arch_setup_dma_ops() is benign for all current implementations, and
> this is working towards removing that anyway).

In iommu code I've been trying to use consistent language with other
parts of the kernel like interval tree and maple tree:

 * In this file the term 'last' indicates an inclusive and closed interval, eg
 * [0,0] refers to a single PFN. 'end' means an open range, eg [0,0) refers to
 * no PFNs.

Here I think you've swapped end to mean last?

Regardless the change looks correct

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/7] OF: Simplify DMA range calculations

[Rob Herring]

On Wed, Nov 29, 2023 at 11:43 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Juggling start, end, and size values for a range is somewhat redundant
> and a little hard to follow. Consolidate down to just using inclusive
> start and end, which saves us worrying about size overflows for full
> 64-bit ranges (note that passing a potentially-overflowed value through
> to arch_setup_dma_ops() is benign for all current implementations, and
> this is working towards removing that anyway).
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/of/device.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/7] OF: Simplify DMA range calculations

[Rob Herring]

On Wed, Nov 29, 2023 at 11:43 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> Juggling start, end, and size values for a range is somewhat redundant
> and a little hard to follow. Consolidate down to just using inclusive
> start and end, which saves us worrying about size overflows for full
> 64-bit ranges (note that passing a potentially-overflowed value through
> to arch_setup_dma_ops() is benign for all current implementations, and
> this is working towards removing that anyway).
>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/of/device.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

Return the Root Complex/Named Component memory address size limit as an
inclusive limit value, rather than an exclusive size.  This saves us
having to special-case 64-bit overflow, and simplifies our caller too.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/acpi/arm64/dma.c  |  9 +++------
 drivers/acpi/arm64/iort.c | 18 ++++++++----------
 include/linux/acpi_iort.h |  4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index 93d796531af3..b98a149f8d50 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -8,7 +8,6 @@ void acpi_arch_dma_setup(struct device *dev)
 {
 	int ret;
 	u64 end, mask;
-	u64 size = 0;
 	const struct bus_dma_region *map = NULL;
 
 	/*
@@ -23,9 +22,9 @@ void acpi_arch_dma_setup(struct device *dev)
 	}
 
 	if (dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
+		end = dev->coherent_dma_mask;
 	else
-		size = 1ULL << 32;
+		end = (1ULL << 32) - 1;
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
@@ -36,18 +35,16 @@ void acpi_arch_dma_setup(struct device *dev)
 				end = r->dma_start + r->size - 1;
 		}
 
-		size = end + 1;
 		dev->dma_range_map = map;
 	}
 
 	if (ret == -ENODEV)
-		ret = iort_dma_get_ranges(dev, &size);
+		ret = iort_dma_get_ranges(dev, &end);
 	if (!ret) {
 		/*
 		 * Limit coherent and dma mask based on size retrieved from
 		 * firmware.
 		 */
-		end = size - 1;
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
 		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 6496ff5a6ba2..eb64d8e17dd1 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
 { return -ENODEV; }
 #endif
 
-static int nc_dma_get_range(struct device *dev, u64 *size)
+static int nc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_named_component *ncomp;
@@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<ncomp->memory_address_limit;
+	*limit = (1ULL << ncomp->memory_address_limit) - 1;
 
 	return 0;
 }
 
-static int rc_dma_get_range(struct device *dev, u64 *size)
+static int rc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_root_complex *rc;
@@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = rc->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<rc->memory_address_limit;
+	*limit = (1ULL << rc->memory_address_limit) - 1;
 
 	return 0;
 }
@@ -1417,16 +1415,16 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 /**
  * iort_dma_get_ranges() - Look up DMA addressing limit for the device
  * @dev: device to lookup
- * @size: DMA range size result pointer
+ * @limit: DMA limit result pointer
  *
  * Return: 0 on success, an error otherwise.
  */
-int iort_dma_get_ranges(struct device *dev, u64 *size)
+int iort_dma_get_ranges(struct device *dev, u64 *limit)
 {
 	if (dev_is_pci(dev))
-		return rc_dma_get_range(dev, size);
+		return rc_dma_get_range(dev, limit);
 	else
-		return nc_dma_get_range(dev, size);
+		return nc_dma_get_range(dev, limit);
 }
 
 static void __init acpi_iort_register_irq(int hwirq, const char *name,
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 1cb65592c95d..d4ed5622cf2b 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -39,7 +39,7 @@ void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode,
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
 		       struct list_head *head);
 /* IOMMU interface */
-int iort_dma_get_ranges(struct device *dev, u64 *size);
+int iort_dma_get_ranges(struct device *dev, u64 *limit);
 int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
 void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
@@ -55,7 +55,7 @@ void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *hea
 static inline
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *head) { }
 /* IOMMU interface */
-static inline int iort_dma_get_ranges(struct device *dev, u64 *size)
+static inline int iort_dma_get_ranges(struct device *dev, u64 *limit)
 { return -ENODEV; }
 static inline int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
 { return -ENODEV; }
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

Return the Root Complex/Named Component memory address size limit as an
inclusive limit value, rather than an exclusive size.  This saves us
having to special-case 64-bit overflow, and simplifies our caller too.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/acpi/arm64/dma.c  |  9 +++------
 drivers/acpi/arm64/iort.c | 18 ++++++++----------
 include/linux/acpi_iort.h |  4 ++--
 3 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index 93d796531af3..b98a149f8d50 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -8,7 +8,6 @@ void acpi_arch_dma_setup(struct device *dev)
 {
 	int ret;
 	u64 end, mask;
-	u64 size = 0;
 	const struct bus_dma_region *map = NULL;
 
 	/*
@@ -23,9 +22,9 @@ void acpi_arch_dma_setup(struct device *dev)
 	}
 
 	if (dev->coherent_dma_mask)
-		size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
+		end = dev->coherent_dma_mask;
 	else
-		size = 1ULL << 32;
+		end = (1ULL << 32) - 1;
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
@@ -36,18 +35,16 @@ void acpi_arch_dma_setup(struct device *dev)
 				end = r->dma_start + r->size - 1;
 		}
 
-		size = end + 1;
 		dev->dma_range_map = map;
 	}
 
 	if (ret == -ENODEV)
-		ret = iort_dma_get_ranges(dev, &size);
+		ret = iort_dma_get_ranges(dev, &end);
 	if (!ret) {
 		/*
 		 * Limit coherent and dma mask based on size retrieved from
 		 * firmware.
 		 */
-		end = size - 1;
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
 		dev->coherent_dma_mask = min(dev->coherent_dma_mask, mask);
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 6496ff5a6ba2..eb64d8e17dd1 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
 { return -ENODEV; }
 #endif
 
-static int nc_dma_get_range(struct device *dev, u64 *size)
+static int nc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_named_component *ncomp;
@@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<ncomp->memory_address_limit;
+	*limit = (1ULL << ncomp->memory_address_limit) - 1;
 
 	return 0;
 }
 
-static int rc_dma_get_range(struct device *dev, u64 *size)
+static int rc_dma_get_range(struct device *dev, u64 *limit)
 {
 	struct acpi_iort_node *node;
 	struct acpi_iort_root_complex *rc;
@@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 		return -EINVAL;
 	}
 
-	*size = rc->memory_address_limit >= 64 ? U64_MAX :
-			1ULL<<rc->memory_address_limit;
+	*limit = (1ULL << rc->memory_address_limit) - 1;
 
 	return 0;
 }
@@ -1417,16 +1415,16 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
 /**
  * iort_dma_get_ranges() - Look up DMA addressing limit for the device
  * @dev: device to lookup
- * @size: DMA range size result pointer
+ * @limit: DMA limit result pointer
  *
  * Return: 0 on success, an error otherwise.
  */
-int iort_dma_get_ranges(struct device *dev, u64 *size)
+int iort_dma_get_ranges(struct device *dev, u64 *limit)
 {
 	if (dev_is_pci(dev))
-		return rc_dma_get_range(dev, size);
+		return rc_dma_get_range(dev, limit);
 	else
-		return nc_dma_get_range(dev, size);
+		return nc_dma_get_range(dev, limit);
 }
 
 static void __init acpi_iort_register_irq(int hwirq, const char *name,
diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
index 1cb65592c95d..d4ed5622cf2b 100644
--- a/include/linux/acpi_iort.h
+++ b/include/linux/acpi_iort.h
@@ -39,7 +39,7 @@ void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode,
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode,
 		       struct list_head *head);
 /* IOMMU interface */
-int iort_dma_get_ranges(struct device *dev, u64 *size);
+int iort_dma_get_ranges(struct device *dev, u64 *limit);
 int iort_iommu_configure_id(struct device *dev, const u32 *id_in);
 void iort_iommu_get_resv_regions(struct device *dev, struct list_head *head);
 phys_addr_t acpi_iort_dma_get_max_cpu_address(void);
@@ -55,7 +55,7 @@ void iort_get_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *hea
 static inline
 void iort_put_rmr_sids(struct fwnode_handle *iommu_fwnode, struct list_head *head) { }
 /* IOMMU interface */
-static inline int iort_dma_get_ranges(struct device *dev, u64 *size)
+static inline int iort_dma_get_ranges(struct device *dev, u64 *limit)
 { return -ENODEV; }
 static inline int iort_iommu_configure_id(struct device *dev, const u32 *id_in)
 { return -ENODEV; }
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> Return the Root Complex/Named Component memory address size limit as an
> inclusive limit value, rather than an exclusive size.  This saves us
> having to special-case 64-bit overflow, and simplifies our caller too.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/dma.c  |  9 +++------
>  drivers/acpi/arm64/iort.c | 18 ++++++++----------
>  include/linux/acpi_iort.h |  4 ++--
>  3 files changed, 13 insertions(+), 18 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> Return the Root Complex/Named Component memory address size limit as an
> inclusive limit value, rather than an exclusive size.  This saves us
> having to special-case 64-bit overflow, and simplifies our caller too.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/dma.c  |  9 +++------
>  drivers/acpi/arm64/iort.c | 18 ++++++++----------
>  include/linux/acpi_iort.h |  4 ++--
>  3 files changed, 13 insertions(+), 18 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Will Deacon]

On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> Return the Root Complex/Named Component memory address size limit as an
> inclusive limit value, rather than an exclusive size.  This saves us
> having to special-case 64-bit overflow, and simplifies our caller too.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/dma.c  |  9 +++------
>  drivers/acpi/arm64/iort.c | 18 ++++++++----------
>  include/linux/acpi_iort.h |  4 ++--
>  3 files changed, 13 insertions(+), 18 deletions(-)

[...]

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 6496ff5a6ba2..eb64d8e17dd1 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>  { return -ENODEV; }
>  #endif
>  
> -static int nc_dma_get_range(struct device *dev, u64 *size)
> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_named_component *ncomp;
> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<ncomp->memory_address_limit;
> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;

The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
to drop that? You mention it in the cover letter, so clearly I'm missing
something!

>  
>  	return 0;
>  }
>  
> -static int rc_dma_get_range(struct device *dev, u64 *size)
> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_root_complex *rc;
> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<rc->memory_address_limit;
> +	*limit = (1ULL << rc->memory_address_limit) - 1;

Same thing here.

Will

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Will Deacon]

On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> Return the Root Complex/Named Component memory address size limit as an
> inclusive limit value, rather than an exclusive size.  This saves us
> having to special-case 64-bit overflow, and simplifies our caller too.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/acpi/arm64/dma.c  |  9 +++------
>  drivers/acpi/arm64/iort.c | 18 ++++++++----------
>  include/linux/acpi_iort.h |  4 ++--
>  3 files changed, 13 insertions(+), 18 deletions(-)

[...]

> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 6496ff5a6ba2..eb64d8e17dd1 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>  { return -ENODEV; }
>  #endif
>  
> -static int nc_dma_get_range(struct device *dev, u64 *size)
> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_named_component *ncomp;
> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<ncomp->memory_address_limit;
> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;

The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
to drop that? You mention it in the cover letter, so clearly I'm missing
something!

>  
>  	return 0;
>  }
>  
> -static int rc_dma_get_range(struct device *dev, u64 *size)
> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>  {
>  	struct acpi_iort_node *node;
>  	struct acpi_iort_root_complex *rc;
> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>  		return -EINVAL;
>  	}
>  
> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> -			1ULL<<rc->memory_address_limit;
> +	*limit = (1ULL << rc->memory_address_limit) - 1;

Same thing here.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 1:27 pm, Will Deacon wrote:
> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>> Return the Root Complex/Named Component memory address size limit as an
>> inclusive limit value, rather than an exclusive size.  This saves us
>> having to special-case 64-bit overflow, and simplifies our caller too.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/acpi/arm64/dma.c  |  9 +++------
>>   drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>   include/linux/acpi_iort.h |  4 ++--
>>   3 files changed, 13 insertions(+), 18 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>   { return -ENODEV; }
>>   #endif
>>   
>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_named_component *ncomp;
>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<ncomp->memory_address_limit;
>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> 
> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> to drop that? You mention it in the cover letter, so clearly I'm missing
> something!

Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus 
subtracting 1 results in the correct all-bits-set value for an inclusive 
64-bit limit.

Thanks,
Robin.

>>   
>>   	return 0;
>>   }
>>   
>> -static int rc_dma_get_range(struct device *dev, u64 *size)
>> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_root_complex *rc;
>> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<rc->memory_address_limit;
>> +	*limit = (1ULL << rc->memory_address_limit) - 1;
> 
> Same thing here.
> 
> Will

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 1:27 pm, Will Deacon wrote:
> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>> Return the Root Complex/Named Component memory address size limit as an
>> inclusive limit value, rather than an exclusive size.  This saves us
>> having to special-case 64-bit overflow, and simplifies our caller too.
>>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>   drivers/acpi/arm64/dma.c  |  9 +++------
>>   drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>   include/linux/acpi_iort.h |  4 ++--
>>   3 files changed, 13 insertions(+), 18 deletions(-)
> 
> [...]
> 
>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>> --- a/drivers/acpi/arm64/iort.c
>> +++ b/drivers/acpi/arm64/iort.c
>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>   { return -ENODEV; }
>>   #endif
>>   
>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_named_component *ncomp;
>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<ncomp->memory_address_limit;
>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> 
> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> to drop that? You mention it in the cover letter, so clearly I'm missing
> something!

Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus 
subtracting 1 results in the correct all-bits-set value for an inclusive 
64-bit limit.

Thanks,
Robin.

>>   
>>   	return 0;
>>   }
>>   
>> -static int rc_dma_get_range(struct device *dev, u64 *size)
>> +static int rc_dma_get_range(struct device *dev, u64 *limit)
>>   {
>>   	struct acpi_iort_node *node;
>>   	struct acpi_iort_root_complex *rc;
>> @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
>>   		return -EINVAL;
>>   	}
>>   
>> -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
>> -			1ULL<<rc->memory_address_limit;
>> +	*limit = (1ULL << rc->memory_address_limit) - 1;
> 
> Same thing here.
> 
> Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Will Deacon]

On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.

Oh, I'd have thought you'd have gotten one of those "left shift count >=
width of type" warnings if you did that.

Will

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Will Deacon]

On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.

Oh, I'd have thought you'd have gotten one of those "left shift count >=
width of type" warnings if you did that.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Jason Gunthorpe]

On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Yes, UBSAN generates warnings for these cases. I'm not sure if it is
actually undefined C behavior or just "suspicious", but such is what
it is..

Jason

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Jason Gunthorpe]

On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Yes, UBSAN generates warnings for these cases. I'm not sure if it is
actually undefined C behavior or just "suspicious", but such is what
it is..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 3:30 pm, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>> Return the Root Complex/Named Component memory address size limit as an
>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>    { return -ENODEV; }
>>>>    #endif
>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>    {
>>>>    	struct acpi_iort_node *node;
>>>>    	struct acpi_iort_named_component *ncomp;
>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>    		return -EINVAL;
>>>>    	}
>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>> -			1ULL<<ncomp->memory_address_limit;
>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>
>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>> something!
>>
>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>> subtracting 1 results in the correct all-bits-set value for an inclusive
>> 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Compilers might give such a warning if it was a constant shift whose 
size was visible at compile time, but even then only because compilers 
seem to have a vendetta against us relying on the well-defined 
behaviours of unsigned integer overflow (it's only *signed* shifts which 
are UB if the result is unrepresentable).

Cheers,
Robin.

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 3:30 pm, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>> Return the Root Complex/Named Component memory address size limit as an
>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>
>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>> ---
>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>> --- a/drivers/acpi/arm64/iort.c
>>>> +++ b/drivers/acpi/arm64/iort.c
>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>    { return -ENODEV; }
>>>>    #endif
>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>    {
>>>>    	struct acpi_iort_node *node;
>>>>    	struct acpi_iort_named_component *ncomp;
>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>    		return -EINVAL;
>>>>    	}
>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>> -			1ULL<<ncomp->memory_address_limit;
>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>
>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>> something!
>>
>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>> subtracting 1 results in the correct all-bits-set value for an inclusive
>> 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

Compilers might give such a warning if it was a constant shift whose 
size was visible at compile time, but even then only because compilers 
seem to have a vendetta against us relying on the well-defined 
behaviours of unsigned integer overflow (it's only *signed* shifts which 
are UB if the result is unrepresentable).

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Mark Rutland]

On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

I think you'll get a UBSAN splat, but here the compiler doesn't know what
'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
warning.

Regardless, it's undefined behaviour.

Mark.

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Mark Rutland]

On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> > On 2023-12-11 1:27 pm, Will Deacon wrote:
> > > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > > Return the Root Complex/Named Component memory address size limit as an
> > > > inclusive limit value, rather than an exclusive size.  This saves us
> > > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > > 
> > > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > > ---
> > > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > > >   include/linux/acpi_iort.h |  4 ++--
> > > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > > 
> > > [...]
> > > 
> > > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > > --- a/drivers/acpi/arm64/iort.c
> > > > +++ b/drivers/acpi/arm64/iort.c
> > > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > > >   { return -ENODEV; }
> > > >   #endif
> > > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > > >   {
> > > >   	struct acpi_iort_node *node;
> > > >   	struct acpi_iort_named_component *ncomp;
> > > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > > >   		return -EINVAL;
> > > >   	}
> > > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > > -			1ULL<<ncomp->memory_address_limit;
> > > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > > 
> > > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > > to drop that? You mention it in the cover letter, so clearly I'm missing
> > > something!
> > 
> > Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
> > subtracting 1 results in the correct all-bits-set value for an inclusive
> > 64-bit limit.
> 
> Oh, I'd have thought you'd have gotten one of those "left shift count >=
> width of type" warnings if you did that.

I think you'll get a UBSAN splat, but here the compiler doesn't know what
'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
warning.

Regardless, it's undefined behaviour.

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 3:39 pm, Mark Rutland wrote:
> On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
>> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>>> Return the Root Complex/Named Component memory address size limit as an
>>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>>    { return -ENODEV; }
>>>>>    #endif
>>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>>    {
>>>>>    	struct acpi_iort_node *node;
>>>>>    	struct acpi_iort_named_component *ncomp;
>>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>>    		return -EINVAL;
>>>>>    	}
>>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>>> -			1ULL<<ncomp->memory_address_limit;
>>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>>
>>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>>> something!
>>>
>>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>>> subtracting 1 results in the correct all-bits-set value for an inclusive
>>> 64-bit limit.
>>
>> Oh, I'd have thought you'd have gotten one of those "left shift count >=
>> width of type" warnings if you did that.
> 
> I think you'll get a UBSAN splat, but here the compiler doesn't know what
> 'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
> warning.
> 
> Regardless, it's undefined behaviour.

Urgh, you're right... I double-checked 6.5.7.4 in the standard but 
managed to miss 6.5.7.3. So yeah, even though "4 << 62" or "2 << 63" are 
well-defined here, "1  << 64" isn't, dang. Thanks, funky old ISAs which 
did weird things for crazy large shifts and have no relevance to this 
code :(

Cheers,
Robin.

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Robin Murphy]

On 2023-12-11 3:39 pm, Mark Rutland wrote:
> On Mon, Dec 11, 2023 at 03:30:24PM +0000, Will Deacon wrote:
>> On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
>>> On 2023-12-11 1:27 pm, Will Deacon wrote:
>>>> On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
>>>>> Return the Root Complex/Named Component memory address size limit as an
>>>>> inclusive limit value, rather than an exclusive size.  This saves us
>>>>> having to special-case 64-bit overflow, and simplifies our caller too.
>>>>>
>>>>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>>>>> ---
>>>>>    drivers/acpi/arm64/dma.c  |  9 +++------
>>>>>    drivers/acpi/arm64/iort.c | 18 ++++++++----------
>>>>>    include/linux/acpi_iort.h |  4 ++--
>>>>>    3 files changed, 13 insertions(+), 18 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
>>>>> index 6496ff5a6ba2..eb64d8e17dd1 100644
>>>>> --- a/drivers/acpi/arm64/iort.c
>>>>> +++ b/drivers/acpi/arm64/iort.c
>>>>> @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
>>>>>    { return -ENODEV; }
>>>>>    #endif
>>>>> -static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>> +static int nc_dma_get_range(struct device *dev, u64 *limit)
>>>>>    {
>>>>>    	struct acpi_iort_node *node;
>>>>>    	struct acpi_iort_named_component *ncomp;
>>>>> @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
>>>>>    		return -EINVAL;
>>>>>    	}
>>>>> -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
>>>>> -			1ULL<<ncomp->memory_address_limit;
>>>>> +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
>>>>
>>>> The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
>>>> to drop that? You mention it in the cover letter, so clearly I'm missing
>>>> something!
>>>
>>> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), thus
>>> subtracting 1 results in the correct all-bits-set value for an inclusive
>>> 64-bit limit.
>>
>> Oh, I'd have thought you'd have gotten one of those "left shift count >=
>> width of type" warnings if you did that.
> 
> I think you'll get a UBSAN splat, but here the compiler doesn't know what
> 'ncomp->memory_address_limit' will be and so doesn't produce a compile-time
> warning.
> 
> Regardless, it's undefined behaviour.

Urgh, you're right... I double-checked 6.5.7.4 in the standard but 
managed to miss 6.5.7.3. So yeah, even though "4 << 62" or "2 << 63" are 
well-defined here, "1  << 64" isn't, dang. Thanks, funky old ISAs which 
did weird things for crazy large shifts and have no relevance to this 
code :(

Cheers,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Mark Rutland]

On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), 

I'm pretty sure that regardless of whether a type is signed, shifting more than
the type's width is undefined behaviour. That causes GCC to scream at compile
time:

|   CC      arch/arm64/kernel/setup.o
| arch/arm64/kernel/setup.c: In function 'shift_test':
| arch/arm64/kernel/setup.c:295:20: warning: left shift count >= width of type [-Wshift-count-overflow]
|   295 |         return 1UL << 64;
|       |                    ^~

... and a UBSAN splat:

| ================================================================================
| UBSAN: shift-out-of-bounds in arch/arm64/kernel/setup.c:295:13
| shift exponent 64 is too large for 64-bit type 'long unsigned int'
| CPU: 0 PID: 0 Comm: swapper Not tainted 6.7.0-rc1-00005-g06034455cb74-dirty #3
| Call trace:
|  dump_backtrace+0x90/0xe8
|  show_stack+0x18/0x24
|  dump_stack_lvl+0x48/0x60
|  dump_stack+0x18/0x24
|  __ubsan_handle_shift_out_of_bounds+0x114/0x244
|  shift_test+0x24/0x34
|  setup_arch+0x238/0x68c
|  start_kernel+0x70/0x610
|  __primary_switched+0xbc/0xc4
| ================================================================================

Mark.

> thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.
> 
> Thanks,
> Robin.
> 
> > >   	return 0;
> > >   }
> > > -static int rc_dma_get_range(struct device *dev, u64 *size)
> > > +static int rc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_root_complex *rc;
> > > @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<rc->memory_address_limit;
> > > +	*limit = (1ULL << rc->memory_address_limit) - 1;
> > 
> > Same thing here.
> > 
> > Will

Re: [PATCH 3/7] ACPI/IORT: Handle memory address size limits as limits

[Mark Rutland]

On Mon, Dec 11, 2023 at 03:01:27PM +0000, Robin Murphy wrote:
> On 2023-12-11 1:27 pm, Will Deacon wrote:
> > On Wed, Nov 29, 2023 at 05:43:00PM +0000, Robin Murphy wrote:
> > > Return the Root Complex/Named Component memory address size limit as an
> > > inclusive limit value, rather than an exclusive size.  This saves us
> > > having to special-case 64-bit overflow, and simplifies our caller too.
> > > 
> > > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > > ---
> > >   drivers/acpi/arm64/dma.c  |  9 +++------
> > >   drivers/acpi/arm64/iort.c | 18 ++++++++----------
> > >   include/linux/acpi_iort.h |  4 ++--
> > >   3 files changed, 13 insertions(+), 18 deletions(-)
> > 
> > [...]
> > 
> > > diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> > > index 6496ff5a6ba2..eb64d8e17dd1 100644
> > > --- a/drivers/acpi/arm64/iort.c
> > > +++ b/drivers/acpi/arm64/iort.c
> > > @@ -1367,7 +1367,7 @@ int iort_iommu_configure_id(struct device *dev, const u32 *input_id)
> > >   { return -ENODEV; }
> > >   #endif
> > > -static int nc_dma_get_range(struct device *dev, u64 *size)
> > > +static int nc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_named_component *ncomp;
> > > @@ -1384,13 +1384,12 @@ static int nc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = ncomp->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<ncomp->memory_address_limit;
> > > +	*limit = (1ULL << ncomp->memory_address_limit) - 1;
> > 
> > The old code handled 'ncomp->memory_address_limit >= 64' -- why is it safe
> > to drop that? You mention it in the cover letter, so clearly I'm missing
> > something!
> 
> Because an unsigned shift by 64 or more generates 0 (modulo 2^64), 

I'm pretty sure that regardless of whether a type is signed, shifting more than
the type's width is undefined behaviour. That causes GCC to scream at compile
time:

|   CC      arch/arm64/kernel/setup.o
| arch/arm64/kernel/setup.c: In function 'shift_test':
| arch/arm64/kernel/setup.c:295:20: warning: left shift count >= width of type [-Wshift-count-overflow]
|   295 |         return 1UL << 64;
|       |                    ^~

... and a UBSAN splat:

| ================================================================================
| UBSAN: shift-out-of-bounds in arch/arm64/kernel/setup.c:295:13
| shift exponent 64 is too large for 64-bit type 'long unsigned int'
| CPU: 0 PID: 0 Comm: swapper Not tainted 6.7.0-rc1-00005-g06034455cb74-dirty #3
| Call trace:
|  dump_backtrace+0x90/0xe8
|  show_stack+0x18/0x24
|  dump_stack_lvl+0x48/0x60
|  dump_stack+0x18/0x24
|  __ubsan_handle_shift_out_of_bounds+0x114/0x244
|  shift_test+0x24/0x34
|  setup_arch+0x238/0x68c
|  start_kernel+0x70/0x610
|  __primary_switched+0xbc/0xc4
| ================================================================================

Mark.

> thus
> subtracting 1 results in the correct all-bits-set value for an inclusive
> 64-bit limit.
> 
> Thanks,
> Robin.
> 
> > >   	return 0;
> > >   }
> > > -static int rc_dma_get_range(struct device *dev, u64 *size)
> > > +static int rc_dma_get_range(struct device *dev, u64 *limit)
> > >   {
> > >   	struct acpi_iort_node *node;
> > >   	struct acpi_iort_root_complex *rc;
> > > @@ -1408,8 +1407,7 @@ static int rc_dma_get_range(struct device *dev, u64 *size)
> > >   		return -EINVAL;
> > >   	}
> > > -	*size = rc->memory_address_limit >= 64 ? U64_MAX :
> > > -			1ULL<<rc->memory_address_limit;
> > > +	*limit = (1ULL << rc->memory_address_limit) - 1;
> > 
> > Same thing here.
> > 
> > Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Robin Murphy]

Several places want to compute the lower and/or upper bounds of a
dma_range_map, so let's factor that out into reusable helpers.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/loongarch/kernel/dma.c |  9 ++-------
 drivers/acpi/arm64/dma.c    |  8 +-------
 drivers/of/device.c         | 11 ++---------
 include/linux/dma-direct.h  | 18 ++++++++++++++++++
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/arch/loongarch/kernel/dma.c b/arch/loongarch/kernel/dma.c
index 7a9c6a9dd2d0..429555fb4e13 100644
--- a/arch/loongarch/kernel/dma.c
+++ b/arch/loongarch/kernel/dma.c
@@ -8,17 +8,12 @@
 void acpi_arch_dma_setup(struct device *dev)
 {
 	int ret;
-	u64 mask, end = 0;
+	u64 mask, end;
 	const struct bus_dma_region *map = NULL;
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
-		const struct bus_dma_region *r = map;
-
-		for (end = 0; r->size; r++) {
-			if (r->dma_start + r->size - 1 > end)
-				end = r->dma_start + r->size - 1;
-		}
+		end = dma_range_map_max(map);
 
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index b98a149f8d50..52b2abf88689 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -28,13 +28,7 @@ void acpi_arch_dma_setup(struct device *dev)
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
-		const struct bus_dma_region *r = map;
-
-		for (end = 0; r->size; r++) {
-			if (r->dma_start + r->size - 1 > end)
-				end = r->dma_start + r->size - 1;
-		}
-
+		end = dma_range_map_max(map);
 		dev->dma_range_map = map;
 	}
 
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 51062a831970..66879edb4a61 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -117,16 +117,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		if (!force_dma)
 			return ret == -ENODEV ? 0 : ret;
 	} else {
-		const struct bus_dma_region *r = map;
-
 		/* Determine the overall bounds of all DMA regions */
-		for (dma_start = ~0; r->size; r++) {
-			/* Take lower and upper limits */
-			if (r->dma_start < dma_start)
-				dma_start = r->dma_start;
-			if (r->dma_start + r->size > end)
-				end = r->dma_start + r->size;
-		}
+		dma_start = dma_range_map_min(map);
+		end = dma_range_map_max(map);
 	}
 
 	/*
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 3eb3589ff43e..b77e3863daab 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -54,6 +54,24 @@ static inline phys_addr_t translate_dma_to_phys(struct device *dev,
 	return (phys_addr_t)-1;
 }
 
+static inline dma_addr_t dma_range_map_min(const struct bus_dma_region *map)
+{
+	dma_addr_t ret = U64_MAX;
+
+	for (; map->size; map++)
+		ret = min(ret, map->dma_start);
+	return ret;
+}
+
+static inline dma_addr_t dma_range_map_max(const struct bus_dma_region *map)
+{
+	dma_addr_t ret = 0;
+
+	for (; map->size; map++)
+		ret = max(ret, map->dma_start + map->size - 1);
+	return ret;
+}
+
 #ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA
 #include <asm/dma-direct.h>
 #ifndef phys_to_dma_unencrypted
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Robin Murphy]

Several places want to compute the lower and/or upper bounds of a
dma_range_map, so let's factor that out into reusable helpers.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/loongarch/kernel/dma.c |  9 ++-------
 drivers/acpi/arm64/dma.c    |  8 +-------
 drivers/of/device.c         | 11 ++---------
 include/linux/dma-direct.h  | 18 ++++++++++++++++++
 4 files changed, 23 insertions(+), 23 deletions(-)

diff --git a/arch/loongarch/kernel/dma.c b/arch/loongarch/kernel/dma.c
index 7a9c6a9dd2d0..429555fb4e13 100644
--- a/arch/loongarch/kernel/dma.c
+++ b/arch/loongarch/kernel/dma.c
@@ -8,17 +8,12 @@
 void acpi_arch_dma_setup(struct device *dev)
 {
 	int ret;
-	u64 mask, end = 0;
+	u64 mask, end;
 	const struct bus_dma_region *map = NULL;
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
-		const struct bus_dma_region *r = map;
-
-		for (end = 0; r->size; r++) {
-			if (r->dma_start + r->size - 1 > end)
-				end = r->dma_start + r->size - 1;
-		}
+		end = dma_range_map_max(map);
 
 		mask = DMA_BIT_MASK(ilog2(end) + 1);
 		dev->bus_dma_limit = end;
diff --git a/drivers/acpi/arm64/dma.c b/drivers/acpi/arm64/dma.c
index b98a149f8d50..52b2abf88689 100644
--- a/drivers/acpi/arm64/dma.c
+++ b/drivers/acpi/arm64/dma.c
@@ -28,13 +28,7 @@ void acpi_arch_dma_setup(struct device *dev)
 
 	ret = acpi_dma_get_range(dev, &map);
 	if (!ret && map) {
-		const struct bus_dma_region *r = map;
-
-		for (end = 0; r->size; r++) {
-			if (r->dma_start + r->size - 1 > end)
-				end = r->dma_start + r->size - 1;
-		}
-
+		end = dma_range_map_max(map);
 		dev->dma_range_map = map;
 	}
 
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 51062a831970..66879edb4a61 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -117,16 +117,9 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 		if (!force_dma)
 			return ret == -ENODEV ? 0 : ret;
 	} else {
-		const struct bus_dma_region *r = map;
-
 		/* Determine the overall bounds of all DMA regions */
-		for (dma_start = ~0; r->size; r++) {
-			/* Take lower and upper limits */
-			if (r->dma_start < dma_start)
-				dma_start = r->dma_start;
-			if (r->dma_start + r->size > end)
-				end = r->dma_start + r->size;
-		}
+		dma_start = dma_range_map_min(map);
+		end = dma_range_map_max(map);
 	}
 
 	/*
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 3eb3589ff43e..b77e3863daab 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -54,6 +54,24 @@ static inline phys_addr_t translate_dma_to_phys(struct device *dev,
 	return (phys_addr_t)-1;
 }
 
+static inline dma_addr_t dma_range_map_min(const struct bus_dma_region *map)
+{
+	dma_addr_t ret = U64_MAX;
+
+	for (; map->size; map++)
+		ret = min(ret, map->dma_start);
+	return ret;
+}
+
+static inline dma_addr_t dma_range_map_max(const struct bus_dma_region *map)
+{
+	dma_addr_t ret = 0;
+
+	for (; map->size; map++)
+		ret = max(ret, map->dma_start + map->size - 1);
+	return ret;
+}
+
 #ifdef CONFIG_ARCH_HAS_PHYS_TO_DMA
 #include <asm/dma-direct.h>
 #ifndef phys_to_dma_unencrypted
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:01PM +0000, Robin Murphy wrote:
> Several places want to compute the lower and/or upper bounds of a
> dma_range_map, so let's factor that out into reusable helpers.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/loongarch/kernel/dma.c |  9 ++-------
>  drivers/acpi/arm64/dma.c    |  8 +-------
>  drivers/of/device.c         | 11 ++---------
>  include/linux/dma-direct.h  | 18 ++++++++++++++++++
>  4 files changed, 23 insertions(+), 23 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:01PM +0000, Robin Murphy wrote:
> Several places want to compute the lower and/or upper bounds of a
> dma_range_map, so let's factor that out into reusable helpers.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/loongarch/kernel/dma.c |  9 ++-------
>  drivers/acpi/arm64/dma.c    |  8 +-------
>  drivers/of/device.c         | 11 ++---------
>  include/linux/dma-direct.h  | 18 ++++++++++++++++++
>  4 files changed, 23 insertions(+), 23 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[kernel test robot]

Hi Robin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/acpi-bus linus/master rafael-pm/devprop v6.7-rc3 next-20231129]
[cannot apply to joro-iommu/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/OF-Simplify-DMA-range-calculations/20231130-024624
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/b6626985d97ddc33a23b4b9fafa881b35001547e.1701268753.git.robin.murphy%40arm.com
patch subject: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds
config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20231130/202311301330.dGAhHx17-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231130/202311301330.dGAhHx17-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311301330.dGAhHx17-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/arm/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/mm.h:7,
                    from include/linux/memblock.h:12,
                    from kernel/dma/mapping.c:8:
   include/linux/dma-direct.h: In function 'dma_range_map_min':
>> include/linux/limits.h:25:25: warning: conversion from 'long long unsigned int' to 'dma_addr_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
      25 | #define U64_MAX         ((u64)~0ULL)
         |                         ^
   include/linux/dma-direct.h:54:26: note: in expansion of macro 'U64_MAX'
      54 |         dma_addr_t ret = U64_MAX;
         |                          ^~~~~~~


vim +25 include/linux/limits.h

3c9d017cc283df Andy Shevchenko 2023-08-04  14  
54d50897d544c8 Masahiro Yamada 2019-03-07  15  #define U8_MAX		((u8)~0U)
54d50897d544c8 Masahiro Yamada 2019-03-07  16  #define S8_MAX		((s8)(U8_MAX >> 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  17  #define S8_MIN		((s8)(-S8_MAX - 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  18  #define U16_MAX		((u16)~0U)
54d50897d544c8 Masahiro Yamada 2019-03-07  19  #define S16_MAX		((s16)(U16_MAX >> 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  20  #define S16_MIN		((s16)(-S16_MAX - 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  21  #define U32_MAX		((u32)~0U)
3f50f132d8400e John Fastabend  2020-03-30  22  #define U32_MIN		((u32)0)
54d50897d544c8 Masahiro Yamada 2019-03-07  23  #define S32_MAX		((s32)(U32_MAX >> 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  24  #define S32_MIN		((s32)(-S32_MAX - 1))
54d50897d544c8 Masahiro Yamada 2019-03-07 @25  #define U64_MAX		((u64)~0ULL)
54d50897d544c8 Masahiro Yamada 2019-03-07  26  #define S64_MAX		((s64)(U64_MAX >> 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  27  #define S64_MIN		((s64)(-S64_MAX - 1))
54d50897d544c8 Masahiro Yamada 2019-03-07  28  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[kernel test robot]

Hi Robin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on rafael-pm/linux-next]
[also build test WARNING on rafael-pm/acpi-bus linus/master rafael-pm/devprop v6.7-rc3 next-20231129]
[cannot apply to joro-iommu/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/OF-Simplify-DMA-range-calculations/20231130-024624
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/b6626985d97ddc33a23b4b9fafa881b35001547e.1701268753.git.robin.murphy%40arm.com
patch subject: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds
config: arm-lpc32xx_defconfig (https://download.01.org/0day-ci/archive/20231130/202311301356.csZAdTTh-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231130/202311301356.csZAdTTh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311301356.csZAdTTh-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from arch/arm/mm/dma-mapping.c:17:
>> include/linux/dma-direct.h:54:19: warning: implicit conversion from 'u64' (aka 'unsigned long long') to 'dma_addr_t' (aka 'unsigned int') changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
      54 |         dma_addr_t ret = U64_MAX;
         |                    ~~~   ^~~~~~~
   include/linux/limits.h:25:19: note: expanded from macro 'U64_MAX'
      25 | #define U64_MAX         ((u64)~0ULL)
         |                          ^~~~~~~~~~
   1 warning generated.


vim +54 include/linux/dma-direct.h

    51	
    52	static inline dma_addr_t dma_range_map_min(const struct bus_dma_region *map)
    53	{
  > 54		dma_addr_t ret = U64_MAX;
    55	
    56		for (; map->size; map++)
    57			ret = min(ret, map->dma_start);
    58		return ret;
    59	}
    60	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Christoph Hellwig]

On Wed, Nov 29, 2023 at 05:43:01PM +0000, Robin Murphy wrote:
> Several places want to compute the lower and/or upper bounds of a
> dma_range_map, so let's factor that out into reusable helpers.

As the build bot pointed out this will need a fix for the initialization
all-Fs for a 32-bit dma_addr_t, e.g. by using (dma_addr_t)-1, but
otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/7] dma-mapping: Add helpers for dma_range_map bounds

[Christoph Hellwig]

On Wed, Nov 29, 2023 at 05:43:01PM +0000, Robin Murphy wrote:
> Several places want to compute the lower and/or upper bounds of a
> dma_range_map, so let's factor that out into reusable helpers.

As the build bot pointed out this will need a fix for the initialization
all-Fs for a 32-bit dma_addr_t, e.g. by using (dma_addr_t)-1, but
otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/7] iommu/dma: Make limit checks self-contained

[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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5dc012220ca9..7745e7e17010 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -659,8 +659,6 @@ 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
@@ -668,10 +666,10 @@ static void iommu_dma_init_options(struct iommu_dma_options *options,
  * 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;
@@ -683,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 */
@@ -1743,7 +1741,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)) {
-		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;
 	}
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 5/7] iommu/dma: Make limit checks self-contained

[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.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 5dc012220ca9..7745e7e17010 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -659,8 +659,6 @@ 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
@@ -668,10 +666,10 @@ static void iommu_dma_init_options(struct iommu_dma_options *options,
  * 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;
@@ -683,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 */
@@ -1743,7 +1741,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)) {
-		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;
 	}
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/7] iommu/dma: Make limit checks self-contained

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:02PM +0000, Robin Murphy wrote:
> 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/dma-iommu.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

When I spent some time noodling on this a few weeks ago I was looking
at putting the dma_range_map_min() effectively as a new reserved
region in the common reserved region code so it naturally flows out to
all the right places.

But this is no worse in that regard than what we have right now:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

>  	/* Check the domain allows at least some access to the device... */
> -	if (domain->geometry.force_aperture) {
> +	if (map) {

Oh, I've been sitting on a patch to delete force_aperture now too..

Jason

Re: [PATCH 5/7] iommu/dma: Make limit checks self-contained

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:02PM +0000, Robin Murphy wrote:
> 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.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/dma-iommu.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)

When I spent some time noodling on this a few weeks ago I was looking
at putting the dma_range_map_min() effectively as a new reserved
region in the common reserved region code so it naturally flows out to
all the right places.

But this is no worse in that regard than what we have right now:

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

>  	/* Check the domain allows at least some access to the device... */
> -	if (domain->geometry.force_aperture) {
> +	if (map) {

Oh, I've been sitting on a patch to delete force_aperture now too..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/7] iommu/dma: Centralise iommu_setup_dma_ops()

[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 instead running it off the back
of iommu_probe_device() itself, as is currently done for x86 and s390
with .probe_finalize bodges. Pull it all into the main flow properly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/mm/dma-mapping.c  |  2 --
 drivers/iommu/amd/iommu.c    |  8 --------
 drivers/iommu/dma-iommu.c    | 17 ++++-------------
 drivers/iommu/dma-iommu.h    |  6 ++++++
 drivers/iommu/intel/iommu.c  |  7 -------
 drivers/iommu/iommu.c        |  2 ++
 drivers/iommu/s390-iommu.c   |  6 ------
 drivers/iommu/virtio-iommu.c | 10 ----------
 include/linux/iommu.h        |  7 -------
 9 files changed, 12 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 3cb101e8cb29..96ff791199e8 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -58,8 +58,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		   ARCH_DMA_MINALIGN, cls);
 
 	dev->dma_coherent = coherent;
-	if (iommu)
-		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
 
 	xen_setup_dma_ops(dev);
 }
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index fcc987f5d4ed..9418808009ba 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1985,13 +1985,6 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 	return iommu_dev;
 }
 
-static void amd_iommu_probe_finalize(struct device *dev)
-{
-	/* Domains are initialized for this device - have a look what we ended up with */
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-}
-
 static void amd_iommu_release_device(struct device *dev)
 {
 	struct amd_iommu *iommu;
@@ -2646,7 +2639,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.domain_alloc_user = amd_iommu_domain_alloc_user,
 	.probe_device = amd_iommu_probe_device,
 	.release_device = amd_iommu_release_device,
-	.probe_finalize = amd_iommu_probe_finalize,
 	.device_group = amd_iommu_device_group,
 	.get_resv_regions = amd_iommu_get_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7745e7e17010..e0ba8714fdbd 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1725,25 +1725,17 @@ static const struct dma_map_ops iommu_dma_ops = {
 	.opt_mapping_size	= iommu_dma_opt_mapping_size,
 };
 
-/*
- * The IOMMU core code allocates the default DMA domain, which the underlying
- * IOMMU driver needs to support via the dma-iommu layer.
- */
-void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
+void iommu_setup_dma_ops(struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 
-	if (!domain)
-		goto out_err;
-
-	/*
-	 * The IOMMU core code allocates the default DMA domain, which the
-	 * underlying IOMMU driver needs to support via the dma-iommu layer.
-	 */
 	if (iommu_is_dma_domain(domain)) {
 		if (iommu_dma_init_domain(domain, dev))
 			goto out_err;
 		dev->dma_ops = &iommu_dma_ops;
+	} else if (dev->dma_ops == &iommu_dma_ops) {
+		/* Clean up if we've switched *from* a DMA domain */
+		dev->dma_ops = NULL;
 	}
 
 	return;
@@ -1751,7 +1743,6 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
 	 pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
 		 dev_name(dev));
 }
-EXPORT_SYMBOL_GPL(iommu_setup_dma_ops);
 
 static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		phys_addr_t msi_addr, struct iommu_domain *domain)
diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
index c829f1f82a99..cf04560531ed 100644
--- a/drivers/iommu/dma-iommu.h
+++ b/drivers/iommu/dma-iommu.h
@@ -9,6 +9,8 @@
 
 #ifdef CONFIG_IOMMU_DMA
 
+void iommu_setup_dma_ops(struct device *dev);
+
 int iommu_get_dma_cookie(struct iommu_domain *domain);
 void iommu_put_dma_cookie(struct iommu_domain *domain);
 
@@ -24,6 +26,10 @@ static inline void iommu_dma_set_pci_32bit_workaround(struct device *dev)
 
 #else /* CONFIG_IOMMU_DMA */
 
+static inline void iommu_setup_dma_ops(struct device *dev)
+{
+}
+
 static inline int iommu_dma_init_fq(struct iommu_domain *domain)
 {
 	return -EINVAL;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3531b956556c..f7347ed41e89 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4480,12 +4480,6 @@ static void intel_iommu_release_device(struct device *dev)
 	set_dma_ops(dev, NULL);
 }
 
-static void intel_iommu_probe_finalize(struct device *dev)
-{
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-}
-
 static void intel_iommu_get_resv_regions(struct device *device,
 					 struct list_head *head)
 {
@@ -4937,7 +4931,6 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.domain_alloc_user	= intel_iommu_domain_alloc_user,
 	.probe_device		= intel_iommu_probe_device,
-	.probe_finalize		= intel_iommu_probe_finalize,
 	.release_device		= intel_iommu_release_device,
 	.get_resv_regions	= intel_iommu_get_resv_regions,
 	.device_group		= intel_iommu_device_group,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 824989874dee..3a0901165b69 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -589,6 +589,8 @@ int iommu_probe_device(struct device *dev)
 	if (ret)
 		return ret;
 
+	iommu_setup_dma_ops(dev);
+
 	ops = dev_iommu_ops(dev);
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 9a5196f523de..d8eaa7ea380b 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -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)
@@ -785,7 +780,6 @@ static const struct iommu_ops s390_iommu_ops = {
 	.capable = s390_iommu_capable,
 	.domain_alloc_paging = s390_domain_alloc_paging,
 	.probe_device = s390_iommu_probe_device,
-	.probe_finalize = s390_iommu_probe_finalize,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
 	.pgsize_bitmap = SZ_4K,
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 9bcffdde6175..5a558456946b 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -998,15 +998,6 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
 	return ERR_PTR(ret);
 }
 
-static void viommu_probe_finalize(struct device *dev)
-{
-#ifndef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-	/* First clear the DMA ops in case we're switching from a DMA domain */
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-#endif
-}
-
 static void viommu_release_device(struct device *dev)
 {
 	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
@@ -1043,7 +1034,6 @@ static struct iommu_ops viommu_ops = {
 	.capable		= viommu_capable,
 	.domain_alloc		= viommu_domain_alloc,
 	.probe_device		= viommu_probe_device,
-	.probe_finalize		= viommu_probe_finalize,
 	.release_device		= viommu_release_device,
 	.device_group		= viommu_device_group,
 	.get_resv_regions	= viommu_get_resv_regions,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20ed3a4fc5e0..3a3bf8afb8ca 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1283,9 +1283,6 @@ static inline void iommu_debugfs_setup(void) {}
 #ifdef CONFIG_IOMMU_DMA
 #include <linux/msi.h>
 
-/* Setup call for arch DMA mapping code */
-void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
-
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
 
 int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr);
@@ -1296,10 +1293,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg);
 struct msi_desc;
 struct msi_msg;
 
-static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
-{
-}
-
 static inline int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 {
 	return -ENODEV;
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 6/7] iommu/dma: Centralise iommu_setup_dma_ops()

[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 instead running it off the back
of iommu_probe_device() itself, as is currently done for x86 and s390
with .probe_finalize bodges. Pull it all into the main flow properly.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/mm/dma-mapping.c  |  2 --
 drivers/iommu/amd/iommu.c    |  8 --------
 drivers/iommu/dma-iommu.c    | 17 ++++-------------
 drivers/iommu/dma-iommu.h    |  6 ++++++
 drivers/iommu/intel/iommu.c  |  7 -------
 drivers/iommu/iommu.c        |  2 ++
 drivers/iommu/s390-iommu.c   |  6 ------
 drivers/iommu/virtio-iommu.c | 10 ----------
 include/linux/iommu.h        |  7 -------
 9 files changed, 12 insertions(+), 53 deletions(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 3cb101e8cb29..96ff791199e8 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -58,8 +58,6 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 		   ARCH_DMA_MINALIGN, cls);
 
 	dev->dma_coherent = coherent;
-	if (iommu)
-		iommu_setup_dma_ops(dev, dma_base, dma_base + size - 1);
 
 	xen_setup_dma_ops(dev);
 }
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index fcc987f5d4ed..9418808009ba 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1985,13 +1985,6 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 	return iommu_dev;
 }
 
-static void amd_iommu_probe_finalize(struct device *dev)
-{
-	/* Domains are initialized for this device - have a look what we ended up with */
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-}
-
 static void amd_iommu_release_device(struct device *dev)
 {
 	struct amd_iommu *iommu;
@@ -2646,7 +2639,6 @@ const struct iommu_ops amd_iommu_ops = {
 	.domain_alloc_user = amd_iommu_domain_alloc_user,
 	.probe_device = amd_iommu_probe_device,
 	.release_device = amd_iommu_release_device,
-	.probe_finalize = amd_iommu_probe_finalize,
 	.device_group = amd_iommu_device_group,
 	.get_resv_regions = amd_iommu_get_resv_regions,
 	.is_attach_deferred = amd_iommu_is_attach_deferred,
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7745e7e17010..e0ba8714fdbd 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1725,25 +1725,17 @@ static const struct dma_map_ops iommu_dma_ops = {
 	.opt_mapping_size	= iommu_dma_opt_mapping_size,
 };
 
-/*
- * The IOMMU core code allocates the default DMA domain, which the underlying
- * IOMMU driver needs to support via the dma-iommu layer.
- */
-void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
+void iommu_setup_dma_ops(struct device *dev)
 {
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
 
-	if (!domain)
-		goto out_err;
-
-	/*
-	 * The IOMMU core code allocates the default DMA domain, which the
-	 * underlying IOMMU driver needs to support via the dma-iommu layer.
-	 */
 	if (iommu_is_dma_domain(domain)) {
 		if (iommu_dma_init_domain(domain, dev))
 			goto out_err;
 		dev->dma_ops = &iommu_dma_ops;
+	} else if (dev->dma_ops == &iommu_dma_ops) {
+		/* Clean up if we've switched *from* a DMA domain */
+		dev->dma_ops = NULL;
 	}
 
 	return;
@@ -1751,7 +1743,6 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
 	 pr_warn("Failed to set up IOMMU for device %s; retaining platform DMA ops\n",
 		 dev_name(dev));
 }
-EXPORT_SYMBOL_GPL(iommu_setup_dma_ops);
 
 static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev,
 		phys_addr_t msi_addr, struct iommu_domain *domain)
diff --git a/drivers/iommu/dma-iommu.h b/drivers/iommu/dma-iommu.h
index c829f1f82a99..cf04560531ed 100644
--- a/drivers/iommu/dma-iommu.h
+++ b/drivers/iommu/dma-iommu.h
@@ -9,6 +9,8 @@
 
 #ifdef CONFIG_IOMMU_DMA
 
+void iommu_setup_dma_ops(struct device *dev);
+
 int iommu_get_dma_cookie(struct iommu_domain *domain);
 void iommu_put_dma_cookie(struct iommu_domain *domain);
 
@@ -24,6 +26,10 @@ static inline void iommu_dma_set_pci_32bit_workaround(struct device *dev)
 
 #else /* CONFIG_IOMMU_DMA */
 
+static inline void iommu_setup_dma_ops(struct device *dev)
+{
+}
+
 static inline int iommu_dma_init_fq(struct iommu_domain *domain)
 {
 	return -EINVAL;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 3531b956556c..f7347ed41e89 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4480,12 +4480,6 @@ static void intel_iommu_release_device(struct device *dev)
 	set_dma_ops(dev, NULL);
 }
 
-static void intel_iommu_probe_finalize(struct device *dev)
-{
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-}
-
 static void intel_iommu_get_resv_regions(struct device *device,
 					 struct list_head *head)
 {
@@ -4937,7 +4931,6 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_alloc		= intel_iommu_domain_alloc,
 	.domain_alloc_user	= intel_iommu_domain_alloc_user,
 	.probe_device		= intel_iommu_probe_device,
-	.probe_finalize		= intel_iommu_probe_finalize,
 	.release_device		= intel_iommu_release_device,
 	.get_resv_regions	= intel_iommu_get_resv_regions,
 	.device_group		= intel_iommu_device_group,
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 824989874dee..3a0901165b69 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -589,6 +589,8 @@ int iommu_probe_device(struct device *dev)
 	if (ret)
 		return ret;
 
+	iommu_setup_dma_ops(dev);
+
 	ops = dev_iommu_ops(dev);
 	if (ops->probe_finalize)
 		ops->probe_finalize(dev);
diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c
index 9a5196f523de..d8eaa7ea380b 100644
--- a/drivers/iommu/s390-iommu.c
+++ b/drivers/iommu/s390-iommu.c
@@ -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)
@@ -785,7 +780,6 @@ static const struct iommu_ops s390_iommu_ops = {
 	.capable = s390_iommu_capable,
 	.domain_alloc_paging = s390_domain_alloc_paging,
 	.probe_device = s390_iommu_probe_device,
-	.probe_finalize = s390_iommu_probe_finalize,
 	.release_device = s390_iommu_release_device,
 	.device_group = generic_device_group,
 	.pgsize_bitmap = SZ_4K,
diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c
index 9bcffdde6175..5a558456946b 100644
--- a/drivers/iommu/virtio-iommu.c
+++ b/drivers/iommu/virtio-iommu.c
@@ -998,15 +998,6 @@ static struct iommu_device *viommu_probe_device(struct device *dev)
 	return ERR_PTR(ret);
 }
 
-static void viommu_probe_finalize(struct device *dev)
-{
-#ifndef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-	/* First clear the DMA ops in case we're switching from a DMA domain */
-	set_dma_ops(dev, NULL);
-	iommu_setup_dma_ops(dev, 0, U64_MAX);
-#endif
-}
-
 static void viommu_release_device(struct device *dev)
 {
 	struct viommu_endpoint *vdev = dev_iommu_priv_get(dev);
@@ -1043,7 +1034,6 @@ static struct iommu_ops viommu_ops = {
 	.capable		= viommu_capable,
 	.domain_alloc		= viommu_domain_alloc,
 	.probe_device		= viommu_probe_device,
-	.probe_finalize		= viommu_probe_finalize,
 	.release_device		= viommu_release_device,
 	.device_group		= viommu_device_group,
 	.get_resv_regions	= viommu_get_resv_regions,
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20ed3a4fc5e0..3a3bf8afb8ca 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1283,9 +1283,6 @@ static inline void iommu_debugfs_setup(void) {}
 #ifdef CONFIG_IOMMU_DMA
 #include <linux/msi.h>
 
-/* Setup call for arch DMA mapping code */
-void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit);
-
 int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base);
 
 int iommu_dma_prepare_msi(struct msi_desc *desc, phys_addr_t msi_addr);
@@ -1296,10 +1293,6 @@ void iommu_dma_compose_msi_msg(struct msi_desc *desc, struct msi_msg *msg);
 struct msi_desc;
 struct msi_msg;
 
-static inline void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
-{
-}
-
 static inline int iommu_get_msi_cookie(struct iommu_domain *domain, dma_addr_t base)
 {
 	return -ENODEV;
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/7] iommu/dma: Centralise iommu_setup_dma_ops()

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:03PM +0000, Robin Murphy wrote:
> 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 instead running it off the back
> of iommu_probe_device() itself, as is currently done for x86 and s390
> with .probe_finalize bodges. Pull it all into the main flow properly.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm64/mm/dma-mapping.c  |  2 --
>  drivers/iommu/amd/iommu.c    |  8 --------
>  drivers/iommu/dma-iommu.c    | 17 ++++-------------
>  drivers/iommu/dma-iommu.h    |  6 ++++++
>  drivers/iommu/intel/iommu.c  |  7 -------
>  drivers/iommu/iommu.c        |  2 ++
>  drivers/iommu/s390-iommu.c   |  6 ------
>  drivers/iommu/virtio-iommu.c | 10 ----------
>  include/linux/iommu.h        |  7 -------
>  9 files changed, 12 insertions(+), 53 deletions(-)

Yes! That probe_finalize() stuff is not nice

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 824989874dee..3a0901165b69 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -589,6 +589,8 @@ int iommu_probe_device(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	iommu_setup_dma_ops(dev);
> +

I'm pretty sure this should be inside the group mutex lock.

The setting of dev->dma_ops should exactly follow the setting of
group->domain, and all transitions, including from the sysfs override
file should update it, right?

Thus to avoid races agsinst concurrent sysfs this should be locked.

I think you can just put this in iommu_setup_default_domain() and it
will take care of all the cases?

Once in iommu_setup_default_domain() it is easy to call it with the
domain argument and it can know the domain type without using
iommu_is_dma_domain() which is one of the very last few places
checking for the DMA domain type.

Jason

Re: [PATCH 6/7] iommu/dma: Centralise iommu_setup_dma_ops()

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:43:03PM +0000, Robin Murphy wrote:
> 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 instead running it off the back
> of iommu_probe_device() itself, as is currently done for x86 and s390
> with .probe_finalize bodges. Pull it all into the main flow properly.
> 
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  arch/arm64/mm/dma-mapping.c  |  2 --
>  drivers/iommu/amd/iommu.c    |  8 --------
>  drivers/iommu/dma-iommu.c    | 17 ++++-------------
>  drivers/iommu/dma-iommu.h    |  6 ++++++
>  drivers/iommu/intel/iommu.c  |  7 -------
>  drivers/iommu/iommu.c        |  2 ++
>  drivers/iommu/s390-iommu.c   |  6 ------
>  drivers/iommu/virtio-iommu.c | 10 ----------
>  include/linux/iommu.h        |  7 -------
>  9 files changed, 12 insertions(+), 53 deletions(-)

Yes! That probe_finalize() stuff is not nice

> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 824989874dee..3a0901165b69 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -589,6 +589,8 @@ int iommu_probe_device(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	iommu_setup_dma_ops(dev);
> +

I'm pretty sure this should be inside the group mutex lock.

The setting of dev->dma_ops should exactly follow the setting of
group->domain, and all transitions, including from the sysfs override
file should update it, right?

Thus to avoid races agsinst concurrent sysfs this should be locked.

I think you can just put this in iommu_setup_default_domain() and it
will take care of all the cases?

Once in iommu_setup_default_domain() it is easy to call it with the
domain argument and it can know the domain type without using
iommu_is_dma_domain() which is one of the very last few places
checking for the DMA domain type.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Robin Murphy]

The dma_base, size and iommu arguments are only used by ARM, and can
now easily be deduced from the device itself, so there's no need to pass
them through the callchain as well.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arc/mm/dma.c               |  3 +--
 arch/arm/mm/dma-mapping-nommu.c |  3 +--
 arch/arm/mm/dma-mapping.c       | 12 ++++++++----
 arch/arm64/mm/dma-mapping.c     |  3 +--
 arch/mips/mm/dma-noncoherent.c  |  3 +--
 arch/riscv/mm/dma-noncoherent.c |  3 +--
 drivers/acpi/scan.c             |  3 +--
 drivers/hv/hv_common.c          |  6 +-----
 drivers/of/device.c             |  4 +---
 include/linux/dma-map-ops.h     |  6 ++----
 10 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/arch/arc/mm/dma.c b/arch/arc/mm/dma.c
index 2a7fbbb83b70..6b85e94f3275 100644
--- a/arch/arc/mm/dma.c
+++ b/arch/arc/mm/dma.c
@@ -90,8 +90,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 /*
  * Plug in direct dma map ops.
  */
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	/*
 	 * IOC hardware snoops all DMA traffic keeping the caches consistent
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index cfd9c933d2f0..97db5397c320 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -33,8 +33,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 	}
 }
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	if (IS_ENABLED(CONFIG_CPU_V7M)) {
 		/*
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 5409225b4abc..70a2131f9b09 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1716,7 +1716,12 @@ static void arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 				    const struct iommu_ops *iommu, bool coherent)
 {
 	struct dma_iommu_mapping *mapping;
+	u64 dma_base = 0, size = SZ_4GB;
 
+	if (dev->dma_range_map) {
+		dma_base = dma_range_map_min(dev->dma_range_map);
+		size = dma_range_map_max(dev->dma_range_map) - dma_base;
+	}
 	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
 	if (IS_ERR(mapping)) {
 		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
@@ -1756,8 +1761,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { }
 
 #endif	/* CONFIG_ARM_DMA_USE_IOMMU */
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	/*
 	 * Due to legacy code that sets the ->dma_coherent flag from a bus
@@ -1776,8 +1780,8 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (dev->dma_ops)
 		return;
 
-	if (iommu)
-		arm_setup_iommu_dma_ops(dev, dma_base, size, iommu, coherent);
+	if (device_iommu_mapped(dev))
+		arm_setup_iommu_dma_ops(dev);
 
 	xen_setup_dma_ops(dev);
 	dev->archdata.dma_ops_setup = true;
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 96ff791199e8..0b320a25a471 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -46,8 +46,7 @@ void arch_teardown_dma_ops(struct device *dev)
 }
 #endif
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	int cls = cache_line_size_of_cpu();
 
diff --git a/arch/mips/mm/dma-noncoherent.c b/arch/mips/mm/dma-noncoherent.c
index 3c4fc97b9f39..ab4f2a75a7d0 100644
--- a/arch/mips/mm/dma-noncoherent.c
+++ b/arch/mips/mm/dma-noncoherent.c
@@ -137,8 +137,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 #endif
 
 #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	dev->dma_coherent = coherent;
 }
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index 4e4e469b8dd6..cb89d7e0ba88 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -128,8 +128,7 @@ void arch_dma_prep_coherent(struct page *page, size_t size)
 	ALT_CMO_OP(FLUSH, flush_addr, size, riscv_cbom_block_size);
 }
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	WARN_TAINT(!coherent && riscv_cbom_block_size > ARCH_DMA_MINALIGN,
 		   TAINT_CPU_OUT_OF_SPEC,
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ee88a727f200..cad171fc31e8 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1640,8 +1640,7 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 	if (PTR_ERR(iommu) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
-	arch_setup_dma_ops(dev, 0, U64_MAX,
-				iommu, attr == DEV_DMA_COHERENT);
+	arch_setup_dma_ops(dev, attr == DEV_DMA_COHERENT);
 
 	return 0;
 }
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 4372f5d146ab..0e2decd1167a 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -484,11 +484,7 @@ EXPORT_SYMBOL_GPL(hv_query_ext_cap);
 
 void hv_setup_dma_ops(struct device *dev, bool coherent)
 {
-	/*
-	 * Hyper-V does not offer a vIOMMU in the guest
-	 * VM, so pass 0/NULL for the IOMMU settings
-	 */
-	arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
+	arch_setup_dma_ops(dev, coherent);
 }
 EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
 
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 66879edb4a61..3394751015d3 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -96,7 +96,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	const struct iommu_ops *iommu;
 	const struct bus_dma_region *map = NULL;
 	struct device_node *bus_np;
-	u64 dma_start = 0;
 	u64 mask, end = 0;
 	bool coherent;
 	int ret;
@@ -118,7 +117,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 			return ret == -ENODEV ? 0 : ret;
 	} else {
 		/* Determine the overall bounds of all DMA regions */
-		dma_start = dma_range_map_min(map);
 		end = dma_range_map_max(map);
 	}
 
@@ -167,7 +165,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_start, end - dma_start + 1, iommu, coherent);
+	arch_setup_dma_ops(dev, coherent);
 
 	if (!iommu)
 		of_dma_set_restricted_buffer(dev, np);
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index a52e508d1869..023f265eae2e 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -426,11 +426,9 @@ bool arch_dma_unmap_sg_direct(struct device *dev, struct scatterlist *sg,
 #endif
 
 #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent);
+void arch_setup_dma_ops(struct device *dev, bool coherent);
 #else
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-		u64 size, const struct iommu_ops *iommu, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 }
 #endif /* CONFIG_ARCH_HAS_SETUP_DMA_OPS */
-- 
2.39.2.101.g768bb238c484.dirty

[PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Robin Murphy]

The dma_base, size and iommu arguments are only used by ARM, and can
now easily be deduced from the device itself, so there's no need to pass
them through the callchain as well.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arc/mm/dma.c               |  3 +--
 arch/arm/mm/dma-mapping-nommu.c |  3 +--
 arch/arm/mm/dma-mapping.c       | 12 ++++++++----
 arch/arm64/mm/dma-mapping.c     |  3 +--
 arch/mips/mm/dma-noncoherent.c  |  3 +--
 arch/riscv/mm/dma-noncoherent.c |  3 +--
 drivers/acpi/scan.c             |  3 +--
 drivers/hv/hv_common.c          |  6 +-----
 drivers/of/device.c             |  4 +---
 include/linux/dma-map-ops.h     |  6 ++----
 10 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/arch/arc/mm/dma.c b/arch/arc/mm/dma.c
index 2a7fbbb83b70..6b85e94f3275 100644
--- a/arch/arc/mm/dma.c
+++ b/arch/arc/mm/dma.c
@@ -90,8 +90,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 /*
  * Plug in direct dma map ops.
  */
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	/*
 	 * IOC hardware snoops all DMA traffic keeping the caches consistent
diff --git a/arch/arm/mm/dma-mapping-nommu.c b/arch/arm/mm/dma-mapping-nommu.c
index cfd9c933d2f0..97db5397c320 100644
--- a/arch/arm/mm/dma-mapping-nommu.c
+++ b/arch/arm/mm/dma-mapping-nommu.c
@@ -33,8 +33,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 	}
 }
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	if (IS_ENABLED(CONFIG_CPU_V7M)) {
 		/*
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 5409225b4abc..70a2131f9b09 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1716,7 +1716,12 @@ static void arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
 				    const struct iommu_ops *iommu, bool coherent)
 {
 	struct dma_iommu_mapping *mapping;
+	u64 dma_base = 0, size = SZ_4GB;
 
+	if (dev->dma_range_map) {
+		dma_base = dma_range_map_min(dev->dma_range_map);
+		size = dma_range_map_max(dev->dma_range_map) - dma_base;
+	}
 	mapping = arm_iommu_create_mapping(dev->bus, dma_base, size);
 	if (IS_ERR(mapping)) {
 		pr_warn("Failed to create %llu-byte IOMMU mapping for device %s\n",
@@ -1756,8 +1761,7 @@ static void arm_teardown_iommu_dma_ops(struct device *dev) { }
 
 #endif	/* CONFIG_ARM_DMA_USE_IOMMU */
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	/*
 	 * Due to legacy code that sets the ->dma_coherent flag from a bus
@@ -1776,8 +1780,8 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (dev->dma_ops)
 		return;
 
-	if (iommu)
-		arm_setup_iommu_dma_ops(dev, dma_base, size, iommu, coherent);
+	if (device_iommu_mapped(dev))
+		arm_setup_iommu_dma_ops(dev);
 
 	xen_setup_dma_ops(dev);
 	dev->archdata.dma_ops_setup = true;
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 96ff791199e8..0b320a25a471 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -46,8 +46,7 @@ void arch_teardown_dma_ops(struct device *dev)
 }
 #endif
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	int cls = cache_line_size_of_cpu();
 
diff --git a/arch/mips/mm/dma-noncoherent.c b/arch/mips/mm/dma-noncoherent.c
index 3c4fc97b9f39..ab4f2a75a7d0 100644
--- a/arch/mips/mm/dma-noncoherent.c
+++ b/arch/mips/mm/dma-noncoherent.c
@@ -137,8 +137,7 @@ void arch_sync_dma_for_cpu(phys_addr_t paddr, size_t size,
 #endif
 
 #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	dev->dma_coherent = coherent;
 }
diff --git a/arch/riscv/mm/dma-noncoherent.c b/arch/riscv/mm/dma-noncoherent.c
index 4e4e469b8dd6..cb89d7e0ba88 100644
--- a/arch/riscv/mm/dma-noncoherent.c
+++ b/arch/riscv/mm/dma-noncoherent.c
@@ -128,8 +128,7 @@ void arch_dma_prep_coherent(struct page *page, size_t size)
 	ALT_CMO_OP(FLUSH, flush_addr, size, riscv_cbom_block_size);
 }
 
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent)
+void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 	WARN_TAINT(!coherent && riscv_cbom_block_size > ARCH_DMA_MINALIGN,
 		   TAINT_CPU_OUT_OF_SPEC,
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index ee88a727f200..cad171fc31e8 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1640,8 +1640,7 @@ int acpi_dma_configure_id(struct device *dev, enum dev_dma_attr attr,
 	if (PTR_ERR(iommu) == -EPROBE_DEFER)
 		return -EPROBE_DEFER;
 
-	arch_setup_dma_ops(dev, 0, U64_MAX,
-				iommu, attr == DEV_DMA_COHERENT);
+	arch_setup_dma_ops(dev, attr == DEV_DMA_COHERENT);
 
 	return 0;
 }
diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c
index 4372f5d146ab..0e2decd1167a 100644
--- a/drivers/hv/hv_common.c
+++ b/drivers/hv/hv_common.c
@@ -484,11 +484,7 @@ EXPORT_SYMBOL_GPL(hv_query_ext_cap);
 
 void hv_setup_dma_ops(struct device *dev, bool coherent)
 {
-	/*
-	 * Hyper-V does not offer a vIOMMU in the guest
-	 * VM, so pass 0/NULL for the IOMMU settings
-	 */
-	arch_setup_dma_ops(dev, 0, 0, NULL, coherent);
+	arch_setup_dma_ops(dev, coherent);
 }
 EXPORT_SYMBOL_GPL(hv_setup_dma_ops);
 
diff --git a/drivers/of/device.c b/drivers/of/device.c
index 66879edb4a61..3394751015d3 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -96,7 +96,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	const struct iommu_ops *iommu;
 	const struct bus_dma_region *map = NULL;
 	struct device_node *bus_np;
-	u64 dma_start = 0;
 	u64 mask, end = 0;
 	bool coherent;
 	int ret;
@@ -118,7 +117,6 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 			return ret == -ENODEV ? 0 : ret;
 	} else {
 		/* Determine the overall bounds of all DMA regions */
-		dma_start = dma_range_map_min(map);
 		end = dma_range_map_max(map);
 	}
 
@@ -167,7 +165,7 @@ int of_dma_configure_id(struct device *dev, struct device_node *np,
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_start, end - dma_start + 1, iommu, coherent);
+	arch_setup_dma_ops(dev, coherent);
 
 	if (!iommu)
 		of_dma_set_restricted_buffer(dev, np);
diff --git a/include/linux/dma-map-ops.h b/include/linux/dma-map-ops.h
index a52e508d1869..023f265eae2e 100644
--- a/include/linux/dma-map-ops.h
+++ b/include/linux/dma-map-ops.h
@@ -426,11 +426,9 @@ bool arch_dma_unmap_sg_direct(struct device *dev, struct scatterlist *sg,
 #endif
 
 #ifdef CONFIG_ARCH_HAS_SETUP_DMA_OPS
-void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-		const struct iommu_ops *iommu, bool coherent);
+void arch_setup_dma_ops(struct device *dev, bool coherent);
 #else
-static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-		u64 size, const struct iommu_ops *iommu, bool coherent)
+static inline void arch_setup_dma_ops(struct device *dev, bool coherent)
 {
 }
 #endif /* CONFIG_ARCH_HAS_SETUP_DMA_OPS */
-- 
2.39.2.101.g768bb238c484.dirty


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[kernel test robot]

Hi Robin,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on linus/master v6.7-rc3 next-20231129]
[cannot apply to joro-iommu/next rafael-pm/acpi-bus rafael-pm/devprop]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Robin-Murphy/OF-Simplify-DMA-range-calculations/20231130-024624
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/590a4a1b7d10fb9bb1c42ca6cd438e98e6cc94a7.1701268753.git.robin.murphy%40arm.com
patch subject: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()
config: arm-aspeed_g5_defconfig (https://download.01.org/0day-ci/archive/20231130/202311301328.28ULuBXT-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231130/202311301328.28ULuBXT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311301328.28ULuBXT-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/thread_info.h:12,
                    from include/asm-generic/preempt.h:5,
                    from ./arch/arm/include/generated/asm/preempt.h:1,
                    from include/linux/preempt.h:79,
                    from include/linux/spinlock.h:56,
                    from include/linux/mmzone.h:8,
                    from include/linux/gfp.h:7,
                    from include/linux/umh.h:4,
                    from include/linux/kmod.h:9,
                    from include/linux/module.h:17,
                    from arch/arm/mm/dma-mapping.c:9:
   include/linux/dma-direct.h: In function 'dma_range_map_min':
   include/linux/limits.h:25:25: warning: conversion from 'long long unsigned int' to 'dma_addr_t' {aka 'unsigned int'} changes value from '18446744073709551615' to '4294967295' [-Woverflow]
      25 | #define U64_MAX         ((u64)~0ULL)
         |                         ^
   include/linux/dma-direct.h:54:26: note: in expansion of macro 'U64_MAX'
      54 |         dma_addr_t ret = U64_MAX;
         |                          ^~~~~~~
   arch/arm/mm/dma-mapping.c: In function 'arch_setup_dma_ops':
>> arch/arm/mm/dma-mapping.c:1784:17: error: too few arguments to function 'arm_setup_iommu_dma_ops'
    1784 |                 arm_setup_iommu_dma_ops(dev);
         |                 ^~~~~~~~~~~~~~~~~~~~~~~
   arch/arm/mm/dma-mapping.c:1755:13: note: declared here
    1755 | static void arm_setup_iommu_dma_ops(struct device *dev, u64 dma_base, u64 size,
         |             ^~~~~~~~~~~~~~~~~~~~~~~


vim +/arm_setup_iommu_dma_ops +1784 arch/arm/mm/dma-mapping.c

  1763	
  1764	void arch_setup_dma_ops(struct device *dev, bool coherent)
  1765	{
  1766		/*
  1767		 * Due to legacy code that sets the ->dma_coherent flag from a bus
  1768		 * notifier we can't just assign coherent to the ->dma_coherent flag
  1769		 * here, but instead have to make sure we only set but never clear it
  1770		 * for now.
  1771		 */
  1772		if (coherent)
  1773			dev->dma_coherent = true;
  1774	
  1775		/*
  1776		 * Don't override the dma_ops if they have already been set. Ideally
  1777		 * this should be the only location where dma_ops are set, remove this
  1778		 * check when all other callers of set_dma_ops will have disappeared.
  1779		 */
  1780		if (dev->dma_ops)
  1781			return;
  1782	
  1783		if (device_iommu_mapped(dev))
> 1784			arm_setup_iommu_dma_ops(dev);
  1785	
  1786		xen_setup_dma_ops(dev);
  1787		dev->archdata.dma_ops_setup = true;
  1788	}
  1789	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Christoph Hellwig]

On Wed, Nov 29, 2023 at 05:43:04PM +0000, Robin Murphy wrote:
> The dma_base, size and iommu arguments are only used by ARM, and can
> now easily be deduced from the device itself, so there's no need to pass
> them through the callchain as well.

This looks even better than the patch form Jason that only removed the
iommu argument:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I wonder if it makes sense to also remove the coherent argument
by setting up dev->dma_coherent in the caller.  That would require
a pretty careful audit as we're doing a few weird things in that
area, though.

Re: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Christoph Hellwig]

On Wed, Nov 29, 2023 at 05:43:04PM +0000, Robin Murphy wrote:
> The dma_base, size and iommu arguments are only used by ARM, and can
> now easily be deduced from the device itself, so there's no need to pass
> them through the callchain as well.

This looks even better than the patch form Jason that only removed the
iommu argument:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I wonder if it makes sense to also remove the coherent argument
by setting up dev->dma_coherent in the caller.  That would require
a pretty careful audit as we're doing a few weird things in that
area, though.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Robin Murphy]

On 04/12/2023 8:44 am, Christoph Hellwig wrote:
> On Wed, Nov 29, 2023 at 05:43:04PM +0000, Robin Murphy wrote:
>> The dma_base, size and iommu arguments are only used by ARM, and can
>> now easily be deduced from the device itself, so there's no need to pass
>> them through the callchain as well.
> 
> This looks even better than the patch form Jason that only removed the
> iommu argument:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder if it makes sense to also remove the coherent argument
> by setting up dev->dma_coherent in the caller.  That would require
> a pretty careful audit as we're doing a few weird things in that
> area, though.

Yeah, it crossed my mind too, but then I remembered we have at least the 
ARM stuff which may have already set a platform-specific value for 
dev->dma_coherent to take precedence over the firmware value, thus still 
needs to differentiate between the two at this point. Leaving the 
established argument in place seems neater IMO than having to go back to 
arch-specific mechanisms for that and any other similar tricks.

Thanks,
Robin.

Re: [PATCH 7/7] dma-mapping: Simplify arch_setup_dma_ops()

[Robin Murphy]

On 04/12/2023 8:44 am, Christoph Hellwig wrote:
> On Wed, Nov 29, 2023 at 05:43:04PM +0000, Robin Murphy wrote:
>> The dma_base, size and iommu arguments are only used by ARM, and can
>> now easily be deduced from the device itself, so there's no need to pass
>> them through the callchain as well.
> 
> This looks even better than the patch form Jason that only removed the
> iommu argument:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder if it makes sense to also remove the coherent argument
> by setting up dev->dma_coherent in the caller.  That would require
> a pretty careful audit as we're doing a few weird things in that
> area, though.

Yeah, it crossed my mind too, but then I remembered we have at least the 
ARM stuff which may have already set a platform-specific value for 
dev->dma_coherent to take precedence over the firmware value, thus still 
needs to differentiate between the two at this point. Leaving the 
established argument in place seems neater IMO than having to go back to 
arch-specific mechanisms for that and any other similar tricks.

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
> Hi all,
> 
> Prompted by Jason's proposal[1], here's a first step towards truly
> unpicking the dma_configure vs. IOMMU mess. As I commented before, we
> have an awful lot of accumulated cruft and technical debt here making
> things more complicated than they need to be, and we already have hacks
> on top of hacks trying to work around it, so polishing those hacks even
> further is really not a desirable direction of travel. And I do know
> they're hacks, because I wrote most of them and still remember enough of
> the context of the time ;)

I quite like this, I was also looking at getting rid of those other
parameters.

I wanted to take smaller steps because it is all pretty hairy.

One thing that still concerns me is if the FW data restricts the valid
IOVA window that really should be reflected into the reserved ranges
and not just dumped into the struct device for use by the DMA API.

Or, perhaps, viof/iommufd should be using the struct device data to
generate some additional reserved ranges?

Either way, I would like to see the dma_iommu and the rest of the
subsystem agree on what the valid IOVA ranges actually are.

Jason

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Jason Gunthorpe]

On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
> Hi all,
> 
> Prompted by Jason's proposal[1], here's a first step towards truly
> unpicking the dma_configure vs. IOMMU mess. As I commented before, we
> have an awful lot of accumulated cruft and technical debt here making
> things more complicated than they need to be, and we already have hacks
> on top of hacks trying to work around it, so polishing those hacks even
> further is really not a desirable direction of travel. And I do know
> they're hacks, because I wrote most of them and still remember enough of
> the context of the time ;)

I quite like this, I was also looking at getting rid of those other
parameters.

I wanted to take smaller steps because it is all pretty hairy.

One thing that still concerns me is if the FW data restricts the valid
IOVA window that really should be reflected into the reserved ranges
and not just dumped into the struct device for use by the DMA API.

Or, perhaps, viof/iommufd should be using the struct device data to
generate some additional reserved ranges?

Either way, I would like to see the dma_iommu and the rest of the
subsystem agree on what the valid IOVA ranges actually are.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Robin Murphy]

On 29/11/2023 8:36 pm, Jason Gunthorpe wrote:
> On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
>> Hi all,
>>
>> Prompted by Jason's proposal[1], here's a first step towards truly
>> unpicking the dma_configure vs. IOMMU mess. As I commented before, we
>> have an awful lot of accumulated cruft and technical debt here making
>> things more complicated than they need to be, and we already have hacks
>> on top of hacks trying to work around it, so polishing those hacks even
>> further is really not a desirable direction of travel. And I do know
>> they're hacks, because I wrote most of them and still remember enough of
>> the context of the time ;)
> 
> I quite like this, I was also looking at getting rid of those other
> parameters.
> 
> I wanted to take smaller steps because it is all pretty hairy.
> 
> One thing that still concerns me is if the FW data restricts the valid
> IOVA window that really should be reflected into the reserved ranges
> and not just dumped into the struct device for use by the DMA API.
> 
> Or, perhaps, viof/iommufd should be using the struct device data to
> generate some additional reserved ranges?
> 
> Either way, I would like to see the dma_iommu and the rest of the
> subsystem agree on what the valid IOVA ranges actually are.

Note that there is some intentional divergence where iommu-dma reserves 
IOVAs matching PCI outbound windows because it knows it wants to avoid 
clashing with potential peer-to-peer addresses and doesn't want to have 
to get into the details of ACS redirect etc., but we don't expose those 
as generic reserved regions because they're firmly a property of the PCI 
host bridge, not of the IOMMU group (and more practically, because we 
did do so briefly and it made QEMU unhappy). I think there may also have 
been some degree of conclusion that it's not the IOMMU API's place to 
get in the way of other domain users trying to do weird P2P stuff if 
they really want to.

Another issue is that the generic dma_range_map strictly represents 
device-specific constraints which may not always be desirable or 
appropriate to apply to a whole group. There wasn't really a conscious 
decision as such, but it kind of works out as why we still only consider 
PCI's bridge->dma_ranges (which comes from the same underlying data), 
since we can at least assume every device behind a bridge accesses 
memory through that bridge and so inherits its restrictions. However I 
don't recall any conscious decision for inbound windows to only be 
considered for DMA domain reservations rather than for proper reserved 
regions - pretty sure that's just a case of that code being added in the 
place where it seemed to fit best at the time (because hey it's more 
host bridge windows and we already have a thing for host bridge windows...)

Thanks,
Robin.

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Robin Murphy]

On 29/11/2023 8:36 pm, Jason Gunthorpe wrote:
> On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
>> Hi all,
>>
>> Prompted by Jason's proposal[1], here's a first step towards truly
>> unpicking the dma_configure vs. IOMMU mess. As I commented before, we
>> have an awful lot of accumulated cruft and technical debt here making
>> things more complicated than they need to be, and we already have hacks
>> on top of hacks trying to work around it, so polishing those hacks even
>> further is really not a desirable direction of travel. And I do know
>> they're hacks, because I wrote most of them and still remember enough of
>> the context of the time ;)
> 
> I quite like this, I was also looking at getting rid of those other
> parameters.
> 
> I wanted to take smaller steps because it is all pretty hairy.
> 
> One thing that still concerns me is if the FW data restricts the valid
> IOVA window that really should be reflected into the reserved ranges
> and not just dumped into the struct device for use by the DMA API.
> 
> Or, perhaps, viof/iommufd should be using the struct device data to
> generate some additional reserved ranges?
> 
> Either way, I would like to see the dma_iommu and the rest of the
> subsystem agree on what the valid IOVA ranges actually are.

Note that there is some intentional divergence where iommu-dma reserves 
IOVAs matching PCI outbound windows because it knows it wants to avoid 
clashing with potential peer-to-peer addresses and doesn't want to have 
to get into the details of ACS redirect etc., but we don't expose those 
as generic reserved regions because they're firmly a property of the PCI 
host bridge, not of the IOMMU group (and more practically, because we 
did do so briefly and it made QEMU unhappy). I think there may also have 
been some degree of conclusion that it's not the IOMMU API's place to 
get in the way of other domain users trying to do weird P2P stuff if 
they really want to.

Another issue is that the generic dma_range_map strictly represents 
device-specific constraints which may not always be desirable or 
appropriate to apply to a whole group. There wasn't really a conscious 
decision as such, but it kind of works out as why we still only consider 
PCI's bridge->dma_ranges (which comes from the same underlying data), 
since we can at least assume every device behind a bridge accesses 
memory through that bridge and so inherits its restrictions. However I 
don't recall any conscious decision for inbound windows to only be 
considered for DMA domain reservations rather than for proper reserved 
regions - pretty sure that's just a case of that code being added in the 
place where it seemed to fit best at the time (because hey it's more 
host bridge windows and we already have a thing for host bridge windows...)

Thanks,
Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Jason Gunthorpe]

On Fri, Dec 01, 2023 at 01:07:36PM +0000, Robin Murphy wrote:
> On 29/11/2023 8:36 pm, Jason Gunthorpe wrote:
> > On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
> > > Hi all,
> > > 
> > > Prompted by Jason's proposal[1], here's a first step towards truly
> > > unpicking the dma_configure vs. IOMMU mess. As I commented before, we
> > > have an awful lot of accumulated cruft and technical debt here making
> > > things more complicated than they need to be, and we already have hacks
> > > on top of hacks trying to work around it, so polishing those hacks even
> > > further is really not a desirable direction of travel. And I do know
> > > they're hacks, because I wrote most of them and still remember enough of
> > > the context of the time ;)
> > 
> > I quite like this, I was also looking at getting rid of those other
> > parameters.
> > 
> > I wanted to take smaller steps because it is all pretty hairy.
> > 
> > One thing that still concerns me is if the FW data restricts the valid
> > IOVA window that really should be reflected into the reserved ranges
> > and not just dumped into the struct device for use by the DMA API.
> > 
> > Or, perhaps, viof/iommufd should be using the struct device data to
> > generate some additional reserved ranges?
> > 
> > Either way, I would like to see the dma_iommu and the rest of the
> > subsystem agree on what the valid IOVA ranges actually are.
> 
> Note that there is some intentional divergence where iommu-dma reserves
> IOVAs matching PCI outbound windows because it knows it wants to avoid
> clashing with potential peer-to-peer addresses and doesn't want to have to
> get into the details of ACS redirect etc., but we don't expose those as
> generic reserved regions because they're firmly a property of the PCI host
> bridge, not of the IOMMU group (and more practically, because we did do so
> briefly and it made QEMU unhappy). I think there may also have been some
> degree of conclusion that it's not the IOMMU API's place to get in the way
> of other domain users trying to do weird P2P stuff if they really want to.

I'm not sure this is the fully correct conclusion - eg if today we
take a NIC device on a non-ACS topology and run DPDK through VFIO it
has a chance of failure because some IOVA simply cannot be used by
DPDK for DMA at all.

qemu and kvm are a different situation that want different things. Eg
it would want to identity map the PCI BAR spaces to the IOVA they are
claiming.

It should still somehow carve out any other IOVA that is unusable due
to guest-invisible ACS and reflect it through FW tables into the VM.

I'm starting to see people build non-ACS systems and want it to work
with VFIO and I'm a little worried we have been too loose here.

> bridge and so inherits its restrictions. However I don't recall any
> conscious decision for inbound windows to only be considered for DMA domain
> reservations rather than for proper reserved regions - pretty sure that's
> just a case of that code being added in the place where it seemed to fit
> best at the time (because hey it's more host bridge windows and we already
> have a thing for host bridge windows...)

Yeah, and I don't think anyone actually cared much..

At least as a step it would be nice if the DMA API only restrictions
can come out as a special type of reserved region. Then the caller
could decide if they want to follow them or not. iommufd could provide
an opt-in API to DPDK that matches DMA API's safe IOVA allocator.

Jason

Re: [PATCH 0/7] dma-mapping: Clean up arch_setup_dma_ops()

[Jason Gunthorpe]

On Fri, Dec 01, 2023 at 01:07:36PM +0000, Robin Murphy wrote:
> On 29/11/2023 8:36 pm, Jason Gunthorpe wrote:
> > On Wed, Nov 29, 2023 at 05:42:57PM +0000, Robin Murphy wrote:
> > > Hi all,
> > > 
> > > Prompted by Jason's proposal[1], here's a first step towards truly
> > > unpicking the dma_configure vs. IOMMU mess. As I commented before, we
> > > have an awful lot of accumulated cruft and technical debt here making
> > > things more complicated than they need to be, and we already have hacks
> > > on top of hacks trying to work around it, so polishing those hacks even
> > > further is really not a desirable direction of travel. And I do know
> > > they're hacks, because I wrote most of them and still remember enough of
> > > the context of the time ;)
> > 
> > I quite like this, I was also looking at getting rid of those other
> > parameters.
> > 
> > I wanted to take smaller steps because it is all pretty hairy.
> > 
> > One thing that still concerns me is if the FW data restricts the valid
> > IOVA window that really should be reflected into the reserved ranges
> > and not just dumped into the struct device for use by the DMA API.
> > 
> > Or, perhaps, viof/iommufd should be using the struct device data to
> > generate some additional reserved ranges?
> > 
> > Either way, I would like to see the dma_iommu and the rest of the
> > subsystem agree on what the valid IOVA ranges actually are.
> 
> Note that there is some intentional divergence where iommu-dma reserves
> IOVAs matching PCI outbound windows because it knows it wants to avoid
> clashing with potential peer-to-peer addresses and doesn't want to have to
> get into the details of ACS redirect etc., but we don't expose those as
> generic reserved regions because they're firmly a property of the PCI host
> bridge, not of the IOMMU group (and more practically, because we did do so
> briefly and it made QEMU unhappy). I think there may also have been some
> degree of conclusion that it's not the IOMMU API's place to get in the way
> of other domain users trying to do weird P2P stuff if they really want to.

I'm not sure this is the fully correct conclusion - eg if today we
take a NIC device on a non-ACS topology and run DPDK through VFIO it
has a chance of failure because some IOVA simply cannot be used by
DPDK for DMA at all.

qemu and kvm are a different situation that want different things. Eg
it would want to identity map the PCI BAR spaces to the IOVA they are
claiming.

It should still somehow carve out any other IOVA that is unusable due
to guest-invisible ACS and reflect it through FW tables into the VM.

I'm starting to see people build non-ACS systems and want it to work
with VFIO and I'm a little worried we have been too loose here.

> bridge and so inherits its restrictions. However I don't recall any
> conscious decision for inbound windows to only be considered for DMA domain
> reservations rather than for proper reserved regions - pretty sure that's
> just a case of that code being added in the place where it seemed to fit
> best at the time (because hey it's more host bridge windows and we already
> have a thing for host bridge windows...)

Yeah, and I don't think anyone actually cared much..

At least as a step it would be nice if the DMA API only restrictions
can come out as a special type of reserved region. Then the caller
could decide if they want to follow them or not. iommufd could provide
an opt-in API to DPDK that matches DMA API's safe IOVA allocator.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel