Re: [PATCH v4 3/5] iommu/vt-d: Disable non-recoverable fault processing before unbind

[Lu Baolu]

Hi Kevin,

On 2020/5/7 13:45, Tian, Kevin wrote:
> > From: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > Sent: Thursday, May 7, 2020 8:56 AM
> >
> > When a PASID is used for SVA by the device, it's possible that the PASID
> > entry is cleared before the device flushes all ongoing DMA requests. The
> > IOMMU should ignore the non-recoverable faults caused by these requests.
> > Intel VT-d provides such function through the FPD bit of the PASID entry.
> > This sets FPD bit when PASID entry is cleared in the mm notifier and
> > clear it when the pasid is unbound.
> >
> > Signed-off-by: Lu Baolu<baolu.lu@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/iommu/intel-iommu.c |  4 ++--
> >   drivers/iommu/intel-pasid.c | 26 +++++++++++++++++++++-----
> >   drivers/iommu/intel-pasid.h |  3 ++-
> >   drivers/iommu/intel-svm.c   |  9 ++++++---
> >   4 files changed, 31 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> > index d1866c0905b1..7811422b5a68 100644
> > --- a/drivers/iommu/intel-iommu.c
> > +++ b/drivers/iommu/intel-iommu.c
> > @@ -5352,7 +5352,7 @@ static void __dmar_remove_one_dev_info(struct
> > device_domain_info *info)
> >   	if (info->dev) {
> >   		if (dev_is_pci(info->dev) && sm_supported(iommu))
> >   			intel_pasid_tear_down_entry(iommu, info->dev,
> > -					PASID_RID2PASID);
> > +					PASID_RID2PASID, false);
> >
> >   		iommu_disable_dev_iotlb(info);
> >   		domain_context_clear(iommu, info->dev);
> > @@ -5587,7 +5587,7 @@ static void aux_domain_remove_dev(struct
> > dmar_domain *domain,
> >   	auxiliary_unlink_device(domain, dev);
> >
> >   	spin_lock(&iommu->lock);
> > -	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid);
> > +	intel_pasid_tear_down_entry(iommu, dev, domain->default_pasid,
> > false);
> >   	domain_detach_iommu(domain, iommu);
> >   	spin_unlock(&iommu->lock);
> >
> > diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> > index 7969e3dac2ad..11aef6c12972 100644
> > --- a/drivers/iommu/intel-pasid.c
> > +++ b/drivers/iommu/intel-pasid.c
> > @@ -292,7 +292,20 @@ static inline void pasid_clear_entry(struct
> > pasid_entry *pe)
> >   	WRITE_ONCE(pe->val[7], 0);
> >   }
> >
> > -static void intel_pasid_clear_entry(struct device *dev, int pasid)
> > +static inline void pasid_clear_entry_with_fpd(struct pasid_entry *pe)
> > +{
> > +	WRITE_ONCE(pe->val[0], PASID_PTE_FPD);
> > +	WRITE_ONCE(pe->val[1], 0);
> > +	WRITE_ONCE(pe->val[2], 0);
> > +	WRITE_ONCE(pe->val[3], 0);
> > +	WRITE_ONCE(pe->val[4], 0);
> > +	WRITE_ONCE(pe->val[5], 0);
> > +	WRITE_ONCE(pe->val[6], 0);
> > +	WRITE_ONCE(pe->val[7], 0);
> > +}
> > +
> > +static void
> > +intel_pasid_clear_entry(struct device *dev, int pasid, bool pf_ignore)
> Hi, Baolu,
>
> Just curious whether it makes sense to always set FPD here. Yes, SVA is
> one known example that non-recoverable fault associated with a PASID
> entry might be caused after the entry is cleared and those are considered
> benign.  But even in a general context (w/o SVA) why do we care about
> such faults after a PASID entry is torn down?

First level page tables are also used for DMA protection. For example,
thunderbolt peripherals are always untrusted and should be protected
with IOMMU. IOMMU should always report unrecoverable faults generated
by those device to detect possible DMA attacks.

ATS/PRI devices are always trusted devices, hence we could tolerate
setting FPD bit in the time window between mm_release notifier and
unbind().

Best regards,
baolu