Re: [PATCH v3 04/10] console: support multiple serial console simultaneously

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -85,6 +85,17 @@ config SERIAL_TX_BUFSIZE
>  
>         Default value is 16384 (16kiB).
>  
> +config MAX_SERCONS
> +     int "Maximum number of serial consoles active at once"
> +     default 4
> +     help
> +      Controls how many serial consoles can be active at once. Configuring 
> more
> +      using `console=` parameter will be ignored.
> +      When multiple consoles are configured, overhead of `sync_console` 
> option
> +      is even bigger.
> +
> +       Default value is 4.

Indentation (the help text ought to be indented by a tab and two spaces).

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -113,7 +113,9 @@ static char *__read_mostly conring = _conring;
>  static uint32_t __read_mostly conring_size = _CONRING_SIZE;
>  static uint32_t conringc, conringp;
>  
> -static int __read_mostly sercon_handle = -1;
> +#define MAX_SERCONS CONFIG_MAX_SERCONS
> +static int __read_mostly sercon_handle[MAX_SERCONS];
> +static int __read_mostly nr_sercon_handle = 0;

unsigned int please.

> @@ -393,32 +395,59 @@ long read_console_ring(struct xen_sysctl_readconsole 
> *op)
>  static char serial_rx_ring[SERIAL_RX_SIZE];
>  static unsigned int serial_rx_cons, serial_rx_prod;
>  
> -static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
> +/* The last entry means "steal from all consoles" */
> +static void (*serial_steal_fn[MAX_SERCONS+1])(const char *, size_t nr) = {

Nit: blanks please around + . But with ...

> +    [MAX_SERCONS] = early_puts,

... this initializer you could as well omit the dimension.

> +};
>  
> +/*
> + * Redirect console *handle* output to *fn*. Use SERHND_STEAL_ALL as 
> *handle* to
> + * redirect all the consoles. 
> + */
>  int console_steal(int handle, void (*fn)(const char *, size_t nr))
>  {
> -    if ( (handle == -1) || (handle != sercon_handle) )
> -        return 0;
> +    int i;

While from the use inside the function this would better be unsigned int,
I can see how that would be slightly odd with the return value. But with
overly high a MAX_SERCONS ...

> +    if ( handle == -1 )
> +        return -ENOENT;
> +    if ( serial_steal_fn[MAX_SERCONS] != NULL )
> +        return -EBUSY;
> +    if ( handle == SERHND_STEAL_ALL )
> +    {
> +        serial_steal_fn[MAX_SERCONS] = fn;
> +        return MAX_SERCONS;
> +    }
> +    for ( i = 0; i < nr_sercon_handle; i++ )
> +        if ( handle == sercon_handle[i] )

... the array access will degenerate when i is "int", but will be okay
when i is "unsigned int" (and of course everything will break if
MAX_SERCONS > UINT_MAX).

> +            break;
> +    if ( i == nr_sercon_handle )
> +        return -ENOENT;
>  
> -    if ( serial_steal_fn != NULL )
> +    if ( serial_steal_fn[i] != NULL )
>          return -EBUSY;
>  
> -    serial_steal_fn = fn;
> -    return 1;
> +    serial_steal_fn[i] = fn;
> +    return i;
>  }
>  
>  void console_giveback(int id)
>  {
> -    if ( id == 1 )
> -        serial_steal_fn = NULL;
> +    if ( id >= 0 && id <= MAX_SERCONS )
> +        serial_steal_fn[id] = NULL;
>  }
>  
>  void console_serial_puts(const char *s, size_t nr)
>  {
> -    if ( serial_steal_fn != NULL )
> -        serial_steal_fn(s, nr);
> +    int i;

unsigned int please, again (yet more further down).

> +    if ( serial_steal_fn[MAX_SERCONS] != NULL )
> +        serial_steal_fn[MAX_SERCONS](s, nr);
>      else
> -        serial_puts(sercon_handle, s, nr);
> +        for ( i = 0; i < nr_sercon_handle; i++ )
> +            if ( serial_steal_fn[i] != NULL )
> +                serial_steal_fn[i](s, nr);
> +            else
> +                serial_puts(sercon_handle[i], s, nr);

This for() would better gain braces.

> @@ -977,8 +1006,12 @@ void __init console_init_preirq(void)
>              continue;
>          else if ( (sh = serial_parse_handle(p)) >= 0 )
>          {
> -            sercon_handle = sh;
> -            serial_steal_fn = NULL;
> +            if ( nr_sercon_handle < MAX_SERCONS )
> +                sercon_handle[nr_sercon_handle++] = sh;
> +            else
> +                printk("Too many consoles (max %d), ignoring '%s'\n",
> +                       MAX_SERCONS, p);

In order to use p here, I think we want (need?) to make
serial_parse_handle()'s parameter const char *.

> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1078,8 +1078,12 @@ void __init xhci_dbc_uart_init(void)
>  
>          e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
>          if ( !e || *e )
> +        {
> +            printk(XENLOG_ERR
> +                   "Invalid dbgp= PCI device spec: '%s'\n",
> +                   opt_dbgp);
>              return;
> -
> +        }
>          dbc->sbdf = PCI_SBDF(0, bus, slot, func);
>      }

Does this change belong elsewhere?

Jan