Received-SPF: pass (google.com: domain of linux-kernel+bounces-55871-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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>, Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Nicolin Chen <nicolinc@nvidia.com>
CC: "Liu, Yi L" <yi.l.liu@intel.com>, Jacob Pan
	<jacob.jun.pan@linux.intel.com>, Longfang Liu <liulongfang@huawei.com>,
	"Zhao, Yan Y" <yan.y.zhao@intel.com>, Joel Granados <j.granados@samsung.com>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>, "kvm@vger.kernel.org"
	<kvm@vger.kernel.org>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>
Subject: RE: [PATCH v12 13/16] iommu: Improve iopf_queue_remove_device()
Thread-Topic: [PATCH v12 13/16] iommu: Improve iopf_queue_remove_device()
Thread-Index: AQHaWWafjR+FYFS4H0+ifBk3us7aubD+LEXA
Date: Wed, 7 Feb 2024 02:50:16 +0000
Message-ID: <BN9PR11MB527603AB5685FF3ED21647958C452@BN9PR11MB5276.namprd11.prod.outlook.com>
References: <20240207013325.95182-1-baolu.lu@linux.intel.com>
 <20240207013325.95182-14-baolu.lu@linux.intel.com>
In-Reply-To: <20240207013325.95182-14-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?OsK7klrUdRDbq/N8vjxsWrUIWzrYZDOMl/0Z1KO65aUU4J3VRupgpSlPVrsM?=
 =?us-ascii?Q?0dI8fv/Au6EAILFz+gDvgIuAIfw4xuBFCuBoLMmpuT1rJ1Ags+ICNFKmWU0l?=
 =?us-ascii?Q?wRwFnEdYdZyvBf4dd9t814kOT1U8p1jzknN7faYNE82aTpfyDD6QlZTywPq8?=
 =?us-ascii?Q?TJsYqxQPiknCYYO5Cd34fO7xCr++PBbro+uWFwDLzYW2jN2yBESW+lSnYGv1?=
 =?us-ascii?Q?pI4C5D1GO9LDNojrbNzx9+QxhK+5X7S8jJbtauyIZACNkIH6/v4SfYSKgA+X?=
 =?us-ascii?Q?USMRVGCZGJfyMILd71bL1dUd9ao0Ml/lxkVwF62+L02Bc2hH9lo/C+iNiPGU?=
 =?us-ascii?Q?ZPJlnKT1nh5NTX5AaJuhaPPjk7q3N5DknjtACqbXBP2nzBrrxILMhdnFfRCR?=
 =?us-ascii?Q?JCEi9E8ZMxJ72kZ0RtdYEjoVWWMzdzX2VH+EFVNAKCFlq86IGkgDWkRwsgk4?=
 =?us-ascii?Q?MrXmzodf4HSlUp8u4x0hn94kPEEyZI2Q6Kjkge8zCLLp8k4FU4ELTE79zfm5?=
 =?us-ascii?Q?yqdtSjXD3QQgH8Za2O3Mqoj6+qfpEdJFBH6PrFao5WZD4R1hDcchv8PjvxBm?=
 =?us-ascii?Q?lZpnqx8o/sQc6Ksezw7okfdzFHrgqKD/AClrtbFCiqRIrY6jRcB55QBmbsG1?=
 =?us-ascii?Q?VtbpyYZeROTMOL/KeoXf0MKXMBOTHGZAbWaaZrBWecrPUfihpcmQwt+fcIY2?=
 =?us-ascii?Q?s53qcMGiqHrIbIJPj4IJqT3t5oZQgeE2ApUIGTmIvlIFUM7sbkKSttoJvaZJ?=
 =?us-ascii?Q?4MBS9+CdOYg1VzRExRfX3TxS0Ca7gek853oq4HSXx/v+vNVwNgCjXsUhHtgw?=
 =?us-ascii?Q?cbaoGLSGyyu3EBPHWBle/EP5V7eDFMP1ZvQefqCccWQQeNSXdjBtEkr0+V37?=
 =?us-ascii?Q?IgriftgY1ogu7UVuDXsBIVu3m8vc22AVOt8xMexyxvr6xahtaE4/L6VoPht3?=
 =?us-ascii?Q?i938EfgnTNvXNgiBucaQdsSRuZvXuxyqR5w9sW5NdKRLL0yU0HosXjv3dnG4?=
 =?us-ascii?Q?xTYEgNs1j+QSHTl9dxrno9IunW41z89lyGQOeefAAVJt7XU8KZpc/F+BeM9H?=
 =?us-ascii?Q?jAKJhAaJwe0yFzTs8r1SqAbtglAg+fhI1XZoySZ0MQsNzfScU2HZsd52m0hY?=
 =?us-ascii?Q?ObcovRScvKVzh5qrlbCZWtRITDSQFB4GWxAo/g8eFlk9NyrPu5lNGZ+uISb8?=
 =?us-ascii?Q?g6LAmDqrmbVaO2Y2TCb//iecNw1iT/+huG9Pm9H8B2Fp4+34jrjtIdaNb52i?=
 =?us-ascii?Q?JWTQMgdXMqOHGsh1uXtmdndeidxB8Z1SwIo3WBeI9gOH3/6QcVQnIIfyH4aw?=
 =?us-ascii?Q?y9c7d5zIPWvlGhKE2feXzfCSHe4aPu5+VUcWLE6TWaTPWvHzBi8aY/JkUsiO?=
 =?us-ascii?Q?d5z/xqSiip2/EeLJynAZZzd5kjTUik2SFzKkJDOI/oVp5laiB680sz2HPr3j?=
 =?us-ascii?Q?89HLCn+WZbQkDKyyB7R1j2/n3X/R+tmLiEtoCDZN40I59wK2WAr6xqr4pE6U?=
 =?us-ascii?Q?Gm6G1oFKUFw180yD8zaFFwVxyA9u8DTAhtxPO2mjYxZ8WVW8aIPtCIRplRrR?=
 =?us-ascii?Q?iF7ojdy7xiWvO7x3TkxWWqG/4mwRBhpF4ZShg9VB?=
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: 59b28788-55a8-4bb3-c7f7-08dc278783ea
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Feb 2024 02:50:16.6495
 (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: u9njTgmWLgYBhv/dCS6/PLMA8I2Evqd4gPkOzobiZ2w1PqfOnA1/TkP9zetQAVbT52NApQpqs6gT7Xsg5RoU0w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7786
X-OriginatorOrg: intel.com

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Wednesday, February 7, 2024 9:33 AM
>=20
> Convert iopf_queue_remove_device() to return void instead of an error cod=
e,
> as the return value is never used. This removal helper is designed to be
> never-failed, so there's no need for error handling.
>=20
> Ack all outstanding page requests from the device with the response code =
of
> IOMMU_PAGE_RESP_INVALID, indicating device should not attempt any retry.
>=20
> Add comments to this helper explaining the steps involved in removing a
> device from the iopf queue and disabling its PRI. The individual drivers
> are expected to be adjusted accordingly. Here we just define the expected
> behaviors of the individual iommu driver from the core's perspective.
>=20
> Suggested-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Tested-by: Yan Zhao <yan.y.zhao@intel.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>, with one nit:

> + * Removing a device from an iopf_queue. It's recommended to follow
> these
> + * steps when removing a device:
>   *
> - * Return: 0 on success and <0 on error.
> + * - Disable new PRI reception: Turn off PRI generation in the IOMMU
> hardware
> + *   and flush any hardware page request queues. This should be done
> before
> + *   calling into this helper.
> + * - Acknowledge all outstanding PRQs to the device: Respond to all
> outstanding
> + *   page requests with IOMMU_PAGE_RESP_INVALID, indicating the device
> should
> + *   not retry. This helper function handles this.

this implies calling iopf_queue_remove_device() here.

> + * - Disable PRI on the device: After calling this helper, the caller co=
uld
> + *   then disable PRI on the device.
> + * - Call iopf_queue_remove_device(): Calling iopf_queue_remove_device()
> + *   essentially disassociates the device. The fault_param might still e=
xist,
> + *   but iommu_page_response() will do nothing. The device fault paramet=
er
> + *   reference count has been properly passed from
> iommu_report_device_fault()
> + *   to the fault handling work, and will eventually be released after
> + *   iommu_page_response().
>   */

but here it suggests calling iopf_queue_remove_device() again. If the comme=
nt
is just about to detail the behavior with that invocation shouldn't it be m=
erged
with the previous one instead of pretending to be the final step for driver
to call?