Received-SPF: pass (google.com: domain of linux-kernel+bounces-38379-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 25 Jan 2024 11:23:26 +0100
From: Joel Granados <j.granados@samsung.com>
To: Lu Baolu <baolu.lu@linux.intel.com>
CC: Joerg Roedel <joro@8bytes.org>, Will Deacon <will@kernel.org>, Robin
	Murphy <robin.murphy@arm.com>, Jason Gunthorpe <jgg@ziepe.ca>, Kevin Tian
	<kevin.tian@intel.com>, Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Nicolin Chen <nicolinc@nvidia.com>, Yi Liu <yi.l.liu@intel.com>, Jacob Pan
	<jacob.jun.pan@linux.intel.com>, Longfang Liu <liulongfang@huawei.com>, Yan
	Zhao <yan.y.zhao@intel.com>, <iommu@lists.linux.dev>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Jason Gunthorpe <jgg@nvidia.com>
Subject: Re: [PATCH v10 04/16] iommu: Cleanup iopf data structure
 definitions
Message-ID: <20240125102326.rgos2wizh273rteq@localhost>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
	protocol="application/pgp-signature"; boundary="g7md5qdsafvmcjsg"
Content-Disposition: inline
In-Reply-To: <20240122054308.23901-5-baolu.lu@linux.intel.com>
CMS-TYPE: 201P
References: <20240122054308.23901-1-baolu.lu@linux.intel.com>
	<20240122054308.23901-5-baolu.lu@linux.intel.com>
	<CGME20240125102328eucas1p288a2c65df13b1f60d60f363447bb8e5c@eucas1p2.samsung.com>

--g7md5qdsafvmcjsg
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 01:42:56PM +0800, Lu Baolu wrote:
> struct iommu_fault_page_request and struct iommu_page_response are not
> part of uAPI anymore. Convert them to data structures for kAPI.
>=20
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
> Reviewed-by: Yi Liu <yi.l.liu@intel.com>
> Tested-by: Yan Zhao <yan.y.zhao@intel.com>
> Tested-by: Longfang Liu <liulongfang@huawei.com>
> ---
>  include/linux/iommu.h      | 27 +++++++++++----------------
>  drivers/iommu/io-pgfault.c |  1 -
>  drivers/iommu/iommu.c      |  4 ----
>  3 files changed, 11 insertions(+), 21 deletions(-)
>=20
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index c960c4fae3bc..829bcb5a8e23 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -71,12 +71,12 @@ struct iommu_fault_page_request {
>  #define IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE	(1 << 1)
>  #define IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA	(1 << 2)
>  #define IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID	(1 << 3)
> -	__u32	flags;
> -	__u32	pasid;
> -	__u32	grpid;
> -	__u32	perm;
> -	__u64	addr;
> -	__u64	private_data[2];
> +	u32	flags;
> +	u32	pasid;
> +	u32	grpid;
> +	u32	perm;
> +	u64	addr;
> +	u64	private_data[2];
>  };
> =20
>  /**
> @@ -85,7 +85,7 @@ struct iommu_fault_page_request {
>   * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
>   */
>  struct iommu_fault {
> -	__u32	type;
> +	u32 type;
>  	struct iommu_fault_page_request prm;
>  };
> =20
> @@ -106,8 +106,6 @@ enum iommu_page_response_code {
> =20
>  /**
>   * struct iommu_page_response - Generic page response information
> - * @argsz: User filled size of this data
> - * @version: API version of this structure
>   * @flags: encodes whether the corresponding fields are valid
>   *         (IOMMU_FAULT_PAGE_RESPONSE_* values)
>   * @pasid: Process Address Space ID
> @@ -115,14 +113,11 @@ enum iommu_page_response_code {
>   * @code: response code from &enum iommu_page_response_code
>   */
>  struct iommu_page_response {
> -	__u32	argsz;
> -#define IOMMU_PAGE_RESP_VERSION_1	1
> -	__u32	version;
>  #define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
> -	__u32	flags;
> -	__u32	pasid;
> -	__u32	grpid;
> -	__u32	code;
> +	u32	flags;
> +	u32	pasid;
> +	u32	grpid;
> +	u32	code;
>  };
> =20
> =20
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index e5b8b9110c13..24b5545352ae 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -56,7 +56,6 @@ static int iopf_complete_group(struct device *dev, stru=
ct iopf_fault *iopf,
>  			       enum iommu_page_response_code status)
>  {
>  	struct iommu_page_response resp =3D {
> -		.version		=3D IOMMU_PAGE_RESP_VERSION_1,
>  		.pasid			=3D iopf->fault.prm.pasid,
>  		.grpid			=3D iopf->fault.prm.grpid,
>  		.code			=3D status,
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 68e648b55767..b88dc3e0595c 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1494,10 +1494,6 @@ int iommu_page_response(struct device *dev,
>  	if (!param || !param->fault_param)
>  		return -EINVAL;
> =20
> -	if (msg->version !=3D IOMMU_PAGE_RESP_VERSION_1 ||
> -	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
> -		return -EINVAL;
> -
I see that this function `iommu_page_response` eventually lands in
drivers/iommu/io-pgfault.c as `iopf_group_response`. But it seems that
the check for IOMMU_PAGE_RESP_PASID_VALID is dropped.

I see that after applying [1] and [2] there are only three places where
IOMMU_PAGE_RESP_PASID_VALID appears in the code: One is the definition
and the other two are just setting the value. We effectively dropped the
check. Is the drop intended? and if so, should we just get rid of
IOMMU_PAGE_RESP_PASID_VALID?

Best

[1] https://lore.kernel.org/all/20240122054308.23901-1-baolu.lu@linux.intel=
=2Ecom
[2] https://lore.kernel.org/all/20240122073903.24406-1-baolu.lu@linux.intel=
=2Ecom

>  	/* Only send response if there is a fault report pending */
>  	mutex_lock(&param->fault_param->lock);
>  	if (list_empty(&param->fault_param->faults)) {
> --=20
> 2.34.1
>=20

--=20

Joel Granados

--g7md5qdsafvmcjsg
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmWyNpsACgkQupfNUreW
QU+6hwv/TaKF2IQiTXor9rKsoUWbb18zSHY+9a6gb2JXmoq36LeghRmGq08Fqlhe
sfWpsfisZ1vQyMisJpmGYUjvADZAwz58CzPuJg5/mvYIQ92MzOdw9uuYtEhQ8NCn
2aBCirkZqnmgs9PT+LeOiZI27SqnJ20BYxHziHwKImO00xdF0ATVYJ0WPrC1bBrQ
2AyluVYBnJBf6eD1aBRyJhd2p+n+Vz5Khjwccg4gHD2wtlHhfaw+bxvHYG/LaTN2
GtPZlIxvszLugo10D0dJPvQT/UyWPok58qV0vs7j7n66xtQYRFC72aV2PgydIvy0
BlQd6ekUVDGKiJ3hCrelfmngenwkO7nEA3IMZizb5jFm7fJTshVyOKJ73cB9AKtr
BVfFbPV+D3a5q5hojISFuouMA0TCXjcbAVxD06LhPly6r6YqC6F2teQK2NzPmZwr
5FN7CR+oA8kYi/BNa7cjX0CWPorJZuJnNT+UEv38+EzuKfLZ3RjCpcN4iHDTorCf
ZknArIcN
=QScI
-----END PGP SIGNATURE-----

--g7md5qdsafvmcjsg--