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 have
addressed in this series.
The major refactoring includes:
- [PATCH 01 ~ 03] Move include/uapi/linux/iommu.h to
include/linux/iommu.h. Remove the unrecoverable fault data definition.
- [PATCH 04 ~ 07] Remove iommu_[un]register_device_fault_handler().
- [PATCH 08 ~ 12] Separate SVA and IOPF. Make IOPF a generic page fault
handling framework.
This is also available at github [2]. I would appreciate your feedback
on this series.
[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://github.com/LuBaolu/intel-iommu/commits/preparatory-io-pgfault-delivery-v2
Change log:
v2:
- 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
avaible for use cases other than SVA.
v1: https://lore.kernel.org/linux-iommu/[email protected]/
Lu Baolu (12):
iommu: Move iommu fault data to linux/iommu.h
iommu/arm-smmu-v3: Remove unrecoverable faults reporting
iommu: Remove unrecoverable fault data
iommu: Replace device fault handler with iommu_queue_iopf()
iommu: Change the return value of dev_iommu_get()
iommu: Make dev->fault_param static
iommu: Remove iommu_[un]register_device_fault_handler()
iommu: Prepare for separating SVA and IOPF
iommu: Move iopf_handler() to iommu-sva.c
iommu: Make iommu_queue_iopf() more generic
iommu: Separate SVA and IOPF in Makefile and Kconfig
iommu: Add helper to set iopf handler for domain
include/linux/iommu.h | 196 +++++++++++++++---
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 | 47 ++---
drivers/iommu/intel/iommu.c | 19 +-
drivers/iommu/intel/svm.c | 1 -
drivers/iommu/io-pgfault.c | 90 +++-----
drivers/iommu/iommu-sva.c | 47 ++++-
drivers/iommu/iommu.c | 153 ++++----------
MAINTAINERS | 1 -
drivers/iommu/Kconfig | 4 +
drivers/iommu/Makefile | 3 +-
drivers/iommu/intel/Kconfig | 1 +
14 files changed, 314 insertions(+), 494 deletions(-)
delete mode 100644 drivers/iommu/iommu-sva.h
delete mode 100644 include/uapi/linux/iommu.h
--
2.34.1
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]>
---
include/linux/iommu.h | 70 +------------------------------------------
1 file changed, 1 insertion(+), 69 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b3537736f9cb..a1e4390d05a8 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -48,69 +48,9 @@ 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;
-};
-
/**
* struct iommu_fault_page_request - Page Request data
* @flags: encodes whether the corresponding fields are valid and whether this
@@ -140,19 +80,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
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]>
---
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 b1dcb1b9b170..b3537736f9cb 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -13,7 +13,6 @@
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/of.h>
-#include <uapi/linux/iommu.h>
#define IOMMU_READ (1 << 0)
#define IOMMU_WRITE (1 << 1)
@@ -42,6 +41,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 3be1bdfe8ecc..8fded0298961 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10877,7 +10877,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
The pair of interfaces are not used anywhere in the tree. Remove it to
avoid dead code.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 23 ---------
drivers/iommu/iommu-sva.h | 4 +-
drivers/iommu/io-pgfault.c | 6 +--
drivers/iommu/iommu.c | 96 --------------------------------------
4 files changed, 4 insertions(+), 125 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a1e4390d05a8..cb12bab38365 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -130,7 +130,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 */
@@ -469,14 +468,10 @@ 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
*/
struct iommu_fault_param {
- iommu_dev_fault_handler_t handler;
- void *data;
struct list_head faults;
struct mutex lock;
};
@@ -602,11 +597,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);
@@ -988,19 +978,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/io-pgfault.c b/drivers/iommu/io-pgfault.c
index e5b8b9110c13..7e6697083f9d 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -103,7 +103,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.
*
@@ -140,14 +140,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 iopf_device_param *iopf_param;
-
- struct device *dev = cookie;
struct dev_iommu *param = dev->iommu;
lockdep_assert_held(¶m->lock);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 3e4ff984aa85..157a28a49473 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1262,98 +1262,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)
- return -EINVAL;
-
- mutex_lock(¶m->lock);
- /* Only allow one fault handler registered for each device */
- if (param->fault_param) {
- 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(¶m->fault_param->lock);
- INIT_LIST_HEAD(¶m->fault_param->faults);
-
-done_unlock:
- mutex_unlock(¶m->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;
- int ret = 0;
-
- if (!param)
- return -EINVAL;
-
- mutex_lock(¶m->lock);
-
- if (!param->fault_param)
- goto unlock;
-
- /* we cannot unregister handler if there are pending faults */
- if (!list_empty(¶m->fault_param->faults)) {
- ret = -EBUSY;
- goto unlock;
- }
-
- kfree(param->fault_param);
- param->fault_param = NULL;
- put_device(dev);
-unlock:
- mutex_unlock(¶m->lock);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
-
/**
* iommu_report_device_fault() - Report fault event to device driver
* @dev: the device
@@ -1378,10 +1286,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(¶m->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)) {
--
2.34.1
This completely separates the IO page fault handling framework from the
SVA implementation. Previously, the SVA implementation was tightly coupled
with the IO page fault handling framework. This makes SVA a "customer" of
the IO page fault handling framework by converting domain's page fault
handler to handle a group of faults and calling it directly from
iommu_queue_iopf().
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 5 +++--
drivers/iommu/iommu-sva.h | 8 --------
drivers/iommu/io-pgfault.c | 16 +++++++++++++---
drivers/iommu/iommu-sva.c | 11 ++---------
drivers/iommu/iommu.c | 2 +-
5 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 607740e548f2..1dafe031a56c 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -40,6 +40,7 @@ struct notifier_block;
struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;
+struct iopf_group;
#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -179,8 +180,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 {
@@ -513,6 +513,7 @@ struct iopf_group {
struct list_head faults;
struct work_struct work;
struct device *dev;
+ void *data;
};
int iommu_device_register(struct iommu_device *iommu,
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index 510a7df23fba..cf41e88fac17 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -22,8 +22,6 @@ 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);
void iopf_free_group(struct iopf_group *group);
int iopf_queue_work(struct iopf_group *group, work_func_t func);
int iommu_sva_handle_iopf_group(struct iopf_group *group);
@@ -65,12 +63,6 @@ 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)
-{
- return IOMMU_PAGE_RESP_INVALID;
-}
-
static inline void iopf_free_group(struct iopf_group *group)
{
}
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 3614a800638c..c5cfa3dfa77b 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -11,8 +11,6 @@
#include <linux/slab.h>
#include <linux/workqueue.h>
-#include "iommu-sva.h"
-
/**
* struct iopf_queue - IO Page Fault queue
* @wq: the fault workqueue
@@ -106,6 +104,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 iopf_device_param *iopf_param;
struct dev_iommu *param = dev->iommu;
@@ -137,6 +136,16 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
return 0;
}
+ if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
+ domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
+ else
+ domain = iommu_get_domain_for_dev(dev);
+
+ if (!domain || !domain->iopf_handler) {
+ ret = -ENODEV;
+ goto cleanup_partial;
+ }
+
group = kzalloc(sizeof(*group), GFP_KERNEL);
if (!group) {
/*
@@ -150,6 +159,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
group->dev = dev;
group->last_fault.fault = *fault;
+ group->data = domain->fault_data;
INIT_LIST_HEAD(&group->faults);
list_add(&group->last_fault.list, &group->faults);
@@ -160,7 +170,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
list_move(&iopf->list, &group->faults);
}
- ret = iommu_sva_handle_iopf_group(group);
+ ret = domain->iopf_handler(group);
if (ret)
iopf_free_group(group);
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index ab42cfdd7636..668f4c2bcf65 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
/*
* I/O page fault handler for SVA
*/
-enum iommu_page_response_code
+static enum iommu_page_response_code
iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
{
vm_fault_t ret;
@@ -241,23 +241,16 @@ 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);
+ status = iommu_sva_handle_iopf(&iopf->fault, group->data);
}
iopf_complete_group(group->dev, &group->last_fault, status);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 157a28a49473..535a36e3edc9 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3330,7 +3330,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
domain->type = IOMMU_DOMAIN_SVA;
mmgrab(mm);
domain->mm = mm;
- domain->iopf_handler = iommu_sva_handle_iopf;
+ domain->iopf_handler = iommu_sva_handle_iopf_group;
domain->fault_data = mm;
return domain;
--
2.34.1
To avoid open code everywhere.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 11 ++++++++++-
drivers/iommu/iommu.c | 20 ++++++++++++++++++--
2 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d725500344a4..4e0b75befefc 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -132,6 +132,7 @@ struct iommu_page_response {
typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
struct device *, unsigned long, int, void *);
+typedef int (*iommu_iopf_handler_t)(struct iopf_group *group);
struct iommu_domain_geometry {
dma_addr_t aperture_start; /* First address that can be mapped */
@@ -181,7 +182,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;
- int (*iopf_handler)(struct iopf_group *group);
+ iommu_iopf_handler_t iopf_handler;
void *fault_data;
union {
struct {
@@ -581,6 +582,8 @@ extern ssize_t iommu_map_sg(struct iommu_domain *domain, unsigned long iova,
extern phys_addr_t iommu_iova_to_phys(struct iommu_domain *domain, dma_addr_t iova);
extern void iommu_set_fault_handler(struct iommu_domain *domain,
iommu_fault_handler_t handler, void *token);
+void iommu_domain_set_iopf_handler(struct iommu_domain *domain,
+ iommu_iopf_handler_t handler, void *data);
extern void iommu_get_resv_regions(struct device *dev, struct list_head *list);
extern void iommu_put_resv_regions(struct device *dev, struct list_head *list);
@@ -904,6 +907,12 @@ static inline void iommu_set_fault_handler(struct iommu_domain *domain,
{
}
+static inline void iommu_domain_set_iopf_handler(struct iommu_domain *domain,
+ iommu_iopf_handler_t handler,
+ void *data)
+{
+}
+
static inline void iommu_get_resv_regions(struct device *dev,
struct list_head *list)
{
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 9d964d16d0ad..2254b0ef91fb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1839,6 +1839,23 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
}
EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
+/**
+ * iommu_domain_set_iopf_handler() - set io page fault handler for a domain
+ * @domain: iommu domain
+ * @handler: fault handler
+ * @data: user data, will be passed back to the fault handler
+ *
+ * This function should be used by iommu domain users which want to be notified
+ * whenever an IOMMU I/O page fault happens.
+ */
+void iommu_domain_set_iopf_handler(struct iommu_domain *domain,
+ iommu_iopf_handler_t handler, void *data)
+{
+ domain->iopf_handler = handler;
+ domain->fault_data = data;
+}
+EXPORT_SYMBOL_GPL(iommu_domain_set_iopf_handler);
+
static struct iommu_domain *__iommu_domain_alloc(const struct bus_type *bus,
unsigned type)
{
@@ -3328,8 +3345,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
domain->type = IOMMU_DOMAIN_SVA;
mmgrab(mm);
domain->mm = mm;
- domain->iopf_handler = iommu_sva_handle_iopf_group;
- domain->fault_data = mm;
+ iommu_domain_set_iopf_handler(domain, iommu_sva_handle_iopf_group, mm);
return domain;
}
--
2.34.1
The iopf_handler() function handles a fault_group for a SVA domain. Move
it to the right place.
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/iommu-sva.h | 17 +++++++++++++
drivers/iommu/io-pgfault.c | 50 +++-----------------------------------
drivers/iommu/iommu-sva.c | 49 +++++++++++++++++++++++++++++++++++++
3 files changed, 69 insertions(+), 47 deletions(-)
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index de7819c796ce..510a7df23fba 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -24,6 +24,9 @@ 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);
+void iopf_free_group(struct iopf_group *group);
+int iopf_queue_work(struct iopf_group *group, work_func_t func);
+int iommu_sva_handle_iopf_group(struct iopf_group *group);
#else /* CONFIG_IOMMU_SVA */
static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
@@ -67,5 +70,19 @@ iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
{
return IOMMU_PAGE_RESP_INVALID;
}
+
+static inline void iopf_free_group(struct iopf_group *group)
+{
+}
+
+static inline int iopf_queue_work(struct iopf_group *group, work_func_t func)
+{
+ return -ENODEV;
+}
+
+static inline int iommu_sva_handle_iopf_group(struct iopf_group *group)
+{
+ return -ENODEV;
+}
#endif /* CONFIG_IOMMU_SVA */
#endif /* _IOMMU_SVA_H */
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1432751ff4d4..3614a800638c 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -40,7 +40,7 @@ struct iopf_device_param {
struct list_head partial;
};
-static void iopf_free_group(struct iopf_group *group)
+void iopf_free_group(struct iopf_group *group)
{
struct iopf_fault *iopf, *next;
@@ -52,51 +52,7 @@ static void iopf_free_group(struct iopf_group *group)
kfree(group);
}
-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,
- };
-
- 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(dev, &resp);
-}
-
-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);
- }
-
- iopf_complete_group(group->dev, &group->last_fault, status);
- iopf_free_group(group);
-}
-
-static int iopf_queue_work(struct iopf_group *group, work_func_t func)
+int iopf_queue_work(struct iopf_group *group, work_func_t func)
{
struct iopf_device_param *iopf_param = group->dev->iommu->iopf_param;
@@ -204,7 +160,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
list_move(&iopf->list, &group->faults);
}
- ret = iopf_queue_work(group, iopf_handler);
+ ret = iommu_sva_handle_iopf_group(group);
if (ret)
iopf_free_group(group);
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 05c0fb2acbc4..ab42cfdd7636 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -219,3 +219,52 @@ void mm_pasid_drop(struct mm_struct *mm)
ida_free(&iommu_global_pasid_ida, mm->pasid);
}
+
+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,
+ };
+
+ 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(dev, &resp);
+}
+
+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);
+ }
+
+ iopf_complete_group(group->dev, &group->last_fault, status);
+ iopf_free_group(group);
+}
+
+int iommu_sva_handle_iopf_group(struct iopf_group *group)
+{
+ return iopf_queue_work(group, iopf_handler);
+}
--
2.34.1
The individual iommu drivers report iommu 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().
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4352a149a935..00309f66153b 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1381,7 +1381,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
Move iopf_group data structure to iommu.h. This is being done to make it
a minimal set of faults that a domain's page fault handler should handle.
Add two new helpers for the domain's page fault handler:
- iopf_free_group: free a fault group after all faults in the group are
handled.
- iopf_queue_work: queue a given work item for a fault group.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 12 ++++++++++
drivers/iommu/io-pgfault.c | 48 ++++++++++++++++++++++----------------
2 files changed, 40 insertions(+), 20 deletions(-)
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index cb12bab38365..607740e548f2 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -503,6 +503,18 @@ struct dev_iommu {
u32 pci_32bit_workaround:1;
};
+struct iopf_fault {
+ struct iommu_fault fault;
+ struct list_head list;
+};
+
+struct iopf_group {
+ struct iopf_fault last_fault;
+ struct list_head faults;
+ struct work_struct work;
+ struct device *dev;
+};
+
int iommu_device_register(struct iommu_device *iommu,
const struct iommu_ops *ops,
struct device *hwdev);
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 7e6697083f9d..1432751ff4d4 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -40,17 +40,17 @@ struct iopf_device_param {
struct list_head partial;
};
-struct iopf_fault {
- struct iommu_fault fault;
- struct list_head list;
-};
+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)
@@ -71,9 +71,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);
@@ -82,7 +82,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.
@@ -90,14 +90,20 @@ 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);
+}
+
+static int iopf_queue_work(struct iopf_group *group, work_func_t func)
+{
+ struct iopf_device_param *iopf_param = group->dev->iommu->iopf_param;
+
+ INIT_WORK(&group->work, func);
+ queue_work(iopf_param->queue->wq, &group->work);
+
+ return 0;
}
/**
@@ -190,7 +196,6 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
group->last_fault.fault = *fault;
INIT_LIST_HEAD(&group->faults);
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) {
@@ -199,8 +204,11 @@ 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;
+ ret = iopf_queue_work(group, iopf_handler);
+ if (ret)
+ iopf_free_group(group);
+
+ return ret;
cleanup_partial:
list_for_each_entry_safe(iopf, next, &iopf_param->partial, list) {
--
2.34.1
The iommu faults, including recoverable faults (IO page faults) and
unrecoverable faults (DMA faults), are generic to all devices. The
iommu faults could possibly be triggered for every device.
The fault_param pointer under struct dev_iommu is the per-device fault
data. Therefore, the fault_param pointer could be static and allocated
during iommu device probe and freed when the device is released.
With this done, the individual iommu drivers that support iopf have no
need to call iommu_[un]register_device_fault_handler() any more.
This will make it easier for the iommu drivers to support iopf, and it
will also make the fault_param allocation and free simpler.
Signed-off-by: Lu Baolu <[email protected]>
---
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 13 +------------
drivers/iommu/intel/iommu.c | 18 ++++--------------
drivers/iommu/iommu.c | 14 ++++++++++++++
3 files changed, 19 insertions(+), 26 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 a5a63b1c947e..fa8ab9d413f8 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
@@ -437,7 +437,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;
/*
@@ -450,16 +449,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)
@@ -469,7 +459,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 d5ca2387e65c..e94e833638a7 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4587,23 +4587,14 @@ 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)
@@ -4630,7 +4621,6 @@ static int intel_iommu_disable_iopf(struct device *dev)
* fault handler and 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/iommu.c b/drivers/iommu/iommu.c
index 4ba3bb692993..3e4ff984aa85 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -302,7 +302,15 @@ static int dev_iommu_get(struct device *dev)
return -ENOMEM;
mutex_init(¶m->lock);
+ param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
+ if (!param->fault_param) {
+ kfree(param);
+ return -ENOMEM;
+ }
+ mutex_init(¶m->fault_param->lock);
+ INIT_LIST_HEAD(¶m->fault_param->faults);
dev->iommu = param;
+
return 0;
}
@@ -315,6 +323,12 @@ static void dev_iommu_free(struct device *dev)
fwnode_handle_put(param->fwspec->iommu_fwnode);
kfree(param->fwspec);
}
+ /*
+ * All pending faults should have been drained before
+ * device release.
+ */
+ WARN_ON_ONCE(!list_empty(¶m->fault_param->faults));
+ kfree(param->fault_param);
kfree(param);
}
--
2.34.1
Make dev_iommu_get() return 0 for success and error numbers for failure.
This will make the code neat and readable. No functionality changes.
Reviewed-by: Jacob Pan <[email protected]>
Signed-off-by: Lu Baolu <[email protected]>
---
drivers/iommu/iommu.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 00309f66153b..4ba3bb692993 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -290,20 +290,20 @@ void iommu_device_unregister(struct iommu_device *iommu)
}
EXPORT_SYMBOL_GPL(iommu_device_unregister);
-static struct dev_iommu *dev_iommu_get(struct device *dev)
+static int dev_iommu_get(struct device *dev)
{
struct dev_iommu *param = dev->iommu;
if (param)
- return param;
+ return 0;
param = kzalloc(sizeof(*param), GFP_KERNEL);
if (!param)
- return NULL;
+ return -ENOMEM;
mutex_init(¶m->lock);
dev->iommu = param;
- return param;
+ return 0;
}
static void dev_iommu_free(struct device *dev)
@@ -346,8 +346,9 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
struct iommu_group *group;
int ret;
- if (!dev_iommu_get(dev))
- return -ENOMEM;
+ ret = dev_iommu_get(dev);
+ if (ret)
+ return ret;
if (!try_module_get(ops->owner)) {
ret = -EINVAL;
@@ -2743,12 +2744,14 @@ int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
const struct iommu_ops *ops)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+ int ret;
if (fwspec)
return ops == fwspec->ops ? 0 : -EINVAL;
- if (!dev_iommu_get(dev))
- return -ENOMEM;
+ ret = dev_iommu_get(dev);
+ if (ret)
+ return ret;
/* Preallocate for the overwhelmingly common case of 1 ID */
fwspec = kzalloc(struct_size(fwspec, ids, 1), GFP_KERNEL);
--
2.34.1
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]>
---
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 9b0dc3505601..1a43f904b6b8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1469,7 +1469,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;
@@ -1479,16 +1478,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;
@@ -1498,6 +1490,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
@@ -1509,32 +1504,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
Add CONFIG_IOMMU_IOPF for page fault handling framework and select it
from its real consumer. Move iopf function declaration from iommu-sva.h
to iommu.h and remove iommu-sva.h as it's empty now.
Signed-off-by: Lu Baolu <[email protected]>
---
include/linux/iommu.h | 63 +++++++++++++++
drivers/iommu/iommu-sva.h | 80 -------------------
.../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 1 -
drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 -
drivers/iommu/intel/iommu.c | 1 -
drivers/iommu/intel/svm.c | 1 -
drivers/iommu/iommu-sva.c | 3 +-
drivers/iommu/iommu.c | 2 -
drivers/iommu/Kconfig | 4 +
drivers/iommu/Makefile | 3 +-
drivers/iommu/intel/Kconfig | 1 +
11 files changed, 71 insertions(+), 89 deletions(-)
delete mode 100644 drivers/iommu/iommu-sva.h
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 1dafe031a56c..d725500344a4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -41,6 +41,7 @@ struct iommu_sva;
struct iommu_fault_event;
struct iommu_dma_cookie;
struct iopf_group;
+struct iopf_queue;
#define IOMMU_FAULT_PERM_READ (1 << 0) /* read */
#define IOMMU_FAULT_PERM_WRITE (1 << 1) /* write */
@@ -1259,6 +1260,7 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
struct mm_struct *mm);
void iommu_sva_unbind_device(struct iommu_sva *handle);
u32 iommu_sva_get_pasid(struct iommu_sva *handle);
+int iommu_sva_handle_iopf_group(struct iopf_group *group);
#else
static inline struct iommu_sva *
iommu_sva_bind_device(struct device *dev, struct mm_struct *mm)
@@ -1277,6 +1279,67 @@ static inline u32 iommu_sva_get_pasid(struct iommu_sva *handle)
static inline void mm_pasid_init(struct mm_struct *mm) {}
static inline bool mm_valid_pasid(struct mm_struct *mm) { return false; }
static inline void mm_pasid_drop(struct mm_struct *mm) {}
+static inline int iommu_sva_handle_iopf_group(struct iopf_group *group)
+{
+ return -ENODEV;
+}
#endif /* CONFIG_IOMMU_SVA */
+#ifdef CONFIG_IOMMU_IOPF
+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,
+ 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);
+int iopf_queue_discard_partial(struct iopf_queue *queue);
+void iopf_free_group(struct iopf_group *group);
+int iopf_queue_work(struct iopf_group *group, work_func_t func);
+#else /* CONFIG_IOMMU_IOPF */
+static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
+{
+ return -ENODEV;
+}
+
+static inline int iopf_queue_add_device(struct iopf_queue *queue,
+ struct device *dev)
+{
+ return -ENODEV;
+}
+
+static inline int iopf_queue_remove_device(struct iopf_queue *queue,
+ struct device *dev)
+{
+ return -ENODEV;
+}
+
+static inline int iopf_queue_flush_dev(struct device *dev)
+{
+ return -ENODEV;
+}
+
+static inline struct iopf_queue *iopf_queue_alloc(const char *name)
+{
+ return NULL;
+}
+
+static inline void iopf_queue_free(struct iopf_queue *queue)
+{
+}
+
+static inline int iopf_queue_discard_partial(struct iopf_queue *queue)
+{
+ return -ENODEV;
+}
+
+static inline void iopf_free_group(struct iopf_group *group)
+{
+}
+
+static inline int iopf_queue_work(struct iopf_group *group, work_func_t func)
+{
+ return -ENODEV;
+}
+#endif /* CONFIG_IOMMU_IOPF */
#endif /* __LINUX_IOMMU_H */
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
deleted file mode 100644
index cf41e88fac17..000000000000
--- a/drivers/iommu/iommu-sva.h
+++ /dev/null
@@ -1,80 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- * SVA library for IOMMU drivers
- */
-#ifndef _IOMMU_SVA_H
-#define _IOMMU_SVA_H
-
-#include <linux/mm_types.h>
-
-/* I/O Page fault */
-struct device;
-struct iommu_fault;
-struct iopf_queue;
-
-#ifdef CONFIG_IOMMU_SVA
-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,
- 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);
-int iopf_queue_discard_partial(struct iopf_queue *queue);
-void iopf_free_group(struct iopf_group *group);
-int iopf_queue_work(struct iopf_group *group, work_func_t func);
-int iommu_sva_handle_iopf_group(struct iopf_group *group);
-
-#else /* CONFIG_IOMMU_SVA */
-static inline int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
-{
- return -ENODEV;
-}
-
-static inline int iopf_queue_add_device(struct iopf_queue *queue,
- struct device *dev)
-{
- return -ENODEV;
-}
-
-static inline int iopf_queue_remove_device(struct iopf_queue *queue,
- struct device *dev)
-{
- return -ENODEV;
-}
-
-static inline int iopf_queue_flush_dev(struct device *dev)
-{
- return -ENODEV;
-}
-
-static inline struct iopf_queue *iopf_queue_alloc(const char *name)
-{
- return NULL;
-}
-
-static inline void iopf_queue_free(struct iopf_queue *queue)
-{
-}
-
-static inline int iopf_queue_discard_partial(struct iopf_queue *queue)
-{
- return -ENODEV;
-}
-
-static inline void iopf_free_group(struct iopf_group *group)
-{
-}
-
-static inline int iopf_queue_work(struct iopf_group *group, work_func_t func)
-{
- return -ENODEV;
-}
-
-static inline int iommu_sva_handle_iopf_group(struct iopf_group *group)
-{
- return -ENODEV;
-}
-#endif /* CONFIG_IOMMU_SVA */
-#endif /* _IOMMU_SVA_H */
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 fa8ab9d413f8..003426327e20 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
@@ -10,7 +10,6 @@
#include <linux/slab.h>
#include "arm-smmu-v3.h"
-#include "../../iommu-sva.h"
#include "../../io-pgtable-arm.h"
struct arm_smmu_mmu_notifier {
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 1a43f904b6b8..cc4223692537 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -29,7 +29,6 @@
#include "arm-smmu-v3.h"
#include "../../dma-iommu.h"
-#include "../../iommu-sva.h"
static bool disable_bypass = true;
module_param(disable_bypass, bool, 0444);
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index e94e833638a7..5a8949d01391 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -26,7 +26,6 @@
#include "iommu.h"
#include "../dma-iommu.h"
#include "../irq_remapping.h"
-#include "../iommu-sva.h"
#include "pasid.h"
#include "cap_audit.h"
#include "perfmon.h"
diff --git a/drivers/iommu/intel/svm.c b/drivers/iommu/intel/svm.c
index e95b339e9cdc..8d409dcda166 100644
--- a/drivers/iommu/intel/svm.c
+++ b/drivers/iommu/intel/svm.c
@@ -22,7 +22,6 @@
#include "iommu.h"
#include "pasid.h"
#include "perf.h"
-#include "../iommu-sva.h"
#include "trace.h"
static irqreturn_t prq_event_thread(int irq, void *d);
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 668f4c2bcf65..3328f00963de 100644
--- a/drivers/iommu/iommu-sva.c
+++ b/drivers/iommu/iommu-sva.c
@@ -6,8 +6,7 @@
#include <linux/mutex.h>
#include <linux/sched/mm.h>
#include <linux/iommu.h>
-
-#include "iommu-sva.h"
+#include <linux/mm_types.h>
static DEFINE_MUTEX(iommu_sva_lock);
static DEFINE_IDA(iommu_global_pasid_ida);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 535a36e3edc9..9d964d16d0ad 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -35,8 +35,6 @@
#include "dma-iommu.h"
-#include "iommu-sva.h"
-
static struct kset *iommu_group_kset;
static DEFINE_IDA(iommu_group_ida);
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 2b12b583ef4b..e7b87ec525a7 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -158,6 +158,9 @@ config IOMMU_DMA
config IOMMU_SVA
bool
+config IOMMU_IOPF
+ bool
+
config FSL_PAMU
bool "Freescale IOMMU support"
depends on PCI
@@ -404,6 +407,7 @@ config ARM_SMMU_V3_SVA
bool "Shared Virtual Addressing support for the ARM SMMUv3"
depends on ARM_SMMU_V3
select IOMMU_SVA
+ select IOMMU_IOPF
select MMU_NOTIFIER
help
Support for sharing process address spaces with devices using the
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 769e43d780ce..6d114fe7ae89 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -27,6 +27,7 @@ obj-$(CONFIG_FSL_PAMU) += fsl_pamu.o fsl_pamu_domain.o
obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
-obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o io-pgfault.o
+obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
+obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
obj-$(CONFIG_APPLE_DART) += apple-dart.o
diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index 2e56bd79f589..613f149510a7 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -50,6 +50,7 @@ config INTEL_IOMMU_SVM
depends on X86_64
select MMU_NOTIFIER
select IOMMU_SVA
+ select IOMMU_IOPF
help
Shared Virtual Memory (SVM) provides a facility for devices
to access DMA resources through process address space by
--
2.34.1
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:48 PM
>
> 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]>
> ---
> 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
>
put this behind patch2/3 then there are less lines to be moved.
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:48 PM
>
> The individual iommu drivers report iommu 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().
>
> Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -4630,7 +4621,6 @@ static int intel_iommu_disable_iopf(struct device
> *dev)
> * fault handler and 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));
the comment should be updated too.
>
> mutex_init(¶m->lock);
> + param->fault_param = kzalloc(sizeof(*param->fault_param),
> GFP_KERNEL);
> + if (!param->fault_param) {
> + kfree(param);
> + return -ENOMEM;
> + }
> + mutex_init(¶m->fault_param->lock);
> + INIT_LIST_HEAD(¶m->fault_param->faults);
let's also move 'partial' from struct iopf_device_param into struct
iommu_fault_param. That logic is not specific to sva.
meanwhile probably iopf_device_param can be renamed to
iopf_sva_param since all the remaining fields are only used by
the sva handler.
current naming (iommu_fault_param vs. iopf_device_param) is a
bit confusing when reading related code.
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> The pair of interfaces are not used anywhere in the tree. Remove it to
> avoid dead code.
>
> Signed-off-by: Lu Baolu <[email protected]>
Reviewed-by: Kevin Tian <[email protected]>
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -82,7 +82,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.
> @@ -90,14 +90,20 @@ 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);
> +}
this is perf-critical path. It's not good to traverse the list twice.
> +
> +static int iopf_queue_work(struct iopf_group *group, work_func_t func)
> +{
> + struct iopf_device_param *iopf_param = group->dev->iommu-
> >iopf_param;
> +
> + INIT_WORK(&group->work, func);
> + queue_work(iopf_param->queue->wq, &group->work);
> +
> + return 0;
> }
Is there plan to introduce further error in the future? otherwise this should
be void.
btw the work queue is only for sva. If there is no other caller this can be
just kept in iommu-sva.c. No need to create a helper.
> @@ -199,8 +204,11 @@ 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;
> + ret = iopf_queue_work(group, iopf_handler);
> + if (ret)
> + iopf_free_group(group);
> +
> + return ret;
>
Here we can document that the iopf handler (in patch10) should free the
group, allowing the optimization inside the handler.
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> @@ -219,3 +219,52 @@ void mm_pasid_drop(struct mm_struct *mm)
>
> ida_free(&iommu_global_pasid_ida, mm->pasid);
> }
> +
> +static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
> + enum iommu_page_response_code status)
iommu_sva_complete_iopf()
> +
> +static void iopf_handler(struct work_struct *work)
> +{
iommu_sva_iopf_handler()
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:49 PM
>
> Make dev_iommu_get() return 0 for success and error numbers for failure.
> This will make the code neat and readable. No functionality changes.
>
> Reviewed-by: Jacob Pan <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00309f66153b..4ba3bb692993 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
> iommu_device *iommu)
> }
> EXPORT_SYMBOL_GPL(iommu_device_unregister);
>
> -static struct dev_iommu *dev_iommu_get(struct device *dev)
> +static int dev_iommu_get(struct device *dev)
> {
> struct dev_iommu *param = dev->iommu;
>
> if (param)
> - return param;
> + return 0;
>
> param = kzalloc(sizeof(*param), GFP_KERNEL);
> if (!param)
> - return NULL;
> + return -ENOMEM;
>
> mutex_init(¶m->lock);
> dev->iommu = param;
> - return param;
> + return 0;
> }
>
Jason's series [1] has been queued. Time to refine according to
the discussion in [2].
[1] https://lore.kernel.org/linux-iommu/[email protected]/
[2] https://lore.kernel.org/linux-iommu/[email protected]/
> From: Lu Baolu <[email protected]>
> Sent: Thursday, July 27, 2023 1:48 PM
>
> struct iommu_fault {
> __u32 type;
> - __u32 padding;
this padding should be kept.
> - union {
> - struct iommu_fault_unrecoverable event;
> - struct iommu_fault_page_request prm;
> - __u8 padding2[56];
> - };
> + struct iommu_fault_page_request prm;
> };
On 2023/8/3 16:08, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> @@ -4630,7 +4621,6 @@ static int intel_iommu_disable_iopf(struct device
>> *dev)
>> * fault handler and 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));
>
> the comment should be updated too.
Ack.
>
>>
>> mutex_init(¶m->lock);
>> + param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
>> + if (!param->fault_param) {
>> + kfree(param);
>> + return -ENOMEM;
>> + }
>> + mutex_init(¶m->fault_param->lock);
>> + INIT_LIST_HEAD(¶m->fault_param->faults);
>
> let's also move 'partial' from struct iopf_device_param into struct
> iommu_fault_param. That logic is not specific to sva.
>
> meanwhile probably iopf_device_param can be renamed to
> iopf_sva_param since all the remaining fields are only used by
> the sva handler.
>
> current naming (iommu_fault_param vs. iopf_device_param) is a
> bit confusing when reading related code.
My understanding is that iommu_fault_param is for all kinds of iommu
faults. Currently they probably include recoverable IO page faults or
unrecoverable DMA faults.
While, iopf_device_param is for the recoverable IO page faults. I agree
that this naming is not specific and even confusing. Perhaps renaming it
to something like iommu_iopf_param?
Best regards,
baolu
On 2023/8/3 15:59, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>> This will make the code neat and readable. No functionality changes.
>>
>> Reviewed-by: Jacob Pan <[email protected]>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/iommu.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 00309f66153b..4ba3bb692993 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
>> iommu_device *iommu)
>> }
>> EXPORT_SYMBOL_GPL(iommu_device_unregister);
>>
>> -static struct dev_iommu *dev_iommu_get(struct device *dev)
>> +static int dev_iommu_get(struct device *dev)
>> {
>> struct dev_iommu *param = dev->iommu;
>>
>> if (param)
>> - return param;
>> + return 0;
>>
>> param = kzalloc(sizeof(*param), GFP_KERNEL);
>> if (!param)
>> - return NULL;
>> + return -ENOMEM;
>>
>> mutex_init(¶m->lock);
>> dev->iommu = param;
>> - return param;
>> + return 0;
>> }
>>
>
> Jason's series [1] has been queued. Time to refine according to
> the discussion in [2].
>
> [1] https://lore.kernel.org/linux-iommu/[email protected]/
> [2] https://lore.kernel.org/linux-iommu/[email protected]/
I'm not sure I understand your point here. This only changes the return
value of dev_iommu_get() to make the code more concise.
Best regards,
baolu
On 2023/8/3 16:16, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> @@ -82,7 +82,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.
>> @@ -90,14 +90,20 @@ 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);
>> +}
>
> this is perf-critical path. It's not good to traverse the list twice.
Freeing the fault group is not critical anymore, right?
>
>> +
>> +static int iopf_queue_work(struct iopf_group *group, work_func_t func)
>> +{
>> + struct iopf_device_param *iopf_param = group->dev->iommu-
>>> iopf_param;
>> +
>> + INIT_WORK(&group->work, func);
>> + queue_work(iopf_param->queue->wq, &group->work);
>> +
>> + return 0;
>> }
>
> Is there plan to introduce further error in the future? otherwise this should
> be void.
queue_work() return true or false. I should check and return the value.
>
> btw the work queue is only for sva. If there is no other caller this can be
> just kept in iommu-sva.c. No need to create a helper.
The definition of struct iopf_device_param is in this file. So I added a
helper to avoid making iopf_device_param visible globally.
>
>> @@ -199,8 +204,11 @@ 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;
>> + ret = iopf_queue_work(group, iopf_handler);
>> + if (ret)
>> + iopf_free_group(group);
>> +
>> + return ret;
>>
>
> Here we can document that the iopf handler (in patch10) should free the
> group, allowing the optimization inside the handler.
Yeah!
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Friday, August 4, 2023 11:10 AM
>
> On 2023/8/3 15:59, Tian, Kevin wrote:
> >> From: Lu Baolu <[email protected]>
> >> Sent: Thursday, July 27, 2023 1:49 PM
> >>
> >> Make dev_iommu_get() return 0 for success and error numbers for failure.
> >> This will make the code neat and readable. No functionality changes.
> >>
> >> Reviewed-by: Jacob Pan <[email protected]>
> >> Signed-off-by: Lu Baolu <[email protected]>
> >> ---
> >> drivers/iommu/iommu.c | 19 +++++++++++--------
> >> 1 file changed, 11 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index 00309f66153b..4ba3bb692993 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
> >> iommu_device *iommu)
> >> }
> >> EXPORT_SYMBOL_GPL(iommu_device_unregister);
> >>
> >> -static struct dev_iommu *dev_iommu_get(struct device *dev)
> >> +static int dev_iommu_get(struct device *dev)
> >> {
> >> struct dev_iommu *param = dev->iommu;
> >>
> >> if (param)
> >> - return param;
> >> + return 0;
> >>
> >> param = kzalloc(sizeof(*param), GFP_KERNEL);
> >> if (!param)
> >> - return NULL;
> >> + return -ENOMEM;
> >>
> >> mutex_init(¶m->lock);
> >> dev->iommu = param;
> >> - return param;
> >> + return 0;
> >> }
> >>
> >
> > Jason's series [1] has been queued. Time to refine according to
> > the discussion in [2].
> >
> > [1] https://lore.kernel.org/linux-iommu/[email protected]/
> > [2] https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-
> [email protected]/
>
> I'm not sure I understand your point here. This only changes the return
> value of dev_iommu_get() to make the code more concise.
>
I thought the purpose of this patch was to prepare for next patch which
moves dev->fault_param initialization to dev_iommu_get().
with Jason's rework IMHO that initialization more fits in iommu_init_device().
that's my real point. If you still want to clean up dev_iommu_get() it's fine
but then it may not belong to this series. ????
On 2023/8/3 16:21, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Thursday, July 27, 2023 1:49 PM
>>
>> @@ -219,3 +219,52 @@ void mm_pasid_drop(struct mm_struct *mm)
>>
>> ida_free(&iommu_global_pasid_ida, mm->pasid);
>> }
>> +
>> +static int iopf_complete_group(struct device *dev, struct iopf_fault *iopf,
>> + enum iommu_page_response_code status)
>
> iommu_sva_complete_iopf()
>
>> +
>> +static void iopf_handler(struct work_struct *work)
>> +{
>
> iommu_sva_iopf_handler()
>
Ack to both.
Best regards,
baolu
On 2023/8/3 15:53, Tian, Kevin wrote:
>> From: Lu Baolu <[email protected]>
>> Sent: Thursday, July 27, 2023 1:48 PM
>>
>> 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]>
>> ---
>> 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
>>
>
> put this behind patch2/3 then there are less lines to be moved.
My idea was that "include/uapi/linux.h" is not used anywhere and we no
longer want to maintain it. Hence, we should remove it before changing
any lines inside it.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Friday, August 4, 2023 11:17 AM
>
> On 2023/8/3 16:08, Tian, Kevin wrote:
> >> From: Lu Baolu <[email protected]>
> >> Sent: Thursday, July 27, 2023 1:49 PM
> >>
> >>
> >> mutex_init(¶m->lock);
> >> + param->fault_param = kzalloc(sizeof(*param->fault_param),
> >> GFP_KERNEL);
> >> + if (!param->fault_param) {
> >> + kfree(param);
> >> + return -ENOMEM;
> >> + }
> >> + mutex_init(¶m->fault_param->lock);
> >> + INIT_LIST_HEAD(¶m->fault_param->faults);
> >
> > let's also move 'partial' from struct iopf_device_param into struct
> > iommu_fault_param. That logic is not specific to sva.
> >
> > meanwhile probably iopf_device_param can be renamed to
> > iopf_sva_param since all the remaining fields are only used by
> > the sva handler.
> >
> > current naming (iommu_fault_param vs. iopf_device_param) is a
> > bit confusing when reading related code.
>
> My understanding is that iommu_fault_param is for all kinds of iommu
> faults. Currently they probably include recoverable IO page faults or
> unrecoverable DMA faults.
>
> While, iopf_device_param is for the recoverable IO page faults. I agree
> that this naming is not specific and even confusing. Perhaps renaming it
> to something like iommu_iopf_param?
>
or just iopf_param...
On 2023/8/3 15:54, Tian, Kevin wrote:
>> From: Lu Baolu<[email protected]>
>> Sent: Thursday, July 27, 2023 1:48 PM
>>
>> struct iommu_fault {
>> __u32 type;
>> - __u32 padding;
> this padding should be kept.
>
To keep above 64-bit aligned, right?
I can't think of any other reason to keep it.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Friday, August 4, 2023 10:59 AM
>
> On 2023/8/3 15:54, Tian, Kevin wrote:
> >> From: Lu Baolu<[email protected]>
> >> Sent: Thursday, July 27, 2023 1:48 PM
> >>
> >> struct iommu_fault {
> >> __u32 type;
> >> - __u32 padding;
> > this padding should be kept.
> >
>
> To keep above 64-bit aligned, right?
>
yes
On 8/4/23 11:51 AM, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Friday, August 4, 2023 10:59 AM
>>
>> On 2023/8/3 15:54, Tian, Kevin wrote:
>>>> From: Lu Baolu<[email protected]>
>>>> Sent: Thursday, July 27, 2023 1:48 PM
>>>>
>>>> struct iommu_fault {
>>>> __u32 type;
>>>> - __u32 padding;
>>> this padding should be kept.
>>>
>>
>> To keep above 64-bit aligned, right?
>>
>
> yes
Okay, thanks!
Best regards,
baolu
On 8/4/23 11:56 AM, Tian, Kevin wrote:
>> From: Baolu Lu <[email protected]>
>> Sent: Friday, August 4, 2023 11:17 AM
>>
>> On 2023/8/3 16:08, Tian, Kevin wrote:
>>>> From: Lu Baolu <[email protected]>
>>>> Sent: Thursday, July 27, 2023 1:49 PM
>>>>
>>>>
>>>> mutex_init(¶m->lock);
>>>> + param->fault_param = kzalloc(sizeof(*param->fault_param),
>>>> GFP_KERNEL);
>>>> + if (!param->fault_param) {
>>>> + kfree(param);
>>>> + return -ENOMEM;
>>>> + }
>>>> + mutex_init(¶m->fault_param->lock);
>>>> + INIT_LIST_HEAD(¶m->fault_param->faults);
>>>
>>> let's also move 'partial' from struct iopf_device_param into struct
>>> iommu_fault_param. That logic is not specific to sva.
>>>
>>> meanwhile probably iopf_device_param can be renamed to
>>> iopf_sva_param since all the remaining fields are only used by
>>> the sva handler.
>>>
>>> current naming (iommu_fault_param vs. iopf_device_param) is a
>>> bit confusing when reading related code.
>>
>> My understanding is that iommu_fault_param is for all kinds of iommu
>> faults. Currently they probably include recoverable IO page faults or
>> unrecoverable DMA faults.
>>
>> While, iopf_device_param is for the recoverable IO page faults. I agree
>> that this naming is not specific and even confusing. Perhaps renaming it
>> to something like iommu_iopf_param?
>>
>
> or just iopf_param.
Okay.
Best regards,
baolu
On 8/4/23 11:55 AM, Tian, Kevin wrote:
>> From: Baolu Lu<[email protected]>
>> Sent: Friday, August 4, 2023 11:10 AM
>>
>> On 2023/8/3 15:59, Tian, Kevin wrote:
>>>> From: Lu Baolu<[email protected]>
>>>> Sent: Thursday, July 27, 2023 1:49 PM
>>>>
>>>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>>>> This will make the code neat and readable. No functionality changes.
>>>>
>>>> Reviewed-by: Jacob Pan<[email protected]>
>>>> Signed-off-by: Lu Baolu<[email protected]>
>>>> ---
>>>> drivers/iommu/iommu.c | 19 +++++++++++--------
>>>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 00309f66153b..4ba3bb692993 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct
>>>> iommu_device *iommu)
>>>> }
>>>> EXPORT_SYMBOL_GPL(iommu_device_unregister);
>>>>
>>>> -static struct dev_iommu *dev_iommu_get(struct device *dev)
>>>> +static int dev_iommu_get(struct device *dev)
>>>> {
>>>> struct dev_iommu *param = dev->iommu;
>>>>
>>>> if (param)
>>>> - return param;
>>>> + return 0;
>>>>
>>>> param = kzalloc(sizeof(*param), GFP_KERNEL);
>>>> if (!param)
>>>> - return NULL;
>>>> + return -ENOMEM;
>>>>
>>>> mutex_init(¶m->lock);
>>>> dev->iommu = param;
>>>> - return param;
>>>> + return 0;
>>>> }
>>>>
>>> Jason's series [1] has been queued. Time to refine according to
>>> the discussion in [2].
>>>
>>> [1]https://lore.kernel.org/linux-iommu/[email protected]/
>>> [2]https://lore.kernel.org/linux-iommu/c815fa2b-00df-91e1-8353-
>> [email protected]/
>>
>> I'm not sure I understand your point here. This only changes the return
>> value of dev_iommu_get() to make the code more concise.
>>
> I thought the purpose of this patch was to prepare for next patch which
> moves dev->fault_param initialization to dev_iommu_get().
Yes.
>
> with Jason's rework IMHO that initialization more fits in iommu_init_device().
>
> that's my real point. If you still want to clean up dev_iommu_get() it's fine
> but then it may not belong to this series. ????
Ah, I see. Let me make a choice in the next version.
Best regards,
baolu
On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <[email protected]>
> > Sent: Friday, August 4, 2023 10:59 AM
> >
> > On 2023/8/3 15:54, Tian, Kevin wrote:
> > >> From: Lu Baolu<[email protected]>
> > >> Sent: Thursday, July 27, 2023 1:48 PM
> > >>
> > >> struct iommu_fault {
> > >> __u32 type;
> > >> - __u32 padding;
> > > this padding should be kept.
> > >
> >
> > To keep above 64-bit aligned, right?
> >
>
> yes
If it is not uapi we should not explicitly document padding (and __u32
should be u32). The compiler will add it if it is necessary.
If the compiler isn't right for some reason then something else has
gone wrong.
Jason
On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> Is there plan to introduce further error in the future? otherwise this should
> be void.
>
> btw the work queue is only for sva. If there is no other caller this can be
> just kept in iommu-sva.c. No need to create a helper.
I think more than just SVA will need a work queue context to process
their faults.
Jason
> From: Jason Gunthorpe <[email protected]>
> Sent: Wednesday, August 9, 2023 2:43 AM
>
> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>
> > Is there plan to introduce further error in the future? otherwise this should
> > be void.
> >
> > btw the work queue is only for sva. If there is no other caller this can be
> > just kept in iommu-sva.c. No need to create a helper.
>
> I think more than just SVA will need a work queue context to process
> their faults.
>
then this series needs more work. Currently the abstraction doesn't
include workqueue in the common fault reporting layer.
> From: Jason Gunthorpe <[email protected]>
> Sent: Wednesday, August 9, 2023 2:40 AM
>
> On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > > From: Baolu Lu <[email protected]>
> > > Sent: Friday, August 4, 2023 10:59 AM
> > >
> > > On 2023/8/3 15:54, Tian, Kevin wrote:
> > > >> From: Lu Baolu<[email protected]>
> > > >> Sent: Thursday, July 27, 2023 1:48 PM
> > > >>
> > > >> struct iommu_fault {
> > > >> __u32 type;
> > > >> - __u32 padding;
> > > > this padding should be kept.
> > > >
> > >
> > > To keep above 64-bit aligned, right?
> > >
> >
> > yes
>
> If it is not uapi we should not explicitly document padding (and __u32
> should be u32). The compiler will add it if it is necessary.
>
> If the compiler isn't right for some reason then something else has
> gone wrong.
>
I thought this will be used as uAPI later. I'm fine to leave it be and
add the padding when the uAPI is introduced.
On 2023/8/9 8:02, Tian, Kevin wrote:
>> From: Jason Gunthorpe <[email protected]>
>> Sent: Wednesday, August 9, 2023 2:43 AM
>>
>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>
>>> Is there plan to introduce further error in the future? otherwise this should
>>> be void.
>>>
>>> btw the work queue is only for sva. If there is no other caller this can be
>>> just kept in iommu-sva.c. No need to create a helper.
>>
>> I think more than just SVA will need a work queue context to process
>> their faults.
>>
>
> then this series needs more work. Currently the abstraction doesn't
> include workqueue in the common fault reporting layer.
Do you mind elaborate a bit here? workqueue is a basic infrastructure in
the fault handling framework, but it lets the consumers choose to use
it, or not to.
Best regards,
baolu
On Wed, Aug 09, 2023 at 12:01:52AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <[email protected]>
> > Sent: Wednesday, August 9, 2023 2:40 AM
> >
> > On Fri, Aug 04, 2023 at 03:51:30AM +0000, Tian, Kevin wrote:
> > > > From: Baolu Lu <[email protected]>
> > > > Sent: Friday, August 4, 2023 10:59 AM
> > > >
> > > > On 2023/8/3 15:54, Tian, Kevin wrote:
> > > > >> From: Lu Baolu<[email protected]>
> > > > >> Sent: Thursday, July 27, 2023 1:48 PM
> > > > >>
> > > > >> struct iommu_fault {
> > > > >> __u32 type;
> > > > >> - __u32 padding;
> > > > > this padding should be kept.
> > > > >
> > > >
> > > > To keep above 64-bit aligned, right?
> > > >
> > >
> > > yes
> >
> > If it is not uapi we should not explicitly document padding (and __u32
> > should be u32). The compiler will add it if it is necessary.
> >
> > If the compiler isn't right for some reason then something else has
> > gone wrong.
> >
>
> I thought this will be used as uAPI later. I'm fine to leave it be and
> add the padding when the uAPI is introduced.
Yes
Jason
On Thu, Jul 27, 2023 at 01:48:30PM +0800, Lu Baolu wrote:
> Make dev_iommu_get() return 0 for success and error numbers for failure.
> This will make the code neat and readable. No functionality changes.
>
> Reviewed-by: Jacob Pan <[email protected]>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 00309f66153b..4ba3bb692993 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct iommu_device *iommu)
> }
> EXPORT_SYMBOL_GPL(iommu_device_unregister);
It could probably use a nicer name too?
iommu_alloc_dev_iommu() ?
Also with Joerg's current tree we can add a device_lock_assert() to
this function, from what I can tell.
Jason
On Thu, Jul 27, 2023 at 01:48:26PM +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.
>
> Signed-off-by: Lu Baolu <[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
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> 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]>
> ---
> include/linux/iommu.h | 70 +------------------------------------------
> 1 file changed, 1 insertion(+), 69 deletions(-)
Do we plan to bring this back in some form? A driver specific fault
report via iommufd?
Reviewed-by: Jason Gunthorpe <[email protected]>
Jason
On 2023/8/10 0:59, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
>> 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]>
>> ---
>> include/linux/iommu.h | 70 +------------------------------------------
>> 1 file changed, 1 insertion(+), 69 deletions(-)
> Do we plan to bring this back in some form? A driver specific fault
> report via iommufd?
I can hardly see the possibility.
The only necessary dma fault messages are the offending address and the
permissions. With these, the user space device model software knows that
"a DMA fault was generated when the IOMMU hardware tried to translate
the offending address with the given permissions".
And then, the device model software will walk the page table and figure
out what is missed before injecting the vendor-specific fault messages
to the VM guest.
Best regards,
baolu
On 2023/8/10 0:58, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:30PM +0800, Lu Baolu wrote:
>> Make dev_iommu_get() return 0 for success and error numbers for failure.
>> This will make the code neat and readable. No functionality changes.
>>
>> Reviewed-by: Jacob Pan<[email protected]>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> drivers/iommu/iommu.c | 19 +++++++++++--------
>> 1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 00309f66153b..4ba3bb692993 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -290,20 +290,20 @@ void iommu_device_unregister(struct iommu_device *iommu)
>> }
>> EXPORT_SYMBOL_GPL(iommu_device_unregister);
> It could probably use a nicer name too?
>
> iommu_alloc_dev_iommu() ?
>
> Also with Joerg's current tree we can add a device_lock_assert() to
> this function, from what I can tell.
Sure. Will update them in the next version.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Wednesday, August 9, 2023 6:41 PM
>
> On 2023/8/9 8:02, Tian, Kevin wrote:
> >> From: Jason Gunthorpe <[email protected]>
> >> Sent: Wednesday, August 9, 2023 2:43 AM
> >>
> >> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> >>
> >>> Is there plan to introduce further error in the future? otherwise this
> should
> >>> be void.
> >>>
> >>> btw the work queue is only for sva. If there is no other caller this can be
> >>> just kept in iommu-sva.c. No need to create a helper.
> >>
> >> I think more than just SVA will need a work queue context to process
> >> their faults.
> >>
> >
> > then this series needs more work. Currently the abstraction doesn't
> > include workqueue in the common fault reporting layer.
>
> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> the fault handling framework, but it lets the consumers choose to use
> it, or not to.
>
My understanding of Jason's comment was to make the workqueue the
default path instead of being opted by the consumer.. that is my 1st
impression but might be wrong...
On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
> > From: Baolu Lu <[email protected]>
> > Sent: Wednesday, August 9, 2023 6:41 PM
> >
> > On 2023/8/9 8:02, Tian, Kevin wrote:
> > >> From: Jason Gunthorpe <[email protected]>
> > >> Sent: Wednesday, August 9, 2023 2:43 AM
> > >>
> > >> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> > >>
> > >>> Is there plan to introduce further error in the future? otherwise this
> > should
> > >>> be void.
> > >>>
> > >>> btw the work queue is only for sva. If there is no other caller this can be
> > >>> just kept in iommu-sva.c. No need to create a helper.
> > >>
> > >> I think more than just SVA will need a work queue context to process
> > >> their faults.
> > >>
> > >
> > > then this series needs more work. Currently the abstraction doesn't
> > > include workqueue in the common fault reporting layer.
> >
> > Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> > the fault handling framework, but it lets the consumers choose to use
> > it, or not to.
> >
>
> My understanding of Jason's comment was to make the workqueue the
> default path instead of being opted by the consumer.. that is my 1st
> impression but might be wrong...
Yeah, that is one path. Do we have anyone that uses this that doesn't
want the WQ? (actually who even uses this besides SVA?)
Jason
On Thu, Jul 27, 2023 at 01:48:29PM +0800, Lu Baolu wrote:
> The individual iommu drivers report iommu 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().
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4352a149a935..00309f66153b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1381,7 +1381,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);
Also fix the function signature at this point:
int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
It should not be 'void *cookie' anymore, it is just 'struct device *dev'
Jason
On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
> On 2023/8/10 0:59, Jason Gunthorpe wrote:
> > On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> > > 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]>
> > > ---
> > > include/linux/iommu.h | 70 +------------------------------------------
> > > 1 file changed, 1 insertion(+), 69 deletions(-)
> > Do we plan to bring this back in some form? A driver specific fault
> > report via iommufd?
>
> I can hardly see the possibility.
>
> The only necessary dma fault messages are the offending address and the
> permissions. With these, the user space device model software knows that
> "a DMA fault was generated when the IOMMU hardware tried to translate
> the offending address with the given permissions".
>
> And then, the device model software will walk the page table and figure
> out what is missed before injecting the vendor-specific fault messages
> to the VM guest.
Avoiding walking the page table sounds like a pretty big win if we
could manage it by forwarding more event data..
Jason
On Thu, Jul 27, 2023 at 01:48:35PM +0800, Lu Baolu wrote:
> @@ -137,6 +136,16 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
> return 0;
> }
>
> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
> + domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
> + else
> + domain = iommu_get_domain_for_dev(dev);
How does the lifetime work for this? What prevents UAF on domain?
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index ab42cfdd7636..668f4c2bcf65 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
> /*
> * I/O page fault handler for SVA
> */
> -enum iommu_page_response_code
> +static enum iommu_page_response_code
> iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> {
> vm_fault_t ret;
> @@ -241,23 +241,16 @@ 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);
> + status = iommu_sva_handle_iopf(&iopf->fault, group->data);
> }
>
> iopf_complete_group(group->dev, &group->last_fault, status);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 157a28a49473..535a36e3edc9 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3330,7 +3330,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
> domain->type = IOMMU_DOMAIN_SVA;
> mmgrab(mm);
> domain->mm = mm;
> - domain->iopf_handler = iommu_sva_handle_iopf;
> + domain->iopf_handler = iommu_sva_handle_iopf_group;
> domain->fault_data = mm;
This also has lifetime problems on the mm.
The domain should flow into the iommu_sva_handle_iopf() instead of the
void *data.
The SVA code can then just use domain->mm directly.
We need to document/figure out some how to ensure that the faults are
all done processing before a fault enabled domain can be freed.
This patch would be better ordered before the prior patch.
Jason
On Thu, Jul 27, 2023 at 01:48:32PM +0800, Lu Baolu wrote:
> The pair of interfaces are not used anywhere in the tree. Remove it to
> avoid dead code.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 23 ---------
> drivers/iommu/iommu-sva.h | 4 +-
> drivers/iommu/io-pgfault.c | 6 +--
> drivers/iommu/iommu.c | 96 --------------------------------------
> 4 files changed, 4 insertions(+), 125 deletions(-)
Some of these hunks should be moved earlier, but
Reviewed-by: Jason Gunthorpe <[email protected]>
Happy to see it go
Jason
On Thu, Jul 27, 2023 at 01:48:34PM +0800, Lu Baolu wrote:
> The iopf_handler() function handles a fault_group for a SVA domain. Move
> it to the right place.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu-sva.h | 17 +++++++++++++
> drivers/iommu/io-pgfault.c | 50 +++-----------------------------------
> drivers/iommu/iommu-sva.c | 49 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 69 insertions(+), 47 deletions(-)
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 05c0fb2acbc4..ab42cfdd7636 100644
> --- a/drivers/iommu/iommu-sva.c
> +++ b/drivers/iommu/iommu-sva.c
> @@ -219,3 +219,52 @@ void mm_pasid_drop(struct mm_struct *mm)
> +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);
> + }
> +
> + iopf_complete_group(group->dev, &group->last_fault, status);
> + iopf_free_group(group);
> +}
Routing faults to domains is generic code, not SVA code.
SVA starts at domain->iopf_handler
Jason
On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
> To avoid open code everywhere.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 11 ++++++++++-
> drivers/iommu/iommu.c | 20 ++++++++++++++++++--
> 2 files changed, 28 insertions(+), 3 deletions(-)
Seems like overkill at this point..
Also, I think this is probably upside down.
We want to create the domains as fault enabled in the first place.
A fault enabled domain should never be attached to something that
cannot support faults. It should also not support changing the fault
handler while it exists.
Thus at the creation point would be the time to supply the fault handler
as part of requesting faulting.
Taking an existing domain and making it faulting enabled is going to
be really messy in all the corner cases.
My advice (and Robin will probably hate me), is to define a new op:
struct domain_alloc_paging_args {
struct fault_handler *fault_handler;
void *fault_data
};
struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
domain_alloc_paging_args *args)
The point would be to leave the majority of drivers using the
simplified, core assisted, domain_alloc_paging() interface and they
just don't have to touch any of this stuff at all.
Obviously if handler is given then the domain will be initialized as
faulting.
Jason
On Thu, Aug 10, 2023 at 03:20:07PM -0300, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 4ba3bb692993..3e4ff984aa85 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -302,7 +302,15 @@ static int dev_iommu_get(struct device *dev)
> > return -ENOMEM;
> >
> > mutex_init(¶m->lock);
> > + param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
> > + if (!param->fault_param) {
> > + kfree(param);
> > + return -ENOMEM;
> > + }
> > + mutex_init(¶m->fault_param->lock);
> > + INIT_LIST_HEAD(¶m->fault_param->faults);
> > dev->iommu = param;
>
> This allocation seems pointless?
>
> If we always allocate the fault param then just don't make it a
> pointer in the first place.
>
> The appeal of allocation would be to save a few bytes in the common
> case that the driver doesn't support faulting.
>
> Which means the driver needs to make some call to enable faulting for
> a device. In this case I'd continue to lazy free on release like this
> patch does.
For instance allocate the fault_param in iopf_queue_add_device() which
is the only thing that needs it.
Actually probably just merge struct iopf_device_param and
iommu_fault_param ?
When you call iopf_queue_add_device() it enables page faulting mode,
does 1 additional allocation for all additional required per-device
memory and thats it.
Jason
On Thu, Jul 27, 2023 at 01:48:29PM +0800, Lu Baolu wrote:
> The individual iommu drivers report iommu 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().
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> drivers/iommu/iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4352a149a935..00309f66153b 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1381,7 +1381,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);
I don't get it, why not remove fparam->handler/data entirely in this
patch? There is no user once you do this change?
Jason
On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 4ba3bb692993..3e4ff984aa85 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -302,7 +302,15 @@ static int dev_iommu_get(struct device *dev)
> return -ENOMEM;
>
> mutex_init(¶m->lock);
> + param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
> + if (!param->fault_param) {
> + kfree(param);
> + return -ENOMEM;
> + }
> + mutex_init(¶m->fault_param->lock);
> + INIT_LIST_HEAD(¶m->fault_param->faults);
> dev->iommu = param;
This allocation seems pointless?
If we always allocate the fault param then just don't make it a
pointer in the first place.
The appeal of allocation would be to save a few bytes in the common
case that the driver doesn't support faulting.
Which means the driver needs to make some call to enable faulting for
a device. In this case I'd continue to lazy free on release like this
patch does.
Jason
On Thu, Jul 27, 2023 at 01:48:36PM +0800, Lu Baolu wrote:
> Add CONFIG_IOMMU_IOPF for page fault handling framework and select it
> from its real consumer. Move iopf function declaration from iommu-sva.h
> to iommu.h and remove iommu-sva.h as it's empty now.
>
> Signed-off-by: Lu Baolu <[email protected]>
> ---
> include/linux/iommu.h | 63 +++++++++++++++
> drivers/iommu/iommu-sva.h | 80 -------------------
> .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 1 -
> drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 1 -
> drivers/iommu/intel/iommu.c | 1 -
> drivers/iommu/intel/svm.c | 1 -
> drivers/iommu/iommu-sva.c | 3 +-
> drivers/iommu/iommu.c | 2 -
> drivers/iommu/Kconfig | 4 +
> drivers/iommu/Makefile | 3 +-
> drivers/iommu/intel/Kconfig | 1 +
> 11 files changed, 71 insertions(+), 89 deletions(-)
> delete mode 100644 drivers/iommu/iommu-sva.h
Reviewed-by: Jason Gunthorpe <[email protected]>
At some point it would be a nice touch to split iommu.h into the
consumer and iommu driver interfaces
Jason
On 2023/8/11 2:33, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:29PM +0800, Lu Baolu wrote:
>> The individual iommu drivers report iommu 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().
>>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> drivers/iommu/iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 4352a149a935..00309f66153b 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1381,7 +1381,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);
> Also fix the function signature at this point:
>
> int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
>
> It should not be 'void *cookie' anymore, it is just 'struct device *dev'
I have included this change in the subsequent cleanup patch.
Best regards,
baolu
On 2023/8/11 3:02, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:34PM +0800, Lu Baolu wrote:
>> The iopf_handler() function handles a fault_group for a SVA domain. Move
>> it to the right place.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> drivers/iommu/iommu-sva.h | 17 +++++++++++++
>> drivers/iommu/io-pgfault.c | 50 +++-----------------------------------
>> drivers/iommu/iommu-sva.c | 49 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 69 insertions(+), 47 deletions(-)
>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>> index 05c0fb2acbc4..ab42cfdd7636 100644
>> --- a/drivers/iommu/iommu-sva.c
>> +++ b/drivers/iommu/iommu-sva.c
>> @@ -219,3 +219,52 @@ void mm_pasid_drop(struct mm_struct *mm)
>
>> +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);
>> + }
>> +
>> + iopf_complete_group(group->dev, &group->last_fault, status);
>> + iopf_free_group(group);
>> +}
>
> Routing faults to domains is generic code, not SVA code.
You are right. This happens in the latter patch.
> SVA starts at domain->iopf_handler
>
> Jason
>
Best regards,
baolu
On 2023/8/11 2:22, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:29PM +0800, Lu Baolu wrote:
>> The individual iommu drivers report iommu 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().
>>
>> Signed-off-by: Lu Baolu<[email protected]>
>> ---
>> drivers/iommu/iommu.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 4352a149a935..00309f66153b 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1381,7 +1381,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);
> I don't get it, why not remove fparam->handler/data entirely in this
> patch? There is no user once you do this change?
It needs some cleanups elsewhere, so I put it in a separate patch.
Best regards,
baolu
On 2023/8/11 0:46, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
>> On 2023/8/10 0:59, Jason Gunthorpe wrote:
>>> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
>>>> 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]>
>>>> ---
>>>> include/linux/iommu.h | 70 +------------------------------------------
>>>> 1 file changed, 1 insertion(+), 69 deletions(-)
>>> Do we plan to bring this back in some form? A driver specific fault
>>> report via iommufd?
>> I can hardly see the possibility.
>>
>> The only necessary dma fault messages are the offending address and the
>> permissions. With these, the user space device model software knows that
>> "a DMA fault was generated when the IOMMU hardware tried to translate
>> the offending address with the given permissions".
>>
>> And then, the device model software will walk the page table and figure
>> out what is missed before injecting the vendor-specific fault messages
>> to the VM guest.
> Avoiding walking the page table sounds like a pretty big win if we
> could manage it by forwarding more event data..
Fair enough. We can discuss what kind of extra event data could be
included later when we have real code for dma fault forwarding support
in iommufd.
Best regards,
baolu
On 2023/8/11 3:07, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:35PM +0800, Lu Baolu wrote:
>> @@ -137,6 +136,16 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
>> return 0;
>> }
>>
>> + if (fault->prm.flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID)
>> + domain = iommu_get_domain_for_dev_pasid(dev, fault->prm.pasid, 0);
>> + else
>> + domain = iommu_get_domain_for_dev(dev);
>
> How does the lifetime work for this? What prevents UAF on domain?
Replied below.
>
>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>> index ab42cfdd7636..668f4c2bcf65 100644
>> --- a/drivers/iommu/iommu-sva.c
>> +++ b/drivers/iommu/iommu-sva.c
>> @@ -157,7 +157,7 @@ EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
>> /*
>> * I/O page fault handler for SVA
>> */
>> -enum iommu_page_response_code
>> +static enum iommu_page_response_code
>> iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>> {
>> vm_fault_t ret;
>> @@ -241,23 +241,16 @@ 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);
>> + status = iommu_sva_handle_iopf(&iopf->fault, group->data);
>> }
>>
>> iopf_complete_group(group->dev, &group->last_fault, status);
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 157a28a49473..535a36e3edc9 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3330,7 +3330,7 @@ struct iommu_domain *iommu_sva_domain_alloc(struct device *dev,
>> domain->type = IOMMU_DOMAIN_SVA;
>> mmgrab(mm);
>> domain->mm = mm;
>> - domain->iopf_handler = iommu_sva_handle_iopf;
>> + domain->iopf_handler = iommu_sva_handle_iopf_group;
>> domain->fault_data = mm;
>
> This also has lifetime problems on the mm.
>
> The domain should flow into the iommu_sva_handle_iopf() instead of the
> void *data.
Okay, but I still want to keep void *data as a private pointer of the
iopf consumer. For SVA, it's probably NULL.
>
> The SVA code can then just use domain->mm directly.
Yes.
>
> We need to document/figure out some how to ensure that the faults are
> all done processing before a fault enabled domain can be freed.
This has been documented in drivers/iommu/io-pgfault.c:
[...]
* Any valid page fault will be eventually routed to an iommu domain
and the
* page fault handler installed there will get called. The users of this
* handling framework should guarantee that the iommu domain could only be
* freed after the device has stopped generating page faults (or the iommu
* hardware has been set to block the page faults) and the pending page
faults
* have been flushed.
*
* Return: 0 on success and <0 on error.
*/
int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
[...]
> This patch would be better ordered before the prior patch.
Let me try this in the next version.
Best regards,
baolu
On 2023/8/11 2:46, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 03:20:07PM -0300, Jason Gunthorpe wrote:
>> On Thu, Jul 27, 2023 at 01:48:31PM +0800, Lu Baolu wrote:
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 4ba3bb692993..3e4ff984aa85 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -302,7 +302,15 @@ static int dev_iommu_get(struct device *dev)
>>> return -ENOMEM;
>>>
>>> mutex_init(¶m->lock);
>>> + param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
>>> + if (!param->fault_param) {
>>> + kfree(param);
>>> + return -ENOMEM;
>>> + }
>>> + mutex_init(¶m->fault_param->lock);
>>> + INIT_LIST_HEAD(¶m->fault_param->faults);
>>> dev->iommu = param;
>> This allocation seems pointless?
>>
>> If we always allocate the fault param then just don't make it a
>> pointer in the first place.
>>
>> The appeal of allocation would be to save a few bytes in the common
>> case that the driver doesn't support faulting.
>>
>> Which means the driver needs to make some call to enable faulting for
>> a device. In this case I'd continue to lazy free on release like this
>> patch does.
> For instance allocate the fault_param in iopf_queue_add_device() which
> is the only thing that needs it.
>
> Actually probably just merge struct iopf_device_param and
> iommu_fault_param ?
>
> When you call iopf_queue_add_device() it enables page faulting mode,
> does 1 additional allocation for all additional required per-device
> memory and thats it.
Agreed.
I originally kept the iommu_fault_param structure because I thought it
could also be used to store temporary data for unrecoverable faults,
just like the iopf_device_param structure is used for iopf. However, I
am not sure whether we actually need any temporary data for
unrecoverable fault forwarding, which doesn't require any response.
So, it's better to do like you suggested. It's cleaner and simpler.
Best regards,
baolu
On 2023/8/11 3:18, Jason Gunthorpe wrote:
> On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
>> To avoid open code everywhere.
>>
>> Signed-off-by: Lu Baolu <[email protected]>
>> ---
>> include/linux/iommu.h | 11 ++++++++++-
>> drivers/iommu/iommu.c | 20 ++++++++++++++++++--
>> 2 files changed, 28 insertions(+), 3 deletions(-)
>
> Seems like overkill at this point..
>
> Also, I think this is probably upside down.
>
> We want to create the domains as fault enabled in the first place.
>
> A fault enabled domain should never be attached to something that
> cannot support faults. It should also not support changing the fault
> handler while it exists.
>
> Thus at the creation point would be the time to supply the fault handler
> as part of requesting faulting.
>
> Taking an existing domain and making it faulting enabled is going to
> be really messy in all the corner cases.
Yes. Agreed.
>
> My advice (and Robin will probably hate me), is to define a new op:
>
> struct domain_alloc_paging_args {
> struct fault_handler *fault_handler;
> void *fault_data
> };
>
> struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
> domain_alloc_paging_args *args)
>
> The point would be to leave the majority of drivers using the
> simplified, core assisted, domain_alloc_paging() interface and they
> just don't have to touch any of this stuff at all.
>
> Obviously if handler is given then the domain will be initialized as
> faulting.
Perhaps we also need an internal helper for iommu drivers to check the
iopf capability of the domain.
Best regards,
baolu
> From: Baolu Lu <[email protected]>
> Sent: Friday, August 11, 2023 9:16 AM
>
> On 2023/8/11 0:46, Jason Gunthorpe wrote:
> > On Thu, Aug 10, 2023 at 10:27:21AM +0800, Baolu Lu wrote:
> >> On 2023/8/10 0:59, Jason Gunthorpe wrote:
> >>> On Thu, Jul 27, 2023 at 01:48:28PM +0800, Lu Baolu wrote:
> >>>> 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]>
> >>>> ---
> >>>> include/linux/iommu.h | 70 +------------------------------------------
> >>>> 1 file changed, 1 insertion(+), 69 deletions(-)
> >>> Do we plan to bring this back in some form? A driver specific fault
> >>> report via iommufd?
> >> I can hardly see the possibility.
> >>
> >> The only necessary dma fault messages are the offending address and the
> >> permissions. With these, the user space device model software knows
> that
> >> "a DMA fault was generated when the IOMMU hardware tried to translate
> >> the offending address with the given permissions".
> >>
> >> And then, the device model software will walk the page table and figure
> >> out what is missed before injecting the vendor-specific fault messages
> >> to the VM guest.
> > Avoiding walking the page table sounds like a pretty big win if we
> > could manage it by forwarding more event data..
>
> Fair enough. We can discuss what kind of extra event data could be
> included later when we have real code for dma fault forwarding support
> in iommufd.
>
I'm afraid there might be cases where VMM cannot rely on the hw event
data. e.g. if there is a misconfiguation in virtual context entry or ste then
vIOMMU may decide to unmap and leave IOAS empty to trigger
unrecoverable fault when any DMA comes. Then upon notification vIOMMU
needs to check virtual context entry/ste to decide the right error code when
injecting unrecoverable fault into the guest. Here the hw event data will be
about missing stage-2 mapping hence incorrect.
and this is not a performance critical path. ????
On 2023/8/11 0:47, Jason Gunthorpe wrote:
> On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
>>> From: Baolu Lu<[email protected]>
>>> Sent: Wednesday, August 9, 2023 6:41 PM
>>>
>>> On 2023/8/9 8:02, Tian, Kevin wrote:
>>>>> From: Jason Gunthorpe<[email protected]>
>>>>> Sent: Wednesday, August 9, 2023 2:43 AM
>>>>>
>>>>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>>>>
>>>>>> Is there plan to introduce further error in the future? otherwise this
>>> should
>>>>>> be void.
>>>>>>
>>>>>> btw the work queue is only for sva. If there is no other caller this can be
>>>>>> just kept in iommu-sva.c. No need to create a helper.
>>>>> I think more than just SVA will need a work queue context to process
>>>>> their faults.
>>>>>
>>>> then this series needs more work. Currently the abstraction doesn't
>>>> include workqueue in the common fault reporting layer.
>>> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
>>> the fault handling framework, but it lets the consumers choose to use
>>> it, or not to.
>>>
>> My understanding of Jason's comment was to make the workqueue the
>> default path instead of being opted by the consumer.. that is my 1st
>> impression but might be wrong...
> Yeah, that is one path. Do we have anyone that uses this that doesn't
> want the WQ? (actually who even uses this besides SVA?)
I am still confused. When we forward iopf's to user space through the
iommufd, we don't need to schedule a WQ, right? Or I misunderstood here?
Best regards,
baolu
On Fri, Aug 11, 2023 at 09:53:41AM +0800, Baolu Lu wrote:
> On 2023/8/11 0:47, Jason Gunthorpe wrote:
> > On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
> > > > From: Baolu Lu<[email protected]>
> > > > Sent: Wednesday, August 9, 2023 6:41 PM
> > > >
> > > > On 2023/8/9 8:02, Tian, Kevin wrote:
> > > > > > From: Jason Gunthorpe<[email protected]>
> > > > > > Sent: Wednesday, August 9, 2023 2:43 AM
> > > > > >
> > > > > > On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
> > > > > >
> > > > > > > Is there plan to introduce further error in the future? otherwise this
> > > > should
> > > > > > > be void.
> > > > > > >
> > > > > > > btw the work queue is only for sva. If there is no other caller this can be
> > > > > > > just kept in iommu-sva.c. No need to create a helper.
> > > > > > I think more than just SVA will need a work queue context to process
> > > > > > their faults.
> > > > > >
> > > > > then this series needs more work. Currently the abstraction doesn't
> > > > > include workqueue in the common fault reporting layer.
> > > > Do you mind elaborate a bit here? workqueue is a basic infrastructure in
> > > > the fault handling framework, but it lets the consumers choose to use
> > > > it, or not to.
> > > >
> > > My understanding of Jason's comment was to make the workqueue the
> > > default path instead of being opted by the consumer.. that is my 1st
> > > impression but might be wrong...
> > Yeah, that is one path. Do we have anyone that uses this that doesn't
> > want the WQ? (actually who even uses this besides SVA?)
>
> I am still confused. When we forward iopf's to user space through the
> iommufd, we don't need to schedule a WQ, right? Or I misunderstood
> here?
Yes, that could be true, iommufd could just queue it from the
interrupt context and trigger a wakeup.
But other iommufd modes would want to invoke hmm_range_fault() which
would need the work queue.
Jason
On Fri, Aug 11, 2023 at 10:21:20AM +0800, Baolu Lu wrote:
> > This also has lifetime problems on the mm.
> >
> > The domain should flow into the iommu_sva_handle_iopf() instead of the
> > void *data.
>
> Okay, but I still want to keep void *data as a private pointer of the
> iopf consumer. For SVA, it's probably NULL.
I'd rather give the iommu_domain some 'private' void * than pass
around weird pointers all over the place... That might be broadly
useful, eg iommufd could store the hwpt in there.
> > We need to document/figure out some how to ensure that the faults are
> > all done processing before a fault enabled domain can be freed.
>
> This has been documented in drivers/iommu/io-pgfault.c:
>
> [...]
> * Any valid page fault will be eventually routed to an iommu domain and the
> * page fault handler installed there will get called. The users of this
> * handling framework should guarantee that the iommu domain could only be
> * freed after the device has stopped generating page faults (or the iommu
> * hardware has been set to block the page faults) and the pending page
> faults
> * have been flushed.
> *
> * Return: 0 on success and <0 on error.
> */
> int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
> [...]
>
> > This patch would be better ordered before the prior patch.
>
> Let me try this in the next version.
Okay.. but can we have some debugging to enforce this maybe? Also add
a comment when we obtain the domain on this path to see the above
about the lifetime
Jason
On Fri, Aug 11, 2023 at 10:40:15AM +0800, Baolu Lu wrote:
> On 2023/8/11 3:18, Jason Gunthorpe wrote:
> > On Thu, Jul 27, 2023 at 01:48:37PM +0800, Lu Baolu wrote:
> > > To avoid open code everywhere.
> > >
> > > Signed-off-by: Lu Baolu <[email protected]>
> > > ---
> > > include/linux/iommu.h | 11 ++++++++++-
> > > drivers/iommu/iommu.c | 20 ++++++++++++++++++--
> > > 2 files changed, 28 insertions(+), 3 deletions(-)
> >
> > Seems like overkill at this point..
> >
> > Also, I think this is probably upside down.
> >
> > We want to create the domains as fault enabled in the first place.
> >
> > A fault enabled domain should never be attached to something that
> > cannot support faults. It should also not support changing the fault
> > handler while it exists.
> >
> > Thus at the creation point would be the time to supply the fault handler
> > as part of requesting faulting.
> >
> > Taking an existing domain and making it faulting enabled is going to
> > be really messy in all the corner cases.
>
> Yes. Agreed.
>
> >
> > My advice (and Robin will probably hate me), is to define a new op:
> >
> > struct domain_alloc_paging_args {
> > struct fault_handler *fault_handler;
> > void *fault_data
> > };
> >
> > struct iommu_domain *domain_alloc_paging2(struct device *dev, struct
> > domain_alloc_paging_args *args)
> >
> > The point would be to leave the majority of drivers using the
> > simplified, core assisted, domain_alloc_paging() interface and they
> > just don't have to touch any of this stuff at all.
> >
> > Obviously if handler is given then the domain will be initialized as
> > faulting.
>
> Perhaps we also need an internal helper for iommu drivers to check the
> iopf capability of the domain.
Yeah, maybe.
I've been mulling over this for a a bit here
Robin suggested to wrap everything in a arg to domain_alloc and build
a giant super multiplexor
I don't really like that because it makes it quite complicated for the
driver and multiplexor APIs are rarely good.
So for simple drivers I like the 'domain_alloc_paging' as the only op
they implement and it is obviously simple and hard to implement
wrong. Most drivers would do this.
We also need a:
struct iommu_domain *domain_alloc_sva(struct device *dev, struct mm_struct *mm)
So SVA can be fully setup at allocation time. SVA doesn't have any
legal permutations so it can be kept simple.
Then we need something to bundle:
- Dirty tracking yes/no
- The iommufd user space blob
- Fault handling yes/no
For complex drivers.
So maybe we should just have a 3rd option
// I'm a complex driver and many people checked that I implemented
// this right:
struct domain_alloc_args {
struct device *dev;
unsigned int flags; // For requesting dirty tracking
// alloc_domain_user interface
struct iommu_domain *parent;
void *user_data;
size_t user_len;
// Faulting
struct fault_handler *fault_handler;
void *fault_data;
};
struct iommu_domain *domain_alloc(struct domain_alloc_args *args);
?
IDK, multiplexor APIs are rarely good, but maybe this is the right
direction?
Jason
On 2023/8/11 21:29, Jason Gunthorpe wrote:
> On Fri, Aug 11, 2023 at 10:21:20AM +0800, Baolu Lu wrote:
>
>>> This also has lifetime problems on the mm.
>>>
>>> The domain should flow into the iommu_sva_handle_iopf() instead of the
>>> void *data.
>>
>> Okay, but I still want to keep void *data as a private pointer of the
>> iopf consumer. For SVA, it's probably NULL.
>
> I'd rather give the iommu_domain some 'private' void * than pass
> around weird pointers all over the place... That might be broadly
> useful, eg iommufd could store the hwpt in there.
Yes, you are right. With the private pointer stored in domain and domain
is passed to the iopf handler, there will be no need for a @data
parameter for iopf handler.
>
>>> We need to document/figure out some how to ensure that the faults are
>>> all done processing before a fault enabled domain can be freed.
>>
>> This has been documented in drivers/iommu/io-pgfault.c:
>>
>> [...]
>> * Any valid page fault will be eventually routed to an iommu domain and the
>> * page fault handler installed there will get called. The users of this
>> * handling framework should guarantee that the iommu domain could only be
>> * freed after the device has stopped generating page faults (or the iommu
>> * hardware has been set to block the page faults) and the pending page
>> faults
>> * have been flushed.
>> *
>> * Return: 0 on success and <0 on error.
>> */
>> int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
>> [...]
>>
>>> This patch would be better ordered before the prior patch.
>>
>> Let me try this in the next version.
>
> Okay.. but can we have some debugging to enforce this maybe? Also add
> a comment when we obtain the domain on this path to see the above
> about the lifetime
Yes. Sure.
Probably I will add a dev_warn() when get_domain for device or pasid
returns NULL...
In the paths of removing domain from device or pasid, I will add a check
for pending faults. Will trigger a dev_warn() if there is any pending
faults for the affected domain.
Best regards,
baolu
On 2023/8/11 21:27, Jason Gunthorpe wrote:
> On Fri, Aug 11, 2023 at 09:53:41AM +0800, Baolu Lu wrote:
>> On 2023/8/11 0:47, Jason Gunthorpe wrote:
>>> On Thu, Aug 10, 2023 at 02:35:40AM +0000, Tian, Kevin wrote:
>>>>> From: Baolu Lu<[email protected]>
>>>>> Sent: Wednesday, August 9, 2023 6:41 PM
>>>>>
>>>>> On 2023/8/9 8:02, Tian, Kevin wrote:
>>>>>>> From: Jason Gunthorpe<[email protected]>
>>>>>>> Sent: Wednesday, August 9, 2023 2:43 AM
>>>>>>>
>>>>>>> On Thu, Aug 03, 2023 at 08:16:47AM +0000, Tian, Kevin wrote:
>>>>>>>
>>>>>>>> Is there plan to introduce further error in the future? otherwise this
>>>>> should
>>>>>>>> be void.
>>>>>>>>
>>>>>>>> btw the work queue is only for sva. If there is no other caller this can be
>>>>>>>> just kept in iommu-sva.c. No need to create a helper.
>>>>>>> I think more than just SVA will need a work queue context to process
>>>>>>> their faults.
>>>>>>>
>>>>>> then this series needs more work. Currently the abstraction doesn't
>>>>>> include workqueue in the common fault reporting layer.
>>>>> Do you mind elaborate a bit here? workqueue is a basic infrastructure in
>>>>> the fault handling framework, but it lets the consumers choose to use
>>>>> it, or not to.
>>>>>
>>>> My understanding of Jason's comment was to make the workqueue the
>>>> default path instead of being opted by the consumer.. that is my 1st
>>>> impression but might be wrong...
>>> Yeah, that is one path. Do we have anyone that uses this that doesn't
>>> want the WQ? (actually who even uses this besides SVA?)
>> I am still confused. When we forward iopf's to user space through the
>> iommufd, we don't need to schedule a WQ, right? Or I misunderstood
>> here?
> Yes, that could be true, iommufd could just queue it from the
> interrupt context and trigger a wakeup.
>
> But other iommufd modes would want to invoke hmm_range_fault() which
> would need the work queue.
Yes. That's the reason why I added below helper
int iopf_queue_work(struct iopf_group *group, work_func_t func)
in the patch 09/12.
Best regards,
baolu