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

Previous thread: [PATCH 0/2]Add Variable Page Size and IA64 Support in Intel IOMMU by Fenghua Yu on Wednesday, October 1, 2008 - 9:56 am. (17 messages)

Next thread: [PATCH 2/2]Add Variable Page Size and IA64 Support in Intel IOMMU: IA64 Specific Part by Fenghua Yu on Wednesday, October 1, 2008 - 9:57 am. (11 messages)
From: Fenghua Yu
Date: Wednesday, October 1, 2008 - 9:57 am

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 <fenghua.yu@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>

---

 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(-)

diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 0c92e2c..31290c2 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -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 ...
From: Ingo Molnar
Date: Thursday, October 2, 2008 - 1:29 am

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

	Ingo
--

From: Yu, Fenghua
Date: Thursday, October 2, 2008 - 3:06 pm

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


--

From: Ingo Molnar
Date: Friday, October 3, 2008 - 1:54 am

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

	Ingo
--

From: Bjorn Helgaas
Date: Thursday, October 2, 2008 - 8:31 am

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

This printk should include a clue, like the IOMMU ID and/or address


These printks should include an IOMMU ID also (I assume a system can

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



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

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

Bjorn
--

From: Yu, Fenghua
Date: Thursday, October 2, 2008 - 2:46 pm

Bjorn
--

From: Bjorn Helgaas
Date: Friday, October 3, 2008 - 8:33 am

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

Since you're not actually adding the printk in this patch, it sounds

Those sound good to me.

Bjorn

--

From: Yu, Fenghua
Date: Friday, October 3, 2008 - 5:21 pm

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
--

From: Fenghua Yu
Date: Monday, October 6, 2008 - 5:01 pm

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 <fenghua.yu@intel.com>
Signed-off-by: Tony Luck <tony.luck@intel.com>

---

 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(-)


diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 23882c4..6751f4c 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -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
diff --git a/drivers/pci/dmar.c b/drivers/pci/dmar.c
index 0c92e2c..59c974a 100644
--- a/drivers/pci/dmar.c
+++ b/drivers/pci/dmar.c
@@ -277,14 +277,15 @@ dmar_table_print_dmar_entry(struct acpi_dmar_header *header)
 		drhd = (struct acpi_dmar_hardware_unit *)header;
 		printk (KERN_INFO ...
Previous thread: [PATCH 0/2]Add Variable Page Size and IA64 Support in Intel IOMMU by Fenghua Yu on Wednesday, October 1, 2008 - 9:56 am. (17 messages)

Next thread: [PATCH 2/2]Add Variable Page Size and IA64 Support in Intel IOMMU: IA64 Specific Part by Fenghua Yu on Wednesday, October 1, 2008 - 9:57 am. (11 messages)