For swiotlb allocations >= PAGE_SIZE, the slab search historically
adjusted the stride to avoid checking unaligned slots. This had the
side-effect of aligning large mapping requests to PAGE_SIZE, but that
was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").

Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
alignment for swiotlb mappings >= PAGE_SIZE.

Reported-by: Michael Kelley <mhklinux@outlook.com>
Signed-off-by: Will Deacon <will@kernel.org>
---
 kernel/dma/swiotlb.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c381a7ed718f..c5851034523f 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	BUG_ON(!nslots);
 	BUG_ON(area_index >= pool->nareas);
 
+	/*
+	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
+	 * page-aligned in the absence of any other alignment requirements.
+	 * 'alloc_align_mask' was later introduced to specify the alignment
+	 * explicitly, however this is passed as zero for streaming mappings
+	 * and so we preserve the old behaviour there in case any drivers are
+	 * relying on it.
+	 */
+	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
+		alloc_align_mask = PAGE_SIZE - 1;
+
 	/*
 	 * Ensure that the allocation is at least slot-aligned and update
 	 * 'iotlb_align_mask' to ignore bits that will be preserved when
@@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
 	 */
 	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
 
-	/*
-	 * For allocations of PAGE_SIZE or larger only look for page aligned
-	 * allocations.
-	 */
-	if (alloc_size >= PAGE_SIZE)
-		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
-
 	spin_lock_irqsave(&area->lock, flags);
 	if (unlikely(nslots > pool->area_nslabs - area->used))
 		goto not_found;
-- 
2.44.0.278.ge034bb2e1d-goog

On Fri,  8 Mar 2024 15:28:29 +0000
Will Deacon <will@kernel.org> wrote:

> For swiotlb allocations >= PAGE_SIZE, the slab search historically
> adjusted the stride to avoid checking unaligned slots. This had the
> side-effect of aligning large mapping requests to PAGE_SIZE, but that
> was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").
> 
> Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
> alignment for swiotlb mappings >= PAGE_SIZE.
> 
> Reported-by: Michael Kelley <mhklinux@outlook.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> 
> ---
>  kernel/dma/swiotlb.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c381a7ed718f..c5851034523f 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  	BUG_ON(!nslots);
>  	BUG_ON(area_index >= pool->nareas);
>  
> +	/*
> +	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
> +	 * page-aligned in the absence of any other alignment requirements.
> +	 * 'alloc_align_mask' was later introduced to specify the alignment
> +	 * explicitly, however this is passed as zero for streaming mappings
> +	 * and so we preserve the old behaviour there in case any drivers are
> +	 * relying on it.
> +	 */
> +	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
> +		alloc_align_mask = PAGE_SIZE - 1;
> +

This could be moved up the call chain to swiotlb_tbl_map_single(), but
since there is already a plan to modify the call signatures for the
next release, let's keep it here.

Reviewed-by: Petr Tesarik <petr.tesarik1@huawei-partners.com>

Petr T

>  	/*
>  	 * Ensure that the allocation is at least slot-aligned and update
>  	 * 'iotlb_align_mask' to ignore bits that will be preserved when
> @@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>  	 */
>  	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
>  
> -	/*
> -	 * For allocations of PAGE_SIZE or larger only look for page aligned
> -	 * allocations.
> -	 */
> -	if (alloc_size >= PAGE_SIZE)
> -		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> -
>  	spin_lock_irqsave(&area->lock, flags);
>  	if (unlikely(nslots > pool->area_nslabs - area->used))
>  		goto not_found;

On 2024-03-08 3:28 pm, Will Deacon wrote:
> For swiotlb allocations >= PAGE_SIZE, the slab search historically
> adjusted the stride to avoid checking unaligned slots. This had the
> side-effect of aligning large mapping requests to PAGE_SIZE, but that
> was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").
> 
> Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
> alignment for swiotlb mappings >= PAGE_SIZE.

This seems clear enough to keep me happy now, thanks! And apologies that 
I managed to confuse even myself in the previous thread...

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

> Reported-by: Michael Kelley <mhklinux@outlook.com>
> Signed-off-by: Will Deacon <will@kernel.org>
> ---
>   kernel/dma/swiotlb.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c381a7ed718f..c5851034523f 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>   	BUG_ON(!nslots);
>   	BUG_ON(area_index >= pool->nareas);
>   
> +	/*
> +	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
> +	 * page-aligned in the absence of any other alignment requirements.
> +	 * 'alloc_align_mask' was later introduced to specify the alignment
> +	 * explicitly, however this is passed as zero for streaming mappings
> +	 * and so we preserve the old behaviour there in case any drivers are
> +	 * relying on it.
> +	 */
> +	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
> +		alloc_align_mask = PAGE_SIZE - 1;
> +
>   	/*
>   	 * Ensure that the allocation is at least slot-aligned and update
>   	 * 'iotlb_align_mask' to ignore bits that will be preserved when
> @@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>   	 */
>   	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
>   
> -	/*
> -	 * For allocations of PAGE_SIZE or larger only look for page aligned
> -	 * allocations.
> -	 */
> -	if (alloc_size >= PAGE_SIZE)
> -		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> -
>   	spin_lock_irqsave(&area->lock, flags);
>   	if (unlikely(nslots > pool->area_nslabs - area->used))
>   		goto not_found;

On Fri, 8 Mar 2024 16:08:01 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2024-03-08 3:28 pm, Will Deacon wrote:
> > For swiotlb allocations >= PAGE_SIZE, the slab search historically
> > adjusted the stride to avoid checking unaligned slots. This had the
> > side-effect of aligning large mapping requests to PAGE_SIZE, but that
> > was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").
> > 
> > Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
> > alignment for swiotlb mappings >= PAGE_SIZE.  
> 
> This seems clear enough to keep me happy now, thanks! And apologies that 
> I managed to confuse even myself in the previous thread...
> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>

I thought we agreed that this stricter alignment is unnecessary:

  https://lore.kernel.org/linux-iommu/20240305140833.GC3659@lst.de/

But if everybody else wants to have it...

Petr T

> > Reported-by: Michael Kelley <mhklinux@outlook.com>
> > Signed-off-by: Will Deacon <will@kernel.org>
> > ---
> >   kernel/dma/swiotlb.c | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > index c381a7ed718f..c5851034523f 100644
> > --- a/kernel/dma/swiotlb.c
> > +++ b/kernel/dma/swiotlb.c
> > @@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >   	BUG_ON(!nslots);
> >   	BUG_ON(area_index >= pool->nareas);
> >   
> > +	/*
> > +	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
> > +	 * page-aligned in the absence of any other alignment requirements.
> > +	 * 'alloc_align_mask' was later introduced to specify the alignment
> > +	 * explicitly, however this is passed as zero for streaming mappings
> > +	 * and so we preserve the old behaviour there in case any drivers are
> > +	 * relying on it.
> > +	 */
> > +	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
> > +		alloc_align_mask = PAGE_SIZE - 1;
> > +
> >   	/*
> >   	 * Ensure that the allocation is at least slot-aligned and update
> >   	 * 'iotlb_align_mask' to ignore bits that will be preserved when
> > @@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >   	 */
> >   	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
> >   
> > -	/*
> > -	 * For allocations of PAGE_SIZE or larger only look for page aligned
> > -	 * allocations.
> > -	 */
> > -	if (alloc_size >= PAGE_SIZE)
> > -		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> > -
> >   	spin_lock_irqsave(&area->lock, flags);
> >   	if (unlikely(nslots > pool->area_nslabs - area->used))
> >   		goto not_found;  
>

On 2024-03-08 4:38 pm, Petr Tesařík wrote:
> On Fri, 8 Mar 2024 16:08:01 +0000
> Robin Murphy <robin.murphy@arm.com> wrote:
> 
>> On 2024-03-08 3:28 pm, Will Deacon wrote:
>>> For swiotlb allocations >= PAGE_SIZE, the slab search historically
>>> adjusted the stride to avoid checking unaligned slots. This had the
>>> side-effect of aligning large mapping requests to PAGE_SIZE, but that
>>> was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").
>>>
>>> Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
>>> alignment for swiotlb mappings >= PAGE_SIZE.
>>
>> This seems clear enough to keep me happy now, thanks! And apologies that
>> I managed to confuse even myself in the previous thread...
>>
>> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
> I thought we agreed that this stricter alignment is unnecessary:
> 
>    https://lore.kernel.org/linux-iommu/20240305140833.GC3659@lst.de/

No, that was about dma_alloc_coherent() again (and TBH I'm not sure we 
should actually relax it anyway, since there definitely are callers who 
rely on size-alignment beyond PAGE_SIZE, however they're typically going 
to be using the common implementations which end up in alloc_pages() or 
CMA and so do offer that, rather than the oddball ones which don't - 
e.g. we're never going to be allocating SMMUv3 Stream Tables out of some 
restricted pool via the emergency swiotlb_alloc() path). If anywhere, 
the place to argue that point would be patch #3 (which as mentioned I'd 
managed to forget about before...)

This one's just about preserving a SWIOTLB-specific behaviour which has 
the practical effect of making SWIOTLB a bit less visible to dma_map_*() 
callers. The impact of keeping this is fairly low, so seems preferable 
to the risk of facing issues 2 or 3 years down the line when someone 
finally upgrades their distro and their data gets eaten because it turns 
out some obscure driver should really have been updated to use 
min_align_mask.

Thanks,
Robin.

> But if everybody else wants to have it...
> 
> Petr T
> 
>>> Reported-by: Michael Kelley <mhklinux@outlook.com>
>>> Signed-off-by: Will Deacon <will@kernel.org>
>>> ---
>>>    kernel/dma/swiotlb.c | 18 +++++++++++-------
>>>    1 file changed, 11 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
>>> index c381a7ed718f..c5851034523f 100644
>>> --- a/kernel/dma/swiotlb.c
>>> +++ b/kernel/dma/swiotlb.c
>>> @@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>>>    	BUG_ON(!nslots);
>>>    	BUG_ON(area_index >= pool->nareas);
>>>    
>>> +	/*
>>> +	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
>>> +	 * page-aligned in the absence of any other alignment requirements.
>>> +	 * 'alloc_align_mask' was later introduced to specify the alignment
>>> +	 * explicitly, however this is passed as zero for streaming mappings
>>> +	 * and so we preserve the old behaviour there in case any drivers are
>>> +	 * relying on it.
>>> +	 */
>>> +	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
>>> +		alloc_align_mask = PAGE_SIZE - 1;
>>> +
>>>    	/*
>>>    	 * Ensure that the allocation is at least slot-aligned and update
>>>    	 * 'iotlb_align_mask' to ignore bits that will be preserved when
>>> @@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
>>>    	 */
>>>    	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
>>>    
>>> -	/*
>>> -	 * For allocations of PAGE_SIZE or larger only look for page aligned
>>> -	 * allocations.
>>> -	 */
>>> -	if (alloc_size >= PAGE_SIZE)
>>> -		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
>>> -
>>>    	spin_lock_irqsave(&area->lock, flags);
>>>    	if (unlikely(nslots > pool->area_nslabs - area->used))
>>>    		goto not_found;
>>
> 

On Fri, 8 Mar 2024 17:17:51 +0000
Robin Murphy <robin.murphy@arm.com> wrote:

> On 2024-03-08 4:38 pm, Petr Tesařík wrote:
> > On Fri, 8 Mar 2024 16:08:01 +0000
> > Robin Murphy <robin.murphy@arm.com> wrote:
> >   
> >> On 2024-03-08 3:28 pm, Will Deacon wrote:  
> >>> For swiotlb allocations >= PAGE_SIZE, the slab search historically
> >>> adjusted the stride to avoid checking unaligned slots. This had the
> >>> side-effect of aligning large mapping requests to PAGE_SIZE, but that
> >>> was broken by 0eee5ae10256 ("swiotlb: fix slot alignment checks").
> >>>
> >>> Since this alignment could be relied upon drivers, reinstate PAGE_SIZE
> >>> alignment for swiotlb mappings >= PAGE_SIZE.  
> >>
> >> This seems clear enough to keep me happy now, thanks! And apologies that
> >> I managed to confuse even myself in the previous thread...
> >>
> >> Reviewed-by: Robin Murphy <robin.murphy@arm.com>  
> > 
> > I thought we agreed that this stricter alignment is unnecessary:
> > 
> >    https://lore.kernel.org/linux-iommu/20240305140833.GC3659@lst.de/  
> 
> No, that was about dma_alloc_coherent() again (and TBH I'm not sure we 
> should actually relax it anyway, since there definitely are callers who 
> rely on size-alignment beyond PAGE_SIZE, however they're typically going 
> to be using the common implementations which end up in alloc_pages() or 
> CMA and so do offer that, rather than the oddball ones which don't - 
> e.g. we're never going to be allocating SMMUv3 Stream Tables out of some 
> restricted pool via the emergency swiotlb_alloc() path). If anywhere, 
> the place to argue that point would be patch #3 (which as mentioned I'd 
> managed to forget about before...)

Sure, swiotlb_alloc() ensures that alloc_align_mask is non-zero, so the
condition in this patch cannot be met. In fact, that's why it could be
moved up to swiotlb_tbl_map_single().

> This one's just about preserving a SWIOTLB-specific behaviour which has 
> the practical effect of making SWIOTLB a bit less visible to dma_map_*() 
> callers. The impact of keeping this is fairly low, so seems preferable 
> to the risk of facing issues 2 or 3 years down the line when someone 
> finally upgrades their distro and their data gets eaten because it turns 
> out some obscure driver should really have been updated to use 
> min_align_mask.

The impact is indeed negligible with 4K pages. It may put a bit of
stress on the SWIOTLB with 64K pages, but if the mapping size somehow
correlates with page size, such drivers would need a larger SWIOTLB
anyway.

I had some doubts if there are any guarantees at all for dma_map_*().
Now I see that the email I linked dealt with dma_alloc_coherent().

OK, let's not open the other discussion now.

Petr T

> Thanks,
> Robin.
> 
> > But if everybody else wants to have it...
> > 
> > Petr T
> >   
> >>> Reported-by: Michael Kelley <mhklinux@outlook.com>
> >>> Signed-off-by: Will Deacon <will@kernel.org>
> >>> ---
> >>>    kernel/dma/swiotlb.c | 18 +++++++++++-------
> >>>    1 file changed, 11 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> >>> index c381a7ed718f..c5851034523f 100644
> >>> --- a/kernel/dma/swiotlb.c
> >>> +++ b/kernel/dma/swiotlb.c
> >>> @@ -992,6 +992,17 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >>>    	BUG_ON(!nslots);
> >>>    	BUG_ON(area_index >= pool->nareas);
> >>>    
> >>> +	/*
> >>> +	 * Historically, swiotlb allocations >= PAGE_SIZE were guaranteed to be
> >>> +	 * page-aligned in the absence of any other alignment requirements.
> >>> +	 * 'alloc_align_mask' was later introduced to specify the alignment
> >>> +	 * explicitly, however this is passed as zero for streaming mappings
> >>> +	 * and so we preserve the old behaviour there in case any drivers are
> >>> +	 * relying on it.
> >>> +	 */
> >>> +	if (!alloc_align_mask && !iotlb_align_mask && alloc_size >= PAGE_SIZE)
> >>> +		alloc_align_mask = PAGE_SIZE - 1;
> >>> +
> >>>    	/*
> >>>    	 * Ensure that the allocation is at least slot-aligned and update
> >>>    	 * 'iotlb_align_mask' to ignore bits that will be preserved when
> >>> @@ -1006,13 +1017,6 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> >>>    	 */
> >>>    	stride = get_max_slots(max(alloc_align_mask, iotlb_align_mask));
> >>>    
> >>> -	/*
> >>> -	 * For allocations of PAGE_SIZE or larger only look for page aligned
> >>> -	 * allocations.
> >>> -	 */
> >>> -	if (alloc_size >= PAGE_SIZE)
> >>> -		stride = umax(stride, PAGE_SHIFT - IO_TLB_SHIFT + 1);
> >>> -
> >>>    	spin_lock_irqsave(&area->lock, flags);
> >>>    	if (unlikely(nslots > pool->area_nslabs - area->used))
> >>>    		goto not_found;  
> >>  
> >   
> 

On Fri, Mar 08, 2024 at 05:38:16PM +0100, Petr Tesařík wrote:
> I thought we agreed that this stricter alignment is unnecessary:
> 
>   https://lore.kernel.org/linux-iommu/20240305140833.GC3659@lst.de/
> 
> But if everybody else wants to have it...

Let's get the fixes in with the existing documented alignment, and
then lift it in the next merge window with enough soaking time in
linux-next.

On Fri, 8 Mar 2024 17:50:25 +0100
Christoph Hellwig <hch@lst.de> wrote:

> On Fri, Mar 08, 2024 at 05:38:16PM +0100, Petr Tesařík wrote:
> > I thought we agreed that this stricter alignment is unnecessary:
> > 
> >   https://lore.kernel.org/linux-iommu/20240305140833.GC3659@lst.de/
> > 
> > But if everybody else wants to have it...  
> 
> Let's get the fixes in with the existing documented alignment, and
> then lift it in the next merge window with enough soaking time in
> linux-next.

I agree. It make sense.

Petr T