Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 10 Apr 2024 18:38:00 +0800 | Subject | Re: [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush | From | Baolu Lu <> |
| |
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
| ![\](/images/icornerr.gif) |