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 05:00:15PM +0100, Jan Beulich wrote:
> On 26.03.2026 16:13, Roger Pau Monné wrote:
> > 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?
> 
> RMRRs in BARs seems unlikely (as BARs can be moved), but you have a
> point in general. Otoh devices are fully under our (later under Dom0's)
> control, so we may clear (or set) bits as we see fit to get a device
> to function. FTAOD, I'm not outright objecting to the tag, I'm merely
> questioning it some.
> 
> >>> --- 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?
> 
> I think this was deliberate, hence why ...
> 
> >>>      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 )
> 
> ... this conditional is now in use.

Right, but then the logic in pci_serial_early_init() doesn't apply to
those devices (device=amt|pci) when the BARs are in IO space?

As uart->ps_bdf_enable == false, and uart->io_base < 0x10000, it will
return early from the function without attempting to enable the IO
BAR.  Is this really expected?  It looks like Xen should always make
sure the respective BARs are enabled if the device is to be used for
serial output?

Thanks, Roger.