diff mbox

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

Message ID 1362423859-18516-1-git-send-email-nhorman@tuxdriver.com
State Not Applicable
Headers show

Commit Message

Neil Horman March 4, 2013, 7:04 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: 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.
---
 arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
 include/linux/pci_ids.h  |  2 ++
 2 files changed, 20 insertions(+)

Comments

Neil Horman March 9, 2013, 8:49 p.m. UTC | #1
On Mon, Mar 04, 2013 at 02:04:19PM -0500, Neil Horman 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: linux-pci@vger.kernel.org
> 
Ping, anyone want to Ack/Nack this?
Neil

--
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
Myron Stowe March 9, 2013, 10:20 p.m. UTC | #2
On Sat, Mar 9, 2013 at 1:49 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Mon, Mar 04, 2013 at 02:04:19PM -0500, Neil Horman 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: linux-pci@vger.kernel.org
>>
> Ping, anyone want to Ack/Nack this?

Don's comment earlier seems to imply that this is a short term fix and
that a more long term fix may be coming soon.  If that is the case
wouldn't we want to wait for the long term fix and just pull that in?

Myron

> Neil
>
> --
> 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
--
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
Don Dutile March 11, 2013, 1:31 a.m. UTC | #3
On 03/09/2013 05:20 PM, Myron Stowe wrote:
> On Sat, Mar 9, 2013 at 1:49 PM, Neil Horman<nhorman@tuxdriver.com>  wrote:
>> On Mon, Mar 04, 2013 at 02:04:19PM -0500, Neil Horman 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: linux-pci@vger.kernel.org
>>>
>> Ping, anyone want to Ack/Nack this?
>
> Don's comment earlier seems to imply that this is a short term fix and
> that a more long term fix may be coming soon.  If that is the case
> wouldn't we want to wait for the long term fix and just pull that in?
>
> Myron
>
At the time of Neil's postings, multiple changes were being considered,
and we didn't know how long it would take to verify any one change.
Thus, Neil's patch was proposed to identify a known problem that was
being seen on multiple systems, and it was proposed so further
system issues wouldn't go mis-diagnosed.

We are still testing a minor change, and test results are positive so far.
After we're sure of its logic as it's results, it can be posted and
this patch can be removed if it is taken before we are certain.

As Prarit stated, we should be sure of changes in this area before
throwing this patch away for an option that could yield other failures.

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

--
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 March 11, 2013, 11:25 a.m. UTC | #4
On Sat, Mar 09, 2013 at 03:20:57PM -0700, Myron Stowe wrote:
> On Sat, Mar 9, 2013 at 1:49 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Mon, Mar 04, 2013 at 02:04:19PM -0500, Neil Horman 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: linux-pci@vger.kernel.org
> >>
> > Ping, anyone want to Ack/Nack this?
> 
> Don's comment earlier seems to imply that this is a short term fix and
> that a more long term fix may be coming soon.  If that is the case
> wouldn't we want to wait for the long term fix and just pull that in?
> 
> Myron
> 
As Don and Prarit have mentioned, an alternate change is being worked on and
tested that may work around this issue, but we're not yet sure that it will, and
we're not sure of the time frame for this fix.  Normally I would agree, that it
would be easier just to wait for the long term fix, but as Prarit noted, since
this hardware is in fact broken, I would rather do a both approach.  Its fine if
this gets reverted tomorrow with a longer term fix as far as I'm concerned, its
just caused enough problems already that I'd like to see it in place until the
better solution arrives.
Neil
 
--
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
Prarit Bhargava March 11, 2013, 12:17 p.m. UTC | #5
On 03/11/2013 07:25 AM, Neil Horman wrote:
> On Sat, Mar 09, 2013 at 03:20:57PM -0700, Myron Stowe wrote:
>> On Sat, Mar 9, 2013 at 1:49 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>>> On Mon, Mar 04, 2013 at 02:04:19PM -0500, Neil Horman 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: linux-pci@vger.kernel.org
>>>>
>>> Ping, anyone want to Ack/Nack this?
>>
>> Don's comment earlier seems to imply that this is a short term fix and
>> that a more long term fix may be coming soon.  If that is the case
>> wouldn't we want to wait for the long term fix and just pull that in?
>>
>> Myron
>>
> As Don and Prarit have mentioned, an alternate change is being worked on and
> tested that may work around this issue, but we're not yet sure that it will, and
> we're not sure of the time frame for this fix.  Normally I would agree, that it
> would be easier just to wait for the long term fix, but as Prarit noted, since
> this hardware is in fact broken, I would rather do a both approach.  Its fine if
> this gets reverted tomorrow with a longer term fix as far as I'm concerned, its
> just caused enough problems already that I'd like to see it in place until the
> better solution arrives.

I agree with Neil on this.  While vendors are supposed to fix their BIOSes,
experience has shown that not all vendors will fix their BIOSes for a problem
like this.

Ack this quirk.

P.

> Neil
>  
> 

--
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 3, 2013, 11:53 p.m. UTC | #6
[+cc David and iommu list, Yinghai, Jiang]

On Mon, Mar 4, 2013 at 12:04 PM, 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: 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.
> ---
>  arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
>  include/linux/pci_ids.h  |  2 ++
>  2 files changed, 20 insertions(+)
>
> diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> index 26ee48a..a718ea2 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);
> +
> +       if ((revision == 0x13) && irq_remapping_enabled) {
> +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> +                        "on a chipset that contains an errata 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, PCI_DEVICE_ID_INTEL_5520_IOHUB, intel_remapping_check);
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5500_IOHUB, intel_remapping_check);

This started as an IOMMU change, and I'm not an expert in that area,
so I added David and the IOMMU list.  I'd rather have him deal with
this than me.

Is this something we can just *fix* in the kernel, e.g., by turning
off interrupt remapping ourselves, or does it have to be done before
the OS boots?

> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> index 31717bd..54027a6 100644
> --- a/include/linux/pci_ids.h
> +++ b/include/linux/pci_ids.h
> @@ -2732,6 +2732,8 @@
>  #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_RANK_REV2  0x2db2
>  #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_TC_REV2    0x2db3
>  #define PCI_DEVICE_ID_INTEL_82855PM_HB 0x3340
> +#define PCI_DEVICE_ID_INTEL_5500_IOHUB 0x3403
> +#define PCI_DEVICE_ID_INTEL_5520_IOHUB 0x3406

For constants only used in one place, we just use the bare constant
(0x3403) in the quirk rather than editing pci_ids.h (see comment at
the top of that file).

>  #define PCI_DEVICE_ID_INTEL_IOAT_TBG4  0x3429
>  #define PCI_DEVICE_ID_INTEL_IOAT_TBG5  0x342a
>  #define PCI_DEVICE_ID_INTEL_IOAT_TBG6  0x342b
> --
> 1.7.11.7
>
> --
> 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
--
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, 11:17 a.m. UTC | #7
On Wed, Apr 03, 2013 at 05:53:27PM -0600, Bjorn Helgaas wrote:
> [+cc David and iommu list, Yinghai, Jiang]
> 
> On Mon, Mar 4, 2013 at 12:04 PM, 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: 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.
> > ---
> >  arch/x86/kernel/quirks.c | 18 ++++++++++++++++++
> >  include/linux/pci_ids.h  |  2 ++
> >  2 files changed, 20 insertions(+)
> >
> > diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
> > index 26ee48a..a718ea2 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);
> > +
> > +       if ((revision == 0x13) && irq_remapping_enabled) {
> > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> > +                        "on a chipset that contains an errata 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, PCI_DEVICE_ID_INTEL_5520_IOHUB, intel_remapping_check);
> > +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5500_IOHUB, intel_remapping_check);
> 
> This started as an IOMMU change, and I'm not an expert in that area,
> so I added David and the IOMMU list.  I'd rather have him deal with
> this than me.
> 
This was never an iommu change, other than the fact that interrupt mapping and
the iommu are tightly coupled.

> Is this something we can just *fix* in the kernel, e.g., by turning
> off interrupt remapping ourselves, or does it have to be done before
> the OS boots?
> 
Possibly, but it didn't seem safe to me.  As it currently stands, pci quirks are
executed after the apic is configured, which means its possible the problem
might trigger before the quirk has a chance to execute (even the early quirk).
There was an exchange about this earlier in the thread IIRC.

> > diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
> > index 31717bd..54027a6 100644
> > --- a/include/linux/pci_ids.h
> > +++ b/include/linux/pci_ids.h
> > @@ -2732,6 +2732,8 @@
> >  #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_RANK_REV2  0x2db2
> >  #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_TC_REV2    0x2db3
> >  #define PCI_DEVICE_ID_INTEL_82855PM_HB 0x3340
> > +#define PCI_DEVICE_ID_INTEL_5500_IOHUB 0x3403
> > +#define PCI_DEVICE_ID_INTEL_5520_IOHUB 0x3406
> 
> For constants only used in one place, we just use the bare constant
> (0x3403) in the quirk rather than editing pci_ids.h (see comment at
> the top of that file).
> 
Ok, I'll repost with this removed.

Thanks & Regards
Neil

--
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
David Woodhouse April 4, 2013, 2:27 p.m. UTC | #8
On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
> );
> > +
> > +       if ((revision == 0x13) && irq_remapping_enabled) {
> > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> > +                        "on a chipset that contains an errata 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");

This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
Neil Horman April 4, 2013, 2:50 p.m. UTC | #9
On Thu, Apr 04, 2013 at 03:27:29PM +0100, David Woodhouse wrote:
> On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
> > );
> > > +
> > > +       if ((revision == 0x13) && irq_remapping_enabled) {
> > > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> > > +                        "on a chipset that contains an errata 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");
> 
> This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
> 
Ok, copy that. I'll repost shortly

> -- 
> dwmw2


--
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, 2:57 p.m. UTC | #10
On Thu, Apr 4, 2013 at 8:50 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Apr 04, 2013 at 03:27:29PM +0100, David Woodhouse wrote:
>> On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
>> > );
>> > > +
>> > > +       if ((revision == 0x13) && irq_remapping_enabled) {
>> > > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
>> > > +                        "on a chipset that contains an errata 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");
>>
>> This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
>>
> Ok, copy that. I'll repost shortly

When you do, please include URLs for any problem reports or bugzillas you have.

I assume Windows "just works" in this situation?
--
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, 3:39 p.m. UTC | #11
On Thu, Apr 04, 2013 at 08:57:06AM -0600, Bjorn Helgaas wrote:
> On Thu, Apr 4, 2013 at 8:50 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Thu, Apr 04, 2013 at 03:27:29PM +0100, David Woodhouse wrote:
> >> On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
> >> > );
> >> > > +
> >> > > +       if ((revision == 0x13) && irq_remapping_enabled) {
> >> > > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> >> > > +                        "on a chipset that contains an errata 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");
> >>
> >> This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
> >>
> > Ok, copy that. I'll repost shortly
> 
> When you do, please include URLs for any problem reports or bugzillas you have.
> 
Well, those are going to be vendor specific, so I'm not sure we can really do
that, at least not in any meaningful way.

> I assume Windows "just works" in this situation?
No more or less than linux does in this case.  The Intel provided errata
indicates that the only acceptable workaround is to disable remapping in the
BIOS, so I would presume that if a windows system has a BIOS that doesn't
implement this fix, its just as exposed as we are.
Neil

--
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, 5:14 p.m. UTC | #12
On Thu, Apr 4, 2013 at 9:39 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Thu, Apr 04, 2013 at 08:57:06AM -0600, Bjorn Helgaas wrote:
>> On Thu, Apr 4, 2013 at 8:50 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
>> > On Thu, Apr 04, 2013 at 03:27:29PM +0100, David Woodhouse wrote:
>> >> On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
>> >> > );
>> >> > > +
>> >> > > +       if ((revision == 0x13) && irq_remapping_enabled) {
>> >> > > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
>> >> > > +                        "on a chipset that contains an errata 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");
>> >>
>> >> This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
>> >>
>> > Ok, copy that. I'll repost shortly
>>
>> When you do, please include URLs for any problem reports or bugzillas you have.
>>
> Well, those are going to be vendor specific, so I'm not sure we can really do
> that, at least not in any meaningful way.

Sorry, I don't understand your point.  It's useful to know who
reported it (e.g., for future testers) and what happened and what
bugzillas it solved.  Of course it applies only to machines with this
chipset and certain BIOS revisions.

>> I assume Windows "just works" in this situation?
> No more or less than linux does in this case.  The Intel provided errata
> indicates that the only acceptable workaround is to disable remapping in the
> BIOS, so I would presume that if a windows system has a BIOS that doesn't
> implement this fix, its just as exposed as we are.

It sounds like the effect of this bug is that on Linux, devices may
not work at all because of lost interrupts.  Either Windows must never
enable remapping (so it never sees the bug), or it must be designed in
a way that tolerates the problem.  I can't believe these machines
shipped with Windows certification if devices didn't work correctly.

Either way, I don't understand why we can't make the quirk just fix
this.  Booting with "nointremap" only sets disable_irq_remap, which is
only used by irq_remapping_supported().  Early quirks are run before
irq_remapping_supported () is ever called, so an early quirk ought to
be just as effective as the command line option.  Here's the relevant
call tree I see:

  start_kernel
    setup_arch
      parse_early_param
      early_quirks
    rest_init
      ...
        <first use of disable_irq_remap>

The x86 setup_arch() does call generic_apic_probe(), but as far as I
can tell, none of the APIC .probe() methods reference
disable_irq_remap, so that doesn't look like a problem.

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
Neil Horman April 4, 2013, 5:51 p.m. UTC | #13
On Thu, Apr 04, 2013 at 11:14:30AM -0600, Bjorn Helgaas wrote:
> On Thu, Apr 4, 2013 at 9:39 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Thu, Apr 04, 2013 at 08:57:06AM -0600, Bjorn Helgaas wrote:
> >> On Thu, Apr 4, 2013 at 8:50 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >> > On Thu, Apr 04, 2013 at 03:27:29PM +0100, David Woodhouse wrote:
> >> >> On Wed, 2013-04-03 at 17:53 -0600, Bjorn Helgaas wrote:
> >> >> > );
> >> >> > > +
> >> >> > > +       if ((revision == 0x13) && irq_remapping_enabled) {
> >> >> > > +                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
> >> >> > > +                        "on a chipset that contains an errata 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");
> >> >>
> >> >> This should be WARN_TAINT(TAINT_FIRMWARE_WORKAROUND). And 'an erratum'.
> >> >>
> >> > Ok, copy that. I'll repost shortly
> >>
> >> When you do, please include URLs for any problem reports or bugzillas you have.
> >>
> > Well, those are going to be vendor specific, so I'm not sure we can really do
> > that, at least not in any meaningful way.
> 
> Sorry, I don't understand your point.  It's useful to know who
> reported it (e.g., for future testers) and what happened and what
> bugzillas it solved.  Of course it applies only to machines with this
> chipset and certain BIOS revisions.
> 
Oh, you want the bug report that I'm fixing this against?  Sure, I can do that.
I thought you wanted me to include a url in the WARN_TAINT, with which user
could report occurances of this bug.  Yeah, the bug that this is reported in is:
https://bugzilla.redhat.com/show_bug.cgi?id=887006

Its standing in for about a dozen or so variants of this issue we've seen

> >> I assume Windows "just works" in this situation?
> > No more or less than linux does in this case.  The Intel provided errata
> > indicates that the only acceptable workaround is to disable remapping in the
> > BIOS, so I would presume that if a windows system has a BIOS that doesn't
> > implement this fix, its just as exposed as we are.
> 
> It sounds like the effect of this bug is that on Linux, devices may
> not work at all because of lost interrupts.  Either Windows must never
> enable remapping (so it never sees the bug), or it must be designed in
> a way that tolerates the problem.  I can't believe these machines
> shipped with Windows certification if devices didn't work correctly.
> 
I don't know what to tell you here.  We explicitly asked intel about this, and
there was an effort to recode the interrupt migration code to properly manage
this situation, and intels response was "No, just disable it in BIOS", so we're
telling people to disable it in BIOS.  You'd have to ask somebody at Microsoft
what Windows did or did not do about this problem.

> Either way, I don't understand why we can't make the quirk just fix
> this.  Booting with "nointremap" only sets disable_irq_remap, which is
> only used by irq_remapping_supported().  Early quirks are run before
> irq_remapping_supported () is ever called, so an early quirk ought to
> be just as effective as the command line option.  Here's the relevant
> call tree I see:
> 
>   start_kernel
>     setup_arch
>       parse_early_param
>       early_quirks
>     rest_init
>       ...
>         <first use of disable_irq_remap>
> 
> The x86 setup_arch() does call generic_apic_probe(), but as far as I
> can tell, none of the APIC .probe() methods reference
> disable_irq_remap, so that doesn't look like a problem.
> 
Well, I can give it a try, but I'm sure something went wrong last time I did.
Regardless, theres also the security issue to consider here - namely that
disabling irq remapping opens up users of virt to a possible security bug
(potential irq injection).  Some users may wish to live with the remapping
error, given that error typically leads to devices that need to be
restarted/reset to start working again, rather than live with the security hole.
I rather like the warning, that gives users a choice, but I'll spin up a version
that just disables it if you would rather.

Neil

--
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, 6:41 p.m. UTC | #14
On Thu, Apr 4, 2013 at 11:51 AM, Neil Horman <nhorman@tuxdriver.com> wrote:

> Oh, you want the bug report that I'm fixing this against?  Sure, I can do that.
> I thought you wanted me to include a url in the WARN_TAINT, with which user
> could report occurances of this bug.  Yeah, the bug that this is reported in is:
> https://bugzilla.redhat.com/show_bug.cgi?id=887006
>
> Its standing in for about a dozen or so variants of this issue we've seen

Exactly -- I'm just hoping for something in the changelog.  BTW, this
particular bugzilla is not public.

> Regardless, theres also the security issue to consider here - namely that
> disabling irq remapping opens up users of virt to a possible security bug
> (potential irq injection).  Some users may wish to live with the remapping
> error, given that error typically leads to devices that need to be
> restarted/reset to start working again, rather than live with the security hole.
> I rather like the warning, that gives users a choice, but I'll spin up a version
> that just disables it if you would rather.

I don't believe users will want to make a choice like that or even be
sophisticated enough to do it, at least not based on something in
dmesg.  I'm pretty sure I'm not  :)

The only supportable thing I can imagine doing would be:

  - Disable interrupt remapping if this chipset defect is present, so
devices work reliably (they don't need whatever restart/reset you
referred to above).
  - Disable virt functionality when interrupt remapping is disabled to
avoid the security problem (I don't know the details of this.)
  - Add a command-line option to enable interrupt remapping (I think
"intremap=on" is currently parsed too early, but maybe this could be
reworked so the option could override the quirk disable).
  - Add release notes saying "boot with 'intremap=on' if you want the
virt functionality and can accept unreliable devices."

That way the default behavior is safe and reliable (though perhaps
lacking some functionality), and you have told the user a way to get
safe and unreliable operation if he's willing to accept that.  At
least, that's what I think I would want if I were in RH's shoes.

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
Neil Horman April 4, 2013, 8:02 p.m. UTC | #15
On Thu, Apr 04, 2013 at 12:41:27PM -0600, Bjorn Helgaas wrote:
> On Thu, Apr 4, 2013 at 11:51 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 
> > Oh, you want the bug report that I'm fixing this against?  Sure, I can do that.
> > I thought you wanted me to include a url in the WARN_TAINT, with which user
> > could report occurances of this bug.  Yeah, the bug that this is reported in is:
> > https://bugzilla.redhat.com/show_bug.cgi?id=887006
> >
> > Its standing in for about a dozen or so variants of this issue we've seen
> 
> Exactly -- I'm just hoping for something in the changelog.  BTW, this
> particular bugzilla is not public.
> 
Ok, that I can do, I'll get the bz fixed up to be public in a bit.


> > Regardless, theres also the security issue to consider here - namely that
> > disabling irq remapping opens up users of virt to a possible security bug
> > (potential irq injection).  Some users may wish to live with the remapping
> > error, given that error typically leads to devices that need to be
> > restarted/reset to start working again, rather than live with the security hole.
> > I rather like the warning, that gives users a choice, but I'll spin up a version
> > that just disables it if you would rather.
> 
> I don't believe users will want to make a choice like that or even be
> sophisticated enough to do it, at least not based on something in
> dmesg.  I'm pretty sure I'm not  :)
> 
> The only supportable thing I can imagine doing would be:
> 
>   - Disable interrupt remapping if this chipset defect is present, so
> devices work reliably (they don't need whatever restart/reset you
> referred to above).
>   - Disable virt functionality when interrupt remapping is disabled to
> avoid the security problem (I don't know the details of this.)
>   - Add a command-line option to enable interrupt remapping (I think
> "intremap=on" is currently parsed too early, but maybe this could be
> reworked so the option could override the quirk disable).
>   - Add release notes saying "boot with 'intremap=on' if you want the
> virt functionality and can accept unreliable devices."
> 
> That way the default behavior is safe and reliable (though perhaps
> lacking some functionality), and you have told the user a way to get
> safe and unreliable operation if he's willing to accept that.  At
> least, that's what I think I would want if I were in RH's shoes.
> 
Theres a long argument behind this, and I'm with you.  At the least I don't see
a problem with disabling it upstream, at least not a policy-oriented reason.
That said, having looked back at my notes, I do now recall a technical
impediment behind disabling interrupt remapping.  If we were to do this in an
early quirk, we would need to determine the status of the interrupt remapping
capability flag in the iommu in question.  Looking at the interrupt remapping
code, it appears that the capability flag isn't directly contained in the pci
config space, but rather in a memory mapped address range who's base address is
parsed out of an acpi table.  Since we check the irq_remapping_enabled flag in
the current quirk (which is set after the early quirks run), to do this in an
early quirk, we would need to somehow parse that base address register out of
the available acpi tables ourselves, and I'm not at all sure how to do that.  Do
you know if theres some available parsing mechanism that early in boot?  If so,
I can probably make this happen.

Neil

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

Patch

diff --git a/arch/x86/kernel/quirks.c b/arch/x86/kernel/quirks.c
index 26ee48a..a718ea2 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);
+
+	if ((revision == 0x13) && irq_remapping_enabled) {
+                pr_warn(HW_ERR "This system BIOS has enabled interrupt remapping\n"
+                        "on a chipset that contains an errata 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, PCI_DEVICE_ID_INTEL_5520_IOHUB, intel_remapping_check);
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_5500_IOHUB, intel_remapping_check);
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index 31717bd..54027a6 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -2732,6 +2732,8 @@ 
 #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_RANK_REV2  0x2db2
 #define PCI_DEVICE_ID_INTEL_LYNNFIELD_MC_CH2_TC_REV2    0x2db3
 #define PCI_DEVICE_ID_INTEL_82855PM_HB	0x3340
+#define PCI_DEVICE_ID_INTEL_5500_IOHUB	0x3403
+#define PCI_DEVICE_ID_INTEL_5520_IOHUB	0x3406
 #define PCI_DEVICE_ID_INTEL_IOAT_TBG4	0x3429
 #define PCI_DEVICE_ID_INTEL_IOAT_TBG5	0x342a
 #define PCI_DEVICE_ID_INTEL_IOAT_TBG6	0x342b