Re: [GIT *] intel-iommu updates for 2.6.30 (second batch)

David Woodhouse
2009-04-07T06:05:35Z

On Tue, 2009-04-07 at 07:52 +0200, Ingo Molnar wrote:
> * Ingo Molnar <mingo@elte.hu> wrote:
>
> >
> > * David Woodhouse <dwmw2@infradead.org> wrote:
> >
> > > On Tue, 2009-04-07 at 07:37 +0200, Ingo Molnar wrote:
> > > > I suspect these bits are the ones that broke the upstream build:
> > > >
> > > > drivers/pci/dmar.c:47: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or
> > > > ‘__attribute__’ before ‘dmar_tbl_size’
> > > > drivers/pci/dmar.c:62: warning: ‘struct acpi_dmar_device_scope’
> > > > declared inside parameter list
> > > > drivers/pci/dmar.c:62: warning: its scope is only this definition or
> > > > declaration, which is probably not what you want
> > >
> > > Yeah, <acpi/acpi.h> was being included implicitly for me, but
> > > certain configs don't do that. Alexander Beregalov sent a patch
> > > for that, which I added to my tree before Linus pulled it.
> > >
> > > Commit 46f06b72378d3187f0d12f7a60d020676bfbf332 is the fix.
> >
> > No, that does not fix it - it's still broken with
> > v2.6.29-9854-gd508afb. Try the config i sent.
>
> The problem is INTR_REMAP. That brings in DMAR but not ACPI.

Er, ...

config INTR_REMAP
bool "Support for Interrupt Remapping (EXPERIMENTAL)"
depends on X86_64 && X86_IO_APIC && PCI_MSI && ACPI && EXPERIMENTAL

> The patch below fixes it, but it is only an ugly workaround: we
> really dont want 'depends on ACPI' dependencies to spread like that.

It _does_ depend on ACPI. Much as I wish it didn't, it's fundamentally
tied to it for now.

> ACPI is a hardware discovery mechanism. It might be the only way to
> discover these pieces of hardware at the moment, but we should not
> tie method of discovery to hardware support. I'd suggest a
> dmar_acpi.c splitout of the ACPI bits.

As I said, DMAR is the name of an ACPI table. If we ever manage to get
non-ACPI discovery of the hardware, we can write new code which mentions
_neither_ DMAR nor ACPI. And at that point, we can talk about moving any
generic parts out of the existing 'dmar.c'. Perhaps into intel-iommu.c?

It's a bit premature to do that now, though -- especially given the
unfortunate likelihood that a sensible discovery method will never
happen.

I think the problem here is actually the fact that we used 'select'
instead of proper dependencies. I wouldn't normally have used the
abomination that is 'select', but since I was just moving things around
I ended up doing so by accident.

X2APIC currently selects INTR_REMAP, and the bug would never have
happened if we'd made X2APIC _depend_ on INTR_REMAP instead.

This is a better fix:

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 4b34082..bc25b9f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -252,17 +252,13 @@ config SMP

config X86_X2APIC
bool "Support x2apic"
- depends on X86_LOCAL_APIC && X86_64
- select INTR_REMAP
+ depends on X86_LOCAL_APIC && X86_64 && INTR_REMAP
---help---
This enables x2apic support on CPUs that have this feature.

This allows 32-bit apic IDs (so it can support very large systems),
and accesses the local apic via MSRs not via mmio.

- ( On certain CPU models you may need to enable INTR_REMAP too,
- to get functional x2apic mode. )
-
If you don't know what to do here, say N.

config SPARSE_IRQ

--
dwmw2