Re: [PATCH 1/1] iommu/vt-d: Avoid draining PRQ in sva mm release path

[Baolu Lu]

On 2024/12/12 14:18, Tian, Kevin wrote:
> > From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Thursday, December 12, 2024 10:15 AM
> >
> > When a PASID is used for SVA by a device, it's possible that the PASID
> > entry is cleared before the device flushes all ongoing DMA requests and
> > removes the SVA domain. This can occur when an exception happens and
> > the
> > process terminates before the device driver stops DMA and calls the
> > iommu driver to unbind the PASID.
> >
> > There's no need to drain the PRQ in the mm release path. Instead, the PRQ
> > will be drained in the SVA unbind path.
> >
> > Unfortunately, commit c43e1ccdebf2 ("iommu/vt-d: Drain PRQs when
> > domain
> > removed from RID") changed this behavior by unconditionally draining the
> > PRQ in intel_pasid_tear_down_entry(). This can lead to a potential
> > sleeping-in-atomic-context issue.
> >
> > Smatch static checker warning:
> >
> > 	drivers/iommu/intel/prq.c:95 intel_iommu_drain_pasid_prq()
> > 	warn: sleeping in atomic context
> >
> > To avoid this issue, prevent draining the PRQ in the SVA mm release path
> > and restore the previous behavior.
> >
> > Fixes: c43e1ccdebf2 ("iommu/vt-d: Drain PRQs when domain removed from
> > RID")
> > Reported-by: Dan Carpenter<dan.carpenter@xxxxxxxxxx>
> > Closes:https://lore.kernel.org/linux-iommu/c5187676-2fa2-4e29-94e0-
> > 4a279dc88b49@stanley.mountain/
> > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/intel/pasid.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
> > index 0f2a926d3bd5..5b7d85f1e143 100644
> > --- a/drivers/iommu/intel/pasid.c
> > +++ b/drivers/iommu/intel/pasid.c
> > @@ -265,7 +265,8 @@ 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_iommu_drain_pasid_prq(dev, pasid);
> > +	if (!fault_ignore)
> > +		intel_iommu_drain_pasid_prq(dev, pasid);
> >   }
> As a regression fix:
>
> Reviewed-by: Kevin Tian<kevin.tian@xxxxxxxxx>
>
> But I doubt whether it's working as expected. According to the
> description and code, intel_pasid_tear_down_entry() is called
> twice: the 1st in the mm release path and the 2nd in the unbind
> path. PRQ draining is skipped in the former.
>
> But intel_pasid_tear_down_entry() has a check at the beginning:
>
> 	pte = intel_pasid_get_entry(dev, pasid);
> 	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
> 		spin_unlock(&iommu->lock);
> 		return;
> 	}
>
> The 1st invocation already clears the pasid entry with FPD set.
>
> Then the 2nd invocation will return early instead of moving to
> the point of PRQ draining.

You are right.

We need to clear the Fault Processing Disabled bit in the unbind path if
it is set and drain the PRQ. What about below code?

diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index 5b7d85f1e143..45bd1b689674 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -244,11 +244,25 @@ void intel_pasid_tear_down_entry(struct 
intel_iommu *iommu, struct device *dev,

 	spin_lock(&iommu->lock);
 	pte = intel_pasid_get_entry(dev, pasid);
-	if (WARN_ON(!pte) || !pasid_pte_is_present(pte)) {
+	if (WARN_ON(!pte)) {
 		spin_unlock(&iommu->lock);
 		return;
 	}

+	if (!pasid_pte_is_present(pte)) {
+		if (!pasid_pte_is_fault_disabled(pte)) {
+			WARN_ON(READ_ONCE(pte->val[0]) != 0);
+			spin_unlock(&iommu->lock);
+			return;
+		}
+
+		pasid_clear_entry(pe);
+		spin_unlock(&iommu->lock);
+		intel_iommu_drain_pasid_prq(dev, pasid);
+
+		return;
+	}
+
 	did = pasid_get_domain_id(pte);
 	pgtt = pasid_pte_get_pgtt(pte);
 	intel_pasid_clear_entry(dev, pasid, fault_ignore);
diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 082f4fe20216..668d8ece6b14 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -73,6 +73,12 @@ static inline bool pasid_pte_is_present(struct 
pasid_entry *pte)
 	return READ_ONCE(pte->val[0]) & PASID_PTE_PRESENT;
 }

+/* Get FPD(Fault Processing Disable) bit of a PASID table entry */
+static inline bool pasid_pte_is_fault_disabled(struct pasid_entry *pte)
+{
+	return READ_ONCE(pte->val[0]) & PASID_PTE_FPD;
+}
+
 /* Get PGTT field of a PASID table entry */
 static inline u16 pasid_pte_get_pgtt(struct pasid_entry *pte)
 {

Thanks,
baolu