Received-SPF: pass (google.com: domain of linux-kernel+bounces-107160-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <13645a9f-239a-46c9-bde2-a1d5c365df4f@intel.com>
Date: Tue, 19 Mar 2024 15:29:39 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/8] iommu: Introduce a replace API for device pasid
Content-Language: en-US
To: Jason Gunthorpe <jgg@nvidia.com>
CC: Baolu Lu <baolu.lu@linux.intel.com>, "Tian, Kevin" <kevin.tian@intel.com>,
	"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>
 <749b23c7-ab0e-42b4-9992-e1867fc7d4d7@intel.com>
 <20240318165247.GD5825@nvidia.com>
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <20240318165247.GD5825@nvidia.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RXZISUtvcDVWR3JYYnBJY1NyZTdZSVkzaGNXeDJJNlRrQlQwaFN0by93bWtI?=
 =?utf-8?B?ZGJDTTNzK0lDakVPNFBwa0NpMkNzTU1UYkVZVm9FdmpUdHh0YjRpWjJ0Mjcz?=
 =?utf-8?B?cWovdGQ2RDBmc2JCbUgxeDFQcUd1ckNEanluKzgzYy9mZFFnQm12ZTY1V1NW?=
 =?utf-8?B?MjNJbDZHTDlRalVCcllyL2tKT3M4RS9Eb3AwQnQ0bE95ZUdpeVNsSy94dFlw?=
 =?utf-8?B?OUJQQnB5R0VWcElIN3JKSCtLK0U0ZG5ZZ0x1UGtTbUpqL1A3WHo0Q2RLVyt2?=
 =?utf-8?B?YkQ3TnlhWVNnRVovcFJadHl4YzJJLzhpaTNOckZnbDBXWldUSnhnZ2FjQzdz?=
 =?utf-8?B?clJ1R1JLWTkyNDI3eUtGUzlTVXFyWkFEbEo4NFVGMkZ1dE41RjVCdElMRGdP?=
 =?utf-8?B?VytKRC9HTnJob3M1azIrSVA0VWtVVHpSV3NGWkRMYjk5SUpIUmlWVitlWWhK?=
 =?utf-8?B?VGVmQ3M2UCtiWXE3eVIxcGhYU2EvTThCWFU4UHVFbENWRzF5eEg1YlE5dXJm?=
 =?utf-8?B?MDJCb1ZFVVc4RHpkbDJpdHdkaFpVS245d2VsclZKTEkvQnIxVmRoNzNYd0hK?=
 =?utf-8?B?QlJmSHhiMnJ4MUh3L3pFWVhaenJJaHdmZ3V4NkUxVmpOdzd1U2tTZTAwQVNh?=
 =?utf-8?B?N1ZJZTZ3YnpORlhJVWg3TCt3UlR3NlgwLzNhVFpFZkp6c2lLUUV5cnB1QTI3?=
 =?utf-8?B?RDFaVVh5czBPR2NlWXJaTC9FMzBMRm14TnRzUVNjSHBYdFpNMXZsS25xemNy?=
 =?utf-8?B?c0R6Y01kbEFiT0lsRzVpUUlhaFlZb3g5WUpIRGlyU0tNL0doR0toRVVSWFNK?=
 =?utf-8?B?TGtSNWdkQm1FWC8rQmRlSFhTMVUwb2U3UU5XOS83Y1Q1VXo5ZHF3YUhUK2Vp?=
 =?utf-8?B?bXJkMUJPakoxRnlxMG11Yjc1UWRiVnZoOVNXOUZaRTNpTGsrbUdjYS9RYUFV?=
 =?utf-8?B?VlVHOG9QeWhkbHM5S2I1VlhqeHlRVWltbUpYT21Vcjc5VUx2Qzhpa09uT2Rv?=
 =?utf-8?B?MHFQOTJ1VmJOSUF0cFQxRnEwcTdtU1VlTnNZSWdHNTJEQU9oRnhTdXlWQ2dB?=
 =?utf-8?B?clpWdlJDdjdWaUZqV2ZIQjVmWXZZcjhNR0RhcTNjcjJ4R2R2eG8wWGQzUy9I?=
 =?utf-8?B?ZGMvK0NKQ0lXNmJrRU83eFo0WTlJb1RqTytScU9jRTM1VlUwZGw1TElUcS9K?=
 =?utf-8?B?Q0RFUWdhTTRndFAzMkF5Wkl4YkFaWW54QWNKdlZLT3NUWnZCaW1ZdFEreFN1?=
 =?utf-8?B?cU93WlNXQnp2UHc3c3hpdHNtUVJ0RWdMS2dpR2ZWaU5XbDYvaWhYNUl5TS9O?=
 =?utf-8?B?MHhydlpoN3AvQXg1Umt0cmhwUXlGTVZhUUxoa2pxR1dIMElUd0dEcGl5Rmds?=
 =?utf-8?B?Y2dydUJ3V29vRW1FSG41SlVWaXZKWjhGUFl6Y0VkNmhEYTdOc0hkOFZHL1p3?=
 =?utf-8?B?cFRveVZFTEszWVNidjZ2WFNUM2hWaHc1QWJUL3V5TkRiSHlBdWFaQ2pNVExB?=
 =?utf-8?B?Rm5pRmNsdVJlYlA4cXNjZlBKY1U5SnpWcHU2QmFxUEVqWTRTTERoUFZORHBP?=
 =?utf-8?B?Z2RLSHJEN2JVME9CRXEyQzRUNVU4dUxiai80cGY2L0g1SThtTDdrSXpSNW1U?=
 =?utf-8?B?NlhSeXB3U2ppdmdZTStlOWY4RUNVaUpsR1hVWTFQTDRLVGttbmpIZW1HWmxL?=
 =?utf-8?B?Nk1JY0lmRlhicW5rZE1FcVlzT0RWNWJJNWxaYWk2ZGZIYUxITmQwWStxM3Zq?=
 =?utf-8?B?bWVRb1RBWjVIMlhPTzB2SUFEMXdrNDZOS3ovSkY5VmViUzJVY3NrYi9PSWla?=
 =?utf-8?B?Y3pXZm9zNGdzYy9tQmt0c0ptQzBrWmVWalNaN3hIYjVGaE1ORUlHRFhNcmRN?=
 =?utf-8?B?U3pRTDk2TDh2bUsrZll2ZUZsUExBajZGckRWUVhzWVpRaDJSb2Ezdnh6a3Q3?=
 =?utf-8?B?RFRubUtKQXV6dEovSEhZYjZtbXpYdEVQbTVtMzJsQm1hMnY1STlrTWFOa2xv?=
 =?utf-8?B?eHhDbG1POXdxOU1SM2d6YXNJMnNBVGlRMXNoaUhiSG95bDJZeHNYQmJTMkxM?=
 =?utf-8?B?bXRtTnVtcDZnT2praGdYenVUUUx5UDlxRzV4cGR0d25IcElqTkM5OEtWR3pr?=
 =?utf-8?Q?QYNYv1N2OO52X8jlrqnEfbe6K?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 981f377f-ffb6-48f8-66e5-08dc47e5dc04
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Mar 2024 07:26:14.7220
 (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: viGh7nzSSe8htbeQUFxcaCtwxXaRGJ6/Xb7sv9Q7BVpd3j4At49pEwtebxYvJ5vxUMUcS5XUNBqBg/ik5Fq1+g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR11MB8716
X-OriginatorOrg: intel.com

On 2024/3/19 00:52, Jason Gunthorpe wrote:
> On Wed, Mar 13, 2024 at 04:11:41PM +0800, Yi Liu wrote:
> 
>> 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.
> 
> Yes, it makes some sense
> 
> But, also I would like it very much if we just have the core pass in
> the actual old domain as a an addition function argument.

ok, this works too. For normal attach, just pass in a NULL old domain.

> I think we have some small mistakes in multi-device group error
> unwinding for remove because the global xarray can't isn't actually
> going to be correct in all scenarios.

do you mean the __iommu_remove_group_pasid() call in the below function?
Currently, it is called when __iommu_set_group_pasid() failed. However,
__iommu_set_group_pasid() may need to do remove itself when error happens,
so the helper can be more self-contained. Or you mean something else?

int iommu_attach_device_pasid(struct iommu_domain *domain,
			      struct device *dev, ioasid_t pasid)
{
	/* Caller must be a probed driver on dev */
	struct iommu_group *group = dev->iommu_group;
	void *curr;
	int ret;

	if (!domain->ops->set_dev_pasid)
		return -EOPNOTSUPP;

	if (!group)
		return -ENODEV;

	if (!dev_has_iommu(dev) || dev_iommu_ops(dev) != domain->owner)
		return -EINVAL;

	mutex_lock(&group->mutex);
	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
	if (curr) {
		ret = xa_err(curr) ? : -EBUSY;
		goto out_unlock;
	}

	ret = __iommu_set_group_pasid(domain, group, pasid);
	if (ret) {
		__iommu_remove_group_pasid(group, pasid);
		xa_erase(&group->pasid_array, pasid);
	}
out_unlock:
	mutex_unlock(&group->mutex);
	return ret;
}
EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);

https://github.com/torvalds/linux/blob/b3603fcb79b1036acae10602bffc4855a4b9af80/drivers/iommu/iommu.c#L3376

-- 
Regards,
Yi Liu