RE: [PATCH v8 3/6] vfio iommu: Add support for mediated devices

Tian, Kevin
2016-10-12T10:31:51Z

> From: Kirti Wankhede [mailto:kwankhede@nvidia.com]
> 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(&current->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