Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Peter Xu
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 

And since after the discussion...

Reviewed-by: Peter Xu 

Thanks,

-- 
Peter Xu



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Peter Xu
On Fri, May 08, 2020 at 09:42:13AM -0600, Alex Williamson wrote:
> Thanks for the discussion.  I gather then that this patch is correct as
> written, which probably also mean the patch Peter linked for KVM should
> not be applied since the logic is the same there.  Correct?  Thanks,

Right.  I've already replied yesterday in that thread so Paolo unqueue that
patch too.  Thanks,

-- 
Peter Xu



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Alex Williamson
On Fri, 8 May 2020 12:05:40 -0300
Jason Gunthorpe  wrote:

> On Fri, May 08, 2020 at 10:30:42AM -0400, Peter Xu wrote:
> > 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 
> > > > > > >  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),
> > > > > > > +);
> > > > > > > + 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),
> >);
> > 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.  
> 
> Ah yes, I didn't notice that, you can't touch vma here if unlocked is true.

Thanks for the discussion.  I gather then that this patch is correct as
written, which probably also mean the patch Peter linked for KVM should
not be applied since the logic is the same there.  Correct?  Thanks,

Alex



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Jason Gunthorpe
On Fri, May 08, 2020 at 10:30:42AM -0400, Peter Xu wrote:
> 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 
> > > > > >  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),
> > > > > > +  );
> > > > > > +   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),
>);
> 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.

Ah yes, I didn't notice that, you can't touch vma here if unlocked is true.

Jason


Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Peter Xu
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 
> > > > >  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),
> > > > > +);
> > > > > + 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),
   );
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



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-08 Thread Jason Gunthorpe
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 
> > > >  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),
> > > > +  );
> > > > +   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?

Jason


Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-07 Thread Peter Xu
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 
> > >  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),
> > > +);
> > > + 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...

Thanks,

-- 
Peter Xu



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-07 Thread Jason Gunthorpe
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 
> >  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),
> > +  );
> > +   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.

Jason


Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-07 Thread Alex Williamson
On Thu, 7 May 2020 17:24:43 -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 
> > ---
> >  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
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ 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),
> > +  );
> > +   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.
> 
> Note: there's another patch just removed the similar retry in kvm:
> 
> https://lore.kernel.org/kvm/20200416155906.267462-1-pet...@redhat.com/

Great, I was basing this on that kvm code, so I can make essentially an
identical fix.  Thanks!

Alex



Re: [PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-07 Thread Peter Xu
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 
> ---
>  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
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ 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),
> +);
> + 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.

Note: there's another patch just removed the similar retry in kvm:

https://lore.kernel.org/kvm/20200416155906.267462-1-pet...@redhat.com/

Thanks,

> +
> + if (ret)
> + return ret;
> +
> + ret = follow_pfn(vma, vaddr, pfn);
> + }
> +
> + return ret;
> +}

-- 
Peter Xu



[PATCH v2 1/3] vfio/type1: Support faulting PFNMAP vmas

2020-05-05 Thread Alex Williamson
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 
---
 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
--- a/drivers/vfio/vfio_iommu_type1.c
+++ 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),
+  );
+   if (unlocked)
+   return -EAGAIN;
+
+   if (ret)
+   return ret;
+
+   ret = follow_pfn(vma, vaddr, pfn);
+   }
+
+   return ret;
+}
+
 static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
 int prot, unsigned long *pfn)
 {
@@ -339,12 +365,16 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned 
long vaddr,
 
vaddr = untagged_addr(vaddr);
 
+retry:
vma = find_vma_intersection(mm, vaddr, vaddr + 1);
 
if (vma && vma->vm_flags & VM_PFNMAP) {
-   if (!follow_pfn(vma, vaddr, pfn) &&
-   is_invalid_reserved_pfn(*pfn))
-   ret = 0;
+   ret = follow_fault_pfn(vma, mm, vaddr, pfn, prot & IOMMU_WRITE);
+   if (ret == -EAGAIN)
+   goto retry;
+
+   if (!ret && !is_invalid_reserved_pfn(*pfn))
+   ret = -EFAULT;
}
 done:
up_read(>mmap_sem);