Re: [PATCH v3 2/4] iommu/vt-d: Check domain force_snooping against attached devices

[Baolu Lu]

On 2022/5/6 14:10, Tian, Kevin wrote:
> > From: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Friday, May 6, 2022 1:27 PM
> > +
> > +/*
> > + * Set the page snoop control for a pasid entry which has been set up.
> > + */
> > +void intel_pasid_setup_page_snoop_control(struct intel_iommu *iommu,
> > +					  struct device *dev, u32 pasid)
> > +{
> > +	struct pasid_entry *pte;
> > +	u16 did;
> > +
> > +	spin_lock(&iommu->lock);
> > +	pte = intel_pasid_get_entry(dev, pasid);
> > +	if (WARN_ON(!pte || !pasid_pte_is_present(pte))) {
> > +		spin_unlock(&iommu->lock);
> > +		return;
> > +	}
> > +
> > +	pasid_set_pgsnp(pte);
> > +	did = pasid_get_domain_id(pte);
> > +	spin_unlock(&iommu->lock);
> > +
> > +	pasid_flush_caches(iommu, pte, pasid, did);
> > +}
>
> The comment of pasid_flush_caches() says:
>
> /*
>   * This function flushes cache for a newly setup pasid table entry.
>   * Caller of it should not modify the in-use pasid table entries.
>   */
>
> Can you check whether that comment still hold?

I am sorry that I overlooked this. Very appreciated for pointing this
out!

How about below additional changes?

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index da47406011d3..68f7e8cfa848 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -782,5 +782,24 @@ void intel_pasid_setup_page_snoop_control(struct 
intel_iommu *iommu,
        did = pasid_get_domain_id(pte);
        spin_unlock(&iommu->lock);

-       pasid_flush_caches(iommu, pte, pasid, did);
+       if (!ecap_coherent(iommu->ecap))
+               clflush_cache_range(pte, sizeof(*pte));
+
+       /*
+        * VT-d spec 3.4 table23 states guides for cache invalidation:
+        *
+        * - PASID-selective-within-Domain PASID-cache invalidation
+        * - PASID-selective PASID-based IOTLB invalidation
+        * - If (pasid is RID_PASID)
+        *    - Global Device-TLB invalidation to affected functions
+        *   Else
+        *    - PASID-based Device-TLB invalidation (with S=1 and
+        *      Addr[63:12]=0x7FFFFFFF_FFFFF) to affected functions
+        */
+       pasid_cache_invalidation_with_pasid(iommu, did, pasid);
+       qi_flush_piotlb(iommu, did, pasid, 0, -1, 0);
+
+       /* Device IOTLB doesn't need to be flushed in caching mode. */
+       if (!cap_caching_mode(iommu->cap))
+               devtlb_invalidation_with_pasid(iommu, dev, pasid);
 }

Best regards,
baolu