| Message ID | 1480972081-773-1-git-send-email-kwankhede@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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;
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; >
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;