[PATCH 0/4] xen/uart: some fixes and improvements

[PATCH 0/4] xen/uart: some fixes and improvements

[Roger Pau Monne]

Hello,

First patch is a fix that otherwise prevents using the serial on some
(newish?) Intel AMT devices (at least the one I have).  Patches 2 and 3
are general improvements and cleanups.  Finally patch 4 adds support for
getting the serial configuration from ACPI SPCR table.

Thanks, Roger.

Roger Pau Monne (4):
  xen/uart: be more careful with changes to the PCI command register
  xen/uart: switch ns16550 to use pci_sbdf_t
  xen/uart: report an error if the device type is not supported
  xen/uart: enable parsing ACPI SPCR on x86

 docs/misc/xen-command-line.pandoc |   8 +-
 xen/drivers/acpi/tables/tbutils.c | 131 +++++++++---
 xen/drivers/char/ns16550.c        | 339 +++++++++++++++++++-----------
 xen/include/acpi/actables.h       |   5 +
 xen/include/acpi/actbl2.h         |  10 +
 5 files changed, 346 insertions(+), 147 deletions(-)

-- 
2.51.0

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

[Roger Pau Monne]

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

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

[Andrew Cooper]

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>

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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.

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

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

[Roger Pau Monné]

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.

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

[Jan Beulich]

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

[PATCH 2/4] xen/uart: switch ns16550 to use pci_sbdf_t

[Roger Pau Monne]

No functional change intended.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/char/ns16550.c | 133 ++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 75 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 41d6380367ca..da3b6fdf99d9 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -67,8 +67,8 @@ static struct ns16550 {
     /* PCI card parameters. */
     bool pb_bdf_enable;     /* if =1, pb-bdf effective, port behind bridge */
     bool ps_bdf_enable;     /* if =1, ps_bdf effective, port on pci card */
-    unsigned int pb_bdf[3]; /* pci bridge BDF */
-    unsigned int ps_bdf[3]; /* pci serial port BDF */
+    pci_sbdf_t pci_bridge;
+    pci_sbdf_t pci_device;
     u32 bar;
     u32 bar64;
     u16 cr;
@@ -286,14 +286,11 @@ static void pci_serial_early_init(struct ns16550 *uart)
     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);
+        cmd = pci_conf_read16(uart->pci_device, 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, cmd | PCI_COMMAND_MEMORY);
+        pci_conf_write16(uart->pci_device, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
         return;
     }
 
@@ -301,19 +298,13 @@ static void pci_serial_early_init(struct ns16550 *uart)
         return;
 
     if ( uart->pb_bdf_enable )
-        pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
-                                  uart->pb_bdf[2]),
-                         PCI_IO_BASE,
+        pci_conf_write16(uart->pci_bridge, PCI_IO_BASE,
                          (uart->io_base & 0xF000) |
                          ((uart->io_base & 0xF000) >> 8));
 
-    pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
-                              uart->ps_bdf[2]),
-                     PCI_BASE_ADDRESS_0,
+    pci_conf_write32(uart->pci_device, PCI_BASE_ADDRESS_0,
                      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, cmd | PCI_COMMAND_IO);
+    pci_conf_write16(uart->pci_device, PCI_COMMAND, cmd | PCI_COMMAND_IO);
 #endif
 }
 
@@ -453,17 +444,16 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
                                 PFN_UP(uart->io_base + uart->io_size) - 1) )
             printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
 
-        if ( pci_ro_device(0, uart->ps_bdf[0],
-                           PCI_DEVFN(uart->ps_bdf[1], uart->ps_bdf[2])) )
-            printk(XENLOG_INFO "Could not mark config space of %02x:%02x.%u read-only.\n",
-                   uart->ps_bdf[0], uart->ps_bdf[1],
-                   uart->ps_bdf[2]);
+        if ( pci_ro_device(uart->pci_device.seg, uart->pci_device.bus,
+                           uart->pci_device.devfn) )
+            printk(XENLOG_INFO
+                   "Could not mark config space of %pp read-only.\n",
+                   &uart->pci_device);
 
         if ( uart->msi )
         {
             struct msi_info msi = {
-                .sbdf = PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
-                                 uart->ps_bdf[2]),
+                .sbdf = uart->pci_device,
                 .irq = uart->irq,
                 .entry_nr = 1
             };
@@ -505,9 +495,8 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
             }
 
             if ( rc )
-                printk(XENLOG_WARNING
-                       "MSI setup failed (%d) for %02x:%02x.%o\n",
-                       rc, uart->ps_bdf[0], uart->ps_bdf[1], uart->ps_bdf[2]);
+                printk(XENLOG_WARNING "MSI setup failed (%d) for %pp\n",
+                       rc, &uart->pci_device);
         }
     }
 #endif
@@ -534,8 +523,7 @@ static void cf_check ns16550_suspend(struct serial_port *port)
 
 #ifdef NS16550_PCI
     if ( uart->bar )
-       uart->cr = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
-                                  uart->ps_bdf[2]), PCI_COMMAND);
+       uart->cr = pci_conf_read16(uart->pci_device, PCI_COMMAND);
 #endif
 }
 
@@ -546,19 +534,15 @@ static void _ns16550_resume(struct serial_port *port)
 
     if ( uart->bar )
     {
-       pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
-                                 uart->ps_bdf[2]),
+       pci_conf_write32(uart->pci_device,
                         PCI_BASE_ADDRESS_0 + uart->bar_idx*4, uart->bar);
 
         /* If 64 bit BAR, write higher 32 bits to BAR+4 */
         if ( uart->bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
-            pci_conf_write32(PCI_SBDF(0, uart->ps_bdf[0],  uart->ps_bdf[1],
-                                      uart->ps_bdf[2]),
+            pci_conf_write32(uart->pci_device,
                         PCI_BASE_ADDRESS_0 + (uart->bar_idx+1)*4, uart->bar64);
 
-       pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
-                                 uart->ps_bdf[2]),
-                        PCI_COMMAND, uart->cr);
+       pci_conf_write16(uart->pci_device, PCI_COMMAND, uart->cr);
     }
 #endif
 
@@ -1218,13 +1202,12 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                 uint32_t bar, bar_64 = 0, len, len_64;
                 u64 size = 0;
                 const struct ns16550_config_param *param = uart_param;
+                pci_sbdf_t sbdf = PCI_SBDF(0, b, d, f);
 
-                nextf = (f || (pci_conf_read16(PCI_SBDF(0, b, d, f),
-                                               PCI_HEADER_TYPE) &
+                nextf = (f || (pci_conf_read16(sbdf, PCI_HEADER_TYPE) &
                                0x80)) ? f + 1 : 8;
 
-                switch ( pci_conf_read16(PCI_SBDF(0, b, d, f),
-                                         PCI_CLASS_DEVICE) )
+                switch ( pci_conf_read16(sbdf, PCI_CLASS_DEVICE) )
                 {
                 case 0x0700: /* single port serial */
                 case 0x0702: /* multi port serial */
@@ -1241,10 +1224,8 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                 /* Check for params in uart_config lookup table */
                 for ( i = 0; i < ARRAY_SIZE(uart_config); i++ )
                 {
-                    u16 vendor = pci_conf_read16(PCI_SBDF(0, b, d, f),
-                                                 PCI_VENDOR_ID);
-                    u16 device = pci_conf_read16(PCI_SBDF(0, b, d, f),
-                                                 PCI_DEVICE_ID);
+                    u16 vendor = pci_conf_read16(sbdf, PCI_VENDOR_ID);
+                    u16 device = pci_conf_read16(sbdf, PCI_DEVICE_ID);
 
                     if ( uart_config[i].vendor_id == vendor &&
                          uart_config[i].dev_id == device )
@@ -1267,29 +1248,26 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                 }
 
                 uart->io_base = 0;
-                bar = pci_conf_read32(PCI_SBDF(0, b, d, f),
-                                      PCI_BASE_ADDRESS_0 + bar_idx * 4);
+                bar = pci_conf_read32(sbdf, PCI_BASE_ADDRESS_0 + bar_idx * 4);
 
                 /* MMIO based */
                 if ( param->mmio && !(bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    pci_conf_write32(PCI_SBDF(0, b, d, f),
-                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
-                    len = pci_conf_read32(PCI_SBDF(0, b, d, f),
+                    pci_conf_write32(sbdf, PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(sbdf,
                                           PCI_BASE_ADDRESS_0 + bar_idx * 4);
-                    pci_conf_write32(PCI_SBDF(0, b, d, f),
-                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+                    pci_conf_write32(sbdf, PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
 
                     /* Handle 64 bit BAR if found */
                     if ( bar & PCI_BASE_ADDRESS_MEM_TYPE_64 )
                     {
-                        bar_64 = pci_conf_read32(PCI_SBDF(0, b, d, f),
+                        bar_64 = pci_conf_read32(sbdf,
                                       PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4);
-                        pci_conf_write32(PCI_SBDF(0, b, d, f),
+                        pci_conf_write32(sbdf,
                                     PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, ~0u);
-                        len_64 = pci_conf_read32(PCI_SBDF(0, b, d, f),
+                        len_64 = pci_conf_read32(sbdf,
                                     PCI_BASE_ADDRESS_0 + (bar_idx + 1) * 4);
-                        pci_conf_write32(PCI_SBDF(0, b, d, f),
+                        pci_conf_write32(sbdf,
                                     PCI_BASE_ADDRESS_0 + (bar_idx+1)*4, bar_64);
                         size  = ((u64)~0 << 32) | PCI_BASE_ADDRESS_MEM_MASK;
                         size &= ((u64)len_64 << 32) | len;
@@ -1303,12 +1281,9 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                 /* IO based */
                 else if ( !param->mmio && (bar & PCI_BASE_ADDRESS_SPACE_IO) )
                 {
-                    pci_conf_write32(PCI_SBDF(0, b, d, f),
-                                     PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
-                    len = pci_conf_read32(PCI_SBDF(0, b, d, f),
-                                          PCI_BASE_ADDRESS_0);
-                    pci_conf_write32(PCI_SBDF(0, b, d, f),
-                                     PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
+                    pci_conf_write32(sbdf, PCI_BASE_ADDRESS_0 + bar_idx*4, ~0u);
+                    len = pci_conf_read32(sbdf, PCI_BASE_ADDRESS_0);
+                    pci_conf_write32(sbdf, PCI_BASE_ADDRESS_0 + bar_idx*4, bar);
                     size = len & PCI_BASE_ADDRESS_IO_MASK;
 
                     uart->io_base = bar & ~PCI_BASE_ADDRESS_SPACE_IO;
@@ -1341,18 +1316,14 @@ pci_uart_config(struct ns16550 *uart, bool skip_amt, unsigned int idx)
                 if ( param->fifo_size )
                     uart->fifo_size = param->fifo_size;
 
-                uart->ps_bdf[0] = b;
-                uart->ps_bdf[1] = d;
-                uart->ps_bdf[2] = f;
+                uart->pci_device = sbdf;
                 uart->bar_idx = bar_idx;
                 uart->bar = bar;
                 uart->bar64 = bar_64;
                 uart->io_size = max(8U << param->reg_shift,
                                     param->uart_offset);
-                uart->irq = pci_conf_read8(PCI_SBDF(0, b, d, f),
-                                           PCI_INTERRUPT_PIN) ?
-                            pci_conf_read8(PCI_SBDF(0, b, d, f),
-                                           PCI_INTERRUPT_LINE) : 0;
+                uart->irq = pci_conf_read8(sbdf, PCI_INTERRUPT_PIN) ?
+                            pci_conf_read8(sbdf, PCI_INTERRUPT_LINE) : 0;
 
 #ifdef CONFIG_X86
                 /*
@@ -1591,18 +1562,22 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
 #ifdef CONFIG_HAS_PCI
     if ( *conf == ',' && *++conf != ',' )
     {
-        conf = parse_pci(conf, NULL, &uart->ps_bdf[0],
-                         &uart->ps_bdf[1], &uart->ps_bdf[2]);
+        unsigned int b, d, f;
+
+        conf = parse_pci(conf, NULL, &b, &d, &f);
         if ( !conf )
             PARSE_ERR_RET("Bad port PCI coordinates");
+        uart->pci_device = PCI_SBDF(0, b, d, f);
         uart->ps_bdf_enable = true;
     }
 
     if ( *conf == ',' && *++conf != ',' )
     {
-        if ( !parse_pci(conf, NULL, &uart->pb_bdf[0],
-                        &uart->pb_bdf[1], &uart->pb_bdf[2]) )
+        unsigned int b, d, f;
+
+        if ( !parse_pci(conf, NULL, &b, &d, &f) )
             PARSE_ERR_RET("Bad bridge PCI coordinates");
+        uart->pci_bridge = PCI_SBDF(0, b, d, f);
         uart->pb_bdf_enable = true;
     }
 #endif
@@ -1683,18 +1658,26 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             break;
 
         case port_bdf:
-            if ( !parse_pci(param_value, NULL, &uart->ps_bdf[0],
-                            &uart->ps_bdf[1], &uart->ps_bdf[2]) )
+        {
+            unsigned int b, d, f;
+
+            if ( !parse_pci(param_value, NULL, &b, &d, &f) )
                 PARSE_ERR_RET("Bad port PCI coordinates\n");
+            uart->pci_device = PCI_SBDF(0, b, d, f);
             uart->ps_bdf_enable = true;
             break;
+        }
 
         case bridge_bdf:
-            if ( !parse_pci(param_value, NULL, &uart->pb_bdf[0],
-                            &uart->pb_bdf[1], &uart->pb_bdf[2]) )
+        {
+            unsigned int b, d, f;
+
+            if ( !parse_pci(param_value, NULL, &b, &d, &f) )
                 PARSE_ERR_RET("Bad bridge PCI coordinates\n");
+            uart->pci_bridge = PCI_SBDF(0, b, d, f);
             uart->pb_bdf_enable = true;
             break;
+        }
 #endif
 
         default:
-- 
2.51.0

Re: [PATCH 2/4] xen/uart: switch ns16550 to use pci_sbdf_t

[Andrew Cooper]

On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> No functional change intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

This looks to be a substantial improvement, even if there is clearly
more to go.

[PATCH 3/4] xen/uart: report an error if the device type is not supported

[Roger Pau Monne]

When using key pairs for the uart configuration (com1 and com2 command line
options), report an error if the passed device=<type> is not recognized
instead of silently ignoring it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/drivers/char/ns16550.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index da3b6fdf99d9..9cd3e471bfa5 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1655,6 +1655,8 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                 pci_uart_config(uart, 0, uart - ns16550_com);
                 dev_set = true;
             }
+            else
+                PARSE_ERR_RET("Unknown device type %s\n", param_value);
             break;
 
         case port_bdf:
-- 
2.51.0

Re: [PATCH 3/4] xen/uart: report an error if the device type is not supported

[Andrew Cooper]

On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> When using key pairs for the uart configuration (com1 and com2 command line
> options), report an error if the passed device=<type> is not recognized
> instead of silently ignoring it.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

[PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Roger Pau Monne]

Introduce extra logic to allow parsing ACPI tables extra early, and use it
to parse the ACPI SPCR table and obtain the serial configuration.

This is gated to the "acpi" device type being set in "com1" on the Xen
command line.  Note that there can only be one serial device described in
the SPCR, so limit it's usage to com1 exclusively for the time being.

I can't test the interrupt information parsing on my system, as the
interrupt is set to GSI with a value of 0xff, which is outside of the range
of GSIs available on the system.  I've also assumed that the interrupt
being 0xff is used to signal not interrupt setup (just like the Interrupt
Pin register on PCI headers).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
WIP/RFC, not sure whether there's interest in attempting to pursue this
further on x86.  So far the device I have is also exposed on the PCI bus
aside from SPCR, so using com1=device=amt also works to detect it.

Posting it kind of early to know whether I should try to polish it for
submission or we are happy with not having this on x86.
---
 docs/misc/xen-command-line.pandoc |   8 +-
 xen/drivers/acpi/tables/tbutils.c | 131 +++++++++++++++----
 xen/drivers/char/ns16550.c        | 202 +++++++++++++++++++++++-------
 xen/include/acpi/actables.h       |   5 +
 xen/include/acpi/actbl2.h         |  10 ++
 5 files changed, 282 insertions(+), 74 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index ebdca007d26b..12e1965cfb60 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -349,7 +349,7 @@ ACPI indicating none to be there.
 
 ### com1 (x86)
 ### com2 (x86)
-> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
+> `= <baud>[/<base-baud>][,[DPS][,[<io-base>|pci|amt|acpi][,[<irq>|msi][,[<port-bdf>][,[<bridge-bdf>]]]]]]`
 
 Both option `com1` and `com2` follow the same format.
 
@@ -379,6 +379,8 @@ Both option `com1` and `com2` follow the same format.
   avoiding Intel AMT devices.
 * `amt` indicated that Xen should scan the PCI bus for the UART,
   including Intel AMT devices if present.
+* `acpi` indicates Xen should use ACPI information (from SPCR table) if present
+  to configure the serial console.  This option is only supported for com1.
 
 A typical setup for most situations might be `com1=115200,8n1`
 
@@ -403,9 +405,9 @@ The accepted name keywords for name=value pairs are:
 * `clock-hz`- accepts large integers to setup UART clock frequencies.
               Do note - these values are multiplied by 16.
 * `data-bits` - integer between 5 and 8
-* `dev` - accepted values are `pci` OR `amt`. If this option
+* `dev` - accepted values are `pci`, `amt` or `acpi`. If this option
           is used to specify if the serial device is pci-based. The io_base
-          cannot be specified when `dev=pci` or `dev=amt` is used.
+          cannot be specified when `dev=pci|amt|acpi` is used.
 * `io-base` - accepts integer which specified IO base port for UART registers
 * `irq` - IRQ number to use
 * `parity` - accepted values are same as positional parameters
diff --git a/xen/drivers/acpi/tables/tbutils.c b/xen/drivers/acpi/tables/tbutils.c
index 458989abea99..c5eb6b1fc523 100644
--- a/xen/drivers/acpi/tables/tbutils.c
+++ b/xen/drivers/acpi/tables/tbutils.c
@@ -341,39 +341,20 @@ acpi_tb_get_root_table_entry(u8 * table_entry,
 	}
 }
 
-/*******************************************************************************
- *
- * FUNCTION:    acpi_tb_parse_root_table
- *
- * PARAMETERS:  Rsdp                    - Pointer to the RSDP
- *              Flags                   - Flags
- *
- * RETURN:      Status
- *
- * DESCRIPTION: This function is called to parse the Root System Description
- *              Table (RSDT or XSDT)
- *
- * NOTE:        Tables are mapped (not copied) for efficiency. The FACS must
- *              be mapped and cannot be copied because it contains the actual
- *              memory location of the ACPI Global Lock.
- *
- ******************************************************************************/
-
-acpi_status __init
-acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
+static acpi_status __init
+acpi_tb_get_root_table(acpi_physical_address rsdp_address,
+	               struct acpi_table_header **out_table,
+		       unsigned int *entry_size)
 {
 	struct acpi_table_rsdp *rsdp;
 	acpi_native_uint table_entry_size;
-	acpi_native_uint i;
-	u32 table_count;
 	struct acpi_table_header *table;
 	acpi_physical_address address;
 	acpi_physical_address rsdt_address = 0;
 	u32 length;
-	u8 *table_entry;
 	acpi_status status;
 
-	ACPI_FUNCTION_TRACE(tb_parse_root_table);
+	ACPI_FUNCTION_TRACE(tb_get_root_table);
 
 	/*
 	 * Map the entire RSDP and extract the address of the RSDT or XSDT
@@ -454,6 +435,42 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 		return_ACPI_STATUS(status);
 	}
 
+	*out_table = table;
+	*entry_size = table_entry_size;
+	return_ACPI_STATUS(AE_OK);
+}
+
+/*******************************************************************************
+ *
+ * FUNCTION:    acpi_tb_parse_root_table
+ *
+ * PARAMETERS:  Rsdp                    - Pointer to the RSDP
+ *              Flags                   - Flags
+ *
+ * RETURN:      Status
+ *
+ * DESCRIPTION: This function is called to parse the Root System Description
+ *              Table (RSDT or XSDT)
+ *
+ * NOTE:        Tables are mapped (not copied) for efficiency. The FACS must
+ *              be mapped and cannot be copied because it contains the actual
+ *              memory location of the ACPI Global Lock.
+ *
+ ******************************************************************************/
+
+acpi_status __init
+acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
+{
+	struct acpi_table_header *table;
+	unsigned int table_count, table_entry_size, i;
+	void *table_entry;
+	acpi_status status;
+
+	status = acpi_tb_get_root_table(rsdp_address, &table,
+	                                &table_entry_size);
+	if (!ACPI_SUCCESS(status))
+		  return_ACPI_STATUS(status);
+
 	/* Calculate the number of tables described in the root table */
 
 	table_count =
@@ -502,7 +519,7 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 	 * It is not possible to map more than one entry in some environments,
 	 * so unmap the root table here before mapping other tables
 	 */
-	acpi_os_unmap_memory(table, length);
+	acpi_os_unmap_memory(table, table->length);
 
 	/*
 	 * Complete the initialization of the root table array by examining
@@ -523,3 +540,67 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
 
 	return_ACPI_STATUS(AE_OK);
 }
+
+acpi_status __init
+acpi_early_get_table(const char *signature, acpi_native_uint instance,
+                     struct acpi_table_header **out_table)
+{
+	static acpi_physical_address __initdata table_addr[128];
+	static unsigned int __initdata table_count;
+	static unsigned int __initdata table_entry_size;
+	unsigned int i;
+
+	ACPI_FUNCTION_TRACE(tb_early_get_table);
+
+	if (!table_count) {
+		struct acpi_table_header *table;
+		void *table_entry;
+		acpi_status status;
+		acpi_physical_address rsdp_address = acpi_os_get_root_pointer();
+
+		if (!rsdp_address)
+			return_ACPI_STATUS(AE_NOT_FOUND);
+
+		status = acpi_tb_get_root_table(rsdp_address, &table,
+			                        &table_entry_size);
+		if (!ACPI_SUCCESS(status))
+			return_ACPI_STATUS(status);
+
+		/* Calculate the number of tables described in the root table */
+		table_count = (table->length - sizeof(*table)) / table_entry_size;
+
+		if (table_count > ARRAY_SIZE(table_addr)) {
+			table_count = 0;
+			return_ACPI_STATUS(AE_NO_MEMORY);
+		}
+
+		table_entry = ACPI_CAST_PTR(uint8_t, table) + sizeof(*table);
+
+		for (i = 0; i < table_count; i++) {
+			table_addr[i] =
+			      acpi_tb_get_root_table_entry(table_entry,
+			                                   table_entry_size);
+			table_entry += table_entry_size;
+		}
+
+		acpi_os_unmap_memory(table, table->length);
+	}
+
+	for (i = 0; i < table_count; i++) {
+		struct acpi_table_header *header =
+		      acpi_os_map_memory(table_addr[i], sizeof(*header));
+
+		if (ACPI_COMPARE_NAME(header->signature, signature)) {
+			unsigned int len = header->length;
+
+			acpi_os_unmap_memory(header, sizeof(*header));
+			*out_table = acpi_os_map_memory(table_addr[i], len);
+
+			return_ACPI_STATUS(AE_OK);
+		}
+
+		acpi_os_unmap_memory(header, sizeof(*header));
+	}
+
+	return_ACPI_STATUS(AE_NOT_FOUND);
+}
diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 9cd3e471bfa5..606dd404fd76 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -1375,6 +1375,136 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
 
 #endif /* CONFIG_HAS_PCI */
 
+#ifdef CONFIG_ACPI
+#include <acpi/acpi.h>
+#include <acpi/actables.h>
+
+#include <xen/acpi.h>
+
+static int __init acpi_uart_config(struct ns16550 *uart, unsigned int idx)
+{
+    struct acpi_table_header *table;
+    struct acpi_table_spcr *spcr;
+    acpi_status status;
+    int rc = 0;
+
+    /*
+     * SPCR specifies a single port, expect it to be configured at position 0
+     * in the uart array.
+     */
+    if ( idx )
+        return -EXDEV;
+
+    if ( system_state <= SYS_STATE_early_boot )
+        status = acpi_early_get_table(ACPI_SIG_SPCR, 0, &table);
+    else
+        status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
+
+    if ( ACPI_FAILURE(status) )
+    {
+        printk(XENLOG_ERR "Failed to find or parse ACPI SPCR table\n");
+        return -ENODEV;
+    }
+
+    spcr = container_of(table, struct acpi_table_spcr, header);
+
+    rc = -EDOM;
+    if ( spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE )
+    {
+        printk(XENLOG_ERR "Incompatible ACPI SPCR UART interface %u\n",
+               spcr->interface_type);
+        goto out;
+    }
+
+    if ( spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+         (IS_ENABLED(CONFIG_ARM) ||
+          spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_IO) )
+    {
+        printk(XENLOG_ERR "Incompatible ACPI SPCR UART address space %u\n",
+               spcr->serial_port.space_id);
+        goto out;
+    }
+
+    if ( !spcr->serial_port.address )
+    {
+        printk(XENLOG_ERR "ACPI SPCR console redirection disabled\n");
+        goto out;
+    }
+
+    uart->io_base = spcr->serial_port.address;
+    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
+    uart->reg_shift = spcr->serial_port.bit_offset;
+
+    uart->parity = spcr->parity;
+    uart->stop_bits = spcr->stop_bits;
+    uart->data_bits = 8;
+
+    if ( uart->baud == BAUD_AUTO && spcr->baud_rate )
+    {
+        switch ( spcr->baud_rate )
+        {
+        case ACPI_SPCR_BAUD_9600:
+            uart->baud = 9600;
+            break;
+
+        case ACPI_SPCR_BAUD_19200:
+            uart->baud = 19200;
+            break;
+
+        case ACPI_SPCR_BAUD_57600:
+            uart->baud = 57600;
+            break;
+
+        case ACPI_SPCR_BAUD_115200:
+            uart->baud = 115200;
+            break;
+
+        default:
+            printk(XENLOG_WARNING
+                   "Ignoring invalid baud rate %u in ACPI SPCR\n",
+                   spcr->baud_rate);
+        }
+    }
+
+    if ( IS_ENABLED(CONFIG_X86) )
+    {
+        /* Use polling mode by default. */
+        uart->irq = 0;
+
+        if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_IO_APIC) &&
+             spcr->interrupt < 0xff )
+            uart->irq = spcr->interrupt;
+        else if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_PC_AT) &&
+                  ((spcr->pc_interrupt >=  2 && spcr->pc_interrupt <=  7) ||
+                   (spcr->pc_interrupt >=  9 && spcr->pc_interrupt <= 12) ||
+                   (spcr->pc_interrupt >= 14 && spcr->pc_interrupt <= 15)) )
+            uart->irq = spcr->pc_interrupt;
+    }
+
+#ifdef CONFIG_ARM
+    /* The trigger/polarity information is not available in spcr. */
+    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
+    uart->irq = spcr->interrupt;
+#endif /* CONFIG_ARM */
+
+#ifdef CONFIG_HAS_PCI
+    if ( spcr->pci_device_id != 0xffff && spcr->pci_vendor_id != 0xffff )
+    {
+        uart->ps_bdf_enable = true;
+        uart->pci_device = PCI_SBDF(spcr->pci_segment, spcr->pci_bus,
+                                    spcr->pci_device, spcr->pci_function);
+    }
+#endif /* CONFIG_HAS_PCI */
+
+    rc = 0;
+
+ out:
+    if ( system_state <= SYS_STATE_early_boot )
+        acpi_os_unmap_memory(&spcr, spcr->header.length);
+    return rc;
+}
+#endif /* CONFIG_ACPI */
+
 /*
  * Configure serial port with a string:
  *   <baud>[/<base_baud>][,DPS[,<io-base>[,<irq>[,<port-bdf>[,<bridge-bdf>]]]]].
@@ -1539,6 +1669,15 @@ static bool __init parse_positional(struct ns16550 *uart, char **str)
             conf += 3;
         }
         else
+#endif
+#ifdef CONFIG_ACPI
+        if ( strncmp(conf, "acpi", 4) == 0 )
+        {
+            if ( acpi_uart_config(uart, uart - ns16550_com) )
+                return true;
+            conf += 4;
+        }
+        else
 #endif
         {
             uart->io_base = simple_strtoull(conf, &conf, 0);
@@ -1643,8 +1782,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             uart->reg_width = simple_strtoul(param_value, NULL, 0);
             break;
 
-#ifdef CONFIG_HAS_PCI
         case device:
+#ifdef CONFIG_ACPI
+            if ( strncmp(param_value, "acpi", 3) == 0 )
+            {
+                acpi_uart_config(uart, uart - ns16550_com);
+                dev_set = true;
+                break;
+            }
+            else
+#endif /* CONFIG_ACPI */
+#ifdef CONFIG_HAS_PCI
             if ( strncmp(param_value, "pci", 3) == 0 )
             {
                 pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
@@ -1656,9 +1804,11 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
                 dev_set = true;
             }
             else
+#endif /* CONFIG_HAS_PCI */
                 PARSE_ERR_RET("Unknown device type %s\n", param_value);
             break;
 
+#if CONFIG_HAS_PCI
         case port_bdf:
         {
             unsigned int b, d, f;
@@ -1680,7 +1830,7 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
             uart->pb_bdf_enable = true;
             break;
         }
-#endif
+#endif /* CONFIG_HAS_PCI */
 
         default:
             PARSE_ERR_RET("Invalid parameter: %s\n", token);
@@ -1845,8 +1995,6 @@ DT_DEVICE_END
 #endif /* HAS_DEVICE_TREE_DISCOVERY */
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_ARM)
-#include <xen/acpi.h>
-
 static int __init ns16550_acpi_uart_init(const void *data)
 {
     struct acpi_table_header *table;
@@ -1857,51 +2005,13 @@ static int __init ns16550_acpi_uart_init(const void *data)
      * Only support one UART on ARM which happen to be ns16550_com[0].
      */
     struct ns16550 *uart = &ns16550_com[0];
-
-    status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
-    if ( ACPI_FAILURE(status) )
-    {
-        printk("ns16550: Failed to get SPCR table\n");
-        return -EINVAL;
-    }
-
-    spcr = container_of(table, struct acpi_table_spcr, header);
-
-    if ( unlikely(spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) )
-    {
-        printk("ns16550: Address space type is not mmio\n");
-        return -EINVAL;
-    }
-
-    /*
-     * The serial port address may be 0 for example
-     * if the console redirection is disabled.
-     */
-    if ( unlikely(!spcr->serial_port.address) )
-    {
-        printk("ns16550: Console redirection is disabled\n");
-        return -EINVAL;
-    }
+    int rc;
 
     ns16550_init_common(uart);
 
-    /*
-     * The baud rate is pre-configured by the firmware.
-     * And currently the ACPI part is only targeting ARM so the flow_control
-     * field and all PCI related ones which we do not care yet are ignored.
-     */
-    uart->baud = BAUD_AUTO;
-    uart->data_bits = 8;
-    uart->parity = spcr->parity;
-    uart->stop_bits = spcr->stop_bits;
-    uart->io_base = spcr->serial_port.address;
-    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
-    uart->reg_shift = spcr->serial_port.bit_offset;
-    uart->reg_width = spcr->serial_port.access_width;
-
-    /* The trigger/polarity information is not available in spcr. */
-    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
-    uart->irq = spcr->interrupt;
+    rc = acpi_uart_config(uart, 0);
+    if ( rc )
+        return rc;
 
     uart->vuart.base_addr = uart->io_base;
     uart->vuart.size = uart->io_size;
diff --git a/xen/include/acpi/actables.h b/xen/include/acpi/actables.h
index 527e1c9f9b9d..afb808998825 100644
--- a/xen/include/acpi/actables.h
+++ b/xen/include/acpi/actables.h
@@ -104,4 +104,9 @@ acpi_tb_install_table(acpi_physical_address address,
 acpi_status
 acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags);
 
+/* Get a table early, before acpi_tb_parse_root_table() is called. */
+acpi_status
+acpi_early_get_table(const char *signature, acpi_native_uint instance,
+                     struct acpi_table_header **out_table);
+
 #endif				/* __ACTABLES_H__ */
diff --git a/xen/include/acpi/actbl2.h b/xen/include/acpi/actbl2.h
index ee96e990d636..923c784e8bd5 100644
--- a/xen/include/acpi/actbl2.h
+++ b/xen/include/acpi/actbl2.h
@@ -1037,6 +1037,16 @@ struct acpi_table_spcr {
 
 #define ACPI_SPCR_DO_NOT_DISABLE    (1)
 
+/* Masks for interrupt_type field above */
+#define ACPI_SPCR_INTR_TYPE_PC_AT   0x01
+#define ACPI_SPCR_INTR_TYPE_IO_APIC 0x02
+
+/* Values for the baud_rate field above */
+#define ACPI_SPCR_BAUD_9600         3
+#define ACPI_SPCR_BAUD_19200        4
+#define ACPI_SPCR_BAUD_57600        5
+#define ACPI_SPCR_BAUD_115200       7
+
 /*******************************************************************************
  *
  * SPMI - Server Platform Management Interface table
-- 
2.51.0

Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Andrew Cooper]

On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> to parse the ACPI SPCR table and obtain the serial configuration.
>
> This is gated to the "acpi" device type being set in "com1" on the Xen
> command line.  Note that there can only be one serial device described in
> the SPCR, so limit it's usage to com1 exclusively for the time being.
>
> I can't test the interrupt information parsing on my system, as the
> interrupt is set to GSI with a value of 0xff, which is outside of the range
> of GSIs available on the system.  I've also assumed that the interrupt
> being 0xff is used to signal not interrupt setup (just like the Interrupt
> Pin register on PCI headers).
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> WIP/RFC, not sure whether there's interest in attempting to pursue this
> further on x86.  So far the device I have is also exposed on the PCI bus
> aside from SPCR, so using com1=device=amt also works to detect it.
>
> Posting it kind of early to know whether I should try to polish it for
> submission or we are happy with not having this on x86.

I think we should be using SPCR/DBG2 when available.  Getting serial
configuration right is always tricky, and we might as well use the help
that Microsoft have forced the OEM/firmware world to provide.

But, I think it should be automatic when the user asked for any kind of
serial.  e.g. console=com1 with no com1 configuration.  The point of
these tables is to provide an enumeration mechanism where none
previously existed.

~Andrew

Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Jan Beulich]

On 26.03.2026 13:11, Andrew Cooper wrote:
> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
>> to parse the ACPI SPCR table and obtain the serial configuration.
>>
>> This is gated to the "acpi" device type being set in "com1" on the Xen
>> command line.  Note that there can only be one serial device described in
>> the SPCR, so limit it's usage to com1 exclusively for the time being.
>>
>> I can't test the interrupt information parsing on my system, as the
>> interrupt is set to GSI with a value of 0xff, which is outside of the range
>> of GSIs available on the system.  I've also assumed that the interrupt
>> being 0xff is used to signal not interrupt setup (just like the Interrupt
>> Pin register on PCI headers).
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>> ---
>> WIP/RFC, not sure whether there's interest in attempting to pursue this
>> further on x86.  So far the device I have is also exposed on the PCI bus
>> aside from SPCR, so using com1=device=amt also works to detect it.
>>
>> Posting it kind of early to know whether I should try to polish it for
>> submission or we are happy with not having this on x86.
> 
> I think we should be using SPCR/DBG2 when available.  Getting serial
> configuration right is always tricky, and we might as well use the help
> that Microsoft have forced the OEM/firmware world to provide.
> 
> But, I think it should be automatic when the user asked for any kind of
> serial.  e.g. console=com1 with no com1 configuration.  The point of
> these tables is to provide an enumeration mechanism where none
> previously existed.

Hmm. In the PC world COM<n> have well-known configurations unless anything
else is provided. With multiple serial ports in a system, which one SPCR
describes also would be (largely) unknown.

Jan

Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Andrew Cooper]

On 26/03/2026 12:48 pm, Jan Beulich wrote:
> On 26.03.2026 13:11, Andrew Cooper wrote:
>> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
>>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
>>> to parse the ACPI SPCR table and obtain the serial configuration.
>>>
>>> This is gated to the "acpi" device type being set in "com1" on the Xen
>>> command line.  Note that there can only be one serial device described in
>>> the SPCR, so limit it's usage to com1 exclusively for the time being.
>>>
>>> I can't test the interrupt information parsing on my system, as the
>>> interrupt is set to GSI with a value of 0xff, which is outside of the range
>>> of GSIs available on the system.  I've also assumed that the interrupt
>>> being 0xff is used to signal not interrupt setup (just like the Interrupt
>>> Pin register on PCI headers).
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>> WIP/RFC, not sure whether there's interest in attempting to pursue this
>>> further on x86.  So far the device I have is also exposed on the PCI bus
>>> aside from SPCR, so using com1=device=amt also works to detect it.
>>>
>>> Posting it kind of early to know whether I should try to polish it for
>>> submission or we are happy with not having this on x86.
>> I think we should be using SPCR/DBG2 when available.  Getting serial
>> configuration right is always tricky, and we might as well use the help
>> that Microsoft have forced the OEM/firmware world to provide.
>>
>> But, I think it should be automatic when the user asked for any kind of
>> serial.  e.g. console=com1 with no com1 configuration.  The point of
>> these tables is to provide an enumeration mechanism where none
>> previously existed.
> Hmm. In the PC world COM<n> have well-known configurations unless anything
> else is provided. With multiple serial ports in a system, which one SPCR
> describes also would be (largely) unknown.

Xen's COM1/2 already do do far more than the PC world.  But ok then, we
invent a new "serial".

My point is, there should be a way to say "please use serial as
described by the system", and it shouldn't even require knowing that the
description is in APCI.

~Andrew

Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Roger Pau Monné]

On Thu, Mar 26, 2026 at 12:52:51PM +0000, Andrew Cooper wrote:
> On 26/03/2026 12:48 pm, Jan Beulich wrote:
> > On 26.03.2026 13:11, Andrew Cooper wrote:
> >> On 25/03/2026 2:58 pm, Roger Pau Monne wrote:
> >>> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> >>> to parse the ACPI SPCR table and obtain the serial configuration.
> >>>
> >>> This is gated to the "acpi" device type being set in "com1" on the Xen
> >>> command line.  Note that there can only be one serial device described in
> >>> the SPCR, so limit it's usage to com1 exclusively for the time being.
> >>>
> >>> I can't test the interrupt information parsing on my system, as the
> >>> interrupt is set to GSI with a value of 0xff, which is outside of the range
> >>> of GSIs available on the system.  I've also assumed that the interrupt
> >>> being 0xff is used to signal not interrupt setup (just like the Interrupt
> >>> Pin register on PCI headers).
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>> WIP/RFC, not sure whether there's interest in attempting to pursue this
> >>> further on x86.  So far the device I have is also exposed on the PCI bus
> >>> aside from SPCR, so using com1=device=amt also works to detect it.
> >>>
> >>> Posting it kind of early to know whether I should try to polish it for
> >>> submission or we are happy with not having this on x86.
> >> I think we should be using SPCR/DBG2 when available.  Getting serial
> >> configuration right is always tricky, and we might as well use the help
> >> that Microsoft have forced the OEM/firmware world to provide.
> >>
> >> But, I think it should be automatic when the user asked for any kind of
> >> serial.  e.g. console=com1 with no com1 configuration.  The point of
> >> these tables is to provide an enumeration mechanism where none
> >> previously existed.
> > Hmm. In the PC world COM<n> have well-known configurations unless anything
> > else is provided. With multiple serial ports in a system, which one SPCR
> > describes also would be (largely) unknown.
> 
> Xen's COM1/2 already do do far more than the PC world.  But ok then, we
> invent a new "serial".
> 
> My point is, there should be a way to say "please use serial as
> described by the system", and it shouldn't even require knowing that the
> description is in APCI.

That's how it kind of works on FreeBSD, the user asks for "serial"
output and the first thing that's checked is SPCR (if the system
support ACPI).

We could have something like `console=serial` and let Xen figure it
out, but it would probably need more logic in case SPCR is not
present, and I'm unsure how the extra heuristics should look like.

Thanks, Roger.

Re: [PATCH 4/4] xen/uart: enable parsing ACPI SPCR on x86

[Jan Beulich]

On 25.03.2026 15:58, Roger Pau Monne wrote:
> Introduce extra logic to allow parsing ACPI tables extra early, and use it
> to parse the ACPI SPCR table and obtain the serial configuration.
> 
> This is gated to the "acpi" device type being set in "com1" on the Xen
> command line.  Note that there can only be one serial device described in
> the SPCR, so limit it's usage to com1 exclusively for the time being.
> 
> I can't test the interrupt information parsing on my system, as the
> interrupt is set to GSI with a value of 0xff, which is outside of the range
> of GSIs available on the system.  I've also assumed that the interrupt
> being 0xff is used to signal not interrupt setup (just like the Interrupt
> Pin register on PCI headers).
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> WIP/RFC, not sure whether there's interest in attempting to pursue this
> further on x86.  So far the device I have is also exposed on the PCI bus
> aside from SPCR, so using com1=device=amt also works to detect it.
> 
> Posting it kind of early to know whether I should try to polish it for
> submission or we are happy with not having this on x86.

One concern of mine is the altering ACPI CA code. Otoh, seeing how early
you need this for SPCR, I wonder if it then couldn't also be used for the
BGRT work that's being done in parallel.

> @@ -523,3 +540,67 @@ acpi_tb_parse_root_table(acpi_physical_address rsdp_address, u8 flags)
>  
>  	return_ACPI_STATUS(AE_OK);
>  }
> +
> +acpi_status __init
> +acpi_early_get_table(const char *signature, acpi_native_uint instance,
> +                     struct acpi_table_header **out_table)
> +{
> +	static acpi_physical_address __initdata table_addr[128];
> +	static unsigned int __initdata table_count;
> +	static unsigned int __initdata table_entry_size;
> +	unsigned int i;
> +
> +	ACPI_FUNCTION_TRACE(tb_early_get_table);
> +
> +	if (!table_count) {
> +		struct acpi_table_header *table;
> +		void *table_entry;
> +		acpi_status status;
> +		acpi_physical_address rsdp_address = acpi_os_get_root_pointer();
> +
> +		if (!rsdp_address)
> +			return_ACPI_STATUS(AE_NOT_FOUND);
> +
> +		status = acpi_tb_get_root_table(rsdp_address, &table,
> +			                        &table_entry_size);
> +		if (!ACPI_SUCCESS(status))
> +			return_ACPI_STATUS(status);
> +
> +		/* Calculate the number of tables described in the root table */
> +		table_count = (table->length - sizeof(*table)) / table_entry_size;
> +
> +		if (table_count > ARRAY_SIZE(table_addr)) {
> +			table_count = 0;
> +			return_ACPI_STATUS(AE_NO_MEMORY);
> +		}

Rather than failing, limit table_count to ARRAY_SIZE()?

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -1375,6 +1375,136 @@ static void enable_exar_enhanced_bits(const struct ns16550 *uart)
>  
>  #endif /* CONFIG_HAS_PCI */
>  
> +#ifdef CONFIG_ACPI
> +#include <acpi/acpi.h>

With xen/acpi.h included (below) this shouldn't be needed.

> +#include <acpi/actables.h>
> +
> +#include <xen/acpi.h>
> +
> +static int __init acpi_uart_config(struct ns16550 *uart, unsigned int idx)
> +{
> +    struct acpi_table_header *table;

While this can't be pointer-to-const, ...

> +    struct acpi_table_spcr *spcr;

... it looks like this can be.

> +    acpi_status status;
> +    int rc = 0;
> +
> +    /*
> +     * SPCR specifies a single port, expect it to be configured at position 0
> +     * in the uart array.
> +     */
> +    if ( idx )
> +        return -EXDEV;

While this matches what ns16550_acpi_uart_init() does / wants, I'm not sure
this is a good idea. If a system had a normal COM1 and something in SPCR,
one would need to re-define COM2 with the COM1 settings.

> +    if ( system_state <= SYS_STATE_early_boot )
> +        status = acpi_early_get_table(ACPI_SIG_SPCR, 0, &table);
> +    else
> +        status = acpi_get_table(ACPI_SIG_SPCR, 0, &table);
> +
> +    if ( ACPI_FAILURE(status) )
> +    {
> +        printk(XENLOG_ERR "Failed to find or parse ACPI SPCR table\n");
> +        return -ENODEV;
> +    }
> +
> +    spcr = container_of(table, struct acpi_table_spcr, header);
> +
> +    rc = -EDOM;
> +    if ( spcr->interface_type != ACPI_DBG2_16550_COMPATIBLE )
> +    {
> +        printk(XENLOG_ERR "Incompatible ACPI SPCR UART interface %u\n",
> +               spcr->interface_type);
> +        goto out;
> +    }
> +
> +    if ( spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
> +         (IS_ENABLED(CONFIG_ARM) ||

Better !IS_ENABLED(CONFIG_X86), seeing how neither RISC-V nor PPC have
I/O ports?

> +          spcr->serial_port.space_id != ACPI_ADR_SPACE_SYSTEM_IO) )
> +    {
> +        printk(XENLOG_ERR "Incompatible ACPI SPCR UART address space %u\n",
> +               spcr->serial_port.space_id);
> +        goto out;
> +    }
> +
> +    if ( !spcr->serial_port.address )
> +    {
> +        printk(XENLOG_ERR "ACPI SPCR console redirection disabled\n");
> +        goto out;
> +    }
> +
> +    uart->io_base = spcr->serial_port.address;

Elsewhere we assume MMIO if the address is 0x10000 or above. Here we have
an ACPI_ADR_SPACE_* indicator, which I think we should take into account.
For now merely to reject values not fitting assumptions elsewhere, I guess.

> +    uart->io_size = DIV_ROUND_UP(spcr->serial_port.bit_width, BITS_PER_BYTE);
> +    uart->reg_shift = spcr->serial_port.bit_offset;
> +
> +    uart->parity = spcr->parity;
> +    uart->stop_bits = spcr->stop_bits;
> +    uart->data_bits = 8;
> +
> +    if ( uart->baud == BAUD_AUTO && spcr->baud_rate )
> +    {
> +        switch ( spcr->baud_rate )
> +        {
> +        case ACPI_SPCR_BAUD_9600:
> +            uart->baud = 9600;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_19200:
> +            uart->baud = 19200;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_57600:
> +            uart->baud = 57600;
> +            break;
> +
> +        case ACPI_SPCR_BAUD_115200:
> +            uart->baud = 115200;
> +            break;
> +
> +        default:
> +            printk(XENLOG_WARNING
> +                   "Ignoring invalid baud rate %u in ACPI SPCR\n",
> +                   spcr->baud_rate);

Maybe better s/invalid/unknown/?

Also, please add "break" for Misra's sake.

> +        }
> +    }
> +
> +    if ( IS_ENABLED(CONFIG_X86) )
> +    {
> +        /* Use polling mode by default. */
> +        uart->irq = 0;
> +
> +        if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_IO_APIC) &&
> +             spcr->interrupt < 0xff )
> +            uart->irq = spcr->interrupt;
> +        else if ( (spcr->interrupt_type & ACPI_SPCR_INTR_TYPE_PC_AT) &&
> +                  ((spcr->pc_interrupt >=  2 && spcr->pc_interrupt <=  7) ||

Is 2 valid to use? That's the cascade in 8259-s.

> +                   (spcr->pc_interrupt >= 14 && spcr->pc_interrupt <= 15)) )
> +            uart->irq = spcr->pc_interrupt;
> +    }
> +
> +#ifdef CONFIG_ARM
> +    /* The trigger/polarity information is not available in spcr. */
> +    irq_set_type(spcr->interrupt, IRQ_TYPE_LEVEL_HIGH);
> +    uart->irq = spcr->interrupt;
> +#endif /* CONFIG_ARM */
> +
> +#ifdef CONFIG_HAS_PCI
> +    if ( spcr->pci_device_id != 0xffff && spcr->pci_vendor_id != 0xffff )
> +    {
> +        uart->ps_bdf_enable = true;
> +        uart->pci_device = PCI_SBDF(spcr->pci_segment, spcr->pci_bus,
> +                                    spcr->pci_device, spcr->pci_function);
> +    }
> +#endif /* CONFIG_HAS_PCI */
> +
> +    rc = 0;
> +
> + out:
> +    if ( system_state <= SYS_STATE_early_boot )
> +        acpi_os_unmap_memory(&spcr, spcr->header.length);

I think you'd better unmap "table" here, and I don't think the & is correct
to use.

> @@ -1643,8 +1782,17 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>              uart->reg_width = simple_strtoul(param_value, NULL, 0);
>              break;
>  
> -#ifdef CONFIG_HAS_PCI
>          case device:
> +#ifdef CONFIG_ACPI
> +            if ( strncmp(param_value, "acpi", 3) == 0 )
> +            {
> +                acpi_uart_config(uart, uart - ns16550_com);
> +                dev_set = true;
> +                break;
> +            }
> +            else

May I ask to drop either the "else" or the "break"? To match PCI code,
it would be the latter.

> +#endif /* CONFIG_ACPI */
> +#ifdef CONFIG_HAS_PCI
>              if ( strncmp(param_value, "pci", 3) == 0 )
>              {
>                  pci_uart_config(uart, 1/* skip AMT */, uart - ns16550_com);
> @@ -1656,9 +1804,11 @@ static bool __init parse_namevalue_pairs(char *str, struct ns16550 *uart)
>                  dev_set = true;
>              }
>              else
> +#endif /* CONFIG_HAS_PCI */
>                  PARSE_ERR_RET("Unknown device type %s\n", param_value);
>              break;

I think for !ACPI && !HAS_PCI we'd better not alter behavior (i.e. that
case would still better end up at default:).

> --- a/xen/include/acpi/actbl2.h
> +++ b/xen/include/acpi/actbl2.h
> @@ -1037,6 +1037,16 @@ struct acpi_table_spcr {
>  
>  #define ACPI_SPCR_DO_NOT_DISABLE    (1)
>  
> +/* Masks for interrupt_type field above */
> +#define ACPI_SPCR_INTR_TYPE_PC_AT   0x01
> +#define ACPI_SPCR_INTR_TYPE_IO_APIC 0x02
> +
> +/* Values for the baud_rate field above */
> +#define ACPI_SPCR_BAUD_9600         3
> +#define ACPI_SPCR_BAUD_19200        4
> +#define ACPI_SPCR_BAUD_57600        5
> +#define ACPI_SPCR_BAUD_115200       7

Not your fault, but I wonder why we have SPCR here when Linux has it in
actbl3.h.

Jan