Received-SPF: pass (google.com: domain of linux-kernel+bounces-201921-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
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 v6 01/10] iommu: Introduce domain attachment handle
Thread-Topic: [PATCH v6 01/10] iommu: Introduce domain attachment handle
Thread-Index: AQHar+toIpQ65UR7IECz6QN4BL8xubG42OXg
Date: Wed, 5 Jun 2024 08:02:38 +0000
Message-ID: <BN9PR11MB52767B3B1F441D9AA656AC258CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-2-baolu.lu@linux.intel.com>
In-Reply-To: <20240527040517.38561-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?tRlRYEV6+590rdYmP7n3zRWIj46tsyDBNQKi2TSQ74ByQkY6xBNXdToXMWVB?=
 =?us-ascii?Q?onGo2HJTN9tyquvYyYV6LRKrFhfyRneAzf8USOzUrhpT+3zJmiwb0AI0l1O1?=
 =?us-ascii?Q?DN0Kw9aELV70qhufYDvpoCAZxFFSTeXJ1THFuIDEw4la3/j2vo414iBPF1nC?=
 =?us-ascii?Q?GEsQgYSHX8U/a70Cxevc5eYTmaE/c0oBi7/Dfs8oCLjpV3GSl03B+Id4L/UN?=
 =?us-ascii?Q?uAhoMVTr6G/oY1pMAWbrci169UfBNq9KDKyNum0KbGQ5UTGVLkk4ocg2Fm+T?=
 =?us-ascii?Q?7Au7WBLGqbaQRqNlnu0xPDCdg4d7wgEx2fWxuGf4oKlwJHwgmZPmM4Z1E35W?=
 =?us-ascii?Q?tEBca7WlU3PfGSPY2gJVcbFMz8bIX9hZeLvyhKwasJpGNr6lE3wDBjOU6LzB?=
 =?us-ascii?Q?0iFMd1Sq5d2iAFKEteAfEDaGsSvziW4ANYN2Sd/E81Fq3kqVqkCXmyCWRODZ?=
 =?us-ascii?Q?c4gk0ZiqQDpBp0XbIYIhJkPRpnN7UxzD5KQ9uw8oy3rV4KEV8abdSX51zp4p?=
 =?us-ascii?Q?KorUe4K9tQvDLO8XExyz5u6YWjVJM3Mq1iDA0GZf6hMPXxias9Xn+FcCXgrh?=
 =?us-ascii?Q?YjIezxvCYyrww4BCoBBC3yM4BzE0Gs4+tRk9ynW5GSoAHsm1xKkfzyjGxwb6?=
 =?us-ascii?Q?Cwj9xqLAsi3KZbbYLit55mA2pVg8aJQ4Gpx9qbz4VYt809VWcOjBrIr06qB0?=
 =?us-ascii?Q?cza7f/0E8P5VbG2DKP4eK7+bGw0mqQqiTDsLy0gKWkXgHZlLAANcyht2kizd?=
 =?us-ascii?Q?4fhPvb5eN1H5r/ZqEFwix9M/aDcz8CRQ3IUq4nRdsDaoagk1UHPoWJGiJRum?=
 =?us-ascii?Q?IKSArRwMKU0yf5cSmpTSkseHfiisgRWHBT5xn4KuWL33ZCjs1S7KLJW6XFfo?=
 =?us-ascii?Q?VXztFlw0uhhWtNG1kTGtRWXRHt088iOMHMLdlPMJPktEh9cgIImEB5EGhLhT?=
 =?us-ascii?Q?cOtJUSYHMu1vNII2VWgANuRnv1rjxCb/Ayvh3KpjWtnZixuvKtkz8jB771r9?=
 =?us-ascii?Q?hT2gQtwwsT6pZeXfEm2ONJwr1ma8vWcJq9FDj+t2COIf3Bw6OcHSSl6qnnXM?=
 =?us-ascii?Q?5hxeO+kVbZ7hjwqNK2G/EnITh+7QLIfaoPFnogK0Q1IpG+onKJXTeXdLNKp3?=
 =?us-ascii?Q?XyzYCIn6DTbKynoh77MMqsRWedGque+eZ/0++CBp4EUlsZDwaGSs2d8AWmdy?=
 =?us-ascii?Q?lginxzciScJy75rfL1snr/83hlPBtbLFPUS8UAY4Cc/nYrrW4eO3o3tiaBhG?=
 =?us-ascii?Q?WK/8XOL6vJgMQDNRGEvAmQeLDIu3pdA0SRaSJf/dwEq79IsEa+osLHsoC5BT?=
 =?us-ascii?Q?MjEU/urOqmslOpFhK1luOi+92f0ti3L/XVtoCcQuiqyWOXxHAXGpXIGqrJBy?=
 =?us-ascii?Q?xI+j+Y8qVsVRehGAT4zC384uz9OP/wdvIo9mLp8ltNRkgZWHnr1cGstr8Tie?=
 =?us-ascii?Q?FLH86RHKyhjtBHLuymVgSXmFCM5zg/mLGWps8U0WSi9refgwxQ4/RnhgYxWS?=
 =?us-ascii?Q?QXIvQkWQKgpXspjLPa48saWxCGLhVut9pVNt8dP8ZklKmxVYtMIJ237SK7E1?=
 =?us-ascii?Q?K4AZoOix/xZP7XnfbAN7aU2raSGeJXFMn4SaminM?=
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: d9282bd4-6fcc-434c-b3e0-08dc8535de0d
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jun 2024 08:02:38.4712
 (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: FVQRSmAnDs/lUokn39a2c6Xm6Qx13E/fD+BNiS+d0IX/hiDB3+dK4PBxHx/I8sZecBLdaYAq7M2cG5I4L1E64Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7548
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, May 27, 2024 12:05 PM
>=20
> @@ -99,7 +99,9 @@ struct iommu_sva *iommu_sva_bind_device(struct device
> *dev, struct mm_struct *mm
>=20
>  	/* Search for an existing domain. */
>  	list_for_each_entry(domain, &mm->iommu_mm->sva_domains, next) {
> -		ret =3D iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid);
> +		handle->handle.domain =3D domain;
> +		ret =3D iommu_attach_device_pasid(domain, dev, iommu_mm-
> >pasid,
> +						&handle->handle);

move the setting of handle.domain into the helper?

> @@ -3382,11 +3383,9 @@ int iommu_attach_device_pasid(struct
> iommu_domain *domain,
>  		}
>  	}
>=20
> -	curr =3D xa_cmpxchg(&group->pasid_array, pasid, NULL, domain,
> GFP_KERNEL);
> -	if (curr) {
> -		ret =3D xa_err(curr) ? : -EBUSY;
> +	ret =3D xa_insert(&group->pasid_array, pasid, handle, GFP_KERNEL);
> +	if (ret)
>  		goto out_unlock;
> -	}

this leads to a slightly different implication comparing to the old code.

the domain pointer was always tracked in the old code but now the handle
is optional.

if iommu core only needs to know whether a pasid has been attached (as
in this helper), it still works as xa_insert() will mark the entry as reser=
ved
if handle=3D=3DNULL and next xa_insert() at the same index will fail due to
the entry being reserved.

But if certain path (other than iopf) in the iommu core needs to know
the exact domain pointer then this change breaks it.

Anyway some explanation is welcomed here for why this change is safe.

> @@ -3414,7 +3413,7 @@ void iommu_detach_device_pasid(struct
> iommu_domain *domain, struct device *dev,
>=20
>  	mutex_lock(&group->mutex);
>  	__iommu_remove_group_pasid(group, pasid, domain);
> -	WARN_ON(xa_erase(&group->pasid_array, pasid) !=3D domain);
> +	xa_erase(&group->pasid_array, pasid);

if the entry is valid do we want to keep the WARN_ON() upon handle->domain?