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(-)
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);
--
git-series 0.9.1
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. -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab
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
© 2016 - 2026 Red Hat, Inc.