Re: [PATCH 1/1] swiotlb: extend buffer pre-padding to alloc_align_mask if necessary

[Petr Tesařík]

On Fri, 15 Mar 2024 14:59:08 +0000
Michael Kelley <mhklinux@xxxxxxxxxxx> wrote:

> From: Petr Tesařík <petr@xxxxxxxxxxx> Sent: Friday, March 15, 2024 5:26 AM
> > 
> > On Fri, 15 Mar 2024 02:53:10 +0000
> > Michael Kelley <mhklinux@xxxxxxxxxxx> wrote:
> >   
> > > From: Petr Tesarik <petrtesarik@xxxxxxxxxxxxxxx> Sent: Tuesday, March 12, 2024 6:42 AM  
> > > >  
> 
> [snip]
> 
> > > > @@ -1349,6 +1353,15 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> > > >  		return (phys_addr_t)DMA_MAPPING_ERROR;
> > > >  	}
> > > >
> > > > +	/*
> > > > +	 * Calculate buffer pre-padding within the allocated space. Use it to
> > > > +	 * preserve the low bits of the original address according to device's
> > > > +	 * min_align_mask. Limit the padding to alloc_align_mask or slot size
> > > > +	 * (whichever is bigger); higher bits of the original address are
> > > > +	 * preserved by selecting a suitable IO TLB slot.
> > > > +	 */
> > > > +	offset = orig_addr & dma_get_min_align_mask(dev) &
> > > > +		(alloc_align_mask | (IO_TLB_SIZE - 1));
> > > >  	index = swiotlb_find_slots(dev, orig_addr,
> > > >  				   alloc_size + offset, alloc_align_mask, &pool);
> > > >  	if (index == -1) {
> > > > @@ -1364,6 +1377,12 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
> > > >  	 * This is needed when we sync the memory.  Then we sync the buffer if
> > > >  	 * needed.
> > > >  	 */
> > > > +	padding = 0;
> > > > +	while (offset >= IO_TLB_SIZE) {
> > > > +		pool->slots[index++].orig_addr = INVALID_PHYS_ADDR;
> > > > +		pool->slots[index].padding = ++padding;
> > > > +		offset -= IO_TLB_SIZE;
> > > > +	}  
> > >
> > > Looping to fill in the padding slots seems unnecessary.
> > > The orig_addr field should already be initialized to
> > > INVALID_PHYS_ADDR, and the padding field should already
> > > be initialized to zero.  
> > 
> > Ack.
> >   
> > > The value of "padding" should be just
> > > (offset / IO_TLB_SIZE), and it only needs to be stored in the
> > > first non-padding slot where swiotlb_release_slots() will
> > > find it.  
> > 
> > This is also right. I asked myself the question what should happen if
> > somebody passes a padding slot's address to swiotlb_tbl_unmap_single().
> > Of course, it's an invalid address, but as a proponent of defensive
> > programming, I still asked what would be the best response? If I store
> > padding in each slot, the whole block is released. If I store it only
> > in the first non-padding slot, some slots may leak.
> > 
> > On a second thought, the resulting SWIOTLB state is consistent either
> > way, and we don't to care about leaking some slots if a driver is
> > buggy. Maybe it's even better, because the leak will be noticed.
> > 
> > In short, I agree, let's keep the code simple.
> >   
> 
> One other thought:  Fundamentally, fixing the core problem
> requires swiotlb_tbl_unmap_single() to have some information
> it doesn't have in the current code.  It needs to know the
> number of padding slots, so that it can free them correctly.
> Your solution is to store the # of padding slots in the
> io_tlb_slot array.
> 
> Another approach would be to have callers pass the
> alloc_align_mask as an argument to swiotlb_tbl_unmap_single().
> It can then calculate the offset and the number of padding
> slots just like swiotlb_tbl_map_single() does.  Nothing
> additional would need to be stored in the io_tlb_slot array.
> The IOMMU code is the only caller than uses a non-zero
> alloc_align_mask.  From a quick look at that code, the
> unmap path has the iova_mask() available, so that would
> work.  Other callers would pass zero, just like they do for
> swiotlb_tbl_map_single().
> 
> I don't immediately have a strong opinion either way, but
> it's something to think about a bit more.

I believe it's slightly more robust to store how the buffer was
actually allocated than to rely on the caller. It seems to me that this
was also a design goal of the original author. For example, note that
swiotlb_tbl_unmap_single() uses mapping_size only to do the final
buffer sync, but not to determine how many slots should be released.
This information is taken from struct io_tlb_slot.alloc_size.

Petr T