Re: [PATCH v6] xen/console: handle multiple domains using console_io hypercalls

[Jan Beulich <jbeulich@xxxxxxxx>]

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).

> 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