RE: [PATCH v3 0/2] swiotlb: allocate padding slots if necessary

[Michael Kelley]

From: Will Deacon <will@xxxxxxxxxx> Sent: Friday, March 22, 2024 8:10 AM
> 
> Hi Petr,
> 
> On Thu, Mar 21, 2024 at 06:19:00PM +0100, Petr Tesarik wrote:
> > From: Petr Tesarik <petr.tesarik1@xxxxxxxxxxxxxxxxxxx>
> >
> > If the allocation alignment is bigger than IO_TLB_SIZE and min_align_mask
> > covers some bits in the original address between IO_TLB_SIZE and
> > alloc_align_mask, preserve these bits by allocating additional padding
> > slots before the actual swiotlb buffer.
> 
> Thanks for fixing this! I was out at a conference last week, so I didn't
> get very far with it myself, but I ended up in a pickle trying to avoid
> extending 'struct io_tlb_slot'. Your solution is much better than the
> crazy avenue I started going down...
> 
> With your changes, can we now simplify swiotlb_align_offset() to ignore
> dma_get_min_align_mask() altogether and just:
> 
> 	return addr & (IO_TLB_SIZE - 1);
> 
> ?
> 

I don't think such a change is correct, since we want to allow the
DMA min_align_mask to work if it is set to 0x3FF or 0x1FF or
something else smaller than IO_TLB_SIZE - 1.

Petr's new offset calculation in swiotlb_tbl_map_single() is this:

offset = orig_addr & dma_get_min_align_mask(dev) &
                (alloc_align_mask | (IO_TLB_SIZE - 1));

In the normal stream mapping case, where alloc_align_mask is
zero, Petr's new statement is equivalent to swiotlb_align_offset().
And I think it needs to continue to be equivalent so that
swiotlb_search_pool_area(), swiotlb_bounce()  and
swiotlb_release_slots() calculate the same offset as
swiotlb_tbl_map_single() uses after it separately processes
the padding slots.

Perhaps a better approach to maintaining the equivalence is
to modify swiotlb_align_offset() to be Petr's new calculation,
with alloc_align_mask passed as an argument.
swiotlb_search_pool_area(), swiotlb_bounce(), and
swiotlb_release_slots() would all pass 0 as the alloc_align_mask
argument.

Michael