Re: [PATCH 0/2] iommu: Remove iommu_sva_ops::mm_exit()

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Jason,

Thanks for the explanation, more comments/questions inline.

On Wed, 8 Apr 2020 16:02:26 -0300
Jason Gunthorpe <jgg@ziepe.ca> wrote:

> On Wed, Apr 08, 2020 at 11:35:52AM -0700, Jacob Pan wrote:
> > Hi Jean,
> > 
> > On Wed,  8 Apr 2020 16:04:25 +0200
> > Jean-Philippe Brucker <jean-philippe@linaro.org> wrote:
> >   
> > > The IOMMU SVA API currently requires device drivers to implement
> > > an mm_exit() callback, which stops device jobs that do DMA. This
> > > function is called in the release() MMU notifier, when an address
> > > space that is shared with a device exits.
> > > 
> > > It has been noted several time during discussions about SVA that
> > > cancelling DMA jobs can be slow and complex, and doing it in the
> > > release() notifier might cause synchronization issues (patch 2 has
> > > more background). Device drivers must in any case call unbind() to
> > > remove their bond, after stopping DMA from a more favorable
> > > context (release of a file descriptor).
> > > 
> > > So after mm exits, rather than notifying device drivers, we can
> > > hold on to the PASID until unbind(), ask IOMMU drivers to
> > > silently abort DMA and Page Requests in the meantime. This change
> > > should relieve the mmput() path.  
> >
> > I assume mm is destroyed after all the FDs are closed  
> 
> FDs do not hold a mmget(), but they may hold a mmgrab(), ie anything
> using mmu_notifiers has to hold a grab until the notifier is
> destroyed, which is often triggered by FD close.
> 
Sorry, I don't get this. Are you saying we have to hold a mmgrab()
between svm_bind/mmu_notifier_register and
svm_unbind/mmu_notifier_unregister?
Isn't the idea of mmu_notifier is to avoid holding the mm reference and
rely on the notifier to tell us when mm is going away?
It seems both Intel and AMD iommu drivers don't hold mmgrab after
mmu_notifier_register.

> So the exit_mmap() -> release() may happen before the FDs are
> destroyed, but the final mmdrop() will be during some FD release when
> the final mmdrop() happens.
> 
Do you mean mmdrop() is after FD release? If so, unbind is called in FD
release should take care of everything, i.e. stops DMA, clear PASID
context on IOMMU, flush PRS queue etc.

Enforcing unbind upon FD close might be a precarious path, perhaps that
is why we have to deal with out of order situation?

> But, in all the drivers I've looked at the PASID and the mmu_notifier
> must have identical lifetimes.
> 
> > In VT-d, because of enqcmd and lazy PASID free we plan to hold on
> > to the PASID until mmdrop.
> > https://lore.kernel.org/patchwork/patch/1217762/  
> 
> Why? The bind already gets a mmu_notifier which has refcounts and the
> right lifetime for PASID.. This code could already be simplified by
> using the mmu_notifier_get()/put() stuff.
> 
Yes, I guess mmu_notifier_get()/put() is new :)
+Fenghua

> A reason to store the PASID in the mm_struct would be if some code
> needs fast access to it, but then I'm not sure how that works with
> SVM_FLAG_PRIVATE_PASID ..
> 
We plan to remove this flag.

> Jason

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu