1. Patchew
  2. Xen
  3. [PATCH v2 0/3] xen/uart: some fixes and improvements
  4. View patch

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

Roger Pau Monne posted 3 patches 6 days, 1 hour ago
[PATCH v2 2/3] xen/uart: be more careful with changes to the PCI command register
Posted by Roger Pau Monne 6 days, 1 hour ago 
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
Posted by Jan Beulich 3 days, 7 hours ago 
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
Posted by Roger Pau Monné 3 days, 6 hours ago 
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
Posted by Jan Beulich 3 days, 6 hours ago 
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
Posted by Roger Pau Monné 3 days, 5 hours ago 
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
Posted by Jan Beulich 3 days, 5 hours ago 
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

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.