More Sanity checks for DMAR

PATCH In review Fenghua Yu 5 participants 14 messages Nov 19, 2007 – Aug 5, 2009 2 messages not in the archive

Subsystems (4)

Share & tools

A message that is not in the archive (sent off-list or not yet received): BAE9DCEF64577A439B3A37F36F9B691C0339372B@orsmsx418.amr.corp.intel.com

Fenghua Yu

author

2007-11-19 23:06 UTC #1 lore raw

PATCH 1/1 More Sanity checks for DMAR

This patch adds and changes a few sanity checks in dmar.c.

1. The haw field in ACPI DMAR table in VT-d spec doesn't describe the range of haw. But since DMA page size is 4KB in DMA remapping, haw should be at least 4KB. The current VT-d code in dmar.c returns failure when haw==0. This sanity check is not accurate and execution can pass when haw is less than one page size 4KB. This patch changes the haw sanity check to validate if haw is less than 4KB.
2. Add dmar_rmrr_units verification.
3. Add parse_dmar_table() verification.

Thanks.

-Fenghua

Signed-off-by Fenghua Yu <[email protected]>

---

 dmar.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)

drivers/pci/dmar.c +17 -3

index 5dfdfda..a5b6ed0 100644
@@ -25,6 +25,7 @@
 
 #include <linux/pci.h>
 #include <linux/dmar.h>
+#include "iova.h"
 
 #undef PREFIX
 #define PREFIX "DMAR:"

@@ -263,8 +264,8 @@ parse_dmar_table(void)
 	if (!dmar)
 		return -ENODEV;
 
-	if (!dmar->width) {
-		printk (KERN_WARNING PREFIX "Zero: Invalid DMAR haw\n");
+	if (dmar->width < PAGE_SHIFT_4K - 1) {
+		printk (KERN_WARNING PREFIX "Invalid DMAR haw\n");
 		return -EINVAL;
 	}

@@ -301,11 +302,24 @@ parse_dmar_table(void)
 int __init dmar_table_init(void)
 {
 
-	parse_dmar_table();
+	int ret;
+	
+	ret = parse_dmar_table();
+	if (ret) {
+		printk(KERN_INFO PREFIX "parse DMAR table failure.\n");
+		return ret;
+	}
+
 	if (list_empty(&dmar_drhd_units)) {
 		printk(KERN_INFO PREFIX "No DMAR devices found\n");
 		return -ENODEV;
 	}
+
+	if (list_empty(&dmar_rmrr_units)) {
+		printk(KERN_INFO PREFIX "No RMRR found\n");
+		return -ENODEV;
+	}
+
 	return 0;
 }

mark gross

2007-11-20 19:02 UTC #2 lore raw

Quoted 63 lines · Fenghua Yu

This patch adds and changes a few sanity checks in dmar.c.
1. The haw field in ACPI DMAR table in VT-d spec doesn't describe the range of haw. But since DMA page size is 4KB in DMA remapping, haw should be at least 4KB. The current VT-d code in dmar.c returns failure when haw==0. This sanity check is not accurate and execution can pass when haw is less than one page size 4KB. This patch changes the haw sanity check to validate if haw is less than 4KB.
2. Add dmar_rmrr_units verification.
3. Add parse_dmar_table() verification.
Thanks.
-Fenghua
Signed-off-by Fenghua Yu <[email protected]>
---
 dmar.c |   20 +++++++++++++++++---
 1 files changed, 17 insertions(+), 3 deletions(-)
drivers/pci/dmar.c +11 -3
index 5dfdfda..a5b6ed0 100644
@@ -25,6 +25,7 @@
 
 #include <linux/pci.h>
 #include <linux/dmar.h>
+#include "iova.h"
 
 #undef PREFIX
 #define PREFIX "DMAR:"
@@ -263,8 +264,8 @@ parse_dmar_table(void)
 	if (!dmar)
 		return -ENODEV;
 
-	if (!dmar->width) {
-		printk (KERN_WARNING PREFIX "Zero: Invalid DMAR haw\n");
+	if (dmar->width < PAGE_SHIFT_4K - 1) {
+		printk (KERN_WARNING PREFIX "Invalid DMAR haw\n");
 		return -EINVAL;
 	}
 
@@ -301,11 +302,24 @@ parse_dmar_table(void)
 int __init dmar_table_init(void)
 {
 
-	parse_dmar_table();
+	int ret;
+	
+	ret = parse_dmar_table();
+	if (ret) {
+		printk(KERN_INFO PREFIX "parse DMAR table failure.\n");
+		return ret;
+	}
+
 	if (list_empty(&dmar_drhd_units)) {
 		printk(KERN_INFO PREFIX "No DMAR devices found\n");
 		return -ENODEV;
 	}

drivers/pci/dmar.c parse_dmar_table(void)

Quoted 10 lines

drivers/pci/dmar.c +6 -1

+
+	if (list_empty(&dmar_rmrr_units)) {
+		printk(KERN_INFO PREFIX "No RMRR found\n");
+		return -ENODEV;
+	}
+
 	return 0;
 }
 
-

Ack : mark gross <[email protected]>

Thanks,

--mgross

A message that is not in the archive (sent off-list or not yet received): 617E1C2C70743745A92448908E030B2A02AD5AD7@scsmsx411.amr.corp.intel.com

Fenghua Yu

author

2008-05-02 22:26 UTC #4 lore raw

PATCH 1/1 Handle invalid ACPI SLIT table

This is a SLIT sanity checking patch. It moves slit_valid() function to generic ACPI code and does sanity checking for both x86 and ia64. It sets up node_distance with LOCAL_DISTANCE and REMOTE_DISTANCE when hitting invalid SLIT table on ia64. It also cleans up unused variable localities in acpi_parse_slit() on x86.

Signed-off-by Fenghua Yu <[email protected]>

---

 arch/ia64/kernel/acpi.c |    9 +++++++--
 arch/x86/mm/srat_64.c   |   27 ---------------------------
 drivers/acpi/numa.c     |   31 +++++++++++++++++++++++++++----
 3 files changed, 34 insertions(+), 33 deletions(-)

arch/ia64/kernel/acpi.c +7 -2

index c7467f8..4f156f4 100644
@@ -460,7 +460,6 @@ void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 		printk(KERN_ERR
 		       "ACPI 2.0 SLIT: size mismatch: %d expected, %d actual\n",
 		       len, slit->header.length);
-		memset(numa_slit, 10, sizeof(numa_slit));
 		return;
 	}
 	slit_table = slit;

@@ -569,8 +568,14 @@ void __init acpi_numa_arch_fixup(void)
 	printk(KERN_INFO "Number of memory chunks in system = %d\n",
 	       num_node_memblks);
 
-	if (!slit_table)
+	if (!slit_table) {
+		for (i = 0; i < MAX_NUMNODES; i++)
+			for (j = 0; j < MAX_NUMNODES; j++)
+				node_distance(i, j) = i = j ? LOCAL_DISTANCE :
+							REMOTE_DISTANCE;
 		return;
+	}
+
 	memset(numa_slit, -1, sizeof(numa_slit));
 	for (i = 0; i < slit_table->locality_count; i++) {
 		if (!pxm_bit_test(i))

arch/x86/mm/srat_64.c +0 ~~-27~~

index 3890234..99649dc 100644
@@ -97,36 +97,9 @@ static __init inline int srat_disabled(void)
 	return numa_off || acpi_numa < 0;
 }
 
-/*
- * A lot of BIOS fill in 10 (= no distance) everywhere. This messes
- * up the NUMA heuristics which wants the local node to have a smaller
- * distance than the others.
- * Do some quick checks here and only use the SLIT if it passes.
- */
-static __init int slit_valid(struct acpi_table_slit *slit)
-{
-	int i, j;
-	int d = slit->locality_count;
-	for (i = 0; i < d; i++) {
-		for (j = 0; j < d; j++)  {
-			u8 val = slit->entry[d*i + j];
-			if (i = j) {
-				if (val != LOCAL_DISTANCE)
-					return 0;
-			} else if (val <= LOCAL_DISTANCE)
-				return 0;
-		}
-	}
-	return 1;
-}
-
 /* Callback for SLIT parsing */
 void __init acpi_numa_slit_init(struct acpi_table_slit *slit)
 {
-	if (!slit_valid(slit)) {
-		printk(KERN_INFO "ACPI: SLIT table looks invalid. Not used.\n");
-		return;
-	}
 	acpi_slit = slit;
 }

drivers/acpi/numa.c +27 -4

index 5d59cb3..658e5f3 100644
@@ -140,19 +140,42 @@ acpi_table_print_srat_entry(struct acpi_subtable_header *header)
 	}
 }
 
+/*
+ * A lot of BIOS fill in 10 (= no distance) everywhere. This messes
+ * up the NUMA heuristics which wants the local node to have a smaller
+ * distance than the others.
+ * Do some quick checks here and only use the SLIT if it passes.
+ */
+static __init int slit_valid(struct acpi_table_slit *slit)
+{
+	int i, j;
+	int d = slit->locality_count;
+	for (i = 0; i < d; i++) {
+		for (j = 0; j < d; j++)  {
+			u8 val = slit->entry[d*i + j];
+			if (i = j) {
+				if (val != LOCAL_DISTANCE)
+					return 0;
+			} else if (val <= LOCAL_DISTANCE)
+				return 0;
+		}
+	}
+	return 1;
+}
+
 static int __init acpi_parse_slit(struct acpi_table_header *table)
 {
 	struct acpi_table_slit *slit;
-	u32 localities;
 
 	if (!table)
 		return -EINVAL;
 
 	slit = (struct acpi_table_slit *)table;
 
-	/* downcast just for %llu vs %lu for i386/ia64  */
-	localities = (u32) slit->locality_count;
-
+	if (!slit_valid(slit)) {
+		printk(KERN_INFO "ACPI: SLIT table looks invalid. Not used.\n");
+		return -EINVAL;
+	}
 	acpi_numa_slit_init(slit);
 
 	return 0;

Fenghua Yu

author

2008-10-01 16:57 UTC #5 lore raw

PATCH 1/2 Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

In reply to a message not in the archive

The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes.

This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, .

Signed-off-by Fenghua Yu <[email protected]>

Signed-off-by Tony Luck <[email protected]>

---

 drivers/pci/dmar.c            |   23 ++++--
 drivers/pci/intel-iommu.c     |  139 ++++++++++++++++++++++++------------------
 include/linux/dma_remapping.h |   28 ++++----
 include/linux/intel-iommu.h   |   49 ++++++++++----
 4 files changed, 145 insertions(+), 94 deletions(-)

drivers/pci/dmar.c +15 -8

index 0c92e2c..31290c2 100644
@@ -35,6 +35,10 @@
 #undef PREFIX
 #define PREFIX "DMAR:"
 
+#ifdef CONFIG_IA64
+#define cpu_has_x2apic 0
+#endif
+
 /* No locks are needed as DMA remapping hardware unit
  * list is constructed at boot time and hotplug of
  * these units are not supported by the architecture.

@@ -277,14 +280,15 @@ dmar_table_print_dmar_entry(struct acpi_dmar_header *header)
 		drhd = (struct acpi_dmar_hardware_unit *)header;
 		printk (KERN_INFO PREFIX
 			"DRHD (flags: 0x%08x)base: 0x%016Lx\n",
-			drhd->flags, drhd->address);
+			drhd->flags, (unsigned long long)drhd->address);
 		break;
 	case ACPI_DMAR_TYPE_RESERVED_MEMORY:
 		rmrr = (struct acpi_dmar_reserved_memory *)header;
 
 		printk (KERN_INFO PREFIX
 			"RMRR base: 0x%016Lx end: 0x%016Lx\n",
-			rmrr->base_address, rmrr->end_address);
+			(unsigned long long)rmrr->base_address,
+			(unsigned long long)rmrr->end_address);
 		break;
 	}
 }

@@ -328,7 +332,7 @@ parse_dmar_table(void)
 	if (!dmar)
 		return -ENODEV;
 
-	if (dmar->width < PAGE_SHIFT_4K - 1) {
+	if (dmar->width < PAGE_SHIFT - 1) {
 		printk(KERN_WARNING PREFIX "Invalid DMAR haw\n");
 		return -EINVAL;
 	}

@@ -510,7 +514,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 	iommu->seq_id = iommu_allocated++;
 
-	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
+	iommu->reg = ioremap(drhd->reg_base_addr, IOMMU_PAGE_SIZE);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");
 		goto error;

@@ -521,8 +525,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	/* the registers might be more than one page */
 	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
 		cap_max_fault_reg_offset(iommu->cap));
-	map_size = PAGE_ALIGN_4K(map_size);
-	if (map_size > PAGE_SIZE_4K) {
+	map_size = IOMMU_PAGE_ALIGN(map_size);
+	if (map_size > IOMMU_PAGE_SIZE) {
 		iounmap(iommu->reg);
 		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
 		if (!iommu->reg) {

@@ -533,8 +537,10 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
 	pr_debug("IOMMU %llx: ver %d:%d cap %llx ecap %llx\n",
-		drhd->reg_base_addr, DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
-		iommu->cap, iommu->ecap);
+		(unsigned long long)drhd->reg_base_addr,
+		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
+		(unsigned long long)iommu->cap,
+		(unsigned long long)iommu->ecap);
 
 	spin_lock_init(&iommu->register_lock);

drivers/pci/intel-iommu.c +81 ~~-58~~

428 lines not on this page · raw message

include/linux/dma_remapping.h +14 ~~-14~~

index bff5c65..218598d 100644
@@ -2,15 +2,15 @@
 #define _DMA_REMAPPING_H
 
 /*
- * We need a fixed PAGE_SIZE of 4K irrespective of
- * arch PAGE_SIZE for IOMMU page tables.
+ * VT-d hardware uses 4K page size regardless host page size.
  */
-#define PAGE_SHIFT_4K		(12)
-#define PAGE_SIZE_4K		(1UL << PAGE_SHIFT_4K)
-#define PAGE_MASK_4K		(((u64)-1) << PAGE_SHIFT_4K)
-#define PAGE_ALIGN_4K(addr)	(((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
+#define IOMMU_PAGE_SHIFT	(12)
+#define IOMMU_PAGE_SIZE		(1UL << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_MASK		(((u64)-1) << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_ALIGN(addr)	\
+	(((addr) + IOMMU_PAGE_SIZE - 1) & IOMMU_PAGE_MASK)
 
-#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT_4K)
+#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
 #define DMA_32BIT_PFN		IOVA_PFN(DMA_32BIT_MASK)
 #define DMA_64BIT_PFN		IOVA_PFN(DMA_64BIT_MASK)

@@ -25,7 +36,7 @@ struct root_entry {
 	u64	val;
 	u64	rsvd1;
 };
-#define ROOT_ENTRY_NR (PAGE_SIZE_4K/sizeof(struct root_entry))
+#define ROOT_ENTRY_NR (IOMMU_PAGE_SIZE/sizeof(struct root_entry))
 static inline bool root_present(struct root_entry *root)
 {
 	return (root->val & 1);

@@ -36,7 +47,7 @@ static inline void set_root_present(struct root_entry *root)
 }
 static inline void set_root_value(struct root_entry *root, unsigned long value)
 {
-	root->val |= value & PAGE_MASK_4K;
+	root->val |= value & IOMMU_PAGE_MASK;
 }
 
 struct context_entry;

@@ -45,7 +56,7 @@ get_context_addr_from_root(struct root_entry *root)
 {
 	return (struct context_entry *)
 		(root_present(root)?phys_to_virt(
-		root->val & PAGE_MASK_4K):
+		root->val & IOMMU_PAGE_MASK) :
 		NULL);
 }

@@ -67,7 +78,7 @@ struct context_entry {
 #define context_present(c) ((c).lo & 1)
 #define context_fault_disable(c) (((c).lo >> 1) & 1)
 #define context_translation_type(c) (((c).lo >> 2) & 3)
-#define context_address_root(c) ((c).lo & PAGE_MASK_4K)
+#define context_address_root(c) ((c).lo & IOMMU_PAGE_MASK)
 #define context_address_width(c) ((c).hi &  7)
 #define context_domain_id(c) (((c).hi >> 8) & ((1 << 16) - 1))

@@ -81,7 +92,7 @@ struct context_entry {
 	} while (0)
 #define CONTEXT_TT_MULTI_LEVEL 0
 #define context_set_address_root(c, val) \
-	do {(c).lo |= (val) & PAGE_MASK_4K;} while (0)
+	do {(c).lo |= (val) & IOMMU_PAGE_MASK; } while (0)
 #define context_set_address_width(c, val) do {(c).hi |= (val) & 7;} while (0)
 #define context_set_domain_id(c, val) \
 	do {(c).hi |= ((val) & ((1 << 16) - 1)) << 8;} while (0)

@@ -107,9 +118,9 @@ struct dma_pte {
 #define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;} while (0)
 #define dma_set_pte_prot(p, prot) \
 		do {(p).val = ((p).val & ~3) | ((prot) & 3); } while (0)
-#define dma_pte_addr(p) ((p).val & PAGE_MASK_4K)
+#define dma_pte_addr(p) ((p).val & IOMMU_PAGE_MASK)
 #define dma_set_pte_addr(p, addr) do {\
-		(p).val |= ((addr) & PAGE_MASK_4K); } while (0)
+		(p).val |= ((addr) & IOMMU_PAGE_MASK); } while (0)
 #define dma_pte_present(p) (((p).val & 3) != 0)
 
 struct intel_iommu;

include/linux/intel-iommu.h +35 ~~-14~~

73 lines not on this page · raw message

Ingo Molnar

2008-10-02 08:29 UTC #6 lore raw

Re: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

drivers/pci/dmar.c line 40

Quoted 9 lines · Fenghua Yu

drivers/pci/dmar.c +3 -0

@@ -35,6 +35,10 @@
 #undef PREFIX
 #define PREFIX "DMAR:"
 
+#ifdef CONFIG_IA64
+#define cpu_has_x2apic 0
+#endif

hm, that's not too nice - why not add it to arch/ia64/include/?

Ingo

Yu, Fenghua

author

2008-10-02 22:06 UTC #7 lore raw

RE: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

Quoted 7 lines

Quoted 7 lines
drivers/pci/dmar.c +0 -0
@@ -35,6 +35,10 @@
 #undef PREFIX
 #define PREFIX "DMAR:"
 

Quoted 5 lines

drivers/pci/dmar.c line 40
Quoted 3 lines
drivers/pci/dmar.c +3 -0
+#ifdef CONFIG_IA64
+#define cpu_has_x2apic 0
+#endif
hm, that's not too nice - why not add it to arch/ia64/include/?

OK. I'll move this to arch/ia64/include (along with other #ifdef CONFIG_IA64 places if needed).

Quoted 18 lines

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

index e7b196b..d84612a 100644
@@ -67,6 +67,13 @@
              hi = readl(dmar + reg + 4); \
              (((u64) hi) << 32) + lo; })
 */
+#ifdef CONFIG_IA64
+#define dmar_readq readq
+static inline void dmar_writeq(void __iomem *addr, u64 val)
+{
+     writeq(val, addr);
+}
+#else
 static inline u64 dmar_readq(void __iomem *addr)
 {
      u32 lo, hi;

Quoted 8 lines

include/linux/intel-iommu.h static inline void dmar_writeq(void __iomem *addr, u64 val) · line 90
Quoted 5 lines
include/linux/intel-iommu.h +1 -0
@@ -80,6 +87,7 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
      writel((u32)val, addr);
      writel((u32)(val >> 32), addr + 4);
 }
+#endif
What's this all about? Why do we need #ifdef CONFIG_IA64 here? Doesn't x86 provide its own readq/writeq implementation?

This is a comment from Bjorn.

In my patch, one readq/one writeq are working faster than two readl/two writel on IA64. X86 uses two readl/two writel so that the code works on both x86 and x86-64 although Intel IOMMU only has x86-64 version currently. dmar_readq() and dmar_writeq() are in moderate performance critical path.

Do you think my current implementation is ok to have #ifdef CONFIG_IA64 here? Or I can change X86 to use readq/writeq as well or IA64 uses two readl/two writel for clean code?

Thanks.

-Fenghua

Ingo Molnar

2008-10-03 08:54 UTC #8 lore raw

Re: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

Quoted 11 lines · Yu, Fenghua

Quoted 2 lines
What's this all about? Why do we need #ifdef CONFIG_IA64 here? Doesn't x86 provide its own readq/writeq implementation?
This is a comment from Bjorn.
In my patch, one readq/one writeq are working faster than two readl/two writel on IA64. X86 uses two readl/two writel so that the code works on both x86 and x86-64 although Intel IOMMU only has x86-64 version currently. dmar_readq() and dmar_writeq() are in moderate performance critical path.

Quoted 3 lines

Do you think my current implementation is ok to have #ifdef CONFIG_IA64 here? Or I can change X86 to use readq/writeq as well or IA64 uses two readl/two writel for clean code?

yes, clean code is very much preferred for a small detail like this.

Ingo

Bjorn Helgaas

2008-10-02 15:31 UTC #9 lore raw

Re: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

In reply to Fenghua Yu

Quoted 3 lines · Fenghua Yu

The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes.
This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, .

Can you split this patch up? It contains several logically separate changes:
- casting things to unsigned long long
- adding stuff under #ifdef CONFIG_IA64
- page size changes
- whitespace changes

drivers/pci/dmar.c int alloc_iommu(struct dmar_drhd_unit *drhd) · line 519

Quoted 8 lines

drivers/pci/dmar.c +1 -1

@@ -510,7 +514,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 	iommu->seq_id = iommu_allocated++;
 
-	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
+	iommu->reg = ioremap(drhd->reg_base_addr, IOMMU_PAGE_SIZE);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");

This printk should include a clue, like the IOMMU ID and/or address we tried to map.

Quoted 15 lines

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

+#ifdef CONFIG_IA64
+static inline void *ia64_get_zeroed_page(gfp_t gfp_mask)
+{
+	struct page *page;
+
+	page = alloc_pages(gfp_mask | __GFP_ZERO, 0);
+	if (page)
+		return (void *)page_address(page);
+	return 0;
+}
+#endif
 
 static inline void *alloc_pgtable_page(void)
 {

drivers/pci/intel-iommu.c static inline void *alloc_pgtable_page(void) · line 162

Quoted 8 lines

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

@@ -141,7 +155,11 @@ static inline void *alloc_pgtable_page(void)
 	/* trying to avoid low memory issues */
 	flags = current->flags & PF_MEMALLOC;
 	current->flags |= PF_MEMALLOC;
+#ifdef CONFIG_IA64
+	vaddr = ia64_get_zeroed_page(GFP_ATOMIC);
+#else
 	vaddr = (void *)get_zeroed_page(GFP_ATOMIC);
+#endif

Why does ia64 need a special case here?

drivers/pci/intel-iommu.c static int __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, · line 677

Quoted 7 lines

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

@@ -655,7 +673,8 @@ static int __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
 		printk(KERN_ERR"IOMMU: flush IOTLB failed\n");
 	if (DMA_TLB_IAIG(val) != DMA_TLB_IIRG(type))
 		pr_debug("IOMMU: tlb flush request %Lx, actual %Lx\n",
-			DMA_TLB_IIRG(type), DMA_TLB_IAIG(val));
+			(unsigned long long)DMA_TLB_IIRG(type),
+			(unsigned long long) DMA_TLB_IAIG(val));

These printks should include an IOMMU ID also (I assume a system can contain multiple IOMMUs).

drivers/pci/intel-iommu.c static int iommu_prepare_identity_map(struct pci_dev *pdev, u64 start, u64 end) · line 1521

Quoted 12 lines

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

@@ -1490,9 +1513,9 @@ static int iommu_prepare_identity_map(struct pci_dev *pdev, u64 start, u64 end)
 		return -ENOMEM;
 
 	/* The address might not be aligned */
-	base = start & PAGE_MASK_4K;
+	base = start & PAGE_MASK;
 	size = end - base;
-	size = PAGE_ALIGN_4K(size);
+	size = PAGE_ALIGN(size);
 	if (!reserve_iova(&domain->iovad, IOVA_PFN(base),
 			IOVA_PFN(base + size) - 1)) {
 		printk(KERN_ERR "IOMMU: reserve iova failed\n");

This should probably be a "dev_err(&pdev->dev," and include the IOMMU ID.

Quoted 19 lines

drivers/pci/intel-iommu.c +2 -6

@@ -1855,27 +1879,23 @@ intel_map_single(struct device *hwdev, phys_addr_t paddr, size_t size, int dir)
 	 * is not a big problem
 	 */
 	ret = domain_page_mapping(domain, start_paddr,
-		((u64)paddr) & PAGE_MASK_4K, size, prot);
+		((u64)paddr) & PAGE_MASK, size, prot);
 	if (ret)
 		goto error;
 
-	pr_debug("Device %s request: %lx@%llx mapping: %lx@%llx, dir %d\n",
-		pci_name(pdev), size, (u64)paddr,
-		size, (u64)start_paddr, dir);
-
 	/* it's a non-present to present mapping */
 	ret = iommu_flush_iotlb_psi(domain->iommu, domain->id,
-			start_paddr, size >> PAGE_SHIFT_4K, 1);
+			start_paddr, size >> IOMMU_PAGE_SHIFT, 1);
 	if (ret)
 		iommu_flush_write_buffer(domain->iommu);

drivers/pci/intel-iommu.c intel_map_single(struct device *hwdev, phys_addr_t paddr, size_t size, int dir) · line 1898

Quoted 10 lines

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

 
-	return (start_paddr + ((u64)paddr & (~PAGE_MASK_4K)));
+	return start_paddr + ((u64)paddr & (~PAGE_MASK));
 
 error:
 	if (iova)
 		__free_iova(&domain->iovad, iova);
 	printk(KERN_ERR"Device %s request: %lx@%llx dir %d --- failed\n",
-		pci_name(pdev), size, (u64)paddr, dir);
+		pci_name(pdev), size, (unsigned long long)paddr, dir);

Use dev_err() here and include IOMMU ID.

drivers/pci/intel-iommu.c static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr, · line 1981

Quoted 11 lines

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

@@ -1953,11 +1974,11 @@ static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr,
 	if (!iova)
 		return;
 
-	start_addr = iova->pfn_lo << PAGE_SHIFT_4K;
+	start_addr = iova->pfn_lo << PAGE_SHIFT;
 	size = aligned_size((u64)dev_addr, size);
 
 	pr_debug("Device %s unmapping: %lx@%llx\n",
-		pci_name(pdev), size, (u64)start_addr);
+		pci_name(pdev), size, (unsigned long long)start_addr);

Use dev_dbg() here.

Quoted 14 lines

include/linux/dma_remapping.h +1 -4

index bff5c65..218598d 100644
@@ -2,15 +2,15 @@
 #define _DMA_REMAPPING_H
 
 /*
- * We need a fixed PAGE_SIZE of 4K irrespective of
- * arch PAGE_SIZE for IOMMU page tables.
+ * VT-d hardware uses 4K page size regardless host page size.
  */
-#define PAGE_SHIFT_4K		(12)
-#define PAGE_SIZE_4K		(1UL << PAGE_SHIFT_4K)

include/linux/dma_remapping.h line 13

Quoted 10 lines

include/linux/dma_remapping.h +6 -3

-#define PAGE_MASK_4K		(((u64)-1) << PAGE_SHIFT_4K)
-#define PAGE_ALIGN_4K(addr)	(((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
+#define IOMMU_PAGE_SHIFT	(12)
+#define IOMMU_PAGE_SIZE		(1UL << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_MASK		(((u64)-1) << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_ALIGN(addr)	\
+	(((addr) + IOMMU_PAGE_SIZE - 1) & IOMMU_PAGE_MASK)
 
-#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT_4K)
+#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)

These are pretty generic names (IOMMU_PAGE_SHIFT, IOVA_PFN, etc), but the definitions seem to be specific to VT-d. I can't tell if this file is supposed to be sort of generic, or if it's Intel-specific.

Quoted 19 lines

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

index e7b196b..d84612a 100644
@@ -67,6 +67,13 @@
 		hi = readl(dmar + reg + 4); \
 		(((u64) hi) << 32) + lo; })
 */
+#ifdef CONFIG_IA64
+#define dmar_readq readq
+static inline void dmar_writeq(void __iomem *addr, u64 val)
+{
+	writeq(val, addr);
+}
+#else
 static inline u64 dmar_readq(void __iomem *addr)
 {
 	u32 lo, hi;

include/linux/intel-iommu.h static inline void dmar_writeq(void __iomem *addr, u64 val) · line 90

Quoted 4 lines

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

@@ -80,6 +87,7 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
 	writel((u32)val, addr);
 	writel((u32)(val >> 32), addr + 4);
 }
+#endif

What's this all about? Why do we need #ifdef CONFIG_IA64 here? Doesn't x86 provide its own readq/writeq implementation?

Bjorn

Yu, Fenghua

author

2008-10-02 21:46 UTC #10 lore raw

RE: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

Quoted 5 lines

Quoted 3 lines
The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes.
This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, .
Can you split this patch up? It contains several logically separate

changes:

Quoted 4 lines

- casting things to unsigned long long
- adding stuff under #ifdef CONFIG_IA64
- page size changes
- whitespace changes

Depends on who is picking up the generic patch. If it's needed, I can split it into multiple patches.

Quoted 11 lines

drivers/pci/dmar.c int alloc_iommu(struct dmar_drhd_unit *drhd) · line 519
Quoted 8 lines
drivers/pci/dmar.c +1 -1
@@ -510,7 +514,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
      iommu->seq_id = iommu_allocated++;
 
-     iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
+     iommu->reg = ioremap(drhd->reg_base_addr, IOMMU_PAGE_SIZE);
      if (!iommu->reg) {
              printk(KERN_ERR "IOMMU: can't map the region\n");
This printk should include a clue, like the IOMMU ID and/or address we tried to map.

This is a good comment. This patch set is mainly for porting IOMMU to IA64. I will add IOMMU ID in a follow-up clean-up patch.

Quoted 12 lines

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

+#ifdef CONFIG_IA64
+static inline void *ia64_get_zeroed_page(gfp_t gfp_mask)
+{
+     struct page *page;
+
+     page = alloc_pages(gfp_mask | __GFP_ZERO, 0);
+     if (page)
+             return (void *)page_address(page);
+     return 0;
+}
+#endif

Quoted 13 lines

drivers/pci/intel-iommu.c static inline void *alloc_pgtable_page(void) · line 162
Quoted 11 lines
static inline void *alloc_pgtable_page(void)
{
drivers/pci/intel-iommu.c +4 -0
@@ -141,7 +155,11 @@ static inline void *alloc_pgtable_page(void)
      /* trying to avoid low memory issues */
      flags = current->flags & PF_MEMALLOC;
      current->flags |= PF_MEMALLOC;
+#ifdef CONFIG_IA64
+     vaddr = ia64_get_zeroed_page(GFP_ATOMIC);
+#else
      vaddr = (void *)get_zeroed_page(GFP_ATOMIC);
+#endif
Why does ia64 need a special case here?

I think old kernel I originally work on doesn't have get_zeroed_page for ia64. Yes, I will remove this change in the updated patch.

Quoted 10 lines

drivers/pci/intel-iommu.c static int __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did, · line 677

Quoted 7 lines

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

@@ -655,7 +673,8 @@ static int __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
              printk(KERN_ERR"IOMMU: flush IOTLB failed\n");
      if (DMA_TLB_IAIG(val) != DMA_TLB_IIRG(type))
              pr_debug("IOMMU: tlb flush request %Lx, actual %Lx\n",
-                     DMA_TLB_IIRG(type), DMA_TLB_IAIG(val));
+                     (unsigned long long)DMA_TLB_IIRG(type),
+                     (unsigned long long) DMA_TLB_IAIG(val));

These printks should include an IOMMU ID also (I assume a system can contain multiple IOMMUs).

This patch set is mainly for porting IOMMU to IA64. I will add IOMMU ID in a follow-up clean-up patch.

Quoted 15 lines

drivers/pci/intel-iommu.c static int iommu_prepare_identity_map(struct pci_dev *pdev, u64 start, u64 end) · line 1521

Quoted 12 lines

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

@@ -1490,9 +1513,9 @@ static int iommu_prepare_identity_map(struct pci_dev *pdev, u64 start, u64 end)
              return -ENOMEM;
 
      /* The address might not be aligned */
-     base = start & PAGE_MASK_4K;
+     base = start & PAGE_MASK;
      size = end - base;
-     size = PAGE_ALIGN_4K(size);
+     size = PAGE_ALIGN(size);
      if (!reserve_iova(&domain->iovad, IOVA_PFN(base),
                      IOVA_PFN(base + size) - 1)) {
              printk(KERN_ERR "IOMMU: reserve iova failed\n");

This should probably be a "dev_err(&pdev->dev," and include the IOMMU ID.

This patch set is mainly for porting IOMMU to IA64. I will add IOMMU ID and dev_err in a follow-up clean-up patch.

Quoted 23 lines

drivers/pci/intel-iommu.c +3 -7

@@ -1855,27 +1879,23 @@ intel_map_single(struct device *hwdev, phys_addr_t paddr, size_t size, int dir)
       * is not a big problem
       */
      ret = domain_page_mapping(domain, start_paddr,
-             ((u64)paddr) & PAGE_MASK_4K, size, prot);
+             ((u64)paddr) & PAGE_MASK, size, prot);
      if (ret)
              goto error;
 
-     pr_debug("Device %s request: %lx@%llx mapping: %lx@%llx, dir %d\n",
-             pci_name(pdev), size, (u64)paddr,
-             size, (u64)start_paddr, dir);
-
      /* it's a non-present to present mapping */
      ret = iommu_flush_iotlb_psi(domain->iommu, domain->id,
-                     start_paddr, size >> PAGE_SHIFT_4K, 1);
+                     start_paddr, size >> IOMMU_PAGE_SHIFT, 1);
      if (ret)
              iommu_flush_write_buffer(domain->iommu);
 
-     return (start_paddr + ((u64)paddr & (~PAGE_MASK_4K)));
+     return start_paddr + ((u64)paddr & (~PAGE_MASK));

Quoted 8 lines

drivers/pci/intel-iommu.c intel_map_single(struct device *hwdev, phys_addr_t paddr, size_t size, int dir) · line 1898
Quoted 6 lines
drivers/pci/intel-iommu.c +1 -1
 error:
      if (iova)
              __free_iova(&domain->iovad, iova);
      printk(KERN_ERR"Device %s request: %lx@%llx dir %d --- failed\n",
-             pci_name(pdev), size, (u64)paddr, dir);
+             pci_name(pdev), size, (unsigned long long)paddr, dir);
Use dev_err() here and include IOMMU ID.

This patch set is mainly for porting IOMMU to IA64. I will add IOMMU ID and use dev_err() in a follow-up clean-up patch.

Quoted 8 lines

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

@@ -1953,11 +1974,11 @@ static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr,
      if (!iova)
              return;
 
-     start_addr = iova->pfn_lo << PAGE_SHIFT_4K;
+     start_addr = iova->pfn_lo << PAGE_SHIFT;
      size = aligned_size((u64)dev_addr, size);

Quoted 5 lines

drivers/pci/intel-iommu.c static void intel_unmap_single(struct device *dev, dma_addr_t dev_addr, · line 1981
Quoted 3 lines
drivers/pci/intel-iommu.c +1 -1
      pr_debug("Device %s unmapping: %lx@%llx\n",
-             pci_name(pdev), size, (u64)start_addr);
+             pci_name(pdev), size, (unsigned long long)start_addr);
Use dev_dbg() here.

This patch set is mainly for porting IOMMU to IA64. I will change to dev_dbg() in a follow-up patch.

Quoted 22 lines

include/linux/dma_remapping.h +6 -6

index bff5c65..218598d 100644
@@ -2,15 +2,15 @@
 #define _DMA_REMAPPING_H
 
 /*
- * We need a fixed PAGE_SIZE of 4K irrespective of
- * arch PAGE_SIZE for IOMMU page tables.
+ * VT-d hardware uses 4K page size regardless host page size.
  */
-#define PAGE_SHIFT_4K                (12)
-#define PAGE_SIZE_4K         (1UL << PAGE_SHIFT_4K)
-#define PAGE_MASK_4K         (((u64)-1) << PAGE_SHIFT_4K)
-#define PAGE_ALIGN_4K(addr)  (((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
+#define IOMMU_PAGE_SHIFT     (12)
+#define IOMMU_PAGE_SIZE              (1UL << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_MASK              (((u64)-1) << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_ALIGN(addr)       \
+     (((addr) + IOMMU_PAGE_SIZE - 1) & IOMMU_PAGE_MASK)

Quoted 6 lines

include/linux/dma_remapping.h line 13
Quoted 2 lines
include/linux/dma_remapping.h +1 -1
-#define IOVA_PFN(addr)               ((addr) >> PAGE_SHIFT_4K)
+#define IOVA_PFN(addr)               ((addr) >> PAGE_SHIFT)
These are pretty generic names (IOMMU_PAGE_SHIFT, IOVA_PFN, etc), but the definitions seem to be specific to VT-d. I can't tell if this file is supposed to be sort of generic, or if it's Intel-specific.

I can change IOMMU_PAGE_SHIFT etc to VTD_PAGE_SHIFT etc and change IOVA_PFN to VTD_IOVA_PFN. What do you think?

Quoted 18 lines

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

index e7b196b..d84612a 100644
@@ -67,6 +67,13 @@
              hi = readl(dmar + reg + 4); \
              (((u64) hi) << 32) + lo; })
 */
+#ifdef CONFIG_IA64
+#define dmar_readq readq
+static inline void dmar_writeq(void __iomem *addr, u64 val)
+{
+     writeq(val, addr);
+}
+#else
 static inline u64 dmar_readq(void __iomem *addr)
 {
      u32 lo, hi;

Quoted 8 lines

include/linux/intel-iommu.h static inline void dmar_writeq(void __iomem *addr, u64 val) · line 90
Quoted 5 lines
include/linux/intel-iommu.h +1 -0
@@ -80,6 +87,7 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
      writel((u32)val, addr);
      writel((u32)(val >> 32), addr + 4);
 }
+#endif
What's this all about? Why do we need #ifdef CONFIG_IA64 here? Doesn't x86 provide its own readq/writeq implementation?

Bjorn

Bjorn Helgaas

2008-10-03 15:33 UTC #11 lore raw

Re: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

Quoted 4 lines · Fenghua

Quoted 3 lines
Quoted 3 lines
The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes.
This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, .

Quoted 8 lines

Quoted 1 line
Can you split this patch up? It contains several logically separate
changes:
Quoted 4 lines
- casting things to unsigned long long
- adding stuff under #ifdef CONFIG_IA64
- page size changes
- whitespace changes
Depends on who is picking up the generic patch. If it's needed, I can split it into multiple patches.

Regardless of who's picking up the patch, splitting it makes it easier to review and easier to spot bugs, and makes bisection yield better information.

Quoted 12 lines

Quoted 11 lines
drivers/pci/dmar.c int alloc_iommu(struct dmar_drhd_unit *drhd) · line 519
Quoted 8 lines
drivers/pci/dmar.c +1 -1
@@ -510,7 +514,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
      iommu->seq_id = iommu_allocated++;
 
-     iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
+     iommu->reg = ioremap(drhd->reg_base_addr, IOMMU_PAGE_SIZE);
      if (!iommu->reg) {
              printk(KERN_ERR "IOMMU: can't map the region\n");
This printk should include a clue, like the IOMMU ID and/or address we tried to map.

Quoted 1 line

This is a good comment. This patch set is mainly for porting IOMMU to IA64. I will add IOMMU ID in a follow-up clean-up patch.

Since you're not actually adding the printk in this patch, it sounds fair to clean it up in a follow-up patch.

Quoted 10 lines

include/linux/dma_remapping.h +5 -4

-#define PAGE_SHIFT_4K                (12)
-#define PAGE_SIZE_4K         (1UL << PAGE_SHIFT_4K)
-#define PAGE_MASK_4K         (((u64)-1) << PAGE_SHIFT_4K)
-#define PAGE_ALIGN_4K(addr)  (((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
+#define IOMMU_PAGE_SHIFT     (12)
+#define IOMMU_PAGE_SIZE              (1UL << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_MASK              (((u64)-1) << IOMMU_PAGE_SHIFT)
+#define IOMMU_PAGE_ALIGN(addr)       \
+     (((addr) + IOMMU_PAGE_SIZE - 1) & IOMMU_PAGE_MASK)

Quoted 8 lines

Quoted 6 lines
include/linux/dma_remapping.h line 13
Quoted 2 lines
include/linux/dma_remapping.h +1 -1
-#define IOVA_PFN(addr)               ((addr) >> PAGE_SHIFT_4K)
+#define IOVA_PFN(addr)               ((addr) >> PAGE_SHIFT)
These are pretty generic names (IOMMU_PAGE_SHIFT, IOVA_PFN, etc), but the definitions seem to be specific to VT-d. I can't tell if this file is supposed to be sort of generic, or if it's Intel-specific.
I can change IOMMU_PAGE_SHIFT etc to VTD_PAGE_SHIFT etc and change IOVA_PFN to VTD_IOVA_PFN. What do you think?

Those sound good to me.

Bjorn

Yu, Fenghua

author

2008-10-04 00:21 UTC #12 lore raw

RE: [PATCH 1/2]Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

Quoted 11 lines

Quoted 11 lines
Quoted 5 lines
Quoted 2 lines
The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. The first patch supports variable page size. This provides support for IA64 which has multiple page sizes.
This patch also adds some other code hooks for IA64 platform including DMAR_OPERATION_TIMEOUT definition, .
Can you split this patch up? It contains several logically separate
changes:
Quoted 4 lines
- casting things to unsigned long long
- adding stuff under #ifdef CONFIG_IA64
- page size changes
- whitespace changes

Quoted 5 lines

Quoted 1 line
Depends on who is picking up the generic patch. If it's needed, I can >split it into multiple patches.
Regardless of who's picking up the patch, splitting it makes it easier to review and easier to spot bugs, and makes bisection yield better information.

Since more than 95% of code in the patch is directly related to page size changes, splitting the patch will generate one big patch (95% of current patch size) and a few small patches. I would like to still send one single updated patch based on collected comments.

Thanks.

-Fenghua

Fenghua Yu

author

2008-10-07 00:01 UTC #13 lore raw

PATCH v2 1/2 Add Variable Page Size and IA64 Support in Intel IOMMU: Generic Part

In reply to Fenghua Yu

The current Intel IOMMU code assumes that both host page size and Intel IOMMU page size are 4K. This patch supports variable page size. It provides support for IA64 which has multiple page sizes.

This patch also re-organizes code to work on both X86-64 and IA64 platforms, e.g. DMAR_OPERATION_TIMEOUT definition etc.

Signed-off-by Fenghua Yu <[email protected]>

Signed-off-by Tony Luck <[email protected]>

---

 arch/x86/kernel/pci-dma.c     |   16 -----
 drivers/pci/dmar.c            |   19 +++---
 drivers/pci/intel-iommu.c     |  124 ++++++++++++++++++++++--------------------
 drivers/pci/quirks.c          |   14 ++++
 include/asm-x86/iommu.h       |    4 +
 include/linux/dma_remapping.h |   27 ++++-----
 include/linux/intel-iommu.h   |   39 +++++++------
 7 files changed, 131 insertions(+), 112 deletions(-)

arch/x86/kernel/pci-dma.c +0 ~~-16~~

index 23882c4..6751f4c 100644
@@ -9,8 +9,6 @@
 #include <asm/calgary.h>
 #include <asm/amd_iommu.h>
 
-static int forbid_dac __read_mostly;
-
 struct dma_mapping_ops *dma_ops;
 EXPORT_SYMBOL(dma_ops);

@@ -262,17 +260,3 @@ void pci_iommu_shutdown(void)
 }
 /* Must execute after PCI subsystem */
 fs_initcall(pci_iommu_init);
-
-#ifdef CONFIG_PCI
-/* Many VIA bridges seem to corrupt data for DAC. Disable it here */
-
-static __devinit void via_no_dac(struct pci_dev *dev)
-{
-	if ((dev->class >> 8) = PCI_CLASS_BRIDGE_PCI && forbid_dac = 0) {
-		printk(KERN_INFO "PCI: VIA PCI bridge detected."
-				 "Disabling DAC.\n");
-		forbid_dac = 1;
-	}
-}
-DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, PCI_ANY_ID, via_no_dac);
-#endif

drivers/pci/dmar.c +11 -8

index 0c92e2c..59c974a 100644
@@ -277,14 +277,15 @@ dmar_table_print_dmar_entry(struct acpi_dmar_header *header)
 		drhd = (struct acpi_dmar_hardware_unit *)header;
 		printk (KERN_INFO PREFIX
 			"DRHD (flags: 0x%08x)base: 0x%016Lx\n",
-			drhd->flags, drhd->address);
+			drhd->flags, (unsigned long long)drhd->address);
 		break;
 	case ACPI_DMAR_TYPE_RESERVED_MEMORY:
 		rmrr = (struct acpi_dmar_reserved_memory *)header;
 
 		printk (KERN_INFO PREFIX
 			"RMRR base: 0x%016Lx end: 0x%016Lx\n",
-			rmrr->base_address, rmrr->end_address);
+			(unsigned long long)rmrr->base_address,
+			(unsigned long long)rmrr->end_address);
 		break;
 	}
 }

@@ -328,7 +329,7 @@ parse_dmar_table(void)
 	if (!dmar)
 		return -ENODEV;
 
-	if (dmar->width < PAGE_SHIFT_4K - 1) {
+	if (dmar->width < PAGE_SHIFT - 1) {
 		printk(KERN_WARNING PREFIX "Invalid DMAR haw\n");
 		return -EINVAL;
 	}

@@ -510,7 +511,7 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 	iommu->seq_id = iommu_allocated++;
 
-	iommu->reg = ioremap(drhd->reg_base_addr, PAGE_SIZE_4K);
+	iommu->reg = ioremap(drhd->reg_base_addr, VTD_PAGE_SIZE);
 	if (!iommu->reg) {
 		printk(KERN_ERR "IOMMU: can't map the region\n");
 		goto error;

@@ -521,8 +522,8 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 	/* the registers might be more than one page */
 	map_size = max_t(int, ecap_max_iotlb_offset(iommu->ecap),
 		cap_max_fault_reg_offset(iommu->cap));
-	map_size = PAGE_ALIGN_4K(map_size);
-	if (map_size > PAGE_SIZE_4K) {
+	map_size = VTD_PAGE_ALIGN(map_size);
+	if (map_size > VTD_PAGE_SIZE) {
 		iounmap(iommu->reg);
 		iommu->reg = ioremap(drhd->reg_base_addr, map_size);
 		if (!iommu->reg) {

@@ -533,8 +534,10 @@ int alloc_iommu(struct dmar_drhd_unit *drhd)
 
 	ver = readl(iommu->reg + DMAR_VER_REG);
 	pr_debug("IOMMU %llx: ver %d:%d cap %llx ecap %llx\n",
-		drhd->reg_base_addr, DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
-		iommu->cap, iommu->ecap);
+		(unsigned long long)drhd->reg_base_addr,
+		DMAR_VER_MAJOR(ver), DMAR_VER_MINOR(ver),
+		(unsigned long long)iommu->cap,
+		(unsigned long long)iommu->ecap);
 
 	spin_lock_init(&iommu->register_lock);

drivers/pci/intel-iommu.c +66 ~~-58~~

400 lines not on this page · raw message

drivers/pci/quirks.c +14 -0

index b7c2378..5c0ad61 100644
@@ -30,8 +30,22 @@ int pci_pci_problems;
 EXPORT_SYMBOL(pci_pci_problems);
 int pcie_mch_quirk;
 EXPORT_SYMBOL(pcie_mch_quirk);
+int forbid_dac __read_mostly;
+EXPORT_SYMBOL(forbid_dac);
 
 #ifdef CONFIG_PCI_QUIRKS
+
+/* Many VIA bridges seem to corrupt data for DAC. Disable it here */
+static __devinit void via_no_dac(struct pci_dev *dev)
+{
+	if ((dev->class >> 8) = PCI_CLASS_BRIDGE_PCI && forbid_dac = 0) {
+		dev_info(&dev->dev,
+			"VIA PCI bridge detected. Disabling DAC.\n");
+		forbid_dac = 1;
+	}
+}
+DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, PCI_ANY_ID, via_no_dac);
+
 /* The Mellanox Tavor device gives false positive parity errors
  * Mark this device with a broken_parity_status, to allow
  * PCI scanning code to "skip" this now blacklisted device.

include/asm-x86/iommu.h +4 -0

index 546ad31..fbccb97 100644
@@ -7,9 +7,13 @@ extern struct dma_mapping_ops nommu_dma_ops;
 extern int force_iommu, no_iommu;
 extern int iommu_detected;
 extern int dmar_disabled;
+extern int forbid_dac;
 
 extern unsigned long iommu_num_pages(unsigned long addr, unsigned long len);
 
+/* 10 seconds */
+#define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000)
+
 #ifdef CONFIG_GART_IOMMU
 extern int gart_iommu_aperture;
 extern int gart_iommu_aperture_allowed;

include/linux/dma_remapping.h +13 ~~-14~~

index bff5c65..1b49810 100644
@@ -2,15 +2,14 @@
 #define _DMA_REMAPPING_H
 
 /*
- * We need a fixed PAGE_SIZE of 4K irrespective of
- * arch PAGE_SIZE for IOMMU page tables.
+ * VT-d hardware uses 4K page size regardless host page size.
  */
-#define PAGE_SHIFT_4K		(12)
-#define PAGE_SIZE_4K		(1UL << PAGE_SHIFT_4K)
-#define PAGE_MASK_4K		(((u64)-1) << PAGE_SHIFT_4K)
-#define PAGE_ALIGN_4K(addr)	(((addr) + PAGE_SIZE_4K - 1) & PAGE_MASK_4K)
+#define VTD_PAGE_SHIFT		(12)
+#define VTD_PAGE_SIZE		(1UL << VTD_PAGE_SHIFT)
+#define VTD_PAGE_MASK		(((u64)-1) << VTD_PAGE_SHIFT)
+#define VTD_PAGE_ALIGN(addr)	(((addr) + VTD_PAGE_SIZE - 1) & VTD_PAGE_MASK)
 
-#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT_4K)
+#define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
 #define DMA_32BIT_PFN		IOVA_PFN(DMA_32BIT_MASK)
 #define DMA_64BIT_PFN		IOVA_PFN(DMA_64BIT_MASK)

@@ -25,7 +24,7 @@ struct root_entry {
 	u64	val;
 	u64	rsvd1;
 };
-#define ROOT_ENTRY_NR (PAGE_SIZE_4K/sizeof(struct root_entry))
+#define ROOT_ENTRY_NR (VTD_PAGE_SIZE/sizeof(struct root_entry))
 static inline bool root_present(struct root_entry *root)
 {
 	return (root->val & 1);

@@ -36,7 +35,7 @@ static inline void set_root_present(struct root_entry *root)
 }
 static inline void set_root_value(struct root_entry *root, unsigned long value)
 {
-	root->val |= value & PAGE_MASK_4K;
+	root->val |= value & VTD_PAGE_MASK;
 }
 
 struct context_entry;

@@ -45,7 +44,7 @@ get_context_addr_from_root(struct root_entry *root)
 {
 	return (struct context_entry *)
 		(root_present(root)?phys_to_virt(
-		root->val & PAGE_MASK_4K):
+		root->val & VTD_PAGE_MASK) :
 		NULL);
 }

@@ -67,7 +66,7 @@ struct context_entry {
 #define context_present(c) ((c).lo & 1)
 #define context_fault_disable(c) (((c).lo >> 1) & 1)
 #define context_translation_type(c) (((c).lo >> 2) & 3)
-#define context_address_root(c) ((c).lo & PAGE_MASK_4K)
+#define context_address_root(c) ((c).lo & VTD_PAGE_MASK)
 #define context_address_width(c) ((c).hi &  7)
 #define context_domain_id(c) (((c).hi >> 8) & ((1 << 16) - 1))

@@ -81,7 +80,7 @@ struct context_entry {
 	} while (0)
 #define CONTEXT_TT_MULTI_LEVEL 0
 #define context_set_address_root(c, val) \
-	do {(c).lo |= (val) & PAGE_MASK_4K;} while (0)
+	do {(c).lo |= (val) & VTD_PAGE_MASK; } while (0)
 #define context_set_address_width(c, val) do {(c).hi |= (val) & 7;} while (0)
 #define context_set_domain_id(c, val) \
 	do {(c).hi |= ((val) & ((1 << 16) - 1)) << 8;} while (0)

@@ -107,9 +106,9 @@ struct dma_pte {
 #define dma_set_pte_writable(p) do {(p).val |= DMA_PTE_WRITE;} while (0)
 #define dma_set_pte_prot(p, prot) \
 		do {(p).val = ((p).val & ~3) | ((prot) & 3); } while (0)
-#define dma_pte_addr(p) ((p).val & PAGE_MASK_4K)
+#define dma_pte_addr(p) ((p).val & VTD_PAGE_MASK)
 #define dma_set_pte_addr(p, addr) do {\
-		(p).val |= ((addr) & PAGE_MASK_4K); } while (0)
+		(p).val |= ((addr) & VTD_PAGE_MASK); } while (0)
 #define dma_pte_present(p) (((p).val & 3) != 0)
 
 struct intel_iommu;

include/linux/intel-iommu.h +23 ~~-16~~

index e7b196b..0683472 100644
@@ -27,8 +27,9 @@
 #include <linux/sysdev.h>
 #include <linux/iova.h>
 #include <linux/io.h>
-#include <asm/cacheflush.h>
 #include <linux/dma_remapping.h>
+#include <asm/iommu.h>
+#include <asm/cacheflush.h>
 
 /*
  * Intel IOMMU register specification per version 1.0 public spec.

@@ -200,22 +201,21 @@ static inline void dmar_writeq(void __iomem *addr, u64 val)
 #define dma_frcd_type(d) ((d >> 30) & 1)
 #define dma_frcd_fault_reason(c) (c & 0xff)
 #define dma_frcd_source_id(c) (c & 0xffff)
-#define dma_frcd_page_addr(d) (d & (((u64)-1) << 12)) /* low 64 bit */
-
-#define DMAR_OPERATION_TIMEOUT ((cycles_t) tsc_khz*10*1000) /* 10sec */
-
-#define IOMMU_WAIT_OP(iommu, offset, op, cond, sts) \
-{\
-	cycles_t start_time = get_cycles();\
-	while (1) {\
-		sts = op (iommu->reg + offset);\
-		if (cond)\
-			break;\
+/* low 64 bit */
+#define dma_frcd_page_addr(d) (d & (((u64)-1) << PAGE_SHIFT))
+
+#define IOMMU_WAIT_OP(iommu, offset, op, cond, sts)			\
+do {									\
+	cycles_t start_time = get_cycles();				\
+	while (1) {							\
+		sts = op(iommu->reg + offset);				\
+		if (cond)						\
+			break;						\
 		if (DMAR_OPERATION_TIMEOUT < (get_cycles() - start_time))\
-			panic("DMAR hardware is malfunctioning\n");\
-		cpu_relax();\
-	}\
-}
+			panic("DMAR hardware is malfunctioning\n");	\
+		cpu_relax();						\
+	}								\
+} while (0)
 
 #define QI_LENGTH	256	/* queue length */

@@ -316,4 +316,11 @@ struct dmar_domain *intel_iommu_find_domain(struct pci_dev *pdev);
 int intel_iommu_found(void);
 u64 intel_iommu_iova_to_pfn(struct dmar_domain *domain, u64 iova);
 
+extern void *intel_alloc_coherent(struct device *, size_t, dma_addr_t *, gfp_t);
+extern void intel_free_coherent(struct device *, size_t, void *, dma_addr_t);
+extern dma_addr_t intel_map_single(struct device *, unsigned long, size_t, int);
+extern void intel_unmap_single(struct device *, dma_addr_t, size_t, int);
+extern int intel_map_sg(struct device *, struct scatterlist *, int, int);
+extern void intel_unmap_sg(struct device *, struct scatterlist *, int, int);
+
 #endif

Fenghua Yu

author

2009-08-04 22:10 UTC #14 lore raw

PATCH 2/4 Bug Fix drivers/pci/intel-iommu.c: secure sg_next() calling

In reply to a message not in the archive

sg_next() is called only when sg is not NULL. This solves a kernel panic on some platforms (e.g. ia64).

Signed-off-by Fenghua Yu <[email protected]>

---

 drivers/pci/intel-iommu.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/pci/intel-iommu.c +6 -2

index bec29ed..54ee63d 100644
@@ -1705,8 +1715,12 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 		iov_pfn++;
 		pteval += VTD_PAGE_SIZE;
 		sg_res--;
-		if (!sg_res)
-			sg = sg_next(sg);
+		if (!sg_res) {
+			if (sg)
+				sg = sg_next(sg);
+			else
+				return 0;
+		}
 	}
 	return 0;
 }

David Woodhouse

2009-08-05 08:09 UTC #15 lore raw

Re: [PATCH 2/4] Bug Fix drivers/pci/intel-iommu.c: secure sg_next() calling

Quoted 2 lines · Fenghua Yu

sg_next() is called only when sg is not NULL. This solves a kernel panic on some platforms (e.g. ia64).

Is this still necessary after we fix the other bugs? When does it happen? And is this fix sufficient (ignore the first hunk; it's just a cleanup that the fix allows me to make, which is why I'd prefer to do it this way):

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

index 11b317a..c9bdb0b 100644
@@ -1675,7 +1675,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 	if (sg)
 		sg_res = 0;
 	else {
-		sg_res = nr_pages + 1;
+		sg_res = nr_pages;
 		pteval = ((phys_addr_t)phys_pfn << VTD_PAGE_SHIFT) | prot;
 	}

@@ -1716,7 +1716,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 		iov_pfn++;
 		pteval += VTD_PAGE_SIZE;
 		sg_res--;
-		if (!sg_res)
+		if (nr_pages && !sg_res)
 			sg = sg_next(sg);
 	}
 	return 0;

Signature · 2 lines

David Woodhouse                            Open Source Technology Centre
[email protected]                              Intel Corporation