Re: [PATCH V2 4/9] vfio/type1: implement unmap all

From: Steven Sistare <steven.sistare@oracle.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: kvm@vger.kernel.org, Cornelia Huck <cohuck@redhat.com>,
	Kirti Wankhede <kwankhede@nvidia.com>
Subject: Re: [PATCH V2 4/9] vfio/type1: implement unmap all
Date: Wed, 27 Jan 2021 18:07:56 -0500	[thread overview]
Message-ID: <646af8fe-dc41-a8c7-0e09-e8247343d526@oracle.com> (raw)
In-Reply-To: <20210122142247.0046a862@omen.home.shazbot.org>

On 1/22/2021 4:22 PM, Alex Williamson wrote:
> On Tue, 19 Jan 2021 09:48:24 -0800
> Steve Sistare <steven.sistare@oracle.com> wrote:
> 
>> Implement VFIO_DMA_UNMAP_FLAG_ALL.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>> ---
>>  drivers/vfio/vfio_iommu_type1.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>> index c687174..ef83018 100644
>> --- a/drivers/vfio/vfio_iommu_type1.c
>> +++ b/drivers/vfio/vfio_iommu_type1.c
>> @@ -1100,6 +1100,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  	unsigned long pgshift;
>>  	dma_addr_t iova = unmap->iova;
>>  	unsigned long size = unmap->size;
>> +	bool unmap_all = !!(unmap->flags & VFIO_DMA_UNMAP_FLAG_ALL);
>>  
>>  	mutex_lock(&iommu->lock);
>>  
>> @@ -1109,8 +1110,13 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  	if (iova & (pgsize - 1))
>>  		goto unlock;
>>  
>> -	if (!size || size & (pgsize - 1))
>> +	if (unmap_all) {
>> +		if (iova || size)
>> +			goto unlock;
>> +		size = SIZE_MAX;
>> +	} else if (!size || size & (pgsize - 1)) {
>>  		goto unlock;
>> +	}
>>  
>>  	if (iova + size - 1 < iova || size > SIZE_MAX)
>>  		goto unlock;
>> @@ -1154,7 +1160,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  	 * will only return success and a size of zero if there were no
>>  	 * mappings within the range.
>>  	 */
>> -	if (iommu->v2) {
>> +	if (iommu->v2 && !unmap_all) {
>>  		dma = vfio_find_dma(iommu, iova, 1);
>>  		if (dma && dma->iova != iova)
>>  			goto unlock;
>> @@ -1165,7 +1171,7 @@ static int vfio_dma_do_unmap(struct vfio_iommu *iommu,
>>  	}
>>  
>>  	ret = 0;
>> -	while ((dma = vfio_find_dma(iommu, iova, size))) {
>> +	while ((dma = vfio_find_dma_first(iommu, iova, size))) {
> 
> 
> Why is this necessary?  Isn't vfio_find_dma_first() O(logN) for this
> operation while vfio_find_dma() is O(1)?

True, vfio_find_dma is O(1) for unmap-all, and find-first is not needed until a later patch.  
I'll continue discussing this issue in my response to your next email.

>>  		if (!iommu->v2 && iova > dma->iova)
>>  			break;
>>  		/*
>> @@ -2538,6 +2544,7 @@ static int vfio_iommu_type1_check_extension(struct vfio_iommu *iommu,
>>  	case VFIO_TYPE1_IOMMU:
>>  	case VFIO_TYPE1v2_IOMMU:
>>  	case VFIO_TYPE1_NESTING_IOMMU:
>> +	case VFIO_UNMAP_ALL:
>>  		return 1;
>>  	case VFIO_DMA_CC_IOMMU:
>>  		if (!iommu)
>> @@ -2710,6 +2717,8 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu,
>>  {
>>  	struct vfio_iommu_type1_dma_unmap unmap;
>>  	struct vfio_bitmap bitmap = { 0 };
>> +	uint32_t mask = VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP |
>> +			VFIO_DMA_UNMAP_FLAG_ALL;
>>  	unsigned long minsz;
>>  	int ret;
>>  
>> @@ -2718,8 +2727,11 @@ static int vfio_iommu_type1_unmap_dma(struct vfio_iommu *iommu,
>>  	if (copy_from_user(&unmap, (void __user *)arg, minsz))
>>  		return -EFAULT;
>>  
>> -	if (unmap.argsz < minsz ||
>> -	    unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP)
>> +	if (unmap.argsz < minsz || unmap.flags & ~mask)
>> +		return -EINVAL;
>> +
>> +	if ((unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) &&
>> +	    (unmap.flags & ~VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP))
> 
> Somehow we're jumping from unmap-all and dirty-bitmap being mutually
> exclusive to dirty-bitmap is absolutely exclusive, which seems like a
> future bug or maintenance issue.  Let's just test specifically what
> we're deciding is unsupported.  Thanks,

OK, I will make it future proof.

- Steve

 		return -EINVAL;
>>  
>>  	if (unmap.flags & VFIO_DMA_UNMAP_FLAG_GET_DIRTY_BITMAP) {
> 