[PATCH v4 0/9] iommu/vt-d: Improve PASID id and table management

[PATCH v4 0/9] iommu/vt-d: Improve PASID id and table management

[Lu Baolu]

Hi,

This patch set improves the PASID id and PASID table management
for Intel IOMMU driver.

PATCH 1~3 replace per IOMMU idr name space with a global one.
Current per IOMMU idr doesn't work in some cases where one
application (associated with a PASID) might talk to two physical
devices simultaneously while the two devices could reside behind
two different IOMMU units.

PATCH 4~9 implement per device PASID table. Current per IOMMU
PASID table implementation is insecure in the cases where
multiple devices under one single IOMMU unit support PASID
feature. With per domain PASID table, we can achieve finer
protection and isolation granularity. This has been discussed
at https://lkml.org/lkml/2018/5/16/154.

Best regards,
Lu Baolu

Change log:
v1->v2:
  - Patches have been reviewed by "Liu Yi L <yi.l.liu@intel.com>".
  - An error case handling was added in PATCH 6/9.
  - Some commit messages are refined to be more accurate.
v2->v3:
  - Patches rebased on top of v4.18-rc1.
  - Replace per-domain with per-device pasid table.
v3->v4:
  - Patches rebased on top of Joerg's x86/vt-d branch.

Lu Baolu (9):
  iommu/vt-d: Global PASID name space
  iommu/vt-d: Avoid using idr_for_each_entry()
  iommu/vt-d: Apply global PASID in SVA
  iommu/vt-d: Move device_domain_info to header
  iommu/vt-d: Add for_each_device_domain() helper
  iommu/vt-d: Per PCI device pasid table interfaces
  iommu/vt-d: Allocate and free pasid table
  iommu/vt-d: Apply per pci device pasid table in SVA
  iommu/vt-d: Remove the obsolete per iommu pasid tables

 drivers/iommu/Makefile      |   2 +-
 drivers/iommu/intel-iommu.c | 143 ++++++++++++--------------
 drivers/iommu/intel-pasid.c | 238 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h |  39 ++++++++
 drivers/iommu/intel-svm.c   |  79 ++++++---------
 include/linux/intel-iommu.h |  72 +++++++++++++-
 6 files changed, 440 insertions(+), 133 deletions(-)
 create mode 100644 drivers/iommu/intel-pasid.c
 create mode 100644 drivers/iommu/intel-pasid.h

-- 
2.7.4

[PATCH v4 1/9] iommu/vt-d: Global PASID name space

[Lu Baolu]

This adds the system wide PASID name space for the PASID
allocation. Currently we are using per IOMMU PASID name
spaces which are not suitable for some use cases. For an
example, one application (associated with a PASID) might
talk to two physical devices simultaneously while the two
devices could reside behind two different IOMMU units.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/Makefile      |  2 +-
 drivers/iommu/intel-iommu.c | 13 ++++++++++
 drivers/iommu/intel-pasid.c | 60 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h | 21 ++++++++++++++++
 4 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 drivers/iommu/intel-pasid.c
 create mode 100644 drivers/iommu/intel-pasid.h

diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 1fb6958..0a190b4 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -14,7 +14,7 @@ obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
 obj-$(CONFIG_ARM_SMMU_V3) += arm-smmu-v3.o
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
-obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o
+obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o intel-pasid.o
 obj-$(CONFIG_INTEL_IOMMU_SVM) += intel-svm.o
 obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
 obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 869321c..dd5a617 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -52,6 +52,7 @@
 #include <asm/iommu.h>
 
 #include "irq_remapping.h"
+#include "intel-pasid.h"
 
 #define ROOT_SIZE		VTD_PAGE_SIZE
 #define CONTEXT_SIZE		VTD_PAGE_SIZE
@@ -3292,6 +3293,18 @@ static int __init init_dmars(void)
 	}
 
 	for_each_active_iommu(iommu, drhd) {
+		/*
+		 * Find the max pasid size of all IOMMU's in the system.
+		 * We need to ensure the system pasid table is no bigger
+		 * than the smallest supported.
+		 */
+		if (pasid_enabled(iommu)) {
+			u32 temp = 2 << ecap_pss(iommu->ecap);
+
+			intel_pasid_max_id = min_t(u32, temp,
+						   intel_pasid_max_id);
+		}
+
 		g_iommus[iommu->seq_id] = iommu;
 
 		intel_iommu_init_qi(iommu);
diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
new file mode 100644
index 0000000..e918fe0
--- /dev/null
+++ b/drivers/iommu/intel-pasid.c
@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * intel-pasid.c - PASID idr, table and entry manipulation
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ */
+
+#define pr_fmt(fmt)	"DMAR: " fmt
+
+#include <linux/dmar.h>
+#include <linux/intel-iommu.h>
+#include <linux/iommu.h>
+#include <linux/memory.h>
+#include <linux/spinlock.h>
+
+#include "intel-pasid.h"
+
+/*
+ * Intel IOMMU system wide PASID name space:
+ */
+static DEFINE_SPINLOCK(pasid_lock);
+u32 intel_pasid_max_id = PASID_MAX;
+static DEFINE_IDR(pasid_idr);
+
+int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp)
+{
+	int ret, min, max;
+
+	min = max_t(int, start, PASID_MIN);
+	max = min_t(int, end, intel_pasid_max_id);
+
+	WARN_ON(in_interrupt());
+	idr_preload(gfp);
+	spin_lock(&pasid_lock);
+	ret = idr_alloc(&pasid_idr, ptr, min, max, GFP_ATOMIC);
+	spin_unlock(&pasid_lock);
+	idr_preload_end();
+
+	return ret;
+}
+
+void intel_pasid_free_id(int pasid)
+{
+	spin_lock(&pasid_lock);
+	idr_remove(&pasid_idr, pasid);
+	spin_unlock(&pasid_lock);
+}
+
+void *intel_pasid_lookup_id(int pasid)
+{
+	void *p;
+
+	spin_lock(&pasid_lock);
+	p = idr_find(&pasid_idr, pasid);
+	spin_unlock(&pasid_lock);
+
+	return p;
+}
diff --git a/drivers/iommu/intel-pasid.h b/drivers/iommu/intel-pasid.h
new file mode 100644
index 0000000..d5feb3d
--- /dev/null
+++ b/drivers/iommu/intel-pasid.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * intel-pasid.h - PASID idr, table and entry header
+ *
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Author: Lu Baolu <baolu.lu@linux.intel.com>
+ */
+
+#ifndef __INTEL_PASID_H
+#define __INTEL_PASID_H
+
+#define PASID_MIN			0x1
+#define PASID_MAX			0x20000
+
+extern u32 intel_pasid_max_id;
+int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp);
+void intel_pasid_free_id(int pasid);
+void *intel_pasid_lookup_id(int pasid);
+
+#endif /* __INTEL_PASID_H */
-- 
2.7.4

[PATCH v4 2/9] iommu/vt-d: Avoid using idr_for_each_entry()

[Lu Baolu]

idr_for_each_entry() is used to iteratte over idr elements
of a given type. It isn't suitable for the globle pasid idr
since the pasid idr consumer could specify different types
of pointers to bind with a pasid.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-svm.c   | 14 ++++++++++----
 include/linux/intel-iommu.h |  1 +
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 2cc0aac..36cc1d1 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -298,6 +298,7 @@ static const struct mmu_notifier_ops intel_mmuops = {
 };
 
 static DEFINE_MUTEX(pasid_mutex);
+static LIST_HEAD(global_svm_list);
 
 int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ops *ops)
 {
@@ -329,13 +330,13 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 
 	mutex_lock(&pasid_mutex);
 	if (pasid && !(flags & SVM_FLAG_PRIVATE_PASID)) {
-		int i;
+		struct intel_svm *t;
 
-		idr_for_each_entry(&iommu->pasid_idr, svm, i) {
-			if (svm->mm != mm ||
-			    (svm->flags & SVM_FLAG_PRIVATE_PASID))
+		list_for_each_entry(t, &global_svm_list, list) {
+			if (t->mm != mm || (t->flags & SVM_FLAG_PRIVATE_PASID))
 				continue;
 
+			svm = t;
 			if (svm->pasid >= pasid_max) {
 				dev_warn(dev,
 					 "Limited PASID width. Cannot use existing PASID %d\n",
@@ -404,6 +405,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		svm->mm = mm;
 		svm->flags = flags;
 		INIT_LIST_HEAD_RCU(&svm->devs);
+		INIT_LIST_HEAD(&svm->list);
 		ret = -ENOMEM;
 		if (mm) {
 			ret = mmu_notifier_register(&svm->notifier, mm);
@@ -430,6 +432,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		 */
 		if (cap_caching_mode(iommu->cap))
 			intel_flush_pasid_dev(svm, sdev, svm->pasid);
+
+		list_add_tail(&svm->list, &global_svm_list);
 	}
 	list_add_rcu(&sdev->list, &svm->devs);
 
@@ -485,6 +489,8 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 					if (svm->mm)
 						mmu_notifier_unregister(&svm->notifier, svm->mm);
 
+					list_del(&svm->list);
+
 					/* We mandate that no page faults may be outstanding
 					 * for the PASID when intel_svm_unbind_mm() is called.
 					 * If that is not obeyed, subtle errors will happen.
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 6692b40..e1e1938 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -487,6 +487,7 @@ struct intel_svm {
 	int flags;
 	int pasid;
 	struct list_head devs;
+	struct list_head list;
 };
 
 extern int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sdev);
-- 
2.7.4

[PATCH v4 3/9] iommu/vt-d: Apply global PASID in SVA

[Lu Baolu]

This patch applies the global pasid name space in the shared
virtual address (SVA) implementation.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-svm.c   | 22 +++++++++++-----------
 include/linux/intel-iommu.h |  1 -
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 36cc1d1..56e65d0 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -26,6 +26,8 @@
 #include <linux/interrupt.h>
 #include <asm/page.h>
 
+#include "intel-pasid.h"
+
 #define PASID_ENTRY_P		BIT_ULL(0)
 #define PASID_ENTRY_FLPM_5LP	BIT_ULL(9)
 #define PASID_ENTRY_SRE		BIT_ULL(11)
@@ -85,8 +87,6 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
 				iommu->name);
 	}
 
-	idr_init(&iommu->pasid_idr);
-
 	return 0;
 }
 
@@ -102,7 +102,7 @@ int intel_svm_free_pasid_tables(struct intel_iommu *iommu)
 		free_pages((unsigned long)iommu->pasid_state_table, order);
 		iommu->pasid_state_table = NULL;
 	}
-	idr_destroy(&iommu->pasid_idr);
+
 	return 0;
 }
 
@@ -392,9 +392,9 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 			pasid_max = iommu->pasid_max;
 
 		/* Do not use PASID 0 in caching mode (virtualised IOMMU) */
-		ret = idr_alloc(&iommu->pasid_idr, svm,
-				!!cap_caching_mode(iommu->cap),
-				pasid_max - 1, GFP_KERNEL);
+		ret = intel_pasid_alloc_id(svm,
+					   !!cap_caching_mode(iommu->cap),
+					   pasid_max - 1, GFP_KERNEL);
 		if (ret < 0) {
 			kfree(svm);
 			kfree(sdev);
@@ -410,7 +410,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		if (mm) {
 			ret = mmu_notifier_register(&svm->notifier, mm);
 			if (ret) {
-				idr_remove(&svm->iommu->pasid_idr, svm->pasid);
+				intel_pasid_free_id(svm->pasid);
 				kfree(svm);
 				kfree(sdev);
 				goto out;
@@ -460,7 +460,7 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 	if (!iommu || !iommu->pasid_table)
 		goto out;
 
-	svm = idr_find(&iommu->pasid_idr, pasid);
+	svm = intel_pasid_lookup_id(pasid);
 	if (!svm)
 		goto out;
 
@@ -485,7 +485,7 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 					svm->iommu->pasid_table[svm->pasid].val = 0;
 					wmb();
 
-					idr_remove(&svm->iommu->pasid_idr, svm->pasid);
+					intel_pasid_free_id(svm->pasid);
 					if (svm->mm)
 						mmu_notifier_unregister(&svm->notifier, svm->mm);
 
@@ -520,7 +520,7 @@ int intel_svm_is_pasid_valid(struct device *dev, int pasid)
 	if (!iommu || !iommu->pasid_table)
 		goto out;
 
-	svm = idr_find(&iommu->pasid_idr, pasid);
+	svm = intel_pasid_lookup_id(pasid);
 	if (!svm)
 		goto out;
 
@@ -618,7 +618,7 @@ static irqreturn_t prq_event_thread(int irq, void *d)
 
 		if (!svm || svm->pasid != req->pasid) {
 			rcu_read_lock();
-			svm = idr_find(&iommu->pasid_idr, req->pasid);
+			svm = intel_pasid_lookup_id(req->pasid);
 			/* It *can't* go away, because the driver is not permitted
 			 * to unbind the mm while any page faults are outstanding.
 			 * So we only need RCU to protect the internal idr code. */
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e1e1938..2ff1519 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -420,7 +420,6 @@ struct intel_iommu {
 	struct pasid_state_entry *pasid_state_table;
 	struct page_req_dsc *prq;
 	unsigned char prq_name[16];    /* Name for PRQ interrupt */
-	struct idr pasid_idr;
 	u32 pasid_max;
 #endif
 	struct q_inval  *qi;            /* Queued invalidation info */
-- 
2.7.4

[PATCH v4 4/9] iommu/vt-d: Move device_domain_info to header

[Lu Baolu]

This allows the per device iommu data and some helpers to be
used in other files.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c | 63 +++------------------------------------------
 include/linux/intel-iommu.h | 61 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 59 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index dd5a617..d7d2ff9 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -380,61 +380,6 @@ static int hw_pass_through = 1;
 	for (idx = 0; idx < g_num_of_iommus; idx++)		\
 		if (domain->iommu_refcnt[idx])
 
-struct dmar_domain {
-	int	nid;			/* node id */
-
-	unsigned	iommu_refcnt[DMAR_UNITS_SUPPORTED];
-					/* Refcount of devices per iommu */
-
-
-	u16		iommu_did[DMAR_UNITS_SUPPORTED];
-					/* Domain ids per IOMMU. Use u16 since
-					 * domain ids are 16 bit wide according
-					 * to VT-d spec, section 9.3 */
-
-	bool has_iotlb_device;
-	struct list_head devices;	/* all devices' list */
-	struct iova_domain iovad;	/* iova's that belong to this domain */
-
-	struct dma_pte	*pgd;		/* virtual address */
-	int		gaw;		/* max guest address width */
-
-	/* adjusted guest address width, 0 is level 2 30-bit */
-	int		agaw;
-
-	int		flags;		/* flags to find out type of domain */
-
-	int		iommu_coherency;/* indicate coherency of iommu access */
-	int		iommu_snooping; /* indicate snooping control feature*/
-	int		iommu_count;	/* reference count of iommu */
-	int		iommu_superpage;/* Level of superpages supported:
-					   0 == 4KiB (no superpages), 1 == 2MiB,
-					   2 == 1GiB, 3 == 512GiB, 4 == 1TiB */
-	u64		max_addr;	/* maximum mapped address */
-
-	struct iommu_domain domain;	/* generic domain data structure for
-					   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 */
-	u16 pfsid;		/* SRIOV physical function source ID */
-	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		*/
@@ -603,7 +548,7 @@ static void set_iommu_domain(struct intel_iommu *iommu, u16 did,
 		domains[did & 0xff] = domain;
 }
 
-static inline void *alloc_pgtable_page(int node)
+void *alloc_pgtable_page(int node)
 {
 	struct page *page;
 	void *vaddr = NULL;
@@ -614,7 +559,7 @@ static inline void *alloc_pgtable_page(int node)
 	return vaddr;
 }
 
-static inline void free_pgtable_page(void *vaddr)
+void free_pgtable_page(void *vaddr)
 {
 	free_page((unsigned long)vaddr);
 }
@@ -697,7 +642,7 @@ int iommu_calculate_agaw(struct intel_iommu *iommu)
 }
 
 /* This functionin only returns single iommu in a domain */
-static struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
+struct intel_iommu *domain_get_iommu(struct dmar_domain *domain)
 {
 	int iommu_id;
 
@@ -3527,7 +3472,7 @@ static unsigned long intel_alloc_iova(struct device *dev,
 	return iova_pfn;
 }
 
-static struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
+struct dmar_domain *get_valid_domain_for_dev(struct device *dev)
 {
 	struct dmar_domain *domain, *tmp;
 	struct dmar_rmrr_unit *rmrr;
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2ff1519..2e1fbde 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -31,6 +31,7 @@
 #include <linux/list.h>
 #include <linux/iommu.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/dmar.h>
 
 #include <asm/cacheflush.h>
 #include <asm/iommu.h>
@@ -387,6 +388,42 @@ struct pasid_entry;
 struct pasid_state_entry;
 struct page_req_dsc;
 
+struct dmar_domain {
+	int	nid;			/* node id */
+
+	unsigned	iommu_refcnt[DMAR_UNITS_SUPPORTED];
+					/* Refcount of devices per iommu */
+
+
+	u16		iommu_did[DMAR_UNITS_SUPPORTED];
+					/* Domain ids per IOMMU. Use u16 since
+					 * domain ids are 16 bit wide according
+					 * to VT-d spec, section 9.3 */
+
+	bool has_iotlb_device;
+	struct list_head devices;	/* all devices' list */
+	struct iova_domain iovad;	/* iova's that belong to this domain */
+
+	struct dma_pte	*pgd;		/* virtual address */
+	int		gaw;		/* max guest address width */
+
+	/* adjusted guest address width, 0 is level 2 30-bit */
+	int		agaw;
+
+	int		flags;		/* flags to find out type of domain */
+
+	int		iommu_coherency;/* indicate coherency of iommu access */
+	int		iommu_snooping; /* indicate snooping control feature*/
+	int		iommu_count;	/* reference count of iommu */
+	int		iommu_superpage;/* Level of superpages supported:
+					   0 == 4KiB (no superpages), 1 == 2MiB,
+					   2 == 1GiB, 3 == 512GiB, 4 == 1TiB */
+	u64		max_addr;	/* maximum mapped address */
+
+	struct iommu_domain domain;	/* generic domain data structure for
+					   iommu core */
+};
+
 struct intel_iommu {
 	void __iomem	*reg; /* Pointer to hardware regs, virtual addr */
 	u64 		reg_phys; /* physical address of hw register set */
@@ -435,6 +472,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 */
+	u16 pfsid;		/* SRIOV physical function source ID */
+	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 */
+};
+
 static inline void __iommu_flush_cache(
 	struct intel_iommu *iommu, void *addr, int size)
 {
@@ -460,6 +516,11 @@ extern int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu);
 
 extern int dmar_ir_support(void);
 
+struct dmar_domain *get_valid_domain_for_dev(struct device *dev);
+void *alloc_pgtable_page(int node);
+void free_pgtable_page(void *vaddr);
+struct intel_iommu *domain_get_iommu(struct dmar_domain *domain);
+
 #ifdef CONFIG_INTEL_IOMMU_SVM
 extern int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu);
 extern int intel_svm_free_pasid_tables(struct intel_iommu *iommu);
-- 
2.7.4

[PATCH v4 5/9] iommu/vt-d: Add for_each_device_domain() helper

[Lu Baolu]

This adds a helper named for_each_device_domain() to iterate
over the elements in device_domain_list and invoke a callback
against each element. This allows to search the device_domain
list in other source files.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c | 21 +++++++++++++++++++++
 include/linux/intel-iommu.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index d7d2ff9..3d802c5 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -447,6 +447,27 @@ EXPORT_SYMBOL_GPL(intel_iommu_gfx_mapped);
 static DEFINE_SPINLOCK(device_domain_lock);
 static LIST_HEAD(device_domain_list);
 
+/*
+ * Iterate over elements in device_domain_list and call the specified
+ * callback @fn against each element. This helper should only be used
+ * in the context where the device_domain_lock has already been holden.
+ */
+int for_each_device_domain(int (*fn)(struct device_domain_info *info,
+				     void *data), void *data)
+{
+	int ret = 0;
+	struct device_domain_info *info;
+
+	assert_spin_locked(&device_domain_lock);
+	list_for_each_entry(info, &device_domain_list, global) {
+		ret = fn(info, data);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 const struct iommu_ops intel_iommu_ops;
 
 static bool translation_pre_enabled(struct intel_iommu *iommu)
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 2e1fbde..4fd4c6f 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -520,6 +520,8 @@ struct dmar_domain *get_valid_domain_for_dev(struct device *dev);
 void *alloc_pgtable_page(int node);
 void free_pgtable_page(void *vaddr);
 struct intel_iommu *domain_get_iommu(struct dmar_domain *domain);
+int for_each_device_domain(int (*fn)(struct device_domain_info *info,
+				     void *data), void *data);
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 extern int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu);
-- 
2.7.4

[PATCH v4 6/9] iommu/vt-d: Per PCI device pasid table interfaces

[Lu Baolu]

This patch adds the interfaces for per PCI device pasid
table management. Currently we allocate one pasid table
for all PCI devices under the scope of an IOMMU. It's
insecure in some cases where multiple devices under one
single IOMMU unit support PASID features. With per PCI
device pasid table, we can achieve finer protection and
isolation granularity.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Suggested-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c |   1 +
 drivers/iommu/intel-pasid.c | 178 ++++++++++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h |  18 +++++
 drivers/iommu/intel-svm.c   |   4 -
 include/linux/intel-iommu.h |   2 +
 5 files changed, 199 insertions(+), 4 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3d802c5..a930918 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2450,6 +2450,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	info->dev = dev;
 	info->domain = domain;
 	info->iommu = iommu;
+	info->pasid_table = NULL;
 
 	if (dev && dev_is_pci(dev)) {
 		struct pci_dev *pdev = to_pci_dev(info->dev);
diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
index e918fe0..1b61942 100644
--- a/drivers/iommu/intel-pasid.c
+++ b/drivers/iommu/intel-pasid.c
@@ -13,6 +13,8 @@
 #include <linux/intel-iommu.h>
 #include <linux/iommu.h>
 #include <linux/memory.h>
+#include <linux/pci.h>
+#include <linux/pci-ats.h>
 #include <linux/spinlock.h>
 
 #include "intel-pasid.h"
@@ -58,3 +60,179 @@ void *intel_pasid_lookup_id(int pasid)
 
 	return p;
 }
+
+/*
+ * Per device pasid table management:
+ */
+static inline void
+device_attach_pasid_table(struct device_domain_info *info,
+			  struct pasid_table *pasid_table)
+{
+	info->pasid_table = pasid_table;
+	list_add(&info->table, &pasid_table->dev);
+}
+
+static inline void
+device_detach_pasid_table(struct device_domain_info *info,
+			  struct pasid_table *pasid_table)
+{
+	info->pasid_table = NULL;
+	list_del(&info->table);
+}
+
+struct pasid_table_opaque {
+	struct pasid_table	**pasid_table;
+	int			segment;
+	int			bus;
+	int			devfn;
+};
+
+static int search_pasid_table(struct device_domain_info *info, void *opaque)
+{
+	struct pasid_table_opaque *data = opaque;
+
+	if (info->iommu->segment == data->segment &&
+	    info->bus == data->bus &&
+	    info->devfn == data->devfn &&
+	    info->pasid_table) {
+		*data->pasid_table = info->pasid_table;
+		return 1;
+	}
+
+	return 0;
+}
+
+static int get_alias_pasid_table(struct pci_dev *pdev, u16 alias, void *opaque)
+{
+	struct pasid_table_opaque *data = opaque;
+
+	data->segment = pci_domain_nr(pdev->bus);
+	data->bus = PCI_BUS_NUM(alias);
+	data->devfn = alias & 0xff;
+
+	return for_each_device_domain(&search_pasid_table, data);
+}
+
+int intel_pasid_alloc_table(struct device *dev)
+{
+	struct device_domain_info *info;
+	struct pasid_table *pasid_table;
+	struct pasid_table_opaque data;
+	struct page *pages;
+	size_t size, count;
+	int ret, order;
+
+	info = dev->archdata.iommu;
+	if (WARN_ON(!info || !dev_is_pci(dev) ||
+		    !info->pasid_supported || info->pasid_table))
+		return -EINVAL;
+
+	/* DMA alias device already has a pasid table, use it: */
+	data.pasid_table = &pasid_table;
+	ret = pci_for_each_dma_alias(to_pci_dev(dev),
+				     &get_alias_pasid_table, &data);
+	if (ret)
+		goto attach_out;
+
+	pasid_table = kzalloc(sizeof(*pasid_table), GFP_ATOMIC);
+	if (!pasid_table)
+		return -ENOMEM;
+	INIT_LIST_HEAD(&pasid_table->dev);
+
+	size = sizeof(struct pasid_entry);
+	count = min_t(int, pci_max_pasids(to_pci_dev(dev)), intel_pasid_max_id);
+	order = get_order(size * count);
+	pages = alloc_pages_node(info->iommu->node,
+				 GFP_ATOMIC | __GFP_ZERO,
+				 order);
+	if (!pages)
+		return -ENOMEM;
+
+	pasid_table->table = page_address(pages);
+	pasid_table->order = order;
+	pasid_table->max_pasid = count;
+
+attach_out:
+	device_attach_pasid_table(info, pasid_table);
+
+	return 0;
+}
+
+void intel_pasid_free_table(struct device *dev)
+{
+	struct device_domain_info *info;
+	struct pasid_table *pasid_table;
+
+	info = dev->archdata.iommu;
+	if (!info || !dev_is_pci(dev) ||
+	    !info->pasid_supported || !info->pasid_table)
+		return;
+
+	pasid_table = info->pasid_table;
+	device_detach_pasid_table(info, pasid_table);
+
+	if (!list_empty(&pasid_table->dev))
+		return;
+
+	free_pages((unsigned long)pasid_table->table, pasid_table->order);
+	kfree(pasid_table);
+}
+
+struct pasid_table *intel_pasid_get_table(struct device *dev)
+{
+	struct device_domain_info *info;
+
+	info = dev->archdata.iommu;
+	if (!info)
+		return NULL;
+
+	return info->pasid_table;
+}
+
+int intel_pasid_get_dev_max_id(struct device *dev)
+{
+	struct device_domain_info *info;
+
+	info = dev->archdata.iommu;
+	if (!info || !info->pasid_table)
+		return 0;
+
+	return info->pasid_table->max_pasid;
+}
+
+struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid)
+{
+	struct pasid_table *pasid_table;
+	struct pasid_entry *entries;
+
+	pasid_table = intel_pasid_get_table(dev);
+	if (WARN_ON(!pasid_table || pasid < 0 ||
+		    pasid >= intel_pasid_get_dev_max_id(dev)))
+		return NULL;
+
+	entries = pasid_table->table;
+
+	return &entries[pasid];
+}
+
+/*
+ * Interfaces for PASID table entry manipulation:
+ */
+static void sm_pasid_clear_entry(struct pasid_entry *pe)
+{
+	memset(pe, 0, sizeof(struct pasid_entry));
+}
+
+void intel_pasid_clear_entry(struct device *dev, int pasid)
+{
+	struct pasid_entry *pe;
+
+	pe = intel_pasid_get_entry(dev, pasid);
+	if (WARN_ON(!pe))
+		return;
+
+	sm_pasid_clear_entry(pe);
+
+	/* Make sure the entry update is visible before translation. */
+	wmb();
+}
diff --git a/drivers/iommu/intel-pasid.h b/drivers/iommu/intel-pasid.h
index d5feb3d..1fb5e12 100644
--- a/drivers/iommu/intel-pasid.h
+++ b/drivers/iommu/intel-pasid.h
@@ -13,9 +13,27 @@
 #define PASID_MIN			0x1
 #define PASID_MAX			0x20000
 
+struct pasid_entry {
+	u64 val;
+};
+
+/* The representative of a PASID table */
+struct pasid_table {
+	void			*table;		/* pasid table pointer */
+	int			order;		/* page order of pasid table */
+	int			max_pasid;	/* max pasid */
+	struct list_head	dev;		/* device list */
+};
+
 extern u32 intel_pasid_max_id;
 int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp);
 void intel_pasid_free_id(int pasid);
 void *intel_pasid_lookup_id(int pasid);
+int intel_pasid_alloc_table(struct device *dev);
+void intel_pasid_free_table(struct device *dev);
+struct pasid_table *intel_pasid_get_table(struct device *dev);
+int intel_pasid_get_dev_max_id(struct device *dev);
+struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid);
+void intel_pasid_clear_entry(struct device *dev, int pasid);
 
 #endif /* __INTEL_PASID_H */
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 56e65d0..9b5dc72 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -34,10 +34,6 @@
 
 static irqreturn_t prq_event_thread(int irq, void *d);
 
-struct pasid_entry {
-	u64 val;
-};
-
 struct pasid_state_entry {
 	u64 val;
 };
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index 4fd4c6f..e7901d4 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -476,6 +476,7 @@ struct intel_iommu {
 struct device_domain_info {
 	struct list_head link;	/* link to domain siblings */
 	struct list_head global; /* link to global list */
+	struct list_head table;	/* link to pasid table */
 	u8 bus;			/* PCI bus number */
 	u8 devfn;		/* PCI devfn number */
 	u16 pfsid;		/* SRIOV physical function source ID */
@@ -489,6 +490,7 @@ struct device_domain_info {
 	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 pasid_table *pasid_table; /* pasid table */
 };
 
 static inline void __iommu_flush_cache(
-- 
2.7.4

[PATCH v4 7/9] iommu/vt-d: Allocate and free pasid table

[Lu Baolu]

This patch allocates a PASID table for a PCI device at the time
when the dmar dev_info is attached to dev->archdata.iommu, and
free it in the opposite case.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a930918..baad27a 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -2508,6 +2508,15 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	list_add(&info->global, &device_domain_list);
 	if (dev)
 		dev->archdata.iommu = info;
+
+	if (dev && dev_is_pci(dev) && info->pasid_supported) {
+		ret = intel_pasid_alloc_table(dev);
+		if (ret) {
+			__dmar_remove_one_dev_info(info);
+			spin_unlock_irqrestore(&device_domain_lock, flags);
+			return NULL;
+		}
+	}
 	spin_unlock_irqrestore(&device_domain_lock, flags);
 
 	if (dev && domain_context_mapping(domain, dev)) {
@@ -4873,6 +4882,7 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	if (info->dev) {
 		iommu_disable_dev_iotlb(info);
 		domain_context_clear(iommu, info->dev);
+		intel_pasid_free_table(info->dev);
 	}
 
 	unlink_domain_info(info);
-- 
2.7.4

[PATCH v4 8/9] iommu/vt-d: Apply per pci device pasid table in SVA

[Lu Baolu]

This patch applies the per pci device pasid table in the Shared
Virtual Address (SVA) implementation.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c | 29 +++++++++--------------------
 drivers/iommu/intel-svm.c   | 22 ++++++++++------------
 2 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index baad27a..f8609b5 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -5208,22 +5208,16 @@ static void intel_iommu_put_resv_regions(struct device *dev,
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 #define MAX_NR_PASID_BITS (20)
-static inline unsigned long intel_iommu_get_pts(struct intel_iommu *iommu)
+static inline unsigned long intel_iommu_get_pts(struct device *dev)
 {
-	/*
-	 * Convert ecap_pss to extend context entry pts encoding, also
-	 * respect the soft pasid_max value set by the iommu.
-	 * - number of PASID bits = ecap_pss + 1
-	 * - number of PASID table entries = 2^(pts + 5)
-	 * Therefore, pts = ecap_pss - 4
-	 * e.g. KBL ecap_pss = 0x13, PASID has 20 bits, pts = 15
-	 */
-	if (ecap_pss(iommu->ecap) < 5)
+	int pts, max_pasid;
+
+	max_pasid = intel_pasid_get_dev_max_id(dev);
+	pts = find_first_bit((unsigned long *)&max_pasid, MAX_NR_PASID_BITS);
+	if (pts < 5)
 		return 0;
 
-	/* pasid_max is encoded as actual number of entries not the bits */
-	return find_first_bit((unsigned long *)&iommu->pasid_max,
-			MAX_NR_PASID_BITS) - 5;
+	return pts - 5;
 }
 
 int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sdev)
@@ -5259,8 +5253,8 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sd
 	if (!(ctx_lo & CONTEXT_PASIDE)) {
 		if (iommu->pasid_state_table)
 			context[1].hi = (u64)virt_to_phys(iommu->pasid_state_table);
-		context[1].lo = (u64)virt_to_phys(iommu->pasid_table) |
-			intel_iommu_get_pts(iommu);
+		context[1].lo = (u64)virt_to_phys(info->pasid_table->table) |
+			intel_iommu_get_pts(sdev->dev);
 
 		wmb();
 		/* CONTEXT_TT_MULTI_LEVEL and CONTEXT_TT_DEV_IOTLB are both
@@ -5327,11 +5321,6 @@ struct intel_iommu *intel_svm_device_to_iommu(struct device *dev)
 		return NULL;
 	}
 
-	if (!iommu->pasid_table) {
-		dev_err(dev, "PASID not enabled on IOMMU; cannot enable SVM\n");
-		return NULL;
-	}
-
 	return iommu;
 }
 #endif /* CONFIG_INTEL_IOMMU_SVM */
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index 9b5dc72..a253cde 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -274,11 +274,9 @@ static void intel_mm_release(struct mmu_notifier *mn, struct mm_struct *mm)
 	 * page) so that we end up taking a fault that the hardware really
 	 * *has* to handle gracefully without affecting other processes.
 	 */
-	svm->iommu->pasid_table[svm->pasid].val = 0;
-	wmb();
-
 	rcu_read_lock();
 	list_for_each_entry_rcu(sdev, &svm->devs, list) {
+		intel_pasid_clear_entry(sdev->dev, svm->pasid);
 		intel_flush_pasid_dev(svm, sdev, svm->pasid);
 		intel_flush_svm_range_dev(svm, sdev, 0, -1, 0, !svm->mm);
 	}
@@ -299,6 +297,7 @@ static LIST_HEAD(global_svm_list);
 int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_ops *ops)
 {
 	struct intel_iommu *iommu = intel_svm_device_to_iommu(dev);
+	struct pasid_entry *entry;
 	struct intel_svm_dev *sdev;
 	struct intel_svm *svm = NULL;
 	struct mm_struct *mm = NULL;
@@ -306,7 +305,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 	int pasid_max;
 	int ret;
 
-	if (!iommu || !iommu->pasid_table)
+	if (!iommu)
 		return -EINVAL;
 
 	if (dev_is_pci(dev)) {
@@ -384,8 +383,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		}
 		svm->iommu = iommu;
 
-		if (pasid_max > iommu->pasid_max)
-			pasid_max = iommu->pasid_max;
+		if (pasid_max > intel_pasid_max_id)
+			pasid_max = intel_pasid_max_id;
 
 		/* Do not use PASID 0 in caching mode (virtualised IOMMU) */
 		ret = intel_pasid_alloc_id(svm,
@@ -418,7 +417,8 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 		if (cpu_feature_enabled(X86_FEATURE_LA57))
 			pasid_entry_val |= PASID_ENTRY_FLPM_5LP;
 
-		iommu->pasid_table[svm->pasid].val = pasid_entry_val;
+		entry = intel_pasid_get_entry(dev, svm->pasid);
+		entry->val = pasid_entry_val;
 
 		wmb();
 
@@ -453,7 +453,7 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 
 	mutex_lock(&pasid_mutex);
 	iommu = intel_svm_device_to_iommu(dev);
-	if (!iommu || !iommu->pasid_table)
+	if (!iommu)
 		goto out;
 
 	svm = intel_pasid_lookup_id(pasid);
@@ -476,11 +476,9 @@ int intel_svm_unbind_mm(struct device *dev, int pasid)
 				intel_flush_pasid_dev(svm, sdev, svm->pasid);
 				intel_flush_svm_range_dev(svm, sdev, 0, -1, 0, !svm->mm);
 				kfree_rcu(sdev, rcu);
+				intel_pasid_clear_entry(dev, svm->pasid);
 
 				if (list_empty(&svm->devs)) {
-					svm->iommu->pasid_table[svm->pasid].val = 0;
-					wmb();
-
 					intel_pasid_free_id(svm->pasid);
 					if (svm->mm)
 						mmu_notifier_unregister(&svm->notifier, svm->mm);
@@ -513,7 +511,7 @@ int intel_svm_is_pasid_valid(struct device *dev, int pasid)
 
 	mutex_lock(&pasid_mutex);
 	iommu = intel_svm_device_to_iommu(dev);
-	if (!iommu || !iommu->pasid_table)
+	if (!iommu)
 		goto out;
 
 	svm = intel_pasid_lookup_id(pasid);
-- 
2.7.4

[PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Lu Baolu]

The obsolete per iommu pasid tables are no longer used. Hence,
clean up them.

Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Liu Yi L <yi.l.liu@intel.com>
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
---
 drivers/iommu/intel-iommu.c |  6 +++---
 drivers/iommu/intel-svm.c   | 17 ++---------------
 include/linux/intel-iommu.h |  5 ++---
 3 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index f8609b5..9a88081 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1755,7 +1755,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
 	if (pasid_enabled(iommu)) {
 		if (ecap_prs(iommu->ecap))
 			intel_svm_finish_prq(iommu);
-		intel_svm_free_pasid_tables(iommu);
+		intel_svm_exit(iommu);
 	}
 #endif
 }
@@ -3336,7 +3336,7 @@ static int __init init_dmars(void)
 			hw_pass_through = 0;
 #ifdef CONFIG_INTEL_IOMMU_SVM
 		if (pasid_enabled(iommu))
-			intel_svm_alloc_pasid_tables(iommu);
+			intel_svm_init(iommu);
 #endif
 	}
 
@@ -4330,7 +4330,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 	if (pasid_enabled(iommu))
-		intel_svm_alloc_pasid_tables(iommu);
+		intel_svm_init(iommu);
 #endif
 
 	if (dmaru->ignored) {
diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
index a253cde..eb30836 100644
--- a/drivers/iommu/intel-svm.c
+++ b/drivers/iommu/intel-svm.c
@@ -38,7 +38,7 @@ struct pasid_state_entry {
 	u64 val;
 };
 
-int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
+int intel_svm_init(struct intel_iommu *iommu)
 {
 	struct page *pages;
 	int order;
@@ -63,15 +63,6 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
 		iommu->pasid_max = 0x20000;
 
 	order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
-	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
-	if (!pages) {
-		pr_warn("IOMMU: %s: Failed to allocate PASID table\n",
-			iommu->name);
-		return -ENOMEM;
-	}
-	iommu->pasid_table = page_address(pages);
-	pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order);
-
 	if (ecap_dis(iommu->ecap)) {
 		/* Just making it explicit... */
 		BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry));
@@ -86,14 +77,10 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
 	return 0;
 }
 
-int intel_svm_free_pasid_tables(struct intel_iommu *iommu)
+int intel_svm_exit(struct intel_iommu *iommu)
 {
 	int order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
 
-	if (iommu->pasid_table) {
-		free_pages((unsigned long)iommu->pasid_table, order);
-		iommu->pasid_table = NULL;
-	}
 	if (iommu->pasid_state_table) {
 		free_pages((unsigned long)iommu->pasid_state_table, order);
 		iommu->pasid_state_table = NULL;
diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
index e7901d4..3c43882 100644
--- a/include/linux/intel-iommu.h
+++ b/include/linux/intel-iommu.h
@@ -453,7 +453,6 @@ struct intel_iommu {
 	 * devices away to userspace processes (e.g. for DPDK) and don't
 	 * want to trust that userspace will use *only* the PASID it was
 	 * told to. But while it's all driver-arbitrated, we're fine. */
-	struct pasid_entry *pasid_table;
 	struct pasid_state_entry *pasid_state_table;
 	struct page_req_dsc *prq;
 	unsigned char prq_name[16];    /* Name for PRQ interrupt */
@@ -526,8 +525,8 @@ int for_each_device_domain(int (*fn)(struct device_domain_info *info,
 				     void *data), void *data);
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
-extern int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu);
-extern int intel_svm_free_pasid_tables(struct intel_iommu *iommu);
+int intel_svm_init(struct intel_iommu *iommu);
+int intel_svm_exit(struct intel_iommu *iommu);
 extern int intel_svm_enable_prq(struct intel_iommu *iommu);
 extern int intel_svm_finish_prq(struct intel_iommu *iommu);
 
-- 
2.7.4

Re: [PATCH v4 6/9] iommu/vt-d: Per PCI device pasid table interfaces

[Peter Xu]

On Mon, Jul 09, 2018 at 01:22:55PM +0800, Lu Baolu wrote:
> This patch adds the interfaces for per PCI device pasid
> table management. Currently we allocate one pasid table
> for all PCI devices under the scope of an IOMMU. It's
> insecure in some cases where multiple devices under one
> single IOMMU unit support PASID features. With per PCI
> device pasid table, we can achieve finer protection and
> isolation granularity.
> 
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Suggested-by: Ashok Raj <ashok.raj@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel-iommu.c |   1 +
>  drivers/iommu/intel-pasid.c | 178 ++++++++++++++++++++++++++++++++++++++++++++
>  drivers/iommu/intel-pasid.h |  18 +++++
>  drivers/iommu/intel-svm.c   |   4 -
>  include/linux/intel-iommu.h |   2 +
>  5 files changed, 199 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 3d802c5..a930918 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -2450,6 +2450,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>  	info->dev = dev;
>  	info->domain = domain;
>  	info->iommu = iommu;
> +	info->pasid_table = NULL;
>  
>  	if (dev && dev_is_pci(dev)) {
>  		struct pci_dev *pdev = to_pci_dev(info->dev);
> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
> index e918fe0..1b61942 100644
> --- a/drivers/iommu/intel-pasid.c
> +++ b/drivers/iommu/intel-pasid.c
> @@ -13,6 +13,8 @@
>  #include <linux/intel-iommu.h>
>  #include <linux/iommu.h>
>  #include <linux/memory.h>
> +#include <linux/pci.h>
> +#include <linux/pci-ats.h>
>  #include <linux/spinlock.h>
>  
>  #include "intel-pasid.h"
> @@ -58,3 +60,179 @@ void *intel_pasid_lookup_id(int pasid)
>  
>  	return p;
>  }
> +
> +/*
> + * Per device pasid table management:
> + */
> +static inline void
> +device_attach_pasid_table(struct device_domain_info *info,
> +			  struct pasid_table *pasid_table)
> +{
> +	info->pasid_table = pasid_table;
> +	list_add(&info->table, &pasid_table->dev);
> +}
> +
> +static inline void
> +device_detach_pasid_table(struct device_domain_info *info,
> +			  struct pasid_table *pasid_table)
> +{
> +	info->pasid_table = NULL;
> +	list_del(&info->table);
> +}
> +
> +struct pasid_table_opaque {
> +	struct pasid_table	**pasid_table;
> +	int			segment;
> +	int			bus;
> +	int			devfn;
> +};
> +
> +static int search_pasid_table(struct device_domain_info *info, void *opaque)
> +{
> +	struct pasid_table_opaque *data = opaque;
> +
> +	if (info->iommu->segment == data->segment &&
> +	    info->bus == data->bus &&
> +	    info->devfn == data->devfn &&
> +	    info->pasid_table) {
> +		*data->pasid_table = info->pasid_table;
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int get_alias_pasid_table(struct pci_dev *pdev, u16 alias, void *opaque)
> +{
> +	struct pasid_table_opaque *data = opaque;
> +
> +	data->segment = pci_domain_nr(pdev->bus);
> +	data->bus = PCI_BUS_NUM(alias);
> +	data->devfn = alias & 0xff;
> +
> +	return for_each_device_domain(&search_pasid_table, data);
> +}
> +
> +int intel_pasid_alloc_table(struct device *dev)
> +{
> +	struct device_domain_info *info;
> +	struct pasid_table *pasid_table;
> +	struct pasid_table_opaque data;
> +	struct page *pages;
> +	size_t size, count;
> +	int ret, order;
> +
> +	info = dev->archdata.iommu;
> +	if (WARN_ON(!info || !dev_is_pci(dev) ||
> +		    !info->pasid_supported || info->pasid_table))
> +		return -EINVAL;
> +
> +	/* DMA alias device already has a pasid table, use it: */
> +	data.pasid_table = &pasid_table;
> +	ret = pci_for_each_dma_alias(to_pci_dev(dev),
> +				     &get_alias_pasid_table, &data);
> +	if (ret)
> +		goto attach_out;
> +
> +	pasid_table = kzalloc(sizeof(*pasid_table), GFP_ATOMIC);

Do we need to take some lock here (e.g., the pasid lock)?  Otherwise
what if two devices (that are sharing the same DMA alias) call the
function intel_pasid_alloc_table() concurrently, then could it
possible that we create one table for each of the device while AFAIU
we should let them share a single pasid table?

> +	if (!pasid_table)
> +		return -ENOMEM;
> +	INIT_LIST_HEAD(&pasid_table->dev);
> +
> +	size = sizeof(struct pasid_entry);
> +	count = min_t(int, pci_max_pasids(to_pci_dev(dev)), intel_pasid_max_id);
> +	order = get_order(size * count);
> +	pages = alloc_pages_node(info->iommu->node,
> +				 GFP_ATOMIC | __GFP_ZERO,
> +				 order);
> +	if (!pages)
> +		return -ENOMEM;
> +
> +	pasid_table->table = page_address(pages);
> +	pasid_table->order = order;
> +	pasid_table->max_pasid = count;
> +
> +attach_out:
> +	device_attach_pasid_table(info, pasid_table);
> +
> +	return 0;
> +}
> +
> +void intel_pasid_free_table(struct device *dev)
> +{
> +	struct device_domain_info *info;
> +	struct pasid_table *pasid_table;
> +
> +	info = dev->archdata.iommu;
> +	if (!info || !dev_is_pci(dev) ||
> +	    !info->pasid_supported || !info->pasid_table)
> +		return;
> +
> +	pasid_table = info->pasid_table;
> +	device_detach_pasid_table(info, pasid_table);
> +
> +	if (!list_empty(&pasid_table->dev))
> +		return;

Same question to here: do we need a lock?

> +
> +	free_pages((unsigned long)pasid_table->table, pasid_table->order);
> +	kfree(pasid_table);
> +}
> +
> +struct pasid_table *intel_pasid_get_table(struct device *dev)
> +{
> +	struct device_domain_info *info;
> +
> +	info = dev->archdata.iommu;
> +	if (!info)
> +		return NULL;
> +
> +	return info->pasid_table;
> +}
> +
> +int intel_pasid_get_dev_max_id(struct device *dev)
> +{
> +	struct device_domain_info *info;
> +
> +	info = dev->archdata.iommu;
> +	if (!info || !info->pasid_table)
> +		return 0;
> +
> +	return info->pasid_table->max_pasid;
> +}
> +
> +struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid)
> +{
> +	struct pasid_table *pasid_table;
> +	struct pasid_entry *entries;
> +
> +	pasid_table = intel_pasid_get_table(dev);
> +	if (WARN_ON(!pasid_table || pasid < 0 ||
> +		    pasid >= intel_pasid_get_dev_max_id(dev)))
> +		return NULL;
> +
> +	entries = pasid_table->table;
> +
> +	return &entries[pasid];
> +}
> +
> +/*
> + * Interfaces for PASID table entry manipulation:
> + */
> +static void sm_pasid_clear_entry(struct pasid_entry *pe)
> +{
> +	memset(pe, 0, sizeof(struct pasid_entry));

[1]

> +}
> +
> +void intel_pasid_clear_entry(struct device *dev, int pasid)
> +{
> +	struct pasid_entry *pe;
> +
> +	pe = intel_pasid_get_entry(dev, pasid);
> +	if (WARN_ON(!pe))
> +		return;
> +
> +	sm_pasid_clear_entry(pe);
> +
> +	/* Make sure the entry update is visible before translation. */
> +	wmb();

Pure question: AFAIU wmb only orders write operation, then do we
really need it here?  Instead from the comment I feel like what we
really need is a WRITE_ONCE() at [1].  Am I wrong somewhere?

Thanks,

> +}
> diff --git a/drivers/iommu/intel-pasid.h b/drivers/iommu/intel-pasid.h
> index d5feb3d..1fb5e12 100644
> --- a/drivers/iommu/intel-pasid.h
> +++ b/drivers/iommu/intel-pasid.h
> @@ -13,9 +13,27 @@
>  #define PASID_MIN			0x1
>  #define PASID_MAX			0x20000
>  
> +struct pasid_entry {
> +	u64 val;
> +};
> +
> +/* The representative of a PASID table */
> +struct pasid_table {
> +	void			*table;		/* pasid table pointer */
> +	int			order;		/* page order of pasid table */
> +	int			max_pasid;	/* max pasid */
> +	struct list_head	dev;		/* device list */
> +};
> +
>  extern u32 intel_pasid_max_id;
>  int intel_pasid_alloc_id(void *ptr, int start, int end, gfp_t gfp);
>  void intel_pasid_free_id(int pasid);
>  void *intel_pasid_lookup_id(int pasid);
> +int intel_pasid_alloc_table(struct device *dev);
> +void intel_pasid_free_table(struct device *dev);
> +struct pasid_table *intel_pasid_get_table(struct device *dev);
> +int intel_pasid_get_dev_max_id(struct device *dev);
> +struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid);
> +void intel_pasid_clear_entry(struct device *dev, int pasid);
>  
>  #endif /* __INTEL_PASID_H */
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index 56e65d0..9b5dc72 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -34,10 +34,6 @@
>  
>  static irqreturn_t prq_event_thread(int irq, void *d);
>  
> -struct pasid_entry {
> -	u64 val;
> -};
> -
>  struct pasid_state_entry {
>  	u64 val;
>  };
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index 4fd4c6f..e7901d4 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -476,6 +476,7 @@ struct intel_iommu {
>  struct device_domain_info {
>  	struct list_head link;	/* link to domain siblings */
>  	struct list_head global; /* link to global list */
> +	struct list_head table;	/* link to pasid table */
>  	u8 bus;			/* PCI bus number */
>  	u8 devfn;		/* PCI devfn number */
>  	u16 pfsid;		/* SRIOV physical function source ID */
> @@ -489,6 +490,7 @@ struct device_domain_info {
>  	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 pasid_table *pasid_table; /* pasid table */
>  };
>  
>  static inline void __iommu_flush_cache(
> -- 
> 2.7.4
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

-- 
Peter Xu

Re: [PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Peter Xu]

On Mon, Jul 09, 2018 at 01:22:58PM +0800, Lu Baolu wrote:
> The obsolete per iommu pasid tables are no longer used. Hence,
> clean up them.
> 
> Cc: Ashok Raj <ashok.raj@intel.com>
> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Cc: Kevin Tian <kevin.tian@intel.com>
> Cc: Liu Yi L <yi.l.liu@intel.com>
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
> ---
>  drivers/iommu/intel-iommu.c |  6 +++---
>  drivers/iommu/intel-svm.c   | 17 ++---------------
>  include/linux/intel-iommu.h |  5 ++---
>  3 files changed, 7 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index f8609b5..9a88081 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1755,7 +1755,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
>  	if (pasid_enabled(iommu)) {
>  		if (ecap_prs(iommu->ecap))
>  			intel_svm_finish_prq(iommu);
> -		intel_svm_free_pasid_tables(iommu);
> +		intel_svm_exit(iommu);
>  	}
>  #endif
>  }
> @@ -3336,7 +3336,7 @@ static int __init init_dmars(void)
>  			hw_pass_through = 0;
>  #ifdef CONFIG_INTEL_IOMMU_SVM
>  		if (pasid_enabled(iommu))
> -			intel_svm_alloc_pasid_tables(iommu);
> +			intel_svm_init(iommu);
>  #endif
>  	}
>  
> @@ -4330,7 +4330,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
>  
>  #ifdef CONFIG_INTEL_IOMMU_SVM
>  	if (pasid_enabled(iommu))
> -		intel_svm_alloc_pasid_tables(iommu);
> +		intel_svm_init(iommu);
>  #endif
>  
>  	if (dmaru->ignored) {
> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
> index a253cde..eb30836 100644
> --- a/drivers/iommu/intel-svm.c
> +++ b/drivers/iommu/intel-svm.c
> @@ -38,7 +38,7 @@ struct pasid_state_entry {
>  	u64 val;
>  };
>  
> -int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
> +int intel_svm_init(struct intel_iommu *iommu)
>  {
>  	struct page *pages;
>  	int order;
> @@ -63,15 +63,6 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>  		iommu->pasid_max = 0x20000;
>  
>  	order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
> -	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
> -	if (!pages) {
> -		pr_warn("IOMMU: %s: Failed to allocate PASID table\n",
> -			iommu->name);
> -		return -ENOMEM;
> -	}
> -	iommu->pasid_table = page_address(pages);
> -	pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order);
> -
>  	if (ecap_dis(iommu->ecap)) {
>  		/* Just making it explicit... */
>  		BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry));

Then do we still need the so-called pasid state table?  I tried to
find out what it did before but I failed since I see that the bit 27
of the extended cap is marked as reserved now in the latest vt-d spec
(then ecap_dis could be meaningless too).

Asked since if we don't need both the per-iommu pasid table and the
pasid state table, then maybe we don't need intel_svm_init() at all?

> @@ -86,14 +77,10 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>  	return 0;
>  }
>  
> -int intel_svm_free_pasid_tables(struct intel_iommu *iommu)
> +int intel_svm_exit(struct intel_iommu *iommu)

Same thing applies to this exit() function - after we removed the
per-iommu idr, per-iommu pasid table, and (possibly obsolete)
per-iommu pasid state table, do we need that at all?

Thanks,

>  {
>  	int order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
>  
> -	if (iommu->pasid_table) {
> -		free_pages((unsigned long)iommu->pasid_table, order);
> -		iommu->pasid_table = NULL;
> -	}
>  	if (iommu->pasid_state_table) {
>  		free_pages((unsigned long)iommu->pasid_state_table, order);
>  		iommu->pasid_state_table = NULL;
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index e7901d4..3c43882 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -453,7 +453,6 @@ struct intel_iommu {
>  	 * devices away to userspace processes (e.g. for DPDK) and don't
>  	 * want to trust that userspace will use *only* the PASID it was
>  	 * told to. But while it's all driver-arbitrated, we're fine. */
> -	struct pasid_entry *pasid_table;
>  	struct pasid_state_entry *pasid_state_table;
>  	struct page_req_dsc *prq;
>  	unsigned char prq_name[16];    /* Name for PRQ interrupt */
> @@ -526,8 +525,8 @@ int for_each_device_domain(int (*fn)(struct device_domain_info *info,
>  				     void *data), void *data);
>  
>  #ifdef CONFIG_INTEL_IOMMU_SVM
> -extern int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu);
> -extern int intel_svm_free_pasid_tables(struct intel_iommu *iommu);
> +int intel_svm_init(struct intel_iommu *iommu);
> +int intel_svm_exit(struct intel_iommu *iommu);
>  extern int intel_svm_enable_prq(struct intel_iommu *iommu);
>  extern int intel_svm_finish_prq(struct intel_iommu *iommu);
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> iommu mailing list
> iommu@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu

-- 
Peter Xu

Re: [PATCH v4 1/9] iommu/vt-d: Global PASID name space

[Peter Xu]

On Mon, Jul 09, 2018 at 01:22:50PM +0800, Lu Baolu wrote:

[...]

> +#ifndef __INTEL_PASID_H
> +#define __INTEL_PASID_H
> +
> +#define PASID_MIN			0x1
> +#define PASID_MAX			0x20000

Could I ask whether there's a reason to explicitly use 0x20000 for the
max value?  Asked since I saw that the example in the spec gave 20
bits for PASID (please refer to spec ver 3.0 section 3.4.3 figure
3-8).  Also I believe that's what I was told by Kevin.

I saw that the old per-iommu max value is set to 0x20000, though I'm
not sure whether that's still needed since if we're going to have
two-level pasid table then AFAIU we don't need physically continuous
memory any more (though I saw that we don't yet have two-level pasid
table implemented):

	/* Eventually I'm promised we will get a multi-level PASID table
	 * and it won't have to be physically contiguous. Until then,
	 * limit the size because 8MiB contiguous allocations can be hard
	 * to come by. The limit of 0x20000, which is 1MiB for each of
	 * the PASID and PASID-state tables, is somewhat arbitrary. */
	if (iommu->pasid_max > 0x20000)
		iommu->pasid_max = 0x20000;

Thanks,

-- 
Peter Xu

Re: [PATCH v4 1/9] iommu/vt-d: Global PASID name space

[Lu Baolu]

Hi Peter,

Thanks for looking into my patches.

On 07/11/2018 10:48 AM, Peter Xu wrote:
> On Mon, Jul 09, 2018 at 01:22:50PM +0800, Lu Baolu wrote:
>
> [...]
>
>> +#ifndef __INTEL_PASID_H
>> +#define __INTEL_PASID_H
>> +
>> +#define PASID_MIN			0x1
>> +#define PASID_MAX			0x20000
> Could I ask whether there's a reason to explicitly use 0x20000 for the
> max value?  Asked since I saw that the example in the spec gave 20
> bits for PASID (please refer to spec ver 3.0 section 3.4.3 figure
> 3-8).  Also I believe that's what I was told by Kevin.
>
> I saw that the old per-iommu max value is set to 0x20000, though I'm
> not sure whether that's still needed since if we're going to have
> two-level pasid table then AFAIU we don't need physically continuous
> memory any more (though I saw that we don't yet have two-level pasid
> table implemented):
>
> 	/* Eventually I'm promised we will get a multi-level PASID table
> 	 * and it won't have to be physically contiguous. Until then,
> 	 * limit the size because 8MiB contiguous allocations can be hard
> 	 * to come by. The limit of 0x20000, which is 1MiB for each of
> 	 * the PASID and PASID-state tables, is somewhat arbitrary. */
> 	if (iommu->pasid_max > 0x20000)
> 		iommu->pasid_max = 0x20000;

You are right.

With the scalable mode defined in vt-d v3.0, wecould use the full 20 bit
pasid. Previous max pasid was intended to save contiguous physical memory.

Best regards,
Lu Baolu

Re: [PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Lu Baolu]

Hi Peter,

On 07/11/2018 10:45 AM, Peter Xu wrote:
> On Mon, Jul 09, 2018 at 01:22:58PM +0800, Lu Baolu wrote:
>> The obsolete per iommu pasid tables are no longer used. Hence,
>> clean up them.
>>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
>> ---
>>  drivers/iommu/intel-iommu.c |  6 +++---
>>  drivers/iommu/intel-svm.c   | 17 ++---------------
>>  include/linux/intel-iommu.h |  5 ++---
>>  3 files changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index f8609b5..9a88081 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -1755,7 +1755,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
>>  	if (pasid_enabled(iommu)) {
>>  		if (ecap_prs(iommu->ecap))
>>  			intel_svm_finish_prq(iommu);
>> -		intel_svm_free_pasid_tables(iommu);
>> +		intel_svm_exit(iommu);
>>  	}
>>  #endif
>>  }
>> @@ -3336,7 +3336,7 @@ static int __init init_dmars(void)
>>  			hw_pass_through = 0;
>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>>  		if (pasid_enabled(iommu))
>> -			intel_svm_alloc_pasid_tables(iommu);
>> +			intel_svm_init(iommu);
>>  #endif
>>  	}
>>  
>> @@ -4330,7 +4330,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
>>  
>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>>  	if (pasid_enabled(iommu))
>> -		intel_svm_alloc_pasid_tables(iommu);
>> +		intel_svm_init(iommu);
>>  #endif
>>  
>>  	if (dmaru->ignored) {
>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>> index a253cde..eb30836 100644
>> --- a/drivers/iommu/intel-svm.c
>> +++ b/drivers/iommu/intel-svm.c
>> @@ -38,7 +38,7 @@ struct pasid_state_entry {
>>  	u64 val;
>>  };
>>  
>> -int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>> +int intel_svm_init(struct intel_iommu *iommu)
>>  {
>>  	struct page *pages;
>>  	int order;
>> @@ -63,15 +63,6 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>>  		iommu->pasid_max = 0x20000;
>>  
>>  	order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
>> -	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
>> -	if (!pages) {
>> -		pr_warn("IOMMU: %s: Failed to allocate PASID table\n",
>> -			iommu->name);
>> -		return -ENOMEM;
>> -	}
>> -	iommu->pasid_table = page_address(pages);
>> -	pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order);
>> -
>>  	if (ecap_dis(iommu->ecap)) {
>>  		/* Just making it explicit... */
>>  		BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry));
> Then do we still need the so-called pasid state table?  I tried to
> find out what it did before but I failed since I see that the bit 27
> of the extended cap is marked as reserved now in the latest vt-d spec
> (then ecap_dis could be meaningless too).
>
> Asked since if we don't need both the per-iommu pasid table and the
> pasid state table, then maybe we don't need intel_svm_init() at all?
>
>> @@ -86,14 +77,10 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>>  	return 0;
>>  }
>>  
>> -int intel_svm_free_pasid_tables(struct intel_iommu *iommu)
>> +int intel_svm_exit(struct intel_iommu *iommu)
> Same thing applies to this exit() function - after we removed the
> per-iommu idr, per-iommu pasid table, and (possibly obsolete)
> per-iommu pasid state table, do we need that at all?
>
> Thanks,
>

Good eyes!

Yes, we will cleanup this with a following patch series which enables
features defined in the vt-d v3.0.

Best regards,
Lu Baolu

Re: [PATCH v4 6/9] iommu/vt-d: Per PCI device pasid table interfaces

[Lu Baolu]

Hi,

On 07/11/2018 10:18 AM, Peter Xu wrote:
> On Mon, Jul 09, 2018 at 01:22:55PM +0800, Lu Baolu wrote:
>> This patch adds the interfaces for per PCI device pasid
>> table management. Currently we allocate one pasid table
>> for all PCI devices under the scope of an IOMMU. It's
>> insecure in some cases where multiple devices under one
>> single IOMMU unit support PASID features. With per PCI
>> device pasid table, we can achieve finer protection and
>> isolation granularity.
>>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Suggested-by: Ashok Raj <ashok.raj@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
>> ---
>>  drivers/iommu/intel-iommu.c |   1 +
>>  drivers/iommu/intel-pasid.c | 178 ++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/iommu/intel-pasid.h |  18 +++++
>>  drivers/iommu/intel-svm.c   |   4 -
>>  include/linux/intel-iommu.h |   2 +
>>  5 files changed, 199 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index 3d802c5..a930918 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -2450,6 +2450,7 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
>>  	info->dev = dev;
>>  	info->domain = domain;
>>  	info->iommu = iommu;
>> +	info->pasid_table = NULL;
>>  
>>  	if (dev && dev_is_pci(dev)) {
>>  		struct pci_dev *pdev = to_pci_dev(info->dev);
>> diff --git a/drivers/iommu/intel-pasid.c b/drivers/iommu/intel-pasid.c
>> index e918fe0..1b61942 100644
>> --- a/drivers/iommu/intel-pasid.c
>> +++ b/drivers/iommu/intel-pasid.c
>> @@ -13,6 +13,8 @@
>>  #include <linux/intel-iommu.h>
>>  #include <linux/iommu.h>
>>  #include <linux/memory.h>
>> +#include <linux/pci.h>
>> +#include <linux/pci-ats.h>
>>  #include <linux/spinlock.h>
>>  
>>  #include "intel-pasid.h"
>> @@ -58,3 +60,179 @@ void *intel_pasid_lookup_id(int pasid)
>>  
>>  	return p;
>>  }
>> +
>> +/*
>> + * Per device pasid table management:
>> + */
>> +static inline void
>> +device_attach_pasid_table(struct device_domain_info *info,
>> +			  struct pasid_table *pasid_table)
>> +{
>> +	info->pasid_table = pasid_table;
>> +	list_add(&info->table, &pasid_table->dev);
>> +}
>> +
>> +static inline void
>> +device_detach_pasid_table(struct device_domain_info *info,
>> +			  struct pasid_table *pasid_table)
>> +{
>> +	info->pasid_table = NULL;
>> +	list_del(&info->table);
>> +}
>> +
>> +struct pasid_table_opaque {
>> +	struct pasid_table	**pasid_table;
>> +	int			segment;
>> +	int			bus;
>> +	int			devfn;
>> +};
>> +
>> +static int search_pasid_table(struct device_domain_info *info, void *opaque)
>> +{
>> +	struct pasid_table_opaque *data = opaque;
>> +
>> +	if (info->iommu->segment == data->segment &&
>> +	    info->bus == data->bus &&
>> +	    info->devfn == data->devfn &&
>> +	    info->pasid_table) {
>> +		*data->pasid_table = info->pasid_table;
>> +		return 1;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int get_alias_pasid_table(struct pci_dev *pdev, u16 alias, void *opaque)
>> +{
>> +	struct pasid_table_opaque *data = opaque;
>> +
>> +	data->segment = pci_domain_nr(pdev->bus);
>> +	data->bus = PCI_BUS_NUM(alias);
>> +	data->devfn = alias & 0xff;
>> +
>> +	return for_each_device_domain(&search_pasid_table, data);
>> +}
>> +
>> +int intel_pasid_alloc_table(struct device *dev)
>> +{
>> +	struct device_domain_info *info;
>> +	struct pasid_table *pasid_table;
>> +	struct pasid_table_opaque data;
>> +	struct page *pages;
>> +	size_t size, count;
>> +	int ret, order;
>> +
>> +	info = dev->archdata.iommu;
>> +	if (WARN_ON(!info || !dev_is_pci(dev) ||
>> +		    !info->pasid_supported || info->pasid_table))
>> +		return -EINVAL;
>> +
>> +	/* DMA alias device already has a pasid table, use it: */
>> +	data.pasid_table = &pasid_table;
>> +	ret = pci_for_each_dma_alias(to_pci_dev(dev),
>> +				     &get_alias_pasid_table, &data);
>> +	if (ret)
>> +		goto attach_out;
>> +
>> +	pasid_table = kzalloc(sizeof(*pasid_table), GFP_ATOMIC);
> Do we need to take some lock here (e.g., the pasid lock)?  Otherwise
> what if two devices (that are sharing the same DMA alias) call the
> function intel_pasid_alloc_table() concurrently, then could it
> possible that we create one table for each of the device while AFAIU
> we should let them share a single pasid table?

The only place where this function is called is in a single-thread context
(protected by a spinlock of device_domain_lock with local interrupt disabled).

So we don't need an extra lock here. But anyway, I should put a comment
here.

>
>> +	if (!pasid_table)
>> +		return -ENOMEM;
>> +	INIT_LIST_HEAD(&pasid_table->dev);
>> +
>> +	size = sizeof(struct pasid_entry);
>> +	count = min_t(int, pci_max_pasids(to_pci_dev(dev)), intel_pasid_max_id);
>> +	order = get_order(size * count);
>> +	pages = alloc_pages_node(info->iommu->node,
>> +				 GFP_ATOMIC | __GFP_ZERO,
>> +				 order);
>> +	if (!pages)
>> +		return -ENOMEM;
>> +
>> +	pasid_table->table = page_address(pages);
>> +	pasid_table->order = order;
>> +	pasid_table->max_pasid = count;
>> +
>> +attach_out:
>> +	device_attach_pasid_table(info, pasid_table);
>> +
>> +	return 0;
>> +}
>> +
>> +void intel_pasid_free_table(struct device *dev)
>> +{
>> +	struct device_domain_info *info;
>> +	struct pasid_table *pasid_table;
>> +
>> +	info = dev->archdata.iommu;
>> +	if (!info || !dev_is_pci(dev) ||
>> +	    !info->pasid_supported || !info->pasid_table)
>> +		return;
>> +
>> +	pasid_table = info->pasid_table;
>> +	device_detach_pasid_table(info, pasid_table);
>> +
>> +	if (!list_empty(&pasid_table->dev))
>> +		return;
> Same question to here: do we need a lock?

Same reason as above.

>
>> +
>> +	free_pages((unsigned long)pasid_table->table, pasid_table->order);
>> +	kfree(pasid_table);
>> +}
>> +
>> +struct pasid_table *intel_pasid_get_table(struct device *dev)
>> +{
>> +	struct device_domain_info *info;
>> +
>> +	info = dev->archdata.iommu;
>> +	if (!info)
>> +		return NULL;
>> +
>> +	return info->pasid_table;
>> +}
>> +
>> +int intel_pasid_get_dev_max_id(struct device *dev)
>> +{
>> +	struct device_domain_info *info;
>> +
>> +	info = dev->archdata.iommu;
>> +	if (!info || !info->pasid_table)
>> +		return 0;
>> +
>> +	return info->pasid_table->max_pasid;
>> +}
>> +
>> +struct pasid_entry *intel_pasid_get_entry(struct device *dev, int pasid)
>> +{
>> +	struct pasid_table *pasid_table;
>> +	struct pasid_entry *entries;
>> +
>> +	pasid_table = intel_pasid_get_table(dev);
>> +	if (WARN_ON(!pasid_table || pasid < 0 ||
>> +		    pasid >= intel_pasid_get_dev_max_id(dev)))
>> +		return NULL;
>> +
>> +	entries = pasid_table->table;
>> +
>> +	return &entries[pasid];
>> +}
>> +
>> +/*
>> + * Interfaces for PASID table entry manipulation:
>> + */
>> +static void sm_pasid_clear_entry(struct pasid_entry *pe)
>> +{
>> +	memset(pe, 0, sizeof(struct pasid_entry));
> [1]
>
>> +}
>> +
>> +void intel_pasid_clear_entry(struct device *dev, int pasid)
>> +{
>> +	struct pasid_entry *pe;
>> +
>> +	pe = intel_pasid_get_entry(dev, pasid);
>> +	if (WARN_ON(!pe))
>> +		return;
>> +
>> +	sm_pasid_clear_entry(pe);
>> +
>> +	/* Make sure the entry update is visible before translation. */
>> +	wmb();
> Pure question: AFAIU wmb only orders write operation, then do we
> really need it here?  Instead from the comment I feel like what we
> really need is a WRITE_ONCE() at [1].  Am I wrong somewhere?

I reconsidered this. Yes, you are right. I *need* a WRITE_ONCE() at [1].

Good catch! Thank you!

Best regards,
Lu Baolu

Re: [PATCH v4 6/9] iommu/vt-d: Per PCI device pasid table interfaces

[Peter Xu]

On Wed, Jul 11, 2018 at 03:26:21PM +0800, Lu Baolu wrote:

[...]

> >> +int intel_pasid_alloc_table(struct device *dev)
> >> +{
> >> +	struct device_domain_info *info;
> >> +	struct pasid_table *pasid_table;
> >> +	struct pasid_table_opaque data;
> >> +	struct page *pages;
> >> +	size_t size, count;
> >> +	int ret, order;
> >> +
> >> +	info = dev->archdata.iommu;
> >> +	if (WARN_ON(!info || !dev_is_pci(dev) ||
> >> +		    !info->pasid_supported || info->pasid_table))
> >> +		return -EINVAL;
> >> +
> >> +	/* DMA alias device already has a pasid table, use it: */
> >> +	data.pasid_table = &pasid_table;
> >> +	ret = pci_for_each_dma_alias(to_pci_dev(dev),
> >> +				     &get_alias_pasid_table, &data);
> >> +	if (ret)
> >> +		goto attach_out;
> >> +
> >> +	pasid_table = kzalloc(sizeof(*pasid_table), GFP_ATOMIC);
> > Do we need to take some lock here (e.g., the pasid lock)?  Otherwise
> > what if two devices (that are sharing the same DMA alias) call the
> > function intel_pasid_alloc_table() concurrently, then could it
> > possible that we create one table for each of the device while AFAIU
> > we should let them share a single pasid table?
> 
> The only place where this function is called is in a single-thread context
> (protected by a spinlock of device_domain_lock with local interrupt disabled).
> 
> So we don't need an extra lock here. But anyway, I should put a comment
> here.

Yeah, that would be nice too!  Or add a comment for both of the
functions:

  /* Must be with device_domain_lock held */

Regards,

-- 
Peter Xu

Re: [PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Lu Baolu]

Hi Peter,

Do you have further comments for this series? I will work out a
new version if there are no ones.

Thank you for reviewing my patches. Do you allow me to add your
reviewed-by to the patches which you've reviewed?

Best regards,
Lu Baolu

On 07/11/2018 10:45 AM, Peter Xu wrote:
> On Mon, Jul 09, 2018 at 01:22:58PM +0800, Lu Baolu wrote:
>> The obsolete per iommu pasid tables are no longer used. Hence,
>> clean up them.
>>
>> Cc: Ashok Raj <ashok.raj@intel.com>
>> Cc: Jacob Pan <jacob.jun.pan@linux.intel.com>
>> Cc: Kevin Tian <kevin.tian@intel.com>
>> Cc: Liu Yi L <yi.l.liu@intel.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Reviewed-by: Liu Yi L <yi.l.liu@intel.com>
>> ---
>>  drivers/iommu/intel-iommu.c |  6 +++---
>>  drivers/iommu/intel-svm.c   | 17 ++---------------
>>  include/linux/intel-iommu.h |  5 ++---
>>  3 files changed, 7 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
>> index f8609b5..9a88081 100644
>> --- a/drivers/iommu/intel-iommu.c
>> +++ b/drivers/iommu/intel-iommu.c
>> @@ -1755,7 +1755,7 @@ static void free_dmar_iommu(struct intel_iommu *iommu)
>>  	if (pasid_enabled(iommu)) {
>>  		if (ecap_prs(iommu->ecap))
>>  			intel_svm_finish_prq(iommu);
>> -		intel_svm_free_pasid_tables(iommu);
>> +		intel_svm_exit(iommu);
>>  	}
>>  #endif
>>  }
>> @@ -3336,7 +3336,7 @@ static int __init init_dmars(void)
>>  			hw_pass_through = 0;
>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>>  		if (pasid_enabled(iommu))
>> -			intel_svm_alloc_pasid_tables(iommu);
>> +			intel_svm_init(iommu);
>>  #endif
>>  	}
>>  
>> @@ -4330,7 +4330,7 @@ static int intel_iommu_add(struct dmar_drhd_unit *dmaru)
>>  
>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>>  	if (pasid_enabled(iommu))
>> -		intel_svm_alloc_pasid_tables(iommu);
>> +		intel_svm_init(iommu);
>>  #endif
>>  
>>  	if (dmaru->ignored) {
>> diff --git a/drivers/iommu/intel-svm.c b/drivers/iommu/intel-svm.c
>> index a253cde..eb30836 100644
>> --- a/drivers/iommu/intel-svm.c
>> +++ b/drivers/iommu/intel-svm.c
>> @@ -38,7 +38,7 @@ struct pasid_state_entry {
>>  	u64 val;
>>  };
>>  
>> -int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>> +int intel_svm_init(struct intel_iommu *iommu)
>>  {
>>  	struct page *pages;
>>  	int order;
>> @@ -63,15 +63,6 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>>  		iommu->pasid_max = 0x20000;
>>  
>>  	order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
>> -	pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
>> -	if (!pages) {
>> -		pr_warn("IOMMU: %s: Failed to allocate PASID table\n",
>> -			iommu->name);
>> -		return -ENOMEM;
>> -	}
>> -	iommu->pasid_table = page_address(pages);
>> -	pr_info("%s: Allocated order %d PASID table.\n", iommu->name, order);
>> -
>>  	if (ecap_dis(iommu->ecap)) {
>>  		/* Just making it explicit... */
>>  		BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry));
> Then do we still need the so-called pasid state table?  I tried to
> find out what it did before but I failed since I see that the bit 27
> of the extended cap is marked as reserved now in the latest vt-d spec
> (then ecap_dis could be meaningless too).
>
> Asked since if we don't need both the per-iommu pasid table and the
> pasid state table, then maybe we don't need intel_svm_init() at all?
>
>> @@ -86,14 +77,10 @@ int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu)
>>  	return 0;
>>  }
>>  
>> -int intel_svm_free_pasid_tables(struct intel_iommu *iommu)
>> +int intel_svm_exit(struct intel_iommu *iommu)
> Same thing applies to this exit() function - after we removed the
> per-iommu idr, per-iommu pasid table, and (possibly obsolete)
> per-iommu pasid state table, do we need that at all?
>
> Thanks,
>
>>  {
>>  	int order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
>>  
>> -	if (iommu->pasid_table) {
>> -		free_pages((unsigned long)iommu->pasid_table, order);
>> -		iommu->pasid_table = NULL;
>> -	}
>>  	if (iommu->pasid_state_table) {
>>  		free_pages((unsigned long)iommu->pasid_state_table, order);
>>  		iommu->pasid_state_table = NULL;
>> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
>> index e7901d4..3c43882 100644
>> --- a/include/linux/intel-iommu.h
>> +++ b/include/linux/intel-iommu.h
>> @@ -453,7 +453,6 @@ struct intel_iommu {
>>  	 * devices away to userspace processes (e.g. for DPDK) and don't
>>  	 * want to trust that userspace will use *only* the PASID it was
>>  	 * told to. But while it's all driver-arbitrated, we're fine. */
>> -	struct pasid_entry *pasid_table;
>>  	struct pasid_state_entry *pasid_state_table;
>>  	struct page_req_dsc *prq;
>>  	unsigned char prq_name[16];    /* Name for PRQ interrupt */
>> @@ -526,8 +525,8 @@ int for_each_device_domain(int (*fn)(struct device_domain_info *info,
>>  				     void *data), void *data);
>>  
>>  #ifdef CONFIG_INTEL_IOMMU_SVM
>> -extern int intel_svm_alloc_pasid_tables(struct intel_iommu *iommu);
>> -extern int intel_svm_free_pasid_tables(struct intel_iommu *iommu);
>> +int intel_svm_init(struct intel_iommu *iommu);
>> +int intel_svm_exit(struct intel_iommu *iommu);
>>  extern int intel_svm_enable_prq(struct intel_iommu *iommu);
>>  extern int intel_svm_finish_prq(struct intel_iommu *iommu);
>>  
>> -- 
>> 2.7.4
>>
>> _______________________________________________
>> iommu mailing list
>> iommu@lists.linux-foundation.org
>> https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Peter Xu]

On Fri, Jul 13, 2018 at 09:34:30AM +0800, Lu Baolu wrote:
> Hi Peter,
> 
> Do you have further comments for this series? I will work out a
> new version if there are no ones.

Not for now.  Though since you mentioned about the work to remove the
pasid state table, not sure whether it'll be nice to put that into
this series, or even put more patches that are directly related
(e.g. including the two-level pasid table work and anything related)
so the series could be more complete.

> 
> Thank you for reviewing my patches. Do you allow me to add your
> reviewed-by to the patches which you've reviewed?

I'm glad to read your work.  I didn't leave r-bs because I'm not that
familiar with the codes so I'm not confident to leave any, just to
raise the questions.  Please feel free to post a new version as you
wish, I'll try to leave r-b there if possible.

Thanks,

-- 
Peter Xu

Re: [PATCH v4 9/9] iommu/vt-d: Remove the obsolete per iommu pasid tables

[Lu Baolu]

Hi,

On 07/13/2018 01:00 PM, Peter Xu wrote:
> On Fri, Jul 13, 2018 at 09:34:30AM +0800, Lu Baolu wrote:
>> Hi Peter,
>>
>> Do you have further comments for this series? I will work out a
>> new version if there are no ones.
> Not for now.  Though since you mentioned about the work to remove the
> pasid state table, not sure whether it'll be nice to put that into
> this series, or even put more patches that are directly related
> (e.g. including the two-level pasid table work and anything related)
> so the series could be more complete.
>
>> Thank you for reviewing my patches. Do you allow me to add your
>> reviewed-by to the patches which you've reviewed?
> I'm glad to read your work.  I didn't leave r-bs because I'm not that
> familiar with the codes so I'm not confident to leave any, just to
> raise the questions.  Please feel free to post a new version as you
> wish, I'll try to leave r-b there if possible.
>
> Thanks,
>

Okay! Thank you!

Best regards,
Lu Baolu