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.
The removal of caching mode check in intel_flush_iotlb_all() doesn't
impact anything no matter the IOMMU is working in caching mode or not.
Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
caching-mode is on") has already disabled flush-queue for caching mode,
hence caching mode will never call intel_flush_iotlb_all().
Fixes: bf92df30df90 ("intel-iommu: Only avoid flushing device IOTLB for domain ID 0 in caching mode")
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/intel/iommu.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Change log:
v2:
- Squash two patches into a single one.
- No functionality changes.
v1: https://lore.kernel.org/linux-iommu/[email protected]/
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 50eb9aed47cc..681789b1258d 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1501,11 +1501,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
else
__iommu_flush_iotlb_psi(iommu, did, pfn, pages, ih);
- /*
- * In caching mode, changes of pages from non-present to present require
- * flush. However, device IOTLB doesn't need to be flushed in this case.
- */
- if (!cap_caching_mode(iommu->cap) || !map)
+ if (!map)
iommu_flush_dev_iotlb(domain, addr, mask);
}
@@ -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)
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Wednesday, April 10, 2024 1:58 PM
>
> 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.
>
> The removal of caching mode check in intel_flush_iotlb_all() doesn't
> impact anything no matter the IOMMU is working in caching mode or not.
> Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
> caching-mode is on") has already disabled flush-queue for caching mode,
> hence caching mode will never call intel_flush_iotlb_all().
>
> Fixes: bf92df30df90 ("intel-iommu: Only avoid flushing device IOTLB for
> domain ID 0 in caching mode")
> Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
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. :)
[1]
https://lore.kernel.org/linux-iommu/[email protected]/
> The removal of caching mode check in intel_flush_iotlb_all() doesn't
> impact anything no matter the IOMMU is working in caching mode or not.
> Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
> caching-mode is on") has already disabled flush-queue for caching mode,
> hence caching mode will never call intel_flush_iotlb_all().
>
> Fixes: bf92df30df90 ("intel-iommu: Only avoid flushing device IOTLB for domain ID 0 in caching mode")
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Change log:
>
> v2:
> - Squash two patches into a single one.
> - No functionality changes.
>
> v1: https://lore.kernel.org/linux-iommu/[email protected]/
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 50eb9aed47cc..681789b1258d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1501,11 +1501,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
> else
> __iommu_flush_iotlb_psi(iommu, did, pfn, pages, ih);
>
> - /*
> - * In caching mode, changes of pages from non-present to present require
> - * flush. However, device IOTLB doesn't need to be flushed in this case.
> - */
> - if (!cap_caching_mode(iommu->cap) || !map)
> + if (!map)
> iommu_flush_dev_iotlb(domain, addr, mask);
> }
>
> @@ -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);
why not one more step to move it out of the iommu_array loop? There is
no more reason to keep it in the loop.
> }
>
> if (dmar_domain->nested_parent)
--
Regards,
Yi Liu
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
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
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
On 10/04/2024 6:58 am, 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.
>
> The removal of caching mode check in intel_flush_iotlb_all() doesn't
> impact anything no matter the IOMMU is working in caching mode or not.
> Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
> caching-mode is on") has already disabled flush-queue for caching mode,
> hence caching mode will never call intel_flush_iotlb_all().
Well, technically it might still, at domain creation via
iommu_create_device_direct_mappings(), but domain->has_iotlb_device
should definitely be false at that point :)
Cheers,
Robin.
> Fixes: bf92df30df90 ("intel-iommu: Only avoid flushing device IOTLB for domain ID 0 in caching mode")
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/intel/iommu.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Change log:
>
> v2:
> - Squash two patches into a single one.
> - No functionality changes.
>
> v1: https://lore.kernel.org/linux-iommu/[email protected]/
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 50eb9aed47cc..681789b1258d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1501,11 +1501,7 @@ static void iommu_flush_iotlb_psi(struct intel_iommu *iommu,
> else
> __iommu_flush_iotlb_psi(iommu, did, pfn, pages, ih);
>
> - /*
> - * In caching mode, changes of pages from non-present to present require
> - * flush. However, device IOTLB doesn't need to be flushed in this case.
> - */
> - if (!cap_caching_mode(iommu->cap) || !map)
> + if (!map)
> iommu_flush_dev_iotlb(domain, addr, mask);
> }
>
> @@ -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)
On 2024/4/11 21:13, Robin Murphy wrote:
> On 10/04/2024 6:58 am, 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.
>>
>> The removal of caching mode check in intel_flush_iotlb_all() doesn't
>> impact anything no matter the IOMMU is working in caching mode or not.
>> Commit <29b32839725f> ("iommu/vt-d: Do not use flush-queue when
>> caching-mode is on") has already disabled flush-queue for caching mode,
>> hence caching mode will never call intel_flush_iotlb_all().
>
> Well, technically it might still, at domain creation via
> iommu_create_device_direct_mappings(), but domain->has_iotlb_device
> should definitely be false at that point ????
Oh! I overlooked that path. :-)
Yes. iommu_create_device_direct_mappings() is called before setting the
domain to device for intel iommu driver, hence in practice the
domain->has_iotlb_device is always false.
Best regards,
baolu
On 2024/4/10 18:38, Baolu Lu wrote:
> 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. :-)
yeah, would be helpful to refine the commit message w.r.t. this change.
>>
>> 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.
ok. So the key reason is that intel_flush_iotlb_all() is not called at
all if CM==1 after that commit. So there is nothing fixed by this change.
right? This just makes the logic cleaner. Perhaps no need for a fix tag.
Reviewed-by: Yi Liu <[email protected]>
--
Regards,
Yi Liu