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@xxxxxxxxxx/ > > > > > > > > Signed-off-by: Eric Farman <farman@xxxxxxxxxxxxx> > > > > --- > > > > 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; > } > > >
