Re: [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush

From: Baolu Lu
Date: Wed Apr 10 2024 - 06:39:05 EST


On 2024/4/10 17:14, Yi Liu wrote:


On 2024/4/10 16:02, Baolu Lu wrote:
On 2024/4/10 14:30, Yi Liu wrote:
On 2024/4/10 13:58, Lu Baolu wrote:
The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
implementation caches not-present or erroneous translation-structure
entries except the first-stage translation. The caching mode is
irrelevant to the device TLB , therefore there is no need to check
it before a device TLB invalidation operation.

iommu_flush_iotlb_psi() is called in map and unmap paths. The caching
mode check before device TLB invalidation will cause device TLB
invalidation always issued if IOMMU is not running in caching mode.
This is wrong and causes unnecessary performance overhead.

I don't think the original code is wrong. As I replied before, if CM==0,
the iommu_flush_iotlb_psi() is only called in unmap path, in which the
@map is false. [1] The reason to make the change is to make the logic
simpler. 🙂

Oh, I see. There is a magic

         if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
                 iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);

in __mapping_notify_one().

So if it's caching mode, then

  - iommu_flush_iotlb_psi() will be called with @map=1 from
    __mapping_notify_one(), "!cap_caching_mode(iommu->cap) || !map" is
    not true, and device TLB is not invalidated.
  - iommu_flush_iotlb_psi() will also be called with @map=0 from
    intel_iommu_tlb_sync(), device TLB is issued there.

That's the expected behavior for caching mode.

If it's not the caching mode, then

  - iommu_flush_iotlb_psi() will be called with @map=0 from
    intel_iommu_tlb_sync(), device TLB is issued there.

That's also the expected behavior.

So the existing code is correct but obscure and difficult to understand,
right? If so, we should make this patch as a cleanup rather than a fix.

aha, yes. As the below table, iommu_flush_iotlb_psi() does flush device TLB
as expected. But there is a NA case. When CM==0, it should not be possible
to call iommu_flush_iotlb_psi() with @map==1 as cache invalidation is not
required when CM==0. So the existing code logic is really confusing,
checking @map is enough and clearer. Since the old code works, so perhaps
no fix tag is needed. :)

+----+------+-----------+------------+
|  \       |            |            |
|   \ @map |            |            |
| CM \     |      0     |     1      |
|     \    |            |            |
+------+---+------------+------------+
|          |            |            |
|     0    |      Y     |     NA     |
+----------+------------+------------+
|          |            |            |
|     1    |      Y     |     N      |
+----------+------------+------------+

Y means flush dev-TLB please
N means no need to flush dev-TLB
NA means not applied

Yes. We have the same understanding now. :-)


BTW. I think it is better to have the below change in a separate patch.
The below change does fix a improper dev-TLB flushing behavior. Also
how about Kevin's concern in the end of [1]. I didn't see your respond
about it.

I had an offline discussion with him and I included the conclusion in
the commit message of this patch.

Best regards,
baolu