IOMMU driver changes for shared virtual memory virtualization

Virtual IOMMU was proposed to support Shared Virtual Memory (SVM) use case in the guest:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05311.html

As part of the proposed architecture, when a SVM capable PCI device is assigned to a guest, nested mode is turned on. Guest owns the first level page tables (request with PASID) and performs GVA->GPA translation. Second level page tables are owned by the host for GPA->HPA translation for both request with and without PASID.

A new IOMMU driver interface is therefore needed to perform tasks as follows:
* Enable nested translation and appropriate translation type
* Assign guest PASID table pointer (in GPA) and size to host IOMMU

This patch introduces new functions called iommu_(un)bind_pasid_table() to IOMMU APIs. Architecture specific IOMMU function can be added later to perform the specific steps for binding pasid table of assigned devices.

This patch also adds model definition in iommu.h. It would be used to check if the bind request is from a compatible entity. e.g. a bind request from an intel_iommu emulator may not be supported by an ARM SMMU driver.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c      | 19 +++++++++++++++++++
 include/linux/iommu.h      | 23 +++++++++++++++++++++++
 include/uapi/linux/iommu.h | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

drivers/iommu/iommu.c +19 -0

index cf7ca7e..494309b 100644
@@ -1328,6 +1328,25 @@ int iommu_attach_device(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+int iommu_bind_pasid_table(struct iommu_domain *domain, struct device *dev,
+			struct pasid_table_info *pasidt_binfo)
+{
+	if (unlikely(!domain->ops->bind_pasid_table))
+		return -EINVAL;
+
+	return domain->ops->bind_pasid_table(domain, dev, pasidt_binfo);
+}
+EXPORT_SYMBOL_GPL(iommu_bind_pasid_table);
+
+int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device *dev)
+{
+	if (unlikely(!domain->ops->unbind_pasid_table))
+		return -EINVAL;
+
+	return domain->ops->unbind_pasid_table(domain, dev);
+}
+EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table);
+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {

include/linux/iommu.h +23 -0

index 2cb54ad..7122add 100644
@@ -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)

@@ -183,6 +184,8 @@ struct iommu_resv_region {
  * @domain_get_windows: Return the number of windows for a domain
  * @of_xlate: add OF master IDs to iommu grouping
  * @pgsize_bitmap: bitmap of all possible supported page sizes
+ * @bind_pasid_table: bind pasid table pointer for guest SVM
+ * @unbind_pasid_table: unbind pasid table pointer and restore defaults
  */
 struct iommu_ops {
 	bool (*capable)(enum iommu_cap);

@@ -225,6 +228,10 @@ struct iommu_ops {
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
 	int (*of_xlate)(struct device *dev, struct of_phandle_args *args);
+	int (*bind_pasid_table)(struct iommu_domain *domain, struct device *dev,
+				struct pasid_table_info *pasidt_binfo);
+	int (*unbind_pasid_table)(struct iommu_domain *domain,
+				struct device *dev);
 
 	unsigned long pgsize_bitmap;
 };

@@ -282,6 +289,10 @@ extern int iommu_attach_device(struct iommu_domain *domain,
 			       struct device *dev);
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
+extern int iommu_bind_pasid_table(struct iommu_domain *domain,
+		struct device *dev, struct pasid_table_info *pasidt_binfo);
+extern int iommu_unbind_pasid_table(struct iommu_domain *domain,
+				struct device *dev);
 extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);

@@ -637,6 +648,18 @@ const struct iommu_ops *iommu_ops_from_fwnode(struct fwnode_handle *fwnode)
 	return NULL;
 }
 
+static inline
+int iommu_bind_pasid_table(struct iommu_domain *domain, struct device *dev,
+			struct pasid_table_info *pasidt_binfo)
+{
+	return -EINVAL;
+}
+static inline
+int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device *dev)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

include/uapi/linux/iommu.h new file +6 -0

new file mode 100644
index 0000000..5ef0e7c
@@ -0,0 +1,33 @@
+/*
+ * IOMMU user API definitions
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as

include/uapi/linux/iommu.h new file +6 -0

+	__u32	length;
+	__u8	opaque[];
+};
+
+
+#endif /* _UAPI_IOMMU_H */

Quoted 156 lines · On Wed, 14 Jun 2017 15:22:55 -0700 Jacob Pan <[email protected]>…

Virtual IOMMU was proposed to support Shared Virtual Memory (SVM) use case in the guest:
https://lists.gnu.org/archive/html/qemu-devel/2016-11/msg05311.html

As part of the proposed architecture, when a SVM capable PCI device is assigned to a guest, nested mode is turned on. Guest owns the first level page tables (request with PASID) and performs GVA->GPA translation. Second level page tables are owned by the host for GPA->HPA translation for both request with and without PASID.

A new IOMMU driver interface is therefore needed to perform tasks as follows:
* Enable nested translation and appropriate translation type
* Assign guest PASID table pointer (in GPA) and size to host IOMMU

This patch introduces new functions called iommu_(un)bind_pasid_table() to IOMMU APIs. Architecture specific IOMMU function can be added later to perform the specific steps for binding pasid table of assigned devices.

This patch also adds model definition in iommu.h. It would be used to check if the bind request is from a compatible entity. e.g. a bind request from an intel_iommu emulator may not be supported by an ARM SMMU driver.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Liu, Yi L <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c      | 19 +++++++++++++++++++
 include/linux/iommu.h      | 23 +++++++++++++++++++++++
 include/uapi/linux/iommu.h | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

drivers/iommu/iommu.c +17 -0

index cf7ca7e..494309b 100644
@@ -1328,6 +1328,25 @@ int iommu_attach_device(struct iommu_domain
 *domain, struct device *dev) }
 EXPORT_SYMBOL_GPL(iommu_attach_device);
 
+int iommu_bind_pasid_table(struct iommu_domain *domain, struct
 device *dev,
+			struct pasid_table_info *pasidt_binfo)
+{
+	if (unlikely(!domain->ops->bind_pasid_table))
+		return -EINVAL;
+
+	return domain->ops->bind_pasid_table(domain, dev,
 pasidt_binfo); +}
+EXPORT_SYMBOL_GPL(iommu_bind_pasid_table);
+
+int iommu_unbind_pasid_table(struct iommu_domain *domain, struct
 device *dev) +{
+	if (unlikely(!domain->ops->unbind_pasid_table))
+		return -EINVAL;
+
+	return domain->ops->unbind_pasid_table(domain, dev);
+}
+EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table);
+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {

include/linux/iommu.h +3 -0

index 2cb54ad..7122add 100644
@@ -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)

@@ -183,6 +184,8 @@ struct iommu_resv_region {
  * @domain_get_windows: Return the number of windows for a domain
  * @of_xlate: add OF master IDs to iommu grouping
  * @pgsize_bitmap: bitmap of all possible supported page sizes
+ * @bind_pasid_table: bind pasid table pointer for guest SVM
+ * @unbind_pasid_table: unbind pasid table pointer and restore

defaults */
struct iommu_ops {

include/linux/iommu.h +4 -0

 	bool (*capable)(enum iommu_cap);

@@ -225,6 +228,10 @@ struct iommu_ops {
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 
 	int (*of_xlate)(struct device *dev, struct of_phandle_args
 *args);
+	int (*bind_pasid_table)(struct iommu_domain *domain,
 struct device *dev,
+				struct pasid_table_info
 *pasidt_binfo);
+	int (*unbind_pasid_table)(struct iommu_domain *domain,
+				struct device *dev);

include/linux/iommu.h +3 -0

 	unsigned long pgsize_bitmap;
 };

@@ -282,6 +289,10 @@ extern int iommu_attach_device(struct
 iommu_domain *domain, struct device *dev);
 extern void iommu_detach_device(struct iommu_domain *domain,
 				struct device *dev);
+extern int iommu_bind_pasid_table(struct iommu_domain *domain,
+		struct device *dev, struct pasid_table_info
 *pasidt_binfo); +extern int iommu_unbind_pasid_table(struct
 iommu_domain *domain,
+				struct device *dev);
 extern struct iommu_domain *iommu_get_domain_for_dev(struct device

*dev); extern int iommu_map(struct iommu_domain *domain, unsigned long iova, phys_addr_t paddr, size_t size, int prot);

include/linux/iommu.h +11 -0

@@ -637,6 +648,18 @@ const struct iommu_ops
 *iommu_ops_from_fwnode(struct fwnode_handle *fwnode) return NULL;
 }
 
+static inline
+int iommu_bind_pasid_table(struct iommu_domain *domain, struct
 device *dev,
+			struct pasid_table_info *pasidt_binfo)
+{
+	return -EINVAL;
+}
+static inline
+int iommu_unbind_pasid_table(struct iommu_domain *domain, struct
 device *dev) +{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

include/uapi/linux/iommu.h new file +5 -0

new file mode 100644
index 0000000..5ef0e7c
@@ -0,0 +1,33 @@
+/*
+ * IOMMU user API definitions
+ *
+ *
+ * This program is free software; you can redistribute it and/or

modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */

include/uapi/linux/iommu.h new file +6 -0

+
+#ifndef _UAPI_IOMMU_H
+#define _UAPI_IOMMU_H
+
+/**
+ * PASID table data used to bind guest PASID table to the host

Quoted 3 lines

IOMMU. This will
+ * enable guest managed first level page tables.
+ * @ptr PASID table pointer in GPA

GPA? I'm confused how the host physical IOMMU needs the guest

physical address of the PASID table here. The first level translation does GVA to GPA lookup, but doesn't that translation still come from a table that the IOMMU references via a host physical address? If not, is the address space of this pointer implementation/architecture specific? In general I think it's good policy to not make the interface specific to the VM use case. After all, vfio is a userspace driver interface and device assignment is just one use case.

Quoted 2 lines

+ * @size size of the guest PASID table, must be <= host
table size

Presumably in bytes, best to say so.

Quoted 2 lines

+ * @model magic number tells vendor apart
+ * @length length of the opaque data

Also in bytes.

Quoted 1 line

+ * @opaque architecture specific IOMMU data

s/architecture/model/?

include/uapi/linux/iommu.h line 27

Quoted 7 lines

+ */

include/uapi/linux/iommu.h new file +6 -0

+struct pasid_table_info {
+	__u64	ptr;
+	__u64	size;
+	__u32	model;
+#define INTEL_IOMMU	(1 << 0)
+#define ARM_SMMU	(1 << 1)  

Why are we using this as a bit field rather than an enum? Does it make sense for model to be (INTEL_IOMMU|ARM_SMMU)?

include/uapi/linux/iommu.h line 33

Quoted 6 lines

include/uapi/linux/iommu.h new file +6 -0

+	__u32	length;
+	__u8	opaque[];
+};
+
+
+#endif /* _UAPI_IOMMU_H */  

Add Intel VT-d ops to the generic iommu_bind_pasid_table API functions.

The primary use case is for direct assignment of SVM capable device. Originated from emulated IOMMU in the guest, the request goes through many layers (e.g. VFIO). Upon calling host IOMMU driver, caller passes guest PASID table pointer (GPA) and size.

Device context table entry is modified by Intel IOMMU specific bind_pasid_table function. This will turn on nesting mode and matching translation type.

The unbind operation restores default context mapping.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/intel-iommu.c   | 109 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma_remapping.h |   1 +
 2 files changed, 110 insertions(+)

drivers/iommu/intel-iommu.c +6 -0

index fc2765c..1d5d9ab9 100644
@@ -5430,6 +5430,111 @@ struct intel_iommu *intel_svm_device_to_iommu(struct device *dev)
 
 	return iommu;
 }
+
+static int intel_iommu_bind_pasid_table(struct iommu_domain *domain,
+		struct device *dev, struct pasid_table_info *pasidt_binfo)
+{
+	struct intel_iommu *iommu;
+	struct context_entry *context;

drivers/iommu/intel-iommu.c +6 -0

+	info = dev->archdata.iommu;
+	if (!info || !info->pasid_supported) {
+		pr_err("PCI %04x:%02x:%02x.%d: has no PASID support\n",
+			       pci_domain_nr(pdev->bus), bus, PCI_SLOT(devfn),
+			       PCI_FUNC(devfn));
+		ret = -EINVAL;

drivers/iommu/intel-iommu.c +36 -0

+	}
+	/* Anticipate guest to use SVM and owns the first level */
+	ctx_lo = context[0].lo;
+	ctx_lo |= CONTEXT_NESTE;
+	ctx_lo |= CONTEXT_PRS;
+	ctx_lo |= CONTEXT_PASIDE;
+	ctx_lo &= ~CONTEXT_TT_MASK;
+	ctx_lo |= CONTEXT_TT_DEV_IOTLB << 2;
+	context[0].lo = ctx_lo;
+
+	/* Assign guest PASID table pointer and size */
+	ctx_lo = (pasidt_binfo->ptr & VTD_PAGE_MASK) | pasidt_binfo->size;
+	context[1].lo = ctx_lo;
+	/* make sure context entry is updated before flushing */
+	wmb();
+	did = dmar_domain->iommu_did[iommu->seq_id];
+	iommu->flush.flush_context(iommu, did,
+				(((u16)bus) << 8) | devfn,
+				DMA_CCMD_MASK_NOBIT,
+				DMA_CCMD_DEVICE_INVL);
+	iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
+	spin_unlock_irqrestore(&iommu->lock, flags);
+
+
+out:
+	return ret;
+}
+
+static int intel_iommu_unbind_pasid_table(struct iommu_domain *domain,
+					struct device *dev)
+{
+	struct intel_iommu *iommu;
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	u8 bus, devfn;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);

drivers/iommu/intel-iommu.c +7 -0

+	domain_context_clear(iommu, dev);
+	return domain_context_mapping_one(dmar_domain, iommu, bus, devfn);
+}
 #endif /* CONFIG_INTEL_IOMMU_SVM */
 
 const struct iommu_ops intel_iommu_ops = {

@@ -5438,6 +5543,10 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_free		= intel_iommu_domain_free,
 	.attach_dev		= intel_iommu_attach_device,
 	.detach_dev		= intel_iommu_detach_device,
+#ifdef CONFIG_INTEL_IOMMU_SVM
+	.bind_pasid_table	= intel_iommu_bind_pasid_table,
+	.unbind_pasid_table	= intel_iommu_unbind_pasid_table,
+#endif
 	.map			= intel_iommu_map,
 	.unmap			= intel_iommu_unmap,
 	.map_sg			= default_iommu_map_sg,

include/linux/dma_remapping.h +1 -0

index 9088407..85367b7 100644
@@ -27,6 +27,7 @@
 
 #define CONTEXT_DINVE		(1ULL << 8)
 #define CONTEXT_PRS		(1ULL << 9)
+#define CONTEXT_NESTE		(1ULL << 10)
 #define CONTEXT_PASIDE		(1ULL << 11)
 
 struct intel_iommu;

Quoted 44 lines · On Wed, 14 Jun 2017 15:22:56 -0700 Jacob Pan <[email protected]>…

Add Intel VT-d ops to the generic iommu_bind_pasid_table API functions.

The primary use case is for direct assignment of SVM capable device. Originated from emulated IOMMU in the guest, the request goes through many layers (e.g. VFIO). Upon calling host IOMMU driver, caller passes guest PASID table pointer (GPA) and size.

Device context table entry is modified by Intel IOMMU specific bind_pasid_table function. This will turn on nesting mode and matching translation type.

The unbind operation restores default context mapping.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Liu, Yi L <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/intel-iommu.c   | 109

++++++++++++++++++++++++++++++++++++++++++
include/linux/dma_remapping.h | 1 + 2 files changed, 110
insertions(+)

(quoted diff) +0 -0

b/drivers/iommu/intel-iommu.c index fc2765c..1d5d9ab9 100644

drivers/iommu/intel-iommu.c +11 -0

@@ -5430,6 +5430,111 @@ struct intel_iommu
 *intel_svm_device_to_iommu(struct device *dev) 
 	return iommu;
 }
+
+static int intel_iommu_bind_pasid_table(struct iommu_domain
 *domain,
+		struct device *dev, struct pasid_table_info
 *pasidt_binfo) +{
+	struct intel_iommu *iommu;
+	struct context_entry *context;
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct device_domain_info *info;
+	struct pci_dev *pdev;
+	u8 bus, devfn;
+	u16 did, *sid;
+	int ret = 0;

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev)

Quoted 5 lines

drivers/iommu/intel-iommu.c +4 -0

+	unsigned long flags;
+	u64 ctx_lo;
+
+	if (pasidt_binfo == NULL || pasidt_binfo->model !=
 INTEL_IOMMU) {  

Clearly model cannot be used as a bit field so it would appear wrong to limit ourselves to 32 possible models by using it that way.

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5449

Quoted 1 line

drivers/iommu/intel-iommu.c +1 -0

+		pr_warn("%s: Invalid bind request!\n", __func__);  

Let the callers deal with error reporting.

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5457

Quoted 8 lines

drivers/iommu/intel-iommu.c +8 -0

+		return -EINVAL;
+	}
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+
+	sid = (u16 *)&pasidt_binfo->opaque;  

Failed to check length.

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5465

Quoted 10 lines

drivers/iommu/intel-iommu.c +2 -0

+	/*
+	 * check SID, if it is not correct, return success to

allow looping

drivers/iommu/intel-iommu.c +6 -0

+	 * through all devices within a group
+	 */
+	if (PCI_DEVID(bus, devfn) != *sid)
+		return 0;
+
+	pdev = to_pci_dev(dev);  

Better test dev_is_pci() first!

Quoted 14 lines

drivers/iommu/intel-iommu.c +3 -0

+	info = dev->archdata.iommu;
+	if (!info || !info->pasid_supported) {
+		pr_err("PCI %04x:%02x:%02x.%d: has no PASID

support\n",

drivers/iommu/intel-iommu.c +1 -0

+			       pci_domain_nr(pdev->bus), bus,

PCI_SLOT(devfn),

drivers/iommu/intel-iommu.c +7 -0

+			       PCI_FUNC(devfn));
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (pasidt_binfo->size > intel_iommu_get_pts(iommu)) {
+		pr_err("Invalid gPASID table size %llu, host size

%lu\n",

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5481

Quoted 5 lines

drivers/iommu/intel-iommu.c +5 -0

+			pasidt_binfo->size,
+			intel_iommu_get_pts(iommu));
+		ret = -EINVAL;
+		goto out;
+	}  

Different errnos here would be more useful to code that handles the return than these pr_err()s.

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5488

Quoted 8 lines

drivers/iommu/intel-iommu.c +4 -0

+	spin_lock_irqsave(&iommu->lock, flags);
+	context = iommu_context_addr(iommu, bus, devfn, 0);
+	if (!context || !context_present(context)) {
+		pr_warn("%s: ctx not present for bus devfn

%x:%x\n",

drivers/iommu/intel-iommu.c +3 -0

+			__func__, bus, devfn);
+		spin_unlock_irqrestore(&iommu->lock, flags);
+		goto out;  

Return success?!

Quoted 44 lines

drivers/iommu/intel-iommu.c +12 -0

+	}
+	/* Anticipate guest to use SVM and owns the first level */
+	ctx_lo = context[0].lo;
+	ctx_lo |= CONTEXT_NESTE;
+	ctx_lo |= CONTEXT_PRS;
+	ctx_lo |= CONTEXT_PASIDE;
+	ctx_lo &= ~CONTEXT_TT_MASK;
+	ctx_lo |= CONTEXT_TT_DEV_IOTLB << 2;
+	context[0].lo = ctx_lo;
+
+	/* Assign guest PASID table pointer and size */
+	ctx_lo = (pasidt_binfo->ptr & VTD_PAGE_MASK) |

pasidt_binfo->size;

drivers/iommu/intel-iommu.c +9 -0

+	context[1].lo = ctx_lo;
+	/* make sure context entry is updated before flushing */
+	wmb();
+	did = dmar_domain->iommu_did[iommu->seq_id];
+	iommu->flush.flush_context(iommu, did,
+				(((u16)bus) << 8) | devfn,
+				DMA_CCMD_MASK_NOBIT,
+				DMA_CCMD_DEVICE_INVL);
+	iommu->flush.flush_iotlb(iommu, did, 0, 0,

DMA_TLB_DSI_FLUSH);

drivers/iommu/intel-iommu.c +8 -0

+	spin_unlock_irqrestore(&iommu->lock, flags);
+
+
+out:
+	return ret;
+}
+
+static int intel_iommu_unbind_pasid_table(struct iommu_domain

*domain,

drivers/iommu/intel-iommu.c +12 -0

+					struct device *dev)
+{
+	struct intel_iommu *iommu;
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	u8 bus, devfn;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+	/*
+	 * REVISIT: we might want to clear the PASID table pointer
+	 * as part of context clear operation. Currently, it leaves

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5534

Quoted 6 lines

drivers/iommu/intel-iommu.c +1 -0

+	 * stale data but should be ignored by hardware since

PASIDE

drivers/iommu/intel-iommu.c +4 -0

+	 * is clear.
+	 */
+	/* ATS will be reenabled when remapping is restored */
+	pci_disable_ats(to_pci_dev(dev));  

dev_is_pci()?

Quoted 29 lines

drivers/iommu/intel-iommu.c +2 -0

+	domain_context_clear(iommu, dev);
+	return domain_context_mapping_one(dmar_domain, iommu, bus,

devfn); +}
#endif /* CONFIG_INTEL_IOMMU_SVM */

const struct iommu_ops intel_iommu_ops = {

drivers/iommu/intel-iommu.c +3 -0

@@ -5438,6 +5543,10 @@ const struct iommu_ops intel_iommu_ops = {
 	.domain_free		= intel_iommu_domain_free,
 	.attach_dev		= intel_iommu_attach_device,
 	.detach_dev		= intel_iommu_detach_device,
+#ifdef CONFIG_INTEL_IOMMU_SVM
+	.bind_pasid_table	= intel_iommu_bind_pasid_table,
+	.unbind_pasid_table	=

intel_iommu_unbind_pasid_table, +#endif

drivers/iommu/intel-iommu.c +0 -0

 	.map			= intel_iommu_map,
 	.unmap			= intel_iommu_unmap,
 	.map_sg			= default_iommu_map_sg,

(quoted diff) +0 -0

b/include/linux/dma_remapping.h index 9088407..85367b7 100644

include/linux/dma_remapping.h +1 -0

@@ -27,6 +27,7 @@
 
 #define CONTEXT_DINVE		(1ULL << 8)
 #define CONTEXT_PRS		(1ULL << 9)
+#define CONTEXT_NESTE		(1ULL << 10)
 #define CONTEXT_PASIDE		(1ULL << 11)
 
 struct intel_iommu;  

Quoted 16 lines · On Thu, 22 Jun 2017 16:52:15 -0600 Alex Williamson…

Quoted 14 lines · On Wed, 14 Jun 2017 15:22:56 -0700 Jacob Pan…

+static int intel_iommu_unbind_pasid_table(struct iommu_domain *domain,

drivers/iommu/intel-iommu.c +12 -0

+					struct device *dev)
+{
+	struct intel_iommu *iommu;
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	u8 bus, devfn;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+	/*
+	 * REVISIT: we might want to clear the PASID table pointer
+	 * as part of context clear operation. Currently, it leaves

Quoted 9 lines

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5534

Quoted 6 lines

(quoted diff) +1 -0

+	 * stale data but should be ignored by hardware since

PASIDE

drivers/iommu/intel-iommu.c +4 -0

+	 * is clear.
+	 */
+	/* ATS will be reenabled when remapping is restored */
+	pci_disable_ats(to_pci_dev(dev));    

dev_is_pci()?

good to check, even thought intel iommu supports PCI only.

Quoted 31 lines · On Fri, 23 Jun 2017 11:19:52 -0700 Jacob Pan <[email protected]>…

Quoted 28 lines · On Thu, 22 Jun 2017 16:52:15 -0600 Alex Williamson…

Quoted 19 lines · On Wed, 14 Jun 2017 15:22:56 -0700 Jacob Pan <[email protected]>…

+static int intel_iommu_unbind_pasid_table(struct iommu_domain *domain,

drivers/iommu/intel-iommu.c +4 -0

+					struct device *dev)
+{
+	struct intel_iommu *iommu;
+	struct dmar_domain *dmar_domain =

to_dmar_domain(domain);

drivers/iommu/intel-iommu.c +7 -0

+	u8 bus, devfn;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+	/*
+	 * REVISIT: we might want to clear the PASID table

pointer

(quoted diff) +1 -0

+	 * as part of context clear operation. Currently, it

leaves

(quoted diff) +1 -0

+	 * stale data but should be ignored by hardware since

PASIDE

drivers/iommu/intel-iommu.c struct intel_iommu *intel_svm_device_to_iommu(struct device *dev) · line 5534

Quoted 4 lines

drivers/iommu/intel-iommu.c +4 -0

+	 * is clear.
+	 */
+	/* ATS will be reenabled when remapping is restored */
+	pci_disable_ats(to_pci_dev(dev));      

dev_is_pci()?

Quoted 1 line

good to check, even thought intel iommu supports PCI only.

That's not true, intel-iommu supports non-PCI devices defined in ACPI as well. Thanks,

Alex

From: "Liu, Yi L" <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>

When a SVM capable device is assigned to a guest, the first level page tables are owned by the guest and the guest PASID table pointer is linked to the device context entry of the physical IOMMU.

Host IOMMU driver has no knowledge of caching structure updates unless the guest invalidation activities are passed down to the host. The primary usage is derived from emulated IOMMU in the guest, where QEMU can trap invalidation activities before pass them down the host/physical IOMMU. There are IOMMU architectural specific actions need to be taken which requires the generic APIs introduced in this patch to have opaque data in the tlb_invalidate_info argument.

Signed-off-by Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c      | 13 +++++++++++++
 include/linux/iommu.h      | 11 +++++++++++
 include/uapi/linux/iommu.h |  4 ++++
 3 files changed, 28 insertions(+)

drivers/iommu/iommu.c +2 -0

index 494309b..c786370 100644
@@ -1347,6 +1347,19 @@ int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device *dev)
 }
 EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table);
 
+int iommu_do_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info *inv_info)

drivers/iommu/iommu.c +1 -0

+
 static void __iommu_detach_device(struct iommu_domain *domain,
 				  struct device *dev)
 {

include/linux/iommu.h +11 -0

index 7122add..2cdbaa3 100644
@@ -232,6 +232,8 @@ struct iommu_ops {
 				struct pasid_table_info *pasidt_binfo);
 	int (*unbind_pasid_table)(struct iommu_domain *domain,
 				struct device *dev);
+	int (*do_invalidate)(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info *inv_info);
 
 	unsigned long pgsize_bitmap;
 };

@@ -293,6 +295,9 @@ extern int iommu_bind_pasid_table(struct iommu_domain *domain,
 		struct device *dev, struct pasid_table_info *pasidt_binfo);
 extern int iommu_unbind_pasid_table(struct iommu_domain *domain,
 				struct device *dev);
+extern int iommu_do_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info *inv_info);
+
 extern struct iommu_domain *iommu_get_domain_for_dev(struct device *dev);
 extern int iommu_map(struct iommu_domain *domain, unsigned long iova,
 		     phys_addr_t paddr, size_t size, int prot);

@@ -660,6 +665,12 @@ int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device *dev)
 	return -EINVAL;
 }
 
+static inline int iommu_do_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info *inv_info)
+{
+	return -EINVAL;
+}
+
 #endif /* CONFIG_IOMMU_API */
 
 #endif /* __LINUX_IOMMU_H */

include/uapi/linux/iommu.h +0 -0

index 5ef0e7c..fcee18f 100644

include/uapi/linux/iommu.h +1 -0

+};
 
 #endif /* _UAPI_IOMMU_H */

This patch adds Intel VT-d specific function to implement iommu_do_invalidate API.

The use case is for supporting caching structure invalidation of assigned SVM capable devices. Emulated IOMMU exposes queue invalidation capability and passes down all descriptors from the guest to the physical IOMMU.

The assumption is that guest to host device ID mapping should be resolved prior to calling IOMMU driver. Based on the device handle, host IOMMU driver can replace certain fields before submit to the invalidation queue.

Signed-off-by Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/intel-iommu.c | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/intel-iommu.h | 11 ++++++++++-
 2 files changed, 51 insertions(+), 1 deletion(-)

drivers/iommu/intel-iommu.c +10 -0

index 1d5d9ab9..6b8e997 100644
@@ -5127,6 +5127,46 @@ static void intel_iommu_detach_device(struct iommu_domain *domain,
 	dmar_remove_one_dev_info(to_dmar_domain(domain), dev);
 }
 
+static int intel_iommu_do_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info *inv_info)
+{
+	struct intel_iommu *iommu;
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_invalidate_data *inv_data;
+	struct qi_desc *qi;
+	u16 did;
+	u8 bus, devfn;
+

drivers/iommu/intel-iommu.c +21 -0

+	if (PCI_DEVID(bus, devfn) != inv_data->sid)
+		return 0;
+
+	qi = &inv_data->inv_desc;
+
+	switch (qi->low & QI_TYPE_MASK) {
+	case QI_DIOTLB_TYPE:
+	case QI_DEIOTLB_TYPE:
+		/* for device IOTLB, we just let it pass through */
+		break;
+	default:
+		did = dmar_domain->iommu_did[iommu->seq_id];
+		qi->low &= ~QI_DID_MASK;
+		qi->low |= QI_DID(did);
+		break;
+	}
+
+	return qi_submit_sync(qi, iommu);
+}
+
 static int intel_iommu_map(struct iommu_domain *domain,
 			   unsigned long iova, phys_addr_t hpa,
 			   size_t size, int iommu_prot)

@@ -5546,6 +5586,7 @@ const struct iommu_ops intel_iommu_ops = {
 #ifdef CONFIG_INTEL_IOMMU_SVM
 	.bind_pasid_table	= intel_iommu_bind_pasid_table,
 	.unbind_pasid_table	= intel_iommu_unbind_pasid_table,
+	.do_invalidate		= intel_iommu_do_invalidate,
 #endif
 	.map			= intel_iommu_map,
 	.unmap			= intel_iommu_unmap,

include/linux/intel-iommu.h +4 -1

index 485a5b4..8df6c91 100644
@@ -31,7 +31,6 @@
 #include <linux/list.h>
 #include <linux/iommu.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
-
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
 

@@ -258,6 +257,10 @@ enum {
 #define QI_PGRP_RESP_TYPE	0x9
 #define QI_PSTRM_RESP_TYPE	0xa
 
+#define QI_DID(did)		(((u64)did & 0xffff) << 16)
+#define QI_DID_MASK		GENMASK(31, 16)
+#define QI_TYPE_MASK		GENMASK(3, 0)
+
 #define QI_IEC_SELECTIVE	(((u64)1) << 4)
 #define QI_IEC_IIDEX(idx)	(((u64)(idx & 0xffff) << 32))
 #define QI_IEC_IM(m)		(((u64)(m & 0x1f) << 27))

include/linux/intel-iommu.h +0 -0

 extern const struct attribute_group *intel_iommu_groups[];
 
 #endif

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.

Faults detected by IOMMU is based on the transaction's source ID which can be reported at per device basis, regardless of the device type is a PCI device or not.

The fault types includes recoverable (e.g. page request) and unrecoverable faults(e.g. invalid context). In most cases, faults can be handled by IOMMU drivers. However, there are use cases that require fault processing outside IOMMU driver, e.g.

1. page request fault originated from an SVM capable device that is assigned to guest via vIOMMU. In this case, the first level page tables are owned by the guest. Page request must be propagated to the guest to let guest OS fault in the pages then send page response. In this mechanism, the direct receiver of IOMMU fault notification is VFIO, which can relay notification events to QEMU or other user space software.

2. faults need more subtle handling by device drivers. Other than simply invoke reset function, there are needs to let device driver handle the fault with a smaller impact.

This patchset is intended to create a generic fault notification API such that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)

The event data contains both generic and raw architectural data such that performance is not compromised as the data propagation may involve many layers.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

drivers/iommu/iommu.c +35 -0

index c786370..04c73f3 100644
@@ -48,6 +48,7 @@ struct iommu_group {
 	struct list_head devices;
 	struct mutex mutex;
 	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head fault_notifier;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;

@@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier);
 
 	ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
 	if (ret < 0) {

@@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct iommu_group *group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier);
 
 /**
+ * iommu_register_fault_notifier - Register a notifier for fault reporting
+ * @dev: device to notify fault events
+ * @nb: notifier block to signal
+ *
+ */
+int iommu_register_fault_notifier(struct device *dev,
+				struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret = blocking_notifier_chain_register(&group->fault_notifier, nb);
+	iommu_group_put(group);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(iommu_register_fault_notifier);
+
+/**
+ * iommu_unregister_fault_notifier - Unregister a notifier for fault reporting
+ * @domain: the domain to watch
+ * @nb: notifier block to signal
+ *
+ */
+int iommu_unregister_fault_notifier(struct device *dev,
+				  struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);

drivers/iommu/iommu.c +18 -0

+
+int iommu_fault_notifier_call_chain(struct iommu_fault_event *event)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(event->dev);
+
+	if (!group)
+		return -EINVAL;
+	/* caller provide generic data related to the event, TBD */
+	ret = (blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event)
+		== NOTIFY_BAD) ? -EINVAL : 0;
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_fault_notifier_call_chain);
+
+/**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
  *

include/linux/iommu.h +0 -0

index 2cdbaa3..fe89e88 100644

include/linux/iommu.h +16 -0

 struct iommu_ops;
 struct iommu_group;

@@ -97,6 +98,36 @@ struct iommu_domain {
 	void *iova_cookie;
 };
 
+/*
+ * Generic fault event notification data, used by all IOMMU architectures.
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request) & un-recoverable faults
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @dev The device which faults are reported by IOMMU
+ * @addr tells the offending address
+ * @pasid contains process address space ID, used in shared virtual memory (SVM)
+ * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @flags contains fault type, etc.
+ * @length tells the size of the buf
+ * @buf contains any raw or arch specific data
+ *
+ */

include/linux/iommu.h +24 -0

+
 enum iommu_cap {
 	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce cache coherent DMA
 					   transactions */

@@ -341,6 +372,12 @@ 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_fault_notifier(struct device *dev,
+					 struct notifier_block *nb);
+extern int iommu_unregister_fault_notifier(struct device *dev,
+					 struct notifier_block *nb);
+extern int iommu_fault_notifier_call_chain(struct iommu_fault_event *event);
+
 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 *);

@@ -573,6 +610,23 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group,
 	return 0;
 }
 
+static inline int iommu_register_fault_notifier(struct device *dev,
+						  struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int iommu_unregister_fault_notifier(struct device *dev,
+						  struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline int iommu_fault_notifier_call_chain(struct iommu_fault_event *event)
+{
+	return 0;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;

Quoted 117 lines · On Wed, 14 Jun 2017 15:22:59 -0700 Jacob Pan <[email protected]>…

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.

Faults detected by IOMMU is based on the transaction's source ID which can be reported at per device basis, regardless of the device type is a PCI device or not.

The fault types includes recoverable (e.g. page request) and unrecoverable faults(e.g. invalid context). In most cases, faults can be handled by IOMMU drivers. However, there are use cases that require fault processing outside IOMMU driver, e.g.

1. page request fault originated from an SVM capable device that is assigned to guest via vIOMMU. In this case, the first level page tables are owned by the guest. Page request must be propagated to the guest to let guest OS fault in the pages then send page response. In this mechanism, the direct receiver of IOMMU fault notification is VFIO, which can relay notification events to QEMU or other user space software.

2. faults need more subtle handling by device drivers. Other than simply invoke reset function, there are needs to let device driver handle the fault with a smaller impact.

This patchset is intended to create a generic fault notification API such that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)

The event data contains both generic and raw architectural data such that performance is not compromised as the data propagation may involve many layers.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c | 63

+++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)

drivers/iommu/iommu.c +3 -0

index c786370..04c73f3 100644
@@ -48,6 +48,7 @@ struct iommu_group {
 	struct list_head devices;
 	struct mutex mutex;
 	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head fault_notifier;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;

@@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier);
 
 	ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
 	if (ret < 0) {

@@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct
 iommu_group *group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier); 
 /**
+ * iommu_register_fault_notifier - Register a notifier for fault

reporting
+ * @dev: device to notify fault events
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_register_fault_notifier(struct device *dev,
+				struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_register(&group->fault_notifier, nb);

drivers/iommu/iommu.c +9 -0

+	iommu_group_put(group);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(iommu_register_fault_notifier);
+
+/**
+ * iommu_unregister_fault_notifier - Unregister a notifier for

fault reporting
+ * @domain: the domain to watch
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_unregister_fault_notifier(struct device *dev,
+				  struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_unregister(&group->fault_notifier, nb);

drivers/iommu/iommu.c +3 -0

+	iommu_group_put(group);
+
+	return ret;

drivers/iommu/iommu.c

Quoted 2 lines

drivers/iommu/iommu.c +2 -0

+}
+EXPORT_SYMBOL_GPL(iommu_unregister_fault_notifier);  

If the call chains are on the group, why do we register with a device? If I registered with a group, I'd know that I only need to register once per group, that's not clear here and may lead to callers of this getting multiple notifications, one for each device in a group.

Quoted 26 lines

drivers/iommu/iommu.c +2 -0

+
+int iommu_fault_notifier_call_chain(struct iommu_fault_event

*event) +{

drivers/iommu/iommu.c +7 -0

+	int ret;
+	struct iommu_group *group = iommu_group_get(event->dev);
+
+	if (!group)
+		return -EINVAL;
+	/* caller provide generic data related to the event, TBD */
+	ret =

(blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event)

drivers/iommu/iommu.c +8 -0

+		== NOTIFY_BAD) ? -EINVAL : 0;
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_fault_notifier_call_chain);
+
+/**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
  *

include/linux/iommu.h +0 -1

index 2cdbaa3..fe89e88 100644
--- a/include/linux/iommu.h

include/linux/iommu.h line 45

Quoted 6 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +1 -0

@@ -42,6 +42,7 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+#define IOMMU_EXEC	(1 << 6)  

Irrelevant change?

Quoted 29 lines

include/linux/iommu.h +2 -0

 struct iommu_ops;
 struct iommu_group;

@@ -97,6 +98,36 @@ struct iommu_domain {
 	void *iova_cookie;
 };
 
+/*
+ * Generic fault event notification data, used by all IOMMU

architectures.
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request) & un-recoverable faults
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @dev The device which faults are reported by IOMMU
+ * @addr tells the offending address
+ * @pasid contains process address space ID, used in shared virtual memory (SVM)
+ * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @flags contains fault type, etc.
+ * @length tells the size of the buf
+ * @buf contains any raw or arch specific data
+ *
+ */

include/linux/iommu.h +4 -0

+struct iommu_fault_event {
+	struct device *dev;
+	__u64 addr;
+	__u32 pasid;

include/linux/iommu.h struct iommu_domain { · line 126

Quoted 6 lines

include/linux/iommu.h +6 -0

+	__u32 prot;
+	__u32 flags;
+#define IOMMU_FAULT_PAGE_REQ	BIT(0)
+#define IOMMU_FAULT_UNRECOV	BIT(1)
+#define IOMMU_FAULT_IRQ_REMAP	BIT(2)
+#define IOMMU_FAULT_INVAL	BIT(3)  

Details of each of these are defined where?

include/linux/iommu.h struct iommu_domain { · line 129

Quoted 3 lines

include/linux/iommu.h +3 -0

+	__u32 length;
+	__u8  buf[];
+};  

This is not UAPI, so I'll be curious how vfio is supposed to expose this to userspace.

Quoted 36 lines

include/linux/iommu.h +1 -0

+
 enum iommu_cap {
 	IOMMU_CAP_CACHE_COHERENCY,	/* IOMMU can enforce

cache coherent DMA transactions */

include/linux/iommu.h +0 -0

@@ -341,6 +372,12 @@ 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_fault_notifier(struct device *dev,

include/linux/iommu.h +1 -0

+					 struct notifier_block

*nb); +extern int iommu_unregister_fault_notifier(struct device *dev,

include/linux/iommu.h +1 -0

+					 struct notifier_block

*nb); +extern int iommu_fault_notifier_call_chain(struct iommu_fault_event *event); +
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 *); @@ -573,6 +610,23 @@ static inline int iommu_group_unregister_notifier(struct iommu_group *group, return 0; }

include/linux/iommu.h +2 -0

+static inline int iommu_register_fault_notifier(struct device *dev,
+						  struct

notifier_block *nb) +{

include/linux/iommu.h +4 -0

+	return 0;
+}
+
+static inline int iommu_unregister_fault_notifier(struct device

*dev,

include/linux/iommu.h +1 -0

+						  struct

notifier_block *nb) +{

include/linux/iommu.h +3 -0

+	return 0;
+}
+

include/linux/iommu.h static inline int iommu_group_unregister_notifier(struct iommu_group *group, · line 632

Quoted 8 lines

+static inline int iommu_fault_notifier_call_chain(struct iommu_fault_event *event) +{

include/linux/iommu.h +3 -0

+	return 0;
+}
+
 static inline int iommu_group_id(struct iommu_group *group)
 {
 	return -ENODEV;  

Quoted 120 lines · On Thu, 22 Jun 2017 16:53:17 -0600 Alex Williamson…

Quoted 117 lines · On Wed, 14 Jun 2017 15:22:59 -0700 Jacob Pan <[email protected]>…

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.

Faults detected by IOMMU is based on the transaction's source ID which can be reported at per device basis, regardless of the device type is a PCI device or not.

The fault types includes recoverable (e.g. page request) and unrecoverable faults(e.g. invalid context). In most cases, faults can be handled by IOMMU drivers. However, there are use cases that require fault processing outside IOMMU driver, e.g.

1. page request fault originated from an SVM capable device that is assigned to guest via vIOMMU. In this case, the first level page tables are owned by the guest. Page request must be propagated to the guest to let guest OS fault in the pages then send page response. In this mechanism, the direct receiver of IOMMU fault notification is VFIO, which can relay notification events to QEMU or other user space software.

2. faults need more subtle handling by device drivers. Other than simply invoke reset function, there are needs to let device driver handle the fault with a smaller impact.

This patchset is intended to create a generic fault notification API such that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)

The event data contains both generic and raw architectural data such that performance is not compromised as the data propagation may involve many layers.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c | 63

+++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)

drivers/iommu/iommu.c +3 -0

index c786370..04c73f3 100644
@@ -48,6 +48,7 @@ struct iommu_group {
 	struct list_head devices;
 	struct mutex mutex;
 	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head fault_notifier;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;

@@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier);
 
 	ret = ida_simple_get(&iommu_group_ida, 0, 0, GFP_KERNEL);
 	if (ret < 0) {

@@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct
 iommu_group *group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier); 
 /**
+ * iommu_register_fault_notifier - Register a notifier for fault

reporting
+ * @dev: device to notify fault events
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_register_fault_notifier(struct device *dev,
+				struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_register(&group->fault_notifier, nb);

drivers/iommu/iommu.c +9 -0

+	iommu_group_put(group);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(iommu_register_fault_notifier);
+
+/**
+ * iommu_unregister_fault_notifier - Unregister a notifier for

fault reporting
+ * @domain: the domain to watch
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_unregister_fault_notifier(struct device *dev,
+				  struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_unregister(&group->fault_notifier, nb);

drivers/iommu/iommu.c +3 -0

+	iommu_group_put(group);
+
+	return ret;

Quoted 9 lines

drivers/iommu/iommu.c

Quoted 2 lines

drivers/iommu/iommu.c +2 -0

+}
+EXPORT_SYMBOL_GPL(iommu_unregister_fault_notifier);    

If the call chains are on the group, why do we register with a device? If I registered with a group, I'd know that I only need to register once per group, that's not clear here and may lead to callers of this getting multiple notifications, one for each device in a group.

My thought was that since iommu_group is already part of struct device, it is more efficient to share group level notifier than introducing per device fault callbacks or notifiers.

So you do need to register per device not just once for the group. In most cases device with SVM has ACS, it will be 1:1 between device and group.

When the fault event occurs, IOMMU can identify the offending device and struct device is embedded in the event data sent to the device driver or VFIO, so the receiver can filter out unwanted events.

Quoted 26 lines

Quoted 26 lines

drivers/iommu/iommu.c +2 -0

+
+int iommu_fault_notifier_call_chain(struct iommu_fault_event

*event) +{

drivers/iommu/iommu.c +7 -0

+	int ret;
+	struct iommu_group *group = iommu_group_get(event->dev);
+
+	if (!group)
+		return -EINVAL;
+	/* caller provide generic data related to the event, TBD */
+	ret =

(blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event)

drivers/iommu/iommu.c +8 -0

+		== NOTIFY_BAD) ? -EINVAL : 0;
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_fault_notifier_call_chain);
+
+/**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
  *

include/linux/iommu.h +0 -1

index 2cdbaa3..fe89e88 100644
--- a/include/linux/iommu.h

Quoted 9 lines

include/linux/iommu.h line 45

Quoted 6 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +1 -0

@@ -42,6 +42,7 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+#define IOMMU_EXEC	(1 << 6)    

Irrelevant change?

yes, it should be separate. used later in svm patch.

Quoted 29 lines

Quoted 29 lines

include/linux/iommu.h +2 -0

 struct iommu_ops;
 struct iommu_group;

@@ -97,6 +98,36 @@ struct iommu_domain {
 	void *iova_cookie;
 };
 
+/*
+ * Generic fault event notification data, used by all IOMMU

architectures.
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request) & un-recoverable faults
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @dev The device which faults are reported by IOMMU
+ * @addr tells the offending address
+ * @pasid contains process address space ID, used in shared virtual memory (SVM)
+ * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @flags contains fault type, etc.
+ * @length tells the size of the buf
+ * @buf contains any raw or arch specific data
+ *
+ */

include/linux/iommu.h +4 -0

+struct iommu_fault_event {
+	struct device *dev;
+	__u64 addr;
+	__u32 pasid;

Quoted 9 lines

include/linux/iommu.h struct iommu_domain { · line 126

Quoted 6 lines

include/linux/iommu.h +6 -0

+	__u32 prot;
+	__u32 flags;
+#define IOMMU_FAULT_PAGE_REQ	BIT(0)
+#define IOMMU_FAULT_UNRECOV	BIT(1)
+#define IOMMU_FAULT_IRQ_REMAP	BIT(2)
+#define IOMMU_FAULT_INVAL	BIT(3)    

Details of each of these are defined where?

good point, will add description.

Quoted 7 lines

include/linux/iommu.h struct iommu_domain { · line 129

Quoted 3 lines

include/linux/iommu.h +3 -0

+	__u32 length;
+	__u8  buf[];
+};    

This is not UAPI, so I'll be curious how vfio is supposed to expose this to userspace.

But this also has other in-kernel users.

Quoted 133 lines · On Fri, 23 Jun 2017 11:59:28 -0700 Jacob Pan <[email protected]>…

Quoted 122 lines · On Thu, 22 Jun 2017 16:53:17 -0600 Alex Williamson…

Quoted 119 lines · On Wed, 14 Jun 2017 15:22:59 -0700 Jacob Pan <[email protected]>…

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.

Faults detected by IOMMU is based on the transaction's source ID which can be reported at per device basis, regardless of the device type is a PCI device or not.

The fault types includes recoverable (e.g. page request) and unrecoverable faults(e.g. invalid context). In most cases, faults can be handled by IOMMU drivers. However, there are use cases that require fault processing outside IOMMU driver, e.g.

1. page request fault originated from an SVM capable device that is assigned to guest via vIOMMU. In this case, the first level page tables are owned by the guest. Page request must be propagated to the guest to let guest OS fault in the pages then send page response. In this mechanism, the direct receiver of IOMMU fault notification is VFIO, which can relay notification events to QEMU or other user space software.

2. faults need more subtle handling by device drivers. Other than simply invoke reset function, there are needs to let device driver handle the fault with a smaller impact.

This patchset is intended to create a generic fault notification API such that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)

The event data contains both generic and raw architectural data such that performance is not compromised as the data propagation may involve many layers.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c | 63

+++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)

drivers/iommu/iommu.c +3 -0

index c786370..04c73f3 100644
@@ -48,6 +48,7 @@ struct iommu_group {
 	struct list_head devices;
 	struct mutex mutex;
 	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head fault_notifier;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;

@@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier);
 
 	ret = ida_simple_get(&iommu_group_ida, 0, 0,
 GFP_KERNEL); if (ret < 0) {

@@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct
 iommu_group *group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier); 
 /**
+ * iommu_register_fault_notifier - Register a notifier for

fault reporting
+ * @dev: device to notify fault events
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_register_fault_notifier(struct device *dev,
+				struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_register(&group->fault_notifier, nb);

drivers/iommu/iommu.c +9 -0

+	iommu_group_put(group);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(iommu_register_fault_notifier);
+
+/**
+ * iommu_unregister_fault_notifier - Unregister a notifier for

fault reporting
+ * @domain: the domain to watch
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_unregister_fault_notifier(struct device *dev,
+				  struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_unregister(&group->fault_notifier, nb);

drivers/iommu/iommu.c +4 -0

+	iommu_group_put(group);
+
+	return ret;
+}

Quoted 8 lines

Quoted 1 line

+EXPORT_SYMBOL_GPL(iommu_unregister_fault_notifier);

If the call chains are on the group, why do we register with a device? If I registered with a group, I'd know that I only need to register once per group, that's not clear here and may lead to callers of this getting multiple notifications, one for each device in a group.

Quoted 3 lines

My thought was that since iommu_group is already part of struct device, it is more efficient to share group level notifier than introducing per device fault callbacks or notifiers.

That's fine, but the interface should reflect the same.

Quoted 3 lines

So you do need to register per device not just once for the group. In most cases device with SVM has ACS, it will be 1:1 between device and group.

ACS at the endpoint is only one factor in determining grouping, we need only look at Intel Core processors to find root ports lacking ACS which will result in multiple devices per group as a very common case.

Quoted 3 lines

When the fault event occurs, IOMMU can identify the offending device and struct device is embedded in the event data sent to the device driver or VFIO, so the receiver can filter out unwanted events.

I think this is a poor design choice. If the notification list is on the group then make the registration be on the group. Otherwise the number of notifies seen by the caller is automatically multiplied by the number of devices they're using in the group. The caller not only needs to disregard unintended devices, but they need to make sure they only act on a notification for the device on the correct notifier. This is designing in unnecessary complexity and overhead.

Quoted 85 lines

Quoted 27 lines

Quoted 27 lines

drivers/iommu/iommu.c +2 -0

+
+int iommu_fault_notifier_call_chain(struct iommu_fault_event

*event) +{

drivers/iommu/iommu.c +2 -0

+	int ret;
+	struct iommu_group *group =

iommu_group_get(event->dev); +

drivers/iommu/iommu.c +3 -0

+	if (!group)
+		return -EINVAL;
+	/* caller provide generic data related to the event,

TBD */

drivers/iommu/iommu.c +1 -0

+	ret =

(blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event)

drivers/iommu/iommu.c +8 -0

+		== NOTIFY_BAD) ? -EINVAL : 0;
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_fault_notifier_call_chain);
+
+/**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
  *

include/linux/iommu.h +0 -1

index 2cdbaa3..fe89e88 100644
--- a/include/linux/iommu.h

Quoted 9 lines

include/linux/iommu.h line 45

Quoted 6 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +1 -0

@@ -42,6 +42,7 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+#define IOMMU_EXEC	(1 << 6)      

Irrelevant change?

yes, it should be separate. used later in svm patch.

Quoted 31 lines

Quoted 31 lines

include/linux/iommu.h +2 -0

 struct iommu_ops;
 struct iommu_group;

@@ -97,6 +98,36 @@ struct iommu_domain {
 	void *iova_cookie;
 };
 
+/*
+ * Generic fault event notification data, used by all IOMMU

architectures.
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request) & un-recoverable faults
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @dev The device which faults are reported by IOMMU
+ * @addr tells the offending address
+ * @pasid contains process address space ID, used in shared virtual memory (SVM)
+ * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @flags contains fault type, etc.
+ * @length tells the size of the buf
+ * @buf contains any raw or arch specific data
+ *
+ */

include/linux/iommu.h +4 -0

+struct iommu_fault_event {
+	struct device *dev;
+	__u64 addr;
+	__u32 pasid;

Quoted 9 lines

include/linux/iommu.h struct iommu_domain { · line 126

Quoted 6 lines

include/linux/iommu.h +6 -0

+	__u32 prot;
+	__u32 flags;
+#define IOMMU_FAULT_PAGE_REQ	BIT(0)
+#define IOMMU_FAULT_UNRECOV	BIT(1)
+#define IOMMU_FAULT_IRQ_REMAP	BIT(2)
+#define IOMMU_FAULT_INVAL	BIT(3)      

Details of each of these are defined where?

good point, will add description.

Quoted 7 lines

include/linux/iommu.h struct iommu_domain { · line 129

Quoted 3 lines

include/linux/iommu.h +3 -0

+	__u32 length;
+	__u8  buf[];
+};      

This is not UAPI, so I'll be curious how vfio is supposed to expose this to userspace.

Quoted 1 line

But this also has other in-kernel users.

Any point where you expect vfio to pass through one of these model specific data structures needs to be fully specified in a uapi header. No magic opaque data please. Thanks,

Alex

Quoted 154 lines · On Fri, 23 Jun 2017 13:15:51 -0600 Alex Williamson…

Quoted 133 lines · On Fri, 23 Jun 2017 11:59:28 -0700 Jacob Pan…

Quoted 122 lines · On Thu, 22 Jun 2017 16:53:17 -0600 Alex Williamson…

Quoted 119 lines · On Wed, 14 Jun 2017 15:22:59 -0700 Jacob Pan…

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.

Faults detected by IOMMU is based on the transaction's source ID which can be reported at per device basis, regardless of the device type is a PCI device or not.

The fault types includes recoverable (e.g. page request) and unrecoverable faults(e.g. invalid context). In most cases, faults can be handled by IOMMU drivers. However, there are use cases that require fault processing outside IOMMU driver, e.g.

1. page request fault originated from an SVM capable device that is assigned to guest via vIOMMU. In this case, the first level page tables are owned by the guest. Page request must be propagated to the guest to let guest OS fault in the pages then send page response. In this mechanism, the direct receiver of IOMMU fault notification is VFIO, which can relay notification events to QEMU or other user space software.

2. faults need more subtle handling by device drivers. Other than simply invoke reset function, there are needs to let device driver handle the fault with a smaller impact.

This patchset is intended to create a generic fault notification API such that it can scale as follows:
- all IOMMU types
- PCI and non-PCI devices
- recoverable and unrecoverable faults
- VFIO and other other in kernel users
- DMA & IRQ remapping (TBD)

The event data contains both generic and raw architectural data such that performance is not compromised as the data propagation may involve many layers.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/iommu.c | 63

+++++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/iommu.h | 54 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+)

drivers/iommu/iommu.c +3 -0

index c786370..04c73f3 100644
@@ -48,6 +48,7 @@ struct iommu_group {
 	struct list_head devices;
 	struct mutex mutex;
 	struct blocking_notifier_head notifier;
+	struct blocking_notifier_head fault_notifier;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
 	char *name;

@@ -345,6 +346,7 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	BLOCKING_INIT_NOTIFIER_HEAD(&group->notifier);
+	BLOCKING_INIT_NOTIFIER_HEAD(&group->fault_notifier);
 
 	ret = ida_simple_get(&iommu_group_ida, 0, 0,
 GFP_KERNEL); if (ret < 0) {

@@ -790,6 +792,67 @@ int iommu_group_unregister_notifier(struct
 iommu_group *group,
 EXPORT_SYMBOL_GPL(iommu_group_unregister_notifier); 
 /**
+ * iommu_register_fault_notifier - Register a notifier for

fault reporting
+ * @dev: device to notify fault events
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_register_fault_notifier(struct device *dev,
+				struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_register(&group->fault_notifier, nb);

drivers/iommu/iommu.c +9 -0

+	iommu_group_put(group);
+
+	return ret;
+
+}
+EXPORT_SYMBOL_GPL(iommu_register_fault_notifier);
+
+/**
+ * iommu_unregister_fault_notifier - Unregister a notifier for

fault reporting
+ * @domain: the domain to watch
+ * @nb: notifier block to signal
+ *
+ */

drivers/iommu/iommu.c +10 -0

+int iommu_unregister_fault_notifier(struct device *dev,
+				  struct notifier_block *nb)
+{
+	int ret;
+	struct iommu_group *group = iommu_group_get(dev);
+
+	if (!group)
+		return -EINVAL;
+
+	ret =

blocking_notifier_chain_unregister(&group->fault_notifier, nb);

drivers/iommu/iommu.c +4 -0

+	iommu_group_put(group);
+
+	return ret;
+}

Quoted 8 lines

Quoted 1 line

+EXPORT_SYMBOL_GPL(iommu_unregister_fault_notifier);

If the call chains are on the group, why do we register with a device? If I registered with a group, I'd know that I only need to register once per group, that's not clear here and may lead to callers of this getting multiple notifications, one for each device in a group.

Quoted 3 lines

My thought was that since iommu_group is already part of struct device, it is more efficient to share group level notifier than introducing per device fault callbacks or notifiers.

That's fine, but the interface should reflect the same.

Quoted 3 lines

So you do need to register per device not just once for the group. In most cases device with SVM has ACS, it will be 1:1 between device and group.

ACS at the endpoint is only one factor in determining grouping, we need only look at Intel Core processors to find root ports lacking ACS which will result in multiple devices per group as a very common case.

Quoted 3 lines

When the fault event occurs, IOMMU can identify the offending device and struct device is embedded in the event data sent to the device driver or VFIO, so the receiver can filter out unwanted events.

Quoted 8 lines

I think this is a poor design choice. If the notification list is on the group then make the registration be on the group. Otherwise the number of notifies seen by the caller is automatically multiplied by the number of devices they're using in the group. The caller not only needs to disregard unintended devices, but they need to make sure they only act on a notification for the device on the correct notifier. This is designing in unnecessary complexity and overhead.

If the design is for high frequency events, I would totally agree with you that having notifier on the group does not scale. But since this is for fault notification, which implies infrequent use, is it still worthwhile to tax strict device to have a per device notifier? Even with recoverable fault, i.e. page request, a properly configured use case would pre-fault pages to minimize PRQ.

I was referring to this previous discussion by David for the need of per device fault handler(https://lwn.net/Articles/608914/). Perhaps I misinterpreted your suggestion for an alternative solution.

Quoted 85 lines

Quoted 85 lines

Quoted 27 lines

Quoted 27 lines

drivers/iommu/iommu.c +2 -0

+
+int iommu_fault_notifier_call_chain(struct iommu_fault_event

*event) +{

drivers/iommu/iommu.c +2 -0

+	int ret;
+	struct iommu_group *group =

iommu_group_get(event->dev); +

drivers/iommu/iommu.c +3 -0

+	if (!group)
+		return -EINVAL;
+	/* caller provide generic data related to the event,

TBD */

drivers/iommu/iommu.c +1 -0

+	ret =

(blocking_notifier_call_chain(&group->fault_notifier, 0, (void *)event)

drivers/iommu/iommu.c +8 -0

+		== NOTIFY_BAD) ? -EINVAL : 0;
+	iommu_group_put(group);
+
+	return ret;
+}
+EXPORT_SYMBOL(iommu_fault_notifier_call_chain);
+
+/**
  * iommu_group_id - Return ID for a group
  * @group: the group to ID
  *

include/linux/iommu.h +0 -1

index 2cdbaa3..fe89e88 100644
--- a/include/linux/iommu.h

Quoted 9 lines

include/linux/iommu.h line 45

Quoted 6 lines

+++ b/include/linux/iommu.h

include/linux/iommu.h +1 -0

@@ -42,6 +42,7 @@
  * if the IOMMU page table format is equivalent.
  */
 #define IOMMU_PRIV	(1 << 5)
+#define IOMMU_EXEC	(1 << 6)        

Irrelevant change?

yes, it should be separate. used later in svm patch.

Quoted 31 lines

Quoted 31 lines

include/linux/iommu.h +2 -0

 struct iommu_ops;
 struct iommu_group;

@@ -97,6 +98,36 @@ struct iommu_domain {
 	void *iova_cookie;
 };
 
+/*
+ * Generic fault event notification data, used by all IOMMU

architectures.
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request) & un-recoverable faults
+ * - DMA remapping and IRQ remapping faults
+ *
+ * @dev The device which faults are reported by IOMMU
+ * @addr tells the offending address
+ * @pasid contains process address space ID, used in shared virtual memory (SVM)
+ * @prot page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @flags contains fault type, etc.
+ * @length tells the size of the buf
+ * @buf contains any raw or arch specific data
+ *
+ */

include/linux/iommu.h +4 -0

+struct iommu_fault_event {
+	struct device *dev;
+	__u64 addr;
+	__u32 pasid;

Quoted 9 lines

include/linux/iommu.h struct iommu_domain { · line 126

Quoted 6 lines

include/linux/iommu.h +6 -0

+	__u32 prot;
+	__u32 flags;
+#define IOMMU_FAULT_PAGE_REQ	BIT(0)
+#define IOMMU_FAULT_UNRECOV	BIT(1)
+#define IOMMU_FAULT_IRQ_REMAP	BIT(2)
+#define IOMMU_FAULT_INVAL	BIT(3)        

Details of each of these are defined where?

good point, will add description.

Quoted 7 lines

include/linux/iommu.h struct iommu_domain { · line 129

Quoted 3 lines

include/linux/iommu.h +3 -0

+	__u32 length;
+	__u8  buf[];
+};        

This is not UAPI, so I'll be curious how vfio is supposed to expose this to userspace.

Quoted 8 lines

Quoted 1 line

But this also has other in-kernel users.

Any point where you expect vfio to pass through one of these model specific data structures needs to be fully specified in a uapi header. No magic opaque data please. Thanks,

Alex

[Jacob Pan]

When PASID table pointer of an assigned device is bond to a guest,

the first level page tables are managed by the guest. However, only host/physical IOMMU can detect fault events, e.g. page requests. Therefore, we need to keep track of which device has its PASID table pointer bond to a guest such that page request and other events can

be propagated to the guest as needed.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/intel-iommu.c | 19 +------------------
 include/linux/intel-iommu.h | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 18 deletions(-)

drivers/iommu/intel-iommu.c +1 -18

index 6b8e997..9765277 100644
@@ -416,24 +416,6 @@ struct dmar_domain {
 					   iommu core */
 };
 
-/* PCI domain-device relationship */
-struct device_domain_info {
-	struct list_head link;	/* link to domain siblings */
-	struct list_head global; /* link to global list */
-	u8 bus;			/* PCI bus number */
-	u8 devfn;		/* PCI devfn number */
-	u8 pasid_supported:3;
-	u8 pasid_enabled:1;
-	u8 pri_supported:1;
-	u8 pri_enabled:1;
-	u8 ats_supported:1;
-	u8 ats_enabled:1;
-	u8 ats_qdep;
-	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
-	struct intel_iommu *iommu; /* IOMMU used by this device */
-	struct dmar_domain *domain; /* pointer to domain */
-};
-
 struct dmar_rmrr_unit {
 	struct list_head list;		/* list of rmrr units	*/
 	struct acpi_dmar_header *hdr;	/* ACPI header		*/

@@ -5547,6 +5529,7 @@ static int intel_iommu_bind_pasid_table(struct iommu_domain *domain,
 				DMA_CCMD_MASK_NOBIT,
 				DMA_CCMD_DEVICE_INVL);
 	iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
+	info->pasid_tbl_bond = 1;
 	spin_unlock_irqrestore(&iommu->lock, flags);
 
 

include/linux/intel-iommu.h +19 -0

index 8df6c91..ed39c56 100644
@@ -434,6 +434,25 @@ struct intel_iommu {
 	u32		flags;      /* Software defined flags */
 };
 
+/* PCI domain-device relationship */
+struct device_domain_info {
+	struct list_head link;	/* link to domain siblings */
+	struct list_head global; /* link to global list */
+	u8 bus;			/* PCI bus number */
+	u8 devfn;		/* PCI devfn number */
+	u8 pasid_supported:3;
+	u8 pasid_enabled:1;
+	u8 pasid_tbl_bond:1;	/* bond to guest PASID table */
+	u8 pri_supported:1;
+	u8 pri_enabled:1;
+	u8 ats_supported:1;
+	u8 ats_enabled:1;
+	u8 ats_qdep;
+	struct device *dev; /* it's NULL for PCIe-to-PCI bridge */
+	struct intel_iommu *iommu; /* IOMMU used by this device */
+	struct dmar_domain *domain; /* pointer to domain */
+};
+
 static inline void __iommu_flush_cache(
 	struct intel_iommu *iommu, void *addr, int size)
 {

Currently, when device DMA faults are detected by IOMMU the fault reasons are printed but the offending device is not notified. This patch allows device drivers to be optionally notified for fault conditions when device specific handling is needed for more subtle processing, e.g. request with PASID transactions.

Signed-off-by Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/dmar.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

drivers/iommu/dmar.c +6 -0

index cbf7763..2c0b80d464 100644
@@ -1587,11 +1587,43 @@ void dmar_msi_read(int irq, struct msi_msg *msg)
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
 }
 
+static int dmar_unrecov_fault_notify(u8 fault_reason, u16 source_id,
+			unsigned long long addr)
+{
+	int ret;
+	struct pci_dev *pdev;
+	struct iommu_fault_event *event;

drivers/iommu/dmar.c +16 -0

+
+	pci_dev_get(pdev);
+	event->dev = &pdev->dev;
+	event->buf[0] = fault_reason;
+	event->addr = addr;
+	event->length = sizeof(fault_reason);
+	event->flags = IOMMU_FAULT_UNRECOV;
+	ret = iommu_fault_notifier_call_chain(event);
+
+	pci_dev_put(pdev);
+	kfree(event);
+
+	return ret;
+}
+
 static int dmar_fault_do_one(struct intel_iommu *iommu, int type,
 		u8 fault_reason, u16 source_id, unsigned long long addr)
 {
 	const char *reason;
 	int fault_type;
+	int ret = 0;
 
 	reason = dmar_get_fault_reason(fault_reason, &fault_type);
 

drivers/iommu/dmar.c +2 -0

+	}
+
 	return 0;
 }
 

Quoted 33 lines · On Wed, 14 Jun 2017 15:23:01 -0700 Jacob Pan <[email protected]>…

Currently, when device DMA faults are detected by IOMMU the fault reasons are printed but the offending device is not notified. This patch allows device drivers to be optionally notified for fault conditions when device specific handling is needed for more subtle processing, e.g. request with PASID transactions.

Signed-off-by Jacob Pan <[email protected]>

Signed-off-by Ashok Raj <[email protected]>

---
 drivers/iommu/dmar.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

drivers/iommu/dmar.c +11 -0

index cbf7763..2c0b80d464 100644
@@ -1587,11 +1587,43 @@ void dmar_msi_read(int irq, struct msi_msg
 *msg) raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
 }
 
+static int dmar_unrecov_fault_notify(u8 fault_reason, u16
 source_id,
+			unsigned long long addr)
+{
+	int ret;
+	struct pci_dev *pdev;
+	struct iommu_fault_event *event;
+
+	pdev = pci_get_bus_and_slot(source_id >> 8, source_id &
 0xFF);
+	if (!pdev)
+		return -ENODEV;
+	pr_debug("Notify PCI device fault [%02x:%02x.%d]\n",

drivers/iommu/dmar.c void dmar_msi_read(int irq, struct msi_msg *msg) · line 1605

Quoted 6 lines

drivers/iommu/dmar.c +5 -0

+		source_id >> 8, PCI_SLOT(source_id & 0xff),
+		PCI_FUNC(source_id & 0xff));
+	event = kzalloc(sizeof(*event) + sizeof(fault_reason),
 GFP_KERNEL);
+	if (!event)
+		return -ENOMEM;  

Leaks pdev reference.

Quoted 24 lines

drivers/iommu/dmar.c +16 -0

+
+	pci_dev_get(pdev);
+	event->dev = &pdev->dev;
+	event->buf[0] = fault_reason;
+	event->addr = addr;
+	event->length = sizeof(fault_reason);
+	event->flags = IOMMU_FAULT_UNRECOV;
+	ret = iommu_fault_notifier_call_chain(event);
+
+	pci_dev_put(pdev);
+	kfree(event);
+
+	return ret;
+}
+
 static int dmar_fault_do_one(struct intel_iommu *iommu, int type,
 		u8 fault_reason, u16 source_id, unsigned long long
 addr) {
 	const char *reason;
 	int fault_type;
+	int ret = 0;

reason = dmar_get_fault_reason(fault_reason, &fault_type);