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
