Re: [PATCH v8 1/2] xen/console: handle multiple domains using console_io hypercalls

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2026 23:08, Stefano Stabellini wrote:
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -518,11 +518,16 @@ static unsigned int __read_mostly console_rx = 0;
>  struct domain *console_get_domain(void)
>  {
>      struct domain *d;
> +    unsigned int rx;
>  
> -    if ( console_rx == 0 )
> +    nrspin_lock(&console_lock);
> +    rx = console_rx;
> +    nrspin_unlock(&console_lock);

Did you test this in a debug build, and it didn't blow up? The console lock
is an IRQ-safe one, so I'd expect check_lock() to complain that you do not
disable IRQs here. At the same time I don't think you can rely on IRQs being
off upon entry into the function.

Anyway - is locking here really needed? Wouldn't suitable use of ACCESS_ONCE()
(also elsewhere) do? (Such a switchover likely could be a separate, prereq
patch.)

Further, if already you add locking on the read sides, what about ...

> @@ -540,6 +545,12 @@ void console_put_domain(struct domain *d)
>          rcu_unlock_domain(d);
>  }
>  
> +static bool is_focus_domain(const struct domain *d)
> +{
> +    ASSERT(rspin_is_locked(&console_lock));
> +    return d != NULL && d->domain_id == console_rx - 1;
> +}
> +
>  static void console_switch_input(void)
>  {
>      unsigned int next_rx = console_rx;

... this read?

> @@ -555,7 +566,10 @@ static void console_switch_input(void)
>  
>          if ( next_rx++ >= max_console_rx )
>          {
> +            nrspin_lock_irq(&console_lock);

As indicated earlier, you can't know IRQ state in anything down the call
tree from serial_rx().

> @@ -742,17 +765,36 @@ 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) )
> +        /*
> +         * Take both cons->lock and console_lock:
> +         * - cons->lock protects cons->buf and cons->idx
> +         * - console_lock protects console_send() and is_focus_domain()
> +         *   checks
> +         *
> +         * The order must be respected. guest_printk() takes the
> +         * console_lock so it is important that cons->lock is taken
> +         * first.
> +         */
> +        spin_lock(&cons->lock);
> +        nrspin_lock_irq(&console_lock);

While guest_printk() does matter here, it taking (down the call tree)
console_lock isn't alone relevant. It being called with cons->lock held
is, .

> @@ -816,22 +857,39 @@ long do_console_io(
>              break;
>  
>          rc = 0;
> +        nrspin_lock_irq(&console_lock);
> +        if ( !is_focus_domain(current->domain) )
> +            count = 0;
>          while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )
>          {
>              idx = SERIAL_RX_MASK(serial_rx_cons);
>              len = serial_rx_prod - serial_rx_cons;
> +            nrspin_unlock_irq(&console_lock);

Can we please have blank lines on both sides of this?

>              if ( (idx + len) > SERIAL_RX_SIZE )
>                  len = SERIAL_RX_SIZE - idx;
>              if ( (rc + len) > count )
>                  len = count - rc;
>              if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) 
> )

Have I perhaps talked you into moving the unlock too early? serial_rx_ring[]
accesses look like they need to be with the lock still held. Or, to avoid
calling copy_to_guest_offset() with the lock held, a local copy would need
making.

Jan