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

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

[Roger Pau Monne]

Hello,

First two patches are bugfixes, last patch switches the ns16550 driver
to use pci_sbdf_t.

Thanks, Roger.

Roger Pau Monne (3):
  xen/uart: uniformly set ->ps_bdf_enable for all PCI serial devices
  xen/uart: be more careful with changes to the PCI command register
  xen/uart: switch ns16550 to use pci_sbdf_t

 xen/drivers/char/ns16550.c | 145 +++++++++++++++++--------------------
 1 file changed, 67 insertions(+), 78 deletions(-)

-- 
2.51.0

[PATCH v2 1/3] xen/uart: uniformly set ->ps_bdf_enable for all PCI serial devices

[Roger Pau Monne]

Auto-discovered serial PCI devices when using device=amt|pci won't get
->ps_bdf_enable, and as such some of the logic (like making sure the
respective BARs are enabled) won't be applied to them.

Fix by unconditionally setting ->ps_bdf_enable for all PCI serial devices,
and removing the special case that was done in some places by checking
whether the ->bar was set.  This also allows simplifying the logic in
pci_serial_early_init().

Fixes: 9738db88f68f ("xen: Automatically find serial port on PCI/PCIe and AMT devices.")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/drivers/char/ns16550.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index d384f1c69d2c..d05dc506ed9c 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -283,7 +283,10 @@ static int cf_check ns16550_getc(struct serial_port *port, char *pc)
 static void pci_serial_early_init(struct ns16550 *uart)
 {
 #ifdef NS16550_PCI
-    if ( uart->bar && uart->io_base >= 0x10000 )
+    if ( !uart->ps_bdf_enable )
+        return;
+
+    if ( uart->io_base >= 0x10000 )
     {
         pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
                                   uart->ps_bdf[2]),
@@ -291,9 +294,6 @@ static void pci_serial_early_init(struct ns16550 *uart)
         return;
     }
 
-    if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
-        return;
-
     if ( uart->pb_bdf_enable )
         pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
                                   uart->pb_bdf[2]),
@@ -440,7 +440,7 @@ static void __init cf_check ns16550_init_postirq(struct serial_port *port)
         unsigned int, 1, (bits * uart->fifo_size * 1000) / uart->baud);
 
 #ifdef NS16550_PCI
-    if ( uart->bar || uart->ps_bdf_enable )
+    if ( uart->ps_bdf_enable )
     {
         if ( uart->param && uart->param->mmio &&
              rangeset_add_range(mmio_ro_ranges, PFN_DOWN(uart->io_base),
@@ -1335,6 +1335,7 @@ 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_enable = true;
                 uart->ps_bdf[0] = b;
                 uart->ps_bdf[1] = d;
                 uart->ps_bdf[2] = f;
-- 
2.51.0

Re: [PATCH v2 1/3] xen/uart: uniformly set ->ps_bdf_enable for all PCI serial devices

[Jan Beulich]

On 27.03.2026 14:54, Roger Pau Monne wrote:
> Auto-discovered serial PCI devices when using device=amt|pci won't get
> ->ps_bdf_enable, and as such some of the logic (like making sure the
> respective BARs are enabled) won't be applied to them.
> 
> Fix by unconditionally setting ->ps_bdf_enable for all PCI serial devices,
> and removing the special case that was done in some places by checking
> whether the ->bar was set.  This also allows simplifying the logic in
> pci_serial_early_init().
> 
> Fixes: 9738db88f68f ("xen: Automatically find serial port on PCI/PCIe and AMT devices.")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
with one suggestion:

> @@ -1335,6 +1335,7 @@ 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_enable = true;
>                  uart->ps_bdf[0] = b;
>                  uart->ps_bdf[1] = d;
>                  uart->ps_bdf[2] = f;

Largely for the look of it, perhaps set the boolean only after having set
the covered fields?

Jan

[PATCH v2 2/3] 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, which might put the device into a non-working state.

Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Adjusted over previous fixes.
---
 xen/drivers/char/ns16550.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index d05dc506ed9c..d16e447c0e76 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -283,14 +283,19 @@ 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;
+
     if ( !uart->ps_bdf_enable )
         return;
 
+    cmd = pci_conf_read16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
+                                  uart->ps_bdf[2]), PCI_COMMAND);
+
     if ( 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 +312,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 v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Jan Beulich]

On 27.03.2026 14:54, 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, which might put the device into a non-working state.
> 
> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

I would have preferred if the description mentioned the particular case,
turning this more into a workaround than an apparent bugfix. As mentioned,
us driving the device generally means we're free to do whatever we want to
the command register, as long as resulting device state is consistent
overall (or else we may indeed have a non-working device). Having to keep
memory decoding enabled in order for I/O ports to function is pretty
clearly a bug in the device, and hence us "violating" that requirement
isn't really o bug of ours.

Jan

Re: [PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Roger Pau Monné]

On Mon, Mar 30, 2026 at 10:00:05AM +0200, Jan Beulich wrote:
> On 27.03.2026 14:54, 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, which might put the device into a non-working state.
> > 
> > Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I would have preferred if the description mentioned the particular case,
> turning this more into a workaround than an apparent bugfix. 

It turns out that the console does seem to work fine, even with memory
decoding disabled on the device (as expected).  I've updated the
firmware in the meantime, so I'm unsure whether that update has
changed the behavior of the device, or it simply was some other
instability that was causing the issue in the past.  This SOL AMT
device is not reliable at all I'm afraid.

> As mentioned,
> us driving the device generally means we're free to do whatever we want to
> the command register, as long as resulting device state is consistent
> overall (or else we may indeed have a non-working device). Having to keep
> memory decoding enabled in order for I/O ports to function is pretty
> clearly a bug in the device, and hence us "violating" that requirement
> isn't really o bug of ours.

I think given the fragility of some of those SOL devices it's best to
limit the number of bits Xen changes, as to having a bigger chances of
getting output working.

Thanks, Roger.

Re: [PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Jan Beulich]

On 30.03.2026 11:06, Roger Pau Monné wrote:
> On Mon, Mar 30, 2026 at 10:00:05AM +0200, Jan Beulich wrote:
>> On 27.03.2026 14:54, 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, which might put the device into a non-working state.
>>>
>>> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>> I would have preferred if the description mentioned the particular case,
>> turning this more into a workaround than an apparent bugfix. 
> 
> It turns out that the console does seem to work fine, even with memory
> decoding disabled on the device (as expected).  I've updated the
> firmware in the meantime, so I'm unsure whether that update has
> changed the behavior of the device, or it simply was some other
> instability that was causing the issue in the past.  This SOL AMT
> device is not reliable at all I'm afraid.
> 
>> As mentioned,
>> us driving the device generally means we're free to do whatever we want to
>> the command register, as long as resulting device state is consistent
>> overall (or else we may indeed have a non-working device). Having to keep
>> memory decoding enabled in order for I/O ports to function is pretty
>> clearly a bug in the device, and hence us "violating" that requirement
>> isn't really o bug of ours.
> 
> I think given the fragility of some of those SOL devices it's best to
> limit the number of bits Xen changes, as to having a bigger chances of
> getting output working.

That's okay(ish); I merely would wish the patch description was less
suggesting that Xen was actually buggy.

Jan

Re: [PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Roger Pau Monné]

On Mon, Mar 30, 2026 at 11:09:10AM +0200, Jan Beulich wrote:
> On 30.03.2026 11:06, Roger Pau Monné wrote:
> > On Mon, Mar 30, 2026 at 10:00:05AM +0200, Jan Beulich wrote:
> >> On 27.03.2026 14:54, 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, which might put the device into a non-working state.
> >>>
> >>> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> I would have preferred if the description mentioned the particular case,
> >> turning this more into a workaround than an apparent bugfix. 
> > 
> > It turns out that the console does seem to work fine, even with memory
> > decoding disabled on the device (as expected).  I've updated the
> > firmware in the meantime, so I'm unsure whether that update has
> > changed the behavior of the device, or it simply was some other
> > instability that was causing the issue in the past.  This SOL AMT
> > device is not reliable at all I'm afraid.
> > 
> >> As mentioned,
> >> us driving the device generally means we're free to do whatever we want to
> >> the command register, as long as resulting device state is consistent
> >> overall (or else we may indeed have a non-working device). Having to keep
> >> memory decoding enabled in order for I/O ports to function is pretty
> >> clearly a bug in the device, and hence us "violating" that requirement
> >> isn't really o bug of ours.
> > 
> > I think given the fragility of some of those SOL devices it's best to
> > limit the number of bits Xen changes, as to having a bigger chances of
> > getting output working.
> 
> That's okay(ish); I merely would wish the patch description was less
> suggesting that Xen was actually buggy.

What about if I change the title to:

xen/uart: avoid clearing PCI command register bits set by the firmware

I think that's clearer and less blameful?

Thanks, Roger.

Re: [PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register

[Jan Beulich]

On 30.03.2026 11:57, Roger Pau Monné wrote:
> On Mon, Mar 30, 2026 at 11:09:10AM +0200, Jan Beulich wrote:
>> On 30.03.2026 11:06, Roger Pau Monné wrote:
>>> On Mon, Mar 30, 2026 at 10:00:05AM +0200, Jan Beulich wrote:
>>>> On 27.03.2026 14:54, 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, which might put the device into a non-working state.
>>>>>
>>>>> Fixes: f2ff5d6628b3 ("ns16550: enable PCI serial card usage")
>>>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>>
>>>> I would have preferred if the description mentioned the particular case,
>>>> turning this more into a workaround than an apparent bugfix. 
>>>
>>> It turns out that the console does seem to work fine, even with memory
>>> decoding disabled on the device (as expected).  I've updated the
>>> firmware in the meantime, so I'm unsure whether that update has
>>> changed the behavior of the device, or it simply was some other
>>> instability that was causing the issue in the past.  This SOL AMT
>>> device is not reliable at all I'm afraid.
>>>
>>>> As mentioned,
>>>> us driving the device generally means we're free to do whatever we want to
>>>> the command register, as long as resulting device state is consistent
>>>> overall (or else we may indeed have a non-working device). Having to keep
>>>> memory decoding enabled in order for I/O ports to function is pretty
>>>> clearly a bug in the device, and hence us "violating" that requirement
>>>> isn't really o bug of ours.
>>>
>>> I think given the fragility of some of those SOL devices it's best to
>>> limit the number of bits Xen changes, as to having a bigger chances of
>>> getting output working.
>>
>> That's okay(ish); I merely would wish the patch description was less
>> suggesting that Xen was actually buggy.
> 
> What about if I change the title to:
> 
> xen/uart: avoid clearing PCI command register bits set by the firmware
> 
> I think that's clearer and less blameful?

Sgtm, ideally also with an explaining sentence in the description.

Jan

[PATCH v2 3/3] 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Not committing this ahead of the bugfixes, as to make those easier to
backport.
---
 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 d16e447c0e76..0d780d82f918 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;
@@ -288,31 +288,22 @@ static void pci_serial_early_init(struct ns16550 *uart)
     if ( !uart->ps_bdf_enable )
         return;
 
-    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->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;
     }
 
     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
 }
 
@@ -452,17 +443,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
             };
@@ -504,9 +494,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
@@ -533,8 +522,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
 }
 
@@ -545,19 +533,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
 
@@ -1217,13 +1201,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 */
@@ -1240,10 +1223,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 )
@@ -1266,29 +1247,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;
@@ -1302,12 +1280,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)
                     uart->fifo_size = param->fifo_size;
 
                 uart->ps_bdf_enable = true;
-                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
@@ -1685,18 +1660,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