Intel IOMMU Support.

drivers/pci/intel-iommu.c new file +9 -0

+static char *fault_reason_strings[] =
+{
+	"Software",
+	"Present bit in root entry is clear",
+	"Present bit in context entry is clear",
+	"Invalid context entry",
+	"Access beyond MGAW",
+	"PTE Write access is not set",
+	"PTE Read access is not set",

drivers/pci/intel-iommu.c new file +6 -0

+static int __init intel_iommu_setup(char *str)
+{
+	if (!str)
+		return -EINVAL;
+	while (*str) {
+		if (!strncmp(str, "off", 3)) {

drivers/pci/intel-iommu.c new file +17 -0

+	spin_unlock_irqrestore(&iommu->register_lock, flag);
+}
+
+static void iommu_flush_write_buffer(struct iommu *iommu)
+{
+	u32 val;
+	unsigned long flag;
+	unsigned long start_time;
+
+	if (!cap_rwbf(iommu->cap))
+		return;
+	val = iommu->gcmd | DMA_GCMD_WBF;
+
+	spin_lock_irqsave(&iommu->register_lock, flag);
+	dmar_writel(iommu->reg, DMAR_GCMD_REG, val);
+
+	/* Make sure hardware complete it */

+/* iommu interrupt handling. Most staffs are MSI-like. */

PCI specs permit zero length reads (ZLR) even if the mapping for that region is write only. Support for this feature is indicated by the presence of a bit in the DMAR capability. If a particular DMAR does not support this capability we map write-only regions as read-write.

This option can also provides a workaround for some drivers that request a write-only mapping when they really should request a read-write. (We ran into one such case in eepro100.c in handling rx_ring_dma)

Floppy disk drivers dont work well with DMA remapping.

Its possible to
extend the current use for x86_64, but the gain is very little. If someone feels compelled to clean this up, its up for grabs. Since these use 16M, we just provide a unity map for the ISA bridge device.

Some devices may not support entire 64bit DMA. In a situation where such devices are co-located in a shared domain, we need to ensure there is some address space reserved for such devices without the low addresses getting depleted by other devices capable of handling high dma addresses.

drivers/pci/dmar.c line 34

Quoted 4 lines

drivers/pci/dmar.c new file +3 -0

+
+LIST_HEAD(dmar_drhd_units);
+LIST_HEAD(dmar_rmrr_units);

Comment describing what lock protects those lists? In fact there seems to be no locking. What about hotplug?

Quoted 1 line · On Tuesday 24 April 2007 08:03:06 Ashok Raj wrote:

Floppy disk drivers dont work well with DMA remapping.

What is the problem? You can't allocate mappings <16MB?

Quoted 4 lines · On Tuesday 24 April 2007 08:03:07 Ashok Raj wrote:

Some devices may not support entire 64bit DMA. In a situation where such devices are co-located in a shared domain, we need to ensure there is some address space reserved for such devices without the low addresses getting depleted by other devices capable of handling high dma addresses.

Sorry, but you need to find some way to make this usually work without special options. Otherwise users will be unhappy.

An possible way would be to allocate space upside down from the limit of the device. Then the lower areas should be usually free.

Quoted 1 line · On Tuesday 24 April 2007 08:03:06 Ashok Raj wrote:

Floppy disk drivers dont work well with DMA remapping.

What is the problem? You can't allocate mappings <16MB?

Quoted 6 lines

Its possible to
extend the current use for x86_64, but the gain is very little. If someone feels compelled to clean this up, its up for grabs. Since these use 16M, we just provide a unity map for the ISA bridge device.

While it's probably not worth for the floppy there are other devices with similar weird addressing limitations. Some generic handling of it would be nice.

Quoted 8 lines · On Tuesday 24 April 2007 08:03:03 Ashok Raj wrote:

PCI specs permit zero length reads (ZLR) even if the mapping for that region is write only. Support for this feature is indicated by the presence of a bit in the DMAR capability. If a particular DMAR does not support this capability we map write-only regions as read-write.

This option can also provides a workaround for some drivers that request a write-only mapping when they really should request a read-write. (We ran into one such case in eepro100.c in handling rx_ring_dma)

Better just fix the drivers instead of adding such hacks

Quoted 8 lines

PCI specs permit zero length reads (ZLR) even if the mapping for that region is write only. Support for this feature is indicated by the presence of a bit in the DMAR capability. If a particular DMAR does not support this capability we map write-only regions as read-write.

This option can also provides a workaround for some drivers that request a write-only mapping when they really should request a read-write. (We ran into one such case in eepro100.c in handling rx_ring_dma)

Better just fix the drivers instead of adding such hacks

arch/x86_64/kernel/io_apic.c void arch_teardown_msi_irq(unsigned int · line 2008

Quoted 4 lines · On Tuesday 24 April 2007 08:03:02 Ashok Raj wrote:

arch/x86_64/kernel/io_apic.c +3 -0

+#ifdef CONFIG_DMAR
+#ifdef CONFIG_SMP
+static void dmar_msi_set_affinity(unsigned int irq, cpumask_t mask)

Why does it need an own interrupt type?

drivers/pci/Kconfig config HT_IRQ · line 70

Quoted 9 lines

drivers/pci/Kconfig +8 -0

+
+config IOVA_GUARD_PAGE
+	bool "Enables gaurd page when allocating IO Virtual Address for IOMMU"
+	depends on DMAR
+
+config IOVA_NEXT_CONTIG
+	bool "Keeps IOVA allocations consequent between allocations"
+	depends on DMAR && EXPERIMENTAL

Needs reference to Intel and better description

The file should have a high level description what it is good for etc.

Need high level overview over what locks protects what and if there is a locking order.

It doesn't seem to enable sg merging? Since you have enough space that should work.

Quoted 15 lines

drivers/pci/intel-iommu.c new file +14 -0

+static char *fault_reason_strings[] =
+{
+	"Software",
+	"Present bit in root entry is clear",
+	"Present bit in context entry is clear",
+	"Invalid context entry",
+	"Access beyond MGAW",
+	"PTE Write access is not set",
+	"PTE Read access is not set",
+	"Next page table ptr is invalid",
+	"Root table address invalid",
+	"Context table ptr is invalid",
+	"non-zero reserved fields in RTP",
+	"non-zero reserved fields in CTP",

drivers/pci/intel-iommu.c line 66

Quoted 5 lines

drivers/pci/intel-iommu.c new file +5 -0

+	"non-zero reserved fields in PTE",
+	"Unknown"
+};
+
+#define MAX_FAULT_REASON_IDX 	(12)

You got 14 of them. better use ARRAY_SIZE

With PCIE there is some benefit to keep dma addr low for performance reasons,
since it will use 32bit Transaction level packets instead of 64bit.

This reservation is only required if we have some legacy device under a p2p where its required to share its addr space with other devices. We could implement a default when one is not specified to keep things simple.

We already have a couple of other IOMMU architectures who essentially have the same problem. Have you checked how they solve this?

My suggestion would be to allocate top-down in the 32-bit IOMMU space.

From: Andi Kleen <[email protected]>
Date: Tue, 24 Apr 2007 23:12:54 +0200

Quoted 2 lines

We already have a couple of other IOMMU architectures who essentially have the same problem. Have you checked how they solve this?

Sparc64, for one, only uses 32-bit IOMMU addresses. And we simply don't try to handle the funny devices at all, in fact we can't handle the ones that want mappings only in the low 16MB for example since the hardware IOMMU window usually starts in the middle of the 32-bit PCI address space.

We do it both because that's faster due to Single Address Cycles, as mentioned, and also because that's simply is where the hardware's IOMMU window is. You can't use 64-bit IOMMU addresses even if you wanted to on sparc64.

My suggestion would be to allocate top-down in the 32-bit IOMMU space.

That might work, but my gut feeling is that this won't be sufficient and we'll need some kind of device driver initiated reservation mechanism for the <16MB et al. weird stuff.

Quoted 1 line

My suggestion would be to allocate top-down in the 32-bit IOMMU space.

I think that's good for normal things, but it's not unreasonable to want to map > 4 GB of memory at once for an Infiniband device.

So maybe we would want some heuristics about the size of the mapping being requested or the amount of 32-bit space left to decide whether a mapping should be below 4 GB.

Its not clear if we have a very generic device breakage.. most devices on these platforms are going to be more recent, (except maybe some legacy fd)...

From: Ashok Raj <[email protected]>
Date: Tue, 24 Apr 2007 14:38:35 -0700

Quoted 3 lines

Its not clear if we have a very generic device breakage.. most devices on these platforms are going to be more recent, (except maybe some legacy fd)...

I'm not so sure, there are some "modern" sound cards that have a 31-bit DMA addressing limitation because they use the 31st bit as a status bit in their DMA descriptors :-)

Quoted 7 lines · On Tuesday 24 April 2007 23:50:26 David Miller wrote:

From: Ashok Raj <[email protected]>
Date: Tue, 24 Apr 2007 14:38:35 -0700

Quoted 3 lines

Its not clear if we have a very generic device breakage.. most devices on these platforms are going to be more recent, (except maybe some legacy fd)...

Quoted 3 lines

I'm not so sure, there are some "modern" sound cards that have a 31-bit DMA addressing limitation because they use the 31st bit as a status bit in their DMA descriptors :-)

There's also a 2GB only megaraid RAID controller that's pretty popular because Dell shipped it for a long time.

From: Ashok Raj <[email protected]>
Date: Tue, 24 Apr 2007 14:38:35 -0700

Quoted 3 lines

Its not clear if we have a very generic device breakage.. most devices on these platforms are going to be more recent, (except maybe some legacy fd)...

I'm not so sure, there are some "modern" sound cards that have a 31-bit DMA addressing limitation because they use the 31st bit as a status bit in their DMA descriptors :-)

Quoted 3 lines · On Tuesday 24 April 2007 23:50:26 David Miller wrote:

From: Ashok Raj <[email protected]>
Date: Tue, 24 Apr 2007 14:38:35 -0700

Quoted 6 lines

Quoted 3 lines

Its not clear if we have a very generic device breakage.. most devices on these platforms are going to be more recent, (except maybe some legacy fd)...

I'm not so sure, there are some "modern" sound cards that have a 31-bit DMA addressing limitation because they use the 31st bit as a status bit in their DMA descriptors :-)

There's also a 2GB only megaraid RAID controller that's pretty popular because Dell shipped it for a long time.

arch/x86_64/kernel/io_apic.c void arch_teardown_msi_irq(unsigned int · line 2008

Quoted 4 lines · On Tuesday 24 April 2007 08:03:02 Ashok Raj wrote:

arch/x86_64/kernel/io_apic.c +3 -0

+#ifdef CONFIG_DMAR
+#ifdef CONFIG_SMP
+static void dmar_msi_set_affinity(unsigned int irq, cpumask_t mask)

Why does it need an own interrupt type?

drivers/pci/Kconfig config HT_IRQ · line 70

Quoted 8 lines

drivers/pci/Kconfig +8 -0

+
+config IOVA_GUARD_PAGE
+	bool "Enables gaurd page when allocating IO Virtual Address for IOMMU"
+	depends on DMAR
+
+config IOVA_NEXT_CONTIG
+	bool "Keeps IOVA allocations consequent between allocations"
+	depends on DMAR && EXPERIMENTAL

Needs reference to Intel and better description

The file should have a high level description what it is good for etc.

Need high level overview over what locks protects what and if there is a locking order.

It doesn't seem to enable sg merging? Since you have enough space that should work.

Quoted 9 lines

drivers/pci/intel-iommu.c new file +9 -0

+static char *fault_reason_strings[] =
+{
+	"Software",
+	"Present bit in root entry is clear",
+	"Present bit in context entry is clear",
+	"Invalid context entry",
+	"Access beyond MGAW",
+	"PTE Write access is not set",
+	"PTE Read access is not set",

drivers/pci/intel-iommu.c line 66

Quoted 10 lines

drivers/pci/intel-iommu.c new file +10 -0

+	"Next page table ptr is invalid",
+	"Root table address invalid",
+	"Context table ptr is invalid",
+	"non-zero reserved fields in RTP",
+	"non-zero reserved fields in CTP",
+	"non-zero reserved fields in PTE",
+	"Unknown"
+};
+
+#define MAX_FAULT_REASON_IDX 	(12)

You got 14 of them. better use ARRAY_SIZE

drivers/pci/intel-iommu.c line 69

Quoted 3 lines

drivers/pci/intel-iommu.c new file +3 -0

+#define IOMMU_NAME_LEN		(7)
+
+struct iommu {

call it intel_iommu or somesuch even when it's private.

Quoted 10 lines

drivers/pci/intel-iommu.c new file +10 -0

+static int __init intel_iommu_setup(char *str)
+{
+	if (!str)
+		return -EINVAL;
+	while (*str) {
+		if (!strncmp(str, "off", 3)) {
+			dmar_disabled = 1;
+			printk(KERN_INFO"Intel-IOMMU: disabled\n");
+		}
+		str += strcspn(str, ",");

drivers/pci/intel-iommu.c line 112

Quoted 6 lines

drivers/pci/intel-iommu.c new file +6 -0

+		while (*str == ',')
+			str++;
+	}
+	return 0;
+}
+__setup("intel_iommu=", intel_iommu_setup);

Why can't you just use the normal iommu=off for this?

drivers/pci/intel-iommu.c line 119

Quoted 7 lines

drivers/pci/intel-iommu.c new file +7 -0

+
+#define MIN_PGTABLE_PAGES	(10)
+static mempool_t *pgtable_mempool;
+#define MIN_DOMAIN_REQ		(20)
+static mempool_t *domain_mempool;
+#define MIN_DEVINFO_REQ		(20)
+static mempool_t *devinfo_mempool;

Lots of mempools. How much memory does this pin?

drivers/pci/intel-iommu.c line 126

Quoted 7 lines

drivers/pci/intel-iommu.c new file +7 -0

+
+#define alloc_pgtable_page() mempool_alloc(pgtable_mempool, GFP_ATOMIC)
+#define free_pgtable_page(vaddr) mempool_free(vaddr, pgtable_mempool)
+#define alloc_domain_mem() mempool_alloc(domain_mempool, GFP_ATOMIC)
+#define free_domain_mem(vaddr) mempool_free(vaddr, domain_mempool)
+#define alloc_devinfo_mem() mempool_alloc(devinfo_mempool, GFP_ATOMIC)
+#define free_devinfo_mem(vaddr) mempool_free(vaddr, devinfo_mempool)

Do we need the macros? Better expand them in the caller.

drivers/pci/intel-iommu.c line 137

Quoted 10 lines

drivers/pci/intel-iommu.c new file +10 -0

+static void __iommu_flush_cache(struct iommu *iommu, void *addr, int size)
+{
+	if (!ecap_coherent(iommu->ecap))
+		clflush_cache_range(addr, size);
+}
+
+#define iommu_flush_cache_entry(iommu, addr) \
+	__iommu_flush_cache(iommu, addr, 8)
+#define iommu_flush_cache_page(iommu, addr) \
+	__iommu_flush_cache(iommu, addr, PAGE_SIZE_4K)

Similar.

And the 8 should be probably something more descriptive (sizeof?)

Quoted 11 lines

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

+/* context entry handling */
+static struct context_entry * device_to_context_entry(struct iommu *iommu,
+		u8 bus, u8 devfn)
+{
+	struct root_entry *root;
+	struct context_entry *context;
+	unsigned long phy_addr;
+	unsigned long flags;
+
+	spin_lock_irqsave(&iommu->lock, flags);
+	root = &iommu->root_entry[bus];

drivers/pci/intel-iommu.c line 151

Quoted 2 lines

drivers/pci/intel-iommu.c new file +2 -0

+	if (!root_present(*root)) {
+		phy_addr = (unsigned long)alloc_pgtable_page();

A GFP_ATOMIC mempool is rather useless. mempool only works if it can block for someone else freeing memory and if it can't do that it's not failsafe. I'm afraid you need to revise the allocation strategy -- best would be to somehow move the memory allocations outside the spinlock paths and preallocate if possible.

Same problem in other code.

drivers/pci/intel-iommu.c line 269

Quoted 2 lines

drivers/pci/intel-iommu.c new file +2 -0

+		if (!dma_pte_present(*pte)) {
+			tmp = alloc_pgtable_page();

Please don't name variable tmp. I know some other code does it, but it's just bad style imho.

drivers/pci/intel-iommu.c line 431

Quoted 10 lines

drivers/pci/intel-iommu.c new file +10 -0

+	/* Make sure hardware complete it */
+	start_time = jiffies;
+	while (1) {
+		sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);
+		if (sts & DMA_GSTS_RTPS)
+			break;
+		if (time_after(jiffies, start_time + DMAR_OPERATION_TIMEOUT))
+			panic("DMAR hardware is malfunctional, please disable IOMMU\n");
+		cpu_relax();
+	}

Could MWAIT/MONITOR be used for this?

Quoted 2 lines

It doesn't seem to enable sg merging? Since you have enough space that should work.

We actually have a patch to do sg merge. In my test, it doesn't have any performance gain.

Quoted 5 lines

A GFP_ATOMIC mempool is rather useless. mempool only works if it can block for someone else freeing memory and if it can't do that it's not failsafe. I'm afraid you need to revise the allocation strategy -- best would be to somehow move the memory allocations outside the spinlock paths and preallocate if possible.

The problem is pci_map_single and friends usually called with interrupt disabled or spin locked, so we must use GFP_ATOMIC.

You can probably find almost any possible bitmask if you look long enough. Hardware vendors are notorious for this kind of "optimizations".