On 2023-05-18 18:34, Catalin Marinas wrote:
Similarly to the direct DMA, bounce small allocations as they may have
originated from a kmalloc() cache not safe for DMA. Unlike the direct
DMA, iommu_dma_map_sg() cannot call iommu_dma_map_sg_swiotlb() for all
non-coherent devices as this would break some cases where the iova is
expected to be contiguous (dmabuf). Instead, scan the scatterlist for
any small sizes and only go the swiotlb path if any element of the list
needs bouncing (note that iommu_dma_map_page() would still only bounce
those buffers which are not DMA-aligned).
To avoid scanning the scatterlist on the 'sync' operations, introduce a
SG_DMA_BOUNCED flag set during the iommu_dma_map_sg() call (suggested by
Robin Murphy).
Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx>
Cc: Joerg Roedel <joro@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Robin Murphy <robin.murphy@xxxxxxx>
---
drivers/iommu/dma-iommu.c | 25 ++++++++++++++++++++-----
include/linux/scatterlist.h | 25 +++++++++++++++++++++++--
2 files changed, 43 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 7a9f0b0bddbd..ab1c1681c06e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -956,7 +956,7 @@ static void iommu_dma_sync_sg_for_cpu(struct device *dev,
struct scatterlist *sg;
int i;
- if (dev_use_swiotlb(dev))
+ if (dev_use_swiotlb(dev) || sg_is_dma_bounced(sgl))
for_each_sg(sgl, sg, nelems, i)
iommu_dma_sync_single_for_cpu(dev, sg_dma_address(sg),
sg->length, dir);
@@ -972,7 +972,7 @@ static void iommu_dma_sync_sg_for_device(struct device *dev,
struct scatterlist *sg;
int i;
- if (dev_use_swiotlb(dev))
+ if (dev_use_swiotlb(dev) || sg_is_dma_bounced(sgl))
for_each_sg(sgl, sg, nelems, i)
iommu_dma_sync_single_for_device(dev,
sg_dma_address(sg),
@@ -998,7 +998,8 @@ static dma_addr_t iommu_dma_map_page(struct device *dev, struct page *page,
* If both the physical buffer start address and size are
* page aligned, we don't need to use a bounce page.
*/
- if (dev_use_swiotlb(dev) && iova_offset(iovad, phys | size)) {
+ if ((dev_use_swiotlb(dev) && iova_offset(iovad, phys | size)) ||
+ dma_kmalloc_needs_bounce(dev, size, dir)) {
void *padding_start;
size_t padding_size, aligned_size;
@@ -1210,7 +1211,21 @@ static int iommu_dma_map_sg(struct device *dev, struct scatterlist *sg,
goto out;
}
- if (dev_use_swiotlb(dev))
+ /*
+ * If kmalloc() buffers are not DMA-safe for this device and
+ * direction, check the individual lengths in the sg list. If one of
+ * the buffers is deemed unsafe, follow the iommu_dma_map_sg_swiotlb()
+ * path for potential bouncing.
+ */
+ if (!dma_kmalloc_safe(dev, dir)) {
+ for_each_sg(sg, s, nents, i)
+ if (!dma_kmalloc_size_aligned(s->length)) {
Just to remind myself, we're not checking s->offset on the grounds that
if anyone wants to DMA into an unaligned part of a larger allocation
that remains at their own risk, is that right?
Do we care about the (probably theoretical) case where someone might
build a scatterlist for multiple small allocations such that ones which
happen to be adjacent might get combined into a single segment of
apparently "safe" length but still at "unsafe" alignment?
+ sg_dma_mark_bounced(sg);
I'd prefer to have iommu_dma_map_sg_swiotlb() mark the segments, since
that's in charge of the actual bouncing. Then we can fold the alignment
check into dev_use_swiotlb() (with the dev_is_untrusted() condition
taking priority), and sync/unmap can simply rely on sg_is_dma_bounced()
alone.
(ultimately I'd like to merge the two separate paths back together and
handle bouncing per-segment, but that can wait)
Thanks,
Robin.
+ break;
+ }
+ }
+
+ if (dev_use_swiotlb(dev) || sg_is_dma_bounced(sg))
return iommu_dma_map_sg_swiotlb(dev, sg, nents, dir, attrs);
if (!(attrs & DMA_ATTR_SKIP_CPU_SYNC))
@@ -1315,7 +1330,7 @@ static void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg,
struct scatterlist *tmp;
int i;
- if (dev_use_swiotlb(dev)) {
+ if (dev_use_swiotlb(dev) || sg_is_dma_bounced(sg)) {
iommu_dma_unmap_sg_swiotlb(dev, sg, nents, dir, attrs);
return;
}
diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 87aaf8b5cdb4..9306880cae1c 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -248,6 +248,29 @@ static inline void sg_unmark_end(struct scatterlist *sg)
sg->page_link &= ~SG_END;
}
+#define SG_DMA_BUS_ADDRESS (1 << 0)
+#define SG_DMA_BOUNCED (1 << 1)
+
+#ifdef CONFIG_DMA_BOUNCE_UNALIGNED_KMALLOC
+static inline bool sg_is_dma_bounced(struct scatterlist *sg)
+{
+ return sg->dma_flags & SG_DMA_BOUNCED;
+}
+
+static inline void sg_dma_mark_bounced(struct scatterlist *sg)
+{
+ sg->dma_flags |= SG_DMA_BOUNCED;
+}
+#else
+static inline bool sg_is_dma_bounced(struct scatterlist *sg)
+{
+ return false;
+}
+static inline void sg_dma_mark_bounced(struct scatterlist *sg)
+{
+}
+#endif
+
/*
* CONFIG_PCI_P2PDMA depends on CONFIG_64BIT which means there is 4 bytes
* in struct scatterlist (assuming also CONFIG_NEED_SG_DMA_LENGTH is set).
@@ -256,8 +279,6 @@ static inline void sg_unmark_end(struct scatterlist *sg)
*/
#ifdef CONFIG_PCI_P2PDMA
-#define SG_DMA_BUS_ADDRESS (1 << 0)
-
/**
* sg_dma_is_bus address - Return whether a given segment was marked
* as a bus address
