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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.07.2022 14:48, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 13, 2022 at 11:39:07AM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> --- 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)?

If allowing multiple serial consoles is deemed useful, then making
their maximum count build-time configurable is quite likely useful.
People may not want to allow multiple of them, for example.

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

And what if the one handle you pass on isn't the one matching the
console the gdbstub is using? While I understand that per-console
stealing may have some sharp edges, I don't currently see how we can
get away here without handling things per-console.

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

Parallelizing vga and serial likely wasn't deemed very useful, as
vga has negligible latency compared to a (slow) serial line (albeit
I leave aside software scrolling here, which indeed is slow). There
are also no commands involved in vga output which may require waiting
for their completion - it's all simple MMIO writes (and hence the
slowness of scrolling could only be dealt with by involving a 2nd
CPU, as the one doing the scrolling can't at the same time do output
to another device; nevertheless some of the latency could be
compensated by doing output in suitable order). This is quite
different when it comes to multiple serial consoles.

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

I think it should be made explicit, yes.

Jan