Hi Shenming,
On 3/9/21 2:22 PM, Shenming Lu wrote:
This patch follows the discussion here:
https://lore.kernel.org/linux-acpi/YAaxjmJW+ZMvrhac@myrica/
In order to support more scenarios of using IOPF (mainly consider
the nested extension), besides keeping IOMMU_DEV_FEAT_IOPF as a
general capability for whether delivering faults through the IOMMU,
we extend iommu_register_fault_handler() with flags and introduce
IOPF_REPORT_FLAT and IOPF_REPORT_NESTED to describe the page fault
reporting capability under a specific configuration.
IOPF_REPORT_NESTED needs additional info to indicate which level/stage
is concerned since the fault client may be interested in only one
level.
Signed-off-by: Shenming Lu <lushenming@xxxxxxxxxx>
---
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 3 +-
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 11 ++--
drivers/iommu/io-pgfault.c | 4 --
drivers/iommu/iommu.c | 56 ++++++++++++++++++-
include/linux/iommu.h | 21 ++++++-
include/uapi/linux/iommu.h | 3 +
6 files changed, 85 insertions(+), 13 deletions(-)
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 ee66d1f4cb81..5de9432349d4 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
@@ -482,7 +482,8 @@ static int arm_smmu_master_sva_enable_iopf(struct arm_smmu_master *master)
if (ret)
return ret;
- ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf, dev);
+ ret = iommu_register_device_fault_handler(dev, iommu_queue_iopf,
+ IOPF_REPORT_FLAT, dev);
if (ret) {
iopf_queue_remove_device(master->smmu->evtq.iopf, dev);
return ret;
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 363744df8d51..f40529d0075d 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1447,10 +1447,6 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
return -EOPNOTSUPP;
}
- /* Stage-2 is always pinned at the moment */
- if (evt[1] & EVTQ_1_S2)
- return -EFAULT;
-
if (evt[1] & EVTQ_1_RnW)
perm |= IOMMU_FAULT_PERM_READ;
else
@@ -1468,13 +1464,18 @@ static int arm_smmu_handle_evt(struct arm_smmu_device *smmu, u64 *evt)
.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]);
}
+
+ if (evt[1] & EVTQ_1_S2) {
+ flt->prm.flags |= IOMMU_FAULT_PAGE_REQUEST_L2;
+ flt->prm.addr = FIELD_GET(EVTQ_3_IPA, evt[3]);
+ } else
+ flt->prm.addr = FIELD_GET(EVTQ_2_ADDR, evt[2]);
} else {
flt->type = IOMMU_FAULT_DMA_UNRECOV;
flt->event = (struct iommu_fault_unrecoverable) {
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1df8c1dcae77..abf16e06bcf5 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -195,10 +195,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
lockdep_assert_held(¶m->lock);
- if (fault->type != IOMMU_FAULT_PAGE_REQ)
- /* Not a recoverable page fault */
- return -EOPNOTSUPP;
-
Any reasons why do you want to remove this check?
/*
* As long as we're holding param->lock, the queue can't be unlinked
* from the device and therefore cannot disappear.
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d0b0a15dba84..cb1d93b00f7d 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1056,6 +1056,40 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
}
EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
+/*
+ * iommu_update_device_fault_handler - Update the device fault handler via flags
+ * @dev: the device
+ * @mask: bits(not set) to clear
+ * @set: bits to set
+ *
+ * Update the device fault handler installed by
+ * iommu_register_device_fault_handler().
+ *
+ * Return 0 on success, or an error.
+ */
+int iommu_update_device_fault_handler(struct device *dev, u32 mask, u32 set)
+{
+ struct dev_iommu *param = dev->iommu;
+ int ret = 0;
+
+ if (!param)
+ return -EINVAL;
+
+ mutex_lock(¶m->lock);
+
+ if (param->fault_param) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ param->fault_param->flags = (param->fault_param->flags & mask) | set;
+
+out_unlock:
+ mutex_unlock(¶m->lock);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_update_device_fault_handler);
When and why will this API be used? Why not registering the fault
handling capabilities of a device driver only once during probe()?
+
/**
* iommu_register_device_fault_handler() - Register a device fault handler
* @dev: the device
@@ -1076,11 +1110,14 @@ EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
*/
int iommu_register_device_fault_handler(struct device *dev,
iommu_dev_fault_handler_t handler,
- void *data)
+ u32 flags, void *data)
{
struct dev_iommu *param = dev->iommu;
int ret = 0;
+ if (flags & IOPF_REPORT_FLAT && flags & IOPF_REPORT_NESTED)
+ return -EINVAL;
+
if (!param)
return -EINVAL;
@@ -1099,6 +1136,7 @@ int iommu_register_device_fault_handler(struct device *dev,
goto done_unlock;
}
param->fault_param->handler = handler;
+ param->fault_param->flags = flags;
param->fault_param->data = data;
mutex_init(¶m->fault_param->lock);
INIT_LIST_HEAD(¶m->fault_param->faults);
@@ -1177,6 +1215,22 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
goto done_unlock;
}
+ /* The unrecoverable fault reporting is not supported at the moment. */
+ if (evt->fault.type != IOMMU_FAULT_PAGE_REQ)
+ return -EOPNOTSUPP;
Any reasons why do you want to disable reporting an unrecoverable fault?
+
+ if (evt->fault.type == IOMMU_FAULT_PAGE_REQ) {
+ if (fparam->flags & IOPF_REPORT_NESTED) {
+ if (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_L2 &&
+ !(fparam->flags & IOPF_REPORT_NESTED_L2_CONCERNED))
+ return -EOPNOTSUPP;
+ if (!(evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_L2) &&
+ !(fparam->flags & IOPF_REPORT_NESTED_L1_CONCERNED))
+ return -EOPNOTSUPP;
+ } else if (!(fparam->flags & IOPF_REPORT_FLAT))
+ return -EOPNOTSUPP;
+ }
+
if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
(evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
evt_pending = kmemdup(evt, sizeof(struct iommu_fault_event),
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 86d688c4418f..f03d761e8310 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -352,12 +352,21 @@ struct iommu_fault_event {
/**
* struct iommu_fault_param - per-device IOMMU fault data
* @handler: Callback function to handle IOMMU faults at device level
+ * @flags: Indicates whether the fault reporting is available under a
+ * specific configuration (1st/2nd-level-only(FLAT), or nested).
+ * IOPF_REPORT_NESTED needs to additionally know which level/stage
+ * is concerned.
If IOPF_REPORT_NESTED only is not valid why do you want to define it?
* @data: handler private data
* @faults: holds the pending faults which needs response
* @lock: protect pending faults list
*/
struct iommu_fault_param {
iommu_dev_fault_handler_t handler;
+#define IOPF_REPORT_FLAT (1 << 0)
+#define IOPF_REPORT_NESTED (1 << 1)
+#define IOPF_REPORT_NESTED_L1_CONCERNED (1 << 2)
+#define IOPF_REPORT_NESTED_L2_CONCERNED (1 << 3)
+ u32 flags;
void *data;
struct list_head faults;
struct mutex lock;
@@ -509,9 +518,11 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
struct notifier_block *nb);
extern int iommu_group_unregister_notifier(struct iommu_group *group,
struct notifier_block *nb);
+extern int iommu_update_device_fault_handler(struct device *dev,
+ u32 mask, u32 set);
extern int iommu_register_device_fault_handler(struct device *dev,
iommu_dev_fault_handler_t handler,
- void *data);
+ u32 flags, void *data);
extern int iommu_unregister_device_fault_handler(struct device *dev);
@@ -873,10 +884,16 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
return 0;
}
+static inline int iommu_update_device_fault_handler(struct device *dev,
+ u32 mask, u32 set)
+{
+ return -ENODEV;
+}
+
static inline
int iommu_register_device_fault_handler(struct device *dev,
iommu_dev_fault_handler_t handler,
- void *data)
+ u32 flags, void *data)
{
return -ENODEV;
}
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index e1d9e75f2c94..0ce0dfb7713e 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -85,6 +85,8 @@ struct iommu_fault_unrecoverable {
* 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.
+ * If IOMMU_FAULT_PAGE_REQUEST_L2 is set, the fault occurred at the
+ * second level/stage, otherwise, occurred at the first level.
* @pasid: Process Address Space ID
* @grpid: Page Request Group Index
* @perm: requested page permissions (IOMMU_FAULT_PERM_* values)
@@ -96,6 +98,7 @@ 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)
+#define IOMMU_FAULT_PAGE_REQUEST_L2 (1 << 4)
__u32 flags;
__u32 pasid;
__u32 grpid;
Best regards,
baolu
