[PATCH v2 1/2] ns16550: reject IRQ above nr_irqs_gsi

[PATCH v2 1/2] ns16550: reject IRQ above nr_irqs_gsi

[Marek Marczykowski-Górecki]

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(+)

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;
             }
         }
-- 
2.35.1

[PATCH v2 2/2] ns16550: Add more device IDs for Intel LPSS UART

[Marek Marczykowski-Górecki]

This is purely based on the spec:
- Intel 500 Series PCH: 635218-006
- Intel 600 Series PCH: 691222-001, 648364-003

This is tested only on TGL-LP added initially, but according to the
spec, they should behave the same.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v2:
 - new patch, adding more IDs to the patch that went in already
---
 xen/drivers/char/ns16550.c | 80 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 79 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 0c6f6ec43de1..b4486a4e8768 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1077,12 +1077,90 @@ static const struct ns16550_config __initconst uart_config[] =
         .dev_id = 0x0358,
         .param = param_exar_xr17v358
     },
-    /* Intel Corp. TGL-LP LPSS PCI */
+    /* Intel Corp. TGL-LP LPSS PCI UART #0 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0xa0a8,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. TGL-LP LPSS PCI UART #1 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0xa0a9,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. TGL-LP LPSS PCI UART #2 */
     {
         .vendor_id = PCI_VENDOR_ID_INTEL,
         .dev_id = 0xa0c7,
         .param = param_intel_lpss
     },
+    /* Intel Corp. TGL-H LPSS PCI UART #0 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x43a8,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. TGL-H LPSS PCI UART #1 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x43a9,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. TGL-H LPSS PCI UART #2 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x43a7,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-P LPSS PCI UART #0 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x51a8,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-P LPSS PCI UART #1 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x51a9,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-P LPSS PCI UART #2 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x51c7,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-P LPSS PCI UART #3 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x51da,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-S LPSS PCI UART #0 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x7aa8,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-S LPSS PCI UART #1 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x7aa9,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-S LPSS PCI UART #2 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x7afe,
+        .param = param_intel_lpss
+    },
+    /* Intel Corp. ADL-S LPSS PCI UART #3 */
+    {
+        .vendor_id = PCI_VENDOR_ID_INTEL,
+        .dev_id = 0x7adc,
+        .param = param_intel_lpss
+    },
 };
 
 static int __init
-- 
2.35.1

Re: [PATCH v2 1/2] ns16550: reject IRQ above nr_irqs_gsi

[Andrew Cooper]

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

Re: [PATCH v2 2/2] ns16550: Add more device IDs for Intel LPSS UART

[Andrew Cooper]

On 10/05/2022 12:55, Marek Marczykowski-Górecki wrote:
> This is purely based on the spec:
> - Intel 500 Series PCH: 635218-006
> - Intel 600 Series PCH: 691222-001, 648364-003
>
> This is tested only on TGL-LP added initially, but according to the
> spec, they should behave the same.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v2 1/2] ns16550: reject IRQ above nr_irqs_gsi

[Roger Pau Monné]

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.

Re: [PATCH v2 1/2] ns16550: reject IRQ above nr_irqs_gsi

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]

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.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]