[v10,04/16] iommu: Cleanup iopf data structure definitions
Commit Message
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
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(¶m->fault_param->lock);
> if (list_empty(¶m->fault_param->faults)) {
> --
> 2.34.1
>
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
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
@@ -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;
};
@@ -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,
@@ -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(¶m->fault_param->lock);
if (list_empty(¶m->fault_param->faults)) {