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

[Michael Kelley]

From: Robin Murphy <robin.murphy@xxxxxxx> Sent: Monday, March 4, 2024 8:54 AM
> 
> On 04/03/2024 4:10 pm, Michael Kelley wrote:
> > From: Will Deacon <will@xxxxxxxxxx> Sent: Monday, March 4, 2024 8:02 AM
> >>
> >> 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;
> >> +
> >
> > Yes, I think that should do it.
> 
> In principle it might be more logical to fudge this into
> iotlb_align_mask rather than alloc_align_mask 

I'm not understanding what you are getting at, but maybe we are
interpreting the historical page alignment requirement differently.
I think of the page alignment requirement as independent of the
orig_addr -- the returned physical address should always be exactly
page aligned, and not offset to match bits in orig_addr.  If that's
the case, then implementing the page alignment via
alloc_align_mask is logically the right place. Fudging into
iotlb_align_mask would do matching of bits in orig_addr.

Or is there something else I'm not considering?

Michael

> - since that's really the
> effective behaviour to preserve for streaming mappings - and then pass
> an explicit alloc_align_mask from swiotlb_alloc() to honour the
> dma-coherent requirements. However I also wouldn't really object to not
> going that far and instead just making the comment a bit clearer that
> this is still serving both purposes.
> 
> Cheers,
> Robin.
> 
> >
> > Michael
> >
> >>          /*
> >>           * 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;
> >