Received-SPF: pass (google.com: domain of linux-kernel+bounces-201955-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::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>
Subject: RE: [PATCH v6 02/10] iommu: Remove sva handle list
Thread-Topic: [PATCH v6 02/10] iommu: Remove sva handle list
Thread-Index: AQHar+tqwnlyEnOTnkSkjxonj6LyULG43azA
Date: Wed, 5 Jun 2024 08:15:53 +0000
Message-ID: <BN9PR11MB527658A85092F88329EB73E98CF92@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240527040517.38561-1-baolu.lu@linux.intel.com>
 <20240527040517.38561-3-baolu.lu@linux.intel.com>
In-Reply-To: <20240527040517.38561-3-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?yOD91JDkEaZad+akrnDCItskzPfTx2MaO0EyNwZcG7Y/gUa7MLQX/kt3ndxw?=
 =?us-ascii?Q?MUNAXLHQqFqlNli6ZdxP7E0jCnqWJIOZwwZ6YMjLtD4LmpOI3FLWJhCjAFJy?=
 =?us-ascii?Q?OOviVr8rk5p8GfyFPuRQWro5YqcA0orcmGfhYo7UEgK0Cd3G5Tvgnck6f7DU?=
 =?us-ascii?Q?Zsb1hxtpQ+uHjiGcCfeFAcdaVAPMz/IXxDQjtvhDrRo1jRHXOwz1sMKfpgMu?=
 =?us-ascii?Q?rWsn1PsyaRw3qsrmp+Q48YJiaAG02PJZl9QegMU9042CopZKxVrfken4APoB?=
 =?us-ascii?Q?jLz039+TNQlWf6+4lH2dnPQ10Yua5k61avD+IFEnzO0jxRgizb6DDx6Rml85?=
 =?us-ascii?Q?JFexFZXks13rVaqdZuZLPaGViOWrBgD4wtu7/ceRKp/EEN/RtKPxkDBzZ8WX?=
 =?us-ascii?Q?6Cqeke8e4sfDjWOR501uSfPp2v0LmJy0GDropuoCi5g4booDSKMJvEcGUf17?=
 =?us-ascii?Q?+KfQf+b9nCQfb1KE9THL1+CH3wX5zZ9Ebn5HkKE4yk5GpqjyLPr5mY1ZoCyr?=
 =?us-ascii?Q?z/Bqrnj/ym940+ppN7fwl1msJ8ZbYm4nLK251T3uJeMe3FL5iGAT4waOm7sp?=
 =?us-ascii?Q?pbVVknJZUhcbaQCCT5U7GkOOpM5+fx6ns/ivgbdGOqYeIDBnKkLDFoPJC4+T?=
 =?us-ascii?Q?hk2FAbhOF5eLvdOfsV8lc+VKMKCvgmkaW3Zzq2JpkaA/RMphtPWWFaE/OlzG?=
 =?us-ascii?Q?OxV1vHGUxKL4otRIWjjEdz2iLlKqfR7VDqKMCJ9duKjnfNty+3+9z0sc8SeL?=
 =?us-ascii?Q?QKY0bSvEPHRKQ+IXyY78GFAogMciDu/4BhQyHC9xc0iNmoSmapF8820Y2cNh?=
 =?us-ascii?Q?ar1pW84hZKbLGEyn2+HY05tyFO7VbMgr9MXE5klqA/3WHFkyotKanj0dBejI?=
 =?us-ascii?Q?5PIw8aWb6mIlVU9Xp2OOUwp7ch7uH0D40KZ8HmNVk0q+tJb1tquLNiwuaVIM?=
 =?us-ascii?Q?WWoYKIleKdc99guWoKVR9WEWCWOXzC/VHSsvEOc6Ft+UpOYj7GormQp5xtWw?=
 =?us-ascii?Q?i9qVhhHH2qmyowqhpEqxusfxFNLuYHzaBGMpVQgkWeKz6l2ZtpuLe7BCmmAK?=
 =?us-ascii?Q?KtvbNn12I+gO5TO081e7LMhERZIQZwMSYvcIqWufWiUROXeN4hdkdTqlGf99?=
 =?us-ascii?Q?BWB1I9wQTSTWhc3Di4DKFiOxVsVod9K6oGfEUs+IpTEQRAWrwUyPvCfaenR0?=
 =?us-ascii?Q?8xxYOv/2EpsP9I88IHPjLD2s+oJSu20geNJJfBzyo2rpbQJWh8Cyckd3Q3Np?=
 =?us-ascii?Q?LE1ss2qv3h9lOEpD8mgko5xkJytWXxoqGPPNXqQSmzMTtxHKMQY1/fGxM4vz?=
 =?us-ascii?Q?GjwGiE4SYicr6y2AgOCkTTdVNoKkPMlT6036TM+4P4Kbpf71F4EtruVN93Ek?=
 =?us-ascii?Q?L2xGggyUxbqZ7WnGkCam+xwuCfAVWK2C8jkxEx9iq/eqVHbR4ftfYBwUN+3H?=
 =?us-ascii?Q?T5cnhDPDPmlQc09ni3uCVEiM4CFzBkYBD1ErZJOsTzyVyLAsENYytOMHali2?=
 =?us-ascii?Q?j66nqub3iLqSU4R7yR5hFkFGtOvZA66dtWn+i9Igpk4lGfq6DBDvi5Ka4z+W?=
 =?us-ascii?Q?gMh9TfwEyzBLtbMf/oTxY1YH+INT5YS+T952jiHm?=
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: d3d2aa10-e87b-4982-6dba-08dc8537b804
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Jun 2024 08:15:53.6141
 (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: ofY4tfFZQFDwgRB/HercUd4uAs6xZxnMpawamjDf4Ze3b+14eq6F7qhdjzNuowNzc6tyqZA/B3ehVlM5cHAIww==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7258
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, May 27, 2024 12:05 PM
>=20
> @@ -69,11 +68,16 @@ static struct iommu_mm_data
> *iommu_alloc_mm_data(struct mm_struct *mm, struct de
>   */
>  struct iommu_sva *iommu_sva_bind_device(struct device *dev, struct
> mm_struct *mm)
>  {
> +	struct iommu_group *group =3D dev->iommu_group;
> +	struct iommu_attach_handle *attach_handle;
>  	struct iommu_mm_data *iommu_mm;
>  	struct iommu_domain *domain;
>  	struct iommu_sva *handle;

it's confusing to have both 'handle' and 'attach_handle' in one function.

Clearer to rename 'handle' as 'sva'.

>  	int ret;
>=20
> +	if (!group)
> +		return ERR_PTR(-ENODEV);
> +
>  	mutex_lock(&iommu_sva_lock);
>=20
>  	/* Allocate mm->pasid if necessary. */
> @@ -83,12 +87,13 @@ struct iommu_sva *iommu_sva_bind_device(struct
> device *dev, struct mm_struct *mm
>  		goto out_unlock;
>  	}
>=20
> -	list_for_each_entry(handle, &mm->iommu_mm->sva_handles,
> handle_item) {
> -		if (handle->dev =3D=3D dev) {
> -			refcount_inc(&handle->users);
> -			mutex_unlock(&iommu_sva_lock);
> -			return handle;
> -		}
> +	/* A bond already exists, just take a reference`. */
> +	attach_handle =3D iommu_attach_handle_get(group, iommu_mm-
> >pasid, IOMMU_DOMAIN_SVA);
> +	if (!IS_ERR(attach_handle)) {
> +		handle =3D container_of(attach_handle, struct iommu_sva,
> handle);
> +		refcount_inc(&handle->users);
> +		mutex_unlock(&iommu_sva_lock);
> +		return handle;
>  	}

It's counter-intuitive to move forward when an error is returned.

e.g. if it's -EBUSY indicating the pasid already used for another type then
following attempts shouldn't been tried.

probably we should have iommu_attach_handle_get() return NULL
instead of -ENOENT when the entry is free? then:

	attach_handle =3D iommu_attach_handle_get();
	if (IS_ERR(attach_handle)) {
		ret =3D PTR_ERR(attach_handle);
		goto out_unlock;
	} else if (attach_handle) {
		/* matched and increase handle->users */
	}

	/* free entry falls through */

But then there is one potential issue with the design that 'handle'
can be optional in iommu_attach_device_pasid(). In that case
xa_load returns NULL then we cannot differentiate a real unused
PASID vs. one which has been attached w/o an handle.

Does it suggest that having the caller to always provide a handle
makes more sense?