Re: [Qemu-devel] [PATCH v8 3/6] vfio iommu: Add support for mediated devices

[Tian, Kevin]

> From: Kirti Wankhede [mailto:address@hidden
> 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