2024-01-22 06:51:57

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 00/16] iommu: Prepare to deliver page faults to user space

When a user-managed page table is attached to an IOMMU, it is necessary
to deliver IO page faults to user space so that they can be handled
appropriately. One use case for this is nested translation, which is
currently being discussed in the mailing list.

I have posted a RFC series [1] that describes the implementation of
delivering page faults to user space through IOMMUFD. This series has
received several comments on the IOMMU refactoring, which I am trying to
address in this series.

The major refactoring includes:

- [PATCH 01 ~ 04] Move include/uapi/linux/iommu.h to
include/linux/iommu.h. Remove the unrecoverable fault data definition.
- [PATCH 05 ~ 06] Remove iommu_[un]register_device_fault_handler().
- [PATCH 07 ~ 10] Separate SVA and IOPF. Make IOPF a generic page fault
handling framework.
- [PATCH 11 ~ 16] Improve iopf framework.

This is also available at github [2].

[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://github.com/LuBaolu/intel-iommu/commits/preparatory-io-pgfault-delivery-v10

Change log:
v10:
- Make iopf_group_response() return void, as nobody can do anything
with the failure.
- Make iommu_report_device_fault() automatically respond to
unhandleable faults and change its return type to void.
- PATCH 01 ~ 14 are in good shapes now.

v9: https://lore.kernel.org/linux-iommu/[email protected]/
- Protecting the assignment of dev->iommu->fault_param with RCU.
- Extending the fault parameter's lifetime to the entire path of iopf
handling.
- Since iopf_queue_flush_dev() can only be called before
iopf_queue_remove_device(), there's no need to hold a reference
count.
- Improve iopf_queue_remove_device() as per Jason's comments on the
device removal sequence from the iopf queue. This will likely
require changes to the iommu drivers, which are supposed to be
addressed in separate series.
- Track the iopf_group as a whole instead of the last fault within the
group to simplify the fault report and response paths.
- PATCH 01 ~ 11 are in good shapes now.

v8: https://lore.kernel.org/linux-iommu/[email protected]/
- Drop PATCH 12/12 as it is no longer necessary to drain page requests
page requests during PASID translation changes.
- Separate PATCH 11/12 into two distinct patches. The first patch
refines locking scheme for protecting per-device fault data, while
the second patch replaces mutex with RCU to enhance locking
efficiency.
- PATCH 01 ~ 10 are in good shapes now.

v7: https://lore.kernel.org/linux-iommu/[email protected]/
- Rebase to v6.7-rc1.
- Export iopf_group_response() for global use.
- Release lock when calling iopf handler.
- The whole series has been verified to work for SVA case on Intel
platforms by Zhao Yan. Add her Tested-by to affected patches.

v6: https://lore.kernel.org/linux-iommu/[email protected]/
- [PATCH 09/12] Check IS_ERR() against the iommu domain. [Jingqi/Jason]
- [PATCH 12/12] Rename the comments and name of iopf_queue_flush_dev(),
no functionality changes. [Kevin]
- All patches rebased on the latest iommu/core branch.

v5: https://lore.kernel.org/linux-iommu/[email protected]/
- Consolidate per-device fault data management. (New patch 11)
- Improve iopf_queue_flush_dev(). (New patch 12)

v4: https://lore.kernel.org/linux-iommu/[email protected]/
- Merge iommu_fault_event and iopf_fault. They are duplicate.
- Move iommu_report_device_fault() and iommu_page_response() to
io-pgfault.c.
- Move iommu_sva_domain_alloc() to iommu-sva.c.
- Add group->domain and use it directly in sva fault handler.
- Misc code refactoring and refining.

v3: https://lore.kernel.org/linux-iommu/[email protected]/
- Convert the fault data structures from uAPI to kAPI.
- Merge iopf_device_param into iommu_fault_param.
- Add debugging on domain lifetime for iopf.
- Remove patch "iommu: Change the return value of dev_iommu_get()".
- Remove patch "iommu: Add helper to set iopf handler for domain".
- Misc code refactoring and refining.

v2: https://lore.kernel.org/linux-iommu/[email protected]/
- Remove unrecoverable fault data definition as suggested by Kevin.
- Drop the per-device fault cookie code considering that doesn't make
much sense for SVA.
- Make the IOMMU page fault handling framework generic. So that it can
available for use cases other than SVA.

v1: https://lore.kernel.org/linux-iommu/[email protected]/

Lu Baolu (16):
iommu: Move iommu fault data to linux/iommu.h
iommu/arm-smmu-v3: Remove unrecoverable faults reporting
iommu: Remove unrecoverable fault data
iommu: Cleanup iopf data structure definitions
iommu: Merge iopf_device_param into iommu_fault_param
iommu: Remove iommu_[un]register_device_fault_handler()
iommu: Merge iommu_fault_event and iopf_fault
iommu: Prepare for separating SVA and IOPF
iommu: Make iommu_queue_iopf() more generic
iommu: Separate SVA and IOPF
iommu: Refine locking for per-device fault data management
iommu: Use refcount for fault data access
iommu: Improve iopf_queue_remove_device()
iommu: Track iopf group instead of last fault
iommu: Make iopf_group_response() return void
iommu: Make iommu_report_device_fault() reutrn void

include/linux/iommu.h | 266 +++++++---
drivers/iommu/intel/iommu.h | 4 +-
drivers/iommu/iommu-sva.h | 71 ---
include/uapi/linux/iommu.h | 161 ------
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 14 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 103 ++--
drivers/iommu/intel/iommu.c | 28 +-
drivers/iommu/intel/svm.c | 41 +-
drivers/iommu/io-pgfault.c | 480 +++++++++++-------
drivers/iommu/iommu-sva.c | 71 ++-
drivers/iommu/iommu.c | 233 ---------
MAINTAINERS | 1 -
drivers/iommu/Kconfig | 4 +
drivers/iommu/Makefile | 3 +-
drivers/iommu/intel/Kconfig | 1 +
15 files changed, 596 insertions(+), 885 deletions(-)
delete mode 100644 drivers/iommu/iommu-sva.h
delete mode 100644 include/uapi/linux/iommu.h

--
2.34.1



2024-01-22 07:02:15

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 15/16] iommu: Make iopf_group_response() return void

The iopf_group_response() should return void, as nothing can do anything
with the failure. This implies that ops->page_response() must also return
void; this is consistent with what the drivers do. The failure paths,
which are all integrity validations of the fault, should be WARN_ON'd,
not return codes.

If the iommu core fails to enqueue the fault, it should respond the fault
directly by calling ops->page_response() instead of returning an error
number and relying on the iommu drivers to do so. Consolidate the error
fault handling code in the core.

Co-developed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 14 +--
drivers/iommu/intel/iommu.h | 4 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +++-----
drivers/iommu/intel/svm.c | 18 +--
drivers/iommu/io-pgfault.c | 132 +++++++++++---------
5 files changed, 99 insertions(+), 119 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 48196efc9327..d7b6f4017254 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -578,9 +578,8 @@ struct iommu_ops {
int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);

- int (*page_response)(struct device *dev,
- struct iopf_fault *evt,
- struct iommu_page_response *msg);
+ void (*page_response)(struct device *dev, struct iopf_fault *evt,
+ struct iommu_page_response *msg);

int (*def_domain_type)(struct device *dev);
void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
@@ -1551,8 +1550,8 @@ void iopf_queue_free(struct iopf_queue *queue);
int iopf_queue_discard_partial(struct iopf_queue *queue);
void iopf_free_group(struct iopf_group *group);
int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
-int iopf_group_response(struct iopf_group *group,
- enum iommu_page_response_code status);
+void iopf_group_response(struct iopf_group *group,
+ enum iommu_page_response_code status);
#else
static inline int
iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
@@ -1594,10 +1593,9 @@ iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
return -ENODEV;
}

-static inline int iopf_group_response(struct iopf_group *group,
- enum iommu_page_response_code status)
+static inline void iopf_group_response(struct iopf_group *group,
+ enum iommu_page_response_code status)
{
- return -ENODEV;
}
#endif /* CONFIG_IOMMU_IOPF */
#endif /* __LINUX_IOMMU_H */
diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index 696d95293a69..cf9a28c7fab8 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -1079,8 +1079,8 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
void intel_svm_check(struct intel_iommu *iommu);
int intel_svm_enable_prq(struct intel_iommu *iommu);
int intel_svm_finish_prq(struct intel_iommu *iommu);
-int intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
- struct iommu_page_response *msg);
+void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
+ struct iommu_page_response *msg);
struct iommu_domain *intel_svm_domain_alloc(void);
void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
void intel_drain_pasid_prq(struct device *dev, u32 pasid);
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 4e93e845458c..42eb59cb99f4 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -920,31 +920,29 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
return arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmds, cmds->num, true);
}

-static int arm_smmu_page_response(struct device *dev,
- struct iopf_fault *unused,
- struct iommu_page_response *resp)
+static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
+ struct iommu_page_response *resp)
{
struct arm_smmu_cmdq_ent cmd = {0};
struct arm_smmu_master *master = dev_iommu_priv_get(dev);
int sid = master->streams[0].id;

- if (master->stall_enabled) {
- cmd.opcode = CMDQ_OP_RESUME;
- cmd.resume.sid = sid;
- cmd.resume.stag = resp->grpid;
- switch (resp->code) {
- case IOMMU_PAGE_RESP_INVALID:
- case IOMMU_PAGE_RESP_FAILURE:
- cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
- break;
- case IOMMU_PAGE_RESP_SUCCESS:
- cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
- break;
- default:
- return -EINVAL;
- }
- } else {
- return -ENODEV;
+ if (WARN_ON(!master->stall_enabled))
+ return;
+
+ cmd.opcode = CMDQ_OP_RESUME;
+ cmd.resume.sid = sid;
+ cmd.resume.stag = resp->grpid;
+ switch (resp->code) {
+ case IOMMU_PAGE_RESP_INVALID:
+ case IOMMU_PAGE_RESP_FAILURE:
+ cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
+ break;
+ case IOMMU_PAGE_RESP_SUCCESS:
+ cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
+ break;
+ default:
+ break;
}

arm_smmu_cmdq_issue_cmd(master->smmu, &cmd);
@@ -954,8 +952,6 @@ static int arm_smmu_page_response(struct device *dev,
* terminated... at some point in the future. PRI_RESP is fire and
* forget.
*/
-
- return 0;
}

/* Context descriptor manipulation functions */
@@ -1516,16 +1512,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
}

ret = iommu_report_device_fault(master->dev, &fault_evt);
- if (ret && flt->type == IOMMU_FAULT_PAGE_REQ) {
- /* Nobody cared, abort the access */
- struct iommu_page_response resp = {
- .pasid = flt->prm.pasid,
- .grpid = flt->prm.grpid,
- .code = IOMMU_PAGE_RESP_FAILURE,
- };
- arm_smmu_page_response(master->dev, &fault_evt, &resp);
- }
-
out_unlock:
mutex_unlock(&smmu->streams_mutex);
return ret;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index e1cbcb9515f0..2f8716636dbb 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -740,9 +740,8 @@ static irqreturn_t prq_event_thread(int irq, void *d)
return IRQ_RETVAL(handled);
}

-int intel_svm_page_response(struct device *dev,
- struct iopf_fault *evt,
- struct iommu_page_response *msg)
+void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
+ struct iommu_page_response *msg)
{
struct device_domain_info *info = dev_iommu_priv_get(dev);
struct intel_iommu *iommu = info->iommu;
@@ -751,7 +750,6 @@ int intel_svm_page_response(struct device *dev,
bool private_present;
bool pasid_present;
bool last_page;
- int ret = 0;
u16 sid;

prm = &evt->fault.prm;
@@ -760,16 +758,6 @@ int intel_svm_page_response(struct device *dev,
private_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA;
last_page = prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;

- if (!pasid_present) {
- ret = -EINVAL;
- goto out;
- }
-
- if (prm->pasid == 0 || prm->pasid >= PASID_MAX) {
- ret = -EINVAL;
- goto out;
- }
-
/*
* Per VT-d spec. v3.0 ch7.7, system software must respond
* with page group response if private data is present (PDP)
@@ -798,8 +786,6 @@ int intel_svm_page_response(struct device *dev,

qi_submit_sync(iommu, &desc, 1, 0);
}
-out:
- return ret;
}

static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index c22e13df84c2..6e63e5a02884 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -39,7 +39,7 @@ static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param)
kfree_rcu(fault_param, rcu);
}

-void iopf_free_group(struct iopf_group *group)
+static void __iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;

@@ -50,6 +50,11 @@ void iopf_free_group(struct iopf_group *group)

/* Pair with iommu_report_device_fault(). */
iopf_put_dev_fault_param(group->fault_param);
+}
+
+void iopf_free_group(struct iopf_group *group)
+{
+ __iopf_free_group(group);
kfree(group);
}
EXPORT_SYMBOL_GPL(iopf_free_group);
@@ -97,14 +102,49 @@ static int report_partial_fault(struct iommu_fault_param *fault_param,
return 0;
}

+static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
+ struct iopf_fault *evt,
+ struct iopf_group *abort_group)
+{
+ struct iopf_fault *iopf, *next;
+ struct iopf_group *group;
+
+ group = kzalloc(sizeof(*group), GFP_KERNEL);
+ if (!group) {
+ /*
+ * We always need to construct the group as we need it to abort
+ * the request at the driver if it cfan't be handled.
+ */
+ group = abort_group;
+ }
+
+ group->fault_param = iopf_param;
+ group->last_fault.fault = evt->fault;
+ INIT_LIST_HEAD(&group->faults);
+ INIT_LIST_HEAD(&group->pending_node);
+ list_add(&group->last_fault.list, &group->faults);
+
+ /* See if we have partial faults for this group */
+ mutex_lock(&iopf_param->lock);
+ list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
+ if (iopf->fault.prm.grpid == evt->fault.prm.grpid)
+ /* Insert *before* the last fault */
+ list_move(&iopf->list, &group->faults);
+ }
+ list_add(&group->pending_node, &iopf_param->faults);
+ mutex_unlock(&iopf_param->lock);
+
+ return group;
+}
+
/**
* iommu_report_device_fault() - Report fault event to device driver
* @dev: the device
* @evt: fault event data
*
* Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
- * handler. When this function fails and the fault is recoverable, it is the
- * caller's responsibility to complete the fault.
+ * handler. If this function fails then ops->page_response() was called to
+ * complete evt if required.
*
* This module doesn't handle PCI PASID Stop Marker; IOMMU drivers must discard
* them before reporting faults. A PASID Stop Marker (LRW = 0b100) doesn't
@@ -143,22 +183,18 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
{
struct iommu_fault *fault = &evt->fault;
struct iommu_fault_param *iopf_param;
- struct iopf_fault *iopf, *next;
- struct iommu_domain *domain;
+ struct iopf_group abort_group = {};
struct iopf_group *group;
int ret;

- if (fault->type != IOMMU_FAULT_PAGE_REQ)
- return -EOPNOTSUPP;
-
iopf_param = iopf_get_dev_fault_param(dev);
- if (!iopf_param)
+ if (WARN_ON(!iopf_param))
return -ENODEV;

if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
ret = report_partial_fault(iopf_param, fault);
iopf_put_dev_fault_param(iopf_param);
-
+ /* A request that is not the last does not need to be ack'd */
return ret;
}

@@ -170,56 +206,33 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
* will send a response to the hardware. We need to clean up before
* leaving, otherwise partial faults will be stuck.
*/
- domain = get_domain_for_iopf(dev, fault);
- if (!domain) {
+ group = iopf_group_alloc(iopf_param, evt, &abort_group);
+ if (group == &abort_group) {
+ ret = -ENOMEM;
+ goto err_abort;
+ }
+
+ group->domain = get_domain_for_iopf(dev, fault);
+ if (!group->domain) {
ret = -EINVAL;
- goto cleanup_partial;
+ goto err_abort;
}

- group = kzalloc(sizeof(*group), GFP_KERNEL);
- if (!group) {
- ret = -ENOMEM;
- goto cleanup_partial;
- }
-
- group->fault_param = iopf_param;
- group->last_fault.fault = *fault;
- INIT_LIST_HEAD(&group->faults);
- INIT_LIST_HEAD(&group->pending_node);
- group->domain = domain;
- list_add(&group->last_fault.list, &group->faults);
-
- /* See if we have partial faults for this group */
- mutex_lock(&iopf_param->lock);
- list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
- if (iopf->fault.prm.grpid == fault->prm.grpid)
- /* Insert *before* the last fault */
- list_move(&iopf->list, &group->faults);
- }
- list_add(&group->pending_node, &iopf_param->faults);
- mutex_unlock(&iopf_param->lock);
+ /*
+ * On success iopf_handler must call iopf_group_response() and
+ * iopf_free_group()
+ */
+ ret = group->domain->iopf_handler(group);
+ if (ret)
+ goto err_abort;
+ return 0;

- ret = domain->iopf_handler(group);
- if (ret) {
- mutex_lock(&iopf_param->lock);
- list_del_init(&group->pending_node);
- mutex_unlock(&iopf_param->lock);
+err_abort:
+ iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
+ if (group == &abort_group)
+ __iopf_free_group(group);
+ else
iopf_free_group(group);
- }
-
- return ret;
-
-cleanup_partial:
- mutex_lock(&iopf_param->lock);
- list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
- if (iopf->fault.prm.grpid == fault->prm.grpid) {
- list_del(&iopf->list);
- kfree(iopf);
- }
- }
- mutex_unlock(&iopf_param->lock);
- iopf_put_dev_fault_param(iopf_param);
-
return ret;
}
EXPORT_SYMBOL_GPL(iommu_report_device_fault);
@@ -262,8 +275,8 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
*
* Return 0 on success and <0 on error.
*/
-int iopf_group_response(struct iopf_group *group,
- enum iommu_page_response_code status)
+void iopf_group_response(struct iopf_group *group,
+ enum iommu_page_response_code status)
{
struct iommu_fault_param *fault_param = group->fault_param;
struct iopf_fault *iopf = &group->last_fault;
@@ -274,7 +287,6 @@ int iopf_group_response(struct iopf_group *group,
.grpid = iopf->fault.prm.grpid,
.code = status,
};
- int ret = -EINVAL;

if ((iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) &&
(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID))
@@ -283,12 +295,10 @@ int iopf_group_response(struct iopf_group *group,
/* Only send response if there is a fault report pending */
mutex_lock(&fault_param->lock);
if (!list_empty(&group->pending_node)) {
- ret = ops->page_response(dev, &group->last_fault, &resp);
+ ops->page_response(dev, &group->last_fault, &resp);
list_del_init(&group->pending_node);
}
mutex_unlock(&fault_param->lock);
-
- return ret;
}
EXPORT_SYMBOL_GPL(iopf_group_response);

--
2.34.1


2024-01-22 08:05:53

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 08/16] iommu: Prepare for separating SVA and IOPF

Move iopf_group data structure to iommu.h to make it a minimal set of
faults that a domain's page fault handler should handle.

Add a new function, iopf_free_group(), to free a fault group after all
faults in the group are handled. This function will be made global so
that it can be called from other files, such as iommu-sva.c.

Move iopf_queue data structure to iommu.h to allow the workqueue to be
scheduled out of this file.

This will simplify the sequential patches.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Yi Liu <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 20 +++++++++++++++++++-
drivers/iommu/io-pgfault.c | 37 +++++++++++++------------------------
2 files changed, 32 insertions(+), 25 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 2320548a90f8..c9d4f175f121 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -41,7 +41,6 @@ struct iommu_dirty_ops;
struct notifier_block;
struct iommu_sva;
struct iommu_dma_cookie;
-struct iopf_queue;

#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -126,6 +125,25 @@ struct iopf_fault {
struct list_head list;
};

+struct iopf_group {
+ struct iopf_fault last_fault;
+ struct list_head faults;
+ struct work_struct work;
+ struct device *dev;
+};
+
+/**
+ * struct iopf_queue - IO Page Fault queue
+ * @wq: the fault workqueue
+ * @devices: devices attached to this queue
+ * @lock: protects the device list
+ */
+struct iopf_queue {
+ struct workqueue_struct *wq;
+ struct list_head devices;
+ struct mutex lock;
+};
+
/* iommu fault flags */
#define IOMMU_FAULT_READ 0x0
#define IOMMU_FAULT_WRITE 0x1
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 10d48eb72608..c7e6bbed5c05 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -13,24 +13,17 @@

#include "iommu-sva.h"

-/**
- * struct iopf_queue - IO Page Fault queue
- * @wq: the fault workqueue
- * @devices: devices attached to this queue
- * @lock: protects the device list
- */
-struct iopf_queue {
- struct workqueue_struct *wq;
- struct list_head devices;
- struct mutex lock;
-};
+static void iopf_free_group(struct iopf_group *group)
+{
+ struct iopf_fault *iopf, *next;

-struct iopf_group {
- struct iopf_fault last_fault;
- struct list_head faults;
- struct work_struct work;
- struct device *dev;
-};
+ list_for_each_entry_safe(iopf, next, &group->faults, list) {
+ if (!(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
+ kfree(iopf);
+ }
+
+ kfree(group);
+}

static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
enum iommu_page_response_code status)
@@ -50,9 +43,9 @@ static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,

static void iopf_handler(struct work_struct *work)
{
+ struct iopf_fault *iopf;
struct iopf_group *group;
struct iommu_domain *domain;
- struct iopf_fault *iopf, *next;
enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;

group = container_of(work, struct iopf_group, work);
@@ -61,7 +54,7 @@ static void iopf_handler(struct work_struct *work)
if (!domain || !domain->iopf_handler)
status = IOMMU_PAGE_RESP_INVALID;

- list_for_each_entry_safe(iopf, next, &group->faults, list) {
+ list_for_each_entry(iopf, &group->faults, list) {
/*
* For the moment, errors are sticky: don't handle subsequent
* faults in the group if there is an error.
@@ -69,14 +62,10 @@ static void iopf_handler(struct work_struct *work)
if (status == IOMMU_PAGE_RESP_SUCCESS)
status = domain->iopf_handler(&iopf->fault,
domain->fault_data);
-
- if (!(iopf->fault.prm.flags &
- IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE))
- kfree(iopf);
}

iopf_complete_group(group->dev, &group->last_fault, status);
- kfree(group);
+ iopf_free_group(group);
}

/**
--
2.34.1


2024-01-22 08:16:22

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 02/16] iommu/arm-smmu-v3: Remove unrecoverable faults reporting

No device driver registers fault handler to handle the reported
unrecoveraable faults. Remove it to avoid dead code.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 46 ++++++---------------
1 file changed, 13 insertions(+), 33 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 0ffb1cf17e0b..4cf1054ed321 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1461,7 +1461,6 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
{
int ret;
- u32 reason;
u32 perm = 0;
struct arm_smmu_master *master;
bool ssid_valid = evt[0] & EVTQ_0_SSV;
@@ -1471,16 +1470,9 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)

switch (FIELD_GET(EVTQ_0_ID, evt[0])) {
case EVT_ID_TRANSLATION_FAULT:
- reason = IOMMU_FAULT_REASON_PTE_FETCH;
- break;
case EVT_ID_ADDR_SIZE_FAULT:
- reason = IOMMU_FAULT_REASON_OOR_ADDRESS;
- break;
case EVT_ID_ACCESS_FAULT:
- reason = IOMMU_FAULT_REASON_ACCESS;
- break;
case EVT_ID_PERMISSION_FAULT:
- reason = IOMMU_FAULT_REASON_PERMISSION;
break;
default:
return -EOPNOTSUPP;
@@ -1490,6 +1482,9 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
if (evt[1] & EVTQ_1_S2)
return -EFAULT;

+ if (!(evt[1] & EVTQ_1_STALL))
+ return -EOPNOTSUPP;
+
if (evt[1] & EVTQ_1_RnW)
perm |= IOMMU_FAULT_PERM_READ;
else
@@ -1501,32 +1496,17 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
if (evt[1] & EVTQ_1_PnU)
perm |= IOMMU_FAULT_PERM_PRIV;

- if (evt[1] & EVTQ_1_STALL) {
- flt->type = IOMMU_FAULT_PAGE_REQ;
- flt->prm = (struct iommu_fault_page_request) {
- .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
- .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
- .perm = perm,
- .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
- };
+ flt->type = IOMMU_FAULT_PAGE_REQ;
+ flt->prm = (struct iommu_fault_page_request) {
+ .flags = IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE,
+ .grpid = FIELD_GET(EVTQ_1_STAG, evt[1]),
+ .perm = perm,
+ .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
+ };

- if (ssid_valid) {
- flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
- flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
- }
- } else {
- flt->type = IOMMU_FAULT_DMA_UNRECOV;
- flt->event = (struct iommu_fault_unrecoverable) {
- .reason = reason,
- .flags = IOMMU_FAULT_UNRECOV_ADDR_VALID,
- .perm = perm,
- .addr = FIELD_GET(EVTQ_2_ADDR, evt[2]),
- };
-
- if (ssid_valid) {
- flt->event.flags |= IOMMU_FAULT_UNRECOV_PASID_VALID;
- flt->event.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
- }
+ if (ssid_valid) {
+ flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+ flt->prm.pasid = FIELD_GET(EVTQ_0_SSID, evt[0]);
}

mutex_lock(&smmu->streams_mutex);
--
2.34.1


2024-01-22 08:17:51

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 06/16] iommu: Remove iommu_[un]register_device_fault_handler()

The individual iommu driver reports the iommu page faults by calling
iommu_report_device_fault(), where a pre-registered device fault handler
is called to route the fault to another fault handler installed on the
corresponding iommu domain.

The pre-registered device fault handler is static and won't be dynamic
as the fault handler is eventually per iommu domain. Replace calling
device fault handler with iommu_queue_iopf().

After this replacement, the registering and unregistering fault handler
interfaces are not needed anywhere. Remove the interfaces and the related
data structures to avoid dead code.

Convert cookie parameter of iommu_queue_iopf() into a device pointer that
is really passed.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 23 ------
drivers/iommu/iommu-sva.h | 4 +-
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 13 +---
drivers/iommu/intel/iommu.c | 24 ++----
drivers/iommu/io-pgfault.c | 6 +-
drivers/iommu/iommu.c | 76 +------------------
6 files changed, 13 insertions(+), 133 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index bbb7c2ad5184..70176c1c5573 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -128,7 +128,6 @@ struct iommu_page_response {

typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
-typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault *, void *);

struct iommu_domain_geometry {
dma_addr_t aperture_start; /* First address that can be mapped */
@@ -671,8 +670,6 @@ struct iommu_fault_event {

/**
* struct iommu_fault_param - per-device IOMMU fault data
- * @handler: Callback function to handle IOMMU faults at device level
- * @data: handler private data
* @lock: protect pending faults list
* @dev: the device that owns this param
* @queue: IOPF queue
@@ -682,8 +679,6 @@ struct iommu_fault_event {
* @faults: holds the pending faults which need response
*/
struct iommu_fault_param {
- iommu_dev_fault_handler_t handler;
- void *data;
struct mutex lock;

struct device *dev;
@@ -806,11 +801,6 @@ extern int iommu_group_for_each_dev(struct iommu_group *group, void *data,
extern struct iommu_group *iommu_group_get(struct device *dev);
extern struct iommu_group *iommu_group_ref_get(struct iommu_group *group);
extern void iommu_group_put(struct iommu_group *group);
-extern int iommu_register_device_fault_handler(struct device *dev,
- iommu_dev_fault_handler_t handler,
- void *data);
-
-extern int iommu_unregister_device_fault_handler(struct device *dev);

extern int iommu_report_device_fault(struct device *dev,
struct iommu_fault_event *evt);
@@ -1222,19 +1212,6 @@ static inline void iommu_group_put(struct iommu_group *group)
{
}

-static inline
-int iommu_register_device_fault_handler(struct device *dev,
- iommu_dev_fault_handler_t handler,
- void *data)
-{
- return -ENODEV;
-}
-
-static inline int iommu_unregister_device_fault_handler(struct device *dev)
-{
- return 0;
-}
-
static inline
int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
{
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index 54946b5a7caf..de7819c796ce 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -13,7 +13,7 @@ struct iommu_fault;
struct iopf_queue;

#ifdef CONFIG_IOMMU_SVA
-int iommu_queue_iopf(struct iommu_fault *fault, void *cookie);
+int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev);

int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev);
int iopf_queue_remove_device(struct iopf_queue *queue,
@@ -26,7 +26,7 @@ enum iommu_page_response_code
iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);

#else /* CONFIG_IOMMU_SVA */
-static inline int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
+static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
{
return -ENODEV;
}
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 05722121f00e..ab2b0a5e4369 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -487,7 +487,6 @@ bool arm_smmu_master_sva_enabled(struct arm_smmu_master *master)

static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
{
- int ret;
struct device *dev = master->dev;

/*
@@ -500,16 +499,7 @@ static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
if (!master->iopf_enabled)
return -EINVAL;

- ret = iopf_queue_add_device(master->smmu->evtq.iopf, dev);
- if (ret)
- return ret;
-
- ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
- if (ret) {
- iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
- return ret;
- }
- return 0;
+ return iopf_queue_add_device(master->smmu->evtq.iopf, dev);
}

static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
@@ -519,7 +509,6 @@ static void arm_smmu_master_sva_disable_iopf(struct arm_smmu_master *master)
if (!master->iopf_enabled)
return;

- iommu_unregister_device_fault_handler(dev);
iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
}

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 6fb5f6fceea1..df6ceefc09ee 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4427,23 +4427,15 @@ static int intel_iommu_enable_iopf(struct device *dev)
if (ret)
return ret;

- ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
- if (ret)
- goto iopf_remove_device;
-
ret = pci_enable_pri(pdev, PRQ_DEPTH);
- if (ret)
- goto iopf_unregister_handler;
+ if (ret) {
+ iopf_queue_remove_device(iommu->iopf_queue, dev);
+ return ret;
+ }
+
info->pri_enabled = 1;

return 0;
-
-iopf_unregister_handler:
- iommu_unregister_device_fault_handler(dev);
-iopf_remove_device:
- iopf_queue_remove_device(iommu->iopf_queue, dev);
-
- return ret;
}

static int intel_iommu_disable_iopf(struct device *dev)
@@ -4466,11 +4458,9 @@ static int intel_iommu_disable_iopf(struct device *dev)
info->pri_enabled = 0;

/*
- * With PRI disabled and outstanding PRQs drained, unregistering
- * fault handler and removing device from iopf queue should never
- * fail.
+ * With PRI disabled and outstanding PRQs drained, removing device
+ * from iopf queue should never fail.
*/
- WARN_ON(iommu_unregister_device_fault_handler(dev));
WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev));

return 0;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index f948303b2a91..4fda01de5589 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -87,7 +87,7 @@ static void iopf_handler(struct work_struct *work)
/**
* iommu_queue_iopf - IO Page Fault handler
* @fault: fault event
- * @cookie: struct device, passed to iommu_register_device_fault_handler.
+ * @dev: struct device.
*
* Add a fault to the device workqueue, to be handled by mm.
*
@@ -124,14 +124,12 @@ static void iopf_handler(struct work_struct *work)
*
* Return: 0 on success and <0 on error.
*/
-int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
+int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
{
int ret;
struct iopf_group *group;
struct iopf_fault *iopf, *next;
struct iommu_fault_param *iopf_param;
-
- struct device *dev = cookie;
struct dev_iommu *param = dev->iommu;

lockdep_assert_held(&param->lock);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index e8f2bcea7f51..bfa3c594542c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1330,76 +1330,6 @@ void iommu_group_put(struct iommu_group *group)
}
EXPORT_SYMBOL_GPL(iommu_group_put);

-/**
- * iommu_register_device_fault_handler() - Register a device fault handler
- * @dev: the device
- * @handler: the fault handler
- * @data: private data passed as argument to the handler
- *
- * When an IOMMU fault event is received, this handler gets called with the
- * fault event and data as argument. The handler should return 0 on success. If
- * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the consumer should also
- * complete the fault by calling iommu_page_response() with one of the following
- * response code:
- * - IOMMU_PAGE_RESP_SUCCESS: retry the translation
- * - IOMMU_PAGE_RESP_INVALID: terminate the fault
- * - IOMMU_PAGE_RESP_FAILURE: terminate the fault and stop reporting
- * page faults if possible.
- *
- * Return 0 if the fault handler was installed successfully, or an error.
- */
-int iommu_register_device_fault_handler(struct device *dev,
- iommu_dev_fault_handler_t handler,
- void *data)
-{
- struct dev_iommu *param = dev->iommu;
- int ret = 0;
-
- if (!param || !param->fault_param)
- return -EINVAL;
-
- mutex_lock(&param->lock);
- /* Only allow one fault handler registered for each device */
- if (param->fault_param->handler) {
- ret = -EBUSY;
- goto done_unlock;
- }
-
- param->fault_param->handler = handler;
- param->fault_param->data = data;
-
-done_unlock:
- mutex_unlock(&param->lock);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
-
-/**
- * iommu_unregister_device_fault_handler() - Unregister the device fault handler
- * @dev: the device
- *
- * Remove the device fault handler installed with
- * iommu_register_device_fault_handler().
- *
- * Return 0 on success, or an error.
- */
-int iommu_unregister_device_fault_handler(struct device *dev)
-{
- struct dev_iommu *param = dev->iommu;
-
- if (!param || !param->fault_param)
- return -EINVAL;
-
- mutex_lock(&param->lock);
- param->fault_param->handler = NULL;
- param->fault_param->data = NULL;
- mutex_unlock(&param->lock);
-
- return 0;
-}
-EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
-
/**
* iommu_report_device_fault() - Report fault event to device driver
* @dev: the device
@@ -1424,10 +1354,6 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
/* we only report device fault if there is a handler registered */
mutex_lock(&param->lock);
fparam = param->fault_param;
- if (!fparam || !fparam->handler) {
- ret = -EINVAL;
- goto done_unlock;
- }

if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
(evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
@@ -1442,7 +1368,7 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
mutex_unlock(&fparam->lock);
}

- ret = fparam->handler(&evt->fault, fparam->data);
+ ret = iommu_queue_iopf(&evt->fault, dev);
if (ret && evt_pending) {
mutex_lock(&fparam->lock);
list_del(&evt_pending->list);
--
2.34.1


2024-01-22 08:42:48

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

The iommu fault data is currently defined in uapi/linux/iommu.h, but is
only used inside the iommu subsystem. Move it to linux/iommu.h, where it
will be more accessible to kernel drivers.

With this done, uapi/linux/iommu.h becomes empty and can be removed from
the tree.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Yi Liu <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 152 +++++++++++++++++++++++++++++++++-
include/uapi/linux/iommu.h | 161 -------------------------------------
MAINTAINERS | 1 -
3 files changed, 151 insertions(+), 163 deletions(-)
delete mode 100644 include/uapi/linux/iommu.h

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1ea2a820e1eb..472a8ce029b1 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -14,7 +14,6 @@
#include <linux/err.h>
#include <linux/of.h>
#include <linux/iova_bitmap.h>
-#include <uapi/linux/iommu.h>

#define IOMMU_READ (1 << 0)
#define IOMMU_WRITE (1 << 1)
@@ -44,6 +43,157 @@ struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;

+#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
+#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
+#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
+#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
+
+/* Generic fault types, can be expanded IRQ remapping fault */
+enum iommu_fault_type {
+ IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
+ IOMMU_FAULT_PAGE_REQ, /* page request fault */
+};
+
+enum iommu_fault_reason {
+ IOMMU_FAULT_REASON_UNKNOWN = 0,
+
+ /* Could not access the PASID table (fetch caused external abort) */
+ IOMMU_FAULT_REASON_PASID_FETCH,
+
+ /* PASID entry is invalid or has configuration errors */
+ IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
+
+ /*
+ * PASID is out of range (e.g. exceeds the maximum PASID
+ * supported by the IOMMU) or disabled.
+ */
+ IOMMU_FAULT_REASON_PASID_INVALID,
+
+ /*
+ * An external abort occurred fetching (or updating) a translation
+ * table descriptor
+ */
+ IOMMU_FAULT_REASON_WALK_EABT,
+
+ /*
+ * Could not access the page table entry (Bad address),
+ * actual translation fault
+ */
+ IOMMU_FAULT_REASON_PTE_FETCH,
+
+ /* Protection flag check failed */
+ IOMMU_FAULT_REASON_PERMISSION,
+
+ /* access flag check failed */
+ IOMMU_FAULT_REASON_ACCESS,
+
+ /* Output address of a translation stage caused Address Size fault */
+ IOMMU_FAULT_REASON_OOR_ADDRESS,
+};
+
+/**
+ * struct iommu_fault_unrecoverable - Unrecoverable fault data
+ * @reason: reason of the fault, from &enum iommu_fault_reason
+ * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
+ * @pasid: Process Address Space ID
+ * @perm: requested permission access using by the incoming transaction
+ * (IOMMU_FAULT_PERM_* values)
+ * @addr: offending page address
+ * @fetch_addr: address that caused a fetch abort, if any
+ */
+struct iommu_fault_unrecoverable {
+ __u32 reason;
+#define IOMMU_FAULT_UNRECOV_PASID_VALID (1 << 0)
+#define IOMMU_FAULT_UNRECOV_ADDR_VALID (1 << 1)
+#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID (1 << 2)
+ __u32 flags;
+ __u32 pasid;
+ __u32 perm;
+ __u64 addr;
+ __u64 fetch_addr;
+};
+
+/**
+ * struct iommu_fault_page_request - Page Request data
+ * @flags: encodes whether the corresponding fields are valid and whether this
+ * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
+ * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
+ * must have the same PASID value as the page request. When it is clear,
+ * the page response should not have a PASID.
+ * @pasid: Process Address Space ID
+ * @grpid: Page Request Group Index
+ * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
+ * @addr: page address
+ * @private_data: device-specific private information
+ */
+struct iommu_fault_page_request {
+#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
+#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];
+};
+
+/**
+ * struct iommu_fault - Generic fault data
+ * @type: fault type from &enum iommu_fault_type
+ * @padding: reserved for future use (should be zero)
+ * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
+ * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
+ * @padding2: sets the fault size to allow for future extensions
+ */
+struct iommu_fault {
+ __u32 type;
+ __u32 padding;
+ union {
+ struct iommu_fault_unrecoverable event;
+ struct iommu_fault_page_request prm;
+ __u8 padding2[56];
+ };
+};
+
+/**
+ * enum iommu_page_response_code - Return status of fault handlers
+ * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
+ * populated, retry the access. This is "Success" in PCI PRI.
+ * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
+ * this device if possible. This is "Response Failure" in PCI PRI.
+ * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
+ * access. This is "Invalid Request" in PCI PRI.
+ */
+enum iommu_page_response_code {
+ IOMMU_PAGE_RESP_SUCCESS = 0,
+ IOMMU_PAGE_RESP_INVALID,
+ IOMMU_PAGE_RESP_FAILURE,
+};
+
+/**
+ * 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
+ * @grpid: Page Request Group Index
+ * @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;
+};
+
+
/* iommu fault flags */
#define IOMMU_FAULT_READ 0x0
#define IOMMU_FAULT_WRITE 0x1
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
deleted file mode 100644
index 65d8b0234f69..000000000000
--- a/include/uapi/linux/iommu.h
+++ /dev/null
@@ -1,161 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
-/*
- * IOMMU user API definitions
- */
-
-#ifndef _UAPI_IOMMU_H
-#define _UAPI_IOMMU_H
-
-#include <linux/types.h>
-
-#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
-#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
-#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
-#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
-
-/* Generic fault types, can be expanded IRQ remapping fault */
-enum iommu_fault_type {
- IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
- IOMMU_FAULT_PAGE_REQ, /* page request fault */
-};
-
-enum iommu_fault_reason {
- IOMMU_FAULT_REASON_UNKNOWN = 0,
-
- /* Could not access the PASID table (fetch caused external abort) */
- IOMMU_FAULT_REASON_PASID_FETCH,
-
- /* PASID entry is invalid or has configuration errors */
- IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
-
- /*
- * PASID is out of range (e.g. exceeds the maximum PASID
- * supported by the IOMMU) or disabled.
- */
- IOMMU_FAULT_REASON_PASID_INVALID,
-
- /*
- * An external abort occurred fetching (or updating) a translation
- * table descriptor
- */
- IOMMU_FAULT_REASON_WALK_EABT,
-
- /*
- * Could not access the page table entry (Bad address),
- * actual translation fault
- */
- IOMMU_FAULT_REASON_PTE_FETCH,
-
- /* Protection flag check failed */
- IOMMU_FAULT_REASON_PERMISSION,
-
- /* access flag check failed */
- IOMMU_FAULT_REASON_ACCESS,
-
- /* Output address of a translation stage caused Address Size fault */
- IOMMU_FAULT_REASON_OOR_ADDRESS,
-};
-
-/**
- * struct iommu_fault_unrecoverable - Unrecoverable fault data
- * @reason: reason of the fault, from &enum iommu_fault_reason
- * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
- * @pasid: Process Address Space ID
- * @perm: requested permission access using by the incoming transaction
- * (IOMMU_FAULT_PERM_* values)
- * @addr: offending page address
- * @fetch_addr: address that caused a fetch abort, if any
- */
-struct iommu_fault_unrecoverable {
- __u32 reason;
-#define IOMMU_FAULT_UNRECOV_PASID_VALID (1 << 0)
-#define IOMMU_FAULT_UNRECOV_ADDR_VALID (1 << 1)
-#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID (1 << 2)
- __u32 flags;
- __u32 pasid;
- __u32 perm;
- __u64 addr;
- __u64 fetch_addr;
-};
-
-/**
- * struct iommu_fault_page_request - Page Request data
- * @flags: encodes whether the corresponding fields are valid and whether this
- * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
- * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
- * must have the same PASID value as the page request. When it is clear,
- * the page response should not have a PASID.
- * @pasid: Process Address Space ID
- * @grpid: Page Request Group Index
- * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
- * @addr: page address
- * @private_data: device-specific private information
- */
-struct iommu_fault_page_request {
-#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
-#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];
-};
-
-/**
- * struct iommu_fault - Generic fault data
- * @type: fault type from &enum iommu_fault_type
- * @padding: reserved for future use (should be zero)
- * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
- * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
- * @padding2: sets the fault size to allow for future extensions
- */
-struct iommu_fault {
- __u32 type;
- __u32 padding;
- union {
- struct iommu_fault_unrecoverable event;
- struct iommu_fault_page_request prm;
- __u8 padding2[56];
- };
-};
-
-/**
- * enum iommu_page_response_code - Return status of fault handlers
- * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
- * populated, retry the access. This is "Success" in PCI PRI.
- * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
- * this device if possible. This is "Response Failure" in PCI PRI.
- * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
- * access. This is "Invalid Request" in PCI PRI.
- */
-enum iommu_page_response_code {
- IOMMU_PAGE_RESP_SUCCESS = 0,
- IOMMU_PAGE_RESP_INVALID,
- IOMMU_PAGE_RESP_FAILURE,
-};
-
-/**
- * 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
- * @grpid: Page Request Group Index
- * @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;
-};
-
-#endif /* _UAPI_IOMMU_H */
diff --git a/MAINTAINERS b/MAINTAINERS
index 8d1052fa6a69..97846088d34d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11239,7 +11239,6 @@ F: drivers/iommu/
F: include/linux/iommu.h
F: include/linux/iova.h
F: include/linux/of_iommu.h
-F: include/uapi/linux/iommu.h

IOMMUFD
M: Jason Gunthorpe <[email protected]>
--
2.34.1


2024-01-22 09:31:17

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 14/16] iommu: Track iopf group instead of last fault

Previously, before a group of page faults was passed to the domain's iopf
handler, the last page fault of the group was kept in the list of
iommu_fault_param::faults. In the page fault response path, the group's
last page fault was used to look up the list, and the page faults were
responded to device only if there was a matched fault.

The previous approach seems unnecessarily complex and not performance
friendly. Put the page fault group itself to the outstanding fault list.
It can be removed in the page fault response path or in the
iopf_queue_remove_device() path. The pending list is protected by
iommu_fault_param::lock. To allow checking for the group's presence in
the list using list_empty(), the iopf group should be removed from the
list with list_del_init().

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Tested-by: Yan Zhao <[email protected]>
---
include/linux/iommu.h | 2 +
drivers/iommu/io-pgfault.c | 237 +++++++++++++------------------------
2 files changed, 87 insertions(+), 152 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d9a99a978ffa..48196efc9327 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -129,6 +129,8 @@ struct iopf_fault {
struct iopf_group {
struct iopf_fault last_fault;
struct list_head faults;
+ /* list node for iommu_fault_param::faults */
+ struct list_head pending_node;
struct work_struct work;
struct iommu_domain *domain;
/* The device's fault data parameter. */
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 26e100ca3221..c22e13df84c2 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -78,12 +78,33 @@ static struct iommu_domain *get_domain_for_iopf(struct device *dev,
return domain;
}

+/* Non-last request of a group. Postpone until the last one. */
+static int report_partial_fault(struct iommu_fault_param *fault_param,
+ struct iommu_fault *fault)
+{
+ struct iopf_fault *iopf;
+
+ iopf = kzalloc(sizeof(*iopf), GFP_KERNEL);
+ if (!iopf)
+ return -ENOMEM;
+
+ iopf->fault = *fault;
+
+ mutex_lock(&fault_param->lock);
+ list_add(&iopf->list, &fault_param->partial);
+ mutex_unlock(&fault_param->lock);
+
+ return 0;
+}
+
/**
- * iommu_handle_iopf - IO Page Fault handler
- * @fault: fault event
- * @iopf_param: the fault parameter of the device.
+ * iommu_report_device_fault() - Report fault event to device driver
+ * @dev: the device
+ * @evt: fault event data
*
- * Add a fault to the device workqueue, to be handled by mm.
+ * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
+ * handler. When this function fails and the fault is recoverable, it is the
+ * caller's responsibility to complete the fault.
*
* This module doesn't handle PCI PASID Stop Marker; IOMMU drivers must discard
* them before reporting faults. A PASID Stop Marker (LRW = 0b100) doesn't
@@ -118,34 +139,37 @@ static struct iommu_domain *get_domain_for_iopf(struct device *dev,
*
* Return: 0 on success and <0 on error.
*/
-static int iommu_handle_iopf(struct iommu_fault *fault,
- struct iommu_fault_param *iopf_param)
+int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
{
- int ret;
+ struct iommu_fault *fault = &evt->fault;
+ struct iommu_fault_param *iopf_param;
+ struct iopf_fault *iopf, *next;
+ struct iommu_domain *domain;
struct iopf_group *group;
- struct iommu_domain *domain;
- struct iopf_fault *iopf, *next;
- struct device *dev = iopf_param->dev;
-
- lockdep_assert_held(&iopf_param->lock);
+ int ret;

if (fault->type != IOMMU_FAULT_PAGE_REQ)
- /* Not a recoverable page fault */
return -EOPNOTSUPP;

+ iopf_param = iopf_get_dev_fault_param(dev);
+ if (!iopf_param)
+ return -ENODEV;
+
if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
- iopf = kzalloc(sizeof(*iopf), GFP_KERNEL);
- if (!iopf)
- return -ENOMEM;
+ ret = report_partial_fault(iopf_param, fault);
+ iopf_put_dev_fault_param(iopf_param);

- iopf->fault = *fault;
-
- /* Non-last request of a group. Postpone until the last one */
- list_add(&iopf->list, &iopf_param->partial);
-
- return 0;
+ return ret;
}

+ /*
+ * This is the last page fault of a group. Allocate an iopf group and
+ * pass it to domain's page fault handler. The group holds a reference
+ * count of the fault parameter. It will be released after response or
+ * error path of this function. If an error is returned, the caller
+ * will send a response to the hardware. We need to clean up before
+ * leaving, otherwise partial faults will be stuck.
+ */
domain = get_domain_for_iopf(dev, fault);
if (!domain) {
ret = -EINVAL;
@@ -154,11 +178,6 @@ static int iommu_handle_iopf(struct iommu_fault *fault,

group = kzalloc(sizeof(*group), GFP_KERNEL);
if (!group) {
- /*
- * The caller will send a response to the hardware. But we do
- * need to clean up before leaving, otherwise partial faults
- * will be stuck.
- */
ret = -ENOMEM;
goto cleanup_partial;
}
@@ -166,145 +185,45 @@ static int iommu_handle_iopf(struct iommu_fault *fault,
group->fault_param = iopf_param;
group->last_fault.fault = *fault;
INIT_LIST_HEAD(&group->faults);
+ INIT_LIST_HEAD(&group->pending_node);
group->domain = domain;
list_add(&group->last_fault.list, &group->faults);

/* See if we have partial faults for this group */
+ mutex_lock(&iopf_param->lock);
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
if (iopf->fault.prm.grpid == fault->prm.grpid)
/* Insert *before* the last fault */
list_move(&iopf->list, &group->faults);
}
-
+ list_add(&group->pending_node, &iopf_param->faults);
mutex_unlock(&iopf_param->lock);
+
ret = domain->iopf_handler(group);
- mutex_lock(&iopf_param->lock);
- if (ret)
+ if (ret) {
+ mutex_lock(&iopf_param->lock);
+ list_del_init(&group->pending_node);
+ mutex_unlock(&iopf_param->lock);
iopf_free_group(group);
+ }

return ret;
+
cleanup_partial:
+ mutex_lock(&iopf_param->lock);
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
if (iopf->fault.prm.grpid == fault->prm.grpid) {
list_del(&iopf->list);
kfree(iopf);
}
}
- return ret;
-}
-
-/**
- * iommu_report_device_fault() - Report fault event to device driver
- * @dev: the device
- * @evt: fault event data
- *
- * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
- * handler. When this function fails and the fault is recoverable, it is the
- * caller's responsibility to complete the fault.
- *
- * Return 0 on success, or an error.
- */
-int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
-{
- bool last_prq = evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
- (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE);
- struct iommu_fault_param *fault_param;
- struct iopf_fault *evt_pending;
- int ret;
-
- fault_param = iopf_get_dev_fault_param(dev);
- if (!fault_param)
- return -EINVAL;
-
- mutex_lock(&fault_param->lock);
- if (last_prq) {
- evt_pending = kmemdup(evt, sizeof(struct iopf_fault),
- GFP_KERNEL);
- if (!evt_pending) {
- ret = -ENOMEM;
- goto err_unlock;
- }
- list_add_tail(&evt_pending->list, &fault_param->faults);
- }
-
- ret = iommu_handle_iopf(&evt->fault, fault_param);
- if (ret)
- goto err_free;
-
- mutex_unlock(&fault_param->lock);
- /* The reference count of fault_param is now held by iopf_group. */
- if (!last_prq)
- iopf_put_dev_fault_param(fault_param);
-
- return 0;
-err_free:
- if (last_prq) {
- list_del(&evt_pending->list);
- kfree(evt_pending);
- }
-err_unlock:
- mutex_unlock(&fault_param->lock);
- iopf_put_dev_fault_param(fault_param);
+ mutex_unlock(&iopf_param->lock);
+ iopf_put_dev_fault_param(iopf_param);

return ret;
}
EXPORT_SYMBOL_GPL(iommu_report_device_fault);

-static int iommu_page_response(struct iopf_group *group,
- struct iommu_page_response *msg)
-{
- bool needs_pasid;
- int ret = -EINVAL;
- struct iopf_fault *evt;
- struct iommu_fault_page_request *prm;
- struct device *dev = group->fault_param->dev;
- const struct iommu_ops *ops = dev_iommu_ops(dev);
- bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
- struct iommu_fault_param *fault_param = group->fault_param;
-
- /* Only send response if there is a fault report pending */
- mutex_lock(&fault_param->lock);
- if (list_empty(&fault_param->faults)) {
- dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
- goto done_unlock;
- }
- /*
- * Check if we have a matching page request pending to respond,
- * otherwise return -EINVAL
- */
- list_for_each_entry(evt, &fault_param->faults, list) {
- prm = &evt->fault.prm;
- if (prm->grpid != msg->grpid)
- continue;
-
- /*
- * If the PASID is required, the corresponding request is
- * matched using the group ID, the PASID valid bit and the PASID
- * value. Otherwise only the group ID matches request and
- * response.
- */
- needs_pasid = prm->flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID;
- if (needs_pasid && (!has_pasid || msg->pasid != prm->pasid))
- continue;
-
- if (!needs_pasid && has_pasid) {
- /* No big deal, just clear it. */
- msg->flags &= ~IOMMU_PAGE_RESP_PASID_VALID;
- msg->pasid = 0;
- }
-
- ret = ops->page_response(dev, evt, msg);
- list_del(&evt->list);
- kfree(evt);
- break;
- }
-
-done_unlock:
- mutex_unlock(&fault_param->lock);
-
- return ret;
-}
-
/**
* iopf_queue_flush_dev - Ensure that all queued faults have been processed
* @dev: the endpoint whose faults need to be flushed.
@@ -346,18 +265,30 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
int iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status)
{
+ struct iommu_fault_param *fault_param = group->fault_param;
struct iopf_fault *iopf = &group->last_fault;
+ struct device *dev = group->fault_param->dev;
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
struct iommu_page_response resp = {
.pasid = iopf->fault.prm.pasid,
.grpid = iopf->fault.prm.grpid,
.code = status,
};
+ int ret = -EINVAL;

if ((iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) &&
(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID))
resp.flags = IOMMU_PAGE_RESP_PASID_VALID;

- return iommu_page_response(group, &resp);
+ /* Only send response if there is a fault report pending */
+ mutex_lock(&fault_param->lock);
+ if (!list_empty(&group->pending_node)) {
+ ret = ops->page_response(dev, &group->last_fault, &resp);
+ list_del_init(&group->pending_node);
+ }
+ mutex_unlock(&fault_param->lock);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(iopf_group_response);

@@ -468,8 +399,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
*/
void iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
{
- struct iopf_fault *iopf, *next;
- struct iommu_page_response resp;
+ struct iopf_fault *partial_iopf;
+ struct iopf_fault *next;
+ struct iopf_group *group, *temp;
struct dev_iommu *param = dev->iommu;
struct iommu_fault_param *fault_param;
const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -483,21 +415,22 @@ void iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
goto unlock;

mutex_lock(&fault_param->lock);
- list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
- kfree(iopf);
+ list_for_each_entry_safe(partial_iopf, next, &fault_param->partial, list)
+ kfree(partial_iopf);

- list_for_each_entry_safe(iopf, next, &fault_param->faults, list) {
- memset(&resp, 0, sizeof(struct iommu_page_response));
- resp.pasid = iopf->fault.prm.pasid;
- resp.grpid = iopf->fault.prm.grpid;
- resp.code = IOMMU_PAGE_RESP_INVALID;
+ list_for_each_entry_safe(group, temp, &fault_param->faults, pending_node) {
+ struct iopf_fault *iopf = &group->last_fault;
+ struct iommu_page_response resp = {
+ .pasid = iopf->fault.prm.pasid,
+ .grpid = iopf->fault.prm.grpid,
+ .code = IOMMU_PAGE_RESP_INVALID
+ };

if (iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID)
resp.flags = IOMMU_PAGE_RESP_PASID_VALID;

ops->page_response(dev, iopf, &resp);
- list_del(&iopf->list);
- kfree(iopf);
+ list_del_init(&group->pending_node);
}
mutex_unlock(&fault_param->lock);

--
2.34.1


2024-01-22 10:43:20

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 03/16] iommu: Remove unrecoverable fault data

The unrecoverable fault data is not used anywhere. Remove it to avoid
dead code.

Suggested-by: Kevin Tian <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 72 ++-----------------------------------------
1 file changed, 2 insertions(+), 70 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 472a8ce029b1..c960c4fae3bc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -50,67 +50,7 @@ struct iommu_dma_cookie;

/* Generic fault types, can be expanded IRQ remapping fault */
enum iommu_fault_type {
- IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
- IOMMU_FAULT_PAGE_REQ, /* page request fault */
-};
-
-enum iommu_fault_reason {
- IOMMU_FAULT_REASON_UNKNOWN = 0,
-
- /* Could not access the PASID table (fetch caused external abort) */
- IOMMU_FAULT_REASON_PASID_FETCH,
-
- /* PASID entry is invalid or has configuration errors */
- IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
-
- /*
- * PASID is out of range (e.g. exceeds the maximum PASID
- * supported by the IOMMU) or disabled.
- */
- IOMMU_FAULT_REASON_PASID_INVALID,
-
- /*
- * An external abort occurred fetching (or updating) a translation
- * table descriptor
- */
- IOMMU_FAULT_REASON_WALK_EABT,
-
- /*
- * Could not access the page table entry (Bad address),
- * actual translation fault
- */
- IOMMU_FAULT_REASON_PTE_FETCH,
-
- /* Protection flag check failed */
- IOMMU_FAULT_REASON_PERMISSION,
-
- /* access flag check failed */
- IOMMU_FAULT_REASON_ACCESS,
-
- /* Output address of a translation stage caused Address Size fault */
- IOMMU_FAULT_REASON_OOR_ADDRESS,
-};
-
-/**
- * struct iommu_fault_unrecoverable - Unrecoverable fault data
- * @reason: reason of the fault, from &enum iommu_fault_reason
- * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
- * @pasid: Process Address Space ID
- * @perm: requested permission access using by the incoming transaction
- * (IOMMU_FAULT_PERM_* values)
- * @addr: offending page address
- * @fetch_addr: address that caused a fetch abort, if any
- */
-struct iommu_fault_unrecoverable {
- __u32 reason;
-#define IOMMU_FAULT_UNRECOV_PASID_VALID (1 << 0)
-#define IOMMU_FAULT_UNRECOV_ADDR_VALID (1 << 1)
-#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID (1 << 2)
- __u32 flags;
- __u32 pasid;
- __u32 perm;
- __u64 addr;
- __u64 fetch_addr;
+ IOMMU_FAULT_PAGE_REQ = 1, /* page request fault */
};

/**
@@ -142,19 +82,11 @@ struct iommu_fault_page_request {
/**
* struct iommu_fault - Generic fault data
* @type: fault type from &enum iommu_fault_type
- * @padding: reserved for future use (should be zero)
- * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
* @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
- * @padding2: sets the fault size to allow for future extensions
*/
struct iommu_fault {
__u32 type;
- __u32 padding;
- union {
- struct iommu_fault_unrecoverable event;
- struct iommu_fault_page_request prm;
- __u8 padding2[56];
- };
+ struct iommu_fault_page_request prm;
};

/**
--
2.34.1


2024-01-22 11:06:35

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 09/16] iommu: Make iommu_queue_iopf() more generic

Make iommu_queue_iopf() more generic by making the iopf_group a minimal
set of iopf's that an iopf handler of domain should handle and respond
to. Add domain parameter to struct iopf_group so that the handler can
retrieve and use it directly.

Change iommu_queue_iopf() to forward groups of iopf's to the domain's
iopf handler. This is also a necessary step to decouple the sva iopf
handling code from this interface.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Yi Liu <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 4 +--
drivers/iommu/iommu-sva.h | 6 ++--
drivers/iommu/io-pgfault.c | 68 +++++++++++++++++++++++++++++++-------
drivers/iommu/iommu-sva.c | 3 +-
4 files changed, 61 insertions(+), 20 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c9d4f175f121..791f183a988e 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -130,6 +130,7 @@ struct iopf_group {
struct list_head faults;
struct work_struct work;
struct device *dev;
+ struct iommu_domain *domain;
};

/**
@@ -209,8 +210,7 @@ struct iommu_domain {
unsigned long pgsize_bitmap; /* Bitmap of page sizes in use */
struct iommu_domain_geometry geometry;
struct iommu_dma_cookie *iova_cookie;
- enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
- void *data);
+ int (*iopf_handler)(struct iopf_group *group);
void *fault_data;
union {
struct {
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index de7819c796ce..27c8da115b41 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -22,8 +22,7 @@ int iopf_queue_flush_dev(struct device *dev);
struct iopf_queue *iopf_queue_alloc(const char *name);
void iopf_queue_free(struct iopf_queue *queue);
int iopf_queue_discard_partial(struct iopf_queue *queue);
-enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
+int iommu_sva_handle_iopf(struct iopf_group *group);

#else /* CONFIG_IOMMU_SVA */
static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
@@ -62,8 +61,7 @@ static inline int iopf_queue_discard_partial(struct iopf_queue *queue)
return -ENODEV;
}

-static inline enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+static inline int iommu_sva_handle_iopf(struct iopf_group *group)
{
return IOMMU_PAGE_RESP_INVALID;
}
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index c7e6bbed5c05..13cd0929e766 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -13,6 +13,9 @@

#include "iommu-sva.h"

+enum iommu_page_response_code
+iommu_sva_handle_mm(struct iommu_fault *fault, struct mm_struct *mm);
+
static void iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;
@@ -45,29 +48,48 @@ static void iopf_handler(struct work_struct *work)
{
struct iopf_fault *iopf;
struct iopf_group *group;
- struct iommu_domain *domain;
enum iommu_page_response_code status = IOMMU_PAGE_RESP_SUCCESS;

group = container_of(work, struct iopf_group, work);
- domain = iommu_get_domain_for_dev_pasid(group->dev,
- group->last_fault.fault.prm.pasid, 0);
- if (!domain || !domain->iopf_handler)
- status = IOMMU_PAGE_RESP_INVALID;
-
list_for_each_entry(iopf, &group->faults, list) {
/*
* For the moment, errors are sticky: don't handle subsequent
* faults in the group if there is an error.
*/
- if (status == IOMMU_PAGE_RESP_SUCCESS)
- status = domain->iopf_handler(&iopf->fault,
- domain->fault_data);
+ if (status != IOMMU_PAGE_RESP_SUCCESS)
+ break;
+
+ status = iommu_sva_handle_mm(&iopf->fault, group->domain->mm);
}

iopf_complete_group(group->dev, &group->last_fault, status);
iopf_free_group(group);
}

+static struct iommu_domain *get_domain_for_iopf(struct device *dev,
+ struct iommu_fault *fault)
+{
+ struct iommu_domain *domain;
+
+ if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) {
+ domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
+ if (IS_ERR(domain))
+ domain = NULL;
+ } else {
+ domain = iommu_get_domain_for_dev(dev);
+ }
+
+ if (!domain || !domain->iopf_handler) {
+ dev_warn_ratelimited(dev,
+ "iopf (pasid %d) without domain attached or handler installed\n",
+ fault->prm.pasid);
+
+ return NULL;
+ }
+
+ return domain;
+}
+
/**
* iommu_queue_iopf - IO Page Fault handler
* @fault: fault event
@@ -112,6 +134,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
{
int ret;
struct iopf_group *group;
+ struct iommu_domain *domain;
struct iopf_fault *iopf, *next;
struct iommu_fault_param *iopf_param;
struct dev_iommu *param = dev->iommu;
@@ -143,6 +166,12 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
return 0;
}

+ domain = get_domain_for_iopf(dev, fault);
+ if (!domain) {
+ ret = -EINVAL;
+ goto cleanup_partial;
+ }
+
group = kzalloc(sizeof(*group), GFP_KERNEL);
if (!group) {
/*
@@ -157,8 +186,8 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
group->dev = dev;
group->last_fault.fault = *fault;
INIT_LIST_HEAD(&group->faults);
+ group->domain = domain;
list_add(&group->last_fault.list, &group->faults);
- INIT_WORK(&group->work, iopf_handler);

/* See if we have partial faults for this group */
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
@@ -167,9 +196,13 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
list_move(&iopf->list, &group->faults);
}

- queue_work(iopf_param->queue->wq, &group->work);
- return 0;
+ mutex_unlock(&iopf_param->lock);
+ ret = domain->iopf_handler(group);
+ mutex_lock(&iopf_param->lock);
+ if (ret)
+ iopf_free_group(group);

+ return ret;
cleanup_partial:
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
if (iopf->fault.prm.grpid == fault->prm.grpid) {
@@ -181,6 +214,17 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
}
EXPORT_SYMBOL_GPL(iommu_queue_iopf);

+int iommu_sva_handle_iopf(struct iopf_group *group)
+{
+ struct iommu_fault_param *fault_param = group->dev->iommu->fault_param;
+
+ INIT_WORK(&group->work, iopf_handler);
+ if (!queue_work(fault_param->queue->wq, &group->work))
+ return -EBUSY;
+
+ return 0;
+}
+
/**
* iopf_queue_flush_dev - Ensure that all queued faults have been processed
* @dev: the endpoint whose faults need to be flushed.
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index c3fc9201d0be..fcae7308fcb7 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -163,11 +163,10 @@ EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
* I/O page fault handler for SVA
*/
enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+iommu_sva_handle_mm(struct iommu_fault *fault, struct mm_struct *mm)
{
vm_fault_t ret;
struct vm_area_struct *vma;
- struct mm_struct *mm = data;
unsigned int access_flags = 0;
unsigned int fault_flags = FAULT_FLAG_REMOTE;
struct iommu_fault_page_request *prm = &fault->prm;
--
2.34.1


2024-01-22 12:11:29

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 12/16] iommu: Use refcount for fault data access

The per-device fault data structure stores information about faults
occurring on a device. Its lifetime spans from IOPF enablement to
disablement. Multiple paths, including IOPF reporting, handling, and
responding, may access it concurrently.

Previously, a mutex protected the fault data from use after free. But
this is not performance friendly due to the critical nature of IOPF
handling paths.

Refine this with a refcount-based approach. The fault data pointer is
obtained within an RCU read region with a refcount. The fault data
pointer is returned for usage only when the pointer is valid and a
refcount is successfully obtained. The fault data is freed with
kfree_rcu(), ensuring data is only freed after all RCU critical regions
complete.

An iopf handling work starts once an iopf group is created. The handling
work continues until iommu_page_response() is called to respond to the
iopf and the iopf group is freed. During this time, the device fault
parameter should always be available. Add a pointer to the device fault
parameter in the iopf_group structure and hold the reference until the
iopf_group is freed.

Make iommu_page_response() static as it is only used in io-pgfault.c.

Co-developed-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Tested-by: Yan Zhao <[email protected]>
---
include/linux/iommu.h | 17 +++--
drivers/iommu/io-pgfault.c | 127 +++++++++++++++++++++++--------------
drivers/iommu/iommu-sva.c | 2 +-
3 files changed, 88 insertions(+), 58 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fc912aed7886..396d7b0d88b2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -41,6 +41,7 @@ struct iommu_dirty_ops;
struct notifier_block;
struct iommu_sva;
struct iommu_dma_cookie;
+struct iommu_fault_param;

#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -129,8 +130,9 @@ struct iopf_group {
struct iopf_fault last_fault;
struct list_head faults;
struct work_struct work;
- struct device *dev;
struct iommu_domain *domain;
+ /* The device's fault data parameter. */
+ struct iommu_fault_param *fault_param;
};

/**
@@ -679,6 +681,8 @@ struct iommu_device {
/**
* struct iommu_fault_param - per-device IOMMU fault data
* @lock: protect pending faults list
+ * @users: user counter to manage the lifetime of the data
+ * @ruc: rcu head for kfree_rcu()
* @dev: the device that owns this param
* @queue: IOPF queue
* @queue_list: index into queue->devices
@@ -688,6 +692,8 @@ struct iommu_device {
*/
struct iommu_fault_param {
struct mutex lock;
+ refcount_t users;
+ struct rcu_head rcu;

struct device *dev;
struct iopf_queue *queue;
@@ -715,7 +721,7 @@ struct iommu_fault_param {
*/
struct dev_iommu {
struct mutex lock;
- struct iommu_fault_param *fault_param;
+ struct iommu_fault_param __rcu *fault_param;
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void *priv;
@@ -1543,7 +1549,6 @@ void iopf_queue_free(struct iopf_queue *queue);
int iopf_queue_discard_partial(struct iopf_queue *queue);
void iopf_free_group(struct iopf_group *group);
int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
-int iommu_page_response(struct device *dev, struct iommu_page_response *msg);
int iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status);
#else
@@ -1588,12 +1593,6 @@ iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
return -ENODEV;
}

-static inline int
-iommu_page_response(struct device *dev, struct iommu_page_response *msg)
-{
- return -ENODEV;
-}
-
static inline int iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status)
{
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 5aea8402be47..ce7058892b59 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -13,6 +13,32 @@

#include "iommu-priv.h"

+/*
+ * Return the fault parameter of a device if it exists. Otherwise, return NULL.
+ * On a successful return, the caller takes a reference of this parameter and
+ * should put it after use by calling iopf_put_dev_fault_param().
+ */
+static struct iommu_fault_param *iopf_get_dev_fault_param(struct device *dev)
+{
+ struct dev_iommu *param = dev->iommu;
+ struct iommu_fault_param *fault_param;
+
+ rcu_read_lock();
+ fault_param = rcu_dereference(param->fault_param);
+ if (fault_param && !refcount_inc_not_zero(&fault_param->users))
+ fault_param = NULL;
+ rcu_read_unlock();
+
+ return fault_param;
+}
+
+/* Caller must hold a reference of the fault parameter. */
+static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param)
+{
+ if (refcount_dec_and_test(&fault_param->users))
+ kfree_rcu(fault_param, rcu);
+}
+
void iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;
@@ -22,6 +48,8 @@ void iopf_free_group(struct iopf_group *group)
kfree(iopf);
}

+ /* Pair with iommu_report_device_fault(). */
+ iopf_put_dev_fault_param(group->fault_param);
kfree(group);
}
EXPORT_SYMBOL_GPL(iopf_free_group);
@@ -135,7 +163,7 @@ static int iommu_handle_iopf(struct iommu_fault *fault,
goto cleanup_partial;
}

- group->dev = dev;
+ group->fault_param = iopf_param;
group->last_fault.fault = *fault;
INIT_LIST_HEAD(&group->faults);
group->domain = domain;
@@ -178,64 +206,61 @@ static int iommu_handle_iopf(struct iommu_fault *fault,
*/
int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
{
+ bool last_prq = evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
+ (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE);
struct iommu_fault_param *fault_param;
- struct iopf_fault *evt_pending = NULL;
- struct dev_iommu *param = dev->iommu;
- int ret = 0;
+ struct iopf_fault *evt_pending;
+ int ret;

- mutex_lock(&param->lock);
- fault_param = param->fault_param;
- if (!fault_param) {
- mutex_unlock(&param->lock);
+ fault_param = iopf_get_dev_fault_param(dev);
+ if (!fault_param)
return -EINVAL;
- }

mutex_lock(&fault_param->lock);
- if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
- (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
+ if (last_prq) {
evt_pending = kmemdup(evt, sizeof(struct iopf_fault),
GFP_KERNEL);
if (!evt_pending) {
ret = -ENOMEM;
- goto done_unlock;
+ goto err_unlock;
}
list_add_tail(&evt_pending->list, &fault_param->faults);
}

ret = iommu_handle_iopf(&evt->fault, fault_param);
- if (ret && evt_pending) {
+ if (ret)
+ goto err_free;
+
+ mutex_unlock(&fault_param->lock);
+ /* The reference count of fault_param is now held by iopf_group. */
+ if (!last_prq)
+ iopf_put_dev_fault_param(fault_param);
+
+ return 0;
+err_free:
+ if (last_prq) {
list_del(&evt_pending->list);
kfree(evt_pending);
}
-done_unlock:
+err_unlock:
mutex_unlock(&fault_param->lock);
- mutex_unlock(&param->lock);
+ iopf_put_dev_fault_param(fault_param);

return ret;
}
EXPORT_SYMBOL_GPL(iommu_report_device_fault);

-int iommu_page_response(struct device *dev,
- struct iommu_page_response *msg)
+static int iommu_page_response(struct iopf_group *group,
+ struct iommu_page_response *msg)
{
bool needs_pasid;
int ret = -EINVAL;
struct iopf_fault *evt;
struct iommu_fault_page_request *prm;
- struct dev_iommu *param = dev->iommu;
- struct iommu_fault_param *fault_param;
+ struct device *dev = group->fault_param->dev;
const struct iommu_ops *ops = dev_iommu_ops(dev);
bool has_pasid = msg->flags & IOMMU_PAGE_RESP_PASID_VALID;
-
- if (!ops->page_response)
- return -ENODEV;
-
- mutex_lock(&param->lock);
- fault_param = param->fault_param;
- if (!fault_param) {
- mutex_unlock(&param->lock);
- return -EINVAL;
- }
+ struct iommu_fault_param *fault_param = group->fault_param;

/* Only send response if there is a fault report pending */
mutex_lock(&fault_param->lock);
@@ -276,10 +301,9 @@ int iommu_page_response(struct device *dev,

done_unlock:
mutex_unlock(&fault_param->lock);
- mutex_unlock(&param->lock);
+
return ret;
}
-EXPORT_SYMBOL_GPL(iommu_page_response);

/**
* iopf_queue_flush_dev - Ensure that all queued faults have been processed
@@ -295,22 +319,20 @@ EXPORT_SYMBOL_GPL(iommu_page_response);
*/
int iopf_queue_flush_dev(struct device *dev)
{
- int ret = 0;
struct iommu_fault_param *iopf_param;
- struct dev_iommu *param = dev->iommu;

- if (!param)
+ /*
+ * It's a driver bug to be here after iopf_queue_remove_device().
+ * Therefore, it's safe to dereference the fault parameter without
+ * holding the lock.
+ */
+ iopf_param = rcu_dereference_check(dev->iommu->fault_param, true);
+ if (WARN_ON(!iopf_param))
return -ENODEV;

- mutex_lock(&param->lock);
- iopf_param = param->fault_param;
- if (iopf_param)
- flush_workqueue(iopf_param->queue->wq);
- else
- ret = -ENODEV;
- mutex_unlock(&param->lock);
+ flush_workqueue(iopf_param->queue->wq);

- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);

@@ -335,7 +357,7 @@ int iopf_group_response(struct iopf_group *group,
(iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID))
resp.flags = IOMMU_PAGE_RESP_PASID_VALID;

- return iommu_page_response(group->dev, &resp);
+ return iommu_page_response(group, &resp);
}
EXPORT_SYMBOL_GPL(iopf_group_response);

@@ -384,10 +406,15 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
int ret = 0;
struct dev_iommu *param = dev->iommu;
struct iommu_fault_param *fault_param;
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+ if (!ops->page_response)
+ return -ENODEV;

mutex_lock(&queue->lock);
mutex_lock(&param->lock);
- if (param->fault_param) {
+ if (rcu_dereference_check(param->fault_param,
+ lockdep_is_held(&param->lock))) {
ret = -EBUSY;
goto done_unlock;
}
@@ -402,10 +429,11 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
INIT_LIST_HEAD(&fault_param->faults);
INIT_LIST_HEAD(&fault_param->partial);
fault_param->dev = dev;
+ refcount_set(&fault_param->users, 1);
list_add(&fault_param->queue_list, &queue->devices);
fault_param->queue = queue;

- param->fault_param = fault_param;
+ rcu_assign_pointer(param->fault_param, fault_param);

done_unlock:
mutex_unlock(&param->lock);
@@ -429,10 +457,12 @@ int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
int ret = 0;
struct iopf_fault *iopf, *next;
struct dev_iommu *param = dev->iommu;
- struct iommu_fault_param *fault_param = param->fault_param;
+ struct iommu_fault_param *fault_param;

mutex_lock(&queue->lock);
mutex_lock(&param->lock);
+ fault_param = rcu_dereference_check(param->fault_param,
+ lockdep_is_held(&param->lock));
if (!fault_param) {
ret = -ENODEV;
goto unlock;
@@ -454,8 +484,9 @@ int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
kfree(iopf);

- param->fault_param = NULL;
- kfree(fault_param);
+ /* dec the ref owned by iopf_queue_add_device() */
+ rcu_assign_pointer(param->fault_param, NULL);
+ iopf_put_dev_fault_param(fault_param);
unlock:
mutex_unlock(&param->lock);
mutex_unlock(&queue->lock);
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 9de878e40413..b51995b4fe90 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -251,7 +251,7 @@ static void iommu_sva_handle_iopf(struct work_struct *work)

static int iommu_sva_iopf_handler(struct iopf_group *group)
{
- struct iommu_fault_param *fault_param = group->dev->iommu->fault_param;
+ struct iommu_fault_param *fault_param = group->fault_param;

INIT_WORK(&group->work, iommu_sva_handle_iopf);
if (!queue_work(fault_param->queue->wq, &group->work))
--
2.34.1


2024-01-22 12:24:36

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 16/16] iommu: Make iommu_report_device_fault() reutrn void

As the iommu_report_device_fault() has been converted to auto-respond a
page fault if it fails to enqueue it, there's no need to return a code
in any case. Make it return void.

Suggested-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 5 ++---
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
drivers/iommu/intel/svm.c | 19 +++++++----------
drivers/iommu/io-pgfault.c | 23 +++++++--------------
4 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d7b6f4017254..1ccad10e8164 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1549,7 +1549,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
void iopf_queue_free(struct iopf_queue *queue);
int iopf_queue_discard_partial(struct iopf_queue *queue);
void iopf_free_group(struct iopf_group *group);
-int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
+void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
void iopf_group_response(struct iopf_group *group,
enum iommu_page_response_code status);
#else
@@ -1587,10 +1587,9 @@ static inline void iopf_free_group(struct iopf_group *group)
{
}

-static inline int
+static inline void
iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
{
- return -ENODEV;
}

static inline void iopf_group_response(struct iopf_group *group,
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 42eb59cb99f4..02580364acda 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1455,7 +1455,7 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
/* IRQ and event handlers */
static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
{
- int ret;
+ int ret = 0;
u32 perm = 0;
struct arm_smmu_master *master;
bool ssid_valid = evt[0] & EVTQ_0_SSV;
@@ -1511,7 +1511,7 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
goto out_unlock;
}

- ret = iommu_report_device_fault(master->dev, &fault_evt);
+ iommu_report_device_fault(master->dev, &fault_evt);
out_unlock:
mutex_unlock(&smmu->streams_mutex);
return ret;
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index 2f8716636dbb..b644d57da841 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -561,14 +561,11 @@ static int prq_to_iommu_prot(struct page_req_dsc *req)
return prot;
}

-static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
- struct page_req_dsc *desc)
+static void intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
+ struct page_req_dsc *desc)
{
struct iopf_fault event = { };

- if (!dev || !dev_is_pci(dev))
- return -ENODEV;
-
/* Fill in event data for device specific processing */
event.fault.type = IOMMU_FAULT_PAGE_REQ;
event.fault.prm.addr = (u64)desc->addr << VTD_PAGE_SHIFT;
@@ -601,7 +598,7 @@ static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
event.fault.prm.private_data[0] = ktime_to_ns(ktime_get());
}

- return iommu_report_device_fault(dev, &event);
+ iommu_report_device_fault(dev, &event);
}

static void handle_bad_prq_event(struct intel_iommu *iommu,
@@ -704,12 +701,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
if (!pdev)
goto bad_req;

- if (intel_svm_prq_report(iommu, &pdev->dev, req))
- handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
- else
- trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
- req->priv_data[0], req->priv_data[1],
- iommu->prq_seq_number++);
+ intel_svm_prq_report(iommu, &pdev->dev, req);
+ trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
+ req->priv_data[0], req->priv_data[1],
+ iommu->prq_seq_number++);
pci_dev_put(pdev);
prq_advance:
head = (head + sizeof(*req)) & PRQ_RING_MASK;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 6e63e5a02884..b64229dab976 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -179,23 +179,21 @@ static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
*
* Return: 0 on success and <0 on error.
*/
-int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
+void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
{
struct iommu_fault *fault = &evt->fault;
struct iommu_fault_param *iopf_param;
struct iopf_group abort_group = {};
struct iopf_group *group;
- int ret;

iopf_param = iopf_get_dev_fault_param(dev);
if (WARN_ON(!iopf_param))
- return -ENODEV;
+ return;

if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
- ret = report_partial_fault(iopf_param, fault);
+ report_partial_fault(iopf_param, fault);
iopf_put_dev_fault_param(iopf_param);
/* A request that is not the last does not need to be ack'd */
- return ret;
}

/*
@@ -207,25 +205,21 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
* leaving, otherwise partial faults will be stuck.
*/
group = iopf_group_alloc(iopf_param, evt, &abort_group);
- if (group == &abort_group) {
- ret = -ENOMEM;
+ if (group == &abort_group)
goto err_abort;
- }

group->domain = get_domain_for_iopf(dev, fault);
- if (!group->domain) {
- ret = -EINVAL;
+ if (!group->domain)
goto err_abort;
- }

/*
* On success iopf_handler must call iopf_group_response() and
* iopf_free_group()
*/
- ret = group->domain->iopf_handler(group);
- if (ret)
+ if (group->domain->iopf_handler(group))
goto err_abort;
- return 0;
+
+ return;

err_abort:
iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
@@ -233,7 +227,6 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
__iopf_free_group(group);
else
iopf_free_group(group);
- return ret;
}
EXPORT_SYMBOL_GPL(iommu_report_device_fault);

--
2.34.1


2024-01-22 12:44:19

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions

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 <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Reviewed-by: Yi Liu <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
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;
-
/* 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


2024-01-22 13:25:53

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 13/16] iommu: Improve iopf_queue_remove_device()

Convert iopf_queue_remove_device() to return void instead of an error code,
as the return value is never used. This removal helper is designed to be
never-failed, so there's no need for error handling.

Ack all outstanding page requests from the device with the response code of
IOMMU_PAGE_RESP_INVALID, indicating device should not attempt any retry.

Add comments to this helper explaining the steps involved in removing a
device from the iopf queue and disabling its PRI.

Suggested-by: Jason Gunthorpe <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Tested-by: Yan Zhao <[email protected]>
---
include/linux/iommu.h | 5 ++--
drivers/iommu/intel/iommu.c | 7 +----
drivers/iommu/io-pgfault.c | 59 ++++++++++++++++++++++++-------------
3 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 396d7b0d88b2..d9a99a978ffa 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -1542,7 +1542,7 @@ iommu_sva_domain_alloc(struct device *dev, struct mm_struct *mm)

#ifdef CONFIG_IOMMU_IOPF
int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev);
-int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev);
+void iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev);
int iopf_queue_flush_dev(struct device *dev);
struct iopf_queue *iopf_queue_alloc(const char *name);
void iopf_queue_free(struct iopf_queue *queue);
@@ -1558,10 +1558,9 @@ iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
return -ENODEV;
}

-static inline int
+static inline void
iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
{
- return -ENODEV;
}

static inline int iopf_queue_flush_dev(struct device *dev)
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 29a12f289e2e..a81a2be9b870 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4455,12 +4455,7 @@ static int intel_iommu_disable_iopf(struct device *dev)
*/
pci_disable_pri(to_pci_dev(dev));
info->pri_enabled = 0;
-
- /*
- * With PRI disabled and outstanding PRQs drained, removing device
- * from iopf queue should never fail.
- */
- WARN_ON(iopf_queue_remove_device(iommu->iopf_queue, dev));
+ iopf_queue_remove_device(iommu->iopf_queue, dev);

return 0;
}
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index ce7058892b59..26e100ca3221 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -448,50 +448,67 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
* @queue: IOPF queue
* @dev: device to remove
*
- * Caller makes sure that no more faults are reported for this device.
+ * 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.
+ * - Disable PRI on the device: After calling this helper, the caller could
+ * then disable PRI on the device.
+ * - Tear down the iopf infrastructure: Calling iopf_queue_remove_device()
+ * essentially disassociates the device. The fault_param might still exist,
+ * but iommu_page_response() will do nothing. The device fault parameter
+ * 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().
*/
-int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
+void iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
{
- int ret = 0;
struct iopf_fault *iopf, *next;
+ struct iommu_page_response resp;
struct dev_iommu *param = dev->iommu;
struct iommu_fault_param *fault_param;
+ const struct iommu_ops *ops = dev_iommu_ops(dev);

mutex_lock(&queue->lock);
mutex_lock(&param->lock);
fault_param = rcu_dereference_check(param->fault_param,
lockdep_is_held(&param->lock));
- if (!fault_param) {
- ret = -ENODEV;
- goto unlock;
- }
-
- if (fault_param->queue != queue) {
- ret = -EINVAL;
- goto unlock;
- }

- if (!list_empty(&fault_param->faults)) {
- ret = -EBUSY;
+ if (WARN_ON(!fault_param || fault_param->queue != queue))
goto unlock;
- }
-
- list_del(&fault_param->queue_list);

- /* Just in case some faults are still stuck */
+ mutex_lock(&fault_param->lock);
list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
kfree(iopf);

+ list_for_each_entry_safe(iopf, next, &fault_param->faults, list) {
+ memset(&resp, 0, sizeof(struct iommu_page_response));
+ resp.pasid = iopf->fault.prm.pasid;
+ resp.grpid = iopf->fault.prm.grpid;
+ resp.code = IOMMU_PAGE_RESP_INVALID;
+
+ if (iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID)
+ resp.flags = IOMMU_PAGE_RESP_PASID_VALID;
+
+ ops->page_response(dev, iopf, &resp);
+ list_del(&iopf->list);
+ kfree(iopf);
+ }
+ mutex_unlock(&fault_param->lock);
+
+ list_del(&fault_param->queue_list);
+
/* dec the ref owned by iopf_queue_add_device() */
rcu_assign_pointer(param->fault_param, NULL);
iopf_put_dev_fault_param(fault_param);
unlock:
mutex_unlock(&param->lock);
mutex_unlock(&queue->lock);
-
- return ret;
}
EXPORT_SYMBOL_GPL(iopf_queue_remove_device);

--
2.34.1


2024-01-22 14:22:37

by Lu Baolu

[permalink] [raw]
Subject: [PATCH v10 05/16] iommu: Merge iopf_device_param into iommu_fault_param

The struct dev_iommu contains two pointers, fault_param and iopf_param.
The fault_param pointer points to a data structure that is used to store
pending faults that are awaiting responses. The iopf_param pointer points
to a data structure that is used to store partial faults that are part of
a Page Request Group.

The fault_param and iopf_param pointers are essentially duplicate. This
causes memory waste. Merge the iopf_device_param pointer into the
iommu_fault_param pointer to consolidate the code and save memory. The
consolidated pointer would be allocated on demand when the device driver
enables the iopf on device, and would be freed after iopf is disabled.

Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
Tested-by: Yan Zhao <[email protected]>
Tested-by: Longfang Liu <[email protected]>
---
include/linux/iommu.h | 18 ++++--
drivers/iommu/io-pgfault.c | 110 ++++++++++++++++++-------------------
drivers/iommu/iommu.c | 34 ++----------
3 files changed, 72 insertions(+), 90 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 829bcb5a8e23..bbb7c2ad5184 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,7 @@ struct notifier_block;
struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;
+struct iopf_queue;

#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -672,21 +673,31 @@ struct iommu_fault_event {
* struct iommu_fault_param - per-device IOMMU fault data
* @handler: Callback function to handle IOMMU faults at device level
* @data: handler private data
- * @faults: holds the pending faults which needs response
* @lock: protect pending faults list
+ * @dev: the device that owns this param
+ * @queue: IOPF queue
+ * @queue_list: index into queue->devices
+ * @partial: faults that are part of a Page Request Group for which the last
+ * request hasn't been submitted yet.
+ * @faults: holds the pending faults which need response
*/
struct iommu_fault_param {
iommu_dev_fault_handler_t handler;
void *data;
+ struct mutex lock;
+
+ struct device *dev;
+ struct iopf_queue *queue;
+ struct list_head queue_list;
+
+ struct list_head partial;
struct list_head faults;
- struct mutex lock;
};

/**
* struct dev_iommu - Collection of per-device IOMMU data
*
* @fault_param: IOMMU detected device fault reporting data
- * @iopf_param: I/O Page Fault queue and data
* @fwspec: IOMMU fwspec data
* @iommu_dev: IOMMU device this device is linked to
* @priv: IOMMU Driver private data
@@ -702,7 +713,6 @@ struct iommu_fault_param {
struct dev_iommu {
struct mutex lock;
struct iommu_fault_param *fault_param;
- struct iopf_device_param *iopf_param;
struct iommu_fwspec *fwspec;
struct iommu_device *iommu_dev;
void *priv;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 24b5545352ae..f948303b2a91 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -25,21 +25,6 @@ struct iopf_queue {
struct mutex lock;
};

-/**
- * struct iopf_device_param - IO Page Fault data attached to a device
- * @dev: the device that owns this param
- * @queue: IOPF queue
- * @queue_list: index into queue->devices
- * @partial: faults that are part of a Page Request Group for which the last
- * request hasn't been submitted yet.
- */
-struct iopf_device_param {
- struct device *dev;
- struct iopf_queue *queue;
- struct list_head queue_list;
- struct list_head partial;
-};
-
struct iopf_fault {
struct iommu_fault fault;
struct list_head list;
@@ -144,7 +129,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
int ret;
struct iopf_group *group;
struct iopf_fault *iopf, *next;
- struct iopf_device_param *iopf_param;
+ struct iommu_fault_param *iopf_param;

struct device *dev = cookie;
struct dev_iommu *param = dev->iommu;
@@ -159,7 +144,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
* As long as we're holding param->lock, the queue can't be unlinked
* from the device and therefore cannot disappear.
*/
- iopf_param = param->iopf_param;
+ iopf_param = param->fault_param;
if (!iopf_param)
return -ENODEV;

@@ -229,14 +214,14 @@ EXPORT_SYMBOL_GPL(iommu_queue_iopf);
int iopf_queue_flush_dev(struct device *dev)
{
int ret = 0;
- struct iopf_device_param *iopf_param;
+ struct iommu_fault_param *iopf_param;
struct dev_iommu *param = dev->iommu;

if (!param)
return -ENODEV;

mutex_lock(&param->lock);
- iopf_param = param->iopf_param;
+ iopf_param = param->fault_param;
if (iopf_param)
flush_workqueue(iopf_param->queue->wq);
else
@@ -260,7 +245,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
int iopf_queue_discard_partial(struct iopf_queue *queue)
{
struct iopf_fault *iopf, *next;
- struct iopf_device_param *iopf_param;
+ struct iommu_fault_param *iopf_param;

if (!queue)
return -EINVAL;
@@ -287,34 +272,36 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
*/
int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
{
- int ret = -EBUSY;
- struct iopf_device_param *iopf_param;
+ int ret = 0;
struct dev_iommu *param = dev->iommu;
-
- if (!param)
- return -ENODEV;
-
- iopf_param = kzalloc(sizeof(*iopf_param), GFP_KERNEL);
- if (!iopf_param)
- return -ENOMEM;
-
- INIT_LIST_HEAD(&iopf_param->partial);
- iopf_param->queue = queue;
- iopf_param->dev = dev;
+ struct iommu_fault_param *fault_param;

mutex_lock(&queue->lock);
mutex_lock(&param->lock);
- if (!param->iopf_param) {
- list_add(&iopf_param->queue_list, &queue->devices);
- param->iopf_param = iopf_param;
- ret = 0;
+ if (param->fault_param) {
+ ret = -EBUSY;
+ goto done_unlock;
}
+
+ fault_param = kzalloc(sizeof(*fault_param), GFP_KERNEL);
+ if (!fault_param) {
+ ret = -ENOMEM;
+ goto done_unlock;
+ }
+
+ mutex_init(&fault_param->lock);
+ INIT_LIST_HEAD(&fault_param->faults);
+ INIT_LIST_HEAD(&fault_param->partial);
+ fault_param->dev = dev;
+ list_add(&fault_param->queue_list, &queue->devices);
+ fault_param->queue = queue;
+
+ param->fault_param = fault_param;
+
+done_unlock:
mutex_unlock(&param->lock);
mutex_unlock(&queue->lock);

- if (ret)
- kfree(iopf_param);
-
return ret;
}
EXPORT_SYMBOL_GPL(iopf_queue_add_device);
@@ -330,34 +317,41 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
*/
int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
{
- int ret = -EINVAL;
+ int ret = 0;
struct iopf_fault *iopf, *next;
- struct iopf_device_param *iopf_param;
struct dev_iommu *param = dev->iommu;
-
- if (!param || !queue)
- return -EINVAL;
+ struct iommu_fault_param *fault_param = param->fault_param;

mutex_lock(&queue->lock);
mutex_lock(&param->lock);
- iopf_param = param->iopf_param;
- if (iopf_param && iopf_param->queue == queue) {
- list_del(&iopf_param->queue_list);
- param->iopf_param = NULL;
- ret = 0;
+ if (!fault_param) {
+ ret = -ENODEV;
+ goto unlock;
}
- mutex_unlock(&param->lock);
- mutex_unlock(&queue->lock);
- if (ret)
- return ret;
+
+ if (fault_param->queue != queue) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ if (!list_empty(&fault_param->faults)) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ list_del(&fault_param->queue_list);

/* Just in case some faults are still stuck */
- list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
+ list_for_each_entry_safe(iopf, next, &fault_param->partial, list)
kfree(iopf);

- kfree(iopf_param);
+ param->fault_param = NULL;
+ kfree(fault_param);
+unlock:
+ mutex_unlock(&param->lock);
+ mutex_unlock(&queue->lock);

- return 0;
+ return ret;
}
EXPORT_SYMBOL_GPL(iopf_queue_remove_device);

@@ -403,7 +397,7 @@ EXPORT_SYMBOL_GPL(iopf_queue_alloc);
*/
void iopf_queue_free(struct iopf_queue *queue)
{
- struct iopf_device_param *iopf_param, *next;
+ struct iommu_fault_param *iopf_param, *next;

if (!queue)
return;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index b88dc3e0595c..e8f2bcea7f51 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1355,27 +1355,18 @@ int iommu_register_device_fault_handler(struct device *dev,
struct dev_iommu *param = dev->iommu;
int ret = 0;

- if (!param)
+ if (!param || !param->fault_param)
return -EINVAL;

mutex_lock(&param->lock);
/* Only allow one fault handler registered for each device */
- if (param->fault_param) {
+ if (param->fault_param->handler) {
ret = -EBUSY;
goto done_unlock;
}

- get_device(dev);
- param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
- if (!param->fault_param) {
- put_device(dev);
- ret = -ENOMEM;
- goto done_unlock;
- }
param->fault_param->handler = handler;
param->fault_param->data = data;
- mutex_init(&param->fault_param->lock);
- INIT_LIST_HEAD(&param->fault_param->faults);

done_unlock:
mutex_unlock(&param->lock);
@@ -1396,29 +1387,16 @@ EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
int iommu_unregister_device_fault_handler(struct device *dev)
{
struct dev_iommu *param = dev->iommu;
- int ret = 0;

- if (!param)
+ if (!param || !param->fault_param)
return -EINVAL;

mutex_lock(&param->lock);
-
- if (!param->fault_param)
- goto unlock;
-
- /* we cannot unregister handler if there are pending faults */
- if (!list_empty(&param->fault_param->faults)) {
- ret = -EBUSY;
- goto unlock;
- }
-
- kfree(param->fault_param);
- param->fault_param = NULL;
- put_device(dev);
-unlock:
+ param->fault_param->handler = NULL;
+ param->fault_param->data = NULL;
mutex_unlock(&param->lock);

- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);

--
2.34.1


2024-01-25 09:47:35

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

On Mon, Jan 22, 2024 at 01:42:53PM +0800, Lu Baolu wrote:
> The iommu fault data is currently defined in uapi/linux/iommu.h, but is
> only used inside the iommu subsystem. Move it to linux/iommu.h, where it
> will be more accessible to kernel drivers.
>
> With this done, uapi/linux/iommu.h becomes empty and can be removed from
> the tree.

The reason for removing this [1] is that it is only being used by
internal code in the kernel. What happens with usespace code that have
used these definitions? Should we deprecate instead of just removing?

Best

[1] https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Lu Baolu <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Kevin Tian <[email protected]>
> Reviewed-by: Yi Liu <[email protected]>
> Tested-by: Yan Zhao <[email protected]>
> Tested-by: Longfang Liu <[email protected]>
> ---
> include/linux/iommu.h | 152 +++++++++++++++++++++++++++++++++-
> include/uapi/linux/iommu.h | 161 -------------------------------------
> MAINTAINERS | 1 -
> 3 files changed, 151 insertions(+), 163 deletions(-)
> delete mode 100644 include/uapi/linux/iommu.h
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 1ea2a820e1eb..472a8ce029b1 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -14,7 +14,6 @@
> #include <linux/err.h>
> #include <linux/of.h>
> #include <linux/iova_bitmap.h>
> -#include <uapi/linux/iommu.h>
>
> #define IOMMU_READ (1 << 0)
> #define IOMMU_WRITE (1 << 1)
> @@ -44,6 +43,157 @@ struct iommu_sva;
> struct iommu_fault_event;
> struct iommu_dma_cookie;
>
> +#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
> +#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
> +#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
> +#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
> +
> +/* Generic fault types, can be expanded IRQ remapping fault */
> +enum iommu_fault_type {
> + IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
> + IOMMU_FAULT_PAGE_REQ, /* page request fault */
> +};
> +
> +enum iommu_fault_reason {
> + IOMMU_FAULT_REASON_UNKNOWN = 0,
> +
> + /* Could not access the PASID table (fetch caused external abort) */
> + IOMMU_FAULT_REASON_PASID_FETCH,
> +
> + /* PASID entry is invalid or has configuration errors */
> + IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
> +
> + /*
> + * PASID is out of range (e.g. exceeds the maximum PASID
> + * supported by the IOMMU) or disabled.
> + */
> + IOMMU_FAULT_REASON_PASID_INVALID,
> +
> + /*
> + * An external abort occurred fetching (or updating) a translation
> + * table descriptor
> + */
> + IOMMU_FAULT_REASON_WALK_EABT,
> +
> + /*
> + * Could not access the page table entry (Bad address),
> + * actual translation fault
> + */
> + IOMMU_FAULT_REASON_PTE_FETCH,
> +
> + /* Protection flag check failed */
> + IOMMU_FAULT_REASON_PERMISSION,
> +
> + /* access flag check failed */
> + IOMMU_FAULT_REASON_ACCESS,
> +
> + /* Output address of a translation stage caused Address Size fault */
> + IOMMU_FAULT_REASON_OOR_ADDRESS,
> +};
> +
> +/**
> + * struct iommu_fault_unrecoverable - Unrecoverable fault data
> + * @reason: reason of the fault, from &enum iommu_fault_reason
> + * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
> + * @pasid: Process Address Space ID
> + * @perm: requested permission access using by the incoming transaction
> + * (IOMMU_FAULT_PERM_* values)
> + * @addr: offending page address
> + * @fetch_addr: address that caused a fetch abort, if any
> + */
> +struct iommu_fault_unrecoverable {
> + __u32 reason;
> +#define IOMMU_FAULT_UNRECOV_PASID_VALID (1 << 0)
> +#define IOMMU_FAULT_UNRECOV_ADDR_VALID (1 << 1)
> +#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID (1 << 2)
> + __u32 flags;
> + __u32 pasid;
> + __u32 perm;
> + __u64 addr;
> + __u64 fetch_addr;
> +};
> +
> +/**
> + * struct iommu_fault_page_request - Page Request data
> + * @flags: encodes whether the corresponding fields are valid and whether this
> + * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
> + * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
> + * must have the same PASID value as the page request. When it is clear,
> + * the page response should not have a PASID.
> + * @pasid: Process Address Space ID
> + * @grpid: Page Request Group Index
> + * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
> + * @addr: page address
> + * @private_data: device-specific private information
> + */
> +struct iommu_fault_page_request {
> +#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
> +#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];
> +};
> +
> +/**
> + * struct iommu_fault - Generic fault data
> + * @type: fault type from &enum iommu_fault_type
> + * @padding: reserved for future use (should be zero)
> + * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
> + * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
> + * @padding2: sets the fault size to allow for future extensions
> + */
> +struct iommu_fault {
> + __u32 type;
> + __u32 padding;
> + union {
> + struct iommu_fault_unrecoverable event;
> + struct iommu_fault_page_request prm;
> + __u8 padding2[56];
> + };
> +};
> +
> +/**
> + * enum iommu_page_response_code - Return status of fault handlers
> + * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
> + * populated, retry the access. This is "Success" in PCI PRI.
> + * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
> + * this device if possible. This is "Response Failure" in PCI PRI.
> + * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
> + * access. This is "Invalid Request" in PCI PRI.
> + */
> +enum iommu_page_response_code {
> + IOMMU_PAGE_RESP_SUCCESS = 0,
> + IOMMU_PAGE_RESP_INVALID,
> + IOMMU_PAGE_RESP_FAILURE,
> +};
> +
> +/**
> + * 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
> + * @grpid: Page Request Group Index
> + * @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;
> +};
> +
> +
> /* iommu fault flags */
> #define IOMMU_FAULT_READ 0x0
> #define IOMMU_FAULT_WRITE 0x1
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> deleted file mode 100644
> index 65d8b0234f69..000000000000
> --- a/include/uapi/linux/iommu.h
> +++ /dev/null
> @@ -1,161 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> -/*
> - * IOMMU user API definitions
> - */
> -
> -#ifndef _UAPI_IOMMU_H
> -#define _UAPI_IOMMU_H
> -
> -#include <linux/types.h>
> -
> -#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
> -#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
> -#define IOMMU_FAULT_PERM_EXEC (1 << 2) /* exec */
> -#define IOMMU_FAULT_PERM_PRIV (1 << 3) /* privileged */
> -
> -/* Generic fault types, can be expanded IRQ remapping fault */
> -enum iommu_fault_type {
> - IOMMU_FAULT_DMA_UNRECOV = 1, /* unrecoverable fault */
> - IOMMU_FAULT_PAGE_REQ, /* page request fault */
> -};
> -
> -enum iommu_fault_reason {
> - IOMMU_FAULT_REASON_UNKNOWN = 0,
> -
> - /* Could not access the PASID table (fetch caused external abort) */
> - IOMMU_FAULT_REASON_PASID_FETCH,
> -
> - /* PASID entry is invalid or has configuration errors */
> - IOMMU_FAULT_REASON_BAD_PASID_ENTRY,
> -
> - /*
> - * PASID is out of range (e.g. exceeds the maximum PASID
> - * supported by the IOMMU) or disabled.
> - */
> - IOMMU_FAULT_REASON_PASID_INVALID,
> -
> - /*
> - * An external abort occurred fetching (or updating) a translation
> - * table descriptor
> - */
> - IOMMU_FAULT_REASON_WALK_EABT,
> -
> - /*
> - * Could not access the page table entry (Bad address),
> - * actual translation fault
> - */
> - IOMMU_FAULT_REASON_PTE_FETCH,
> -
> - /* Protection flag check failed */
> - IOMMU_FAULT_REASON_PERMISSION,
> -
> - /* access flag check failed */
> - IOMMU_FAULT_REASON_ACCESS,
> -
> - /* Output address of a translation stage caused Address Size fault */
> - IOMMU_FAULT_REASON_OOR_ADDRESS,
> -};
> -
> -/**
> - * struct iommu_fault_unrecoverable - Unrecoverable fault data
> - * @reason: reason of the fault, from &enum iommu_fault_reason
> - * @flags: parameters of this fault (IOMMU_FAULT_UNRECOV_* values)
> - * @pasid: Process Address Space ID
> - * @perm: requested permission access using by the incoming transaction
> - * (IOMMU_FAULT_PERM_* values)
> - * @addr: offending page address
> - * @fetch_addr: address that caused a fetch abort, if any
> - */
> -struct iommu_fault_unrecoverable {
> - __u32 reason;
> -#define IOMMU_FAULT_UNRECOV_PASID_VALID (1 << 0)
> -#define IOMMU_FAULT_UNRECOV_ADDR_VALID (1 << 1)
> -#define IOMMU_FAULT_UNRECOV_FETCH_ADDR_VALID (1 << 2)
> - __u32 flags;
> - __u32 pasid;
> - __u32 perm;
> - __u64 addr;
> - __u64 fetch_addr;
> -};
> -
> -/**
> - * struct iommu_fault_page_request - Page Request data
> - * @flags: encodes whether the corresponding fields are valid and whether this
> - * is the last page in group (IOMMU_FAULT_PAGE_REQUEST_* values).
> - * When IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID is set, the page response
> - * must have the same PASID value as the page request. When it is clear,
> - * the page response should not have a PASID.
> - * @pasid: Process Address Space ID
> - * @grpid: Page Request Group Index
> - * @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
> - * @addr: page address
> - * @private_data: device-specific private information
> - */
> -struct iommu_fault_page_request {
> -#define IOMMU_FAULT_PAGE_REQUEST_PASID_VALID (1 << 0)
> -#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];
> -};
> -
> -/**
> - * struct iommu_fault - Generic fault data
> - * @type: fault type from &enum iommu_fault_type
> - * @padding: reserved for future use (should be zero)
> - * @event: fault event, when @type is %IOMMU_FAULT_DMA_UNRECOV
> - * @prm: Page Request message, when @type is %IOMMU_FAULT_PAGE_REQ
> - * @padding2: sets the fault size to allow for future extensions
> - */
> -struct iommu_fault {
> - __u32 type;
> - __u32 padding;
> - union {
> - struct iommu_fault_unrecoverable event;
> - struct iommu_fault_page_request prm;
> - __u8 padding2[56];
> - };
> -};
> -
> -/**
> - * enum iommu_page_response_code - Return status of fault handlers
> - * @IOMMU_PAGE_RESP_SUCCESS: Fault has been handled and the page tables
> - * populated, retry the access. This is "Success" in PCI PRI.
> - * @IOMMU_PAGE_RESP_FAILURE: General error. Drop all subsequent faults from
> - * this device if possible. This is "Response Failure" in PCI PRI.
> - * @IOMMU_PAGE_RESP_INVALID: Could not handle this fault, don't retry the
> - * access. This is "Invalid Request" in PCI PRI.
> - */
> -enum iommu_page_response_code {
> - IOMMU_PAGE_RESP_SUCCESS = 0,
> - IOMMU_PAGE_RESP_INVALID,
> - IOMMU_PAGE_RESP_FAILURE,
> -};
> -
> -/**
> - * 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
> - * @grpid: Page Request Group Index
> - * @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;
> -};
> -
> -#endif /* _UAPI_IOMMU_H */
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8d1052fa6a69..97846088d34d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11239,7 +11239,6 @@ F: drivers/iommu/
> F: include/linux/iommu.h
> F: include/linux/iova.h
> F: include/linux/of_iommu.h
> -F: include/uapi/linux/iommu.h
>
> IOMMUFD
> M: Jason Gunthorpe <[email protected]>
> --
> 2.34.1
>

--

Joel Granados


Attachments:
(No filename) (13.02 kB)
signature.asc (673.00 B)
Download all attachments

2024-01-25 10:23:51

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions

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 <[email protected]>
> Reviewed-by: Jason Gunthorpe <[email protected]>
> Reviewed-by: Kevin Tian <[email protected]>
> Reviewed-by: Yi Liu <[email protected]>
> Tested-by: Yan Zhao <[email protected]>
> Tested-by: Longfang Liu <[email protected]>
> ---
> 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/[email protected]
[2] https://lore.kernel.org/all/[email protected]

> /* 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
>

--

Joel Granados


Attachments:
(No filename) (4.18 kB)
signature.asc (673.00 B)
Download all attachments

2024-01-25 13:44:20

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

On Thu, Jan 25, 2024 at 10:17:34AM +0100, Joel Granados wrote:
> On Mon, Jan 22, 2024 at 01:42:53PM +0800, Lu Baolu wrote:
> > The iommu fault data is currently defined in uapi/linux/iommu.h, but is
> > only used inside the iommu subsystem. Move it to linux/iommu.h, where it
> > will be more accessible to kernel drivers.
> >
> > With this done, uapi/linux/iommu.h becomes empty and can be removed from
> > the tree.
>
> The reason for removing this [1] is that it is only being used by
> internal code in the kernel. What happens with usespace code that have
> used these definitions? Should we deprecate instead of just removing?

There was never an in-tree kernel implementation. Any userspace that
implemented this needs to decide on its own if it will continue to
support the non-mainline kernel and provide a copy of the definitions
itself..

(it was a process mistake to merge a uapi header without a
corresponding uapi implementation, sorry)

Jason

2024-01-25 13:52:14

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions

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

2024-01-25 14:05:06

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 04/16] iommu: Cleanup iopf data structure definitions

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/[email protected]
> [2]https://lore.kernel.org/all/[email protected]

Best regards,
baolu

2024-01-25 16:27:47

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 15/16] iommu: Make iopf_group_response() return void

On Mon, Jan 22, 2024 at 01:43:07PM +0800, Lu Baolu wrote:
> The iopf_group_response() should return void, as nothing can do anything
> with the failure. This implies that ops->page_response() must also return
> void; this is consistent with what the drivers do. The failure paths,
> which are all integrity validations of the fault, should be WARN_ON'd,
> not return codes.
>
> If the iommu core fails to enqueue the fault, it should respond the fault
> directly by calling ops->page_response() instead of returning an error
> number and relying on the iommu drivers to do so. Consolidate the error
> fault handling code in the core.
>
> Co-developed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 14 +--
> drivers/iommu/intel/iommu.h | 4 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +++-----
> drivers/iommu/intel/svm.c | 18 +--
> drivers/iommu/io-pgfault.c | 132 +++++++++++---------
> 5 files changed, 99 insertions(+), 119 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 48196efc9327..d7b6f4017254 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -578,9 +578,8 @@ struct iommu_ops {
> int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
> int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
>
> - int (*page_response)(struct device *dev,
> - struct iopf_fault *evt,
> - struct iommu_page_response *msg);
> + void (*page_response)(struct device *dev, struct iopf_fault *evt,
> + struct iommu_page_response *msg);
>
> int (*def_domain_type)(struct device *dev);
> void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
> @@ -1551,8 +1550,8 @@ void iopf_queue_free(struct iopf_queue *queue);
> int iopf_queue_discard_partial(struct iopf_queue *queue);
> void iopf_free_group(struct iopf_group *group);
> int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
> -int iopf_group_response(struct iopf_group *group,
> - enum iommu_page_response_code status);
> +void iopf_group_response(struct iopf_group *group,
> + enum iommu_page_response_code status);
> #else
> static inline int
> iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
> @@ -1594,10 +1593,9 @@ iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> return -ENODEV;
> }
>
> -static inline int iopf_group_response(struct iopf_group *group,
> - enum iommu_page_response_code status)
> +static inline void iopf_group_response(struct iopf_group *group,
> + enum iommu_page_response_code status)
> {
> - return -ENODEV;
> }
> #endif /* CONFIG_IOMMU_IOPF */
> #endif /* __LINUX_IOMMU_H */
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index 696d95293a69..cf9a28c7fab8 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -1079,8 +1079,8 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
> void intel_svm_check(struct intel_iommu *iommu);
> int intel_svm_enable_prq(struct intel_iommu *iommu);
> int intel_svm_finish_prq(struct intel_iommu *iommu);
> -int intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
> - struct iommu_page_response *msg);
> +void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
> + struct iommu_page_response *msg);
> struct iommu_domain *intel_svm_domain_alloc(void);
> void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
> void intel_drain_pasid_prq(struct device *dev, u32 pasid);
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 4e93e845458c..42eb59cb99f4 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -920,31 +920,29 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
> return arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmds, cmds->num, true);
> }
>
> -static int arm_smmu_page_response(struct device *dev,
> - struct iopf_fault *unused,
> - struct iommu_page_response *resp)
> +static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
> + struct iommu_page_response *resp)
> {
> struct arm_smmu_cmdq_ent cmd = {0};
> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
> int sid = master->streams[0].id;
>
> - if (master->stall_enabled) {
> - cmd.opcode = CMDQ_OP_RESUME;
> - cmd.resume.sid = sid;
> - cmd.resume.stag = resp->grpid;
> - switch (resp->code) {
> - case IOMMU_PAGE_RESP_INVALID:
> - case IOMMU_PAGE_RESP_FAILURE:
> - cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
> - break;
> - case IOMMU_PAGE_RESP_SUCCESS:
> - cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
> - break;
> - default:
> - return -EINVAL;
> - }
> - } else {
> - return -ENODEV;
> + if (WARN_ON(!master->stall_enabled))
> + return;
> +
> + cmd.opcode = CMDQ_OP_RESUME;
> + cmd.resume.sid = sid;
> + cmd.resume.stag = resp->grpid;
> + switch (resp->code) {
> + case IOMMU_PAGE_RESP_INVALID:
> + case IOMMU_PAGE_RESP_FAILURE:
> + cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
> + break;
> + case IOMMU_PAGE_RESP_SUCCESS:
> + cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
> + break;
> + default:
> + break;
> }
>
> arm_smmu_cmdq_issue_cmd(master->smmu, &cmd);
> @@ -954,8 +952,6 @@ static int arm_smmu_page_response(struct device *dev,
> * terminated... at some point in the future. PRI_RESP is fire and
> * forget.
> */
> -
> - return 0;
> }
>
> /* Context descriptor manipulation functions */
> @@ -1516,16 +1512,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> }
>
> ret = iommu_report_device_fault(master->dev, &fault_evt);
> - if (ret && flt->type == IOMMU_FAULT_PAGE_REQ) {
> - /* Nobody cared, abort the access */
> - struct iommu_page_response resp = {
> - .pasid = flt->prm.pasid,
> - .grpid = flt->prm.grpid,
> - .code = IOMMU_PAGE_RESP_FAILURE,
> - };
> - arm_smmu_page_response(master->dev, &fault_evt, &resp);
> - }
> -
> out_unlock:
> mutex_unlock(&smmu->streams_mutex);
> return ret;
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index e1cbcb9515f0..2f8716636dbb 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -740,9 +740,8 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> return IRQ_RETVAL(handled);
> }
>
> -int intel_svm_page_response(struct device *dev,
> - struct iopf_fault *evt,
> - struct iommu_page_response *msg)
> +void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
> + struct iommu_page_response *msg)
> {
> struct device_domain_info *info = dev_iommu_priv_get(dev);
> struct intel_iommu *iommu = info->iommu;
> @@ -751,7 +750,6 @@ int intel_svm_page_response(struct device *dev,
> bool private_present;
> bool pasid_present;
> bool last_page;
> - int ret = 0;
> u16 sid;
>
> prm = &evt->fault.prm;
> @@ -760,16 +758,6 @@ int intel_svm_page_response(struct device *dev,
> private_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA;
> last_page = prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
>
> - if (!pasid_present) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> - if (prm->pasid == 0 || prm->pasid >= PASID_MAX) {
> - ret = -EINVAL;
> - goto out;
> - }
> -
> /*
> * Per VT-d spec. v3.0 ch7.7, system software must respond
> * with page group response if private data is present (PDP)
> @@ -798,8 +786,6 @@ int intel_svm_page_response(struct device *dev,
>
> qi_submit_sync(iommu, &desc, 1, 0);
> }
> -out:
> - return ret;
> }
>
> static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index c22e13df84c2..6e63e5a02884 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -39,7 +39,7 @@ static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param)
> kfree_rcu(fault_param, rcu);
> }
>
> -void iopf_free_group(struct iopf_group *group)
> +static void __iopf_free_group(struct iopf_group *group)
> {
> struct iopf_fault *iopf, *next;
>
> @@ -50,6 +50,11 @@ void iopf_free_group(struct iopf_group *group)
>
> /* Pair with iommu_report_device_fault(). */
> iopf_put_dev_fault_param(group->fault_param);
> +}
> +
> +void iopf_free_group(struct iopf_group *group)
> +{
> + __iopf_free_group(group);
> kfree(group);
> }
> EXPORT_SYMBOL_GPL(iopf_free_group);
> @@ -97,14 +102,49 @@ static int report_partial_fault(struct iommu_fault_param *fault_param,
> return 0;
> }
>
> +static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
> + struct iopf_fault *evt,
> + struct iopf_group *abort_group)
> +{
> + struct iopf_fault *iopf, *next;
> + struct iopf_group *group;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group) {
> + /*
> + * We always need to construct the group as we need it to abort
> + * the request at the driver if it cfan't be handled.
> + */
> + group = abort_group;
> + }
> +
> + group->fault_param = iopf_param;
> + group->last_fault.fault = evt->fault;
> + INIT_LIST_HEAD(&group->faults);
> + INIT_LIST_HEAD(&group->pending_node);
> + list_add(&group->last_fault.list, &group->faults);
> +
> + /* See if we have partial faults for this group */
> + mutex_lock(&iopf_param->lock);
> + list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
> + if (iopf->fault.prm.grpid == evt->fault.prm.grpid)
> + /* Insert *before* the last fault */
> + list_move(&iopf->list, &group->faults);
> + }
> + list_add(&group->pending_node, &iopf_param->faults);
> + mutex_unlock(&iopf_param->lock);
> +
> + return group;
> +}
> +
> /**
> * iommu_report_device_fault() - Report fault event to device driver
> * @dev: the device
> * @evt: fault event data
> *
> * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
> - * handler. When this function fails and the fault is recoverable, it is the
> - * caller's responsibility to complete the fault.
> + * handler. If this function fails then ops->page_response() was called to
> + * complete evt if required.
> *
> * This module doesn't handle PCI PASID Stop Marker; IOMMU drivers must discard
> * them before reporting faults. A PASID Stop Marker (LRW = 0b100) doesn't
> @@ -143,22 +183,18 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> {
> struct iommu_fault *fault = &evt->fault;
> struct iommu_fault_param *iopf_param;
> - struct iopf_fault *iopf, *next;
> - struct iommu_domain *domain;
> + struct iopf_group abort_group = {};
> struct iopf_group *group;
> int ret;
>
> - if (fault->type != IOMMU_FAULT_PAGE_REQ)
> - return -EOPNOTSUPP;
> -
> iopf_param = iopf_get_dev_fault_param(dev);
> - if (!iopf_param)
> + if (WARN_ON(!iopf_param))
> return -ENODEV;
>
> if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> ret = report_partial_fault(iopf_param, fault);
> iopf_put_dev_fault_param(iopf_param);
> -
> + /* A request that is not the last does not need to be ack'd */
> return ret;
> }
>
> @@ -170,56 +206,33 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> * will send a response to the hardware. We need to clean up before
> * leaving, otherwise partial faults will be stuck.
> */
> - domain = get_domain_for_iopf(dev, fault);
> - if (!domain) {
> + group = iopf_group_alloc(iopf_param, evt, &abort_group);
> + if (group == &abort_group) {
> + ret = -ENOMEM;
> + goto err_abort;
> + }
> +
> + group->domain = get_domain_for_iopf(dev, fault);
> + if (!group->domain) {
> ret = -EINVAL;
> - goto cleanup_partial;
> + goto err_abort;
> }
>
> - group = kzalloc(sizeof(*group), GFP_KERNEL);
> - if (!group) {
> - ret = -ENOMEM;
> - goto cleanup_partial;
> - }
> -
> - group->fault_param = iopf_param;
> - group->last_fault.fault = *fault;
> - INIT_LIST_HEAD(&group->faults);
> - INIT_LIST_HEAD(&group->pending_node);
> - group->domain = domain;
> - list_add(&group->last_fault.list, &group->faults);
> -
> - /* See if we have partial faults for this group */
> - mutex_lock(&iopf_param->lock);
> - list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
> - if (iopf->fault.prm.grpid == fault->prm.grpid)
> - /* Insert *before* the last fault */
> - list_move(&iopf->list, &group->faults);
> - }
> - list_add(&group->pending_node, &iopf_param->faults);
> - mutex_unlock(&iopf_param->lock);
> + /*
> + * On success iopf_handler must call iopf_group_response() and
> + * iopf_free_group()
> + */
> + ret = group->domain->iopf_handler(group);
> + if (ret)
> + goto err_abort;
> + return 0;
>
> - ret = domain->iopf_handler(group);
> - if (ret) {
> - mutex_lock(&iopf_param->lock);
> - list_del_init(&group->pending_node);
> - mutex_unlock(&iopf_param->lock);
> +err_abort:
> + iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> + if (group == &abort_group)
> + __iopf_free_group(group);
> + else
> iopf_free_group(group);
> - }
> -
> - return ret;
> -
> -cleanup_partial:
> - mutex_lock(&iopf_param->lock);
> - list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
> - if (iopf->fault.prm.grpid == fault->prm.grpid) {
> - list_del(&iopf->list);
> - kfree(iopf);
> - }
> - }
> - mutex_unlock(&iopf_param->lock);
> - iopf_put_dev_fault_param(iopf_param);
> -
> return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_report_device_fault);
> @@ -262,8 +275,8 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
> *
> * Return 0 on success and <0 on error.
> */
Should you adjust the docs as well?

> -int iopf_group_response(struct iopf_group *group,
> - enum iommu_page_response_code status)
> +void iopf_group_response(struct iopf_group *group,
> + enum iommu_page_response_code status)
> {
> struct iommu_fault_param *fault_param = group->fault_param;
> struct iopf_fault *iopf = &group->last_fault;
> @@ -274,7 +287,6 @@ int iopf_group_response(struct iopf_group *group,
> .grpid = iopf->fault.prm.grpid,
> .code = status,
> };
> - int ret = -EINVAL;
>
> if ((iopf->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID) &&
> (iopf->fault.prm.flags & IOMMU_FAULT_PAGE_RESPONSE_NEEDS_PASID))
> @@ -283,12 +295,10 @@ int iopf_group_response(struct iopf_group *group,
> /* Only send response if there is a fault report pending */
> mutex_lock(&fault_param->lock);
> if (!list_empty(&group->pending_node)) {
> - ret = ops->page_response(dev, &group->last_fault, &resp);
> + ops->page_response(dev, &group->last_fault, &resp);
> list_del_init(&group->pending_node);
> }
> mutex_unlock(&fault_param->lock);
> -
> - return ret;
> }
> EXPORT_SYMBOL_GPL(iopf_group_response);
>
> --
> 2.34.1
>

--

Joel Granados


Attachments:
(No filename) (15.19 kB)
signature.asc (673.00 B)
Download all attachments

2024-01-25 16:32:59

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 16/16] iommu: Make iommu_report_device_fault() reutrn void

On Mon, Jan 22, 2024 at 01:43:08PM +0800, Lu Baolu wrote:
> As the iommu_report_device_fault() has been converted to auto-respond a
> page fault if it fails to enqueue it, there's no need to return a code
> in any case. Make it return void.
>
> Suggested-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 5 ++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
> drivers/iommu/intel/svm.c | 19 +++++++----------
> drivers/iommu/io-pgfault.c | 23 +++++++--------------
> 4 files changed, 19 insertions(+), 32 deletions(-)
>
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index d7b6f4017254..1ccad10e8164 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -1549,7 +1549,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
> void iopf_queue_free(struct iopf_queue *queue);
> int iopf_queue_discard_partial(struct iopf_queue *queue);
> void iopf_free_group(struct iopf_group *group);
> -int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
> +void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
> void iopf_group_response(struct iopf_group *group,
> enum iommu_page_response_code status);
> #else
> @@ -1587,10 +1587,9 @@ static inline void iopf_free_group(struct iopf_group *group)
> {
> }
>
> -static inline int
> +static inline void
> iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> {
> - return -ENODEV;
> }
>
> static inline void iopf_group_response(struct iopf_group *group,
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 42eb59cb99f4..02580364acda 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -1455,7 +1455,7 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
> /* IRQ and event handlers */
> static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> {
> - int ret;
> + int ret = 0;
> u32 perm = 0;
> struct arm_smmu_master *master;
> bool ssid_valid = evt[0] & EVTQ_0_SSV;
> @@ -1511,7 +1511,7 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
> goto out_unlock;
> }
>
> - ret = iommu_report_device_fault(master->dev, &fault_evt);
> + iommu_report_device_fault(master->dev, &fault_evt);
> out_unlock:
> mutex_unlock(&smmu->streams_mutex);
> return ret;
> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
> index 2f8716636dbb..b644d57da841 100644
> --- a/drivers/iommu/intel/svm.c
> +++ b/drivers/iommu/intel/svm.c
> @@ -561,14 +561,11 @@ static int prq_to_iommu_prot(struct page_req_dsc *req)
> return prot;
> }
>
> -static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
> - struct page_req_dsc *desc)
> +static void intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
> + struct page_req_dsc *desc)
> {
> struct iopf_fault event = { };
>
> - if (!dev || !dev_is_pci(dev))
> - return -ENODEV;
> -
> /* Fill in event data for device specific processing */
> event.fault.type = IOMMU_FAULT_PAGE_REQ;
> event.fault.prm.addr = (u64)desc->addr << VTD_PAGE_SHIFT;
> @@ -601,7 +598,7 @@ static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
> event.fault.prm.private_data[0] = ktime_to_ns(ktime_get());
> }
>
> - return iommu_report_device_fault(dev, &event);
> + iommu_report_device_fault(dev, &event);
> }
>
> static void handle_bad_prq_event(struct intel_iommu *iommu,
> @@ -704,12 +701,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
> if (!pdev)
> goto bad_req;
>
> - if (intel_svm_prq_report(iommu, &pdev->dev, req))
> - handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
> - else
> - trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
> - req->priv_data[0], req->priv_data[1],
> - iommu->prq_seq_number++);
> + intel_svm_prq_report(iommu, &pdev->dev, req);
> + trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
> + req->priv_data[0], req->priv_data[1],
> + iommu->prq_seq_number++);
> pci_dev_put(pdev);
> prq_advance:
> head = (head + sizeof(*req)) & PRQ_RING_MASK;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 6e63e5a02884..b64229dab976 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -179,23 +179,21 @@ static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
> *
> * Return: 0 on success and <0 on error.
> */
Should you remove the documentation that describes the return also?

> -int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> +void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> {
> struct iommu_fault *fault = &evt->fault;
> struct iommu_fault_param *iopf_param;
> struct iopf_group abort_group = {};
> struct iopf_group *group;
> - int ret;
>
> iopf_param = iopf_get_dev_fault_param(dev);
> if (WARN_ON(!iopf_param))
> - return -ENODEV;
> + return;
>
> if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> - ret = report_partial_fault(iopf_param, fault);
> + report_partial_fault(iopf_param, fault);
> iopf_put_dev_fault_param(iopf_param);
> /* A request that is not the last does not need to be ack'd */
> - return ret;
> }
>
> /*
> @@ -207,25 +205,21 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> * leaving, otherwise partial faults will be stuck.
> */
> group = iopf_group_alloc(iopf_param, evt, &abort_group);
> - if (group == &abort_group) {
> - ret = -ENOMEM;
> + if (group == &abort_group)
> goto err_abort;
> - }
>
> group->domain = get_domain_for_iopf(dev, fault);
> - if (!group->domain) {
> - ret = -EINVAL;
> + if (!group->domain)
> goto err_abort;
> - }
>
> /*
> * On success iopf_handler must call iopf_group_response() and
> * iopf_free_group()
> */
> - ret = group->domain->iopf_handler(group);
> - if (ret)
> + if (group->domain->iopf_handler(group))
> goto err_abort;
> - return 0;
> +
> + return;
>
> err_abort:
> iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
> @@ -233,7 +227,6 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
> __iopf_free_group(group);
> else
> iopf_free_group(group);
> - return ret;
> }
> EXPORT_SYMBOL_GPL(iommu_report_device_fault);
>
> --
> 2.34.1
>

--

Joel Granados


Attachments:
(No filename) (6.66 kB)
signature.asc (673.00 B)
Download all attachments

2024-01-25 17:23:13

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

On 2024/1/25 17:17, Joel Granados wrote:
> On Mon, Jan 22, 2024 at 01:42:53PM +0800, Lu Baolu wrote:
>> The iommu fault data is currently defined in uapi/linux/iommu.h, but is
>> only used inside the iommu subsystem. Move it to linux/iommu.h, where it
>> will be more accessible to kernel drivers.
>>
>> With this done, uapi/linux/iommu.h becomes empty and can be removed from
>> the tree.
> The reason for removing this [1] is that it is only being used by
> internal code in the kernel. What happens with usespace code that have
> used these definitions? Should we deprecate instead of just removing?

The interfaces to deliver I/O page faults to user space have never been
implemented in the Linux kernel before. Therefore, from a uAPI point of
view, this definition is actually dead code.

Best regards,
baolu

2024-01-26 10:25:47

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 16/16] iommu: Make iommu_report_device_fault() reutrn void

On 2024/1/26 0:26, Joel Granados wrote:
> On Mon, Jan 22, 2024 at 01:43:08PM +0800, Lu Baolu wrote:
>> As the iommu_report_device_fault() has been converted to auto-respond a
>> page fault if it fails to enqueue it, there's no need to return a code
>> in any case. Make it return void.
>>
>> Suggested-by: Jason Gunthorpe<[email protected]>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> include/linux/iommu.h | 5 ++---
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
>> drivers/iommu/intel/svm.c | 19 +++++++----------
>> drivers/iommu/io-pgfault.c | 23 +++++++--------------
>> 4 files changed, 19 insertions(+), 32 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index d7b6f4017254..1ccad10e8164 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -1549,7 +1549,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
>> void iopf_queue_free(struct iopf_queue *queue);
>> int iopf_queue_discard_partial(struct iopf_queue *queue);
>> void iopf_free_group(struct iopf_group *group);
>> -int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
>> +void iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
>> void iopf_group_response(struct iopf_group *group,
>> enum iommu_page_response_code status);
>> #else
>> @@ -1587,10 +1587,9 @@ static inline void iopf_free_group(struct iopf_group *group)
>> {
>> }
>>
>> -static inline int
>> +static inline void
>> iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
>> {
>> - return -ENODEV;
>> }
>>
>> static inline void iopf_group_response(struct iopf_group *group,
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 42eb59cb99f4..02580364acda 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -1455,7 +1455,7 @@ arm_smmu_find_master(struct arm_smmu_device *smmu, u32 sid)
>> /* IRQ and event handlers */
>> static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>> {
>> - int ret;
>> + int ret = 0;
>> u32 perm = 0;
>> struct arm_smmu_master *master;
>> bool ssid_valid = evt[0] & EVTQ_0_SSV;
>> @@ -1511,7 +1511,7 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>> goto out_unlock;
>> }
>>
>> - ret = iommu_report_device_fault(master->dev, &fault_evt);
>> + iommu_report_device_fault(master->dev, &fault_evt);
>> out_unlock:
>> mutex_unlock(&smmu->streams_mutex);
>> return ret;
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index 2f8716636dbb..b644d57da841 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -561,14 +561,11 @@ static int prq_to_iommu_prot(struct page_req_dsc *req)
>> return prot;
>> }
>>
>> -static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
>> - struct page_req_dsc *desc)
>> +static void intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
>> + struct page_req_dsc *desc)
>> {
>> struct iopf_fault event = { };
>>
>> - if (!dev || !dev_is_pci(dev))
>> - return -ENODEV;
>> -
>> /* Fill in event data for device specific processing */
>> event.fault.type = IOMMU_FAULT_PAGE_REQ;
>> event.fault.prm.addr = (u64)desc->addr << VTD_PAGE_SHIFT;
>> @@ -601,7 +598,7 @@ static int intel_svm_prq_report(struct intel_iommu *iommu, struct device *dev,
>> event.fault.prm.private_data[0] = ktime_to_ns(ktime_get());
>> }
>>
>> - return iommu_report_device_fault(dev, &event);
>> + iommu_report_device_fault(dev, &event);
>> }
>>
>> static void handle_bad_prq_event(struct intel_iommu *iommu,
>> @@ -704,12 +701,10 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>> if (!pdev)
>> goto bad_req;
>>
>> - if (intel_svm_prq_report(iommu, &pdev->dev, req))
>> - handle_bad_prq_event(iommu, req, QI_RESP_INVALID);
>> - else
>> - trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
>> - req->priv_data[0], req->priv_data[1],
>> - iommu->prq_seq_number++);
>> + intel_svm_prq_report(iommu, &pdev->dev, req);
>> + trace_prq_report(iommu, &pdev->dev, req->qw_0, req->qw_1,
>> + req->priv_data[0], req->priv_data[1],
>> + iommu->prq_seq_number++);
>> pci_dev_put(pdev);
>> prq_advance:
>> head = (head + sizeof(*req)) & PRQ_RING_MASK;
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 6e63e5a02884..b64229dab976 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -179,23 +179,21 @@ static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
>> *
>> * Return: 0 on success and <0 on error.
>> */
> Should you remove the documentation that describes the return also?

Yes. Will do.

Best regards,
baolu

2024-01-26 11:16:16

by Lu Baolu

[permalink] [raw]
Subject: Re: [PATCH v10 15/16] iommu: Make iopf_group_response() return void

On 2024/1/26 0:27, Joel Granados wrote:
> On Mon, Jan 22, 2024 at 01:43:07PM +0800, Lu Baolu wrote:
>> The iopf_group_response() should return void, as nothing can do anything
>> with the failure. This implies that ops->page_response() must also return
>> void; this is consistent with what the drivers do. The failure paths,
>> which are all integrity validations of the fault, should be WARN_ON'd,
>> not return codes.
>>
>> If the iommu core fails to enqueue the fault, it should respond the fault
>> directly by calling ops->page_response() instead of returning an error
>> number and relying on the iommu drivers to do so. Consolidate the error
>> fault handling code in the core.
>>
>> Co-developed-by: Jason Gunthorpe<[email protected]>
>> Signed-off-by: Jason Gunthorpe<[email protected]>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> include/linux/iommu.h | 14 +--
>> drivers/iommu/intel/iommu.h | 4 +-
>> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +++-----
>> drivers/iommu/intel/svm.c | 18 +--
>> drivers/iommu/io-pgfault.c | 132 +++++++++++---------
>> 5 files changed, 99 insertions(+), 119 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 48196efc9327..d7b6f4017254 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -578,9 +578,8 @@ struct iommu_ops {
>> int (*dev_enable_feat)(struct device *dev, enum iommu_dev_features f);
>> int (*dev_disable_feat)(struct device *dev, enum iommu_dev_features f);
>>
>> - int (*page_response)(struct device *dev,
>> - struct iopf_fault *evt,
>> - struct iommu_page_response *msg);
>> + void (*page_response)(struct device *dev, struct iopf_fault *evt,
>> + struct iommu_page_response *msg);
>>
>> int (*def_domain_type)(struct device *dev);
>> void (*remove_dev_pasid)(struct device *dev, ioasid_t pasid);
>> @@ -1551,8 +1550,8 @@ void iopf_queue_free(struct iopf_queue *queue);
>> int iopf_queue_discard_partial(struct iopf_queue *queue);
>> void iopf_free_group(struct iopf_group *group);
>> int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt);
>> -int iopf_group_response(struct iopf_group *group,
>> - enum iommu_page_response_code status);
>> +void iopf_group_response(struct iopf_group *group,
>> + enum iommu_page_response_code status);
>> #else
>> static inline int
>> iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
>> @@ -1594,10 +1593,9 @@ iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
>> return -ENODEV;
>> }
>>
>> -static inline int iopf_group_response(struct iopf_group *group,
>> - enum iommu_page_response_code status)
>> +static inline void iopf_group_response(struct iopf_group *group,
>> + enum iommu_page_response_code status)
>> {
>> - return -ENODEV;
>> }
>> #endif /* CONFIG_IOMMU_IOPF */
>> #endif /* __LINUX_IOMMU_H */
>> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
>> index 696d95293a69..cf9a28c7fab8 100644
>> --- a/drivers/iommu/intel/iommu.h
>> +++ b/drivers/iommu/intel/iommu.h
>> @@ -1079,8 +1079,8 @@ struct iommu_domain *intel_nested_domain_alloc(struct iommu_domain *parent,
>> void intel_svm_check(struct intel_iommu *iommu);
>> int intel_svm_enable_prq(struct intel_iommu *iommu);
>> int intel_svm_finish_prq(struct intel_iommu *iommu);
>> -int intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
>> - struct iommu_page_response *msg);
>> +void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
>> + struct iommu_page_response *msg);
>> struct iommu_domain *intel_svm_domain_alloc(void);
>> void intel_svm_remove_dev_pasid(struct device *dev, ioasid_t pasid);
>> void intel_drain_pasid_prq(struct device *dev, u32 pasid);
>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> index 4e93e845458c..42eb59cb99f4 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -920,31 +920,29 @@ static int arm_smmu_cmdq_batch_submit(struct arm_smmu_device *smmu,
>> return arm_smmu_cmdq_issue_cmdlist(smmu, cmds->cmds, cmds->num, true);
>> }
>>
>> -static int arm_smmu_page_response(struct device *dev,
>> - struct iopf_fault *unused,
>> - struct iommu_page_response *resp)
>> +static void arm_smmu_page_response(struct device *dev, struct iopf_fault *unused,
>> + struct iommu_page_response *resp)
>> {
>> struct arm_smmu_cmdq_ent cmd = {0};
>> struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>> int sid = master->streams[0].id;
>>
>> - if (master->stall_enabled) {
>> - cmd.opcode = CMDQ_OP_RESUME;
>> - cmd.resume.sid = sid;
>> - cmd.resume.stag = resp->grpid;
>> - switch (resp->code) {
>> - case IOMMU_PAGE_RESP_INVALID:
>> - case IOMMU_PAGE_RESP_FAILURE:
>> - cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
>> - break;
>> - case IOMMU_PAGE_RESP_SUCCESS:
>> - cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
>> - break;
>> - default:
>> - return -EINVAL;
>> - }
>> - } else {
>> - return -ENODEV;
>> + if (WARN_ON(!master->stall_enabled))
>> + return;
>> +
>> + cmd.opcode = CMDQ_OP_RESUME;
>> + cmd.resume.sid = sid;
>> + cmd.resume.stag = resp->grpid;
>> + switch (resp->code) {
>> + case IOMMU_PAGE_RESP_INVALID:
>> + case IOMMU_PAGE_RESP_FAILURE:
>> + cmd.resume.resp = CMDQ_RESUME_0_RESP_ABORT;
>> + break;
>> + case IOMMU_PAGE_RESP_SUCCESS:
>> + cmd.resume.resp = CMDQ_RESUME_0_RESP_RETRY;
>> + break;
>> + default:
>> + break;
>> }
>>
>> arm_smmu_cmdq_issue_cmd(master->smmu, &cmd);
>> @@ -954,8 +952,6 @@ static int arm_smmu_page_response(struct device *dev,
>> * terminated... at some point in the future. PRI_RESP is fire and
>> * forget.
>> */
>> -
>> - return 0;
>> }
>>
>> /* Context descriptor manipulation functions */
>> @@ -1516,16 +1512,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
>> }
>>
>> ret = iommu_report_device_fault(master->dev, &fault_evt);
>> - if (ret && flt->type == IOMMU_FAULT_PAGE_REQ) {
>> - /* Nobody cared, abort the access */
>> - struct iommu_page_response resp = {
>> - .pasid = flt->prm.pasid,
>> - .grpid = flt->prm.grpid,
>> - .code = IOMMU_PAGE_RESP_FAILURE,
>> - };
>> - arm_smmu_page_response(master->dev, &fault_evt, &resp);
>> - }
>> -
>> out_unlock:
>> mutex_unlock(&smmu->streams_mutex);
>> return ret;
>> diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
>> index e1cbcb9515f0..2f8716636dbb 100644
>> --- a/drivers/iommu/intel/svm.c
>> +++ b/drivers/iommu/intel/svm.c
>> @@ -740,9 +740,8 @@ static irqreturn_t prq_event_thread(int irq, void *d)
>> return IRQ_RETVAL(handled);
>> }
>>
>> -int intel_svm_page_response(struct device *dev,
>> - struct iopf_fault *evt,
>> - struct iommu_page_response *msg)
>> +void intel_svm_page_response(struct device *dev, struct iopf_fault *evt,
>> + struct iommu_page_response *msg)
>> {
>> struct device_domain_info *info = dev_iommu_priv_get(dev);
>> struct intel_iommu *iommu = info->iommu;
>> @@ -751,7 +750,6 @@ int intel_svm_page_response(struct device *dev,
>> bool private_present;
>> bool pasid_present;
>> bool last_page;
>> - int ret = 0;
>> u16 sid;
>>
>> prm = &evt->fault.prm;
>> @@ -760,16 +758,6 @@ int intel_svm_page_response(struct device *dev,
>> private_present = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PRIV_DATA;
>> last_page = prm->flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE;
>>
>> - if (!pasid_present) {
>> - ret = -EINVAL;
>> - goto out;
>> - }
>> -
>> - if (prm->pasid == 0 || prm->pasid >= PASID_MAX) {
>> - ret = -EINVAL;
>> - goto out;
>> - }
>> -
>> /*
>> * Per VT-d spec. v3.0 ch7.7, system software must respond
>> * with page group response if private data is present (PDP)
>> @@ -798,8 +786,6 @@ int intel_svm_page_response(struct device *dev,
>>
>> qi_submit_sync(iommu, &desc, 1, 0);
>> }
>> -out:
>> - return ret;
>> }
>>
>> static int intel_svm_set_dev_pasid(struct iommu_domain *domain,
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index c22e13df84c2..6e63e5a02884 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -39,7 +39,7 @@ static void iopf_put_dev_fault_param(struct iommu_fault_param *fault_param)
>> kfree_rcu(fault_param, rcu);
>> }
>>
>> -void iopf_free_group(struct iopf_group *group)
>> +static void __iopf_free_group(struct iopf_group *group)
>> {
>> struct iopf_fault *iopf, *next;
>>
>> @@ -50,6 +50,11 @@ void iopf_free_group(struct iopf_group *group)
>>
>> /* Pair with iommu_report_device_fault(). */
>> iopf_put_dev_fault_param(group->fault_param);
>> +}
>> +
>> +void iopf_free_group(struct iopf_group *group)
>> +{
>> + __iopf_free_group(group);
>> kfree(group);
>> }
>> EXPORT_SYMBOL_GPL(iopf_free_group);
>> @@ -97,14 +102,49 @@ static int report_partial_fault(struct iommu_fault_param *fault_param,
>> return 0;
>> }
>>
>> +static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
>> + struct iopf_fault *evt,
>> + struct iopf_group *abort_group)
>> +{
>> + struct iopf_fault *iopf, *next;
>> + struct iopf_group *group;
>> +
>> + group = kzalloc(sizeof(*group), GFP_KERNEL);
>> + if (!group) {
>> + /*
>> + * We always need to construct the group as we need it to abort
>> + * the request at the driver if it cfan't be handled.
>> + */
>> + group = abort_group;
>> + }
>> +
>> + group->fault_param = iopf_param;
>> + group->last_fault.fault = evt->fault;
>> + INIT_LIST_HEAD(&group->faults);
>> + INIT_LIST_HEAD(&group->pending_node);
>> + list_add(&group->last_fault.list, &group->faults);
>> +
>> + /* See if we have partial faults for this group */
>> + mutex_lock(&iopf_param->lock);
>> + list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
>> + if (iopf->fault.prm.grpid == evt->fault.prm.grpid)
>> + /* Insert*before* the last fault */
>> + list_move(&iopf->list, &group->faults);
>> + }
>> + list_add(&group->pending_node, &iopf_param->faults);
>> + mutex_unlock(&iopf_param->lock);
>> +
>> + return group;
>> +}
>> +
>> /**
>> * iommu_report_device_fault() - Report fault event to device driver
>> * @dev: the device
>> * @evt: fault event data
>> *
>> * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
>> - * handler. When this function fails and the fault is recoverable, it is the
>> - * caller's responsibility to complete the fault.
>> + * handler. If this function fails then ops->page_response() was called to
>> + * complete evt if required.
>> *
>> * This module doesn't handle PCI PASID Stop Marker; IOMMU drivers must discard
>> * them before reporting faults. A PASID Stop Marker (LRW = 0b100) doesn't
>> @@ -143,22 +183,18 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
>> {
>> struct iommu_fault *fault = &evt->fault;
>> struct iommu_fault_param *iopf_param;
>> - struct iopf_fault *iopf, *next;
>> - struct iommu_domain *domain;
>> + struct iopf_group abort_group = {};
>> struct iopf_group *group;
>> int ret;
>>
>> - if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> - return -EOPNOTSUPP;
>> -
>> iopf_param = iopf_get_dev_fault_param(dev);
>> - if (!iopf_param)
>> + if (WARN_ON(!iopf_param))
>> return -ENODEV;
>>
>> if (!(fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
>> ret = report_partial_fault(iopf_param, fault);
>> iopf_put_dev_fault_param(iopf_param);
>> -
>> + /* A request that is not the last does not need to be ack'd */
>> return ret;
>> }
>>
>> @@ -170,56 +206,33 @@ int iommu_report_device_fault(struct device *dev, struct iopf_fault *evt)
>> * will send a response to the hardware. We need to clean up before
>> * leaving, otherwise partial faults will be stuck.
>> */
>> - domain = get_domain_for_iopf(dev, fault);
>> - if (!domain) {
>> + group = iopf_group_alloc(iopf_param, evt, &abort_group);
>> + if (group == &abort_group) {
>> + ret = -ENOMEM;
>> + goto err_abort;
>> + }
>> +
>> + group->domain = get_domain_for_iopf(dev, fault);
>> + if (!group->domain) {
>> ret = -EINVAL;
>> - goto cleanup_partial;
>> + goto err_abort;
>> }
>>
>> - group = kzalloc(sizeof(*group), GFP_KERNEL);
>> - if (!group) {
>> - ret = -ENOMEM;
>> - goto cleanup_partial;
>> - }
>> -
>> - group->fault_param = iopf_param;
>> - group->last_fault.fault = *fault;
>> - INIT_LIST_HEAD(&group->faults);
>> - INIT_LIST_HEAD(&group->pending_node);
>> - group->domain = domain;
>> - list_add(&group->last_fault.list, &group->faults);
>> -
>> - /* See if we have partial faults for this group */
>> - mutex_lock(&iopf_param->lock);
>> - list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
>> - if (iopf->fault.prm.grpid == fault->prm.grpid)
>> - /* Insert*before* the last fault */
>> - list_move(&iopf->list, &group->faults);
>> - }
>> - list_add(&group->pending_node, &iopf_param->faults);
>> - mutex_unlock(&iopf_param->lock);
>> + /*
>> + * On success iopf_handler must call iopf_group_response() and
>> + * iopf_free_group()
>> + */
>> + ret = group->domain->iopf_handler(group);
>> + if (ret)
>> + goto err_abort;
>> + return 0;
>>
>> - ret = domain->iopf_handler(group);
>> - if (ret) {
>> - mutex_lock(&iopf_param->lock);
>> - list_del_init(&group->pending_node);
>> - mutex_unlock(&iopf_param->lock);
>> +err_abort:
>> + iopf_group_response(group, IOMMU_PAGE_RESP_FAILURE);
>> + if (group == &abort_group)
>> + __iopf_free_group(group);
>> + else
>> iopf_free_group(group);
>> - }
>> -
>> - return ret;
>> -
>> -cleanup_partial:
>> - mutex_lock(&iopf_param->lock);
>> - list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
>> - if (iopf->fault.prm.grpid == fault->prm.grpid) {
>> - list_del(&iopf->list);
>> - kfree(iopf);
>> - }
>> - }
>> - mutex_unlock(&iopf_param->lock);
>> - iopf_put_dev_fault_param(iopf_param);
>> -
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(iommu_report_device_fault);
>> @@ -262,8 +275,8 @@ EXPORT_SYMBOL_GPL(iopf_queue_flush_dev);
>> *
>> * Return 0 on success and <0 on error.
>> */
> Should you adjust the docs as well?

Yes. Will do.

Best regards,
baolu

2024-01-26 14:27:58

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

On Thu, Jan 25, 2024 at 09:43:37AM -0400, Jason Gunthorpe wrote:
> On Thu, Jan 25, 2024 at 10:17:34AM +0100, Joel Granados wrote:
> > On Mon, Jan 22, 2024 at 01:42:53PM +0800, Lu Baolu wrote:
> > > The iommu fault data is currently defined in uapi/linux/iommu.h, but is
> > > only used inside the iommu subsystem. Move it to linux/iommu.h, where it
> > > will be more accessible to kernel drivers.
> > >
> > > With this done, uapi/linux/iommu.h becomes empty and can be removed from
> > > the tree.
> >
> > The reason for removing this [1] is that it is only being used by
> > internal code in the kernel. What happens with usespace code that have
> > used these definitions? Should we deprecate instead of just removing?
>
> There was never an in-tree kernel implementation. Any userspace that
> implemented this needs to decide on its own if it will continue to
> support the non-mainline kernel and provide a copy of the definitions
> itself..
>
> (it was a process mistake to merge a uapi header without a
> corresponding uapi implementation, sorry)
Thx.
This makes sense and its actually a good thing that we remove it.

Best
>
> Jason

--

Joel Granados


Attachments:
(No filename) (1.17 kB)
signature.asc (673.00 B)
Download all attachments

2024-01-26 14:44:51

by Joel Granados

[permalink] [raw]
Subject: Re: [PATCH v10 01/16] iommu: Move iommu fault data to linux/iommu.h

On Thu, Jan 25, 2024 at 07:21:53PM +0800, Baolu Lu wrote:
> On 2024/1/25 17:17, Joel Granados wrote:
> > On Mon, Jan 22, 2024 at 01:42:53PM +0800, Lu Baolu wrote:
> >> The iommu fault data is currently defined in uapi/linux/iommu.h, but is
> >> only used inside the iommu subsystem. Move it to linux/iommu.h, where it
> >> will be more accessible to kernel drivers.
> >>
> >> With this done, uapi/linux/iommu.h becomes empty and can be removed from
> >> the tree.
> > The reason for removing this [1] is that it is only being used by
> > internal code in the kernel. What happens with usespace code that have
> > used these definitions? Should we deprecate instead of just removing?
>
> The interfaces to deliver I/O page faults to user space have never been
> implemented in the Linux kernel before. Therefore, from a uAPI point of
> view, this definition is actually dead code.
thx for the explanation.
I was thinking it was something like that. Just wanted to make sure.

Best

--

Joel Granados


Attachments:
(No filename) (1.00 kB)
signature.asc (673.00 B)
Download all attachments

2024-02-02 15:37:01

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v10 15/16] iommu: Make iopf_group_response() return void

On Mon, Jan 22, 2024 at 01:43:07PM +0800, Lu Baolu wrote:
> The iopf_group_response() should return void, as nothing can do anything
> with the failure. This implies that ops->page_response() must also return
> void; this is consistent with what the drivers do. The failure paths,
> which are all integrity validations of the fault, should be WARN_ON'd,
> not return codes.
>
> If the iommu core fails to enqueue the fault, it should respond the fault
> directly by calling ops->page_response() instead of returning an error
> number and relying on the iommu drivers to do so. Consolidate the error
> fault handling code in the core.
>
> Co-developed-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 14 +--
> drivers/iommu/intel/iommu.h | 4 +-
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +++-----
> drivers/iommu/intel/svm.c | 18 +--
> drivers/iommu/io-pgfault.c | 132 +++++++++++---------
> 5 files changed, 99 insertions(+), 119 deletions(-)

Reviewed-by: Jason Gunthorpe <[email protected]>

>
> +static struct iopf_group *iopf_group_alloc(struct iommu_fault_param *iopf_param,
> + struct iopf_fault *evt,
> + struct iopf_group *abort_group)
> +{
> + struct iopf_fault *iopf, *next;
> + struct iopf_group *group;
> +
> + group = kzalloc(sizeof(*group), GFP_KERNEL);
> + if (!group) {
> + /*
> + * We always need to construct the group as we need it to abort
> + * the request at the driver if it cfan't be handled.
^^^^^^ can't


Jason

2024-02-02 15:39:49

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v10 16/16] iommu: Make iommu_report_device_fault() reutrn void

On Mon, Jan 22, 2024 at 01:43:08PM +0800, Lu Baolu wrote:
> As the iommu_report_device_fault() has been converted to auto-respond a
> page fault if it fails to enqueue it, there's no need to return a code
> in any case. Make it return void.
>
> Suggested-by: Jason Gunthorpe <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 5 ++---
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
> drivers/iommu/intel/svm.c | 19 +++++++----------
> drivers/iommu/io-pgfault.c | 23 +++++++--------------
> 4 files changed, 19 insertions(+), 32 deletions(-)

Reviewed-by: Jason Gunthorpe <[email protected]>

Typo in subject 'reutrn' -> 'return'

Jason