Received-SPF: pass (google.com: domain of linux-kernel+bounces-179566-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Lu Baolu <baolu.lu@linux.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>
CC: "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>, Jason Gunthorpe <jgg@nvidia.com>
Subject: RE: [PATCH v5 1/9] iommu: Introduce domain attachment handle
Thread-Topic: [PATCH v5 1/9] iommu: Introduce domain attachment handle
Thread-Index: AQHamw76XegsRd/ffUStCLWsuFz9nbGX86hA
Date: Wed, 15 May 2024 07:17:29 +0000
Message-ID: <BN9PR11MB5276F0DAE4B5F63513C38A158CEC2@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240430145710.68112-1-baolu.lu@linux.intel.com>
 <20240430145710.68112-2-baolu.lu@linux.intel.com>
In-Reply-To: <20240430145710.68112-2-baolu.lu@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?yFRcmbPHLsR0nh9VLk+t8gNQ/E3jVEBDF1OHRvBDtGb6oi2W9WXe6mnvBiLj?=
 =?us-ascii?Q?gflLhFefxbPOycMjzJhFq4ymP9epOPhtnsxQCmbHkTXysGyviytlWzLPkrR6?=
 =?us-ascii?Q?Iessu3v6Ry0in6IBO/TxZOXyF/tUsr3ab2luXaherAatDk/9xtFOMg64z/Vx?=
 =?us-ascii?Q?gXMWN+UUyFY/fOZS6muUUu0WEZqJFKMjdZeDeRC/hxmdesegvhCSHvT3Wylr?=
 =?us-ascii?Q?y1Hm5jJYqzuol8ejfIZW0U3ZxUuiviPIwdh57dTSjkF/rnGql5biKUGlcRAk?=
 =?us-ascii?Q?/idnPQmWEpk/0wouj8y4bwZ9DxELNwC076Wu1YB/f3mrlGR9j4CgdYkCzso3?=
 =?us-ascii?Q?822XL2rXDbBCMDqL/K2uHDwk0t++xDmvcqAV4WMsv6StwQIpVPtQ346Zy7fZ?=
 =?us-ascii?Q?5sO4elytGOvuYWFLnyPI7HX87WreO2Thg3K6W9btmmhr7vsnf6RlGQK7Myf4?=
 =?us-ascii?Q?fXD7XExm3XFvAo9TaHzQenMasMjPscbu+kHrNNNXLN9viLDTC67LSJLA3v75?=
 =?us-ascii?Q?n2eTBdHNvzPv5JNzhFv0nafNoeV8srYKWFnxdqdowVFwZpPoSCGP3lYKrZVA?=
 =?us-ascii?Q?78wH4od0dCjiBsdSkSVPSCbqkCHevePatsfro5viqhtEE4Lb8A/AcS/zr8Dt?=
 =?us-ascii?Q?SqEDc1DIf4ywIrkuJi54s10fK10wYGr3SW/1+WUf6EYpaZ5+ZkN2x665AiyT?=
 =?us-ascii?Q?QjFFecbLxurO6YVAPeGGY2Fyl7QkivjU6UV2+HD4PbEtitZZgwLPXfEpa1g2?=
 =?us-ascii?Q?fjR9If5ZzHJTHAr++fMnx47QD9DOycMQA8y5dL/34yEFJomkbfy6vYw8M/xz?=
 =?us-ascii?Q?xfbfKmhxItBAwA97jukSukt7phAt7NidWZTH90IPa8vttohF41cye9ukPfMo?=
 =?us-ascii?Q?2foIG8KyRt/yGfxUk3VXWb+y2qQn/THqs58DLsIUCsk3WmckGiO108PlRjZL?=
 =?us-ascii?Q?bRJjRR1MJaxBPP4p9KEoFrp5upzCebpf6HtJ+Y350pVAUl2/H47jYhTKu42K?=
 =?us-ascii?Q?qNizCFfBN6f4jKNnu32NDVDhuITHy1+VqiZ7thmJALiVyDAhk5FvEBqbiim2?=
 =?us-ascii?Q?TWd/WlDYxJoFMxTKlVEWGgDo5U4lH+bx0GbDinVME0qN9ljMHj+dgg3rTUmR?=
 =?us-ascii?Q?AaHx/+MBndcvVyNMmc224Sw/ODIg4MK+weomWbF2AdBsXR3OTlEoWn+5sP+Y?=
 =?us-ascii?Q?MiJmM2v01cLnc9PZ3KyxvSN+5O2clIsyY1A0FyPvLxjDYfisThxZEPeDUHYb?=
 =?us-ascii?Q?SnFNSt9GjqVAxHpKbBowC10Y2f3QCnHMkPeSL52FIer66AaEJwqfpkgUIGiJ?=
 =?us-ascii?Q?2yHaosH68vm0cPBnd7R9L9rI1mJQjaa1SK+2V0TdhGm3kCGrujhHkifhAmXV?=
 =?us-ascii?Q?opLPqsHxi/ypULOrrLPUEWhyKkDMC4rYQeDPH0dop7imM/MEWngAYRgfVKXk?=
 =?us-ascii?Q?XkLEYyvnnitM6iiipuR47HMOJ+6zJZ4z+kBxb9BAHe/0kMdicrJagEQFobwL?=
 =?us-ascii?Q?37rOq9xmIOfdnZ38yL8wuXQT7iRTtm0Atm84l4vOKhskXpH0DVyKUzFbc9Bo?=
 =?us-ascii?Q?kP18YCvC22UjKmiiWI8KFK8ExJSaSEXLhdCDNdz6?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: BN9PR11MB5276.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c053e132-dc8a-421d-1f0e-08dc74af1488
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 May 2024 07:17:29.1709
 (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: wv73bUZWN/QBnwh9g+flygScKjcLnCElFjg7ALUxZRZHwnVmHBR+G5XLBC2c0ORCDQ+w0PYMEbLLplRYVs6h0Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5223
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, April 30, 2024 10:57 PM
>=20
> +/* Add an attach handle to the group's pasid array. */
> +static struct iommu_attach_handle *
> +iommu_attach_handle_set(struct iommu_domain *domain,
> +			struct iommu_group *group, ioasid_t pasid)
> +{
> +	struct iommu_attach_handle *handle;
> +	void *curr;
> +
> +	handle =3D kzalloc(sizeof(*handle), GFP_KERNEL);
> +	if (!handle)
> +		return ERR_PTR(-ENOMEM);
> +
> +	handle->domain =3D domain;
> +	curr =3D xa_cmpxchg(&group->pasid_array, pasid, NULL, handle,
> GFP_KERNEL);

this could be just xa_insert() which returns -EBUSY if the entry isn't NULL=
.

> +	if (curr) {
> +		kfree(handle);
> +		return xa_err(curr) ? curr : ERR_PTR(-EBUSY);

'curr' is not an error pointer. You need ERR_PTR(xa_err(curr)).

>  int iommu_attach_group(struct iommu_domain *domain, struct
> iommu_group *group)
>  {
> +	struct iommu_attach_handle *handle;
>  	int ret;
>=20
>  	mutex_lock(&group->mutex);
> +	handle =3D iommu_attach_handle_set(domain, group,
> IOMMU_NO_PASID);
> +	if (IS_ERR(handle)) {
> +		ret =3D PTR_ERR(handle);
> +		goto out_unlock;
> +	}
>  	ret =3D __iommu_attach_group(domain, group);
> +	if (ret)
> +		goto out_remove_handle;
>  	mutex_unlock(&group->mutex);

It's slightly better to set the handler after __iommu_attach_group().

doing so is aligned to the sequence in iommu_group_replace_domain().

> @@ -2211,13 +2307,33 @@ EXPORT_SYMBOL_GPL(iommu_attach_group);
>  int iommu_group_replace_domain(struct iommu_group *group,
>  			       struct iommu_domain *new_domain)
>  {
> +	struct iommu_domain *old_domain =3D group->domain;
> +	struct iommu_attach_handle *handle;
>  	int ret;
>=20
>  	if (!new_domain)
>  		return -EINVAL;
>=20
> +	if (new_domain =3D=3D old_domain)
> +		return 0;
> +

__iommu_group_set_domain() already does the check.