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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 10.03.14 at 17:07, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> wrote:
> 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.

And I realize that there was a "non-" missing in front of the "zero"
in my reply.

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

Looks good to me.

Jan


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