diff mbox

[v4] irq: add quirk for broken interrupt remapping on 55XX chipsets

Message ID 1365088091-10862-1-git-send-email-nhorman@tuxdriver.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Neil Horman April 4, 2013, 3:08 p.m. UTC
A few years back intel published a spec update:
http://www.intel.com/content/dam/doc/specification-update/5520-and-5500-chipset-ioh-specification-update.pdf

For the 5520 and 5500 chipsets which contained an errata (specificially errata
53), which noted that these chipsets can't properly do interrupt remapping, and
as a result the recommend that interrupt remapping be disabled in bios.  While
many vendors have a bios update to do exactly that, not all do, and of course
not all users update their bios to a level that corrects the problem.  As a
result, occasionally interrupts can arrive at a cpu even after affinity for that
interrupt has be moved, leading to lost or spurrious interrupts (usually
characterized by the message:
kernel: do_IRQ: 7.71 No irq handler for vector (irq -1)

There have been several incidents recently of people seeing this error, and
investigation has shown that they have system for which their BIOS level is such
that this feature was not properly turned off.  As such, it would be good to
give them a reminder that their systems are vulnurable to this problem.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Prarit Bhargava <prarit@redhat.com>
CC: Don Zickus <dzickus@redhat.com>
CC: Don Dutile <ddutile@redhat.com>
CC: Bjorn Helgaas <bhelgaas@google.com>
CC: Asit Mallick <asit.k.mallick@intel.com>
CC: David Woodhouse <dwmw2@infradead.org>
CC: linux-pci@vger.kernel.org
---

Change notes:

v2)

* Moved the quirk to the x86 arch, since consensus seems to be that the 55XX
chipset series is x86 only.  I decided however to keep the quirk as a regular
quirk, not an early_quirk.  Early quirks have no way currently to determine if
BIOS has properly disabled the feature in the iommu, at least not without
significant hacking, and since its quite possible this will be a short lived
quirk, should Don Z's workaround code prove successful (and it looks like it may
well), I don't think that necessecary.

* Removed the WARNING banner from the quirk, and added the HW_ERR token to the
string, I opted to leave the newlines in place however, as I really couldnt
find a way to keep the text on a single line is still legible from a code
perspective.  I think theres enough language in there that using cscope on just
about any substring however will turn it up, and again, this may be a short
lived quirk.

v3)

* Removed defines from pci_ids.h, and used direct id values as per request from
Bjorn.

v4)

* Converted pr_warn to WARN_TAINT(TAINT_FIRMWARE_WORKAROUND) as per David
Woodhouse
---
 arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

Comments

Yinghai Lu April 4, 2013, 4:16 p.m. UTC | #1
On Thu, Apr 4, 2013 at 8:08 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> A few years back intel published a spec update:
> http://www.intel.com/content/dam/doc/specification-update/5520-and-5500-chipset-ioh-specification-update.pdf
>
> For the 5520 and 5500 chipsets which contained an errata (specificially errata
> 53), which noted that these chipsets can't properly do interrupt remapping, and
> as a result the recommend that interrupt remapping be disabled in bios.  While
> many vendors have a bios update to do exactly that, not all do, and of course
> not all users update their bios to a level that corrects the problem.  As a
> result, occasionally interrupts can arrive at a cpu even after affinity for that
> interrupt has be moved, leading to lost or spurrious interrupts (usually
> characterized by the message:
> kernel: do_IRQ: 7.71 No irq handler for vector (irq -1)
>
> There have been several incidents recently of people seeing this error, and
> investigation has shown that they have system for which their BIOS level is such
> that this feature was not properly turned off.  As such, it would be good to
> give them a reminder that their systems are vulnurable to this problem.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Prarit Bhargava <prarit@redhat.com>
> CC: Don Zickus <dzickus@redhat.com>
> CC: Don Dutile <ddutile@redhat.com>
> CC: Bjorn Helgaas <bhelgaas@google.com>
> CC: Asit Mallick <asit.k.mallick@intel.com>
> CC: David Woodhouse <dwmw2@infradead.org>
> CC: linux-pci@vger.kernel.org
> ---
>
> Change notes:
>
> v2)
>
> * Moved the quirk to the x86 arch, since consensus seems to be that the 55XX
> chipset series is x86 only.  I decided however to keep the quirk as a regular
> quirk, not an early_quirk.  Early quirks have no way currently to determine if
> BIOS has properly disabled the feature in the iommu, at least not without
> significant hacking, and since its quite possible this will be a short lived
> quirk, should Don Z's workaround code prove successful (and it looks like it may
> well), I don't think that necessecary.
>
> * Removed the WARNING banner from the quirk, and added the HW_ERR token to the
> string, I opted to leave the newlines in place however, as I really couldnt
> find a way to keep the text on a single line is still legible from a code
> perspective.  I think theres enough language in there that using cscope on just
> about any substring however will turn it up, and again, this may be a short
> lived quirk.
>
> v3)
>
> * Removed defines from pci_ids.h, and used direct id values as per request from
> Bjorn.
>
> v4)
>
> * Converted pr_warn to WARN_TAINT(TAINT_FIRMWARE_WORKAROUND) as per David
> Woodhouse
> ---
>  arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> index 26ee48a..eb0785d 100644
> --- a/arch/x86/kernel/quirks.c
> +++ b/arch/x86/kernel/quirks.c
> @@ -5,6 +5,7 @@
>  #include <linux/irq.h>
>
>  #include <asm/hpet.h>
> +#include "../../../drivers/iommu/irq_remapping.h"
>
>  #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
>
> @@ -567,3 +568,20 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F5,
>                         quirk_amd_nb_node);
>
>  #endif
> +
> +static void intel_remapping_check(struct pci_dev *dev)
> +{
> +       u8 revision;
> +
> +       pci_read_config_byte(dev, PCI_REVISION_ID, &revision);
> +
> +       WARN_TAINT(((revision == 0x13) && irq_remapping_enabled),
> +                  TAINT_FIRMWARE_WORKAROUND,
> +                   "This system BIOS has enabled interrupt remapping\n"
> +                   "on a chipset that contains an erratum making that\n"
> +                   "feature unstable.  Please reboot with nointremap\n"
> +                   "added to the kernel command line and contact\n"
> +                   "your BIOS vendor for an update");
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3406, intel_remapping_check);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3403, intel_remapping_check);

No, you only address my one request, move to arch/x86 directory.

You need to move the quirk to early_quirk to append nointremap to
avoid extra rebooting.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Donald Dutile April 4, 2013, 5:27 p.m. UTC | #2
On 04/04/2013 12:16 PM, Yinghai Lu wrote:
> On Thu, Apr 4, 2013 at 8:08 AM, Neil Horman<nhorman@tuxdriver.com>  wrote:
>> A few years back intel published a spec update:
>> http://www.intel.com/content/dam/doc/specification-update/5520-and-5500-chipset-ioh-specification-update.pdf
>>
>> For the 5520 and 5500 chipsets which contained an errata (specificially errata
>> 53), which noted that these chipsets can't properly do interrupt remapping, and
>> as a result the recommend that interrupt remapping be disabled in bios.  While
>> many vendors have a bios update to do exactly that, not all do, and of course
>> not all users update their bios to a level that corrects the problem.  As a
>> result, occasionally interrupts can arrive at a cpu even after affinity for that
>> interrupt has be moved, leading to lost or spurrious interrupts (usually
>> characterized by the message:
>> kernel: do_IRQ: 7.71 No irq handler for vector (irq -1)
>>
>> There have been several incidents recently of people seeing this error, and
>> investigation has shown that they have system for which their BIOS level is such
>> that this feature was not properly turned off.  As such, it would be good to
>> give them a reminder that their systems are vulnurable to this problem.
>>
>> Signed-off-by: Neil Horman<nhorman@tuxdriver.com>
>> CC: Prarit Bhargava<prarit@redhat.com>
>> CC: Don Zickus<dzickus@redhat.com>
>> CC: Don Dutile<ddutile@redhat.com>
>> CC: Bjorn Helgaas<bhelgaas@google.com>
>> CC: Asit Mallick<asit.k.mallick@intel.com>
>> CC: David Woodhouse<dwmw2@infradead.org>
>> CC: linux-pci@vger.kernel.org
>> ---
>>
>> Change notes:
>>
>> v2)
>>
>> * Moved the quirk to the x86 arch, since consensus seems to be that the 55XX
>> chipset series is x86 only.  I decided however to keep the quirk as a regular
>> quirk, not an early_quirk.  Early quirks have no way currently to determine if
>> BIOS has properly disabled the feature in the iommu, at least not without
>> significant hacking, and since its quite possible this will be a short lived
>> quirk, should Don Z's workaround code prove successful (and it looks like it may
>> well), I don't think that necessecary.
>>
>> * Removed the WARNING banner from the quirk, and added the HW_ERR token to the
>> string, I opted to leave the newlines in place however, as I really couldnt
>> find a way to keep the text on a single line is still legible from a code
>> perspective.  I think theres enough language in there that using cscope on just
>> about any substring however will turn it up, and again, this may be a short
>> lived quirk.
>>
>> v3)
>>
>> * Removed defines from pci_ids.h, and used direct id values as per request from
>> Bjorn.
>>
>> v4)
>>
>> * Converted pr_warn to WARN_TAINT(TAINT_FIRMWARE_WORKAROUND) as per David
>> Woodhouse
>> ---
>>   arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
>> index 26ee48a..eb0785d 100644
>> --- a/arch/x86/kernel/quirks.c
>> +++ b/arch/x86/kernel/quirks.c
>> @@ -5,6 +5,7 @@
>>   #include<linux/irq.h>
>>
>>   #include<asm/hpet.h>
>> +#include "../../../drivers/iommu/irq_remapping.h"
>>
>>   #if defined(CONFIG_X86_IO_APIC)&&  defined(CONFIG_SMP)&&  defined(CONFIG_PCI)
>>
>> @@ -567,3 +568,20 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F5,
>>                          quirk_amd_nb_node);
>>
>>   #endif
>> +
>> +static void intel_remapping_check(struct pci_dev *dev)
>> +{
>> +       u8 revision;
>> +
>> +       pci_read_config_byte(dev, PCI_REVISION_ID,&revision);
>> +
>> +       WARN_TAINT(((revision == 0x13)&&  irq_remapping_enabled),
>> +                  TAINT_FIRMWARE_WORKAROUND,
>> +                   "This system BIOS has enabled interrupt remapping\n"
>> +                   "on a chipset that contains an erratum making that\n"
>> +                   "feature unstable.  Please reboot with nointremap\n"
>> +                   "added to the kernel command line and contact\n"
>> +                   "your BIOS vendor for an update");
>> +}
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3406, intel_remapping_check);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3403, intel_remapping_check);
>
> No, you only address my one request, move to arch/x86 directory.
>
> You need to move the quirk to early_quirk to append nointremap to
> avoid extra rebooting.
>
> Thanks
>
> Yinghai
The pci-dev's of all the (minimally, root, 5500-chipset) pci-dev's are known/scanned/created by that time?

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu April 4, 2013, 5:40 p.m. UTC | #3
On Thu, Apr 4, 2013 at 10:27 AM, Don Dutile <ddutile@redhat.com> wrote:
>> You need to move the quirk to early_quirk to append nointremap to
>> avoid extra rebooting.
>>
> The pci-dev's of all the (minimally, root, 5500-chipset) pci-dev's are
> known/scanned/created by that time?

in arch/x86/kernel/early-quirk.c

and on top of
https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/commit/?h=for-x86-early-quirk-usb&id=de38757e964cfee20e6da1977572a2191d7f4aa0

You could add one entry in early_qrk[].

Some one already try to use that path to disable x2apic on some thinkpad.

So it should work on nointrremap too.

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Neil Horman April 4, 2013, 8:04 p.m. UTC | #4
On Thu, Apr 04, 2013 at 10:40:07AM -0700, Yinghai Lu wrote:
> On Thu, Apr 4, 2013 at 10:27 AM, Don Dutile <ddutile@redhat.com> wrote:
> >> You need to move the quirk to early_quirk to append nointremap to
> >> avoid extra rebooting.
> >>
> > The pci-dev's of all the (minimally, root, 5500-chipset) pci-dev's are
> > known/scanned/created by that time?
> 
> in arch/x86/kernel/early-quirk.c
> 
> and on top of
> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/commit/?h=for-x86-early-quirk-usb&id=de38757e964cfee20e6da1977572a2191d7f4aa0
> 
> You could add one entry in early_qrk[].
> 
> Some one already try to use that path to disable x2apic on some thinkpad.
> 
> So it should work on nointrremap too.
> 
See my last email to Bjorn.  Doing this in early-quirks in such a way that we
can detect an iommu that has interrupt remapping enabled (so we don't just
unilaterally print this quirk all the time) requires that we be able to parse
acpi tables very early in the boot.  If you know of how to do that, I can make
this happen.  If not, I suppose another alternative would be to have the early
quirk set a flag that tells us this is a bogus chip, and if we try to enable irq
remapping with that flag set, we should fail, and report the error at that time,
but I'm not sure I like that solution.

Neil

> Thanks
> 
> Yinghai
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas April 4, 2013, 8:33 p.m. UTC | #5
On Thu, Apr 4, 2013 at 2:04 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Apr 04, 2013 at 10:40:07AM -0700, Yinghai Lu wrote:
>> On Thu, Apr 4, 2013 at 10:27 AM, Don Dutile <ddutile@redhat.com> wrote:
>> >> You need to move the quirk to early_quirk to append nointremap to
>> >> avoid extra rebooting.
>> >>
>> > The pci-dev's of all the (minimally, root, 5500-chipset) pci-dev's are
>> > known/scanned/created by that time?
>>
>> in arch/x86/kernel/early-quirk.c
>>
>> and on top of
>> https://git.kernel.org/cgit/linux/kernel/git/yinghai/linux-yinghai.git/commit/?h=for-x86-early-quirk-usb&id=de38757e964cfee20e6da1977572a2191d7f4aa0
>>
>> You could add one entry in early_qrk[].
>>
>> Some one already try to use that path to disable x2apic on some thinkpad.
>>
>> So it should work on nointrremap too.
>>
> See my last email to Bjorn.  Doing this in early-quirks in such a way that we
> can detect an iommu that has interrupt remapping enabled (so we don't just
> unilaterally print this quirk all the time) requires that we be able to parse
> acpi tables very early in the boot.  If you know of how to do that, I can make
> this happen.  If not, I suppose another alternative would be to have the early
> quirk set a flag that tells us this is a bogus chip, and if we try to enable irq
> remapping with that flag set, we should fail, and report the error at that time,
> but I'm not sure I like that solution.

I like that solution :)  It seems very simple -- you don't have to
parse any tables or anything.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Yinghai Lu April 4, 2013, 9:11 p.m. UTC | #6
On Thu, Apr 4, 2013 at 1:33 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> See my last email to Bjorn.  Doing this in early-quirks in such a way that we
>> can detect an iommu that has interrupt remapping enabled (so we don't just
>> unilaterally print this quirk all the time) requires that we be able to parse
>> acpi tables very early in the boot.  If you know of how to do that, I can make
>> this happen.  If not, I suppose another alternative would be to have the early
>> quirk set a flag that tells us this is a bogus chip, and if we try to enable irq
>> remapping with that flag set, we should fail, and report the error at that time,
>> but I'm not sure I like that solution.
>
> I like that solution :)  It seems very simple -- you don't have to
> parse any tables or anything.

You are right, we don't need to parse any acpi tables.

just add one quirk in early-quirk.c to set
   disable_irq_remap = 1;

Thanks

Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Neil Horman April 5, 2013, 12:24 a.m. UTC | #7
On Thu, Apr 04, 2013 at 02:11:54PM -0700, Yinghai Lu wrote:
> On Thu, Apr 4, 2013 at 1:33 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> >> See my last email to Bjorn.  Doing this in early-quirks in such a way that we
> >> can detect an iommu that has interrupt remapping enabled (so we don't just
> >> unilaterally print this quirk all the time) requires that we be able to parse
> >> acpi tables very early in the boot.  If you know of how to do that, I can make
> >> this happen.  If not, I suppose another alternative would be to have the early
> >> quirk set a flag that tells us this is a bogus chip, and if we try to enable irq
> >> remapping with that flag set, we should fail, and report the error at that time,
> >> but I'm not sure I like that solution.
> >
> > I like that solution :)  It seems very simple -- you don't have to
> > parse any tables or anything.
> 
> You are right, we don't need to parse any acpi tables.
> 
> just add one quirk in early-quirk.c to set
>    disable_irq_remap = 1;
> 
Well, I can't just do that.  We need to issue a warning to the user as well, and
to do so conditionally (we don't want to warn users who have prorperly updated
BIOSes), I would need to know if irq remapping is actually on or not, which
would require parsing ACPI tables

But, as noted above, I can just set a flag, and defer the printing of the
warning until later in the boot process, when we know that information already.
Bjorn seems on board with that idea, so I'll spin up a patch for it in the AM.

Thanks!
Neil

> Thanks
> 
> Yinghai
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index 26ee48a..eb0785d 100644
--- a/arch/x86/kernel/quirks.c
+++ b/arch/x86/kernel/quirks.c
@@ -5,6 +5,7 @@ 
 #include <linux/irq.h>
 
 #include <asm/hpet.h>
+#include "../../../drivers/iommu/irq_remapping.h"
 
 #if defined(CONFIG_X86_IO_APIC) && defined(CONFIG_SMP) && defined(CONFIG_PCI)
 
@@ -567,3 +568,20 @@  DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_15H_NB_F5,
 			quirk_amd_nb_node);
 
 #endif
+
+static void intel_remapping_check(struct pci_dev *dev)
+{
+	u8 revision;
+
+	pci_read_config_byte(dev, PCI_REVISION_ID, &revision);
+
+	WARN_TAINT(((revision == 0x13) && irq_remapping_enabled),
+		   TAINT_FIRMWARE_WORKAROUND,
+                   "This system BIOS has enabled interrupt remapping\n"
+                   "on a chipset that contains an erratum making that\n"
+                   "feature unstable.  Please reboot with nointremap\n"
+                   "added to the kernel command line and contact\n"
+                   "your BIOS vendor for an update");
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3406, intel_remapping_check);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x3403, intel_remapping_check);