[SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Mauricio Faria de Oliveira-3
BugLink: https://bugs.launchpad.net/bugs/1797990

(note: the patch set for later releases will be sent soon)

[Impact]

 * A kexec/crash kernel might get stuck and fail to boot
   (for crash kernel, kdump fails to collect a crashdump)
   if a PCI device is buggy/stuck/looping and triggers a
   continuous flood of MSI(X) interrupts (that the kernel
   does not yet know about).

 * This fix allowed to obtain crashdumps when debugging a
   heavy-load scenario, in which a (heavy-loaded) network
   adapter wouldn't stop triggering MSI-X interrupts ever
   after panic()->kdump kicked in.

 * This fix disables MSI(X) in all PCI devices on early
   boot (this is OK as it's (re-)enabled normally later)
   with a kernel cmdline parameter (disabled by default).

[Test Case]

 * A synthetic test-case is not yet available, however,
   this particular system/workload triggered the problem
   consistently, and it was used for development/testing.

 * We'll update this bug once a synthetic test-case is
   available; we're working on patching QEMU for this.

 * $ dmesg | grep 'Clearing MSI'
   [    0.000000] Clearing MSI/MSI-X enable bits early in boot (quirk)

 * The comparison of 'dmesg -t | sort' has been reviewed
   between option disabled/enabled on boot & kexec modes,
   and only expected differences found (MHz, PIDs, MIPS).

[Regression Potential]

 * The potential area for regressions is early boot,
   particularly effects of applying quirks during PCI
   bus scan, which is changed/broader w/ these patches.

 * However, all quirks are applied based on PCI ID
   matching, so would only apply if actually targeting
   a new device.

 * Moreover, the new quirk is only applied based on
   a kernel cmdline parameter that is disabled by
   default, which constraints even more when this
   is actually in effect.

[Other Info]
 
 * The patch series is still under review/discussion
   upstream, but it's relatively important for Ubuntu
   users at this point, and after internal discussions
   we decided to submit it for SRU.

 * These are links to the linux-pci archive with the
   patches [1, 2, 3]

   [1] [PATCH 1/3] x86/quirks: Scan all busses for early PCI quirks
       https://lore.kernel.org/linux-pci/20181018183721.27467-1-gpiccoli@.../

   [2] [PATCH 2/3] x86/PCI: Export find_cap() to be used in early PCI code
       https://lore.kernel.org/linux-pci/20181018183721.27467-2-gpiccoli@.../

   [3] [PATCH 3/3] x86/quirks: Add parameter to clear MSIs early on boot
       https://lore.kernel.org/linux-pci/20181018183721.27467-3-gpiccoli@.../

Guilherme G. Piccoli (3):
  UBUNTU: SAUCE: x86/quirks: Scan all busses for early PCI quirks
  UBUNTU: SAUCE: x86/PCI: Export find_cap() to be used in early PCI code
  UBUNTU: SAUCE: x86/quirks: Add parameter to clear MSIs early on boot

 Documentation/kernel-parameters.txt |  6 ++++
 arch/x86/include/asm/pci-direct.h   |  2 ++
 arch/x86/kernel/aperture_64.c       | 30 ++-----------------
 arch/x86/kernel/early-quirks.c      | 45 ++++++++++++++++++++++++-----
 arch/x86/pci/common.c               |  4 +++
 arch/x86/pci/early.c                | 25 ++++++++++++++++
 6 files changed, 77 insertions(+), 35 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 1/3] UBUNTU: SAUCE: x86/quirks: Scan all busses for early PCI quirks

Mauricio Faria de Oliveira-3
From: "Guilherme G. Piccoli" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1797990

Recently was noticed in an HP GEN9 system that kdump couldn't succeed
due to an irq storm coming from an Intel NIC, narrowed down to be lack
of clearing the MSI/MSI-X enable bits during the kdump kernel boot.
For that, we need an early quirk to manually turn off MSI/MSI-X for
PCI devices - this was worked as an optional boot parameter in a
subsequent patch.

Problem is that in our test system, the Intel NICs were not present in
any secondary bus under the first PCIe root complex, so they couldn't
be reached by the recursion in check_dev_quirk(). Modern systems,
specially with multi-processors and multiple NUMA nodes expose multiple
root complexes, describing more than one PCI hierarchy domain. Currently
the simple recursion present in the early-quirks code from x86 starts a
descending recursion from bus 0000:00, and reach many other busses by
navigating this hierarchy walking through the bridges. This is not
enough in systems with more than one root complex/host bridge, since
the recursion won't "traverse" to other root complexes by starting
statically in 0000:00 (for more details, see [0]).

This patch hence implements the full bus/device/function scan in
early_quirks(), by checking all possible busses instead of using a
recursion based on the first root bus or limiting the search scope to
the first 32 busses (like it was done in the beginning [1]).

[0] https://bugs.launchpad.net/bugs/1797990

[1] From historical perspective, early PCI scan dates back
to BitKeeper, added by Andi Kleen's "[PATCH] APIC fixes for x86-64",
on October/2003. It initially restricted the search to the first
32 busses and slots.

Due to a potential bug found in Nvidia chipsets, the scan
was changed to run only in the first root bus: see
commit 8659c406ade3 ("x86: only scan the root bus in early PCI quirks")

Finally, secondary busses reachable from the 1st bus were re-added back by:
commit 850c321027c2 ("x86/quirks: Reintroduce scanning of secondary buses")

Reported-by: Dan Streetman <[hidden email]>
Signed-off-by: Guilherme G. Piccoli <[hidden email]>
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 arch/x86/kernel/early-quirks.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index a257d6077d1b..318e6baca319 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -723,7 +723,6 @@ static int __init check_dev_quirk(int num, int slot, int func)
  u16 vendor;
  u16 device;
  u8 type;
- u8 sec;
  int i;
 
  class = read_pci_config_16(num, slot, func, PCI_CLASS_DEVICE);
@@ -752,11 +751,8 @@ static int __init check_dev_quirk(int num, int slot, int func)
  type = read_pci_config_byte(num, slot, func,
     PCI_HEADER_TYPE);
 
- if ((type & 0x7f) == PCI_HEADER_TYPE_BRIDGE) {
- sec = read_pci_config_byte(num, slot, func, PCI_SECONDARY_BUS);
- if (sec > num)
- early_pci_scan_bus(sec);
- }
+ if ((type & 0x7f) == PCI_HEADER_TYPE_BRIDGE)
+ return -1;
 
  if (!(type & 0x80))
  return -1;
@@ -779,8 +775,11 @@ static void __init early_pci_scan_bus(int bus)
 
 void __init early_quirks(void)
 {
+ int bus;
+
  if (!early_pci_allowed())
  return;
 
- early_pci_scan_bus(0);
+ for (bus = 0; bus < 256; bus++)
+ early_pci_scan_bus(bus);
 }
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 2/3] UBUNTU: SAUCE: x86/PCI: Export find_cap() to be used in early PCI code

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: "Guilherme G. Piccoli" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1797990

This patch exports (and renames) the function find_cap() to be used
in the early PCI quirk code, by the next patch.

This is being moved out from AGP code to generic early-PCI code
since it's not AGP-specific and can be used for any PCI device.
No functional changes intended.

Signed-off-by: Guilherme G. Piccoli <[hidden email]>
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 arch/x86/include/asm/pci-direct.h |  1 +
 arch/x86/kernel/aperture_64.c     | 30 ++----------------------------
 arch/x86/pci/early.c              | 25 +++++++++++++++++++++++++
 3 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/pci-direct.h b/arch/x86/include/asm/pci-direct.h
index b1e7a45d868a..c5a8edf2dc8b 100644
--- a/arch/x86/include/asm/pci-direct.h
+++ b/arch/x86/include/asm/pci-direct.h
@@ -9,6 +9,7 @@
 extern u32 read_pci_config(u8 bus, u8 slot, u8 func, u8 offset);
 extern u8 read_pci_config_byte(u8 bus, u8 slot, u8 func, u8 offset);
 extern u16 read_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset);
+extern u32 pci_early_find_cap(int bus, int slot, int func, int cap);
 extern void write_pci_config(u8 bus, u8 slot, u8 func, u8 offset, u32 val);
 extern void write_pci_config_byte(u8 bus, u8 slot, u8 func, u8 offset, u8 val);
 extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val);
diff --git a/arch/x86/kernel/aperture_64.c b/arch/x86/kernel/aperture_64.c
index 6e85f713641d..cabb09540662 100644
--- a/arch/x86/kernel/aperture_64.c
+++ b/arch/x86/kernel/aperture_64.c
@@ -91,32 +91,6 @@ static u32 __init allocate_aperture(void)
 }
 
 
-/* Find a PCI capability */
-static u32 __init find_cap(int bus, int slot, int func, int cap)
-{
- int bytes;
- u8 pos;
-
- if (!(read_pci_config_16(bus, slot, func, PCI_STATUS) &
- PCI_STATUS_CAP_LIST))
- return 0;
-
- pos = read_pci_config_byte(bus, slot, func, PCI_CAPABILITY_LIST);
- for (bytes = 0; bytes < 48 && pos >= 0x40; bytes++) {
- u8 id;
-
- pos &= ~3;
- id = read_pci_config_byte(bus, slot, func, pos+PCI_CAP_LIST_ID);
- if (id == 0xff)
- break;
- if (id == cap)
- return pos;
- pos = read_pci_config_byte(bus, slot, func,
- pos+PCI_CAP_LIST_NEXT);
- }
- return 0;
-}
-
 /* Read a standard AGPv3 bridge header */
 static u32 __init read_agp(int bus, int slot, int func, int cap, u32 *order)
 {
@@ -205,8 +179,8 @@ static u32 __init search_agp_bridge(u32 *order, int *valid_agp)
  case PCI_CLASS_BRIDGE_HOST:
  case PCI_CLASS_BRIDGE_OTHER: /* needed? */
  /* AGP bridge? */
- cap = find_cap(bus, slot, func,
- PCI_CAP_ID_AGP);
+ cap = pci_early_find_cap(bus, slot,
+ func, PCI_CAP_ID_AGP);
  if (!cap)
  break;
  *valid_agp = 1;
diff --git a/arch/x86/pci/early.c b/arch/x86/pci/early.c
index d1067d539bee..354c50788aae 100644
--- a/arch/x86/pci/early.c
+++ b/arch/x86/pci/early.c
@@ -50,6 +50,31 @@ void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val)
  outw(val, 0xcfc + (offset&2));
 }
 
+u32 pci_early_find_cap(int bus, int slot, int func, int cap)
+{
+ int bytes;
+ u8 pos;
+
+ if (!(read_pci_config_16(bus, slot, func, PCI_STATUS) &
+ PCI_STATUS_CAP_LIST))
+ return 0;
+
+ pos = read_pci_config_byte(bus, slot, func, PCI_CAPABILITY_LIST);
+ for (bytes = 0; bytes < 48 && pos >= 0x40; bytes++) {
+ u8 id;
+
+ pos &= ~3;
+ id = read_pci_config_byte(bus, slot, func, pos+PCI_CAP_LIST_ID);
+ if (id == 0xff)
+ break;
+ if (id == cap)
+ return pos;
+ pos = read_pci_config_byte(bus, slot, func,
+ pos+PCI_CAP_LIST_NEXT);
+ }
+ return 0;
+}
+
 int early_pci_allowed(void)
 {
  return (pci_probe & (PCI_PROBE_CONF1|PCI_PROBE_NOEARLY)) ==
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU X][PATCH 3/3] UBUNTU: SAUCE: x86/quirks: Add parameter to clear MSIs early on boot

Mauricio Faria de Oliveira-3
In reply to this post by Mauricio Faria de Oliveira-3
From: "Guilherme G. Piccoli" <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1797990

We observed a kdump failure in x86 that was narrowed down to MSI irq
storm coming from a PCI network device. The bug manifests as a lack of
progress in the boot process of kdump kernel, and a flood of kernel
messages like:

[...]
[ 342.265294] do_IRQ: 0.155 No irq handler for vector
[ 342.266916] do_IRQ: 0.155 No irq handler for vector
[ 347.258422] do_IRQ: 14053260 callbacks suppressed
[...]

The root cause of the issue is that kexec process of the kdump kernel
doesn't ensure PCI devices are reset or MSI capabilities are disabled,
so a PCI adapter could produce a huge amount of irqs which would steal
all the processing time for the CPU (specially since we usually restrict
kdump kernel to use a single CPU only).

This patch implements the kernel parameter "pci=clearmsi" to clear the
MSI/MSI-X enable bits in the Message Control register for all PCI devices
during early boot time, thus preventing potential issues in the kexec'ed
kernel. PCI spec also supports/enforces this need (see PCI Local Bus
spec sections 6.8.1.3 and 6.8.2.3).

Suggested-by: Dan Streetman <[hidden email]>
Suggested-by: Gavin Shan <[hidden email]>
Signed-off-by: Guilherme G. Piccoli <[hidden email]>
[mfo: backport to ubuntu-xenial:
 - different path for Documentation/.../kernel-parameters.txt
 - update context lines in pci-direct.h and early-quirks.c]
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 Documentation/kernel-parameters.txt |  6 ++++++
 arch/x86/include/asm/pci-direct.h   |  1 +
 arch/x86/kernel/early-quirks.c      | 32 +++++++++++++++++++++++++++++
 arch/x86/pci/common.c               |  4 ++++
 4 files changed, 43 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index c8f9d0bbf949..71c4280f9509 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2872,6 +2872,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
  nomsi [MSI] If the PCI_MSI kernel config parameter is
  enabled, this kernel boot option can be used to
  disable the use of MSI interrupts system-wide.
+ clearmsi [X86] Clears MSI/MSI-X enable bits early in boot
+ time in order to avoid issues like adapters
+ screaming irqs and preventing boot progress.
+ Also, it enforces the PCI Local Bus spec
+ rule that those bits should be 0 in system reset
+ events (useful for kexec/kdump cases).
  noioapicquirk [APIC] Disable all boot interrupt quirks.
  Safety option to keep boot IRQs enabled. This
  should never be necessary.
diff --git a/arch/x86/include/asm/pci-direct.h b/arch/x86/include/asm/pci-direct.h
index c5a8edf2dc8b..4af4c656a4cc 100644
--- a/arch/x86/include/asm/pci-direct.h
+++ b/arch/x86/include/asm/pci-direct.h
@@ -14,6 +14,7 @@ extern void write_pci_config(u8 bus, u8 slot, u8 func, u8 offset, u32 val);
 extern void write_pci_config_byte(u8 bus, u8 slot, u8 func, u8 offset, u8 val);
 extern void write_pci_config_16(u8 bus, u8 slot, u8 func, u8 offset, u16 val);
 
+extern unsigned int pci_early_clear_msi;
 extern int early_pci_allowed(void);
 
 extern unsigned int pci_early_dump_regs;
diff --git a/arch/x86/kernel/early-quirks.c b/arch/x86/kernel/early-quirks.c
index 318e6baca319..25a3eba6295c 100644
--- a/arch/x86/kernel/early-quirks.c
+++ b/arch/x86/kernel/early-quirks.c
@@ -27,6 +27,37 @@
 #include <asm/irq_remapping.h>
 #include <asm/early_ioremap.h>
 
+static void __init early_pci_clear_msi(int bus, int slot, int func)
+{
+ int pos;
+ u16 ctrl;
+
+ if (likely(!pci_early_clear_msi))
+ return;
+
+ pr_info_once("Clearing MSI/MSI-X enable bits early in boot (quirk)\n");
+
+ pos = pci_early_find_cap(bus, slot, func, PCI_CAP_ID_MSI);
+ if (pos) {
+ ctrl = read_pci_config_16(bus, slot, func, pos + PCI_MSI_FLAGS);
+ ctrl &= ~PCI_MSI_FLAGS_ENABLE;
+ write_pci_config_16(bus, slot, func, pos + PCI_MSI_FLAGS, ctrl);
+
+ /* Read again to flush previous write */
+ ctrl = read_pci_config_16(bus, slot, func, pos + PCI_MSI_FLAGS);
+ }
+
+ pos = pci_early_find_cap(bus, slot, func, PCI_CAP_ID_MSIX);
+ if (pos) {
+ ctrl = read_pci_config_16(bus, slot, func, pos + PCI_MSIX_FLAGS);
+ ctrl &= ~PCI_MSIX_FLAGS_ENABLE;
+ write_pci_config_16(bus, slot, func, pos + PCI_MSIX_FLAGS, ctrl);
+
+ /* Read again to flush previous write */
+ ctrl = read_pci_config_16(bus, slot, func, pos + PCI_MSIX_FLAGS);
+ }
+}
+
 #define dev_err(msg)  pr_err("pci 0000:%02x:%02x.%d: %s", bus, slot, func, msg)
 
 static void __init fix_hypertransport_config(int num, int slot, int func)
@@ -701,6 +732,7 @@ static struct chipset early_qrk[] __initdata = {
  PCI_CLASS_BRIDGE_HOST, PCI_ANY_ID, 0, force_disable_hpet},
  { PCI_VENDOR_ID_BROADCOM, 0x4331,
   PCI_CLASS_NETWORK_OTHER, PCI_ANY_ID, 0, apple_airport_reset},
+ { PCI_ANY_ID, PCI_ANY_ID, 0, 0, 0, early_pci_clear_msi},
  {}
 };
 
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 8fd6f44aee83..c049ccca03a5 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -35,6 +35,7 @@ int noioapicreroute = 1;
 #endif
 int pcibios_last_bus = -1;
 unsigned long pirq_table_addr;
+unsigned int pci_early_clear_msi;
 const struct pci_raw_ops *__read_mostly raw_pci_ops;
 const struct pci_raw_ops *__read_mostly raw_pci_ext_ops;
 
@@ -621,6 +622,9 @@ char *__init pcibios_setup(char *str)
  } else if (!strcmp(str, "skip_isa_align")) {
  pci_probe |= PCI_CAN_SKIP_ISA_ALIGN;
  return NULL;
+ } else if (!strcmp(str, "clearmsi")) {
+ pci_early_clear_msi = 1;
+ return NULL;
  } else if (!strcmp(str, "noioapicquirk")) {
  noioapicquirk = 1;
  return NULL;
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Stefan Bader-2
In reply to this post by Mauricio Faria de Oliveira-3
On 08.11.18 02:12, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1797990
>
> (note: the patch set for later releases will be sent soon)
>
> [Impact]
>
>  * A kexec/crash kernel might get stuck and fail to boot
>    (for crash kernel, kdump fails to collect a crashdump)
>    if a PCI device is buggy/stuck/looping and triggers a
>    continuous flood of MSI(X) interrupts (that the kernel
>    does not yet know about).
>
>  * This fix allowed to obtain crashdumps when debugging a
>    heavy-load scenario, in which a (heavy-loaded) network
>    adapter wouldn't stop triggering MSI-X interrupts ever
>    after panic()->kdump kicked in.
>
>  * This fix disables MSI(X) in all PCI devices on early
>    boot (this is OK as it's (re-)enabled normally later)
>    with a kernel cmdline parameter (disabled by default).
>
> [Test Case]
>
>  * A synthetic test-case is not yet available, however,
>    this particular system/workload triggered the problem
>    consistently, and it was used for development/testing.
>
>  * We'll update this bug once a synthetic test-case is
>    available; we're working on patching QEMU for this.
>
>  * $ dmesg | grep 'Clearing MSI'
>    [    0.000000] Clearing MSI/MSI-X enable bits early in boot (quirk)
>
>  * The comparison of 'dmesg -t | sort' has been reviewed
>    between option disabled/enabled on boot & kexec modes,
>    and only expected differences found (MHz, PIDs, MIPS).
>
> [Regression Potential]
>
>  * The potential area for regressions is early boot,
>    particularly effects of applying quirks during PCI
>    bus scan, which is changed/broader w/ these patches.
>
>  * However, all quirks are applied based on PCI ID
>    matching, so would only apply if actually targeting
>    a new device.
>
>  * Moreover, the new quirk is only applied based on
>    a kernel cmdline parameter that is disabled by
>    default, which constraints even more when this
>    is actually in effect.
>
> [Other Info]
>  
>  * The patch series is still under review/discussion
>    upstream, but it's relatively important for Ubuntu
>    users at this point, and after internal discussions
>    we decided to submit it for SRU.
>
>  * These are links to the linux-pci archive with the
>    patches [1, 2, 3]
>
>    [1] [PATCH 1/3] x86/quirks: Scan all busses for early PCI quirks
>        https://lore.kernel.org/linux-pci/20181018183721.27467-1-gpiccoli@.../
>
>    [2] [PATCH 2/3] x86/PCI: Export find_cap() to be used in early PCI code
>        https://lore.kernel.org/linux-pci/20181018183721.27467-2-gpiccoli@.../
>
>    [3] [PATCH 3/3] x86/quirks: Add parameter to clear MSIs early on boot
>        https://lore.kernel.org/linux-pci/20181018183721.27467-3-gpiccoli@.../
>
> Guilherme G. Piccoli (3):
>   UBUNTU: SAUCE: x86/quirks: Scan all busses for early PCI quirks
>   UBUNTU: SAUCE: x86/PCI: Export find_cap() to be used in early PCI code
>   UBUNTU: SAUCE: x86/quirks: Add parameter to clear MSIs early on boot
>
>  Documentation/kernel-parameters.txt |  6 ++++
>  arch/x86/include/asm/pci-direct.h   |  2 ++
>  arch/x86/kernel/aperture_64.c       | 30 ++-----------------
>  arch/x86/kernel/early-quirks.c      | 45 ++++++++++++++++++++++++-----
>  arch/x86/pci/common.c               |  4 +++
>  arch/x86/pci/early.c                | 25 ++++++++++++++++
>  6 files changed, 77 insertions(+), 35 deletions(-)
>
Since at least the new code is off by default the risk should be acceptable.

Acked-by: Stefan Bader <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[Acked/cmt] [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Andy Whitcroft-3
In reply to this post by Mauricio Faria de Oliveira-3
On Wed, Nov 07, 2018 at 11:12:17PM -0200, Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1797990
>
> (note: the patch set for later releases will be sent soon)
>
> [Impact]
>
>  * A kexec/crash kernel might get stuck and fail to boot
>    (for crash kernel, kdump fails to collect a crashdump)
>    if a PCI device is buggy/stuck/looping and triggers a
>    continuous flood of MSI(X) interrupts (that the kernel
>    does not yet know about).
>
>  * This fix allowed to obtain crashdumps when debugging a
>    heavy-load scenario, in which a (heavy-loaded) network
>    adapter wouldn't stop triggering MSI-X interrupts ever
>    after panic()->kdump kicked in.
>
>  * This fix disables MSI(X) in all PCI devices on early
>    boot (this is OK as it's (re-)enabled normally later)
>    with a kernel cmdline parameter (disabled by default).
>
> [Test Case]
>
>  * A synthetic test-case is not yet available, however,
>    this particular system/workload triggered the problem
>    consistently, and it was used for development/testing.
>
>  * We'll update this bug once a synthetic test-case is
>    available; we're working on patching QEMU for this.
>
>  * $ dmesg | grep 'Clearing MSI'
>    [    0.000000] Clearing MSI/MSI-X enable bits early in boot (quirk)
>
>  * The comparison of 'dmesg -t | sort' has been reviewed
>    between option disabled/enabled on boot & kexec modes,
>    and only expected differences found (MHz, PIDs, MIPS).
>
> [Regression Potential]
>
>  * The potential area for regressions is early boot,
>    particularly effects of applying quirks during PCI
>    bus scan, which is changed/broader w/ these patches.
>
>  * However, all quirks are applied based on PCI ID
>    matching, so would only apply if actually targeting
>    a new device.
>
>  * Moreover, the new quirk is only applied based on
>    a kernel cmdline parameter that is disabled by
>    default, which constraints even more when this
>    is actually in effect.
>
> [Other Info]
>  
>  * The patch series is still under review/discussion
>    upstream, but it's relatively important for Ubuntu
>    users at this point, and after internal discussions
>    we decided to submit it for SRU.
>
>  * These are links to the linux-pci archive with the
>    patches [1, 2, 3]
>
>    [1] [PATCH 1/3] x86/quirks: Scan all busses for early PCI quirks
>        https://lore.kernel.org/linux-pci/20181018183721.27467-1-gpiccoli@.../
>
>    [2] [PATCH 2/3] x86/PCI: Export find_cap() to be used in early PCI code
>        https://lore.kernel.org/linux-pci/20181018183721.27467-2-gpiccoli@.../
>
>    [3] [PATCH 3/3] x86/quirks: Add parameter to clear MSIs early on boot
>        https://lore.kernel.org/linux-pci/20181018183721.27467-3-gpiccoli@.../
>
> Guilherme G. Piccoli (3):
>   UBUNTU: SAUCE: x86/quirks: Scan all busses for early PCI quirks
>   UBUNTU: SAUCE: x86/PCI: Export find_cap() to be used in early PCI code
>   UBUNTU: SAUCE: x86/quirks: Add parameter to clear MSIs early on boot
>
>  Documentation/kernel-parameters.txt |  6 ++++
>  arch/x86/include/asm/pci-direct.h   |  2 ++
>  arch/x86/kernel/aperture_64.c       | 30 ++-----------------
>  arch/x86/kernel/early-quirks.c      | 45 ++++++++++++++++++++++++-----
>  arch/x86/pci/common.c               |  4 +++
>  arch/x86/pci/early.c                | 25 ++++++++++++++++
>  6 files changed, 77 insertions(+), 35 deletions(-)
>

As the new functionality is off by default this should be a low risk.
The only obvious remaining risk is that we scan all of the pci busses
where we did not necessarily do so before.  And as the patch itself
mentions this might have been done to avoid bugs in Nvidia chipsets.
Likely some nvidia based and specific testing is warrented before this
cycle releases.

Acked-by: Andy Whitcroft <[hidden email]>

-apw

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

NACK: [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Thadeu Lima de Souza Cascardo-3
On Thu, Nov 08, 2018 at 10:11:54AM +0000, Andy Whitcroft wrote:

> On Wed, Nov 07, 2018 at 11:12:17PM -0200, Mauricio Faria de Oliveira wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1797990
> >
> > (note: the patch set for later releases will be sent soon)
> >
> > [Impact]
> >
> >  * A kexec/crash kernel might get stuck and fail to boot
> >    (for crash kernel, kdump fails to collect a crashdump)
> >    if a PCI device is buggy/stuck/looping and triggers a
> >    continuous flood of MSI(X) interrupts (that the kernel
> >    does not yet know about).
> >
> >  * This fix allowed to obtain crashdumps when debugging a
> >    heavy-load scenario, in which a (heavy-loaded) network
> >    adapter wouldn't stop triggering MSI-X interrupts ever
> >    after panic()->kdump kicked in.
> >
> >  * This fix disables MSI(X) in all PCI devices on early
> >    boot (this is OK as it's (re-)enabled normally later)
> >    with a kernel cmdline parameter (disabled by default).
> >
> > [Test Case]
> >
> >  * A synthetic test-case is not yet available, however,
> >    this particular system/workload triggered the problem
> >    consistently, and it was used for development/testing.
> >
> >  * We'll update this bug once a synthetic test-case is
> >    available; we're working on patching QEMU for this.
> >
> >  * $ dmesg | grep 'Clearing MSI'
> >    [    0.000000] Clearing MSI/MSI-X enable bits early in boot (quirk)
> >
> >  * The comparison of 'dmesg -t | sort' has been reviewed
> >    between option disabled/enabled on boot & kexec modes,
> >    and only expected differences found (MHz, PIDs, MIPS).
> >
> > [Regression Potential]
> >
> >  * The potential area for regressions is early boot,
> >    particularly effects of applying quirks during PCI
> >    bus scan, which is changed/broader w/ these patches.
> >
> >  * However, all quirks are applied based on PCI ID
> >    matching, so would only apply if actually targeting
> >    a new device.
> >
> >  * Moreover, the new quirk is only applied based on
> >    a kernel cmdline parameter that is disabled by
> >    default, which constraints even more when this
> >    is actually in effect.
> >
> > [Other Info]
> >  
> >  * The patch series is still under review/discussion
> >    upstream, but it's relatively important for Ubuntu
> >    users at this point, and after internal discussions
> >    we decided to submit it for SRU.
> >
> >  * These are links to the linux-pci archive with the
> >    patches [1, 2, 3]
> >
> >    [1] [PATCH 1/3] x86/quirks: Scan all busses for early PCI quirks
> >        https://lore.kernel.org/linux-pci/20181018183721.27467-1-gpiccoli@.../
> >
> >    [2] [PATCH 2/3] x86/PCI: Export find_cap() to be used in early PCI code
> >        https://lore.kernel.org/linux-pci/20181018183721.27467-2-gpiccoli@.../
> >
> >    [3] [PATCH 3/3] x86/quirks: Add parameter to clear MSIs early on boot
> >        https://lore.kernel.org/linux-pci/20181018183721.27467-3-gpiccoli@.../
> >
> > Guilherme G. Piccoli (3):
> >   UBUNTU: SAUCE: x86/quirks: Scan all busses for early PCI quirks
> >   UBUNTU: SAUCE: x86/PCI: Export find_cap() to be used in early PCI code
> >   UBUNTU: SAUCE: x86/quirks: Add parameter to clear MSIs early on boot
> >
> >  Documentation/kernel-parameters.txt |  6 ++++
> >  arch/x86/include/asm/pci-direct.h   |  2 ++
> >  arch/x86/kernel/aperture_64.c       | 30 ++-----------------
> >  arch/x86/kernel/early-quirks.c      | 45 ++++++++++++++++++++++++-----
> >  arch/x86/pci/common.c               |  4 +++
> >  arch/x86/pci/early.c                | 25 ++++++++++++++++
> >  6 files changed, 77 insertions(+), 35 deletions(-)
> >
>
> As the new functionality is off by default this should be a low risk.
> The only obvious remaining risk is that we scan all of the pci busses
> where we did not necessarily do so before.  And as the patch itself
> mentions this might have been done to avoid bugs in Nvidia chipsets.
> Likely some nvidia based and specific testing is warrented before this
> cycle releases.
>
> Acked-by: Andy Whitcroft <[hidden email]>
>
> -apw

This is a NACK from me. If we are gonna skip upstream review and
extensive system testing before applying, I would like to see the bus
scan change restricted to the command line option.

That is, please resubmit, but only scan all busses when the command line
option is given. If the command line option is not give, the kernel
behavior should be the same as before.

Cascardo.

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: NACK: [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Mauricio Faria de Oliveira-3
On Thu, Nov 8, 2018 at 9:02 AM Thadeu Lima de Souza Cascardo <[hidden email]> wrote:
This is a NACK from me. If we are gonna skip upstream review and
extensive system testing before applying, I would like to see the bus
scan change restricted to the command line option.

Right, that's certainly understandable.

That is, please resubmit, but only scan all busses when the command line
option is given. If the command line option is not give, the kernel
behavior should be the same as before.

Sure thing.

--
Mauricio Faria de Oliveira

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: [Acked/cmt] [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Mauricio Faria de Oliveira-3
In reply to this post by Andy Whitcroft-3
On Thu, Nov 8, 2018 at 8:11 AM Andy Whitcroft <[hidden email]> wrote:
As the new functionality is off by default this should be a low risk.
The only obvious remaining risk is that we scan all of the pci busses
where we did not necessarily do so before.  And as the patch itself
mentions this might have been done to avoid bugs in Nvidia chipsets.
Likely some nvidia based and specific testing is warrented before this
cycle releases.

I'll rework the new pci bus scan to be gated by the cmdline option as well,
as requested by Cascardo in the other review/reply, which should address
the remaining concern here.  Thanks for reviewing!
 

Acked-by: Andy Whitcroft <[hidden email]>

-apw


--
Mauricio Faria de Oliveira

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK: [SRU X][PATCH 0/3] Add kernel parameter 'pci=clearmsi' to clear MSI(X)s early on boot

Mauricio Faria de Oliveira-2
In reply to this post by Stefan Bader-2


On Thu, Nov 8, 2018 at 7:51 AM Stefan Bader <[hidden email]> wrote:
Since at least the new code is off by default the risk should be acceptable.

I'll rework the remaining code change that's not off by default to be gated
under the cmdline option as well, per another request. Thanks for reviewing!

 
Acked-by: Stefan Bader <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team


--
Mauricio Faria de Oliveira

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team