Re: [PATCH v2 4/9] console: support multiple serial console simultaneously

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> Previously only one serial console was supported at the same time. Using
> console=com1,dbgp,vga silently ignored all but last serial console (in
> this case: only dbgp and vga were active).
> 
> Fix this by storing not a single sercon_handle, but an array of them, up
> to MAX_SERCONS entries. The value of MAX_SERCONS (4) is arbitrary,
> inspired by the number of SERHND_IDX values.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>  xen/drivers/char/console.c | 58 ++++++++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index f9937c5134c0..44b703296487 100644
> --- 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 4

Might this want to be a Kconfig setting?

> +static int __read_mostly sercon_handle[MAX_SERCONS];
> +static int __read_mostly nr_sercon_handle = 0;

I would have wanted to ask for __ro_after_init here, but Arm still
hasn't enabled support for it.

> @@ -395,9 +397,17 @@ static unsigned int serial_rx_cons, serial_rx_prod;
>  
>  static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
>  
> +/* Redirect any console output to *fn*, if *handle* is configured as a 
> console. */
>  int console_steal(int handle, void (*fn)(const char *, size_t nr))
>  {
> -    if ( (handle == -1) || (handle != sercon_handle) )
> +    int i;

unsigned int please (also elsewhere).

> +    if ( handle == -1 )
> +        return 0;
> +    for ( i = 0; i < nr_sercon_handle; i++ )
> +        if ( handle == sercon_handle[i] )
> +            break;
> +    if ( nr_sercon_handle && i == nr_sercon_handle )
>          return 0;

No need for the left side of the &&, I think. I guess it's actively
wrong to be there.

>      if ( serial_steal_fn != NULL )

I guess you then also want to make serial_steal_fn an array, and no
longer return constant 1 from the function.

> @@ -977,7 +990,8 @@ void __init console_init_preirq(void)
>              continue;
>          else if ( (sh = serial_parse_handle(p)) >= 0 )
>          {
> -            sercon_handle = sh;
> +            if ( nr_sercon_handle < MAX_SERCONS )
> +                sercon_handle[nr_sercon_handle++] = sh;

else <log a message>

> @@ -1291,7 +1322,8 @@ static int suspend_steal_id;
>  
>  int console_suspend(void)
>  {
> -    suspend_steal_id = console_steal(sercon_handle, suspend_steal_fn);
> +    if ( nr_sercon_handle )
> +        suspend_steal_id = console_steal(sercon_handle[0], suspend_steal_fn);
>      serial_suspend();
>      return 0;
>  }

The commit message gives no explanation why only the first handle
would want/need dealing with here.

One overall remark: Especially with sync_console latency is going to
be yet worse with all output being done sequentially. The help text
for "console=" will want to mention this, up and until this would be
parallelized.

Jan