lkml.org 
[lkml]   [2024]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [PATCH v2 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
From


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

\
 
 \ /
  Last update: 2024-05-27 16:32    [W:0.079 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site