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

[Baolu Lu]

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.

Best regards,
baolu