Subject: Re: [PATCH v14 08/22] vfio iommu type1: Add find_iommu_group()
 function
To: Kirti Wankhede <kwankhede@nvidia.com>, alex.williamson@redhat.com,
        pbonzini@redhat.com, kraxel@redhat.com, cjia@nvidia.com
References: <1479329194-10247-1-git-send-email-kwankhede@nvidia.com>
 <1479329194-10247-9-git-send-email-kwankhede@nvidia.com>
 <b31392c8-b5e2-c0ff-3fbe-4fcf26838874@redhat.com>
 <47e90cff-2af2-e292-ec0f-eb92e2ef0492@nvidia.com>
Cc: 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
From: Auger Eric <eric.auger@redhat.com>
Message-ID: <2f54d6b7-9cb2-cf85-5d2e-1018dc9f077b@redhat.com>
Date: Mon, 21 Nov 2016 13:42:29 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <47e90cff-2af2-e292-ec0f-eb92e2ef0492@nvidia.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4016
Lines: 130

Hi Kirti,

On 21/11/2016 12:55, Kirti Wankhede wrote:
> 
> Hi Eric,
> 
> Thanks for your reviews.
> Alex had already pulled this series into his next branch for v4.10.
> 
> http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1274745.html

Yes I am showing up too late and noticed that later :-(

Anyway I needed (and still need) to look at it.

Thanks

Eric
> 
> Thanks,
> Kirti
> 
> On 11/21/2016 4:59 PM, Auger Eric wrote:
>> Hi,
>> On 16/11/2016 21:46, Kirti Wankhede wrote:
>>> Add find_iommu_group()
>>>
>>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>>> Signed-off-by: Neo Jia <cjia@nvidia.com>
>>> Reviewed-by: Jike Song <jike.song@intel.com>
>>> Reviewed-by: Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
>>>
>>> Change-Id: I9d372f1ebe9eb01a5a21374b8a2b03f7df73601f
>>> ---
>>>  drivers/vfio/vfio_iommu_type1.c | 57 ++++++++++++++++++++++++-----------------
>>>  1 file changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
>>> index 52af5fc01d91..ffe2026f1341 100644
>>> --- a/drivers/vfio/vfio_iommu_type1.c
>>> +++ b/drivers/vfio/vfio_iommu_type1.c
>>> @@ -752,11 +752,24 @@ static void vfio_test_domain_fgsp(struct vfio_domain *domain)
>>>  	__free_pages(pages, order);
>>>  }
>>>  
>>> +static struct vfio_group *find_iommu_group(struct vfio_domain *domain,
>>> +					   struct iommu_group *iommu_group)
>>> +{
>>> +	struct vfio_group *g;
>>> +
>>> +	list_for_each_entry(g, &domain->group_list, next) {
>>> +		if (g->iommu_group == iommu_group)
>>> +			return g;
>>> +	}
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>>  static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>  					 struct iommu_group *iommu_group)
>>>  {
>>>  	struct vfio_iommu *iommu = iommu_data;
>>> -	struct vfio_group *group, *g;
>>> +	struct vfio_group *group;
>>>  	struct vfio_domain *domain, *d;
>>>  	struct bus_type *bus = NULL;
>>>  	int ret;
>>> @@ -764,10 +777,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>>>  	mutex_lock(&iommu->lock);
>>>  
>>>  	list_for_each_entry(d, &iommu->domain_list, next) {
>>> -		list_for_each_entry(g, &d->group_list, next) {
>>> -			if (g->iommu_group != iommu_group)
>>> -				continue;
>>> -
>>> +		if (find_iommu_group(d, iommu_group)) {
>>>  			mutex_unlock(&iommu->lock);
>>>  			return -EINVAL;
>>>  		}
>>> @@ -887,27 +897,26 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
>>>  	mutex_lock(&iommu->lock);
>>>  
>>>  	list_for_each_entry(domain, &iommu->domain_list, next) {
>>> -		list_for_each_entry(group, &domain->group_list, next) {
>>> -			if (group->iommu_group != iommu_group)
>>> -				continue;
>>> +		group = find_iommu_group(domain, iommu_group);
>>> +		if (!group)
>>> +			continue;
>>>  
>>> -			iommu_detach_group(domain->domain, iommu_group);
>>> -			list_del(&group->next);
>>> -			kfree(group);
>>> -			/*
>>> -			 * Group ownership provides privilege, if the group
>>> -			 * list is empty, the domain goes away.  If it's the
>>> -			 * last domain, then all the mappings go away too.
>>> -			 */
>>> -			if (list_empty(&domain->group_list)) {
>>> -				if (list_is_singular(&iommu->domain_list))
>>> -					vfio_iommu_unmap_unpin_all(iommu);
>>> -				iommu_domain_free(domain->domain);
>>> -				list_del(&domain->next);
>>> -				kfree(domain);
>>> -			}
>>> -			goto done;
>>> +		iommu_detach_group(domain->domain, iommu_group);
>>> +		list_del(&group->next);
>>> +		kfree(group);
>>> +		/*
>>> +		 * Group ownership provides privilege, if the group
>>> +		 * list is empty, the domain goes away.  If it's the
>>> +		 * last domain, then all the mappings go away too.
>>> +		 */
>>> +		if (list_empty(&domain->group_list)) {
>>> +			if (list_is_singular(&iommu->domain_list))
>>> +				vfio_iommu_unmap_unpin_all(iommu);
>>> +			iommu_domain_free(domain->domain);
>>> +			list_del(&domain->next);
>>> +			kfree(domain);
>>>  		}
>>> +		goto done;
>>>  	}
>>>  
>>>  done:
>>>
>> Reviewed-by: Eric Auger <eric.auger@redhat.com>
>>
>> Eric
>>