> On Apr 8, 2021, at 12:18 AM, Joerg Roedel <[email protected]> wrote:
>
> Hi Nadav,
>
> On Wed, Apr 07, 2021 at 05:57:31PM +0000, Nadav Amit wrote:
>> I tested it on real bare-metal hardware. I ran some basic I/O workloads
>> with the IOMMU enabled, checkers enabled/disabled, and so on.
>>
>> However, I only tested the IOMMU-flushes and I did not test that the
>> device-IOTLB flush work, since I did not have the hardware for that.
>>
>> If you can refer me to the old patches, I will have a look and see
>> whether I can see a difference in the logic or test them. If you want
>> me to run different tests - let me know. If you want me to remove
>> the device-IOTLB invalidations logic - that is also fine with me.
>
> Here is the patch-set, it is from 2010 and against a very old version of
> the AMD IOMMU driver:
Thanks. I looked at your code and I see a difference between the
implementations.
As far as I understand, pages are always assumed to be aligned to their
own sizes. I therefore assume that flushes should regard the lower bits
as a “mask” and not just as encoding of the size.
In the version that you referred me to, iommu_update_domain_tlb() only
regards the size of the region to be flushed and disregards the
alignment:
+ order = get_order(domain->flush.end - domain->flush.start);
+ mask = (0x1000ULL << order) - 1;
+ address = ((domain->flush.start & ~mask) | (mask >> 1)) & ~0xfffULL;
If you need to flush for instance the region between 0x1000-0x5000, this
version would use the address|mask of 0x1000 (16KB page). The version I
sent regards the alignment, and since the range is not aligned would use
address|mask of 0x3000 (32KB page).
IIUC, IOVA allocations today are aligned in such way, but at least in
the past (looking on 3.19 for the matter), it was not like always like
that, which can explain the problems.
Thoughts?
On Thu, Apr 08, 2021 at 10:29:25AM +0000, Nadav Amit wrote:
> In the version that you referred me to, iommu_update_domain_tlb() only
> regards the size of the region to be flushed and disregards the
> alignment:
>
> + order = get_order(domain->flush.end - domain->flush.start);
> + mask = (0x1000ULL << order) - 1;
> + address = ((domain->flush.start & ~mask) | (mask >> 1)) & ~0xfffULL;
>
>
> If you need to flush for instance the region between 0x1000-0x5000, this
> version would use the address|mask of 0x1000 (16KB page). The version I
> sent regards the alignment, and since the range is not aligned would use
> address|mask of 0x3000 (32KB page).
>
> IIUC, IOVA allocations today are aligned in such way, but at least in
> the past (looking on 3.19 for the matter), it was not like always like
> that, which can explain the problems.
Yeah, that make sense and explains the data corruption problems. I will
give your patch a try on one of my test machines and consider it for
v5.13 if all goes well.
Thanks,
Joerg