Subject: Re: [PATCH v13 09/22] vfio iommu type1: Add task structure to
 vfio_dma
To: <alex.williamson@redhat.com>, <pbonzini@redhat.com>,
        <kraxel@redhat.com>, <cjia@nvidia.com>, <qemu-devel@nongnu.org>,
        <kvm@vger.kernel.org>, <kevin.tian@intel.com>, <jike.song@intel.com>,
        <bjsdjshi@linux.vnet.ibm.com>, <linux-kernel@vger.kernel.org>
References: <1479223805-22895-1-git-send-email-kwankhede@nvidia.com>
 <1479223805-22895-10-git-send-email-kwankhede@nvidia.com>
 <20161116060628.GC5531@bjsdjshi@linux.vnet.ibm.com>
From: Kirti Wankhede <kwankhede@nvidia.com>
Message-ID: <b3cdb748-ea5b-d00f-fbb5-b1609ab424a5@nvidia.com>
Date: Wed, 16 Nov 2016 20:41:57 +0530
MIME-Version: 1.0
In-Reply-To: <20161116060628.GC5531@bjsdjshi@linux.vnet.ibm.com>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4505
Lines: 184


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