| Message ID | 14411aa674b61d22d9626a3455206454793b6a37.1658804819.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: > --- a/xen/drivers/char/Kconfig > +++ b/xen/drivers/char/Kconfig > @@ -85,6 +85,17 @@ config SERIAL_TX_BUFSIZE > > Default value is 16384 (16kiB). > > +config MAX_SERCONS > + int "Maximum number of serial consoles active at once" > + default 4 > + help > + Controls how many serial consoles can be active at once. Configuring more > + using `console=` parameter will be ignored. > + When multiple consoles are configured, overhead of `sync_console` option > + is even bigger. > + > + Default value is 4. Indentation (the help text ought to be indented by a tab and two spaces). > --- 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 CONFIG_MAX_SERCONS > +static int __read_mostly sercon_handle[MAX_SERCONS]; > +static int __read_mostly nr_sercon_handle = 0; unsigned int please. > @@ -393,32 +395,59 @@ long read_console_ring(struct xen_sysctl_readconsole *op) > static char serial_rx_ring[SERIAL_RX_SIZE]; > static unsigned int serial_rx_cons, serial_rx_prod; > > -static void (*serial_steal_fn)(const char *, size_t nr) = early_puts; > +/* The last entry means "steal from all consoles" */ > +static void (*serial_steal_fn[MAX_SERCONS+1])(const char *, size_t nr) = { Nit: blanks please around + . But with ... > + [MAX_SERCONS] = early_puts, ... this initializer you could as well omit the dimension. > +}; > > +/* > + * Redirect console *handle* output to *fn*. Use SERHND_STEAL_ALL as *handle* to > + * redirect all the consoles. > + */ > int console_steal(int handle, void (*fn)(const char *, size_t nr)) > { > - if ( (handle == -1) || (handle != sercon_handle) ) > - return 0; > + int i; While from the use inside the function this would better be unsigned int, I can see how that would be slightly odd with the return value. But with overly high a MAX_SERCONS ... > + if ( handle == -1 ) > + return -ENOENT; > + if ( serial_steal_fn[MAX_SERCONS] != NULL ) > + return -EBUSY; > + if ( handle == SERHND_STEAL_ALL ) > + { > + serial_steal_fn[MAX_SERCONS] = fn; > + return MAX_SERCONS; > + } > + for ( i = 0; i < nr_sercon_handle; i++ ) > + if ( handle == sercon_handle[i] ) ... the array access will degenerate when i is "int", but will be okay when i is "unsigned int" (and of course everything will break if MAX_SERCONS > UINT_MAX). > + break; > + if ( i == nr_sercon_handle ) > + return -ENOENT; > > - if ( serial_steal_fn != NULL ) > + if ( serial_steal_fn[i] != NULL ) > return -EBUSY; > > - serial_steal_fn = fn; > - return 1; > + serial_steal_fn[i] = fn; > + return i; > } > > void console_giveback(int id) > { > - if ( id == 1 ) > - serial_steal_fn = NULL; > + if ( id >= 0 && id <= MAX_SERCONS ) > + serial_steal_fn[id] = NULL; > } > > void console_serial_puts(const char *s, size_t nr) > { > - if ( serial_steal_fn != NULL ) > - serial_steal_fn(s, nr); > + int i; unsigned int please, again (yet more further down). > + if ( serial_steal_fn[MAX_SERCONS] != NULL ) > + serial_steal_fn[MAX_SERCONS](s, nr); > else > - serial_puts(sercon_handle, s, nr); > + for ( i = 0; i < nr_sercon_handle; i++ ) > + if ( serial_steal_fn[i] != NULL ) > + serial_steal_fn[i](s, nr); > + else > + serial_puts(sercon_handle[i], s, nr); This for() would better gain braces. > @@ -977,8 +1006,12 @@ void __init console_init_preirq(void) > continue; > else if ( (sh = serial_parse_handle(p)) >= 0 ) > { > - sercon_handle = sh; > - serial_steal_fn = NULL; > + if ( nr_sercon_handle < MAX_SERCONS ) > + sercon_handle[nr_sercon_handle++] = sh; > + else > + printk("Too many consoles (max %d), ignoring '%s'\n", > + MAX_SERCONS, p); In order to use p here, I think we want (need?) to make serial_parse_handle()'s parameter const char *. > --- a/xen/drivers/char/xhci-dbc.c > +++ b/xen/drivers/char/xhci-dbc.c > @@ -1078,8 +1078,12 @@ void __init xhci_dbc_uart_init(void) > > e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func); > if ( !e || *e ) > + { > + printk(XENLOG_ERR > + "Invalid dbgp= PCI device spec: '%s'\n", > + opt_dbgp); > return; > - > + } > dbc->sbdf = PCI_SBDF(0, bus, slot, func); > } Does this change belong elsewhere? Jan
On 26.07.2022 05:23, 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 can be chosen in > kconfig, the default (4) is arbitrary, inspired by the number of > SERHND_IDX values. > > Make console_steal() aware of multiple consoles too. It can now either > steal output from specific console (for gdbstub), or from all of them at > once (for console suspend). > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Actually I should have clarified yesterday that despite my effort to review this change, I'm not convinced of its need. I simply don't see the use case of having multiple serial consoles at a time, and afaict console and gdb connection can already be run over different channels. Jan
On Fri, Aug 05, 2022 at 09:41:09AM +0200, Jan Beulich wrote: > On 26.07.2022 05:23, 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 can be chosen in > > kconfig, the default (4) is arbitrary, inspired by the number of > > SERHND_IDX values. > > > > Make console_steal() aware of multiple consoles too. It can now either > > steal output from specific console (for gdbstub), or from all of them at > > once (for console suspend). > > > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > > Actually I should have clarified yesterday that despite my effort to > review this change, I'm not convinced of its need. I simply don't see > the use case of having multiple serial consoles at a time, and afaict > console and gdb connection can already be run over different channels. I agree the usefulness is limited. I needed this only to debug the xhci console driver itself. But since I did this change already, I figured I'd share it as it might be useful for others in similar situation.
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index d594bd5c7436..e53efdb324b3 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -433,6 +433,9 @@ only available when used together with `pv-in-pvh`. `none` indicates that Xen should not use a console. This option only makes sense on its own. +Specifying more than one serial console will increase console latency, +especially when `sync_console` option is used. + ### console_timestamps > `= none | date | datems | boot | raw` @@ -2372,6 +2375,7 @@ vulnerabilities. Flag to force synchronous console output. Useful for debugging, but not suitable for production environments due to incurred overhead. +If multiple consoles are configured, the incurred overhead is even bigger. ### tboot (x86) > `= 0x<phys_addr>` diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig index 06350c387371..1010436d245c 100644 --- a/xen/drivers/char/Kconfig +++ b/xen/drivers/char/Kconfig @@ -85,6 +85,17 @@ config SERIAL_TX_BUFSIZE Default value is 16384 (16kiB). +config MAX_SERCONS + int "Maximum number of serial consoles active at once" + default 4 + help + Controls how many serial consoles can be active at once. Configuring more + using `console=` parameter will be ignored. + When multiple consoles are configured, overhead of `sync_console` option + is even bigger. + + Default value is 4. + config XHCI bool "XHCI DbC UART driver" depends on X86 diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c index f9937c5134c0..2ffc919445c6 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 CONFIG_MAX_SERCONS +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 */ @@ -393,32 +395,59 @@ long read_console_ring(struct xen_sysctl_readconsole *op) static char serial_rx_ring[SERIAL_RX_SIZE]; static unsigned int serial_rx_cons, serial_rx_prod; -static void (*serial_steal_fn)(const char *, size_t nr) = early_puts; +/* The last entry means "steal from all consoles" */ +static void (*serial_steal_fn[MAX_SERCONS+1])(const char *, size_t nr) = { + [MAX_SERCONS] = early_puts, +}; +/* + * Redirect console *handle* output to *fn*. Use SERHND_STEAL_ALL as *handle* to + * redirect all the consoles. + */ int console_steal(int handle, void (*fn)(const char *, size_t nr)) { - if ( (handle == -1) || (handle != sercon_handle) ) - return 0; + int i; + + if ( handle == -1 ) + return -ENOENT; + if ( serial_steal_fn[MAX_SERCONS] != NULL ) + return -EBUSY; + if ( handle == SERHND_STEAL_ALL ) + { + serial_steal_fn[MAX_SERCONS] = fn; + return MAX_SERCONS; + } + for ( i = 0; i < nr_sercon_handle; i++ ) + if ( handle == sercon_handle[i] ) + break; + if ( i == nr_sercon_handle ) + return -ENOENT; - if ( serial_steal_fn != NULL ) + if ( serial_steal_fn[i] != NULL ) return -EBUSY; - serial_steal_fn = fn; - return 1; + serial_steal_fn[i] = fn; + return i; } void console_giveback(int id) { - if ( id == 1 ) - serial_steal_fn = NULL; + if ( id >= 0 && id <= MAX_SERCONS ) + serial_steal_fn[id] = NULL; } void console_serial_puts(const char *s, size_t nr) { - if ( serial_steal_fn != NULL ) - serial_steal_fn(s, nr); + int i; + + if ( serial_steal_fn[MAX_SERCONS] != NULL ) + serial_steal_fn[MAX_SERCONS](s, nr); else - serial_puts(sercon_handle, s, nr); + for ( i = 0; i < nr_sercon_handle; i++ ) + if ( serial_steal_fn[i] != NULL ) + serial_steal_fn[i](s, nr); + else + serial_puts(sercon_handle[i], s, nr); /* Copy all serial output into PV console */ pv_console_puts(s, nr); @@ -956,7 +985,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,8 +1006,12 @@ void __init console_init_preirq(void) continue; else if ( (sh = serial_parse_handle(p)) >= 0 ) { - sercon_handle = sh; - serial_steal_fn = NULL; + if ( nr_sercon_handle < MAX_SERCONS ) + sercon_handle[nr_sercon_handle++] = sh; + else + printk("Too many consoles (max %d), ignoring '%s'\n", + MAX_SERCONS, p); + serial_steal_fn[MAX_SERCONS] = NULL; } else { @@ -996,7 +1029,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 +1048,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 +1156,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 +1190,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 +1341,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(SERHND_STEAL_ALL, suspend_steal_fn); serial_suspend(); return 0; } diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index 11026d3b71f0..14a2d3eb0ee2 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -1078,8 +1078,12 @@ void __init xhci_dbc_uart_init(void) e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func); if ( !e || *e ) + { + printk(XENLOG_ERR + "Invalid dbgp= PCI device spec: '%s'\n", + opt_dbgp); return; - + } dbc->sbdf = PCI_SBDF(0, bus, slot, func); } diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h index 181e026967bc..c3bca000e238 100644 --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -99,6 +99,7 @@ struct uart_driver { #define SERHND_HI (1<<2) /* Mux/demux each transferred char by MSB. */ #define SERHND_LO (1<<3) /* Ditto, except that the MSB is cleared. */ #define SERHND_COOKED (1<<4) /* Newline/carriage-return translation? */ +#define SERHND_STEAL_ALL 0xff /* Synthetic handle used in console_steal() */ /* Three-stage initialisation (before/during/after IRQ-subsystem setup). */ void serial_init_preirq(void);
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 can be chosen in kconfig, the default (4) is arbitrary, inspired by the number of SERHND_IDX values. Make console_steal() aware of multiple consoles too. It can now either steal output from specific console (for gdbstub), or from all of them at once (for console suspend). Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v3: - adjust console_steal() for multiple consoles too - add MAX_SERCONS to kconfig - add warning about sync_console impact - add warning if too many consoles are configured - log issue with PCI spec parsing --- docs/misc/xen-command-line.pandoc | 4 +- xen/drivers/char/Kconfig | 11 ++++- xen/drivers/char/console.c | 97 ++++++++++++++++++++++++-------- xen/drivers/char/xhci-dbc.c | 6 +- xen/include/xen/serial.h | 1 +- 5 files changed, 95 insertions(+), 24 deletions(-)