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@citrix.com>
---
 xen/drivers/char/ns16550.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index df7fff7f81df..41d6380367ca 100644
--- 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);
+
     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;
     }
 
@@ -307,7 +313,7 @@ static void pci_serial_early_init(struct ns16550 *uart)
                      uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
     pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
                               uart->ps_bdf[2]),
-                     PCI_COMMAND, PCI_COMMAND_IO);
+                     PCI_COMMAND, cmd | PCI_COMMAND_IO);
 #endif
 }
 
-- 
2.51.0

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@citrix.com>

I'm not convinced Fixes: is appropriate here. There's nothing wrong with that
commit, aiui. What's bogus is the device behavior.

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

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

for example. With the new conditional updated accordingly:
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

> @@ -307,7 +313,7 @@ static void pci_serial_early_init(struct ns16550 *uart)
>                       uart->io_base | PCI_BASE_ADDRESS_SPACE_IO);
>      pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>                                uart->ps_bdf[2]),
> -                     PCI_COMMAND, PCI_COMMAND_IO);
> +                     PCI_COMMAND, cmd | PCI_COMMAND_IO);
>  #endif
>  }
>  

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@citrix.com>
> 
> 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?

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

> >      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 )
> 
> for example. With the new conditional updated accordingly:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks for the review, I don't mind adjusting, but I have a further
question above.

Roger.

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@citrix.com>
>>
>> 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@suse.com>
> 
> Thanks for the review, I don't mind adjusting, but I have a further
> question above.
> 
> Roger.

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@citrix.com>
> >>
> >> 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.

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@citrix.com>
>>>>
>>>> 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

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@citrix.com>
> >>>>
> >>>> 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.

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@citrix.com>
>>>>>>
>>>>>> 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

On 25/03/2026 2:58 pm, 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@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>