Re: [PATCH v5 6/6] swiotlb: Remove pointless stride adjustment for allocations >= PAGE_SIZE

[Will Deacon]

Hi folks,

On Mon, Mar 04, 2024 at 04:55:06PM +0100, Petr Tesařík wrote:
> On Mon, 4 Mar 2024 13:37:56 +0000
> Robin Murphy <robin.murphy@xxxxxxx> wrote:
> > On 04/03/2024 11:00 am, Petr Tesařík wrote:
> > [...]
> > >> Here's my take on tying all the threads together. There are
> > >> four alignment combinations:
> > >>
> > >> 1. alloc_align_mask: zero; min_align_mask: zero

Based on this ^^^ ...

> > >> xen_swiotlb_map_page() and dma_direct_map_page() are #1 or #2
> > >> via swiotlb_map() and swiotlb_tbl_map_single()
> > >>
> > >> iommu_dma_map_page() is #3 and #4 via swiotlb_tbl_map_single()
> > >>
> > >> swiotlb_alloc() is #3, directly to swiotlb_find_slots()
> > >>
> > >> For #1, the returned physical address has no constraints if
> > >> the requested size is less than a page. For page size or
> > >> greater, the discussed historical requirement for page
> > >> alignment applies.

.. and this ^^^ ...


> I believe this patch series is now good as is, except the commit
> message should make it clear that alloc_align_mask and min_align_mask
> can both be zero, but that simply means no alignment constraints.

.. my (possibly incorrect!) reading of the thread so far is that we
should preserve page-aligned allocation in this case if the allocation
size is >= PAGE_SIZE.

Something like the diff below, to replace this final patch?

Will

--->8

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c381a7ed718f..67eac05728c0 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -992,6 +992,14 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
        BUG_ON(!nslots);
        BUG_ON(area_index >= pool->nareas);
 
+       /*
+        * Historically, allocations >= PAGE_SIZE were guaranteed to be
+        * page-aligned in the absence of any other alignment requirements.
+        * Since drivers may be relying on this, preserve the old behaviour.
+        */
+       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 +1014,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;