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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2026 09:14, Roger Pau Monné wrote:
> 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?

Maybe.

>  I find it confusing that there are two
> different fields (->ps_bdf_enable and ->bar) that signal whether a PCI
> device is in-use.

I found the field itself confusing, not the least because of its name
and its sibling pb_bdf_enable.

Jan