[Qemu-devel] [PATCH 1/1] vfio iommu type1: set size to PAGE_SIZE while looking for iova in dma list

[Qemu-devel] [PATCH 1/1] vfio iommu type1: set size to PAGE_SIZE while looking for iova in dma list

[Kirti Wankhede]

In the functions of pin_pages/unpin_pages from mdev vendor driver,
vfio_find_dma() should be called with size as PAGE_SIZE instead of 0.
vfio_find_dma() searches for the range in dma_list.

In vfio_dma_do_unmap(), vfio_find_dma() when used to look for start
address of unmap->iova, size should be 1, not 0. Otherwise vfio_find_dma()
returns NULL.

Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Signed-off-by: Neo Jia <cjia@nvidia.com>
Change-Id: Iee6fd45441c342b5e8626087046b2e0075d19a08
---
 drivers/vfio/vfio_iommu_type1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index a28fbddb505c..023fba7b8d5a 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -581,7 +581,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
 		struct vfio_pfn *vpfn;
 
 		iova = user_pfn[i] << PAGE_SHIFT;
-		dma = vfio_find_dma(iommu, iova, 0);
+		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
 		if (!dma) {
 			ret = -EINVAL;
 			goto pin_unwind;
@@ -622,7 +622,7 @@ pin_unwind:
 		dma_addr_t iova;
 
 		iova = user_pfn[j] << PAGE_SHIFT;
-		dma = vfio_find_dma(iommu, iova, 0);
+		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
 		vfio_unpin_page_external(dma, iova, do_accounting);
 		phys_pfn[j] = 0;
 	}
@@ -659,7 +659,7 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
 		dma_addr_t iova;
 
 		iova = user_pfn[i] << PAGE_SHIFT;
-		dma = vfio_find_dma(iommu, iova, 0);
+		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
 		if (!dma)
 			goto unpin_exit;
 		vfio_unpin_page_external(dma, iova, do_accounting);
@@ -826,7 +826,7 @@ again:
 	 * mappings within the range.
 	 */
 	if (iommu->v2) {
-		dma = vfio_find_dma(iommu, unmap->iova, 0);
+		dma = vfio_find_dma(iommu, unmap->iova, 1);
 		if (dma && dma->iova != unmap->iova) {
 			ret = -EINVAL;
 			goto unlock;
-- 
2.7.0

Re: [Qemu-devel] [PATCH 1/1] vfio iommu type1: set size to PAGE_SIZE while looking for iova in dma list

[Alex Williamson]

On Tue, 6 Dec 2016 02:38:01 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> In the functions of pin_pages/unpin_pages from mdev vendor driver,
> vfio_find_dma() should be called with size as PAGE_SIZE instead of 0.
> vfio_find_dma() searches for the range in dma_list.
> 
> In vfio_dma_do_unmap(), vfio_find_dma() when used to look for start
> address of unmap->iova, size should be 1, not 0. Otherwise vfio_find_dma()
> returns NULL.

Hi Kirti,

I'd prefer to fix the preexisting case of this issue as a separate
patch, we might want to backport it for stable trees separately from the
new cases introduced with mdev.  It would also be great if we could go
into some detail about why size=0 doesn't work in these cases, ie.
vfio_find_dma() accounting for the end address of the range makes it
fall through at the wrong branch point.  For the case of the
preexisting issue, it would also be useful to mention that the other
case of passing size=0 works correctly due to the -1 in the start
address calculation.  Finally, I think I agree with your choice to use
PAGE_SIZE in one case and 1 in the preexisting case, but perhaps we
could spell that out as one is related to a page size interface and the
other is trying to test boundary conditions.  Thanks,

Alex

> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> Signed-off-by: Neo Jia <cjia@nvidia.com>
> Change-Id: Iee6fd45441c342b5e8626087046b2e0075d19a08
> ---
>  drivers/vfio/vfio_iommu_type1.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index a28fbddb505c..023fba7b8d5a 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -581,7 +581,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  		struct vfio_pfn *vpfn;
>  
>  		iova = user_pfn[i] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		if (!dma) {
>  			ret = -EINVAL;
>  			goto pin_unwind;
> @@ -622,7 +622,7 @@ pin_unwind:
>  		dma_addr_t iova;
>  
>  		iova = user_pfn[j] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		vfio_unpin_page_external(dma, iova, do_accounting);
>  		phys_pfn[j] = 0;
>  	}
> @@ -659,7 +659,7 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
>  		dma_addr_t iova;
>  
>  		iova = user_pfn[i] << PAGE_SHIFT;
> -		dma = vfio_find_dma(iommu, iova, 0);
> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>  		if (!dma)
>  			goto unpin_exit;
>  		vfio_unpin_page_external(dma, iova, do_accounting);
> @@ -826,7 +826,7 @@ again:
>  	 * mappings within the range.
>  	 */
>  	if (iommu->v2) {
> -		dma = vfio_find_dma(iommu, unmap->iova, 0);
> +		dma = vfio_find_dma(iommu, unmap->iova, 1);
>  		if (dma && dma->iova != unmap->iova) {
>  			ret = -EINVAL;
>  			goto unlock;

Re: [Qemu-devel] [PATCH 1/1] vfio iommu type1: set size to PAGE_SIZE while looking for iova in dma list

[Kirti Wankhede]

On 12/6/2016 4:24 AM, Alex Williamson wrote:
> On Tue, 6 Dec 2016 02:38:01 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> In the functions of pin_pages/unpin_pages from mdev vendor driver,
>> vfio_find_dma() should be called with size as PAGE_SIZE instead of 0.
>> vfio_find_dma() searches for the range in dma_list.
>>
>> In vfio_dma_do_unmap(), vfio_find_dma() when used to look for start
>> address of unmap->iova, size should be 1, not 0. Otherwise vfio_find_dma()
>> returns NULL.
> 
> Hi Kirti,
> 
> I'd prefer to fix the preexisting case of this issue as a separate
> patch, we might want to backport it for stable trees separately from the
> new cases introduced with mdev.  It would also be great if we could go
> into some detail about why size=0 doesn't work in these cases, ie.
> vfio_find_dma() accounting for the end address of the range makes it
> fall through at the wrong branch point.  For the case of the
> preexisting issue, it would also be useful to mention that the other
> case of passing size=0 works correctly due to the -1 in the start
> address calculation.  Finally, I think I agree with your choice to use
> PAGE_SIZE in one case and 1 in the preexisting case, but perhaps we
> could spell that out as one is related to a page size interface and the
> other is trying to test boundary conditions.  Thanks,
> 

Thanks Alex,

Actual problem here is: vfio_dma keeps track of address range from
(dma->iova + 0) to (dma->iova + dma->size - 1), while vfio_find_dma()
search logic looks for range from (dma->iova + 1) to (dma->iova +
dma->size). So either we have to fix the size argument send to
vfio_find_dma() or we have to fix rb-tree creation and search logic.
I took the former path here. I'll add this detail in commit description
and split it as you mentioned above.

Thanks,
Kirti

> Alex
> 
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>> Change-Id: Iee6fd45441c342b5e8626087046b2e0075d19a08
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index a28fbddb505c..023fba7b8d5a 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -581,7 +581,7 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>>  		struct vfio_pfn *vpfn;
>>  
>>  		iova = user_pfn[i] << PAGE_SHIFT;
>> -		dma = vfio_find_dma(iommu, iova, 0);
>> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>>  		if (!dma) {
>>  			ret = -EINVAL;
>>  			goto pin_unwind;
>> @@ -622,7 +622,7 @@ pin_unwind:
>>  		dma_addr_t iova;
>>  
>>  		iova = user_pfn[j] << PAGE_SHIFT;
>> -		dma = vfio_find_dma(iommu, iova, 0);
>> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>>  		vfio_unpin_page_external(dma, iova, do_accounting);
>>  		phys_pfn[j] = 0;
>>  	}
>> @@ -659,7 +659,7 @@ static int vfio_iommu_type1_unpin_pages(void *iommu_data,
>>  		dma_addr_t iova;
>>  
>>  		iova = user_pfn[i] << PAGE_SHIFT;
>> -		dma = vfio_find_dma(iommu, iova, 0);
>> +		dma = vfio_find_dma(iommu, iova, PAGE_SIZE);
>>  		if (!dma)
>>  			goto unpin_exit;
>>  		vfio_unpin_page_external(dma, iova, do_accounting);
>> @@ -826,7 +826,7 @@ again:
>>  	 * mappings within the range.
>>  	 */
>>  	if (iommu->v2) {
>> -		dma = vfio_find_dma(iommu, unmap->iova, 0);
>> +		dma = vfio_find_dma(iommu, unmap->iova, 1);
>>  		if (dma && dma->iova != unmap->iova) {
>>  			ret = -EINVAL;
>>  			goto unlock;
>