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.
Move the vpl011 check first so that if vpl011 is enabled, it will be
used. In the future, the is_hardware_domain path might also be used by
other predefined domains so it makes sense to prioritize vpl011 to
retain the current behavior on ARM.
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 cd->pbuf_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 v6:
- moved locks out of console_get/put_domain
- reworked locking from the ground up
- introduced is_focus_domain
- improved commit message
---
xen/drivers/char/console.c | 64 +++++++++++++++++++++++++++++---------
1 file changed, 49 insertions(+), 15 deletions(-)
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2bdb4d5fb4..d586572c5e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -540,6 +540,11 @@ void console_put_domain(struct domain *d)
rcu_unlock_domain(d);
}
+static bool is_focus_domain(struct domain *d)
+{
+ return d != NULL && d->domain_id == console_rx - 1;
+}
+
static void console_switch_input(void)
{
unsigned int next_rx = console_rx;
@@ -555,8 +560,11 @@ static void console_switch_input(void)
if ( next_rx++ >= max_console_rx )
{
- console_rx = 0;
printk("*** Serial input to Xen");
+ nrspin_lock_irq(&console_lock);
+ console_rx = 0;
+ serial_rx_cons = serial_rx_prod;
+ nrspin_unlock_irq(&console_lock);
break;
}
@@ -575,8 +583,12 @@ static void console_switch_input(void)
rcu_unlock_domain(d);
- console_rx = next_rx;
printk("*** Serial input to DOM%u", domid);
+ 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);
break;
}
}
@@ -599,14 +611,25 @@ static void __serial_rx(char c)
if ( !d )
return;
+#ifdef CONFIG_SBSA_VUART_CONSOLE
+ /* Prioritize vpl011 if enabled for this domain */
+ if ( d->arch.vpl011.base_addr )
+ {
+ /* Deliver input to the emulated UART. */
+ rc = vpl011_rx_char_xen(d, c);
+ }
+ else
+#endif
if ( is_hardware_domain(d) )
{
/*
* Deliver input to the hardware domain buffer, unless it is
* already full.
*/
+ nrspin_lock_irq(&console_lock);
if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
+ nrspin_unlock_irq(&console_lock);
/*
* Always notify the hardware domain: prevents receive path from
@@ -614,11 +637,6 @@ static void __serial_rx(char c)
*/
send_global_virq(VIRQ_CONSOLE);
}
-#ifdef CONFIG_SBSA_VUART_CONSOLE
- else
- /* Deliver input to the emulated UART. */
- rc = vpl011_rx_char_xen(d, c);
-#endif
if ( consoled_is_enabled() )
/* Deliver input to the PV shim console. */
@@ -730,6 +748,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,18 +761,25 @@ 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) )
+ 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;
+ }
/* Use direct console output as it could be interactive */
- nrspin_lock_irq(&console_lock);
console_send(kbuf, kcount, flags);
nrspin_unlock_irq(&console_lock);
+ spin_unlock(&cons->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 +791,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)) )
@@ -815,6 +840,13 @@ long do_console_io(
if ( count > INT_MAX )
break;
+ nrspin_lock_irq(&console_lock);
+ if ( !is_focus_domain(current->domain) )
+ {
+ nrspin_unlock_irq(&console_lock);
+ return 0;
+ }
+
rc = 0;
while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
{
@@ -824,14 +856,16 @@ long do_console_io(
len = SERIAL_RX_SIZE - idx;
if ( (rc + len) > count )
len = count - rc;
+ nrspin_unlock_irq(&console_lock);
if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
- {
- rc = -EFAULT;
- break;
- }
+ return -EFAULT;
rc += len;
+ 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 22.01.2026 02:34, Stefano Stabellini wrote:
> 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.
>
> Move the vpl011 check first so that if vpl011 is enabled, it will be
> used. In the future, the is_hardware_domain path might also be used by
> other predefined domains so it makes sense to prioritize vpl011 to
> retain the current behavior on ARM.
As indicated elsewhere already, I think this wants moving out of here.
A question is going to be whether the consoled part then also wants
moving ahead (albeit I fear for now I don't really understand the need
for this movement, seeing that the is_hardware_domain() check there
remains as is).
> 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
Shouldn't this then ...
> - Hold cd->pbuf_lock while flushing buffered writes in the focus path
> so the direct-write fast path does not race buffered guests or HVM
> debug output
(What's ->pbuf_lock again?)
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -540,6 +540,11 @@ void console_put_domain(struct domain *d)
> rcu_unlock_domain(d);
> }
>
> +static bool is_focus_domain(struct domain *d)
> +{
> + return d != NULL && d->domain_id == console_rx - 1;
> +}
... be expressed in a comment here as well (or even an assertion)?
Also please make the function parameter pointer-to-const.
> @@ -599,14 +611,25 @@ static void __serial_rx(char c)
> if ( !d )
> return;
>
> +#ifdef CONFIG_SBSA_VUART_CONSOLE
> + /* Prioritize vpl011 if enabled for this domain */
> + if ( d->arch.vpl011.base_addr )
> + {
> + /* Deliver input to the emulated UART. */
> + rc = vpl011_rx_char_xen(d, c);
> + }
> + else
> +#endif
> if ( is_hardware_domain(d) )
> {
> /*
> * Deliver input to the hardware domain buffer, unless it is
> * already full.
> */
> + nrspin_lock_irq(&console_lock);
> if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> + nrspin_unlock_irq(&console_lock);
Hmm, now that there's more context here: The comment looks to still be
correct as per the enclosing if(), but how does that fit with the purpose
of this patch? Isn't it part of the goal to allow input to non-hwdom as
well?
> @@ -742,18 +761,25 @@ 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) )
> + spin_lock(&cons->lock);
> + nrspin_lock_irq(&console_lock);
This double lock (and the need for this specific order) might better be
commented upon, too.
> + if ( is_focus_domain(cd) )
> {
> + if ( cons->idx )
> + {
> + console_send(cons->buf, cons->idx, flags);
> + cons->idx = 0;
> + }
> /* Use direct console output as it could be interactive */
> - nrspin_lock_irq(&console_lock);
> console_send(kbuf, kcount, flags);
> nrspin_unlock_irq(&console_lock);
> + spin_unlock(&cons->lock);
Why is it that this lock can be dropped only here? It's not needed anymore
past the if()'s body?
> }
> else
> {
> char *kin = kbuf, *kout = kbuf, c;
> - struct domain_console *cons = cd->console;
>
> + nrspin_unlock_irq(&console_lock);
> /* Strip non-printable characters */
Blank line between these?
> @@ -824,14 +856,16 @@ long do_console_io(
> len = SERIAL_RX_SIZE - idx;
> if ( (rc + len) > count )
> len = count - rc;
> + nrspin_unlock_irq(&console_lock);
> if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
> - {
> - rc = -EFAULT;
> - break;
> - }
> + return -EFAULT;
> rc += len;
> + nrspin_lock_irq(&console_lock);
> + if ( !is_focus_domain(current->domain) )
> + break;
> serial_rx_cons += len;
The placement of the check between both updates wants commenting upon, imo.
Another blank line or two would also help, I think.
> }
> + nrspin_unlock_irq(&console_lock);
> break;
> default:
> rc = -ENOSYS;
Much better locking-wise here than in the earlier version.
Jan
On Thu, 22 Jan 2026, Jan Beulich wrote:
> On 22.01.2026 02:34, Stefano Stabellini wrote:
> > 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.
> >
> > Move the vpl011 check first so that if vpl011 is enabled, it will be
> > used. In the future, the is_hardware_domain path might also be used by
> > other predefined domains so it makes sense to prioritize vpl011 to
> > retain the current behavior on ARM.
>
> As indicated elsewhere already, I think this wants moving out of here.
> A question is going to be whether the consoled part then also wants
> moving ahead (albeit I fear for now I don't really understand the need
> for this movement, seeing that the is_hardware_domain() check there
> remains as is).
You are right that the is_hardware_domain() check is now wrong. The
reason I didn't notice before is that I was testing on a branch with a
more complete hyperlaunch implementation where the check was different.
Without the vpl011 hunk the patch is not functional but the change can
go into a separate patch. The separate patch also needs to do the
following:
- get rid of the is_hardware_domain() check
- set input_allowed for dom0less guests to make use of console_io
With this, everything works properly upstream now.
> > 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
>
> Shouldn't this then ...
>
> > - Hold cd->pbuf_lock while flushing buffered writes in the focus path
> > so the direct-write fast path does not race buffered guests or HVM
> > debug output
>
> (What's ->pbuf_lock again?)
I updated the commit message. It is cons->lock.
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -540,6 +540,11 @@ void console_put_domain(struct domain *d)
> > rcu_unlock_domain(d);
> > }
> >
> > +static bool is_focus_domain(struct domain *d)
> > +{
> > + return d != NULL && d->domain_id == console_rx - 1;
> > +}
>
> ... be expressed in a comment here as well (or even an assertion)?
>
> Also please make the function parameter pointer-to-const.
Yes and yes
> > @@ -599,14 +611,25 @@ static void __serial_rx(char c)
> > if ( !d )
> > return;
> >
> > +#ifdef CONFIG_SBSA_VUART_CONSOLE
> > + /* Prioritize vpl011 if enabled for this domain */
> > + if ( d->arch.vpl011.base_addr )
> > + {
> > + /* Deliver input to the emulated UART. */
> > + rc = vpl011_rx_char_xen(d, c);
> > + }
> > + else
> > +#endif
> > if ( is_hardware_domain(d) )
> > {
> > /*
> > * Deliver input to the hardware domain buffer, unless it is
> > * already full.
> > */
> > + nrspin_lock_irq(&console_lock);
> > if ( (serial_rx_prod - serial_rx_cons) != SERIAL_RX_SIZE )
> > serial_rx_ring[SERIAL_RX_MASK(serial_rx_prod++)] = c;
> > + nrspin_unlock_irq(&console_lock);
>
> Hmm, now that there's more context here: The comment looks to still be
> correct as per the enclosing if(), but how does that fit with the purpose
> of this patch? Isn't it part of the goal to allow input to non-hwdom as
> well?
I updated the comment (in the second patch). It should say focus domain
now, instead of hardware domain.
> > @@ -742,18 +761,25 @@ 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) )
> > + spin_lock(&cons->lock);
> > + nrspin_lock_irq(&console_lock);
>
> This double lock (and the need for this specific order) might better be
> commented upon, too.
+1
> > + if ( is_focus_domain(cd) )
> > {
> > + if ( cons->idx )
> > + {
> > + console_send(cons->buf, cons->idx, flags);
> > + cons->idx = 0;
> > + }
> > /* Use direct console output as it could be interactive */
> > - nrspin_lock_irq(&console_lock);
> > console_send(kbuf, kcount, flags);
> > nrspin_unlock_irq(&console_lock);
> > + spin_unlock(&cons->lock);
>
> Why is it that this lock can be dropped only here? It's not needed anymore
> past the if()'s body?
Yes, you are technically correct, but it is easier to understand lock if
they are always taken in order and released in the opposite order:
A-B [...] B-A
That said, I couldn't find anything wrong in this case with moving the
cons->unlock just after the if, so I did as you suggested.
> > }
> > else
> > {
> > char *kin = kbuf, *kout = kbuf, c;
> > - struct domain_console *cons = cd->console;
> >
> > + nrspin_unlock_irq(&console_lock);
> > /* Strip non-printable characters */
>
> Blank line between these?
Yes
> > @@ -824,14 +856,16 @@ long do_console_io(
> > len = SERIAL_RX_SIZE - idx;
> > if ( (rc + len) > count )
> > len = count - rc;
> > + nrspin_unlock_irq(&console_lock);
> > if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
> > - {
> > - rc = -EFAULT;
> > - break;
> > - }
> > + return -EFAULT;
> > rc += len;
> > + nrspin_lock_irq(&console_lock);
> > + if ( !is_focus_domain(current->domain) )
> > + break;
> > serial_rx_cons += len;
>
> The placement of the check between both updates wants commenting upon, imo.
> Another blank line or two would also help, I think.
OK
> > }
> > + nrspin_unlock_irq(&console_lock);
> > break;
> > default:
> > rc = -ENOSYS;
>
> Much better locking-wise here than in the earlier version.
Thanks! Next version is here:
https://marc.info/?l=xen-devel&m=176913312213329
© 2016 - 2026 Red Hat, Inc.