On Thu, Apr 16, 2020 at 01:58:29PM -0700, Jacob Pan wrote: > On Wed, 15 Apr 2020 09:47:36 +0200 > Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote: > > > On Fri, Apr 10, 2020 at 08:52:49AM -0700, Jacob Pan wrote: > > > On Thu, 9 Apr 2020 16:50:58 +0200 > > > Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote: > > > > > > > > So unbind is coming anyway, the difference in handling in mmu > > > > > release notifier is whether we silently drop DMA fault vs. > > > > > reporting fault? > > > > > > > > What I meant is, between mmu release notifier and unbind(), we > > > > can't print any error from DMA fault on dmesg, because an mm exit > > > > is easily triggered by userspace. Look at the lifetime of the > > > > bond: > > > > > > > > bind() > > > > | > > > > : Here any DMA fault is handled by mm, and on error we don't > > > > print : anything to dmesg. Userspace can easily trigger faults by > > > > issuing DMA : on unmapped buffers. > > > > | > > > > mm exit -> clear pgd, invalidate IOTLBs > > > > | > > > > : Here the PASID descriptor doesn't have the pgd anymore, but we > > > > don't : print out any error to dmesg either. DMA is likely still > > > > running but : any fault has to be ignored. > > > > : > > > > : We also can't free the PASID yet, since transactions are still > > > > coming : in with this PASID. > > > > | > > > > unbind() -> clear context descriptor, release PASID and mmu > > > > notifier | > > > > : Here the PASID descriptor is clear. If DMA is still running the > > > > device : driver really messed up and we have to print out any > > > > fault. > > > > > > > > For that middle state I had to introduce a new pasid descriptor > > > > state in the SMMU driver, to avoid reporting errors between mm > > > > exit and unbind(). > > > I must have missed something, but why bother with a state when you > > > can always check if the mm is dead by mmget_not_zero()? You would > > > not handle IOPF if the mm is dead anyway, similarly for other DMA > > > errors. > > > > In the SMMU a cleared PASID descriptor results in unrecoverable > > faults, which do not go through the I/O page fault handler. I've been > > thinking about injecting everything to the IOPF handler, recoverable > > or not, but filtering down the stream is complicated. Most of the > > time outside this small window, we really need to print out those > > messages because they would indicate serious bugs. > > > VT-d also results in unrecoverable fault for a cleared PASID. I am > assuming in the fault record, SMMU can also identify the PASID and > source ID. So that should be able to find the matching mm. > Then you can check if the mm is defunct? I'd rather not add a PASID lookup in the SMMU event thread, as that function needs to be fast and there already is a PASID lookup in the IOPF worker > > > Also, since you are not freeing ioasid in mmu_notifier release > > > anymore, does it mean the IOASID notifier chain can be non-atomic? > > > > Unfortunately not, ioasid_free() is called from > > mmu_notifier_ops::free_notifier() in the RCU callback that results > > from mmu_notifier_put(). > > > I agree. I looked at the code, it is much more clean with the > mmu_notifier_get/put. > > I am thinking perhaps adding a reclaim mechanism such that IOASID not > directly freed can stay in an in_active list (while waiting for its > states get cleared) until it can be reclaimed. Do you see this is > useful for SMMU? Probably not useful for SMMU, but I don't see any issue with adding it. > This is useful for VT-d, since we have more consumers for a given PASID, > i.e. VMCS, VDCM, and IOMMU. Each consumer has its own PASID context to > clean up. Right I'm still having trouble understanding the whole picture, will need to read more about this. The main point is not adding too much work in the mm exit path. Thanks, Jean
