On Thu, Jun 10, 2021 at 11:21 PM Christoph Hellwig <hch@xxxxxx> wrote: > > FYI, there has been a patch on the list that should have fixed this > for about a month: > > https://lore.kernel.org/linux-iommu/20210510091816.GA2084@xxxxxx/T/#m0d0df6490350a08dcc24c9086c8edc165b402d6f Honestly, that patch is all kinds of strange. This expression: tlb_offset = (tlb_addr & (IO_TLB_SIZE - 1)) - swiotlb_align_offset(dev, orig_addr); makes no sense to me. Maybe it happens to work, but I think it does so by mistake rather than by design. What my patch used was: unsigned long offset = (tlb_addr - mem->start) & (IO_TLB_SIZE - 1); which actually pairs with - and makes sense with - the index calculation: int index = (tlb_addr - mem->start) >> IO_TLB_SHIFT; so that offset truly is the offset within that index. Look at how that 'index' calculation calculates the high bits of the difference, and the 'offset' calculation now literally is the low bits of the same thing that got dropped on the floor by the 'index' calculation? So those two calculations actually make sense. The swiotlb_align_offset() one doesn't. It's possible that that swiotlb_align_offset() function ends up giving the right answer just almost by mistake (because of how tlb_addr and orig_addr end up being related - the swiotlb_align_offset() expression might just end up being the same thing - I didn't look deeper), but even if the result is the same, it's not _sensible_ code, It's also possible that the swiotlb_align_offset() function ends up giving the right answer very much by design and because of how orig_addr works - because maybe the remapping is doing odd things and using that swiotlb_align_offset() function in ways that make the *obvious* and natural offset calculation not actually work. So it's at least in theory possible that my "natural offset" calculation that matches how the index is calculated doesn't actually work. But that means that the swiotlb remapping is doing some really odd things, and then I think the patch would need a lot more commentary on exactly what those very odd things are. Linus
