[PATCH v3 0/5] Fixes for dma-iommu swiotlb bounce buffers

[PATCH v3 0/5] Fixes for dma-iommu swiotlb bounce buffers

[David Stevens]

From: David Stevens <stevensd@chromium.org>

This patch set includes various fixes for dma-iommu's swiotlb bounce
buffers for untrusted devices. There are three fixes for correctness
issues, one performance issue, and one general cleanup.

The min_align_mask issue was found when running fio on an untrusted nvme
device with bs=512. The other issues were found via code inspection, so
I don't have any specific use cases where things were not working, nor
any concrete performance numbers.

v2 -> v3:
 - Add new patch to address min_align_mask bug
 - Set SKIP_CPU_SYNC flag after syncing in map/unmap
 - Properly call arch_sync_dma_for_cpu in iommu_dma_sync_sg_for_cpu

v1 -> v2:
 - Split fixes into dedicated patches
 - Less invasive changes to fix arch_sync when mapping
 - Leave dev_is_untrusted check for strict iommu

David Stevens (5):
  dma-iommu: fix sync_sg with swiotlb
  dma-iommu: fix arch_sync_dma for map
  dma-iommu: add SKIP_CPU_SYNC after syncing
  dma-iommu: Check CONFIG_SWIOTLB more broadly
  dma-iommu: account for min_align_mask

 drivers/iommu/dma-iommu.c | 97 +++++++++++++++++++++------------------
 1 file changed, 53 insertions(+), 44 deletions(-)

-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v3 1/5] dma-iommu: fix sync_sg with swiotlb

[David Stevens]

From: David Stevens <stevensd@chromium.org>

The is_swiotlb_buffer function takes the physical address of the swiotlb
buffer, not the physical address of the original buffer. The sglist
contains the physical addresses of the original buffer, so for the
sync_sg functions to work properly when a bounce buffer might have been
used, we need to use iommu_iova_to_phys to look up the physical address.
This is what sync_single does, so call that function on each sglist
segment.

The previous code mostly worked because swiotlb does the transfer on map
and unmap. However, any callers which use DMA_ATTR_SKIP_CPU_SYNC with
sglists or which call sync_sg would not have had anything copied to the
bounce buffer.

Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/iommu/dma-iommu.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 98ba927aee1a..54e103b989d9 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -813,14 +813,13 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
 	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
 		return;
 
-	for_each_sg(sgl, sg, nelems, i) {
-		if (!dev_is_dma_coherent(dev))
+	if (dev_is_untrusted(dev))
+		for_each_sg(sgl, sg, nelems, i)
+			iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
+						      sg->length, dir);
+	else
+		for_each_sg(sgl, sg, nelems, i)
 			arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
-
-		if (is_swiotlb_buffer(sg_phys(sg)))
-			swiotlb_sync_single_for_cpu(dev, sg_phys(sg),
-						    sg->length, dir);
-	}
 }
 
 static void iommu_dma_sync_sg_for_device(struct device *dev,
@@ -833,14 +832,14 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
 	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
 		return;
 
-	for_each_sg(sgl, sg, nelems, i) {
-		if (is_swiotlb_buffer(sg_phys(sg)))
-			swiotlb_sync_single_for_device(dev, sg_phys(sg),
-						       sg->length, dir);
-
-		if (!dev_is_dma_coherent(dev))
+	if (dev_is_untrusted(dev))
+		for_each_sg(sgl, sg, nelems, i)
+			iommu_dma_sync_single_for_device(dev,
+							 sg_dma_address(sg),
+							 sg->length, dir);
+	else
+		for_each_sg(sgl, sg, nelems, i)
 			arch_sync_dma_for_device(sg_phys(sg), sg->length, dir);
-	}
 }
 
 static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v3 2/5] dma-iommu: fix arch_sync_dma for map

[David Stevens]

From: David Stevens <stevensd@chromium.org>

When calling arch_sync_dma, we need to pass it the memory that's
actually being used for dma. When using swiotlb bounce buffers, this is
the bounce buffer. Move arch_sync_dma into the __iommu_dma_map_swiotlb
helper, so it can use the bounce buffer address if necessary. This also
means it is no longer necessary to call iommu_dma_sync_sg_for_device in
iommu_dma_map_sg for untrusted devices.

Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/iommu/dma-iommu.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 54e103b989d9..4f0cc4a0a61f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -576,6 +576,9 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
 		memset(padding_start, 0, padding_size);
 	}
 
+	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
+		arch_sync_dma_for_device(phys, org_size, dir);
+
 	iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
 		swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
@@ -848,14 +851,9 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 {
 	phys_addr_t phys = page_to_phys(page) + offset;
 	bool coherent = dev_is_dma_coherent(dev);
-	dma_addr_t dma_handle;
 
-	dma_handle = __iommu_dma_map_swiotlb(dev, phys, size, dma_get_mask(dev),
+	return __iommu_dma_map_swiotlb(dev, phys, size, dma_get_mask(dev),
 			coherent, dir, attrs);
-	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
-	    dma_handle != DMA_MAPPING_ERROR)
-		arch_sync_dma_for_device(phys, size, dir);
-	return dma_handle;
 }
 
 static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
@@ -998,12 +996,12 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	    iommu_deferred_attach(dev, domain))
 		return 0;
 
-	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
-		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
-
 	if (dev_is_untrusted(dev))
 		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
 
+	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
+		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
+
 	/*
 	 * Work out how much IOVA space we need, and align the segments to
 	 * IOVA granules for the IOMMU driver to handle. With some clever
-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v3 3/5] dma-iommu: add SKIP_CPU_SYNC after syncing

[David Stevens]

From: David Stevens <stevensd@chromium.org>

After syncing in map/unmap, add the DMA_ATTR_SKIP_CPU_SYNC flag so
anything that uses attrs later on will skip any sync work that has
already been completed. In particular, this skips copying from the
swiotlb twice during unmap.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/iommu/dma-iommu.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4f0cc4a0a61f..be0214b1455c 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -859,8 +859,11 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
 static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
 		size_t size, enum dma_data_direction dir, unsigned long attrs)
 {
-	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
+	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
 		iommu_dma_sync_single_for_cpu(dev, dma_handle, size, dir);
+		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
+	}
+
 	__iommu_dma_unmap_swiotlb(dev, dma_handle, size, dir, attrs);
 }
 
@@ -999,8 +1002,10 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	if (dev_is_untrusted(dev))
 		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
 
-	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
+	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
 		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
+		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
+	}
 
 	/*
 	 * Work out how much IOVA space we need, and align the segments to
@@ -1068,8 +1073,10 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
 	struct scatterlist *tmp;
 	int i;
 
-	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
+	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
 		iommu_dma_sync_sg_for_cpu(dev, sg, nents, dir);
+		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
+	}
 
 	if (dev_is_untrusted(dev)) {
 		iommu_dma_unmap_sg_swiotlb(dev, sg, nents, dir, attrs);
-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v3 4/5] dma-iommu: Check CONFIG_SWIOTLB more broadly

[David Stevens]

From: David Stevens <stevensd@chromium.org>

Introduce a new dev_use_swiotlb function to guard swiotlb code, instead
of overloading dev_is_untrusted. This allows CONFIG_SWIOTLB to be
checked more broadly, so the swiotlb related code can be removed more
aggressively.

Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/iommu/dma-iommu.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index be0214b1455c..89b689bf801f 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -317,6 +317,11 @@ static bool dev_is_untrusted(struct device *dev)
 	return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
 }
 
+static bool dev_use_swiotlb(struct device *dev)
+{
+	return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
+}
+
 /**
  * iommu_dma_init_domain - Initialise a DMA mapping domain
  * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
@@ -553,8 +558,7 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
 	 * If both the physical buffer start address and size are
 	 * page aligned, we don't need to use a bounce page.
 	 */
-	if (IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev) &&
-	    iova_offset(iovad, phys | org_size)) {
+	if (dev_use_swiotlb(dev) && iova_offset(iovad, phys | org_size)) {
 		aligned_size = iova_align(iovad, org_size);
 		phys = swiotlb_tbl_map_single(dev, phys, org_size,
 					      aligned_size, dir, attrs);
@@ -779,7 +783,7 @@ static void iommu_dma_sync_single_for_cpu(struct device *dev,
 {
 	phys_addr_t phys;
 
-	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
+	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
@@ -795,7 +799,7 @@ static void iommu_dma_sync_single_for_device(struct device *dev,
 {
 	phys_addr_t phys;
 
-	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
+	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
 		return;
 
 	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
@@ -813,10 +817,10 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
 	struct scatterlist *sg;
 	int i;
 
-	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
+	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
 		return;
 
-	if (dev_is_untrusted(dev))
+	if (dev_use_swiotlb(dev))
 		for_each_sg(sgl, sg, nelems, i)
 			iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
 						      sg->length, dir);
@@ -832,10 +836,10 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
 	struct scatterlist *sg;
 	int i;
 
-	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
+	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
 		return;
 
-	if (dev_is_untrusted(dev))
+	if (dev_use_swiotlb(dev))
 		for_each_sg(sgl, sg, nelems, i)
 			iommu_dma_sync_single_for_device(dev,
 							 sg_dma_address(sg),
@@ -999,7 +1003,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
 	    iommu_deferred_attach(dev, domain))
 		return 0;
 
-	if (dev_is_untrusted(dev))
+	if (dev_use_swiotlb(dev))
 		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
 
 	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
@@ -1078,7 +1082,7 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
 		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
 	}
 
-	if (dev_is_untrusted(dev)) {
+	if (dev_use_swiotlb(dev)) {
 		iommu_dma_unmap_sg_swiotlb(dev, sg, nents, dir, attrs);
 		return;
 	}
-- 
2.32.0.605.g8dce9f2422-goog

[PATCH v3 5/5] dma-iommu: account for min_align_mask

[David Stevens]

From: David Stevens <stevensd@chromium.org>

For devices which set min_align_mask, swiotlb preserves the offset of
the original physical address within that mask. Since __iommu_dma_map
accounts for non-aligned addresses, passing a non-aligned swiotlb
address with the swiotlb aligned size results in the offset being
accounted for twice in the size passed to iommu_map_atomic. The extra
page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
tht at function unmaps with the correct size. This causes mapping failures
if the iova gets reused, due to collisions in the iommu page tables.

To fix this, pass the original size to __iommu_dma_map, since that
function already handles alignment.

Additionally, when swiotlb returns non-aligned addresses, there is
padding at the start of the bounce buffer that needs to be cleared.

Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
Signed-off-by: David Stevens <stevensd@chromium.org>
---
 drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 89b689bf801f..ffa7e8ef5db4 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -549,9 +549,8 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
 	struct iommu_domain *domain = iommu_get_dma_domain(dev);
 	struct iommu_dma_cookie *cookie = domain->iova_cookie;
 	struct iova_domain *iovad = &cookie->iovad;
-	size_t aligned_size = org_size;
-	void *padding_start;
-	size_t padding_size;
+	void *tlb_start;
+	size_t aligned_size, iova_off, mapping_end_off;
 	dma_addr_t iova;
 
 	/*
@@ -566,24 +565,26 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
 		if (phys == DMA_MAPPING_ERROR)
 			return DMA_MAPPING_ERROR;
 
-		/* Cleanup the padding area. */
-		padding_start = phys_to_virt(phys);
-		padding_size = aligned_size;
+		iova_off = iova_offset(iovad, phys);
+		tlb_start = phys_to_virt(phys - iova_off);
 
+		/* Cleanup the padding area. */
 		if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
 		    (dir == DMA_TO_DEVICE ||
 		     dir == DMA_BIDIRECTIONAL)) {
-			padding_start += org_size;
-			padding_size -= org_size;
+			mapping_end_off = iova_off + org_size;
+			memset(tlb_start, 0, iova_off);
+			memset(tlb_start + mapping_end_off, 0,
+			       aligned_size - mapping_end_off);
+		} else {
+			memset(tlb_start, 0, aligned_size);
 		}
-
-		memset(padding_start, 0, padding_size);
 	}
 
 	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
 		arch_sync_dma_for_device(phys, org_size, dir);
 
-	iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
+	iova = __iommu_dma_map(dev, phys, org_size, prot, dma_mask);
 	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
 		swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
 	return iova;
-- 
2.32.0.605.g8dce9f2422-goog

Re: [PATCH v3 1/5] dma-iommu: fix sync_sg with swiotlb

[Christoph Hellwig]

On Wed, Aug 11, 2021 at 11:42:43AM +0900, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> The is_swiotlb_buffer function takes the physical address of the swiotlb
> buffer, not the physical address of the original buffer. The sglist
> contains the physical addresses of the original buffer, so for the
> sync_sg functions to work properly when a bounce buffer might have been
> used, we need to use iommu_iova_to_phys to look up the physical address.
> This is what sync_single does, so call that function on each sglist
> segment.
> 
> The previous code mostly worked because swiotlb does the transfer on map
> and unmap. However, any callers which use DMA_ATTR_SKIP_CPU_SYNC with
> sglists or which call sync_sg would not have had anything copied to the
> bounce buffer.
> 
> Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  drivers/iommu/dma-iommu.c | 27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 98ba927aee1a..54e103b989d9 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -813,14 +813,13 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
>  	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
>  		return;
>  
> +	if (dev_is_untrusted(dev))
> +		for_each_sg(sgl, sg, nelems, i)
> +			iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
> +						      sg->length, dir);
> +	else
> +		for_each_sg(sgl, sg, nelems, i)
>  			arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
>  }

I'd remove the above check and fold the if (!dev_is_dma_coherent(dev))
into the else line.  Same for iommu_dma_sync_sg_for_device.

Re: [PATCH v3 2/5] dma-iommu: fix arch_sync_dma for map

[Christoph Hellwig]

Looks good,

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

Re: [PATCH v3 3/5] dma-iommu: add SKIP_CPU_SYNC after syncing

[Christoph Hellwig]

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 4f0cc4a0a61f..be0214b1455c 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -859,8 +859,11 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
>  static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>  		size_t size, enum dma_data_direction dir, unsigned long attrs)
>  {
> -	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> +	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
>  		iommu_dma_sync_single_for_cpu(dev, dma_handle, size, dir);
> +		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> +	}
> +
>  	__iommu_dma_unmap_swiotlb(dev, dma_handle, size, dir, attrs);

I don't think this is the correct way to go.  Instead just call the
raw cache sync helper instead of iommu_dma_sync_single_for_cpu, similar
to what dma-direct does.  Same for the map side.

RE: [PATCH v3 5/5] dma-iommu: account for min_align_mask

[Mi, Dapeng1]

> -----Original Message-----
> From: iommu <iommu-bounces@lists.linux-foundation.org> On Behalf Of
> David Stevens
> Sent: Wednesday, August 11, 2021 10:43 AM
> To: Robin Murphy <robin.murphy@arm.com>; Will Deacon <will@kernel.org>
> Cc: linux-kernel@vger.kernel.org; Tom Murphy <murphyt7@tcd.ie>;
> iommu@lists.linux-foundation.org; David Stevens <stevensd@chromium.org>
> Subject: [PATCH v3 5/5] dma-iommu: account for min_align_mask
> 
> From: David Stevens <stevensd@chromium.org>
> 
> For devices which set min_align_mask, swiotlb preserves the offset of the
> original physical address within that mask. Since __iommu_dma_map
> accounts for non-aligned addresses, passing a non-aligned swiotlb address
> with the swiotlb aligned size results in the offset being accounted for twice in
> the size passed to iommu_map_atomic. The extra page exposed to DMA is
> also not cleaned up by __iommu_dma_unmap, since tht at function unmaps
> with the correct size. This causes mapping failures if the iova gets reused,
> due to collisions in the iommu page tables.
> 
> To fix this, pass the original size to __iommu_dma_map, since that function
> already handles alignment.
> 
> Additionally, when swiotlb returns non-aligned addresses, there is padding at
> the start of the bounce buffer that needs to be cleared.
> 
> Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>  drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 89b689bf801f..ffa7e8ef5db4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -549,9 +549,8 @@ static dma_addr_t
> __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>  	struct iommu_domain *domain = iommu_get_dma_domain(dev);
>  	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>  	struct iova_domain *iovad = &cookie->iovad;
> -	size_t aligned_size = org_size;
> -	void *padding_start;
> -	size_t padding_size;
> +	void *tlb_start;
> +	size_t aligned_size, iova_off, mapping_end_off;
>  	dma_addr_t iova;
> 
>  	/*
> @@ -566,24 +565,26 @@ static dma_addr_t
> __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>  		if (phys == DMA_MAPPING_ERROR)
>  			return DMA_MAPPING_ERROR;
> 
> -		/* Cleanup the padding area. */
> -		padding_start = phys_to_virt(phys);
> -		padding_size = aligned_size;
> +		iova_off = iova_offset(iovad, phys);
> +		tlb_start = phys_to_virt(phys - iova_off);
> 
> +		/* Cleanup the padding area. */
>  		if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
>  		    (dir == DMA_TO_DEVICE ||
>  		     dir == DMA_BIDIRECTIONAL)) {
> -			padding_start += org_size;
> -			padding_size -= org_size;
> +			mapping_end_off = iova_off + org_size;
> +			memset(tlb_start, 0, iova_off);
> +			memset(tlb_start + mapping_end_off, 0,
> +			       aligned_size - mapping_end_off);
> +		} else {
> +			memset(tlb_start, 0, aligned_size);
>  		}

Nice fix. It's better move the "cleanup ..." comment into if case which looks more accurate.

Re: [PATCH v3 1/5] dma-iommu: fix sync_sg with swiotlb

[Robin Murphy]

On 2021-08-11 06:57, Christoph Hellwig wrote:
> On Wed, Aug 11, 2021 at 11:42:43AM +0900, David Stevens wrote:
>> From: David Stevens <stevensd@chromium.org>
>>
>> The is_swiotlb_buffer function takes the physical address of the swiotlb
>> buffer, not the physical address of the original buffer. The sglist
>> contains the physical addresses of the original buffer, so for the
>> sync_sg functions to work properly when a bounce buffer might have been
>> used, we need to use iommu_iova_to_phys to look up the physical address.
>> This is what sync_single does, so call that function on each sglist
>> segment.
>>
>> The previous code mostly worked because swiotlb does the transfer on map
>> and unmap. However, any callers which use DMA_ATTR_SKIP_CPU_SYNC with
>> sglists or which call sync_sg would not have had anything copied to the
>> bounce buffer.
>>
>> Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
>> Signed-off-by: David Stevens <stevensd@chromium.org>
>> ---
>>   drivers/iommu/dma-iommu.c | 27 +++++++++++++--------------
>>   1 file changed, 13 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index 98ba927aee1a..54e103b989d9 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -813,14 +813,13 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
>>   	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
>>   		return;
>>   
>> +	if (dev_is_untrusted(dev))
>> +		for_each_sg(sgl, sg, nelems, i)
>> +			iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
>> +						      sg->length, dir);
>> +	else
>> +		for_each_sg(sgl, sg, nelems, i)
>>   			arch_sync_dma_for_cpu(sg_phys(sg), sg->length, dir);
>>   }
> 
> I'd remove the above check and fold the if (!dev_is_dma_coherent(dev))
> into the else line.  Same for iommu_dma_sync_sg_for_device.

+1

With those also cleaned up,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

Re: [PATCH v3 2/5] dma-iommu: fix arch_sync_dma for map

[Robin Murphy]

On 2021-08-11 03:42, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> When calling arch_sync_dma, we need to pass it the memory that's
> actually being used for dma. When using swiotlb bounce buffers, this is
> the bounce buffer. Move arch_sync_dma into the __iommu_dma_map_swiotlb
> helper, so it can use the bounce buffer address if necessary. This also
> means it is no longer necessary to call iommu_dma_sync_sg_for_device in
> iommu_dma_map_sg for untrusted devices.
> 
> Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>   drivers/iommu/dma-iommu.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 54e103b989d9..4f0cc4a0a61f 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -576,6 +576,9 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>   		memset(padding_start, 0, padding_size);
>   	}
>   
> +	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))

Make that an "else if" - otherwise you're just reintroducing the same 
thing that the third hunk is trying to clean up.

> +		arch_sync_dma_for_device(phys, org_size, dir);
> +
>   	iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
>   	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
>   		swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
> @@ -848,14 +851,9 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
>   {
>   	phys_addr_t phys = page_to_phys(page) + offset;
>   	bool coherent = dev_is_dma_coherent(dev);
> -	dma_addr_t dma_handle;
>   
> -	dma_handle = __iommu_dma_map_swiotlb(dev, phys, size, dma_get_mask(dev),
> +	return __iommu_dma_map_swiotlb(dev, phys, size, dma_get_mask(dev),
>   			coherent, dir, attrs);

Just fold __iommu_dma_map_swiotlb() back into here and have 
iommu_dma_map_sg_swiotlb() call iommu_dma_map_page() in the typical 
pattern of dma-direct and others. Apparently the only purpose served by 
that indirection was allowing these bugs to exist...

Robin.

> -	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
> -	    dma_handle != DMA_MAPPING_ERROR)
> -		arch_sync_dma_for_device(phys, size, dir);
> -	return dma_handle;
>   }
>   
>   static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
> @@ -998,12 +996,12 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	    iommu_deferred_attach(dev, domain))
>   		return 0;
>   
> -	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> -		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
> -
>   	if (dev_is_untrusted(dev))
>   		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
>   
> +	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> +		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
> +
>   	/*
>   	 * Work out how much IOVA space we need, and align the segments to
>   	 * IOVA granules for the IOMMU driver to handle. With some clever
> 

Re: [PATCH v3 3/5] dma-iommu: add SKIP_CPU_SYNC after syncing

[Robin Murphy]

On 2021-08-11 03:42, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> After syncing in map/unmap, add the DMA_ATTR_SKIP_CPU_SYNC flag so
> anything that uses attrs later on will skip any sync work that has
> already been completed. In particular, this skips copying from the
> swiotlb twice during unmap.
> 
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>   drivers/iommu/dma-iommu.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 4f0cc4a0a61f..be0214b1455c 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -859,8 +859,11 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
>   static void iommu_dma_unmap_page(struct device *dev, dma_addr_t dma_handle,
>   		size_t size, enum dma_data_direction dir, unsigned long attrs)
>   {
> -	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> +	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
>   		iommu_dma_sync_single_for_cpu(dev, dma_handle, size, dir);
> +		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> +	}
> +
>   	__iommu_dma_unmap_swiotlb(dev, dma_handle, size, dir, attrs);

Again, fold that into here and put the arch sync in an else case to the 
is_swiotlb_buffer() check.

>   }
>   
> @@ -999,8 +1002,10 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	if (dev_is_untrusted(dev))
>   		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
>   
> -	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> +	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
>   		iommu_dma_sync_sg_for_device(dev, sg, nents, dir);
> +		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> +	}

Why? attrs is never referenced again after this.

>   	/*
>   	 * Work out how much IOVA space we need, and align the segments to
> @@ -1068,8 +1073,10 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>   	struct scatterlist *tmp;
>   	int i;
>   
> -	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> +	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
>   		iommu_dma_sync_sg_for_cpu(dev, sg, nents, dir);
> +		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
> +	}

Just move it down so it's out of the SWIOTLB path entirely. Exactly like 
you did in patch #2 for map_sg, conspicuous in the hunk above.

Robin.

>   
>   	if (dev_is_untrusted(dev)) {
>   		iommu_dma_unmap_sg_swiotlb(dev, sg, nents, dir, attrs);
> 

Re: [PATCH v3 4/5] dma-iommu: Check CONFIG_SWIOTLB more broadly

[Robin Murphy]

On 2021-08-11 03:42, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> Introduce a new dev_use_swiotlb function to guard swiotlb code, instead
> of overloading dev_is_untrusted. This allows CONFIG_SWIOTLB to be
> checked more broadly, so the swiotlb related code can be removed more
> aggressively.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>   drivers/iommu/dma-iommu.c | 24 ++++++++++++++----------
>   1 file changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index be0214b1455c..89b689bf801f 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -317,6 +317,11 @@ static bool dev_is_untrusted(struct device *dev)
>   	return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
>   }
>   
> +static bool dev_use_swiotlb(struct device *dev)
> +{
> +	return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
> +}
> +
>   /**
>    * iommu_dma_init_domain - Initialise a DMA mapping domain
>    * @domain: IOMMU domain previously prepared by iommu_get_dma_cookie()
> @@ -553,8 +558,7 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>   	 * If both the physical buffer start address and size are
>   	 * page aligned, we don't need to use a bounce page.
>   	 */
> -	if (IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev) &&
> -	    iova_offset(iovad, phys | org_size)) {
> +	if (dev_use_swiotlb(dev) && iova_offset(iovad, phys | org_size)) {
>   		aligned_size = iova_align(iovad, org_size);
>   		phys = swiotlb_tbl_map_single(dev, phys, org_size,
>   					      aligned_size, dir, attrs);
> @@ -779,7 +783,7 @@ static void iommu_dma_sync_single_for_cpu(struct device *dev,
>   {
>   	phys_addr_t phys;
>   
> -	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
> +	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
>   		return;
>   
>   	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
> @@ -795,7 +799,7 @@ static void iommu_dma_sync_single_for_device(struct device *dev,
>   {
>   	phys_addr_t phys;
>   
> -	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
> +	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
>   		return;
>   
>   	phys = iommu_iova_to_phys(iommu_get_dma_domain(dev), dma_handle);
> @@ -813,10 +817,10 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
>   	struct scatterlist *sg;
>   	int i;
>   
> -	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
> +	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
>   		return;
>   
> -	if (dev_is_untrusted(dev))
> +	if (dev_use_swiotlb(dev))
>   		for_each_sg(sgl, sg, nelems, i)
>   			iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
>   						      sg->length, dir);
> @@ -832,10 +836,10 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
>   	struct scatterlist *sg;
>   	int i;
>   
> -	if (dev_is_dma_coherent(dev) && !dev_is_untrusted(dev))
> +	if (dev_is_dma_coherent(dev) && !dev_use_swiotlb(dev))
>   		return;
>   
> -	if (dev_is_untrusted(dev))
> +	if (dev_use_swiotlb(dev))
>   		for_each_sg(sgl, sg, nelems, i)
>   			iommu_dma_sync_single_for_device(dev,
>   							 sg_dma_address(sg),
> @@ -999,7 +1003,7 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
>   	    iommu_deferred_attach(dev, domain))
>   		return 0;
>   
> -	if (dev_is_untrusted(dev))
> +	if (dev_use_swiotlb(dev))
>   		return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
>   
>   	if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC)) {
> @@ -1078,7 +1082,7 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
>   		attrs |= DMA_ATTR_SKIP_CPU_SYNC;
>   	}
>   
> -	if (dev_is_untrusted(dev)) {
> +	if (dev_use_swiotlb(dev)) {
>   		iommu_dma_unmap_sg_swiotlb(dev, sg, nents, dir, attrs);
>   		return;
>   	}
> 

Re: [PATCH v3 5/5] dma-iommu: account for min_align_mask

[Robin Murphy]

On 2021-08-11 03:42, David Stevens wrote:
> From: David Stevens <stevensd@chromium.org>
> 
> For devices which set min_align_mask, swiotlb preserves the offset of
> the original physical address within that mask. Since __iommu_dma_map
> accounts for non-aligned addresses, passing a non-aligned swiotlb
> address with the swiotlb aligned size results in the offset being
> accounted for twice in the size passed to iommu_map_atomic. The extra
> page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
> tht at function unmaps with the correct size. This causes mapping failures
> if the iova gets reused, due to collisions in the iommu page tables.
> 
> To fix this, pass the original size to __iommu_dma_map, since that
> function already handles alignment.
> 
> Additionally, when swiotlb returns non-aligned addresses, there is
> padding at the start of the bounce buffer that needs to be cleared.
> 
> Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
> Signed-off-by: David Stevens <stevensd@chromium.org>
> ---
>   drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 89b689bf801f..ffa7e8ef5db4 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -549,9 +549,8 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>   	struct iommu_domain *domain = iommu_get_dma_domain(dev);
>   	struct iommu_dma_cookie *cookie = domain->iova_cookie;
>   	struct iova_domain *iovad = &cookie->iovad;
> -	size_t aligned_size = org_size;
> -	void *padding_start;
> -	size_t padding_size;
> +	void *tlb_start;
> +	size_t aligned_size, iova_off, mapping_end_off;
>   	dma_addr_t iova;
>   
>   	/*
> @@ -566,24 +565,26 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>   		if (phys == DMA_MAPPING_ERROR)
>   			return DMA_MAPPING_ERROR;
>   
> -		/* Cleanup the padding area. */
> -		padding_start = phys_to_virt(phys);
> -		padding_size = aligned_size;
> +		iova_off = iova_offset(iovad, phys);
> +		tlb_start = phys_to_virt(phys - iova_off);
>   
> +		/* Cleanup the padding area. */
>   		if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
>   		    (dir == DMA_TO_DEVICE ||
>   		     dir == DMA_BIDIRECTIONAL)) {
> -			padding_start += org_size;
> -			padding_size -= org_size;
> +			mapping_end_off = iova_off + org_size;
> +			memset(tlb_start, 0, iova_off);
> +			memset(tlb_start + mapping_end_off, 0,
> +			       aligned_size - mapping_end_off);
> +		} else {
> +			memset(tlb_start, 0, aligned_size);
>   		}
> -
> -		memset(padding_start, 0, padding_size);
>   	}
>   
>   	if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
>   		arch_sync_dma_for_device(phys, org_size, dir);
>   
> -	iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
> +	iova = __iommu_dma_map(dev, phys, org_size, prot, dma_mask);

This doesn't feel right - what if the IOVA granule was equal to or 
smaller than min_align_mask, wouldn't you potentially end up mapping the 
padding rather than the data?

Robin.

>   	if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
>   		swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
>   	return iova;
> 

Re: [PATCH v3 5/5] dma-iommu: account for min_align_mask

[David Stevens]

On Thu, Aug 12, 2021 at 4:12 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-11 03:42, David Stevens wrote:
> > From: David Stevens <stevensd@chromium.org>
> >
> > For devices which set min_align_mask, swiotlb preserves the offset of
> > the original physical address within that mask. Since __iommu_dma_map
> > accounts for non-aligned addresses, passing a non-aligned swiotlb
> > address with the swiotlb aligned size results in the offset being
> > accounted for twice in the size passed to iommu_map_atomic. The extra
> > page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
> > tht at function unmaps with the correct size. This causes mapping failures
> > if the iova gets reused, due to collisions in the iommu page tables.
> >
> > To fix this, pass the original size to __iommu_dma_map, since that
> > function already handles alignment.
> >
> > Additionally, when swiotlb returns non-aligned addresses, there is
> > padding at the start of the bounce buffer that needs to be cleared.
> >
> > Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> >   drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
> >   1 file changed, 12 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 89b689bf801f..ffa7e8ef5db4 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -549,9 +549,8 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
> >       struct iommu_domain *domain = iommu_get_dma_domain(dev);
> >       struct iommu_dma_cookie *cookie = domain->iova_cookie;
> >       struct iova_domain *iovad = &cookie->iovad;
> > -     size_t aligned_size = org_size;
> > -     void *padding_start;
> > -     size_t padding_size;
> > +     void *tlb_start;
> > +     size_t aligned_size, iova_off, mapping_end_off;
> >       dma_addr_t iova;
> >
> >       /*
> > @@ -566,24 +565,26 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
> >               if (phys == DMA_MAPPING_ERROR)
> >                       return DMA_MAPPING_ERROR;
> >
> > -             /* Cleanup the padding area. */
> > -             padding_start = phys_to_virt(phys);
> > -             padding_size = aligned_size;
> > +             iova_off = iova_offset(iovad, phys);
> > +             tlb_start = phys_to_virt(phys - iova_off);
> >
> > +             /* Cleanup the padding area. */
> >               if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
> >                   (dir == DMA_TO_DEVICE ||
> >                    dir == DMA_BIDIRECTIONAL)) {
> > -                     padding_start += org_size;
> > -                     padding_size -= org_size;
> > +                     mapping_end_off = iova_off + org_size;
> > +                     memset(tlb_start, 0, iova_off);
> > +                     memset(tlb_start + mapping_end_off, 0,
> > +                            aligned_size - mapping_end_off);
> > +             } else {
> > +                     memset(tlb_start, 0, aligned_size);
> >               }
> > -
> > -             memset(padding_start, 0, padding_size);
> >       }
> >
> >       if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
> >               arch_sync_dma_for_device(phys, org_size, dir);
> >
> > -     iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
> > +     iova = __iommu_dma_map(dev, phys, org_size, prot, dma_mask);
>
> This doesn't feel right - what if the IOVA granule was equal to or
> smaller than min_align_mask, wouldn't you potentially end up mapping the
> padding rather than the data?

The phys value returned by swiotlb_tbl_map_single is the address of
the start of the data in the swiotlb buffer, so the range that needs
to be mapped is [phys, phys + org_size). __iommu_dma_map will handle
this the same as it handles a misaligned mapping in the non-swiotlb
case. It might map memory before/after the desired range, but it will
map the entire range and iova will be the mapped address of phys. Is
there something I'm missing there?

That said, considering that memory before phys might be mapped, I
think there is actually still a bug. The buffer allocated by swiotlb
needs to be aligned to the granule size to ensure that preceding
swiotlb slots aren't mapped. The swiotlb does align allocations larger
than a page to PAGE_SIZE, but if IO_TLB_SIZE < IOVA granule <
PAGE_SIZE, then there can be problems. That can't happen if PAGE_SIZE
is 4k, but it can for larger page sizes. I'll add a fix for that to
the next version of this series.

-David

> Robin.
>
> >       if (iova == DMA_MAPPING_ERROR && is_swiotlb_buffer(phys))
> >               swiotlb_tbl_unmap_single(dev, phys, org_size, dir, attrs);
> >       return iova;
> >

Re: [PATCH v3 2/5] dma-iommu: fix arch_sync_dma for map

[David Stevens]

On Thu, Aug 12, 2021 at 3:47 AM Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2021-08-11 03:42, David Stevens wrote:
> > From: David Stevens <stevensd@chromium.org>
> >
> > When calling arch_sync_dma, we need to pass it the memory that's
> > actually being used for dma. When using swiotlb bounce buffers, this is
> > the bounce buffer. Move arch_sync_dma into the __iommu_dma_map_swiotlb
> > helper, so it can use the bounce buffer address if necessary. This also
> > means it is no longer necessary to call iommu_dma_sync_sg_for_device in
> > iommu_dma_map_sg for untrusted devices.
> >
> > Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
> > Signed-off-by: David Stevens <stevensd@chromium.org>
> > ---
> >   drivers/iommu/dma-iommu.c | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 54e103b989d9..4f0cc4a0a61f 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -576,6 +576,9 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
> >               memset(padding_start, 0, padding_size);
> >       }
> >
> > +     if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
>
> Make that an "else if" - otherwise you're just reintroducing the same
> thing that the third hunk is trying to clean up.

swiotlb_tbl_map_single only copies into the swiotlb buffer, it doesn't
do any architectural syncing. So this block is necessary in the
swiotlb case as well, for iommu_dma_map_page to work properly.

The third chunk is a separate optimization, so I'll split it out into
its own patch.

-David

Re: [PATCH v3 5/5] dma-iommu: account for min_align_mask

[Robin Murphy]

On 2021-08-12 02:45, David Stevens wrote:
> On Thu, Aug 12, 2021 at 4:12 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-08-11 03:42, David Stevens wrote:
>>> From: David Stevens <stevensd@chromium.org>
>>>
>>> For devices which set min_align_mask, swiotlb preserves the offset of
>>> the original physical address within that mask. Since __iommu_dma_map
>>> accounts for non-aligned addresses, passing a non-aligned swiotlb
>>> address with the swiotlb aligned size results in the offset being
>>> accounted for twice in the size passed to iommu_map_atomic. The extra
>>> page exposed to DMA is also not cleaned up by __iommu_dma_unmap, since
>>> tht at function unmaps with the correct size. This causes mapping failures
>>> if the iova gets reused, due to collisions in the iommu page tables.
>>>
>>> To fix this, pass the original size to __iommu_dma_map, since that
>>> function already handles alignment.
>>>
>>> Additionally, when swiotlb returns non-aligned addresses, there is
>>> padding at the start of the bounce buffer that needs to be cleared.
>>>
>>> Fixes: 1f221a0d0dbf ("swiotlb: respect min_align_mask")
>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> ---
>>>    drivers/iommu/dma-iommu.c | 23 ++++++++++++-----------
>>>    1 file changed, 12 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 89b689bf801f..ffa7e8ef5db4 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -549,9 +549,8 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>>>        struct iommu_domain *domain = iommu_get_dma_domain(dev);
>>>        struct iommu_dma_cookie *cookie = domain->iova_cookie;
>>>        struct iova_domain *iovad = &cookie->iovad;
>>> -     size_t aligned_size = org_size;
>>> -     void *padding_start;
>>> -     size_t padding_size;
>>> +     void *tlb_start;
>>> +     size_t aligned_size, iova_off, mapping_end_off;
>>>        dma_addr_t iova;
>>>
>>>        /*
>>> @@ -566,24 +565,26 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>>>                if (phys == DMA_MAPPING_ERROR)
>>>                        return DMA_MAPPING_ERROR;
>>>
>>> -             /* Cleanup the padding area. */
>>> -             padding_start = phys_to_virt(phys);
>>> -             padding_size = aligned_size;
>>> +             iova_off = iova_offset(iovad, phys);
>>> +             tlb_start = phys_to_virt(phys - iova_off);
>>>
>>> +             /* Cleanup the padding area. */
>>>                if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC) &&
>>>                    (dir == DMA_TO_DEVICE ||
>>>                     dir == DMA_BIDIRECTIONAL)) {
>>> -                     padding_start += org_size;
>>> -                     padding_size -= org_size;
>>> +                     mapping_end_off = iova_off + org_size;
>>> +                     memset(tlb_start, 0, iova_off);
>>> +                     memset(tlb_start + mapping_end_off, 0,
>>> +                            aligned_size - mapping_end_off);
>>> +             } else {
>>> +                     memset(tlb_start, 0, aligned_size);
>>>                }
>>> -
>>> -             memset(padding_start, 0, padding_size);
>>>        }
>>>
>>>        if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
>>>                arch_sync_dma_for_device(phys, org_size, dir);
>>>
>>> -     iova = __iommu_dma_map(dev, phys, aligned_size, prot, dma_mask);
>>> +     iova = __iommu_dma_map(dev, phys, org_size, prot, dma_mask);
>>
>> This doesn't feel right - what if the IOVA granule was equal to or
>> smaller than min_align_mask, wouldn't you potentially end up mapping the
>> padding rather than the data?
> 
> The phys value returned by swiotlb_tbl_map_single is the address of
> the start of the data in the swiotlb buffer, so the range that needs
> to be mapped is [phys, phys + org_size). __iommu_dma_map will handle
> this the same as it handles a misaligned mapping in the non-swiotlb
> case. It might map memory before/after the desired range, but it will
> map the entire range and iova will be the mapped address of phys. Is
> there something I'm missing there?

No, my bad - I overlooked that phys got rewritten, so that aspect is OK, 
but...

> That said, considering that memory before phys might be mapped, I
> think there is actually still a bug. The buffer allocated by swiotlb
> needs to be aligned to the granule size to ensure that preceding
> swiotlb slots aren't mapped. The swiotlb does align allocations larger
> than a page to PAGE_SIZE, but if IO_TLB_SIZE < IOVA granule <
> PAGE_SIZE, then there can be problems. That can't happen if PAGE_SIZE
> is 4k, but it can for larger page sizes. I'll add a fix for that to
> the next version of this series.

I was mainly thinking that we still need to map aligned_size (where that 
also accounts for min_align_mask) from tlb_start in order to guarantee 
that the buffer ends up at the right offset in IOVA space. I suppose 
technically we could be cleverer about only padding one thing or the 
other depending on the IOVA granule, but I'm not sure it's worth the 
bother of decoupling the IOMMU mapping from the IOVA allocation just for 
this niche case.

Robin.

Re: [PATCH v3 2/5] dma-iommu: fix arch_sync_dma for map

[Robin Murphy]

On 2021-08-12 10:21, David Stevens wrote:
> On Thu, Aug 12, 2021 at 3:47 AM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2021-08-11 03:42, David Stevens wrote:
>>> From: David Stevens <stevensd@chromium.org>
>>>
>>> When calling arch_sync_dma, we need to pass it the memory that's
>>> actually being used for dma. When using swiotlb bounce buffers, this is
>>> the bounce buffer. Move arch_sync_dma into the __iommu_dma_map_swiotlb
>>> helper, so it can use the bounce buffer address if necessary. This also
>>> means it is no longer necessary to call iommu_dma_sync_sg_for_device in
>>> iommu_dma_map_sg for untrusted devices.
>>>
>>> Fixes: 82612d66d51d ("iommu: Allow the dma-iommu api to use bounce buffers")
>>> Signed-off-by: David Stevens <stevensd@chromium.org>
>>> ---
>>>    drivers/iommu/dma-iommu.c | 16 +++++++---------
>>>    1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>>> index 54e103b989d9..4f0cc4a0a61f 100644
>>> --- a/drivers/iommu/dma-iommu.c
>>> +++ b/drivers/iommu/dma-iommu.c
>>> @@ -576,6 +576,9 @@ static dma_addr_t __iommu_dma_map_swiotlb(struct device *dev, phys_addr_t phys,
>>>                memset(padding_start, 0, padding_size);
>>>        }
>>>
>>> +     if (!coherent && !(attrs & DMA_ATTR_SKIP_CPU_SYNC))
>>
>> Make that an "else if" - otherwise you're just reintroducing the same
>> thing that the third hunk is trying to clean up.
> 
> swiotlb_tbl_map_single only copies into the swiotlb buffer, it doesn't
> do any architectural syncing. So this block is necessary in the
> swiotlb case as well, for iommu_dma_map_page to work properly.

Urgh, apologies again - I had a mental short-circuit and was convinced 
that the SWIOTLB call already did its own cache maintenance. I really 
should have learned by now not to review fiddly patches like these while 
tired...
> The third chunk is a separate optimization, so I'll split it out into
> its own patch.

It's still a logical part of this change - cleaning up the potentially 
wrong sync while you make sure the right one happens. Even more so if 
you make the fix the better way by having iommu_dma_map_sg_swiotlb() 
call iommu_dma_map_page().

Robin.