Re: [Qemu-devel] [PATCH v13 09/22] vfio iommu type1: Add task structure to vfio_dma

[Dong Jia Shi]

* Kirti Wankhede <address@hidden> [2016-11-15 20:59:52 +0530]:

Hi Kirti,

[...]
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c

> @@ -331,13 +338,16 @@ static long vfio_pin_pages_remote(unsigned long vaddr, 
> long npage,
>       }
> 
>       if (!rsvd)
> -             vfio_lock_acct(current, i);
> +             vfio_lock_acct(dma->task, i);
> +     ret = i;
> 
> -     return i;
> +pin_pg_remote_exit:
out_mmput sounds a better name to me.

> +     mmput(mm);
> +     return ret;
>  }
> 
[...]

> @@ -510,6 +521,12 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>       while ((dma = vfio_find_dma(iommu, unmap->iova, unmap->size))) {
>               if (!iommu->v2 && unmap->iova > dma->iova)
>                       break;
> +             /*
> +              * Task with same address space who mapped this iova range is
> +              * allowed to unmap the iova range.
> +              */
> +             if (dma->task->mm != current->mm)
How about:
                if (dma->task != current)

> +                     break;
>               unmapped += dma->size;
>               vfio_remove_dma(iommu, dma);
>       }
> @@ -576,17 +593,55 @@ unwind:
>       return ret;
>  }
> 
> +static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
> +                         size_t map_size)
Do you factor out this function for future usage?
I didn't find the other callers.

> +{
> +     dma_addr_t iova = dma->iova;
> +     unsigned long vaddr = dma->vaddr;
> +     size_t size = map_size;
> +     long npage;
> +     unsigned long pfn;
> +     int ret = 0;
> +
> +     while (size) {
> +             /* Pin a contiguous chunk of memory */
> +             npage = vfio_pin_pages_remote(dma, vaddr + dma->size,
> +                                           size >> PAGE_SHIFT, dma->prot,
> +                                           &pfn);
> +             if (npage <= 0) {
> +                     WARN_ON(!npage);
> +                     ret = (int)npage;
> +                     break;
> +             }
> +
> +             /* Map it! */
> +             ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage,
> +                                  dma->prot);
> +             if (ret) {
> +                     vfio_unpin_pages_remote(dma, pfn, npage,
> +                                              dma->prot, true);
> +                     break;
> +             }
> +
> +             size -= npage << PAGE_SHIFT;
> +             dma->size += npage << PAGE_SHIFT;
> +     }
> +
> +     if (ret)
> +             vfio_remove_dma(iommu, dma);
> +
> +     return ret;
> +}
> +
>  static int vfio_dma_do_map(struct vfio_iommu *iommu,
>                          struct vfio_iommu_type1_dma_map *map)
>  {
>       dma_addr_t iova = map->iova;
>       unsigned long vaddr = map->vaddr;
>       size_t size = map->size;
> -     long npage;
>       int ret = 0, prot = 0;
>       uint64_t mask;
>       struct vfio_dma *dma;
> -     unsigned long pfn;
> 
>       /* Verify that none of our __u64 fields overflow */
>       if (map->size != size || map->vaddr != vaddr || map->iova != iova)
> @@ -612,47 +667,27 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
>       mutex_lock(&iommu->lock);
> 
>       if (vfio_find_dma(iommu, iova, size)) {
> -             mutex_unlock(&iommu->lock);
> -             return -EEXIST;
> +             ret = -EEXIST;
> +             goto do_map_err;
>       }
> 
>       dma = kzalloc(sizeof(*dma), GFP_KERNEL);
>       if (!dma) {
> -             mutex_unlock(&iommu->lock);
> -             return -ENOMEM;
> +             ret = -ENOMEM;
> +             goto do_map_err;
>       }
> 
>       dma->iova = iova;
>       dma->vaddr = vaddr;
>       dma->prot = prot;
> +     get_task_struct(current);
> +     dma->task = current;
> 
>       /* Insert zero-sized and grow as we map chunks of it */
>       vfio_link_dma(iommu, dma);
> 
> -     while (size) {
> -             /* Pin a contiguous chunk of memory */
> -             npage = vfio_pin_pages_remote(vaddr + dma->size,
> -                                           size >> PAGE_SHIFT, prot, &pfn);
> -             if (npage <= 0) {
> -                     WARN_ON(!npage);
> -                     ret = (int)npage;
> -                     break;
> -             }
> -
> -             /* Map it! */
> -             ret = vfio_iommu_map(iommu, iova + dma->size, pfn, npage, prot);
> -             if (ret) {
> -                     vfio_unpin_pages_remote(pfn, npage, prot, true);
> -                     break;
> -             }
> -
> -             size -= npage << PAGE_SHIFT;
> -             dma->size += npage << PAGE_SHIFT;
> -     }
> -
> -     if (ret)
> -             vfio_remove_dma(iommu, dma);
> -
> +     ret = vfio_pin_map_dma(iommu, dma, size);
> +do_map_err:
Rename to out_unlock?

>       mutex_unlock(&iommu->lock);
>       return ret;
>  }
> -- 
> 2.7.0
> 

Otherwise, LGTM!

-- 
Dong Jia