Re: [PATCH 1/4] xen/uart: be more careful with changes to the PCI command register

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.03.2026 15:58, Roger Pau Monne wrote:
> Read the existing PCI command register and only add the required bits to
> it, as to avoid clearing bits that might be possibly set by the firmware
> already.
> 
> This fixes serial output when booting with `com1=device=amt` on a system
> using an "Alder Lake AMT SOL Redirection" PCI device (Vendor ID 0x8086 and
> Device ID 0x51e3).  That device has both IO and memory decoding enabled by
> the firmware, and disabling memory decoding causes the serial to stop
> working (even when the serial register BAR is in the IO space).
> 
> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I'm not convinced Fixes: is appropriate here. There's nothing wrong with that
commit, aiui. What's bogus is the device behavior.

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -283,11 +283,17 @@ static int cf_check ns16550_getc(struct serial_port 
> *port, char *pc)
>  static void pci_serial_early_init(struct ns16550 *uart)
>  {
>  #ifdef NS16550_PCI
> +    uint16_t cmd = 0;
> +
> +    if ( uart->ps_bdf_enable )
> +        cmd = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
> +                                       uart->ps_bdf[2]), PCI_COMMAND);

Why is this conditional? While fine for the use at the bottom, ...

>      if ( uart->bar && uart->io_base >= 0x10000 )
>      {
>          pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                    uart->ps_bdf[2]),
> -                         PCI_COMMAND, PCI_COMMAND_MEMORY);
> +                         PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
>          return;
>      }

... it looks wrong(ish) for this path. Actually, in ns16550_init_postirq()
we use

    if ( uart->bar || uart->ps_bdf_enable )

for example. With the new conditional updated accordingly:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

> @@ -307,7 +313,7 @@ static void pci_serial_early_init(struct ns16550 *uart)
>                       uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
>      pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                uart->ps_bdf[2]),
> -                     PCI_COMMAND, PCI_COMMAND_IO);
> +                     PCI_COMMAND, cmd | PCI_COMMAND_IO);
>  #endif
>  }
>