* [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
@ 2024-04-15 1:38 Lu Baolu
2024-04-15 2:22 ` Zhang, Tina
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Lu Baolu @ 2024-04-15 1:38 UTC (permalink / raw
To: iommu
Cc: Kevin Tian, Yi Liu, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel, Lu Baolu
The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
implementation caches not-present or erroneous translation-structure
entries except for 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.
Remove two caching mode checks before device TLB invalidation in the
driver. The removal of these checks doesn't change the driver's behavior
in critical map/unmap paths. Hence, there is no functionality or
performance impact, especially since commit <29b32839725f> ("iommu/vt-d:
Do not use flush-queue when caching-mode is on") has already disabled
flush-queue for caching mode. Therefore, caching mode will never call
intel_flush_iotlb_all().
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
---
drivers/iommu/intel/iommu.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
Change log:
v3:
- It turned out that the removals don't change the driver's behavior,
hence change it from a fix patch to a cleanup one.
- No functionality changes.
v2: https://lore.kernel.org/lkml/20240410055823.264501-1-baolu.lu@linux.intel.com/
- Squash two patches into a single one.
- No functionality changes.
v1: https://lore.kernel.org/linux-iommu/20240407144232.190355-1-baolu.lu@linux.intel.com/
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index a7ecd90303dc..f0a67e9d9faf 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 1:38 [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
@ 2024-04-15 2:22 ` Zhang, Tina
2024-04-15 2:43 ` Yi Liu
2024-04-16 0:53 ` Ethan Zhao
2024-04-24 3:41 ` Baolu Lu
2 siblings, 1 reply; 11+ messages in thread
From: Zhang, Tina @ 2024-04-15 2:22 UTC (permalink / raw
To: Lu Baolu, iommu@lists.linux.dev
Cc: Tian, Kevin, Liu, Yi L, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel@vger.kernel.org
> -----Original Message-----
> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, April 15, 2024 9:39 AM
> To: iommu@lists.linux.dev
> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Jacob
> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel <joro@8bytes.org>; Will
> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
> device TLB flush
>
> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
> implementation caches not-present or erroneous translation-structure entries
> except for 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.
>
> Remove two caching mode checks before device TLB invalidation in the driver.
> The removal of these checks doesn't change the driver's behavior in critical
> map/unmap paths. Hence, there is no functionality or performance impact,
> especially since commit <29b32839725f> ("iommu/vt-d:
> Do not use flush-queue when caching-mode is on") has already disabled
> flush-queue for caching mode. Therefore, caching mode will never call
> intel_flush_iotlb_all().
The current logic is if the caching mode is being used and a domain isn't using first level I/O page table, then flush-queue won't be used. Otherwise, the flush-queue can be enabled.
See https://github.com/torvalds/linux/commit/257ec29074
In other words, if the caching mode is being used and a domain is using first level I/O page table, the flush-queue can be used for this domain to flush iotlb. Could the code change in this patch bring any performance impact to this case?
Regards,
-Tina
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 2:22 ` Zhang, Tina
@ 2024-04-15 2:43 ` Yi Liu
2024-04-15 5:00 ` Baolu Lu
0 siblings, 1 reply; 11+ messages in thread
From: Yi Liu @ 2024-04-15 2:43 UTC (permalink / raw
To: Zhang, Tina, Lu Baolu, iommu@lists.linux.dev
Cc: Tian, Kevin, Jacob Pan, Joerg Roedel, Will Deacon, Robin Murphy,
linux-kernel@vger.kernel.org
On 2024/4/15 10:22, Zhang, Tina wrote:
>
>
>> -----Original Message-----
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, April 15, 2024 9:39 AM
>> To: iommu@lists.linux.dev
>> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>; Jacob
>> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel <joro@8bytes.org>; Will
>> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
>> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
>> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
>> device TLB flush
>>
>> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
>> implementation caches not-present or erroneous translation-structure entries
>> except for 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.
>>
>> Remove two caching mode checks before device TLB invalidation in the driver.
>> The removal of these checks doesn't change the driver's behavior in critical
>> map/unmap paths. Hence, there is no functionality or performance impact,
>> especially since commit <29b32839725f> ("iommu/vt-d:
>> Do not use flush-queue when caching-mode is on") has already disabled
>> flush-queue for caching mode. Therefore, caching mode will never call
>> intel_flush_iotlb_all().
> The current logic is if the caching mode is being used and a domain isn't using first level I/O page table, then flush-queue won't be used. Otherwise, the flush-queue can be enabled.
> See https://github.com/torvalds/linux/commit/257ec29074
>
> In other words, if the caching mode is being used and a domain is using first level I/O page table, the flush-queue can be used for this domain to flush iotlb. Could the code change in this patch bring any performance impact to this case?
This seems to have performance deduction in the nested translation case.
The iommufd nested support bas been merged in 6.8, while the Qemu side
is wip. So this performance deduction does not happen until Qemu is
done. Should this also be considered as a performance regression? TBH.
I doubt if it should be.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 2:43 ` Yi Liu
@ 2024-04-15 5:00 ` Baolu Lu
2024-04-15 6:56 ` Yi Liu
0 siblings, 1 reply; 11+ messages in thread
From: Baolu Lu @ 2024-04-15 5:00 UTC (permalink / raw
To: Yi Liu, Zhang, Tina, iommu@lists.linux.dev
Cc: baolu.lu, Tian, Kevin, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel@vger.kernel.org
On 4/15/24 10:43 AM, Yi Liu wrote:
> On 2024/4/15 10:22, Zhang, Tina wrote:
>>
>>
>>> -----Original Message-----
>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>> Sent: Monday, April 15, 2024 9:39 AM
>>> To: iommu@lists.linux.dev
>>> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L
>>> <yi.l.liu@intel.com>; Jacob
>>> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel <joro@8bytes.org>;
>>> Will
>>> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
>>> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
>>> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
>>> device TLB flush
>>>
>>> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
>>> implementation caches not-present or erroneous translation-structure
>>> entries
>>> except for 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.
>>>
>>> Remove two caching mode checks before device TLB invalidation in the
>>> driver.
>>> The removal of these checks doesn't change the driver's behavior in
>>> critical
>>> map/unmap paths. Hence, there is no functionality or performance impact,
>>> especially since commit <29b32839725f> ("iommu/vt-d:
>>> Do not use flush-queue when caching-mode is on") has already disabled
>>> flush-queue for caching mode. Therefore, caching mode will never call
>>> intel_flush_iotlb_all().
>> The current logic is if the caching mode is being used and a domain
>> isn't using first level I/O page table, then flush-queue won't be
>> used. Otherwise, the flush-queue can be enabled.
>> See https://github.com/torvalds/linux/commit/257ec29074
>>
>> In other words, if the caching mode is being used and a domain is
>> using first level I/O page table, the flush-queue can be used for this
>> domain to flush iotlb. Could the code change in this patch bring any
>> performance impact to this case?
>
> This seems to have performance deduction in the nested translation case.
> The iommufd nested support bas been merged in 6.8, while the Qemu side
> is wip. So this performance deduction does not happen until Qemu is
> done. Should this also be considered as a performance regression? TBH.
Caching mode is irrelevant to first-stage and nesting translations. If
the QEMU implementation still relies on caching mode for nesting
support, it's already broken.
Best regards,
baolu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 5:00 ` Baolu Lu
@ 2024-04-15 6:56 ` Yi Liu
2024-04-16 3:00 ` Baolu Lu
0 siblings, 1 reply; 11+ messages in thread
From: Yi Liu @ 2024-04-15 6:56 UTC (permalink / raw
To: Baolu Lu, Zhang, Tina, iommu@lists.linux.dev
Cc: Tian, Kevin, Jacob Pan, Joerg Roedel, Will Deacon, Robin Murphy,
linux-kernel@vger.kernel.org
On 2024/4/15 13:00, Baolu Lu wrote:
> On 4/15/24 10:43 AM, Yi Liu wrote:
>> On 2024/4/15 10:22, Zhang, Tina wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, April 15, 2024 9:39 AM
>>>> To: iommu@lists.linux.dev
>>>> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L <yi.l.liu@intel.com>;
>>>> Jacob
>>>> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel <joro@8bytes.org>; Will
>>>> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
>>>> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
>>>> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
>>>> device TLB flush
>>>>
>>>> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
>>>> implementation caches not-present or erroneous translation-structure
>>>> entries
>>>> except for 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.
>>>>
>>>> Remove two caching mode checks before device TLB invalidation in the
>>>> driver.
>>>> The removal of these checks doesn't change the driver's behavior in
>>>> critical
>>>> map/unmap paths. Hence, there is no functionality or performance impact,
>>>> especially since commit <29b32839725f> ("iommu/vt-d:
>>>> Do not use flush-queue when caching-mode is on") has already disabled
>>>> flush-queue for caching mode. Therefore, caching mode will never call
>>>> intel_flush_iotlb_all().
>>> The current logic is if the caching mode is being used and a domain
>>> isn't using first level I/O page table, then flush-queue won't be used.
>>> Otherwise, the flush-queue can be enabled.
>>> See https://github.com/torvalds/linux/commit/257ec29074
>>>
>>> In other words, if the caching mode is being used and a domain is using
>>> first level I/O page table, the flush-queue can be used for this domain
>>> to flush iotlb. Could the code change in this patch bring any
>>> performance impact to this case?
>>
>> This seems to have performance deduction in the nested translation case.
>> The iommufd nested support bas been merged in 6.8, while the Qemu side
>> is wip. So this performance deduction does not happen until Qemu is
>> done. Should this also be considered as a performance regression? TBH.
>
> Caching mode is irrelevant to first-stage and nesting translations. If
> the QEMU implementation still relies on caching mode for nesting
> support, it's already broken.
For first-stage, yes, caching mode is irrelevant. But for nesting, still
be relevant. Because it relies on caching mode to capture the mofifications
of the pasid entry. Without caching mode, vIOMMU device model within QEMU
will miss the newly setup pasid entries, hence no way to setup nested
translation at all.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 1:38 [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
2024-04-15 2:22 ` Zhang, Tina
@ 2024-04-16 0:53 ` Ethan Zhao
2024-04-16 2:57 ` Baolu Lu
2024-04-24 3:41 ` Baolu Lu
2 siblings, 1 reply; 11+ messages in thread
From: Ethan Zhao @ 2024-04-16 0:53 UTC (permalink / raw
To: Lu Baolu, iommu
Cc: Kevin Tian, Yi Liu, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel
On 4/15/2024 9:38 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 for 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.
>
> Remove two caching mode checks before device TLB invalidation in the
> driver. The removal of these checks doesn't change the driver's behavior
> in critical map/unmap paths. Hence, there is no functionality or
> performance impact, especially since commit <29b32839725f> ("iommu/vt-d:
> Do not use flush-queue when caching-mode is on") has already disabled
> flush-queue for caching mode. Therefore, caching mode will never call
> intel_flush_iotlb_all().
>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> ---
> drivers/iommu/intel/iommu.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> Change log:
> v3:
> - It turned out that the removals don't change the driver's behavior,
> hence change it from a fix patch to a cleanup one.
> - No functionality changes.
> v2: https://lore.kernel.org/lkml/20240410055823.264501-1-baolu.lu@linux.intel.com/
> - Squash two patches into a single one.
> - No functionality changes.
> v1: https://lore.kernel.org/linux-iommu/20240407144232.190355-1-baolu.lu@linux.intel.com/
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index a7ecd90303dc..f0a67e9d9faf 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);
> }
Given devTLB flushing is irrelavent to CM, put iommu_flush_dev_iotlb()
in iommu_flush_iotlb_psi() and called with CM checking context is not
reasonable. the logic is buggy.
static void __mapping_notify_one(struct intel_iommu *iommu, struct dmar_domain *domain,
unsigned long pfn, unsigned int pages)
{
/*
* It's a non-present to present mapping. Only flush if caching mode
* and second level.
*/
if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
else
iommu_flush_write_buffer(iommu);
then how about fold all CM checking logic in iommu_flush_iotlb_psi()
or speperate iommu_flush_dev_iotlb() from iommu_flush_iotlb_psi() ?
Thanks,
Ethan
>
> @@ -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)
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-16 0:53 ` Ethan Zhao
@ 2024-04-16 2:57 ` Baolu Lu
2024-04-16 6:30 ` Ethan Zhao
0 siblings, 1 reply; 11+ messages in thread
From: Baolu Lu @ 2024-04-16 2:57 UTC (permalink / raw
To: Ethan Zhao, iommu
Cc: baolu.lu, Kevin Tian, Yi Liu, Jacob Pan, Joerg Roedel,
Will Deacon, Robin Murphy, linux-kernel
On 4/16/24 8:53 AM, Ethan Zhao wrote:
> On 4/15/2024 9:38 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 for 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.
>>
>> Remove two caching mode checks before device TLB invalidation in the
>> driver. The removal of these checks doesn't change the driver's behavior
>> in critical map/unmap paths. Hence, there is no functionality or
>> performance impact, especially since commit <29b32839725f> ("iommu/vt-d:
>> Do not use flush-queue when caching-mode is on") has already disabled
>> flush-queue for caching mode. Therefore, caching mode will never call
>> intel_flush_iotlb_all().
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>> ---
>> drivers/iommu/intel/iommu.c | 9 ++-------
>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> Change log:
>> v3:
>> - It turned out that the removals don't change the driver's behavior,
>> hence change it from a fix patch to a cleanup one.
>> - No functionality changes.
>> v2:
>> https://lore.kernel.org/lkml/20240410055823.264501-1-baolu.lu@linux.intel.com/
>> - Squash two patches into a single one.
>> - No functionality changes.
>> v1:
>> https://lore.kernel.org/linux-iommu/20240407144232.190355-1-baolu.lu@linux.intel.com/
>>
>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>> index a7ecd90303dc..f0a67e9d9faf 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);
>> }
>
> Given devTLB flushing is irrelavent to CM, put iommu_flush_dev_iotlb()
> in iommu_flush_iotlb_psi() and called with CM checking context is not
> reasonable. the logic is buggy.
>
> static void __mapping_notify_one(struct intel_iommu *iommu, struct
> dmar_domain *domain,
> unsigned long pfn, unsigned int pages)
> {
> /*
> * It's a non-present to present mapping. Only flush if caching mode
> * and second level.
> */
> if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
> iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
> else
> iommu_flush_write_buffer(iommu);
>
>
> then how about fold all CM checking logic in iommu_flush_iotlb_psi()
> or speperate iommu_flush_dev_iotlb() from iommu_flush_iotlb_psi() ?
I am refactoring the code with a new series.
https://lore.kernel.org/linux-iommu/20240410020844.253535-1-baolu.lu@linux.intel.com/
Best regards,
baolu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 6:56 ` Yi Liu
@ 2024-04-16 3:00 ` Baolu Lu
2024-04-17 3:40 ` Yi Liu
0 siblings, 1 reply; 11+ messages in thread
From: Baolu Lu @ 2024-04-16 3:00 UTC (permalink / raw
To: Yi Liu, Zhang, Tina, iommu@lists.linux.dev
Cc: baolu.lu, Tian, Kevin, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel@vger.kernel.org
On 4/15/24 2:56 PM, Yi Liu wrote:
> On 2024/4/15 13:00, Baolu Lu wrote:
>> On 4/15/24 10:43 AM, Yi Liu wrote:
>>> On 2024/4/15 10:22, Zhang, Tina wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>> Sent: Monday, April 15, 2024 9:39 AM
>>>>> To: iommu@lists.linux.dev
>>>>> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L
>>>>> <yi.l.liu@intel.com>; Jacob
>>>>> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel
>>>>> <joro@8bytes.org>; Will
>>>>> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
>>>>> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
>>>>> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
>>>>> device TLB flush
>>>>>
>>>>> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
>>>>> implementation caches not-present or erroneous
>>>>> translation-structure entries
>>>>> except for 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.
>>>>>
>>>>> Remove two caching mode checks before device TLB invalidation in
>>>>> the driver.
>>>>> The removal of these checks doesn't change the driver's behavior in
>>>>> critical
>>>>> map/unmap paths. Hence, there is no functionality or performance
>>>>> impact,
>>>>> especially since commit <29b32839725f> ("iommu/vt-d:
>>>>> Do not use flush-queue when caching-mode is on") has already disabled
>>>>> flush-queue for caching mode. Therefore, caching mode will never call
>>>>> intel_flush_iotlb_all().
>>>> The current logic is if the caching mode is being used and a domain
>>>> isn't using first level I/O page table, then flush-queue won't be
>>>> used. Otherwise, the flush-queue can be enabled.
>>>> See https://github.com/torvalds/linux/commit/257ec29074
>>>>
>>>> In other words, if the caching mode is being used and a domain is
>>>> using first level I/O page table, the flush-queue can be used for
>>>> this domain to flush iotlb. Could the code change in this patch
>>>> bring any performance impact to this case?
>>>
>>> This seems to have performance deduction in the nested translation case.
>>> The iommufd nested support bas been merged in 6.8, while the Qemu side
>>> is wip. So this performance deduction does not happen until Qemu is
>>> done. Should this also be considered as a performance regression? TBH.
>>
>> Caching mode is irrelevant to first-stage and nesting translations. If
>> the QEMU implementation still relies on caching mode for nesting
>> support, it's already broken.
>
> For first-stage, yes, caching mode is irrelevant. But for nesting, still
> be relevant. Because it relies on caching mode to capture the mofifications
> of the pasid entry. Without caching mode, vIOMMU device model within
> QEMU will miss the newly setup pasid entries, hence no way to setup nested
> translation at all.
Okay, it's reasonable that nesting translation still uses second-stage.
But the removed checks in this patch don't impact the pasid table change
capture, right?
Best regards,
baolu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-16 2:57 ` Baolu Lu
@ 2024-04-16 6:30 ` Ethan Zhao
0 siblings, 0 replies; 11+ messages in thread
From: Ethan Zhao @ 2024-04-16 6:30 UTC (permalink / raw
To: Baolu Lu, iommu
Cc: Kevin Tian, Yi Liu, Jacob Pan, Joerg Roedel, Will Deacon,
Robin Murphy, linux-kernel
On 4/16/2024 10:57 AM, Baolu Lu wrote:
> On 4/16/24 8:53 AM, Ethan Zhao wrote:
>> On 4/15/2024 9:38 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 for 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.
>>>
>>> Remove two caching mode checks before device TLB invalidation in the
>>> driver. The removal of these checks doesn't change the driver's
>>> behavior
>>> in critical map/unmap paths. Hence, there is no functionality or
>>> performance impact, especially since commit <29b32839725f>
>>> ("iommu/vt-d:
>>> Do not use flush-queue when caching-mode is on") has already disabled
>>> flush-queue for caching mode. Therefore, caching mode will never call
>>> intel_flush_iotlb_all().
>>>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>> ---
>>> drivers/iommu/intel/iommu.c | 9 ++-------
>>> 1 file changed, 2 insertions(+), 7 deletions(-)
>>>
>>> Change log:
>>> v3:
>>> - It turned out that the removals don't change the driver's behavior,
>>> hence change it from a fix patch to a cleanup one.
>>> - No functionality changes.
>>> v2:
>>> https://lore.kernel.org/lkml/20240410055823.264501-1-baolu.lu@linux.intel.com/
>>> - Squash two patches into a single one.
>>> - No functionality changes.
>>> v1:
>>> https://lore.kernel.org/linux-iommu/20240407144232.190355-1-baolu.lu@linux.intel.com/
>>>
>>> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
>>> index a7ecd90303dc..f0a67e9d9faf 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);
>>> }
>>
>> Given devTLB flushing is irrelavent to CM, put iommu_flush_dev_iotlb()
>> in iommu_flush_iotlb_psi() and called with CM checking context is not
>> reasonable. the logic is buggy.
>>
>> static void __mapping_notify_one(struct intel_iommu *iommu, struct
>> dmar_domain *domain,
>> unsigned long pfn, unsigned int pages)
>> {
>> /*
>> * It's a non-present to present mapping. Only flush if caching
>> mode
>> * and second level.
>> */
>> if (cap_caching_mode(iommu->cap) && !domain->use_first_level)
>> iommu_flush_iotlb_psi(iommu, domain, pfn, pages, 0, 1);
>> else
>> iommu_flush_write_buffer(iommu);
>>
>>
>> then how about fold all CM checking logic in iommu_flush_iotlb_psi()
>> or speperate iommu_flush_dev_iotlb() from iommu_flush_iotlb_psi() ?
>
> I am refactoring the code with a new series.
>
> https://lore.kernel.org/linux-iommu/20240410020844.253535-1-baolu.lu@linux.intel.com/
>
Great, thx.
>
> Best regards,
> baolu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-16 3:00 ` Baolu Lu
@ 2024-04-17 3:40 ` Yi Liu
0 siblings, 0 replies; 11+ messages in thread
From: Yi Liu @ 2024-04-17 3:40 UTC (permalink / raw
To: Baolu Lu, Zhang, Tina, iommu@lists.linux.dev
Cc: Tian, Kevin, Jacob Pan, Joerg Roedel, Will Deacon, Robin Murphy,
linux-kernel@vger.kernel.org
On 2024/4/16 11:00, Baolu Lu wrote:
> On 4/15/24 2:56 PM, Yi Liu wrote:
>> On 2024/4/15 13:00, Baolu Lu wrote:
>>> On 4/15/24 10:43 AM, Yi Liu wrote:
>>>> On 2024/4/15 10:22, Zhang, Tina wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Sent: Monday, April 15, 2024 9:39 AM
>>>>>> To: iommu@lists.linux.dev
>>>>>> Cc: Tian, Kevin <kevin.tian@intel.com>; Liu, Yi L
>>>>>> <yi.l.liu@intel.com>; Jacob
>>>>>> Pan <jacob.jun.pan@linux.intel.com>; Joerg Roedel <joro@8bytes.org>;
>>>>>> Will
>>>>>> Deacon <will@kernel.org>; Robin Murphy <robin.murphy@arm.com>; linux-
>>>>>> kernel@vger.kernel.org; Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Subject: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before
>>>>>> device TLB flush
>>>>>>
>>>>>> The Caching Mode (CM) of the Intel IOMMU indicates if the hardware
>>>>>> implementation caches not-present or erroneous translation-structure
>>>>>> entries
>>>>>> except for 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.
>>>>>>
>>>>>> Remove two caching mode checks before device TLB invalidation in the
>>>>>> driver.
>>>>>> The removal of these checks doesn't change the driver's behavior in
>>>>>> critical
>>>>>> map/unmap paths. Hence, there is no functionality or performance impact,
>>>>>> especially since commit <29b32839725f> ("iommu/vt-d:
>>>>>> Do not use flush-queue when caching-mode is on") has already disabled
>>>>>> flush-queue for caching mode. Therefore, caching mode will never call
>>>>>> intel_flush_iotlb_all().
>>>>> The current logic is if the caching mode is being used and a domain
>>>>> isn't using first level I/O page table, then flush-queue won't be
>>>>> used. Otherwise, the flush-queue can be enabled.
>>>>> See https://github.com/torvalds/linux/commit/257ec29074
>>>>>
>>>>> In other words, if the caching mode is being used and a domain is
>>>>> using first level I/O page table, the flush-queue can be used for this
>>>>> domain to flush iotlb. Could the code change in this patch bring any
>>>>> performance impact to this case?
>>>>
>>>> This seems to have performance deduction in the nested translation case.
>>>> The iommufd nested support bas been merged in 6.8, while the Qemu side
>>>> is wip. So this performance deduction does not happen until Qemu is
>>>> done. Should this also be considered as a performance regression? TBH.
>>>
>>> Caching mode is irrelevant to first-stage and nesting translations. If
>>> the QEMU implementation still relies on caching mode for nesting
>>> support, it's already broken.
>>
>> For first-stage, yes, caching mode is irrelevant. But for nesting, still
>> be relevant. Because it relies on caching mode to capture the mofifications
>> of the pasid entry. Without caching mode, vIOMMU device model within QEMU
>> will miss the newly setup pasid entries, hence no way to setup nested
>> translation at all.
>
> Okay, it's reasonable that nesting translation still uses second-stage.
the key point is not nesting uses second-stage, it is because of the guest
pasid entry capturing :)
> But the removed checks in this patch don't impact the pasid table change
> capture, right?
yes. The removal in this patch is irrelevant to it.
So Tina's concern makes sense. But I don't know if it should be considered
as a performance regression since nesting is not really fully ready (QEMU
side is not there yet).
BTW. I'm not sure if you have considered to correct the other caching mode
checks before dev-TLB invalidation. e.g. the CM check in
drivers/iommu/intel/pasid.c.
--
Regards,
Yi Liu
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush
2024-04-15 1:38 [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
2024-04-15 2:22 ` Zhang, Tina
2024-04-16 0:53 ` Ethan Zhao
@ 2024-04-24 3:41 ` Baolu Lu
2 siblings, 0 replies; 11+ messages in thread
From: Baolu Lu @ 2024-04-24 3:41 UTC (permalink / raw
To: iommu
Cc: baolu.lu, Kevin Tian, Yi Liu, Jacob Pan, Joerg Roedel,
Will Deacon, Robin Murphy, linux-kernel
On 4/15/24 9:38 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 for 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.
>
> Remove two caching mode checks before device TLB invalidation in the
> driver. The removal of these checks doesn't change the driver's behavior
> in critical map/unmap paths. Hence, there is no functionality or
> performance impact, especially since commit <29b32839725f> ("iommu/vt-d:
> Do not use flush-queue when caching-mode is on") has already disabled
> flush-queue for caching mode. Therefore, caching mode will never call
> intel_flush_iotlb_all().
>
> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> Reviewed-by: Kevin Tian<kevin.tian@intel.com>
> ---
> drivers/iommu/intel/iommu.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
Patch has been queued for iommu/vt-d.
Best regards,
baolu
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-04-24 3:42 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-15 1:38 [PATCH v3 1/1] iommu/vt-d: Remove caching mode check before device TLB flush Lu Baolu
2024-04-15 2:22 ` Zhang, Tina
2024-04-15 2:43 ` Yi Liu
2024-04-15 5:00 ` Baolu Lu
2024-04-15 6:56 ` Yi Liu
2024-04-16 3:00 ` Baolu Lu
2024-04-17 3:40 ` Yi Liu
2024-04-16 0:53 ` Ethan Zhao
2024-04-16 2:57 ` Baolu Lu
2024-04-16 6:30 ` Ethan Zhao
2024-04-24 3:41 ` Baolu Lu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).