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 <marma...@invisiblethingslab.com>
> ---
> 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
