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

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

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

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

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

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

> 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>

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

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

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

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

> 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. 😉

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