Received-SPF: pass (google.com: domain of linux-kernel+bounces-74038-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <55a4b5e5-e0ad-4aa4-a29b-5fff4ef69063@linux.intel.com>
Date: Wed, 21 Feb 2024 11:52:48 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: baolu.lu@linux.intel.com, "Zhang, Tina" <tina.zhang@intel.com>,
 "iommu@lists.linux.dev" <iommu@lists.linux.dev>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
 David Woodhouse <dwmw2@infradead.org>, Joerg Roedel <joro@8bytes.org>,
 Will Deacon <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>,
 Jason Gunthorpe <jgg@ziepe.ca>, "Tian, Kevin" <kevin.tian@intel.com>,
 Nicolin Chen <nicolinc@nvidia.com>, Michael Shavit <mshavit@google.com>,
 Vasant Hegde <vasant.hegde@amd.com>, Jason Gunthorpe <jgg@nvidia.com>,
 Jean-Philippe Brucker <jean-philippe@linaro.org>,
 Hao Fang <fanghao11@huawei.com>
Subject: Re: [PATCH v10 5/6] iommu: Support mm PASID 1:n with sva domains
Content-Language: en-US
To: Zhangfei Gao <zhangfei.gao@linaro.org>
References: <20231027000525.1278806-1-tina.zhang@intel.com>
 <20231027000525.1278806-6-tina.zhang@intel.com>
 <CABQgh9GWcqUeBkHQCpj5tzu6FnEgpOp3KOQ6s9c0X0KU7Ov1qw@mail.gmail.com>
 <MW5PR11MB5881C5CC15452017867B412889502@MW5PR11MB5881.namprd11.prod.outlook.com>
 <CABQgh9GxQmGU2HR73bSoZLuf4XZFhThXwUs_HJx6KwxDmXrXgg@mail.gmail.com>
 <b05958ba-0c47-45ba-8159-372779f9cc8b@linux.intel.com>
 <CABQgh9FFpL=mEZ-7PqRRVg1eniYV176B7USbGP5MLPvhJaGo9A@mail.gmail.com>
From: Baolu Lu <baolu.lu@linux.intel.com>
In-Reply-To: <CABQgh9FFpL=mEZ-7PqRRVg1eniYV176B7USbGP5MLPvhJaGo9A@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2024/2/21 10:45, Zhangfei Gao wrote:
> On Wed, 21 Feb 2024 at 10:06, Baolu Lu<baolu.lu@linux.intel.com> 
> wrote:
>> On 2024/2/21 9:28, Zhangfei Gao wrote:
>>> On Wed, 21 Feb 2024 at 07:58, Zhang, Tina<tina.zhang@intel.com> 
>>> wrote:
>>> 
>>>>>> struct iommu_sva *iommu_sva_bind_device(struct device
>>>>>> *dev, struct mm_struct *mm)  { +       struct
>>>>>> iommu_mm_data *iommu_mm; struct iommu_domain *domain;
>>>>>> struct iommu_sva *handle; int ret;
>>>>>> 
>>>>>> +       mutex_lock(&iommu_sva_lock); + /* Allocate 
>>>>>> mm->pasid if necessary. */ -       ret = 
>>>>>> iommu_sva_alloc_pasid(mm, dev); -       if (ret) - return
>>>>>> ERR_PTR(ret); +       iommu_mm = iommu_alloc_mm_data(mm,
>>>>>> dev); +       if (IS_ERR(iommu_mm)) { +               ret =
>>>>>> PTR_ERR(iommu_mm); + goto out_unlock; +       }
>>>>>> 
>>>>>> handle = kzalloc(sizeof(*handle), GFP_KERNEL); -       if 
>>>>>> (!handle) -               return ERR_PTR(-ENOMEM); - - 
>>>>>> mutex_lock(&iommu_sva_lock); -       /* Search for an 
>>>>>> existing domain. */ -       domain = 
>>>>>> iommu_get_domain_for_dev_pasid(dev, mm->pasid, - 
>>>>>> IOMMU_DOMAIN_SVA); -       if (IS_ERR(domain)) { - ret =
>>>>>> PTR_ERR(domain); +       if (!handle) { + ret = -ENOMEM;
>>>>>> goto out_unlock; }
>>>>>> 
>>>>>> -       if (domain) { -               domain->users++; - 
>>>>>> goto out;
>>>>> Our multi bind test case broke since 6.8-rc1. The test case 
>>>>> can use same domain & pasid, return different handle, 6.7 
>>>>> simply  domain->users ++ and return.
>>>>> 
>>>>>> +       /* Search for an existing domain. */ + 
>>>>>> list_for_each_entry(domain, &mm->iommu_mm->sva_domains, 
>>>>>> next)
>>>>> {
>>>>>> +               ret = iommu_attach_device_pasid(domain, 
>>>>>> dev, + iommu_mm->pasid);
>>>>> Now iommu_attach_device_pasid return BUSY since the same 
>>>>> pasid. And then iommu_sva_bind_device attach ret=-16
>>>> Sounds like the test case tries to bind a device to a same mm 
>>>> multiple times without unbinding the device and the
>>>> expectation is that it can always return a valid handle to pass
>>>> the test. Right?
>>> Yes
>>> 
>>> The device can bind to the same mm multi-times and return 
>>> different handle, Since the refcount, no need to unbind and bind 
>>> sequently, The unbind can happen later with the handle.
>> Is there any real use case to bind an mm to the pasid of a device 
>> multiple times? If there are cases, is it better to handle this in 
>> the uacce driver?
> Yes, it is required for multi-thread, the device can provide 
> multi-queue to speed up.
> 
>> From iommu core's perspective, it doesn't make sense to attach the 
>> same domain to the same device (or pasid) multiple times.
> But is it the refcount domain->user++ used for? Is there any reason 
> not doing this.

I was just thinking about whether to do this in the iommu core, or in
the upper layers, like uacce or iommufd. It seems that there is no need
to attach a domain to a device or pasid again if it has already been
attached.

Best regards,
baolu