[v10,04/16] iommu: Cleanup iopf data structure definitions

Message ID	20240122054308.23901-5-baolu.lu@linux.intel.com (mailing list archive)
State	New, archived
Headers	show Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id D831212B81; Mon, 22 Jan 2024 05:48:49 +0000 (UTC) From: Lu Baolu <baolu.lu@linux.intel.com> To: 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> Cc: 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, Lu Baolu <baolu.lu@linux.intel.com>, Jason Gunthorpe <jgg@nvidia.com> Subject: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions Date: Mon, 22 Jan 2024 13:42:56 +0800 Message-Id: <20240122054308.23901-5-baolu.lu@linux.intel.com> In-Reply-To: <20240122054308.23901-1-baolu.lu@linux.intel.com> References: <20240122054308.23901-1-baolu.lu@linux.intel.com> Precedence: bulk MIME-Version: 1.0 Content-Transfer-Encoding: 8bit
Series	iommu: Prepare to deliver page faults to user space \| expand [v10,00/16] iommu: Prepare to deliver page faults to user space [v10,01/16] iommu: Move iommu fault data to linux/iommu.h [v10,02/16] iommu/arm-smmu-v3: Remove unrecoverable faults reporting [v10,03/16] iommu: Remove unrecoverable fault data [v10,04/16] iommu: Cleanup iopf data structure definitions [v10,05/16] iommu: Merge iopf_device_param into iommu_fault_param [v10,06/16] iommu: Remove iommu_[un]register_device_fault_handler() [v10,07/16] iommu: Merge iommu_fault_event and iopf_fault [v10,08/16] iommu: Prepare for separating SVA and IOPF [v10,09/16] iommu: Make iommu_queue_iopf() more generic [v10,10/16] iommu: Separate SVA and IOPF [v10,11/16] iommu: Refine locking for per-device fault data management [v10,12/16] iommu: Use refcount for fault data access [v10,13/16] iommu: Improve iopf_queue_remove_device() [v10,14/16] iommu: Track iopf group instead of last fault [v10,15/16] iommu: Make iopf_group_response() return void [v10,16/16] iommu: Make iommu_report_device_fault() reutrn void

Message ID

20240122054308.23901-5-baolu.lu@linux.intel.com (mailing list archive)

State

New, archived

Headers

From: Lu Baolu <baolu.lu@linux.intel.com>
To: 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>
Cc: 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,
	Lu Baolu <baolu.lu@linux.intel.com>,
	Jason Gunthorpe <jgg@nvidia.com>
Subject: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions
Date: Mon, 22 Jan 2024 13:42:56 +0800
Message-Id: <20240122054308.23901-5-baolu.lu@linux.intel.com>
In-Reply-To: <20240122054308.23901-1-baolu.lu@linux.intel.com>
References: <20240122054308.23901-1-baolu.lu@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

Series

iommu: Prepare to deliver page faults to user space | expand

Commit Message

Baolu Lu Jan. 22, 2024, 5:42 a.m. UTC

struct iommu_fault_page_request and struct iommu_page_response are not
part of uAPI anymore. Convert them to data structures for kAPI.

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(-)

Comments

Joel Granados Jan. 25, 2024, 10:23 a.m. UTC | #1

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.
> 
> 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(-)
> 
> 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];
>  };
>  
>  /**
> @@ -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;
>  };
>  
> @@ -106,8 +106,6 @@ enum iommu_page_response_code {
>  
>  /**
>   * 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;
>  };
>  
>  
> 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, struct iopf_fault *iopf,
>  			       enum iommu_page_response_code status)
>  {
>  	struct iommu_page_response resp = {
> -		.version		= IOMMU_PAGE_RESP_VERSION_1,
>  		.pasid			= iopf->fault.prm.pasid,
>  		.grpid			= iopf->fault.prm.grpid,
>  		.code			= 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;
>  
> -	if (msg->version != 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.com
[2] https://lore.kernel.org/all/20240122073903.24406-1-baolu.lu@linux.intel.com

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

Baolu Lu Jan. 25, 2024, 11:33 a.m. UTC | #2

On 2024/1/25 18:23, Joel Granados wrote:
>> 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, struct iopf_fault *iopf,
>>   			       enum iommu_page_response_code status)
>>   {
>>   	struct iommu_page_response resp = {
>> -		.version		= IOMMU_PAGE_RESP_VERSION_1,
>>   		.pasid			= iopf->fault.prm.pasid,
>>   		.grpid			= iopf->fault.prm.grpid,
>>   		.code			= 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;
>>   
>> -	if (msg->version != 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

Yes, really. Thanks for pointing this out.

$ git grep IOMMU_PAGE_RESP_PASID_VALID
drivers/iommu/io-pgfault.c:             resp.flags = 
IOMMU_PAGE_RESP_PASID_VALID;
drivers/iommu/io-pgfault.c:                     resp.flags = 
IOMMU_PAGE_RESP_PASID_VALID;
include/linux/iommu.h:#define IOMMU_PAGE_RESP_PASID_VALID       (1 << 0)

> check. Is the drop intended? and if so, should we just get rid of
> IOMMU_PAGE_RESP_PASID_VALID?

In my opinion, we should keep this hardware detail in the individual
driver. When the page fault handling framework in IOMMU and IOMMUFD
subsystems includes a valid PASID in the fault message, the response
message should also contain the *same* PASID value. Individual drivers
should be responsible for deciding whether to include the PASID in the
messages they provide for the hardware.

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

Best regards,
baolu

Jason Gunthorpe Jan. 25, 2024, 1:44 p.m. UTC | #3

On Thu, Jan 25, 2024 at 07:33:45PM +0800, Baolu Lu wrote:
> > check. Is the drop intended? and if so, should we just get rid of
> > IOMMU_PAGE_RESP_PASID_VALID?
> 
> In my opinion, we should keep this hardware detail in the individual
> driver. When the page fault handling framework in IOMMU and IOMMUFD
> subsystems includes a valid PASID in the fault message, the response
> message should also contain the *same* PASID value. Individual drivers
> should be responsible for deciding whether to include the PASID in the
> messages they provide for the hardware.

+1

Jason

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];
 };
 
 /**
@@ -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;
 };
 
@@ -106,8 +106,6 @@  enum iommu_page_response_code {
 
 /**
  * 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;
 };
 
 
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, struct iopf_fault *iopf,
 			       enum iommu_page_response_code status)
 {
 	struct iommu_page_response resp = {
-		.version		= IOMMU_PAGE_RESP_VERSION_1,
 		.pasid			= iopf->fault.prm.pasid,
 		.grpid			= iopf->fault.prm.grpid,
 		.code			= 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;
 
-	if (msg->version != IOMMU_PAGE_RESP_VERSION_1 ||
-	    msg->flags & ~IOMMU_PAGE_RESP_PASID_VALID)
-		return -EINVAL;
-
 	/* Only send response if there is a fault report pending */
 	mutex_lock(&param->fault_param->lock);
 	if (list_empty(&param->fault_param->faults)) {

[v10,04/16] iommu: Cleanup iopf data structure definitions

Commit Message

Comments

Patch