Received-SPF: pass (google.com: domain of linux-kernel+bounces-122489-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
From: "Tian, Kevin" <kevin.tian@intel.com>
To: Lu Baolu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, "Will
 Deacon" <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, "Jason
 Gunthorpe" <jgg@ziepe.ca>
CC: "Zhang, Tina" <tina.zhang@intel.com>, "Liu, Yi L" <yi.l.liu@intel.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface
Thread-Topic: [PATCH 01/12] iommu/vt-d: Add cache tag assignment interface
Thread-Index: AQHaflqwfTup1ArZ00iKq9wT+H7xNLFMvHPg
Date: Thu, 28 Mar 2024 07:12:06 +0000
Message-ID: <BN9PR11MB5276A6DDE82623A32FF80F4F8C3B2@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240325021705.249769-1-baolu.lu@linux.intel.com>
 <20240325021705.249769-2-baolu.lu@linux.intel.com>
In-Reply-To: <20240325021705.249769-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?LZ5Lm041xeGVIn2tc4zqsYi0qycyDFn9yhxdB28q+cReehgpCImzVAiHSzRr?=
 =?us-ascii?Q?VD5NrBi6E2FrgzcbeOqxVFC/37mO6wplGBpzTiCsuIJ6E2FLbYS1mVYWaRcw?=
 =?us-ascii?Q?5ronuVypl+q5dRW1D4epkFb8BvMkpE/EJ2mdQuN9lDlIMWeiAVA3nzSVJsPl?=
 =?us-ascii?Q?HJ0rwHXF0F6BrN3+mwAlMdvpTo+tK5PC662bzngyha4WvDTG7YUj0C5bhK1p?=
 =?us-ascii?Q?B09RjmWJZiBJPmEOjyoXG6sJZ//5k70rqIPrqGszuD/3I1mh9rJb9pqr7V2J?=
 =?us-ascii?Q?KrIFAhq5OKpKU/j1Jye2da7Zamk5wOWXEjcM/hCF3SRcC7HSi1DPsepr0PdH?=
 =?us-ascii?Q?pv0SFNHwvocbkahe90opNeFDe0dHHNt1seiAVXbY7CL0gIdd2zpH/wN5Df4D?=
 =?us-ascii?Q?0DAOBYYi7V3tTr5VwG1264GQbAANprlTZ7C9cg2UJAkWRppnou3+QuwujZR0?=
 =?us-ascii?Q?5TBPjv4/N4LfjtHw3xX5KZf7gZnB2Vh+3KXYbfJQ132FCPxzHJVkZroVbSa1?=
 =?us-ascii?Q?IvBKlQDclq9K/1CL417Uz9KIcE9MY4i20llnArRrfx/vVu4f5n7SzDHFTz+B?=
 =?us-ascii?Q?7J7IwQ3Q/n2Y6eT6XgpU4ZgQ0z/INatI6GvFAfsOdjubx9/WXe2cxRFa3DBb?=
 =?us-ascii?Q?W8wIdblZrqaEP4qXHKNkfnm/raylCl5O43h/h1u5+56/U44nUSoLFUd8uy1w?=
 =?us-ascii?Q?KqogKAb6nfpjkNiLM7O1YsukofNu+JRhsbqssQv4ik1MZt3v3pyQmLAEfuM+?=
 =?us-ascii?Q?z0rvF+IDxJ1BArR8l/lK3n7EQP/wfxEaD/RlK7O158FmGnGQykqgr9U8mXfS?=
 =?us-ascii?Q?h9vJcyYhoUBrDzgLzCcmYqfy4LZ4unsU809t7xqlHvnQy5Cgs29WtY2w6f+L?=
 =?us-ascii?Q?uEnPvXSiRE/jcAQbD/fZuEjSXi7KUGf0pGMIYD8vGD9PAtIEyVcZZ/WBXlfc?=
 =?us-ascii?Q?rAdU/FPd4v0XceHJiikcgOkhb5dTFC9VUyZgdUNEedX3H7m6+fFbr6FaaJT3?=
 =?us-ascii?Q?nK6bc+gMYg/80Afg5uTp6qtynMAUmViGv5f6FoQOstGsIfot9kj6hQ/biSNd?=
 =?us-ascii?Q?aeoZYyk5RbtTW9AZPTkK+8gqOjTknFVPUaJi4+9cdiZJqln87GZ1cK6GqLuQ?=
 =?us-ascii?Q?LbnofqDJJvhDzuMNOy0wFRC0xfsjwS4F4lBN4lIEf2viKdh80DGM2LrfYXGd?=
 =?us-ascii?Q?tTCepDQsqO6PjOaOWLsvJYSs7OPNwk8yeogAaGue0r2NJ7H89vhGGlUanyay?=
 =?us-ascii?Q?M8j9UdqO7tow+/rH4aTl6ej1yUAQ6YsT5Bdf6h7lKX15J040Y388S+hFvSYR?=
 =?us-ascii?Q?b3SbpxdZAMYcIkYhmsN6eGaGafL+VjpW+Nw+XWcGfSWMkoLKA0j/ZTMYbexa?=
 =?us-ascii?Q?ayrnzySuXoxpAa/ragG1V/NUr6sC9BxmqPU+EiXau+aZ20FlidkNa0U4RWHf?=
 =?us-ascii?Q?72SoD1Vp6etYE/t8Cl8BpvfyZIbHVLSjZdYxNEBrVr4DIOBmO6AQAJ/B2HaH?=
 =?us-ascii?Q?V7T6HCyqyWzpOdi4vVdzsIDI3LVZsYMkMAqvQfY3fE3XUrRvs70glu2f55Kt?=
 =?us-ascii?Q?/HFYMNfVpRL6MwwuMVnK1ENAXHgoyyYa+naIC/Xd?=
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: 24f50580-c462-498b-64f6-08dc4ef66098
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Mar 2024 07:12:06.9142
 (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: iRjY1mmrTzMThOPg6F4MjPTacKVZHI+Hk+QhbdALiGiK/qsDN5VQeMmTatnilKzanIyt5uVD2u1WNX/d74ilVw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR11MB7282
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, March 25, 2024 10:17 AM
>=20
> +enum cache_tag_type {
> +	CACHE_TAG_TYPE_IOTLB,
> +	CACHE_TAG_TYPE_DEVTLB,
> +	CACHE_TAG_TYPE_PARENT_IOTLB,
> +	CACHE_TAG_TYPE_PARENT_DEVTLB,
> +};

'_TYPE_' can be removed to make it shorter

> +
> +/* Checks if an existing cache tag can be reused for a new association. =
*/
> +static bool cache_tag_reusable(struct cache_tag *tag, u16 domain_id,
> +			       struct intel_iommu *iommu, struct device *dev,
> +			       ioasid_t pasid, enum cache_tag_type type)

cache_tage_match()

> +{
> +	if (tag->type !=3D type)
> +		return false;
> +
> +	if (tag->domain_id !=3D domain_id || tag->pasid !=3D pasid)
> +		return false;
> +
> +	if (type =3D=3D CACHE_TAG_TYPE_IOTLB)
> +		return tag->iommu =3D=3D iommu;
> +
> +	if (type =3D=3D CACHE_TAG_TYPE_DEVTLB)
> +		return tag->dev =3D=3D dev;
> +
> +	return false;

why do you disallow PARENT_TYPE from reusing? It's not uncommon
to have two devices attached to a same nested domain hence with
the same parent domain. Disallowing tag reuse implies unnecessarily
duplicated cache flushes...

> +}
> +
> +/* Assign a cache tag with specified type to domain. */
> +static int cache_tag_assign(struct dmar_domain *domain, u16 did,
> +			    struct device *dev, ioasid_t pasid,
> +			    enum cache_tag_type type)
> +{
> +	struct device_domain_info *info =3D dev_iommu_priv_get(dev);
> +	struct intel_iommu *iommu =3D info->iommu;
> +	struct cache_tag *tag, *temp;
> +	unsigned long flags;
> +
> +	tag =3D kzalloc(sizeof(*tag), GFP_KERNEL);
> +	if (!tag)
> +		return -ENOMEM;
> +
> +	tag->type =3D type;
> +	tag->iommu =3D iommu;
> +	tag->dev =3D dev;

should we set tag->dev only for DEVTLB type? It's a bit confusing to set
it for IOTLB type which doesn't care about device. Actually doing so
is instead misleading as the 1st device creating the tag may have been
detached but then it will still show up in the trace when the last device
detach destroying the tag.

> +static int __cache_tag_assign_parent_domain(struct dmar_domain
> *domain, u16 did,
> +					    struct device *dev, ioasid_t pasid)

this pair is similar to the earlier one except the difference on type.

what about keeping just one pair which accepts a 'parent' argument to
decide the type internally?


> +/*
> + * Assigns cache tags to a domain when it's associated with a device's
> + * PASID using a specific domain ID.

s/Assigns/Assign/

> +
> +	did =3D domain_id_iommu(domain, iommu);
> +	ret =3D cache_tag_assign_domain(domain, did, dev,
> IOMMU_NO_PASID);

there are many occurrences of this pattern. What about passing in
a 'iommu' parameter and getting 'did' inside the helper? for svm
it can be specialized internally too.

> @@ -4607,10 +4623,11 @@ static void
> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  	 */
>  	if (domain->type =3D=3D IOMMU_DOMAIN_SVA) {
>  		intel_svm_remove_dev_pasid(dev, pasid);
> +		cache_tag_unassign_domain(dmar_domain,
> +					  FLPT_DEFAULT_DID, dev, pasid);

is it correct to destroy the tag before teardown completes, e.g. iotlb stil=
l
needs to be flushed in intel_pasid_tear_down_entry()?