Re: [PATCH v5 6/6] swiotlb: Remove pointless stride adjustment for allocations >= PAGE_SIZE
From: Robin Murphy
Date: Mon Mar 04 2024 - 12:11:37 EST
On 04/03/2024 4:04 pm, Michael Kelley wrote:
From: Petr Tesařík <petr@xxxxxxxxxxx> Sent: Monday, March 4, 2024 7:55 AM
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
2. alloc_align_mask: zero; min_align_mask: non-zero
3. alloc_align_mask: non-zero; min_align_mask: zero/ignored
4. alloc_align_mask: non-zero; min_align_mask: non-zero
What does "min_align_mask: zero/ignored" mean? Under which
circumstances should be a non-zero min_align_mask ignored?
"Ignored" was my short-hand for the swiotlb_alloc() case where
orig_addr is zero. Even if min_align_mask is set for the device, it
doesn't have any effect when orig_addr is zero.
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.
For #2, min_align_mask governs the bits of the returned
physical address that must match the original address. When
needed, swiotlb must also allocate pre-padding aligned to
IO_TLB_SIZE that precedes the returned physical address. A
request size <= swiotlb_max_mapping_size() will not exceed
IO_TLB_SEGSIZE even with the padding. The historical
requirement for page alignment does not apply because the
driver has explicitly used the newer min_align_mask feature.
What is the idea here? Is it the assumption that only old drivers rely
on page alignment, so if they use min_align_mask, it proves that they
are new and must not rely on page alignment?
Yes, if a driver goes out of its way to set a min_align_mask which is
smaller than its actual alignment constraint, that is clearly the
driver's own bug. Strictly we only need to be sympathetic to drivers
which predate min_align_mask, when implicitly relying on page alignment
was all they had.
For #3, alloc_align_mask specifies the required alignment. No
pre-padding is needed. Per earlier comments from Robin[1],
it's reasonable to assume alloc_align_mask (i.e., the granule)
is >= IO_TLB_SIZE. The original address is not relevant in
determining the alignment, and the historical page alignment
requirement does not apply since alloc_align_mask explicitly
states the alignment.
FWIW I'm also starting to wonder about getting rid of the alloc_size
argument and just have SWIOTLB round the end address up to
alloc_align_mask itself as part of all these calculations. Seems like it
could potentially end up a little simpler, maybe?
Yes, I was thinking exactly this. But my reasoning was to solve the
bug in #4 that I previously pointed out. If iommu_dma_map_page()
does *not* do
aligned_size = iova_align(iovad, size);
but swiotlb_tbl_map_single() rounds up the size based on
alloc_align_mask *after* adding the offset modulo
min_align_mask, then the rounded-up size won't exceed IO_TLB_SIZE,
regardless of which bits are set in orig_addr.
Ah, neat, I had a gut feeling that something like that might also fall
out, I just didn't feel like working through the details to see if
"simpler" could lead to "objectively better" :)
I guess at worst we might also need to pass an alloc_align_mask to
swiotlb_max_mapping_size() as well, but even that's not necessarily a
bad thing if it keeps the equivalent calculations close together within
SWIOTLB and makes things more robust overall.
Cheers,
Robin.