On 11/16/2016 11:36 AM, Dong Jia Shi wrote:
> * Kirti Wankhede <kwankhede@xxxxxxxxxx> [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.
--
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
