| Message ID | 20220510115546.1779279-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] ns16550: reject IRQ above nr_irqs_gsi | expand |
On 10/05/2022 12:55, Marek Marczykowski-Górecki wrote: > Intel LPSS has INTERRUPT_LINE set to 0xff by default, that can't > possibly work. While a proper IRQ configuration may be useful, > validating value retrieved from the hardware is still necessary. If it > fails, use the device in poll mode, instead of crashing down the line > (at smp_initr_init()). Currently it's > x86-specific, as the surrounding code is guarded with CONFIG_X86 anyway. > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> This isn't invalid per say. It explicitly means unknown/no connection and is used in practice to mean "never generate line interrupts, even when MSI is disabled". It's part of PCI 3.0 if the internet is to be believed, but ISTR is mandatory for SRIOV devices where the use of line interrupts is prohibited by the spec. Also, there are systems where nr_irq_gsi is greater than 0xff. I'd recommend handling 0xff specially as "no legacy irq", and not involving nr_irq_gsi at all. ~Andrew
On Tue, May 10, 2022 at 02:30:41PM +0000, Andrew Cooper wrote: > On 10/05/2022 12:55, Marek Marczykowski-Górecki wrote: > > Intel LPSS has INTERRUPT_LINE set to 0xff by default, that can't > > possibly work. While a proper IRQ configuration may be useful, > > validating value retrieved from the hardware is still necessary. If it > > fails, use the device in poll mode, instead of crashing down the line > > (at smp_initr_init()). Currently it's > > x86-specific, as the surrounding code is guarded with CONFIG_X86 anyway. > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > This isn't invalid per say. It explicitly means unknown/no connection > and is used in practice to mean "never generate line interrupts, even > when MSI is disabled". It's part of PCI 3.0 if the internet is to be > believed, but ISTR is mandatory for SRIOV devices where the use of line > interrupts is prohibited by the spec. > > Also, there are systems where nr_irq_gsi is greater than 0xff. > > I'd recommend handling 0xff specially as "no legacy irq", and not > involving nr_irq_gsi at all. I've finally found the reference for it in (one) PCI specification. It's in the PCI Local Bus Specification Revision 3.0 (from 2004) as a footnote, so for the reference I'm going to paste it here: Interrupt Line The Interrupt Line register is an eight-bit register used to communicate interrupt line routing information. The register is read/write and must be implemented by any device (or device function) that uses an interrupt pin. POST software will write the routing information into this register as it initializes and configures the system. The value in this register tells which input of the system interrupt controller(s) the device's interrupt pin is connected to. The device itself does not use this value, rather it is used by device drivers and operating systems. Device drivers and operating systems can use this information to determine priority and vector information. Values in this register are system architecture specific. [43] [...] [43] For x86 based PCs, the values in this register correspond to IRQ numbers (0-15) of the standard dual 8259 configuration. The value 255 is defined as meaning "unknown" or "no connection" to the interrupt controller. Values between 15 and 254 are reserved. That note however is completely missed on the newer PCI Express specifications. Marek, can you please adjust the code as suggested by Andrew and add the reference to the commit message? Thanks, Roger.
On Tue, May 10, 2022 at 05:46:12PM +0200, Roger Pau Monné wrote: > On Tue, May 10, 2022 at 02:30:41PM +0000, Andrew Cooper wrote: > > On 10/05/2022 12:55, Marek Marczykowski-Górecki wrote: > > > Intel LPSS has INTERRUPT_LINE set to 0xff by default, that can't > > > possibly work. While a proper IRQ configuration may be useful, > > > validating value retrieved from the hardware is still necessary. If it > > > fails, use the device in poll mode, instead of crashing down the line > > > (at smp_initr_init()). Currently it's > > > x86-specific, as the surrounding code is guarded with CONFIG_X86 anyway. > > > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > > > This isn't invalid per say. It explicitly means unknown/no connection > > and is used in practice to mean "never generate line interrupts, even > > when MSI is disabled". It's part of PCI 3.0 if the internet is to be > > believed, but ISTR is mandatory for SRIOV devices where the use of line > > interrupts is prohibited by the spec. > > > > Also, there are systems where nr_irq_gsi is greater than 0xff. > > > > I'd recommend handling 0xff specially as "no legacy irq", and not > > involving nr_irq_gsi at all. > > I've finally found the reference for it in (one) PCI specification. > It's in the PCI Local Bus Specification Revision 3.0 (from 2004) as a > footnote, so for the reference I'm going to paste it here: > > Interrupt Line > > The Interrupt Line register is an eight-bit register used to > communicate interrupt line routing information. The register is > read/write and must be implemented by any device (or device function) > that uses an interrupt pin. POST software will write the routing > information into this register as it initializes and configures the > system. The value in this register tells which input of the system > interrupt controller(s) the device's interrupt pin is connected to. > The device itself does not use this value, rather it is used by device > drivers and operating systems. Device drivers and operating systems > can use this information to determine priority and vector information. > Values in this register are system architecture specific. [43] > > [...] > > [43] For x86 based PCs, the values in this register correspond to IRQ > numbers (0-15) of the standard dual 8259 configuration. The value 255 > is defined as meaning "unknown" or "no connection" to the interrupt > controller. Values between 15 and 254 are reserved. > > That note however is completely missed on the newer PCI Express > specifications. > > Marek, can you please adjust the code as suggested by Andrew and add > the reference to the commit message? Sure.
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c index fb75cee4a13a..0c6f6ec43de1 100644 --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -1238,6 +1238,15 @@ pci_uart_config(struct ns16550 *uart, bool_t skip_amt, unsigned int idx) pci_conf_read8(PCI_SBDF(0, b, d, f), PCI_INTERRUPT_LINE) : 0; + if ( uart->irq >= nr_irqs_gsi ) + { + printk(XENLOG_WARNING + "ns16550: %02x:%02x.%u reports invalid IRQ %d, " + "falling back to a poll mode\n", + b, d, f, uart->irq); + uart->irq = 0; + } + return 0; } }
Intel LPSS has INTERRUPT_LINE set to 0xff by default, that can't possibly work. While a proper IRQ configuration may be useful, validating value retrieved from the hardware is still necessary. If it fails, use the device in poll mode, instead of crashing down the line (at smp_initr_init()). Currently it's x86-specific, as the surrounding code is guarded with CONFIG_X86 anyway. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v2: - add log message - extend commit message - code style fix --- xen/drivers/char/ns16550.c | 9 +++++++++ 1 file changed, 9 insertions(+)