All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ns16550: find the console UART on PCI when there is no legacy one
@ 2026-09-22 18:43 Benjamin Leggett
  2026-09-23  6:11 ` Jan Beulich
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Benjamin Leggett @ 2026-09-22 18:43 UTC (permalink / raw)
  To: xen-devel
  Cc: Benjamin Leggett, Andrew Cooper, Anthony PERARD, Michal Orzel,
	Jan Beulich, Julien Grall, Roger Pau Monné,
	Stefano Stabellini

Amazon EC2 bare metal instances have no UART at the legacy I/O port
0x3f8. Their only serial port is a 16550-compatible PCI device (vendor
0x1d0f, device 0x8250) with its registers in the MMIO space of BAR 0.
Today Xen has no console on these systems unless the command line
names that device, and "com1=...,pci" cannot find it, because
uart_config[] doesn't have it.

Add the device to uart_config[].

Additionally, when the port that com1 describes is not present, scan PCI
for a known UART before giving up. This way the same command line works on
systems with and without a legacy UART, and a machine-specific "pci"
option is not needed. The fallback does not run when the command line
gave an I/O base, or when it already asked for a scan with "pci" or
"amt", so explicit config keeps its current meaning. It is
for com1 only: for com2, pci_uart_config() skips the first port it
finds, so it can never match a single-port device.

When the scan finds nothing, pci_uart_config() puts back the original
base, and check_existence() does not test MMIO addresses.

On a system with a legacy UART, check_existence() passes and nothing
changes.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Leggett <benjamin@edera.io>
---
 xen/drivers/char/ns16550.c | 47 +++++++++++++++++++++++++++++++++++++-
 xen/include/xen/pci_ids.h  |  2 ++
 2 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 120ac09d23..d5403598d2 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -64,8 +64,10 @@ static struct ns16550 {
     bool intr_works;
     bool force_polling;
     bool dw_usr_bsy;
+    bool io_base_set;       /* if =1, io_base came from the command line */
 #ifdef NS16550_PCI
     /* PCI card parameters. */
+    bool pci_scanned;       /* if =1, pci_uart_config() already ran */
     bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind bridge */
     bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */
     pci_sbdf_t pci_bridge;
@@ -98,6 +100,7 @@ struct ns16550_config {
         param_intel_lpss,
         param_wch_ch382,
         param_asix,
+        param_amazon,
     } param;
 };
 
@@ -909,6 +912,13 @@ static const struct ns16550_config_param __initconst uart_param[] = {
         .bar0 = true,
         .max_ports = 1,
     },
+    [param_amazon] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = true,
+        .mmio = true,
+        .max_ports = 1,
+    },
 };
 
 static const struct ns16550_config __initconst uart_config[] =
@@ -1255,6 +1265,12 @@ static const struct ns16550_config __initconst uart_config[] =
         .dev_id = 0x9910,
         .param = param_asix
     },
+    /* Amazon EC2 bare metal UART, the only serial port on those systems */
+    {
+        .vendor_id = PCI_VENDOR_ID_AMAZON,
+        .dev_id = 0x8250,
+        .param = param_amazon
+    },
 };
 
 static int __init
@@ -1263,6 +1279,8 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
     u64 orig_base = uart->io_base;
     unsigned int b, d, f, nextf, i;
 
+    uart->pci_scanned = true;
+
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
     {
@@ -1618,6 +1636,7 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
 #endif
         {
             uart->io_base = simple_strtoull(conf, &conf, 0);
+            uart->io_base_set = true;
         }
     }
 
@@ -1693,6 +1712,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                 break;
             }
             uart->io_base = simple_strtoull(param_value, NULL, 0);
+            uart->io_base_set = true;
             break;
 
         case irq:
@@ -1805,7 +1825,32 @@ static void __init ns16550_parse_port_config(
     if ( uart->io_base == 0 )
         PARSE_ERR("I/O base address must be specified.");
     if ( !check_existence(uart) )
-        PARSE_ERR("16550-compatible serial UART not present");
+    {
+        bool present = false;
+
+#ifdef NS16550_PCI
+        /*
+         * Some systems, EC2 bare metal among them, have no legacy UART and
+         * carry their only serial port on PCI. Look for one before giving up,
+         * unless the command line named a base or already asked for a scan.
+         * com1 only: for com2 the scan skips the first port it finds, so it
+         * can never match a single-port device.
+         */
+        if ( uart == ns16550_com && !uart->io_base_set && !uart->pci_scanned )
+        {
+            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
+            /*
+             * A scan that matched nothing puts back the base we just rejected,
+             * and check_existence() passes MMIO addresses through untested, so
+             * ps_bdf_enable is what says a device was found.
+             */
+            present = uart->ps_bdf_enable && check_existence(uart);
+        }
+#endif
+
+        if ( !present )
+            PARSE_ERR("16550-compatible serial UART not present");
+    }
 
     /* Register with generic serial driver. */
     serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index fd424ef55d..a17c88dcf7 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -17,6 +17,8 @@
 
 #define PCI_VENDOR_ID_WCHIC              0x1c00
 
+#define PCI_VENDOR_ID_AMAZON             0x1d0f
+
 #define PCI_VENDOR_ID_INTEL              0x8086
 
 #endif /* XEN_PCI_IDS_H */
-- 
2.55.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ns16550: find the console UART on PCI when there is no legacy one
  2026-09-22 18:43 [PATCH] ns16550: find the console UART on PCI when there is no legacy one Benjamin Leggett
@ 2026-09-23  6:11 ` Jan Beulich
  2026-09-23 15:38   ` Benjamin Leggett
  2026-09-23 15:39 ` [PATCH] ns16550: add the Amazon EC2 PCI serial device Benjamin Leggett
  2026-10-09 20:24 ` [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART Benjamin Leggett
  2 siblings, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2026-09-23  6:11 UTC (permalink / raw)
  To: Benjamin Leggett
  Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
	Roger Pau Monné, Stefano Stabellini, xen-devel

On 22.09.2026 20:43, Benjamin Leggett wrote:
> Amazon EC2 bare metal instances have no UART at the legacy I/O port
> 0x3f8. Their only serial port is a 16550-compatible PCI device (vendor
> 0x1d0f, device 0x8250) with its registers in the MMIO space of BAR 0.
> Today Xen has no console on these systems unless the command line
> names that device, and "com1=...,pci" cannot find it, because
> uart_config[] doesn't have it.
> 
> Add the device to uart_config[].

Is there a spec that you could point to here?

> Additionally, when the port that com1 describes is not present, scan PCI
> for a known UART before giving up. This way the same command line works on
> systems with and without a legacy UART, and a machine-specific "pci"
> option is not needed. The fallback does not run when the command line
> gave an I/O base, or when it already asked for a scan with "pci" or
> "amt", so explicit config keeps its current meaning. It is
> for com1 only: for com2, pci_uart_config() skips the first port it
> finds, so it can never match a single-port device.

This needs to be split to a separate patch, and not only because right
now you're doing two entirely unrelated things in a single change. The
addition of the Amazon device is likely uncontroversial, so presumably
can go in quickly. Doing a scan when none was asked for, otoh, is
potentially problematic: What if there's an issue during scanning? With
not having any output set up yet, we couldn't even indicate the problem,
and a possible crash would also go entirely silently.

> When the scan finds nothing, pci_uart_config() puts back the original
> base, and check_existence() does not test MMIO addresses.
> 
> On a system with a legacy UART, check_existence() passes and nothing
> changes.

This isn't really true, is it? You check ...

> @@ -1805,7 +1825,32 @@ static void __init ns16550_parse_port_config(
>      if ( uart->io_base == 0 )
>          PARSE_ERR("I/O base address must be specified.");
>      if ( !check_existence(uart) )
> -        PARSE_ERR("16550-compatible serial UART not present");
> +    {
> +        bool present = false;
> +
> +#ifdef NS16550_PCI
> +        /*
> +         * Some systems, EC2 bare metal among them, have no legacy UART and
> +         * carry their only serial port on PCI. Look for one before giving up,
> +         * unless the command line named a base or already asked for a scan.
> +         * com1 only: for com2 the scan skips the first port it finds, so it
> +         * can never match a single-port device.
> +         */
> +        if ( uart == ns16550_com && !uart->io_base_set && !uart->pci_scanned )

... ->io_base_set here, which can only be true when a command line option
provided the base address. A cmdline option like "com1=115200" doesn't,
and instead the value set by (on x86) __start_xen() is used. If that isn't
the correct address to use, check_existence() is (hopefully) going to fail.
(For example, I have a system where firmware mixes up COM1 and COM2
settings, which you may only notice after having played with things for a
while. In such a case it doesn't help if unexpected PCI bus scanning gets
in the way.)

Jan


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] ns16550: find the console UART on PCI when there is no legacy one
  2026-09-23  6:11 ` Jan Beulich
@ 2026-09-23 15:38   ` Benjamin Leggett
  0 siblings, 0 replies; 8+ messages in thread
From: Benjamin Leggett @ 2026-09-23 15:38 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
	Roger Pau Monné, Stefano Stabellini, xen-devel

Jan Beulich <jbeulich@suse.com> writes:

> Is there a spec that you could point to here?

Unfortunately not really, just an upstream Linux commit from 2017 (<https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=3bfd1300abfe3adb18e84a89d97a0e82a22124bb>
), and the fact that I’ve tested it on EC2 bare metal boxes. The bare metal
boxes are being phased out, but people still use them.

> This needs to be split to a separate patch, and not only because right
> now you’re doing two entirely unrelated things in a single change. The
> addition of the Amazon device is likely uncontroversial, so presumably
> can go in quickly.

Yes, agreed. I will split and separately submit them.

> Doing a scan when none was asked for, otoh, is
> potentially problematic: What if there’s an issue during scanning? With
> not having any output set up yet, we couldn’t even indicate the problem,
> and a possible crash would also go entirely silently.

Yep, thanks. I will see if I can address these with a different version of
the autoscan patch as a followup.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ns16550: add the Amazon EC2 PCI serial device
  2026-09-22 18:43 [PATCH] ns16550: find the console UART on PCI when there is no legacy one Benjamin Leggett
  2026-09-23  6:11 ` Jan Beulich
@ 2026-09-23 15:39 ` Benjamin Leggett
  2026-09-24  8:26   ` Jan Beulich
  2026-10-09 20:24 ` [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART Benjamin Leggett
  2 siblings, 1 reply; 8+ messages in thread
From: Benjamin Leggett @ 2026-09-23 15:39 UTC (permalink / raw)
  To: xen-devel
  Cc: Benjamin Leggett, Andrew Cooper, Anthony PERARD, Michal Orzel,
	Jan Beulich, Julien Grall, Roger Pau Monné,
	Stefano Stabellini

Amazon EC2 bare metal instances have no UART at the legacy I/O port
0x3f8. Their only serial port is a 16550-compatible PCI device (vendor
0x1d0f, device 0x8250) with its registers in the MMIO space of BAR 0.

pci_uart_config() uses the default parameters for a device that is not
in uart_config[], and those only accept an I/O BAR, so "com1=...,pci"
cannot find this device. Add it, with the same layout that Linux uses
(commit 3bfd1300abfe ("serial: 8250_pci: Add Amazon PCI serial device
ID")): one port at the start of BAR 0, 1-byte registers, and the
default 1.8432MHz clock.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Leggett <benjamin@edera.io>
---
 xen/drivers/char/ns16550.c | 14 ++++++++++++++
 xen/include/xen/pci_ids.h  |  2 ++
 2 files changed, 16 insertions(+)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 120ac09d23..593d208483 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -98,6 +98,7 @@ struct ns16550_config {
         param_intel_lpss,
         param_wch_ch382,
         param_asix,
+        param_amazon,
     } param;
 };
 
@@ -909,6 +910,13 @@ static const struct ns16550_config_param __initconst uart_param[] = {
         .bar0 = true,
         .max_ports = 1,
     },
+    [param_amazon] = {
+        .reg_width = 1,
+        .lsr_mask = UART_LSR_THRE,
+        .bar0 = true,
+        .mmio = true,
+        .max_ports = 1,
+    },
 };
 
 static const struct ns16550_config __initconst uart_config[] =
@@ -1255,6 +1263,12 @@ static const struct ns16550_config __initconst uart_config[] =
         .dev_id = 0x9910,
         .param = param_asix
     },
+    /* Amazon EC2 bare metal UART */
+    {
+        .vendor_id = PCI_VENDOR_ID_AMAZON,
+        .dev_id = 0x8250,
+        .param = param_amazon
+    },
 };
 
 static int __init
diff --git a/xen/include/xen/pci_ids.h b/xen/include/xen/pci_ids.h
index fd424ef55d..a17c88dcf7 100644
--- a/xen/include/xen/pci_ids.h
+++ b/xen/include/xen/pci_ids.h
@@ -17,6 +17,8 @@
 
 #define PCI_VENDOR_ID_WCHIC              0x1c00
 
+#define PCI_VENDOR_ID_AMAZON             0x1d0f
+
 #define PCI_VENDOR_ID_INTEL              0x8086
 
 #endif /* XEN_PCI_IDS_H */
-- 
2.55.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ns16550: add the Amazon EC2 PCI serial device
  2026-09-23 15:39 ` [PATCH] ns16550: add the Amazon EC2 PCI serial device Benjamin Leggett
@ 2026-09-24  8:26   ` Jan Beulich
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2026-09-24  8:26 UTC (permalink / raw)
  To: Benjamin Leggett
  Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
	Roger Pau Monné, Stefano Stabellini, xen-devel

On 23.09.2026 17:39, Benjamin Leggett wrote:
> Amazon EC2 bare metal instances have no UART at the legacy I/O port
> 0x3f8. Their only serial port is a 16550-compatible PCI device (vendor
> 0x1d0f, device 0x8250) with its registers in the MMIO space of BAR 0.
> 
> pci_uart_config() uses the default parameters for a device that is not
> in uart_config[], and those only accept an I/O BAR, so "com1=...,pci"
> cannot find this device. Add it, with the same layout that Linux uses
> (commit 3bfd1300abfe ("serial: 8250_pci: Add Amazon PCI serial device
> ID")): one port at the start of BAR 0, 1-byte registers, and the
> default 1.8432MHz clock.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Benjamin Leggett <benjamin@edera.io>

Acked-by: Jan Beulich <jbeulich@suse.com>



^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART
  2026-09-22 18:43 [PATCH] ns16550: find the console UART on PCI when there is no legacy one Benjamin Leggett
  2026-09-23  6:11 ` Jan Beulich
  2026-09-23 15:39 ` [PATCH] ns16550: add the Amazon EC2 PCI serial device Benjamin Leggett
@ 2026-10-09 20:24 ` Benjamin Leggett
  2026-10-12 12:30   ` Jan Beulich
  2026-10-13 20:49   ` Benjamin Leggett
  2 siblings, 2 replies; 8+ messages in thread
From: Benjamin Leggett @ 2026-10-09 20:24 UTC (permalink / raw)
  To: xen-devel
  Cc: Benjamin Leggett, Andrew Cooper, Anthony PERARD, Michal Orzel,
	Jan Beulich, Julien Grall, Roger Pau Monné,
	Stefano Stabellini

On some systems the console UART is at the legacy I/O port, and on
others, such as Amazon EC2 bare metal instances, the only UART is on
PCI. "com1=...,pci" finds the PCI UART, but it prefers any PCI serial
device to the legacy port, so it is not correct on a system that has
both. No option today says "use the legacy port, and scan PCI only if
it has no UART". A deployment that boots one command line on many
kinds of machine needs exactly that.

Add "auto" as a value in the <io-base> position. With it, Xen probes
the default base as usual. Only if check_existence() fails there does
it scan PCI, as "pci" does. Without "auto", nothing changes.

The scan runs only when the command line asks for it, at the same
point in boot as "pci" and "amt". There is no console yet, but the
scan is not silent: it logs the device that it picked, and
PARSE_ERR() logs a failure. Both messages go to the console ring, and
console_init_preirq() replays the ring to each console that comes up,
VGA included. Only a hang or a crash inside the scan goes unreported,
as with "pci" and "amt" today.

To limit what an unexpected scan can touch, "auto" only considers
devices that are in uart_config[]. A serial-class device without
built-in parameters is skipped before its BARs are sized, so a
firmware that misreports the legacy port cannot move com1 to an
arbitrary plug-in card.

When the scan finds nothing, pci_uart_config() puts back the original
base, and check_existence() does not test MMIO addresses. Thus
ps_bdf_enable, not check_existence(), tells whether a device was
found.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Leggett <benjamin@edera.io>
---
 docs/misc/xen-command-line.pandoc |  5 ++++-
 xen/drivers/char/ns16550.c        | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index ea0e3368b1..58ef53a5b2 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -357,7 +357,7 @@ ACPI indicating none to be there.
 
 ### com1 (x86)
 ### com2 (x86)
-> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt|auto][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -387,6 +387,9 @@ Both option `com1` and `com2` follow the same format.
   avoiding Intel AMT devices.
 * `amt` indicated that Xen should scan the PCI bus for the UART,
   including Intel AMT devices if present.
+* `auto` indicates that Xen should use the default I/O base if a UART
+  responds there, and otherwise scan the PCI bus for the UART as `pci`
+  does, but only for devices with built-in parameters in Xen.
 
 A typical setup for most situations might be `com1=115200,8n1`
 
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index cdda28879e..ab55e2ba45 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -66,6 +66,7 @@ static struct ns16550 {
     bool dw_usr_bsy;
 #ifdef NS16550_PCI
     /* PCI card parameters. */
+    bool pci_fallback;      /* if =1, scan PCI if io_base has no UART */
     bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind bridge */
     bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */
     pci_sbdf_t pci_bridge;
@@ -1321,6 +1322,10 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                     }
                 }
 
+                /* Keep "auto" off unknown devices, including their BARs. */
+                if ( uart->pci_fallback && i == ARRAY_SIZE(uart_config) )
+                    continue;
+
                 if ( port_idx >= param->max_ports )
                 {
                     idx -= param->max_ports;
@@ -1628,6 +1633,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
                 return true;
             conf += 3;
         }
+        else if ( strncmp(conf, "auto", 4) == 0 )
+        {
+            uart->pci_fallback = true;
+            conf += 4;
+        }
         else
 #endif
         {
@@ -1819,7 +1829,26 @@ static void __init ns16550_parse_port_config(
     if ( uart->io_base == 0 )
         PARSE_ERR("I/O base address must be specified.");
     if ( !check_existence(uart) )
-        PARSE_ERR("16550-compatible serial UART not present");
+    {
+#ifdef CONFIG_HAS_PCI
+        if ( uart->pci_fallback )
+        {
+            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
+            if ( uart->ps_bdf_enable )
+                printk("ns16550: com%u: no UART at default base, using %pp\n",
+                       (unsigned int)(uart - ns16550_com) + 1,
+                       &uart->pci_device);
+        }
+
+        /*
+         * A failed scan puts back the rejected base, and check_existence()
+         * passes MMIO addresses untested, so only ps_bdf_enable says that
+         * a device was found.
+         */
+        if ( !uart->ps_bdf_enable || !check_existence(uart) )
+#endif
+            PARSE_ERR("16550-compatible serial UART not present");
+    }
 
     /* Register with generic serial driver. */
     serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
-- 
2.55.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART
  2026-10-09 20:24 ` [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART Benjamin Leggett
@ 2026-10-12 12:30   ` Jan Beulich
  2026-10-13 20:49   ` Benjamin Leggett
  1 sibling, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2026-10-12 12:30 UTC (permalink / raw)
  To: Benjamin Leggett
  Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Julien Grall,
	Roger Pau Monné, Stefano Stabellini, xen-devel

On 09.10.2026 22:24, Benjamin Leggett wrote:
> On some systems the console UART is at the legacy I/O port, and on
> others, such as Amazon EC2 bare metal instances, the only UART is on
> PCI. "com1=...,pci" finds the PCI UART, but it prefers any PCI serial
> device to the legacy port, so it is not correct on a system that has
> both. No option today says "use the legacy port, and scan PCI only if
> it has no UART". A deployment that boots one command line on many
> kinds of machine needs exactly that.
> 
> Add "auto" as a value in the <io-base> position. With it, Xen probes
> the default base as usual. Only if check_existence() fails there does
> it scan PCI, as "pci" does. Without "auto", nothing changes.

I've always been considering check_existence() as somewhat fragile (it's
not very difficult for it to yield a false positive, especially when
HAS_IOPORTS=n), which means that I'm not really convinced it can (reliably)
be used for the purpose here.

> @@ -1628,6 +1633,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
>                  return true;
>              conf += 3;
>          }
> +        else if ( strncmp(conf, "auto", 4) == 0 )
> +        {
> +            uart->pci_fallback = true;
> +            conf += 4;
> +        }
>          else
>  #endif
>          {

What about parse_namevalue_pairs()? Once you also adjust that, you'll notice
that ...

> @@ -1819,7 +1829,26 @@ static void __init ns16550_parse_port_config(
>      if ( uart->io_base == 0 )
>          PARSE_ERR("I/O base address must be specified.");
>      if ( !check_existence(uart) )
> -        PARSE_ERR("16550-compatible serial UART not present");
> +    {
> +#ifdef CONFIG_HAS_PCI
> +        if ( uart->pci_fallback )
> +        {
> +            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
> +            if ( uart->ps_bdf_enable )

... this isn't a good value to check, as it can also be set from the cmdline.
Perhaps checking ->param is an alternative.

> +                printk("ns16550: com%u: no UART at default base, using %pp\n",
> +                       (unsigned int)(uart - ns16550_com) + 1,

Would be nice if we could get away without cast here. While out printk()
still doesn't support the t modifier, aiui the z one should do here.

Jan


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART
  2026-10-09 20:24 ` [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART Benjamin Leggett
  2026-10-12 12:30   ` Jan Beulich
@ 2026-10-13 20:49   ` Benjamin Leggett
  1 sibling, 0 replies; 8+ messages in thread
From: Benjamin Leggett @ 2026-10-13 20:49 UTC (permalink / raw)
  To: xen-devel
  Cc: Benjamin Leggett, Andrew Cooper, Anthony PERARD, Michal Orzel,
	Jan Beulich, Julien Grall, Roger Pau Monné,
	Stefano Stabellini

On some systems the console UART is at the legacy I/O port, and on
others, such as Amazon EC2 bare metal instances, the only UART is on
PCI. "com1=...,pci" finds the PCI UART, but it prefers any PCI serial
device to the legacy port, so it is not correct on a system that has
both. No option today says "use the legacy port, and scan PCI only if
it has no UART". A deployment that boots one command line on many
kinds of machine needs exactly that.

Add "auto" as a value in the <io-base> position, and as "dev=auto".
With it, Xen probes the default base as usual. Only if
check_existence() fails there does it scan PCI, as "pci" does.
Without "auto", nothing changes.

The decision rests on check_existence(), which until now reported an
MMIO base as present without testing it. Make it return -1 for a base
that it cannot probe, so that callers can tell "no UART" apart from
"not probed". The HAS_IOPORTS=n branch returns -1 too, for the same
reason, although it is not reachable today: the function is x86-only,
and x86 always selects HAS_IOPORTS. Existing behavior does not change,
as -1 is still non-zero, but "auto" now refuses a base that cannot be
probed. The decision therefore always comes from the I/O port probe,
and with nothing at the port, reads return 0xff and the IER test
fails.

The scan runs only when the command line asks for it, at the same
point in boot as "pci" and "amt". There is no console yet, but the
scan is not silent: it logs the device that it picked, and
PARSE_ERR() logs a failure. Both messages go to the console ring, and
console_init_preirq() replays the ring to each console that comes up,
VGA included. Only a hang or a crash inside the scan goes unreported,
as with "pci" and "amt" today.

To limit what an unexpected scan can touch, "auto" only considers
devices that are in uart_config[]. A serial-class device without
built-in parameters is skipped before its BARs are sized, so a
firmware that misreports the legacy port cannot move com1 to an
arbitrary plug-in card.

Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Benjamin Leggett <benjamin@edera.io>
---
 docs/misc/xen-command-line.pandoc |  9 ++++--
 xen/drivers/char/ns16550.c        | 46 ++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index a6f08463d1..6fff62800f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -357,7 +357,7 @@ ACPI indicating none to be there.
 
 ### com1 (x86)
 ### com2 (x86)
-> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt|auto][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -387,6 +387,9 @@ Both option `com1` and `com2` follow the same format.
   avoiding Intel AMT devices.
 * `amt` indicated that Xen should scan the PCI bus for the UART,
   including Intel AMT devices if present.
+* `auto` indicates that Xen should use the default I/O base if a UART
+  responds there, and otherwise scan the PCI bus for the UART as `pci`
+  does, but only for devices with built-in parameters in Xen.
 
 A typical setup for most situations might be `com1=115200,8n1`
 
@@ -411,9 +414,9 @@ The accepted name keywords for name=value pairs are:
 * `clock-hz`- accepts large integers to setup UART clock frequencies.
               Do note - these values are multiplied by 16.
 * `data-bits` - integer between 5 and 8
-* `dev` - accepted values are `pci` OR `amt`. If this option
+* `dev` - accepted values are `pci`, `amt` OR `auto`. If this option
           is used to specify if the serial device is pci-based. The io_base
-          cannot be specified when `dev=pci` or `dev=amt` is used.
+          cannot be specified when `dev=pci`, `dev=amt` or `dev=auto` is used.
 * `io-base` - accepts integer which specified IO base port for UART registers
 * `irq` - IRQ number to use
 * `parity` - accepted values are same as positional parameters
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index cdda28879e..56de4d50fb 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -66,6 +66,7 @@ static struct ns16550 {
     bool dw_usr_bsy;
 #ifdef NS16550_PCI
     /* PCI card parameters. */
+    bool pci_fallback;      /* if =1, scan PCI if io_base has no UART */
     bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind bridge */
     bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */
     pci_sbdf_t pci_bridge;
@@ -727,6 +728,7 @@ static int __init parse_parity_char(int c)
     return 0;
 }
 
+/* Returns 1 if a UART answers at io_base, 0 if none does, -1 if untestable. */
 static int __init check_existence(struct ns16550 *uart)
 {
     unsigned char status, scratch, scratch2, scratch3;
@@ -737,9 +739,9 @@ static int __init check_existence(struct ns16550 *uart)
      * if we're getting MMIO UARTs, the arch code knows what it's doing.
      */
     if ( uart->io_base >= 0x10000 )
-        return 1;
+        return -1;
 #else
-    return 1; /* Everything is MMIO */
+    return -1; /* Everything is MMIO */
 #endif
 
     pci_serial_early_init(uart);
@@ -1321,6 +1323,10 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                     }
                 }
 
+                /* Keep "auto" off unknown devices, including their BARs. */
+                if ( uart->pci_fallback && i == ARRAY_SIZE(uart_config) )
+                    continue;
+
                 if ( port_idx >= param->max_ports )
                 {
                     idx -= param->max_ports;
@@ -1628,6 +1634,11 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
                 return true;
             conf += 3;
         }
+        else if ( strncmp(conf, "auto", 4) == 0 )
+        {
+            uart->pci_fallback = true;
+            conf += 4;
+        }
         else
 #endif
         {
@@ -1745,6 +1756,11 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                 pci_uart_config(uart, 0, uart - ns16550_com);
                 dev_set = true;
             }
+            else if ( strncmp(param_value, "auto", 4) == 0 )
+            {
+                uart->pci_fallback = true;
+                dev_set = true;
+            }
             else
                 PARSE_ERR_RET("Unknown device type %s\n", param_value);
             break;
@@ -1785,6 +1801,7 @@ static void __init ns16550_parse_port_config(
 {
     char com_console_options[128];
     char *str;
+    int present;
 
     /* No user-specified configuration? */
     if ( (conf == NULL) || (*conf == '\0') )
@@ -1818,8 +1835,29 @@ static void __init ns16550_parse_port_config(
         PARSE_ERR("%d stop bits are unsupported.", uart->stop_bits);
     if ( uart->io_base == 0 )
         PARSE_ERR("I/O base address must be specified.");
-    if ( !check_existence(uart) )
-        PARSE_ERR("16550-compatible serial UART not present");
+    present = check_existence(uart);
+#ifdef CONFIG_HAS_PCI
+    if ( uart->pci_fallback && present < 0 )
+        PARSE_ERR("\"auto\" needs a base that can be probed");
+#endif
+    if ( !present )
+    {
+#ifdef CONFIG_HAS_PCI
+        if ( uart->pci_fallback )
+            pci_uart_config(uart, 1 /* skip AMT */, uart - ns16550_com);
+
+        /*
+         * A failed scan puts back the rejected base, and check_existence()
+         * can't test an MMIO BAR, so only ->param says that a device was
+         * found.
+         */
+        if ( uart->pci_fallback && uart->param && check_existence(uart) )
+            printk("ns16550: com%zd: no UART at default base, using %pp\n",
+                   uart - ns16550_com + 1, &uart->pci_device);
+        else
+#endif
+            PARSE_ERR("16550-compatible serial UART not present");
+    }
 
     /* Register with generic serial driver. */
     serial_register_uart(uart - ns16550_com, &ns16550_driver, uart);
-- 
2.55.0



^ permalink raw reply related	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-10-13 20:50 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-22 18:43 [PATCH] ns16550: find the console UART on PCI when there is no legacy one Benjamin Leggett
2026-09-23  6:11 ` Jan Beulich
2026-09-23 15:38   ` Benjamin Leggett
2026-09-23 15:39 ` [PATCH] ns16550: add the Amazon EC2 PCI serial device Benjamin Leggett
2026-09-24  8:26   ` Jan Beulich
2026-10-09 20:24 ` [PATCH] ns16550: add "auto" to scan PCI only when the default base has no UART Benjamin Leggett
2026-10-12 12:30   ` Jan Beulich
2026-10-13 20:49   ` Benjamin Leggett

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.