| Message ID | e40a6de7f032c776e889e4ca6d68579fbb3ad57a.1657121519.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
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@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
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@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? 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?
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
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 +static int __read_mostly sercon_handle[MAX_SERCONS]; +static int __read_mostly nr_sercon_handle = 0; #ifdef CONFIG_X86 /* Tristate: 0 disabled, 1 user enabled, -1 default enabled */ @@ -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; + + 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; if ( serial_steal_fn != NULL ) @@ -415,10 +425,13 @@ void console_giveback(int id) void console_serial_puts(const char *s, size_t nr) { + int i; + if ( serial_steal_fn != NULL ) serial_steal_fn(s, nr); else - serial_puts(sercon_handle, s, nr); + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_puts(sercon_handle[i], s, nr); /* Copy all serial output into PV console */ pv_console_puts(s, nr); @@ -956,7 +969,7 @@ void guest_printk(const struct domain *d, const char *fmt, ...) void __init console_init_preirq(void) { char *p; - int sh; + int sh, i; serial_init_preirq(); @@ -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; serial_steal_fn = NULL; } else @@ -996,7 +1010,8 @@ void __init console_init_preirq(void) opt_console_xen = 0; #endif - serial_set_rx_handler(sercon_handle, serial_rx); + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_set_rx_handler(sercon_handle[i], serial_rx); pv_console_set_rx_handler(serial_rx); /* HELLO WORLD --- start-of-day banner text. */ @@ -1014,7 +1029,8 @@ void __init console_init_preirq(void) if ( opt_sync_console ) { - serial_start_sync(sercon_handle); + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_start_sync(sercon_handle[i]); add_taint(TAINT_SYNC_CONSOLE); printk("Console output is synchronous.\n"); warning_add(warning_sync_console); @@ -1121,13 +1137,19 @@ int __init console_has(const char *device) void console_start_log_everything(void) { - serial_start_log_everything(sercon_handle); + int i; + + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_start_log_everything(sercon_handle[i]); atomic_inc(&print_everything); } void console_end_log_everything(void) { - serial_end_log_everything(sercon_handle); + int i; + + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_end_log_everything(sercon_handle[i]); atomic_dec(&print_everything); } @@ -1149,23 +1171,32 @@ void console_unlock_recursive_irqrestore(unsigned long flags) void console_force_unlock(void) { + int i; + watchdog_disable(); spin_debug_disable(); spin_lock_init(&console_lock); - serial_force_unlock(sercon_handle); + for ( i = 0 ; i < nr_sercon_handle ; i++ ) + serial_force_unlock(sercon_handle[i]); console_locks_busted = 1; console_start_sync(); } void console_start_sync(void) { + int i; + atomic_inc(&print_everything); - serial_start_sync(sercon_handle); + for ( i = 0 ; i < nr_sercon_handle ; i++ ) + serial_start_sync(sercon_handle[i]); } void console_end_sync(void) { - serial_end_sync(sercon_handle); + int i; + + for ( i = 0; i < nr_sercon_handle; i++ ) + serial_end_sync(sercon_handle[i]); atomic_dec(&print_everything); } @@ -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; }
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@invisiblethingslab.com> --- xen/drivers/char/console.c | 58 ++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 13 deletions(-)