Re: [PATCH 1/2] vfio/ccw: Add length to DMA_UNMAP checks

[Eric Farman <farman@linux.ibm.com>]

On Wed, 2022-07-27 at 13:16 -0400, Matthew Rosato wrote:
> On 7/27/22 12:45 PM, Eric Farman wrote:
> > On Tue, 2022-07-26 at 12:12 -0400, Matthew Rosato wrote:
> > > On 7/26/22 11:01 AM, Eric Farman wrote:
> > > > As pointed out with the simplification of the
> > > > VFIO_IOMMU_NOTIFY_DMA_UNMAP notifier [1], the length
> > > > parameter was never used to check against the pinned
> > > > pages.
> > > > 
> > > > Let's correct that, and see if a page is within the
> > > > affected range instead of simply the first page of
> > > > the range.
> > > > 
> > > > [1]
> > > > https://lore.kernel.org/kvm/20220720170457.39cda0d0.alex.williamson@redhat.com/
> > > > 
> > > > Signed-off-by: Eric Farman <farman@linux.ibm.com>
> > > > ---
> > > >    drivers/s390/cio/vfio_ccw_cp.c  | 11 +++++++----
> > > >    drivers/s390/cio/vfio_ccw_cp.h  |  2 +-
> > > >    drivers/s390/cio/vfio_ccw_ops.c |  2 +-
> > > >    3 files changed, 9 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/s390/cio/vfio_ccw_cp.c
> > > > b/drivers/s390/cio/vfio_ccw_cp.c
> > > > index 8963f452f963..f15b5114abd1 100644
> > > > --- a/drivers/s390/cio/vfio_ccw_cp.c
> > > > +++ b/drivers/s390/cio/vfio_ccw_cp.c
> > > > @@ -170,12 +170,14 @@ static void page_array_unpin_free(struct
> > > > page_array *pa, struct vfio_device *vde
> > > >    	kfree(pa->pa_iova);
> > > >    }
> > > >    
> > > > -static bool page_array_iova_pinned(struct page_array *pa,
> > > > unsigned
> > > > long iova)
> > > > +static bool page_array_iova_pinned(struct page_array *pa,
> > > > unsigned
> > > > long iova,
> > > > +				   unsigned long length)
> > > >    {
> > > >    	int i;
> > > >    
> > > >    	for (i = 0; i < pa->pa_nr; i++)
> > > > -		if (pa->pa_iova[i] == iova)
> > > > +		if (pa->pa_iova[i] >= iova &&
> > > > +		    pa->pa_iova[i] <= iova + length)
> > > 
> > > For the sake of completeness, I think you want to be checking to
> > > make
> > > sure the end of the page is also within the range, not just the
> > > start?
> > > 
> > > if (pa->pa_iova[i] >= iova &&
> > >       pa->pa_iova[i] + PAGE_SIZE <= iova + length)
> > 
> > Well +PAGE_SIZE would iterate to the next page, so that would be
> > captured on the next iteration of the for(i) loop if the pages were
> > contiguous (or not applicable, if the pages weren't).
> 
> FWIW, the '+ PAGE_SIZE' was to match the '+ length' in your
> comparison.
> 
> If you really only want to only look at the start of the pa_iova
> being 
> within the range, then I think you want 'pa->pa_iova[i] < iova + 
> length', not <=.

Touche.

> 
> > But, since the comment is really about the end of the page (0xfff),
> > I
> > guess I'm not understanding what that gets us so perhaps you could
> > help
> > elaborate your question? From my chair, since the pa_iova argument
> > passed to vfio_pin_pages() pins the whole page, checking the start
> > address versus the end (or anywhere in between) should still
> > capture
> > its interaction with an affected range. That is to say, we don't
> > care
> > about the -whole- page being within the unmap range, but -any- part
> > of
> > it.
> > 
> 
> As far as my suggestion to also look at the end of the pa_iova[i] -- 
> This was particularly geared at ensuring the entire page fell within
> the 
> range, not just a subset.  But I think you're right, we don't really 
> care about that.  On the flip side, do we care if the iova somehow 
> starts sometime between pa_iova[i] and pa_iova[i] + PAGE_SIZE - 1?  

As I was mulling your original reply, I was having the same thought. I
think the answer is it's probably unlikely, but was trying to figure
out how to rope something in for that.

> That 
> would still be a subset, though I'm not sure such a thing could
> happen 
> today (e.g. an input 'iova' that is not on a page boundary)..
> 
> I wonder if the simplest thing would be to just copy what gvt does
> and 
> convert to pfn as it takes all of this out of the equation and looks 
> instead at whether the inputs overlaps at a page granularity (which
> is 
> what we really care about)

Agreed.

> , e.g. something like (untested):

Ha. This is almost exactly what I had in place originally, before I
applied Nicolin's series and the pfns were still available in the
struct that pa points to. I should have left that all in place instead
of cleaning it up this way. Let me get that refit and doublechecked,
and I'll send a v2. Thanks!

> 
> u64 iov_pfn = iova >> PAGE_SHIFT;
> u64 end_iov_pfn = iov_pfn + (length / PAGE_SIZE);
> u64 pfn;
> int i;
> 
> for (i = 0; i < pa->pa_nr; i++) {
>     pfn = pa->pa_iova[i] >> PAGE_SHIFT;
>     if (pfn >= iov_pfn && pfn < end_iov_pfn)
> 	return true;
> }
> 
> 
>