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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.01.2026 02:06, Stefano Stabellini wrote:
> @@ -555,8 +561,11 @@ static void console_switch_input(void)
>  
>          if ( next_rx++ >= max_console_rx )
>          {
> -            console_rx = 0;
>              printk("*** Serial input to Xen");
> +            nrspin_lock_irq(&console_lock);
> +            console_rx = 0;
> +            serial_rx_cons = serial_rx_prod;
> +            nrspin_unlock_irq(&console_lock);
>              break;
>          }
>  
> @@ -575,8 +584,12 @@ static void console_switch_input(void)
>  
>              rcu_unlock_domain(d);
>  
> -            console_rx = next_rx;
>              printk("*** Serial input to DOM%u", domid);
> +            nrspin_lock_irq(&console_lock);
> +            console_rx = next_rx;
> +            /* Don't let the next dom read the previous dom's unread data. */
> +            serial_rx_cons = serial_rx_prod;
> +            nrspin_unlock_irq(&console_lock);
>              break;
>          }

As in both cases you're also moving console_rx updates into the locked
region, what about readers of the variable, first and foremost
console_get_domain()? Else why the movement?

Also I think that the printk()s would better be last, indicating the
completion of the switching.

> @@ -605,8 +618,10 @@ static void __serial_rx(char c)
>           * Deliver input to the hardware domain buffer, unless it is
>           * already full.
>           */

This comment goes stale with the buffer being used for whichever is the
focus domain in do_console_io().

> +        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);

I don't think you can unconditionally enable interrupts here, as this may
be running in the context of an IRQ handler.

> @@ -742,17 +758,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);
> +        if ( is_focus_domain(cd) )

Why would any of the domains possibly being permitted to be "focus" suddenly
gain direct access here? Full access in do_console_io() is still prevented by
the XSM check there, afaict. Cc-ing Daniel, as it's not quite clear to me
whether to introduce another XSM check here, whether to check ->is_console
directly, or yet something else.

>          {
> +            if ( cons->idx )
> +            {
> +                console_send(cons->buf, cons->idx, flags);
> +                cons->idx = 0;
> +            }
> +            spin_unlock(&cons->lock);
> +
>              /* Use direct console output as it could be interactive */
> -            nrspin_lock_irq(&console_lock);
>              console_send(kbuf, kcount, flags);
>              nrspin_unlock_irq(&console_lock);
>          }
>          else
>          {
>              char *kin = kbuf, *kout = kbuf, c;
> -            struct domain_console *cons = cd->console;
> +
> +            nrspin_unlock_irq(&console_lock);
>  
>              /* Strip non-printable characters */
>              do
> @@ -765,7 +800,6 @@ static long 
> guest_console_write(XEN_GUEST_HANDLE_PARAM(char) buffer,
>              } while ( --kcount > 0 );
>  
>              *kout = '\0';
> -            spin_lock(&cons->lock);
>              kcount = kin - kbuf;
>              if ( c != '\n' &&
>                   (cons->idx + (kout - kbuf) < (ARRAY_SIZE(cons->buf) - 1)) )
> @@ -815,6 +849,13 @@ long do_console_io(
>          if ( count > INT_MAX )
>              break;
>  
> +        nrspin_lock_irq(&console_lock);
> +        if ( !is_focus_domain(current->domain) )
> +        {
> +            nrspin_unlock_irq(&console_lock);
> +            return 0;
> +        }

To avoid the extra unlock-and-return, how about simply setting "count" to 0,
leveraging ...

>          rc = 0;
>          while ( (serial_rx_cons != serial_rx_prod) && (rc < count) )

... the rhs check here?

> @@ -824,14 +865,28 @@ long do_console_io(
>                  len = SERIAL_RX_SIZE - idx;
>              if ( (rc + len) > count )
>                  len = count - rc;
> +            nrspin_unlock_irq(&console_lock);

This can be moved up a few lines, as none of the local variable manipulations
needs guarding.

>              if ( copy_to_guest_offset(buffer, rc, &serial_rx_ring[idx], len) 
> )
> -            {
> -                rc = -EFAULT;
> -                break;
> -            }
> +                return -EFAULT;
>              rc += len;
> +
> +            /*
> +             * First get console_lock again, then check is_focus_domain.
> +             * It is possible that we switched focus domain during
> +             * copy_to_guest_offset. In that case, serial_rx_cons and

Please can you append () to the function name, to identify it as a function,
as opposed to ...

> +             * serial_rx_prod have been reset so it would be wrong to
> +             * update serial_rx_cons here. Get out of the loop instead.

... the two variables referenced here?

Jan