xen/drivers/char/console.c | 30 +++++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-)
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 when the domain enters focus.
Add the console_lock around serial_rx_cons modifications to protect it
against concurrent writes to it.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
Changes in v3:
- move serial_rx_cons before printk
- call console_put_domain earlier on CONSOLEIO_read
- take console_lock earlier
- add console_lock around serial_rx_cons modifications
Changes in v2:
- fix code style
- pbuf_idx/idx after ada53067083e
- don't add extra \0
- clear input on console switch
---
xen/drivers/char/console.c | 30 +++++++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 2bdb4d5fb4..58c32f22ef 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -577,6 +577,10 @@ static void console_switch_input(void)
console_rx = next_rx;
printk("*** Serial input to DOM%u", domid);
+ /* Don't let the next dom read the previous dom's unread data. */
+ nrspin_lock_irq(&console_lock);
+ serial_rx_cons = serial_rx_prod;
+ nrspin_unlock_irq(&console_lock);
break;
}
}
@@ -730,6 +734,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 *input;
while ( count > 0 )
{
@@ -742,18 +747,28 @@ 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) )
+ input = console_get_domain();
+ if ( input && cd == input )
{
- /* Use direct console output as it could be interactive */
+ struct domain_console *cons = cd->console;
+
nrspin_lock_irq(&console_lock);
+ if ( cons->idx )
+ {
+ console_send(cons->buf, cons->idx, flags);
+ cons->idx = 0;
+ }
+ /* Use direct console output as it could be interactive */
console_send(kbuf, kcount, flags);
nrspin_unlock_irq(&console_lock);
+ console_put_domain(input);
}
else
{
char *kin = kbuf, *kout = kbuf, c;
struct domain_console *cons = cd->console;
+ console_put_domain(input);
/* Strip non-printable characters */
do
{
@@ -795,6 +810,7 @@ long do_console_io(
{
long rc;
unsigned int idx, len;
+ struct domain *d;
rc = xsm_console_io(XSM_OTHER, current->domain, cmd);
if ( rc )
@@ -815,6 +831,11 @@ long do_console_io(
if ( count > INT_MAX )
break;
+ d = console_get_domain();
+ console_put_domain(d);
+ if ( d != current->domain )
+ return 0;
+
rc = 0;
while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
{
@@ -830,7 +851,10 @@ long do_console_io(
break;
}
rc += len;
- serial_rx_cons += len;
+ nrspin_lock_irq(&console_lock);
+ if ( serial_rx_cons != serial_rx_prod )
+ serial_rx_cons += len;
+ nrspin_unlock_irq(&console_lock);
}
break;
default:
--
2.25.1On 14.01.2026 01:39, Stefano Stabellini wrote:
> @@ -815,6 +831,11 @@ long do_console_io(
> if ( count > INT_MAX )
> break;
>
> + d = console_get_domain();
> + console_put_domain(d);
> + if ( d != current->domain )
> + return 0;
This isn't atomic (as in: in a suitably locked region) with ...
> @@ -830,7 +851,10 @@ long do_console_io(
> break;
> }
> rc += len;
> - serial_rx_cons += len;
> + nrspin_lock_irq(&console_lock);
> + if ( serial_rx_cons != serial_rx_prod )
> + serial_rx_cons += len;
> + nrspin_unlock_irq(&console_lock);
> }
> break;
... this. If the focus domain changes after the check in the earlier hunk,
I think you need to also return with no input here. Or you need to acquire
the lock earlier (and then similarly in console_switch_input()), albeit
that would then mean holding it across a copy-to-guest. Which technically
is perhaps not a problem, but it leaves an uneasy feeling.
In no case may you continue the loop if the focus domain changed, or else
you potentially hand the old one input targeted at the new one.
For context: What caught my attention is the conditional inside the locked
region. This, imo, shouldn't be necessary when everything else is properly
structured.
Actually, the lock strictly needs holding now across all accesses to
serial_rx_{cons,prod}. That'll then naturally make the conditional
unnecessary.
Jan
On Mon, 19 Jan 2026, Jan Beulich wrote:
> On 14.01.2026 01:39, Stefano Stabellini wrote:
> > @@ -815,6 +831,11 @@ long do_console_io(
> > if ( count > INT_MAX )
> > break;
> >
> > + d = console_get_domain();
> > + console_put_domain(d);
> > + if ( d != current->domain )
> > + return 0;
>
> This isn't atomic (as in: in a suitably locked region) with ...
>
> > @@ -830,7 +851,10 @@ long do_console_io(
> > break;
> > }
> > rc += len;
> > - serial_rx_cons += len;
> > + nrspin_lock_irq(&console_lock);
> > + if ( serial_rx_cons != serial_rx_prod )
> > + serial_rx_cons += len;
> > + nrspin_unlock_irq(&console_lock);
> > }
> > break;
>
> ... this. If the focus domain changes after the check in the earlier hunk,
> I think you need to also return with no input here. Or you need to acquire
> the lock earlier (and then similarly in console_switch_input()), albeit
> that would then mean holding it across a copy-to-guest. Which technically
> is perhaps not a problem, but it leaves an uneasy feeling.
I thought about it when writing this patch and I had the same feeling as
you. However, especially considering the other feedback, I don't see
another viable solution.
I'll extend the locking.
> In no case may you continue the loop if the focus domain changed, or else
> you potentially hand the old one input targeted at the new one.
>
> For context: What caught my attention is the conditional inside the locked
> region. This, imo, shouldn't be necessary when everything else is properly
> structured.
>
> Actually, the lock strictly needs holding now across all accesses to
> serial_rx_{cons,prod}. That'll then naturally make the conditional
> unnecessary.
On 20.01.2026 00:23, Stefano Stabellini wrote: > On Mon, 19 Jan 2026, Jan Beulich wrote: >> On 14.01.2026 01:39, Stefano Stabellini wrote: >>> @@ -815,6 +831,11 @@ long do_console_io( >>> if ( count > INT_MAX ) >>> break; >>> >>> + d = console_get_domain(); >>> + console_put_domain(d); >>> + if ( d != current->domain ) >>> + return 0; >> >> This isn't atomic (as in: in a suitably locked region) with ... >> >>> @@ -830,7 +851,10 @@ long do_console_io( >>> break; >>> } >>> rc += len; >>> - serial_rx_cons += len; >>> + nrspin_lock_irq(&console_lock); >>> + if ( serial_rx_cons != serial_rx_prod ) >>> + serial_rx_cons += len; >>> + nrspin_unlock_irq(&console_lock); >>> } >>> break; >> >> ... this. If the focus domain changes after the check in the earlier hunk, >> I think you need to also return with no input here. Or you need to acquire >> the lock earlier (and then similarly in console_switch_input()), albeit >> that would then mean holding it across a copy-to-guest. Which technically >> is perhaps not a problem, but it leaves an uneasy feeling. > > I thought about it when writing this patch and I had the same feeling as > you. However, especially considering the other feedback, I don't see > another viable solution. Taking just the logic here, an option might be to re-check the focus domain once holding the lock, and discard the most recent chunk of input from what would go back to the caller if the focus changed. But that would come with its own new complexities. Jan
On Tue, 20 Jan 2026, Jan Beulich wrote:
> On 20.01.2026 00:23, Stefano Stabellini wrote:
> > On Mon, 19 Jan 2026, Jan Beulich wrote:
> >> On 14.01.2026 01:39, Stefano Stabellini wrote:
> >>> @@ -815,6 +831,11 @@ long do_console_io(
> >>> if ( count > INT_MAX )
> >>> break;
> >>>
> >>> + d = console_get_domain();
> >>> + console_put_domain(d);
> >>> + if ( d != current->domain )
> >>> + return 0;
> >>
> >> This isn't atomic (as in: in a suitably locked region) with ...
> >>
> >>> @@ -830,7 +851,10 @@ long do_console_io(
> >>> break;
> >>> }
> >>> rc += len;
> >>> - serial_rx_cons += len;
> >>> + nrspin_lock_irq(&console_lock);
> >>> + if ( serial_rx_cons != serial_rx_prod )
> >>> + serial_rx_cons += len;
> >>> + nrspin_unlock_irq(&console_lock);
> >>> }
> >>> break;
> >>
> >> ... this. If the focus domain changes after the check in the earlier hunk,
> >> I think you need to also return with no input here. Or you need to acquire
> >> the lock earlier (and then similarly in console_switch_input()), albeit
> >> that would then mean holding it across a copy-to-guest. Which technically
> >> is perhaps not a problem, but it leaves an uneasy feeling.
> >
> > I thought about it when writing this patch and I had the same feeling as
> > you. However, especially considering the other feedback, I don't see
> > another viable solution.
>
> Taking just the logic here, an option might be to re-check the focus domain
> once holding the lock, and discard the most recent chunk of input from what
> would go back to the caller if the focus changed. But that would come with
> its own new complexities.
I attempted to make changes on top of v4 so that copy_to_guest_offset is
not called with the lock held.
I did introduce a focus domain check in the CONSOLEIO_read while loop,
but didn't discard input because now the focus domain is only changed
with the console_lock held and also the while loop is executed with the
console_lock held (except for copy_to_guest_offset) so as far as I can
tell it shouldn't be possible that a domain is reading someone else's
input data.
>From 71de8ddd0ce31090362115a3d54d1ebe161c229f Mon Sep 17 00:00:00 2001
From: Stefano Stabellini <stefano.stabellini@amd.com>
Date: Fri, 16 Jan 2026 13:07:43 -0800
Subject: [PATCH v5] xen/console: handle multiple domains using console_io
hypercalls
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 when the domain enters focus.
Add the console_lock around serial_rx_prod and serial_rx_cons
modifications to protect it against concurrent writes to it. Also
protect against changes of domain with focus while reading from the
serial.
Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
---
xen/drivers/char/console.c | 56 ++++++++++++++++++++++++++++++++------
1 file changed, 47 insertions(+), 9 deletions(-)
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 7b176da71a..5c621b39bd 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -521,6 +521,8 @@ struct domain *console_get_domain(void)
{
struct domain *d;
+ nrspin_lock_irq(&console_lock);
+
if ( console_rx == 0 )
return NULL;
@@ -540,6 +542,8 @@ void console_put_domain(struct domain *d)
{
if ( d )
rcu_unlock_domain(d);
+
+ nrspin_unlock_irq(&console_lock);
}
static void console_switch_input(void)
@@ -574,8 +578,12 @@ static void console_switch_input(void)
if ( !d->console.input_allowed )
continue;
- 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;
}
}
@@ -596,8 +604,19 @@ static void __serial_rx(char c)
d = console_get_domain();
if ( !d )
+ {
+ console_put_domain(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) || IS_ENABLED(CONFIG_DOM0LESS_BOOT) )
{
/*
@@ -613,11 +632,6 @@ static void __serial_rx(char c)
*/
send_guest_domain_virq(d, 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. */
@@ -729,6 +743,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 *input;
while ( count > 0 )
{
@@ -741,17 +756,23 @@ 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) )
+ input = console_get_domain();
+ if ( input && cd == input )
{
+ if ( cd->pbuf_idx )
+ {
+ console_send(cd->pbuf, cd->pbuf_idx, flags);
+ cd->pbuf_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);
+ console_put_domain(input);
}
else
{
char *kin = kbuf, *kout = kbuf, c;
+ console_put_domain(input);
/* Strip non-printable characters */
do
{
@@ -793,6 +814,7 @@ long do_console_io(
{
long rc;
unsigned int idx, len;
+ struct domain *d;
rc = xsm_console_io(XSM_OTHER, current->domain, cmd);
if ( rc )
@@ -813,6 +835,13 @@ long do_console_io(
if ( count > INT_MAX )
break;
+ d = console_get_domain();
+ if ( d != current->domain )
+ {
+ console_put_domain(d);
+ return 0;
+ }
+
rc = 0;
while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
{
@@ -822,14 +851,23 @@ long do_console_io(
len = SERIAL_RX_SIZE - idx;
if ( (rc + len) > count )
len = count - rc;
+
+ console_put_domain(d);
if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
{
rc = -EFAULT;
break;
}
+ d = console_get_domain();
+ if ( d != current->domain )
+ {
+ console_put_domain(d);
+ break;
+ }
rc += len;
serial_rx_cons += len;
}
+ console_put_domain(d);
break;
default:
rc = -ENOSYS;
--
2.25.1
On 21.01.2026 01:07, Stefano Stabellini wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -521,6 +521,8 @@ struct domain *console_get_domain(void)
> {
> struct domain *d;
>
> + nrspin_lock_irq(&console_lock);
> +
> if ( console_rx == 0 )
> return NULL;
>
> @@ -540,6 +542,8 @@ void console_put_domain(struct domain *d)
> {
> if ( d )
> rcu_unlock_domain(d);
> +
> + nrspin_unlock_irq(&console_lock);
> }
Hmm, I'd much prefer if we could avoid this. The functions aren't even
static, and new uses could easily appear. Such a locking model, even
disabling IRQs, feels pretty dangerous. (If it was to be kept, prominent
comments would need adding imo. However, for now I'm not going to make
any effort to verify this is actually safe, on the assumption that this
will go away again.)
> @@ -596,8 +604,19 @@ static void __serial_rx(char c)
>
> d = console_get_domain();
> if ( !d )
> + {
> + console_put_domain(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
Nit: Style.
> +#endif
> if ( is_hardware_domain(d) || IS_ENABLED(CONFIG_DOM0LESS_BOOT) )
> {
> /*
> @@ -613,11 +632,6 @@ static void __serial_rx(char c)
> */
> send_guest_domain_virq(d, VIRQ_CONSOLE);
> }
> -#ifdef CONFIG_SBSA_VUART_CONSOLE
> - else
> - /* Deliver input to the emulated UART. */
> - rc = vpl011_rx_char_xen(d, c);
> -#endif
I don't understand this movement, and iirc it also wasn't there in v3.
There's no explanation in the description, unless I'm overlooking the
crucial few words.
> @@ -741,17 +756,23 @@ 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) )
> + input = console_get_domain();
> + if ( input && cd == input )
> {
> + if ( cd->pbuf_idx )
> + {
> + console_send(cd->pbuf, cd->pbuf_idx, flags);
> + cd->pbuf_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);
> + console_put_domain(input);
> }
> else
> {
> char *kin = kbuf, *kout = kbuf, c;
>
> + console_put_domain(input);
> /* Strip non-printable characters */
> do
> {
The folding of locking into console_{get,put}_domain() again results in overly
long windows where the "put" is still outstanding. As said before, the current
domain can't go away behind your back.
> @@ -813,6 +835,13 @@ long do_console_io(
> if ( count > INT_MAX )
> break;
>
> + d = console_get_domain();
> + if ( d != current->domain )
> + {
> + console_put_domain(d);
> + return 0;
> + }
> +
> rc = 0;
> while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> {
> @@ -822,14 +851,23 @@ long do_console_io(
> len = SERIAL_RX_SIZE - idx;
> if ( (rc + len) > count )
> len = count - rc;
> +
> + console_put_domain(d);
> if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
> {
> rc = -EFAULT;
> break;
> }
> + d = console_get_domain();
> + if ( d != current->domain )
> + {
> + console_put_domain(d);
> + break;
> + }
> rc += len;
> serial_rx_cons += len;
> }
> + console_put_domain(d);
> break;
This is pretty horrible, don't you agree? Demonstrated by the fact that you
look to have encoded a double put: The 2nd to last one conflicts with the
one right after the loop. Whereas the earlier "break" has no reference at
all, but still takes the path with the "put" right after the loop. At the
same time it also looks wrong to simply drop that last "put".
Jan
On Wed, 21 Jan 2026, Jan Beulich wrote:
> On 21.01.2026 01:07, Stefano Stabellini wrote:
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -521,6 +521,8 @@ struct domain *console_get_domain(void)
> > {
> > struct domain *d;
> >
> > + nrspin_lock_irq(&console_lock);
> > +
> > if ( console_rx == 0 )
> > return NULL;
> >
> > @@ -540,6 +542,8 @@ void console_put_domain(struct domain *d)
> > {
> > if ( d )
> > rcu_unlock_domain(d);
> > +
> > + nrspin_unlock_irq(&console_lock);
> > }
>
> Hmm, I'd much prefer if we could avoid this. The functions aren't even
> static, and new uses could easily appear. Such a locking model, even
> disabling IRQs, feels pretty dangerous. (If it was to be kept, prominent
> comments would need adding imo. However, for now I'm not going to make
> any effort to verify this is actually safe, on the assumption that this
> will go away again.)
It is totally fine to get rid of it and revert back to explicit locks
outside of the console_get_domain and console_put_domain functions as it
was done in v4: https://marc.info/?l=xen-devel&m=176886821718712
However, the locked regions would still need to extended, more on this
below.
> > @@ -596,8 +604,19 @@ static void __serial_rx(char c)
> >
> > d = console_get_domain();
> > if ( !d )
> > + {
> > + console_put_domain(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
>
> Nit: Style.
>
> > +#endif
> > if ( is_hardware_domain(d) || IS_ENABLED(CONFIG_DOM0LESS_BOOT) )
> > {
> > /*
> > @@ -613,11 +632,6 @@ static void __serial_rx(char c)
> > */
> > send_guest_domain_virq(d, VIRQ_CONSOLE);
> > }
> > -#ifdef CONFIG_SBSA_VUART_CONSOLE
> > - else
> > - /* Deliver input to the emulated UART. */
> > - rc = vpl011_rx_char_xen(d, c);
> > -#endif
>
> I don't understand this movement, and iirc it also wasn't there in v3.
> There's no explanation in the description, unless I'm overlooking the
> crucial few words.
This chunk fixes an unrelated bug on ARM. We need to move the
CONFIG_SBSA_VUART_CONSOLE check earlier otherwise this patch will never
be taken when IS_ENABLED(CONFIG_DOM0LESS_BOOT).
I wrote a note in the changelog here:
https://marc.info/?l=xen-devel&m=176886821718712
- if vpl011 is enabled, send characters to it (retains current behavior
on ARM)
I'll be clearer in the next commit message.
> > @@ -741,17 +756,23 @@ 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) )
> > + input = console_get_domain();
> > + if ( input && cd == input )
> > {
> > + if ( cd->pbuf_idx )
> > + {
> > + console_send(cd->pbuf, cd->pbuf_idx, flags);
> > + cd->pbuf_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);
> > + console_put_domain(input);
> > }
> > else
> > {
> > char *kin = kbuf, *kout = kbuf, c;
> >
> > + console_put_domain(input);
> > /* Strip non-printable characters */
> > do
> > {
>
> The folding of locking into console_{get,put}_domain() again results in overly
> long windows where the "put" is still outstanding. As said before, the current
> domain can't go away behind your back.
I wrote in the commit message: "Add the console_lock around
serial_rx_prod and serial_rx_cons modifications to protect it against
concurrent writes to it. Also protect against changes of domain with
focus while reading from the serial."
Following your past review comments, I switched to using the
console_lock (folded into console_get/put_domain, but it can be
separate) to protect both serial_rx_prod/serial_rx_cons accesses and
also console_rx accesses.
> > @@ -813,6 +835,13 @@ long do_console_io(
> > if ( count > INT_MAX )
> > break;
> >
> > + d = console_get_domain();
> > + if ( d != current->domain )
> > + {
> > + console_put_domain(d);
> > + return 0;
> > + }
> > +
> > rc = 0;
> > while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> > {
> > @@ -822,14 +851,23 @@ long do_console_io(
> > len = SERIAL_RX_SIZE - idx;
> > if ( (rc + len) > count )
> > len = count - rc;
> > +
> > + console_put_domain(d);
> > if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) )
> > {
> > rc = -EFAULT;
> > break;
> > }
> > + d = console_get_domain();
> > + if ( d != current->domain )
> > + {
> > + console_put_domain(d);
> > + break;
> > + }
> > rc += len;
> > serial_rx_cons += len;
> > }
> > + console_put_domain(d);
> > break;
>
> This is pretty horrible, don't you agree? Demonstrated by the fact that you
> look to have encoded a double put: The 2nd to last one conflicts with the
> one right after the loop. Whereas the earlier "break" has no reference at
> all, but still takes the path with the "put" right after the loop. At the
> same time it also looks wrong to simply drop that last "put".
Yes I agree it is horrible. I did a deep review of all locking scenarios
and moved console_lock out of console_get/put_domain.
I sent out an update, expanding the commit message to explain the
locking, and re-testing all scenarios on both ARM and x86.
https://marc.info/?l=xen-devel&m=176904847332141
There were at 2-3 locking issues in this version of the patch and they
have all being resolved now.
On 22.01.2026 02:36, Stefano Stabellini wrote:
> On Wed, 21 Jan 2026, Jan Beulich wrote:
>> On 21.01.2026 01:07, Stefano Stabellini wrote:
>>> @@ -596,8 +604,19 @@ static void __serial_rx(char c)
>>>
>>> d = console_get_domain();
>>> if ( !d )
>>> + {
>>> + console_put_domain(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
>>
>> Nit: Style.
>>
>>> +#endif
>>> if ( is_hardware_domain(d) || IS_ENABLED(CONFIG_DOM0LESS_BOOT) )
>>> {
>>> /*
>>> @@ -613,11 +632,6 @@ static void __serial_rx(char c)
>>> */
>>> send_guest_domain_virq(d, VIRQ_CONSOLE);
>>> }
>>> -#ifdef CONFIG_SBSA_VUART_CONSOLE
>>> - else
>>> - /* Deliver input to the emulated UART. */
>>> - rc = vpl011_rx_char_xen(d, c);
>>> -#endif
>>
>> I don't understand this movement, and iirc it also wasn't there in v3.
>> There's no explanation in the description, unless I'm overlooking the
>> crucial few words.
>
> This chunk fixes an unrelated bug on ARM. We need to move the
> CONFIG_SBSA_VUART_CONSOLE check earlier otherwise this patch will never
> be taken when IS_ENABLED(CONFIG_DOM0LESS_BOOT).
Which suggests it wants to be a separate, backportable patch?
Jan
On Tue, 13 Jan 2026, 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 when the domain enters focus.
>
> Add the console_lock around serial_rx_cons modifications to protect it
> against concurrent writes to it.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com>
Ping?
> ---
> Changes in v3:
> - move serial_rx_cons before printk
> - call console_put_domain earlier on CONSOLEIO_read
> - take console_lock earlier
> - add console_lock around serial_rx_cons modifications
>
> Changes in v2:
> - fix code style
> - pbuf_idx/idx after ada53067083e
> - don't add extra \0
> - clear input on console switch
> ---
> xen/drivers/char/console.c | 30 +++++++++++++++++++++++++++---
> 1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 2bdb4d5fb4..58c32f22ef 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -577,6 +577,10 @@ static void console_switch_input(void)
>
> console_rx = next_rx;
> printk("*** Serial input to DOM%u", domid);
> + /* Don't let the next dom read the previous dom's unread data. */
> + nrspin_lock_irq(&console_lock);
> + serial_rx_cons = serial_rx_prod;
> + nrspin_unlock_irq(&console_lock);
> break;
> }
> }
> @@ -730,6 +734,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 *input;
>
> while ( count > 0 )
> {
> @@ -742,18 +747,28 @@ 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) )
> + input = console_get_domain();
> + if ( input && cd == input )
> {
> - /* Use direct console output as it could be interactive */
> + struct domain_console *cons = cd->console;
> +
> nrspin_lock_irq(&console_lock);
> + if ( cons->idx )
> + {
> + console_send(cons->buf, cons->idx, flags);
> + cons->idx = 0;
> + }
> + /* Use direct console output as it could be interactive */
> console_send(kbuf, kcount, flags);
> nrspin_unlock_irq(&console_lock);
> + console_put_domain(input);
> }
> else
> {
> char *kin = kbuf, *kout = kbuf, c;
> struct domain_console *cons = cd->console;
>
> + console_put_domain(input);
> /* Strip non-printable characters */
> do
> {
> @@ -795,6 +810,7 @@ long do_console_io(
> {
> long rc;
> unsigned int idx, len;
> + struct domain *d;
>
> rc = xsm_console_io(XSM_OTHER, current->domain, cmd);
> if ( rc )
> @@ -815,6 +831,11 @@ long do_console_io(
> if ( count > INT_MAX )
> break;
>
> + d = console_get_domain();
> + console_put_domain(d);
> + if ( d != current->domain )
> + return 0;
> +
> rc = 0;
> while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> {
> @@ -830,7 +851,10 @@ long do_console_io(
> break;
> }
> rc += len;
> - serial_rx_cons += len;
> + nrspin_lock_irq(&console_lock);
> + if ( serial_rx_cons != serial_rx_prod )
> + serial_rx_cons += len;
> + nrspin_unlock_irq(&console_lock);
> }
> break;
> default:
> --
> 2.25.1
>
On 16.01.2026 22:07, Stefano Stabellini wrote: > On Tue, 13 Jan 2026, 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 when the domain enters focus. >> >> Add the console_lock around serial_rx_cons modifications to protect it >> against concurrent writes to it. >> >> Signed-off-by: Stefano Stabellini <stefano.stabellini@amd.com> > > Ping? I'm sorry to ask, but doesn't this come a little early? (I for one simply haven't found time yet to look at the v3 of this.) Jan
© 2016 - 2026 Red Hat, Inc.