Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>
> ---
> 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