RE: [PATCH 1/1] swiotlb: Fix swiotlb_bounce() to do partial sync's correctly

[Michael Kelley]

From: Dominique Martinet <dominique.martinet@xxxxxxxxxxxxxxxxx> Sent: Friday, March 29, 2024 7:56 PM
> 
> Michael Kelley wrote on Fri, Mar 29, 2024 at 03:18:16PM +0000:
> > * tlb_offset = 1 - 3 = -2, as you describe above
> > * orig_addr = 39 + -2 = 37.  The computation uses 39 from
> > slot[1], not the 7 from slot[0].  This computed 37 is the
> > correct orig_addr to use for the memcpy().
> 
> There are two things I don't understand here:
> 1/ Why orig_addr would come from slot[1] ?
> 
> We have index = (tlb_addr - mem->start) >> IO_TLB_SHIFT,
> so index = (33 - 7) >> 5 = 26 >> 5 = 0
> 
> As such, orig_addr = mem->slots[0].orig_addr and we'd need the offset to
> be 30, not -2 ?

mem->start is the physical address of the global pool of
memory allocated for swiotlb buffers.  That global pool defaults
to 64 Mbytes in size, and is allocated starting on a page boundary
(which is important).  It is divided into "slots", which in a real
kernel are 2 Kbytes each (IO_TLB_SHIFT is 11).  When a mapping
is created, swiotlb_tbl_map_single() returns a tlb_addr between
mem->start and mem->start + 64 Mbytes - 1.  The slot index
for any tlb_addr can be obtained by doing

	index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;

assuming that mem->start is on a boundary that is at least
as big as IO_TLB_SHIFT.  So your example of mem->start being
7 isn't valid.  If we're using an example where tlb_addr "3" is
returned from swiotlb_tbl_map_single(), and tlb_addr 33
is passed as the argument to swiotlb_bounce(), then
mem->start would need to be zero for things to work.  If
mem->start is zero, then tlb_addr's 0 thru 31 are in slot 0,
and tlb_addr's 32 thru 63 are in slot 1, etc.

> Well, either work - if we fix index to point to the next slot in the
> negative case that's also acceptable if we're sure it's valid, but I'm
> worried it might not be in cases there was only one slot e.g. mapping
> [7; 34] and calling with 33 size 2 would try to access slot 1 with a
> negative offset in your example, but slot[0] is the last valid slot.

Right, but there wouldn't be one slot mapping [7; 34] if the
alignment rules are followed when the global swiotlb memory
pool is originally created.  The low order IO_TLB_SHIFT bits
of slot physical addresses must be zero for the arithmetic
using shifts to work, so [7; 34] will cross a slot boundary and
two slots are needed.

> 
> 
> 2/ Why is orig_addr 37 the correct address to use for memcpy, and not
> 33? I'd think it's off by a "minimum alignment page", for me this
> computation only works if the dma_get_min_align size is bigger than io
> tlb size.

The swiotlb mapping operation establishes a pair-wise mapping between
an orig_addr and tlb_addr, with the mapping extending for a specified
number of bytes.   Your example started with orig_addr = 7, and I
posited that the mapping extends for 40 bytes.   I further posited
that the tlb_addr returned by swiotlb_tbl_map_single() would
be 3 to meet the min alignment requirement (which again
only works if mem->start is 0).  So the pair-wise mapping is (7, 3).
Now when swiotlb_bounce() is called with a tlb_addr of 33 and
a size of 4, we know:

* tlb_addr 33 is in slot 1
* tlb_addr 33 is 30 bytes (33 - 3) from the beginning of the 
swiotlb area allocated for the mapping by swiotlb_tbl_map_single()
* So we want to add 30 bytes to the orig_addr to get the address
where we want to do the memcpy.  That is 7 + 30 = 37.
* The swiotlb area allocated for the mapping goes from
tlb_addr 3 through tlb_addr 43 since the size of the mapping
is 40 bytes.  If we do a partial sync of 4 bytes starting at
tlb_addr 37, then that's valid because all 4 bytes fit within
the originally mapped 40 bytes.

Michael

> 
> 
> > * size is still 4.  There's no computation in swiotlb_bounce()
> > that changes "size".
> > * alloc_size is pulled from slot[1], and is adjusted by tlb_offset.
> > This adjusted alloc_size isn't used for anything except as a sanity
> > check against "size".
> 
> Right, sorry - so size is ok (assuming slot[1] is used, I conflated the
> two sizes.
> 
> 
> I'm probably still missing something here, thanks for bearing with me.
> --
> Dominique