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

[Matthew Rosato <mjrosato@linux.ibm.com>]

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 <=.

> 
> 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;
}