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 <=.
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? 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), e.g. something like (untested):
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;
}
