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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 13 Jan 2026, Jan Beulich wrote:
> On 13.01.2026 02:30, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> > ---
> > 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 | 25 ++++++++++++++++++++++++-
> >  1 file changed, 24 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index 2bdb4d5fb4..6c7a6592c5 100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -576,6 +576,8 @@ static void console_switch_input(void)
> >              rcu_unlock_domain(d);
> >  
> >              console_rx = next_rx;
> > +            /* don't let the next dom read the previous dom's unread data 
> > */
> 
> Nit: Comment style.
> 
> > +            serial_rx_cons = serial_rx_prod;
> >              printk("*** Serial input to DOM%u", domid);
> 
> Imo the flushing of input needs to come after the printk(), as it's only
> then that the user gets confirmation of the change.
> 
> As to flushing (rather than storing) leftover input: That's strictly an
> improvement over v1, but imo unhelpful. I may not be willing to ack this
> (still need to think about it some more), but at the very least this
> somewhat odd behavior needs calling out (and perhaps also justifying) in
> the description.

Input is typically read quickly; if there is unread data is because the
domain is slow or stuck. In this situation, the user is both providing
the unread input and also typing Ctrl-AAA to switch domain.  The user
must be aware of the unread input. In my direct experience using this
feature, it is natural that the unread data is lost and wouldn't expect
otherwise.

You are right that the explanation is missing from the commit message.
I'll add it.


> Further, did you think through the interaction with a racing
> CONSOLEIO_read? Right now that's the only place where serial_rx_cons is
> updated, and hence there was no issue with there being multiple reads
> of the variable (perhaps unless a domain issued racing CONSOLEIO_read).
> That changes now. I can't convince myself (yet) that's entirely safe,
> and hence if it is that also wants discussing in the description.

Looking carefully I also cannot convince myself it is OK. I wonder if we
should use nrspin_lock_irq and nrspin_unlock_irq here and in
CONSOLEIO_read.

I'll send a patch update doing that. I tested it and works.


> > @@ -730,6 +732,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 +745,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 )
> >          {
> > +            struct domain_console *cons = cd->console;
> > +
> > +            if ( cons->idx )
> > +            {
> > +                console_send(cons->buf, cons->idx, flags);
> > +                cons->idx = 0;
> > +            }
> 
> I probably should have said so on v1 already: What is this about? There's
> no comment and nothing in the description that I could associate with this
> code.

Something else to add to the commit message.

The domain output is buffered when not in focus. When need to clear it
out as the domain enters focus.


> And then - is this safe without holding cons->lock?

I'll move the console_lock taking earlier


> > @@ -815,6 +829,13 @@ long do_console_io(
> >          if ( count > INT_MAX )
> >              break;
> >  
> > +        d = console_get_domain();
> > +        if ( d != current->domain )
> > +        {
> > +            console_put_domain(d);
> > +            return 0;
> > +        }
> 
> As of here d == current domain. Why are you holding onto ...
> 
> >          rc = 0;
> >          while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
> >          {
> > @@ -826,12 +847,14 @@ long do_console_io(
> >                  len = count - rc;
> >              if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], 
> > len) )
> >              {
> > +                console_put_domain(d);
> >                  rc = -EFAULT;
> >                  break;
> >              }
> >              rc += len;
> >              serial_rx_cons += len;
> >          }
> > +        console_put_domain(d);
> >          break;
> 
> ... the domain for this long (requiring multiple console_put_domain()
> invocations)? The current domain can't go away under your feet. Hence imo
> a single (early) call will do.

Yes, good point