On Fri, May 08, 2020 at 09:10:13AM -0300, Jason Gunthorpe wrote: > On Thu, May 07, 2020 at 10:19:39PM -0400, Peter Xu wrote: > > On Thu, May 07, 2020 at 08:54:21PM -0300, Jason Gunthorpe wrote: > > > On Thu, May 07, 2020 at 05:24:43PM -0400, Peter Xu wrote: > > > > On Tue, May 05, 2020 at 03:54:44PM -0600, Alex Williamson wrote: > > > > > With conversion to follow_pfn(), DMA mapping a PFNMAP range depends on > > > > > the range being faulted into the vma. Add support to manually provide > > > > > that, in the same way as done on KVM with hva_to_pfn_remapped(). > > > > > > > > > > Signed-off-by: Alex Williamson <alex.william...@redhat.com> > > > > > drivers/vfio/vfio_iommu_type1.c | 36 > > > > > +++++++++++++++++++++++++++++++++--- > > > > > 1 file changed, 33 insertions(+), 3 deletions(-) > > > > > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > > > b/drivers/vfio/vfio_iommu_type1.c > > > > > index cc1d64765ce7..4a4cb7cd86b2 100644 > > > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > > > @@ -317,6 +317,32 @@ static int put_pfn(unsigned long pfn, int prot) > > > > > return 0; > > > > > } > > > > > > > > > > +static int follow_fault_pfn(struct vm_area_struct *vma, struct > > > > > mm_struct *mm, > > > > > + unsigned long vaddr, unsigned long *pfn, > > > > > + bool write_fault) > > > > > +{ > > > > > + int ret; > > > > > + > > > > > + ret = follow_pfn(vma, vaddr, pfn); > > > > > + if (ret) { > > > > > + bool unlocked = false; > > > > > + > > > > > + ret = fixup_user_fault(NULL, mm, vaddr, > > > > > + FAULT_FLAG_REMOTE | > > > > > + (write_fault ? FAULT_FLAG_WRITE > > > > > : 0), > > > > > + &unlocked); > > > > > + if (unlocked) > > > > > + return -EAGAIN; > > > > > > > > Hi, Alex, > > > > > > > > IIUC this retry is not needed too because fixup_user_fault() will > > > > guarantee the > > > > fault-in is done correctly with the valid PTE as long as ret==0, even if > > > > unlocked==true. > > > > > > It is true, and today it is fine, but be careful when reworking this > > > to use notifiers as unlocked also means things like the vma pointer > > > are invalidated. > > > > Oh right, thanks for noticing that. Then we should probably still keep the > > retry logic... because otherwise the latter follow_pfn() could be > > referencing > > an invalid vma already... > > I looked briefly and thought this flow used the vma only once?
ret = follow_pfn(vma, vaddr, pfn); if (ret) { bool unlocked = false; ret = fixup_user_fault(NULL, mm, vaddr, FAULT_FLAG_REMOTE | (write_fault ? FAULT_FLAG_WRITE : 0), &unlocked); if (unlocked) return -EAGAIN; if (ret) return ret; ret = follow_pfn(vma, vaddr, pfn); <--------------- [1] } So imo the 2nd follow_pfn() [1] could be racy if without the unlocked check. Thanks, -- Peter Xu