Re: [PATCH 1/2] swiotlb: Fix allocation alignment requirement when searching slots

[Petr Tesařík]

On Mon, 29 Jan 2024 19:32:50 +0000
Will Deacon <will@xxxxxxxxxx> wrote:

> On Fri, Jan 26, 2024 at 06:01:27PM +0100, Petr Tesařík wrote:
> > On Fri, 26 Jan 2024 15:19:55 +0000
> > Will Deacon <will@xxxxxxxxxx> wrote:  
> > > diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> > > index b079a9a8e087..25febb9e670c 100644
> > > --- a/kernel/dma/swiotlb.c
> > > +++ b/kernel/dma/swiotlb.c
> > > @@ -982,7 +982,7 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > >  		phys_to_dma_unencrypted(dev, pool->start) & boundary_mask;
> > >  	unsigned long max_slots = get_max_slots(boundary_mask);
> > >  	unsigned int iotlb_align_mask =
> > > -		dma_get_min_align_mask(dev) | alloc_align_mask;
> > > +		dma_get_min_align_mask(dev) & ~(IO_TLB_SIZE - 1);  
> > 
> > Good. So, iotlb_align_mask now specifies how many low bits of orig_addr
> > should be preserved in the bounce buffer address, ignoring the offset
> > within the TLB slot...  
> 
> Yup, this is basically restoring the old behaviour.
> 
> > >  	unsigned int nslots = nr_slots(alloc_size), stride;
> > >  	unsigned int offset = swiotlb_align_offset(dev, orig_addr);
> > >  	unsigned int index, slots_checked, count = 0, i;
> > > @@ -998,14 +998,13 @@ static int swiotlb_search_pool_area(struct device *dev, struct io_tlb_pool *pool
> > >  	 * allocations.
> > >  	 */
> > >  	if (alloc_size >= PAGE_SIZE)
> > > -		iotlb_align_mask |= ~PAGE_MASK;
> > > -	iotlb_align_mask &= ~(IO_TLB_SIZE - 1);
> > > +		alloc_align_mask |= ~PAGE_MASK;  
> > 
> > ...and alloc_align_mask specifies the desired TLB slot alignment.  
> 
> Yes, although actually I'm now wondering whether there's another bug here
> in that we don't return naturally aligned buffers for allocations bigger
> than a page. I think that was broken in 0eee5ae10256 ("swiotlb: fix slot
> alignment checks") because that stopped aligning the initial search index
> to the stride (which was in turn previously aligned to the allocation size).

The question is whether there is any NEED that allocations bigger than
a page are naturally aligned. For my part, I don't see why there should
be, but I might be missing something.

> > >  	/*
> > >  	 * For mappings with an alignment requirement don't bother looping to
> > >  	 * unaligned slots once we found an aligned one.
> > >  	 */
> > > -	stride = (iotlb_align_mask >> IO_TLB_SHIFT) + 1;
> > > +	stride = (max(alloc_align_mask, iotlb_align_mask) >> IO_TLB_SHIFT) + 1;  
> > 
> > I'm not quite sure about this one.
> > 
> > And I'm not even sure all combinations make sense!
> > 
> > For example, take these values:
> > 
> > *         TLB_SIZE ==              0x800  (2K)
> > * alloc_align_mask == 0xffffffffffffc000  (16K alignment, could be page size)
> > * iotlb_align_mask == 0xffffffffffff0000  (64K alignment)
> > *        orig_addr == 0x0000000000001234
> > 
> > Only the lowest 16 bits are relevant for the alignment check.
> > Device alignment requires 0x1000.
> > Alloc alignment requires one of 0x0000, 0x4000, 0x8000, or 0xc000.
> > Obviously, such allocation must always fail...  
> 
> Having an iotlb_align_mask with all those upper bits set looks wrong to me.
> Is that the same "braino" as bbb73a103fbb?

I must always stop and think at least twice before I can be sure
whether a "mask" has the high bits set, or the low bits set...

On an x86, PAGE_SHIFT is 12, PAGE_SIZE is 1UL << PAGE_SHIFT or 0x1000,
PAGE_MASK is ~(PAGE_SIZE-1)) or 0xfffffffffffff000, and there's one
more bitwise negation, so you're right. Both masks above should be
inverted, and using max() to find the stride is correct.

Petr T