Received-SPF: pass (google.com: domain of linux-kernel+bounces-42404-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: Ethan Zhao <haifeng.zhao@linux.intel.com>, "baolu.lu@linux.intel.com"
	<baolu.lu@linux.intel.com>, "bhelgaas@google.com" <bhelgaas@google.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>, "jgg@ziepe.ca" <jgg@ziepe.ca>
CC: "dwmw2@infradead.org" <dwmw2@infradead.org>, "will@kernel.org"
	<will@kernel.org>, "lukas@wunner.de" <lukas@wunner.de>, "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>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: RE: [PATCH v12 5/5] iommu/vt-d: improve ITE fault handling if target
 device isn't present
Thread-Topic: [PATCH v12 5/5] iommu/vt-d: improve ITE fault handling if target
 device isn't present
Thread-Index: AQHaUmY538BcsBx6UEOae7WqjB1D5LDweayA
Date: Mon, 29 Jan 2024 09:06:36 +0000
Message-ID: <BN9PR11MB52761CC3E5F08D4B7BAD7F918C7E2@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240129034924.817005-1-haifeng.zhao@linux.intel.com>
 <20240129034924.817005-6-haifeng.zhao@linux.intel.com>
In-Reply-To: <20240129034924.817005-6-haifeng.zhao@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?z7iO5wV2mvPbHunK7XcaOMscQ/5q0cC0kqGB6G3Bay3/ybArxJKRMw+hDyRe?=
 =?us-ascii?Q?ayRyRZPBch7rAeMLeD0KdZHZXFvAGrBHSI7ecyexNpxlfOJtmfJMVL94SO9O?=
 =?us-ascii?Q?lwLcXhlcY/rr6ALNfBUtGdCXmpGv34/QCQUCRD63mZJpsPeW7RaRbEA+HmUA?=
 =?us-ascii?Q?S3DurgcVtvn7ZZdTqnGBJ3AxA5m62QqrOrvafT0n3xs2xOx4A8NsxowBcKfB?=
 =?us-ascii?Q?cXzNlK0licPLvvwsTUr4a+dLuAAAgEMymp367Vt21JwtboVOaoP80w8cdB5R?=
 =?us-ascii?Q?5aTut/0SvqnRGtpDPtM2MnZVYx5d3Ul0wFPj2ZYFuZFn/L3Ohg6rnfcHKcqq?=
 =?us-ascii?Q?GV+h8SD57M7weG0utWmh+39pCehqTt/1jqMtRPxkGRwD+HVlc3jfDkp9rSHE?=
 =?us-ascii?Q?c9E/sGmh3JVmtMzmPaUZNBJJIONOLLKkyPtIhM4whesgVsKeWBxPXSniezjx?=
 =?us-ascii?Q?mAJwlIpfoe2L5GuEPp0xQMBBiVOyk04DNEp1F37dvlK+xhLI7MFS5wYbi/0F?=
 =?us-ascii?Q?09gQ2keUw9kh5J/OwPPmRrd6CB6v6kT0lFYsRRlR/ZJoZIUCupoPxz4SqE1J?=
 =?us-ascii?Q?xOpHQC0OZKBMJCaQRmbJJ6VtC3jsY+xOih1Spfv5JRhJTzZFTwUGNslry4GJ?=
 =?us-ascii?Q?Qt8NzdSDi8KcEaPr2k0AzO5VhHWf9lsbFY2YhKMGhVJqH9qQTg26aEhimBUl?=
 =?us-ascii?Q?LAXgXMJr6BmRvv/OBTbECqJAr+kN6tRHPyQDDTjnI+9BIbFkfNbQX7ut/HBb?=
 =?us-ascii?Q?naFXIJUFq7/rDumWU9X4zuqOKmRAkRQX5jvdq7mZElfPF7XHEeONBHPwvOC6?=
 =?us-ascii?Q?rf70tu5rpeEqG13yrjoHQmYi87lHD3+ItEFPtWhKGMxyOnSkN6mFHhdciygu?=
 =?us-ascii?Q?9P5eNRwkkhtfWCnKNkGUy5qJW48bp/IfmCl3bq5almckMlFJ+o/ArfgaEAgK?=
 =?us-ascii?Q?f+Iizli+qV+o5SZucpEBk0eXD5KEbWNlLd2fURraRgGcli7I4aOyJlqT6peN?=
 =?us-ascii?Q?Gedp8qg1QAWW2wEf3OwznY7zlD2uGq+xhK9ayT0IYy2CsspRA1ODU51rTXYY?=
 =?us-ascii?Q?Vo9vEp4ZG16zwFQ6tAQcxeYvjCgVgZEuazJw52qjqyQFTvQVZ6jddzXIvMUy?=
 =?us-ascii?Q?JFDjU+cGZUeuC4nY5PTSV5WBB/pVAYCxQe/DaTPdv+KZU5Tcp/chJqqUOJv/?=
 =?us-ascii?Q?haTakaFVD2huv4alwauJrhE/jDlrfKXCMANtP+XuIEK7hGcfFXGomgqLRXD2?=
 =?us-ascii?Q?14QpycWS+iyGjNcv/3rOwML4MZfqy4a2jQJLBM6wM+jIpLM0QBsDg1Z9mDqt?=
 =?us-ascii?Q?l0B3s/04y52YQz4DMcR2RC+ZIV5Znoboo75CuQWVfu0w9UrCCswtxSet9FO6?=
 =?us-ascii?Q?QcCUx7e/PcwmWZPA4V92wwac06m2uCVU6SVzuA2xlDSLwUvObtpwxRqduxQz?=
 =?us-ascii?Q?GOFIhbufJ9mHusDNqIAagX4Lt6CwygPJFq5/WQgTRO3TLBG6IdMJ8/+97+vk?=
 =?us-ascii?Q?7tcrvoRm6bFCP+Cf6lhuphDq7HkXnpxZ48OecVmZ1BH2tGtfgYQKUPNpn32s?=
 =?us-ascii?Q?swWmV44fuoZPVYraEdXc+E6Ad9Picd3akkmRaIDD?=
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: 4188209c-dc2a-4515-3bc7-08dc20a998c8
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jan 2024 09:06:36.4350
 (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: qn705kve2I0PM57S/rUNC+ikfdmeH3DDRLthOlv7uxTY3FEmr0pM0NuwSdAhJtB/O+NYtvbZAOjsBDj6Ukkmrw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7596
X-OriginatorOrg: intel.com

> From: Ethan Zhao <haifeng.zhao@linux.intel.com>
> Sent: Monday, January 29, 2024 11:49 AM
>=20
> Because surprise removal could happen anytime, e.g. user could request sa=
fe
> removal to EP(endpoint device) via sysfs and brings its link down to do
> surprise removal cocurrently. such aggressive cases would cause ATS
> invalidation request issued to non-existence target device, then deadly
> loop to retry that request after ITE fault triggered in interrupt context=
.
> this patch aims to optimize the ITE handling by checking the target devic=
e
> presence state to avoid retrying the timeout request blindly, thus avoid
> hard lockup or system hang.
>=20
> Signed-off-by: Ethan Zhao <haifeng.zhao@linux.intel.com>
> ---
>  drivers/iommu/intel/dmar.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>=20
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 814134e9aa5a..2e214b43725c 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -1272,6 +1272,7 @@ static int qi_check_fault(struct intel_iommu
> *iommu, int index, int wait_index,
>  {
>  	u32 fault;
>  	int head, tail;
> +	u64 iqe_err, ite_sid;
>  	struct q_inval *qi =3D iommu->qi;
>  	int shift =3D qi_shift(iommu);
>=20
> @@ -1316,6 +1317,13 @@ static int qi_check_fault(struct intel_iommu
> *iommu, int index, int wait_index,
>  		tail =3D readl(iommu->reg + DMAR_IQT_REG);
>  		tail =3D ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
>=20
> +		/*
> +		 * SID field is valid only when the ITE field is Set in FSTS_REG
> +		 * see Intel VT-d spec r4.1, section 11.4.9.9
> +		 */
> +		iqe_err =3D dmar_readq(iommu->reg + DMAR_IQER_REG);
> +		ite_sid =3D DMAR_IQER_REG_ITESID(iqe_err);
> +
>  		writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);
>  		pr_info("Invalidation Time-out Error (ITE) cleared\n");
>=20
> @@ -1325,6 +1333,16 @@ static int qi_check_fault(struct intel_iommu
> *iommu, int index, int wait_index,
>  			head =3D (head - 2 + QI_LENGTH) % QI_LENGTH;
>  		} while (head !=3D tail);
>=20
> +		/*
> +		 * If got ITE, we need to check if the sid of ITE is the same as
> +		 * current ATS invalidation target device, if yes, don't try this
> +		 * request anymore if the target device isn't present.
> +		 * 0 value of ite_sid means old VT-d device, no ite_sid value.
> +		 */
> +		if (pdev && ite_sid && !pci_device_is_present(pdev) &&
> +			ite_sid =3D=3D pci_dev_id(pci_physfn(pdev)))
> +			return -ETIMEDOUT;
> +

since the hardware already reports source id leading to timeout, can't we
just find the pci_dev according to reported ite_sid? this is a slow path (e=
ither
due to device in bad state or removed) hence it's not necessary to add more
intelligence to pass the pci_dev in, leading to only a partial fix can be b=
ackported.

It's also more future-proof, say if one day the driver allows batching inva=
lidation
requests for multiple devices then no need to pass in a list of devices.

Then it's easier to backport a full fix.