Re: [Xen-devel] [PATCH v2 2/7] serial: Fix COM1 assumption if pci_uart_config did not find the PCI serial card.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Mar 10, 2014 at 09:35:16AM +0000, Jan Beulich wrote:
> >>> On 07.03.14 at 20:01, Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> wrote:
> > The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
> > in __setup_xen. Then we call 'ns16550_init' which copies those in
> > the appropiate uart, which then calls 'ns16550_parse_port_config'
> > to deal with parameter parsing. If the 'pci' or 'amt' parameter
> > has been specified we further call 'pci_uart_config code' which
> > scans the PCI bus. If it does not find any relevant devices
> > it will overwrite io_base with 0x3f8 regardless whether this is
> > COM1 or COM2. The overwrite is a way to set it back to the
> > failsafe defaults - except for COM2 of course.
> > 
> > This in theory (as I don't have a machine with two COM ports
> > readily available) means that if the user specified 'com2=9600,8n1,pci'
> > and the device did not have an PCI serial card, instead of using 0x2f8
> > for the io_base it ends up using 0x3f8 - and we don't get the
> > output on COM2.
> > 
> > Fix it by saving the original io_base before starting the
> > scan of the PCI bus. If we don't find an serial PCI device then
> > assign the original io_base value back.
> 
> While reviewing patch 1 I was specifically thinking of why you didn't
> do this at once. But then I realized that this is done only in the AMT
> case, and was assuming that you, when originally adding AMT
> support, specifically did it that way knowing that if anything it could
> only sit on port 0x3f8. If that wasn't the original intention, the
> change is fine, but the description should be updated to say that
> this only affects the AMT case.
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -612,10 +612,11 @@ static int __init
> >  pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
> >  {
> >      uint32_t bar, bar_64 = 0, len, len_64;
> > -    u64 size, mask;
> > +    u64 size, mask, orig_base;
> >      unsigned int b, d, f, nextf, i;
> >      u16 vendor, device;
> >  
> > +    orig_base = uart->io_base;
> >      uart->io_base = 0;
> >      /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. 
> > */
> >      for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
> > @@ -747,7 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, 
> > int bar_idx)
> >      if ( !skip_amt )
> >          return -1;
> >  
> > -    uart->io_base = 0x3f8;
> > +    if ( !uart->io_base )
> > +        uart->io_base = orig_base;
> 
> I don't think io_base can be zero when getting here. And even if it
> could, what would be there would be bogus/wrong. Hence I think
> you will want to unconditionally restore the original value.

It can (with the "serial: Skip over PCIe device which have no quirks
(fix AMT regression)." patch.

If an user specified 'com1=115200,8n1,amt' on a machine without
AMT but with the old-fashioned COM1, this would still work.

The scan of the PCI bus would naturally fail, so the io_base would
be zero (as we just had set it to zero at the start of the loop).

But as you mentioned - if we are done with the loop and hadn't
found anything - we might as well uncondionally set it to the
original value. I was being a bit conservative here.

> 
> >      uart->irq = 0;
> 
> I further wonder whether that's not really suffering from the same
> issue: Shouldn't you save/restore this too? Or rather, looking at the
> flow, simply delete the line, leaving the old value in place?

Yes.
<nods>

Let me do it per you suggestion and just do this:

From 5519bf9cfcaf6ec99af146825ae53f7f53deb37c Mon Sep 17 00:00:00 2001
From: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
Date: Fri, 7 Mar 2014 10:45:04 -0500
Subject: [PATCH] serial: Fix COM1 assumption if pci_uart_config did not find
 the PCI serial card.

The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
in __setup_xen. Then we call 'ns16550_init' which copies those in
the appropiate uart, which then calls 'ns16550_parse_port_config'
to deal with parameter parsing. If the 'pci' or 'amt' parameter
has been specified we further call 'pci_uart_config code' which
scans the PCI bus. If it does not find any relevant devices
it will overwrite io_base with 0x3f8 regardless whether this is
COM1 or COM2. The overwrite is a way to set it back to the
failsafe defaults - except for COM2 of course.

This in theory (as I don't have a machine with two COM ports
readily available) means that if the user specified 'com2=9600,8n1,pci'
and the device did not have an PCI serial card, instead of using 0x2f8
for the io_base it ends up using 0x3f8 - and we don't get the
output on COM2.

Fix it by saving the original io_base before starting the
scan of the PCI bus. If we don't find an serial PCI device then
assign the original io_base value back.

Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
[v1: Also remove the irq overwrite]
---
 xen/drivers/char/ns16550.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 2fded08..b440e01 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -612,10 +612,11 @@ static int __init
 pci_uart_config (struct ns16550 *uart, int skip_amt, int bar_idx)
 {
     uint32_t bar, bar_64 = 0, len, len_64;
-    u64 size, mask;
+    u64 size, mask, orig_base;
     unsigned int b, d, f, nextf, i;
     u16 vendor, device;
 
+    orig_base = uart->io_base;
     uart->io_base = 0;
     /* NB. Start at bus 1 to avoid AMT: a plug-in card cannot be on bus 0. */
     for ( b = skip_amt ? 1 : 0; b < 0x100; b++ )
@@ -747,8 +748,8 @@ pci_uart_config (struct ns16550 *uart, int skip_amt, int 
bar_idx)
     if ( !skip_amt )
         return -1;
 
-    uart->io_base = 0x3f8;
-    uart->irq = 0;
+    /* No PCI or PCIe found, fallback to the defaults. */
+    uart->io_base = orig_base;
     uart->clock_hz  = UART_CLOCK_HZ;
 
     return 0;
-- 
1.7.7.6


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel