The intel_context_flush_present() is called in places where either the
scalable mode is disabled, or scalable mode is enabled but all PASID
entries are known to be non-present. In these cases, the flush_domains
path within intel_context_flush_present() will never execute. This dead
code is therefore removed.
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
drivers/iommu/intel/iommu.c | 2 +-
drivers/iommu/intel/iommu.h | 3 +--
drivers/iommu/intel/pasid.c | 39 ++++++-------------------------------
3 files changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 91d49e2cea34..1d564240c977 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1730,7 +1730,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
context_clear_entry(context);
__iommu_flush_cache(iommu, context, sizeof(*context));
spin_unlock(&iommu->lock);
- intel_context_flush_present(info, context, did, true);
+ intel_context_flush_present(info, context, did);
}
int __domain_setup_first_level(struct intel_iommu *iommu,
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index f7d78cf0778c..754f6d7ade26 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1306,8 +1306,7 @@ void cache_tag_flush_range_np(struct dmar_domain *domain, unsigned long start,
unsigned long end);
void intel_context_flush_present(struct device_domain_info *info,
- struct context_entry *context,
- u16 did, bool affect_domains);
+ struct context_entry *context, u16 did);
int intel_iommu_enable_prq(struct intel_iommu *iommu);
int intel_iommu_finish_prq(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index c2742e256552..a2c6be624dbf 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -932,7 +932,7 @@ static void device_pasid_table_teardown(struct device *dev, u8 bus, u8 devfn)
context_clear_entry(context);
__iommu_flush_cache(iommu, context, sizeof(*context));
spin_unlock(&iommu->lock);
- intel_context_flush_present(info, context, did, false);
+ intel_context_flush_present(info, context, did);
}
static int pci_pasid_table_teardown(struct pci_dev *pdev, u16 alias, void *data)
@@ -1119,17 +1119,15 @@ static void __context_flush_dev_iotlb(struct device_domain_info *info)
/*
* Cache invalidations after change in a context table entry that was present
- * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations). If
- * IOMMU is in scalable mode and all PASID table entries of the device were
- * non-present, set flush_domains to false. Otherwise, true.
+ * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations).
+ * This helper can only be used when IOMMU is working in the legacy mode or
+ * IOMMU is in scalable mode but all PASID table entries of the device are
+ * non-present.
*/
void intel_context_flush_present(struct device_domain_info *info,
- struct context_entry *context,
- u16 did, bool flush_domains)
+ struct context_entry *context, u16 did)
{
struct intel_iommu *iommu = info->iommu;
- struct pasid_entry *pte;
- int i;
/*
* Device-selective context-cache invalidation. The Domain-ID field
@@ -1152,30 +1150,5 @@ void intel_context_flush_present(struct device_domain_info *info,
return;
}
- /*
- * For scalable mode:
- * - Domain-selective PASID-cache invalidation to affected domains
- * - Domain-selective IOTLB invalidation to affected domains
- * - Global Device-TLB invalidation to affected functions
- */
- if (flush_domains) {
- /*
- * If the IOMMU is running in scalable mode and there might
- * be potential PASID translations, the caller should hold
- * the lock to ensure that context changes and cache flushes
- * are atomic.
- */
- assert_spin_locked(&iommu->lock);
- for (i = 0; i < info->pasid_table->max_pasid; i++) {
- pte = intel_pasid_get_entry(info->dev, i);
- if (!pte || !pasid_pte_is_present(pte))
- continue;
-
- did = pasid_get_domain_id(pte);
- qi_flush_pasid_cache(iommu, did, QI_PC_ALL_PASIDS, 0);
- iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
- }
- }
-
__context_flush_dev_iotlb(info);
}
--
2.43.0Hi Baolu,
On 2025/2/24 13:16, Lu Baolu wrote:
> The intel_context_flush_present() is called in places where either the
> scalable mode is disabled, or scalable mode is enabled but all PASID
> entries are known to be non-present. In these cases, the flush_domains
> path within intel_context_flush_present() will never execute. This dead
> code is therefore removed.
The reason for this path is the remaining caller of
intel_context_flush_present() is only the domain_context_clear_one() which
is called in legacy mode path. Is it?
If so, it seems unnecessary to keep __context_flush_dev_iotlb(info); in the
end of the new intel_context_flush_present(). Also, since this helper is
more for legacy mode, might be good to move it out of pasid.c.:)
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
> drivers/iommu/intel/iommu.c | 2 +-
> drivers/iommu/intel/iommu.h | 3 +--
> drivers/iommu/intel/pasid.c | 39 ++++++-------------------------------
> 3 files changed, 8 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 91d49e2cea34..1d564240c977 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -1730,7 +1730,7 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
> context_clear_entry(context);
> __iommu_flush_cache(iommu, context, sizeof(*context));
> spin_unlock(&iommu->lock);
> - intel_context_flush_present(info, context, did, true);
> + intel_context_flush_present(info, context, did);
> }
>
> int __domain_setup_first_level(struct intel_iommu *iommu,
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index f7d78cf0778c..754f6d7ade26 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -1306,8 +1306,7 @@ void cache_tag_flush_range_np(struct dmar_domain *domain, unsigned long start,
> unsigned long end);
>
> void intel_context_flush_present(struct device_domain_info *info,
> - struct context_entry *context,
> - u16 did, bool affect_domains);
> + struct context_entry *context, u16 did);
>
> int intel_iommu_enable_prq(struct intel_iommu *iommu);
> int intel_iommu_finish_prq(struct intel_iommu *iommu);
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index c2742e256552..a2c6be624dbf 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -932,7 +932,7 @@ static void device_pasid_table_teardown(struct device *dev, u8 bus, u8 devfn)
> context_clear_entry(context);
> __iommu_flush_cache(iommu, context, sizeof(*context));
> spin_unlock(&iommu->lock);
> - intel_context_flush_present(info, context, did, false);
> + intel_context_flush_present(info, context, did);
> }
>
> static int pci_pasid_table_teardown(struct pci_dev *pdev, u16 alias, void *data)
> @@ -1119,17 +1119,15 @@ static void __context_flush_dev_iotlb(struct device_domain_info *info)
>
> /*
> * Cache invalidations after change in a context table entry that was present
> - * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations). If
> - * IOMMU is in scalable mode and all PASID table entries of the device were
> - * non-present, set flush_domains to false. Otherwise, true.
> + * according to the Spec 6.5.3.3 (Guidance to Software for Invalidations).
> + * This helper can only be used when IOMMU is working in the legacy mode or
> + * IOMMU is in scalable mode but all PASID table entries of the device are
> + * non-present.
> */
> void intel_context_flush_present(struct device_domain_info *info,
> - struct context_entry *context,
> - u16 did, bool flush_domains)
> + struct context_entry *context, u16 did)
> {
> struct intel_iommu *iommu = info->iommu;
> - struct pasid_entry *pte;
> - int i;
>
> /*
> * Device-selective context-cache invalidation. The Domain-ID field
> @@ -1152,30 +1150,5 @@ void intel_context_flush_present(struct device_domain_info *info,
> return;
> }
>
> - /*
> - * For scalable mode:
> - * - Domain-selective PASID-cache invalidation to affected domains
> - * - Domain-selective IOTLB invalidation to affected domains
> - * - Global Device-TLB invalidation to affected functions
> - */
> - if (flush_domains) {
> - /*
> - * If the IOMMU is running in scalable mode and there might
> - * be potential PASID translations, the caller should hold
> - * the lock to ensure that context changes and cache flushes
> - * are atomic.
> - */
> - assert_spin_locked(&iommu->lock);
> - for (i = 0; i < info->pasid_table->max_pasid; i++) {
> - pte = intel_pasid_get_entry(info->dev, i);
> - if (!pte || !pasid_pte_is_present(pte))
> - continue;
> -
> - did = pasid_get_domain_id(pte);
> - qi_flush_pasid_cache(iommu, did, QI_PC_ALL_PASIDS, 0);
> - iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
> - }
> - }
> -
> __context_flush_dev_iotlb(info);
> }
--
Regards,
Yi Liu
On 3/4/25 16:43, Yi Liu wrote: > On 2025/2/24 13:16, Lu Baolu wrote: >> The intel_context_flush_present() is called in places where either the >> scalable mode is disabled, or scalable mode is enabled but all PASID >> entries are known to be non-present. In these cases, the flush_domains >> path within intel_context_flush_present() will never execute. This dead >> code is therefore removed. > > The reason for this path is the remaining caller of > intel_context_flush_present() is only the domain_context_clear_one() which > is called in legacy mode path. Is it? > If so, it seems unnecessary to keep __context_flush_dev_iotlb(info); in the > end of the new intel_context_flush_present(). Also, since this helper is > more for legacy mode, might be good to move it out of pasid.c.:) This helper is for invalidating various caches when a context entry is present and certain fields are changed. It is used in both legacy and scalable modes. In the past, this helper would work even if some PASIDs were still in use. After the changes introduced in this series, this PASID-in-use case is removed. So remove the dead code. Thanks, baolu
On 2025/3/5 10:21, Baolu Lu wrote: > On 3/4/25 16:43, Yi Liu wrote: >> On 2025/2/24 13:16, Lu Baolu wrote: >>> The intel_context_flush_present() is called in places where either the >>> scalable mode is disabled, or scalable mode is enabled but all PASID >>> entries are known to be non-present. In these cases, the flush_domains >>> path within intel_context_flush_present() will never execute. This dead >>> code is therefore removed. >> >> The reason for this path is the remaining caller of >> intel_context_flush_present() is only the domain_context_clear_one() which >> is called in legacy mode path. Is it? >> If so, it seems unnecessary to keep __context_flush_dev_iotlb(info); in the >> end of the new intel_context_flush_present(). Also, since this helper is >> more for legacy mode, might be good to move it out of pasid.c.:) > > This helper is for invalidating various caches when a context entry is > present and certain fields are changed. It is used in both legacy and > scalable modes. hmmm. the kdoc says all the pasid entries are non-present, is it necessary to flush dev_tlb in such scenario? I suppose no present pasid entry means no pagetable as well. > In the past, this helper would work even if some PASIDs were still in > use. After the changes introduced in this series, this PASID-in-use case > is removed. So remove the dead code. yeah, I got this part. As I mentioned, the only caller now is the domain_context_clear_one() which is used in the legacy path. That's why I feel like it is now more for legacy mode path now after this series. -- Regards, Yi Liu
On 3/5/25 11:34, Yi Liu wrote: > On 2025/3/5 10:21, Baolu Lu wrote: >> On 3/4/25 16:43, Yi Liu wrote: >>> On 2025/2/24 13:16, Lu Baolu wrote: >>>> The intel_context_flush_present() is called in places where either the >>>> scalable mode is disabled, or scalable mode is enabled but all PASID >>>> entries are known to be non-present. In these cases, the flush_domains >>>> path within intel_context_flush_present() will never execute. This dead >>>> code is therefore removed. >>> >>> The reason for this path is the remaining caller of >>> intel_context_flush_present() is only the domain_context_clear_one() >>> which >>> is called in legacy mode path. Is it? >>> If so, it seems unnecessary to keep __context_flush_dev_iotlb(info); >>> in the >>> end of the new intel_context_flush_present(). Also, since this helper >>> is more for legacy mode, might be good to move it out of pasid.c.:) >> >> This helper is for invalidating various caches when a context entry is >> present and certain fields are changed. It is used in both legacy and >> scalable modes. > > hmmm. the kdoc says all the pasid entries are non-present, is it necessary > to flush dev_tlb in such scenario? I suppose no present pasid entry means > no pagetable as well. The spec has defined the software behavior for cache invalidation in "Table 28: Guidance to Software for Invalidations". This helper was written according to it. > >> In the past, this helper would work even if some PASIDs were still in >> use. After the changes introduced in this series, this PASID-in-use case >> is removed. So remove the dead code. > > yeah, I got this part. As I mentioned, the only caller now is the > domain_context_clear_one() which is used in the legacy path. That's why I > feel like it is now more for legacy mode path now after this series. It's also called by device_pasid_table_teardown(). Thanks, baolu
> From: Lu Baolu <baolu.lu@linux.intel.com> > Sent: Monday, February 24, 2025 1:16 PM > > The intel_context_flush_present() is called in places where either the > scalable mode is disabled, or scalable mode is enabled but all PASID > entries are known to be non-present. In these cases, the flush_domains > path within intel_context_flush_present() will never execute. This dead > code is therefore removed. > > Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com> usually a suffix "_present()" indicates that the helper can be called on an object which is currently in-use, which is obviously not the case here. To avoid confusion probably just call it intel_context_flush() or intel_context_flush_no_user() is clearer. Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On 2/25/25 15:43, Tian, Kevin wrote: >> From: Lu Baolu<baolu.lu@linux.intel.com> >> Sent: Monday, February 24, 2025 1:16 PM >> >> The intel_context_flush_present() is called in places where either the >> scalable mode is disabled, or scalable mode is enabled but all PASID >> entries are known to be non-present. In these cases, the flush_domains >> path within intel_context_flush_present() will never execute. This dead >> code is therefore removed. >> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> > usually a suffix "_present()" indicates that the helper can be called > on an object which is currently in-use, which is obviously not the > case here. > > To avoid confusion probably just call it intel_context_flush() or > intel_context_flush_no_user() is clearer. How about intel_context_flush_no_pasid()?
> From: Baolu Lu <baolu.lu@linux.intel.com> > Sent: Wednesday, February 26, 2025 11:58 AM > > On 2/25/25 15:43, Tian, Kevin wrote: > >> From: Lu Baolu<baolu.lu@linux.intel.com> > >> Sent: Monday, February 24, 2025 1:16 PM > >> > >> The intel_context_flush_present() is called in places where either the > >> scalable mode is disabled, or scalable mode is enabled but all PASID > >> entries are known to be non-present. In these cases, the flush_domains > >> path within intel_context_flush_present() will never execute. This dead > >> code is therefore removed. > >> > >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com> > > usually a suffix "_present()" indicates that the helper can be called > > on an object which is currently in-use, which is obviously not the > > case here. > > > > To avoid confusion probably just call it intel_context_flush() or > > intel_context_flush_no_user() is clearer. > > How about intel_context_flush_no_pasid()? yes
© 2016 - 2025 Red Hat, Inc.