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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Mar 26, 2026 at 01:02:22PM +0100, Jan Beulich wrote:
> 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.

Hm, I would argue that disabling command register bits for devices
that have those enabled is in general dangerous.  What about device
RMRR or similar residing in BARs, and Xen disabling memory decoding
unintentionally while attempting to enable IO decoding?

> > --- 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, ...

The comment next to the field states:

    bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */

So it didn't seem like further checking was needed and that was the
sole filed to decide whether ps_bdf is populated or not.

However, I also found that when using device=amt|pci ps_bdf_enable
doesn't get set, and hence I'm not sure if that's intended or not.
Shouldn't ps_bdf_enable get set unconditionally when the serial device
is a PCI one?

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

Thanks for the review, I don't mind adjusting, but I have a further
question above.

Roger.