Messages in this thread Patch in this message | | | Date | Wed, 10 Apr 2024 17:14:38 +0800 | Subject | Re: [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush | From | Yi Liu <> |
| |
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
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.
@@ -1579,8 +1575,7 @@ static void intel_flush_iotlb_all(struct iommu_domain *domain) iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
- if (!cap_caching_mode(iommu->cap)) - iommu_flush_dev_iotlb(dmar_domain, 0, MAX_AGAW_PFN_WIDTH); + iommu_flush_dev_iotlb(dmar_domain, 0, MAX_AGAW_PFN_WIDTH); }
if (dmar_domain->nested_parent)
[1] https://lore.kernel.org/linux-iommu/BN9PR11MB52764F42C20B6B18DDE7E66B8C072@BN9PR11MB5276.namprd11.prod.outlook.com/
-- Regards, Yi Liu
| |