Received-SPF: pass (google.com: domain of linux-kernel+bounces-203638-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <afaf133b-7175-467f-a254-060b66b9cb4e@linux.intel.com>
Date: Thu, 6 Jun 2024 14:06:32 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, "iommu@lists.linux.dev"
 <iommu@lists.linux.dev>,
 "virtualization@lists.linux-foundation.org"
 <virtualization@lists.linux-foundation.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 02/10] iommu: Remove sva handle list
To: "Tian, Kevin" <kevin.tian@intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
 Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>,
 Robin Murphy <robin.murphy@arm.com>,
 Jean-Philippe Brucker <jean-philippe@linaro.org>,
 Nicolin Chen <nicolinc@nvidia.com>, "Liu, Yi L" <yi.l.liu@intel.com>,
 Jacob Pan <jacob.jun.pan@linux.intel.com>,
 Joel Granados <j.granados@samsung.com>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-3-baolu.lu@linux.intel.com>
 <BN9PR11MB527658A85092F88329EB73E98CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Language: en-US
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <BN9PR11MB527658A85092F88329EB73E98CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 6/5/24 4:15 PM, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, May 27, 2024 12:05 PM
>>
>> @@ -69,11 +68,16 @@ static struct iommu_mm_data
>> *iommu_alloc_mm_data(struct mm_struct *mm, struct de
>>    */
>>   struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct
>> mm_struct *mm)
>>   {
>> +	struct iommu_group *group = dev->iommu_group;
>> +	struct iommu_attach_handle *attach_handle;
>>   	struct iommu_mm_data *iommu_mm;
>>   	struct iommu_domain *domain;
>>   	struct iommu_sva *handle;
> 
> it's confusing to have both 'handle' and 'attach_handle' in one function.
> 
> Clearer to rename 'handle' as 'sva'.

Yes. Could be cleaned up in a separated patch. All sva handle in
iommu-sva.c should be converted if we decide to do that.

> 
>>   	int ret;
>>
>> +	if (!group)
>> +		return ERR_PTR(-ENODEV);
>> +
>>   	mutex_lock(&iommu_sva_lock);
>>
>>   	/* Allocate mm->pasid if necessary. */
>> @@ -83,12 +87,13 @@ struct iommu_sva *iommu_sva_bind_device(struct
>> device *dev, struct mm_struct *mm
>>   		goto out_unlock;
>>   	}
>>
>> -	list_for_each_entry(handle, &mm->iommu_mm->sva_handles,
>> handle_item) {
>> -		if (handle->dev == dev) {
>> -			refcount_inc(&handle->users);
>> -			mutex_unlock(&iommu_sva_lock);
>> -			return handle;
>> -		}
>> +	/* A bond already exists, just take a reference`. */
>> +	attach_handle = iommu_attach_handle_get(group, iommu_mm-
>>> pasid, IOMMU_DOMAIN_SVA);
>> +	if (!IS_ERR(attach_handle)) {
>> +		handle = container_of(attach_handle, struct iommu_sva,
>> handle);
>> +		refcount_inc(&handle->users);
>> +		mutex_unlock(&iommu_sva_lock);
>> +		return handle;
>>   	}
> 
> It's counter-intuitive to move forward when an error is returned.
> 
> e.g. if it's -EBUSY indicating the pasid already used for another type then
> following attempts shouldn't been tried.
> 
> probably we should have iommu_attach_handle_get() return NULL
> instead of -ENOENT when the entry is free? then:
> 
> 	attach_handle = iommu_attach_handle_get();
> 	if (IS_ERR(attach_handle)) {
> 		ret = PTR_ERR(attach_handle);
> 		goto out_unlock;
> 	} else if (attach_handle) {
> 		/* matched and increase handle->users */
> 	}
> 
> 	/* free entry falls through */
> But then there is one potential issue with the design that 'handle'
> can be optional in iommu_attach_device_pasid(). In that case
> xa_load returns NULL then we cannot differentiate a real unused
> PASID vs. one which has been attached w/o an handle.

The PASID should be allocated exclusively. This means that once a PASID
is assigned to A, it shouldn't be assigned to B at the same time. If a
single PASID is used for multiple purposes, it's likely a bug in the
system.

So the logic of iommu_attach_handle_get() here is: has an SVA domain
already been installed for this PASID? If so, just reuse it. Otherwise,
try to install a new SVA domain.

> Does it suggest that having the caller to always provide a handle
> makes more sense?

I'm neutral on this since only sva bind and iopf path delivery currently
require an attach handle.

Best regards,
baolu