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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

Jan