Message-ID: <ZgZNAM337-UEY1DH@atmark-techno.com>
Date: Fri, 29 Mar 2024 14:09:20 +0900
From: Dominique Martinet <dominique.martinet@...ark-techno.com>
To: mhklinux@...look.com
Cc: hch@....de, m.szyprowski@...sung.com, robin.murphy@....com,
	konrad.wilk@...cle.com, bumyong.lee@...sung.com,
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org,
	will@...nel.org, petr@...arici.cz, roberto.sassu@...weicloud.com,
	lukas@...mn.com
Subject: Re: [PATCH 1/1] swiotlb: Fix swiotlb_bounce() to do partial sync's
 correctly

Dominique Martinet wrote on Wed, Mar 27, 2024 at 03:05:18PM +0900:
> Unfortunately that was ages ago so I don't really remember exactly on
> which device that was reproduced.. Given the Cc I'm sure Lukas had hit
> it on the MNT reform (i.MX8MQ), but I did say I tested it so I probably
> could reproduce on my i.MX8MP?
> I'll try to give a try at reproducing the old bug, and if I do test your
> fix over next week.

grmbl, sorry I cannot reproduce the problem on devices I have readily
accessible, and don't have time to dig out my reform to test there in
the forseeable future, so cannot confirm if that also fixes the problem
we reported two years ago.
However I had misunderstood your patch, I thought you were also
reverting commit 5f89468e2f06 ("swiotlb: manipulate orig_addr when
tlb_addr has offset") but you're keeping it and just making it signed --
this should cause no problem for the use case I was concerned about as
it fell within the bounds I had defined, so this is basically a no-op
patch for my usecase and I don't expect this particular failure to pop
back up here.


Code-wise, I agree the checks I added in commit 868c9ddc182b ("swiotlb:
add overflow checks to swiotlb_bounce") are too strict - I failed to
consider the device minimum alignment part of swiotlb_align_offset, and
thinking this through this can get weird.
.. And now I'm looking again even with a big minimum alignment it's
also too strict, so, right, let's work through an example.


>From my understanding of how orig_addr is computed, in the multi-block
case we have something like this:

(+ represent device's minimum alignment, bigger blocks with | are
IO_TLB_SIZE blocks)

         10        20        30        40        50        60
01234567890123456789012345678901234567890123456789012345678901234...
|---+---+---+-block1+---+---+---|---+---+---+-block2+---+---+---|...
       ^                               ^                          
       mem->slots[n].orig_addr         mem->slots[n+1].orig_addr
       (=7)                            (=32+7=39)

(memo of the code with your patch:
  index = (tlb_addr - mem->start) >> IO_TLB_SHIFT;
  orig_addr = mem->slots[index].orig_addr;
  swiotlb_align_offset(dev, orig_addr) = orig_addr & dev min align mask & (IO_TLB_SIZE-1)
  tlb_offset = (tlb_addr & (IO_TLB_SIZE - 1)) - swiotlb_align_offset(dev, orig_addr);
  orig_addr += tlb_offset;
  alloc_size -= tlb_offset;
  vaddr = mem->vaddr + tlb_addr - mem->start
)

So for this example we have IO_TLB_SIZE=32, dev min alignment=4,
orig_addr's align value would be 7%4=3.
Given say tlb_addr at 33, we'd find slot n, and compute
tlb_offset = 1 - 3 = -2

.. And then I just don't follow anymore?

Computing the rest mechanically, for the example's sake let's say n=0
so mem->start=7, index=0, and also set size to 4 bytes, vaddr to 0x1000.

vaddr = 0x1000 + 33 - 7 = 0x1000 + 26
orig_addr = 7 + -2 = 5
size = 4 - -2 = 6

then we'd proceed to memcpy either (vaddr, p2v(orig_addr), size) or the
other way around, but this cannot be right:
 - size is bigger than what was requested, I fail to see how that can be
allowed. I'd understand a smaller size assuming swiotlb_bounce gets
called for each interval, but not a bigger size.
 - orig_addr is nowhere near 33.


I thought this didn't make sense because of the minimum device
alignment, but even with device alignment >= io tlb's with the very same
example we would get 
tld_offset = 1 - 7 = -6
now that one could make sense if we had used the following slot e.g.
orig_addr being slot[1].orig_addr and we'd get back to 31, but that's
not the case, and the size calculation is still off.


So, long story short it took me half a day to get back into this code
and the only thing I understand about it is that I don't understand it.

I'm sure it works most of the case because everything is nicely aligned
(since nobody complained about my checks before, and if there's no
warning with these the code works), but I'd require some convincing to
give a reviewed-by tag to this patch.

Thanks for working on this though, I'll be happy to be pointed out at
flaws in my logic or to look at another attempt...!!
-- 
Dominique

Powered by blists - more mailing lists