Re: [PATCH 2/3] iommu/vt-d: Clear Present bit before tearing down PASID entry

[Baolu Lu <baolu.lu@linux.intel.com>]

On 1/14/26 15:32, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, January 13, 2026 11:01 AM
>>
>> The Intel VT-d Scalable Mode PASID table entry consists of 512 bits (64
>> bytes). When tearing down an entry, the current implementation zeros the
>> entire 64-byte structure immediately.
>>
>> However, the IOMMU hardware may fetch these 64 bytes using multiple
>> internal transactions (e.g., four 128-bit bursts). If a hardware fetch
>> occurs simultaneously with the CPU zeroing the entry, the hardware could
>> observe a "torn" entry — where some chunks are zeroed and others still
>> contain old data — leading to unpredictable behavior or spurious faults.
>>
>> Follow the "Guidance to Software for Invalidations" in the VT-d spec
>> (Section 6.5.3.3) by implementing a proper ownership handshake:
>>
>> 1. Clear only the 'Present' (P) bit of the PASID entry. This tells the
>>     hardware that the entry is no longer valid.
>> 2. Execute the required invalidation sequence (PASID cache, IOTLB, and
>>     Device-TLB flush) to ensure the hardware has released all cached
>>     references to the entry.
>> 3. Only after the flushes are complete, zero out the remaining fields of
>>     the PASID entry.
>>
>> Additionally, add an explicit clflush in intel_pasid_clear_entry() to
>> ensure that the cleared entry is visible to the IOMMU on systems where
>> memory coherency (ecap_coherent) is not supported.
>>
> 
> better split the clflush part into a separate patch

Ah, the commit message is a bit confusing. Previously, 
intel_pasid_clear_entry() was followed by a clflush. Now that we've 
moved the clearing logic after the cache invalidation, an explicit 
clflush is required. Instead of splitting this into a separate patch, 
how about amending the commit message to make this clear.

Or, keep intel_pasid_clear_entry() untouched like the following:

iommu/vt-d: Clear Present bit before tearing down PASID entry

The Intel VT-d Scalable Mode PASID table entry consists of 512 bits (64
bytes). When tearing down an entry, the current implementation zeros the
entire 64-byte structure immediately.

However, the IOMMU hardware may fetch these 64 bytes using multiple
internal transactions (e.g., four 128-bit bursts). If a hardware fetch
occurs simultaneously with the CPU zeroing the entry, the hardware could
observe a "torn" entry — where some chunks are zeroed and others still
contain old data — leading to unpredictable behavior or spurious faults.

Follow the "Guidance to Software for Invalidations" in the VT-d spec
(Section 6.5.3.3) by implementing a proper ownership handshake:

1. Clear only the 'Present' (P) bit of the PASID entry. This tells the
    hardware that the entry is no longer valid.
2. Execute the required invalidation sequence (PASID cache, IOTLB, and
    Device-TLB flush) to ensure the hardware has released all cached
    references to the entry.
3. Only after the flushes are complete, zero out the remaining fields of
    the PASID entry.

Fixes: 0bbeb01a4faf ("iommu/vt-d: Manage scalalble mode PASID tables")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
  drivers/iommu/intel/pasid.h | 12 ++++++++++++
  drivers/iommu/intel/pasid.c |  6 +++++-
  2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index b4c85242dc79..35de1d77355f 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -237,6 +237,18 @@ static inline void pasid_set_present(struct 
pasid_entry *pe)
  	pasid_set_bits(&pe->val[0], 1 << 0, 1);
  }

+/*
+ * Clear the Present (P) bit (bit 0) of a scalable-mode PASID table entry.
+ * This initiates the transition of the entry's ownership from hardware
+ * to software. The caller is responsible for fulfilling the invalidation
+ * handshake recommended by the VT-d spec, Section 6.5.3.3 (Guidance to
+ * Software for Invalidations).
+ */
+static inline void pasid_clear_present(struct pasid_entry *pe)
+{
+	pasid_set_bits(&pe->val[0], 1 << 0, 0);
+}
+
  /*
   * Setup Page Walk Snoop bit (Bit 87) of a scalable mode PASID
   * entry.
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 298a39183996..3a8ec5eba83e 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -272,7 +272,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu 
*iommu, struct device *dev,

  	did = pasid_get_domain_id(pte);
  	pgtt = pasid_pte_get_pgtt(pte);
-	intel_pasid_clear_entry(dev, pasid, fault_ignore);
+	pasid_clear_present(pte);
  	spin_unlock(&iommu->lock);

  	if (!ecap_coherent(iommu->ecap))
@@ -286,6 +286,10 @@ void intel_pasid_tear_down_entry(struct intel_iommu 
*iommu, struct device *dev,
  		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);

  	devtlb_invalidation_with_pasid(iommu, dev, pasid);
+	intel_pasid_clear_entry(dev, pasid, fault_ignore);
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(pte, sizeof(*pte));
+
  	if (!fault_ignore)
  		intel_iommu_drain_pasid_prq(dev, pasid);
  }
-- 
2.43.0

Thanks,
baolu