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

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@xxxxxxxxxxxxxxx>
> Sent: Thursday, December 12, 2024 2:58 PM
> 
> 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)) {

add a comment here. otherwise looks good

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