Allow multiple dom0less domains to use the console_io hypercalls to
print to the console. Handle them in a similar way to vpl011: only the
domain which has focus can read from the console. All domains can write
to the console but the ones without focus have a prefix. In this case
the prefix is applied by using guest_printk instead of printk or
console_puts which is what the original code was already doing.
When switching focus using Ctrl-AAA, discard any unread data in the
input buffer. Input is read quickly and the user would be aware of it
being slow or stuck as they use Ctrl-AAA to switch focus domain.
In that situation, it is to be expected that the unread input is lost.
The domain writes are buffered when the domain is not in focus. Push out
the buffer after the domain enters focus on the first guest write.
Locking updates:
- Guard every mutation of serial_rx_cons/prod with console_lock and
discard unread input under the lock when switching focus (including when
returning to Xen) so that cross-domain reads can't see stale data
- Require is_focus_domain() callers to hold console_lock, and take that
lock around the entire CONSOLEIO_read loop, re-checking focus after each
chunk so a focus change simply stops further copies without duplicating
or leaking input
- Hold cons->lock while flushing buffered writes in the focus path
so the direct-write fast path does not race buffered guests or HVM
debug output
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v8:
- protect console_rx read
- move printk() to last
- update "Deliver input..." comment
- in __serial_rx() use nrspin_lock_irqsave
- in do_console_io() use count = 0 to skip the loop
- in do_console_io() move nrspin_unlock_irq up a few lines
- append () to function names in in-code comments
---
xen/drivers/char/console.c | 80 ++++++++++++++++++++++++++++++++------
1 file changed, 69 insertions(+), 11 deletions(-)
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2bdb4d5fb4..ed8f1ad8f2 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -518,11 +518,16 @@ static unsigned int __read_mostly console_rx = 0;
struct domain *console_get_domain(void)
{
struct domain *d;
+ unsigned int rx;
- if ( console_rx == 0 )
+ nrspin_lock(&console_lock);
+ rx = console_rx;
+ nrspin_unlock(&console_lock);
+
+ if ( rx == 0 )
return NULL;
- d = rcu_lock_domain_by_id(console_rx - 1);
+ d = rcu_lock_domain_by_id(rx - 1);
if ( !d )
return NULL;
@@ -540,6 +545,12 @@ void console_put_domain(struct domain *d)
rcu_unlock_domain(d);
}
+static bool is_focus_domain(const struct domain *d)
+{
+ ASSERT(rspin_is_locked(&console_lock));
+ return d != NULL && d->domain_id == console_rx - 1;
+}
+
static void console_switch_input(void)
{
unsigned int next_rx = console_rx;
@@ -555,7 +566,10 @@ static void console_switch_input(void)
if ( next_rx++ >= max_console_rx )
{
+ nrspin_lock_irq(&console_lock);
console_rx = 0;
+ serial_rx_cons = serial_rx_prod;
+ nrspin_unlock_irq(&console_lock);
printk("*** Serial input to Xen");
break;
}
@@ -575,7 +589,11 @@ static void console_switch_input(void)
rcu_unlock_domain(d);
+ nrspin_lock_irq(&console_lock);
console_rx = next_rx;
+ /* Don't let the next dom read the previous dom's unread data. */
+ serial_rx_cons = serial_rx_prod;
+ nrspin_unlock_irq(&console_lock);
printk("*** Serial input to DOM%u", domid);
break;
}
@@ -601,12 +619,16 @@ static void __serial_rx(char c)
if ( is_hardware_domain(d) )
{
+ unsigned long flags;
+
/*
- * Deliver input to the hardware domain buffer, unless it is
+ * Deliver input to the focus domain buffer, unless it is
* already full.
*/
+ nrspin_lock_irqsave(&console_lock, flags);
if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
+ nrspin_unlock_irqrestore(&console_lock, flags);
/*
* Always notify the hardware domain: prevents receive path from
@@ -730,6 +752,7 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
unsigned int flags = opt_console_to_ring
? CONSOLE_ALL : CONSOLE_DEFAULT;
struct domain *cd = current->domain;
+ struct domain_console *cons = cd->console;
while ( count > 0 )
{
@@ -742,17 +765,36 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
if ( copy_from_guest(kbuf, buffer, kcount) )
return -EFAULT;
- if ( is_hardware_domain(cd) )
+ /*
+ * Take both cons->lock and console_lock:
+ * - cons->lock protects cons->buf and cons->idx
+ * - console_lock protects console_send() and is_focus_domain()
+ * checks
+ *
+ * The order must be respected. guest_printk() takes the
+ * console_lock so it is important that cons->lock is taken
+ * first.
+ */
+ spin_lock(&cons->lock);
+ nrspin_lock_irq(&console_lock);
+ if ( is_focus_domain(cd) )
{
+ if ( cons->idx )
+ {
+ console_send(cons->buf, cons->idx, flags);
+ cons->idx = 0;
+ }
+ spin_unlock(&cons->lock);
+
/* Use direct console output as it could be interactive */
- nrspin_lock_irq(&console_lock);
console_send(kbuf, kcount, flags);
nrspin_unlock_irq(&console_lock);
}
else
{
char *kin = kbuf, *kout = kbuf, c;
- struct domain_console *cons = cd->console;
+
+ nrspin_unlock_irq(&console_lock);
/* Strip non-printable characters */
do
@@ -765,7 +807,6 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
} while ( --kcount > 0 );
*kout = '\0';
- spin_lock(&cons->lock);
kcount = kin - kbuf;
if ( c != '\n' &&
(cons->idx + (kout - kbuf) < (ARRAY_SIZE(cons->buf) - 1)) )
@@ -816,22 +857,39 @@ long do_console_io(
break;
rc = 0;
+ nrspin_lock_irq(&console_lock);
+ if ( !is_focus_domain(current->domain) )
+ count = 0;
while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
{
idx = SERIAL_RX_MASK(serial_rx_cons);
len = serial_rx_prod - serial_rx_cons;
+ nrspin_unlock_irq(&console_lock);
if ( (idx + len) > SERIAL_RX_SIZE )
len = SERIAL_RX_SIZE - idx;
if ( (rc + len) > count )
len = count - rc;
if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
- {
- rc = -EFAULT;
- break;
- }
+ return -EFAULT;
rc += len;
+
+ /*
+ * First get console_lock again, then check is_focus_domain().
+ * It is possible that we switched focus domain during
+ * copy_to_guest_offset(). In that case, serial_rx_cons and
+ * serial_rx_prod have been reset so it would be wrong to
+ * update serial_rx_cons here. Get out of the loop instead.
+ *
+ * rc is updated regardless to provide the correct return
+ * value to the VM as the data has been copied.
+ */
+ nrspin_lock_irq(&console_lock);
+ if ( !is_focus_domain(current->domain) )
+ break;
+
serial_rx_cons += len;
}
+ nrspin_unlock_irq(&console_lock);
break;
default:
rc = -ENOSYS;
--
2.25.1On 29.01.2026 23:08, Stefano Stabellini wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -518,11 +518,16 @@ static unsigned int __read_mostly console_rx = 0;
> struct domain *console_get_domain(void)
> {
> struct domain *d;
> + unsigned int rx;
>
> - if ( console_rx == 0 )
> + nrspin_lock(&console_lock);
> + rx = console_rx;
> + nrspin_unlock(&console_lock);
Did you test this in a debug build, and it didn't blow up? The console lock
is an IRQ-safe one, so I'd expect check_lock() to complain that you do not
disable IRQs here. At the same time I don't think you can rely on IRQs being
off upon entry into the function.
Anyway - is locking here really needed? Wouldn't suitable use of ACCESS_ONCE()
(also elsewhere) do? (Such a switchover likely could be a separate, prereq
patch.)
Further, if already you add locking on the read sides, what about ...
> @@ -540,6 +545,12 @@ void console_put_domain(struct domain *d)
> rcu_unlock_domain(d);
> }
>
> +static bool is_focus_domain(const struct domain *d)
> +{
> + ASSERT(rspin_is_locked(&console_lock));
> + return d != NULL && d->domain_id == console_rx - 1;
> +}
> +
> static void console_switch_input(void)
> {
> unsigned int next_rx = console_rx;
... this read?
> @@ -555,7 +566,10 @@ static void console_switch_input(void)
>
> if ( next_rx++ >= max_console_rx )
> {
> + nrspin_lock_irq(&console_lock);
As indicated earlier, you can't know IRQ state in anything down the call
tree from serial_rx().
> @@ -742,17 +765,36 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> if ( copy_from_guest(kbuf, buffer, kcount) )
> return -EFAULT;
>
> - if ( is_hardware_domain(cd) )
> + /*
> + * Take both cons->lock and console_lock:
> + * - cons->lock protects cons->buf and cons->idx
> + * - console_lock protects console_send() and is_focus_domain()
> + * checks
> + *
> + * The order must be respected. guest_printk() takes the
> + * console_lock so it is important that cons->lock is taken
> + * first.
> + */
> + spin_lock(&cons->lock);
> + nrspin_lock_irq(&console_lock);
While guest_printk() does matter here, it taking (down the call tree)
console_lock isn't alone relevant. It being called with cons->lock held
is, .
> @@ -816,22 +857,39 @@ long do_console_io(
> break;
>
> rc = 0;
> + nrspin_lock_irq(&console_lock);
> + if ( !is_focus_domain(current->domain) )
> + count = 0;
> while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> {
> idx = SERIAL_RX_MASK(serial_rx_cons);
> len = serial_rx_prod - serial_rx_cons;
> + nrspin_unlock_irq(&console_lock);
Can we please have blank lines on both sides of this?
> if ( (idx + len) > SERIAL_RX_SIZE )
> len = SERIAL_RX_SIZE - idx;
> if ( (rc + len) > count )
> len = count - rc;
> if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
Have I perhaps talked you into moving the unlock too early? serial_rx_ring[]
accesses look like they need to be with the lock still held. Or, to avoid
calling copy_to_guest_offset() with the lock held, a local copy would need
making.
Jan
On Tue, 3 Feb 2026, Jan Beulich wrote:
> On 29.01.2026 23:08, Stefano Stabellini wrote:
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -518,11 +518,16 @@ static unsigned int __read_mostly console_rx = 0;
> > struct domain *console_get_domain(void)
> > {
> > struct domain *d;
> > + unsigned int rx;
> >
> > - if ( console_rx == 0 )
> > + nrspin_lock(&console_lock);
> > + rx = console_rx;
> > + nrspin_unlock(&console_lock);
>
> Did you test this in a debug build, and it didn't blow up? The console lock
> is an IRQ-safe one, so I'd expect check_lock() to complain that you do not
> disable IRQs here. At the same time I don't think you can rely on IRQs being
> off upon entry into the function.
>
> Anyway - is locking here really needed? Wouldn't suitable use of ACCESS_ONCE()
> (also elsewhere) do? (Such a switchover likely could be a separate, prereq
> patch.)
I created a prereq patch which introduces ACCESS_ONCE everywhere for
console_rx
> Further, if already you add locking on the read sides, what about ...
>
> > @@ -540,6 +545,12 @@ void console_put_domain(struct domain *d)
> > rcu_unlock_domain(d);
> > }
> >
> > +static bool is_focus_domain(const struct domain *d)
> > +{
> > + ASSERT(rspin_is_locked(&console_lock));
> > + return d != NULL && d->domain_id == console_rx - 1;
> > +}
> > +
> > static void console_switch_input(void)
> > {
> > unsigned int next_rx = console_rx;
>
> ... this read?
>
> > @@ -555,7 +566,10 @@ static void console_switch_input(void)
> >
> > if ( next_rx++ >= max_console_rx )
> > {
> > + nrspin_lock_irq(&console_lock);
>
> As indicated earlier, you can't know IRQ state in anything down the call
> tree from serial_rx().
I'll switch to the irqsave/restore versions in console_switch_input
>
> > @@ -742,17 +765,36 @@ static long guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
> > if ( copy_from_guest(kbuf, buffer, kcount) )
> > return -EFAULT;
> >
> > - if ( is_hardware_domain(cd) )
> > + /*
> > + * Take both cons->lock and console_lock:
> > + * - cons->lock protects cons->buf and cons->idx
> > + * - console_lock protects console_send() and is_focus_domain()
> > + * checks
> > + *
> > + * The order must be respected. guest_printk() takes the
> > + * console_lock so it is important that cons->lock is taken
> > + * first.
> > + */
> > + spin_lock(&cons->lock);
> > + nrspin_lock_irq(&console_lock);
>
> While guest_printk() does matter here, it taking (down the call tree)
> console_lock isn't alone relevant. It being called with cons->lock held
> is, .
I updated the in-code comment
>
> > @@ -816,22 +857,39 @@ long do_console_io(
> > break;
> >
> > rc = 0;
> > + nrspin_lock_irq(&console_lock);
> > + if ( !is_focus_domain(current->domain) )
> > + count = 0;
> > while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> > {
> > idx = SERIAL_RX_MASK(serial_rx_cons);
> > len = serial_rx_prod - serial_rx_cons;
> > + nrspin_unlock_irq(&console_lock);
>
> Can we please have blank lines on both sides of this?
sure
> > if ( (idx + len) > SERIAL_RX_SIZE )
> > len = SERIAL_RX_SIZE - idx;
> > if ( (rc + len) > count )
> > len = count - rc;
> > if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
>
> Have I perhaps talked you into moving the unlock too early? serial_rx_ring[]
> accesses look like they need to be with the lock still held. Or, to avoid
> calling copy_to_guest_offset() with the lock held, a local copy would need
> making.
I introduced a local copy
On 04.02.2026 00:02, Stefano Stabellini wrote:
> On Tue, 3 Feb 2026, Jan Beulich wrote:
>> On 29.01.2026 23:08, Stefano Stabellini wrote:
>>> @@ -555,7 +566,10 @@ static void console_switch_input(void)
>>>
>>> if ( next_rx++ >= max_console_rx )
>>> {
>>> + nrspin_lock_irq(&console_lock);
>>
>> As indicated earlier, you can't know IRQ state in anything down the call
>> tree from serial_rx().
>
> I'll switch to the irqsave/restore versions in console_switch_input
I've seen that you already sent v9, but seeing how getting the locking right
has proven to be difficult, I have two more remarks towards this.
1) Can the locking additions to existing code please be split out into a
separate patch?
2) As all of this is for dom0less only (for now at least), did you consider
to make all of these changes dependent upon a new Kconfig option, so to
avoid impacting other environments in case issues remain when this has
gone in?
Jan
On Wed, 4 Feb 2026, Jan Beulich wrote:
> On 04.02.2026 00:02, Stefano Stabellini wrote:
> > On Tue, 3 Feb 2026, Jan Beulich wrote:
> >> On 29.01.2026 23:08, Stefano Stabellini wrote:
> >>> @@ -555,7 +566,10 @@ static void console_switch_input(void)
> >>>
> >>> if ( next_rx++ >= max_console_rx )
> >>> {
> >>> + nrspin_lock_irq(&console_lock);
> >>
> >> As indicated earlier, you can't know IRQ state in anything down the call
> >> tree from serial_rx().
> >
> > I'll switch to the irqsave/restore versions in console_switch_input
>
> I've seen that you already sent v9, but seeing how getting the locking right
> has proven to be difficult, I have two more remarks towards this.
>
> 1) Can the locking additions to existing code please be split out into a
> separate patch?
I did this
> 2) As all of this is for dom0less only (for now at least), did you consider
> to make all of these changes dependent upon a new Kconfig option, so to
> avoid impacting other environments in case issues remain when this has
> gone in?
Effectively it is already the case because:
#define max_console_rx (max_init_domid + 1)
I could easily add an #ifdef around is_focus_domain() so that in case
CONFIG_DOM0LESS_BOOT is disabled it defaults to is_hardware_domain() but
that wouldn't really help because thanks to the definition of
max_console_rx, effectively it works the same way when
CONFIG_DOM0LESS_BOOT is disabled.
I think what you are asking would be more about the locking changes in
guest_console_write, but for those I cannot really use #ifdef to retain
the current position of the locks. The resulting code would not be good.
So I decided to not make any changes in this regard. However, I am happy
to #ifdef is_focus_domain() although as I said I don't think it would
bring much value.
On 05.02.2026 00:30, Stefano Stabellini wrote: > On Wed, 4 Feb 2026, Jan Beulich wrote: >> 2) As all of this is for dom0less only (for now at least), did you consider >> to make all of these changes dependent upon a new Kconfig option, so to >> avoid impacting other environments in case issues remain when this has >> gone in? > > Effectively it is already the case because: > > #define max_console_rx (max_init_domid + 1) > > I could easily add an #ifdef around is_focus_domain() so that in case > CONFIG_DOM0LESS_BOOT is disabled it defaults to is_hardware_domain() but > that wouldn't really help because thanks to the definition of > max_console_rx, effectively it works the same way when > CONFIG_DOM0LESS_BOOT is disabled. > > I think what you are asking would be more about the locking changes in > guest_console_write, but for those I cannot really use #ifdef to retain > the current position of the locks. The resulting code would not be good. Yes, the request was (primarily) about the locking changes (additions!), as these can affect non-dom0less as well. I may try to make suggestions when looking at the next version. Jan
© 2016 - 2026 Red Hat, Inc.