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

[Kirti Wankhede]

On 11/16/2016 11:36 AM, Dong Jia Shi wrote:
> * 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)
> 

As I mentioned in comment above this and commit description, if a
process calls DMA_MAP, forks a thread and then child thread calls
DMA_UNMAP, this should be allowed since address space is same for parent
process and child. QEMU also works that way.

>> +                    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.
>

This is pulled out to make caller simple and short. Otherwise
vfio_dma_do_map() would have become a long function.


>> +{
>> +    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!
> 

Thanks.

Kirti.