Received-SPF: pass (google.com: domain of linux-kernel+bounces-101198-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <749b23c7-ab0e-42b4-9992-e1867fc7d4d7@intel.com>
Date: Wed, 13 Mar 2024 16:11:41 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/8] iommu: Introduce a replace API for device pasid
Content-Language: en-US
To: Baolu Lu <baolu.lu@linux.intel.com>, "Tian, Kevin" <kevin.tian@intel.com>,
	Jason Gunthorpe <jgg@nvidia.com>
CC: "joro@8bytes.org" <joro@8bytes.org>, "alex.williamson@redhat.com"
	<alex.williamson@redhat.com>, "robin.murphy@arm.com" <robin.murphy@arm.com>,
	"cohuck@redhat.com" <cohuck@redhat.com>, "eric.auger@redhat.com"
	<eric.auger@redhat.com>, "nicolinc@nvidia.com" <nicolinc@nvidia.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>, "mjrosato@linux.ibm.com"
	<mjrosato@linux.ibm.com>, "chao.p.peng@linux.intel.com"
	<chao.p.peng@linux.intel.com>, "yi.y.sun@linux.intel.com"
	<yi.y.sun@linux.intel.com>, "peterx@redhat.com" <peterx@redhat.com>,
	"jasowang@redhat.com" <jasowang@redhat.com>,
	"shameerali.kolothum.thodi@huawei.com"
	<shameerali.kolothum.thodi@huawei.com>, "lulu@redhat.com" <lulu@redhat.com>,
	"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>, "Duan,
 Zhenzhong" <zhenzhong.duan@intel.com>, "joao.m.martins@oracle.com"
	<joao.m.martins@oracle.com>, "Zeng, Xin" <xin.zeng@intel.com>, "Zhao, Yan Y"
	<yan.y.zhao@intel.com>
References: <20231127063428.127436-1-yi.l.liu@intel.com>
 <20231127063428.127436-2-yi.l.liu@intel.com>
 <20240115171950.GL734935@nvidia.com>
 <c831bf5e-f623-402d-9347-8718987d1610@intel.com>
 <BN9PR11MB52766161477C2540969C83568C242@BN9PR11MB5276.namprd11.prod.outlook.com>
 <585423de-9173-4c97-b596-71e1564d8b4e@intel.com>
 <87a2be0d-6a24-4ca8-be30-35287072dda4@linux.intel.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <87a2be0d-6a24-4ca8-be30-35287072dda4@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VjhTT2xYc3lIVjQxTUFvZUlhUFhpaDBRTGc5cmhNWlRCendWeE55anpYVGVr?=
 =?utf-8?B?eXN5Q2JLbHBZbXRNK3hjL05PTTYrTWJlOTIxeTNIYTdON2lwUmx0SE56NEFm?=
 =?utf-8?B?QkFoUnZ3aWlFM0oxdi9DN1hqL2wyK21WV2JueHhQRS9nYUJZTU9ScU10Zmtx?=
 =?utf-8?B?czBJUkxVYmYvQVBnTEdjSFk1ai9OM1JHQ3lJU2pGTlU0dmZTeHB3eEJaT2FZ?=
 =?utf-8?B?UEhMdFd3QzFPOXk4MzRZR2FqeiszaDhkNUxFQ3grUGdYUTQ1WkN5MzkrM3lM?=
 =?utf-8?B?cFRRTTM0T3owZlFka0lkLzZXaXNEVlpma2xhdGNYbW9hejA3UFJyVlltZERt?=
 =?utf-8?B?WHNvbnlQd2tyYnQxclNxRzJqZHI1VU9mOEpDZDJPWmRzNE1rMUgyREZlR0tr?=
 =?utf-8?B?aExMK1MvemZNc0tBNk9BQkFmbEg3ME1tVDVwaldyMUpJT3lub0JoSFVYeXEr?=
 =?utf-8?B?Y21Xc0hTYUJPY0JpZTZuVnJHZ2xlZVEyS1NxMElKK1BoUjRnMjJLcXhXQjd5?=
 =?utf-8?B?MmpJSHp3SzNJTWVlcmcrcWJ5djEwenVmRzZnNUZSOWJyRDFpdGJ6cXJzMDd5?=
 =?utf-8?B?YVAyeHNWREtXc0diRkozcEFGaDhlbGtDWUsyaHBoTzJ3VDUweXIyMkZwNlNY?=
 =?utf-8?B?TUo0STY3Mzg3WHRDZkdVVCtlWHRCL3JLcU5ETmlEdTlYRTNhd0RtU2oycWZy?=
 =?utf-8?B?L28zTUFHUGlkN2NudmtVWE5qalEvSWNNSFNWZnJtR2RrTTZ5UUdVZjBTV2pP?=
 =?utf-8?B?cjlMQVRjd1BneGlBQXFaWFVid3hob0w4Z2JkbjF1cHdyN21wNllNRGZMREdE?=
 =?utf-8?B?eW4wMERxMFV5cXFJMHdrNTkyUGhDNmV5ZlkxK2U0WEZkVWdCbTNVemt6aTFD?=
 =?utf-8?B?RlZnUGowN0doWmFiTTZYcmdabTdDd1VqTFpxK0NxTkRkdDV6Ymx2WWRXSS94?=
 =?utf-8?B?NTFBRjFzM0t1YWpyMDNkVUw2Tnk4TGhmekpXempvRzlWanlLSHV5M2pweDVK?=
 =?utf-8?B?b3dqcCtENDlUS3dBK0FKaFJBZ3g2Qm9qSlJvWkJJTThZM2l4ZFZveXJiQisx?=
 =?utf-8?B?cFFFZzQ2elRUSU1IeDVKdHFLRGVOeHhpMDJMRVc5eG8rZ0s4ZkdITk5wdVZX?=
 =?utf-8?B?ekg1dXlWdTdnVHVFTWNtY01Ia2xtWk9PQzVrMW1iVElCNUk5cVh0NnlmTlFJ?=
 =?utf-8?B?ZkNsMFhnSk1lazgzbVBlR09DOFgvYTFaL3FlTFZIcFBMN1BnTi9jSEhpcUE3?=
 =?utf-8?B?ZnZjZmNIK1hpaG1WeFVMVHV2QkZwaUo2RmNJem1WWnRKaGRheVVxRUsxYXYx?=
 =?utf-8?B?STFtdFo4NVJNUjE0NTNEM2ZWUStOQjRHTjNXQ0lGQitseTVPWWVOdC9oOERP?=
 =?utf-8?B?VWt5UEk1Y0hvVjc3OUh1TFhIbVc1UE9iWkFGNWJRakdQR1RaQzh6V2RQQVdo?=
 =?utf-8?B?UUl5MTMxS2JPdDdYZHcyWHRPZTF5emovUS9pOVFKSzZVWTJreU1LT0NJSnNY?=
 =?utf-8?B?WmdTeEgyY3ZmVmtHOVRDVVFPSUE0VjE3U0hvUzQwUWZRYzNXbGNzeG1rT0lQ?=
 =?utf-8?B?Y0xnVGJxUTJ5Tld2R2kxRi9MTDgxbVRGeWdBVCtLeXIwYll5b0Z4WldOQ2NN?=
 =?utf-8?B?NUhUU2Iyc1kzdVJoTHlNc2FOeTFEV2VoVkxrS3RLb1RnZFd1dzZRbjJscnlT?=
 =?utf-8?B?YUF6MUgzUEtHUFdBVDE3a295T1k5SUdKOWRXajJZRzRTTksxakk3b3N4UW5I?=
 =?utf-8?B?Z21rUE1QMzlZUnJZVTZCVlZhempSSTNoZ1ZRSXAvYjEwKzJhUmRrRlp5TmRv?=
 =?utf-8?B?SEVFSHpPelBBYmUrRTVoUXdBb21FNncvZE51WmlDdlcvU2crRVFPcGpLQk5y?=
 =?utf-8?B?bEMydU9NTlpoZU5FUHVvaXFFbXFuMit2KzdneDVBTXlaWGRjaVhrY1hMYkFM?=
 =?utf-8?B?TGFtUmdFc3hNVkxnSlc2VzJsYUJadU1CeDZxRHJqbVR3OERCNVBjdThpdFZa?=
 =?utf-8?B?R3MzTTRQYnhibU5tV0dhRU9ZQU04Wms4U3k4WXd6RnpzQUwycTRFaGM2YThX?=
 =?utf-8?B?V1R1WHlRNHYvSFVjTWUzMzdUMTZlbFJVMzRPa3FjakdEcGk1R2hXM1ZIbm9w?=
 =?utf-8?Q?zUkj10n+9jrCGnvL2FdWQji9T?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6cdc7f66-1536-42db-d0a9-08dc4334c000
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2024 08:08:21.9710
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 3mCPsh/GeM01d7uN8ZV9qkdyE7gn/AB2PGV/iHtNR+inY/KVjVu4pSOUufNWftZpBouvQXoKd4ai9LgLUV2BOA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5087
X-OriginatorOrg: intel.com

On 2024/3/13 11:13, Baolu Lu wrote:
> On 2024/3/12 11:07, Yi Liu wrote:
>> On 2024/3/11 17:26, Tian, Kevin wrote:
>>>> From: Liu, Yi L <yi.l.liu@intel.com>
>>>> Sent: Sunday, March 10, 2024 9:06 PM
>>>>
>>>> On 2024/1/16 01:19, Jason Gunthorpe wrote:
>>>>> On Sun, Nov 26, 2023 at 10:34:21PM -0800, Yi Liu wrote:
>>>>>> +int iommu_replace_device_pasid(struct iommu_domain *domain,
>>>>>> +                   struct device *dev, ioasid_t pasid)
>>>>>> +{
>>>>>> +    struct iommu_group *group = dev->iommu_group;
>>>>>> +    struct iommu_domain *old_domain;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (!domain)
>>>>>> +        return -EINVAL;
>>>>>> +
>>>>>> +    if (!group)
>>>>>> +        return -ENODEV;
>>>>>> +
>>>>>> +    mutex_lock(&group->mutex);
>>>>>> +    __iommu_remove_group_pasid(group, pasid);
>>>>>
>>>>> It is not replace if you do remove first.
>>>>>
>>>>> Replace must just call set_dev_pasid and nothing much else..
>>>>
>>>> Seems uneasy to do it so far. The VT-d driver needs to get the old domain
>>>> first in order to do replacement. However, VT-d driver does not track the
>>>> attached domains of pasids. It gets domain of a pasid
>>>> by iommu_get_domain_for_dev_pasid(). Like
>>>> intel_iommu_remove_dev_pasid)
>>>> in link [1]. While the iommu layer exchanges the domain in the
>>>> group->pasid_array before calling into VT-d driver. So when calling into
>>>> VT-d driver, the domain got by iommu_get_domain_for_dev_pasid() is
>>>> already
>>>> the new domain. To solve it, we may need to let VT-d driver have its
>>>> own tracking on the domains. How about your thoughts? @Jason, @Kevin,
>>>> @Baoplu?
>>>>
>>>> [1]
>>>> https://github.com/torvalds/linux/blob/master/drivers/iommu/intel/iommu
>>>> .c#L4621C19-L4621C20
>>>>
>>>
>>> Jason's point was that the core helper should directly call set_dev_pasid
>>> and underlying iommu driver decides whether atomic replacement
>>> can be implemented inside the callback. If you first remove in the core
>>> then one can never implement a replace semantics.
>>
>> yeah, I got Jason's point. I'm raising an open to make the set_dev_pasid
>> callback to handle domain replacement. The existing intel iommu driver
>> does not track attached domains for pasid. But it's needed to know the
>> attached domain of a pasid and find the dev_pasid under the domain, hence
>> be able to clean up the old attachment and do the new attachment. Existing
>> code cannot work as I mentioned above. The group->pasid_xarray is updated
>> before calling set_dev_pasid callback. This means the underlying iommu
>> driver cannot get the old domain in the set_dev_pasid callback by the
>> iommu_get_domain_for_dev_pasid() helper.
>>
>> As above, I'm considering the possibility to track attached domains for
>> pasid by an xarray in the struct device_domain_info. Hence, intel iommu
>> driver could store/retrieve attached domain for pasids. If it's
>> replacement, the set_dev_pasid callback could find the attached domain and
>> the dev_pasid instance in the domain. Then be able to do detach and clean
>> up the tracking structures (e.g. dev_pasid).
> 
> Maintaining the same data structure in both core and individual iommu
> drivers seems to be duplicate effort. It appears that what you want here
> is just to get the currently used domain in the set_dev_pasid path. Is
> it possible to adjust the core code so that the pasid array is updated
> after the driver's set_dev_pasid callback returns success?

yes. how about your opinion? @Jason. I noticed the set_dev_pasid callback 
and pasid_array update is under the group->lock, so update it should be
fine to adjust the order to update pasid_array after set_dev_pasid returns.

-- 
Regards,
Yi Liu