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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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.