[PATCH 0/9] iommu: Prepare to deliver page faults to user space

[PATCH 0/9] iommu: Prepare to deliver page faults to user space

[Lu Baolu]

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:

- Removing include/uapi/linux/iommu.h.
- Removing iommu_[un]register_device_fault_handler().
- Making fault_param always available between iommu device probe and
  release.
- Using fault cookie to store temporary data used for processing faults.

This is also available at github [2]. I would appreciate your feedback
on this series.

[1] https://lore.kernel.org/linux-iommu/20230530053724.232765-1-baolu.lu@linux.intel.com/
[2] https://github.com/LuBaolu/intel-iommu/commits/preparatory-io-pgfault-delivery-v1

Best regards,
baolu

Lu Baolu (9):
  iommu: Move iommu fault data to linux/iommu.h
  iommu: Add device parameter to iopf handler
  iommu: Add common code to handle IO page faults
  iommu: Change the return value of dev_iommu_get()
  iommu: Make fault_param generic
  iommu: Remove iommu_[un]register_device_fault_handler()
  iommu: Add helper to set iopf handler for domain
  iommu: Add iommu page fault cookie helpers
  iommu: Use fault cookie to store iopf_param

 include/linux/iommu.h                         | 206 +++++++++++++++---
 drivers/iommu/iommu-sva.h                     |   8 +-
 include/uapi/linux/iommu.h                    | 161 --------------
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  13 +-
 drivers/iommu/intel/iommu.c                   |  18 +-
 drivers/iommu/io-pgfault.c                    |  55 +++--
 drivers/iommu/iommu-sva.c                     |   2 +-
 drivers/iommu/iommu.c                         | 199 ++++++++---------
 MAINTAINERS                                   |   1 -
 9 files changed, 320 insertions(+), 343 deletions(-)
 delete mode 100644 include/uapi/linux/iommu.h

-- 
2.34.1

[PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Lu Baolu]

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 <baolu.lu@linux.intel.com>
---
 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 d31642596675..0eb0fb852020 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 <jgg@nvidia.com>
-- 
2.34.1

RE: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> +
> +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;
> +};

Currently there is no handler for unrecoverable faults. 

Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
It returns -EOPNOTSUPP for unrecoverable faults.

In your series the common iommu_handle_io_pgfault() also only works
for PRQ.

It kinds of suggest above definitions are dead code, though arm-smmu-v3
does attempt to set them.

Probably it's right time to remove them.

In the future even if there might be a need of forwarding unrecoverable
faults to the user via iommufd, fault reasons reported by the physical
IOMMU doesn't make any sense to the guest. Presumably the vIOMMU
should walk guest configurations to set a fault reason which makes sense
from guest p.o.v.

Re: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Baolu Lu]

On 2023/7/11 14:05, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>> +
>> +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;
>> +};
> 
> Currently there is no handler for unrecoverable faults.
> 
> Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> It returns -EOPNOTSUPP for unrecoverable faults.
> 
> In your series the common iommu_handle_io_pgfault() also only works
> for PRQ.
> 
> It kinds of suggest above definitions are dead code, though arm-smmu-v3
> does attempt to set them.
> 
> Probably it's right time to remove them.
> 
> In the future even if there might be a need of forwarding unrecoverable
> faults to the user via iommufd, fault reasons reported by the physical
> IOMMU doesn't make any sense to the guest. Presumably the vIOMMU
> should walk guest configurations to set a fault reason which makes sense
> from guest p.o.v.

I am fine to remove unrecoverable faults data. But it was added by Jean,
so I'd like to know his opinion on this.

Best regards,
baolu

Re: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Jean-Philippe Brucker]

On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
> > > +/**
> > > + * 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;
> > > +};
> > 
> > Currently there is no handler for unrecoverable faults.

Yes those were meant for guest injection. Another goal was to replace
report_iommu_fault(), which also passes unrecoverable faults to host
drivers. Three drivers use that API:
* usnic just prints the error, which could be done by the IOMMU driver,
* remoteproc attempts to recover from the crash,
* msm attempts to handle the fault, or at least recover from the crash.

So the first one can be removed, and the others could move over to IOPF
(which may need to indicate that the fault is not actually recoverable by
the IOMMU) and return IOMMU_PAGE_RESP_INVALID.

> > 
> > Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> > It returns -EOPNOTSUPP for unrecoverable faults.
> > 
> > In your series the common iommu_handle_io_pgfault() also only works
> > for PRQ.
> > 
> > It kinds of suggest above definitions are dead code, though arm-smmu-v3
> > does attempt to set them.
> > 
> > Probably it's right time to remove them.
> > 
> > In the future even if there might be a need of forwarding unrecoverable
> > faults to the user via iommufd, fault reasons reported by the physical
> > IOMMU doesn't make any sense to the guest.

I guess it depends on the architecture?  The SMMU driver can report only
stage-1 faults through iommu_report_device_fault(), which are faults due
to a guest misconfiguring the tables assigned to it. At the moment
arm_smmu_handle_evt() only passes down stage-1 page table errors, the rest
is printed by the host.

> > Presumably the vIOMMU
> > should walk guest configurations to set a fault reason which makes sense
> > from guest p.o.v.
> 
> I am fine to remove unrecoverable faults data. But it was added by Jean,
> so I'd like to know his opinion on this.

Passing errors to the guest could be a useful diagnostics tool for
debugging, once the guest gets more controls over the IOMMU hardware, but
it doesn't have a purpose beyond that. It could be the only tool
available, though: to avoid a guest voluntarily flooding the host logs by
misconfiguring its tables, we may have to disable printing in the host
errors that come from guest misconfiguration, in which case there won't be
any diagnostics available for guest bugs.

For now I don't mind if they're removed, if there is an easy way to
reintroduce them later.

Thanks,
Jean

RE: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Tian, Kevin]

> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Wednesday, July 12, 2023 5:34 PM
> 
> On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
> > > > +/**
> > > > + * 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;
> > > > +};
> > >
> > > Currently there is no handler for unrecoverable faults.
> 
> Yes those were meant for guest injection. Another goal was to replace
> report_iommu_fault(), which also passes unrecoverable faults to host
> drivers. Three drivers use that API:
> * usnic just prints the error, which could be done by the IOMMU driver,
> * remoteproc attempts to recover from the crash,
> * msm attempts to handle the fault, or at least recover from the crash.

I was not aware of them. Thanks for pointing out.

> 
> So the first one can be removed, and the others could move over to IOPF
> (which may need to indicate that the fault is not actually recoverable by
> the IOMMU) and return IOMMU_PAGE_RESP_INVALID.

Yep, presumably we should have just one interface to handle fault.

> 
> > >
> > > Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
> > > It returns -EOPNOTSUPP for unrecoverable faults.
> > >
> > > In your series the common iommu_handle_io_pgfault() also only works
> > > for PRQ.
> > >
> > > It kinds of suggest above definitions are dead code, though arm-smmu-v3
> > > does attempt to set them.
> > >
> > > Probably it's right time to remove them.
> > >
> > > In the future even if there might be a need of forwarding unrecoverable
> > > faults to the user via iommufd, fault reasons reported by the physical
> > > IOMMU doesn't make any sense to the guest.
> 
> I guess it depends on the architecture?  The SMMU driver can report only
> stage-1 faults through iommu_report_device_fault(), which are faults due
> to a guest misconfiguring the tables assigned to it. At the moment
> arm_smmu_handle_evt() only passes down stage-1 page table errors, the
> rest
> is printed by the host.

In that case the kernel just needs to notify the vIOMMU an error happened
along with access permissions (r/w/e/p). vIOMMU can figure out the reason
itself by walking the stage-1 page table. Likely it will find the same reason
as host reports, but that sounds a clearer path in concept.

> 
> > > Presumably the vIOMMU
> > > should walk guest configurations to set a fault reason which makes sense
> > > from guest p.o.v.
> >
> > I am fine to remove unrecoverable faults data. But it was added by Jean,
> > so I'd like to know his opinion on this.
> 
> Passing errors to the guest could be a useful diagnostics tool for
> debugging, once the guest gets more controls over the IOMMU hardware,
> but
> it doesn't have a purpose beyond that. It could be the only tool
> available, though: to avoid a guest voluntarily flooding the host logs by
> misconfiguring its tables, we may have to disable printing in the host
> errors that come from guest misconfiguration, in which case there won't be
> any diagnostics available for guest bugs.
> 
> For now I don't mind if they're removed, if there is an easy way to
> reintroduce them later.
> 

We can keep whatever is required to satisfy the kernel drivers which
want to know the fault.

But for anything invented for old uAPI (e.g. fault_reason) let's remove
them and redefine later when introducing the support to the user.

Re: [PATCH 1/9] iommu: Move iommu fault data to linux/iommu.h

[Baolu Lu]

On 2023/7/13 11:22, Tian, Kevin wrote:
>> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
>> Sent: Wednesday, July 12, 2023 5:34 PM
>>
>> On Wed, Jul 12, 2023 at 10:07:22AM +0800, Baolu Lu wrote:
>>>>> +/**
>>>>> + * 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;
>>>>> +};
>>>>
>>>> Currently there is no handler for unrecoverable faults.
>>
>> Yes those were meant for guest injection. Another goal was to replace
>> report_iommu_fault(), which also passes unrecoverable faults to host
>> drivers. Three drivers use that API:
>> * usnic just prints the error, which could be done by the IOMMU driver,
>> * remoteproc attempts to recover from the crash,
>> * msm attempts to handle the fault, or at least recover from the crash.
> 
> I was not aware of them. Thanks for pointing out.
> 
>>
>> So the first one can be removed, and the others could move over to IOPF
>> (which may need to indicate that the fault is not actually recoverable by
>> the IOMMU) and return IOMMU_PAGE_RESP_INVALID.
> 
> Yep, presumably we should have just one interface to handle fault.
> 
>>
>>>>
>>>> Both Intel/ARM register iommu_queue_iopf() as the device fault handler.
>>>> It returns -EOPNOTSUPP for unrecoverable faults.
>>>>
>>>> In your series the common iommu_handle_io_pgfault() also only works
>>>> for PRQ.
>>>>
>>>> It kinds of suggest above definitions are dead code, though arm-smmu-v3
>>>> does attempt to set them.
>>>>
>>>> Probably it's right time to remove them.
>>>>
>>>> In the future even if there might be a need of forwarding unrecoverable
>>>> faults to the user via iommufd, fault reasons reported by the physical
>>>> IOMMU doesn't make any sense to the guest.
>>
>> I guess it depends on the architecture?  The SMMU driver can report only
>> stage-1 faults through iommu_report_device_fault(), which are faults due
>> to a guest misconfiguring the tables assigned to it. At the moment
>> arm_smmu_handle_evt() only passes down stage-1 page table errors, the
>> rest
>> is printed by the host.
> 
> In that case the kernel just needs to notify the vIOMMU an error happened
> along with access permissions (r/w/e/p). vIOMMU can figure out the reason
> itself by walking the stage-1 page table. Likely it will find the same reason
> as host reports, but that sounds a clearer path in concept.
> 
>>
>>>> Presumably the vIOMMU
>>>> should walk guest configurations to set a fault reason which makes sense
>>>> from guest p.o.v.
>>>
>>> I am fine to remove unrecoverable faults data. But it was added by Jean,
>>> so I'd like to know his opinion on this.
>>
>> Passing errors to the guest could be a useful diagnostics tool for
>> debugging, once the guest gets more controls over the IOMMU hardware,
>> but
>> it doesn't have a purpose beyond that. It could be the only tool
>> available, though: to avoid a guest voluntarily flooding the host logs by
>> misconfiguring its tables, we may have to disable printing in the host
>> errors that come from guest misconfiguration, in which case there won't be
>> any diagnostics available for guest bugs.
>>
>> For now I don't mind if they're removed, if there is an easy way to
>> reintroduce them later.
>>
> 
> We can keep whatever is required to satisfy the kernel drivers which
> want to know the fault.
> 
> But for anything invented for old uAPI (e.g. fault_reason) let's remove
> them and redefine later when introducing the support to the user.

Okay, I will do this in the next version.

Best regards,
baolu

[PATCH 2/9] iommu: Add device parameter to iopf handler

[Lu Baolu]

Add the device parameter to the iopf handler so that it can know which
device this fault was generated.

This is necessary for use cases such as delivering IO page faults to user
space. The IOMMUFD layer needs to be able to lookup the device id of a
fault and route it together with the fault message to the user space.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      | 1 +
 drivers/iommu/iommu-sva.h  | 4 ++--
 drivers/iommu/io-pgfault.c | 2 +-
 drivers/iommu/iommu-sva.c  | 2 +-
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 0eb0fb852020..a00fb43b5e73 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -249,6 +249,7 @@ struct iommu_domain {
 	struct iommu_domain_geometry geometry;
 	struct iommu_dma_cookie *iova_cookie;
 	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault *fault,
+						      struct device *dev,
 						      void *data);
 	void *fault_data;
 	union {
diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
index 54946b5a7caf..c848661c4e20 100644
--- a/drivers/iommu/iommu-sva.h
+++ b/drivers/iommu/iommu-sva.h
@@ -23,7 +23,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
 void iopf_queue_free(struct iopf_queue *queue);
 int iopf_queue_discard_partial(struct iopf_queue *queue);
 enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
+iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev, void *data);
 
 #else /* CONFIG_IOMMU_SVA */
 static inline int iommu_queue_iopf(struct iommu_fault *fault, void *cookie)
@@ -63,7 +63,7 @@ static inline int iopf_queue_discard_partial(struct iopf_queue *queue)
 }
 
 static inline enum iommu_page_response_code
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev, void *data)
 {
 	return IOMMU_PAGE_RESP_INVALID;
 }
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index e5b8b9110c13..fa604e1b5c5c 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -88,7 +88,7 @@ static void iopf_handler(struct work_struct *work)
 		 * faults in the group if there is an error.
 		 */
 		if (status == IOMMU_PAGE_RESP_SUCCESS)
-			status = domain->iopf_handler(&iopf->fault,
+			status = domain->iopf_handler(&iopf->fault, group->dev,
 						      domain->fault_data);
 
 		if (!(iopf->fault.prm.flags &
diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
index 3ebd4b6586b3..14766a2b61af 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
-iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
+iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev, void *data)
 {
 	vm_fault_t ret;
 	struct vm_area_struct *vma;
-- 
2.34.1

Re: [PATCH 2/9] iommu: Add device parameter to iopf handler

[Jacob Pan]

Hi BaoLu,

On Tue, 11 Jul 2023 09:06:35 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Add the device parameter to the iopf handler so that it can know which
> device this fault was generated.
> 
> This is necessary for use cases such as delivering IO page faults to user
> space. The IOMMUFD layer needs to be able to lookup the device id of a
> fault and route it together with the fault message to the user space.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h      | 1 +
>  drivers/iommu/iommu-sva.h  | 4 ++--
>  drivers/iommu/io-pgfault.c | 2 +-
>  drivers/iommu/iommu-sva.c  | 2 +-
>  4 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 0eb0fb852020..a00fb43b5e73 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -249,6 +249,7 @@ struct iommu_domain {
>  	struct iommu_domain_geometry geometry;
>  	struct iommu_dma_cookie *iova_cookie;
>  	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault
> *fault,
> +						      struct device *dev,
>  						      void *data);
>  	void *fault_data;
>  	union {
> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> index 54946b5a7caf..c848661c4e20 100644
> --- a/drivers/iommu/iommu-sva.h
> +++ b/drivers/iommu/iommu-sva.h
> @@ -23,7 +23,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
>  void iopf_queue_free(struct iopf_queue *queue);
>  int iopf_queue_discard_partial(struct iopf_queue *queue);
>  enum iommu_page_response_code
> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> void *data); 
>  #else /* CONFIG_IOMMU_SVA */
>  static inline int iommu_queue_iopf(struct iommu_fault *fault, void
> *cookie) @@ -63,7 +63,7 @@ static inline int
> iopf_queue_discard_partial(struct iopf_queue *queue) }
>  
>  static inline enum iommu_page_response_code
> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> void *data) {
>  	return IOMMU_PAGE_RESP_INVALID;
>  }
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index e5b8b9110c13..fa604e1b5c5c 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -88,7 +88,7 @@ static void iopf_handler(struct work_struct *work)
>  		 * faults in the group if there is an error.
>  		 */
>  		if (status == IOMMU_PAGE_RESP_SUCCESS)
> -			status = domain->iopf_handler(&iopf->fault,
> +			status = domain->iopf_handler(&iopf->fault,
> group->dev, domain->fault_data);
>  
>  		if (!(iopf->fault.prm.flags &
> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> index 3ebd4b6586b3..14766a2b61af 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
> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
dev has no use for sva handler, right? mark them __always_unused?

> void *data) {
>  	vm_fault_t ret;
>  	struct vm_area_struct *vma;


Thanks,

Jacob

Re: [PATCH 2/9] iommu: Add device parameter to iopf handler

[Baolu Lu]

On 2023/7/12 1:26, Jacob Pan wrote:
> Hi BaoLu,

Hi Jacob,

> 
> On Tue, 11 Jul 2023 09:06:35 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> Add the device parameter to the iopf handler so that it can know which
>> device this fault was generated.
>>
>> This is necessary for use cases such as delivering IO page faults to user
>> space. The IOMMUFD layer needs to be able to lookup the device id of a
>> fault and route it together with the fault message to the user space.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h      | 1 +
>>   drivers/iommu/iommu-sva.h  | 4 ++--
>>   drivers/iommu/io-pgfault.c | 2 +-
>>   drivers/iommu/iommu-sva.c  | 2 +-
>>   4 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 0eb0fb852020..a00fb43b5e73 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -249,6 +249,7 @@ struct iommu_domain {
>>   	struct iommu_domain_geometry geometry;
>>   	struct iommu_dma_cookie *iova_cookie;
>>   	enum iommu_page_response_code (*iopf_handler)(struct iommu_fault
>> *fault,
>> +						      struct device *dev,
>>   						      void *data);
>>   	void *fault_data;
>>   	union {
>> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
>> index 54946b5a7caf..c848661c4e20 100644
>> --- a/drivers/iommu/iommu-sva.h
>> +++ b/drivers/iommu/iommu-sva.h
>> @@ -23,7 +23,7 @@ struct iopf_queue *iopf_queue_alloc(const char *name);
>>   void iopf_queue_free(struct iopf_queue *queue);
>>   int iopf_queue_discard_partial(struct iopf_queue *queue);
>>   enum iommu_page_response_code
>> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
>> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
>> void *data);
>>   #else /* CONFIG_IOMMU_SVA */
>>   static inline int iommu_queue_iopf(struct iommu_fault *fault, void
>> *cookie) @@ -63,7 +63,7 @@ static inline int
>> iopf_queue_discard_partial(struct iopf_queue *queue) }
>>   
>>   static inline enum iommu_page_response_code
>> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
>> void *data) {
>>   	return IOMMU_PAGE_RESP_INVALID;
>>   }
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index e5b8b9110c13..fa604e1b5c5c 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -88,7 +88,7 @@ static void iopf_handler(struct work_struct *work)
>>   		 * faults in the group if there is an error.
>>   		 */
>>   		if (status == IOMMU_PAGE_RESP_SUCCESS)
>> -			status = domain->iopf_handler(&iopf->fault,
>> +			status = domain->iopf_handler(&iopf->fault,
>> group->dev, domain->fault_data);
>>   
>>   		if (!(iopf->fault.prm.flags &
>> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
>> index 3ebd4b6586b3..14766a2b61af 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
>> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
>> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> dev has no use for sva handler, right? mark them __always_unused?

My understanding is that __always_unused attribute in Linux kernel code
marks a variable or function as unused. It implies that the compiler is
free to optimize the variable or function away.

If my understanding is correct, then it may not be suitable here.

Best regards,
baolu

Re: [PATCH 2/9] iommu: Add device parameter to iopf handler

[Jacob Pan]

Hi Baolu,

On Wed, 12 Jul 2023 10:16:11 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 2023/7/12 1:26, Jacob Pan wrote:
> > Hi BaoLu,  
> 
> Hi Jacob,
> 
> > 
> > On Tue, 11 Jul 2023 09:06:35 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> > wrote:
> >   
> >> Add the device parameter to the iopf handler so that it can know which
> >> device this fault was generated.
> >>
> >> This is necessary for use cases such as delivering IO page faults to
> >> user space. The IOMMUFD layer needs to be able to lookup the device id
> >> of a fault and route it together with the fault message to the user
> >> space.
> >>
> >> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >> ---
> >>   include/linux/iommu.h      | 1 +
> >>   drivers/iommu/iommu-sva.h  | 4 ++--
> >>   drivers/iommu/io-pgfault.c | 2 +-
> >>   drivers/iommu/iommu-sva.c  | 2 +-
> >>   4 files changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> >> index 0eb0fb852020..a00fb43b5e73 100644
> >> --- a/include/linux/iommu.h
> >> +++ b/include/linux/iommu.h
> >> @@ -249,6 +249,7 @@ struct iommu_domain {
> >>   	struct iommu_domain_geometry geometry;
> >>   	struct iommu_dma_cookie *iova_cookie;
> >>   	enum iommu_page_response_code (*iopf_handler)(struct
> >> iommu_fault *fault,
> >> +						      struct device
> >> *dev, void *data);
> >>   	void *fault_data;
> >>   	union {
> >> diff --git a/drivers/iommu/iommu-sva.h b/drivers/iommu/iommu-sva.h
> >> index 54946b5a7caf..c848661c4e20 100644
> >> --- a/drivers/iommu/iommu-sva.h
> >> +++ b/drivers/iommu/iommu-sva.h
> >> @@ -23,7 +23,7 @@ struct iopf_queue *iopf_queue_alloc(const char
> >> *name); void iopf_queue_free(struct iopf_queue *queue);
> >>   int iopf_queue_discard_partial(struct iopf_queue *queue);
> >>   enum iommu_page_response_code
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data);
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> >> void *data);
> >>   #else /* CONFIG_IOMMU_SVA */
> >>   static inline int iommu_queue_iopf(struct iommu_fault *fault, void
> >> *cookie) @@ -63,7 +63,7 @@ static inline int
> >> iopf_queue_discard_partial(struct iopf_queue *queue) }
> >>   
> >>   static inline enum iommu_page_response_code
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,
> >> void *data) {
> >>   	return IOMMU_PAGE_RESP_INVALID;
> >>   }
> >> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> >> index e5b8b9110c13..fa604e1b5c5c 100644
> >> --- a/drivers/iommu/io-pgfault.c
> >> +++ b/drivers/iommu/io-pgfault.c
> >> @@ -88,7 +88,7 @@ static void iopf_handler(struct work_struct *work)
> >>   		 * faults in the group if there is an error.
> >>   		 */
> >>   		if (status == IOMMU_PAGE_RESP_SUCCESS)
> >> -			status = domain->iopf_handler(&iopf->fault,
> >> +			status = domain->iopf_handler(&iopf->fault,
> >> group->dev, domain->fault_data);
> >>   
> >>   		if (!(iopf->fault.prm.flags &
> >> diff --git a/drivers/iommu/iommu-sva.c b/drivers/iommu/iommu-sva.c
> >> index 3ebd4b6586b3..14766a2b61af 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
> >> -iommu_sva_handle_iopf(struct iommu_fault *fault, void *data)
> >> +iommu_sva_handle_iopf(struct iommu_fault *fault, struct device *dev,  
> > dev has no use for sva handler, right? mark them __always_unused?  
> 
> My understanding is that __always_unused attribute in Linux kernel code
> marks a variable or function as unused. It implies that the compiler is
> free to optimize the variable or function away.
that is my understanding as well, I meant
iommu_sva_handle_iopf(struct iommu_fault *fault, struct device
__always_unused *dev,  

I tested compile w/ and w/o __always_unused, seems no difference but it
makes the code clear that dev is not used here.


Thanks,

Jacob

[PATCH 3/9] iommu: Add common code to handle IO page faults

[Lu Baolu]

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 the device
fault handler with a static common code to avoid unnecessary code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index da340f11c5f5..41328f03e8b4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
 
+static int iommu_handle_io_pgfault(struct device *dev,
+				   struct iommu_fault *fault)
+{
+	struct iommu_domain *domain;
+
+	if (fault->type != IOMMU_FAULT_PAGE_REQ)
+		return -EINVAL;
+
+	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)
+		return -ENODEV;
+
+	if (domain->iopf_handler == iommu_sva_handle_iopf)
+		return iommu_queue_iopf(fault, dev);
+
+	return domain->iopf_handler(fault, dev, domain->fault_data);
+}
+
 /**
  * iommu_report_device_fault() - Report fault event to device driver
  * @dev: the device
@@ -1320,7 +1342,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_handle_io_pgfault(dev, &evt->fault);
 	if (ret && evt_pending) {
 		mutex_lock(&fparam->lock);
 		list_del(&evt_pending->list);
-- 
2.34.1

RE: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> 
> +static int iommu_handle_io_pgfault(struct device *dev,
> +				   struct iommu_fault *fault)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (fault->type != IOMMU_FAULT_PAGE_REQ)
> +		return -EINVAL;
> +
> +	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)
> +		return -ENODEV;
> +
> +	if (domain->iopf_handler == iommu_sva_handle_iopf)
> +		return iommu_queue_iopf(fault, dev);

You can avoid the special check by directly making iommu_queue_iopf
as the iopf_handler for sva domain.

> +
> +	return domain->iopf_handler(fault, dev, domain->fault_data);
> +}

btw is there value of moving the group handling logic from
iommu_queue_iopf() to this common function?

I wonder whether there is any correctness issue if not forwarding
partial request to iommufd. If not this can also help reduce
notifications to the user until the group is ready.

Re: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Baolu Lu]

On 2023/7/11 14:12, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> +static int iommu_handle_io_pgfault(struct device *dev,
>> +				   struct iommu_fault *fault)
>> +{
>> +	struct iommu_domain *domain;
>> +
>> +	if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> +		return -EINVAL;
>> +
>> +	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)
>> +		return -ENODEV;
>> +
>> +	if (domain->iopf_handler == iommu_sva_handle_iopf)
>> +		return iommu_queue_iopf(fault, dev);
> 
> You can avoid the special check by directly making iommu_queue_iopf
> as the iopf_handler for sva domain.

Yeah, good catch!

> 
>> +
>> +	return domain->iopf_handler(fault, dev, domain->fault_data);
>> +}
> 
> btw is there value of moving the group handling logic from
> iommu_queue_iopf() to this common function?
> 
> I wonder whether there is any correctness issue if not forwarding
> partial request to iommufd. If not this can also help reduce
> notifications to the user until the group is ready.

I don't think there's any correctness issue. But it should be better if
we can inject the page faults to vm guests as soon as possible. There's
no requirement to put page requests to vIOMMU's hardware page request
queue at the granularity of a fault group. Thoughts?

Best regards,
baolu

Re: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Jean-Philippe Brucker]

On Wed, Jul 12, 2023 at 10:32:13AM +0800, Baolu Lu wrote:
> > btw is there value of moving the group handling logic from
> > iommu_queue_iopf() to this common function?
> > 
> > I wonder whether there is any correctness issue if not forwarding
> > partial request to iommufd. If not this can also help reduce
> > notifications to the user until the group is ready.
> 
> I don't think there's any correctness issue. But it should be better if
> we can inject the page faults to vm guests as soon as possible. There's
> no requirement to put page requests to vIOMMU's hardware page request
> queue at the granularity of a fault group. Thoughts?

Not sure I understand you correctly, but we can't inject partial fault
groups: if the HW PRI queue overflows, the last fault in a group may be
lost, so the non-last faults in that group already injected won't be
completed (until PRGI reuse), leaking PRI request credits and guest
resources.

Thanks,
Jean

Re: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Baolu Lu]

On 2023/7/12 17:45, Jean-Philippe Brucker wrote:
> On Wed, Jul 12, 2023 at 10:32:13AM +0800, Baolu Lu wrote:
>>> btw is there value of moving the group handling logic from
>>> iommu_queue_iopf() to this common function?
>>>
>>> I wonder whether there is any correctness issue if not forwarding
>>> partial request to iommufd. If not this can also help reduce
>>> notifications to the user until the group is ready.
>>
>> I don't think there's any correctness issue. But it should be better if
>> we can inject the page faults to vm guests as soon as possible. There's
>> no requirement to put page requests to vIOMMU's hardware page request
>> queue at the granularity of a fault group. Thoughts?
> 
> Not sure I understand you correctly, but we can't inject partial fault
> groups: if the HW PRI queue overflows, the last fault in a group may be
> lost, so the non-last faults in that group already injected won't be
> completed (until PRGI reuse), leaking PRI request credits and guest
> resources.

Yeah, that's how vIOMMU injects the faults. On host/hypervisor side, my
understanding is that faults should be uploaded as soon as possible.

Best regards,
baolu

Re: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Jacob Pan]

Hi Lu,

On Tue, 11 Jul 2023 09:06:36 +0800, Lu Baolu <baolu.lu@linux.intel.com>
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 the device
> fault handler with a static common code to avoid unnecessary code.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index da340f11c5f5..41328f03e8b4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct
> device *dev) }
>  EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>  
> +static int iommu_handle_io_pgfault(struct device *dev,
> +				   struct iommu_fault *fault)
> +{
> +	struct iommu_domain *domain;
> +
> +	if (fault->type != IOMMU_FAULT_PAGE_REQ)
> +		return -EINVAL;
> +
> +	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);
we don't support IOPF w/o PASID yet, right?

> +
> +	if (!domain || !domain->iopf_handler)
> +		return -ENODEV;
> +
> +	if (domain->iopf_handler == iommu_sva_handle_iopf)
> +		return iommu_queue_iopf(fault, dev);
Just wondering why have a special treatment for SVA domain. Can
iommu_queue_iopf() be used as SVA domain->iopf_handler?

> +
> +	return domain->iopf_handler(fault, dev, domain->fault_data);
> +}
> +
>  /**
>   * iommu_report_device_fault() - Report fault event to device driver
>   * @dev: the device
> @@ -1320,7 +1342,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_handle_io_pgfault(dev, &evt->fault);
>  	if (ret && evt_pending) {
>  		mutex_lock(&fparam->lock);
>  		list_del(&evt_pending->list);


Thanks,

Jacob

Re: [PATCH 3/9] iommu: Add common code to handle IO page faults

[Baolu Lu]

On 2023/7/12 4:50, Jacob Pan wrote:
> Hi Lu,
> 
> On Tue, 11 Jul 2023 09:06:36 +0800, Lu Baolu <baolu.lu@linux.intel.com>
> 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 the device
>> fault handler with a static common code to avoid unnecessary code.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index da340f11c5f5..41328f03e8b4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -1278,6 +1278,28 @@ int iommu_unregister_device_fault_handler(struct
>> device *dev) }
>>   EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>>   
>> +static int iommu_handle_io_pgfault(struct device *dev,
>> +				   struct iommu_fault *fault)
>> +{
>> +	struct iommu_domain *domain;
>> +
>> +	if (fault->type != IOMMU_FAULT_PAGE_REQ)
>> +		return -EINVAL;
>> +
>> +	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);
> we don't support IOPF w/o PASID yet, right?

It's the individual driver that decides whether iopf w/o pasid is
supported or not. The iommu core doesn't need to make such assumption.

> 
>> +
>> +	if (!domain || !domain->iopf_handler)
>> +		return -ENODEV;
>> +
>> +	if (domain->iopf_handler == iommu_sva_handle_iopf)
>> +		return iommu_queue_iopf(fault, dev);
> Just wondering why have a special treatment for SVA domain. Can
> iommu_queue_iopf() be used as SVA domain->iopf_handler?

Yes. I will make this change according to Kevin's suggestion in this
thread.

> 
>> +
>> +	return domain->iopf_handler(fault, dev, domain->fault_data);
>> +}
>> +
>>   /**
>>    * iommu_report_device_fault() - Report fault event to device driver
>>    * @dev: the device
>> @@ -1320,7 +1342,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_handle_io_pgfault(dev, &evt->fault);
>>   	if (ret && evt_pending) {
>>   		mutex_lock(&fparam->lock);
>>   		list_del(&evt_pending->list);
> 
> 
> Thanks,
> 
> Jacob
> 

Best regards,
baolu

[PATCH 4/9] iommu: Change the return value of dev_iommu_get()

[Lu Baolu]

Make dev_iommu_get() return 0 for success and error numbers for failure.
This will make the code neat and readable. No functionality changes.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 41328f03e8b4..65895b987e22 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -287,20 +287,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(&param->lock);
 	dev->iommu = param;
-	return param;
+	return 0;
 }
 
 static void dev_iommu_free(struct device *dev)
@@ -351,10 +351,9 @@ static int __iommu_probe_device(struct device *dev, struct list_head *group_list
 	 * but for now enforcing a simple global ordering is fine.
 	 */
 	mutex_lock(&iommu_probe_device_lock);
-	if (!dev_iommu_get(dev)) {
-		ret = -ENOMEM;
+	ret = dev_iommu_get(dev);
+	if (ret)
 		goto err_unlock;
-	}
 
 	if (!try_module_get(ops->owner)) {
 		ret = -EINVAL;
@@ -2751,12 +2750,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

Re: [PATCH 4/9] iommu: Change the return value of dev_iommu_get()

[Jacob Pan]

Hi BaoLu,

On Tue, 11 Jul 2023 09:06:37 +0800, Lu Baolu <baolu.lu@linux.intel.com>
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.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 41328f03e8b4..65895b987e22 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -287,20 +287,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(&param->lock);
>  	dev->iommu = param;
> -	return param;
> +	return 0;
>  }
>  
>  static void dev_iommu_free(struct device *dev)
> @@ -351,10 +351,9 @@ static int __iommu_probe_device(struct device *dev,
> struct list_head *group_list
>  	 * but for now enforcing a simple global ordering is fine.
>  	 */
>  	mutex_lock(&iommu_probe_device_lock);
> -	if (!dev_iommu_get(dev)) {
> -		ret = -ENOMEM;
> +	ret = dev_iommu_get(dev);
> +	if (ret)
>  		goto err_unlock;
> -	}
>  
>  	if (!try_module_get(ops->owner)) {
>  		ret = -EINVAL;
> @@ -2751,12 +2750,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);

Reviewed-by: Jacob Pan <jacob.jun.pan@linux.intel.com>

Thanks,

Jacob

[PATCH 5/9] iommu: Make fault_param generic

[Lu Baolu]

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 should be 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 <baolu.lu@linux.intel.com>
---
 .../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 5c8c5cdc36cf..22e43db20252 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4594,23 +4594,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)
@@ -4637,7 +4628,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 65895b987e22..8d1f0935ea71 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
 		return -ENOMEM;
 
 	mutex_init(&param->lock);
+	param->fault_param = kzalloc(sizeof(*param->fault_param), GFP_KERNEL);
+	if (!param->fault_param) {
+		kfree(param);
+		return -ENOMEM;
+	}
+	mutex_init(&param->fault_param->lock);
+	INIT_LIST_HEAD(&param->fault_param->faults);
 	dev->iommu = param;
+
 	return 0;
 }
 
@@ -312,6 +320,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(&param->fault_param->faults));
+	kfree(param->fault_param);
 	kfree(param);
 }
 
-- 
2.34.1

RE: [PATCH 5/9] iommu: Make fault_param generic

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> 
> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>  		return -ENOMEM;
> 
>  	mutex_init(&param->lock);
> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
> GFP_KERNEL);
> +	if (!param->fault_param) {
> +		kfree(param);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&param->fault_param->lock);
> +	INIT_LIST_HEAD(&param->fault_param->faults);
>  	dev->iommu = param;
> +
>  	return 0;
>  }

Upon above changes is it slightly cleaner to call it dev_iommu_init()
to better pair with dev_iommu_free()?

Re: [PATCH 5/9] iommu: Make fault_param generic

[Baolu Lu]

On 2023/7/11 14:14, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>>   		return -ENOMEM;
>>
>>   	mutex_init(&param->lock);
>> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
>> +	if (!param->fault_param) {
>> +		kfree(param);
>> +		return -ENOMEM;
>> +	}
>> +	mutex_init(&param->fault_param->lock);
>> +	INIT_LIST_HEAD(&param->fault_param->faults);
>>   	dev->iommu = param;
>> +
>>   	return 0;
>>   }
> 
> Upon above changes is it slightly cleaner to call it dev_iommu_init()
> to better pair with dev_iommu_free()?

dev_iommu_init() was introduced in Jason's series. It's not landed yet.

https://lore.kernel.org/linux-iommu/0-v3-328044aa278c+45e49-iommu_probe_jgg@nvidia.com/

Sure. Should refine this code after above patches are landed.

Best regards,
baolu

Re: [PATCH 5/9] iommu: Make fault_param generic

[Jacob Pan]

Hi BaoLu,

On Tue, 11 Jul 2023 09:06:38 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> 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 should be 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 <baolu.lu@linux.intel.com>
> ---
>  .../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 5c8c5cdc36cf..22e43db20252 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4594,23 +4594,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)
> @@ -4637,7 +4628,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 65895b987e22..8d1f0935ea71 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>  		return -ENOMEM;
>  
>  	mutex_init(&param->lock);
> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
> GFP_KERNEL);
since fault_param is _always_ allocated/freed along with param, can we merge
into one allocation? i.e.
 struct dev_iommu {
        struct mutex lock;
-       struct iommu_fault_param        *fault_param;
+       struct iommu_fault_param        fault_param;


> +	if (!param->fault_param) {
> +		kfree(param);
> +		return -ENOMEM;
> +	}
> +	mutex_init(&param->fault_param->lock);
> +	INIT_LIST_HEAD(&param->fault_param->faults);
>  	dev->iommu = param;
> +
>  	return 0;
>  }
>  
> @@ -312,6 +320,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(&param->fault_param->faults));
> +	kfree(param->fault_param);
>  	kfree(param);
>  }
>  


Thanks,

Jacob

Re: [PATCH 5/9] iommu: Make fault_param generic

[Baolu Lu]

On 2023/7/12 5:31, Jacob Pan wrote:
> On Tue, 11 Jul 2023 09:06:38 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> 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 should be 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<baolu.lu@linux.intel.com>
>> ---
>>   .../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 5c8c5cdc36cf..22e43db20252 100644
>> --- a/drivers/iommu/intel/iommu.c
>> +++ b/drivers/iommu/intel/iommu.c
>> @@ -4594,23 +4594,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)
>> @@ -4637,7 +4628,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 65895b987e22..8d1f0935ea71 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -299,7 +299,15 @@ static int dev_iommu_get(struct device *dev)
>>   		return -ENOMEM;
>>   
>>   	mutex_init(&param->lock);
>> +	param->fault_param = kzalloc(sizeof(*param->fault_param),
>> GFP_KERNEL);
> since fault_param is_always_  allocated/freed along with param, can we merge
> into one allocation? i.e.
>   struct dev_iommu {
>          struct mutex lock;
> -       struct iommu_fault_param        *fault_param;
> +       struct iommu_fault_param        fault_param;

I am not pretty sure about the change in this patch. It's a simple-and-
stupid way. But it also wastes memory for devices that have not pri-
capable domain attached.

So probably it's better to allocate fault_param at the place where a
real pri-capable domain is attached to the device?

Best regards,
baolu

[PATCH 6/9] iommu: Remove iommu_[un]register_device_fault_handler()

[Lu Baolu]

This pair of interfaces are not used anywhere in the tree. Remove it to
avoid dead code.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 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 a00fb43b5e73..f10be6680f8a 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -198,7 +198,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    */
@@ -538,14 +537,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;
 };
@@ -669,11 +664,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);
@@ -1055,19 +1045,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 c848661c4e20..7b26224c536b 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, struct device *dev, 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 fa604e1b5c5c..1749e0869f2e 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(&param->lock);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 8d1f0935ea71..fc5e9698b35c 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1199,98 +1199,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(&param->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(&param->fault_param->lock);
-	INIT_LIST_HEAD(&param->fault_param->faults);
-
-done_unlock:
-	mutex_unlock(&param->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(iommu_register_device_fault_handler);
-
-/**
- * iommu_unregister_device_fault_handler() - Unregister the device fault handler
- * @dev: the device
- *
- * Remove the device fault handler installed with
- * iommu_register_device_fault_handler().
- *
- * Return 0 on success, or an error.
- */
-int iommu_unregister_device_fault_handler(struct device *dev)
-{
-	struct dev_iommu *param = dev->iommu;
-	int ret = 0;
-
-	if (!param)
-		return -EINVAL;
-
-	mutex_lock(&param->lock);
-
-	if (!param->fault_param)
-		goto unlock;
-
-	/* we cannot unregister handler if there are pending faults */
-	if (!list_empty(&param->fault_param->faults)) {
-		ret = -EBUSY;
-		goto unlock;
-	}
-
-	kfree(param->fault_param);
-	param->fault_param = NULL;
-	put_device(dev);
-unlock:
-	mutex_unlock(&param->lock);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
-
 static int iommu_handle_io_pgfault(struct device *dev,
 				   struct iommu_fault *fault)
 {
@@ -1337,10 +1245,6 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 	/* we only report device fault if there is a handler registered */
 	mutex_lock(&param->lock);
 	fparam = param->fault_param;
-	if (!fparam || !fparam->handler) {
-		ret = -EINVAL;
-		goto done_unlock;
-	}
 
 	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
 	    (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
-- 
2.34.1

[PATCH 7/9] iommu: Add helper to set iopf handler for domain

[Lu Baolu]

To avoid open code everywhere.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 14 +++++++++++---
 drivers/iommu/iommu.c | 20 ++++++++++++++++++--
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f10be6680f8a..c86ff10b40f3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -198,6 +198,8 @@ struct iommu_page_response {
 
 typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
+typedef enum iommu_page_response_code (*iommu_iopf_handler_t)(struct iommu_fault *,
+			struct device *dev, void *data);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */
@@ -247,9 +249,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,
-						      struct device *dev,
-						      void *data);
+	iommu_iopf_handler_t iopf_handler;
 	void *fault_data;
 	union {
 		struct {
@@ -634,6 +634,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);
@@ -957,6 +959,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 fc5e9698b35c..3dc59af24208 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1847,6 +1847,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)
 {
@@ -3335,8 +3352,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->fault_data = mm;
+	iommu_domain_set_iopf_handler(domain, iommu_sva_handle_iopf, mm);
 
 	return domain;
 }
-- 
2.34.1

[PATCH 8/9] iommu: Add iommu page fault cookie helpers

[Lu Baolu]

Add an xarray in iommu_fault_param as place holder for per-{device, pasid}
fault cookie. They could be used in varous cases. For example, SVA needs
to handle IO page faults through work queues to batch process all faults
in a fault group. It needs a fault cookie as a temporary storage of partial
faults, together with other meta data.

The iommufd will also use the fault cookie to store the mapping of device
object ID and a pasid of a device. This allows the iommufd to quickly
retrieve the device object ID for a given {device, pasid} pair in the hot
path of IO page fault delivery.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h | 16 ++++++++++++++++
 drivers/iommu/iommu.c | 44 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index c86ff10b40f3..ffd6fe1317f4 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -539,10 +539,12 @@ struct iommu_fault_event {
  * struct iommu_fault_param - per-device IOMMU fault data
  * @faults: holds the pending faults which needs response
  * @lock: protect pending faults list
+ * @pasid_cookie: per-pasid fault cookie
  */
 struct iommu_fault_param {
 	struct list_head faults;
 	struct mutex lock;
+	struct xarray pasid_cookie;
 };
 
 /**
@@ -636,6 +638,8 @@ 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);
+void *iommu_set_device_fault_cookie(struct device *dev, ioasid_t pasid, void *cookie);
+void *iommu_get_device_fault_cookie(struct device *dev, ioasid_t pasid);
 
 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);
@@ -965,6 +969,18 @@ static inline void iommu_domain_set_iopf_handler(struct iommu_domain *domain,
 {
 }
 
+static inline void *iommu_set_device_fault_cookie(struct device *dev,
+						  ioasid_t pasid, void *cookie)
+{
+	return ERR_PTR(-ENODEV);
+}
+
+static inline void *iommu_get_device_fault_cookie(struct device *dev,
+						  ioasid_t pasid)
+{
+	return ERR_PTR(-ENODEV);
+}
+
 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 3dc59af24208..d52b827982a4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1336,6 +1336,50 @@ int iommu_page_response(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(iommu_page_response);
 
+/**
+ * iommu_set_device_fault_cookie - Set a fault cookie for per-{device, pasid}
+ * @dev: the device to set the cookie
+ * @pasid: the pasid on this device
+ * @cookie: the opaque data
+ *
+ * Return the old cookie on success, or ERR_PTR(err#) on failure.
+ */
+void *iommu_set_device_fault_cookie(struct device *dev, ioasid_t pasid,
+				    void *cookie)
+{
+	struct iommu_fault_param *fault_param;
+	void *curr;
+
+	if (!dev->iommu || !dev->iommu->fault_param)
+		return ERR_PTR(-ENODEV);
+
+	fault_param = dev->iommu->fault_param;
+	curr = xa_store(&fault_param->pasid_cookie, pasid, cookie, GFP_KERNEL);
+
+	return xa_is_err(curr) ? ERR_PTR(xa_err(curr)) : curr;
+}
+EXPORT_SYMBOL_GPL(iommu_set_device_fault_cookie);
+
+/**
+ * iommu_get_device_fault_cookie - Get the fault cookie for {device, pasid}
+ * @dev: the device to set the cookie
+ * @pasid: the pasid on this device
+ *
+ * Return the cookie on success, or ERR_PTR(err#) on failure.
+ */
+void *iommu_get_device_fault_cookie(struct device *dev, ioasid_t pasid)
+{
+	struct iommu_fault_param *fault_param;
+
+	if (!dev->iommu || !dev->iommu->fault_param)
+		return ERR_PTR(-ENODEV);
+
+	fault_param = dev->iommu->fault_param;
+
+	return xa_load(&fault_param->pasid_cookie, pasid);
+}
+EXPORT_SYMBOL_GPL(iommu_get_device_fault_cookie);
+
 /**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
-- 
2.34.1

[PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Lu Baolu]

Remove the static iopf_param pointer from struct iommu_fault_param to
save memory.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h      |  2 --
 drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index ffd6fe1317f4..5fe37a7c5a55 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -551,7 +551,6 @@ struct iommu_fault_param {
  * struct dev_iommu - Collection of per-device IOMMU data
  *
  * @fault_param: IOMMU detected device fault reporting data
- * @iopf_param:	 I/O Page Fault queue and data
  * @fwspec:	 IOMMU fwspec data
  * @iommu_dev:	 IOMMU device this device is linked to
  * @priv:	 IOMMU Driver private data
@@ -564,7 +563,6 @@ struct iommu_fault_param {
 struct dev_iommu {
 	struct mutex lock;
 	struct iommu_fault_param	*fault_param;
-	struct iopf_device_param	*iopf_param;
 	struct iommu_fwspec		*fwspec;
 	struct iommu_device		*iommu_dev;
 	void				*priv;
diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
index 1749e0869f2e..6a3a4e08e67e 100644
--- a/drivers/iommu/io-pgfault.c
+++ b/drivers/iommu/io-pgfault.c
@@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault, struct device *dev)
 	 * As long as we're holding param->lock, the queue can't be unlinked
 	 * from the device and therefore cannot disappear.
 	 */
-	iopf_param = param->iopf_param;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
 	if (!iopf_param)
 		return -ENODEV;
 
@@ -235,7 +235,7 @@ int iopf_queue_flush_dev(struct device *dev)
 		return -ENODEV;
 
 	mutex_lock(&param->lock);
-	iopf_param = param->iopf_param;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
 	if (iopf_param)
 		flush_workqueue(iopf_param->queue->wq);
 	else
@@ -286,9 +286,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
  */
 int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
 {
-	int ret = -EBUSY;
-	struct iopf_device_param *iopf_param;
+	struct iopf_device_param *iopf_param, *curr;
 	struct dev_iommu *param = dev->iommu;
+	int ret;
 
 	if (!param)
 		return -ENODEV;
@@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
 
 	mutex_lock(&queue->lock);
 	mutex_lock(&param->lock);
-	if (!param->iopf_param) {
-		list_add(&iopf_param->queue_list, &queue->devices);
-		param->iopf_param = iopf_param;
-		ret = 0;
+	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
+	if (IS_ERR(curr)) {
+		ret = PTR_ERR(curr);
+		goto err_free;
 	}
+
+	if (curr) {
+		ret = -EBUSY;
+		goto err_restore;
+	}
+	list_add(&iopf_param->queue_list, &queue->devices);
 	mutex_unlock(&param->lock);
 	mutex_unlock(&queue->lock);
 
-	if (ret)
-		kfree(iopf_param);
+	return 0;
+err_restore:
+	iommu_set_device_fault_cookie(dev, 0, curr);
+err_free:
+	mutex_unlock(&param->lock);
+	mutex_unlock(&queue->lock);
+	kfree(iopf_param);
 
 	return ret;
 }
@@ -329,7 +340,6 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
  */
 int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
 {
-	int ret = -EINVAL;
 	struct iopf_fault *iopf, *next;
 	struct iopf_device_param *iopf_param;
 	struct dev_iommu *param = dev->iommu;
@@ -339,16 +349,17 @@ int iopf_queue_remove_device(struct iopf_queue *queue, struct device *dev)
 
 	mutex_lock(&queue->lock);
 	mutex_lock(&param->lock);
-	iopf_param = param->iopf_param;
-	if (iopf_param && iopf_param->queue == queue) {
-		list_del(&iopf_param->queue_list);
-		param->iopf_param = NULL;
-		ret = 0;
+	iopf_param = iommu_get_device_fault_cookie(dev, 0);
+	if (!iopf_param || iopf_param->queue != queue) {
+		mutex_unlock(&param->lock);
+		mutex_unlock(&queue->lock);
+		return -EINVAL;
 	}
+
+	list_del(&iopf_param->queue_list);
+	iommu_set_device_fault_cookie(dev, 0, NULL);
 	mutex_unlock(&param->lock);
 	mutex_unlock(&queue->lock);
-	if (ret)
-		return ret;
 
 	/* Just in case some faults are still stuck */
 	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)
-- 
2.34.1

RE: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, July 11, 2023 9:07 AM
> 
> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.

why is there memory saving? you replace a single pointer with a xarray now...

> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
> *queue, struct device *dev)
> 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	if (!param->iopf_param) {
> -		list_add(&iopf_param->queue_list, &queue->devices);
> -		param->iopf_param = iopf_param;
> -		ret = 0;
> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> +	if (IS_ERR(curr)) {
> +		ret = PTR_ERR(curr);
> +		goto err_free;
>  	}

So although the new xarray is called a per-pasid storage, here only
slot#0 is used for sva which includes a list containing partial req's
for many pasid's. It doesn't sound clean...

Re: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Baolu Lu]

On 2023/7/11 14:26, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, July 11, 2023 9:07 AM
>>
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
> 
> why is there memory saving? you replace a single pointer with a xarray now...

iopf_param is duplicate with the fault cookie. So replace it with the
fault cookie to remove duplication and save memory.

> 
>> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue
>> *queue, struct device *dev)
>>
>>   	mutex_lock(&queue->lock);
>>   	mutex_lock(&param->lock);
>> -	if (!param->iopf_param) {
>> -		list_add(&iopf_param->queue_list, &queue->devices);
>> -		param->iopf_param = iopf_param;
>> -		ret = 0;
>> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
>> +	if (IS_ERR(curr)) {
>> +		ret = PTR_ERR(curr);
>> +		goto err_free;
>>   	}
> 
> So although the new xarray is called a per-pasid storage, here only
> slot#0 is used for sva which includes a list containing partial req's
> for many pasid's. It doesn't sound clean...

Just to make it generic so that IOMMUFD can also use it. IOMMUFD will
use it to store the per-{device, pasid} object id (and possibly other
data) so that it can be quickly retrieved in the critical fault
delivering patch.

Best regards,
baolu

Re: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Jacob Pan]

Hi BaoLu,

On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu <baolu.lu@linux.intel.com>
wrote:

> Remove the static iopf_param pointer from struct iommu_fault_param to
> save memory.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  include/linux/iommu.h      |  2 --
>  drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
>  2 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index ffd6fe1317f4..5fe37a7c5a55 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -551,7 +551,6 @@ struct iommu_fault_param {
>   * struct dev_iommu - Collection of per-device IOMMU data
>   *
>   * @fault_param: IOMMU detected device fault reporting data
> - * @iopf_param:	 I/O Page Fault queue and data
>   * @fwspec:	 IOMMU fwspec data
>   * @iommu_dev:	 IOMMU device this device is linked to
>   * @priv:	 IOMMU Driver private data
> @@ -564,7 +563,6 @@ struct iommu_fault_param {
>  struct dev_iommu {
>  	struct mutex lock;
>  	struct iommu_fault_param	*fault_param;
> -	struct iopf_device_param	*iopf_param;
>  	struct iommu_fwspec		*fwspec;
>  	struct iommu_device		*iommu_dev;
>  	void				*priv;
> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
> index 1749e0869f2e..6a3a4e08e67e 100644
> --- a/drivers/iommu/io-pgfault.c
> +++ b/drivers/iommu/io-pgfault.c
> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> struct device *dev)
>  	 * As long as we're holding param->lock, the queue can't be
> unlinked
>  	 * from the device and therefore cannot disappear.
>  	 */
> -	iopf_param = param->iopf_param;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
I am not sure I understand how does it know the cookie type is iopf_param
for PASID 0?

Between IOPF and IOMMUFD use of the cookie, cookie types are different,
right?

>  	if (!iopf_param)
>  		return -ENODEV;
>  
> @@ -235,7 +235,7 @@ int iopf_queue_flush_dev(struct device *dev)
>  		return -ENODEV;
>  
>  	mutex_lock(&param->lock);
> -	iopf_param = param->iopf_param;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>  	if (iopf_param)
>  		flush_workqueue(iopf_param->queue->wq);
>  	else
> @@ -286,9 +286,9 @@ EXPORT_SYMBOL_GPL(iopf_queue_discard_partial);
>   */
>  int iopf_queue_add_device(struct iopf_queue *queue, struct device *dev)
>  {
> -	int ret = -EBUSY;
> -	struct iopf_device_param *iopf_param;
> +	struct iopf_device_param *iopf_param, *curr;
>  	struct dev_iommu *param = dev->iommu;
> +	int ret;
>  
>  	if (!param)
>  		return -ENODEV;
> @@ -303,16 +303,27 @@ int iopf_queue_add_device(struct iopf_queue *queue,
> struct device *dev) 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	if (!param->iopf_param) {
> -		list_add(&iopf_param->queue_list, &queue->devices);
> -		param->iopf_param = iopf_param;
> -		ret = 0;
> +	curr = iommu_set_device_fault_cookie(dev, 0, iopf_param);
> +	if (IS_ERR(curr)) {
> +		ret = PTR_ERR(curr);
> +		goto err_free;
>  	}
> +
> +	if (curr) {
> +		ret = -EBUSY;
> +		goto err_restore;
> +	}
> +	list_add(&iopf_param->queue_list, &queue->devices);
>  	mutex_unlock(&param->lock);
>  	mutex_unlock(&queue->lock);
>  
> -	if (ret)
> -		kfree(iopf_param);
> +	return 0;
> +err_restore:
> +	iommu_set_device_fault_cookie(dev, 0, curr);
> +err_free:
> +	mutex_unlock(&param->lock);
> +	mutex_unlock(&queue->lock);
> +	kfree(iopf_param);
>  
>  	return ret;
>  }
> @@ -329,7 +340,6 @@ EXPORT_SYMBOL_GPL(iopf_queue_add_device);
>   */
>  int iopf_queue_remove_device(struct iopf_queue *queue, struct device
> *dev) {
> -	int ret = -EINVAL;
>  	struct iopf_fault *iopf, *next;
>  	struct iopf_device_param *iopf_param;
>  	struct dev_iommu *param = dev->iommu;
> @@ -339,16 +349,17 @@ int iopf_queue_remove_device(struct iopf_queue
> *queue, struct device *dev) 
>  	mutex_lock(&queue->lock);
>  	mutex_lock(&param->lock);
> -	iopf_param = param->iopf_param;
> -	if (iopf_param && iopf_param->queue == queue) {
> -		list_del(&iopf_param->queue_list);
> -		param->iopf_param = NULL;
> -		ret = 0;
> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> +	if (!iopf_param || iopf_param->queue != queue) {
> +		mutex_unlock(&param->lock);
> +		mutex_unlock(&queue->lock);
> +		return -EINVAL;
>  	}
> +
> +	list_del(&iopf_param->queue_list);
> +	iommu_set_device_fault_cookie(dev, 0, NULL);
>  	mutex_unlock(&param->lock);
>  	mutex_unlock(&queue->lock);
> -	if (ret)
> -		return ret;
>  
>  	/* Just in case some faults are still stuck */
>  	list_for_each_entry_safe(iopf, next, &iopf_param->partial, list)


Thanks,

Jacob

Re: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Baolu Lu]

On 2023/7/12 6:02, Jacob Pan wrote:
> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> wrote:
> 
>> Remove the static iopf_param pointer from struct iommu_fault_param to
>> save memory.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   include/linux/iommu.h      |  2 --
>>   drivers/iommu/io-pgfault.c | 47 +++++++++++++++++++++++---------------
>>   2 files changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index ffd6fe1317f4..5fe37a7c5a55 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -551,7 +551,6 @@ struct iommu_fault_param {
>>    * struct dev_iommu - Collection of per-device IOMMU data
>>    *
>>    * @fault_param: IOMMU detected device fault reporting data
>> - * @iopf_param:	 I/O Page Fault queue and data
>>    * @fwspec:	 IOMMU fwspec data
>>    * @iommu_dev:	 IOMMU device this device is linked to
>>    * @priv:	 IOMMU Driver private data
>> @@ -564,7 +563,6 @@ struct iommu_fault_param {
>>   struct dev_iommu {
>>   	struct mutex lock;
>>   	struct iommu_fault_param	*fault_param;
>> -	struct iopf_device_param	*iopf_param;
>>   	struct iommu_fwspec		*fwspec;
>>   	struct iommu_device		*iommu_dev;
>>   	void				*priv;
>> diff --git a/drivers/iommu/io-pgfault.c b/drivers/iommu/io-pgfault.c
>> index 1749e0869f2e..6a3a4e08e67e 100644
>> --- a/drivers/iommu/io-pgfault.c
>> +++ b/drivers/iommu/io-pgfault.c
>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>> struct device *dev)
>>   	 * As long as we're holding param->lock, the queue can't be
>> unlinked
>>   	 * from the device and therefore cannot disappear.
>>   	 */
>> -	iopf_param = param->iopf_param;
>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> I am not sure I understand how does it know the cookie type is iopf_param
> for PASID 0?
> 
> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> right?
> 

The fault cookie is managed by the code that delivers or handles the
faults. The sva and IOMMUFD paths are exclusive.

Best regards,
baolu

RE: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Wednesday, July 12, 2023 11:13 AM
> 
> On 2023/7/12 6:02, Jacob Pan wrote:
> > On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> > wrote:
> >
> >> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
> >> struct device *dev)
> >>   	 * As long as we're holding param->lock, the queue can't be
> >> unlinked
> >>   	 * from the device and therefore cannot disappear.
> >>   	 */
> >> -	iopf_param = param->iopf_param;
> >> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> > I am not sure I understand how does it know the cookie type is iopf_param
> > for PASID 0?
> >
> > Between IOPF and IOMMUFD use of the cookie, cookie types are different,
> > right?
> >
> 
> The fault cookie is managed by the code that delivers or handles the
> faults. The sva and IOMMUFD paths are exclusive.
> 

what about siov? A siov-capable device can support sva and iommufd
simultaneously.

Re: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Baolu Lu]

On 2023/7/13 11:24, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Wednesday, July 12, 2023 11:13 AM
>>
>> On 2023/7/12 6:02, Jacob Pan wrote:
>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
>>> wrote:
>>>
>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault *fault,
>>>> struct device *dev)
>>>>    	 * As long as we're holding param->lock, the queue can't be
>>>> unlinked
>>>>    	 * from the device and therefore cannot disappear.
>>>>    	 */
>>>> -	iopf_param = param->iopf_param;
>>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>> I am not sure I understand how does it know the cookie type is iopf_param
>>> for PASID 0?
>>>
>>> Between IOPF and IOMMUFD use of the cookie, cookie types are different,
>>> right?
>>>
>>
>> The fault cookie is managed by the code that delivers or handles the
>> faults. The sva and IOMMUFD paths are exclusive.
>>
> 
> what about siov? A siov-capable device can support sva and iommufd
> simultaneously.

For siov case, the pasid should be global. RID and each pasid are still
exclusive, so I don't see any problem. Did I overlook anything?

Best regards,
baolu

RE: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, July 13, 2023 11:44 AM
> 
> On 2023/7/13 11:24, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Wednesday, July 12, 2023 11:13 AM
> >>
> >> On 2023/7/12 6:02, Jacob Pan wrote:
> >>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
> >>> wrote:
> >>>
> >>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
> *fault,
> >>>> struct device *dev)
> >>>>    	 * As long as we're holding param->lock, the queue can't be
> >>>> unlinked
> >>>>    	 * from the device and therefore cannot disappear.
> >>>>    	 */
> >>>> -	iopf_param = param->iopf_param;
> >>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
> >>> I am not sure I understand how does it know the cookie type is
> iopf_param
> >>> for PASID 0?
> >>>
> >>> Between IOPF and IOMMUFD use of the cookie, cookie types are
> different,
> >>> right?
> >>>
> >>
> >> The fault cookie is managed by the code that delivers or handles the
> >> faults. The sva and IOMMUFD paths are exclusive.
> >>
> >
> > what about siov? A siov-capable device can support sva and iommufd
> > simultaneously.
> 
> For siov case, the pasid should be global. RID and each pasid are still
> exclusive, so I don't see any problem. Did I overlook anything?
> 

they are exclusive but it's weird to see some pasids (for sva) on this
device are tracked by slot#0 while other pasids (for iommufd) occupies
per-pasid slot.

why not generalizing them given you name it as "per-pasid fault cookie"?

Re: [PATCH 9/9] iommu: Use fault cookie to store iopf_param

[Baolu Lu]

On 2023/7/13 16:01, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, July 13, 2023 11:44 AM
>>
>> On 2023/7/13 11:24, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Wednesday, July 12, 2023 11:13 AM
>>>>
>>>> On 2023/7/12 6:02, Jacob Pan wrote:
>>>>> On Tue, 11 Jul 2023 09:06:42 +0800, Lu Baolu<baolu.lu@linux.intel.com>
>>>>> wrote:
>>>>>
>>>>>> @@ -158,7 +158,7 @@ int iommu_queue_iopf(struct iommu_fault
>> *fault,
>>>>>> struct device *dev)
>>>>>>     	 * As long as we're holding param->lock, the queue can't be
>>>>>> unlinked
>>>>>>     	 * from the device and therefore cannot disappear.
>>>>>>     	 */
>>>>>> -	iopf_param = param->iopf_param;
>>>>>> +	iopf_param = iommu_get_device_fault_cookie(dev, 0);
>>>>> I am not sure I understand how does it know the cookie type is
>> iopf_param
>>>>> for PASID 0?
>>>>>
>>>>> Between IOPF and IOMMUFD use of the cookie, cookie types are
>> different,
>>>>> right?
>>>>>
>>>>
>>>> The fault cookie is managed by the code that delivers or handles the
>>>> faults. The sva and IOMMUFD paths are exclusive.
>>>>
>>>
>>> what about siov? A siov-capable device can support sva and iommufd
>>> simultaneously.
>>
>> For siov case, the pasid should be global. RID and each pasid are still
>> exclusive, so I don't see any problem. Did I overlook anything?
>>
> 
> they are exclusive but it's weird to see some pasids (for sva) on this
> device are tracked by slot#0 while other pasids (for iommufd) occupies
> per-pasid slot.
> 
> why not generalizing them given you name it as "per-pasid fault cookie"?

Yeah! Get your point now. At least the partial list should be per-pasid.

Let me invest more time on this.

Best regards,
baolu