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

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

Jan


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