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

[Jan Beulich <jbeulich@xxxxxxxx>]

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