IOMMU driver support for SVM virtualization

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

+	PASID_TABLE_FORMAT_ARM_1LVL,
+	PASID_TABLE_FORMAT_ARM_2LVL,
+	PASID_TABLE_FORMAT_AMD,
+	PASID_TABLE_FORMAT_INTEL,
+};
+
+/**
+ * PASID table data used to bind guest PASID table to the host IOMMU. This will

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

+	__u32 bytes;
+	__u64 base_ptr;
+	__u8 pasid_bits;
+	enum pasid_table_model model;
+	union {
+		struct {
+			/* Intel specific fields */

Hi Jacob,

drivers/iommu/iommu.c int iommu_attach_device(struct iommu_domain *domain, struct device *dev) · line 1342

Quoted 8 lines · Jacob Pan

+int iommu_unbind_pasid_table(struct iommu_domain *domain, struct device *dev) +{

drivers/iommu/iommu.c +6 -0

+	if (unlikely(!domain->ops->unbind_pasid_table))
+		return -EINVAL;
+
+	return domain->ops->unbind_pasid_table(domain, dev);
+}
+EXPORT_SYMBOL_GPL(iommu_unbind_pasid_table);  

Are there other reasons to let the unbind fail? Otherwise I'd suggest to just make this a void function. Also not sure what the user of this function should do when the unbind really fails.

include/uapi/linux/iommu.h line 16

Quoted 2 lines

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

+enum pasid_table_model {
+	PASID_TABLE_FORMAT_HOST,  

What is this FORMAT_HOST for?

Quoted 16 lines

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

+	PASID_TABLE_FORMAT_ARM_1LVL,
+	PASID_TABLE_FORMAT_ARM_2LVL,
+	PASID_TABLE_FORMAT_AMD,
+	PASID_TABLE_FORMAT_INTEL,
+};
+
+/**
+ * PASID table data used to bind guest PASID table to the host

IOMMU. This will
+ * enable guest managed first level page tables.
+ * @version: for future extensions and identification of the data format
+ * @bytes: size of this structure
+ * @base_ptr: PASID table pointer
+ * @pasid_bits: number of bits supported in the guest PASID
table, must be less

include/uapi/linux/iommu.h line 34

Quoted 5 lines

+ * or equal than the host table size.
+ * @model: PASID table format for different IOMMU models
+ */

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

+struct pasid_table_config {
+	__u32 version;  

Can you also add a define for the version number? Userspace needs it to initialize the struct and the kernel to check against it.

Quoted 13 lines

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

+	__u32 bytes;
+	__u64 base_ptr;
+	__u8 pasid_bits;
+	enum pasid_table_model model;
+	union {
+		struct {
+			/* Intel specific fields */
+		} intel;
+
+		struct {
+			/* ARM specific fields */
+			bool pasid0_dma_no_pasid;
+		} arm;

include/uapi/linux/iommu.h line 51

Quoted 4 lines

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

+
+		struct {
+			/* AMD specific fields */
+		} amd;  

Thinking more about this, we can omit the sub-structs for models that don't need them. For the amd-model for example the base_ptr and pasid_bits fields are sufficient.

Regards,

Joerg

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

As part of the proposed architecture, when an SVM capable PCI device is assigned to a guest, nested mode is turned on. Guest owns the first level page tables (request with PASID) which 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 API functions to perform bind/unbind guest PASID tables. Based on common data, model specific IOMMU drivers can be extended to perform the specific steps for binding pasid table of assigned devices.

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      | 25 +++++++++++++++++++++
 include/uapi/linux/iommu.h | 55 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 insertions(+)
 create mode 100644 include/uapi/linux/iommu.h

drivers/iommu/iommu.c +19 -0

index 3de5c0b..761cf50 100644
@@ -1322,6 +1322,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_config *pasidt_binfo)
+{
+	if (unlikely(!domain->ops->bind_pasid_table))
+		return -ENODEV;
+
+	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 41b8c57..672cc06 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)

@@ -187,6 +188,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);

include/linux/iommu.h +21 -0

 	bool (*is_attach_deferred)(struct iommu_domain *domain, struct device *dev);
 
+	int (*bind_pasid_table)(struct iommu_domain *domain, struct device *dev,
+				struct pasid_table_config *pasidt_binfo);
+	int (*unbind_pasid_table)(struct iommu_domain *domain,
+				struct device *dev);
+
 	unsigned long pgsize_bitmap;
 };
 

@@ -296,6 +305,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_config *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);

@@ -696,6 +709,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_config *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 +8 -0

new file mode 100644
index 0000000..aeeaf0e
@@ -0,0 +1,55 @@
+/*
+ * 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 +25 -0

+ * @model:	PASID table format for different IOMMU models
+ */
+struct pasid_table_config {
+	__u32 version;
+	__u32 bytes;
+	__u64 base_ptr;
+	__u8 pasid_bits;
+	enum pasid_table_model model;
+	union {
+		struct {
+			/* Intel specific fields */
+		} intel;
+
+		struct {
+			/* ARM specific fields */
+			bool pasid0_dma_no_pasid;
+		} arm;
+
+		struct {
+			/* AMD specific fields */
+		} amd;
+	};
+};
+
+#endif /* _UAPI_IOMMU_H */

Quoted 86 lines · Jacob Pan

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

As part of the proposed architecture, when an SVM capable PCI device is assigned to a guest, nested mode is turned on. Guest owns the first level page tables (request with PASID) which 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 API functions to perform bind/unbind guest PASID tables. Based on common data, model specific IOMMU drivers can be extended to perform the specific steps for binding pasid table of assigned devices.

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      | 25 +++++++++++++++++++++
 include/uapi/linux/iommu.h | 55

++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 include/uapi/linux/iommu.h

drivers/iommu/iommu.c +17 -0

index 3de5c0b..761cf50 100644
@@ -1322,6 +1322,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_config *pasidt_binfo)
+{
+	if (unlikely(!domain->ops->bind_pasid_table))
+		return -ENODEV;
+
+	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 41b8c57..672cc06 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)

@@ -187,6 +188,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 +0 -0

 	bool (*capable)(enum iommu_cap);

@@ -233,8 +236,14 @@ struct iommu_ops {
 	u32 (*domain_get_windows)(struct iommu_domain *domain);
 

include/linux/iommu.h struct iommu_ops { · line 240

Quoted 3 lines

include/linux/iommu.h +1 -0

 	int (*of_xlate)(struct device *dev, struct of_phandle_args
 *args);
+  

(whitespace change)

Quoted 69 lines

include/linux/iommu.h +5 -0

 	bool (*is_attach_deferred)(struct iommu_domain *domain,
 struct device *dev); 
+	int (*bind_pasid_table)(struct iommu_domain *domain,
 struct device *dev,
+				struct pasid_table_config
 *pasidt_binfo);
+	int (*unbind_pasid_table)(struct iommu_domain *domain,
+				struct device *dev);
+

include/linux/iommu.h +3 -0

 	unsigned long pgsize_bitmap;
 };

@@ -296,6 +305,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_config
 *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

@@ -696,6 +709,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_config *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..aeeaf0e
@@ -0,0 +1,55 @@
+/*
+ * 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 +8 -0

+
+#ifndef _UAPI_IOMMU_H
+#define _UAPI_IOMMU_H
+
+#include <linux/types.h>
+
+enum pasid_table_model {
+	PASID_TABLE_FORMAT_HOST,

include/uapi/linux/iommu.h line 18

Quoted 2 lines

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

+	PASID_TABLE_FORMAT_ARM_1LVL,
+	PASID_TABLE_FORMAT_ARM_2LVL,  

Maybe remove the ARM values and struct for the moment, I'm still not sure how to implement it. I think this should be a single ARM_SMMUV3 model (2LVL might correspond to two different formats in SMMUv3, and a future SMMU version could still have 1- or 2-level PASID table but incompatible format).

Quoted 12 lines

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

+	PASID_TABLE_FORMAT_AMD,
+	PASID_TABLE_FORMAT_INTEL,
+};
+
+/**
+ * PASID table data used to bind guest PASID table to the host

IOMMU. This will
+ * enable guest managed first level page tables.
+ * @version: for future extensions and identification of the data format
+ * @bytes: size of this structure
+ * @base_ptr: PASID table pointer

Quoted 3 lines

+ * @pasid_bits: number of bits supported in the guest PASID
table, must be less
+ * or equal than the host table size.

"host table size" is a bit confusing in this context, especially if using multi-level tables. Perhaps it's clear enough that @pasid_bits must be smaller or equal than the PASID size supported by the IOMMU, and we can remove that second part?

Thanks,
Jean

Quoted 18 lines

+ * @model: PASID table format for different IOMMU models
+ */

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

+struct pasid_table_config {
+	__u32 version;
+	__u32 bytes;
+	__u64 base_ptr;
+	__u8 pasid_bits;
+	enum pasid_table_model model;
+	union {
+		struct {
+			/* Intel specific fields */
+		} intel;
+
+		struct {
+			/* ARM specific fields */
+			bool pasid0_dma_no_pasid;
+		} arm;
+

include/uapi/linux/iommu.h line 55

Quoted 8 lines

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

+		struct {
+			/* AMD specific fields */
+		} amd;
+	};
+};
+
+#endif /* _UAPI_IOMMU_H */

Quoted 12 lines

Quoted 11 lines

+/**
+ * PASID table data used to bind guest PASID table to the host IOMMU. This will
+ * enable guest managed first level page tables.
+ * @version: for future extensions and identification of the data format
+ * @bytes: size of this structure
+ * @base_ptr: PASID table pointer
+ * @pasid_bits: number of bits supported in the guest PASID
table, must be less
+ * or equal than the host table size.

Quoted 5 lines

"host table size" is a bit confusing in this context, especially if using multi-level tables. Perhaps it's clear enough that @pasid_bits must be smaller or equal than the PASID size supported by the IOMMU, and we can remove that second part?

Not sure what is the second part?

#define CONTEXT_DINVE (1ULL << 8)
#define CONTEXT_PRS (1ULL << 9)
+#define CONTEXT_NESTE (1ULL << 10)

From: Jacob Pan [mailto:jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Friday, October 6, 2017 7:04 AM
To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; LKML <[email protected]>; Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; Jean-Philippe Brucker <jean- [email protected]>
Cc: Liu, Yi L <[email protected]>; Lan, Tianyu <[email protected]>; Tian, Kevin <[email protected]>; Raj, Ashok <[email protected]>; Alex Williamson <[email protected]>; Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>; Liu; Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>
Subject: [PATCH v2 02/16] iommu/vt-d: add bind_pasid_table function

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   | 117
++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma_remapping.h |   1 +
 2 files changed, 118 insertions(+)

drivers/iommu/intel-iommu.c index renamed from drivers/iommu/intel-iommu.c +0 -0

209d99a..7ae569c 100644

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

@@ -5200,6 +5200,7 @@ static void intel_iommu_put_resv_regions(struct device
 *dev,
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 #define MAX_NR_PASID_BITS (20)
+#define MIN_NR_PASID_BITS (5)
 static inline unsigned long intel_iommu_get_pts(struct intel_iommu *iommu)  {
 	/*

@@ -5326,6 +5327,118 @@ 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_config *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, host_table_pasid_bits;
+	u16 did, sid;
+	int ret = 0;
+	unsigned long flags;
+	u64 ctx_lo;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+	/* VT-d spec 9.4 says pasid table size is encoded as 2^(x+5) */
+	host_table_pasid_bits = intel_iommu_get_pts(iommu) +
 MIN_NR_PASID_BITS;
+	if (!pasidt_binfo || pasidt_binfo->pasid_bits > host_table_pasid_bits ||
+		pasidt_binfo->pasid_bits < MIN_NR_PASID_BITS) {
+		pr_err("Invalid gPASID bits %d, host range %d - %d\n",
+			pasidt_binfo->pasid_bits,
+			MIN_NR_PASID_BITS, host_table_pasid_bits);
+		return -ERANGE;
+	}
+
+	pdev = to_pci_dev(dev);
+	if (!pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI))
+		return -EINVAL;
+	sid = PCI_DEVID(bus, devfn);
+
+	info = dev->archdata.iommu;
+	if (!info || !info->pasid_supported) {
+		dev_err(dev, "No PASID support\n");
+		ret = -EINVAL;
+		goto out;
+	}

Quoted 107 lines

From: Jacob Pan [mailto:[email protected]]
Sent: Friday, October 6, 2017 7:04 AM
To: [email protected]; LKML <[email protected]>; Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; Jean-Philippe Brucker <jean- [email protected]> Cc: Liu, Yi L <[email protected]>; Lan, Tianyu <[email protected]>; Tian, Kevin <[email protected]>; Raj, Ashok <[email protected]>; Alex Williamson <[email protected]>; Jacob Pan <[email protected]>; Liu; Yi L <[email protected]> Subject: [PATCH v2 02/16] iommu/vt-d:
add bind_pasid_table function

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   | 117
++++++++++++++++++++++++++++++++++++++++++
 include/linux/dma_remapping.h |   1 +
 2 files changed, 118 insertions(+)

(quoted diff) +0 -0

b/drivers/iommu/intel-iommu.c index 209d99a..7ae569c 100644

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

@@ -5200,6 +5200,7 @@ static void
 intel_iommu_put_resv_regions(struct device *dev,
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 #define MAX_NR_PASID_BITS (20)
+#define MIN_NR_PASID_BITS (5)
 static inline unsigned long intel_iommu_get_pts(struct intel_iommu
 *iommu)  { /*

@@ -5326,6 +5327,118 @@ 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_config
 *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, host_table_pasid_bits;
+	u16 did, sid;
+	int ret = 0;
+	unsigned long flags;
+	u64 ctx_lo;
+
+	iommu = device_to_iommu(dev, &bus, &devfn);
+	if (!iommu)
+		return -ENODEV;
+	/* VT-d spec 9.4 says pasid table size is encoded as

2^(x+5) */

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

+	host_table_pasid_bits = intel_iommu_get_pts(iommu) +

MIN_NR_PASID_BITS;

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

+	if (!pasidt_binfo || pasidt_binfo->pasid_bits >

host_table_pasid_bits ||

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

+		pasidt_binfo->pasid_bits < MIN_NR_PASID_BITS) {
+		pr_err("Invalid gPASID bits %d, host range %d -

%d\n",

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

+			pasidt_binfo->pasid_bits,
+			MIN_NR_PASID_BITS, host_table_pasid_bits);
+		return -ERANGE;
+	}
+
+	pdev = to_pci_dev(dev);
+	if (!pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PRI))
+		return -EINVAL;
+	sid = PCI_DEVID(bus, devfn);
+
+	info = dev->archdata.iommu;
+	if (!info || !info->pasid_supported) {
+		dev_err(dev, "No PASID support\n");
+		ret = -EINVAL;
+		goto out;
+	}
+	if (!info->pasid_enabled) {
+		ret = pci_enable_pasid(pdev, info->pasid_supported

& ~1);

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

+		if (ret)
+			goto out;
+	}
+	if (!device_context_mapped(iommu, bus, devfn)) {
+		pr_warn("ctx not mapped for bus devfn %x:%x\n",

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

Quoted 4 lines

bus, devfn);

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

+		ret = -EINVAL;
+		goto out;
+	}  

[Liu, Yi L] This is checking whether ctx is present. So if it is true, then the following 6 line should be always true. Perhaps, a merge could be done here with the following 6 lines.

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

Quoted 7 lines

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

+	spin_lock_irqsave(&iommu->lock, flags);
+	context = iommu_context_addr(iommu, bus, devfn, 0);
+	if (!context) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+  

Regards,
Yi L

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

+	IOMMU_INV_GRANU_DOMAN_PAGE,	/* address range with a domain */
+	IOMMU_INV_GRANU_ALL_PASID,	/* cache of a given PASID */
+	IOMMU_INV_GRANU_PASID_SEL,	/* only invalidate specified PASID */
+
+	IOMMU_INV_GRANU_NG_ALL_PASID,	/* non-global within all PASIDs */

Quoted 3 lines · Jacob Pan

(quoted diff) +2 -0

+int iommu_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info

*inv_info)

This name is way too generic, it should at least be called iommu_svm_invalidate() or something like that. With the name above it is easily confused with the other TLB invalidation functions of the IOMMU-API.

Quoted 3 lines

(quoted diff) +2 -0

+enum iommu_inv_granularity {
+	IOMMU_INV_GRANU_GLOBAL,		/* all TLBs

invalidated */

Is that needed? We certainly don't want to give userspace/guests that fine-grained control about IOMMU cache invalidations.

In the end a guest issues flush-global command does not translate to a flush-global on the host, but to separate flushes for the domains the guest uses.

include/uapi/linux/iommu.h struct pasid_table_config { · line 59

Quoted 5 lines

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DOMAIN,		/* all TLBs

associated with a domain */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DEVICE,		/* caching

structure associated with a

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

+					 * device ID  

What is the difference between a DOMAIN and a DEVICE flush?

Quoted 18 lines

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DOMAN_PAGE,	/* address range with a

domain */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_ALL_PASID,	/* cache of a given

PASID */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_PASID_SEL,	/* only invalidate

specified PASID */ +

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_NG_ALL_PASID,	/* non-global within

all PASIDs */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_NG_PASID,	/* non-global within a

PASIDs */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_PAGE_PASID,	/* page-selective

within a PASID */

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

+	IOMMU_INV_NR_GRANU,
+};
+
+enum iommu_inv_type {
+	IOMMU_INV_TYPE_DTLB,	/* device IOTLB */
+	IOMMU_INV_TYPE_TLB,	/* IOMMU paging structure cache

Quoted 4 lines

*/

(quoted diff) +2 -0

+	IOMMU_INV_TYPE_PASID,	/* PASID cache */
+	IOMMU_INV_TYPE_CONTEXT,	/* device context entry

cache */

Is that really needed? When the guest updates it context-entry equivalent it translates to bind_pasid_table/unbind_pasid_table calls, no?

Quoted 10 lines · On Tue, 10 Oct 2017 15:35:42 +0200 Joerg Roedel…

Quoted 3 lines · Jacob Pan

(quoted diff) +2 -0

+int iommu_invalidate(struct iommu_domain *domain,
+		struct device *dev, struct tlb_invalidate_info

*inv_info)

This name is way too generic, it should at least be called iommu_svm_invalidate() or something like that. With the name above it is easily confused with the other TLB invalidation functions of the IOMMU-API.

Good point. I was calling it iommu_passdown_invalidate() originally. The invalidation request comes from guest or user space instead of in-kernel unmap kind of calls.

Quoted 7 lines

Quoted 3 lines

(quoted diff) +2 -0

+enum iommu_inv_granularity {
+	IOMMU_INV_GRANU_GLOBAL,		/* all TLBs

invalidated */

Is that needed? We certainly don't want to give userspace/guests that fine-grained control about IOMMU cache invalidations.

Quoted 4 lines

In the end a guest issues flush-global command does not translate to a flush-global on the host, but to separate flushes for the domains the guest uses.

Right, guest should not go beyond its own domain.

Quoted 8 lines

include/uapi/linux/iommu.h struct pasid_table_config { · line 59

Quoted 5 lines

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DOMAIN,		/* all TLBs

associated with a domain */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DEVICE,		/* caching

structure associated with a

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

+					 * device ID

What is the difference between a DOMAIN and a DEVICE flush?

Those are based on vt-d context cache flush granularity, domain selective flushes all context caches associated with a domain ID.
Device selective flush flushes context caches of a source ID.
But like you pointed out below, since context cache flush will come in as unbind call, there is no need to do passdown invalidate. I can remove that.

Here I am trying to use all generic definitions, which is a superset of all vendor models. I am likely missing out some non-vt-d cases.

Quoted 18 lines

Quoted 18 lines

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_DOMAN_PAGE,	/* address range with a

domain */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_ALL_PASID,	/* cache of a given

PASID */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_PASID_SEL,	/* only invalidate

specified PASID */ +

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_NG_ALL_PASID,	/* non-global within

all PASIDs */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_NG_PASID,	/* non-global within a

PASIDs */

(quoted diff) +1 -0

+	IOMMU_INV_GRANU_PAGE_PASID,	/* page-selective

within a PASID */

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

+	IOMMU_INV_NR_GRANU,
+};
+
+enum iommu_inv_type {
+	IOMMU_INV_TYPE_DTLB,	/* device IOTLB */
+	IOMMU_INV_TYPE_TLB,	/* IOMMU paging structure cache

Quoted 9 lines

Quoted 4 lines

*/

(quoted diff) +2 -0

+	IOMMU_INV_TYPE_PASID,	/* PASID cache */
+	IOMMU_INV_TYPE_CONTEXT,	/* device context entry

cache */

Is that really needed? When the guest updates it context-entry equivalent it translates to bind_pasid_table/unbind_pasid_table calls, no?

Right no need to passdown context cache invalidation for VT-d. I just wasn't sure it is the same for all models. Again, trying to have a superset of generic fields.

Thanks!

Jacob

I agree that iommu_invalidate() is too generic. Additionally, also better to avoid making it svm specific.

The reason we introduce this API is in vSVM case is that guest owns the first level page table(vtd). If we use similar mechanism for vIOVA, then we also need to passdown guest's vIOVA tlb flush.

Since it is to expose an API for iommu tlb flushes requests from userspace/guest which is out of iommu. How about naming it as iommu_tlb_external_invalidate()?

Quoted 2 lines

I agree that iommu_invalidate() is too generic. Additionally, also better to avoid making it svm specific.

I also don't like to name the functions after the Intel feature, but I failed to come up with a better alternative so far. The only one I can come up with for now would be 'iovm', so the function name would be iommu_iovm_invalidate().

On the other side, the ARM guys also already call the feature set 'SVM', despite it being ambiguous and Intel specific. I don't have a strong opinion on the naming.

Quoted 4 lines

The reason we introduce this API is in vSVM case is that guest owns the first level page table(vtd). If we use similar mechanism for vIOVA, then we also need to passdown guest's vIOVA tlb flush.

Quoted 3 lines

Since it is to expose an API for iommu tlb flushes requests from userspace/guest which is out of iommu. How about naming it as iommu_tlb_external_invalidate()?

If you only read the function name, 'external' could mean everything. It is not clear

from the name when to use this function. So something like iommu_iovm_invalidate() is better.

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

Quoted 5 lines · Joerg Roedel

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

-----Original Message-----
From: Bob Liu [mailto:[email protected]]
Sent: Thursday, October 12, 2017 5:39 PM
To: Jean-Philippe Brucker <[email protected]>; Joerg Roedel <[email protected]>; Liu, Yi L <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; LKML <[email protected]>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; David Woodhouse <[email protected]>
Subject: Re: [PATCH v2 03/16] iommu: introduce iommu invalidate API function

Quoted 7 lines · Jean-Philippe Brucker

Quoted 5 lines · Joerg Roedel

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

Quoted 7 lines

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

The process "vaddr" can be the same as "IOVA" by using the classical map/unmap way.
This is also a kind of share virtual memory/address(except have to pin physical memory).
How to distinguish these two different implementation of "share virtual memory/address"?

Quoted 29 lines

-----Original Message-----
From: Bob Liu [mailto:[email protected]]
Sent: Thursday, October 12, 2017 5:39 PM
To: Jean-Philippe Brucker <[email protected]>; Joerg Roedel <[email protected]>; Liu, Yi L <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; LKML <[email protected]>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; David Woodhouse <[email protected]>
Subject: Re: [PATCH v2 03/16] iommu: introduce iommu invalidate API function

Quoted 7 lines · Jean-Philippe Brucker

Quoted 5 lines · Joerg Roedel

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

Quoted 7 lines

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

Quoted 7 lines

The process "vaddr" can be the same as "IOVA" by using the classical map/unmap way.
This is also a kind of share virtual memory/address(except have to pin physical memory).
How to distinguish these two different implementation of "share virtual memory/address"?

[Liu, Yi L] Not sure if I get your idea well. Process "vaddr" is owned by process and maintained by mmu, while "IOVA" is maintained by iommu. So they are different in the way they are maintained. Since process "vaddr" is maintained by mmu and then used by iommu, so we call it shared virtual memory/address. This is how "shared" term comes.

Quoted 36 lines · Yi L

Quoted 29 lines

-----Original Message-----
From: Bob Liu [mailto:[email protected]]
Sent: Thursday, October 12, 2017 5:39 PM
To: Jean-Philippe Brucker <[email protected]>; Joerg Roedel <[email protected]>; Liu, Yi L <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Liu, Yi L <[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; LKML <[email protected]>; [email protected]; David Woodhouse <[email protected]>
Subject: Re: [PATCH v2 03/16] iommu: introduce iommu invalidate API function

Quoted 7 lines · Jean-Philippe Brucker

Quoted 5 lines · Joerg Roedel

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

Quoted 7 lines

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

Quoted 7 lines

The process "vaddr" can be the same as "IOVA" by using the classical map/unmap way.
This is also a kind of share virtual memory/address(except have to pin physical memory).
How to distinguish these two different implementation of "share virtual memory/address"?

Quoted 4 lines

[Liu, Yi L] Not sure if I get your idea well. Process "vaddr" is owned by process and maintained by mmu, while "IOVA" is maintained by iommu. So they are different in the way they are maintained. Since process "vaddr" is maintained by mmu and then used by iommu, so we call it shared virtual memory/address. This is how "shared" term comes.

I think from the view of application, the share virtual memory/address(or Nvidia-CUDA unify virtual address) is like this:

1. vaddr = malloc(); e.g vaddr=0x10000
2. device can get the same data(accessing the same physical memory) through same address e.g 0x10000, and don't care about it's a vaddr or IOVA.. (actually in Nvidia-cuda case, the data will be migrated between system-ddr and gpu-memory, but the vaddr is always the same for CPU and GPU).

So there are two ways(beside Nvidia way) to implement this requirement:
1)
get the physical memory of vaddr;
dma_map the paddr to iova;
If we appoint iova = vaddr (e.g iova can be controlled by the user space driver through vfio DMA_MAP), This can also be called share virtual address between CPU process and device..

2)
The second way is what this RFC did.

-----Original Message-----
From: Bob Liu [mailto:[email protected]]
Sent: Thursday, October 12, 2017 6:08 PM
To: Liu, Yi L <[email protected]>; Jean-Philippe Brucker <jean- [email protected]>; Joerg Roedel <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; LKML <[email protected]>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; David Woodhouse <[email protected]>
Subject: Re: [PATCH v2 03/16] iommu: introduce iommu invalidate API function

Quoted 39 lines · Yi L

Quoted 32 lines

-----Original Message-----
From: Bob Liu [mailto:[email protected]]
Sent: Thursday, October 12, 2017 5:39 PM
To: Jean-Philippe Brucker <[email protected]>; Joerg
Roedel <[email protected]>; Liu, Yi L <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Liu, Yi L <yi.l.liu-VuQAYsv1563Yd54FQh9/[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>; LKML <[email protected]>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; David Woodhouse <[email protected]>
Subject: Re: [PATCH v2 03/16] iommu: introduce iommu invalidate API
function

Quoted 7 lines · Jean-Philippe Brucker

Quoted 5 lines · Joerg Roedel

Quoted 1 line · Yi L

I didn't quite get 'iovm' mean. Can you explain a bit about the idea?

It's short for IO Virtual Memory, basically a replacement term for 'svm' that is not ambiguous (afaik) and not specific to Intel.

Quoted 7 lines

I wonder if SVM originated in OpenCL first, rather than intel? That's why I'm using it, but it is ambiguous. I'm not sure IOVM is precise enough though, since the name could as well be used without shared tables, for classical map/unmap and IOVAs. Kevin Tian suggested SVA "Shared Virtual Addressing" last time, which is a little more clear than SVM and isn't used elsewhere in the kernel either.

Quoted 7 lines

The process "vaddr" can be the same as "IOVA" by using the classical map/unmap way.
This is also a kind of share virtual memory/address(except have to pin physical memory).
How to distinguish these two different implementation of "share virtual memory/address"?

Quoted 4 lines

[Liu, Yi L] Not sure if I get your idea well. Process "vaddr" is owned by process and maintained by mmu, while "IOVA" is maintained by iommu. So they are different in the way they are maintained. Since process "vaddr" is maintained by mmu and then used by iommu, so we call it shared virtual

memory/address. This is how "shared" term comes.

I think from the view of application, the share virtual memory/address(or Nvidia- CUDA unify virtual address) is like this:

1. vaddr = malloc(); e.g vaddr=0x10000
2. device can get the same data(accessing the same physical memory) through same address e.g 0x10000, and don't care about it's a vaddr or IOVA.. (actually in Nvidia-cuda case, the data will be migrated between system-ddr and gpu- memory, but the vaddr is always the same for CPU and GPU).

So there are two ways(beside Nvidia way) to implement this requirement:
1)
get the physical memory of vaddr;
dma_map the paddr to iova;
If we appoint iova = vaddr (e.g iova can be controlled by the user space driver through vfio DMA_MAP), This can also be called share virtual address between CPU process and device..

2)
The second way is what this RFC did.

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

---
  drivers/iommu/dmar.c        | 53 ++++++++++++++++++++++++++++++++++++++++++---
  drivers/iommu/intel-iommu.c |  3 ++-
  include/linux/intel-iommu.h | 10 +++++++--
  3 files changed, 60 insertions(+), 6 deletions(-)

drivers/iommu/dmar.c +16 -2

index 57c920c..2fbff8b 100644
@@ -1336,11 +1336,25 @@ void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
  	qi_submit_sync(&desc, iommu);
  }
 
-void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
-			u64 addr, unsigned mask)
+void qi_flush_eiotlb(struct intel_iommu *iommu, u16 did, u64 addr, u32 pasid,
+		unsigned int size_order, u64 granu, bool global)
  {
  	struct qi_desc desc;
 
+	desc.low = QI_EIOTLB_PASID(pasid) | QI_EIOTLB_DID(did) |
+		QI_EIOTLB_GRAN(granu) | QI_EIOTLB_TYPE;
+	desc.high = QI_EIOTLB_ADDR(addr) | QI_EIOTLB_GL(global) |
+		QI_EIOTLB_IH(0) | QI_EIOTLB_AM(size_order);
+	qi_submit_sync(&desc, iommu);
+}
+
+void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
+			u16 qdep, u64 addr, unsigned mask)
+{
+	struct qi_desc desc;
+
+	pr_debug_ratelimited("%s: sid %d, pfsid %d, qdep %d, addr %llx, mask %d\n",
+		__func__, sid, pfsid, qdep, addr, mask);
  	if (mask) {
  		BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) - 1));
  		addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;

drivers/iommu/dmar.c +33 -0

+
+	qi_submit_sync(&desc, iommu);
+}
+
+void qi_flush_dev_eiotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
+			u32 pasid,  u16 qdep, u64 addr, unsigned size)
+{
+	struct qi_desc desc;
+
+	desc.low = QI_DEV_EIOTLB_PASID(pasid) | QI_DEV_EIOTLB_SID(sid) |
+		QI_DEV_EIOTLB_QDEP(qdep) | QI_DEIOTLB_TYPE |
+		QI_DEV_EIOTLB_PFSID(pfsid);
+
+	/* If S bit is 0, we only flush a single page. If S bit is set,
+	 * The least significant zero bit indicates the size. VT-d spec
+	 * 6.5.2.6
+	 */
+	if (!size)
+		desc.high = QI_DEV_EIOTLB_ADDR(addr) & ~QI_DEV_EIOTLB_SIZE;
+	else {
+		unsigned long mask = 1UL << (VTD_PAGE_SHIFT + size);
+
+		desc.high = QI_DEV_EIOTLB_ADDR(addr & ~mask) | QI_DEV_EIOTLB_SIZE;
+	}
+	qi_submit_sync(&desc, iommu);
+}
+
+void qi_flush_pasid(struct intel_iommu *iommu, u16 did, u64 granu, int pasid)
+{
+	struct qi_desc desc;
+
+	desc.high = 0;
+	desc.low = QI_PC_TYPE | QI_PC_DID(did) | QI_PC_GRAN(granu) | QI_PC_PASID(pasid);
 
  	qi_submit_sync(&desc, iommu);
  }

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

index 7ae569c..6832f73 100644
@@ -1567,7 +1567,8 @@ static void iommu_flush_dev_iotlb(struct dmar_domain *domain,
 
  		sid = info->bus << 8 | info->devfn;
  		qdep = info->ats_qdep;
-		qi_flush_dev_iotlb(info->iommu, sid, qdep, addr, mask);
+		qi_flush_dev_iotlb(info->iommu, sid, info->pfsid,
+				qdep, addr, mask);
  	}
  	spin_unlock_irqrestore(&device_domain_lock, flags);
  }

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

index 485a5b4..e42d317 100644
@@ -305,6 +305,7 @@ enum {
  #define QI_DEV_EIOTLB_PASID(p)	(((u64)p) << 32)
  #define QI_DEV_EIOTLB_SID(sid)	((u64)((sid) & 0xffff) << 16)
  #define QI_DEV_EIOTLB_QDEP(qd)	((u64)((qd) & 0x1f) << 4)
+#define QI_DEV_EIOTLB_PFSID(pfsid) (((u64)(pfsid & 0xf) << 12) | ((u64)(pfsid & 0xff0) << 48))
  #define QI_DEV_EIOTLB_MAX_INVS	32
 
  #define QI_PGRP_IDX(idx)	(((u64)(idx)) << 55)

@@ -450,8 +451,13 @@ extern void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid,
  			     u8 fm, u64 type);
  extern void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
  			  unsigned int size_order, u64 type);
-extern void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep,
-			       u64 addr, unsigned mask);

Quoted 46 lines · Jacob Pan

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

---
  drivers/iommu/dmar.c        | 53

++++++++++++++++++++++++++++++++++++++++++---
drivers/iommu/intel-iommu.c | 3 ++- include/linux/intel-iommu.h | 10 +++++++-- 3 files changed, 60 insertions(+), 6 deletions(-)

drivers/iommu/dmar.c +15 -2

index 57c920c..2fbff8b 100644
@@ -1336,11 +1336,25 @@ void qi_flush_iotlb(struct intel_iommu
 *iommu, u16 did, u64 addr, qi_submit_sync(&desc, iommu);
  }
 
-void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16
 qdep,
-			u64 addr, unsigned mask)
+void qi_flush_eiotlb(struct intel_iommu *iommu, u16 did, u64 addr,
 u32 pasid,
+		unsigned int size_order, u64 granu, bool global)
  {
  	struct qi_desc desc;
 
+	desc.low = QI_EIOTLB_PASID(pasid) | QI_EIOTLB_DID(did) |
+		QI_EIOTLB_GRAN(granu) | QI_EIOTLB_TYPE;
+	desc.high = QI_EIOTLB_ADDR(addr) | QI_EIOTLB_GL(global) |
+		QI_EIOTLB_IH(0) | QI_EIOTLB_AM(size_order);
+	qi_submit_sync(&desc, iommu);
+}
+
+void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16
 pfsid,
+			u16 qdep, u64 addr, unsigned mask)
+{
+	struct qi_desc desc;
+
+	pr_debug_ratelimited("%s: sid %d, pfsid %d, qdep %d, addr

%llx, mask %d\n",

drivers/iommu/dmar.c +1 -0

+		__func__, sid, pfsid, qdep, addr, mask);
  	if (mask) {
  		BUG_ON(addr & ((1 << (VTD_PAGE_SHIFT + mask)) -

1)); addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;

drivers/iommu/dmar.c +0 -0

@@ -1352,7 +1366,40 @@ void qi_flush_dev_iotlb(struct intel_iommu
 *iommu, u16 sid, u16 qdep, qdep = 0;
 

drivers/iommu/dmar.c void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 qdep, · line 1369

Quoted 4 lines

drivers/iommu/dmar.c +0 -0

  	desc.low = QI_DEV_IOTLB_SID(sid) |

QI_DEV_IOTLB_QDEP(qdep) |

drivers/iommu/dmar.c +1 -1

-		   QI_DIOTLB_TYPE;
+		   QI_DIOTLB_TYPE | QI_DEV_IOTLB_SID(pfsid);  

So this change just combining sid and pfsid together, i.e. (sid | pfsid)? What if both of them are not zero?

[...]

Device faults detected by IOMMU can be reported outside IOMMU subsystem. This patch intends to provide a generic device fault data such that device drivers can communicate IOMMU faults without model specific knowledge.

The assumption is that model specific IOMMU driver can filter and handle most of the IOMMU faults if the cause is within IOMMU driver control. Therefore, the fault reasons can be reported are grouped and generalized based common specifications such as PCI ATS.

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

---
 include/linux/iommu.h | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

include/linux/iommu.h +2 -0

index 4af1820..3f9b367 100644
@@ -49,6 +49,7 @@ struct bus_type;
 struct device;
 struct iommu_domain;
 struct notifier_block;
+struct iommu_fault_event;
 
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0

@@ -56,6 +57,7 @@ struct notifier_block;
 
 typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
+typedef int (*iommu_dev_fault_handler_t)(struct device *, struct iommu_fault_event *);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */

include/linux/iommu.h +10 -0

+/**
+ * struct iommu_fault_event - Generic per device fault data
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request), information based on PCI ATS
+ * and PASID spec.
+ * - Un-recoverable faults of device interest
+ * - DMA remapping and IRQ remapping faults
+
+ * @type contains fault type.

include/linux/iommu.h +6 -0

+	u32 page_req_group_id : 9;> +	u32 last_req : 1;
+	u32 pasid_valid : 1;
+	u32 prot;
+	u32 private_data;
+};
+
 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,

@@ -425,6 +481,18 @@ struct iommu_fwspec {
 	u32			ids[1];
 };
 

include/linux/iommu.h +3 -0

+};
+
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops);
 void iommu_fwspec_free(struct device *dev);

@@ -437,6 +505,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)
 {

include/linux/iommu.h struct iommu_device { · line 273

Quoted 5 lines

include/linux/iommu.h +5 -0

+enum iommu_model {
+	IOMMU_MODEL_INTEL = 1,
+	IOMMU_MODEL_AMD,
+	IOMMU_MODEL_SMMU3,
+};  

Now unused, I guess?

-----Original Message-----
From: Jean-Philippe Brucker [mailto:[email protected]]
Sent: Wednesday, October 11, 2017 3:29 AM
To: Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>; iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; LKML <[email protected]>; Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: Liu, Yi L <[email protected]>; Lan, Tianyu <[email protected]>; Tian, Kevin <[email protected]>; Raj, Ashok <[email protected]>; Alex Williamson <[email protected]>
Subject: Re: [PATCH v2 08/16] iommu: introduce device fault data

Quoted 38 lines · Jacob Pan

Device faults detected by IOMMU can be reported outside IOMMU subsystem. This patch intends to provide a generic device fault data such that device drivers can communicate IOMMU faults without model specific knowledge.

The assumption is that model specific IOMMU driver can filter and handle most of the IOMMU faults if the cause is within IOMMU driver control. Therefore, the fault reasons can be reported are grouped and generalized based common specifications such as PCI ATS.

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

---
 include/linux/iommu.h | 69
+++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

include/linux/iommu.h index renamed from include/linux/iommu.h +0 -0

4af1820..3f9b367 100644

include/linux/iommu.h +3 -0

@@ -49,6 +49,7 @@ struct bus_type;
 struct device;
 struct iommu_domain;
 struct notifier_block;
+struct iommu_fault_event;
 
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0

@@ -56,6 +57,7 @@ struct notifier_block;
 
 typedef int (*iommu_fault_handler_t)(struct iommu_domain *,
 			struct device *, unsigned long, int, void *);
+typedef int (*iommu_dev_fault_handler_t)(struct device *, struct
+iommu_fault_event *);
 
 struct iommu_domain_geometry {
 	dma_addr_t aperture_start; /* First address that can be mapped    */

include/linux/iommu.h struct iommu_device { · line 273

Quoted 8 lines

include/linux/iommu.h +5 -0

@@ -264,6 +266,60 @@ struct iommu_device {
 	struct device *dev;
 };
 
+enum iommu_model {
+	IOMMU_MODEL_INTEL = 1,
+	IOMMU_MODEL_AMD,
+	IOMMU_MODEL_SMMU3,
+};

Now unused, I guess?

include/linux/iommu.h struct iommu_device { · line 282

Quoted 9 lines

include/linux/iommu.h +9 -0

+
+/*  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_CTX = 1,

If I read the VT-d spec right, this is a fault encountered while fetching the PASID table pointer?

include/linux/iommu.h struct iommu_device { · line 283

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_ACCESS,

And this a pgd or pte access fault?

include/linux/iommu.h struct iommu_device { · line 284

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_INVALIDATE,

What would this be?

include/linux/iommu.h struct iommu_device { · line 286

Quoted 2 lines

include/linux/iommu.h +2 -0

+	IOMMU_FAULT_REASON_UNKNOWN,
+};

I'm currently doing the same exploratory work for virtio-iommu, and I'd be tempted to report reasons as detailed as possible to guest or device driver, but it's not clear what they need, how they would use this information. I'd like to discuss this some more.

For unrecoverable faults I guess CTX means "the host IOMMU driver is broken", since the device tables are invalid. In which case there is no use continuing, trying to shutdown the device cleanly is really all the guest/device driver can do.

For ACCESS the error is the device driver's or guest's fault, since the device driver triggered DMA on unmapped buffers, or the guest didn't install the right page tables. This can be repaired without shutting down, it may even just be one execution stream that failed in the device while the others continued normally. It's not as recoverable as a PRI Page Request, but the device driver may still be able to isolate the problem (e.g. by killing the process responsible) and the device to recover from it.

So maybe ACCESS would benefit from more details, for example differentiating faults encountered while fetching the pgd from those encountered while fetching a second-level table or pte. The former is a lot less recoverable than the latter (bug in the guest IOMMU driver vs.
bug in the device driver).

Generalizing this maybe we should differentiate each step of the translation in fault_reason:

* Device entry (context) fetch -> host IOMMU driver's fault
* PASID table fetch -> guest IOMMU driver or host userspace's fault
* pgd fetch -> guest IOMMU driver's fault
* pte fetch, including validity and access check -> device driver's fault

It's probably not worth mentioning intermediate table levels (pmd, etc). Thoughts?

include/linux/iommu.h struct iommu_device {

Quoted 12 lines

include/linux/iommu.h +12 -0

+/**
+ * struct iommu_fault_event - Generic per device fault data
+ *
+ * - PCI and non-PCI devices
+ * - Recoverable faults (e.g. page request), information based on PCI
+ATS
+ * and PASID spec.
+ * - Un-recoverable faults of device interest
+ * - DMA remapping and IRQ remapping faults
+
+ * @type contains fault type.
+ * @reason fault reasons if relevant outside IOMMU driver, IOMMU driver

internal

include/linux/iommu.h struct iommu_device { · line 308

Quoted 11 lines

include/linux/iommu.h +11 -0

+ *         faults are not reported
+ * @paddr: tells the offending page address
+ * @pasid: contains process address space ID, used in shared virtual
+ memory(SVM)
+ * @rid: requestor ID> + * @page_req_group_id: page request group
+ index
+ * @last_req: last request in a page request group
+ * @pasid_valid: indicates if the PRQ has a valid PASID
+ * @prot: page access protection flag, e.g. IOMMU_READ, IOMMU_WRITE
+ * @private_data: uniquely identify device-specific private data for an
+ *                individual page request

I understand this is for the streaming extension on VT-d, is it IOMMU-specific or

specific to the faulting endpoint? Could the device driver receiving the fault attempt to decode or modify this field before sending the page response?

include/linux/iommu.h struct iommu_device { · line 315

Quoted 8 lines

include/linux/iommu.h +7 -0

+ */
+struct iommu_fault_event {
+	enum iommu_fault_type type;
+	enum iommu_fault_reason reason;
+	u64 paddr;
+	u32 pasid;
+	u32 rid:16;

I think this is redundant, since you already pass the struct device to the fault handler. Otherwise it should probably be extended to 32 bits, for non-PCI or multiple PCI domains.

Quoted 14 lines

include/linux/iommu.h +6 -0

+	u32 page_req_group_id : 9;> +	u32 last_req : 1;
+	u32 pasid_valid : 1;
+	u32 prot;
+	u32 private_data;
+};
+
 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, @@ -425,6 +481,18 @@ struct iommu_fwspec {

include/linux/iommu.h +1 -0

 	u32			ids[1];
 };
 
+/**

include/linux/iommu.h struct iommu_fwspec { · line 493

Quoted 10 lines

include/linux/iommu.h +10 -0

+ * struct iommu_fault_param - per-device IOMMU runtime data
+ * @dev_fault_handler: Callback function to handle IOMMU faults at
+device level
+ * @pasid_tbl_bound: Device PASID table is bound to a guest
+ *
+ */
+struct iommu_fault_param {
+	iommu_dev_fault_handler_t dev_fault_handler;
+	bool pasid_tbl_bound:1;
+	bool pasid_tbl_shadowed:1;

I guess you can remove this?

Thanks,
Jean

Quoted 11 lines

include/linux/iommu.h +2 -0

+};
+
 int iommu_fwspec_init(struct device *dev, struct fwnode_handle *iommu_fwnode,
 		      const struct iommu_ops *ops);  void iommu_fwspec_free(struct

device *dev); @@ -437,6 +505,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) {

Quoted 21 lines

include/linux/iommu.h struct iommu_device { · line 282

Quoted 9 lines

include/linux/iommu.h +9 -0

+
+/*  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_CTX = 1,

If I read the VT-d spec right, this is a fault encountered while fetching the PASID table pointer?

include/linux/iommu.h struct iommu_device { · line 283

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_ACCESS,

And this a pgd or pte access fault?

include/linux/iommu.h struct iommu_device { · line 284

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_INVALIDATE,

What would this be?

Quoted 7 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+	IOMMU_FAULT_REASON_UNKNOWN,
+};

I'm currently doing the same exploratory work for virtio-iommu, and I'd be tempted to report reasons as detailed as possible to guest or device driver, but it's not clear what they need, how they would use this information. I'd like to discuss this some more.

[Liu, Yi L] In fact, it's not necessary to pass the detailed unrecoverable fault to guest in virtualization case. Unrecoverable fault happened on native indicates fault during native IOMMU address translation. If the fault is not due to guest IOMMU page table setting, then it is not necessary to inject the fault to guest. And hypervisor should be able to deduce it by walking the guest IOMMU page table with the fault address.

So I think for
virtualization case, pass the fault address is enough. If hypervisor doesn't see any issue after checking the guest IOMMU translation hierarchy, no use to let guest know it. Hypervisor can either throw error log or stop the guest. If hypervisor see any error in the guest iommu translation hierarchy, then inject the error to guest with a proper fault type.> But for device driver or other user-space driver, I'm not sure if they need detailed fault info. In fact, it is enough to pass the possible info which would help them to deduce whether the unrecoverable fault is due to them. This need more inputs from device driver reviewers.

Quoted 3 lines

For unrecoverable faults I guess CTX means "the host IOMMU driver is broken", since the device tables are invalid. In which case there is no use continuing, trying to shutdown the device cleanly is really all the guest/device driver can do.

[Liu, Yi L] Not sure about what device table mean here. But I agree that if host IOMMU driver has no valid CTX for the device, then this kind of error should result in a shutdown to the device.

Quoted 16 lines

For ACCESS the error is the device driver's or guest's fault, since the device driver triggered DMA on unmapped buffers, or the guest didn't install the right page tables. This can be repaired without shutting down, it may even just be one execution stream that failed in the device while the others continued normally. It's not as recoverable as a PRI Page Request, but the device driver may still be able to isolate the problem (e.g. by killing the process responsible) and the device to recover from it.

So maybe ACCESS would benefit from more details, for example differentiating faults encountered while fetching the pgd from those encountered while fetching a second-level table or pte. The former is a lot less recoverable than the latter (bug in the guest IOMMU driver vs.
bug in the device driver).

Generalizing this maybe we should differentiate each step of the translation in fault_reason:

Quoted 4 lines

* Device entry (context) fetch -> host IOMMU driver's fault
* PASID table fetch -> guest IOMMU driver or host userspace's fault
* pgd fetch -> guest IOMMU driver's fault
* pte fetch, including validity and access check -> device driver's fault

[Liu, Yi L] It's a good summary here. BTW. why pte fetch is due to device driver's fault?

-----Original Message-----
From: Jean-Philippe Brucker [mailto:[email protected]]
Sent: Tuesday, November 7, 2017 3:02 AM
To: Liu, Yi L <[email protected]>; Jacob Pan <[email protected]>; [email protected]; LKML <[email protected]>; Joerg Roedel <[email protected]>; David Woodhouse <[email protected]>; Greg Kroah-Hartman <[email protected]>; Wysocki, Rafael J <[email protected]>
Cc: Lan, Tianyu <[email protected]>; Tian, Kevin <[email protected]>; Raj, Ashok <[email protected]>; Alex Williamson <[email protected]>
Subject: Re: [PATCH v2 08/16] iommu: introduce device fault data

Hi Yi,

Sorry for the late reply, I seem to have missed this.

On 20/10/17 11:07, Liu, Yi L wrote:
[...]

Quoted 29 lines

Quoted 28 lines

include/linux/iommu.h struct iommu_device { · line 282

Quoted 9 lines

include/linux/iommu.h +9 -0

+
+/*  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_CTX = 1,

If I read the VT-d spec right, this is a fault encountered while fetching the PASID table pointer?

include/linux/iommu.h struct iommu_device { · line 283

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_ACCESS,

And this a pgd or pte access fault?

include/linux/iommu.h struct iommu_device { · line 284

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_INVALIDATE,

What would this be?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	IOMMU_FAULT_REASON_UNKNOWN,
+};

I'm currently doing the same exploratory work for virtio-iommu, and I'd be tempted to report reasons as detailed as possible to guest or device driver, but it's not clear what they need, how they would use this information. I'd like to discuss this some more.

Quoted 5 lines

[Liu, Yi L] In fact, it's not necessary to pass the detailed unrecoverable fault to guest in virtualization case. Unrecoverable fault happened on native indicates fault during native IOMMU address translation. If the fault is not due to guest IOMMU page table
setting, then it is not necessary to inject the fault to guest. And hypervisor should

be able to deduce it by walking the guest IOMMU page table with the fault address.

I'm not sure the hypervisor should go and inspect the guest's page tables.

The pIOMMU already did the walk and reported the fault, so the hypervisor knows that they are invalid. I thought VT-d and other pIOMMUs provide enough information in the fault report to tell if the error was due to invalid page tables?

Quoted 9 lines

So I think for
virtualization case, pass the fault address is enough. If hypervisor doesn't see any issue after checking the guest IOMMU translation hierarchy, no use to let guest know it. Hypervisor can either throw error log or stop the guest. If hypervisor see any error in the guest iommu translation hierarchy, then inject the error to guest with a proper fault type.> But for device driver or other user-space driver, I'm not sure if they need detailed fault info. In fact, it is enough to pass the

possible info which would help them to deduce whether the unrecoverable fault is due to them. This need more inputs from device driver reviewers.

Agreed, though I'm not sure how to reach them.

At the moment, the only users of report_iommu_fault, the existing fault reporting mechanism, are ARM-based IOMMU drivers and there are only four device drivers that register a handler with iommu_set_fault_handler. Two of them simply print the fault, one resets the offending device, and the last one (msm GPU) wants to provide more detailed debugging information about the device state.

Quoted 3 lines

Quoted 3 lines

For unrecoverable faults I guess CTX means "the host IOMMU driver is broken", since the device tables are invalid. In which case there is no use continuing, trying to shutdown the device cleanly is really all the

guest/device driver can do.

Quoted 4 lines

[Liu, Yi L] Not sure about what device table mean here. But I agree that if host IOMMU driver has no valid CTX for the device, then this kind of error should result in a shutdown to the device.

Yes by device table I meant VT-d's root table and context tables.

Quoted 2 lines

Quoted 2 lines

For ACCESS the error is the device driver's or guest's fault, since the device driver triggered DMA on unmapped buffers, or the guest didn't install

the right page tables.

Quoted 4 lines

Quoted 4 lines

This can be repaired without shutting down, it may even just be one execution stream that failed in the device while the others continued normally. It's not as recoverable as a PRI Page Request, but the device driver may still be able to isolate the problem (e.g. by killing the process

responsible) and the device to recover from it.

Quoted 17 lines

Quoted 16 lines

So maybe ACCESS would benefit from more details, for example differentiating faults encountered while fetching the pgd from those encountered while fetching a second-level table or pte. The former is a lot less recoverable than the latter (bug in the guest IOMMU driver vs. bug in the device driver).

Generalizing this maybe we should differentiate each step of the translation in
fault_reason:

* Device entry (context) fetch -> host IOMMU driver's fault
* PASID table fetch -> guest IOMMU driver or host userspace's fault
* pgd fetch -> guest IOMMU driver's fault
* pte fetch, including validity and access check -> device driver's fault

Quoted 1 line

[Liu, Yi L] It's a good summary here. BTW. why pte fetch is due to device driver's

fault?

Mmh, not necessarily the device driver's fault, but the most likely cause is that the device driver didn't call map() before triggering the DMA.
Another less likely cause is a programming error in the vIOMMU driver, where it failed to perform the map() or populate the page tables properly.

Quoted 29 lines

Quoted 29 lines

Quoted 28 lines

include/linux/iommu.h struct iommu_device { · line 282

Quoted 9 lines

include/linux/iommu.h +9 -0

+
+/*  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_CTX = 1,

If I read the VT-d spec right, this is a fault encountered while fetching the PASID table pointer?

include/linux/iommu.h struct iommu_device { · line 283

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_ACCESS,

And this a pgd or pte access fault?

include/linux/iommu.h struct iommu_device { · line 284

Quoted 1 line

include/linux/iommu.h +1 -0

+	IOMMU_FAULT_REASON_INVALIDATE,

What would this be?

Review

Quoted 2 lines

(quoted diff) +2 -0

+	IOMMU_FAULT_REASON_UNKNOWN,
+};

I'm currently doing the same exploratory work for virtio-iommu, and I'd be tempted to report reasons as detailed as possible to guest or device driver, but it's not clear what they need, how they would use this information. I'd like to discuss this some more.

Quoted 8 lines

Quoted 5 lines

[Liu, Yi L] In fact, it's not necessary to pass the detailed unrecoverable fault to guest in virtualization case. Unrecoverable fault happened on native indicates fault during native IOMMU address translation. If the fault is not due to guest IOMMU page table
setting, then it is not necessary to inject the fault to guest. And hypervisor should

be able to deduce it by walking the guest IOMMU page table with the fault address.

I'm not sure the hypervisor should go and inspect the guest's page tables.

[Liu, Yi L] I think hypervisor needs to do it to make sure reporting fault to guest correctly. If not, hypervisor may report some fault to guest and make guest confused. e.g. pIOMMU walks page table and failed during walking root table(VT-d) or device table(SMMU). such fault is due to no valid programming in host, guest has no duty on it and neither has knowledge to fix it. it would make guest to believe that it has programmed the root table or device table in the wrong way while the fact is not.

Quoted 3 lines

The pIOMMU already did the walk and reported the fault, so the hypervisor knows that they are invalid. I thought VT-d and other pIOMMUs provide enough information in the fault report to tell if the error was due to invalid page tables?

[Liu, Yi L] yes, pIOMMU did walk and get the fault info, but it's not sure who is responsible to the fault. With inspecting the guest table, hypervisor may know who should be responsible to the fault.

Quoted 8 lines

Quoted 8 lines

So I think for
virtualization case, pass the fault address is enough. If hypervisor doesn't see any issue after checking the guest IOMMU translation hierarchy, no use to let guest know it. Hypervisor can either throw error log or stop the guest. If hypervisor see any error in the guest iommu translation hierarchy, then inject the error to guest with a proper fault type.> But for device driver or other user-space driver, I'm not sure if they need detailed fault info. In fact, it is enough to pass the

Quoted 4 lines

possible info which would help them to deduce whether the unrecoverable fault is due to them. This need more inputs from device driver reviewers.

Agreed, though I'm not sure how to reach them.

[Liu, Yi L] I'd like to supplement my words here. Except the fault address, we may also need to provide the BDF and PASID if it is there.

Quoted 5 lines

At the moment, the only users of report_iommu_fault, the existing fault reporting mechanism, are ARM-based IOMMU drivers and there are only four device drivers that register a handler with iommu_set_fault_handler. Two of them simply print the fault, one resets the offending device, and the last one (msm GPU) wants to provide more detailed debugging information about the device state.

[Liu, Yi L] Well, it looks like device driver may not try to fix the fault, instead, it would more likely do kind of clean up after un-recoverable fault. If so, it may be enough to have a notification to device driver.

Quoted 10 lines

Quoted 3 lines

Quoted 3 lines

For unrecoverable faults I guess CTX means "the host IOMMU driver is broken", since the device tables are invalid. In which case there is no use continuing, trying to shutdown the device cleanly is really all the

guest/device driver can do.

Quoted 4 lines

[Liu, Yi L] Not sure about what device table mean here. But I agree that if host IOMMU driver has no valid CTX for the device, then this kind of error should result in a shutdown to the device.

Yes by device table I meant VT-d's root table and context tables.

[Liu, Yi L] I see.

Quoted 20 lines

Quoted 2 lines

Quoted 2 lines

For ACCESS the error is the device driver's or guest's fault, since the device driver triggered DMA on unmapped buffers, or the guest didn't install

the right page tables.

Quoted 4 lines

Quoted 4 lines

This can be repaired without shutting down, it may even just be one execution stream that failed in the device while the others continued normally. It's not as recoverable as a PRI Page Request, but the device driver may still be able to isolate the problem (e.g. by killing the process

responsible) and the device to recover from it.

Quoted 12 lines

Quoted 12 lines

So maybe ACCESS would benefit from more details, for example differentiating faults encountered while fetching the pgd from those encountered while fetching a second-level table or pte. The former is a lot less recoverable than the latter (bug in the guest IOMMU driver vs. bug in the device driver).

Generalizing this maybe we should differentiate each step of the translation in
fault_reason:

* Device entry (context) fetch -> host IOMMU driver's fault

Quoted 1 line

Quoted 1 line

Quoted 1 line

* PASID table fetch -> guest IOMMU driver or host userspace's fault

Quoted 11 lines

Quoted 5 lines

Quoted 3 lines

* pgd fetch -> guest IOMMU driver's fault
* pte fetch, including validity and access check -> device driver's fault

[Liu, Yi L] It's a good summary here. BTW. why pte fetch is due to device driver's

fault?

Mmh, not necessarily the device driver's fault, but the most likely cause is that the device driver didn't call map() before triggering the DMA.
Another less likely cause is a programming error in the vIOMMU driver, where it failed to perform the map() or populate the page tables properly.

[Liu, Yi L] Yes, I think for fault during iova(host iova or GPA) translation, the most likely reason would be no calling of map() since we are using synchronized map API.

Besides the four reasons you listed above, I think there is still other reasons like no present bit, invalid programming or so. And also, we have several tables which may be referenced in an address translation. e.g. VT-d, we have root table, CTX table, pasid table, translation page table(1st level, 2nd level). I think AMD-iommu and SMMU should have similar stuffs?

I think the IOMMU should pass the struct device associated to the BDF to the fault handler. The fault handler can then deduce the BDF from struct device if it needs to. This also allows to support faults from non-PCI devices, where the BDF or deviceID is specific to the IOMMU and doesn't mean anything to the device driver.

I agree that we should provide the PASID if present.

[...]
[...]

Yes, and reporting faulting address and PASID can help the device driver decide what to do. For example a GPU driver might share the device between multiple processes, and an unrecoverable fault might simply be that one of the process tried to DMA outside its boundaries. In that case you'd want to kill the process, not reset the device.

[...]
[...]
[...]
[...]
[...]
[...]
[...]
[...]
[...]
[...]

Actually this could also be that the device simply isn't allowed to DMA, so not necessarily the pIOMMU driver misprogramming its tables. So the host IOMMU driver should notify the device driver when encountering an invalid device entry, telling it to reset the device.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

* PASID table fetch -> guest IOMMU driver or host userspace's fault

Another reason I missed before is "invalid PASID". This means that the device driver used a PASID that wasn't reserved or that's outside of the PASID table, so is really the device drivers's fault. It should probably be distinguished from "pgd fetch error" below, which is the vIOMMU driver misprogramming the PASID table.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

* pgd fetch -> guest IOMMU driver's fault
* pte fetch, including validity and access check -> device driver's fault

[...]
[...]

Quoted 11 lines

[Liu, Yi L] Yes, I think for fault during iova(host iova or GPA) translation, the most likely reason would be no calling of map() since we are using synchronized map API.

Besides the four reasons you listed above, I think there is still other reasons like no present bit, invalid programming or so. And also, we have several tables which may be referenced in an address translation. e.g. VT-d, we have root table, CTX table, pasid table, translation page table(1st level, 2nd level). I think AMD-iommu and SMMU should have similar stuffs?