[PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots

Will Deacon posted 2 patches 1 year, 11 months ago
There is a newer version of this series
[PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Posted by Will Deacon 1 year, 11 months ago
Commit bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix"),
which was a fix for commit 0eee5ae10256 ("swiotlb: fix slot alignment
checks"), causes a functional regression with vsock in a virtual machine
using bouncing via a restricted DMA SWIOTLB pool.

When virtio allocates the virtqueues for the vsock device using
dma_alloc_coherent(), the SWIOTLB search fails to take into account the
8KiB buffer size and returns page-unaligned allocations if 'area->index'
was left unaligned by a previous allocation from the buffer:

 # Final address in brackets is the SWIOTLB address returned to the caller
 | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1645-1649/7168 (0x98326800)
 | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1649-1653/7168 (0x98328800)
 | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1653-1657/7168 (0x9832a800)

This ends in tears (typically buffer corruption and/or a hang) because
swiotlb_alloc() blindly returns the 'struct page' corresponding to the
allocation and therefore the first half of the page ends up being
allocated twice.

Fix the problem by treating the allocation alignment separately to any
additional alignment requirements from the device, using the maximum
of the two as the stride to search the buffer slots.

Fixes: bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix")
Fixes: 0eee5ae10256 ("swiotlb: fix slot alignment checks")
Cc: Christoph Hellwig <hch@lst.de>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Cc: Robin Murphy <robin.murphy@arm.com>
Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
Cc: Dexuan Cui <decui@microsoft.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/dma/swiotlb.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index b079a9a8e087..25febb9e670c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
 	unsigned long max_slots = get_max_slots(boundary_mask);
 	unsigned int iotlb_align_mask =
-		dma_get_min_align_mask(dev) | alloc_align_mask;
+		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
 	unsigned int nslots = nr_slots(alloc_size), stride;
 	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
 	unsigned int index, slots_checked, count = 0, i;
@@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	 * allocations.
 	 */
 	if (alloc_size >= PAGE_SIZE)
-		iotlb_align_mask |= ~PAGE_MASK;
-	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
+		alloc_align_mask |= ~PAGE_MASK;
 
 	/*
 	 * For mappings with an alignment requirement don't bother looping to
 	 * unaligned slots once we found an aligned one.
 	 */
-	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
+	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;
 
 	spin_lock_irqsave(&area->lock, flags);
 	if (unlikely(nslots > pool->area_nslabs - area->used))
@@ -1015,15 +1014,18 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	index = area->index;
 
 	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
+		phys_addr_t tlb_addr;
+
 		slot_index = slot_base + index;
+		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
+
+		if (tlb_addr & alloc_align_mask)
+			goto next_slot;
 
 		if (orig_addr &&
-		    (slot_addr(tbl_dma_addr, slot_index) &
-		     iotlb_align_mask) != (orig_addr & iotlb_align_mask)) {
-			index = wrap_area_index(pool, index + 1);
-			slots_checked++;
-			continue;
-		}
+		    (tlb_addr & iotlb_align_mask) !=
+		    (orig_addr & iotlb_align_mask))
+			goto next_slot;
 
 		if (!iommu_is_span_boundary(slot_index, nslots,
 					    nr_slots(tbl_dma_addr),
@@ -1033,6 +1035,10 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 		}
 		index = wrap_area_index(pool, index + stride);
 		slots_checked += stride;
+		continue;
+next_slot:
+		index = wrap_area_index(pool, index + 1);
+		slots_checked++;
 	}
 
 not_found:
-- 
2.43.0.429.g432eaa2c6b-goog
Re: [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Posted by Petr Tesařík 1 year, 11 months ago
On Fri, 26 Jan 2024 15:19:55 +0000
Will Deacon <will@kernel.org> wrote:

> Commit bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix"),
> which was a fix for commit 0eee5ae10256 ("swiotlb: fix slot alignment
> checks"), causes a functional regression with vsock in a virtual machine
> using bouncing via a restricted DMA SWIOTLB pool.
> 
> When virtio allocates the virtqueues for the vsock device using
> dma_alloc_coherent(), the SWIOTLB search fails to take into account the
> 8KiB buffer size and returns page-unaligned allocations if 'area->index'
> was left unaligned by a previous allocation from the buffer:
> 
>  # Final address in brackets is the SWIOTLB address returned to the caller
>  | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1645-1649/7168 (0x98326800)
>  | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1649-1653/7168 (0x98328800)
>  | virtio-pci 0000:00:07.0: orig_addr 0x0 alloc_size 0x2000, iotlb_align_mask 0x800 stride 0x2: got slot 1653-1657/7168 (0x9832a800)
> 
> This ends in tears (typically buffer corruption and/or a hang) because
> swiotlb_alloc() blindly returns the 'struct page' corresponding to the
> allocation and therefore the first half of the page ends up being
> allocated twice.
> 
> Fix the problem by treating the allocation alignment separately to any
> additional alignment requirements from the device, using the maximum
> of the two as the stride to search the buffer slots.
> 
> Fixes: bbb73a103fbb ("swiotlb: fix a braino in the alignment check fix")
> Fixes: 0eee5ae10256 ("swiotlb: fix slot alignment checks")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Cc: Robin Murphy <robin.murphy@arm.com>
> Cc: Petr Tesarik <petr.tesarik1@huawei-partners.com>
> Cc: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>  kernel/dma/swiotlb.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index b079a9a8e087..25febb9e670c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
>  	unsigned long max_slots = get_max_slots(boundary_mask);
>  	unsigned int iotlb_align_mask =
> -		dma_get_min_align_mask(dev) | alloc_align_mask;
> +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);

Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
should be preserved in the bounce buffer address, ignoring the offset
within the TLB slot...

>  	unsigned int nslots = nr_slots(alloc_size), stride;
>  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
>  	unsigned int index, slots_checked, count = 0, i;
> @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  	 * allocations.
>  	 */
>  	if (alloc_size >= PAGE_SIZE)
> -		iotlb_align_mask |= ~PAGE_MASK;
> -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> +		alloc_align_mask |= ~PAGE_MASK;

...and alloc_align_mask specifies the desired TLB slot alignment.

>  
>  	/*
>  	 * For mappings with an alignment requirement don't bother looping to
>  	 * unaligned slots once we found an aligned one.
>  	 */
> -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> +	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;

I'm not quite sure about this one.

And I'm not even sure all combinations make sense!

For example, take these values:

*         TLB_SIZE ==              0x800  (2K)
* alloc_align_mask == 0xffffffffffffc000  (16K alignment, could be page size)
* iotlb_align_mask == 0xffffffffffff0000  (64K alignment)
*        orig_addr == 0x0000000000001234

Only the lowest 16 bits are relevant for the alignment check.
Device alignment requires 0x1000.
Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
Obviously, such allocation must always fail...

>  
>  	spin_lock_irqsave(&area->lock, flags);
>  	if (unlikely(nslots > pool->area_nslabs - area->used))
> @@ -1015,15 +1014,18 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  	index = area->index;
>  
>  	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
> +		phys_addr_t tlb_addr;
> +
>  		slot_index = slot_base + index;
> +		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
> +
> +		if (tlb_addr & alloc_align_mask)
> +			goto next_slot;

Awww, come on. So your code jumps to a label and then inserts an
unconditional continue just before that label? I'm sure we'll find a
cleaner way to convey the loop logic. What about this:

		if ((tlb_addr & alloc_align_mask) != 0 ||
		    (orig_addr && (tlb_addr & io_tlb_align_mask !=
				   orig_addr & iotlb_align_mask))) {
			index = wrap_area_index(pool, index + 1);
			slots_checked++;
			continue;
		}

But yes, this patch looks like it should finally do the right thing.

Petr T

>  
>  		if (orig_addr &&
> -		    (slot_addr(tbl_dma_addr, slot_index) &
> -		     iotlb_align_mask) != (orig_addr & iotlb_align_mask)) {
> -			index = wrap_area_index(pool, index + 1);
> -			slots_checked++;
> -			continue;
> -		}
> +		    (tlb_addr & iotlb_align_mask) !=
> +		    (orig_addr & iotlb_align_mask))
> +			goto next_slot;
>  
>  		if (!iommu_is_span_boundary(slot_index, nslots,
>  					    nr_slots(tbl_dma_addr),
> @@ -1033,6 +1035,10 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  		}
>  		index = wrap_area_index(pool, index + stride);
>  		slots_checked += stride;
> +		continue;
> +next_slot:
> +		index = wrap_area_index(pool, index + 1);
> +		slots_checked++;
>  	}
>  
>  not_found:
Re: [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Posted by Will Deacon 1 year, 10 months ago
On Fri, Jan 26, 2024 at 06:01:27PM +0100, Petr Tesařík wrote:
> On Fri, 26 Jan 2024 15:19:55 +0000
> Will Deacon <will@kernel.org> wrote:
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index b079a9a8e087..25febb9e670c 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> >  	unsigned long max_slots = get_max_slots(boundary_mask);
> >  	unsigned int iotlb_align_mask =
> > -		dma_get_min_align_mask(dev) | alloc_align_mask;
> > +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);
> 
> Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
> should be preserved in the bounce buffer address, ignoring the offset
> within the TLB slot...

Yup, this is basically restoring the old behaviour.

> >  	unsigned int nslots = nr_slots(alloc_size), stride;
> >  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> >  	unsigned int index, slots_checked, count = 0, i;
> > @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >  	 * allocations.
> >  	 */
> >  	if (alloc_size >= PAGE_SIZE)
> > -		iotlb_align_mask |= ~PAGE_MASK;
> > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > +		alloc_align_mask |= ~PAGE_MASK;
> 
> ...and alloc_align_mask specifies the desired TLB slot alignment.

Yes, although actually I'm now wondering whether there's another bug here
in that we don't return naturally aligned buffers for allocations bigger
than a page. I think that was broken in 0eee5ae10256 ("swiotlb: fix slot
alignment checks") because that stopped aligning the initial search index
to the stride (which was in turn previously aligned to the allocation size).

> >  	/*
> >  	 * For mappings with an alignment requirement don't bother looping to
> >  	 * unaligned slots once we found an aligned one.
> >  	 */
> > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > +	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;
> 
> I'm not quite sure about this one.
> 
> And I'm not even sure all combinations make sense!
> 
> For example, take these values:
> 
> *         TLB_SIZE ==              0x800  (2K)
> * alloc_align_mask == 0xffffffffffffc000  (16K alignment, could be page size)
> * iotlb_align_mask == 0xffffffffffff0000  (64K alignment)
> *        orig_addr == 0x0000000000001234
> 
> Only the lowest 16 bits are relevant for the alignment check.
> Device alignment requires 0x1000.
> Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
> Obviously, such allocation must always fail...

Having an iotlb_align_mask with all those upper bits set looks wrong to me.
Is that the same "braino" as bbb73a103fbb?

> >  	spin_lock_irqsave(&area->lock, flags);
> >  	if (unlikely(nslots > pool->area_nslabs - area->used))
> > @@ -1015,15 +1014,18 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >  	index = area->index;
> >  
> >  	for (slots_checked = 0; slots_checked < pool->area_nslabs; ) {
> > +		phys_addr_t tlb_addr;
> > +
> >  		slot_index = slot_base + index;
> > +		tlb_addr = slot_addr(tbl_dma_addr, slot_index);
> > +
> > +		if (tlb_addr & alloc_align_mask)
> > +			goto next_slot;
> 
> Awww, come on. So your code jumps to a label and then inserts an
> unconditional continue just before that label? I'm sure we'll find a
> cleaner way to convey the loop logic. What about this:
> 
> 		if ((tlb_addr & alloc_align_mask) != 0 ||
> 		    (orig_addr && (tlb_addr & io_tlb_align_mask !=
> 				   orig_addr & iotlb_align_mask))) {
> 			index = wrap_area_index(pool, index + 1);
> 			slots_checked++;
> 			continue;
> 		}

I'm hoping I can drop the alloc_align_mask check entirely if I restore
the alignment of the index.

> But yes, this patch looks like it should finally do the right thing.

I don't think we're quite there yet. I'll spin a v2.

Thanks for the review,

Will
Re: [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Posted by Petr Tesařík 1 year, 10 months ago
On Mon, 29 Jan 2024 19:32:50 +0000
Will Deacon <will@kernel.org> wrote:

> On Fri, Jan 26, 2024 at 06:01:27PM +0100, Petr Tesařík wrote:
> > On Fri, 26 Jan 2024 15:19:55 +0000
> > Will Deacon <will@kernel.org> wrote:  
> > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > > index b079a9a8e087..25febb9e670c 100644
> > > --- a/kernel/dma/swiotlb.c
> > > +++ b/kernel/dma/swiotlb.c
> > > @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > >  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> > >  	unsigned long max_slots = get_max_slots(boundary_mask);
> > >  	unsigned int iotlb_align_mask =
> > > -		dma_get_min_align_mask(dev) | alloc_align_mask;
> > > +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);  
> > 
> > Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
> > should be preserved in the bounce buffer address, ignoring the offset
> > within the TLB slot...  
> 
> Yup, this is basically restoring the old behaviour.
> 
> > >  	unsigned int nslots = nr_slots(alloc_size), stride;
> > >  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > >  	unsigned int index, slots_checked, count = 0, i;
> > > @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > >  	 * allocations.
> > >  	 */
> > >  	if (alloc_size >= PAGE_SIZE)
> > > -		iotlb_align_mask |= ~PAGE_MASK;
> > > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > +		alloc_align_mask |= ~PAGE_MASK;  
> > 
> > ...and alloc_align_mask specifies the desired TLB slot alignment.  
> 
> Yes, although actually I'm now wondering whether there's another bug here
> in that we don't return naturally aligned buffers for allocations bigger
> than a page. I think that was broken in 0eee5ae10256 ("swiotlb: fix slot
> alignment checks") because that stopped aligning the initial search index
> to the stride (which was in turn previously aligned to the allocation size).

The question is whether there is any NEED that allocations bigger than
a page are naturally aligned. For my part, I don't see why there should
be, but I might be missing something.

> > >  	/*
> > >  	 * For mappings with an alignment requirement don't bother looping to
> > >  	 * unaligned slots once we found an aligned one.
> > >  	 */
> > > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > > +	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;  
> > 
> > I'm not quite sure about this one.
> > 
> > And I'm not even sure all combinations make sense!
> > 
> > For example, take these values:
> > 
> > *         TLB_SIZE ==              0x800  (2K)
> > * alloc_align_mask == 0xffffffffffffc000  (16K alignment, could be page size)
> > * iotlb_align_mask == 0xffffffffffff0000  (64K alignment)
> > *        orig_addr == 0x0000000000001234
> > 
> > Only the lowest 16 bits are relevant for the alignment check.
> > Device alignment requires 0x1000.
> > Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
> > Obviously, such allocation must always fail...  
> 
> Having an iotlb_align_mask with all those upper bits set looks wrong to me.
> Is that the same "braino" as bbb73a103fbb?

I must always stop and think at least twice before I can be sure
whether a "mask" has the high bits set, or the low bits set...

On an x86, PAGE_SHIFT is 12, PAGE_SIZE is 1UL << PAGE_SHIFT or 0x1000,
PAGE_MASK is ~(PAGE_SIZE-1)) or 0xfffffffffffff000, and there's one
more bitwise negation, so you're right. Both masks above should be
inverted, and using max() to find the stride is correct.

Petr T
Re: [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots
Posted by Will Deacon 1 year, 10 months ago
On Mon, Jan 29, 2024 at 09:40:34PM +0100, Petr Tesařík wrote:
> On Mon, 29 Jan 2024 19:32:50 +0000
> Will Deacon <will@kernel.org> wrote:
> 
> > On Fri, Jan 26, 2024 at 06:01:27PM +0100, Petr Tesařík wrote:
> > > On Fri, 26 Jan 2024 15:19:55 +0000
> > > Will Deacon <will@kernel.org> wrote:  
> > > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > > > index b079a9a8e087..25febb9e670c 100644
> > > > --- a/kernel/dma/swiotlb.c
> > > > +++ b/kernel/dma/swiotlb.c
> > > > @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > > >  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> > > >  	unsigned long max_slots = get_max_slots(boundary_mask);
> > > >  	unsigned int iotlb_align_mask =
> > > > -		dma_get_min_align_mask(dev) | alloc_align_mask;
> > > > +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);  
> > > 
> > > Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
> > > should be preserved in the bounce buffer address, ignoring the offset
> > > within the TLB slot...  
> > 
> > Yup, this is basically restoring the old behaviour.
> > 
> > > >  	unsigned int nslots = nr_slots(alloc_size), stride;
> > > >  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > > >  	unsigned int index, slots_checked, count = 0, i;
> > > > @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > > >  	 * allocations.
> > > >  	 */
> > > >  	if (alloc_size >= PAGE_SIZE)
> > > > -		iotlb_align_mask |= ~PAGE_MASK;
> > > > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > > +		alloc_align_mask |= ~PAGE_MASK;  
> > > 
> > > ...and alloc_align_mask specifies the desired TLB slot alignment.  
> > 
> > Yes, although actually I'm now wondering whether there's another bug here
> > in that we don't return naturally aligned buffers for allocations bigger
> > than a page. I think that was broken in 0eee5ae10256 ("swiotlb: fix slot
> > alignment checks") because that stopped aligning the initial search index
> > to the stride (which was in turn previously aligned to the allocation size).
> 
> The question is whether there is any NEED that allocations bigger than
> a page are naturally aligned. For my part, I don't see why there should
> be, but I might be missing something.

I think some drivers rely on that. As per core-api/dma-api-howto.rst:

  (Using Consistent DMA mappings::dma_alloc_coherent())
  | The CPU virtual address and the DMA address are both guaranteed to
  | be aligned to the smallest PAGE_SIZE order which is greater than or
  | equal to the requested size.

I've certainly written code that relies on it and the swiotlb logic used
to honour that requirement.

> > > >  	/*
> > > >  	 * For mappings with an alignment requirement don't bother looping to
> > > >  	 * unaligned slots once we found an aligned one.
> > > >  	 */
> > > > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > > > +	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;  
> > > 
> > > I'm not quite sure about this one.
> > > 
> > > And I'm not even sure all combinations make sense!
> > > 
> > > For example, take these values:
> > > 
> > > *         TLB_SIZE ==              0x800  (2K)
> > > * alloc_align_mask == 0xffffffffffffc000  (16K alignment, could be page size)
> > > * iotlb_align_mask == 0xffffffffffff0000  (64K alignment)
> > > *        orig_addr == 0x0000000000001234
> > > 
> > > Only the lowest 16 bits are relevant for the alignment check.
> > > Device alignment requires 0x1000.
> > > Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
> > > Obviously, such allocation must always fail...  
> > 
> > Having an iotlb_align_mask with all those upper bits set looks wrong to me.
> > Is that the same "braino" as bbb73a103fbb?
> 
> I must always stop and think at least twice before I can be sure
> whether a "mask" has the high bits set, or the low bits set...
> 
> On an x86, PAGE_SHIFT is 12, PAGE_SIZE is 1UL << PAGE_SHIFT or 0x1000,
> PAGE_MASK is ~(PAGE_SIZE-1)) or 0xfffffffffffff000, and there's one
> more bitwise negation, so you're right. Both masks above should be
> inverted, and using max() to find the stride is correct.

Heh. It's not straightforward, is it?

Will