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

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

On Fri, Mar 27, 2026 at 08:59:29AM +0100, Jan Beulich wrote:
> On 26.03.2026 18:02, Roger Pau Monné wrote:
> > 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?
> 
> I agree. Many of the changes were hacked in just to make someone's
> device work, without having general aspects in mind. I expect most if
> not all checks of ->ps_bdf_enable want amending by adding ->bar ones.

Wouldn't it be easier to unconditionally set ->ps_bdf_enable when a
PCI device is being used?  I find it confusing that there are two
different fields (->ps_bdf_enable and ->bar) that signal whether a PCI
device is in-use.

Thanks, Roger.