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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan

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