> From: Kirti Wankhede [mailto:kwankhede@xxxxxxxxxx] > Sent: Tuesday, October 11, 2016 4:29 AM > [...] > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 2ba19424e4a1..ce6d6dcbd9a8 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -55,18 +55,26 @@ MODULE_PARM_DESC(disable_hugepages, > > struct vfio_iommu { > struct list_head domain_list; > + struct vfio_domain *local_domain; Hi, Kirti, can you help explain the meaning of 'local" here? I have a hard time to understand its intention... In your later change of vaddr_get_pfn, it's even more confusing where get_user_pages_remote is used on a 'local_mm': + if (mm) { + down_read(&local_mm->mmap_sem); + ret = get_user_pages_remote(NULL, local_mm, vaddr, 1, + !!(prot & IOMMU_WRITE), 0, page, NULL); + up_read(&local_mm->mmap_sem); + } else + ret = get_user_pages_fast(vaddr, 1, + !!(prot & IOMMU_WRITE), page); [...] > -static int vaddr_get_pfn(unsigned long vaddr, int prot, unsigned long *pfn) > +static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr, > + int prot, unsigned long *pfn) > { > struct page *page[1]; > struct vm_area_struct *vma; > + struct mm_struct *local_mm = (mm ? mm : current->mm); it'd be clearer if you call this variable as 'mm' while the earlier input parameter as 'local_mm'. > int ret = -EFAULT; > > - if (get_user_pages_fast(vaddr, 1, !!(prot & IOMMU_WRITE), page) == 1) { > + if (mm) { > + down_read(&local_mm->mmap_sem); > + ret = get_user_pages_remote(NULL, local_mm, vaddr, 1, > + !!(prot & IOMMU_WRITE), 0, page, NULL); > + up_read(&local_mm->mmap_sem); > + } else > + ret = get_user_pages_fast(vaddr, 1, > + !!(prot & IOMMU_WRITE), page); > + > + if (ret == 1) { > *pfn = page_to_pfn(page[0]); > return 0; > } > > - down_read(¤t->mm->mmap_sem); > + down_read(&local_mm->mmap_sem); > > - vma = find_vma_intersection(current->mm, vaddr, vaddr + 1); > + vma = find_vma_intersection(local_mm, vaddr, vaddr + 1); > > if (vma && vma->vm_flags & VM_PFNMAP) { > *pfn = ((vaddr - vma->vm_start) >> PAGE_SHIFT) + vma->vm_pgoff; [...] > +static long __vfio_pin_pages_local(struct vfio_domain *domain, > + unsigned long vaddr, int prot, > + unsigned long *pfn_base, > + bool do_accounting) 'pages' -> 'page' since only one page is handled here. [...] > + > +static void __vfio_unpin_pages_local(struct vfio_domain *domain, > + unsigned long pfn, int prot, > + bool do_accounting) ditto > +{ > + put_pfn(pfn, prot); > + > + if (do_accounting) > + vfio_lock_acct(domain->local_addr_space->task, -1); > +} > + > +static int vfio_unpin_pfn(struct vfio_domain *domain, > + struct vfio_pfn *vpfn, bool do_accounting) > +{ > + __vfio_unpin_pages_local(domain, vpfn->pfn, vpfn->prot, > + do_accounting); > + > + if (atomic_dec_and_test(&vpfn->ref_count)) > + vfio_remove_from_pfn_list(domain, vpfn); > + > + return 1; > +} > + > +static long vfio_iommu_type1_pin_pages(void *iommu_data, > + unsigned long *user_pfn, > + long npage, int prot, > + unsigned long *phys_pfn) > +{ > + struct vfio_iommu *iommu = iommu_data; > + struct vfio_domain *domain; > + int i, j, ret; > + long retpage; > + unsigned long remote_vaddr; > + unsigned long *pfn = phys_pfn; > + struct vfio_dma *dma; > + bool do_accounting = false; > + > + if (!iommu || !user_pfn || !phys_pfn) > + return -EINVAL; > + > + mutex_lock(&iommu->lock); > + > + if (!iommu->local_domain) { > + ret = -EINVAL; > + goto pin_done; > + } > + > + domain = iommu->local_domain; > + > + /* > + * If iommu capable domain exist in the container then all pages are > + * already pinned and accounted. Accouting should be done if there is no > + * iommu capable domain in the container. > + */ > + do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu); > + > + for (i = 0; i < npage; i++) { > + struct vfio_pfn *p; > + dma_addr_t iova; > + > + iova = user_pfn[i] << PAGE_SHIFT; > + > + dma = vfio_find_dma(iommu, iova, 0); > + if (!dma) { > + ret = -EINVAL; > + goto pin_unwind; > + } > + > + remote_vaddr = dma->vaddr + iova - dma->iova; again, why "remote"_vaddr on a 'local' function? > + > + retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot, > + &pfn[i], do_accounting); > + if (retpage <= 0) { > + WARN_ON(!retpage); > + ret = (int)retpage; > + goto pin_unwind; > + } > + > + mutex_lock(&domain->local_addr_space->pfn_list_lock); > + > + /* search if pfn exist */ > + p = vfio_find_pfn(domain, pfn[i]); > + if (p) { > + atomic_inc(&p->ref_count); > + mutex_unlock(&domain->local_addr_space->pfn_list_lock); > + continue; > + } > + > + ret = vfio_add_to_pfn_list(domain, remote_vaddr, iova, > + pfn[i], prot); > + mutex_unlock(&domain->local_addr_space->pfn_list_lock); > + > + if (ret) { > + __vfio_unpin_pages_local(domain, pfn[i], prot, > + do_accounting); > + goto pin_unwind; > + } > + } > + > + ret = i; > + goto pin_done; > + > +pin_unwind: > + pfn[i] = 0; > + mutex_lock(&domain->local_addr_space->pfn_list_lock); > + for (j = 0; j < i; j++) { > + struct vfio_pfn *p; > + > + p = vfio_find_pfn(domain, pfn[j]); > + if (p) > + vfio_unpin_pfn(domain, p, do_accounting); > + > + pfn[j] = 0; > + } > + mutex_unlock(&domain->local_addr_space->pfn_list_lock); > + > +pin_done: > + mutex_unlock(&iommu->lock); > + return ret; > +} > + > +static long vfio_iommu_type1_unpin_pages(void *iommu_data, unsigned long *pfn, > + long npage) > +{ > + struct vfio_iommu *iommu = iommu_data; > + struct vfio_domain *domain = NULL; > + long unlocked = 0; > + int i; > + > + if (!iommu || !pfn) > + return -EINVAL; > + acquire iommu lock... > + domain = iommu->local_domain; > + > + for (i = 0; i < npage; i++) { > + struct vfio_pfn *p; > + > + mutex_lock(&domain->local_addr_space->pfn_list_lock); > + > + /* verify if pfn exist in pfn_list */ > + p = vfio_find_pfn(domain, pfn[i]); > + if (p) > + unlocked += vfio_unpin_pfn(domain, p, true); Should we force update accounting here even when there is iommu capable domain? It's not consistent to earlier pin_pages. > + > + mutex_unlock(&domain->local_addr_space->pfn_list_lock); > + } > > return unlocked; > } > @@ -341,6 +636,12 @@ static void vfio_unmap_unpin(struct vfio_iommu *iommu, struct > vfio_dma *dma) > > if (!dma->size) > return; > + > + if (!IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu)) > + return; Is above check redundant to following dma->iommu_mapped? > + > + if (!dma->iommu_mapped) > + return; > /* > * We use the IOMMU to track the physical addresses, otherwise we'd > * need a much more complicated tracking system. Unfortunately that Thanks Kevin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
