[PATCH] x86: Fix CONFIG_PCI=n compile failure

[PATCH] x86: Fix CONFIG_PCI=n compile failure

[James Bottomley]

This:

commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
Author: Glauber Costa <gcosta@redhat.com>
Date:   Tue Apr 8 13:20:56 2008 -0300

    x86: merge iommu initialization parameters

Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI
only symbol thus causing a compile failure if CONFIG_PCI=N.  Fix by
surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 1972266..47c5a7a 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
 			iommu_merge = 0;
 		if (!strncmp(p, "forcesac", 8))
 			iommu_sac_force = 1;
+#ifdef CONFIG_PCI
 		if (!strncmp(p, "allowdac", 8))
 			forbid_dac = 0;
 		if (!strncmp(p, "nodac", 5))
@@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
 			forbid_dac = -1;
 			return 1;
 		}
+#endif
 #ifdef CONFIG_SWIOTLB
 		if (!strncmp(p, "soft", 4))
 			swiotlb = 1;

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[FUJITA Tomonori]

On Tue, 28 Oct 2008 16:22:23 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> This:
> 
> commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
> Author: Glauber Costa <gcosta@redhat.com>
> Date:   Tue Apr 8 13:20:56 2008 -0300
> 
>     x86: merge iommu initialization parameters
> 
> Moved the forbid_dac parameter into pci-dma.c but forgot that it's a PCI
> only symbol thus causing a compile failure if CONFIG_PCI=N.  Fix by
> surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

http://marc.info/?l=linux-kernel&m=122480590627590&w=2


I guess that this patch is in the PCI tree.

Can you please send this quickly? I've seen bug reports due to this
too many times.

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[Ingo Molnar]

* James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> This:
> 
> commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
> Author: Glauber Costa <gcosta@redhat.com>
> Date:   Tue Apr 8 13:20:56 2008 -0300
> 
>     x86: merge iommu initialization parameters
> 
> Moved the forbid_dac parameter into pci-dma.c but forgot that it's a 
> PCI only symbol thus causing a compile failure if CONFIG_PCI=N.  Fix 
> by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI

uhm, that's wrong James, the above commit you refer to is half a year 
old and has been released in v2.6.26 and v2.6.27 ;-)

The patch that broke the !CONFIG_PCI build in this cycle is this one:

| From 5b6985ce8ec7127b4d60ad450b64ca8b82748a3b Mon Sep 17 00:00:00 2001
| From: Fenghua Yu <fenghua.yu@intel.com>
| Date: Thu, 16 Oct 2008 18:02:32 -0700
| Subject: [PATCH] intel-iommu: IA64 support
|
| The current Intel IOMMU code assumes that both host page size and 
| Intel IOMMU page size are 4KiB. 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.
|
| [dwmw2: some cleanup]
| Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
| Signed-off-by: Tony Luck <tony.luck@intel.com>
| Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>


> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> ---
> 
> diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> index 1972266..47c5a7a 100644
> --- a/arch/x86/kernel/pci-dma.c
> +++ b/arch/x86/kernel/pci-dma.c
> @@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
>  			iommu_merge = 0;
>  		if (!strncmp(p, "forcesac", 8))
>  			iommu_sac_force = 1;
> +#ifdef CONFIG_PCI
>  		if (!strncmp(p, "allowdac", 8))
>  			forbid_dac = 0;
>  		if (!strncmp(p, "nodac", 5))
> @@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
>  			forbid_dac = -1;
>  			return 1;
>  		}
> +#endif

that's the wrong fix, the right fix from Fenghua Yu (from about a week 
ago) is at:

  http://marc.info/?l=linux-kernel&m=122480590627590&w=2

and it is in the PCI tree already.

	Ingo

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[James Bottomley]

On Wed, 2008-10-29 at 08:36 +0100, Ingo Molnar wrote:
> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > This:
> > 
> > commit fae9a0d8ca68a14da8d2351ad3e0bf42f3b29899
> > Author: Glauber Costa <gcosta@redhat.com>
> > Date:   Tue Apr 8 13:20:56 2008 -0300
> > 
> >     x86: merge iommu initialization parameters
> > 
> > Moved the forbid_dac parameter into pci-dma.c but forgot that it's a 
> > PCI only symbol thus causing a compile failure if CONFIG_PCI=N.  Fix 
> > by surrounding the set clause in iommu_setup with #ifdef CONFIG_PCI
> 
> uhm, that's wrong James, the above commit you refer to is half a year 
> old and has been released in v2.6.26 and v2.6.27 ;-)
> 
> The patch that broke the !CONFIG_PCI build in this cycle is this one:
> 
> | From 5b6985ce8ec7127b4d60ad450b64ca8b82748a3b Mon Sep 17 00:00:00 2001
> | From: Fenghua Yu <fenghua.yu@intel.com>
> | Date: Thu, 16 Oct 2008 18:02:32 -0700
> | Subject: [PATCH] intel-iommu: IA64 support
> |
> | The current Intel IOMMU code assumes that both host page size and 
> | Intel IOMMU page size are 4KiB. 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.
> |
> | [dwmw2: some cleanup]
> | Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> | Signed-off-by: Tony Luck <tony.luck@intel.com>
> | Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

Hmm, well, the triage could be wrong ... however the prior patch seemed
to move forbid_dac out from under the PCI defines.

> > Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > ---
> > 
> > diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
> > index 1972266..47c5a7a 100644
> > --- a/arch/x86/kernel/pci-dma.c
> > +++ b/arch/x86/kernel/pci-dma.c
> > @@ -202,6 +202,7 @@ static __init int iommu_setup(char *p)
> >  			iommu_merge = 0;
> >  		if (!strncmp(p, "forcesac", 8))
> >  			iommu_sac_force = 1;
> > +#ifdef CONFIG_PCI
> >  		if (!strncmp(p, "allowdac", 8))
> >  			forbid_dac = 0;
> >  		if (!strncmp(p, "nodac", 5))
> > @@ -210,6 +211,7 @@ static __init int iommu_setup(char *p)
> >  			forbid_dac = -1;
> >  			return 1;
> >  		}
> > +#endif
> 
> that's the wrong fix, the right fix from Fenghua Yu (from about a week 
> ago) is at:
> 
>   http://marc.info/?l=linux-kernel&m=122480590627590&w=2
> 
> and it is in the PCI tree already.

This is obviously some strange definition of the word "right" of which I
was previously unaware.  That patch moves forbid_dac plus a load of
quirk processing (also for a PCI bus) out from under CONFIG_PCI only ...
which will fix the compile error, sure.

However, if you'd be so kind, please explain how a DAC (meaning Dual
Addressing Cycle on the PCI bus) is useful (or even can be effected)
without a PCI bus?

All its really doing is contaminating pci-dma.c with clutter that only
needs to be there because someone can't get the separation right.

James

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[Arjan van de Ven]

On Wed, 29 Oct 2008 09:01:46 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> This is obviously some strange definition of the word "right" of
> which I was previously unaware.  That patch moves forbid_dac plus a
> load of quirk processing (also for a PCI bus) out from under
> CONFIG_PCI only ... which will fix the compile error, sure.
> 
> However, if you'd be so kind, please explain how a DAC (meaning Dual
> Addressing Cycle on the PCI bus) is useful (or even can be effected)
> without a PCI bus?
> 
> All its really doing is contaminating pci-dma.c with clutter that only
> needs to be there because someone can't get the separation right.o
>

why do you even want that file for CONFIG_PCI=n ??

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[James Bottomley]

On Wed, 2008-10-29 at 07:08 -0700, Arjan van de Ven wrote:
> On Wed, 29 Oct 2008 09:01:46 -0500
> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> 
> > This is obviously some strange definition of the word "right" of
> > which I was previously unaware.  That patch moves forbid_dac plus a
> > load of quirk processing (also for a PCI bus) out from under
> > CONFIG_PCI only ... which will fix the compile error, sure.
> > 
> > However, if you'd be so kind, please explain how a DAC (meaning Dual
> > Addressing Cycle on the PCI bus) is useful (or even can be effected)
> > without a PCI bus?
> > 
> > All its really doing is contaminating pci-dma.c with clutter that only
> > needs to be there because someone can't get the separation right.o
> >
> 
> why do you even want that file for CONFIG_PCI=n ??

Um, my fault from long ago.  It's contaminated with the dma_ API pieces
that are bus generic.

The correct solution, I think, is to split it out into a bus generic
piece and a PCI specific piece.  I think all the intel/amd IOMMU stuff
should be in the PCI specific piece ... although I know there's a
theoretical case where the AMD iommu can be ht bus only with no PCI bus,
I don't think anyone's built such a beast, in which case it's safe to
condition iommu presence on CONFIG_PCI?

James

Re: [PATCH] x86: Fix CONFIG_PCI=n compile failure

[Arjan van de Ven]

On Wed, 29 Oct 2008 09:18:03 -0500
James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> On Wed, 2008-10-29 at 07:08 -0700, Arjan van de Ven wrote:
> > On Wed, 29 Oct 2008 09:01:46 -0500
> > James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> > 
> > > This is obviously some strange definition of the word "right" of
> > > which I was previously unaware.  That patch moves forbid_dac plus
> > > a load of quirk processing (also for a PCI bus) out from under
> > > CONFIG_PCI only ... which will fix the compile error, sure.
> > > 
> > > However, if you'd be so kind, please explain how a DAC (meaning
> > > Dual Addressing Cycle on the PCI bus) is useful (or even can be
> > > effected) without a PCI bus?
> > > 
> > > All its really doing is contaminating pci-dma.c with clutter that
> > > only needs to be there because someone can't get the separation
> > > right.o
> > >
> > 
> > why do you even want that file for CONFIG_PCI=n ??
> 
> Um, my fault from long ago.  It's contaminated with the dma_ API
> pieces that are bus generic.
> 
> The correct solution, I think, is to split it out into a bus generic
> piece and a PCI specific piece.  

agreed

> I think all the intel/amd IOMMU stuff
> should be in the PCI specific piece ... although I know there's a
> theoretical case where the AMD iommu can be ht bus only with no PCI
> bus, I don't think anyone's built such a beast, in which case it's
> safe to condition iommu presence on CONFIG_PCI?

I would think that's not relevant; AMD seems to have PCI in it's cpu
package already, never mind what it is connected to.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

[PATCH] x86 separate PCI out from DMA operations

[James Bottomley]

This splits pci-dma.c into two pieces (a PCI specific pci-dma.c and a
generic dma.c) and conditions the compile of the PCI specific piece on
CONFIG_PCI.

This has the benefit of sweeping all the IOMMU glue into PCI specific
files.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>

---

Note: it's possible to merge pci-nommu into this if desired, since that
file really has nothing to do with PCI and everything to do with generic
dma operations.

Also, I used the CONFIG_PCI conditioning dma_supported() rather than
weak symbol trickery because of the EXPORT_SYMBOL() requirements.

James

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 58814cc..be44178 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -34,7 +34,8 @@ obj-$(CONFIG_X86_32)	+= sys_i386_32.o i386_ksyms_32.o
 obj-$(CONFIG_X86_64)	+= sys_x86_64.o x8664_ksyms_64.o
 obj-$(CONFIG_X86_64)	+= syscall_64.o vsyscall_64.o
 obj-y			+= bootflag.o e820.o
-obj-y			+= pci-dma.o quirks.o i8237.o topology.o kdebugfs.o
+obj-$(CONFIG_PCI)	+= pci-dma.o
+obj-y			+= dma.o quirks.o i8237.o topology.o kdebugfs.o
 obj-y			+= alternative.o i8253.o pci-nommu.o
 obj-y			+= tsc.o io_delay.o rtc.o
 
diff --git a/arch/x86/kernel/pci-dma.c b/arch/x86/kernel/pci-dma.c
index 1926248..8cbf10c 100644
--- a/arch/x86/kernel/pci-dma.c
+++ b/arch/x86/kernel/pci-dma.c
@@ -11,9 +11,6 @@
 
 static int forbid_dac __read_mostly;
 
-struct dma_mapping_ops *dma_ops;
-EXPORT_SYMBOL(dma_ops);
-
 static int iommu_sac_force __read_mostly;
 
 #ifdef CONFIG_IOMMU_DEBUG
@@ -35,30 +32,6 @@ int iommu_detected __read_mostly = 0;
 int iommu_bio_merge __read_mostly = 0;
 EXPORT_SYMBOL(iommu_bio_merge);
 
-dma_addr_t bad_dma_address __read_mostly = 0;
-EXPORT_SYMBOL(bad_dma_address);
-
-/* Dummy device used for NULL arguments (normally ISA). Better would
-   be probably a smaller DMA mask, but this is bug-to-bug compatible
-   to older i386. */
-struct device x86_dma_fallback_dev = {
-	.bus_id = "fallback device",
-	.coherent_dma_mask = DMA_32BIT_MASK,
-	.dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
-};
-EXPORT_SYMBOL(x86_dma_fallback_dev);
-
-int dma_set_mask(struct device *dev, u64 mask)
-{
-	if (!dev->dma_mask || !dma_supported(dev, mask))
-		return -EIO;
-
-	*dev->dma_mask = mask;
-
-	return 0;
-}
-EXPORT_SYMBOL(dma_set_mask);
-
 #ifdef CONFIG_X86_64
 static __initdata void *dma32_bootmem_ptr;
 static unsigned long dma32_bootmem_size __initdata = (128ULL<<20);
@@ -134,37 +107,6 @@ unsigned long iommu_nr_pages(unsigned long addr, unsigned long len)
 EXPORT_SYMBOL(iommu_nr_pages);
 #endif
 
-void *dma_generic_alloc_coherent(struct device *dev, size_t size,
-				 dma_addr_t *dma_addr, gfp_t flag)
-{
-	unsigned long dma_mask;
-	struct page *page;
-	dma_addr_t addr;
-
-	dma_mask = dma_alloc_coherent_mask(dev, flag);
-
-	flag |= __GFP_ZERO;
-again:
-	page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
-	if (!page)
-		return NULL;
-
-	addr = page_to_phys(page);
-	if (!is_buffer_dma_capable(dma_mask, addr, size)) {
-		__free_pages(page, get_order(size));
-
-		if (dma_mask < DMA_32BIT_MASK && !(flag & GFP_DMA)) {
-			flag = (flag & ~GFP_DMA32) | GFP_DMA;
-			goto again;
-		}
-
-		return NULL;
-	}
-
-	*dma_addr = addr;
-	return page_address(page);
-}
-
 /*
  * See <Documentation/x86_64/boot-options.txt> for the iommu kernel parameter
  * documentation.
@@ -232,16 +174,15 @@ static __init int iommu_setup(char *p)
 }
 early_param("iommu", iommu_setup);
 
+/* the non PCI version of this file is in dma.c */
 int dma_supported(struct device *dev, u64 mask)
 {
 	struct dma_mapping_ops *ops = get_dma_ops(dev);
 
-#ifdef CONFIG_PCI
 	if (mask > 0xffffffff && forbid_dac > 0) {
 		dev_info(dev, "PCI: Disallowing DAC for device\n");
 		return 0;
 	}
-#endif
 
 	if (ops->dma_supported)
 		return ops->dma_supported(dev, mask);
@@ -273,7 +214,7 @@ int dma_supported(struct device *dev, u64 mask)
 }
 EXPORT_SYMBOL(dma_supported);
 
-static int __init pci_iommu_init(void)
+void __init pci_iommu_init(void)
 {
 	calgary_iommu_init();
 
@@ -283,18 +224,15 @@ static int __init pci_iommu_init(void)
 
 	gart_iommu_init();
 
-	no_iommu_init();
-	return 0;
+	if (!dma_ops)
+		force_iommu = 0;
 }
 
 void pci_iommu_shutdown(void)
 {
 	gart_iommu_shutdown();
 }
-/* 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)
@@ -306,4 +244,3 @@ static __devinit void via_no_dac(struct pci_dev *dev)
 	}
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_VIA, PCI_ANY_ID, via_no_dac);
-#endif
diff --git a/arch/x86/kernel/pci-nommu.c b/arch/x86/kernel/pci-nommu.c
index c70ab5a..5e7dd85 100644
--- a/arch/x86/kernel/pci-nommu.c
+++ b/arch/x86/kernel/pci-nommu.c
@@ -90,7 +90,5 @@ void __init no_iommu_init(void)
 {
 	if (dma_ops)
 		return;
-
-	force_iommu = 0; /* no HW IOMMU */
 	dma_ops = &nommu_dma_ops;
 }
diff --git a/dev/null b/arch/x86/kernel/dma.c
new file mode 100644
index 0000000..0896152
--- /dev/null
+++ b/arch/x86/kernel/dma.c
@@ -0,0 +1,104 @@
+#include <linux/dma-mapping.h>
+#include <linux/dmar.h>
+#include <linux/bootmem.h>
+
+#include <asm/proto.h>
+#include <asm/dma.h>
+#include <asm/iommu.h>
+
+struct dma_mapping_ops *dma_ops;
+EXPORT_SYMBOL(dma_ops);
+
+dma_addr_t bad_dma_address __read_mostly = 0;
+EXPORT_SYMBOL(bad_dma_address);
+
+/* Dummy device used for NULL arguments (normally ISA). Better would
+   be probably a smaller DMA mask, but this is bug-to-bug compatible
+   to older i386. */
+struct device x86_dma_fallback_dev = {
+	.bus_id = "fallback device",
+	.coherent_dma_mask = DMA_32BIT_MASK,
+	.dma_mask = &x86_dma_fallback_dev.coherent_dma_mask,
+};
+EXPORT_SYMBOL(x86_dma_fallback_dev);
+
+int dma_set_mask(struct device *dev, u64 mask)
+{
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+void *dma_generic_alloc_coherent(struct device *dev, size_t size,
+				 dma_addr_t *dma_addr, gfp_t flag)
+{
+	unsigned long dma_mask;
+	struct page *page;
+	dma_addr_t addr;
+
+	dma_mask = dma_alloc_coherent_mask(dev, flag);
+
+	flag |= __GFP_ZERO;
+again:
+	page = alloc_pages_node(dev_to_node(dev), flag, get_order(size));
+	if (!page)
+		return NULL;
+
+	addr = page_to_phys(page);
+	if (!is_buffer_dma_capable(dma_mask, addr, size)) {
+		__free_pages(page, get_order(size));
+
+		if (dma_mask < DMA_32BIT_MASK && !(flag & GFP_DMA)) {
+			flag = (flag & ~GFP_DMA32) | GFP_DMA;
+			goto again;
+		}
+
+		return NULL;
+	}
+
+	*dma_addr = addr;
+	return page_address(page);
+}
+
+#ifndef CONFIG_PCI
+int dma_supported(struct device *dev, u64 mask)
+{
+	struct dma_mapping_ops *ops = get_dma_ops(dev);
+
+	if (ops->dma_supported)
+		return ops->dma_supported(dev, mask);
+
+	/* Copied from i386. Doesn't make much sense, because it will
+	   only work for pci_alloc_coherent.
+	   The caller just has to use GFP_DMA in this case. */
+	if (mask < DMA_24BIT_MASK)
+		return 0;
+
+	return 1;
+}
+EXPORT_SYMBOL(dma_supported);
+#endif
+
+/* dummy weak initialisation function.  If CONFIG_PCI is selected
+ * pci-dma.c has a strong version of this which gets called instead */
+void __init __weak pci_iommu_init(void)
+{
+}
+
+static __init int dma_init(void)
+{
+	pci_iommu_init();
+
+	no_iommu_init();
+
+	return 0;
+}
+
+/* Must execute after PCI subsystem */
+fs_initcall(dma_init);
+
+