[PATCH v3 0/2] iommu/vt-d: Refactor PRI enable/disable steps

[PATCH v3 0/2] iommu/vt-d: Refactor PRI enable/disable steps

[Lu Baolu]

The page fault handling framework within the iommu core has defined the
PRI enable and disable flows in the comments for the
iopf_queue_remove_device() interface. This series aims to refactor the
PRI enable/disable steps in the Intel iommu driver to align with these
definitions.

Change log:
v3:
 - Refine the lock requirement. Only assert the lock when it is
   required.

v2:
 - https://lore.kernel.org/linux-iommu/20240627023121.50166-1-baolu.lu@linux.intel.com/
 - The cache invalidation for a context entry change should not affect
   the devices not related to the entry. Fix this by always using
   device-selective cache invalidation.
v1:
 - https://lore.kernel.org/linux-iommu/20240606034019.42795-1-baolu.lu@linux.intel.com/

Lu Baolu (2):
  iommu/vt-d: Add helper to flush caches for context change
  iommu/vt-d: Refactor PCI PRI enabling/disabling callbacks

 drivers/iommu/intel/iommu.h |  13 +++++
 drivers/iommu/intel/iommu.c |  89 +++++++++++++++++------------
 drivers/iommu/intel/pasid.c | 108 +++++++++++++++++++++++++++++-------
 3 files changed, 153 insertions(+), 57 deletions(-)

-- 
2.34.1

[PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Lu Baolu]

This helper is used to flush the related caches following a change in a
context table entry that was previously present. The VT-d specification
provides guidance for such invalidations in section 6.5.3.3.

This helper replaces the existing open code in the code paths where a
present context entry is being torn down.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.h |   4 ++
 drivers/iommu/intel/iommu.c |  32 +----------
 drivers/iommu/intel/pasid.c | 106 +++++++++++++++++++++++++++++-------
 3 files changed, 92 insertions(+), 50 deletions(-)

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 9a3b064126de..63eb3306c025 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1143,6 +1143,10 @@ void cache_tag_flush_all(struct dmar_domain *domain);
 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,
+				 bool affect_domains);
+
 #ifdef CONFIG_INTEL_IOMMU_SVM
 void intel_svm_check(struct intel_iommu *iommu);
 int intel_svm_enable_prq(struct intel_iommu *iommu);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 1f0d6892a0b6..e84b0fdca107 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -1359,21 +1359,6 @@ static void iommu_disable_pci_caps(struct device_domain_info *info)
 	}
 }
 
-static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
-				    u64 addr, unsigned int mask)
-{
-	u16 sid, qdep;
-
-	if (!info || !info->ats_enabled)
-		return;
-
-	sid = info->bus << 8 | info->devfn;
-	qdep = info->ats_qdep;
-	qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
-			   qdep, addr, mask);
-	quirk_extra_dev_tlb_flush(info, addr, mask, IOMMU_NO_PASID, qdep);
-}
-
 static void intel_flush_iotlb_all(struct iommu_domain *domain)
 {
 	cache_tag_flush_all(to_dmar_domain(domain));
@@ -1959,7 +1944,6 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 {
 	struct intel_iommu *iommu = info->iommu;
 	struct context_entry *context;
-	u16 did_old;
 
 	spin_lock(&iommu->lock);
 	context = iommu_context_addr(iommu, bus, devfn, 0);
@@ -1968,24 +1952,10 @@ static void domain_context_clear_one(struct device_domain_info *info, u8 bus, u8
 		return;
 	}
 
-	did_old = context_domain_id(context);
-
 	context_clear_entry(context);
 	__iommu_flush_cache(iommu, context, sizeof(*context));
 	spin_unlock(&iommu->lock);
-	iommu->flush.flush_context(iommu,
-				   did_old,
-				   (((u16)bus) << 8) | devfn,
-				   DMA_CCMD_MASK_NOBIT,
-				   DMA_CCMD_DEVICE_INVL);
-
-	iommu->flush.flush_iotlb(iommu,
-				 did_old,
-				 0,
-				 0,
-				 DMA_TLB_DSI_FLUSH);
-
-	__iommu_flush_dev_iotlb(info, 0, MAX_AGAW_PFN_WIDTH);
+	intel_context_flush_present(info, context, true);
 }
 
 static int domain_setup_first_level(struct intel_iommu *iommu,
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index aabcdf756581..d6623d2c2050 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -691,25 +691,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);
-
-	/*
-	 * Cache invalidation for changes to a scalable-mode context table
-	 * entry.
-	 *
-	 * Section 6.5.3.3 of the VT-d spec:
-	 * - Device-selective context-cache invalidation;
-	 * - Domain-selective PASID-cache invalidation to affected domains
-	 *   (can be skipped if all PASID entries were not-present);
-	 * - Domain-selective IOTLB invalidation to affected domains;
-	 * - Global Device-TLB invalidation to affected functions.
-	 *
-	 * The iommu has been parked in the blocking state. All domains have
-	 * been detached from the device or PASID. The PASID and IOTLB caches
-	 * have been invalidated during the domain detach path.
-	 */
-	iommu->flush.flush_context(iommu, 0, PCI_DEVID(bus, devfn),
-				   DMA_CCMD_MASK_NOBIT, DMA_CCMD_DEVICE_INVL);
-	devtlb_invalidation_with_pasid(iommu, dev, IOMMU_NO_PASID);
+	intel_context_flush_present(info, context, false);
 }
 
 static int pci_pasid_table_teardown(struct pci_dev *pdev, u16 alias, void *data)
@@ -871,3 +853,89 @@ int intel_pasid_setup_sm_context(struct device *dev)
 
 	return pci_for_each_dma_alias(to_pci_dev(dev), pci_pasid_table_setup, dev);
 }
+
+/*
+ * Global Device-TLB invalidation following changes in a context entry which
+ * was present.
+ */
+static void __context_flush_dev_iotlb(struct device_domain_info *info)
+{
+	if (!info->ats_enabled)
+		return;
+
+	qi_flush_dev_iotlb(info->iommu, PCI_DEVID(info->bus, info->devfn),
+			   info->pfsid, info->ats_qdep, 0, MAX_AGAW_PFN_WIDTH);
+
+	/*
+	 * There is no guarantee that the device DMA is stopped when it reaches
+	 * here. Therefore, always attempt the extra device TLB invalidation
+	 * quirk. The impact on performance is acceptable since this is not a
+	 * performance-critical path.
+	 */
+	quirk_extra_dev_tlb_flush(info, 0, MAX_AGAW_PFN_WIDTH, IOMMU_NO_PASID,
+				  info->ats_qdep);
+}
+
+/*
+ * 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.
+ */
+void intel_context_flush_present(struct device_domain_info *info,
+				 struct context_entry *context,
+				 bool flush_domains)
+{
+	struct intel_iommu *iommu = info->iommu;
+	u16 did = context_domain_id(context);
+	struct pasid_entry *pte;
+	int i;
+
+	/*
+	 * Device-selective context-cache invalidation. The Domain-ID field
+	 * of the Context-cache Invalidate Descriptor is ignored by hardware
+	 * when operating in scalable mode. Therefore the @did value doesn't
+	 * matter in scalable mode.
+	 */
+	iommu->flush.flush_context(iommu, did, PCI_DEVID(info->bus, info->devfn),
+				   DMA_CCMD_MASK_NOBIT, DMA_CCMD_DEVICE_INVL);
+
+	/*
+	 * For legacy mode:
+	 * - Domain-selective IOTLB invalidation
+	 * - Global Device-TLB invalidation to all affected functions
+	 */
+	if (!sm_supported(iommu)) {
+		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
+		__context_flush_dev_iotlb(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.34.1

[PATCH v3 2/2] iommu/vt-d: Refactor PCI PRI enabling/disabling callbacks

[Lu Baolu]

Commit 0095bf83554f8 ("iommu: Improve iopf_queue_remove_device()")
specified the flow for disabling the PRI on a device. Refactor the
PRI callbacks in the intel iommu driver to better manage PRI
enabling and disabling and align it with the device queue interfaces
in the iommu core.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.h |  9 ++++++
 drivers/iommu/intel/iommu.c | 57 +++++++++++++++++++++++++++++++++----
 drivers/iommu/intel/pasid.c |  2 --
 3 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 63eb3306c025..b67c14da1240 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1045,6 +1045,15 @@ static inline void context_set_sm_pre(struct context_entry *context)
 	context->lo |= BIT_ULL(4);
 }
 
+/*
+ * Clear the PRE(Page Request Enable) field of a scalable mode context
+ * entry.
+ */
+static inline void context_clear_sm_pre(struct context_entry *context)
+{
+	context->lo &= ~BIT_ULL(4);
+}
+
 /* Returns a number of VTD pages, but aligned to MM page size */
 static inline unsigned long aligned_nrpages(unsigned long host_addr, size_t size)
 {
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e84b0fdca107..523407f6f6b2 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4244,6 +4244,37 @@ static int intel_iommu_enable_sva(struct device *dev)
 	return 0;
 }
 
+static int context_flip_pri(struct device_domain_info *info, bool enable)
+{
+	struct intel_iommu *iommu = info->iommu;
+	u8 bus = info->bus, devfn = info->devfn;
+	struct context_entry *context;
+
+	spin_lock(&iommu->lock);
+	if (context_copied(iommu, bus, devfn)) {
+		spin_unlock(&iommu->lock);
+		return -EINVAL;
+	}
+
+	context = iommu_context_addr(iommu, bus, devfn, false);
+	if (!context || !context_present(context)) {
+		spin_unlock(&iommu->lock);
+		return -ENODEV;
+	}
+
+	if (enable)
+		context_set_sm_pre(context);
+	else
+		context_clear_sm_pre(context);
+
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(context, sizeof(*context));
+	intel_context_flush_present(info, context, true);
+	spin_unlock(&iommu->lock);
+
+	return 0;
+}
+
 static int intel_iommu_enable_iopf(struct device *dev)
 {
 	struct pci_dev *pdev = dev_is_pci(dev) ? to_pci_dev(dev) : NULL;
@@ -4273,15 +4304,23 @@ static int intel_iommu_enable_iopf(struct device *dev)
 	if (ret)
 		return ret;
 
+	ret = context_flip_pri(info, true);
+	if (ret)
+		goto err_remove_device;
+
 	ret = pci_enable_pri(pdev, PRQ_DEPTH);
-	if (ret) {
-		iopf_queue_remove_device(iommu->iopf_queue, dev);
-		return ret;
-	}
+	if (ret)
+		goto err_clear_pri;
 
 	info->pri_enabled = 1;
 
 	return 0;
+err_clear_pri:
+	context_flip_pri(info, false);
+err_remove_device:
+	iopf_queue_remove_device(iommu->iopf_queue, dev);
+
+	return ret;
 }
 
 static int intel_iommu_disable_iopf(struct device *dev)
@@ -4292,6 +4331,15 @@ static int intel_iommu_disable_iopf(struct device *dev)
 	if (!info->pri_enabled)
 		return -EINVAL;
 
+	/* Disable new PRI reception: */
+	context_flip_pri(info, false);
+
+	/*
+	 * Remove device from fault queue and acknowledge all outstanding
+	 * PRQs to the device:
+	 */
+	iopf_queue_remove_device(iommu->iopf_queue, dev);
+
 	/*
 	 * PCIe spec states that by clearing PRI enable bit, the Page
 	 * Request Interface will not issue new page requests, but has
@@ -4302,7 +4350,6 @@ static int intel_iommu_disable_iopf(struct device *dev)
 	 */
 	pci_disable_pri(to_pci_dev(dev));
 	info->pri_enabled = 0;
-	iopf_queue_remove_device(iommu->iopf_queue, dev);
 
 	return 0;
 }
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index d6623d2c2050..9a7b5668c723 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -749,8 +749,6 @@ static int context_entry_set_pasid_table(struct context_entry *context,
 
 	if (info->ats_supported)
 		context_set_sm_dte(context);
-	if (info->pri_supported)
-		context_set_sm_pre(context);
 	if (info->pasid_supported)
 		context_set_pasid(context);
 
-- 
2.34.1

RE: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, July 1, 2024 7:23 PM
> +
> +	/*
> +	 * 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);
> +}

this only invalidates devtlb w/o PASID. We miss a pasid devtlb invalidation
with global bit set.

otherwise this looks good:

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

RE: [PATCH v3 2/2] iommu/vt-d: Refactor PCI PRI enabling/disabling callbacks

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, July 1, 2024 7:23 PM
> 
> Commit 0095bf83554f8 ("iommu: Improve iopf_queue_remove_device()")
> specified the flow for disabling the PRI on a device. Refactor the
> PRI callbacks in the intel iommu driver to better manage PRI
> enabling and disabling and align it with the device queue interfaces
> in the iommu core.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 7/2/24 9:11 AM, Tian, Kevin wrote:
>> From: Lu Baolu<baolu.lu@linux.intel.com>
>> Sent: Monday, July 1, 2024 7:23 PM
>> +
>> +	/*
>> +	 * 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);
>> +}
> this only invalidates devtlb w/o PASID. We miss a pasid devtlb invalidation
> with global bit set.

I am not sure about this. The spec says "Global Device-TLB invalidation
to affected functions", I am not sure whether this implies any PASID-
based-Device-TLB invalidation.

If so, perhaps we need a separated fix to address this.

Best regards,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 7/2/24 9:47 AM, Baolu Lu wrote:
> On 7/2/24 9:11 AM, Tian, Kevin wrote:
>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>> Sent: Monday, July 1, 2024 7:23 PM
>>> +
>>> +    /*
>>> +     * 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);
>>> +}
>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb 
>> invalidation
>> with global bit set.
> 
> I am not sure about this. The spec says "Global Device-TLB invalidation
> to affected functions", I am not sure whether this implies any PASID-
> based-Device-TLB invalidation.

I just revisited the spec, Device-TLB invalidation only covers caches
for requests-without-PASID. If pasid translation is affected while
updating the context entry, we should also take care of the caches for
requests-with-pasid.

I will add below line to address this.

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 9a7b5668c723..91db0876682e 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -932,6 +932,7 @@ void intel_context_flush_present(struct 
device_domain_info *info,
                         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);
+                       pasid_cache_invalidation_with_pasid(iommu, did, i);
                 }
         }

Thanks!

Best regards,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Jacob Pan]

On Mon,  1 Jul 2024 19:23:16 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> +	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;
Is it worth going through 1M PASIDs just to skip the PASID cache
invalidation? Or just do the flush on all used DIDs unconditionally.

> +			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);
> +		}
> +	}

Thanks,

Jacob

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 2024/7/2 12:41, Jacob Pan wrote:
> On Mon,  1 Jul 2024 19:23:16 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> +	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;
> Is it worth going through 1M PASIDs just to skip the PASID cache
> invalidation? Or just do the flush on all used DIDs unconditionally.

Currently we don't track all domains attached to a device. If such
optimization is necessary, perhaps we can add it later.

Best regards,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 2024/7/2 10:43, Baolu Lu wrote:
> On 7/2/24 9:47 AM, Baolu Lu wrote:
>> On 7/2/24 9:11 AM, Tian, Kevin wrote:
>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>> Sent: Monday, July 1, 2024 7:23 PM
>>>> +
>>>> +    /*
>>>> +     * 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);
>>>> +}
>>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb 
>>> invalidation
>>> with global bit set.
>>
>> I am not sure about this. The spec says "Global Device-TLB invalidation
>> to affected functions", I am not sure whether this implies any PASID-
>> based-Device-TLB invalidation.
> 
> I just revisited the spec, Device-TLB invalidation only covers caches
> for requests-without-PASID. If pasid translation is affected while
> updating the context entry, we should also take care of the caches for
> requests-with-pasid.
> 
> I will add below line to address this.
> 
> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> index 9a7b5668c723..91db0876682e 100644
> --- a/drivers/iommu/intel/pasid.c
> +++ b/drivers/iommu/intel/pasid.c
> @@ -932,6 +932,7 @@ void intel_context_flush_present(struct 
> device_domain_info *info,
>                          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);
> +                       pasid_cache_invalidation_with_pasid(iommu, did, i);

Should be

	devtlb_invalidation_with_pasid(iommu, info->dev, i);

Sorry for the typo.

Best regards,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Yi Liu]

On 2024/7/2 12:51, Baolu Lu wrote:
> On 2024/7/2 10:43, Baolu Lu wrote:
>> On 7/2/24 9:47 AM, Baolu Lu wrote:
>>> On 7/2/24 9:11 AM, Tian, Kevin wrote:
>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>>> Sent: Monday, July 1, 2024 7:23 PM
>>>>> +
>>>>> +    /*
>>>>> +     * 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);
>>>>> +}
>>>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb 
>>>> invalidation
>>>> with global bit set.
>>>
>>> I am not sure about this. The spec says "Global Device-TLB invalidation
>>> to affected functions", I am not sure whether this implies any PASID-
>>> based-Device-TLB invalidation.
>>
>> I just revisited the spec, Device-TLB invalidation only covers caches
>> for requests-without-PASID. If pasid translation is affected while
>> updating the context entry, we should also take care of the caches for
>> requests-with-pasid.
>>

hmmm. "Table 25. Guidance to Software for Invalidations" only mentions
global devTLB invalidation. 10.3.8 PASID and Global Invalidate of PCIe 6.2
spec has below definition.

For Invalidation Requests that do not have a PASID, the ATC shall:
• Invalidate ATC entries within the indicate memory range that were 
requested without a PASID value.
• Invalidate ATC entries at all addresses that were requested with any 
PASID value.

AFAIK. A devTLB invalidate descriptor submitted to VT-d should be converted
to be a PCIe ATC invalidation request without PASID prefix. So it may be
subjected to the above definition. If so, no need to have a PASID-based 
devTLB invalidation descriptor.

>> I will add below line to address this.
>>
>> diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
>> index 9a7b5668c723..91db0876682e 100644
>> --- a/drivers/iommu/intel/pasid.c
>> +++ b/drivers/iommu/intel/pasid.c
>> @@ -932,6 +932,7 @@ void intel_context_flush_present(struct 
>> device_domain_info *info,
>>                          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);
>> +                       pasid_cache_invalidation_with_pasid(iommu, did, i);
> 
> Should be
> 
>      devtlb_invalidation_with_pasid(iommu, info->dev, i);
> 
> Sorry for the typo.
> 
> Best regards,
> baolu
> 

-- 
Regards,
Yi Liu

RE: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Tian, Kevin]

> From: Liu, Yi L <yi.l.liu@intel.com>
> Sent: Tuesday, July 2, 2024 2:25 PM
> 
> On 2024/7/2 12:51, Baolu Lu wrote:
> > On 2024/7/2 10:43, Baolu Lu wrote:
> >> On 7/2/24 9:47 AM, Baolu Lu wrote:
> >>> On 7/2/24 9:11 AM, Tian, Kevin wrote:
> >>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
> >>>>> Sent: Monday, July 1, 2024 7:23 PM
> >>>>> +
> >>>>> +    /*
> >>>>> +     * 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);
> >>>>> +}
> >>>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb
> >>>> invalidation
> >>>> with global bit set.
> >>>
> >>> I am not sure about this. The spec says "Global Device-TLB invalidation
> >>> to affected functions", I am not sure whether this implies any PASID-
> >>> based-Device-TLB invalidation.
> >>
> >> I just revisited the spec, Device-TLB invalidation only covers caches
> >> for requests-without-PASID. If pasid translation is affected while
> >> updating the context entry, we should also take care of the caches for
> >> requests-with-pasid.
> >>
> 
> hmmm. "Table 25. Guidance to Software for Invalidations" only mentions
> global devTLB invalidation. 10.3.8 PASID and Global Invalidate of PCIe 6.2
> spec has below definition.
> 
> For Invalidation Requests that do not have a PASID, the ATC shall:
> • Invalidate ATC entries within the indicate memory range that were
> requested without a PASID value.
> • Invalidate ATC entries at all addresses that were requested with any
> PASID value.
> 
> AFAIK. A devTLB invalidate descriptor submitted to VT-d should be converted
> to be a PCIe ATC invalidation request without PASID prefix. So it may be
> subjected to the above definition. If so, no need to have a PASID-based
> devTLB invalidation descriptor.
> 

You are correct. The wording in VT-d spec is a bit confusing on saying
that devtlb invalidation descriptor is only for request w/o PASID. 😉

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 2024/7/2 14:39, Tian, Kevin wrote:
>> From: Liu, Yi L <yi.l.liu@intel.com>
>> Sent: Tuesday, July 2, 2024 2:25 PM
>>
>> On 2024/7/2 12:51, Baolu Lu wrote:
>>> On 2024/7/2 10:43, Baolu Lu wrote:
>>>> On 7/2/24 9:47 AM, Baolu Lu wrote:
>>>>> On 7/2/24 9:11 AM, Tian, Kevin wrote:
>>>>>>> From: Lu Baolu<baolu.lu@linux.intel.com>
>>>>>>> Sent: Monday, July 1, 2024 7:23 PM
>>>>>>> +
>>>>>>> +    /*
>>>>>>> +     * 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);
>>>>>>> +}
>>>>>> this only invalidates devtlb w/o PASID. We miss a pasid devtlb
>>>>>> invalidation
>>>>>> with global bit set.
>>>>>
>>>>> I am not sure about this. The spec says "Global Device-TLB invalidation
>>>>> to affected functions", I am not sure whether this implies any PASID-
>>>>> based-Device-TLB invalidation.
>>>>
>>>> I just revisited the spec, Device-TLB invalidation only covers caches
>>>> for requests-without-PASID. If pasid translation is affected while
>>>> updating the context entry, we should also take care of the caches for
>>>> requests-with-pasid.
>>>>
>>
>> hmmm. "Table 25. Guidance to Software for Invalidations" only mentions
>> global devTLB invalidation. 10.3.8 PASID and Global Invalidate of PCIe 6.2
>> spec has below definition.
>>
>> For Invalidation Requests that do not have a PASID, the ATC shall:
>> • Invalidate ATC entries within the indicate memory range that were
>> requested without a PASID value.
>> • Invalidate ATC entries at all addresses that were requested with any
>> PASID value.
>>
>> AFAIK. A devTLB invalidate descriptor submitted to VT-d should be converted
>> to be a PCIe ATC invalidation request without PASID prefix. So it may be
>> subjected to the above definition. If so, no need to have a PASID-based
>> devTLB invalidation descriptor.
>>
> 
> You are correct. The wording in VT-d spec is a bit confusing on saying
> that devtlb invalidation descriptor is only for request w/o PASID. 😉

Okay, so I will remove the line of pasid-based-dev-iotlb invalidation.

Thank you Yi, for the clarification.

Best regards,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Jacob Pan]

On Tue, 2 Jul 2024 12:43:41 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2024/7/2 12:41, Jacob Pan wrote:
> > On Mon,  1 Jul 2024 19:23:16 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> > wrote:
> >   
> >> +	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;  
> > Is it worth going through 1M PASIDs just to skip the PASID cache
> > invalidation? Or just do the flush on all used DIDs unconditionally.  
> 
> Currently we don't track all domains attached to a device. If such
> optimization is necessary, perhaps we can add it later.

I think it is necessary, because without tracking domain IDs, the code
above would have duplicated invalidations.
For example: a device PASID table has the following entries
	PASID	DomainID
-------------------------
	100	1
	200	1
	300	2
-------------------------
When a present context entry changes, we need to do:
qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);

With this code, we do
qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);//duplicated!
qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);

Thanks,

Jacob

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Baolu Lu]

On 7/2/24 11:57 PM, Jacob Pan wrote:
> On Tue, 2 Jul 2024 12:43:41 +0800, Baolu Lu<baolu.lu@linux.intel.com>
> wrote:
> 
>> On 2024/7/2 12:41, Jacob Pan wrote:
>>> On Mon,  1 Jul 2024 19:23:16 +0800, Lu Baolu<baolu.lu@linux.intel.com>
>>> wrote:
>>>    
>>>> +	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;
>>> Is it worth going through 1M PASIDs just to skip the PASID cache
>>> invalidation? Or just do the flush on all used DIDs unconditionally.
>> Currently we don't track all domains attached to a device. If such
>> optimization is necessary, perhaps we can add it later.
> I think it is necessary, because without tracking domain IDs, the code
> above would have duplicated invalidations.
> For example: a device PASID table has the following entries
> 	PASID	DomainID
> -------------------------
> 	100	1
> 	200	1
> 	300	2
> -------------------------
> When a present context entry changes, we need to do:
> qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
> qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);
> 
> With this code, we do
> qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
> qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);//duplicated!
> qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);

Yes, this is likely. But currently enabling and disabling PRI happens in
driver's probe and release paths. Therefore such duplicate is not so
critical.

For long term, I have a plan to abstract the domain id into an object so
that domains attached to different PASIDs of a device could share a
domain id. With that done, we could improve this code by iterating the
domain id objects for a device and performing cache invalidation
directly.

Thanks,
baolu

Re: [PATCH v3 1/2] iommu/vt-d: Add helper to flush caches for context change

[Jacob Pan]

On Wed, 3 Jul 2024 10:49:19 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 7/2/24 11:57 PM, Jacob Pan wrote:
> > On Tue, 2 Jul 2024 12:43:41 +0800, Baolu Lu<baolu.lu@linux.intel.com>
> > wrote:
> >   
> >> On 2024/7/2 12:41, Jacob Pan wrote:  
> >>> On Mon,  1 Jul 2024 19:23:16 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> >>> wrote:
> >>>      
> >>>> +	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;  
> >>> Is it worth going through 1M PASIDs just to skip the PASID cache
> >>> invalidation? Or just do the flush on all used DIDs unconditionally.  
> >> Currently we don't track all domains attached to a device. If such
> >> optimization is necessary, perhaps we can add it later.  
> > I think it is necessary, because without tracking domain IDs, the code
> > above would have duplicated invalidations.
> > For example: a device PASID table has the following entries
> > 	PASID	DomainID
> > -------------------------
> > 	100	1
> > 	200	1
> > 	300	2
> > -------------------------
> > When a present context entry changes, we need to do:
> > qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
> > qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);
> > 
> > With this code, we do
> > qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);
> > qi_flush_pasid_cache(iommu, 1, QI_PC_ALL_PASIDS, 0);//duplicated!
> > qi_flush_pasid_cache(iommu, 2, QI_PC_ALL_PASIDS, 0);  
> 
> Yes, this is likely. But currently enabling and disabling PRI happens in
> driver's probe and release paths. Therefore such duplicate is not so
> critical.
> 
> For long term, I have a plan to abstract the domain id into an object so
> that domains attached to different PASIDs of a device could share a
> domain id. With that done, we could improve this code by iterating the
> domain id objects for a device and performing cache invalidation
> directly.

Sounds good. It might be helpful to add a comment to clarify for others who
might wonder about the duplicates.

Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Thanks,

Jacob