[PATCH 0/4] iommu: Add device fault reporting API

[PATCH 0/4] iommu: Add device fault reporting API

[Jean-Philippe Brucker]

Allow device drivers and VFIO to get notifications on IOMMU translation
fault, and to handle recoverable faults (PCI PRI). These four patches
are relatively mature since they are required by three different series,
and have been under discussion for a while:

* Nested translation support for SMMUv3 [1].
* vSVA for VT-d [2].
* My generic host SVA implementation.

I reworked patch 4 according to previous discussions, and moved the page
response structure to UAPI. For the other patches I only fixed comments
and whitespaces. Please have a look and see if it works for you.

[1] [PATCH v7 00/23] SMMUv3 Nested Stage Setup
    https://lore.kernel.org/lkml/20190408121911.24103-1-eric.auger@redhat.com/
[2] [PATCH v3 00/16] Shared virtual address IOMMU and VT-d support
    https://lore.kernel.org/lkml/1556922737-76313-1-git-send-email-jacob.jun.pan@linux.intel.com/

Jacob Pan (3):
  driver core: Add per device iommu param
  iommu: Introduce device fault data
  iommu: Introduce device fault report API

Jean-Philippe Brucker (1):
  iommu: Add recoverable fault reporting

 drivers/iommu/iommu.c      | 218 +++++++++++++++++++++++++++++++++++++
 include/linux/device.h     |   3 +
 include/linux/iommu.h      |  91 ++++++++++++++++
 include/uapi/linux/iommu.h | 152 ++++++++++++++++++++++++++
 4 files changed, 464 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

-- 
2.21.0

[PATCH 0/4] iommu: Add device fault reporting API

[Jean-Philippe Brucker]

Allow device drivers and VFIO to get notifications on IOMMU translation
fault, and to handle recoverable faults (PCI PRI). These four patches
are relatively mature since they are required by three different series,
and have been under discussion for a while:

* Nested translation support for SMMUv3 [1].
* vSVA for VT-d [2].
* My generic host SVA implementation.

I reworked patch 4 according to previous discussions, and moved the page
response structure to UAPI. For the other patches I only fixed comments
and whitespaces. Please have a look and see if it works for you.

[1] [PATCH v7 00/23] SMMUv3 Nested Stage Setup
    https://lore.kernel.org/lkml/20190408121911.24103-1-eric.auger@redhat.com/
[2] [PATCH v3 00/16] Shared virtual address IOMMU and VT-d support
    https://lore.kernel.org/lkml/1556922737-76313-1-git-send-email-jacob.jun.pan@linux.intel.com/

Jacob Pan (3):
  driver core: Add per device iommu param
  iommu: Introduce device fault data
  iommu: Introduce device fault report API

Jean-Philippe Brucker (1):
  iommu: Add recoverable fault reporting

 drivers/iommu/iommu.c      | 218 +++++++++++++++++++++++++++++++++++++
 include/linux/device.h     |   3 +
 include/linux/iommu.h      |  91 ++++++++++++++++
 include/uapi/linux/iommu.h | 152 ++++++++++++++++++++++++++
 4 files changed, 464 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

-- 
2.21.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 1/4] driver core: Add per device iommu param

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

DMA faults can be detected by IOMMU at device level. Adding a pointer
to struct device allows IOMMU subsystem to report relevant faults
back to the device driver for further handling.
For direct assigned device (or user space drivers), guest OS holds
responsibility to handle and respond per device IOMMU fault.
Therefore we need fault reporting mechanism to propagate faults beyond
IOMMU subsystem.

There are two other IOMMU data pointers under struct device today, here
we introduce iommu_param as a parent pointer such that all device IOMMU
data can be consolidated here. The idea was suggested here by Greg KH
and Joerg. The name iommu_param is chosen here since iommu_data has been
used.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Link: https://lkml.org/lkml/2017/10/6/81
---
 include/linux/device.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index e85264fb6616..f0a975abd6e9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -42,6 +42,7 @@ struct iommu_ops;
 struct iommu_group;
 struct iommu_fwspec;
 struct dev_pin_info;
+struct iommu_param;
 
 struct bus_attribute {
 	struct attribute	attr;
@@ -959,6 +960,7 @@ struct dev_links_info {
  * 		device (i.e. the bus driver that discovered the device).
  * @iommu_group: IOMMU group the device belongs to.
  * @iommu_fwspec: IOMMU-specific properties supplied by firmware.
+ * @iommu_param: Per device generic IOMMU runtime data
  *
  * @offline_disabled: If set, the device is permanently online.
  * @offline:	Set after successful invocation of bus type's .offline().
@@ -1052,6 +1054,7 @@ struct device {
 	void	(*release)(struct device *dev);
 	struct iommu_group	*iommu_group;
 	struct iommu_fwspec	*iommu_fwspec;
+	struct iommu_param	*iommu_param;
 
 	bool			offline_disabled:1;
 	bool			offline:1;
-- 
2.21.0

[PATCH 1/4] driver core: Add per device iommu param

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

DMA faults can be detected by IOMMU at device level. Adding a pointer
to struct device allows IOMMU subsystem to report relevant faults
back to the device driver for further handling.
For direct assigned device (or user space drivers), guest OS holds
responsibility to handle and respond per device IOMMU fault.
Therefore we need fault reporting mechanism to propagate faults beyond
IOMMU subsystem.

There are two other IOMMU data pointers under struct device today, here
we introduce iommu_param as a parent pointer such that all device IOMMU
data can be consolidated here. The idea was suggested here by Greg KH
and Joerg. The name iommu_param is chosen here since iommu_data has been
used.

Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Link: https://lkml.org/lkml/2017/10/6/81
---
 include/linux/device.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index e85264fb6616..f0a975abd6e9 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -42,6 +42,7 @@ struct iommu_ops;
 struct iommu_group;
 struct iommu_fwspec;
 struct dev_pin_info;
+struct iommu_param;
 
 struct bus_attribute {
 	struct attribute	attr;
@@ -959,6 +960,7 @@ struct dev_links_info {
  * 		device (i.e. the bus driver that discovered the device).
  * @iommu_group: IOMMU group the device belongs to.
  * @iommu_fwspec: IOMMU-specific properties supplied by firmware.
+ * @iommu_param: Per device generic IOMMU runtime data
  *
  * @offline_disabled: If set, the device is permanently online.
  * @offline:	Set after successful invocation of bus type's .offline().
@@ -1052,6 +1054,7 @@ struct device {
 	void	(*release)(struct device *dev);
 	struct iommu_group	*iommu_group;
 	struct iommu_fwspec	*iommu_fwspec;
+	struct iommu_param	*iommu_param;
 
 	bool			offline_disabled:1;
 	bool			offline:1;
-- 
2.21.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 2/4] iommu: Introduce device fault data

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

Device faults detected by IOMMU can be reported outside the IOMMU
subsystem for further processing. This patch introduces
a generic device fault data structure.

The fault can be either an unrecoverable fault or a page request,
also referred to as a recoverable fault.

We only care about non internal faults that are likely to be reported
to an external subsystem.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/linux/iommu.h      |  43 ++++++++++++++
 include/uapi/linux/iommu.h | 118 +++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a815cf6f6f47..d442f5f3fa93 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,6 +25,7 @@
 #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)
@@ -49,6 +50,7 @@ struct device;
 struct iommu_domain;
 struct notifier_block;
 struct iommu_sva;
+struct iommu_fault_event;
 
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
@@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
 typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct iommu_sva *,
 				       void *);
+typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *, void *);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */
@@ -301,6 +304,45 @@ struct iommu_device {
 	struct device *dev;
 };
 
+/**
+ * struct iommu_fault_event - Generic fault event
+ *
+ * Can represent recoverable faults such as a page requests or
+ * unrecoverable faults such as DMA or IRQ remapping faults.
+ *
+ * @fault: fault descriptor
+ * @iommu_private: used by the IOMMU driver for storing fault-specific
+ *                 data. Users should not modify this field before
+ *                 sending the fault response.
+ */
+struct iommu_fault_event {
+	struct iommu_fault fault;
+	u64 iommu_private;
+};
+
+/**
+ * struct iommu_fault_param - per-device IOMMU fault data
+ * @handler: Callback function to handle IOMMU faults at device level
+ * @data: handler private data
+ */
+struct iommu_fault_param {
+	iommu_dev_fault_handler_t handler;
+	void *data;
+};
+
+/**
+ * struct iommu_param - collection of per-device IOMMU data
+ *
+ * @fault_param: IOMMU detected device fault reporting data
+ *
+ * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
+ *	struct iommu_group	*iommu_group;
+ *	struct iommu_fwspec	*iommu_fwspec;
+ */
+struct iommu_param {
+	struct iommu_fault_param *fault_param;
+};
+
 int  iommu_device_register(struct iommu_device *iommu);
 void iommu_device_unregister(struct iommu_device *iommu);
 int  iommu_device_sysfs_add(struct iommu_device *iommu,
@@ -504,6 +546,7 @@ struct iommu_ops {};
 struct iommu_group {};
 struct iommu_fwspec {};
 struct iommu_device {};
+struct iommu_fault_param {};
 
 static inline bool iommu_present(struct bus_type *bus)
 {
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
new file mode 100644
index 000000000000..796402174d6c
--- /dev/null
+++ b/include/uapi/linux/iommu.h
@@ -0,0 +1,118 @@
+/* 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)
+ * @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)
+	__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
+ */
+struct iommu_fault {
+	__u32	type;
+	__u32	padding;
+	union {
+		struct iommu_fault_unrecoverable event;
+		struct iommu_fault_page_request prm;
+	};
+};
+#endif /* _UAPI_IOMMU_H */
-- 
2.21.0

[PATCH 2/4] iommu: Introduce device fault data

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

Device faults detected by IOMMU can be reported outside the IOMMU
subsystem for further processing. This patch introduces
a generic device fault data structure.

The fault can be either an unrecoverable fault or a page request,
also referred to as a recoverable fault.

We only care about non internal faults that are likely to be reported
to an external subsystem.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 include/linux/iommu.h      |  43 ++++++++++++++
 include/uapi/linux/iommu.h | 118 +++++++++++++++++++++++++++++++++++++
 2 files changed, 161 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index a815cf6f6f47..d442f5f3fa93 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -25,6 +25,7 @@
 #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)
@@ -49,6 +50,7 @@ struct device;
 struct iommu_domain;
 struct notifier_block;
 struct iommu_sva;
+struct iommu_fault_event;
 
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
@@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
 typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct iommu_sva *,
 				       void *);
+typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *, void *);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */
@@ -301,6 +304,45 @@ struct iommu_device {
 	struct device *dev;
 };
 
+/**
+ * struct iommu_fault_event - Generic fault event
+ *
+ * Can represent recoverable faults such as a page requests or
+ * unrecoverable faults such as DMA or IRQ remapping faults.
+ *
+ * @fault: fault descriptor
+ * @iommu_private: used by the IOMMU driver for storing fault-specific
+ *                 data. Users should not modify this field before
+ *                 sending the fault response.
+ */
+struct iommu_fault_event {
+	struct iommu_fault fault;
+	u64 iommu_private;
+};
+
+/**
+ * struct iommu_fault_param - per-device IOMMU fault data
+ * @handler: Callback function to handle IOMMU faults at device level
+ * @data: handler private data
+ */
+struct iommu_fault_param {
+	iommu_dev_fault_handler_t handler;
+	void *data;
+};
+
+/**
+ * struct iommu_param - collection of per-device IOMMU data
+ *
+ * @fault_param: IOMMU detected device fault reporting data
+ *
+ * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
+ *	struct iommu_group	*iommu_group;
+ *	struct iommu_fwspec	*iommu_fwspec;
+ */
+struct iommu_param {
+	struct iommu_fault_param *fault_param;
+};
+
 int  iommu_device_register(struct iommu_device *iommu);
 void iommu_device_unregister(struct iommu_device *iommu);
 int  iommu_device_sysfs_add(struct iommu_device *iommu,
@@ -504,6 +546,7 @@ struct iommu_ops {};
 struct iommu_group {};
 struct iommu_fwspec {};
 struct iommu_device {};
+struct iommu_fault_param {};
 
 static inline bool iommu_present(struct bus_type *bus)
 {
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
new file mode 100644
index 000000000000..796402174d6c
--- /dev/null
+++ b/include/uapi/linux/iommu.h
@@ -0,0 +1,118 @@
+/* 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)
+ * @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)
+	__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
+ */
+struct iommu_fault {
+	__u32	type;
+	__u32	padding;
+	union {
+		struct iommu_fault_unrecoverable event;
+		struct iommu_fault_page_request prm;
+	};
+};
+#endif /* _UAPI_IOMMU_H */
-- 
2.21.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu: Introduce device fault data

[Robin Murphy]

On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> Device faults detected by IOMMU can be reported outside the IOMMU
> subsystem for further processing. This patch introduces
> a generic device fault data structure.
> 
> The fault can be either an unrecoverable fault or a page request,
> also referred to as a recoverable fault.
> 
> We only care about non internal faults that are likely to be reported
> to an external subsystem.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   include/linux/iommu.h      |  43 ++++++++++++++
>   include/uapi/linux/iommu.h | 118 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 161 insertions(+)
>   create mode 100644 include/uapi/linux/iommu.h
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a815cf6f6f47..d442f5f3fa93 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -25,6 +25,7 @@
>   #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)
> @@ -49,6 +50,7 @@ struct device;
>   struct iommu_domain;
>   struct notifier_block;
>   struct iommu_sva;
> +struct iommu_fault_event;
>   
>   /* iommu fault flags */
>   #define IOMMU_FAULT_READ	0x0
> @@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
>   			struct device *, unsigned long, int, void *);
>   typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct iommu_sva *,
>   				       void *);
> +typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *, void *);
>   
>   struct iommu_domain_geometry {
>   	dma_addr_t aperture_start; /* First address that can be mapped    */
> @@ -301,6 +304,45 @@ struct iommu_device {
>   	struct device *dev;
>   };
>   
> +/**
> + * struct iommu_fault_event - Generic fault event
> + *
> + * Can represent recoverable faults such as a page requests or
> + * unrecoverable faults such as DMA or IRQ remapping faults.
> + *
> + * @fault: fault descriptor
> + * @iommu_private: used by the IOMMU driver for storing fault-specific
> + *                 data. Users should not modify this field before
> + *                 sending the fault response.

Sorry if I'm a bit late to the party, but given that description, if 
users aren't allowed to touch this then why expose it to them at all? 
I.e. why not have iommu_report_device_fault() pass just the fault itself 
to the fault handler:

	ret = fparam->handler(&evt->fault, fparam->data);

and let the IOMMU core/drivers decapsulate it again later if need be. 
AFAICS drivers could also just embed the entire generic event in their 
own private structure anyway, just as we do for domains.

Robin.

> + */
> +struct iommu_fault_event {
> +	struct iommu_fault fault;
> +	u64 iommu_private;
> +};
> +
> +/**
> + * struct iommu_fault_param - per-device IOMMU fault data
> + * @handler: Callback function to handle IOMMU faults at device level
> + * @data: handler private data
> + */
> +struct iommu_fault_param {
> +	iommu_dev_fault_handler_t handler;
> +	void *data;
> +};
> +
> +/**
> + * struct iommu_param - collection of per-device IOMMU data
> + *
> + * @fault_param: IOMMU detected device fault reporting data
> + *
> + * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
> + *	struct iommu_group	*iommu_group;
> + *	struct iommu_fwspec	*iommu_fwspec;
> + */
> +struct iommu_param {
> +	struct iommu_fault_param *fault_param;
> +};
> +
>   int  iommu_device_register(struct iommu_device *iommu);
>   void iommu_device_unregister(struct iommu_device *iommu);
>   int  iommu_device_sysfs_add(struct iommu_device *iommu,
> @@ -504,6 +546,7 @@ struct iommu_ops {};
>   struct iommu_group {};
>   struct iommu_fwspec {};
>   struct iommu_device {};
> +struct iommu_fault_param {};
>   
>   static inline bool iommu_present(struct bus_type *bus)
>   {
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> new file mode 100644
> index 000000000000..796402174d6c
> --- /dev/null
> +++ b/include/uapi/linux/iommu.h
> @@ -0,0 +1,118 @@
> +/* 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)
> + * @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)
> +	__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
> + */
> +struct iommu_fault {
> +	__u32	type;
> +	__u32	padding;
> +	union {
> +		struct iommu_fault_unrecoverable event;
> +		struct iommu_fault_page_request prm;
> +	};
> +};
> +#endif /* _UAPI_IOMMU_H */
> 

Re: [PATCH 2/4] iommu: Introduce device fault data

[Robin Murphy]

On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> Device faults detected by IOMMU can be reported outside the IOMMU
> subsystem for further processing. This patch introduces
> a generic device fault data structure.
> 
> The fault can be either an unrecoverable fault or a page request,
> also referred to as a recoverable fault.
> 
> We only care about non internal faults that are likely to be reported
> to an external subsystem.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   include/linux/iommu.h      |  43 ++++++++++++++
>   include/uapi/linux/iommu.h | 118 +++++++++++++++++++++++++++++++++++++
>   2 files changed, 161 insertions(+)
>   create mode 100644 include/uapi/linux/iommu.h
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index a815cf6f6f47..d442f5f3fa93 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -25,6 +25,7 @@
>   #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)
> @@ -49,6 +50,7 @@ struct device;
>   struct iommu_domain;
>   struct notifier_block;
>   struct iommu_sva;
> +struct iommu_fault_event;
>   
>   /* iommu fault flags */
>   #define IOMMU_FAULT_READ	0x0
> @@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
>   			struct device *, unsigned long, int, void *);
>   typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct iommu_sva *,
>   				       void *);
> +typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event *, void *);
>   
>   struct iommu_domain_geometry {
>   	dma_addr_t aperture_start; /* First address that can be mapped    */
> @@ -301,6 +304,45 @@ struct iommu_device {
>   	struct device *dev;
>   };
>   
> +/**
> + * struct iommu_fault_event - Generic fault event
> + *
> + * Can represent recoverable faults such as a page requests or
> + * unrecoverable faults such as DMA or IRQ remapping faults.
> + *
> + * @fault: fault descriptor
> + * @iommu_private: used by the IOMMU driver for storing fault-specific
> + *                 data. Users should not modify this field before
> + *                 sending the fault response.

Sorry if I'm a bit late to the party, but given that description, if 
users aren't allowed to touch this then why expose it to them at all? 
I.e. why not have iommu_report_device_fault() pass just the fault itself 
to the fault handler:

	ret = fparam->handler(&evt->fault, fparam->data);

and let the IOMMU core/drivers decapsulate it again later if need be. 
AFAICS drivers could also just embed the entire generic event in their 
own private structure anyway, just as we do for domains.

Robin.

> + */
> +struct iommu_fault_event {
> +	struct iommu_fault fault;
> +	u64 iommu_private;
> +};
> +
> +/**
> + * struct iommu_fault_param - per-device IOMMU fault data
> + * @handler: Callback function to handle IOMMU faults at device level
> + * @data: handler private data
> + */
> +struct iommu_fault_param {
> +	iommu_dev_fault_handler_t handler;
> +	void *data;
> +};
> +
> +/**
> + * struct iommu_param - collection of per-device IOMMU data
> + *
> + * @fault_param: IOMMU detected device fault reporting data
> + *
> + * TODO: migrate other per device data pointers under iommu_dev_data, e.g.
> + *	struct iommu_group	*iommu_group;
> + *	struct iommu_fwspec	*iommu_fwspec;
> + */
> +struct iommu_param {
> +	struct iommu_fault_param *fault_param;
> +};
> +
>   int  iommu_device_register(struct iommu_device *iommu);
>   void iommu_device_unregister(struct iommu_device *iommu);
>   int  iommu_device_sysfs_add(struct iommu_device *iommu,
> @@ -504,6 +546,7 @@ struct iommu_ops {};
>   struct iommu_group {};
>   struct iommu_fwspec {};
>   struct iommu_device {};
> +struct iommu_fault_param {};
>   
>   static inline bool iommu_present(struct bus_type *bus)
>   {
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> new file mode 100644
> index 000000000000..796402174d6c
> --- /dev/null
> +++ b/include/uapi/linux/iommu.h
> @@ -0,0 +1,118 @@
> +/* 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)
> + * @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)
> +	__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
> + */
> +struct iommu_fault {
> +	__u32	type;
> +	__u32	padding;
> +	union {
> +		struct iommu_fault_unrecoverable event;
> +		struct iommu_fault_page_request prm;
> +	};
> +};
> +#endif /* _UAPI_IOMMU_H */
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jacob Pan]

On Thu, 23 May 2019 19:43:46 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > 
> > Device faults detected by IOMMU can be reported outside the IOMMU
> > subsystem for further processing. This patch introduces
> > a generic device fault data structure.
> > 
> > The fault can be either an unrecoverable fault or a page request,
> > also referred to as a recoverable fault.
> > 
> > We only care about non internal faults that are likely to be
> > reported to an external subsystem.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >   include/linux/iommu.h      |  43 ++++++++++++++
> >   include/uapi/linux/iommu.h | 118
> > +++++++++++++++++++++++++++++++++++++ 2 files changed, 161
> > insertions(+) create mode 100644 include/uapi/linux/iommu.h
> > 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index a815cf6f6f47..d442f5f3fa93 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -25,6 +25,7 @@
> >   #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)
> > @@ -49,6 +50,7 @@ struct device;
> >   struct iommu_domain;
> >   struct notifier_block;
> >   struct iommu_sva;
> > +struct iommu_fault_event;
> >   
> >   /* iommu fault flags */
> >   #define IOMMU_FAULT_READ	0x0
> > @@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct
> > iommu_domain *, struct device *, unsigned long, int, void *);
> >   typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct
> > iommu_sva *, void *);
> > +typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event
> > *, void *); 
> >   struct iommu_domain_geometry {
> >   	dma_addr_t aperture_start; /* First address that can be
> > mapped    */ @@ -301,6 +304,45 @@ struct iommu_device {
> >   	struct device *dev;
> >   };
> >   
> > +/**
> > + * struct iommu_fault_event - Generic fault event
> > + *
> > + * Can represent recoverable faults such as a page requests or
> > + * unrecoverable faults such as DMA or IRQ remapping faults.
> > + *
> > + * @fault: fault descriptor
> > + * @iommu_private: used by the IOMMU driver for storing
> > fault-specific
> > + *                 data. Users should not modify this field before
> > + *                 sending the fault response.  
> 
> Sorry if I'm a bit late to the party, but given that description, if 
> users aren't allowed to touch this then why expose it to them at all? 
> I.e. why not have iommu_report_device_fault() pass just the fault
> itself to the fault handler:
> 
> 	ret = fparam->handler(&evt->fault, fparam->data);
> 
> and let the IOMMU core/drivers decapsulate it again later if need be. 
> AFAICS drivers could also just embed the entire generic event in
> their own private structure anyway, just as we do for domains.
> 
I can't remember all the discussion history but I think iommu_private
is used similarly to the page request private data (device private). We
need to inject the data to the guest and the guest will send the
unmodified data back along with response. The private data can be used
to tag internal device/iommu context.

I think we can do the way you said by keeping them within iommu core
and recover it based on the response but that would require tracking
each fault report, right?

If we pass on the private data, we only need to check if the response
belong to the device but not exact match of a specific fault since the
damage is contained in the assigned device. In case of injection
fault into the guest, the response will come asynchronously after the
handler completes.

> Robin.
> 
> > + */
> > +struct iommu_fault_event {
> > +	struct iommu_fault fault;
> > +	u64 iommu_private;
> > +};
> > +
> > +/**
> > + * struct iommu_fault_param - per-device IOMMU fault data
> > + * @handler: Callback function to handle IOMMU faults at device
> > level
> > + * @data: handler private data
> > + */
> > +struct iommu_fault_param {
> > +	iommu_dev_fault_handler_t handler;
> > +	void *data;
> > +};
> > +
> > +/**
> > + * struct iommu_param - collection of per-device IOMMU data
> > + *
> > + * @fault_param: IOMMU detected device fault reporting data
> > + *
> > + * TODO: migrate other per device data pointers under
> > iommu_dev_data, e.g.
> > + *	struct iommu_group	*iommu_group;
> > + *	struct iommu_fwspec	*iommu_fwspec;
> > + */
> > +struct iommu_param {
> > +	struct iommu_fault_param *fault_param;
> > +};
> > +
> >   int  iommu_device_register(struct iommu_device *iommu);
> >   void iommu_device_unregister(struct iommu_device *iommu);
> >   int  iommu_device_sysfs_add(struct iommu_device *iommu,
> > @@ -504,6 +546,7 @@ struct iommu_ops {};
> >   struct iommu_group {};
> >   struct iommu_fwspec {};
> >   struct iommu_device {};
> > +struct iommu_fault_param {};
> >   
> >   static inline bool iommu_present(struct bus_type *bus)
> >   {
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > new file mode 100644
> > index 000000000000..796402174d6c
> > --- /dev/null
> > +++ b/include/uapi/linux/iommu.h
> > @@ -0,0 +1,118 @@
> > +/* 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)
> > + * @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)
> > +	__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
> > + */
> > +struct iommu_fault {
> > +	__u32	type;
> > +	__u32	padding;
> > +	union {
> > +		struct iommu_fault_unrecoverable event;
> > +		struct iommu_fault_page_request prm;
> > +	};
> > +};
> > +#endif /* _UAPI_IOMMU_H */
> >   
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[Jacob Pan]

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jacob Pan]

On Thu, 23 May 2019 19:43:46 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > 
> > Device faults detected by IOMMU can be reported outside the IOMMU
> > subsystem for further processing. This patch introduces
> > a generic device fault data structure.
> > 
> > The fault can be either an unrecoverable fault or a page request,
> > also referred to as a recoverable fault.
> > 
> > We only care about non internal faults that are likely to be
> > reported to an external subsystem.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Liu, Yi L <yi.l.liu@linux.intel.com>
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >   include/linux/iommu.h      |  43 ++++++++++++++
> >   include/uapi/linux/iommu.h | 118
> > +++++++++++++++++++++++++++++++++++++ 2 files changed, 161
> > insertions(+) create mode 100644 include/uapi/linux/iommu.h
> > 
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> > index a815cf6f6f47..d442f5f3fa93 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -25,6 +25,7 @@
> >   #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)
> > @@ -49,6 +50,7 @@ struct device;
> >   struct iommu_domain;
> >   struct notifier_block;
> >   struct iommu_sva;
> > +struct iommu_fault_event;
> >   
> >   /* iommu fault flags */
> >   #define IOMMU_FAULT_READ	0x0
> > @@ -58,6 +60,7 @@ typedef int (*iommu_fault_handler_t)(struct
> > iommu_domain *, struct device *, unsigned long, int, void *);
> >   typedef int (*iommu_mm_exit_handler_t)(struct device *dev, struct
> > iommu_sva *, void *);
> > +typedef int (*iommu_dev_fault_handler_t)(struct iommu_fault_event
> > *, void *); 
> >   struct iommu_domain_geometry {
> >   	dma_addr_t aperture_start; /* First address that can be
> > mapped    */ @@ -301,6 +304,45 @@ struct iommu_device {
> >   	struct device *dev;
> >   };
> >   
> > +/**
> > + * struct iommu_fault_event - Generic fault event
> > + *
> > + * Can represent recoverable faults such as a page requests or
> > + * unrecoverable faults such as DMA or IRQ remapping faults.
> > + *
> > + * @fault: fault descriptor
> > + * @iommu_private: used by the IOMMU driver for storing
> > fault-specific
> > + *                 data. Users should not modify this field before
> > + *                 sending the fault response.  
> 
> Sorry if I'm a bit late to the party, but given that description, if 
> users aren't allowed to touch this then why expose it to them at all? 
> I.e. why not have iommu_report_device_fault() pass just the fault
> itself to the fault handler:
> 
> 	ret = fparam->handler(&evt->fault, fparam->data);
> 
> and let the IOMMU core/drivers decapsulate it again later if need be. 
> AFAICS drivers could also just embed the entire generic event in
> their own private structure anyway, just as we do for domains.
> 
I can't remember all the discussion history but I think iommu_private
is used similarly to the page request private data (device private). We
need to inject the data to the guest and the guest will send the
unmodified data back along with response. The private data can be used
to tag internal device/iommu context.

I think we can do the way you said by keeping them within iommu core
and recover it based on the response but that would require tracking
each fault report, right?

If we pass on the private data, we only need to check if the response
belong to the device but not exact match of a specific fault since the
damage is contained in the assigned device. In case of injection
fault into the guest, the response will come asynchronously after the
handler completes.

> Robin.
> 
> > + */
> > +struct iommu_fault_event {
> > +	struct iommu_fault fault;
> > +	u64 iommu_private;
> > +};
> > +
> > +/**
> > + * struct iommu_fault_param - per-device IOMMU fault data
> > + * @handler: Callback function to handle IOMMU faults at device
> > level
> > + * @data: handler private data
> > + */
> > +struct iommu_fault_param {
> > +	iommu_dev_fault_handler_t handler;
> > +	void *data;
> > +};
> > +
> > +/**
> > + * struct iommu_param - collection of per-device IOMMU data
> > + *
> > + * @fault_param: IOMMU detected device fault reporting data
> > + *
> > + * TODO: migrate other per device data pointers under
> > iommu_dev_data, e.g.
> > + *	struct iommu_group	*iommu_group;
> > + *	struct iommu_fwspec	*iommu_fwspec;
> > + */
> > +struct iommu_param {
> > +	struct iommu_fault_param *fault_param;
> > +};
> > +
> >   int  iommu_device_register(struct iommu_device *iommu);
> >   void iommu_device_unregister(struct iommu_device *iommu);
> >   int  iommu_device_sysfs_add(struct iommu_device *iommu,
> > @@ -504,6 +546,7 @@ struct iommu_ops {};
> >   struct iommu_group {};
> >   struct iommu_fwspec {};
> >   struct iommu_device {};
> > +struct iommu_fault_param {};
> >   
> >   static inline bool iommu_present(struct bus_type *bus)
> >   {
> > diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> > new file mode 100644
> > index 000000000000..796402174d6c
> > --- /dev/null
> > +++ b/include/uapi/linux/iommu.h
> > @@ -0,0 +1,118 @@
> > +/* 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)
> > + * @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)
> > +	__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
> > + */
> > +struct iommu_fault {
> > +	__u32	type;
> > +	__u32	padding;
> > +	union {
> > +		struct iommu_fault_unrecoverable event;
> > +		struct iommu_fault_page_request prm;
> > +	};
> > +};
> > +#endif /* _UAPI_IOMMU_H */
> >   
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jean-Philippe Brucker]

On 24/05/2019 14:49, Jacob Pan wrote:
> On Thu, 23 May 2019 19:43:46 +0100
> Robin Murphy <robin.murphy@arm.com> wrote:
>>> +/**
>>> + * struct iommu_fault_event - Generic fault event
>>> + *
>>> + * Can represent recoverable faults such as a page requests or
>>> + * unrecoverable faults such as DMA or IRQ remapping faults.
>>> + *
>>> + * @fault: fault descriptor
>>> + * @iommu_private: used by the IOMMU driver for storing
>>> fault-specific
>>> + *                 data. Users should not modify this field before
>>> + *                 sending the fault response.  
>>
>> Sorry if I'm a bit late to the party, but given that description, if 
>> users aren't allowed to touch this then why expose it to them at all? 
>> I.e. why not have iommu_report_device_fault() pass just the fault
>> itself to the fault handler:
>>
>> 	ret = fparam->handler(&evt->fault, fparam->data);
>>
>> and let the IOMMU core/drivers decapsulate it again later if need be. 
>> AFAICS drivers could also just embed the entire generic event in
>> their own private structure anyway, just as we do for domains.
>>
> I can't remember all the discussion history but I think iommu_private
> is used similarly to the page request private data (device private).

Hm yes, we already have iommu_fault_page_request::private_data for that.
I think I used to stash flags in iommu_private (is_stall and
needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them
from the device structure, but I removed them. If VT-d doesn't need
iommu_private either, maybe we can remove it entirely?

In any case I agree that device drivers should only need to know about
evt->fault.

> We
> need to inject the data to the guest and the guest will send the
> unmodified data back along with response.

By the way, does private_data need to go back through the
iommu_page_response() path? The current series doesn't do that.

> The private data can be used
> to tag internal device/iommu context.

> I think we can do the way you said by keeping them within iommu core
> and recover it based on the response but that would require tracking
> each fault report, right?

That's already the case: we decided in thread [1] to track recoverable
faults in the IOMMU core, in order to check that the response is sane
and to set a quota and/or timeout. (I didn't include your timeout
patches here because I think they need a little more work. They are on
my sva/api branch.)

I already dropped iommu_private from the iommu_page_response structure.
In patch 4 iommu_page_response() retrieves the fault event and pass the
corresponding iommu_private back to the IOMMU driver.

[1] https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/

Thanks,
Jean

> 
> If we pass on the private data, we only need to check if the response
> belong to the device but not exact match of a specific fault since the
> damage is contained in the assigned device. In case of injection
> fault into the guest, the response will come asynchronously after the
> handler completes.

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jean-Philippe Brucker]

On 24/05/2019 14:49, Jacob Pan wrote:
> On Thu, 23 May 2019 19:43:46 +0100
> Robin Murphy <robin.murphy@arm.com> wrote:
>>> +/**
>>> + * struct iommu_fault_event - Generic fault event
>>> + *
>>> + * Can represent recoverable faults such as a page requests or
>>> + * unrecoverable faults such as DMA or IRQ remapping faults.
>>> + *
>>> + * @fault: fault descriptor
>>> + * @iommu_private: used by the IOMMU driver for storing
>>> fault-specific
>>> + *                 data. Users should not modify this field before
>>> + *                 sending the fault response.  
>>
>> Sorry if I'm a bit late to the party, but given that description, if 
>> users aren't allowed to touch this then why expose it to them at all? 
>> I.e. why not have iommu_report_device_fault() pass just the fault
>> itself to the fault handler:
>>
>> 	ret = fparam->handler(&evt->fault, fparam->data);
>>
>> and let the IOMMU core/drivers decapsulate it again later if need be. 
>> AFAICS drivers could also just embed the entire generic event in
>> their own private structure anyway, just as we do for domains.
>>
> I can't remember all the discussion history but I think iommu_private
> is used similarly to the page request private data (device private).

Hm yes, we already have iommu_fault_page_request::private_data for that.
I think I used to stash flags in iommu_private (is_stall and
needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them
from the device structure, but I removed them. If VT-d doesn't need
iommu_private either, maybe we can remove it entirely?

In any case I agree that device drivers should only need to know about
evt->fault.

> We
> need to inject the data to the guest and the guest will send the
> unmodified data back along with response.

By the way, does private_data need to go back through the
iommu_page_response() path? The current series doesn't do that.

> The private data can be used
> to tag internal device/iommu context.

> I think we can do the way you said by keeping them within iommu core
> and recover it based on the response but that would require tracking
> each fault report, right?

That's already the case: we decided in thread [1] to track recoverable
faults in the IOMMU core, in order to check that the response is sane
and to set a quota and/or timeout. (I didn't include your timeout
patches here because I think they need a little more work. They are on
my sva/api branch.)

I already dropped iommu_private from the iommu_page_response structure.
In patch 4 iommu_page_response() retrieves the fault event and pass the
corresponding iommu_private back to the IOMMU driver.

[1] https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/

Thanks,
Jean

> 
> If we pass on the private data, we only need to check if the response
> belong to the device but not exact match of a specific fault since the
> damage is contained in the assigned device. In case of injection
> fault into the guest, the response will come asynchronously after the
> handler completes.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jacob Pan]

On Fri, 24 May 2019 17:14:30 +0100
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> On 24/05/2019 14:49, Jacob Pan wrote:
> > On Thu, 23 May 2019 19:43:46 +0100
> > Robin Murphy <robin.murphy@arm.com> wrote:  
> >>> +/**
> >>> + * struct iommu_fault_event - Generic fault event
> >>> + *
> >>> + * Can represent recoverable faults such as a page requests or
> >>> + * unrecoverable faults such as DMA or IRQ remapping faults.
> >>> + *
> >>> + * @fault: fault descriptor
> >>> + * @iommu_private: used by the IOMMU driver for storing
> >>> fault-specific
> >>> + *                 data. Users should not modify this field
> >>> before
> >>> + *                 sending the fault response.    
> >>
> >> Sorry if I'm a bit late to the party, but given that description,
> >> if users aren't allowed to touch this then why expose it to them
> >> at all? I.e. why not have iommu_report_device_fault() pass just
> >> the fault itself to the fault handler:
> >>
> >> 	ret = fparam->handler(&evt->fault, fparam->data);
> >>
> >> and let the IOMMU core/drivers decapsulate it again later if need
> >> be. AFAICS drivers could also just embed the entire generic event
> >> in their own private structure anyway, just as we do for domains.
> >>  
> > I can't remember all the discussion history but I think
> > iommu_private is used similarly to the page request private data
> > (device private).  
> 
> Hm yes, we already have iommu_fault_page_request::private_data for
> that. I think I used to stash flags in iommu_private (is_stall and
> needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them
> from the device structure, but I removed them. If VT-d doesn't need
> iommu_private either, maybe we can remove it entirely?
> 
yes, vt-d does not use or plan to use it.
> In any case I agree that device drivers should only need to know about
> evt->fault.
> 
> > We
> > need to inject the data to the guest and the guest will send the
> > unmodified data back along with response.  
> 
> By the way, does private_data need to go back through the
> iommu_page_response() path? The current series doesn't do that.
> 
yes, private needs to go back in the page_response path. perhaps just
send the response with the match prm?
-ret = domain->ops->page_response(dev, msg, evt->iommu_private);
+ret = domain->ops->page_response(dev, msg, prm);


> > The private data can be used
> > to tag internal device/iommu context.  
> 
> > I think we can do the way you said by keeping them within iommu core
> > and recover it based on the response but that would require tracking
> > each fault report, right?  
> 
> That's already the case: we decided in thread [1] to track recoverable
> faults in the IOMMU core, in order to check that the response is sane
> and to set a quota and/or timeout. (I didn't include your timeout
> patches here because I think they need a little more work. They are on
> my sva/api branch.)
> 
> I already dropped iommu_private from the iommu_page_response
> structure. In patch 4 iommu_page_response() retrieves the fault event
> and pass the corresponding iommu_private back to the IOMMU driver.
> 
> [1]
> https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/
>
great, as planned :) I lost track where the discussion ended and
haven't read the latest code. Thanks

> Thanks,
> Jean
> 
> > 
> > If we pass on the private data, we only need to check if the
> > response belong to the device but not exact match of a specific
> > fault since the damage is contained in the assigned device. In case
> > of injection fault into the guest, the response will come
> > asynchronously after the handler completes.  

[Jacob Pan]

Re: [PATCH 2/4] iommu: Introduce device fault data

[Jacob Pan]

On Fri, 24 May 2019 17:14:30 +0100
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> On 24/05/2019 14:49, Jacob Pan wrote:
> > On Thu, 23 May 2019 19:43:46 +0100
> > Robin Murphy <robin.murphy@arm.com> wrote:  
> >>> +/**
> >>> + * struct iommu_fault_event - Generic fault event
> >>> + *
> >>> + * Can represent recoverable faults such as a page requests or
> >>> + * unrecoverable faults such as DMA or IRQ remapping faults.
> >>> + *
> >>> + * @fault: fault descriptor
> >>> + * @iommu_private: used by the IOMMU driver for storing
> >>> fault-specific
> >>> + *                 data. Users should not modify this field
> >>> before
> >>> + *                 sending the fault response.    
> >>
> >> Sorry if I'm a bit late to the party, but given that description,
> >> if users aren't allowed to touch this then why expose it to them
> >> at all? I.e. why not have iommu_report_device_fault() pass just
> >> the fault itself to the fault handler:
> >>
> >> 	ret = fparam->handler(&evt->fault, fparam->data);
> >>
> >> and let the IOMMU core/drivers decapsulate it again later if need
> >> be. AFAICS drivers could also just embed the entire generic event
> >> in their own private structure anyway, just as we do for domains.
> >>  
> > I can't remember all the discussion history but I think
> > iommu_private is used similarly to the page request private data
> > (device private).  
> 
> Hm yes, we already have iommu_fault_page_request::private_data for
> that. I think I used to stash flags in iommu_private (is_stall and
> needs_pasid), so that the SMMUv3 driver doesn't need to go fetch them
> from the device structure, but I removed them. If VT-d doesn't need
> iommu_private either, maybe we can remove it entirely?
> 
yes, vt-d does not use or plan to use it.
> In any case I agree that device drivers should only need to know about
> evt->fault.
> 
> > We
> > need to inject the data to the guest and the guest will send the
> > unmodified data back along with response.  
> 
> By the way, does private_data need to go back through the
> iommu_page_response() path? The current series doesn't do that.
> 
yes, private needs to go back in the page_response path. perhaps just
send the response with the match prm?
-ret = domain->ops->page_response(dev, msg, evt->iommu_private);
+ret = domain->ops->page_response(dev, msg, prm);


> > The private data can be used
> > to tag internal device/iommu context.  
> 
> > I think we can do the way you said by keeping them within iommu core
> > and recover it based on the response but that would require tracking
> > each fault report, right?  
> 
> That's already the case: we decided in thread [1] to track recoverable
> faults in the IOMMU core, in order to check that the response is sane
> and to set a quota and/or timeout. (I didn't include your timeout
> patches here because I think they need a little more work. They are on
> my sva/api branch.)
> 
> I already dropped iommu_private from the iommu_page_response
> structure. In patch 4 iommu_page_response() retrieves the fault event
> and pass the corresponding iommu_private back to the IOMMU driver.
> 
> [1]
> https://lore.kernel.org/lkml/20171206112521.1edf8e9b@jacob-builder/
>
great, as planned :) I lost track where the discussion ended and
haven't read the latest code. Thanks

> Thanks,
> Jean
> 
> > 
> > If we pass on the private data, we only need to check if the
> > response belong to the device but not exact match of a specific
> > fault since the damage is contained in the assigned device. In case
> > of injection fault into the guest, the response will come
> > asynchronously after the handler completes.  

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 3/4] iommu: Introduce device fault report API

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

Traditionally, device specific faults are detected and handled within
their own device drivers. When IOMMU is enabled, faults such as DMA
related transactions are detected by IOMMU. There is no generic
reporting mechanism to report faults back to the in-kernel device
driver or the guest OS in case of assigned devices.

This patch introduces a registration API for device specific fault
handlers. This differs from the existing iommu_set_fault_handler/
report_iommu_fault infrastructures in several ways:
- it allows to report more sophisticated fault events (both
  unrecoverable faults and page request faults) due to the nature
  of the iommu_fault struct
- it is device specific and not domain specific.

The current iommu_report_device_fault() implementation only handles
the "shoot and forget" unrecoverable fault case. Handling of page
request faults or stalled faults will come later.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h |  29 ++++++++++
 2 files changed, 156 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 67ee6623f9b2..d546f7baa0d4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 		goto err_free_name;
 	}
 
+	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
+	if (!dev->iommu_param) {
+		ret = -ENOMEM;
+		goto err_free_name;
+	}
+	mutex_init(&dev->iommu_param->lock);
+
 	kobject_get(group->devices_kobj);
 
 	dev->iommu_group = group;
@@ -674,6 +681,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_unlock(&group->mutex);
 	dev->iommu_group = NULL;
 	kobject_put(group->devices_kobj);
+	kfree(dev->iommu_param);
+	dev->iommu_param = NULL;
 err_free_name:
 	kfree(device->name);
 err_remove_link:
@@ -721,6 +730,8 @@ void iommu_group_remove_device(struct device *dev)
 
 	trace_remove_device_from_group(group->id, dev);
 
+	kfree(dev->iommu_param);
+	dev->iommu_param = NULL;
 	kfree(device->name);
 	kfree(device);
 	dev->iommu_group = NULL;
@@ -854,6 +865,122 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
 }
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
+/**
+ * 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.
+ *
+ * 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 iommu_param *param = dev->iommu_param;
+	int ret = 0;
+
+	/*
+	 * Device iommu_param should have been allocated when device is
+	 * added to its iommu_group.
+	 */
+	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(struct iommu_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;
+
+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 iommu_param *param = dev->iommu_param;
+	int ret = 0;
+
+	if (!param)
+		return -EINVAL;
+
+	mutex_lock(&param->lock);
+
+	if (!param->fault_param)
+		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);
+
+/**
+ * iommu_report_device_fault() - Report fault event to device driver
+ * @dev: the device
+ * @evt: fault event data
+ *
+ * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
+ * handler.
+ *
+ * Return 0 on success, or an error.
+ */
+int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
+{
+	struct iommu_param *param = dev->iommu_param;
+	struct iommu_fault_param *fparam;
+	int ret = 0;
+
+	/* iommu_param is allocated when device is added to group */
+	if (!param || !evt)
+		return -EINVAL;
+
+	/* 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;
+	}
+	ret = fparam->handler(evt, fparam->data);
+done_unlock:
+	mutex_unlock(&param->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_report_device_fault);
+
 /**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d442f5f3fa93..f95e376a7ed3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -340,6 +340,7 @@ struct iommu_fault_param {
  *	struct iommu_fwspec	*iommu_fwspec;
  */
 struct iommu_param {
+	struct mutex lock;
 	struct iommu_fault_param *fault_param;
 };
 
@@ -432,6 +433,15 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
 					 struct notifier_block *nb);
 extern int iommu_group_unregister_notifier(struct iommu_group *group,
 					   struct notifier_block *nb);
+extern int iommu_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);
+
 extern int iommu_group_id(struct iommu_group *group);
 extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
@@ -740,6 +750,25 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
 	return 0;
 }
 
+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)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;
-- 
2.21.0

[PATCH 3/4] iommu: Introduce device fault report API

[Jean-Philippe Brucker]

From: Jacob Pan <jacob.jun.pan@linux.intel.com>

Traditionally, device specific faults are detected and handled within
their own device drivers. When IOMMU is enabled, faults such as DMA
related transactions are detected by IOMMU. There is no generic
reporting mechanism to report faults back to the in-kernel device
driver or the guest OS in case of assigned devices.

This patch introduces a registration API for device specific fault
handlers. This differs from the existing iommu_set_fault_handler/
report_iommu_fault infrastructures in several ways:
- it allows to report more sophisticated fault events (both
  unrecoverable faults and page request faults) due to the nature
  of the iommu_fault struct
- it is device specific and not domain specific.

The current iommu_report_device_fault() implementation only handles
the "shoot and forget" unrecoverable fault case. Handling of page
request faults or stalled faults will come later.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
---
 drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h |  29 ++++++++++
 2 files changed, 156 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 67ee6623f9b2..d546f7baa0d4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 		goto err_free_name;
 	}
 
+	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
+	if (!dev->iommu_param) {
+		ret = -ENOMEM;
+		goto err_free_name;
+	}
+	mutex_init(&dev->iommu_param->lock);
+
 	kobject_get(group->devices_kobj);
 
 	dev->iommu_group = group;
@@ -674,6 +681,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
 	mutex_unlock(&group->mutex);
 	dev->iommu_group = NULL;
 	kobject_put(group->devices_kobj);
+	kfree(dev->iommu_param);
+	dev->iommu_param = NULL;
 err_free_name:
 	kfree(device->name);
 err_remove_link:
@@ -721,6 +730,8 @@ void iommu_group_remove_device(struct device *dev)
 
 	trace_remove_device_from_group(group->id, dev);
 
+	kfree(dev->iommu_param);
+	dev->iommu_param = NULL;
 	kfree(device->name);
 	kfree(device);
 	dev->iommu_group = NULL;
@@ -854,6 +865,122 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
 }
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
+/**
+ * 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.
+ *
+ * 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 iommu_param *param = dev->iommu_param;
+	int ret = 0;
+
+	/*
+	 * Device iommu_param should have been allocated when device is
+	 * added to its iommu_group.
+	 */
+	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(struct iommu_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;
+
+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 iommu_param *param = dev->iommu_param;
+	int ret = 0;
+
+	if (!param)
+		return -EINVAL;
+
+	mutex_lock(&param->lock);
+
+	if (!param->fault_param)
+		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);
+
+/**
+ * iommu_report_device_fault() - Report fault event to device driver
+ * @dev: the device
+ * @evt: fault event data
+ *
+ * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
+ * handler.
+ *
+ * Return 0 on success, or an error.
+ */
+int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
+{
+	struct iommu_param *param = dev->iommu_param;
+	struct iommu_fault_param *fparam;
+	int ret = 0;
+
+	/* iommu_param is allocated when device is added to group */
+	if (!param || !evt)
+		return -EINVAL;
+
+	/* 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;
+	}
+	ret = fparam->handler(evt, fparam->data);
+done_unlock:
+	mutex_unlock(&param->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_report_device_fault);
+
 /**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index d442f5f3fa93..f95e376a7ed3 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -340,6 +340,7 @@ struct iommu_fault_param {
  *	struct iommu_fwspec	*iommu_fwspec;
  */
 struct iommu_param {
+	struct mutex lock;
 	struct iommu_fault_param *fault_param;
 };
 
@@ -432,6 +433,15 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
 					 struct notifier_block *nb);
 extern int iommu_group_unregister_notifier(struct iommu_group *group,
 					   struct notifier_block *nb);
+extern int iommu_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);
+
 extern int iommu_group_id(struct iommu_group *group);
 extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
@@ -740,6 +750,25 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
 	return 0;
 }
 
+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)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;
-- 
2.21.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Robin Murphy]

On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> Traditionally, device specific faults are detected and handled within
> their own device drivers. When IOMMU is enabled, faults such as DMA
> related transactions are detected by IOMMU. There is no generic
> reporting mechanism to report faults back to the in-kernel device
> driver or the guest OS in case of assigned devices.
> 
> This patch introduces a registration API for device specific fault
> handlers. This differs from the existing iommu_set_fault_handler/
> report_iommu_fault infrastructures in several ways:
> - it allows to report more sophisticated fault events (both
>    unrecoverable faults and page request faults) due to the nature
>    of the iommu_fault struct
> - it is device specific and not domain specific.
> 
> The current iommu_report_device_fault() implementation only handles
> the "shoot and forget" unrecoverable fault case. Handling of page
> request faults or stalled faults will come later.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>   include/linux/iommu.h |  29 ++++++++++
>   2 files changed, 156 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 67ee6623f9b2..d546f7baa0d4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   		goto err_free_name;
>   	}
>   
> +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
> +	if (!dev->iommu_param) {
> +		ret = -ENOMEM;
> +		goto err_free_name;
> +	}
> +	mutex_init(&dev->iommu_param->lock);
> +

Note that this gets a bit tricky when we come to move to move the 
fwspec/ops/etc. into iommu_param, since that data can have a longer 
lifespan than the group association. I'd suggest moving this management 
out to the iommu_{probe,release}_device() level from the start, but 
maybe we're happy to come back and change things later as necessary.

Robin.

>   	kobject_get(group->devices_kobj);
>   
>   	dev->iommu_group = group;
> @@ -674,6 +681,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   	mutex_unlock(&group->mutex);
>   	dev->iommu_group = NULL;
>   	kobject_put(group->devices_kobj);
> +	kfree(dev->iommu_param);
> +	dev->iommu_param = NULL;
>   err_free_name:
>   	kfree(device->name);
>   err_remove_link:
> @@ -721,6 +730,8 @@ void iommu_group_remove_device(struct device *dev)
>   
>   	trace_remove_device_from_group(group->id, dev);
>   
> +	kfree(dev->iommu_param);
> +	dev->iommu_param = NULL;
>   	kfree(device->name);
>   	kfree(device);
>   	dev->iommu_group = NULL;
> @@ -854,6 +865,122 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
>   }
>   EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>   
> +/**
> + * 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.
> + *
> + * 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 iommu_param *param = dev->iommu_param;
> +	int ret = 0;
> +
> +	/*
> +	 * Device iommu_param should have been allocated when device is
> +	 * added to its iommu_group.
> +	 */
> +	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(struct iommu_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;
> +
> +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 iommu_param *param = dev->iommu_param;
> +	int ret = 0;
> +
> +	if (!param)
> +		return -EINVAL;
> +
> +	mutex_lock(&param->lock);
> +
> +	if (!param->fault_param)
> +		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);
> +
> +/**
> + * iommu_report_device_fault() - Report fault event to device driver
> + * @dev: the device
> + * @evt: fault event data
> + *
> + * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
> + * handler.
> + *
> + * Return 0 on success, or an error.
> + */
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
> +{
> +	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_fault_param *fparam;
> +	int ret = 0;
> +
> +	/* iommu_param is allocated when device is added to group */
> +	if (!param || !evt)
> +		return -EINVAL;
> +
> +	/* 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;
> +	}
> +	ret = fparam->handler(evt, fparam->data);
> +done_unlock:
> +	mutex_unlock(&param->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_report_device_fault);
> +
>   /**
>    * iommu_group_id - Return ID for a group
>    * @group: the group to ID
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index d442f5f3fa93..f95e376a7ed3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -340,6 +340,7 @@ struct iommu_fault_param {
>    *	struct iommu_fwspec	*iommu_fwspec;
>    */
>   struct iommu_param {
> +	struct mutex lock;
>   	struct iommu_fault_param *fault_param;
>   };
>   
> @@ -432,6 +433,15 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
>   					 struct notifier_block *nb);
>   extern int iommu_group_unregister_notifier(struct iommu_group *group,
>   					   struct notifier_block *nb);
> +extern int iommu_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);
> +
>   extern int iommu_group_id(struct iommu_group *group);
>   extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>   extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
> @@ -740,6 +750,25 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
>   	return 0;
>   }
>   
> +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)
> +{
> +	return -ENODEV;
> +}
> +
>   static inline int iommu_group_id(struct iommu_group *group)
>   {
>   	return -ENODEV;
> 

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Robin Murphy]

On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> 
> Traditionally, device specific faults are detected and handled within
> their own device drivers. When IOMMU is enabled, faults such as DMA
> related transactions are detected by IOMMU. There is no generic
> reporting mechanism to report faults back to the in-kernel device
> driver or the guest OS in case of assigned devices.
> 
> This patch introduces a registration API for device specific fault
> handlers. This differs from the existing iommu_set_fault_handler/
> report_iommu_fault infrastructures in several ways:
> - it allows to report more sophisticated fault events (both
>    unrecoverable faults and page request faults) due to the nature
>    of the iommu_fault struct
> - it is device specific and not domain specific.
> 
> The current iommu_report_device_fault() implementation only handles
> the "shoot and forget" unrecoverable fault case. Handling of page
> request faults or stalled faults will come later.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> ---
>   drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>   include/linux/iommu.h |  29 ++++++++++
>   2 files changed, 156 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 67ee6623f9b2..d546f7baa0d4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   		goto err_free_name;
>   	}
>   
> +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
> +	if (!dev->iommu_param) {
> +		ret = -ENOMEM;
> +		goto err_free_name;
> +	}
> +	mutex_init(&dev->iommu_param->lock);
> +

Note that this gets a bit tricky when we come to move to move the 
fwspec/ops/etc. into iommu_param, since that data can have a longer 
lifespan than the group association. I'd suggest moving this management 
out to the iommu_{probe,release}_device() level from the start, but 
maybe we're happy to come back and change things later as necessary.

Robin.

>   	kobject_get(group->devices_kobj);
>   
>   	dev->iommu_group = group;
> @@ -674,6 +681,8 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>   	mutex_unlock(&group->mutex);
>   	dev->iommu_group = NULL;
>   	kobject_put(group->devices_kobj);
> +	kfree(dev->iommu_param);
> +	dev->iommu_param = NULL;
>   err_free_name:
>   	kfree(device->name);
>   err_remove_link:
> @@ -721,6 +730,8 @@ void iommu_group_remove_device(struct device *dev)
>   
>   	trace_remove_device_from_group(group->id, dev);
>   
> +	kfree(dev->iommu_param);
> +	dev->iommu_param = NULL;
>   	kfree(device->name);
>   	kfree(device);
>   	dev->iommu_group = NULL;
> @@ -854,6 +865,122 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
>   }
>   EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>   
> +/**
> + * 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.
> + *
> + * 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 iommu_param *param = dev->iommu_param;
> +	int ret = 0;
> +
> +	/*
> +	 * Device iommu_param should have been allocated when device is
> +	 * added to its iommu_group.
> +	 */
> +	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(struct iommu_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;
> +
> +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 iommu_param *param = dev->iommu_param;
> +	int ret = 0;
> +
> +	if (!param)
> +		return -EINVAL;
> +
> +	mutex_lock(&param->lock);
> +
> +	if (!param->fault_param)
> +		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);
> +
> +/**
> + * iommu_report_device_fault() - Report fault event to device driver
> + * @dev: the device
> + * @evt: fault event data
> + *
> + * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
> + * handler.
> + *
> + * Return 0 on success, or an error.
> + */
> +int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
> +{
> +	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_fault_param *fparam;
> +	int ret = 0;
> +
> +	/* iommu_param is allocated when device is added to group */
> +	if (!param || !evt)
> +		return -EINVAL;
> +
> +	/* 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;
> +	}
> +	ret = fparam->handler(evt, fparam->data);
> +done_unlock:
> +	mutex_unlock(&param->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_report_device_fault);
> +
>   /**
>    * iommu_group_id - Return ID for a group
>    * @group: the group to ID
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index d442f5f3fa93..f95e376a7ed3 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -340,6 +340,7 @@ struct iommu_fault_param {
>    *	struct iommu_fwspec	*iommu_fwspec;
>    */
>   struct iommu_param {
> +	struct mutex lock;
>   	struct iommu_fault_param *fault_param;
>   };
>   
> @@ -432,6 +433,15 @@ extern int iommu_group_register_notifier(struct iommu_group *group,
>   					 struct notifier_block *nb);
>   extern int iommu_group_unregister_notifier(struct iommu_group *group,
>   					   struct notifier_block *nb);
> +extern int iommu_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);
> +
>   extern int iommu_group_id(struct iommu_group *group);
>   extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
>   extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
> @@ -740,6 +750,25 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
>   	return 0;
>   }
>   
> +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)
> +{
> +	return -ENODEV;
> +}
> +
>   static inline int iommu_group_id(struct iommu_group *group)
>   {
>   	return -ENODEV;
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Jacob Pan]

On Thu, 23 May 2019 19:56:54 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > 
> > Traditionally, device specific faults are detected and handled
> > within their own device drivers. When IOMMU is enabled, faults such
> > as DMA related transactions are detected by IOMMU. There is no
> > generic reporting mechanism to report faults back to the in-kernel
> > device driver or the guest OS in case of assigned devices.
> > 
> > This patch introduces a registration API for device specific fault
> > handlers. This differs from the existing iommu_set_fault_handler/
> > report_iommu_fault infrastructures in several ways:
> > - it allows to report more sophisticated fault events (both
> >    unrecoverable faults and page request faults) due to the nature
> >    of the iommu_fault struct
> > - it is device specific and not domain specific.
> > 
> > The current iommu_report_device_fault() implementation only handles
> > the "shoot and forget" unrecoverable fault case. Handling of page
> > request faults or stalled faults will come later.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >   drivers/iommu/iommu.c | 127
> > ++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> > 29 ++++++++++ 2 files changed, 156 insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 67ee6623f9b2..d546f7baa0d4 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> >   	}
> >   
> > +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param),
> > GFP_KERNEL);
> > +	if (!dev->iommu_param) {
> > +		ret = -ENOMEM;
> > +		goto err_free_name;
> > +	}
> > +	mutex_init(&dev->iommu_param->lock);
> > +  
> 
> Note that this gets a bit tricky when we come to move to move the 
> fwspec/ops/etc. into iommu_param, since that data can have a longer 
> lifespan than the group association. I'd suggest moving this
> management out to the iommu_{probe,release}_device() level from the
> start, but maybe we're happy to come back and change things later as
> necessary.
> 
Agreed, I can't think of any downside of moving it earlier.
> Robin.
> 
>  [...]  
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[Jacob Pan]

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Jacob Pan]

On Thu, 23 May 2019 19:56:54 +0100
Robin Murphy <robin.murphy@arm.com> wrote:

> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > 
> > Traditionally, device specific faults are detected and handled
> > within their own device drivers. When IOMMU is enabled, faults such
> > as DMA related transactions are detected by IOMMU. There is no
> > generic reporting mechanism to report faults back to the in-kernel
> > device driver or the guest OS in case of assigned devices.
> > 
> > This patch introduces a registration API for device specific fault
> > handlers. This differs from the existing iommu_set_fault_handler/
> > report_iommu_fault infrastructures in several ways:
> > - it allows to report more sophisticated fault events (both
> >    unrecoverable faults and page request faults) due to the nature
> >    of the iommu_fault struct
> > - it is device specific and not domain specific.
> > 
> > The current iommu_report_device_fault() implementation only handles
> > the "shoot and forget" unrecoverable fault case. Handling of page
> > request faults or stalled faults will come later.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> > Signed-off-by: Eric Auger <eric.auger@redhat.com>
> > ---
> >   drivers/iommu/iommu.c | 127
> > ++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> > 29 ++++++++++ 2 files changed, 156 insertions(+)
> > 
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 67ee6623f9b2..d546f7baa0d4 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group
> > *group, struct device *dev) goto err_free_name;
> >   	}
> >   
> > +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param),
> > GFP_KERNEL);
> > +	if (!dev->iommu_param) {
> > +		ret = -ENOMEM;
> > +		goto err_free_name;
> > +	}
> > +	mutex_init(&dev->iommu_param->lock);
> > +  
> 
> Note that this gets a bit tricky when we come to move to move the 
> fwspec/ops/etc. into iommu_param, since that data can have a longer 
> lifespan than the group association. I'd suggest moving this
> management out to the iommu_{probe,release}_device() level from the
> start, but maybe we're happy to come back and change things later as
> necessary.
> 
Agreed, I can't think of any downside of moving it earlier.
> Robin.
> 
>  [...]  
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Jean-Philippe Brucker]

On 23/05/2019 19:56, Robin Murphy wrote:
> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
>> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>
>> Traditionally, device specific faults are detected and handled within
>> their own device drivers. When IOMMU is enabled, faults such as DMA
>> related transactions are detected by IOMMU. There is no generic
>> reporting mechanism to report faults back to the in-kernel device
>> driver or the guest OS in case of assigned devices.
>>
>> This patch introduces a registration API for device specific fault
>> handlers. This differs from the existing iommu_set_fault_handler/
>> report_iommu_fault infrastructures in several ways:
>> - it allows to report more sophisticated fault events (both
>>    unrecoverable faults and page request faults) due to the nature
>>    of the iommu_fault struct
>> - it is device specific and not domain specific.
>>
>> The current iommu_report_device_fault() implementation only handles
>> the "shoot and forget" unrecoverable fault case. Handling of page
>> request faults or stalled faults will come later.
>>
>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/iommu.h |  29 ++++++++++
>>   2 files changed, 156 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 67ee6623f9b2..d546f7baa0d4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>>   		goto err_free_name;
>>   	}
>>   
>> +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
>> +	if (!dev->iommu_param) {
>> +		ret = -ENOMEM;
>> +		goto err_free_name;
>> +	}
>> +	mutex_init(&dev->iommu_param->lock);
>> +
> 
> Note that this gets a bit tricky when we come to move to move the 
> fwspec/ops/etc. into iommu_param, since that data can have a longer 
> lifespan than the group association. I'd suggest moving this management 
> out to the iommu_{probe,release}_device() level from the start, but 
> maybe we're happy to come back and change things later as necessary.

I'll do that, but iommu_probe_device() might still be too late.
According to of_iommu_configure() there might be cases where
iommu_probe_device() is called after iommu_fwspec_init(). So when moving
everything to iommu_param, we might need to introduce something like
iommu_get_dev_param() which allocates the param if it doesn't exist.

Thanks,
Jean

Re: [PATCH 3/4] iommu: Introduce device fault report API

[Jean-Philippe Brucker]

On 23/05/2019 19:56, Robin Murphy wrote:
> On 23/05/2019 19:06, Jean-Philippe Brucker wrote:
>> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
>>
>> Traditionally, device specific faults are detected and handled within
>> their own device drivers. When IOMMU is enabled, faults such as DMA
>> related transactions are detected by IOMMU. There is no generic
>> reporting mechanism to report faults back to the in-kernel device
>> driver or the guest OS in case of assigned devices.
>>
>> This patch introduces a registration API for device specific fault
>> handlers. This differs from the existing iommu_set_fault_handler/
>> report_iommu_fault infrastructures in several ways:
>> - it allows to report more sophisticated fault events (both
>>    unrecoverable faults and page request faults) due to the nature
>>    of the iommu_fault struct
>> - it is device specific and not domain specific.
>>
>> The current iommu_report_device_fault() implementation only handles
>> the "shoot and forget" unrecoverable fault case. Handling of page
>> request faults or stalled faults will come later.
>>
>> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
>> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> ---
>>   drivers/iommu/iommu.c | 127 ++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/iommu.h |  29 ++++++++++
>>   2 files changed, 156 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 67ee6623f9b2..d546f7baa0d4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -644,6 +644,13 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
>>   		goto err_free_name;
>>   	}
>>   
>> +	dev->iommu_param = kzalloc(sizeof(*dev->iommu_param), GFP_KERNEL);
>> +	if (!dev->iommu_param) {
>> +		ret = -ENOMEM;
>> +		goto err_free_name;
>> +	}
>> +	mutex_init(&dev->iommu_param->lock);
>> +
> 
> Note that this gets a bit tricky when we come to move to move the 
> fwspec/ops/etc. into iommu_param, since that data can have a longer 
> lifespan than the group association. I'd suggest moving this management 
> out to the iommu_{probe,release}_device() level from the start, but 
> maybe we're happy to come back and change things later as necessary.

I'll do that, but iommu_probe_device() might still be too late.
According to of_iommu_configure() there might be cases where
iommu_probe_device() is called after iommu_fwspec_init(). So when moving
everything to iommu_param, we might need to introduce something like
iommu_get_dev_param() which allocates the param if it doesn't exist.

Thanks,
Jean
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH 4/4] iommu: Add recoverable fault reporting

[Jean-Philippe Brucker]

Some IOMMU hardware features, for example PCI PRI and Arm SMMU Stall,
enable recoverable I/O page faults. Allow IOMMU drivers to report PRI Page
Requests and Stall events through the new fault reporting API. The
consumer of the fault can be either an I/O page fault handler in the host,
or a guest OS.

Once handled, the fault must be completed by sending a page response back
to the IOMMU. Add an iommu_page_response() function to complete a page
fault.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 drivers/iommu/iommu.c      | 95 +++++++++++++++++++++++++++++++++++++-
 include/linux/iommu.h      | 19 ++++++++
 include/uapi/linux/iommu.h | 34 ++++++++++++++
 3 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d546f7baa0d4..b09b3707f0e4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -872,7 +872,14 @@ EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
  * @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.
+ * fault event and data as argument. The handler should return 0 on success. If
+ * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler 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.
  */
@@ -907,6 +914,8 @@ int iommu_register_device_fault_handler(struct device *dev,
 	}
 	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);
@@ -937,6 +946,12 @@ int iommu_unregister_device_fault_handler(struct device *dev)
 	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);
@@ -953,13 +968,15 @@ EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
  * @evt: fault event data
  *
  * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
- * handler.
+ * handler. When this function fails and the fault is recoverable, it is the
+ * caller's responsibility to complete the fault.
  *
  * Return 0 on success, or an error.
  */
 int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 {
 	struct iommu_param *param = dev->iommu_param;
+	struct iommu_fault_event *evt_pending = NULL;
 	struct iommu_fault_param *fparam;
 	int ret = 0;
 
@@ -974,7 +991,27 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 		ret = -EINVAL;
 		goto done_unlock;
 	}
+
+	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
+	    (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
+		evt_pending = kmemdup(evt, sizeof(struct iommu_fault_event),
+				      GFP_KERNEL);
+		if (!evt_pending) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+		mutex_lock(&fparam->lock);
+		list_add_tail(&evt_pending->list, &fparam->faults);
+		mutex_unlock(&fparam->lock);
+	}
+
 	ret = fparam->handler(evt, fparam->data);
+	if (ret && evt_pending) {
+		mutex_lock(&fparam->lock);
+		list_del(&evt_pending->list);
+		mutex_unlock(&fparam->lock);
+		kfree(evt_pending);
+	}
 done_unlock:
 	mutex_unlock(&param->lock);
 	return ret;
@@ -1515,6 +1552,60 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+int iommu_page_response(struct device *dev,
+			struct iommu_page_response *msg)
+{
+	bool pasid_valid;
+	int ret = -EINVAL;
+	struct iommu_fault_event *evt;
+	struct iommu_fault_page_request *prm;
+	struct iommu_param *param = dev->iommu_param;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain || !domain->ops->page_response)
+		return -ENODEV;
+
+	/*
+	 * Device iommu_param should have been allocated when device is
+	 * added to its iommu_group.
+	 */
+	if (!param || !param->fault_param)
+		return -EINVAL;
+
+	/* Only send response if there is a fault report pending */
+	mutex_lock(&param->fault_param->lock);
+	if (list_empty(&param->fault_param->faults)) {
+		dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
+		goto done_unlock;
+	}
+	/*
+	 * Check if we have a matching page request pending to respond,
+	 * otherwise return -EINVAL
+	 */
+	list_for_each_entry(evt, &param->fault_param->faults, list) {
+		prm = &evt->fault.prm;
+		pasid_valid = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+
+		if ((pasid_valid && prm->pasid != msg->pasid) ||
+		    prm->grpid != msg->grpid)
+			continue;
+
+		/* Sanitize the reply */
+		msg->addr = prm->addr;
+		msg->flags = pasid_valid ? IOMMU_PAGE_RESP_PASID_VALID : 0;
+
+		ret = domain->ops->page_response(dev, msg, evt->iommu_private);
+		list_del(&evt->list);
+		kfree(evt);
+		break;
+	}
+
+done_unlock:
+	mutex_unlock(&param->fault_param->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_page_response);
+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f95e376a7ed3..a78c5f571082 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -227,6 +227,7 @@ struct iommu_sva_ops {
  * @sva_bind: Bind process address space to device
  * @sva_unbind: Unbind process address space from device
  * @sva_get_pasid: Get PASID associated to a SVA handle
+ * @page_response: handle page request response
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  */
 struct iommu_ops {
@@ -287,6 +288,10 @@ struct iommu_ops {
 	void (*sva_unbind)(struct iommu_sva *handle);
 	int (*sva_get_pasid)(struct iommu_sva *handle);
 
+	int (*page_response)(struct device *dev,
+			     struct iommu_page_response *msg,
+			     u64 iommu_private);
+
 	unsigned long pgsize_bitmap;
 };
 
@@ -311,11 +316,13 @@ struct iommu_device {
  * unrecoverable faults such as DMA or IRQ remapping faults.
  *
  * @fault: fault descriptor
+ * @list: pending fault event list, used for tracking responses
  * @iommu_private: used by the IOMMU driver for storing fault-specific
  *                 data. Users should not modify this field before
  *                 sending the fault response.
  */
 struct iommu_fault_event {
+	struct list_head list;
 	struct iommu_fault fault;
 	u64 iommu_private;
 };
@@ -324,10 +331,14 @@ 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, e.g. page 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;
 };
 
 /**
@@ -442,6 +453,8 @@ extern int iommu_unregister_device_fault_handler(struct device *dev);
 extern int iommu_report_device_fault(struct device *dev,
 				     struct iommu_fault_event *evt);
 
+extern int iommu_page_response(struct device *dev,
+			       struct iommu_page_response *msg);
 extern int iommu_group_id(struct iommu_group *group);
 extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
@@ -769,6 +782,12 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 	return -ENODEV;
 }
 
+static inline int iommu_page_response(struct device *dev,
+				      struct iommu_page_response *msg)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 796402174d6c..166500036557 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -115,4 +115,38 @@ struct iommu_fault {
 		struct iommu_fault_page_request prm;
 	};
 };
+
+/**
+ * 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
+ * @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
+ * @addr: page address
+ */
+struct iommu_page_response {
+#define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
+	__u32	flags;
+	__u32	pasid;
+	__u32	grpid;
+	__u32	code;
+	__u64	addr;
+};
+
 #endif /* _UAPI_IOMMU_H */
-- 
2.21.0

[PATCH 4/4] iommu: Add recoverable fault reporting

[Jean-Philippe Brucker]

Some IOMMU hardware features, for example PCI PRI and Arm SMMU Stall,
enable recoverable I/O page faults. Allow IOMMU drivers to report PRI Page
Requests and Stall events through the new fault reporting API. The
consumer of the fault can be either an I/O page fault handler in the host,
or a guest OS.

Once handled, the fault must be completed by sending a page response back
to the IOMMU. Add an iommu_page_response() function to complete a page
fault.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
---
 drivers/iommu/iommu.c      | 95 +++++++++++++++++++++++++++++++++++++-
 include/linux/iommu.h      | 19 ++++++++
 include/uapi/linux/iommu.h | 34 ++++++++++++++
 3 files changed, 146 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d546f7baa0d4..b09b3707f0e4 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -872,7 +872,14 @@ EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
  * @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.
+ * fault event and data as argument. The handler should return 0 on success. If
+ * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler 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.
  */
@@ -907,6 +914,8 @@ int iommu_register_device_fault_handler(struct device *dev,
 	}
 	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);
@@ -937,6 +946,12 @@ int iommu_unregister_device_fault_handler(struct device *dev)
 	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);
@@ -953,13 +968,15 @@ EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
  * @evt: fault event data
  *
  * Called by IOMMU drivers when a fault is detected, typically in a threaded IRQ
- * handler.
+ * handler. When this function fails and the fault is recoverable, it is the
+ * caller's responsibility to complete the fault.
  *
  * Return 0 on success, or an error.
  */
 int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 {
 	struct iommu_param *param = dev->iommu_param;
+	struct iommu_fault_event *evt_pending = NULL;
 	struct iommu_fault_param *fparam;
 	int ret = 0;
 
@@ -974,7 +991,27 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 		ret = -EINVAL;
 		goto done_unlock;
 	}
+
+	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
+	    (evt->fault.prm.flags & IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
+		evt_pending = kmemdup(evt, sizeof(struct iommu_fault_event),
+				      GFP_KERNEL);
+		if (!evt_pending) {
+			ret = -ENOMEM;
+			goto done_unlock;
+		}
+		mutex_lock(&fparam->lock);
+		list_add_tail(&evt_pending->list, &fparam->faults);
+		mutex_unlock(&fparam->lock);
+	}
+
 	ret = fparam->handler(evt, fparam->data);
+	if (ret && evt_pending) {
+		mutex_lock(&fparam->lock);
+		list_del(&evt_pending->list);
+		mutex_unlock(&fparam->lock);
+		kfree(evt_pending);
+	}
 done_unlock:
 	mutex_unlock(&param->lock);
 	return ret;
@@ -1515,6 +1552,60 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+int iommu_page_response(struct device *dev,
+			struct iommu_page_response *msg)
+{
+	bool pasid_valid;
+	int ret = -EINVAL;
+	struct iommu_fault_event *evt;
+	struct iommu_fault_page_request *prm;
+	struct iommu_param *param = dev->iommu_param;
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (!domain || !domain->ops->page_response)
+		return -ENODEV;
+
+	/*
+	 * Device iommu_param should have been allocated when device is
+	 * added to its iommu_group.
+	 */
+	if (!param || !param->fault_param)
+		return -EINVAL;
+
+	/* Only send response if there is a fault report pending */
+	mutex_lock(&param->fault_param->lock);
+	if (list_empty(&param->fault_param->faults)) {
+		dev_warn_ratelimited(dev, "no pending PRQ, drop response\n");
+		goto done_unlock;
+	}
+	/*
+	 * Check if we have a matching page request pending to respond,
+	 * otherwise return -EINVAL
+	 */
+	list_for_each_entry(evt, &param->fault_param->faults, list) {
+		prm = &evt->fault.prm;
+		pasid_valid = prm->flags & IOMMU_FAULT_PAGE_REQUEST_PASID_VALID;
+
+		if ((pasid_valid && prm->pasid != msg->pasid) ||
+		    prm->grpid != msg->grpid)
+			continue;
+
+		/* Sanitize the reply */
+		msg->addr = prm->addr;
+		msg->flags = pasid_valid ? IOMMU_PAGE_RESP_PASID_VALID : 0;
+
+		ret = domain->ops->page_response(dev, msg, evt->iommu_private);
+		list_del(&evt->list);
+		kfree(evt);
+		break;
+	}
+
+done_unlock:
+	mutex_unlock(&param->fault_param->lock);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iommu_page_response);
+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index f95e376a7ed3..a78c5f571082 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -227,6 +227,7 @@ struct iommu_sva_ops {
  * @sva_bind: Bind process address space to device
  * @sva_unbind: Unbind process address space from device
  * @sva_get_pasid: Get PASID associated to a SVA handle
+ * @page_response: handle page request response
  * @pgsize_bitmap: bitmap of all possible supported page sizes
  */
 struct iommu_ops {
@@ -287,6 +288,10 @@ struct iommu_ops {
 	void (*sva_unbind)(struct iommu_sva *handle);
 	int (*sva_get_pasid)(struct iommu_sva *handle);
 
+	int (*page_response)(struct device *dev,
+			     struct iommu_page_response *msg,
+			     u64 iommu_private);
+
 	unsigned long pgsize_bitmap;
 };
 
@@ -311,11 +316,13 @@ struct iommu_device {
  * unrecoverable faults such as DMA or IRQ remapping faults.
  *
  * @fault: fault descriptor
+ * @list: pending fault event list, used for tracking responses
  * @iommu_private: used by the IOMMU driver for storing fault-specific
  *                 data. Users should not modify this field before
  *                 sending the fault response.
  */
 struct iommu_fault_event {
+	struct list_head list;
 	struct iommu_fault fault;
 	u64 iommu_private;
 };
@@ -324,10 +331,14 @@ 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, e.g. page 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;
 };
 
 /**
@@ -442,6 +453,8 @@ extern int iommu_unregister_device_fault_handler(struct device *dev);
 extern int iommu_report_device_fault(struct device *dev,
 				     struct iommu_fault_event *evt);
 
+extern int iommu_page_response(struct device *dev,
+			       struct iommu_page_response *msg);
 extern int iommu_group_id(struct iommu_group *group);
 extern struct iommu_group *iommu_group_get_for_dev(struct device *dev);
 extern struct iommu_domain *iommu_group_default_domain(struct iommu_group *);
@@ -769,6 +782,12 @@ int iommu_report_device_fault(struct device *dev, struct iommu_fault_event *evt)
 	return -ENODEV;
 }
 
+static inline int iommu_page_response(struct device *dev,
+				      struct iommu_page_response *msg)
+{
+	return -ENODEV;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;
diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
index 796402174d6c..166500036557 100644
--- a/include/uapi/linux/iommu.h
+++ b/include/uapi/linux/iommu.h
@@ -115,4 +115,38 @@ struct iommu_fault {
 		struct iommu_fault_page_request prm;
 	};
 };
+
+/**
+ * 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
+ * @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
+ * @addr: page address
+ */
+struct iommu_page_response {
+#define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
+	__u32	flags;
+	__u32	pasid;
+	__u32	grpid;
+	__u32	code;
+	__u64	addr;
+};
+
 #endif /* _UAPI_IOMMU_H */
-- 
2.21.0

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu: Add recoverable fault reporting

[Jacob Pan]

On Thu, 23 May 2019 19:06:13 +0100
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> Some IOMMU hardware features, for example PCI PRI and Arm SMMU Stall,
> enable recoverable I/O page faults. Allow IOMMU drivers to report PRI
> Page Requests and Stall events through the new fault reporting API.
> The consumer of the fault can be either an I/O page fault handler in
> the host, or a guest OS.
> 
> Once handled, the fault must be completed by sending a page response
> back to the IOMMU. Add an iommu_page_response() function to complete
> a page fault.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
>  drivers/iommu/iommu.c      | 95
> +++++++++++++++++++++++++++++++++++++- include/linux/iommu.h      |
> 19 ++++++++ include/uapi/linux/iommu.h | 34 ++++++++++++++
>  3 files changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d546f7baa0d4..b09b3707f0e4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -872,7 +872,14 @@
> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>   * @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.
> + * fault event and data as argument. The handler should return 0 on
> success. If
> + * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler
> should also
> + * complete the fault by calling iommu_page_response() with one of
> the following
nit, in case of injecting into the guest, handler does not have to call
iommu_page_response() directly.
> + * 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. */
> @@ -907,6 +914,8 @@ int iommu_register_device_fault_handler(struct
> device *dev, }
>  	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);
> @@ -937,6 +946,12 @@ int iommu_unregister_device_fault_handler(struct
> device *dev) 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);
> @@ -953,13 +968,15 @@
> EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>   * @evt: fault event data
>   *
>   * Called by IOMMU drivers when a fault is detected, typically in a
> threaded IRQ
> - * handler.
> + * handler. When this function fails and the fault is recoverable,
> it is the
> + * caller's responsibility to complete the fault.
>   *
>   * Return 0 on success, or an error.
>   */
>  int iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt) {
>  	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_fault_event *evt_pending = NULL;
>  	struct iommu_fault_param *fparam;
>  	int ret = 0;
>  
> @@ -974,7 +991,27 @@ int iommu_report_device_fault(struct device
> *dev, struct iommu_fault_event *evt) ret = -EINVAL;
>  		goto done_unlock;
>  	}
> +
> +	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
> +	    (evt->fault.prm.flags &
> IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> +		evt_pending = kmemdup(evt, sizeof(struct
> iommu_fault_event),
> +				      GFP_KERNEL);
> +		if (!evt_pending) {
> +			ret = -ENOMEM;
> +			goto done_unlock;
> +		}
> +		mutex_lock(&fparam->lock);
> +		list_add_tail(&evt_pending->list, &fparam->faults);
> +		mutex_unlock(&fparam->lock);
> +	}
> +
>  	ret = fparam->handler(evt, fparam->data);
> +	if (ret && evt_pending) {
> +		mutex_lock(&fparam->lock);
> +		list_del(&evt_pending->list);
> +		mutex_unlock(&fparam->lock);
> +		kfree(evt_pending);
> +	}
>  done_unlock:
>  	mutex_unlock(&param->lock);
>  	return ret;
> @@ -1515,6 +1552,60 @@ int iommu_attach_device(struct iommu_domain
> *domain, struct device *dev) }
>  EXPORT_SYMBOL_GPL(iommu_attach_device);
>  
> +int iommu_page_response(struct device *dev,
> +			struct iommu_page_response *msg)
> +{
> +	bool pasid_valid;
> +	int ret = -EINVAL;
> +	struct iommu_fault_event *evt;
> +	struct iommu_fault_page_request *prm;
> +	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +
> +	if (!domain || !domain->ops->page_response)
> +		return -ENODEV;
> +
> +	/*
> +	 * Device iommu_param should have been allocated when device
> is
> +	 * added to its iommu_group.
> +	 */
> +	if (!param || !param->fault_param)
> +		return -EINVAL;
> +
> +	/* Only send response if there is a fault report pending */
> +	mutex_lock(&param->fault_param->lock);
> +	if (list_empty(&param->fault_param->faults)) {
> +		dev_warn_ratelimited(dev, "no pending PRQ, drop
> response\n");
> +		goto done_unlock;
> +	}
> +	/*
> +	 * Check if we have a matching page request pending to
> respond,
> +	 * otherwise return -EINVAL
> +	 */
> +	list_for_each_entry(evt, &param->fault_param->faults, list) {
> +		prm = &evt->fault.prm;
> +		pasid_valid = prm->flags &
> IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; +
> +		if ((pasid_valid && prm->pasid != msg->pasid) ||
> +		    prm->grpid != msg->grpid)
> +			continue;
> +
> +		/* Sanitize the reply */
> +		msg->addr = prm->addr;
> +		msg->flags = pasid_valid ?
> IOMMU_PAGE_RESP_PASID_VALID : 0; +
> +		ret = domain->ops->page_response(dev, msg,
> evt->iommu_private);
I guess here you could drop iommu_private in favor of prm such that
drivers such as vt-d can recover private data as needed?

> +		list_del(&evt->list);
> +		kfree(evt);
> +		break;
> +	}
> +
> +done_unlock:
> +	mutex_unlock(&param->fault_param->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_page_response);
> +
>  static void __iommu_detach_device(struct iommu_domain *domain,
>  				  struct device *dev)
>  {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f95e376a7ed3..a78c5f571082 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -227,6 +227,7 @@ struct iommu_sva_ops {
>   * @sva_bind: Bind process address space to device
>   * @sva_unbind: Unbind process address space from device
>   * @sva_get_pasid: Get PASID associated to a SVA handle
> + * @page_response: handle page request response
>   * @pgsize_bitmap: bitmap of all possible supported page sizes
>   */
>  struct iommu_ops {
> @@ -287,6 +288,10 @@ struct iommu_ops {
>  	void (*sva_unbind)(struct iommu_sva *handle);
>  	int (*sva_get_pasid)(struct iommu_sva *handle);
>  
> +	int (*page_response)(struct device *dev,
> +			     struct iommu_page_response *msg,
> +			     u64 iommu_private);
> +
>  	unsigned long pgsize_bitmap;
>  };
>  
> @@ -311,11 +316,13 @@ struct iommu_device {
>   * unrecoverable faults such as DMA or IRQ remapping faults.
>   *
>   * @fault: fault descriptor
> + * @list: pending fault event list, used for tracking responses
>   * @iommu_private: used by the IOMMU driver for storing
> fault-specific
>   *                 data. Users should not modify this field before
>   *                 sending the fault response.
>   */
>  struct iommu_fault_event {
> +	struct list_head list;
>  	struct iommu_fault fault;
>  	u64 iommu_private;
>  };
> @@ -324,10 +331,14 @@ 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, e.g. page
> 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;
>  };
>  
>  /**
> @@ -442,6 +453,8 @@ extern int
> iommu_unregister_device_fault_handler(struct device *dev); extern int
> iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt); 
> +extern int iommu_page_response(struct device *dev,
> +			       struct iommu_page_response *msg);
>  extern int iommu_group_id(struct iommu_group *group);
>  extern struct iommu_group *iommu_group_get_for_dev(struct device
> *dev); extern struct iommu_domain *iommu_group_default_domain(struct
> iommu_group *); @@ -769,6 +782,12 @@ int
> iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt) return -ENODEV; }
>  
> +static inline int iommu_page_response(struct device *dev,
> +				      struct iommu_page_response
> *msg) +{
> +	return -ENODEV;
> +}
> +
>  static inline int iommu_group_id(struct iommu_group *group)
>  {
>  	return -ENODEV;
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index 796402174d6c..166500036557 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -115,4 +115,38 @@ struct iommu_fault {
>  		struct iommu_fault_page_request prm;
>  	};
>  };
> +
> +/**
> + * 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
> + * @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
> + * @addr: page address
> + */
> +struct iommu_page_response {
> +#define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
> +	__u32	flags;
> +	__u32	pasid;
> +	__u32	grpid;
> +	__u32	code;
> +	__u64	addr;
> +};
> +
>  #endif /* _UAPI_IOMMU_H */

[Jacob Pan]

Re: [PATCH 4/4] iommu: Add recoverable fault reporting

[Jacob Pan]

On Thu, 23 May 2019 19:06:13 +0100
Jean-Philippe Brucker <jean-philippe.brucker@arm.com> wrote:

> Some IOMMU hardware features, for example PCI PRI and Arm SMMU Stall,
> enable recoverable I/O page faults. Allow IOMMU drivers to report PRI
> Page Requests and Stall events through the new fault reporting API.
> The consumer of the fault can be either an I/O page fault handler in
> the host, or a guest OS.
> 
> Once handled, the fault must be completed by sending a page response
> back to the IOMMU. Add an iommu_page_response() function to complete
> a page fault.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe.brucker@arm.com>
> ---
>  drivers/iommu/iommu.c      | 95
> +++++++++++++++++++++++++++++++++++++- include/linux/iommu.h      |
> 19 ++++++++ include/uapi/linux/iommu.h | 34 ++++++++++++++
>  3 files changed, 146 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index d546f7baa0d4..b09b3707f0e4 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -872,7 +872,14 @@
> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>   * @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.
> + * fault event and data as argument. The handler should return 0 on
> success. If
> + * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler
> should also
> + * complete the fault by calling iommu_page_response() with one of
> the following
nit, in case of injecting into the guest, handler does not have to call
iommu_page_response() directly.
> + * 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. */
> @@ -907,6 +914,8 @@ int iommu_register_device_fault_handler(struct
> device *dev, }
>  	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);
> @@ -937,6 +946,12 @@ int iommu_unregister_device_fault_handler(struct
> device *dev) 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);
> @@ -953,13 +968,15 @@
> EXPORT_SYMBOL_GPL(iommu_unregister_device_fault_handler);
>   * @evt: fault event data
>   *
>   * Called by IOMMU drivers when a fault is detected, typically in a
> threaded IRQ
> - * handler.
> + * handler. When this function fails and the fault is recoverable,
> it is the
> + * caller's responsibility to complete the fault.
>   *
>   * Return 0 on success, or an error.
>   */
>  int iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt) {
>  	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_fault_event *evt_pending = NULL;
>  	struct iommu_fault_param *fparam;
>  	int ret = 0;
>  
> @@ -974,7 +991,27 @@ int iommu_report_device_fault(struct device
> *dev, struct iommu_fault_event *evt) ret = -EINVAL;
>  		goto done_unlock;
>  	}
> +
> +	if (evt->fault.type == IOMMU_FAULT_PAGE_REQ &&
> +	    (evt->fault.prm.flags &
> IOMMU_FAULT_PAGE_REQUEST_LAST_PAGE)) {
> +		evt_pending = kmemdup(evt, sizeof(struct
> iommu_fault_event),
> +				      GFP_KERNEL);
> +		if (!evt_pending) {
> +			ret = -ENOMEM;
> +			goto done_unlock;
> +		}
> +		mutex_lock(&fparam->lock);
> +		list_add_tail(&evt_pending->list, &fparam->faults);
> +		mutex_unlock(&fparam->lock);
> +	}
> +
>  	ret = fparam->handler(evt, fparam->data);
> +	if (ret && evt_pending) {
> +		mutex_lock(&fparam->lock);
> +		list_del(&evt_pending->list);
> +		mutex_unlock(&fparam->lock);
> +		kfree(evt_pending);
> +	}
>  done_unlock:
>  	mutex_unlock(&param->lock);
>  	return ret;
> @@ -1515,6 +1552,60 @@ int iommu_attach_device(struct iommu_domain
> *domain, struct device *dev) }
>  EXPORT_SYMBOL_GPL(iommu_attach_device);
>  
> +int iommu_page_response(struct device *dev,
> +			struct iommu_page_response *msg)
> +{
> +	bool pasid_valid;
> +	int ret = -EINVAL;
> +	struct iommu_fault_event *evt;
> +	struct iommu_fault_page_request *prm;
> +	struct iommu_param *param = dev->iommu_param;
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> +
> +	if (!domain || !domain->ops->page_response)
> +		return -ENODEV;
> +
> +	/*
> +	 * Device iommu_param should have been allocated when device
> is
> +	 * added to its iommu_group.
> +	 */
> +	if (!param || !param->fault_param)
> +		return -EINVAL;
> +
> +	/* Only send response if there is a fault report pending */
> +	mutex_lock(&param->fault_param->lock);
> +	if (list_empty(&param->fault_param->faults)) {
> +		dev_warn_ratelimited(dev, "no pending PRQ, drop
> response\n");
> +		goto done_unlock;
> +	}
> +	/*
> +	 * Check if we have a matching page request pending to
> respond,
> +	 * otherwise return -EINVAL
> +	 */
> +	list_for_each_entry(evt, &param->fault_param->faults, list) {
> +		prm = &evt->fault.prm;
> +		pasid_valid = prm->flags &
> IOMMU_FAULT_PAGE_REQUEST_PASID_VALID; +
> +		if ((pasid_valid && prm->pasid != msg->pasid) ||
> +		    prm->grpid != msg->grpid)
> +			continue;
> +
> +		/* Sanitize the reply */
> +		msg->addr = prm->addr;
> +		msg->flags = pasid_valid ?
> IOMMU_PAGE_RESP_PASID_VALID : 0; +
> +		ret = domain->ops->page_response(dev, msg,
> evt->iommu_private);
I guess here you could drop iommu_private in favor of prm such that
drivers such as vt-d can recover private data as needed?

> +		list_del(&evt->list);
> +		kfree(evt);
> +		break;
> +	}
> +
> +done_unlock:
> +	mutex_unlock(&param->fault_param->lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_page_response);
> +
>  static void __iommu_detach_device(struct iommu_domain *domain,
>  				  struct device *dev)
>  {
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f95e376a7ed3..a78c5f571082 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -227,6 +227,7 @@ struct iommu_sva_ops {
>   * @sva_bind: Bind process address space to device
>   * @sva_unbind: Unbind process address space from device
>   * @sva_get_pasid: Get PASID associated to a SVA handle
> + * @page_response: handle page request response
>   * @pgsize_bitmap: bitmap of all possible supported page sizes
>   */
>  struct iommu_ops {
> @@ -287,6 +288,10 @@ struct iommu_ops {
>  	void (*sva_unbind)(struct iommu_sva *handle);
>  	int (*sva_get_pasid)(struct iommu_sva *handle);
>  
> +	int (*page_response)(struct device *dev,
> +			     struct iommu_page_response *msg,
> +			     u64 iommu_private);
> +
>  	unsigned long pgsize_bitmap;
>  };
>  
> @@ -311,11 +316,13 @@ struct iommu_device {
>   * unrecoverable faults such as DMA or IRQ remapping faults.
>   *
>   * @fault: fault descriptor
> + * @list: pending fault event list, used for tracking responses
>   * @iommu_private: used by the IOMMU driver for storing
> fault-specific
>   *                 data. Users should not modify this field before
>   *                 sending the fault response.
>   */
>  struct iommu_fault_event {
> +	struct list_head list;
>  	struct iommu_fault fault;
>  	u64 iommu_private;
>  };
> @@ -324,10 +331,14 @@ 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, e.g. page
> 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;
>  };
>  
>  /**
> @@ -442,6 +453,8 @@ extern int
> iommu_unregister_device_fault_handler(struct device *dev); extern int
> iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt); 
> +extern int iommu_page_response(struct device *dev,
> +			       struct iommu_page_response *msg);
>  extern int iommu_group_id(struct iommu_group *group);
>  extern struct iommu_group *iommu_group_get_for_dev(struct device
> *dev); extern struct iommu_domain *iommu_group_default_domain(struct
> iommu_group *); @@ -769,6 +782,12 @@ int
> iommu_report_device_fault(struct device *dev, struct
> iommu_fault_event *evt) return -ENODEV; }
>  
> +static inline int iommu_page_response(struct device *dev,
> +				      struct iommu_page_response
> *msg) +{
> +	return -ENODEV;
> +}
> +
>  static inline int iommu_group_id(struct iommu_group *group)
>  {
>  	return -ENODEV;
> diff --git a/include/uapi/linux/iommu.h b/include/uapi/linux/iommu.h
> index 796402174d6c..166500036557 100644
> --- a/include/uapi/linux/iommu.h
> +++ b/include/uapi/linux/iommu.h
> @@ -115,4 +115,38 @@ struct iommu_fault {
>  		struct iommu_fault_page_request prm;
>  	};
>  };
> +
> +/**
> + * 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
> + * @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
> + * @addr: page address
> + */
> +struct iommu_page_response {
> +#define IOMMU_PAGE_RESP_PASID_VALID	(1 << 0)
> +	__u32	flags;
> +	__u32	pasid;
> +	__u32	grpid;
> +	__u32	code;
> +	__u64	addr;
> +};
> +
>  #endif /* _UAPI_IOMMU_H */

[Jacob Pan]
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH 4/4] iommu: Add recoverable fault reporting

[Jean-Philippe Brucker]

On 24/05/2019 19:14, Jacob Pan wrote:
> On Thu, 23 May 2019 19:06:13 +0100
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index d546f7baa0d4..b09b3707f0e4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -872,7 +872,14 @@
>> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>>   * @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.
>> + * fault event and data as argument. The handler should return 0 on
>> success. If
>> + * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler
>> should also
>> + * complete the fault by calling iommu_page_response() with one of
>> the following
> nit, in case of injecting into the guest, handler does not have to call
> iommu_page_response() directly.

True, I'll think of a better wording. Maybe just s/handler/consumer/
although we didn't define consumer anywhere


>> IOMMU_PAGE_RESP_PASID_VALID : 0; +
>> +		ret = domain->ops->page_response(dev, msg,
>> evt->iommu_private);
> I guess here you could drop iommu_private in favor of prm such that
> drivers such as vt-d can recover private data as needed?

Yes, will change this

Thanks,
Jean

Re: [PATCH 4/4] iommu: Add recoverable fault reporting

[Jean-Philippe Brucker]

On 24/05/2019 19:14, Jacob Pan wrote:
> On Thu, 23 May 2019 19:06:13 +0100
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index d546f7baa0d4..b09b3707f0e4 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -872,7 +872,14 @@
>> EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
>>   * @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.
>> + * fault event and data as argument. The handler should return 0 on
>> success. If
>> + * the fault is recoverable (IOMMU_FAULT_PAGE_REQ), the handler
>> should also
>> + * complete the fault by calling iommu_page_response() with one of
>> the following
> nit, in case of injecting into the guest, handler does not have to call
> iommu_page_response() directly.

True, I'll think of a better wording. Maybe just s/handler/consumer/
although we didn't define consumer anywhere


>> IOMMU_PAGE_RESP_PASID_VALID : 0; +
>> +		ret = domain->ops->page_response(dev, msg,
>> evt->iommu_private);
> I guess here you could drop iommu_private in favor of prm such that
> drivers such as vt-d can recover private data as needed?

Yes, will change this

Thanks,
Jean
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu