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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Jul 13, 2022 at 11:39:07AM +0200, Jan Beulich wrote:
> 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?

Is that going to be useful for anybody (who isn't modifying the code
anyway, for example to add yet another console driver)?

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

Sure, I can add an explanation. I'm adding this comment to console_steal():
/* Redirect any console output to *fn*, if *handle* is configured as a console. 
*/

So, calling console_steal() is about all serial consoles, not just a
specific one. The use case for this "if" part is gdbstub, which wants
to redirect serial output only if that serial was configured as both
console and gdb. Having proper per-console stealing is doable, but IMO
not worth it (it would require also avoiding duplicated output in case
of multiple serial consoles, and probably few more corner cases).

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

I don't think it was parallelized anywhere. All the relevant functions
(__putstr especially) write to various console types sequentially. The
difference is that previously only the last "serial" console was used,
all the other were silently ignored. So, it was "parallel" with all
_zero other_ serial consoles, but not other console types.
Anyway, both help text and boot warning for sync_console already warn
about it. Do you want me to include relation to number of configured
console explicitly?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature