Introduce extra logic to allow parsing ACPI tables extra early, and use it
to parse the ACPI SPCR table and obtain the serial configuration.

This is gated to the "acpi" device type being set in "com1" on the Xen
command line.  Note that there can only be one serial device described in
the SPCR, so limit it's usage to com1 exclusively for the time being.

I can't test the interrupt information parsing on my system, as the
interrupt is set to GSI with a value of 0xff, which is outside of the range
of GSIs available on the system.  I've also assumed that the interrupt
being 0xff is used to signal not interrupt setup (just like the Interrupt
Pin register on PCI headers).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
WIP/RFC, not sure whether there's interest in attempting to pursue this
further on x86.  So far the device I have is also exposed on the PCI bus
aside from SPCR, so using com1=device=amt also works to detect it.

Posting it kind of early to know whether I should try to polish it for
submission or we are happy with not having this on x86.
---
 docs/misc/xen-command-line.pandoc |   8 +-
 xen/drivers/acpi/tables/tbutils.c | 131 +++++++++++++++----
 xen/drivers/char/ns16550.c        | 202 +++++++++++++++++++++++-------
 xen/include/acpi/actables.h       |   5 +
 xen/include/acpi/actbl2.h         |  10 ++
 5 files changed, 282 insertions(+), 74 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index ebdca007d26b..12e1965cfb60 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -349,7 +349,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|acpi][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -379,6 +379,8 @@ 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.
+* `acpi` indicates Xen should use ACPI information (from SPCR table) if present
+  to configure the serial console.  This option is only supported for com1.
 
 A typical setup for most situations might be `com1=115200,8n1`
 
@@ -403,9 +405,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 `acpi`. 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|amt|acpi` 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/acpi/tables/tbutils.c b/xen/drivers/acpi/tables/tbutils.c
index 458989abea99..c5eb6b1fc523 100644
--- a/xen/drivers/acpi/tables/tbutils.c
+++ b/xen/drivers/acpi/tables/tbutils.c
@@ -341,39 +341,20 @@ acpi_tb_get_root_table_entry(u8 * table_entry,
 	}
 }
 
-/*******************************************************************************
- *
- * FUNCTION:    acpi_tb_parse_root_table
- *
- * PARAMETERS:  Rsdp                    - Pointer to the RSDP
- *              Flags                   - Flags
- *
- * RETURN:      Status
- *
- * DESCRIPTION: This function is called to parse the Root System Description
- *              Table (RSDT or XSDT)
- *
- * NOTE:        Tables are mapped (not copied) for efficiency. The FACS must
- *              be mapped and cannot be copied because it contains the actual
- *              memory location of the ACPI Global Lock.
- *
- ******************************************************************************/
-
-acpi_status __init
-acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
+static acpi_status __init
+acpi_tb_get_root_table(acpi_physical_address rsdp_address,
+	               struct acpi_table_header **out_table,
+		       unsigned int *entry_size)
 {
 	struct acpi_table_rsdp *rsdp;
 	acpi_native_uint table_entry_size;
-	acpi_native_uint i;
-	u32 table_count;
 	struct acpi_table_header *table;
 	acpi_physical_address address;
 	acpi_physical_address rsdt_address = 0;
 	u32 length;
-	u8 *table_entry;
 	acpi_status status;
 
-	ACPI_FUNCTION_TRACE(tb_parse_root_table);
+	ACPI_FUNCTION_TRACE(tb_get_root_table);
 
 	/*
 	 * Map the entire RSDP and extract the address of the RSDT or XSDT
@@ -454,6 +435,42 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 		return_ACPI_STATUS(status);
 	}
 
+	*out_table = table;
+	*entry_size = table_entry_size;
+	return_ACPI_STATUS(AE_OK);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_tb_parse_root_table
+ *
+ * PARAMETERS:  Rsdp                    - Pointer to the RSDP
+ *              Flags                   - Flags
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: This function is called to parse the Root System Description
+ *              Table (RSDT or XSDT)
+ *
+ * NOTE:        Tables are mapped (not copied) for efficiency. The FACS must
+ *              be mapped and cannot be copied because it contains the actual
+ *              memory location of the ACPI Global Lock.
+ *
+ ******************************************************************************/
+
+acpi_status __init
+acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
+{
+	struct acpi_table_header *table;
+	unsigned int table_count, table_entry_size, i;
+	void *table_entry;
+	acpi_status status;
+
+	status = acpi_tb_get_root_table(rsdp_address, &table,
+	                                &table_entry_size);
+	if (!ACPI_SUCCESS(status))
+		  return_ACPI_STATUS(status);
+
 	/* Calculate the number of tables described in the root table */
 
 	table_count =
@@ -502,7 +519,7 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 	 * It is not possible to map more than one entry in some environments,
 	 * so unmap the root table here before mapping other tables
 	 */
-	acpi_os_unmap_memory(table, length);
+	acpi_os_unmap_memory(table, table->length);
 
 	/*
 	 * Complete the initialization of the root table array by examining
@@ -523,3 +540,67 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 
 	return_ACPI_STATUS(AE_OK);
 }
+
+acpi_status __init
+acpi_early_get_table(const char *signature, acpi_native_uint instance,
+                     struct acpi_table_header **out_table)
+{
+	static acpi_physical_address __initdata table_addr[128];
+	static unsigned int __initdata table_count;
+	static unsigned int __initdata table_entry_size;
+	unsigned int i;
+
+	ACPI_FUNCTION_TRACE(tb_early_get_table);
+
+	if (!table_count) {
+		struct acpi_table_header *table;
+		void *table_entry;
+		acpi_status status;
+		acpi_physical_address rsdp_address = acpi_os_get_root_pointer();
+
+		if (!rsdp_address)
+			return_ACPI_STATUS(AE_NOT_FOUND);
+
+		status = acpi_tb_get_root_table(rsdp_address, &table,
+			                        &table_entry_size);
+		if (!ACPI_SUCCESS(status))
+			return_ACPI_STATUS(status);
+
+		/* Calculate the number of tables described in the root table */
+		table_count = (table->length - sizeof(*table)) / table_entry_size;
+
+		if (table_count > ARRAY_SIZE(table_addr)) {
+			table_count = 0;
+			return_ACPI_STATUS(AE_NO_MEMORY);
+		}
+
+		table_entry = ACPI_CAST_PTR(uint8_t, table) + sizeof(*table);
+
+		for (i = 0; i < table_count; i++) {
+			table_addr[i] =
+			      acpi_tb_get_root_table_entry(table_entry,
+			                                   table_entry_size);
+			table_entry += table_entry_size;
+		}
+
+		acpi_os_unmap_memory(table, table->length);
+	}
+
+	for (i = 0; i < table_count; i++) {
+		struct acpi_table_header *header =
+		      acpi_os_map_memory(table_addr[i], sizeof(*header));
+
+		if (ACPI_COMPARE_NAME(header->signature, signature)) {
+			unsigned int len = header->length;
+
+			acpi_os_unmap_memory(header, sizeof(*header));
+			*out_table = acpi_os_map_memory(table_addr[i], len);
+
+			return_ACPI_STATUS(AE_OK);
+		}
+
+		acpi_os_unmap_memory(header, sizeof(*header));
+	}
+
+	return_ACPI_STATUS(AE_NOT_FOUND);
+}
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9cd3e471bfa5..606dd404fd76 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1375,6 +1375,136 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
 
 #endif /* CONFIG_HAS_PCI */
 
+#ifdef CONFIG_ACPI
+#include <acpi/acpi.h>
+#include <acpi/actables.h>
+
+#include <xen/acpi.h>
+
+static int __init acpi_uart_config(struct ns16550 *uart, unsigned int idx)
+{
+    struct acpi_table_header *table;
+    struct acpi_table_spcr *spcr;
+    acpi_status status;
+    int rc = 0;
+
+    /*
+     * SPCR specifies a single port, expect it to be configured at position 0
+     * in the uart array.
+     */
+    if ( idx )
+        return -EXDEV;
+
+    if ( system_state <= SYS_STATE_early_boot )
+        status = acpi_early_get_table(ACPI_SIG_SPCR, 0, &table);
+    else
+        status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
+
+    if ( ACPI_FAILURE(status) )
+    {
+        printk(XENLOG_ERR "Failed to find or parse ACPI SPCR table\n");
+        return -ENODEV;
+    }
+
+    spcr = container_of(table, struct acpi_table_spcr, header);
+
+    rc = -EDOM;
+    if ( spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE )
+    {
+        printk(XENLOG_ERR "Incompatible ACPI SPCR UART interface %u\n",
+               spcr->interface_type);
+        goto out;
+    }
+
+    if ( spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+         (IS_ENABLED(CONFIG_ARM) ||
+          spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_IO) )
+    {
+        printk(XENLOG_ERR "Incompatible ACPI SPCR UART address space %u\n",
+               spcr->serial_port.space_id);
+        goto out;
+    }
+
+    if ( !spcr->serial_port.address )
+    {
+        printk(XENLOG_ERR "ACPI SPCR console redirection disabled\n");
+        goto out;
+    }
+
+    uart->io_base = spcr->serial_port.address;
+    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
+    uart->reg_shift = spcr->serial_port.bit_offset;
+
+    uart->parity = spcr->parity;
+    uart->stop_bits = spcr->stop_bits;
+    uart->data_bits = 8;
+
+    if ( uart->baud == BAUD_AUTO && spcr->baud_rate )
+    {
+        switch ( spcr->baud_rate )
+        {
+        case ACPI_SPCR_BAUD_9600:
+            uart->baud = 9600;
+            break;
+
+        case ACPI_SPCR_BAUD_19200:
+            uart->baud = 19200;
+            break;
+
+        case ACPI_SPCR_BAUD_57600:
+            uart->baud = 57600;
+            break;
+
+        case ACPI_SPCR_BAUD_115200:
+            uart->baud = 115200;
+            break;
+
+        default:
+            printk(XENLOG_WARNING
+                   "Ignoring invalid baud rate %u in ACPI SPCR\n",
+                   spcr->baud_rate);
+        }
+    }
+
+    if ( IS_ENABLED(CONFIG_X86) )
+    {
+        /* Use polling mode by default. */
+        uart->irq = 0;
+
+        if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_IO_APIC) &&
+             spcr->interrupt < 0xff )
+            uart->irq = spcr->interrupt;
+        else if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_PC_AT) &&
+                  ((spcr->pc_interrupt >=  2 && spcr->pc_interrupt <=  7) ||
+                   (spcr->pc_interrupt >=  9 && spcr->pc_interrupt <= 12) ||
+                   (spcr->pc_interrupt >= 14 && spcr->pc_interrupt <= 15)) )
+            uart->irq = spcr->pc_interrupt;
+    }
+
+#ifdef CONFIG_ARM
+    /* The trigger/polarity information is not available in spcr. */
+    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    uart->irq = spcr->interrupt;
+#endif /* CONFIG_ARM */
+
+#ifdef CONFIG_HAS_PCI
+    if ( spcr->pci_device_id != 0xffff && spcr->pci_vendor_id != 0xffff )
+    {
+        uart->ps_bdf_enable = true;
+        uart->pci_device = PCI_SBDF(spcr->pci_segment, spcr->pci_bus,
+                                    spcr->pci_device, spcr->pci_function);
+    }
+#endif /* CONFIG_HAS_PCI */
+
+    rc = 0;
+
+ out:
+    if ( system_state <= SYS_STATE_early_boot )
+        acpi_os_unmap_memory(&spcr, spcr->header.length);
+    return rc;
+}
+#endif /* CONFIG_ACPI */
+
 /*
  * Configure serial port with a string:
  *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
@@ -1539,6 +1669,15 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
             conf += 3;
         }
         else
+#endif
+#ifdef CONFIG_ACPI
+        if ( strncmp(conf, "acpi", 4) == 0 )
+        {
+            if ( acpi_uart_config(uart, uart - ns16550_com) )
+                return true;
+            conf += 4;
+        }
+        else
 #endif
         {
             uart->io_base = simple_strtoull(conf, &conf, 0);
@@ -1643,8 +1782,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             uart->reg_width = simple_strtoul(param_value, NULL, 0);
             break;
 
-#ifdef CONFIG_HAS_PCI
         case device:
+#ifdef CONFIG_ACPI
+            if ( strncmp(param_value, "acpi", 3) == 0 )
+            {
+                acpi_uart_config(uart, uart - ns16550_com);
+                dev_set = true;
+                break;
+            }
+            else
+#endif /* CONFIG_ACPI */
+#ifdef CONFIG_HAS_PCI
             if ( strncmp(param_value, "pci", 3) == 0 )
             {
                 pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
@@ -1656,9 +1804,11 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                 dev_set = true;
             }
             else
+#endif /* CONFIG_HAS_PCI */
                 PARSE_ERR_RET("Unknown device type %s\n", param_value);
             break;
 
+#if CONFIG_HAS_PCI
         case port_bdf:
         {
             unsigned int b, d, f;
@@ -1680,7 +1830,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             uart->pb_bdf_enable = true;
             break;
         }
-#endif
+#endif /* CONFIG_HAS_PCI */
 
         default:
             PARSE_ERR_RET("Invalid parameter: %s\n", token);
@@ -1845,8 +1995,6 @@ DT_DEVICE_END
 #endif /* HAS_DEVICE_TREE_DISCOVERY */
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
-#include <xen/acpi.h>
-
 static int __init ns16550_acpi_uart_init(const void *data)
 {
     struct acpi_table_header *table;
@@ -1857,51 +2005,13 @@ static int __init ns16550_acpi_uart_init(const void *data)
      * Only support one UART on ARM which happen to be ns16550_com[0].
      */
     struct ns16550 *uart = &ns16550_com[0];
-
-    status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
-    if ( ACPI_FAILURE(status) )
-    {
-        printk("ns16550: Failed to get SPCR table\n");
-        return -EINVAL;
-    }
-
-    spcr = container_of(table, struct acpi_table_spcr, header);
-
-    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
-    {
-        printk("ns16550: Address space type is not mmio\n");
-        return -EINVAL;
-    }
-
-    /*
-     * The serial port address may be 0 for example
-     * if the console redirection is disabled.
-     */
-    if ( unlikely(!spcr->serial_port.address) )
-    {
-        printk("ns16550: Console redirection is disabled\n");
-        return -EINVAL;
-    }
+    int rc;
 
     ns16550_init_common(uart);
 
-    /*
-     * The baud rate is pre-configured by the firmware.
-     * And currently the ACPI part is only targeting ARM so the flow_control
-     * field and all PCI related ones which we do not care yet are ignored.
-     */
-    uart->baud = BAUD_AUTO;
-    uart->data_bits = 8;
-    uart->parity = spcr->parity;
-    uart->stop_bits = spcr->stop_bits;
-    uart->io_base = spcr->serial_port.address;
-    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
-    uart->reg_shift = spcr->serial_port.bit_offset;
-    uart->reg_width = spcr->serial_port.access_width;
-
-    /* The trigger/polarity information is not available in spcr. */
-    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
-    uart->irq = spcr->interrupt;
+    rc = acpi_uart_config(uart, 0);
+    if ( rc )
+        return rc;
 
     uart->vuart.base_addr = uart->io_base;
     uart->vuart.size = uart->io_size;
diff --git a/xen/include/acpi/actables.h b/xen/include/acpi/actables.h
index 527e1c9f9b9d..afb808998825 100644
--- a/xen/include/acpi/actables.h
+++ b/xen/include/acpi/actables.h
@@ -104,4 +104,9 @@ acpi_tb_install_table(acpi_physical_address address,
 acpi_status
 acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags);
 
+/* Get a table early, before acpi_tb_parse_root_table() is called. */
+acpi_status
+acpi_early_get_table(const char *signature, acpi_native_uint instance,
+                     struct acpi_table_header **out_table);
+
 #endif				/* __ACTABLES_H__ */
diff --git a/xen/include/acpi/actbl2.h b/xen/include/acpi/actbl2.h
index ee96e990d636..923c784e8bd5 100644
--- a/xen/include/acpi/actbl2.h
+++ b/xen/include/acpi/actbl2.h
@@ -1037,6 +1037,16 @@ struct acpi_table_spcr {
 
 #define ACPI_SPCR_DO_NOT_DISABLE    (1)
 
+/* Masks for interrupt_type field above */
+#define ACPI_SPCR_INTR_TYPE_PC_AT   0x01
+#define ACPI_SPCR_INTR_TYPE_IO_APIC 0x02
+
+/* Values for the baud_rate field above */
+#define ACPI_SPCR_BAUD_9600         3
+#define ACPI_SPCR_BAUD_19200        4
+#define ACPI_SPCR_BAUD_57600        5
+#define ACPI_SPCR_BAUD_115200       7
+
 /*******************************************************************************
  *
  * SPMI - Server Platform Management Interface table
-- 
2.51.0

On 25.03.2026 15:58, Roger Pau Monne wrote:
> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> to parse the ACPI SPCR table and obtain the serial configuration.
> 
> This is gated to the "acpi" device type being set in "com1" on the Xen
> command line.  Note that there can only be one serial device described in
> the SPCR, so limit it's usage to com1 exclusively for the time being.
> 
> I can't test the interrupt information parsing on my system, as the
> interrupt is set to GSI with a value of 0xff, which is outside of the range
> of GSIs available on the system.  I've also assumed that the interrupt
> being 0xff is used to signal not interrupt setup (just like the Interrupt
> Pin register on PCI headers).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> WIP/RFC, not sure whether there's interest in attempting to pursue this
> further on x86.  So far the device I have is also exposed on the PCI bus
> aside from SPCR, so using com1=device=amt also works to detect it.
> 
> Posting it kind of early to know whether I should try to polish it for
> submission or we are happy with not having this on x86.

One concern of mine is the altering ACPI CA code. Otoh, seeing how early
you need this for SPCR, I wonder if it then couldn't also be used for the
BGRT work that's being done in parallel.

> @@ -523,3 +540,67 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
>  
>  	return_ACPI_STATUS(AE_OK);
>  }
> +
> +acpi_status __init
> +acpi_early_get_table(const char *signature, acpi_native_uint instance,
> +                     struct acpi_table_header **out_table)
> +{
> +	static acpi_physical_address __initdata table_addr[128];
> +	static unsigned int __initdata table_count;
> +	static unsigned int __initdata table_entry_size;
> +	unsigned int i;
> +
> +	ACPI_FUNCTION_TRACE(tb_early_get_table);
> +
> +	if (!table_count) {
> +		struct acpi_table_header *table;
> +		void *table_entry;
> +		acpi_status status;
> +		acpi_physical_address rsdp_address = acpi_os_get_root_pointer();
> +
> +		if (!rsdp_address)
> +			return_ACPI_STATUS(AE_NOT_FOUND);
> +
> +		status = acpi_tb_get_root_table(rsdp_address, &table,
> +			                        &table_entry_size);
> +		if (!ACPI_SUCCESS(status))
> +			return_ACPI_STATUS(status);
> +
> +		/* Calculate the number of tables described in the root table */
> +		table_count = (table->length - sizeof(*table)) / table_entry_size;
> +
> +		if (table_count > ARRAY_SIZE(table_addr)) {
> +			table_count = 0;
> +			return_ACPI_STATUS(AE_NO_MEMORY);
> +		}

Rather than failing, limit table_count to ARRAY_SIZE()?

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1375,6 +1375,136 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
>  
>  #endif /* CONFIG_HAS_PCI */
>  
> +#ifdef CONFIG_ACPI
> +#include <acpi/acpi.h>

With xen/acpi.h included (below) this shouldn't be needed.

> +#include <acpi/actables.h>
> +
> +#include <xen/acpi.h>
> +
> +static int __init acpi_uart_config(struct ns16550 *uart, unsigned int idx)
> +{
> +    struct acpi_table_header *table;

While this can't be pointer-to-const, ...

> +    struct acpi_table_spcr *spcr;

... it looks like this can be.

> +    acpi_status status;
> +    int rc = 0;
> +
> +    /*
> +     * SPCR specifies a single port, expect it to be configured at position 0
> +     * in the uart array.
> +     */
> +    if ( idx )
> +        return -EXDEV;

While this matches what ns16550_acpi_uart_init() does / wants, I'm not sure
this is a good idea. If a system had a normal COM1 and something in SPCR,
one would need to re-define COM2 with the COM1 settings.

> +    if ( system_state <= SYS_STATE_early_boot )
> +        status = acpi_early_get_table(ACPI_SIG_SPCR, 0, &table);
> +    else
> +        status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
> +
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk(XENLOG_ERR "Failed to find or parse ACPI SPCR table\n");
> +        return -ENODEV;
> +    }
> +
> +    spcr = container_of(table, struct acpi_table_spcr, header);
> +
> +    rc = -EDOM;
> +    if ( spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE )
> +    {
> +        printk(XENLOG_ERR "Incompatible ACPI SPCR UART interface %u\n",
> +               spcr->interface_type);
> +        goto out;
> +    }
> +
> +    if ( spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +         (IS_ENABLED(CONFIG_ARM) ||

Better !IS_ENABLED(CONFIG_X86), seeing how neither RISC-V nor PPC have
I/O ports?

> +          spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_IO) )
> +    {
> +        printk(XENLOG_ERR "Incompatible ACPI SPCR UART address space %u\n",
> +               spcr->serial_port.space_id);
> +        goto out;
> +    }
> +
> +    if ( !spcr->serial_port.address )
> +    {
> +        printk(XENLOG_ERR "ACPI SPCR console redirection disabled\n");
> +        goto out;
> +    }
> +
> +    uart->io_base = spcr->serial_port.address;

Elsewhere we assume MMIO if the address is 0x10000 or above. Here we have
an ACPI_ADR_SPACE_* indicator, which I think we should take into account.
For now merely to reject values not fitting assumptions elsewhere, I guess.

> +    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
> +    uart->reg_shift = spcr->serial_port.bit_offset;
> +
> +    uart->parity = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->data_bits = 8;
> +
> +    if ( uart->baud == BAUD_AUTO && spcr->baud_rate )
> +    {
> +        switch ( spcr->baud_rate )
> +        {
> +        case ACPI_SPCR_BAUD_9600:
> +            uart->baud = 9600;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_19200:
> +            uart->baud = 19200;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_57600:
> +            uart->baud = 57600;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_115200:
> +            uart->baud = 115200;
> +            break;
> +
> +        default:
> +            printk(XENLOG_WARNING
> +                   "Ignoring invalid baud rate %u in ACPI SPCR\n",
> +                   spcr->baud_rate);

Maybe better s/invalid/unknown/?

Also, please add "break" for Misra's sake.

> +        }
> +    }
> +
> +    if ( IS_ENABLED(CONFIG_X86) )
> +    {
> +        /* Use polling mode by default. */
> +        uart->irq = 0;
> +
> +        if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_IO_APIC) &&
> +             spcr->interrupt < 0xff )
> +            uart->irq = spcr->interrupt;
> +        else if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_PC_AT) &&
> +                  ((spcr->pc_interrupt >=  2 && spcr->pc_interrupt <=  7) ||

Is 2 valid to use? That's the cascade in 8259-s.

> +                   (spcr->pc_interrupt >= 14 && spcr->pc_interrupt <= 15)) )
> +            uart->irq = spcr->pc_interrupt;
> +    }
> +
> +#ifdef CONFIG_ARM
> +    /* The trigger/polarity information is not available in spcr. */
> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    uart->irq = spcr->interrupt;
> +#endif /* CONFIG_ARM */
> +
> +#ifdef CONFIG_HAS_PCI
> +    if ( spcr->pci_device_id != 0xffff && spcr->pci_vendor_id != 0xffff )
> +    {
> +        uart->ps_bdf_enable = true;
> +        uart->pci_device = PCI_SBDF(spcr->pci_segment, spcr->pci_bus,
> +                                    spcr->pci_device, spcr->pci_function);
> +    }
> +#endif /* CONFIG_HAS_PCI */
> +
> +    rc = 0;
> +
> + out:
> +    if ( system_state <= SYS_STATE_early_boot )
> +        acpi_os_unmap_memory(&spcr, spcr->header.length);

I think you'd better unmap "table" here, and I don't think the & is correct
to use.

> @@ -1643,8 +1782,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>              uart->reg_width = simple_strtoul(param_value, NULL, 0);
>              break;
>  
> -#ifdef CONFIG_HAS_PCI
>          case device:
> +#ifdef CONFIG_ACPI
> +            if ( strncmp(param_value, "acpi", 3) == 0 )
> +            {
> +                acpi_uart_config(uart, uart - ns16550_com);
> +                dev_set = true;
> +                break;
> +            }
> +            else

May I ask to drop either the "else" or the "break"? To match PCI code,
it would be the latter.

> +#endif /* CONFIG_ACPI */
> +#ifdef CONFIG_HAS_PCI
>              if ( strncmp(param_value, "pci", 3) == 0 )
>              {
>                  pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> @@ -1656,9 +1804,11 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>                  dev_set = true;
>              }
>              else
> +#endif /* CONFIG_HAS_PCI */
>                  PARSE_ERR_RET("Unknown device type %s\n", param_value);
>              break;

I think for !ACPI && !HAS_PCI we'd better not alter behavior (i.e. that
case would still better end up at default:).

> --- a/xen/include/acpi/actbl2.h
> +++ b/xen/include/acpi/actbl2.h
> @@ -1037,6 +1037,16 @@ struct acpi_table_spcr {
>  
>  #define ACPI_SPCR_DO_NOT_DISABLE    (1)
>  
> +/* Masks for interrupt_type field above */
> +#define ACPI_SPCR_INTR_TYPE_PC_AT   0x01
> +#define ACPI_SPCR_INTR_TYPE_IO_APIC 0x02
> +
> +/* Values for the baud_rate field above */
> +#define ACPI_SPCR_BAUD_9600         3
> +#define ACPI_SPCR_BAUD_19200        4
> +#define ACPI_SPCR_BAUD_57600        5
> +#define ACPI_SPCR_BAUD_115200       7

Not your fault, but I wonder why we have SPCR here when Linux has it in
actbl3.h.

Jan

On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> to parse the ACPI SPCR table and obtain the serial configuration.
>
> This is gated to the "acpi" device type being set in "com1" on the Xen
> command line.  Note that there can only be one serial device described in
> the SPCR, so limit it's usage to com1 exclusively for the time being.
>
> I can't test the interrupt information parsing on my system, as the
> interrupt is set to GSI with a value of 0xff, which is outside of the range
> of GSIs available on the system.  I've also assumed that the interrupt
> being 0xff is used to signal not interrupt setup (just like the Interrupt
> Pin register on PCI headers).
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> WIP/RFC, not sure whether there's interest in attempting to pursue this
> further on x86.  So far the device I have is also exposed on the PCI bus
> aside from SPCR, so using com1=device=amt also works to detect it.
>
> Posting it kind of early to know whether I should try to polish it for
> submission or we are happy with not having this on x86.

I think we should be using SPCR/DBG2 when available.  Getting serial
configuration right is always tricky, and we might as well use the help
that Microsoft have forced the OEM/firmware world to provide.

But, I think it should be automatic when the user asked for any kind of
serial.  e.g. console=com1 with no com1 configuration.  The point of
these tables is to provide an enumeration mechanism where none
previously existed.

~Andrew

On 26.03.2026 13:11, Andrew Cooper wrote:
> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
>> to parse the ACPI SPCR table and obtain the serial configuration.
>>
>> This is gated to the "acpi" device type being set in "com1" on the Xen
>> command line.  Note that there can only be one serial device described in
>> the SPCR, so limit it's usage to com1 exclusively for the time being.
>>
>> I can't test the interrupt information parsing on my system, as the
>> interrupt is set to GSI with a value of 0xff, which is outside of the range
>> of GSIs available on the system.  I've also assumed that the interrupt
>> being 0xff is used to signal not interrupt setup (just like the Interrupt
>> Pin register on PCI headers).
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> WIP/RFC, not sure whether there's interest in attempting to pursue this
>> further on x86.  So far the device I have is also exposed on the PCI bus
>> aside from SPCR, so using com1=device=amt also works to detect it.
>>
>> Posting it kind of early to know whether I should try to polish it for
>> submission or we are happy with not having this on x86.
> 
> I think we should be using SPCR/DBG2 when available.  Getting serial
> configuration right is always tricky, and we might as well use the help
> that Microsoft have forced the OEM/firmware world to provide.
> 
> But, I think it should be automatic when the user asked for any kind of
> serial.  e.g. console=com1 with no com1 configuration.  The point of
> these tables is to provide an enumeration mechanism where none
> previously existed.

Hmm. In the PC world COM<n> have well-known configurations unless anything
else is provided. With multiple serial ports in a system, which one SPCR
describes also would be (largely) unknown.

Jan

On 26/03/2026 12:48 pm, Jan Beulich wrote:
> On 26.03.2026 13:11, Andrew Cooper wrote:
>> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
>>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
>>> to parse the ACPI SPCR table and obtain the serial configuration.
>>>
>>> This is gated to the "acpi" device type being set in "com1" on the Xen
>>> command line.  Note that there can only be one serial device described in
>>> the SPCR, so limit it's usage to com1 exclusively for the time being.
>>>
>>> I can't test the interrupt information parsing on my system, as the
>>> interrupt is set to GSI with a value of 0xff, which is outside of the range
>>> of GSIs available on the system.  I've also assumed that the interrupt
>>> being 0xff is used to signal not interrupt setup (just like the Interrupt
>>> Pin register on PCI headers).
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> WIP/RFC, not sure whether there's interest in attempting to pursue this
>>> further on x86.  So far the device I have is also exposed on the PCI bus
>>> aside from SPCR, so using com1=device=amt also works to detect it.
>>>
>>> Posting it kind of early to know whether I should try to polish it for
>>> submission or we are happy with not having this on x86.
>> I think we should be using SPCR/DBG2 when available.  Getting serial
>> configuration right is always tricky, and we might as well use the help
>> that Microsoft have forced the OEM/firmware world to provide.
>>
>> But, I think it should be automatic when the user asked for any kind of
>> serial.  e.g. console=com1 with no com1 configuration.  The point of
>> these tables is to provide an enumeration mechanism where none
>> previously existed.
> Hmm. In the PC world COM<n> have well-known configurations unless anything
> else is provided. With multiple serial ports in a system, which one SPCR
> describes also would be (largely) unknown.

Xen's COM1/2 already do do far more than the PC world.  But ok then, we
invent a new "serial".

My point is, there should be a way to say "please use serial as
described by the system", and it shouldn't even require knowing that the
description is in APCI.

~Andrew

On Thu, Mar 26, 2026 at 12:52:51PM +0000, Andrew Cooper wrote:
> On 26/03/2026 12:48 pm, Jan Beulich wrote:
> > On 26.03.2026 13:11, Andrew Cooper wrote:
> >> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> >>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> >>> to parse the ACPI SPCR table and obtain the serial configuration.
> >>>
> >>> This is gated to the "acpi" device type being set in "com1" on the Xen
> >>> command line.  Note that there can only be one serial device described in
> >>> the SPCR, so limit it's usage to com1 exclusively for the time being.
> >>>
> >>> I can't test the interrupt information parsing on my system, as the
> >>> interrupt is set to GSI with a value of 0xff, which is outside of the range
> >>> of GSIs available on the system.  I've also assumed that the interrupt
> >>> being 0xff is used to signal not interrupt setup (just like the Interrupt
> >>> Pin register on PCI headers).
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> WIP/RFC, not sure whether there's interest in attempting to pursue this
> >>> further on x86.  So far the device I have is also exposed on the PCI bus
> >>> aside from SPCR, so using com1=device=amt also works to detect it.
> >>>
> >>> Posting it kind of early to know whether I should try to polish it for
> >>> submission or we are happy with not having this on x86.
> >> I think we should be using SPCR/DBG2 when available.  Getting serial
> >> configuration right is always tricky, and we might as well use the help
> >> that Microsoft have forced the OEM/firmware world to provide.
> >>
> >> But, I think it should be automatic when the user asked for any kind of
> >> serial.  e.g. console=com1 with no com1 configuration.  The point of
> >> these tables is to provide an enumeration mechanism where none
> >> previously existed.
> > Hmm. In the PC world COM<n> have well-known configurations unless anything
> > else is provided. With multiple serial ports in a system, which one SPCR
> > describes also would be (largely) unknown.
> 
> Xen's COM1/2 already do do far more than the PC world.  But ok then, we
> invent a new "serial".
> 
> My point is, there should be a way to say "please use serial as
> described by the system", and it shouldn't even require knowing that the
> description is in APCI.

That's how it kind of works on FreeBSD, the user asks for "serial"
output and the first thing that's checked is SPCR (if the system
support ACPI).

We could have something like `console=serial` and let Xen figure it
out, but it would probably need more logic in case SPCR is not
present, and I'm unsure how the extra heuristics should look like.

Thanks, Roger.