iommu/vt-d: Add scalable mode support

Signed-off-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

---
 .../admin-guide/kernel-parameters.txt         | 12 ++--
 drivers/iommu/intel-iommu.c                   | 64 +++++--------------
 include/linux/intel-iommu.h                   |  1 +
 3 files changed, 24 insertions(+), 53 deletions(-)

Documentation/admin-guide/kernel-parameters.txt +6 -6

index 81d1d5a74728..abe9769a9276 100644
@@ -1682,12 +1682,12 @@
 			By default, super page will be supported if Intel IOMMU
 			has the capability. With this option, super page will
 			not be supported.
-		ecs_off [Default Off]
-			By default, extended context tables will be supported if
-			the hardware advertises that it has support both for the
-			extended tables themselves, and also PASID support. With
-			this option set, extended tables will not be used even
-			on hardware which claims to support them.
+		sm_off [Default Off]
+			By default, scalable mode will be supported if the
+			hardware advertises that it has support for the scalable
+			mode translation. With this option set, scalable mode
+			will not be used even on hardware which claims to support
+			it.
 		tboot_noforce [Default Off]
 			Do not force the Intel IOMMU enabled under tboot.
 			By default, tboot will force Intel IOMMU on, which

drivers/iommu/intel-iommu.c +17 ~~-47~~

142 lines not on this page · raw message

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

index b0ae25837361..21e60b3ec263 100644
@@ -151,6 +151,7 @@
  * Extended Capability Register
  */
 
+#define ecap_smts(e)		(((e) >> 43) & 0x1)
 #define ecap_dit(e)		((e >> 41) & 0x1)
 #define ecap_pasid(e)		((e >> 40) & 0x1)
 #define ecap_pss(e)		((e >> 35) & 0x1f)

Signature · 1 line

2.17.1

Lu Baolu

author

2018-11-05 05:31 UTC #2 lore raw

PATCH v4 2/12 iommu/vt-d: Manage scalalble mode PASID tables

In scalable mode, pasid structure is a two level table with a pasid directory table and a pasid table. Any pasid entry can be identified by a pasid value in below way.

1
9 6 5 0

    .-----------------------.-------.
    |              PASID    |       |
    '-----------------------'-------'    .-------------.
             |                    |      |             |
             |                    |      |             |
             |                    |      |             |
             |     .-----------.  |      .-------------.
             |     |           |  |----->| PASID Entry |
             |     |           |  |      '-------------'
             |     |           |  |Plus  |             |
             |     .-----------.  |      |             |
             |---->| DIR Entry |-------->|             |
             |     '-----------'         '-------------'
.---------.  |Plus |           |
| Context |  |     |           |
|  Entry  |------->|           |
'---------'        '-----------'

This changes the pasid table APIs to support scalable mode PASID directory and PASID table. It also adds a helper to get the PASID table entry according to the pasid value.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c |  9 ++--
 drivers/iommu/intel-pasid.c | 84 ++++++++++++++++++++++++++++++-------
 drivers/iommu/intel-pasid.h | 12 +++++-
 drivers/iommu/intel-svm.c   |  6 +--
 4 files changed, 86 insertions(+), 25 deletions(-)

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

index 149561420a75..9c8fabe42b04 100644
@@ -2482,12 +2482,13 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 	if (dev)
 		dev->archdata.iommu = info;
 
-	if (dev && dev_is_pci(dev) && info->pasid_supported) {
+	/* PASID table is mandatory for a PCI device in scalable mode. */
+	if (dev && dev_is_pci(dev) && sm_supported(iommu)) {
 		ret = intel_pasid_alloc_table(dev);
 		if (ret) {
-			pr_warn("No pasid table for %s, pasid disabled\n",
-				dev_name(dev));
-			info->pasid_supported = 0;
+			__dmar_remove_one_dev_info(info);
+			spin_unlock_irqrestore(&device_domain_lock, flags);
+			return NULL;
 		}
 	}
 	spin_unlock_irqrestore(&device_domain_lock, flags);

drivers/iommu/intel-pasid.c +70 ~~-14~~

144 lines not on this page · raw message

drivers/iommu/intel-pasid.h +10 -2

index 1fb5e12b029a..12f480c2bb8b 100644
@@ -11,12 +11,20 @@
 #define __INTEL_PASID_H
 
 #define PASID_MIN			0x1
-#define PASID_MAX			0x20000
+#define PASID_MAX			0x100000
+#define PASID_PTE_MASK			0x3F
+#define PASID_PTE_PRESENT		1
+#define PDE_PFN_MASK			PAGE_MASK
+#define PASID_PDE_SHIFT			6
 
-struct pasid_entry {
+struct pasid_dir_entry {
 	u64 val;
 };
 
+struct pasid_entry {
+	u64 val[8];
+};
+
 /* The representative of a PASID table */
 struct pasid_table {
 	void			*table;		/* pasid table pointer */

drivers/iommu/intel-svm.c +1 -5

index 887150907526..9014b07ab975 100644
@@ -65,8 +65,6 @@ int intel_svm_init(struct intel_iommu *iommu)
 
 	order = get_order(sizeof(struct pasid_entry) * iommu->pasid_max);
 	if (ecap_dis(iommu->ecap)) {
-		/* Just making it explicit... */
-		BUILD_BUG_ON(sizeof(struct pasid_entry) != sizeof(struct pasid_state_entry));
 		pages = alloc_pages(GFP_KERNEL | __GFP_ZERO, order);
 		if (pages)
 			iommu->pasid_state_table = page_address(pages);

@@ -405,9 +403,7 @@ int intel_svm_bind_mm(struct device *dev, int *pasid, int flags, struct svm_dev_
 			pasid_entry_val |= PASID_ENTRY_FLPM_5LP;
 
 		entry = intel_pasid_get_entry(dev, svm->pasid);
-		entry->val = pasid_entry_val;
-
-		wmb();
+		WRITE_ONCE(entry->val[0], pasid_entry_val);
 
 		/*
 		 * Flush PASID cache when a PASID table entry becomes

Signature · 1 line

2.17.1

Lu Baolu

author

2018-11-05 05:31 UTC #3 lore raw

PATCH v4 3/12 iommu/vt-d: Move page table helpers into header

So that they could also be used in other source files.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

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

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

index 9c8fabe42b04..9331240c70b8 100644
@@ -290,49 +290,6 @@ static inline void context_clear_entry(struct context_entry *context)
 	context->hi = 0;
 }
 
-/*
- * 0: readable
- * 1: writable
- * 2-6: reserved
- * 7: super page
- * 8-10: available
- * 11: snoop behavior
- * 12-63: Host physcial address
- */
-struct dma_pte {
-	u64 val;
-};
-
-static inline void dma_clear_pte(struct dma_pte *pte)
-{
-	pte->val = 0;
-}
-
-static inline u64 dma_pte_addr(struct dma_pte *pte)
-{
-#ifdef CONFIG_64BIT
-	return pte->val & VTD_PAGE_MASK;
-#else
-	/* Must have a full atomic 64-bit read */
-	return  __cmpxchg64(&pte->val, 0ULL, 0ULL) & VTD_PAGE_MASK;
-#endif
-}
-
-static inline bool dma_pte_present(struct dma_pte *pte)
-{
-	return (pte->val & 3) != 0;
-}
-
-static inline bool dma_pte_superpage(struct dma_pte *pte)
-{
-	return (pte->val & DMA_PTE_LARGE_PAGE);
-}
-
-static inline int first_pte_in_page(struct dma_pte *pte)
-{
-	return !((unsigned long)pte & ~VTD_PAGE_MASK);
-}
-
 /*
  * This domain is a statically identity mapping domain.
  *	1. This domain creats a static 1:1 mapping to all usable memory.

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

49 lines not on this page · raw message

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 07:41 UTC #4 lore raw

RE: [PATCH v4 03/12] iommu/vt-d: Move page table helpers into header

Hi Baolu,

Quoted 5 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 03/12] iommu/vt-d: Move page table helpers into header
So that they could also be used in other source files.

Just a refine. :)
"This patch moves page table helpers to header file, so that other source files can use them."

Thanks,
Yi Liu

Lu Baolu

author

2018-11-05 05:31 UTC #5 lore raw

PATCH v4 4/12 iommu/vt-d: Add 256-bit invalidation descriptor support

Intel vt-d spec rev3.0 requires software to use 256-bit descriptors in invalidation queue. As the spec reads in section 6.5.2:

Remapping hardware supporting Scalable Mode Translations (ECAP_REG.SMTS=1) allow software to additionally program the width of the descriptors (128-bits or 256-bits) that will be written into the Queue. Software should setup the Invalidation Queue for 256-bit descriptors before progra- mming remapping hardware for scalable-mode translation as 128-bit descriptors are treated as invalid descriptors (see Table 21 in Section 6.5.2.10) in scalable-mode.

This patch adds 256-bit invalidation descriptor support if the hardware presents scalable mode capability.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
 drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
 drivers/iommu/intel_irq_remapping.c |  6 ++-
 include/linux/intel-iommu.h         |  9 +++-
 4 files changed, 115 insertions(+), 59 deletions(-)

drivers/iommu/dmar.c +55 ~~-28~~

index d9c748b6f9e4..ec10427b98ac 100644
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu *iommu, int index)
 	int head, tail;
 	struct q_inval *qi = iommu->qi;
 	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
 	if (qi->desc_status[wait_index] == QI_ABORT)
 		return -EAGAIN;

@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu *iommu, int index)
 	 */
 	if (fault & DMA_FSTS_IQE) {
 		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
 			pr_err("VT-d detected invalid descriptor: "
 				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
+			memcpy(desc, qi->desc + (wait_index << shift),
+			       1 << shift);
 			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
 			return -EINVAL;
 		}

@@ -1191,10 +1194,10 @@ static int qi_check_fault(struct intel_iommu *iommu, int index)
 	 */
 	if (fault & DMA_FSTS_ITE) {
 		head = readl(iommu->reg + DMAR_IQH_REG);
-		head = ((head >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		head = ((head >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
 		head |= 1;
 		tail = readl(iommu->reg + DMAR_IQT_REG);
-		tail = ((tail >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
 
 		writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);

@@ -1222,15 +1225,14 @@ int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu)
 {
 	int rc;
 	struct q_inval *qi = iommu->qi;
-	struct qi_desc *hw, wait_desc;
+	int offset, shift, length;
+	struct qi_desc wait_desc;
 	int wait_index, index;
 	unsigned long flags;
 
 	if (!qi)
 		return 0;
 
-	hw = qi->desc;
-
 restart:
 	rc = 0;

@@ -1243,16 +1245,21 @@ int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu)
 
 	index = qi->free_head;
 	wait_index = (index + 1) % QI_LENGTH;
+	shift = qi_shift(iommu);
+	length = 1 << shift;
 
 	qi->desc_status[index] = qi->desc_status[wait_index] = QI_IN_USE;
 
-	hw[index] = *desc;
-
-	wait_desc.low = QI_IWD_STATUS_DATA(QI_DONE) |
+	offset = index << shift;
+	memcpy(qi->desc + offset, desc, length);
+	wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
 			QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
-	wait_desc.high = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw2 = 0;
+	wait_desc.qw3 = 0;
 
-	hw[wait_index] = wait_desc;
+	offset = wait_index << shift;
+	memcpy(qi->desc + offset, &wait_desc, length);
 
 	qi->free_head = (qi->free_head + 2) % QI_LENGTH;
 	qi->free_cnt -= 2;

@@ -1261,7 +1268,7 @@ int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu)
 	 * update the HW tail register indicating the presence of
 	 * new descriptors.
 	 */
-	writel(qi->free_head << DMAR_IQ_SHIFT, iommu->reg + DMAR_IQT_REG);
+	writel(qi->free_head << shift, iommu->reg + DMAR_IQT_REG);
 
 	while (qi->desc_status[wait_index] != QI_DONE) {
 		/*

@@ -1298,8 +1305,10 @@ void qi_global_iec(struct intel_iommu *iommu)
 {
 	struct qi_desc desc;
 
-	desc.low = QI_IEC_TYPE;
-	desc.high = 0;
+	desc.qw0 = QI_IEC_TYPE;
+	desc.qw1 = 0;
+	desc.qw2 = 0;
+	desc.qw3 = 0;
 
 	/* should never fail */
 	qi_submit_sync(&desc, iommu);

@@ -1310,9 +1319,11 @@ void qi_flush_context(struct intel_iommu *iommu, u16 did, u16 sid, u8 fm,
 {
 	struct qi_desc desc;
 
-	desc.low = QI_CC_FM(fm) | QI_CC_SID(sid) | QI_CC_DID(did)
+	desc.qw0 = QI_CC_FM(fm) | QI_CC_SID(sid) | QI_CC_DID(did)
 			| QI_CC_GRAN(type) | QI_CC_TYPE;
-	desc.high = 0;
+	desc.qw1 = 0;
+	desc.qw2 = 0;
+	desc.qw3 = 0;
 
 	qi_submit_sync(&desc, iommu);
 }

@@ -1331,10 +1342,12 @@ void qi_flush_iotlb(struct intel_iommu *iommu, u16 did, u64 addr,
 	if (cap_read_drain(iommu->cap))
 		dr = 1;
 
-	desc.low = QI_IOTLB_DID(did) | QI_IOTLB_DR(dr) | QI_IOTLB_DW(dw)
+	desc.qw0 = QI_IOTLB_DID(did) | QI_IOTLB_DR(dr) | QI_IOTLB_DW(dw)
 		| QI_IOTLB_GRAN(type) | QI_IOTLB_TYPE;
-	desc.high = QI_IOTLB_ADDR(addr) | QI_IOTLB_IH(ih)
+	desc.qw1 = QI_IOTLB_ADDR(addr) | QI_IOTLB_IH(ih)
 		| QI_IOTLB_AM(size_order);
+	desc.qw2 = 0;
+	desc.qw3 = 0;
 
 	qi_submit_sync(&desc, iommu);
 }

@@ -1347,15 +1360,17 @@ void qi_flush_dev_iotlb(struct intel_iommu *iommu, u16 sid, u16 pfsid,
 	if (mask) {
 		WARN_ON_ONCE(addr & ((1ULL << (VTD_PAGE_SHIFT + mask)) - 1));
 		addr |= (1ULL << (VTD_PAGE_SHIFT + mask - 1)) - 1;
-		desc.high = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
+		desc.qw1 = QI_DEV_IOTLB_ADDR(addr) | QI_DEV_IOTLB_SIZE;
 	} else
-		desc.high = QI_DEV_IOTLB_ADDR(addr);
+		desc.qw1 = QI_DEV_IOTLB_ADDR(addr);
 
 	if (qdep >= QI_DEV_IOTLB_MAX_INVS)
 		qdep = 0;
 
-	desc.low = QI_DEV_IOTLB_SID(sid) | QI_DEV_IOTLB_QDEP(qdep) |
+	desc.qw0 = QI_DEV_IOTLB_SID(sid) | QI_DEV_IOTLB_QDEP(qdep) |
 		   QI_DIOTLB_TYPE | QI_DEV_IOTLB_PFSID(pfsid);
+	desc.qw2 = 0;
+	desc.qw3 = 0;
 
 	qi_submit_sync(&desc, iommu);
 }

@@ -1403,16 +1418,24 @@ static void __dmar_enable_qi(struct intel_iommu *iommu)
 	u32 sts;
 	unsigned long flags;
 	struct q_inval *qi = iommu->qi;
+	u64 val = virt_to_phys(qi->desc);
 
 	qi->free_head = qi->free_tail = 0;
 	qi->free_cnt = QI_LENGTH;
 
+	/*
+	 * Set DW=1 and QS=1 in IQA_REG when Scalable Mode capability
+	 * is present.
+	 */
+	if (ecap_smts(iommu->ecap))
+		val |= (1 << 11) | 1;
+
 	raw_spin_lock_irqsave(&iommu->register_lock, flags);
 
 	/* write zero to the tail reg */
 	writel(0, iommu->reg + DMAR_IQT_REG);
 
-	dmar_writeq(iommu->reg + DMAR_IQA_REG, virt_to_phys(qi->desc));
+	dmar_writeq(iommu->reg + DMAR_IQA_REG, val);
 
 	iommu->gcmd |= DMA_GCMD_QIE;
 	writel(iommu->gcmd, iommu->reg + DMAR_GCMD_REG);

@@ -1448,8 +1471,12 @@ int dmar_enable_qi(struct intel_iommu *iommu)
 
 	qi = iommu->qi;
 
-
-	desc_page = alloc_pages_node(iommu->node, GFP_ATOMIC | __GFP_ZERO, 0);
+	/*
+	 * Need two pages to accommodate 256 descriptors of 256 bits each
+	 * if the remapping hardware supports scalable mode translation.
+	 */
+	desc_page = alloc_pages_node(iommu->node, GFP_ATOMIC | __GFP_ZERO,
+				     !!ecap_smts(iommu->ecap));
 	if (!desc_page) {
 		kfree(qi);
 		iommu->qi = NULL;

drivers/iommu/intel-svm.c +49 ~~-27~~

118 lines not on this page · raw message

drivers/iommu/intel_irq_remapping.c +4 -2

index c2d6c11431de..24d45b07f425 100644
@@ -145,9 +145,11 @@ static int qi_flush_iec(struct intel_iommu *iommu, int index, int mask)
 {
 	struct qi_desc desc;
 
-	desc.low = QI_IEC_IIDEX(index) | QI_IEC_TYPE | QI_IEC_IM(mask)
+	desc.qw0 = QI_IEC_IIDEX(index) | QI_IEC_TYPE | QI_IEC_IM(mask)
 		   | QI_IEC_SELECTIVE;
-	desc.high = 0;
+	desc.qw1 = 0;
+	desc.qw2 = 0;
+	desc.qw3 = 0;
 
 	return qi_submit_sync(&desc, iommu);
 }

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

index b9ec2d93eb57..0459ac28552e 100644
@@ -375,13 +375,18 @@ enum {
 #define QI_GRAN_NONG_PASID		2
 #define QI_GRAN_PSI_PASID		3
 
+#define qi_shift(iommu)		(DMAR_IQ_SHIFT + !!ecap_smts((iommu)->ecap))
+
 struct qi_desc {
-	u64 low, high;
+	u64 qw0;
+	u64 qw1;
+	u64 qw2;
+	u64 qw3;
 };
 
 struct q_inval {
 	raw_spinlock_t  q_lock;
-	struct qi_desc  *desc;          /* invalidation queue */
+	void		*desc;          /* invalidation queue */
 	int             *desc_status;   /* desc status */
 	int             free_head;      /* first free entry */
 	int             free_tail;      /* last free entry */

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 06:07 UTC #6 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi Baolu,

Quoted 2 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM

[...]

Quoted 27 lines

---
 drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
 drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
 drivers/iommu/intel_irq_remapping.c |  6 ++-
 include/linux/intel-iommu.h         |  9 +++-
 4 files changed, 115 insertions(+), 59 deletions(-)

drivers/iommu/dmar.c +1 -0

index d9c748b6f9e4..ec10427b98ac 100644
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu *iommu, int
 index)
 	int head, tail;
 	struct q_inval *qi = iommu->qi;
 	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
 	if (qi->desc_status[wait_index] == QI_ABORT)

drivers/iommu/dmar.c +1 -1

 		return -EAGAIN;

@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu *iommu,
 int index)
 	 */
 	if (fault & DMA_FSTS_IQE) {
 		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {

drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, · line 1184

Quoted 10 lines

drivers/iommu/dmar.c +4 -4

+			struct qi_desc *desc = qi->desc + head;
+
 			pr_err("VT-d detected invalid descriptor: "
 				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);

Still missing qw2 and qw3. May make the print differ based on if smts is configed.

drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184

Quoted 1 line

drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),

Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?

Quoted 52 lines

drivers/iommu/dmar.c +1 -0

+			       1 << shift);
 			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
 			return -EINVAL;

drivers/iommu/dmar.c +8 -8

@@ -1191,10 +1194,10 @@ static int qi_check_fault(struct intel_iommu *iommu,
 int index)
 	 */
 	if (fault & DMA_FSTS_ITE) {
 		head = readl(iommu->reg + DMAR_IQH_REG);
-		head = ((head >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		head = ((head >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
 		head |= 1;
 		tail = readl(iommu->reg + DMAR_IQT_REG);
-		tail = ((tail >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
 
 		writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);

@@ -1222,15 +1225,14 @@ int qi_submit_sync(struct qi_desc *desc, struct
 intel_iommu *iommu)
 {
 	int rc;
 	struct q_inval *qi = iommu->qi;
-	struct qi_desc *hw, wait_desc;
+	int offset, shift, length;
+	struct qi_desc wait_desc;
 	int wait_index, index;
 	unsigned long flags;
 
 	if (!qi)
 		return 0;
 
-	hw = qi->desc;
-
 restart:
 	rc = 0;

@@ -1243,16 +1245,21 @@ int qi_submit_sync(struct qi_desc *desc, struct
 intel_iommu *iommu)
 
 	index = qi->free_head;
 	wait_index = (index + 1) % QI_LENGTH;
+	shift = qi_shift(iommu);
+	length = 1 << shift;
 
 	qi->desc_status[index] = qi->desc_status[wait_index] = QI_IN_USE;
 
-	hw[index] = *desc;
-
-	wait_desc.low = QI_IWD_STATUS_DATA(QI_DONE) |
+	offset = index << shift;
+	memcpy(qi->desc + offset, desc, length);

drivers/iommu/dmar.c int qi_submit_sync(struct qi_desc *desc, struct · line 1263

Quoted 10 lines

drivers/iommu/dmar.c +6 -2

+	wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
 			QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
-	wait_desc.high = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw2 = 0;
+	wait_desc.qw3 = 0;
 
-	hw[wait_index] = wait_desc;
+	offset = wait_index << shift;
+	memcpy(qi->desc + offset, &wait_desc, length);

same question with above one.

Thanks,
Yi Liu

Lu Baolu

author

2018-11-08 02:16 UTC #7 lore raw

Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi Yi,

Quoted 38 lines · Yi L

Hi Baolu,

Quoted 2 lines

From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM

[...]

Quoted 31 lines

---
  drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
  drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
  drivers/iommu/intel_irq_remapping.c |  6 ++-
  include/linux/intel-iommu.h         |  9 +++-
  4 files changed, 115 insertions(+), 59 deletions(-)

drivers/iommu/dmar.c +1 -0

index d9c748b6f9e4..ec10427b98ac 100644
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu *iommu, int
 index)
  	int head, tail;
  	struct q_inval *qi = iommu->qi;
  	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
  	if (qi->desc_status[wait_index] == QI_ABORT)

return -EAGAIN;

drivers/iommu/dmar.c +3 -1

@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu *iommu,
 int index)
  	 */
  	if (fault & DMA_FSTS_IQE) {
  		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
  			pr_err("VT-d detected invalid descriptor: "
  				"low=%llx, high=%llx\n",

Quoted 8 lines

drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1183
Quoted 6 lines
drivers/iommu/dmar.c +2 -4
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is configed.

qw2 and qw3 are reserved from software point of view. We don't need to print it for information.

Quoted 5 lines

drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 2 lines
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?

Can that be compiled? memcpy() requires a "const void *" for the second parameter. By the way, why it's safer with this casting?

Quoted 60 lines

drivers/iommu/dmar.c +1 -0

+			       1 << shift);
  			writel(DMA_FSTS_IQE, iommu->reg + DMAR_FSTS_REG);
  			return -EINVAL;

drivers/iommu/dmar.c +12 -9

@@ -1191,10 +1194,10 @@ static int qi_check_fault(struct intel_iommu *iommu,
 int index)
  	 */
  	if (fault & DMA_FSTS_ITE) {
  		head = readl(iommu->reg + DMAR_IQH_REG);
-		head = ((head >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		head = ((head >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
  		head |= 1;
  		tail = readl(iommu->reg + DMAR_IQT_REG);
-		tail = ((tail >> DMAR_IQ_SHIFT) - 1 + QI_LENGTH) % QI_LENGTH;
+		tail = ((tail >> shift) - 1 + QI_LENGTH) % QI_LENGTH;
 
  		writel(DMA_FSTS_ITE, iommu->reg + DMAR_FSTS_REG);

@@ -1222,15 +1225,14 @@ int qi_submit_sync(struct qi_desc *desc, struct
 intel_iommu *iommu)
  {
  	int rc;
  	struct q_inval *qi = iommu->qi;
-	struct qi_desc *hw, wait_desc;
+	int offset, shift, length;
+	struct qi_desc wait_desc;
  	int wait_index, index;
  	unsigned long flags;
 
  	if (!qi)
  		return 0;
 
-	hw = qi->desc;
-
  restart:
  	rc = 0;

@@ -1243,16 +1245,21 @@ int qi_submit_sync(struct qi_desc *desc, struct
 intel_iommu *iommu)
 
  	index = qi->free_head;
  	wait_index = (index + 1) % QI_LENGTH;
+	shift = qi_shift(iommu);
+	length = 1 << shift;
 
  	qi->desc_status[index] = qi->desc_status[wait_index] = QI_IN_USE;
 
-	hw[index] = *desc;
-
-	wait_desc.low = QI_IWD_STATUS_DATA(QI_DONE) |
+	offset = index << shift;
+	memcpy(qi->desc + offset, desc, length);
+	wait_desc.qw0 = QI_IWD_STATUS_DATA(QI_DONE) |
  			QI_IWD_STATUS_WRITE | QI_IWD_TYPE;
-	wait_desc.high = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw1 = virt_to_phys(&qi->desc_status[wait_index]);
+	wait_desc.qw2 = 0;
+	wait_desc.qw3 = 0;

Quoted 6 lines

drivers/iommu/dmar.c int qi_submit_sync(struct qi_desc *desc, struct intel_iommu *iommu) · line 1262
Quoted 3 lines
drivers/iommu/dmar.c +2 -1
-	hw[wait_index] = wait_desc;
+	offset = wait_index << shift;
+	memcpy(qi->desc + offset, &wait_desc, length);
same question with above one.

Ditto.

Best regards,
Lu Baolu

Liu, Yi L

2018-11-08 03:49 UTC #8 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 56 lines

From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 39 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 32 lines
---
  drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
  drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
  drivers/iommu/intel_irq_remapping.c |  6 ++-
  include/linux/intel-iommu.h         |  9 +++-
  4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +1 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int
 index)
  	int head, tail;
  	struct q_inval *qi = iommu->qi;
  	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
if (qi->desc_status[wait_index] == QI_ABORT)
return -EAGAIN;
drivers/iommu/dmar.c +3 -1
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int index)
  	 */
  	if (fault & DMA_FSTS_IQE) {
  		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
  			pr_err("VT-d detected invalid descriptor: "
  				"low=%llx, high=%llx\n",
Quoted 8 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1183
Quoted 6 lines
drivers/iommu/dmar.c +2 -4
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is configed.

Quoted 2 lines

qw2 and qw3 are reserved from software point of view. We don't need to print it for information.

But for Scalable mode, it should be valid?

Quoted 9 lines

Quoted 6 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 3 lines
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?
Can that be compiled? memcpy() requires a "const void *" for the second parameter. By the way, why it's safer with this casting?

This is just an example. My point is the possibility that "qi->desc + (wait_index << shift)" would be treated as "qi->desc plus (wait_index << shift)*sizeof(*qi->desc)". Is it possible for kernel build?

Regards,
Yi Liu

Lu Baolu

author

2018-11-08 05:24 UTC #9 lore raw

Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 58 lines · Yi L

Hi,
Quoted 56 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 47 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 28 lines
---
   drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
   drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
   drivers/iommu/intel_irq_remapping.c |  6 ++-
   include/linux/intel-iommu.h         |  9 +++-
   4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +1 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int
 index)
   	int head, tail;
   	struct q_inval *qi = iommu->qi;
   	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
   	if (qi->desc_status[wait_index] == QI_ABORT)
   		return -EAGAIN;
drivers/iommu/dmar.c +1 -1
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int index)
   	 */
   	if (fault & DMA_FSTS_IQE) {
   		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu · line 1184
Quoted 10 lines
drivers/iommu/dmar.c +4 -4
+			struct qi_desc *desc = qi->desc + head;
+
   			pr_err("VT-d detected invalid descriptor: "
   				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is configed.

Quoted 4 lines

Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?

No. It's reserved for software.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 4 lines
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?

Quoted 6 lines

Quoted 2 lines
Can that be compiled? memcpy() requires a "const void *" for the second parameter. By the way, why it's safer with this casting?
This is just an example. My point is the possibility that "qi->desc + (wait_index << shift)" would be treated as "qi->desc plus (wait_index << shift)*sizeof(*qi->desc)". Is it possible for kernel build?

qi->desc is of type of "void *".

Best regards,
Lu Baolu

Liu, Yi L

2018-11-08 05:45 UTC #10 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Quoted 67 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 1:25 PM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 58 lines · Yi L
Hi,
Quoted 56 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 47 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 28 lines
---
   drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
   drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
   drivers/iommu/intel_irq_remapping.c |  6 ++-
   include/linux/intel-iommu.h         |  9 +++-
   4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +1 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int
 index)
   	int head, tail;
   	struct q_inval *qi = iommu->qi;
   	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
   	if (qi->desc_status[wait_index] == QI_ABORT)
   		return -EAGAIN;
drivers/iommu/dmar.c +1 -1
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct
 intel_iommu *iommu, int index)
   	 */
   	if (fault & DMA_FSTS_IQE) {
   		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
drivers/iommu/dmar.c static int qi_check_fault(struct · line 1184
Quoted 10 lines
drivers/iommu/dmar.c +4 -4
+			struct qi_desc *desc = qi->desc + head;
+
   			pr_err("VT-d detected invalid descriptor: "
   				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is configed.

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?
No. It's reserved for software.

No, I don’t think so. PRQ response would also be queued to hardware by QI. For such QI descriptors, the high bits are not reserved.

Quoted 9 lines

Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 3 lines
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?
Can that be compiled? memcpy() requires a "const void *" for the second
parameter.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
By the way, why it's safer with this casting?
This is just an example. My point is the possibility that "qi->desc + (wait_index <<
shift)"
Quoted 2 lines
would be treated as "qi->desc plus (wait_index << shift)*sizeof(*qi->desc)". Is it possible for kernel build?
qi->desc is of type of "void *".

no, I don’t think so... Refer to the code below. Even it has no correctness issue her, It's not due to qi->desc is "void *" type...

struct qi_desc {
- u64 low, high;
+ u64 qw0;
+ u64 qw1;
+ u64 qw2;
+ u64 qw3;
};

Regards,
Yi Liu

Lu Baolu

author

2018-11-08 06:14 UTC #11 lore raw

Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 63 lines · Yi L

Quoted 63 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 1:25 PM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 55 lines · Yi L
Hi,
Quoted 53 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 45 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 28 lines
---
    drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
    drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
    drivers/iommu/intel_irq_remapping.c |  6 ++-
    include/linux/intel-iommu.h         |  9 +++-
    4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +1 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct intel_iommu
 *iommu, int
 index)
    	int head, tail;
    	struct q_inval *qi = iommu->qi;
    	int wait_index = (index + 1) % QI_LENGTH;
+	int shift = qi_shift(iommu);
 
    	if (qi->desc_status[wait_index] == QI_ABORT)
    		return -EAGAIN;
drivers/iommu/dmar.c +1 -1
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct
 intel_iommu *iommu, int index)
    	 */
    	if (fault & DMA_FSTS_IQE) {
    		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
drivers/iommu/dmar.c static int qi_check_fault(struct · line 1184
Quoted 10 lines
drivers/iommu/dmar.c +4 -4
+			struct qi_desc *desc = qi->desc + head;
+
    			pr_err("VT-d detected invalid descriptor: "
    				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is configed.

Quoted 7 lines

Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?
No. It's reserved for software.
No, I don’t think so. PRQ response would also be queued to hardware by QI. For such QI descriptors, the high bits are not reserved.

Do you mean the private data fields of a page request descriptor or a page group response descriptor? Those fields contains software defined private data (might a kernel pointer?). We should avoid leaking such information in the generic kernel message for security consideration. Or anything I missed?

Best regards,
Lu Baolu

Liu, Yi L

2018-11-08 07:20 UTC #12 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 78 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 2:14 PM
Hi,
Quoted 55 lines · Yi L
Quoted 55 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 1:25 PM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 47 lines · Yi L
Hi,
Quoted 45 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 37 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 31 lines
---
    drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
    drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
    drivers/iommu/intel_irq_remapping.c |  6 ++-
    include/linux/intel-iommu.h         |  9 +++-
    4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +0 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct
intel_iommu *iommu, int
index)
    	int head, tail;
    	struct q_inval *qi = iommu->qi;
    	int wait_index = (index + 1) % QI_LENGTH;
drivers/iommu/dmar.c +4 -1
+	int shift = qi_shift(iommu);
 
    	if (qi->desc_status[wait_index] == QI_ABORT)
    		return -EAGAIN;
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct
 intel_iommu *iommu, int index)
    	 */
    	if (fault & DMA_FSTS_IQE) {
    		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
    			pr_err("VT-d detected invalid descriptor: "
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/dmar.c static int qi_check_fault(struct · line 1184
Quoted 7 lines
drivers/iommu/dmar.c +2 -4
    				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is
configed.
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?
No. It's reserved for software.
No, I don’t think so. PRQ response would also be queued to hardware by QI. For such QI descriptors, the high bits are not reserved.

Quoted 5 lines

Do you mean the private data fields of a page request descriptor or a page group response descriptor? Those fields contains software defined private data (might a kernel pointer?). We should avoid leaking such information in the generic kernel message for security consideration.
Or anything I missed?

yes, I'm not sure what kind of data it may be in the private data field. From software point of view, it may be helpful to show the full content of the QI descriptor for error triage. Personally, I'm fine if you keep it on this point.

Regards,
Yi Liu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Lu Baolu

author

2018-11-09 01:39 UTC #13 lore raw

Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 86 lines · Yi L

Hi,
Quoted 78 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 2:14 PM
Hi,
Quoted 55 lines · Yi L
Quoted 55 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 1:25 PM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 47 lines · Yi L
Hi,
Quoted 45 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 37 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 31 lines
---
     drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
     drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
     drivers/iommu/intel_irq_remapping.c |  6 ++-
     include/linux/intel-iommu.h         |  9 +++-
     4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +0 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct
intel_iommu *iommu, int
index)
     	int head, tail;
     	struct q_inval *qi = iommu->qi;
     	int wait_index = (index + 1) % QI_LENGTH;
drivers/iommu/dmar.c +4 -1
+	int shift = qi_shift(iommu);
 
     	if (qi->desc_status[wait_index] == QI_ABORT)
     		return -EAGAIN;
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct
 intel_iommu *iommu, int index)
     	 */
     	if (fault & DMA_FSTS_IQE) {
     		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
     			pr_err("VT-d detected invalid descriptor: "
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/dmar.c static int qi_check_fault(struct · line 1184
Quoted 7 lines
drivers/iommu/dmar.c +2 -4
     				"low=%llx, high=%llx\n",
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is
configed.
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?
No. It's reserved for software.
No, I don’t think so. PRQ response would also be queued to hardware by QI. For such QI descriptors, the high bits are not reserved.
Quoted 5 lines
Do you mean the private data fields of a page request descriptor or a page group response descriptor? Those fields contains software defined private data (might a kernel pointer?). We should avoid leaking such information in the generic kernel message for security consideration.
Or anything I missed?

Quoted 4 lines

yes, I'm not sure what kind of data it may be in the private data field. From software point of view, it may be helpful to show the full content of the QI descriptor for error triage. Personally, I'm fine if you keep it on this point.

Okay, thanks.

I think I need to put some comments there so that people could understand my consideration.

Best regards,
Lu Baolu

Liu, Yi L

2018-11-09 02:59 UTC #14 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Hi,

Quoted 98 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Friday, November 9, 2018 9:40 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 87 lines · Yi L
Hi,
Quoted 79 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 2:14 PM
Hi,
Quoted 56 lines · Yi L
Quoted 56 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 1:25 PM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi,
Quoted 48 lines · Yi L
Hi,
Quoted 46 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:17 AM
Subject: Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support
Hi Yi,
Quoted 38 lines · Yi L
Hi Baolu,
Quoted 2 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
[...]
Quoted 32 lines
---
     drivers/iommu/dmar.c                | 83 +++++++++++++++++++----------
     drivers/iommu/intel-svm.c           | 76 ++++++++++++++++----------
     drivers/iommu/intel_irq_remapping.c |  6 ++-
     include/linux/intel-iommu.h         |  9 +++-
     4 files changed, 115 insertions(+), 59 deletions(-)
drivers/iommu/dmar.c index renamed from drivers/iommu/dmar.c +0 -0
d9c748b6f9e4..ec10427b98ac 100644
drivers/iommu/dmar.c +0 -0
@@ -1160,6 +1160,7 @@ static int qi_check_fault(struct
intel_iommu *iommu, int
index)
     	int head, tail;
     	struct q_inval *qi = iommu->qi;
     	int wait_index = (index + 1) % QI_LENGTH;
drivers/iommu/dmar.c +4 -1
+	int shift = qi_shift(iommu);
 
     	if (qi->desc_status[wait_index] == QI_ABORT)
     		return -EAGAIN;
@@ -1173,13 +1174,15 @@ static int qi_check_fault(struct
 intel_iommu *iommu, int index)
     	 */
     	if (fault & DMA_FSTS_IQE) {
     		head = readl(iommu->reg + DMAR_IQH_REG);
-		if ((head >> DMAR_IQ_SHIFT) == index) {
+		if ((head >> shift) == index) {
+			struct qi_desc *desc = qi->desc + head;
+
     			pr_err("VT-d detected invalid descriptor: "
     				"low=%llx, high=%llx\n",
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iommu/dmar.c static int qi_check_fault(struct · line 1184
Quoted 6 lines
drivers/iommu/dmar.c +2 -4
-				(unsigned long long)qi->desc[index].low,
-				(unsigned long long)qi->desc[index].high);
-			memcpy(&qi->desc[index], &qi->desc[wait_index],
-					sizeof(struct qi_desc));
+				(unsigned long long)desc->qw0,
+				(unsigned long long)desc->qw1);
Still missing qw2 and qw3. May make the print differ based on if smts is
configed.
Quoted 7 lines
Quoted 4 lines
Quoted 3 lines
Quoted 2 lines
qw2 and qw3 are reserved from software point of view. We don't need to print it for information.
But for Scalable mode, it should be valid?
No. It's reserved for software.
No, I don’t think so. PRQ response would also be queued to hardware by QI. For such QI descriptors, the high bits are not reserved.
Quoted 5 lines
Do you mean the private data fields of a page request descriptor or a page group response descriptor? Those fields contains software defined private data (might a kernel pointer?). We should avoid leaking such information in the generic kernel message for security consideration. Or anything I missed?
Quoted 3 lines
yes, I'm not sure what kind of data it may be in the private data field. From software point of view, it may be helpful to show the full content of the QI descriptor for error triage. Personally, I'm fine if you keep it on

Quoted 7 lines

this point.
Quoted 1 line
Okay, thanks.
I think I need to put some comments there so that people could understand my consideration.

yeah, that would be helpful. :-)

Regards,
Yi Liu

Liu, Yi L

2018-11-08 05:48 UTC #15 lore raw

RE: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Quoted 29 lines

From: Liu, Yi L
Sent: Thursday, November 8, 2018 1:45 PM
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 1 line
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?
Can that be compiled? memcpy() requires a "const void *" for the second
parameter.
Quoted 2 lines
Quoted 1 line
By the way, why it's safer with this casting?
Quoted 7 lines
Quoted 2 lines
This is just an example. My point is the possibility that "qi->desc
+ (wait_index <<
shift)"
Quoted 2 lines
would be treated as "qi->desc plus (wait_index << shift)*sizeof(*qi->desc)". Is it possible for kernel build?
qi->desc is of type of "void *".
no, I don’t think so... Refer to the code below. Even it has no correctness issue her, It's not due to qi->desc is "void *" type...
struct qi_desc {
include/linux/intel-iommu.h +4 -1
-	u64 low, high;
+	u64 qw0;
+	u64 qw1;
+	u64 qw2;
+	u64 qw3;

Quoted 1 line

};

Oops, just see you modified it to be "void *" in this patch. Ok, then this is fair enough.

Thanks,
Yi Liu
_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Lu Baolu

author

2018-11-08 06:15 UTC #16 lore raw

Re: [PATCH v4 04/12] iommu/vt-d: Add 256-bit invalidation descriptor support

Quoted 23 lines · Yi L

Quoted 23 lines
From: Liu, Yi L
Sent: Thursday, November 8, 2018 1:45 PM
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
drivers/iommu/dmar.c static int qi_check_fault(struct intel_iommu *iommu, int index) · line 1184
Quoted 1 line
drivers/iommu/dmar.c +1 -0
+			memcpy(desc, qi->desc + (wait_index << shift),
Would "memcpy(desc, (unsigned long long) (qi->desc + (wait_index << shift)," be more safe?
Can that be compiled? memcpy() requires a "const void *" for the second
parameter.
Quoted 2 lines
Quoted 1 line
By the way, why it's safer with this casting?
Quoted 7 lines
Quoted 2 lines
This is just an example. My point is the possibility that "qi->desc
+ (wait_index <<
shift)"
Quoted 2 lines
would be treated as "qi->desc plus (wait_index << shift)*sizeof(*qi->desc)". Is it possible for kernel build?
qi->desc is of type of "void *".
no, I don’t think so... Refer to the code below. Even it has no correctness issue her, It's not due to qi->desc is "void *" type...

Quoted 9 lines

Quoted 7 lines
struct qi_desc {
include/linux/intel-iommu.h +4 -1
-	u64 low, high;
+	u64 qw0;
+	u64 qw1;
+	u64 qw2;
+	u64 qw3;
};
Oops, just see you modified it to be "void *" in this patch. Ok, then this is fair enough.

Yes. :-)

Best regards,
Lu Baolu

Lu Baolu

author

2018-11-05 05:31 UTC #17 lore raw

PATCH v4 5/12 iommu/vt-d: Reserve a domain id for FL and PT modes

Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid entry for first-level or pass-through translation should be programmed with a domain id different from those used for second-level or nested translation. It is recommended that software could use a same domain id for all first-only and pass-through translations.

This reserves a domain id for first-level and pass-through translations.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Cc Sanjay Kumar <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c | 10 ++++++++++
 drivers/iommu/intel-pasid.h |  6 ++++++
 2 files changed, 16 insertions(+)

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

index 9331240c70b8..2f7455ee4e7a 100644
@@ -1618,6 +1618,16 @@ static int iommu_init_domains(struct intel_iommu *iommu)
 	 */
 	set_bit(0, iommu->domain_ids);
 
+	/*
+	 * Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid
+	 * entry for first-level or pass-through translation modes should
+	 * be programmed with a domain id different from those used for
+	 * second-level or nested translation. We reserve a domain id for
+	 * this purpose.
+	 */
+	if (sm_supported(iommu))
+		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
+
 	return 0;
 }

drivers/iommu/intel-pasid.h +6 -0

index 12f480c2bb8b..03c1612d173c 100644
@@ -17,6 +17,12 @@
 #define PDE_PFN_MASK			PAGE_MASK
 #define PASID_PDE_SHIFT			6
 
+/*
+ * Domain ID reserved for pasid entries programmed for first-level
+ * only and pass-through transfer modes.
+ */
+#define FLPT_DEFAULT_DID		1
+
 struct pasid_dir_entry {
 	u64 val;
 };

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 06:55 UTC #18 lore raw

RE: [PATCH v4 05/12] iommu/vt-d: Reserve a domain id for FL and PT modes

Hi Baolu,

Quoted 33 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 05/12] iommu/vt-d: Reserve a domain id for FL and PT modes
Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid entry for first-level or pass-through translation should be programmed with a domain id different from those used for second-level or nested translation. It is recommended that software could use a same domain id for all first-only and pass-through translations.
This reserves a domain id for first-level and pass-through translations.
Cc Ashok Raj <[email protected]>
Cc Jacob Pan <[email protected]>
Cc Kevin Tian <[email protected]>
Cc Liu Yi L <[email protected]>
Cc Sanjay Kumar <[email protected]>
Signed-off-by Lu Baolu <[email protected]>
---
 drivers/iommu/intel-iommu.c | 10 ++++++++++
 drivers/iommu/intel-pasid.h |  6 ++++++
 2 files changed, 16 insertions(+)
drivers/iommu/intel-iommu.c +0 -0
index 9331240c70b8..2f7455ee4e7a 100644
@@ -1618,6 +1618,16 @@ static int iommu_init_domains(struct intel_iommu
 *iommu)
 	 */
 	set_bit(0, iommu->domain_ids);

drivers/iommu/intel-iommu.c static int iommu_init_domains(struct intel_iommu · line 1630

Quoted 10 lines

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

 
+	/*
+	 * Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid
+	 * entry for first-level or pass-through translation modes should
+	 * be programmed with a domain id different from those used for
+	 * second-level or nested translation. We reserve a domain id for
+	 * this purpose.
+	 */
+	if (sm_supported(iommu))
+		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);

"FLPT_DEFAULT_DID" looks very likely for first level translation. How about "PT_FL_DEFAULT_DID"?

Quoted 8 lines

drivers/iommu/intel-iommu.c +0 -0
 	return 0;
 }
drivers/iommu/intel-pasid.h +0 -0
index 12f480c2bb8b..03c1612d173c 100644

drivers/iommu/intel-pasid.h line 24

Quoted 8 lines

drivers/iommu/intel-pasid.h +5 -0

@@ -17,6 +17,12 @@
 #define PDE_PFN_MASK			PAGE_MASK
 #define PASID_PDE_SHIFT			6
 
+/*
+ * Domain ID reserved for pasid entries programmed for first-level
+ * only and pass-through transfer modes.
+ */
+#define FLPT_DEFAULT_DID		1

Would be helpful to elaborate why DID 1 is selected in the patch description.

Regards,
Yi Liu

Lu Baolu

author

2018-11-08 02:22 UTC #19 lore raw

Re: [PATCH v4 05/12] iommu/vt-d: Reserve a domain id for FL and PT modes

Hi,

Quoted 36 lines · Yi L

Hi Baolu,
Quoted 34 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 05/12] iommu/vt-d: Reserve a domain id for FL and PT modes
Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid entry for first-level or pass-through translation should be programmed with a domain id different from those used for second-level or nested translation. It is recommended that software could use a same domain id for all first-only and pass-through translations.
This reserves a domain id for first-level and pass-through translations.
Cc Ashok Raj <[email protected]>
Cc Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
Cc Kevin Tian <[email protected]>
Cc Liu Yi L <[email protected]>
Cc Sanjay Kumar <[email protected]>
Signed-off-by Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>
---
  drivers/iommu/intel-iommu.c | 10 ++++++++++
  drivers/iommu/intel-pasid.h |  6 ++++++
  2 files changed, 16 insertions(+)
drivers/iommu/intel-iommu.c +0 -0
index 9331240c70b8..2f7455ee4e7a 100644
@@ -1618,6 +1618,16 @@ static int iommu_init_domains(struct intel_iommu
 *iommu)
  	 */
  	set_bit(0, iommu->domain_ids);
 

Quoted 12 lines

drivers/iommu/intel-iommu.c static int iommu_init_domains(struct intel_iommu *iommu) · line 1629
Quoted 9 lines
drivers/iommu/intel-iommu.c +9 -0
+	/*
+	 * Vt-d spec rev3.0 (section 6.2.3.1) requires that each pasid
+	 * entry for first-level or pass-through translation modes should
+	 * be programmed with a domain id different from those used for
+	 * second-level or nested translation. We reserve a domain id for
+	 * this purpose.
+	 */
+	if (sm_supported(iommu))
+		set_bit(FLPT_DEFAULT_DID, iommu->domain_ids);
"FLPT_DEFAULT_DID" looks very likely for first level translation. How about "PT_FL_DEFAULT_DID"?

We have comments above it, so people won't be confused.

Quoted 12 lines

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

  	return 0;
  }

drivers/iommu/intel-pasid.h +0 -0

index 12f480c2bb8b..03c1612d173c 100644
@@ -17,6 +17,12 @@
  #define PDE_PFN_MASK			PAGE_MASK
  #define PASID_PDE_SHIFT			6

Quoted 8 lines

drivers/iommu/intel-pasid.h line 24
Quoted 5 lines
drivers/iommu/intel-pasid.h +5 -0
+/*
+ * Domain ID reserved for pasid entries programmed for first-level
+ * only and pass-through transfer modes.
+ */
+#define FLPT_DEFAULT_DID		1
Would be helpful to elaborate why DID 1 is selected in the patch description.

Yeah. DID 0 has been caved out for caching mode and we start from 1 for this.

Best regards,
Lu Baolu

Lu Baolu

author

2018-11-05 05:31 UTC #20 lore raw

PATCH v4 6/12 iommu/vt-d: Add second level page table interface

This adds the interfaces to setup or tear down the structures for second level page table translations. This includes types of second level only translation and pass through.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c |   2 +-
 drivers/iommu/intel-pasid.c | 280 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h |   8 ++
 include/linux/intel-iommu.h |   3 +
 4 files changed, 292 insertions(+), 1 deletion(-)

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

index 2f7455ee4e7a..fb2eca83ed00 100644
@@ -1207,7 +1207,7 @@ static void iommu_set_root_entry(struct intel_iommu *iommu)
 	raw_spin_unlock_irqrestore(&iommu->register_lock, flag);
 }
 
-static void iommu_flush_write_buffer(struct intel_iommu *iommu)
+void iommu_flush_write_buffer(struct intel_iommu *iommu)
 {
 	u32 val;
 	unsigned long flag;

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

289 lines not on this page · raw message

drivers/iommu/intel-pasid.h +8 -0

index 03c1612d173c..3c70522091d3 100644
@@ -49,5 +49,13 @@ 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);
+int intel_pasid_setup_second_level(struct intel_iommu *iommu,
+				   struct dmar_domain *domain,
+				   struct device *dev, int pasid);
+int intel_pasid_setup_pass_through(struct intel_iommu *iommu,
+				   struct dmar_domain *domain,
+				   struct device *dev, int pasid);
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
+				 struct device *dev, int pasid);
 
 #endif /* __INTEL_PASID_H */

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

index 0459ac28552e..d83908420a83 100644
@@ -151,6 +151,8 @@
  * Extended Capability Register
  */
 
+#define ecap_smpwc(e)		(((e) >> 48) & 0x1)
+#define ecap_slts(e)		(((e) >> 46) & 0x1)
 #define ecap_smts(e)		(((e) >> 43) & 0x1)
 #define ecap_dit(e)		((e >> 41) & 0x1)
 #define ecap_pasid(e)		((e >> 40) & 0x1)

@@ -636,6 +638,7 @@ 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);
+void iommu_flush_write_buffer(struct intel_iommu *iommu);
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
 int intel_svm_init(struct intel_iommu *iommu);

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 07:13 UTC #21 lore raw

RE: [PATCH v4 06/12] iommu/vt-d: Add second level page table interface

Hi Baolu,

Quoted 6 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
This adds the interfaces to setup or tear down the structures for second level page table translations. This includes types of second level only translation and pass through.

A little bit refining to the description:)
"This patch adds interfaces for setup or tear down second level translation in PASID granularity. Translation type includes second level only type and pass-through type."

Quoted 5 lines

Cc Ashok Raj <[email protected]>
Cc Jacob Pan <[email protected]>
Cc Kevin Tian <[email protected]>
Cc Liu Yi L <[email protected]>
Signed-off-by Sanjay Kumar <[email protected]>

[...]

drivers/iommu/intel-pasid.c void intel_pasid_clear_entry(struct device *dev, int pasid) · line 440

Quoted 5 lines

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

+
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
+				 struct device *dev, int pasid)
+{
+	struct pasid_entry *pte;

pte is confusing as it is similar with pte in paging structures. may use pt_entry or just pasid_entry. This comment applies to other "pte"s in this patch.

Thanks,
Yi Liu

Lu Baolu

author

2018-11-08 02:27 UTC #22 lore raw

Re: [PATCH v4 06/12] iommu/vt-d: Add second level page table interface

Hi,

Quoted 22 lines · Yi L

Hi Baolu,
Quoted 6 lines
From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
This adds the interfaces to setup or tear down the structures for second level page table translations. This includes types of second level only translation and pass through.
A little bit refining to the description:)
"This patch adds interfaces for setup or tear down second level translation in PASID granularity. Translation type includes second level only type and pass-through type."
Quoted 5 lines
Cc Ashok Raj <[email protected]>
Cc Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>
Cc Kevin Tian <[email protected]>
Cc Liu Yi L <[email protected]>
Signed-off-by Sanjay Kumar <[email protected]>
[...]

Quoted 9 lines

drivers/iommu/intel-pasid.c void intel_pasid_clear_entry(struct device *dev, int pasid) · line 440
Quoted 5 lines
drivers/iommu/intel-pasid.c +5 -0
+
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
+				 struct device *dev, int pasid)
+{
+	struct pasid_entry *pte;
pte is confusing as it is similar with pte in paging structures. may use pt_entry or just pasid_entry. This comment applies to other "pte"s in this patch.

"pte" in this file means "pasid table entry", not "page table entry". This file holds code to handle pasid table related staff. It has nothing to do with paging structure. I think there should be no confusion here. :-)

Best regards,
Lu Baolu

Liu, Yi L

2018-11-08 04:00 UTC #23 lore raw

RE: [PATCH v4 06/12] iommu/vt-d: Add second level page table interface

Hi,

Quoted 39 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Thursday, November 8, 2018 10:28 AM
Subject: Re: [PATCH v4 06/12] iommu/vt-d: Add second level page table interface
Hi,
Quoted 22 lines · Yi L
Hi Baolu,
Quoted 6 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
This adds the interfaces to setup or tear down the structures for second level page table translations. This includes types of second level only translation and pass through.
A little bit refining to the description:) "This patch adds interfaces for setup or tear down second level translation in PASID granularity. Translation type includes second level only type and pass-through type."
Quoted 5 lines
Cc Ashok Raj <[email protected]>
Cc Jacob Pan <[email protected]>
Cc Kevin Tian <[email protected]>
Cc Liu Yi L <[email protected]>
Signed-off-by Sanjay Kumar <[email protected]>
[...]
Quoted 9 lines
drivers/iommu/intel-pasid.c void intel_pasid_clear_entry(struct device *dev, int pasid) · line 440
Quoted 5 lines
drivers/iommu/intel-pasid.c +5 -0
+
+void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
+				 struct device *dev, int pasid)
+{
+	struct pasid_entry *pte;
pte is confusing as it is similar with pte in paging structures. may use pt_entry or just pasid_entry. This comment applies to other "pte"s in this patch.

Quoted 4 lines

"pte" in this file means "pasid table entry", not "page table entry". This file holds code to handle pasid table related staff. It has nothing to do with paging structure. I think there should be no confusion here.
:-)

I see. Then up to you. :) It's just my feeling when reading the patch, it leads me to believe it is paging structure.

Regards,
Yi Liu

Lu Baolu

author

2018-11-05 05:31 UTC #24 lore raw

PATCH v4 7/12 iommu/vt-d: Setup pasid entry for RID2PASID support

when the scalable mode is enabled, there is no second level page translation pointer in the context entry any more (for DMA request without PASID). Instead, a new RID2PASID field is introduced in the context entry. Software can choose any PASID value to set RID2PASID and then setup the translation in the corresponding PASID entry. Upon receiving a DMA request without PASID, hardware will firstly look at this RID2PASID field and then treat this request as a request with a pasid value specified in RID2PASID field.

Though software is allowed to use any PASID for the RID2PASID, we will always use the PASID 0 as a sort of design decision.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c | 19 +++++++++++++++++++
 drivers/iommu/intel-pasid.h |  1 +
 2 files changed, 20 insertions(+)

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

index fb2eca83ed00..6d2ee8639e4c 100644
@@ -2457,6 +2457,21 @@ static struct dmar_domain *dmar_insert_one_dev_info(struct intel_iommu *iommu,
 			spin_unlock_irqrestore(&device_domain_lock, flags);
 			return NULL;
 		}
+
+		/* Setup the PASID entry for requests without PASID: */
+		spin_lock(&iommu->lock);
+		if (hw_pass_through && domain_type_is_si(domain))
+			ret = intel_pasid_setup_pass_through(iommu, domain,
+					dev, PASID_RID2PASID);
+		else
+			ret = intel_pasid_setup_second_level(iommu, domain,
+					dev, PASID_RID2PASID);
+		spin_unlock(&iommu->lock);
+		if (ret) {
+			__dmar_remove_one_dev_info(info);
+			spin_unlock_irqrestore(&device_domain_lock, flags);
+			return NULL;
+		}
 	}
 	spin_unlock_irqrestore(&device_domain_lock, flags);

@@ -4821,6 +4836,10 @@ static void __dmar_remove_one_dev_info(struct device_domain_info *info)
 	iommu = info->iommu;
 
 	if (info->dev) {
+		if (dev_is_pci(info->dev) && sm_supported(iommu))
+			intel_pasid_tear_down_entry(iommu, info->dev,
+					PASID_RID2PASID);
+
 		iommu_disable_dev_iotlb(info);
 		domain_context_clear(iommu, info->dev);
 		intel_pasid_free_table(info->dev);

drivers/iommu/intel-pasid.h +1 -0

index 3c70522091d3..d6f4fead4491 100644
@@ -10,6 +10,7 @@
 #ifndef __INTEL_PASID_H
 #define __INTEL_PASID_H
 
+#define PASID_RID2PASID			0x0
 #define PASID_MIN			0x1
 #define PASID_MAX			0x100000
 #define PASID_PTE_MASK			0x3F

Signature · 1 line

2.17.1

Lu Baolu

author

2018-11-05 05:31 UTC #25 lore raw

PATCH v4 8/12 iommu/vt-d: Pass pasid table to context mapping

So that the pasid related info, such as the pasid table and the maximum of pasid could be used during setting up scalable mode context.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

---
 drivers/iommu/intel-iommu.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

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

index 6d2ee8639e4c..a761c0aa3438 100644
@@ -1917,6 +1917,7 @@ static void domain_exit(struct dmar_domain *domain)
 
 static int domain_context_mapping_one(struct dmar_domain *domain,
 				      struct intel_iommu *iommu,
+				      struct pasid_table *table,
 				      u8 bus, u8 devfn)
 {
 	u16 did = domain->iommu_did[iommu->seq_id];

@@ -2039,6 +2040,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 struct domain_context_mapping_data {
 	struct dmar_domain *domain;
 	struct intel_iommu *iommu;
+	struct pasid_table *table;
 };
 
 static int domain_context_mapping_cb(struct pci_dev *pdev,

@@ -2047,25 +2049,31 @@ static int domain_context_mapping_cb(struct pci_dev *pdev,
 	struct domain_context_mapping_data *data = opaque;
 
 	return domain_context_mapping_one(data->domain, data->iommu,
-					  PCI_BUS_NUM(alias), alias & 0xff);
+					  data->table, PCI_BUS_NUM(alias),
+					  alias & 0xff);
 }
 
 static int
 domain_context_mapping(struct dmar_domain *domain, struct device *dev)
 {
+	struct domain_context_mapping_data data;
+	struct pasid_table *table;
 	struct intel_iommu *iommu;
 	u8 bus, devfn;
-	struct domain_context_mapping_data data;
 
 	iommu = device_to_iommu(dev, &bus, &devfn);
 	if (!iommu)
 		return -ENODEV;
 
+	table = intel_pasid_get_table(dev);
+
 	if (!dev_is_pci(dev))
-		return domain_context_mapping_one(domain, iommu, bus, devfn);
+		return domain_context_mapping_one(domain, iommu, table,
+						  bus, devfn);
 
 	data.domain = domain;
 	data.iommu = iommu;
+	data.table = table;
 
 	return pci_for_each_dma_alias(to_pci_dev(dev),
 				      &domain_context_mapping_cb, &data);

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 07:25 UTC #26 lore raw

RE: [PATCH v4 08/12] iommu/vt-d: Pass pasid table to context mapping

Hi Baolu,

Quoted 7 lines

From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 08/12] iommu/vt-d: Pass pasid table to context mapping
So that the pasid related info, such as the pasid table and the maximum of pasid could be used during setting up scalable mode context.

A little bit refine. Wish it helps. :)
"This patch passes the pasid related info(e.g. the pasid table and the maximum of pasid) to context mapping, so that pasid related fields can be setup accordingly in scalable mode context entry."

Regards,
Yi Liu

Lu Baolu

author

2018-11-08 02:34 UTC #27 lore raw

Re: [PATCH v4 08/12] iommu/vt-d: Pass pasid table to context mapping

Hi,

Quoted 6 lines · Yi L

Hi Baolu,
Quoted 4 lines
From: Lu Baolu [mailto:[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 08/12] iommu/vt-d: Pass pasid table to context mapping

Quoted 8 lines

Quoted 3 lines
So that the pasid related info, such as the pasid table and the maximum of pasid could be used during setting up scalable mode context.
A little bit refine. Wish it helps. :)
"This patch passes the pasid related info(e.g. the pasid table and the maximum of pasid) to context mapping, so that pasid related fields can be setup accordingly in scalable mode context entry."

Yeah, thanks.

Best regards,
Lu Baolu

Lu Baolu

author

2018-11-05 05:31 UTC #28 lore raw

PATCH v4 9/12 iommu/vt-d: Setup context and enable RID2PASID support

This patch enables the translation for requests without PASID in the scalable mode by setting up the root and context entries.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c | 109 ++++++++++++++++++++++++++++++------
 drivers/iommu/intel-pasid.h |   1 +
 include/linux/intel-iommu.h |   1 +
 3 files changed, 95 insertions(+), 16 deletions(-)

drivers/iommu/intel-iommu.c +93 ~~-16~~

index a761c0aa3438..9c5e0d5530a2 100644
@@ -1194,6 +1194,8 @@ static void iommu_set_root_entry(struct intel_iommu *iommu)
 	unsigned long flag;
 
 	addr = virt_to_phys(iommu->root_entry);
+	if (sm_supported(iommu))
+		addr |= DMA_RTADDR_SMT;
 
 	raw_spin_lock_irqsave(&iommu->register_lock, flag);
 	dmar_writeq(iommu->reg + DMAR_RTADDR_REG, addr);

@@ -1915,6 +1917,55 @@ static void domain_exit(struct dmar_domain *domain)
 	free_domain_mem(domain);
 }
 
+/*
+ * Get the PASID directory size for scalable mode context entry.
+ * Value of X in the PDTS field of a scalable mode context entry
+ * indicates PASID directory with 2^(X + 7) entries.
+ */
+static inline unsigned long context_get_sm_pds(struct pasid_table *table)
+{
+	int pds, max_pde;
+
+	max_pde = table->max_pasid >> PASID_PDE_SHIFT;
+	pds = find_first_bit((unsigned long *)&max_pde, MAX_NR_PASID_BITS);
+	if (pds < 7)
+		return 0;
+
+	return pds - 7;
+}
+
+/*
+ * Set the RID_PASID field of a scalable mode context entry. The
+ * IOMMU hardware will use the PASID value set in this field for
+ * DMA translations of DMA requests without PASID.
+ */
+static inline void
+context_set_sm_rid2pasid(struct context_entry *context, unsigned long pasid)
+{
+	context->hi |= pasid & ((1 << 20) - 1);
+}
+
+/*
+ * Set the DTE(Device-TLB Enable) field of a scalable mode context
+ * entry.
+ */
+static inline void context_set_sm_dte(struct context_entry *context)
+{
+	context->lo |= (1 << 2);
+}
+
+/*
+ * Set the PRE(Page Request Enable) field of a scalable mode context
+ * entry.
+ */
+static inline void context_set_sm_pre(struct context_entry *context)
+{
+	context->lo |= (1 << 4);
+}
+
+/* Convert value to context PASID directory size field coding. */
+#define context_pdts(pds)	(((pds) & 0x7) << 9)
+
 static int domain_context_mapping_one(struct dmar_domain *domain,
 				      struct intel_iommu *iommu,
 				      struct pasid_table *table,

@@ -1973,9 +2024,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 	}
 
 	pgd = domain->pgd;
-
 	context_clear_entry(context);
-	context_set_domain_id(context, did);
 
 	/*
 	 * Skip top levels of page tables for iommu which has less agaw

@@ -1988,25 +2037,54 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 			if (!dma_pte_present(pgd))
 				goto out_unlock;
 		}
+	}
 
-		info = iommu_support_dev_iotlb(domain, iommu, bus, devfn);
-		if (info && info->ats_supported)
-			translation = CONTEXT_TT_DEV_IOTLB;
-		else
-			translation = CONTEXT_TT_MULTI_LEVEL;
+	if (sm_supported(iommu)) {
+		unsigned long pds;
+
+		WARN_ON(!table);
+
+		/* Setup the PASID DIR pointer: */
+		pds = context_get_sm_pds(table);
+		context->lo = (u64)virt_to_phys(table->table) |
+				context_pdts(pds);
+
+		/* Setup the RID_PASID field: */
+		context_set_sm_rid2pasid(context, PASID_RID2PASID);
 
-		context_set_address_root(context, virt_to_phys(pgd));
-		context_set_address_width(context, iommu->agaw);
-	} else {
 		/*
-		 * In pass through mode, AW must be programmed to
-		 * indicate the largest AGAW value supported by
-		 * hardware. And ASR is ignored by hardware.
+		 * Setup the Device-TLB enable bit and Page request
+		 * Enable bit:
 		 */
-		context_set_address_width(context, iommu->msagaw);
+		info = iommu_support_dev_iotlb(domain, iommu, bus, devfn);
+		if (info && info->ats_supported)
+			context_set_sm_dte(context);
+		if (info && info->pri_supported)
+			context_set_sm_pre(context);
+	} else {
+		context_set_domain_id(context, did);
+
+		if (translation != CONTEXT_TT_PASS_THROUGH) {
+			info = iommu_support_dev_iotlb(domain, iommu,
+						       bus, devfn);
+			if (info && info->ats_supported)
+				translation = CONTEXT_TT_DEV_IOTLB;
+			else
+				translation = CONTEXT_TT_MULTI_LEVEL;
+
+			context_set_address_root(context, virt_to_phys(pgd));
+			context_set_address_width(context, iommu->agaw);
+		} else {
+			/*
+			 * In pass through mode, AW must be programmed to
+			 * indicate the largest AGAW value supported by
+			 * hardware. And ASR is ignored by hardware.
+			 */
+			context_set_address_width(context, iommu->msagaw);
+		}
+		context_set_translation_type(context, translation);
 	}
 
-	context_set_translation_type(context, translation);
 	context_set_fault_enable(context);
 	context_set_present(context);
 	domain_flush_cache(domain, context, sizeof(*context));

@@ -5175,7 +5253,6 @@ 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 device *dev)
 {
 	int pts, max_pasid;

drivers/iommu/intel-pasid.h +1 -0

index d6f4fead4491..55bb8715329d 100644
@@ -17,6 +17,7 @@
 #define PASID_PTE_PRESENT		1
 #define PDE_PFN_MASK			PAGE_MASK
 #define PASID_PDE_SHIFT			6
+#define MAX_NR_PASID_BITS		20
 
 /*
  * Domain ID reserved for pasid entries programmed for first-level

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

index d83908420a83..0bff03309822 100644
@@ -232,6 +232,7 @@
 
 /* DMA_RTADDR_REG */
 #define DMA_RTADDR_RTT (((u64)1) << 11)
+#define DMA_RTADDR_SMT (((u64)1) << 10)
 
 /* CCMD_REG */
 #define DMA_CCMD_ICC (((u64)1) << 63)

Signature · 1 line

2.17.1

Lu Baolu

author

2018-11-05 05:31 UTC #29 lore raw

PATCH v4 10/12 iommu/vt-d: Add first level page table interface

This adds an interface to setup the PASID entries for first level page table translation.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-pasid.c | 81 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h | 11 +++++
 include/linux/intel-iommu.h |  1 +
 3 files changed, 93 insertions(+)

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

index 69530317c323..d8ca1e6a8e5e 100644
@@ -10,6 +10,7 @@
 #define pr_fmt(fmt)	"DMAR: " fmt
 
 #include <linux/bitops.h>
+#include <linux/cpufeature.h>
 #include <linux/dmar.h>
 #include <linux/intel-iommu.h>
 #include <linux/iommu.h>

@@ -388,6 +389,26 @@ static inline void pasid_set_page_snoop(struct pasid_entry *pe, bool value)
 	pasid_set_bits(&pe->val[1], 1 << 23, value);
 }
 
+/*
+ * Setup the First Level Page table Pointer field (Bit 140~191)
+ * of a scalable mode PASID entry.
+ */
+static inline void
+pasid_set_flptr(struct pasid_entry *pe, u64 value)
+{
+	pasid_set_bits(&pe->val[2], VTD_PAGE_MASK, value);
+}
+
+/*
+ * Setup the First Level Paging Mode field (Bit 130~131) of a
+ * scalable mode PASID entry.
+ */
+static inline void
+pasid_set_flpm(struct pasid_entry *pe, u64 value)
+{
+	pasid_set_bits(&pe->val[2], GENMASK_ULL(3, 2), value << 2);
+}
+
 static void
 pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,
 				    u16 did, int pasid)

@@ -458,6 +479,66 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu,
 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
 }
 
+/*
+ * Set up the scalable mode pasid table entry for first only
+ * translation type.
+ */
+int intel_pasid_setup_first_level(struct intel_iommu *iommu,
+				  struct device *dev, pgd_t *pgd,
+				  int pasid, int flags)
+{
+	u16 did = FLPT_DEFAULT_DID;
+	struct pasid_entry *pte;
+
+	if (!ecap_flts(iommu->ecap)) {
+		pr_err("No first level translation support on %s\n",
+		       iommu->name);
+		return -EINVAL;
+	}
+
+	pte = intel_pasid_get_entry(dev, pasid);
+	if (WARN_ON(!pte))
+		return -EINVAL;
+
+	pasid_clear_entry(pte);
+
+	/* Setup the first level page table pointer: */
+	pasid_set_flptr(pte, (u64)__pa(pgd));
+	if (flags & PASID_FLAG_SUPERVISOR_MODE) {
+		if (!ecap_srs(iommu->ecap)) {
+			pr_err("No supervisor request support on %s\n",
+			       iommu->name);
+			return -EINVAL;
+		}
+		pasid_set_sre(pte);
+	}
+
+#ifdef CONFIG_X86
+	if (cpu_feature_enabled(X86_FEATURE_LA57))
+		pasid_set_flpm(pte, 1);
+#endif /* CONFIG_X86 */
+
+	pasid_set_domain_id(pte, did);
+	pasid_set_address_width(pte, iommu->agaw);
+	pasid_set_page_snoop(pte, !!ecap_smpwc(iommu->ecap));
+
+	/* Setup Present and PASID Granular Transfer Type: */
+	pasid_set_translation_type(pte, 1);
+	pasid_set_present(pte);
+
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(pte, sizeof(*pte));
+
+	if (cap_caching_mode(iommu->cap)) {
+		pasid_cache_invalidation_with_pasid(iommu, did, pasid);
+		iotlb_invalidation_with_pasid(iommu, did, pasid);
+	} else {
+		iommu_flush_write_buffer(iommu);
+	}
+
+	return 0;
+}
+
 /*
  * Set up the scalable mode pasid entry for second only translation type.
  */

drivers/iommu/intel-pasid.h +11 -0

index 55bb8715329d..80db18441b1d 100644
@@ -25,6 +25,14 @@
  */
 #define FLPT_DEFAULT_DID		1
 
+/*
+ * The SUPERVISOR_MODE flag indicates a first level translation which
+ * can be used for access to kernel addresses. It is valid only for
+ * access to the kernel's static 1:1 mapping of physical memory — not
+ * to vmalloc or even module mappings.
+ */
+#define PASID_FLAG_SUPERVISOR_MODE	BIT(0)
+
 struct pasid_dir_entry {
 	u64 val;
 };

@@ -51,6 +59,9 @@ 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);
+int intel_pasid_setup_first_level(struct intel_iommu *iommu,
+				  struct device *dev, pgd_t *pgd,
+				  int pasid, int flags);
 int intel_pasid_setup_second_level(struct intel_iommu *iommu,
 				   struct dmar_domain *domain,
 				   struct device *dev, int pasid);

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

index 0bff03309822..b3f7cabdf6d7 100644
@@ -152,6 +152,7 @@
  */
 
 #define ecap_smpwc(e)		(((e) >> 48) & 0x1)
+#define ecap_flts(e)		(((e) >> 47) & 0x1)
 #define ecap_slts(e)		(((e) >> 46) & 0x1)
 #define ecap_smts(e)		(((e) >> 43) & 0x1)
 #define ecap_dit(e)		((e >> 41) & 0x1)

Signature · 1 line

2.17.1

Liu, Yi L

2018-11-07 07:34 UTC #30 lore raw

RE: [PATCH v4 10/12] iommu/vt-d: Add first level page table interface

Hi Baolu,

Quoted 5 lines

From: Lu Baolu [mailto:baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]]
Sent: Monday, November 5, 2018 1:32 PM
Subject: [PATCH v4 10/12] iommu/vt-d: Add first level page table interface
This adds an interface to setup the PASID entries for first

This patch adds interface to setup the PASID entries for first. :)

Quoted 60 lines

level page table translation.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <jacob.jun.pan-VuQAYsv1563Yd54FQh9/[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <baolu.lu-VuQAYsv1563Yd54FQh9/[email protected]>

---
 drivers/iommu/intel-pasid.c | 81 +++++++++++++++++++++++++++++++++++++
 drivers/iommu/intel-pasid.h | 11 +++++
 include/linux/intel-iommu.h |  1 +
 3 files changed, 93 insertions(+)

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

index 69530317c323..d8ca1e6a8e5e 100644
@@ -10,6 +10,7 @@
 #define pr_fmt(fmt)	"DMAR: " fmt
 
 #include <linux/bitops.h>
+#include <linux/cpufeature.h>
 #include <linux/dmar.h>
 #include <linux/intel-iommu.h>
 #include <linux/iommu.h>

@@ -388,6 +389,26 @@ static inline void pasid_set_page_snoop(struct pasid_entry
 *pe, bool value)
 	pasid_set_bits(&pe->val[1], 1 << 23, value);
 }
 
+/*
+ * Setup the First Level Page table Pointer field (Bit 140~191)
+ * of a scalable mode PASID entry.
+ */
+static inline void
+pasid_set_flptr(struct pasid_entry *pe, u64 value)
+{
+	pasid_set_bits(&pe->val[2], VTD_PAGE_MASK, value);
+}
+
+/*
+ * Setup the First Level Paging Mode field (Bit 130~131) of a
+ * scalable mode PASID entry.
+ */
+static inline void
+pasid_set_flpm(struct pasid_entry *pe, u64 value)
+{
+	pasid_set_bits(&pe->val[2], GENMASK_ULL(3, 2), value << 2);
+}
+
 static void
 pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,

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

 				    u16 did, int pasid)

@@ -458,6 +479,66 @@ void intel_pasid_tear_down_entry(struct intel_iommu
 *iommu,
 		devtlb_invalidation_with_pasid(iommu, dev, pasid);
 }

drivers/iommu/intel-pasid.c void intel_pasid_tear_down_entry(struct intel_iommu · line 492

Quoted 10 lines

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

+/*
+ * Set up the scalable mode pasid table entry for first only
+ * translation type.
+ */
+int intel_pasid_setup_first_level(struct intel_iommu *iommu,
+				  struct device *dev, pgd_t *pgd,
+				  int pasid, int flags)
+{
+	u16 did = FLPT_DEFAULT_DID;
+	struct pasid_entry *pte;

aha, same comment with previous patch. may be better us pt_entry or pasid_entry.

Regards,
Yi Liu

Lu Baolu

author

2018-11-05 05:31 UTC #31 lore raw

PATCH v4 11/12 iommu/vt-d: Shared virtual address in scalable mode

This patch enables the current SVA (Shared Virtual Address) implementation to work in the scalable mode.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>

---
 drivers/iommu/intel-iommu.c   | 38 ------------------------
 drivers/iommu/intel-pasid.c   |  2 +-
 drivers/iommu/intel-pasid.h   |  1 -
 drivers/iommu/intel-svm.c     | 55 ++++++++++-------------------------
 include/linux/dma_remapping.h |  9 +-----
 5 files changed, 17 insertions(+), 88 deletions(-)

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

index 9c5e0d5530a2..3c3a882964a0 100644
@@ -5253,18 +5253,6 @@ static void intel_iommu_put_resv_regions(struct device *dev,
 }
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
-static inline unsigned long intel_iommu_get_pts(struct device *dev)
-{
-	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;
-
-	return pts - 5;
-}
-
 int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sdev)
 {
 	struct device_domain_info *info;

@@ -5296,33 +5284,7 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct intel_svm_dev *sd
 	sdev->sid = PCI_DEVID(info->bus, info->devfn);
 
 	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(info->pasid_table->table) |
-			intel_iommu_get_pts(sdev->dev);
-
-		wmb();
-		/* CONTEXT_TT_MULTI_LEVEL and CONTEXT_TT_DEV_IOTLB are both
-		 * extended to permit requests-with-PASID if the PASIDE bit
-		 * is set. which makes sense. For CONTEXT_TT_PASS_THROUGH,
-		 * however, the PASIDE bit is ignored and requests-with-PASID
-		 * are unconditionally blocked. Which makes less sense.
-		 * So convert from CONTEXT_TT_PASS_THROUGH to one of the new
-		 * "guest mode" translation types depending on whether ATS
-		 * is available or not. Annoyingly, we can't use the new
-		 * modes *unless* PASIDE is set. */
-		if ((ctx_lo & CONTEXT_TT_MASK) == (CONTEXT_TT_PASS_THROUGH << 2)) {
-			ctx_lo &= ~CONTEXT_TT_MASK;
-			if (info->ats_supported)
-				ctx_lo |= CONTEXT_TT_PT_PASID_DEV_IOTLB << 2;
-			else
-				ctx_lo |= CONTEXT_TT_PT_PASID << 2;
-		}
 		ctx_lo |= CONTEXT_PASIDE;
-		if (iommu->pasid_state_table)
-			ctx_lo |= CONTEXT_DINVE;
-		if (info->pri_supported)
-			ctx_lo |= CONTEXT_PRS;
 		context[0].lo = ctx_lo;
 		wmb();
 		iommu->flush.flush_context(iommu, sdev->did, sdev->sid,

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

index d8ca1e6a8e5e..839f83974aca 100644
@@ -285,7 +285,7 @@ static inline void pasid_clear_entry(struct pasid_entry *pe)
 	WRITE_ONCE(pe->val[7], 0);
 }
 
-void intel_pasid_clear_entry(struct device *dev, int pasid)
+static void intel_pasid_clear_entry(struct device *dev, int pasid)
 {
 	struct pasid_entry *pe;

drivers/iommu/intel-pasid.h +0 -1

index 80db18441b1d..381545ff9fb7 100644
@@ -58,7 +58,6 @@ 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);
 int intel_pasid_setup_first_level(struct intel_iommu *iommu,
 				  struct device *dev, pgd_t *pgd,
 				  int pasid, int flags);

drivers/iommu/intel-svm.c +15 ~~-40~~

96 lines not on this page · raw message

include/linux/dma_remapping.h +1 -8

index 21b3e7d33d68..6f01e54702e5 100644
@@ -21,14 +21,7 @@
 #define CONTEXT_TT_MULTI_LEVEL	0
 #define CONTEXT_TT_DEV_IOTLB	1
 #define CONTEXT_TT_PASS_THROUGH 2
-/* Extended context entry types */
-#define CONTEXT_TT_PT_PASID	4
-#define CONTEXT_TT_PT_PASID_DEV_IOTLB 5
-#define CONTEXT_TT_MASK (7ULL << 2)
-
-#define CONTEXT_DINVE		(1ULL << 8)
-#define CONTEXT_PRS		(1ULL << 9)
-#define CONTEXT_PASIDE		(1ULL << 11)
+#define CONTEXT_PASIDE		BIT_ULL(3)
 
 struct intel_iommu;
 struct dmar_domain;

Signature · 1 line

2.17.1

Christoph Hellwig

2018-11-05 07:15 UTC #32 lore raw

Re: [PATCH v4 11/12] iommu/vt-d: Shared virtual address in scalable mode

include/linux/dma_remapping.h

Quoted 2 lines

include/linux/dma_remapping.h +0 -0

FYI, this file seems to be entirely VT-D specific, but has an awfully generic name. It should be renamed to something more suitable (or maybe just merged into intel-iommu.h?) and also grow some comments describing the contents.

Lu Baolu

author

2018-11-05 07:17 UTC #33 lore raw

Re: [PATCH v4 11/12] iommu/vt-d: Shared virtual address in scalable mode

Hi,

Quoted 8 lines · Christoph Hellwig

include/linux/dma_remapping.h
Quoted 2 lines
include/linux/dma_remapping.h +0 -0
FYI, this file seems to be entirely VT-D specific, but has an awfully generic name. It should be renamed to something more suitable (or maybe just merged into intel-iommu.h?) and also grow some comments describing the contents.

Yes, good catch. I will do this in a separated patch.

Best regards,
Lu Baolu

Lu Baolu

author

2018-11-05 05:31 UTC #34 lore raw

PATCH v4 12/12 iommu/vt-d: Remove deferred invalidation

Deferred invalidation is an ECS specific feature. It will not be supported when IOMMU works in scalable mode. As we deprecated the ECS support, remove deferred invalidation and cleanup the code.

Cc Ashok Raj <[email protected]>

Cc Jacob Pan <[email protected]>

Cc Kevin Tian <[email protected]>

Cc Liu Yi L <[email protected]>

Signed-off-by Lu Baolu <[email protected]>